Remove Delegate Checks Audience Validator and Prevents Null Setting of Delegate (#2758)

* Re-factor audience validator to use ValidatationParameters.

* Addressing feedback

* Address PR feedback. (#2765)

* Addressed PR feedback. Updated tests

* Removed exception from documentation

* Updated TokenType tests

---------

Co-authored-by: Ignacio Inglese <[email protected]>

---------

Co-authored-by: Franco Fung <[email protected]>
Co-authored-by: Ignacio Inglese <[email protected]>

Author: 3 people

src/Microsoft.IdentityModel.Tokens/LogMessages.cs

@@ -37,6 +37,7 @@ internal static class LogMessages
         public const string IDX10209 = "IDX10209: Token has length: '{0}' which is larger than the MaximumTokenSizeInBytes: '{1}'.";
         public const string IDX10211 = "IDX10211: Unable to validate issuer. The 'issuer' parameter is null or whitespace.";
         public const string IDX10214 = "IDX10214: Audience validation failed. Audiences: '{0}'. Did not match: validationParameters.ValidAudience: '{1}' or validationParameters.ValidAudiences: '{2}'.";
+        public const string IDX10215 = "IDX10215: Audience validation failed. Audiences: '{0}'. Did not match: validationParameters.ValidAudiences: '{1}'.";
         public const string IDX10222 = "IDX10222: Lifetime validation failed. The token is not yet valid. ValidFrom (UTC): '{0}', Current time (UTC): '{1}'.";
         public const string IDX10223 = "IDX10223: Lifetime validation failed. The token is expired. ValidTo (UTC): '{0}', Current time (UTC): '{1}'.";
         public const string IDX10224 = "IDX10224: Lifetime validation failed. The NotBefore (UTC): '{0}' is after Expires (UTC): '{1}'.";

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

@@ -21,7 +21,8 @@ internal class ValidationParameters
         private string _nameClaimType = ClaimsIdentity.DefaultNameClaimType;
         private string _roleClaimType = ClaimsIdentity.DefaultRoleClaimType;
         private Dictionary<string, object> _instancePropertyBag;
-        private IList<string> _validTokenTypes = [];
+        private IList<string> _validTokenTypes;
+        private IList<string> _validAudiences;
 
         private AlgorithmValidatorDelegate _algorithmValidator = Validators.ValidateAlgorithm;
         private AudienceValidatorDelegate _audienceValidator = Validators.ValidateAudience;
@@ -89,9 +90,9 @@ protected ValidationParameters(ValidationParameters other)
             ValidateSignatureLast = other.ValidateSignatureLast;
             ValidateWithLKG = other.ValidateWithLKG;
             ValidAlgorithms = other.ValidAlgorithms;
-            ValidAudiences = other.ValidAudiences;
+            _validAudiences = other.ValidAudiences;
             ValidIssuers = other.ValidIssuers;
-            ValidTypes = other.ValidTypes;
+            _validTokenTypes = other.ValidTypes;
         }
 
         /// <summary>
@@ -513,9 +514,12 @@ public TypeValidatorDelegate TypeValidator
 
         /// <summary>
         /// Gets the <see cref="IList{String}"/> that contains valid audiences that will be used to check against the token's audience.
-        /// The default is <c>null</c>.
+        /// The default is an empty collection.
         /// </summary>
-        public IList<string> ValidAudiences { get; }
+        public IList<string> ValidAudiences =>
+            _validAudiences ??
+            Interlocked.CompareExchange(ref _validAudiences, [], null) ??
+            _validAudiences;
 
         /// <summary>
         /// Gets the <see cref="IList{String}"/> that contains valid issuers that will be used to check against the token's issuer.
@@ -529,19 +533,11 @@ public TypeValidatorDelegate TypeValidator
         /// In the case of a JWE, this property will ONLY apply to the inner token header.
         /// The default is an empty collection.
         /// </summary>
-        /// <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 IList<string> ValidTypes =>
+            _validTokenTypes ??
+            Interlocked.CompareExchange(ref _validTokenTypes, [], null) ??
+            _validTokenTypes;
 
         public bool ValidateActor { get; set; }
     }

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

@@ -21,9 +21,9 @@ namespace Microsoft.IdentityModel.Tokens
     /// <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 AudienceValidatorDelegate(
-        IEnumerable<string> audiences,
+        IList<string> audiences,
         SecurityToken? securityToken,
-        TokenValidationParameters validationParameters,
+        ValidationParameters validationParameters,
         CallContext callContext);
 
     /// <summary>
@@ -34,7 +34,7 @@ public static partial class Validators
         /// <summary>
         /// Determines if the audiences found in a <see cref="SecurityToken"/> are valid.
         /// </summary>
-        /// <param name="audiences">The audiences found in the <see cref="SecurityToken"/>.</param>
+        /// <param name="tokenAudiences">The audiences 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="callContext"></param>
@@ -44,12 +44,12 @@ public static partial class Validators
         /// <exception cref="SecurityTokenInvalidAudienceException">If none of the 'audiences' matched either <see cref="TokenValidationParameters.ValidAudience"/> or one of <see cref="TokenValidationParameters.ValidAudiences"/>.</exception>
         /// <remarks>An EXACT match is required.</remarks>
 #pragma warning disable CA1801 // TODO: remove pragma disable once callContext is used for logging
-        internal static AudienceValidationResult ValidateAudience(IEnumerable<string> audiences, SecurityToken? securityToken, TokenValidationParameters validationParameters, CallContext callContext)
+        internal static AudienceValidationResult ValidateAudience(IList<string> tokenAudiences, SecurityToken? securityToken, ValidationParameters validationParameters, CallContext callContext)
 #pragma warning restore CA1801
         {
             if (validationParameters == null)
                 return new AudienceValidationResult(
-                    Utility.SerializeAsSingleCommaDelimitedString(audiences),
+                    Utility.SerializeAsSingleCommaDelimitedString(tokenAudiences),
                     ValidationFailureType.NullArgument,
                     new ExceptionDetail(
                         new MessageDetail(
@@ -58,15 +58,9 @@ internal static AudienceValidationResult ValidateAudience(IEnumerable<string> au
                         typeof(ArgumentNullException),
                         new StackFrame(true)));
 
-            if (!validationParameters.ValidateAudience)
-            {
-                LogHelper.LogWarning(LogMessages.IDX10233);
-                return new AudienceValidationResult(Utility.SerializeAsSingleCommaDelimitedString(audiences));
-            }
-
-            if (audiences == null)
+            if (tokenAudiences == null)
                 return new AudienceValidationResult(
-                    Utility.SerializeAsSingleCommaDelimitedString(audiences),
+                    Utility.SerializeAsSingleCommaDelimitedString(tokenAudiences),
                     ValidationFailureType.NullArgument,
                     new ExceptionDetail(
                         new MessageDetail(
@@ -75,23 +69,9 @@ internal static AudienceValidationResult ValidateAudience(IEnumerable<string> au
                         typeof(SecurityTokenInvalidAudienceException),
                         new StackFrame(true)));
 
-            if (string.IsNullOrWhiteSpace(validationParameters.ValidAudience) && (validationParameters.ValidAudiences == null))
+            if (tokenAudiences.Count == 0)
                 return new AudienceValidationResult(
-                    Utility.SerializeAsSingleCommaDelimitedString(audiences),
-                    ValidationFailureType.NullArgument,
-                    new ExceptionDetail(
-                        new MessageDetail(
-                            LogMessages.IDX10208,
-                            null),
-                        typeof(SecurityTokenInvalidAudienceException),
-                        new StackFrame(true)));
-
-            if (audiences is not List<string> audiencesAsList)
-                audiencesAsList = audiences.ToList();
-
-            if (audiencesAsList.Count == 0)
-                return new AudienceValidationResult(
-                    Utility.SerializeAsSingleCommaDelimitedString(audiencesAsList),
+                    Utility.SerializeAsSingleCommaDelimitedString(tokenAudiences),
                     ValidationFailureType.NullArgument,
                     new ExceptionDetail(
                         new MessageDetail(
@@ -100,81 +80,37 @@ internal static AudienceValidationResult ValidateAudience(IEnumerable<string> au
                         typeof(SecurityTokenInvalidAudienceException),
                         new StackFrame(true)));
 
-            string? validAudience = AudienceIsValidReturning(audiencesAsList, validationParameters);
+            string? validAudience = ValidTokenAudience(tokenAudiences, validationParameters.ValidAudiences, validationParameters.IgnoreTrailingSlashWhenValidatingAudience);
             if (validAudience != null)
-            {
                 return new AudienceValidationResult(validAudience);
-            }
 
             return new AudienceValidationResult(
-                    Utility.SerializeAsSingleCommaDelimitedString(audiencesAsList),
+                    Utility.SerializeAsSingleCommaDelimitedString(tokenAudiences),
                     ValidationFailureType.AudienceValidationFailed,
                     new ExceptionDetail(
                         new MessageDetail(
-                            LogMessages.IDX10214,
-                            LogHelper.MarkAsNonPII(Utility.SerializeAsSingleCommaDelimitedString(audiencesAsList)),
-                            LogHelper.MarkAsNonPII(validationParameters.ValidAudience ?? "null"),
+                            LogMessages.IDX10215,
+                            LogHelper.MarkAsNonPII(Utility.SerializeAsSingleCommaDelimitedString(tokenAudiences)),
                             LogHelper.MarkAsNonPII(Utility.SerializeAsSingleCommaDelimitedString(validationParameters.ValidAudiences))),
                         typeof(SecurityTokenInvalidAudienceException),
                         new StackFrame(true)));
         }
 
-        private static bool AudienceIsValid(List<string> audiences, TokenValidationParameters validationParameters)
-        {
-            return AudienceIsValidReturning(audiences, validationParameters) != null;
-        }
-
-        private static string? AudienceIsValidReturning(List<string> audiences, TokenValidationParameters validationParameters)
-        {
-            string? validAudience = null;
-            if (!string.IsNullOrWhiteSpace(validationParameters.ValidAudience))
-                validAudience = AudiencesMatchSingle(audiences, validationParameters.ValidAudience, validationParameters.IgnoreTrailingSlashWhenValidatingAudience);
-
-            if (validAudience == null && validationParameters.ValidAudiences != null)
-            {
-                if (validationParameters.ValidAudiences is not List<string> validAudiences)
-                    validAudiences = validationParameters.ValidAudiences.ToList();
-
-                validAudience = AudiencesMatchList(audiences, validAudiences, validationParameters.IgnoreTrailingSlashWhenValidatingAudience);
-            }
-
-            return validAudience;
-        }
-
-        private static string? AudiencesMatchSingle(List<string> audiences, string validAudience, bool ignoreTrailingSlashWhenValidatingAudience)
+        private static string? ValidTokenAudience(IList<string> tokenAudiences, IList<string> validAudiences, bool ignoreTrailingSlashWhenValidatingAudience)
         {
-            for (int i = 0; i < audiences.Count; i++)
+            for (int i = 0; i < tokenAudiences.Count; i++)
             {
-                string tokenAudience = audiences[i];
-                if (string.IsNullOrWhiteSpace(tokenAudience))
+                string tokenAudience = tokenAudiences[i];
+                if (string.IsNullOrEmpty(tokenAudience))
                     continue;
 
-                if (AudiencesMatch(ignoreTrailingSlashWhenValidatingAudience, tokenAudience, validAudience))
+                for (int j = 0; j < validAudiences.Count; j++)
                 {
-                    if (LogHelper.IsEnabled(EventLogLevel.Informational))
-                        LogHelper.LogInformation(LogMessages.IDX10234, LogHelper.MarkAsNonPII(tokenAudience));
-
-                    return tokenAudience;
-                }
-            }
-
-            return null;
-        }
-
-        private static string? AudiencesMatchList(IList<string> audiences, List<string> validAudiences, bool ignoreTrailingSlashWhenValidatingAudience)
-        {
-            for (int i = 0; i < audiences.Count; i++)
-            {
-                string tokenAudience = audiences[i];
-                if (string.IsNullOrWhiteSpace(tokenAudience))
-                    continue;
-
-                foreach (string validAudience in validAudiences)
-                {
-                    if (string.IsNullOrEmpty(validAudience))
+                    if (string.IsNullOrEmpty(validAudiences[j]))
                         continue;
 
-                    if (AudiencesMatch(ignoreTrailingSlashWhenValidatingAudience, tokenAudience, validAudience))
+
+                    if (AudienceMatches(ignoreTrailingSlashWhenValidatingAudience, tokenAudience, validAudiences[j]))
                     {
                         if (LogHelper.IsEnabled(EventLogLevel.Informational))
                             LogHelper.LogInformation(LogMessages.IDX10234, LogHelper.MarkAsNonPII(tokenAudience));
@@ -187,17 +123,17 @@ private static bool AudienceIsValid(List<string> audiences, TokenValidationParam
             return null;
         }
 
-        private static bool AudiencesMatch(bool ignoreTrailingSlashWhenValidatingAudience, string tokenAudience, string validAudience)
+        private static bool AudienceMatches(bool ignoreTrailingSlashWhenValidatingAudience, string tokenAudience, string validAudience)
         {
             if (validAudience.Length == tokenAudience.Length)
                 return string.Equals(validAudience, tokenAudience);
-            else if (ignoreTrailingSlashWhenValidatingAudience && NewAudiencesMatchIgnoringTrailingSlash(tokenAudience, validAudience))
+            else if (ignoreTrailingSlashWhenValidatingAudience && AudienceMatchesIgnoringTrailingSlash(tokenAudience, validAudience))
                 return true;
 
             return false;
         }
 
-        private static bool NewAudiencesMatchIgnoringTrailingSlash(string tokenAudience, string validAudience)
+        private static bool AudienceMatchesIgnoringTrailingSlash(string tokenAudience, string validAudience)
         {
             int length = -1;