From f2498bcb1c712a1d3a849c649a0fd552d8d883cd Mon Sep 17 00:00:00 2001 From: jennyf19 Date: Mon, 22 May 2023 12:09:03 -0700 Subject: [PATCH] =?UTF-8?q?[Suggestion]=20Fix=20two=20issues=20with=20dSTS?= =?UTF-8?q?=20authority,=20one=20when=20using=20WithTenantId,=20and=20?= =?UTF-8?q?=E2=80=A6=20(#4146)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix two issues with dSTS authority, one when using WithTenantId, and adding dSTS as a supported tenant override. Add a test * Update fix * fix tests --------- Co-authored-by: Bogdan Gavril Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com> --- .../AbstractAcquireTokenParameterBuilder.cs | 10 ++-- .../AppConfig/AuthorityInfo.cs | 29 ++++++---- .../TestConstants.cs | 4 +- .../TestData.cs | 2 +- .../ApiConfigTests/AuthorityTests.cs | 2 +- .../InstanceTests/DstsAuthorityTests.cs | 58 ++++++++++++++++--- .../PublicApiTests/TenantIdTests.cs | 2 +- 7 files changed, 79 insertions(+), 28 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs b/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs index a74fbbe746..3bc0e53baf 100644 --- a/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs +++ b/src/client/Microsoft.Identity.Client/ApiConfig/AbstractAcquireTokenParameterBuilder.cs @@ -274,11 +274,13 @@ public T WithTenantId(string tenantId) MsalErrorMessage.TenantOverrideNonAad); } - AadAuthority aadAuthority = (AadAuthority)ServiceBundle.Config.Authority; - string tenantedAuthority = aadAuthority.GetTenantedAuthority(tenantId, true); - var newAuthorityInfo = AuthorityInfo.FromAadAuthority( + Authority originalAuthority = ServiceBundle.Config.Authority; + string tenantedAuthority = originalAuthority.GetTenantedAuthority(tenantId, true); + + var newAuthorityInfo = new AuthorityInfo( + originalAuthority.AuthorityInfo.AuthorityType, tenantedAuthority, - ServiceBundle.Config.Authority.AuthorityInfo.ValidateAuthority); + originalAuthority.AuthorityInfo.ValidateAuthority); CommonParameters.AuthorityOverride = newAuthorityInfo; diff --git a/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs b/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs index 1b5743fc77..d952049585 100644 --- a/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs +++ b/src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs @@ -136,11 +136,25 @@ private AuthorityInfo( internal bool IsInstanceDiscoverySupported => AuthorityType == AuthorityType.Aad; - internal bool IsUserAssertionSupported => AuthorityType != AuthorityType.Adfs && AuthorityType != AuthorityType.B2C; - - internal bool IsTenantOverrideSupported => AuthorityType == AuthorityType.Aad; - internal bool IsMultiTenantSupported => AuthorityType != AuthorityType.Adfs; - internal bool IsClientInfoSupported => AuthorityType == AuthorityType.Aad || AuthorityType == AuthorityType.Dsts || AuthorityType == AuthorityType.B2C; + internal bool IsUserAssertionSupported => + AuthorityType != AuthorityType.Adfs && + AuthorityType != AuthorityType.B2C; + + internal bool IsTenantOverrideSupported => + AuthorityType == AuthorityType.Aad || + AuthorityType == AuthorityType.Dsts; + + internal bool IsMultiTenantSupported => + AuthorityType == AuthorityType.Aad || + AuthorityType == AuthorityType.Dsts || + AuthorityType == AuthorityType.B2C || + AuthorityType == AuthorityType.Ciam; + + internal bool IsClientInfoSupported => + AuthorityType == AuthorityType.Aad || + AuthorityType == AuthorityType.Dsts || + AuthorityType == AuthorityType.B2C || + AuthorityType == AuthorityType.Ciam; #region Builders internal static AuthorityInfo FromAuthorityUri(string authorityUri, bool validateAuthority) @@ -228,11 +242,6 @@ internal static AuthorityInfo FromAadAuthority(AadAuthorityAudience authorityAud return new AuthorityInfo(AuthorityType.Aad, authorityUri, validateAuthority); } - internal static AuthorityInfo FromAadAuthority(string authorityUri, bool validateAuthority) - { - return new AuthorityInfo(AuthorityType.Aad, authorityUri, validateAuthority); - } - internal static AuthorityInfo FromAdfsAuthority(string authorityUri, bool validateAuthority) { return new AuthorityInfo(AuthorityType.Adfs, authorityUri, validateAuthority); diff --git a/tests/Microsoft.Identity.Test.Common/TestConstants.cs b/tests/Microsoft.Identity.Test.Common/TestConstants.cs index 7076fa214a..94bc8e8348 100644 --- a/tests/Microsoft.Identity.Test.Common/TestConstants.cs +++ b/tests/Microsoft.Identity.Test.Common/TestConstants.cs @@ -104,8 +104,8 @@ public static HashSet s_scope public const string ADFSAuthority2 = "https://someAdfs.com/adfs/"; public const string DstsAuthorityTenantless = "https://some.url.dsts.core.azure-test.net/dstsv2/"; - public const string DstsAuthorityTenanted = "https://some.url.dsts.core.azure-test.net/dstsv2/" + TenantIdString; - public const string DstsAuthorityCommon = "https://some.url.dsts.core.azure-test.net/dstsv2/" + Common; + public const string DstsAuthorityTenanted = "https://some.url.dsts.core.azure-test.net/dstsv2/" + TenantId + "/"; + public const string DstsAuthorityCommon = "https://some.url.dsts.core.azure-test.net/dstsv2/" + Common + "/"; public const string B2CLoginGlobal = ".b2clogin.com"; public const string B2CLoginUSGov = ".b2clogin.us"; diff --git a/tests/Microsoft.Identity.Test.Common/TestData.cs b/tests/Microsoft.Identity.Test.Common/TestData.cs index 83725395e8..3405df494c 100644 --- a/tests/Microsoft.Identity.Test.Common/TestData.cs +++ b/tests/Microsoft.Identity.Test.Common/TestData.cs @@ -37,7 +37,7 @@ public static IEnumerable GetAuthorityWithExpectedTenantId() yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.AuthorityTestTenant), ExpectedTenantId = TestConstants.Utid }.ToObjectArray(); yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.AadAuthorityWithTestTenantId), ExpectedTenantId = TestConstants.AadTenantId }.ToObjectArray(); yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.AuthorityWindowsNet), ExpectedTenantId = TestConstants.Utid }.ToObjectArray(); - yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.DstsAuthorityTenanted), ExpectedTenantId = TestConstants.TenantIdString }.ToObjectArray(); + yield return new AuthorityWithExpectedTenantId { Authority = new Uri(TestConstants.DstsAuthorityTenanted), ExpectedTenantId = TestConstants.TenantId }.ToObjectArray(); } } } diff --git a/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs index 645b3ff02b..c80146d490 100644 --- a/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs @@ -11,7 +11,6 @@ using Microsoft.Identity.Test.Common.Core.Helpers; using Microsoft.Identity.Test.Common.Core.Mocks; using Microsoft.VisualStudio.TestTools.UnitTesting; -using NSubstitute.ReceivedExtensions; namespace Microsoft.Identity.Test.Unit.ApiConfigTests { @@ -77,6 +76,7 @@ public void WithTenantIdExceptions() Assert.AreEqual(ex1.ErrorCode, MsalError.TenantOverrideNonAad); Assert.AreEqual(ex2.ErrorCode, MsalError.TenantOverrideNonAad); } + [DataTestMethod] [DynamicData(nameof(TestData.GetAuthorityWithExpectedTenantId), typeof(TestData), DynamicDataSourceType.Method)] diff --git a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/DstsAuthorityTests.cs b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/DstsAuthorityTests.cs index 7c091c1ab7..3dcc67a812 100644 --- a/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/DstsAuthorityTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/CoreTests/InstanceTests/DstsAuthorityTests.cs @@ -12,6 +12,7 @@ using Microsoft.Identity.Test.Common; using Microsoft.Identity.Test.Common.Core.Mocks; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Microsoft.Identity.Client.Utils; namespace Microsoft.Identity.Test.Unit.CoreTests.InstanceTests { @@ -36,7 +37,7 @@ private static MockHttpMessageHandler CreateTokenResponseHttpHandler(string auth return new MockHttpMessageHandler() { - ExpectedUrl = $"{authority}/oauth2/v2.0/token", + ExpectedUrl = $"{authority}oauth2/v2.0/token", ExpectedMethod = HttpMethod.Post, ExpectedPostData = expectedRequestBody, ResponseMessage = MockHelpers.CreateSuccessfulClientCredentialTokenResponseMessage(MockHelpers.CreateClientInfo(TestConstants.Uid, TestConstants.Utid)) @@ -57,7 +58,7 @@ public async Task DstsClientCredentialSuccessfulTestAsync(string authority) .WithClientSecret(TestConstants.ClientSecret) .Build(); - Assert.AreEqual(authority + "/", app.Authority); + Assert.AreEqual(authority, app.Authority); var confidentailClientApp = (ConfidentialClientApplication)app; Assert.AreEqual(AuthorityType.Dsts, confidentailClientApp.AuthorityInfo.AuthorityType); @@ -83,6 +84,46 @@ public async Task DstsClientCredentialSuccessfulTestAsync(string authority) } } + [TestMethod] + public void DstsAuthorityFlags() + { + var app = ConfidentialClientApplicationBuilder + .Create(TestConstants.ClientId) + .WithAuthority(TestConstants.DstsAuthorityTenanted) + .WithClientSecret("secret") + .Build(); + + Assert.AreEqual(AuthorityType.Dsts, (app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.AuthorityType); + + Assert.IsTrue((app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.IsMultiTenantSupported); + Assert.IsTrue((app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.IsClientInfoSupported); + Assert.IsFalse((app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.IsInstanceDiscoverySupported); + Assert.IsTrue((app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.IsTenantOverrideSupported); + Assert.IsTrue((app.AppConfig as ApplicationConfiguration).Authority.AuthorityInfo.IsUserAssertionSupported); + } + + [TestMethod] + public void DstsAuthority_WithTenantId_Success() + { + var app = ConfidentialClientApplicationBuilder + .Create(TestConstants.ClientId) + .WithAuthority(TestConstants.DstsAuthorityTenanted) + .WithClientSecret("secret") + .Build(); + + Assert.AreEqual(TestConstants.DstsAuthorityTenanted, app.Authority); + + // change the tenant id + var parameterBuilder = app.AcquireTokenByAuthorizationCode(TestConstants.s_scope, "code") + .WithTenantId(TestConstants.TenantId2); + + // Verify Host still matches the original Authority + Assert.AreEqual(new Uri(TestConstants.DstsAuthorityTenanted).Host, parameterBuilder.CommonParameters.AuthorityOverride.Host); + + // Verify the Tenant Id matches + Assert.AreEqual(TestConstants.TenantId2, AuthorityHelpers.GetTenantId(parameterBuilder.CommonParameters.AuthorityOverride.CanonicalAuthority)); + } + [DataTestMethod] [DataRow(TestConstants.DstsAuthorityCommon)] [DataRow(TestConstants.DstsAuthorityTenanted)] @@ -94,9 +135,9 @@ public void DstsEndpointsTest(string authority) _harness.ServiceBundle, Guid.NewGuid()); - Assert.AreEqual($"{authority}/oauth2/v2.0/token", instance.GetTokenEndpointAsync(_testRequestContext).Result); - Assert.AreEqual($"{authority}/oauth2/v2.0/authorize", instance.GetAuthorizationEndpointAsync(_testRequestContext).Result); - Assert.AreEqual($"{authority}/oauth2/v2.0/devicecode", instance.GetDeviceCodeEndpointAsync(_testRequestContext).Result); + Assert.AreEqual($"{authority}oauth2/v2.0/token", instance.GetTokenEndpointAsync(_testRequestContext).Result); + Assert.AreEqual($"{authority}oauth2/v2.0/authorize", instance.GetAuthorizationEndpointAsync(_testRequestContext).Result); + Assert.AreEqual($"{authority}oauth2/v2.0/devicecode", instance.GetDeviceCodeEndpointAsync(_testRequestContext).Result); Assert.AreEqual($"https://some.url.dsts.core.azure-test.net/dstsv2/common/userrealm/", instance.AuthorityInfo.UserRealmUriPrefix); } @@ -118,16 +159,15 @@ public void Validate_MinNumberOfSegments() [TestMethod] public void CreateAuthorityFromTenantedWithTenantTest() - { - + { Authority authority = AuthorityTestHelper.CreateAuthorityFromUrl(TestConstants.DstsAuthorityTenanted); - Assert.AreEqual("tenantid", authority.TenantId); + Assert.AreEqual(TestConstants.TenantId, authority.TenantId); string updatedAuthority = authority.GetTenantedAuthority("tenant2"); Assert.AreEqual( TestConstants.DstsAuthorityTenanted, - updatedAuthority.TrimEnd('/'), + updatedAuthority, "Not changed, original authority already has tenant id"); string updatedAuthority2 = authority.GetTenantedAuthority("tenant2", true); diff --git a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/TenantIdTests.cs b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/TenantIdTests.cs index fdc8a1fe27..6409baff3e 100644 --- a/tests/Microsoft.Identity.Test.Unit/PublicApiTests/TenantIdTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/PublicApiTests/TenantIdTests.cs @@ -38,7 +38,7 @@ public void TestInitialize() [DataRow(TestConstants.B2CLoginAuthorityMoonCake, TestConstants.SomeTenantId, DisplayName = "B2C MoonCake Tenant Id")] [DataRow(TestConstants.AuthoritySovereignCNTenant, TestConstants.TenantId, DisplayName = "Sovereign Tenant Id")] [DataRow(TestConstants.AuthoritySovereignDETenant, TestConstants.TenantId, DisplayName = "Sovereign Tenant Id")] - [DataRow(TestConstants.DstsAuthorityTenanted, "tenantid", DisplayName = "DSTS Tenant Id")] + [DataRow(TestConstants.DstsAuthorityTenanted, TestConstants.TenantId, DisplayName = "DSTS Tenant Id")] [DataRow(TestConstants.DstsAuthorityCommon, TestConstants.Common, DisplayName = "DSTS Common Tenant Id")] public void ParseTest_Success(string authorityUrl, string expectedTenantId) {