aspnet / Identity

[Archived] ASP.NET Core Identity is the membership system for building ASP.NET Core web applications, including membership, login, and user data. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
1.96k stars 870 forks source link

ExternalLoginInfo.ProviderDisplayName doesn't use the authentication scheme display name #1892

Closed thomaslevesque closed 6 years ago

thomaslevesque commented 6 years ago

I use ASP.NET Identity with Azure AD as an external identity provider. I configured Azure AD with a display name:

services.AddAuthentication()
    .AddOpenIdConnect("AzureAD", "Azure Active Directory", options =>
            {
                ...
            });

When I call SignInManager.GetExternalAuthenticationSchemesAsync(), I would expect ProviderDisplayName in the result to be "Azure Active Directory", but it's "AzureAD" instead.

thomaslevesque commented 6 years ago

Apparently this is a known issue:

https://github.com/aspnet/Identity/blob/ff0e9257421a296012d03e8729ad79e9471f72e8/src/Identity/SignInManager.cs#L616-L620

thomaslevesque commented 6 years ago

Current workaround:

    public class SignInManagerWithCorrectProviderDisplayName<TUser> : SignInManager<TUser>
        where TUser : class
    {
        public SignInManagerWithCorrectProviderDisplayName(UserManager<TUser> userManager, IHttpContextAccessor contextAccessor, IUserClaimsPrincipalFactory<TUser> claimsFactory, IOptions<IdentityOptions> optionsAccessor, ILogger<SignInManager<TUser>> logger, IAuthenticationSchemeProvider schemes)
            : base(userManager, contextAccessor, claimsFactory, optionsAccessor, logger, schemes)
        {
        }

        public override async Task<ExternalLoginInfo> GetExternalLoginInfoAsync(string expectedXsrf = null)
        {
            var info = await base.GetExternalLoginInfoAsync(expectedXsrf);
            if (info != null)
            {
                var schemes = await GetExternalAuthenticationSchemesAsync();
                var scheme = schemes.FirstOrDefault(s => s.Name == info.LoginProvider);
                if (!string.IsNullOrEmpty(scheme?.DisplayName))
                    info.ProviderDisplayName = scheme.DisplayName;
            }

            return info;
        }
    }
blowdart commented 6 years ago

You are, as you've discovered, conflating the display name with the scheme. So your workaround isn't a work around, it's the right way to do it, it's what it done in templates. There's no bug here, it's by design.

thomaslevesque commented 6 years ago

@blowdart I'm confused... Are you saying that ExternalLoginInfo.ProviderDisplayName is supposed to return the same thing as ExternalLoginInfo.LoginProvider and isn't supposed to actually return a display name? In this case the property is useless...

it's what it done in templates

Which templates are you referring to? I don't think the Identity template does this; I wouldn't have needed the workaround if it did.

thomaslevesque commented 6 years ago

If you're talking about this, it doesn't even attempt to use a display name, it just uses LoginProvider, which is a "technical" name not intended to be seen by users.

HaoK commented 6 years ago

Ah I misread the issue, yep its a bug, we can fix this in 2.2

HaoK commented 6 years ago

The default UI does use the DisplayName here https://github.com/aspnet/Identity/blob/master/src/UI/Areas/Identity/Pages/Account/Login.cshtml#L69 so the templates should be affected by this bug too

thomaslevesque commented 6 years ago

The default UI does use the DisplayName here https://github.com/aspnet/Identity/blob/master/src/UI/Areas/Identity/Pages/Account/Login.cshtml#L69 so the templates should be affected by this bug too

Some places use the display name, others use the provider name, e.g.:

https://github.com/aspnet/Identity/blob/ff0e9257421a296012d03e8729ad79e9471f72e8/src/UI/Areas/Identity/Pages/Account/ExternalLogin.cshtml#L12

https://github.com/aspnet/Identity/blob/ff0e9257421a296012d03e8729ad79e9471f72e8/src/UI/Areas/Identity/Pages/Account/Manage/ExternalLogins.cshtml#L17

https://github.com/aspnet/Identity/blob/ff0e9257421a296012d03e8729ad79e9471f72e8/src/UI/Areas/Identity/Pages/Account/Manage/ExternalLogins.cshtml#L25

blowdart commented 6 years ago

@hoak We should fix them all to be consistent.

thomaslevesque commented 6 years ago

Would you be willing to accept a PR for this?

HaoK commented 6 years ago

Sure that would be great

HaoK commented 6 years ago

Thanks @thomaslevesque !