Validators call delegates before checking if validation should occur (#…

…1272)
AzureAD · Jan 7, 2020 · 0cd60ff · 0cd60ff
1 parent 4ffb9a3
commit 0cd60ff
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 45 deletions.
diff --git a/src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs b/src/Microsoft.IdentityModel.Tokens/TokenValidationParameters.cs
@@ -214,8 +214,8 @@ public TokenValidationParameters()
         /// Gets or sets a delegate that will be used to validate the audience.
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the 'audience' instead of normal processing.
-        /// If <see cref="ValidateAudience"/> is false, this delegate will not be called.
+        /// If set, this delegate will be called to validate the 'audience', instead of normal processing. This means that no default 'audience' validation will occur.
+        /// Even if <see cref="ValidateAudience"/> is false, this delegate will still be called.
         /// </remarks>
         public AudienceValidator AudienceValidator { get; set; }
 
@@ -329,7 +329,8 @@ public virtual ClaimsIdentity CreateClaimsIdentity(SecurityToken securityToken,
         /// Gets or sets a delegate for validating the <see cref="SecurityKey"/> that signed the token.
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the <see cref="SecurityKey"/> that signed the token, instead of normal processing.
+        /// If set, this delegate will be called to validate the <see cref="SecurityKey"/> that signed the token, instead of normal processing. This means that no default <see cref="SecurityKey"/> validation will occur.
+        /// Even if <see cref="ValidateIssuerSigningKey"/> is false, this delegate will still be called.
         /// </remarks>
         public IssuerSigningKeyValidator IssuerSigningKeyValidator { get; set; }
 
@@ -355,17 +356,17 @@ public virtual ClaimsIdentity CreateClaimsIdentity(SecurityToken securityToken,
         /// Gets or sets a delegate that will be used to validate the issuer of the token.
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the 'issuer' of the token, instead of normal processing.
-        /// If <see cref="ValidateIssuer"/> is false, this delegate will not be called.
+        /// If set, this delegate will be called to validate the 'issuer' of the token, instead of normal processing. This means that no default 'issuer' validation will occur.
+        /// Even if <see cref="ValidateIssuer"/> is false, this delegate will still be called.
         /// </remarks>
         public IssuerValidator IssuerValidator { get; set; }
 
         /// <summary>
         /// Gets or sets a delegate that will be used to validate the lifetime of the token
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the lifetime of the token, instead of normal processing.
-        /// If <see cref="ValidateLifetime"/> is false, this delegate will not be called.
+        /// If set, this delegate will be called to validate the lifetime of the token, instead of normal processing. This means that no default lifetime validation will occur.
+        /// Even if <see cref="ValidateLifetime"/> is false, this delegate will still be called.
         /// </remarks>
         public LifetimeValidator LifetimeValidator { get; set; }
 
@@ -500,8 +501,8 @@ public string RoleClaimType
         /// Gets or sets a delegate that will be used to validate the token replay of the token
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the token replay of the token, instead of normal processing.
-        /// If <see cref="ValidateTokenReplay"/> is false, this delegate will not be called.
+        /// If set, this delegate will be called to validate the token replay of the token, instead of normal processing. This means no default token replay validation will occur.
+        /// Even if <see cref="ValidateTokenReplay"/> is false, this delegate will still be called.
         /// </remarks>
         public TokenReplayValidator TokenReplayValidator { get; set; }
 
@@ -515,7 +516,9 @@ public string RoleClaimType
         /// Gets or sets a boolean to control if the audience will be validated during token validation.
         /// </summary>
         /// <remarks>Validation of the audience, mitigates forwarding attacks. For example, a site that receives a token, could not replay it to another side.
-        /// A forwarded token would contain the audience of the original site.</remarks>
+        /// A forwarded token would contain the audience of the original site.
+        /// This boolean only applies to default audience validation. If <see cref="AudienceValidator"/> is set, it will be called regardless of whether this
+        /// property is true or false.</remarks>
         [DefaultValue(true)]
         public bool ValidateAudience { get; set; }
 
@@ -528,27 +531,40 @@ public string RoleClaimType
         /// It is possible that a token issued for the same audience could be from a different tenant. For example an application could accept users from
         /// contoso.onmicrosoft.com but not fabrikam.onmicrosoft.com, both valid tenants. A application that accepts tokens from fabrikam could forward them
         /// to the application that accepts tokens for contoso.
+        /// This boolean only applies to default issuer validation. If <see cref= "IssuerValidator" /> is set, it will be called regardless of whether this
+        /// property is true or false.
         /// </remarks>
         [DefaultValue(true)]
         public bool ValidateIssuer { get; set; }
 
         /// <summary>
         /// Gets or sets a boolean to control if the lifetime will be validated during token validation.
-        /// </summary>                
+        /// </summary> 
+        /// <remarks>
+        /// This boolean only applies to default lifetime validation. If <see cref= "LifetimeValidator" /> is set, it will be called regardless of whether this
+        /// property is true or false.
+        /// </remarks>
         [DefaultValue(true)]
         public bool ValidateLifetime { get; set; }
 
         /// <summary>
         /// Gets or sets a boolean that controls if validation of the <see cref="SecurityKey"/> that signed the securityToken is called.
         /// </summary>
         /// <remarks>It is possible for tokens to contain the public key needed to check the signature. For example, X509Data can be hydrated into an X509Certificate,
-        /// which can be used to validate the signature. In these cases it is important to validate the SigningKey that was used to validate the signature. </remarks>
+        /// which can be used to validate the signature. In these cases it is important to validate the SigningKey that was used to validate the signature. 
+        /// This boolean only applies to default signing key validation. If <see cref= "IssuerSigningKeyValidator" /> is set, it will be called regardless of whether this
+        /// property is true or false.
+        /// </remarks>
         [DefaultValue(false)]
         public bool ValidateIssuerSigningKey { get; set; }
 
         /// <summary>
         /// Gets or sets a boolean to control if the token replay will be validated during token validation.
-        /// </summary>                
+        /// </summary> 
+        /// <remarks>
+        /// This boolean only applies to default token replay validation. If <see cref= "TokenReplayValidator" /> is set, it will be called regardless of whether this
+        /// property is true or false.
+        /// </remarks>
         [DefaultValue(false)]
         public bool ValidateTokenReplay { get; set; }
 

diff --git a/src/Microsoft.IdentityModel.Tokens/Validators.cs b/src/Microsoft.IdentityModel.Tokens/Validators.cs
@@ -53,12 +53,6 @@ public static void ValidateAudience(IEnumerable<string> audiences, SecurityToken
             if (validationParameters == null)
                 throw LogHelper.LogArgumentNullException(nameof(validationParameters));
 
-            if (!validationParameters.ValidateAudience)
-            {
-                LogHelper.LogWarning(LogMessages.IDX10233);
-                return;
-            }
-
             if (validationParameters.AudienceValidator != null)
             {
                 if (!validationParameters.AudienceValidator(audiences, securityToken, validationParameters))
@@ -70,6 +64,12 @@ public static void ValidateAudience(IEnumerable<string> audiences, SecurityToken
                 return;
             }
 
+            if (!validationParameters.ValidateAudience)
+            {
+                LogHelper.LogWarning(LogMessages.IDX10233);
+                return;
+            }
+
             if (audiences == null)
                 throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidAudienceException(LogMessages.IDX10207) { InvalidAudience = null });
 
@@ -127,15 +127,15 @@ public static string ValidateIssuer(string issuer, SecurityToken securityToken,
             if (validationParameters == null)
                 throw LogHelper.LogArgumentNullException(nameof(validationParameters));
 
+            if (validationParameters.IssuerValidator != null)
+                return validationParameters.IssuerValidator(issuer, securityToken, validationParameters);
+
             if (!validationParameters.ValidateIssuer)
             {
                 LogHelper.LogInformation(LogMessages.IDX10235);
                 return issuer;
             }
 
-            if (validationParameters.IssuerValidator != null)
-                return validationParameters.IssuerValidator(issuer, securityToken, validationParameters);
-
             if (string.IsNullOrWhiteSpace(issuer))
                 throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidIssuerException(LogMessages.IDX10211)
                     { InvalidIssuer = issuer });
@@ -188,16 +188,16 @@ public static void ValidateIssuerSecurityKey(SecurityKey securityKey, SecurityTo
             if (validationParameters == null)
                 throw LogHelper.LogArgumentNullException(nameof(validationParameters));
 
-            if (!validationParameters.ValidateIssuerSigningKey)
+            if (validationParameters.IssuerSigningKeyValidator != null)
             {
-                LogHelper.LogInformation(LogMessages.IDX10237);
-                return;
+                if (!validationParameters.IssuerSigningKeyValidator(securityKey, securityToken, validationParameters))
+                    throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidSigningKeyException(LogHelper.FormatInvariant(LogMessages.IDX10232, securityKey)) { SigningKey = securityKey });
             }
 
-            if (validationParameters.IssuerSigningKeyValidator != null)
+            if (!validationParameters.ValidateIssuerSigningKey)
             {
-                if (!validationParameters.IssuerSigningKeyValidator(securityKey, securityToken, validationParameters))
-                    throw LogHelper.LogExceptionMessage(new SecurityTokenInvalidSigningKeyException(LogHelper.FormatInvariant(LogMessages.IDX10232, securityKey)){ SigningKey = securityKey });
+                LogHelper.LogInformation(LogMessages.IDX10237);
+                return;
             }
 
             if (!validationParameters.RequireSignedTokens && securityKey == null)
@@ -250,12 +250,6 @@ public static void ValidateLifetime(DateTime? notBefore, DateTime? expires, Secu
             if (validationParameters == null)
                 throw LogHelper.LogArgumentNullException(nameof(validationParameters));
 
-            if (!validationParameters.ValidateLifetime)
-            {
-                LogHelper.LogInformation(LogMessages.IDX10238);
-                return;
-            }
-
             if (validationParameters.LifetimeValidator != null)
             {
                 if (!validationParameters.LifetimeValidator(notBefore, expires, securityToken, validationParameters))
@@ -265,6 +259,12 @@ public static void ValidateLifetime(DateTime? notBefore, DateTime? expires, Secu
                 return;
             }
 
+            if (!validationParameters.ValidateLifetime)
+            {
+                LogHelper.LogInformation(LogMessages.IDX10238);
+                return;
+            }
+
             if (!expires.HasValue && validationParameters.RequireExpirationTime)
                 throw LogHelper.LogExceptionMessage(new SecurityTokenNoExpirationException(LogHelper.FormatInvariant(LogMessages.IDX10225, securityToken == null ? "null" : securityToken.GetType().ToString())));
 
@@ -304,16 +304,16 @@ public static void ValidateTokenReplay(DateTime? expirationTime, string security
             if (validationParameters == null)
                 throw LogHelper.LogArgumentNullException(nameof(validationParameters));
 
-            if (!validationParameters.ValidateTokenReplay)
+            if (validationParameters.TokenReplayValidator != null)
             {
-                LogHelper.LogInformation(LogMessages.IDX10246);
+                if (!validationParameters.TokenReplayValidator(expirationTime, securityToken, validationParameters))
+                    throw LogHelper.LogExceptionMessage(new SecurityTokenReplayDetectedException(LogHelper.FormatInvariant(LogMessages.IDX10228, securityToken)));
                 return;
             }
 
-            if (validationParameters.TokenReplayValidator != null)
+            if (!validationParameters.ValidateTokenReplay)
             {
-                if (!validationParameters.TokenReplayValidator(expirationTime, securityToken, validationParameters))
-                    throw LogHelper.LogExceptionMessage(new SecurityTokenReplayDetectedException(LogHelper.FormatInvariant(LogMessages.IDX10228, securityToken)));
+                LogHelper.LogInformation(LogMessages.IDX10246);
                 return;
             }
 

diff --git a/test/Microsoft.IdentityModel.TestUtils/ReferenceTheoryData.cs b/test/Microsoft.IdentityModel.TestUtils/ReferenceTheoryData.cs
@@ -26,12 +26,14 @@ public static TheoryData<TokenReplayTheoryData> TokenReplayValidationTheoryData
                     new TokenReplayTheoryData
                     {
                         TestId = $"ValidateTokenReplay: false, {nameof(ValidationDelegates.TokenReplayValidatorReturnsFalse)}",
-                        TokenReplayValidator = ValidationDelegates.TokenReplayValidatorReturnsFalse
+                        TokenReplayValidator = ValidationDelegates.TokenReplayValidatorReturnsFalse,
+                        ExpectedException = ExpectedException.SecurityTokenReplayDetected("IDX10228:")
                     },
                     new TokenReplayTheoryData
                     {
                         TestId = $"ValidateTokenReplay: false, {nameof(ValidationDelegates.TokenReplayValidatorThrows)}",
-                        TokenReplayValidator = ValidationDelegates.TokenReplayValidatorThrows
+                        TokenReplayValidator = ValidationDelegates.TokenReplayValidatorThrows,
+                        ExpectedException = ExpectedException.SecurityTokenReplayDetected("TokenReplayValidatorThrows")
                     },
                     new TokenReplayTheoryData
                     {

diff --git a/test/Microsoft.IdentityModel.Tokens.Saml.Tests/Saml2SecurityTokenHandlerTests.cs b/test/Microsoft.IdentityModel.Tokens.Saml.Tests/Saml2SecurityTokenHandlerTests.cs
@@ -431,6 +431,7 @@ public static TheoryData<Saml2TheoryData> WriteTokenTheoryData
 
                 theoryData.Add(new Saml2TheoryData
                 {
+                    ExpectedException = new ExpectedException(typeof(SecurityTokenInvalidSigningKeyException)),
                     SecurityToken = tokenHandler.CreateToken(tokenDescriptor),
                     TestId = nameof(ValidationDelegates.IssuerSecurityKeyValidatorThrows) + "-false",
                     ValidationParameters = validationParameters
@@ -468,6 +469,7 @@ public static TheoryData<Saml2TheoryData> WriteTokenTheoryData
 
                 theoryData.Add(new Saml2TheoryData
                 {
+                    ExpectedException = new ExpectedException(typeof(SecurityTokenInvalidAudienceException)),
                     SecurityToken = tokenHandler.CreateToken(tokenDescriptor),
                     TestId = nameof(ValidationDelegates.AudienceValidatorThrows) + "-false",
                     ValidationParameters = validationParameters

diff --git a/test/Microsoft.IdentityModel.Tokens.Saml.Tests/SamlSecurityTokenHandlerTests.cs b/test/Microsoft.IdentityModel.Tokens.Saml.Tests/SamlSecurityTokenHandlerTests.cs
@@ -873,6 +873,7 @@ public static TheoryData<SamlTheoryData> WriteTokenTheoryData
 
                 theoryData.Add(new SamlTheoryData
                 {
+                    ExpectedException = new ExpectedException(typeof(SecurityTokenInvalidSigningKeyException)),
                     SecurityToken = tokenHandler.CreateToken(tokenDescriptor),
                     TestId = nameof(ValidationDelegates.IssuerSecurityKeyValidatorThrows) + "-false",
                     ValidationParameters = validationParameters
@@ -910,6 +911,7 @@ public static TheoryData<SamlTheoryData> WriteTokenTheoryData
 
                 theoryData.Add(new SamlTheoryData
                 {
+                    ExpectedException = new ExpectedException(typeof(SecurityTokenInvalidAudienceException)),
                     SecurityToken = tokenHandler.CreateToken(tokenDescriptor),
                     TestId = nameof(ValidationDelegates.AudienceValidatorThrows) + "-false",
                     ValidationParameters = validationParameters

diff --git a/test/Microsoft.IdentityModel.Tokens.Saml.Tests/ValidateTheoryData.cs b/test/Microsoft.IdentityModel.Tokens.Saml.Tests/ValidateTheoryData.cs
@@ -106,6 +106,7 @@ public static void AddValidateAudienceTheoryData(List<TokenTheoryData> theoryDat
             theoryData.Add(new TokenTheoryData
             {
                 Audiences = new List<string> { "John" },
+                ExpectedException = ExpectedException.SecurityTokenInvalidAudienceException(),
                 TestId = "AudienceValidator throws, validateAudience false",
                 ValidationParameters = new TokenValidationParameters
                 {