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

[Bug] Subtle breaking change in claim deserialization in v7 #2354

Closed
2 of 14 tasks
Suchiman opened this issue Oct 11, 2023 · 3 comments · Fixed by #2367
Closed
2 of 14 tasks

[Bug] Subtle breaking change in claim deserialization in v7 #2354

Suchiman opened this issue Oct 11, 2023 · 3 comments · Fixed by #2367
Labels
Customer reported Indicates issue was opened by customer

Comments

@Suchiman
Copy link

Suchiman commented Oct 11, 2023

Which version of Microsoft.IdentityModel are you using?
Microsoft.IdentityModel.Protocols.OpenIdConnect: 7.0.2

Where is the issue?

  • M.IM.JsonWebTokens
  • M.IM.KeyVaultExtensions
  • M.IM.Logging
  • M.IM.ManagedKeyVaultSecurityKey
  • M.IM.Protocols
  • M.IM.Protocols.OpenIdConnect
  • M.IM.Protocols.SignedHttpRequest
  • M.IM.Protocols.WsFederation
  • M.IM.TestExtensions
  • M.IM.Tokens
  • M.IM.Tokens.Saml
  • M.IM.Validators
  • M.IM.Xml
  • S.IM.Tokens.Jwt
  • Other (please describe)

Is this a new or an existing app?
a. The app is in production and I have upgraded from 6.0.33 to 7.0.2 of Microsoft.IdentityModel.Protocols.OpenIdConnect

Repro
Have a JWT that contains a json boolean, like so

{
    "testClaim": true
}

When this token is received by ASP.NET Core, a ClaimsPrincipal is produced with an ClaimsIdentity that has a "testClaim" Claim on it

Expected behavior
The value of "testClaim" used to be "true"

Actual behavior
The value of "testClaim" used is now "True"

This is breaking because if you have code like

new AuthorizationPolicyBuilder()
    .RequireAuthenticatedUser()
    .RequireClaim("testClaim", "true")

then "True" will not satisfy that requirement anymore.
I'm not sure if this was an intentional change but it should be at a minimum called out in the breaking changes

@brentschmaltz brentschmaltz added Customer reported Indicates issue was opened by customer JSON labels Oct 11, 2023
@brentschmaltz brentschmaltz added this to the December refresh milestone Oct 11, 2023
@brentschmaltz
Copy link
Member

@Suchiman this is a Claim in the ClaimsIdentity?

@Suchiman
Copy link
Author

Suchiman commented Oct 11, 2023

@Suchiman this is a Claim in the ClaimsIdentity?

correct. Since Claim.Value is a string, my assumption would be that true is somewhere read and parsed as a System.Boolean to be then .ToString()'d when constructing the Claims because true.ToString() results in "True"

@brentschmaltz
Copy link
Member

brentschmaltz commented Oct 11, 2023

@Suchiman it would make sense to me if the JsonWebToken and ClaimsIdentity had the same value.

You are correct we use bool.ToString().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer reported Indicates issue was opened by customer
Projects
None yet
2 participants