From 01597486aebb5986a69c12d24899c3f595ffca5f Mon Sep 17 00:00:00 2001 From: Bogdan Gavril Date: Tue, 1 Aug 2023 15:03:58 +0100 Subject: [PATCH] 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 --- .../Instance/AdfsAuthority.cs | 13 +++ .../Instance/Authority.cs | 20 ++-- .../Instance/AuthorityManager.cs | 9 +- .../Instance/GenericAuthority.cs | 7 +- .../Validation/AadAuthorityValidator.cs | 2 +- .../PublicClientApplication.cs | 6 +- .../Core/Mocks/MockHttpManagerExtensions.cs | 2 + .../ApiConfigTests/AuthorityTests.cs | 52 ++++----- .../PublicApiTests/AccountTests.cs | 1 + .../PublicApiTests/AdfsAcceptanceTests.cs | 107 ++++++++++++++++++ 10 files changed, 174 insertions(+), 45 deletions(-) create mode 100644 tests/Microsoft.Identity.Test.Unit/PublicApiTests/AdfsAcceptanceTests.cs diff --git a/src/client/Microsoft.Identity.Client/Instance/AdfsAuthority.cs b/src/client/Microsoft.Identity.Client/Instance/AdfsAuthority.cs index 11c4ac14be..a96ba70949 100644 --- a/src/client/Microsoft.Identity.Client/Instance/AdfsAuthority.cs +++ b/src/client/Microsoft.Identity.Client/Instance/AdfsAuthority.cs @@ -48,6 +48,19 @@ internal override Task GetDeviceCodeEndpointAsync(RequestContext request return Task.FromResult(deviceEndpoint); } + /// + /// ADFS seems to support tenanted authorities, but the tenant ID is fixed so for all intents and purposes + /// it remains constant + /// + /// + /// + /// + /// + internal override string GetTenantedAuthority(string tenantId, bool forceSpecifiedTenant) + { + return AuthorityInfo.CanonicalAuthority.ToString(); + } + internal override string TenantId => null; } } diff --git a/src/client/Microsoft.Identity.Client/Instance/Authority.cs b/src/client/Microsoft.Identity.Client/Instance/Authority.cs index b0f88d088c..9385e97362 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Authority.cs +++ b/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 /// /// The new tenant id /// Forces the change, even if the current tenant is not "common" or "organizations" or "consumers" - 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 GetTokenEndpointAsync(RequestContext requestContext); internal abstract Task GetAuthorizationEndpointAsync(RequestContext requestContext); diff --git a/src/client/Microsoft.Identity.Client/Instance/AuthorityManager.cs b/src/client/Microsoft.Identity.Client/Instance/AuthorityManager.cs index f9cdd70dd9..6f5eedc09e 100644 --- a/src/client/Microsoft.Identity.Client/Instance/AuthorityManager.cs +++ b/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); diff --git a/src/client/Microsoft.Identity.Client/Instance/GenericAuthority.cs b/src/client/Microsoft.Identity.Client/Instance/GenericAuthority.cs index 6a73305de7..2278dcba4d 100644 --- a/src/client/Microsoft.Identity.Client/Instance/GenericAuthority.cs +++ b/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 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(); + } } } diff --git a/src/client/Microsoft.Identity.Client/Instance/Validation/AadAuthorityValidator.cs b/src/client/Microsoft.Identity.Client/Instance/Validation/AadAuthorityValidator.cs index f7777181d4..2b1ea2e2bd 100644 --- a/src/client/Microsoft.Identity.Client/Instance/Validation/AadAuthorityValidator.cs +++ b/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. "); diff --git a/src/client/Microsoft.Identity.Client/PublicClientApplication.cs b/src/client/Microsoft.Identity.Client/PublicClientApplication.cs index 3dc3c0b7e5..7b9427cf2d 100644 --- a/src/client/Microsoft.Identity.Client/PublicClientApplication.cs +++ b/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); } + /// + /// Returns true if the system web view can be used. + /// 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)] + /// + [CLSCompliant(false)] public AcquireTokenInteractiveParameterBuilder AcquireTokenInteractive( IEnumerable scopes) { diff --git a/tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManagerExtensions.cs b/tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManagerExtensions.cs index 9ba7ddc24c..7a7bd8b109 100644 --- a/tests/Microsoft.Identity.Test.Common/Core/Mocks/MockHttpManagerExtensions.cs +++ b/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(); diff --git a/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs index c100a9ba6d..851f1f2f7d 100644 --- a/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs +++ b/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(() => - 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(() => - 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 diff --git a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AccountTests.cs b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AccountTests.cs index 7a9d32a3e6..bed168ef65 100644 --- a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AccountTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AccountTests.cs @@ -20,6 +20,7 @@ namespace Microsoft.Identity.Test.Unit.PublicApiTests { + [TestClass] public class AccountTests { diff --git a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AdfsAcceptanceTests.cs b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/AdfsAcceptanceTests.cs new file mode 100644 index 0000000000..f45923f314 --- /dev/null +++ b/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("admin@demo.asz", account.Username); + Assert.AreEqual("FTiFcJ97JrNoywo4SSdQjA", account.HomeAccountId.Identifier); + Assert.AreEqual("FTiFcJ97JrNoywo4SSdQjA", account.HomeAccountId.ObjectId); + Assert.IsNull(account.HomeAccountId.TenantId); + } + + /// + /// Token response where tid claim is present in the Id Token. Can occurs as a result of token transformation rules. + /// + /// + 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) + }); + } + } +}