AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.05k stars 399 forks source link

JwtSecurityTokenHandler looking for Claim properties which are not being added to JWT payload #1538

Open Herostwist opened 4 years ago

Herostwist commented 4 years ago

When a JWT is handled by JwtSecurityTokenHandler it tries to map any properties in the JwtClaim to a System.Security.Claims.Claim -see line 1136

However when the token is created using IEnumerable<Claim>, the JwtPayload only includes the claim type and value and ignores any properties. /// <param name="claims">If this value is not null then for each <see cref="Claim"/> a { 'Claim.Type', 'Claim.Value' } see line 359

There doesn't appear to be any way to pass Claim properties along through the token. The JwtSecurityTokenHandler is looking for them but JwtPayload never adds them.

brentschmaltz commented 3 years ago

We'll have a look into this.

brentschmaltz commented 3 years ago

@Herostwist JwtPayload.Claims does not add any properties to a Claim. If you need to add properties, then it would be necessary to override JwtPayload.Claims to do so.

JwtSecurityTokenHandler adds properties in case JwtPayload.Claims was overridden.

Herostwist commented 3 years ago

Seems like a very odd choice to implement claim properties one end and not the other. I can't see any way of overriding JwtPayload so it serializes Claim properties. Also if I were to inherit from JwtPayload it means that i would need to write my own version of JwtSecurityToken as it can't be overridden and accesses static methods on JwtPayload, which then means writing my own handler e.t.c.. This doesn't seem like something that can be solved by simply overriding JwtPayload.Claims.

The whole JwtPayload inherits from Dictionary<string, object> where the object is the Claim value, there is nowhere for Claim properties to be added. In public void AddClaims(IEnumerable<Claim> claims) it simply extracts the Claim.Type as the dictionary key and Claim.Value as the dictionary value.

Are there any unit tests that cover line 1136? I can't see any way of getting Claim properties into a JwtSecurityToken.

brentschmaltz commented 3 years ago

@Herostwist i had another look at this and you are correct, if you have some information inside Claim.Properties there is really no way for you to add that to the token. We need to add extensibility here.

brentschmaltz commented 2 years ago

@Herostwist revisiting this as i am trying to understand the ask. There is no natural place in a JWT that map to Claim properties. The reason we added properties to the Claim inside the JwtPayload is if we map the Claim.Type, we include the original property name from the JWT as a property on the claim.