-
Notifications
You must be signed in to change notification settings - Fork 433
Update return from validation steps to use M.I.Abstractions.OperationResult #3284
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
Conversation
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenDecryptionFailedException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenDecryptionFailedException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenValidationException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenInvalidAlgorithmException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenInvalidAlgorithmException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenExpiredException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenExpiredException.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenInvalidOperationException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.JsonWebTokens/Experimental/JsonWebTokenHandler.DecryptToken.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Experimental/Validation/Results/Details/ValidationError.cs
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
This PR updates validation step return types from ValidationResult to use OperationResult from M.I.Abstractions, as part of redesigning the validation architecture. The validation steps now return ValidationError objects that are hierarchical, and validation logic has been consolidated across JWT, SAML, and SAML2 tokens to be token-agnostic.
Key Changes:
- Modified validation delegates to return
OperationResult<T, ValidationError>
instead of custom ValidationResult types - Updated ValidationError classes to use new exception patterns and remove ExceptionType dependencies
- Consolidated validation logic across token types to be token-agnostic
- Renamed properties (IssuerSigningKeys → SignatureKeys, TokenDecryptionKeys → DecryptionKeys)
Reviewed Changes
Copilot reviewed 137 out of 200 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
ExtensibilityTheoryData.cs | Updated base test class to use TokenHandler and SecurityToken directly instead of string types |
CustomValidationErrors.cs | Refactored validation error classes to use new exception patterns and remove ExceptionType references |
CustomTokenTypeValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomTokenReplayValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomSignatureValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomLifetimeValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomIssuerValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomIssuerSigningKeyValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomExceptions.cs | Updated custom exception constructors to accept ValidationError parameters |
CustomAudienceValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
CustomAlgorithmValidationDelegates.cs | Updated delegates to return OperationResult and use new validation failure types |
TestUtilities.cs | Updated property names from IssuerSigningKeys to SignatureKeys |
TestCaseProvider.cs | Added comprehensive test case provider for various validation scenarios |
SkipValidationDelegates.cs | Updated delegate signatures and return types |
SamlClaimsIdentityComparisonTestBase.cs | Updated to use OperationResult and new property names |
IdentityComparer.cs | Updated validation error comparison logic |
ITestingTokenHandler.cs | Complete rewrite of testing token handler interface and implementations |
Comments suppressed due to low confidence (4)
test/Microsoft.IdentityModel.TestUtils/TokenValidationExtensibility/CustomValidationErrors.cs:4
- Remove commented-out using statements. These should be deleted rather than commented out to maintain code cleanliness.
using System;
test/Microsoft.IdentityModel.TestUtils/TokenValidationExtensibility/CustomValidationErrors.cs:4
- Remove commented-out using statements. These should be deleted rather than commented out to maintain code cleanliness.
using System;
test/Microsoft.IdentityModel.TestUtils/TokenValidationExtensibility/CustomAudienceValidationDelegates.cs:66
- Misplaced semicolon after the constructor call. The semicolon should be on the same line as the closing parenthesis rather than on a separate line.
null)
test/Microsoft.IdentityModel.TestUtils/TokenValidationExtensibility/CustomAudienceValidationDelegates.cs:67
- Unnecessary semicolon on separate line. This should be removed or moved to the previous line.
;
...rosoft.IdentityModel.TestUtils/TokenValidationExtensibility/Tests/ExtensibilityTheoryData.cs
Outdated
Show resolved
Hide resolved
...rosoft.IdentityModel.TestUtils/TokenValidationExtensibility/Tests/ExtensibilityTheoryData.cs
Outdated
Show resolved
Hide resolved
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
81e78cf
to
d53180e
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
d53180e
to
a8712df
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
a8712df
to
5c5e9aa
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenInvalidOperationException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenInvalidOperationException.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Experimental/Validation/ValidationFailureType.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Xml/Exceptions/XmlWriteException.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Validation/LifetimeValidationResultTests.cs
Show resolved
Hide resolved
test/Microsoft.IdentityModel.Tokens.Tests/Extensibility/ExtensibilityRunner.cs
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens.Saml/Saml/Experimental/SamlSecurityTokenHandler.ReadToken.cs
Outdated
Show resolved
Hide resolved
Standardize validation across token types.
5c5e9aa
to
4d49ed4
Compare
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
src/Microsoft.IdentityModel.Tokens.Saml/Saml/Experimental/SamlSecurityTokenHandler.ReadToken.cs
Outdated
Show resolved
Hide resolved
...icrosoft.IdentityModel.Tokens.Saml/Saml2/Experimental/Saml2SecurityTokenHandler.ReadToken.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.IdentityModel.Tokens/Experimental/Validation/Results/Details/MessageDetail.cs
Show resolved
Hide resolved
SummarySummary
CoverageMicrosoft.IdentityModel.JsonWebTokens - 80.3%
|
Validation steps for all but Signature have been redesigned so that each token handler will call the same validation method.
Each validation step will return a ValidationError that is associated with that step.
For example, AudienceValidation will return a result (string) OR an AudienceValidationError.
An internal ValidationStep, such as ValidateAudienceInternal was created so the users can hook a custom validation step, then call into the default, ValidateAudience, if desired.
Each ValidationError derived type will return an Exception associated with the error.
For example, AudienceValiationError will return SecurityTokenInvalidAudienceException.
ValidationFailureTypes are now hierarchial.
Tests for Extensibility, TokenValidation were rewritten in a token agnostic manner so that a validation step returned the same results regardless of JWT, Saml or Saml2.
Test results were compared to the previous validation logic using TokenValidationParameters to ensure correctness.
Saml and Saml2 tokens now share the same signature validation logic.
Saml and Saml2 validation step called ValidateConditions was replaced with ValidateAudience, ValidateLifetime to be consistent with JWT.
Code that was not used or not needed was removed.
Names of some properties were renamed, such as ValidationParameters.IssuerSigningKeys -> ValidationParameters.SignatureKeys.
Some names were shortened, ValidationParameters.TokenDecryptionKeys -> ValidationParameters.DecryptionKeys.
Redundant tests were removed.
This is still experimental code and there needs to be additional work before shipping such as: