Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User.Identity.Name in v.1.9.0 now shows displayname instead of account name #1136

Closed
Yomodo opened this issue Apr 13, 2021 · 20 comments
Closed

Comments

@Yomodo
Copy link

Yomodo commented Apr 13, 2021

Which version of Microsoft Identity Web are you using?
Microsoft.Identity.Web 1.9.0

Microsoft.Identity.Web 1.8.2: user.Identity.Name contains my account name (UPN): "account@TENANT.onmicrosoft.com"

Microsoft.Identity.Web 1.9.0: user.Identity.Name contains my account display name.

@jmprieur
Copy link
Collaborator

@Yomodo : do you have repro steps?

  • Is it in web apps or web APIs?
  • Is it with .NET Core 3.1 or .NET 5.0 or both?

@Yomodo
Copy link
Author

Yomodo commented Apr 13, 2021

This is a .NET 5 blazor server web app.

@jmprieur
Copy link
Collaborator

@Yomodo : could you please check what happens if you force Microsoft.IdentityModel assemblies to be version="6.8.*" ?

    <PackageReference Include="Microsoft.IdentityModel.Logging" Version="6.8.*" />
    <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="6.8.*" />
    <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="6.8.*" />

@Yomodo
Copy link
Author

Yomodo commented Apr 13, 2021

These are the used package references:

    <PackageReference Include="Humanizer.Core" Version="2.8.26" />
    <PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.17.0" />
    <PackageReference Include="Microsoft.Extensions.Logging.AzureAppServices" Version="5.0.5" />
    <PackageReference Include="Microsoft.Identity.Web" Version="1.9.0" />
    <PackageReference Include="Microsoft.Identity.Web.UI" Version="1.9.0" />
    <PackageReference Include="MudBlazor" Version="5.0.7" />
    <PackageReference Include="System.Linq.Async" Version="5.0.0" />

@jmprieur
Copy link
Collaborator

I understand, @Yomodo
I'm asking if you could add the three package references I provided above to your csproj, in order to see if this still reproes ? I'd like to rule out a change in Microsoft.IdentityModel (as I don't see change in Microsoft.Identity.Web for this)

@Yomodo
Copy link
Author

Yomodo commented Apr 13, 2021

  1. Removed v1.9.0
  2. Added the 3 references
  3. Added v1.8.2
  4. Adding v1.9.0 now throws these nuget errors:

Microsoft.Identity.Web 1.9.0:
image

Microsoft.Identity.Web.UI 1.9.0:
image

@jmprieur
Copy link
Collaborator

Thank you very much @Yomodo for trying this.
@brentschmaltz @mafurman @GeoK could it be that Wilson 8.10 has changed the mapping of the Name claim?

@travisneale
Copy link

I think I'm seeing something similar. Downgrading System.IdentityModel.Tokens.Jwt to 6.8.0 resolved my problem.

To repo:
From Blazor server template, connect to Azure AD authentication and using AuthorizeRouteView:
In Index.razor add:
`
@code {

[CascadingParameter]
private Task<AuthenticationState> authenticationStateTask { get; set; }

private string currUserID;

protected override async Task OnInitializedAsync()
{
    var user = (await authenticationStateTask).User;

    currUserID = user.FindFirst("name").Value.ToString();

}

}`

You will get a
System.NullReferenceException: 'Object reference not set to an instance of an object.'

Reverting back to Microsoft.Identity.Web 1.8.2 resolves issue.
Staying with 1.9.0 and reverting back System.IdentityModel.Tokens.Jwt to 6.8.0
and Microsoft.IdentityModel.Protocols.OpenIdConnect to 6.8.0 also removes the error and returns the proper Name.

@jennyf19
Copy link
Collaborator

thanks for the quick update @travisneale and investigation.
cc: @jmprieur @GeoK

@jennyf19 jennyf19 added the P1 label Apr 13, 2021
@brentschmaltz
Copy link
Member

brentschmaltz commented Apr 13, 2021

@jennyf19 @travisneale while we look into this, in general the pattern:
use.FindFirst("<someclaimname>").Value.ToString();
Can always throw a null exception.

It would be helpful to have the jwt token that was used.

@jennyf19
Copy link
Collaborator

@travisneale @Yomodo i'm not able to repro it w/a Blazor server web app, which calls Graph. Can you provide repro code?

@Yomodo
Copy link
Author

Yomodo commented Apr 13, 2021

My issue started with the exact same code as provided by @travisneale.

@jmprieur
Copy link
Collaborator

Identity.Model just added an issue for this regression: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#1627

@travisneale
Copy link

@travisneale @Yomodo i'm not able to repro it w/a Blazor server web app, which calls Graph. Can you provide repro code?

https://github.com/travisneale/MID-1136

To get this app to work: You will need to add domain name, tenant ID, client ID to the appsettings.json file to point to your AzureAD, which will need the app registration with a Redirect URI pointing to https://localhost:44367/signin-oidc (or whatever local host port number you set).
It's currently set to Microsoft.Identity.Web 1.8.2 so that it will work. Update to 1.9.0 to see the error.

@jennyf19
Copy link
Collaborator

@travisneale the repro was super helpful. thank you so much. we should have a workaround out in our next release 1.9.1 on 4.14.

@travisneale
Copy link

travisneale commented Apr 14, 2021

@jennyf19 Thank you for the quick patch!

@jmprieur
Copy link
Collaborator

@Yomodo @travisneale we just released Microsoft.Identity.Web 1.9.1 which works around this issue until IdentityModel provides a definitive fix.

@Yomodo
Copy link
Author

Yomodo commented Apr 14, 2021

Thanks a lot everyone!

@jennyf19
Copy link
Collaborator

@travisneale @Yomodo Are you able to try out this branch which removes the workaround we had in place? @brentschmaltz made the fix in Wilson, so I've removed the workaround in MS Id Web and we automatically pick up the latest wilson build (6.10.2 has the fix in this case).

I tested w/the sample @travisneale provided and seems to work as expected. Would one of you mind confirming? This will be in our next release (1.10).

cc: @jmprieur

@Yomodo
Copy link
Author

Yomodo commented Apr 16, 2021

Yes, all is fine now. Thanks again!

jennyf19 added a commit that referenced this issue May 1, 2021
…#1168)

* Fix for #1136 - case insensitive comparison to detect app service env

* Add another test to keep SQ happy

* Test

* update test (#1173)

Co-authored-by: jennyf19 <jeferrie@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants