Throw ArgumentException for malformed authorities (#4347)

* Throw argument exceptions for consistency when authority or tenant are invalid. Update method comments. * Update and add tests. * Apply suggestions from code review Co-authored-by: Den Delimarsky <53200638+localden@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com> * Small test fixes. * Update comments. --------- Co-authored-by: Den Delimarsky <53200638+localden@users.noreply.github.com> Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
AzureAD · Oct 11, 2023 · 474026f · 474026f · github-actions · Oct 11, 2023
1 parent b259cf0
commit 474026f
Show file tree

Hide file tree

Showing 21 changed files with 320 additions and 209 deletions.
diff --git a/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs b/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs
@@ -254,13 +254,16 @@ public T WithAuthority(AadAuthorityAudience authorityAudience, bool validateAuth
         /// 
         /// If an authority was not specified at the application level, the default used is https://login.microsoftonline.com/common.
         /// </summary>
-        /// <param name="tenantId">The tenant ID, which can be either in GUID format or a domain name. Also known as the Directory ID.</param>
+        /// <param name="tenantId">Tenant ID of the Microsoft Entra ID tenant
+        /// or a domain associated with this Microsoft Entra ID tenant, in order to sign-in a user of a specific organization only.</param>
         /// <returns>The builder to chain the .With methods.</returns>
-        /// <exception cref="ArgumentNullException">Thrown if tenantId is null or an empty string</exception>
-        /// <exception cref="MsalClientException">Thrown if the application was configured with an authority that is not AAD specific (e.g. ADFS or B2C).</exception>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="tenantId"/> is null or an empty string.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="tenantId"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in more general exception scenarios (for ex. if the application was configured with an authority that does not allow tenants).</exception>
         /// <remarks>
         /// The tenant should be more restrictive than the one configured at the application level, e.g. don't use "common".
-        /// Does not affect authority validation, which is specified at the application level.</remarks>
+        /// Does not affect authority validation, which is specified at the application level.
+        /// </remarks>
         public T WithTenantId(string tenantId)
         {
             if (string.IsNullOrEmpty(tenantId))
@@ -280,17 +283,17 @@ public T WithTenantId(string tenantId)
 
         /// <summary>
         /// Extracts the tenant ID from the provided authority URI and overrides the tenant ID specified in the authority at the application level. This operation preserves the authority host (environment) provided to the application builder.
-        /// 
         /// If an authority was not provided to the application builder, this method will replace the tenant ID in the default authority - https://login.microsoftonline.com/common.
         /// </summary>
         /// <param name="authorityUri">URI from which to extract the tenant ID</param>
-
         /// <returns>The builder to chain the .With methods.</returns>
-        /// <exception cref="ArgumentNullException">Thrown if <paramref name="authorityUri"/> is null or an empty string</exception>
-        /// <exception cref="MsalClientException">Thrown if the application was configured with an authority that is not AAD specific (e.g. ADFS or B2C).</exception>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="authorityUri"/> is null or an empty string.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="authorityUri"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in general exception scenarios (for example if the application was configured with an authority that does not allow tenants).</exception>
         /// <remarks>
         /// The tenant should be more restrictive than the one configured at the application level, e.g. don't use "common".
-        /// Does not affect authority validation, which is specified at the application level.</remarks>
+        /// Does not affect authority validation, which is specified at the application level.
+        /// </remarks>
         public T WithTenantIdFromAuthority(Uri authorityUri)
         {
             if (authorityUri == null)

diff --git a/src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs b/src/client/Microsoft.Identity.Client/AppConfig/AbstractApplicationBuilder.cs
@@ -256,11 +256,11 @@ public T WithRedirectUri(string redirectUri)
         }
 
         /// <summary>
-        /// Sets the Tenant Id of the organization from which the application will let
+        /// Sets the tenant ID of the organization from which the application will let
         /// users sign-in. This is classically a GUID or a domain name. See https://aka.ms/msal-net-application-configuration.
         /// Although it is also possible to set <paramref name="tenantId"/> to <c>common</c>,
         /// <c>organizations</c>, and <c>consumers</c>, it's recommended to use one of the
-        /// overrides of <see cref="WithAuthority(AzureCloudInstance, AadAuthorityAudience, bool)"/>
+        /// overrides of <see cref="WithAuthority(AzureCloudInstance, AadAuthorityAudience, bool)"/>.
         /// </summary>
         /// <param name="tenantId">tenant ID of the Azure AD tenant
         /// or a domain associated with this Azure AD tenant, in order to sign-in a user of a specific organization only</param>
@@ -428,16 +428,19 @@ internal override ApplicationConfiguration BuildConfiguration()
         }
 
         #region Authority
-        
+
 
         /// <summary>
-        /// Adds a known authority to the application from its Uri. See https://aka.ms/msal-net-application-configuration.
+        /// Adds a known authority to the application. See <see href="https://aka.ms/msal-net-application-configuration">Application configuration options</see>.
         /// This constructor is mainly used for scenarios where the authority is not a standard Azure AD authority,
         /// nor an ADFS authority, nor an Azure AD B2C authority. For Azure AD, even in sovereign clouds, prefer
         /// using other overrides such as <see cref="WithAuthority(AzureCloudInstance, AadAuthorityAudience, bool)"/>
         /// </summary>
-        /// <param name="authorityUri">Uri of the authority</param>
+        /// <param name="authorityUri">URI of the authority</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="authorityUri"/> is null.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="authorityUri"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in general exception scenarios (for example if the application was configured with multiple different authority hosts).</exception>
         /// <returns>The builder to chain the .With methods</returns>
         public T WithAuthority(Uri authorityUri, bool validateAuthority = true)
         {
@@ -451,19 +454,22 @@ public T WithAuthority(Uri authorityUri, bool validateAuthority = true)
 
         /// <summary>
         /// Adds a known Azure AD authority to the application to sign-in users specifying
-        /// the full authority Uri. See https://aka.ms/msal-net-application-configuration.
+        /// the full authority URI. See <see href="https://aka.ms/msal-net-application-configuration">Application configuration options</see>.
         /// </summary>
-        /// <param name="authorityUri">URL of the security token service (STS) from which MSAL.NET will acquire the tokens.
-        ///  Usual authorities endpoints for the Azure public Cloud are:
+        /// <param name="authorityUri">URI of the authority from which MSAL.NET will acquire the tokens.
+        ///  Authority endpoints for the Azure public Cloud are:
         ///  <list type="bullet">
         ///  <item><description><c>https://login.microsoftonline.com/tenant/</c> where <c>tenant</c> is the tenant ID of the Azure AD tenant
         ///  or a domain associated with this Azure AD tenant, in order to sign-in users of a specific organization only</description></item>
-        ///  <item><description><c>https://login.microsoftonline.com/common/</c> to sign-in users with any work and school accounts or Microsoft personal account</description></item>
+        ///  <item><description><c>https://login.microsoftonline.com/common/</c> to sign-in users with any work and school accounts or personal Microsoft accounts</description></item>
         ///  <item><description><c>https://login.microsoftonline.com/organizations/</c> to sign-in users with any work and school accounts</description></item>
         ///  <item><description><c>https://login.microsoftonline.com/consumers/</c> to sign-in users with only personal Microsoft accounts (live)</description></item>
         ///  </list>
         ///  Note that this setting needs to be consistent with what is declared in the application registration portal</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="authorityUri"/> is null or empty.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="authorityUri"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in general exception scenarios (for example if the application was configured with multiple different authority hosts).</exception>
         /// <returns>The builder to chain the .With methods</returns>
         public T WithAuthority(string authorityUri, bool validateAuthority = true)
         {
@@ -479,11 +485,14 @@ public T WithAuthority(string authorityUri, bool validateAuthority = true)
 
         /// <summary>
         /// Adds a known Azure AD authority to the application to sign-in users from a single
-        /// organization (single tenant application) specified by its tenant ID. See https://aka.ms/msal-net-application-configuration.
+        /// organization (single-tenant application) specified by its tenant ID. See <see href="https://aka.ms/msal-net-application-configuration">Application configuration options</see>.
         /// </summary>
-        /// <param name="cloudInstanceUri">Azure Cloud instance.</param>
+        /// <param name="cloudInstanceUri">Azure cloud instance.</param>
         /// <param name="tenantId">GUID of the tenant from which to sign-in users.</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="cloudInstanceUri"/> is null or empty.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="cloudInstanceUri"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in more general exception scenarios (for example if the application was configured with multiple different authority hosts).</exception>
         /// <returns>The builder to chain the .With methods.</returns>
         public T WithAuthority(
             string cloudInstanceUri,
@@ -496,18 +505,21 @@ public T WithAuthority(
 
         /// <summary>
         /// Adds a known Azure AD authority to the application to sign-in users from a single
-        /// organization (single tenant application) described by its domain name. See https://aka.ms/msal-net-application-configuration.
+        /// organization (single-tenant application) described by its domain name. See https://aka.ms/msal-net-application-configuration.
         /// </summary>
-        /// <param name="cloudInstanceUri">Uri to the Azure Cloud instance (for instance
+        /// <param name="cloudInstanceUri">Uri to the Azure cloud instance (for instance
         /// <c>https://login.microsoftonline.com)</c></param>
-        /// <param name="tenant">domain name associated with the tenant from which to sign-in users</param>
+        /// <param name="tenant">Domain name associated with the tenant from which to sign-in users</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
         /// <remarks>
         /// <paramref name="tenant"/> can also contain the string representation of a GUID (tenantId),
         /// or even <c>common</c>, <c>organizations</c> or <c>consumers</c> but in this case
         /// it's recommended to use another override (<see cref="WithAuthority(AzureCloudInstance, Guid, bool)"/>
         /// and <see cref="WithAuthority(AzureCloudInstance, AadAuthorityAudience, bool)"/>
         /// </remarks>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="cloudInstanceUri"/> or <paramref name="tenant"/> is null or empty.</exception>
+        /// <exception cref="ArgumentException">Thrown if <paramref name="cloudInstanceUri"/> or <paramref name="tenant"/> is not well-formatted (for example, has spaces).</exception>
+        /// <exception cref="MsalClientException">Thrown in more general exception scenarios (for example if the application was configured with multiple different authority hosts).</exception>
         /// <returns>The builder to chain the .With methods</returns>
         public T WithAuthority(
             string cloudInstanceUri,
@@ -537,8 +549,8 @@ public T WithAuthority(
         /// organization (single tenant application) described by its cloud instance and its tenant ID.
         /// See https://aka.ms/msal-net-application-configuration.
         /// </summary>
-        /// <param name="azureCloudInstance">Instance of Azure Cloud (for instance Azure
-        /// worldwide cloud, Azure German Cloud, US government ...)</param>
+        /// <param name="azureCloudInstance">Instance of Azure cloud (for example, Azure
+        /// public cloud, Azure China, or Azure Government).</param>
         /// <param name="tenantId">Tenant Id of the tenant from which to sign-in users</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
         /// <returns>The builder to chain the .With methods</returns>
@@ -553,14 +565,15 @@ public T WithAuthority(
 
         /// <summary>
         /// Adds a known Azure AD authority to the application to sign-in users from a single
-        /// organization (single tenant application) described by its cloud instance and its domain
+        /// organization (single-tenant application) described by its cloud instance and its domain
         /// name or tenant ID. See https://aka.ms/msal-net-application-configuration.
         /// </summary>
-        /// <param name="azureCloudInstance">Instance of Azure Cloud (for instance Azure
-        /// worldwide cloud, Azure German Cloud, US government ...).</param>
+        /// <param name="azureCloudInstance">Instance of Azure cloud (for example, Azure
+        /// public cloud, Azure China, or Azure Government).</param>
         /// <param name="tenant">Domain name associated with the Azure AD tenant from which
         /// to sign-in users. This can also be a GUID.</param>
         /// <param name="validateAuthority">Whether the authority should be validated against the server metadata.</param>
+        /// <exception cref="ArgumentNullException">Thrown if <paramref name="tenant"/> or <paramref name="tenant"/> is null or empty.</exception>
         /// <returns>The builder to chain the .With methods.</returns>
         public T WithAuthority(
             AzureCloudInstance azureCloudInstance,

diff --git a/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs b/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs
@@ -22,7 +22,7 @@ public AuthorityInfo(
             AuthorityType authorityType,
             string authority,
             bool validateAuthority)
-            : this(authorityType, new Uri(authority), validateAuthority)
+            : this(authorityType, ValidateAndCreateAuthorityUri(authority, authorityType), validateAuthority)
         {
 
         }
@@ -144,7 +144,7 @@ private AuthorityInfo(
             AuthorityType != AuthorityType.B2C;
 
         /// <summary>
-        /// Authority support multi-tenantcy. ADFS and Generic authorities are not tenanted.
+        /// Authority supports multi-tenancy. ADFS and Generic authorities are not tenanted.
         /// B2C doesn't allow multi-tenancy scenarios, but the authority itself is tenanted. 
         /// For CIAM, we allow multi-tenancy scenarios, and expect the STS to fail.
         /// </summary>
@@ -356,6 +356,11 @@ internal Authority CreateAuthority()
 
         #endregion
 
+        /// <summary>
+        /// Validates the authority string and creates a Uri object out of it.
+        /// Authority must not be null or whitespace, must be a well-formed URI (e.g. not include spaces), and must have an HTTPS schema.
+        /// Non-generic authorities must have at least one segment in the path.
+        /// </summary>
         private static Uri ValidateAndCreateAuthorityUri(string authority, AuthorityType? authorityType = null)
         {
             if (string.IsNullOrWhiteSpace(authority))
@@ -375,14 +380,14 @@ private static Uri ValidateAndCreateAuthorityUri(string authority, AuthorityType
                 throw new ArgumentException(MsalErrorMessage.AuthorityUriInsecure, nameof(authority));
             }
 
-            string path = authorityUri.AbsolutePath.Substring(1);
-            if (string.IsNullOrWhiteSpace(path) && !IsCiamAuthority(authorityUri))
-            {
-                throw new ArgumentException(MsalErrorMessage.AuthorityUriInvalidPath, nameof(authority));
-            }
-
             if (authorityType is not AuthorityType.Generic)
             {
+                string path = authorityUri.AbsolutePath.Substring(1);
+                if (string.IsNullOrWhiteSpace(path) && !IsCiamAuthority(authorityUri))
+                {
+                    throw new ArgumentException(MsalErrorMessage.AuthorityUriInvalidPath, nameof(authority));
+                }
+
                 string[] pathSegments = authorityUri.AbsolutePath.Substring(1).Split('/');
                 if (pathSegments == null || pathSegments.Length == 0)
                 {

diff --git a/src/client/Microsoft.Identity.Client/AppConfig/BaseAbstractApplicationBuilder.cs b/src/client/Microsoft.Identity.Client/AppConfig/BaseAbstractApplicationBuilder.cs
@@ -222,7 +222,7 @@ internal void ResolveAuthority()
                     {
                         throw new MsalClientException(
                             MsalError.TenantOverrideNonAad,
-                            $"Cannot use WithTenantId() in the application builder, because the authority {Config.Authority.AuthorityInfo.AuthorityType} doesn't support it");
+                            $"Cannot use WithTenantId(tenantId) in the application builder, because the authority {Config.Authority.AuthorityInfo.AuthorityType} doesn't support it.");
                     }
 
                     string tenantedAuthority = Config.Authority.GetTenantedAuthority(

diff --git a/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs b/src/client/Microsoft.Identity.Client/AppConfig/ConfidentialClientApplicationBuilder.cs
@@ -405,18 +405,16 @@ internal override void Validate()
         }
 
         /// <summary>
-        /// Builds the ConfidentialClientApplication from the parameters set
-        /// in the builder
+        /// Builds an instance of <see cref="IConfidentialClientApplication"/> 
+        /// from the parameters set in the <see cref="ConfidentialClientApplicationBuilder"/>.
         /// </summary>
-        /// <returns></returns>
+        /// <exception cref="MsalClientException">Thrown when errors occur locally in the library itself (for example, because of incorrect configuration).</exception>
+        /// <returns>An instance of <see cref="IConfidentialClientApplication"/></returns>
         public IConfidentialClientApplication Build()
         {
             return BuildConcrete();
         }
 
-        /// <summary>
-        /// </summary>
-        /// <returns></returns>
         internal ConfidentialClientApplication BuildConcrete()
         {
             return new ConfidentialClientApplication(BuildConfiguration());
Benchmark suite	Current: `474026f`	Previous: `b259cf0`	Ratio
`Microsoft.Identity.Test.Performance.AcquireTokenNoCacheTests.AcquireTokenForClient_TestAsync`	`728166.0721649484` ns (`± 479141.5139465203`)	`381486.5319148936` ns (`± 14794.855089657327`)	`1.91`
`Microsoft.Identity.Test.Performance.AcquireTokenNoCacheTests.AcquireTokenOnBehalfOf_TestAsync`	`721329.5348837209` ns (`± 217295.77053536402`)	`520742.75` ns (`± 10230.99661160469`)	`1.39`