Updated documentation for the new validation model and restructured internals #3056
+1,044
−462
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
… of GetCurrentStackFrame() and AddCurrentStackFrame()
…fields onto their own files and made the structures read-only.
src/Microsoft.IdentityModel.Tokens/Validation/Validators.IssuerSigningKey.cs
Show resolved
Hide resolved
…g IList values from two-part constructors.
… no ActorValidationParameters are provided.
…once the classes/structures are made public.
There was a problem hiding this comment.
Copilot reviewed 28 out of 43 changed files in this pull request and generated no comments.
Files not reviewed (15)
- src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt: Language not supported
- src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateToken.StackFrames.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Saml2SecurityTokenHandler.ValidateToken.StackFrames.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens.Saml/Saml/SamlSecurityTokenHandler.ValidateToken.StackFrames.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/AlgorithmValidationError.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Saml2SecurityTokenHandler.ValidateToken.Internal.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/IssuerSigningKeyValidationError.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/AudienceValidationError.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens.Saml/Saml/SamlSecurityTokenHandler.ValidateToken.Internal.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens.Saml/Saml2/Exceptions/Saml2ValidationError.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/MessageDetail.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.DecryptToken.cs: Evaluated as low risk
- src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ReadToken.cs: Evaluated as low risk
jmprieur
approved these changes
Jan 5, 2025
| /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/> | ||
| /// </summary> | ||
| /// <returns>An instance of an Exception.</returns> | ||
| public override Exception GetException() |
There was a problem hiding this comment.
If this method is called several times, shouldn't the exception be cached?
| /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/> | ||
| /// </summary> | ||
| /// <returns>An instance of an Exception.</returns> | ||
| public override Exception GetException() | ||
| { | ||
| if (ExceptionType == typeof(Saml2SecurityTokenReadException)) | ||
| { |
There was a problem hiding this comment.
same question. Don't we want to cache the exception?
| /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/> | ||
| /// </summary> | ||
| /// <returns>An instance of an Exception.</returns> | ||
| public override Exception GetException() | ||
| { | ||
| if (ExceptionType == typeof(SecurityTokenInvalidAlgorithmException)) | ||
| { |
| @@ -28,7 +41,7 @@ public AudienceValidationError( | |||
| /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/> | |||
| /// </summary> | |||
| /// <returns>An instance of an Exception.</returns> | |||
| internal override Exception GetException() | |||
| public override Exception GetException() | |||
| { | |||
| if (ExceptionType == typeof(SecurityTokenInvalidAudienceException)) | |||
| { | |||
| /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/> | ||
| /// </summary> | ||
| /// <returns>An instance of an Exception.</returns> | ||
| public override Exception GetException() | ||
| { | ||
| if (ExceptionType == typeof(SecurityTokenInvalidSigningKeyException)) | ||
| { |
| /// <param name="securityToken">The <see cref="SecurityToken"/> that is being validated.</param> | ||
| /// <param name="tokenHandler">The <see cref="TokenHandler"/> that is being used to validate the token.</param> | ||
| /// <param name="validationParameters">The <see cref="ValidationParameters"/> to be used for validating the token.</param> | ||
| internal class ValidatedToken( |
There was a problem hiding this comment.
is it ValidatedToken? Or ValidatedTokenResult?
| @@ -177,6 +201,9 @@ private object ClaimsIdentitySyncObj | |||
| #endregion | |||
|
|
|||
| #region Logging | |||
| /// <summary> | |||
| /// Internal class used for logging. | |||
| /// </summary> | |||
| private static class Logger | |||
| { | |||
| private static readonly Action<ILogger, string, string, Exception?> s_tokenValidationFailed = | |||
There was a problem hiding this comment.
no test against the log level?
| @@ -233,8 +213,7 @@ await ValidateJWSAsync(decryptedToken!, validationParameters, configuration, cal | |||
|
|
|||
| if (!validationResult.IsValid) | |||
| { | |||
| StackFrame validationFailureStackFrame = StackFrames.JWEValidationFailed ??= new StackFrame(true); | |||
| return validationResult.UnwrapError().AddStackFrame(validationFailureStackFrame); | |||
| return validationResult.UnwrapError().AddCurrentStackFrame(); | |||
| } | |||
|
|
|||
| JsonWebToken jsonWebToken = (validationResult.UnwrapResult().SecurityToken as JsonWebToken)!; | |||
There was a problem hiding this comment.
line 248, can we do better (more precise?) than catching the general Exception class?
| return ValidationError.NullParameter( | ||
| nameof(jwtToken), | ||
| tokenNullStackFrame); | ||
| ValidationError.GetCurrentStackFrame()); |
There was a problem hiding this comment.
Suggested change
| ValidationError.GetCurrentStackFrame()); | |
| ValidationError.GetCurrentStackFrame(), | |
| "The JWT token provided is null. Ensure that a valid token is passed."); |
| return ValidationError.NullParameter( | ||
| nameof(validationParameters), | ||
| validationParametersNullStackFrame); | ||
| ValidationError.GetCurrentStackFrame()); |
There was a problem hiding this comment.
Suggested change
| ValidationError.GetCurrentStackFrame()); | |
| ValidationError.GetCurrentStackFrame() | |
| "Validation parameters are missing. Provide the necessary parameters for token validation."); |
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.
Updated documentation for the new validation model and restructured internals
Clean up work and documentation updates for the new validation model.
GetCurrentStackFrame()andAddCurrentStackFrame()IssuerValidationSourceto be extensible.Part of #2711