IdentityServer / IdentityServer4

OpenID Connect and OAuth 2.0 Framework for ASP.NET Core
https://identityserver.io
Apache License 2.0
9.23k stars 4.02k forks source link

Multiple claims with the same claim types are getting merged into one claim. #2999

Closed phantasm33 closed 5 years ago

phantasm33 commented 5 years ago

Good morning,

I need to be able to support users having multiple roles within my application, For example, a user can be Admin and Supervisor, but when I add the claims for this separately, they get merged together into one claims which cause the User.IsInRole to fail.

Here is some sample code that I put together from my IProfileService:

 public async Task GetProfileDataAsync(ProfileDataRequestContext context)
        {
            var newClaims = new List<Claim>();
            newClaims.Add(new Claim("User", "James"));
            newClaims.Add(new Claim("role", "Admin"));
            newClaims.Add(new Claim("role", "Supervisor"));
            newClaims.Add(new Claim("DummyClaim", "Value 1"));
            newClaims.Add(new Claim("DummyClaim", "Value 2"));
            context.IssuedClaims = newClaims;
        }

When I parse the User.Claims collection, this is what I get:

User Claims:

Count: 5 sid c64e00090cf702090f65ed3048a50534 sub 17d1533c-675a-40cb-ab03-e3c25ea0ec95 User James role [ "Admin", "Supervisor" ] DummyClaim [ "Value 1", "Value 2" ]

At first I thought it was an issue with the "role" claim type, but as the last example shows, its not.

Is there some property that I need to set to get it to not merge the claim together? I am assuming it has something to do to with the Claims Transformation, but I can't figure out how to get it to not to do the merging.

Thanks for you help, David

brockallen commented 5 years ago

IIRC, this was a bug somewhere in the client side parsing of the id_token from Microsoft.

phantasm33 commented 5 years ago

Hi Brock,

Thanks for responding to my question. I turned on logging to see if I could trace what was going on. Based upon the screenshot below, it looks like its an issue with how the Access Token in being created somewhere in the pipeline (perhaps the DefaultClaimsService).

screen shot 2019-02-01 at 2 45 04 pm

This is what I get when I decode the Access Token:

{
  "nbf": 1549050016,
  "exp": 1549053616,
  "iss": "https://localhost:44300",
  "aud": [
    "https://localhost:44300/resources",
    "DtmsApi"
  ],
  "client_id": "DTMS3Web",
  "sub": "ba7bf876-660e-4cdc-8404-46df9dcd7489",
  "auth_time": 1549050012,
  "idp": "local",
  "User": "James",
  "role": [
    "Admin",
    "Supervisor"
  ],
  "DummyClaim": [
    "Value 1",
    "Value 2"
  ],
  "scope": [
    "openid",
    "profile",
    "PSAIdentity",
    "DtmsApi"
  ],
  "amr": [
    "pwd"
  ]
}

As you can see, both the "roles" and "DummyClaim" claims are merged to form a single claim for each claim type when they should be returned as separate claims. I tried looking through your source code, but I quickly got lost trying to figure out what routine was actually creating the tokens. I couldn't tell if it was here in this repo or in the IdentityModel repo.

Am I missing a connection here or should I be looking to the client side parsing as you suggested earlier. Please let me know.

Thanks, David

phantasm33 commented 5 years ago

I found a solution to this issue over at the Aspnetcore repo. The JsonKeyClaimAction classes was added in 2.1 to provide a way for transforming the merged claims back into separate individual claims.

My only issue with the new code is that it still leaves the original claim in place. IMHO, once the transformation has been made there is no longer a need to keep the original merged claim in place. I submitted a issue to modify the code to remove the merged claim after the transformation has been made. For anyone interested here is the link to my request -

https://github.com/aspnet/AspNetCore/issues/7204#issue-405928931

Brock - thanks for the point me in the right direction.

brockallen commented 5 years ago

Well, it's correct in the json above since you can't have two properties with the same name, so the rules are to collapse multiple claims of the same type into an array. You then need to process it on the consumer side to reverse that procedure. I thought the latest OIDC in AspNet core did the right thing, but maybe it's broken (again)./

phantasm33 commented 5 years ago

Thanks Brock. It appreciate you taken the time to help me out.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.