-
Notifications
You must be signed in to change notification settings - Fork 409
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
ValidateTokenAsync - New Path: Refactor result types #2794
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cWithVPTests.cs Co-authored-by: Keegan Caruso <[email protected]>
Adding Result and Unit type. Experiment around allocations and performance.
…sult return values to use implicit initialisation
…dated tests for Algorithm and Audience validation
…es to failures, added initial cache experiment for stack frames
jennyf19
reviewed
Aug 26, 2024
jennyf19
reviewed
Aug 26, 2024
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.DecryptToken.cs
Show resolved
Hide resolved
jennyf19
reviewed
Aug 26, 2024
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.DecryptToken.cs
Outdated
Show resolved
Hide resolved
jennyf19
reviewed
Aug 26, 2024
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateSignature.cs
Show resolved
Hide resolved
jennyf19
reviewed
Aug 26, 2024
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateSignature.cs
Show resolved
Hide resolved
jennyf19
reviewed
Aug 26, 2024
src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateSignature.cs
Show resolved
Hide resolved
…eworks change to LoggingExtensions
# Conflicts: # test/Microsoft.IdentityModel.Tokens.Tests/Validation/AsyncValidatorsTests.cs # test/Microsoft.IdentityModel.Tokens.Tests/Validation/ValidationParametersTests.cs
benchmark/Microsoft.IdentityModel.Benchmarks/ValidateTokenAsyncTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Validation/Validators.IssuerSigningKey.cs
Show resolved
Hide resolved
test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenHandler.DecryptTokenTests.cs
Show resolved
Hide resolved
test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenHandler.ReadTokenTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.JsonWebTokens.Tests/JsonWebTokenHandler.ValidateSignatureTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/AlgorithmValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/AudienceValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/IssuerValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/LifetimeValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/ReplayValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/SigningKeyValidationResultTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/SigningKeyValidationResultTests.cs
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/TokenTypeValidationResultTests.cs
Outdated
Show resolved
Hide resolved
FuPingFranco
approved these changes
Aug 27, 2024
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.
LGTM, just a couple of minor things like removing the additional imports, flaky test and a couple of questions.
…datetime comparison in tests.
brentschmaltz
approved these changes
Aug 28, 2024
brentschmaltz
pushed a commit
that referenced
this pull request
Sep 8, 2024
* Adding benchmark for new ValidateTokenAsync model vs old. * Update benchmark/Microsoft.IdentityModel.Benchmarks/ValidateTokenAsyncWithVPTests.cs * Removed IdentityComparer methods relating to removed result types. Updated tests for Algorithm and Audience validation * Removed ITokenValidationError interface. Updated tests * Replaced TokenValidationError with ExceptionDetails. Added stack frames to failures, added initial cache experiment for stack frames * Removed optionality from CancellationToken parameter * Updated tests * Added ClaimsIdentity to ValidationResult * Updated benchmarks to match the result types and added CancellationToken * Removed test consoleapp, re-grouped benchmarks for better comparison * Removed unit type * Removed TokenValidationError since it is no longer used * Restored ExceptionType type name * Restored ExceptionFromType method in ExceptionDetail * Override Result's empty initialiser and annotate it to prevent wrong initialisation * Removed commented code. * Commented empty if statement * Added cached stack frames and nullability annotations for ValidateTokenAsync Changed return type to use Result and removed error information from ValidationResult Added method to add StackFrames to ExceptionDetail * Added stack frames for ReadToken and DecryptToken * Added ValidationFailureType to ExceptionDetail * Updated tests to use ValidationFailureType * Update src/Microsoft.IdentityModel.Abstractions/Result.cs Co-authored-by: Keegan Caruso <[email protected]> * Moved Result to Tokens project, made it internal. Reverted TargetFrameworks change to LoggingExtensions * Addressed PR feedback around argument null exceptions * Addressed PR comments. Removed unused imports. Increased epsilon for datetime comparison in tests. --------- Co-authored-by: Franco Fung <[email protected]> Co-authored-by: Franco Fung <[email protected]> Co-authored-by: Keegan Caruso <[email protected]> Add benchmark to test validation with an issuer delegate using string vs bytes issuer. Add a class with profiler methods.
brentschmaltz
pushed a commit
that referenced
this pull request
Sep 9, 2024
* Adding benchmark for new ValidateTokenAsync model vs old. * Update benchmark/Microsoft.IdentityModel.Benchmarks/ValidateTokenAsyncWithVPTests.cs * Removed IdentityComparer methods relating to removed result types. Updated tests for Algorithm and Audience validation * Removed ITokenValidationError interface. Updated tests * Replaced TokenValidationError with ExceptionDetails. Added stack frames to failures, added initial cache experiment for stack frames * Removed optionality from CancellationToken parameter * Updated tests * Added ClaimsIdentity to ValidationResult * Updated benchmarks to match the result types and added CancellationToken * Removed test consoleapp, re-grouped benchmarks for better comparison * Removed unit type * Removed TokenValidationError since it is no longer used * Restored ExceptionType type name * Restored ExceptionFromType method in ExceptionDetail * Override Result's empty initialiser and annotate it to prevent wrong initialisation * Removed commented code. * Commented empty if statement * Added cached stack frames and nullability annotations for ValidateTokenAsync Changed return type to use Result and removed error information from ValidationResult Added method to add StackFrames to ExceptionDetail * Added stack frames for ReadToken and DecryptToken * Added ValidationFailureType to ExceptionDetail * Updated tests to use ValidationFailureType * Update src/Microsoft.IdentityModel.Abstractions/Result.cs Co-authored-by: Keegan Caruso <[email protected]> * Moved Result to Tokens project, made it internal. Reverted TargetFrameworks change to LoggingExtensions * Addressed PR feedback around argument null exceptions * Addressed PR comments. Removed unused imports. Increased epsilon for datetime comparison in tests. --------- Co-authored-by: Franco Fung <[email protected]> Co-authored-by: Franco Fung <[email protected]> Co-authored-by: Keegan Caruso <[email protected]> Add benchmark to test validation with an issuer delegate using string vs bytes issuer. Add a class with profiler methods.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ValidateTokenAsync - New Path: Refactor result types
Refactored Result types to reduce allocations and improve performance.
Description
Result<ValidationResult, ExceptionDetail>
fromTokenValidationAsync
to limitValidationResult
to only success scenarios, simplifying scenarios like creating and accessing the ClaimsIdentity.Next steps after this:
Benchmarks for this work:
Sample exception message + stack trace generated with the new stack frames:
Part of #2711