Suggested changes to AudienceValidation (#2902)

* Suggested changes to AudienceValidation Remove ISecurityTokenException Some smaller cleanups * address PR feedback --------- Co-authored-by: id4s <[email protected]>
AzureAD · Oct 16, 2024 · abd7bd8 · abd7bd8
1 parent 5ac40df
commit abd7bd8
Show file tree

Hide file tree

Showing 16 changed files with 1,005 additions and 97 deletions.
diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ReadToken.cs b/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ReadToken.cs
@@ -26,7 +26,7 @@ internal static ValidationResult<SecurityToken> ReadToken(
             CallContext? callContext)
 #pragma warning disable CA1801 // TODO: remove pragma disable once callContext is used for logging
         {
-            if (String.IsNullOrEmpty(token))
+            if (string.IsNullOrEmpty(token))
             {
                 StackFrame nullTokenStackFrame = StackFrames.ReadTokenNullOrEmpty ?? new StackFrame(true);
                 return ValidationError.NullParameter(

diff --git a/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateSignature.cs b/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.ValidateSignature.cs
@@ -245,10 +245,9 @@ private static ValidationResult<SecurityKey> ValidateSignatureWithKey(
                     new MessageDetail(
                         TokenLogMessages.IDX10518,
                         result.UnwrapError().MessageDetail.Message),
-                    ValidationFailureType.SignatureValidationFailed,
-                    typeof(SecurityTokenInvalidSignatureException),
-                    new StackFrame(true),
-                    result.UnwrapError());
+                    ValidationFailureType.SignatureAlgorithmValidationFailed,
+                    typeof(SecurityTokenInvalidAlgorithmException),
+                    new StackFrame(true));
 
             SignatureProvider signatureProvider = cryptoProviderFactory.CreateForVerifying(key, jsonWebToken.Alg);
             try

diff --git a/src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenArgumentNullException.cs b/src/Microsoft.IdentityModel.Tokens/Exceptions/SecurityTokenArgumentNullException.cs
@@ -11,7 +11,7 @@
 
 namespace Microsoft.IdentityModel.Tokens
 {
-    internal class SecurityTokenArgumentNullException : ArgumentNullException, ISecurityTokenException
+    internal class SecurityTokenArgumentNullException : ArgumentNullException
     {
         private string? _stackTrace;
         private ValidationError? _validationError;

diff --git a/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt b/src/Microsoft.IdentityModel.Tokens/InternalAPI.Unshipped.txt
diff --git a/src/Microsoft.IdentityModel.Tokens/LogMessages.cs b/src/Microsoft.IdentityModel.Tokens/LogMessages.cs
@@ -84,6 +84,7 @@ internal static class LogMessages
         public const string IDX10265 = "IDX10265: Reading issuer signing keys from configuration.";
         //public const string IDX10266 = "IDX10266: Unable to validate issuer. validationParameters.ValidIssuer is null or whitespace, validationParameters.ValidIssuers is null or empty and ConfigurationManager is null.";
         public const string IDX10267 = "IDX10267: '{0}' has been called by a derived class '{1}' which has not implemented this method. For this call graph to succeed, '{1}' will need to implement '{0}'.";
+        public const string IDX10268 = "IDX10268: Unable to validate audience, validationParameters.ValidAudiences.Count == 0.";
 
 
         // 10500 - SignatureValidation

diff --git a/src/Microsoft.IdentityModel.Tokens/Utility.cs b/src/Microsoft.IdentityModel.Tokens/Utility.cs
@@ -79,6 +79,47 @@ internal static string SerializeAsSingleCommaDelimitedString(IEnumerable<string>
             return sb.ToString();
         }
 
+
+        /// <summary>
+        /// Serializes the list of strings into string as follows:
+        /// 'str1','str2','str3' ...
+        /// </summary>
+        /// <param name="strings">
+        /// The strings used to build a comma delimited string.
+        /// </param>
+        /// <returns>
+        /// The single <see cref="string"/>.
+        /// </returns>
+        internal static string SerializeAsSingleCommaDelimitedString(IList<string> strings)
+        {
+            if (strings == null)
+            {
+                return Utility.Null;
+            }
+
+            StringBuilder sb = new();
+            bool first = true;
+            for (int i = 0; i < strings.Count; i++)
+            {
+                if (first)
+                {
+                    sb.AppendFormat(CultureInfo.InvariantCulture, "{0}", strings[i] ?? Utility.Null);
+                    first = false;
+                }
+                else
+                {
+                    sb.AppendFormat(CultureInfo.InvariantCulture, ", {0}", strings[i] ?? Utility.Null);
+                }
+            }
+
+            if (first)
+            {
+                return Utility.Empty;
+            }
+
+            return sb.ToString();
+        }
+
         /// <summary>
         /// Returns whether the input string is https.
         /// </summary>

diff --git a/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/AudienceValidationError.cs b/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/AudienceValidationError.cs
@@ -10,31 +10,39 @@ namespace Microsoft.IdentityModel.Tokens
 {
     internal class AudienceValidationError : ValidationError
     {
-        private IList<string>? _invalidAudiences;
+        private IList<string>? _tokenAudiences;
+        private IList<string>? _validAudiences;
+
+        // stack frames associated with AudienceValidationErrors
+        internal static StackFrame? ValidationParametersNull;
+        internal static StackFrame? AudiencesNull;
+        internal static StackFrame? AudiencesCountZero;
+        internal static StackFrame? ValidationParametersAudiencesCountZero;
+        internal static StackFrame? ValidateAudienceFailed;
 
         public AudienceValidationError(
             MessageDetail messageDetail,
+            ValidationFailureType failureType,
             Type exceptionType,
             StackFrame stackFrame,
-            IList<string>? invalidAudiences)
-            : base(messageDetail, ValidationFailureType.AudienceValidationFailed, exceptionType, stackFrame)
+            IList<string>? tokenAudiences,
+            IList<string>? validAudiences)
+            : base(messageDetail, failureType, exceptionType, stackFrame)
         {
-            _invalidAudiences = invalidAudiences;
-        }
-
-        internal override void AddAdditionalInformation(ISecurityTokenException exception)
-        {
-            if (exception is SecurityTokenInvalidAudienceException invalidAudienceException)
-                invalidAudienceException.InvalidAudience = Utility.SerializeAsSingleCommaDelimitedString(_invalidAudiences);
+            _tokenAudiences = tokenAudiences;
+            _validAudiences = validAudiences;
         }
 
         /// <summary>
         /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/>
         /// </summary>
         /// <returns>An instance of an Exception.</returns>
-        public override Exception GetException()
+        internal override Exception GetException()
         {
-            return new SecurityTokenInvalidAudienceException(MessageDetail.Message) { InvalidAudience = Utility.SerializeAsSingleCommaDelimitedString(_invalidAudiences) };
+            if (ExceptionType == typeof(SecurityTokenInvalidAudienceException))
+                return new SecurityTokenInvalidAudienceException(MessageDetail.Message) { InvalidAudience = Utility.SerializeAsSingleCommaDelimitedString(_tokenAudiences) };
+
+            return base.GetException(ExceptionType, null);
         }
     }
 }

diff --git a/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/IssuerValidationError.cs b/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/IssuerValidationError.cs
@@ -21,7 +21,7 @@ internal IssuerValidationError(
             _invalidIssuer = invalidIssuer;
         }
 
-        public override Exception GetException()
+        internal override Exception GetException()
         {
             if (ExceptionType == typeof(SecurityTokenInvalidIssuerException))
             {

diff --git a/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/LifetimeValidationError.cs b/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/LifetimeValidationError.cs
@@ -46,7 +46,7 @@ public LifetimeValidationError(
         /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/>
         /// </summary>
         /// <returns>An instance of an Exception.</returns>
-        public override Exception GetException()
+        internal override Exception GetException()
         {
             if (ExceptionType == typeof(SecurityTokenNoExpirationException))
             {
@@ -75,7 +75,7 @@ public override Exception GetException()
                 };
             }
             else
-                return base.GetException();
+                return base.GetException(ExceptionType, null);
         }
     }
 }

diff --git a/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/ValidationError.cs b/src/Microsoft.IdentityModel.Tokens/Validation/Results/Details/ValidationError.cs
@@ -7,11 +7,6 @@
 
 namespace Microsoft.IdentityModel.Tokens
 {
-    internal interface ISecurityTokenException
-    {
-        void SetValidationError(ValidationError validationError);
-    }
-
     /// <summary>
     /// Contains information so that Exceptions can be logged or thrown written as required.
     /// </summary>
@@ -31,8 +26,9 @@ internal ValidationError(
             ValidationFailureType failureType,
             Type exceptionType,
             StackFrame stackFrame)
-            : this(MessageDetail, failureType, exceptionType, stackFrame, innerException: null)
+            : this(MessageDetail, failureType, exceptionType, stackFrame, null)
         {
+            // TODO: need to include CallContext.
         }
 
         /// <summary>
@@ -60,47 +56,24 @@ internal ValidationError(
             };
         }
 
-        internal ValidationError(
-            MessageDetail messageDetail,
-            ValidationFailureType failureType,
-            Type exceptionType,
-            StackFrame stackFrame,
-            ValidationError innerValidationError)
-        {
-            InnerValidationError = innerValidationError;
-            MessageDetail = messageDetail;
-            _exceptionType = exceptionType;
-            FailureType = failureType;
-            StackFrames = new List<StackFrame>(4)
-            {
-                stackFrame
-            };
-        }
-
         /// <summary>
         /// Creates an instance of an <see cref="Exception"/> using <see cref="ValidationError"/>
         /// </summary>
         /// <returns>An instance of an Exception.</returns>
-        public virtual Exception GetException()
+        internal virtual Exception GetException()
         {
-            Exception exception = GetException(ExceptionType, InnerException);
-
-            if (exception is ISecurityTokenException securityTokenException)
-            {
-                securityTokenException.SetValidationError(this);
-                AddAdditionalInformation(securityTokenException);
-            }
-
-            return exception;
+            return GetException(ExceptionType, InnerException);
         }
 
-        private Exception GetException(Type exceptionType, Exception innerException)
+        internal Exception GetException(Type exceptionType, Exception innerException)
         {
             Exception exception = null;
 
             if (innerException == null && InnerValidationError == null)
             {
-                if (exceptionType == typeof(SecurityTokenInvalidAudienceException))
+                if (exceptionType == typeof(SecurityTokenArgumentNullException))
+                    return new SecurityTokenArgumentNullException(MessageDetail.Message);
+                else if (exceptionType == typeof(SecurityTokenInvalidAudienceException))
                     exception = new SecurityTokenInvalidAudienceException(MessageDetail.Message);
                 else if (exceptionType == typeof(SecurityTokenInvalidIssuerException))
                     exception = new SecurityTokenInvalidIssuerException(MessageDetail.Message);
@@ -149,7 +122,9 @@ private Exception GetException(Type exceptionType, Exception innerException)
             {
                 Exception actualException = innerException ?? InnerValidationError.GetException();
 
-                if (exceptionType == typeof(SecurityTokenInvalidAudienceException))
+                if (exceptionType == typeof(SecurityTokenArgumentNullException))
+                    return new SecurityTokenArgumentNullException(MessageDetail.Message, innerException);
+                else if (exceptionType == typeof(SecurityTokenInvalidAudienceException))
                     exception = new SecurityTokenInvalidAudienceException(MessageDetail.Message, actualException);
                 else if (exceptionType == typeof(SecurityTokenInvalidIssuerException))
                     exception = new SecurityTokenInvalidIssuerException(MessageDetail.Message, actualException);
@@ -198,11 +173,6 @@ private Exception GetException(Type exceptionType, Exception innerException)
             return exception;
         }
 
-        internal virtual void AddAdditionalInformation(ISecurityTokenException exception)
-        {
-            // base implementation is no-op. Derived classes can override to add additional information to the exception.
-        }
-
         internal static ValidationError NullParameter(string parameterName, StackFrame stackFrame) => new(
             MessageDetail.NullParameter(parameterName),
             ValidationFailureType.NullArgument,

diff --git a/src/Microsoft.IdentityModel.Tokens/Validation/ValidationFailureType.cs b/src/Microsoft.IdentityModel.Tokens/Validation/ValidationFailureType.cs
@@ -45,6 +45,18 @@ private class AlgorithmValidationFailure : ValidationFailureType { internal Algo
         public static readonly ValidationFailureType AudienceValidationFailed = new AudienceValidationFailure("AudienceValidationFailed");
         private class AudienceValidationFailure : ValidationFailureType { internal AudienceValidationFailure(string name) : base(name) { } }
 
+        /// <summary>
+        /// Defines a type that represents that audience validation failed.
+        /// </summary>
+        public static readonly ValidationFailureType NoTokenAudiencesProvided = new NoTokenAudiencesProvidedFailure("NoTokenAudiencesProvided");
+        private class NoTokenAudiencesProvidedFailure : ValidationFailureType { internal NoTokenAudiencesProvidedFailure(string name) : base(name) { } }
+
+        /// <summary>
+        /// Defines a type that represents that audience validation failed.
+        /// </summary>
+        public static readonly ValidationFailureType NoValidationParameterAudiencesProvided = new NoValidationParameterAudiencesProvidedFailure("NoValidationParameterAudiencesProvided");
+        private class NoValidationParameterAudiencesProvidedFailure : ValidationFailureType { internal NoValidationParameterAudiencesProvidedFailure(string name) : base(name) { } }
+
         /// <summary>
         /// Defines a type that represents that token type validation failed.
         /// </summary>
@@ -57,6 +69,12 @@ private class TokenTypeValidationFailure : ValidationFailureType { internal Toke
         public static readonly ValidationFailureType SignatureValidationFailed = new SignatureValidationFailure("SignatureValidationFailed");
         private class SignatureValidationFailure : ValidationFailureType { internal SignatureValidationFailure(string name) : base(name) { } }
 
+        /// <summary>
+        /// Defines a type that represents that the token's signature algorithm validation failed.
+        /// </summary>
+        public static readonly ValidationFailureType SignatureAlgorithmValidationFailed = new SignatureAlgorithmValidationFailure("SignatureAlgorithmValidationFailed");
+        private class SignatureAlgorithmValidationFailure : ValidationFailureType { internal SignatureAlgorithmValidationFailure(string name) : base(name) { } }
+
         /// <summary>
         /// Defines a type that represents that signing key validation failed.
         /// </summary>