Implement New Audience Validator Method in SAML2TokenHandler#2853
Implement New Audience Validator Method in SAML2TokenHandler#2853FuPingFranco wants to merge 1 commit intodevfrom
Conversation
| First = true, | ||
| ExpectedException = ExpectedException.ArgumentNullException("IDX10000:"), | ||
| TestId = "TokenValidationParameters null", | ||
| TestId = "TokenValidationParameters_Null", |
There was a problem hiding this comment.
Instead of setting TestId this way - could you do it like so new TokenTheoryData("TokenValidationParameters_Null")? TokenTheoryData already has a ctor that takes a TestId and doing so this way allows theoryData.CallContext.DebugId to be set.
| #pragma warning disable 1591 | ||
|
|
||
| // general | ||
| internal const string IDX10000 = "IDX10000: The parameter '{0}' cannot be a 'null' or an empty object. "; //Q: Should we just use one set of validation errors? |
There was a problem hiding this comment.
why do we need new IDX???? codes?
There was a problem hiding this comment.
If this is just to use in the tests, we could simply import the LogMessages from the outer package, Tokens.
| /// <summary> | ||
| /// A <see cref="SecurityTokenHandler"/> designed for creating and validating Saml2 Tokens. See: http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf | ||
| /// </summary> | ||
| public partial class Saml2SecurityTokenHandler : SecurityTokenHandler |
There was a problem hiding this comment.
should this be internal for now?
| /// <remarks><see cref="Validators.ValidateAudience(IList{string}, SecurityToken, ValidationParameters, CallContext)"/> for additional details.</remarks> | ||
| internal static ValidationResult<string> ValidateAudience(IList<string> audiences, SecurityToken securityToken, ValidationParameters validationParameters, CallContext callContext) | ||
| { | ||
| return Validators.ValidateAudience(audiences, securityToken, validationParameters, callContext); |
There was a problem hiding this comment.
I do not think that adding methods simply to call the delegates is a good idea. We can refactor the methods that call this to use the delegates directly.
| { | ||
| get | ||
| { | ||
| return new TheoryData<AudienceValidationTheoryData> |
There was a problem hiding this comment.
Are all these tests re-testing the audience validation delegate that already has these tests?
See: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/test/Microsoft.IdentityModel.Tokens.Tests/Validation/AudienceValidationResultTests.cs
|
Closing this PR in favor of a different agreed approach. |
4 participants
Implement New Audience Validator Method in SAML2TokenHandler
Description
Adding new internal method to validate audience in SAML2TokenHandler