-
Notifications
You must be signed in to change notification settings - Fork 420
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
Replace JsonWebToken ReadPayloadValue with a delegate #2981
base: dev
Are you sure you want to change the base?
Conversation
3613363
to
3cbc31c
Compare
Edit: These results are outdated. Looks like when using delegates there're extra allocations because of: string claimName = reader.GetString();
claims[claimName] = ReadTokenPayloadValueDelegate(ref reader, claimName);
|
3cbc31c
to
4d5347b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- src/Microsoft.IdentityModel.JsonWebTokens/InternalAPI.Unshipped.txt: Language not supported
- src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt: Language not supported
- test/Microsoft.IdentityModel.JsonWebTokens.Tests/CustomJsonWebToken.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonClaimSet.cs:31
- The initialization of _jsonClaims should be 'new Dictionary<string, object>()' instead of '[]'.
_jsonClaims = [];
Edit: These results are outdated. Updated results comparing delegates which have a dictionary as a parameter. Ran JsonWebTokenHandler_ValidateTokenAsyncWithTVP and ReadJWS_FromMemory with extended claims. These test using the default delegate implemenation.
BenchmarkDotNet v0.13.12, Windows 11 (10.0.26100.2605) (Hyper-V) |
add645b
to
89a8c8a
Compare
89a8c8a
to
442baa3
Compare
e810ae2
to
aecf790
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
} | ||
} | ||
} | ||
else if (reader.ValueTextEquals(JwtPayloadUtf8Bytes.Azp)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user will have no ability to parse any of the claims that are parsed before calling the delegate.
There are a couple of things we can do.
-
Have a model where a power user can take total control.
-
Call the delegate if hook has been added for 'aud' or any of the internal types.
in this case we could create a u8 byte array for the claim type associated with a delegate so that reader.ValueTextEquals performant check could be used.
It would be great to have the same design when reading the header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to allow the user to parse all claims in a performant manner.
This design skips claims that are parsed.
Allowing the user to parse all claims allows for an implementation where validation could fail fast.
Failing fast needs to have a design that can propagate a detailed error result to upper layers for error reporting.
Fixes #2982
This pull request introduces several updates and improvements to the
Microsoft.IdentityModel.JsonWebTokens
library, focusing on enhancing the handling of token payloads and custom claims. The most important changes include adding new constructors to theJsonWebToken
class, updating theJsonClaimSet
initialization, and introducing a new delegate for reading custom token payload values.Enhancements to
JsonWebToken
:JsonWebToken
class to support initializing tokens with custom delegates for reading token payload values. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs
) [1] [2] [3]ReadTokenPayloadValueDelegates
property to theJsonWebToken
class, allowing custom handling of specific claim names during token reading. (src/Microsoft.IdentityModel.JsonWebTokens/JsonWebToken.cs
)Updates to
TokenValidationParameters
:ReadTokenPayloadValueDelegates
property to theTokenValidationParameters
class to support custom claim handling during token validation. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
) [1] [2]TokenValidationParameters
to include the newReadTokenPayloadValueDelegates
property. (src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
)Refactoring and cleanup:
ReadPayloadValue
method inJsonWebToken.PayloadClaimSet
to simplify the handling of standard claims and integrate custom delegate handling. (src/Microsoft.IdentityModel.JsonWebTokens/Json/JsonWebToken.PayloadClaimSet.cs
) [1] [2]CustomJsonWebToken
class from the tests, as its functionality is now covered by the new delegate-based approach. (test/Microsoft.IdentityModel.JsonWebTokens.Tests/CustomJsonWebToken.cs
)Delegate introduction:
ReadTokenPayloadValueDelegate
delegate to handle custom claim reading during token payload processing. (src/Microsoft.IdentityModel.Tokens/Delegates.cs
)