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

Single-item-claim-array when using ClaimValueTypes.Json changing schema #18178

Closed
azydevelopment opened this issue Jan 8, 2020 · 5 comments
Closed
Labels
area-identity Includes: Identity and providers Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue.

Comments

@azydevelopment
Copy link

Describe the bug

Please see this issue: IdentityServer/IdentityServer4#3968

When sending over a JWT with an array claim with only one value, the claim in user.profile gets transformed to just a string. When an array with 2 values gets sent over in the JWT claim, user.profile sets it to an array with those 2 values.

This means that the schema changes depending on how many items are in the claim which is less than ideal and forces some special casing.

To Reproduce

Consider this code applied to a React + ASP.NET template with Individual User authentication/authorization. The snippet has IdentityServer send over the JWT claim as an array even if it only has one value in it:

public Task GetProfileDataAsync(ProfileDataRequestContext context)
{
    // Get all the roles associated with this subject
    IEnumerable<Claim> roleClaims = context.Subject.FindAll(JwtClaimTypes.Role);

    // Put those roles into a list so we can serialize them after
    IList<string> roles = new List<string>();
    foreach (Claim roleClaim in roleClaims)
    {
        Debug.Assert(roleClaim.Type == JwtClaimTypes.Role);

        roles.Add(roleClaim.Value);
    }

    // Serialize the roles into JSON
    string rolesJson = JsonSerializer.Serialize(roles);

    // Put the roles JSON into an actual Claim object
    Claim claim = new Claim(JwtClaimTypes.Role, rolesJson, IdentityServerConstants.ClaimValueTypes.Json);

    // Add the claim to the IssuedClaims which will eventually end up in the issued JWT
    context.IssuedClaims.Add(claim);

    return Task.CompletedTask;
}

The JWT that comes out on the other end does have an array with 1 value in it but user.profile transforms it into a string.

If you don't want to actually create a role before testing this, just replace rolesJson above with "[\"role1\"]".

From AuthorizeServices.js checking out user.profile where the claims end up on the client side:

image

image

I marked this as a bug because it overrides the explicitly defined preference of IdentityServer.

Further technical details

  • ASP.NET Core version 3.1

.NET Core SDK (reflecting any global.json):
Version: 3.1.100
Commit: cd82f021f4

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18362
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.1.100\

Host (useful for support):
Version: 3.1.0
Commit: 65f04fb6db

.NET Core SDKs installed:
3.0.100 [C:\Program Files\dotnet\sdk]
3.1.100 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
https://aka.ms/dotnet-download

  • The VSCode 1.41.1
@mkArtakMSFT mkArtakMSFT added the area-identity Includes: Identity and providers label Jan 8, 2020
@javiercn
Copy link
Member

javiercn commented Jan 8, 2020

@azydevelopment Thanks for contacting us.

Is there anything specific about ASP.NET Core here? All that seems to be Identity Server specific code, is there any specific piece of code where we are overriding your settings?

Templates are a starting point and you are allowed to change them in whatever way suits your scenario first.

I don't think this is an issue, but if you provide more details we can re-evaluate.

@javiercn javiercn added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Jan 8, 2020
@azydevelopment
Copy link
Author

azydevelopment commented Jan 8, 2020

Yep this is an issue in code in AspNetCore. Specifically in JavaScript client code. The IdentityServer code is only needed to expose the issue. The main issue is that the client side JWT processing does not honor what is actually said in the JWT.

I'm not sure where exactly the bad transformation is happening but it does seem to be in AspNetCore code since the JWT itself has the right single element array in it (which also means the issue isn't in IdentityServer).

@blowdart
Copy link
Contributor

blowdart commented Jan 8, 2020

We don't parse the JWT ourselves, that's part of https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet

Please file the issue there.

@blowdart blowdart closed this as completed Jan 8, 2020
@azydevelopment
Copy link
Author

Thanks. Not a problem. Down the rabbit hole I go.

@leastprivilege
Copy link
Contributor

Well - if you think this is happening in JavaScript - then probably this is the better place to go:

https://github.com/IdentityModel/oidc-client-js

@ghost ghost locked as resolved and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-identity Includes: Identity and providers Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue.
Projects
None yet
Development

No branches or pull requests

6 participants
@blowdart @leastprivilege @javiercn @azydevelopment @mkArtakMSFT and others