From 800d531d2499edcb1ff9b937e9825af99de44b12 Mon Sep 17 00:00:00 2001 From: Gladwin Johnson Date: Fri, 9 Sep 2022 14:50:30 -0700 Subject: [PATCH 1/5] [Bug] Crashing Microsoft.Identity.Client.Broker - WithBrokerPreview() https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3654 --- .../RuntimeBroker.cs | 32 +++++++++++++++---- .../WamAdapters.cs | 32 ++++++++++++++++++- .../Microsoft.Identity.Client/MsalError.cs | 6 ++++ .../Broker/AndroidContentProviderBroker.cs | 2 +- .../Mocks/MsalMockHelpers.cs | 2 +- .../Throttling/ThrottlingAcceptanceTests.cs | 2 +- tests/devapps/WAM/NetCoreWinFormsWam/Form1.cs | 4 ++- tests/devapps/WAM/NetFrameworkWam/Form1.cs | 3 +- 8 files changed, 70 insertions(+), 13 deletions(-) diff --git a/src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs b/src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs index 4e8dd111ba..fd9ce29c5e 100644 --- a/src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs +++ b/src/client/Microsoft.Identity.Client.Broker/RuntimeBroker.cs @@ -109,7 +109,10 @@ private static void OnProcessExit(object sender, EventArgs e) if (authenticationRequestParameters?.Account?.HomeAccountId?.ObjectId != null) { - using (var authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (var authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { using (var readAccountResult = await s_lazyCore.Value.ReadAccountByIdAsync( authenticationRequestParameters.Account.HomeAccountId.ObjectId, @@ -161,7 +164,10 @@ private static void OnProcessExit(object sender, EventArgs e) var cancellationToken = authenticationRequestParameters.RequestContext.UserCancellationToken; Debug.Assert(s_lazyCore.Value != null, "Should not call this API if msal runtime init failed"); - using (var authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (var authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { //Login Hint string loginHint = authenticationRequestParameters.LoginHint ?? authenticationRequestParameters?.Account?.Username; @@ -193,7 +199,10 @@ private static void OnProcessExit(object sender, EventArgs e) _logger.Verbose("[WamBroker] Signing in with the default user account."); - using (var authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (var authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { using (NativeInterop.AuthResult result = await s_lazyCore.Value.SignInAsync( _parentHandle, @@ -230,7 +239,10 @@ private IntPtr GetParentWindow(CoreUIParent uiParent) _logger.Verbose("[WamBroker] Acquiring token silently."); - using (var authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (var authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { using (var readAccountResult = await s_lazyCore.Value.ReadAccountByIdAsync( acquireTokenSilentParameters.Account.HomeAccountId.ObjectId, @@ -274,7 +286,10 @@ private IntPtr GetParentWindow(CoreUIParent uiParent) _logger.Verbose("[WamBroker] Acquiring token silently for default account."); - using (var authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (var authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { using (NativeInterop.AuthResult result = await s_lazyCore.Value.SignInSilentlyAsync( authParams, @@ -300,7 +315,10 @@ private IntPtr GetParentWindow(CoreUIParent uiParent) _logger.Verbose("[WamBroker] Acquiring token with Username Password flow."); - using (AuthParameters authParams = WamAdapters.GetCommonAuthParameters(authenticationRequestParameters, _wamOptions.MsaPassthrough)) + using (AuthParameters authParams = WamAdapters.GetCommonAuthParameters( + authenticationRequestParameters, + _wamOptions.MsaPassthrough, + _logger)) { authParams.Properties["MSALRuntime_Username"] = acquireTokenByUsernamePasswordParameters.Username; authParams.Properties["MSALRuntime_Password"] = acquireTokenByUsernamePasswordParameters.Password; @@ -424,7 +442,7 @@ public bool IsBrokerInstalledAndInvokable(AuthorityType authorityType) return false; } - _logger.Verbose($"[WAM Broker] MsalRuntime init succesful."); + _logger.Verbose($"[WAM Broker] MsalRuntime init successful."); return true; } } diff --git a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs index 1d66c4b523..e42b714d82 100644 --- a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs +++ b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs @@ -130,10 +130,15 @@ private enum ResponseStatus /// /// /// + /// public static NativeInterop.AuthParameters GetCommonAuthParameters( AuthenticationRequestParameters authenticationRequestParameters, - bool isMsaPassthrough) + bool isMsaPassthrough, + ILoggerAdapter logger) { + logger.Info("[WamBroker] Validating Common Auth Parameters."); + ValidateAuthParams(authenticationRequestParameters, logger); + var authParams = new NativeInterop.AuthParameters (authenticationRequestParameters.AppConfig.ClientId, authenticationRequestParameters.Authority.AuthorityInfo.CanonicalAuthority.ToString()); @@ -171,6 +176,8 @@ private enum ResponseStatus AddPopParams(authenticationRequestParameters, authParams); + logger.Info("[WamBroker] Acquired Common Auth Parameters."); + return authParams; } @@ -281,5 +288,28 @@ private static string GetExpectedRedirectUri(string clientId) { return $"ms-appx-web://microsoft.aad.brokerplugin/{clientId}"; } + + /// + /// Validate common auth params + /// + /// + /// + /// + public static void ValidateAuthParams( + AuthenticationRequestParameters authenticationRequestParameters, + ILoggerAdapter logger) + { + //MSAL Runtime throws an ApiContractViolation Exception with Tag: 0x2039c1cb (InvalidArg) + //When no scopes are passed, this will check if user is passing scopes + if (!authenticationRequestParameters.HasScopes) + { + logger.Error($"[WamBroker] {MsalError.WamScopesRequired} " + + $"{MsalErrorMessage.ScopesRequired}"); + + throw new MsalClientException( + MsalError.WamScopesRequired, + MsalErrorMessage.ScopesRequired); + } + } } } diff --git a/src/client/Microsoft.Identity.Client/MsalError.cs b/src/client/Microsoft.Identity.Client/MsalError.cs index ab75591e7a..bc80557e07 100644 --- a/src/client/Microsoft.Identity.Client/MsalError.cs +++ b/src/client/Microsoft.Identity.Client/MsalError.cs @@ -1016,6 +1016,12 @@ public static class MsalError /// public const string WamPickerError = "wam_interactive_picker_error"; + /// + /// What happens?No scopes have been requested + /// MitigationAt least one scope must be specified for MSAL Runtime WAM + /// + public const string WamScopesRequired = "scopes_required_wam"; + /// /// What happens?The embedded browser cannot be started because a runtime component is missing. /// Mitigation"The embedded browser needs WebView2 runtime to be installed. An end user of the app can download and install the WebView2 runtime from https://go.microsoft.com/fwlink/p/?LinkId=2124703 and restart the app. diff --git a/src/client/Microsoft.Identity.Client/Platforms/Android/Broker/AndroidContentProviderBroker.cs b/src/client/Microsoft.Identity.Client/Platforms/Android/Broker/AndroidContentProviderBroker.cs index 78c2a5d1a6..70a4e7a7e5 100644 --- a/src/client/Microsoft.Identity.Client/Platforms/Android/Broker/AndroidContentProviderBroker.cs +++ b/src/client/Microsoft.Identity.Client/Platforms/Android/Broker/AndroidContentProviderBroker.cs @@ -340,7 +340,7 @@ private async Task PerformContentResolverOperationAsync(ContentResolverO if (resultBundle != null) { - _logger.Verbose($"[Android broker] Content resolver operation completed succesfully. Operation: {operation} URI: {contentResolverUri}"); + _logger.Verbose($"[Android broker] Content resolver operation completed successfully. Operation: {operation} URI: {contentResolverUri}"); } return resultBundle; diff --git a/tests/Microsoft.Identity.Test.Common/Mocks/MsalMockHelpers.cs b/tests/Microsoft.Identity.Test.Common/Mocks/MsalMockHelpers.cs index e7f834bef8..fd508d8169 100644 --- a/tests/Microsoft.Identity.Test.Common/Mocks/MsalMockHelpers.cs +++ b/tests/Microsoft.Identity.Test.Common/Mocks/MsalMockHelpers.cs @@ -35,7 +35,7 @@ internal static class MsalMockHelpers } /// - /// Configures a web ui that returns a succesfull result + /// Configures a web ui that returns a successfull result /// public static void ConfigureMockWebUI( this IServiceBundle serviceBundle, diff --git a/tests/Microsoft.Identity.Test.Unit/Throttling/ThrottlingAcceptanceTests.cs b/tests/Microsoft.Identity.Test.Unit/Throttling/ThrottlingAcceptanceTests.cs index 4faa428b49..d53060c58d 100644 --- a/tests/Microsoft.Identity.Test.Unit/Throttling/ThrottlingAcceptanceTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/Throttling/ThrottlingAcceptanceTests.cs @@ -458,7 +458,7 @@ await app.AcquireTokenSilent(TestConstants.s_scope, account).ExecuteAsync() await app.AcquireTokenSilent(singleScope, account).WithForceRefresh(true).ExecuteAsync() .ConfigureAwait(false); - // there will still be an entry here because the refresh token written by the first succesful ATS + // there will still be an entry here because the refresh token written by the first successful ATS // is different from the initial RT AssertThrottlingCacheEntryCount(throttlingManager, uiRequiredEntryCount: 1); } diff --git a/tests/devapps/WAM/NetCoreWinFormsWam/Form1.cs b/tests/devapps/WAM/NetCoreWinFormsWam/Form1.cs index 527da559e4..a1101da94e 100644 --- a/tests/devapps/WAM/NetCoreWinFormsWam/Form1.cs +++ b/tests/devapps/WAM/NetCoreWinFormsWam/Form1.cs @@ -273,9 +273,11 @@ private async Task RunAtsAsync(IPublicClientApplication pc private string[] GetScopes() { string[] result = null; + cbxScopes.Invoke((MethodInvoker)delegate { - result = cbxScopes.Text.Split(' '); + if(!string.IsNullOrWhiteSpace(cbxScopes.Text)) + result = cbxScopes.Text.Split(' '); }); return result; diff --git a/tests/devapps/WAM/NetFrameworkWam/Form1.cs b/tests/devapps/WAM/NetFrameworkWam/Form1.cs index 96f39ac1c9..2a093f3af4 100644 --- a/tests/devapps/WAM/NetFrameworkWam/Form1.cs +++ b/tests/devapps/WAM/NetFrameworkWam/Form1.cs @@ -237,7 +237,8 @@ private string[] GetScopes() string[] result = null; cbxScopes.Invoke((MethodInvoker)delegate { - result = cbxScopes.Text.Split(' '); + if (!string.IsNullOrWhiteSpace(cbxScopes.Text)) + result = cbxScopes.Text.Split(' '); }); return result; From 1df98758b18ed7d76ac5b2b00c9eede7283985fd Mon Sep 17 00:00:00 2001 From: Gladwin Johnson Date: Fri, 9 Sep 2022 16:36:18 -0700 Subject: [PATCH 2/5] WamScopesRequired --- .../Microsoft.Identity.Client.Broker/WamAdapters.cs | 8 +++++--- src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | 2 ++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs index e42b714d82..4094ed0363 100644 --- a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs +++ b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs @@ -299,16 +299,18 @@ private static string GetExpectedRedirectUri(string clientId) AuthenticationRequestParameters authenticationRequestParameters, ILoggerAdapter logger) { + string[] oidcScopes = { "offline_access", "openid", "profile" }; + //MSAL Runtime throws an ApiContractViolation Exception with Tag: 0x2039c1cb (InvalidArg) //When no scopes are passed, this will check if user is passing scopes - if (!authenticationRequestParameters.HasScopes) + if (!authenticationRequestParameters.HasScopes || authenticationRequestParameters.Scope.All(oidcScopes.Contains)) { logger.Error($"[WamBroker] {MsalError.WamScopesRequired} " + - $"{MsalErrorMessage.ScopesRequired}"); + $"{MsalErrorMessage.WamScopesRequired}"); throw new MsalClientException( MsalError.WamScopesRequired, - MsalErrorMessage.ScopesRequired); + MsalErrorMessage.WamScopesRequired); } } } diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index 24bdd82639..77bb03bdda 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -337,6 +337,8 @@ public static string RedirectUriMismatch(string expectedUri, string actualUri) "See https://aka.ms/msal-net-custom-instance-metadata for more details. "; public const string ScopesRequired = "At least one scope needs to be requested for this authentication flow. "; + public const string WamScopesRequired = "At least one resource based scope (For example, scope=User.Read or https://graph.microsoft.com/User.Read) " + + "is required to be passed in addition to any OpenID Connect scopes (openid, profile, and offline_access)"; public const string InvalidAdalCacheMultipleRTs = "The ADAL cache is invalid as it contains multiple refresh token entries for one user. Deleting invalid ADAL cache. "; public const string CryptoNet45 = From 7d81ba56f863d2af81b86118ae6d46d05eaec8fd Mon Sep 17 00:00:00 2001 From: Gladwin Johnson Date: Fri, 9 Sep 2022 16:37:12 -0700 Subject: [PATCH 3/5] mesaage updated --- src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index 77bb03bdda..f9bc5fd942 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -338,7 +338,7 @@ public static string RedirectUriMismatch(string expectedUri, string actualUri) public const string ScopesRequired = "At least one scope needs to be requested for this authentication flow. "; public const string WamScopesRequired = "At least one resource based scope (For example, scope=User.Read or https://graph.microsoft.com/User.Read) " + - "is required to be passed in addition to any OpenID Connect scopes (openid, profile, and offline_access)"; + "is required to be passed, in addition to any OpenID Connect scopes (openid, profile, and offline_access)"; public const string InvalidAdalCacheMultipleRTs = "The ADAL cache is invalid as it contains multiple refresh token entries for one user. Deleting invalid ADAL cache. "; public const string CryptoNet45 = From 02615342b9c48773203074232a6c58439293b658 Mon Sep 17 00:00:00 2001 From: Gladwin Johnson Date: Sat, 10 Sep 2022 08:57:03 -0700 Subject: [PATCH 4/5] verbose --- src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs index 4094ed0363..bd2dcaed3e 100644 --- a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs +++ b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs @@ -136,7 +136,7 @@ private enum ResponseStatus bool isMsaPassthrough, ILoggerAdapter logger) { - logger.Info("[WamBroker] Validating Common Auth Parameters."); + logger.Verbose("[WamBroker] Validating Common Auth Parameters."); ValidateAuthParams(authenticationRequestParameters, logger); var authParams = new NativeInterop.AuthParameters @@ -176,7 +176,7 @@ private enum ResponseStatus AddPopParams(authenticationRequestParameters, authParams); - logger.Info("[WamBroker] Acquired Common Auth Parameters."); + logger.Verbose("[WamBroker] Acquired Common Auth Parameters."); return authParams; } From bc656d131b069eefb4a76c2e91483ecab9a793a3 Mon Sep 17 00:00:00 2001 From: Gladwin Johnson Date: Mon, 12 Sep 2022 17:19:45 -0700 Subject: [PATCH 5/5] removing oidc scopes check --- .../Microsoft.Identity.Client.Broker/WamAdapters.cs | 8 +++----- src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | 2 -- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs index bd2dcaed3e..331083ca6c 100644 --- a/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs +++ b/src/client/Microsoft.Identity.Client.Broker/WamAdapters.cs @@ -299,18 +299,16 @@ private static string GetExpectedRedirectUri(string clientId) AuthenticationRequestParameters authenticationRequestParameters, ILoggerAdapter logger) { - string[] oidcScopes = { "offline_access", "openid", "profile" }; - //MSAL Runtime throws an ApiContractViolation Exception with Tag: 0x2039c1cb (InvalidArg) //When no scopes are passed, this will check if user is passing scopes - if (!authenticationRequestParameters.HasScopes || authenticationRequestParameters.Scope.All(oidcScopes.Contains)) + if (!authenticationRequestParameters.HasScopes) { logger.Error($"[WamBroker] {MsalError.WamScopesRequired} " + - $"{MsalErrorMessage.WamScopesRequired}"); + $"{MsalErrorMessage.ScopesRequired}"); throw new MsalClientException( MsalError.WamScopesRequired, - MsalErrorMessage.WamScopesRequired); + MsalErrorMessage.ScopesRequired); } } } diff --git a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs index f9bc5fd942..24bdd82639 100644 --- a/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs +++ b/src/client/Microsoft.Identity.Client/MsalErrorMessage.cs @@ -337,8 +337,6 @@ public static string RedirectUriMismatch(string expectedUri, string actualUri) "See https://aka.ms/msal-net-custom-instance-metadata for more details. "; public const string ScopesRequired = "At least one scope needs to be requested for this authentication flow. "; - public const string WamScopesRequired = "At least one resource based scope (For example, scope=User.Read or https://graph.microsoft.com/User.Read) " + - "is required to be passed, in addition to any OpenID Connect scopes (openid, profile, and offline_access)"; public const string InvalidAdalCacheMultipleRTs = "The ADAL cache is invalid as it contains multiple refresh token entries for one user. Deleting invalid ADAL cache. "; public const string CryptoNet45 =