Fix for #4272 - WithAdfsAuthority when id token has tid (#4275)

* Fix for #4272 - WithAdfsAuthority when id token has tid

* Fix for #4272 - WithAdfsAuthority when id token has tid

* Fixes

* fix

src/client/Microsoft.Identity.Client/Instance/AdfsAuthority.cs

@@ -48,6 +48,19 @@ internal override Task<string> GetDeviceCodeEndpointAsync(RequestContext request
             return Task.FromResult(deviceEndpoint);  
         }
 
+        /// <summary>
+        /// ADFS seems to support tenanted authorities, but the tenant ID is fixed so for all intents and purposes 
+        /// it remains constant
+        /// </summary>
+        /// <param name="tenantId"></param>
+        /// <param name="forceSpecifiedTenant"></param>
+        /// <returns></returns>
+        /// <exception cref="System.NotImplementedException"></exception>
+        internal override string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant)
+        {
+            return AuthorityInfo.CanonicalAuthority.ToString();
+        }
+
         internal override string TenantId => null;
     }
 }

src/client/Microsoft.Identity.Client/Instance/Authority.cs

@@ -82,12 +82,19 @@ internal static Authority CreateAuthorityWithTenant(AuthorityInfo authorityInfo,
 
             string tenantedAuthority = initialAuthority.GetTenantedAuthority(tenantId, forceSpecifiedTenant: false);
 
-            return CreateAuthority(tenantedAuthority, authorityInfo.ValidateAuthority);
+            // don't re-create the whole authority info, no need for parsing, as the type cannot change
+            var newAuthorityInfo = new AuthorityInfo(
+                initialAuthority.AuthorityInfo.AuthorityType,
+                tenantedAuthority,
+                initialAuthority.AuthorityInfo.ValidateAuthority);
+
+            return CreateAuthority(newAuthorityInfo);
         }
 
         internal static Authority CreateAuthorityWithEnvironment(AuthorityInfo authorityInfo, string environment)
         {
-            if (authorityInfo.AuthorityType == AuthorityType.Generic)
+            // don't change the environment if it's not supported
+            if (!authorityInfo.IsInstanceDiscoverySupported)
             {
                 return CreateAuthority(authorityInfo);
             }
@@ -110,13 +117,8 @@ internal static Authority CreateAuthorityWithEnvironment(AuthorityInfo authority
         /// </summary>
         /// <param name="tenantId">The new tenant id</param>
         /// <param name="forceSpecifiedTenant">Forces the change, even if the current tenant is not "common" or "organizations" or "consumers"</param>
-        internal virtual string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant)
-        {
-            throw new MsalClientException(
-                MsalError.TenantOverrideNonAad,
-                MsalErrorMessage.TenantOverrideNonAad);
-        }
-
+        internal abstract string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant);
+
         internal abstract Task<string> GetTokenEndpointAsync(RequestContext requestContext);
 
         internal abstract Task<string> GetAuthorizationEndpointAsync(RequestContext requestContext);

src/client/Microsoft.Identity.Client/Instance/AuthorityManager.cs

@@ -62,7 +62,11 @@ public async Task RunInstanceDiscoveryAndValidationAsync()
                                     _metadata.PreferredNetwork);
 
                 // We can only validate the initial environment, not regional environments
-                await ValidateAuthorityAsync(_initialAuthority).ConfigureAwait(false);
+                if (_initialAuthority.AuthorityInfo.ValidateAuthority &&
+                    _requestContext.ServiceBundle.Config.IsInstanceDiscoveryEnabled)
+                {
+                    await ValidateAuthorityAsync(_initialAuthority).ConfigureAwait(false);
+                }
 
                 _instanceDiscoveryAndValidationExecuted = true;
             }
@@ -77,7 +81,8 @@ private async Task ValidateAuthorityAsync(Authority authority)
         {
             // race conditions could occur here, where multiple requests validate the authority at the same time
             // but this is acceptable and once the cache is filled, no more HTTP requests will be made
-            if (!s_validatedEnvironments.Contains(authority.AuthorityInfo.Host))
+            if (
+                !s_validatedEnvironments.Contains(authority.AuthorityInfo.Host))
             {
                 // validate the original authority, as the resolved authority might be regionalized and we cannot validate regionalized authorities.
                 var validator = AuthorityInfoHelper.CreateAuthorityValidator(authority.AuthorityInfo, _requestContext);

src/client/Microsoft.Identity.Client/Instance/GenericAuthority.cs

@@ -14,7 +14,6 @@ internal class GenericAuthority : Authority
         internal GenericAuthority(AuthorityInfo authorityInfo)
             : base(authorityInfo)
         {
-
         }
 
         internal override string TenantId => null;
@@ -43,5 +42,11 @@ internal override Task<string> GetDeviceCodeEndpointAsync(RequestContext request
             // prevents device_code flow which requires knowledge of the device_authorization_endpoint.
             throw new NotImplementedException();
         }
+
+        internal override string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant)
+        {
+            // Assume generic authorities are not tenanted
+            return AuthorityInfo.CanonicalAuthority.ToString();
+        }
     }
 }

src/client/Microsoft.Identity.Client/Instance/Validation/AadAuthorityValidator.cs

@@ -32,7 +32,7 @@ public async Task ValidateAuthorityAsync(
             _requestContext.Logger.Info(() => $"Authority validation enabled? {authorityInfo.ValidateAuthority}. ");
             _requestContext.Logger.Info(() => $"Authority validation - is known env? {isKnownEnv}. ");
 
-            if (authorityInfo.ValidateAuthority && !isKnownEnv)
+            if (!isKnownEnv)
             {
                 _requestContext.Logger.Info("Authority validation is being performed. ");
 

src/client/Microsoft.Identity.Client/PublicClientApplication.cs

@@ -44,6 +44,9 @@ internal static bool IsOperatingSystemAccount(IAccount account)
             return string.Equals(account?.HomeAccountId?.Identifier, CurrentOSAccountDescriptor, StringComparison.Ordinal);
         }
 
+        /// <summary>
+        /// Returns true if the system web view can be used.
+        /// </summary>
         public bool IsSystemWebViewAvailable // TODO MSAL5: consolidate these helpers in the interface
         {
             get
@@ -96,7 +99,8 @@ public bool IsBrokerAvailable()
                     .IsBrokerInstalledAndInvokable(ServiceBundle.Config.Authority?.AuthorityInfo?.AuthorityType ?? AuthorityType.Aad);
         }
 
-        [CLSCompliant(false)]
+        /// <inheritdoc cref="IPublicClientApplication.AcquireTokenInteractive(IEnumerable{string})"/>
+        [CLSCompliant(false)]        
         public AcquireTokenInteractiveParameterBuilder AcquireTokenInteractive(
             IEnumerable<string> scopes)
         {

tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManagerExtensions.cs

@@ -247,6 +247,8 @@ public static void AddAdfs2019MockHandler(this MockHttpManager httpManager)
             });
         }
 
+
+
         public static MockHttpMessageHandler AddAllMocks(this MockHttpManager httpManager, TokenResponseType aadResponse)
         {
             httpManager.AddInstanceDiscoveryMockHandler();

tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs

@@ -49,48 +49,38 @@ public override void TestCleanup()
             base.TestCleanup();
         }
 
-        [TestMethod]      
-        public void WithTenantId_Adfs_Exception()
+        [DataRow(TestConstants.ADFSAuthority)]
+        [DataRow(TestConstants.GenericAuthority)]
+        public void WithTenantIdAtRequestLevel_Noop_AdfsGeneric(string inputAuthority)
         {
-            var app1 = ConfidentialClientApplicationBuilder
-                .Create(TestConstants.ClientId)
-                .WithAuthority(TestConstants.ADFSAuthority)
-                .WithClientSecret("secret")
-                .Build();
-
-            var ex1 = AssertException.Throws<MsalClientException>(() =>
-                app1
-                    .AcquireTokenByAuthorizationCode(TestConstants.s_scope, "code")
-                    .WithTenantId(TestConstants.TenantId));
-
-            Assert.AreEqual(ex1.ErrorCode, MsalError.TenantOverrideNonAad);
-        }
-
+            var app = ConfidentialClientApplicationBuilder
+               .Create(TestConstants.ClientId)
+               .WithAuthority(inputAuthority)
+               .WithClientSecret("secret")
+               .Build();
 
-        [TestMethod]
-        public void GenericAuthority_WithTenantId_Exceptions()
-        {
-            var app1 = ConfidentialClientApplicationBuilder
-                .Create(TestConstants.ClientId)
-                .WithExperimentalFeatures(true)
-                .WithGenericAuthority(TestConstants.GenericAuthority)
-                .WithClientSecret("secret")
-                .Build();
+            var parameterBuilder = app
+                .AcquireTokenByAuthorizationCode(TestConstants.s_scope, "code")
+                .WithTenantId(TestConstants.TenantId2);
 
-            var ex1 = AssertException.Throws<MsalClientException>(() =>
-                app1
-                    .AcquireTokenByAuthorizationCode(TestConstants.s_scope, "code")
-                    .WithTenantId(TestConstants.TenantId));
+            Assert.AreEqual(
+                new Uri(inputAuthority).Host,
+                parameterBuilder.CommonParameters.AuthorityOverride.Host,
+                "The host should have stayed the same");
 
-            Assert.AreEqual(ex1.ErrorCode, MsalError.TenantOverrideNonAad);
+            Assert.AreEqual(
+                TestConstants.TenantId2,
+                AuthorityHelpers.GetTenantId(parameterBuilder.CommonParameters.AuthorityOverride.CanonicalAuthority),
+                "The tenant id should have been changed");
         }
 
+
         [DataTestMethod]
         [DataRow(TestConstants.DstsAuthorityCommon)]
         [DataRow(TestConstants.DstsAuthorityTenanted)]
         [DataRow(TestConstants.CiamAuthorityMainFormat)]
         [DataRow(TestConstants.CiamAuthorityWithFriendlyName)]
-        [DataRow(TestConstants.CiamAuthorityWithGuid)]
+        [DataRow(TestConstants.CiamAuthorityWithGuid)]        
         public void WithTenantIdAtRequestLevel_NonAad(string inputAuthority)
         {
             var app = ConfidentialClientApplicationBuilder

tests/Microsoft.Identity.Test.Unit/PublicApiTests/AccountTests.cs

@@ -20,6 +20,7 @@
 
 namespace Microsoft.Identity.Test.Unit.PublicApiTests
 {
+
     [TestClass]
     public class AccountTests
     {

tests/Microsoft.Identity.Test.Unit/PublicApiTests/AdfsAcceptanceTests.cs

@@ -0,0 +1,107 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.Identity.Client;
+using Microsoft.Identity.Test.Common.Core.Mocks;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Microsoft.Identity.Test.Unit.PublicApiTests
+{
+    [TestClass]
+    public class AdfsAcceptanceTests : TestBase
+    {
+        // Possible authorities copied from: https://msazure.visualstudio.com/One/_search?action=contents&text=CanAcquireToken_UsingRefreshToken&type=code&lp=code-Project&filters=ProjectFilters%7BOne%7DRepositoryFilters%7BAzureStack-Services-Graph%7D&pageSize=25&result=DefaultCollection/One/AzureStack-Services-Graph/GBmain//src/Identity.Web.Tests/MsalTests.cs
+        [DataTestMethod]
+        [DataRow("https://localhost:3001/adfs")]
+        [DataRow("https://localhost:3001/460afc9d-718d-40c8-8d03-954")]
+        [DataRow("https://localhost:3001/contoso.int.test")]
+        public async Task AdfsAuthorityVariants_WithAdfsAuthority_Async(string authority)
+        {
+            await RunAuthCodeFlowAsync(authority, useWithAdfsAuthority: true).ConfigureAwait(false);
+        }
+
+        [DataTestMethod]
+        [DataRow("https://localhost:3001/adfs")]
+        [DataRow("https://localhost:3001/460afc9d-718d-40c8-8d03-954")]
+        [DataRow("https://localhost:3001/contoso.int.test")]
+        public async Task AdfsAuthorityVariants_WithAuthority_Async(string authority)
+        {
+            await RunAuthCodeFlowAsync(authority, useWithAdfsAuthority: false).ConfigureAwait(false);
+        }
+
+        private static async Task RunAuthCodeFlowAsync(string authority, bool useWithAdfsAuthority)
+        {
+            using (var httpManager = new MockHttpManager())
+            {
+                // specific client id used by the id token in AddAdfsWithTenantIdMockHandler
+                var builder = ConfidentialClientApplicationBuilder
+                    .Create("e68c40a5-a8e5-4250-bbea-5b43ab18cf0d");
+
+                builder = useWithAdfsAuthority ?
+                    builder.WithAdfsAuthority(authority, false) :
+                    builder.WithAuthority(authority, false);
+
+                var app = builder
+                    .WithRedirectUri("http://localhost")
+                    .WithHttpManager(httpManager)
+                    .WithInstanceDiscovery(false)
+                    .WithClientSecret(TestConstants.ClientSecret)
+                    .Build();
+
+                AddAdfsWithTenantIdMockHandler(httpManager);
+
+                var result = await app.AcquireTokenByAuthorizationCode(new[] { "https://arm.asz/.default" }, "authcode")
+                    .ExecuteAsync()
+                    .ConfigureAwait(false);
+
+                var account = await app.GetAccountAsync(result.Account.HomeAccountId.Identifier).ConfigureAwait(false);
+
+                AssertAdfsResult(result, account);
+
+                var result2 = await app.AcquireTokenSilent(new[] { "https://arm.asz/.default" }, account)
+                    .ExecuteAsync()
+                    .ConfigureAwait(false);
+
+                var account2 = await app.GetAccountAsync(result.Account.HomeAccountId.Identifier).ConfigureAwait(false);
+                AssertAdfsResult(result2, account2);
+                Assert.AreEqual(TokenSource.Cache, result2.AuthenticationResultMetadata.TokenSource);
+            }
+        }
+
+        private static void AssertAdfsResult(AuthenticationResult result, IAccount account)
+        {
+            Assert.AreEqual("460afc9d-718d-40c8-8d03-9540fa56cc2c", result.TenantId);
+            Assert.AreEqual("460afc9d-718d-40c8-8d03-9540fa56cc2c", result.ClaimsPrincipal.FindFirst("tid").Value);
+
+            Assert.AreEqual("localhost", account.Environment);
+            Assert.AreEqual("[email protected]", account.Username);
+            Assert.AreEqual("FTiFcJ97JrNoywo4SSdQjA", account.HomeAccountId.Identifier);
+            Assert.AreEqual("FTiFcJ97JrNoywo4SSdQjA", account.HomeAccountId.ObjectId);
+            Assert.IsNull(account.HomeAccountId.TenantId);            
+        }
+
+        /// <summary>
+        /// Token response where  tid claim is present in the Id Token. Can occurs as a result of token transformation rules.
+        /// </summary>
+        /// <param name="httpManager"></param>
+        internal static void AddAdfsWithTenantIdMockHandler(MockHttpManager httpManager)
+        {
+            string resp = @"{
+""access_token"":""secret"",
+""id_token"":""eyJhbGciOiJSUzI1NiIsImtpZCI6IjMzRTFFQUNEMzRDOEEyM0NGOEE5N0ZDMDM4N0U1Rjk0MzZFMEUyODIiLCJ4NXQiOiJNLUhxelRUSW9qejRxWF9BT0g1ZmxEYmc0b0kiLCJ0eXAiOiJKV1QifQ.eyJ2ZXIiOiIxLjAiLCJpYXQiOjE2OTAyNTQwNjMsImF1ZCI6ImU2OGM0MGE1LWE4ZTUtNDI1MC1iYmVhLTViNDNhYjE4Y2YwZCIsImlzcyI6Imh0dHBzOi8vbG9jYWxob3N0OjMwMDEvYWRmcy80NjBhZmM5ZC03MThkLTQwYzgtOGQwMy05NTQwZmE1NmNjMmMvIiwibmFtZSI6ImFkbWluQGRlbW8uYXN6Iiwib2lkIjoiMDAwMDAwMDAtMDAwMC0wMDAwLTAwMDAtODY5OThjZmIzNGY0Iiwic3ViIjoiRlRpRmNKOTdKck5veXdvNFNTZFFqQSIsInRpZCI6IjQ2MGFmYzlkLTcxOGQtNDBjOC04ZDAzLTk1NDBmYTU2Y2MyYyIsInVuaXF1ZV9uYW1lIjoiYWRtaW5AZGVtby5hc3oiLCJ1dGkiOiIxM2I2MDUzMy1hNDMxLTQyNmUtOTIyYi0wNzFiYTA2MWU3NGMiLCJ1cG4iOiJhZG1pbkBkZW1vLmFzeiIsImV4cCI6MTY5MDI1OTQ2MywibmJmIjoxNjkwMjU0MDYzfQ.Bqo2oF4YI40RbT5N4IFYcXriqDAKi6cwcmKYoCNTq3CE4RGdq2RJ_OJvrC6pSU9gIEOlyA2VYgedmeZrUoTzSuZXfV-UA5qvm9xPfYASGqp0TDaUYYgsUpCeD1w4On2g_MueGV-ZhAcgbW3tN3QIenYgqf7tq6MxOBgsIlc1DPiiyatBVGEK6NGMRVrZqw64IX3FKtsrdFjzTCn_k2QSzBAVK7DB9nIi-9GKcG33hRlMWFeqNZ_1sI1ReYyTtTcep3vytcRSxQ4cvsKVWRdlM9DCOZvMMgDuKKxrbOo70VzxRmw9xNR2FZQ618-0EmGTrpXcEH1HcWhlXxt-GBJLuw"",
+""refresh_token"":""secret"",
+""resource"":""https://arm.asz"",
+""scope"":""https://arm.asz/.default offline_access openid profile"",
+""token_type"":""bearer"",
+""expires_in"":4260}";
+
+            httpManager.AddMockHandler(new MockHttpMessageHandler
+            {
+                ExpectedMethod = HttpMethod.Post,
+                ResponseMessage = MockHelpers.CreateSuccessResponseMessage(resp)
+            });
+        }
+    }
+}