From b3d8ce5eb09fea92d4f20b393f55dddf0cc4cbf0 Mon Sep 17 00:00:00 2001 From: Robert <36173263+kurtanr@users.noreply.github.com> Date: Thu, 9 May 2024 19:48:56 +0200 Subject: [PATCH] PublicClientApplicationBuilder - allow non-GUID client id for AuthorityType.Generic (#4748) * PublicClientApplicationBuilder - allow non-GUID client id for AuthorityType.Generic * Completely removed ClientIdMustBeAGuid validation + related test and error messages. * Moved generic authority interactive test from GenericAuthorityTests to InteractiveFlowTests. --- .../PublicClientApplicationBuilder.cs | 6 --- .../Microsoft.Identity.Client/MsalError.cs | 6 --- .../MsalErrorMessage.cs | 1 - .../InteractiveFlowTests.NetFwk.cs | 44 +++++++++++++++++++ .../PublicClientApplicationBuilderTests.cs | 24 ---------- 5 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs b/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs index b35eb548f2..2932e323e5 100644 --- a/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs +++ b/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs @@ -377,12 +377,6 @@ internal override void Validate() { base.Validate(); - //ADFS does not require client id to be in the form of a GUID. - if (Config.Authority.AuthorityInfo?.AuthorityType != AuthorityType.Adfs && !Guid.TryParse(Config.ClientId, out _)) - { - throw new MsalClientException(MsalError.ClientIdMustBeAGuid, MsalErrorMessage.ClientIdMustBeAGuid); - } - #if __MOBILE__ if (Config.IsBrokerEnabled && Config.MultiCloudSupportEnabled) { diff --git a/src/client/Microsoft.Identity.Client/MsalError.cs b/src/client/Microsoft.Identity.Client/MsalError.cs index 571c1248cf..3ae5e404ed 100644 --- a/src/client/Microsoft.Identity.Client/MsalError.cs +++ b/src/client/Microsoft.Identity.Client/MsalError.cs @@ -763,12 +763,6 @@ public static class MsalError /// public const string NoClientId = "no_client_id"; - /// - /// What happens?You've specified a client ID that is not a - /// MitigationUse the application ID (a GUID) from the application portal as client ID in this SDK - /// - public const string ClientIdMustBeAGuid = "client_id_must_be_guid"; - /// /// What happens?You have configured both a telemetry callback and a telemetry config. /// MitigationOnly one telemetry mechanism can be configured. diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index ab16a63a11..ab7477b920 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -245,7 +245,6 @@ public static string iOSBrokerKeySaveFailed(string keyChainResult) public const string ClientCredentialAuthenticationTypesAreMutuallyExclusive = "ClientSecret, Certificate and ClientAssertion are mutually exclusive properties. Only specify one. See https://aka.ms/msal-net-client-credentials. "; public const string ClientCredentialAuthenticationTypeMustBeDefined = "One client credential type required either: ClientSecret, Certificate, ClientAssertion or AppTokenProvider must be defined when creating a Confidential Client. Only specify one. See https://aka.ms/msal-net-client-credentials. "; - public const string ClientIdMustBeAGuid = "Error: ClientId is not a GUID. "; public static string InvalidRedirectUriReceived(string invalidRedirectUri) { diff --git a/tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs b/tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs index 5c03fef1d7..7dafe73b05 100644 --- a/tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs +++ b/tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs @@ -189,6 +189,36 @@ public async Task ValidateCcsHeadersForInteractiveAuthCodeFlowAsync() Assert.IsNotNull(authResult.AccessToken); } + /// Based on the publicly available https://demo.duendesoftware.com/ + [RunOn(TargetFrameworks.NetCore)] + public async Task Interactive_GenericAuthority_DuendeDemoInstanceAsync() + { + string[] scopes = new[] { "openid profile email api offline_access" }; + const string username = "bob", password = "bob"; + const string demoDuendeSoftwareDotCom = "https://demo.duendesoftware.com"; + + var pca = PublicClientApplicationBuilder + .Create("interactive.public") + .WithRedirectUri(SeleniumWebUI.FindFreeLocalhostRedirectUri()) + .WithTestLogging() + .WithExperimentalFeatures() + .WithOidcAuthority(demoDuendeSoftwareDotCom) + .Build(); + + AuthenticationResult authResult = await pca + .AcquireTokenInteractive(scopes) + .WithCustomWebUi(CreateSeleniumCustomWebUIForDuende(username, password)) + .ExecuteAsync(new CancellationTokenSource(_interactiveAuthTimeout).Token) + .ConfigureAwait(false); + + Assert.IsNotNull(authResult); + Assert.IsNotNull(authResult.Scopes); + Assert.IsNotNull(authResult.AccessToken); + Assert.IsNotNull(authResult.IdToken); + Assert.AreEqual(5, authResult.Scopes.Count()); + Assert.AreEqual("Bearer", authResult.TokenType); + } + private async Task RunTestForUserAsync(LabResponse labResponse, bool directToAdfs = false) { HttpSnifferClientFactory factory = null; @@ -353,6 +383,20 @@ private SeleniumWebUI CreateSeleniumCustomWebUI(LabUser user, Prompt prompt, boo }, TestContext); } + private SeleniumWebUI CreateSeleniumCustomWebUIForDuende(string username, string password) + { + return new SeleniumWebUI((driver) => + { + Trace.WriteLine("Starting Selenium automation"); + + driver.FindElementById("Input_Username").SendKeys(username); + driver.FindElementById("Input_Password").SendKeys(password); + + var loginBtn = driver.WaitForElementToBeVisibleAndEnabled(OpenQA.Selenium.By.Name("Input.Button")); + loginBtn?.Click(); + }, TestContext); + } + #region Azure AD Kerberos Feature Tests [IgnoreOnOneBranch] public async Task Kerberos_Interactive_AADAsync() diff --git a/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs b/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs index 74b66b31bc..466b3c5477 100644 --- a/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/AppConfigTests/PublicClientApplicationBuilderTests.cs @@ -50,30 +50,6 @@ public void TestWithDifferentClientId() Assert.AreEqual(ClientId, pca.AppConfig.ClientId); } - [TestMethod] - public void ClientIdMustBeAGuid() - { - var ex = AssertException.Throws( - () => PublicClientApplicationBuilder.Create("http://my.app") - .WithAuthority(TestConstants.AadAuthorityWithTestTenantId) - .Build()); - - Assert.AreEqual(MsalError.ClientIdMustBeAGuid, ex.ErrorCode); - - ex = AssertException.Throws( - () => PublicClientApplicationBuilder.Create("http://my.app") - .WithAuthority(TestConstants.B2CAuthority) - .Build()); - - Assert.AreEqual(MsalError.ClientIdMustBeAGuid, ex.ErrorCode); - - // ADFS does not have this constraint - PublicClientApplicationBuilder.Create("http://my.app") - .WithAuthority(new Uri(TestConstants.ADFSAuthority)) - .Build(); - - } - [TestMethod] public void TestConstructor_ClientIdOverride() {