Skip to content
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

Modified token validation to be async throughout the call graph #2075

Merged
merged 1 commit into from
May 10, 2023

Conversation

brentschmaltz
Copy link
Member

ValidateTokenAsync was not async all the way through, this PR fixes that.
We added a new internal async delegate to validate the issuer to help think about how we should design a new set of delegates that support async.

@@ -100,6 +101,18 @@ namespace Microsoft.IdentityModel.Tokens
/// </remarks>
public delegate string IssuerValidatorUsingConfiguration(string issuer, SecurityToken securityToken, TokenValidationParameters validationParameters, BaseConfiguration configuration);

/// <summary>
/// Definition for IssuerValidatorAsync. Left internal for now while we work out the details of async validation for all delegates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal

This sounds like a TODO, should we link this with a bug/work item so its in our radar?

@@ -518,6 +536,17 @@ public virtual ClaimsIdentity CreateClaimsIdentity(SecurityToken securityToken,
public IssuerValidator IssuerValidator { get; set; }


/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<

nit: extra line above

/// <param name="configuration">The <see cref="BaseConfiguration"/> required for issuer and signing key validation.</param>
/// <returns>The issuer to use when creating the "Claim"(s) in a "ClaimsIdentity".</returns>
/// <exception cref="ArgumentNullException">If 'validationParameters' is null.</exception>
/// <exception cref="ArgumentNullException">If 'issuer' is null or whitespace and <see cref="TokenValidationParameters.ValidateIssuer"/> is true.</exception>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArgumentNullException

It seems this is not an ArgumentNullException but a SecurityTokenInvalidIssuerException (line 255)

/// <returns>The issuer to use when creating the "Claim"(s) in a "ClaimsIdentity".</returns>
/// <exception cref="ArgumentNullException">If 'validationParameters' is null.</exception>
/// <exception cref="ArgumentNullException">If 'issuer' is null or whitespace and <see cref="TokenValidationParameters.ValidateIssuer"/> is true.</exception>
/// <exception cref="ArgumentNullException">If ' configuration' is null.</exception>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ArgumentNullException

This argument can be null and not exception will be thrown.

if (string.IsNullOrWhiteSpace(validationParameters.ValidIssuer) && validationParameters.ValidIssuers.IsNullOrEmpty() && string.IsNullOrWhiteSpace(configuration?.Issuer))
throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX10204)
{ InvalidIssuer = issuer });
if ( string.IsNullOrWhiteSpace(validationParameters.ValidIssuer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space

/// <exception cref="SecurityTokenInvalidIssuerException">If <see cref="TokenValidationParameters.ValidIssuer"/> is null or whitespace and <see cref="TokenValidationParameters.ValidIssuers"/> is null and <see cref="BaseConfiguration.Issuer"/> is null.</exception>
/// <exception cref="SecurityTokenInvalidIssuerException">If 'issuer' failed to matched either <see cref="TokenValidationParameters.ValidIssuer"/> or one of <see cref="TokenValidationParameters.ValidIssuers"/> or <see cref="BaseConfiguration.Issuer"/>.</exception>
/// <remarks>An EXACT match is required.</remarks>
internal static async Task<string> ValidateIssuerAsync(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string

I think its strange that we return the string given that it is exactly the same as the parameter passed.

@@ -26,4 +26,10 @@
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.Validators, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.Validators.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.TestExtensions, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
[assembly: InternalsVisibleTo("Microsoft.IdentityModel.S2S, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InternalsVisibleTo

This could be part of the TODO mentioned on the TokenValidationParameters.cs, remove these entries once the method is public instead of internal.

/// <param name="validationParameters">Token validation parameters.</param>
/// <example><code>
/// AadIssuerValidator aadIssuerValidator = AadIssuerValidator.GetAadIssuerValidator(authority, httpClient);
/// TokenValidationParameters.IssuerValidator = aadIssuerValidator.Validate;
Copy link
Contributor

@victorm-hernandez victorm-hernandez May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code example was not updated to reflect the async change.

The return value should also reflect this is async returning a Task object.

Copy link
Contributor

@victorm-hernandez victorm-hernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@brentschmaltz brentschmaltz merged commit 2468413 into dev May 10, 2023
@brentschmaltz brentschmaltz deleted the brentsch/ValidateAsync branch May 16, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants