Remove Delegate Checks in Multiple Validators and Prevents Null Setting of Delegates (#2725)

* Removed delegate checks for token type validation.

* Fix un-referenced message check

* Added internal delegate for token type validation

* Prevent delegates from being null.

* Simplify Valid types getter

---------

Co-authored-by: Franco Fung <francofung@microsoft.com>

Author: FuPingFranco

src/Microsoft.IdentityModel.Tokens/LogMessages.cs

@@ -73,7 +73,6 @@ internal static class LogMessages
         public const string IDX10256 = "IDX10256: Unable to validate the token type. TokenValidationParameters.ValidTypes is set, but the 'typ' header claim is null or empty.";
         public const string IDX10257 = "IDX10257: Token type validation failed. Type: '{0}'. Did not match: validationParameters.TokenTypes: '{1}'.";
         public const string IDX10258 = "IDX10258: Token type validated. Type: '{0}'.";
-        public const string IDX10259 = "IDX10259: Unable to validate the token type, delegate threw an exception.";
         // public const string IDX10260 = "IDX10260:";
         public const string IDX10261 = "IDX10261: Unable to retrieve configuration from authority: '{0}'. \nProceeding with token validation in case the relevant properties have been set manually on the TokenValidationParameters. Exception caught: \n {1}. See https://aka.ms/validate-using-configuration-manager for additional information.";
         public const string IDX10262 = "IDX10262: One of the issuers in TokenValidationParameters.ValidIssuers was null or an empty string. See https://aka.ms/wilson/tokenvalidation for details.";

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Security.Claims;
+using System.Threading;
 using Microsoft.IdentityModel.Abstractions;
 using Microsoft.IdentityModel.Logging;
 
@@ -20,8 +21,13 @@ internal class ValidationParameters
         private string _nameClaimType = ClaimsIdentity.DefaultNameClaimType;
         private string _roleClaimType = ClaimsIdentity.DefaultRoleClaimType;
         private Dictionary<string, object> _instancePropertyBag;
+        private IList<string> _validTokenTypes = [];
 
-        private IssuerValidationDelegateAsync _issuerValidationDelegate = Validators.ValidateIssuerAsync;
+        private AudienceValidatorDelegate _audienceValidator = Validators.ValidateAudience;
+        private IssuerValidationDelegateAsync _issuerValidatorAsync = Validators.ValidateIssuerAsync;
+        private LifetimeValidatorDelegate _lifetimeValidator = Validators.ValidateLifetime;
+        private TypeValidatorDelegate _typeValidator = Validators.ValidateTokenType;
+        private TokenReplayValidatorDelegate _tokenReplayValidator = Validators.ValidateTokenReplay;
 
         /// <summary>
         /// This is the default value of <see cref="ClaimsIdentity.AuthenticationType"/> when creating a <see cref="ClaimsIdentity"/>.
@@ -112,14 +118,25 @@ public ValidationParameters()
         public AlgorithmValidator AlgorithmValidator { get; set; }
 
         /// <summary>
-        /// Gets or sets a delegate that will be used to validate the audience.
+        /// Allows overriding the delegate that will be used to validate the audience.
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the 'audience', instead of default processing.
+        /// If set, this delegate will be responsible for validating the 'audience', instead of default 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; }
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="AudienceValidatorDelegate"/> used to validate the issuer of a token</returns>
+        public AudienceValidatorDelegate AudienceValidator
+        {
+            get
+            {
+                return _audienceValidator;
+            }
+            set
+            {
+                _audienceValidator = value ?? throw new ArgumentNullException(nameof(value), "AudienceValidator cannot be set as null.");
+            }
+        }
 
         /// <summary>
         /// Gets or sets the AuthenticationType when creating a <see cref="ClaimsIdentity"/>.
@@ -278,17 +295,19 @@ public virtual ClaimsIdentity CreateClaimsIdentity(SecurityToken securityToken,
         public IList<SecurityKey> IssuerSigningKeys { get; }
 
         /// <summary>
-        /// Gets or sets a delegate that will be used to validate the issuer of the token.
+        /// Allows overriding the delegate that will be used to validate the issuer of the token.
         /// </summary>
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="IssuerValidationDelegateAsync"/> used to validate the issuer of a token</returns>
         public IssuerValidationDelegateAsync IssuerValidatorAsync
         {
             get
             {
-                return _issuerValidationDelegate;
+                return _issuerValidatorAsync;
             }
             set
             {
-                _issuerValidationDelegate = value ?? throw LogHelper.LogArgumentNullException(nameof(value));
+                _issuerValidatorAsync = value ?? throw new ArgumentNullException(nameof(value), "IssuerValidatorAsync cannot be set as null.");
             }
         }
 
@@ -298,9 +317,21 @@ public IssuerValidationDelegateAsync IssuerValidatorAsync
         public TransformBeforeSignatureValidation TransformBeforeSignatureValidation { get; set; }
 
         /// <summary>
-        /// Gets or sets a delegate that will be used to validate the lifetime of the token
+        /// Allows overriding the delegate that will be used to validate the lifetime of the token
         /// </summary>
-        public LifetimeValidator LifetimeValidator { get; set; }
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="LifetimeValidatorDelegate"/> used to validate the lifetime of a token</returns>
+        public LifetimeValidatorDelegate LifetimeValidator
+        {
+            get
+            {
+                return _lifetimeValidator;
+            }
+            set
+            {
+                _lifetimeValidator = value ?? throw new ArgumentNullException(nameof(value), "LifetimeValidator cannot be set as null.");
+            }
+        }
 
         /// <summary>
         /// Gets or sets a <see cref="bool"/> that will decide if the token identifier claim needs to be logged.
@@ -430,26 +461,51 @@ public string RoleClaimType
         public ITokenReplayCache TokenReplayCache { get; set; }
 
         /// <summary>
-        /// Gets or sets a delegate that will be used to validate the token replay of the token
+        /// Allows overriding the 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 default processing.
+        /// If no delegate is set, the default implementation will be used.
         /// 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; }
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="TokenReplayValidatorDelegate"/> used to validate the token replay of the token.</returns>
+        public TokenReplayValidatorDelegate TokenReplayValidator
+        {
+            get
+            {
+                return _tokenReplayValidator;
+            }
+
+            set
+            {
+                _tokenReplayValidator = value ?? throw new ArgumentNullException(nameof(value), "TokenReplayValidator cannot be set as null.");
+            }
+        }
 
         /// <summary>
-        /// Gets or sets a delegate that will be used to validate the type of the token.
-        /// If the token type cannot be validated, an exception MUST be thrown by the delegate.
+        /// Allows overriding the delegate that will be used to validate the type of the token.
+        /// If the token type cannot be validated, a <see cref="TokenTypeValidationResult"/> MUST be returned by the delegate.
         /// Note: the 'type' parameter may be null if it couldn't be extracted from its usual location.
         /// Implementations that need to resolve it from a different location can use the 'token' parameter.
         /// </summary>
         /// <remarks>
-        /// If set, this delegate will be called to validate the 'type' of the token, instead of default processing.
-        /// This means that no default 'type' validation will occur.
+        /// If no delegate is set, the default implementation will be used. The default checks the type
+        /// against the <see cref="ValidTypes"/> property, if the type is present then, it will succeed.
         /// </remarks>
-        public TypeValidator TypeValidator { get; set; }
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="TypeValidatorDelegate"/> used to validate the token type of a token</returns>
+        public TypeValidatorDelegate TypeValidator
+        {
+            get
+            {
+                return _typeValidator;
+            }
+
+            set
+            {
+                _typeValidator = value ?? throw new ArgumentNullException(nameof(value), "TypeValidator cannot be set as null.");
+            }
+        }
 
         /// <summary>
         /// Gets or sets a boolean to control if the LKG configuration will be used for token validation.
@@ -494,9 +550,21 @@ public string RoleClaimType
         /// Gets the <see cref="IList{String}"/> that contains valid types that will be used to check against the JWT header's 'typ' claim.
         /// If this property is not set, the 'typ' header claim will not be validated and all types will be accepted.
         /// In the case of a JWE, this property will ONLY apply to the inner token header.
-        /// The default is <c>null</c>.
+        /// The default is an empty collection.
         /// </summary>
-        public IList<string> ValidTypes { get; }
+        /// <exception cref="ArgumentNullException">Thrown when the value is set as null.</exception>
+        /// <returns>The <see cref="IList{String}"/> that contains valid token types that will be used to check against the token's 'typ' claim.</returns>
+        public IList<string> ValidTypes
+        {
+            get
+            {
+                return _validTokenTypes;
+            }
+            set
+            {
+                _validTokenTypes = value ?? throw new ArgumentNullException(nameof(value));
+            }
+        }
 
         public bool ValidateActor { get; set; }
     }

src/Microsoft.IdentityModel.Tokens/Validation/Validators.Algorithm.cs

@@ -17,7 +17,7 @@ public static partial class Validators
         /// <param name="algorithm">The algorithm to be validated.</param>
         /// <param name="securityKey">The <see cref="SecurityKey"/> that signed the <see cref="SecurityToken"/>.</param>
         /// <param name="securityToken">The <see cref="SecurityToken"/> being validated.</param>
-        /// <param name="validationParameters"><see cref="TokenValidationParameters"/> required for validation.</param>
+        /// <param name="validationParameters"><see cref="ValidationParameters"/> required for validation.</param>
         /// <param name="callContext"></param>
 #pragma warning disable CA1801 // TODO: remove pragma disable once callContext is used for logging
         internal static AlgorithmValidationResult ValidateAlgorithm(

src/Microsoft.IdentityModel.Tokens/Validation/Validators.Audience.cs

@@ -20,7 +20,7 @@ namespace Microsoft.IdentityModel.Tokens
     /// <param name="callContext"></param>
     /// <returns>A <see cref="IssuerValidationResult"/>that contains the results of validating the issuer.</returns>
     /// <remarks>This delegate is not expected to throw.</remarks>
-    internal delegate AudienceValidationResult ValidateAudience(
+    internal delegate AudienceValidationResult AudienceValidatorDelegate(
         IEnumerable<string> audiences,
         SecurityToken? securityToken,
         TokenValidationParameters validationParameters,

src/Microsoft.IdentityModel.Tokens/Validation/Validators.Lifetime.cs

@@ -19,11 +19,11 @@ namespace Microsoft.IdentityModel.Tokens
     /// <param name="callContext"></param>
     /// <returns>A <see cref="IssuerValidationResult"/>that contains the results of validating the issuer.</returns>
     /// <remarks>This delegate is not expected to throw.</remarks>
-    internal delegate LifetimeValidationResult LifetimeValidationDelegate(
+    internal delegate LifetimeValidationResult LifetimeValidatorDelegate(
         DateTime? notBefore,
         DateTime? expires,
         SecurityToken? securityToken,
-        TokenValidationParameters validationParameters,
+        ValidationParameters validationParameters,
         CallContext callContext);
 
     /// <summary>
@@ -37,18 +37,18 @@ public static partial class Validators
         /// <param name="notBefore">The 'notBefore' time found in the <see cref="SecurityToken"/>.</param>
         /// <param name="expires">The 'expiration' time found in the <see cref="SecurityToken"/>.</param>
         /// <param name="securityToken">The <see cref="SecurityToken"/> being validated.</param>
-        /// <param name="validationParameters">The <see cref="TokenValidationParameters"/> to be used for validating the token.</param>
+        /// <param name="validationParameters">The <see cref="ValidationParameters"/> to be used for validating the token.</param>
         /// <param name="callContext"></param>
         /// <returns>A <see cref="LifetimeValidationResult"/> indicating whether validation was successful, and providing a <see cref="SecurityTokenInvalidLifetimeException"/> if it was not.</returns>
         /// <exception cref="ArgumentNullException">If 'validationParameters' is null.</exception>
-        /// <exception cref="SecurityTokenNoExpirationException">If 'expires.HasValue' is false and <see cref="TokenValidationParameters.RequireExpirationTime"/> is true.</exception>
+        /// <exception cref="SecurityTokenNoExpirationException">If 'expires.HasValue' is false.</exception>
         /// <exception cref="SecurityTokenInvalidLifetimeException">If 'notBefore' is &gt; 'expires'.</exception>
         /// <exception cref="SecurityTokenNotYetValidException">If 'notBefore' is &gt; DateTime.UtcNow.</exception>
         /// <exception cref="SecurityTokenExpiredException">If 'expires' is &lt; DateTime.UtcNow.</exception>
-        /// <remarks>All time comparisons apply <see cref="TokenValidationParameters.ClockSkew"/>.</remarks>
+        /// <remarks>All time comparisons apply <see cref="ValidationParameters.ClockSkew"/>.</remarks>
         /// <remarks>Exceptions are not thrown, but embedded in <see cref="LifetimeValidationResult.Exception"/>.</remarks>
 #pragma warning disable CA1801 // TODO: remove pragma disable once callContext is used for logging
-        internal static LifetimeValidationResult ValidateLifetime(DateTime? notBefore, DateTime? expires, SecurityToken? securityToken, TokenValidationParameters validationParameters, CallContext callContext)
+        internal static LifetimeValidationResult ValidateLifetime(DateTime? notBefore, DateTime? expires, SecurityToken? securityToken, ValidationParameters validationParameters, CallContext callContext)
 #pragma warning restore CA1801
         {
             if (validationParameters == null)
@@ -63,16 +63,7 @@ internal static LifetimeValidationResult ValidateLifetime(DateTime? notBefore, D
                         typeof(ArgumentNullException),
                         new StackFrame(true)));
 
-            if (validationParameters.LifetimeValidator != null)
-                return ValidateLifetimeUsingDelegate(notBefore, expires, securityToken, validationParameters);
-
-            if (!validationParameters.ValidateLifetime)
-            {
-                LogHelper.LogInformation(LogMessages.IDX10238);
-                return new LifetimeValidationResult(notBefore, expires);
-            }
-
-            if (!expires.HasValue && validationParameters.RequireExpirationTime)
+            if (!expires.HasValue)
                 return new LifetimeValidationResult(
                     notBefore,
                     expires,
@@ -130,42 +121,6 @@ internal static LifetimeValidationResult ValidateLifetime(DateTime? notBefore, D
 
             return new LifetimeValidationResult(notBefore, expires);
         }
-
-        private static LifetimeValidationResult ValidateLifetimeUsingDelegate(DateTime? notBefore, DateTime? expires, SecurityToken? securityToken, TokenValidationParameters validationParameters)
-        {
-            try
-            {
-                if (!validationParameters.LifetimeValidator(notBefore, expires, securityToken, validationParameters))
-                    return new LifetimeValidationResult(
-                    notBefore,
-                    expires,
-                    ValidationFailureType.LifetimeValidationFailed,
-                    new ExceptionDetail(
-                        new MessageDetail(
-                            LogMessages.IDX10230,
-                            securityToken),
-                        typeof(SecurityTokenInvalidLifetimeException),
-                        new StackFrame(true)));
-
-                return new LifetimeValidationResult(notBefore, expires);
-            }
-#pragma warning disable CA1031 // Do not catch general exception types
-            catch (Exception delegateException)
-#pragma warning restore CA1031 // Do not catch general exception types
-            {
-                return new LifetimeValidationResult(
-                    notBefore,
-                    expires,
-                    ValidationFailureType.LifetimeValidationFailed,
-                    new ExceptionDetail(
-                        new MessageDetail(
-                            LogMessages.IDX10230,
-                            securityToken),
-                        delegateException.GetType(),
-                        new StackFrame(true),
-                        delegateException));
-            }
-        }
     }
 }
 #nullable restore