diff --git a/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs b/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs index f185cc1fb..f474cff82 100644 --- a/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs +++ b/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs @@ -389,8 +389,7 @@ private async Task GetAuthenticationResultForUserInternalA catch (MsalUiRequiredException ex) { // GetAccessTokenForUserAsync is an abstraction that can be called from a web app or a web API - Logger.TokenAcquisitionError(_logger, ex.Message, ex); - + // MsalUiRequiredException is already logged by MSAL. Re-logging here would produce duplicates. // Case of the web app: we let the MsalUiRequiredException be caught by the // AuthorizeForScopesAttribute exception filter so that the user can consent, do 2FA, etc ... throw new MicrosoftIdentityWebChallengeUserException(ex, scopes.ToArray(), userFlow); @@ -1273,171 +1272,160 @@ private void NotifyCertificateSelection( MergedOptions mergedOptions, ClaimsPrincipal? userHint) { - try - { - // In web API, validatedToken will not be null - SecurityToken? validatedToken = userHint?.GetBootstrapToken() ?? _tokenAcquisitionHost.GetTokenUsedToCallWebAPI(); + // In web API, validatedToken will not be null + SecurityToken? validatedToken = userHint?.GetBootstrapToken() ?? _tokenAcquisitionHost.GetTokenUsedToCallWebAPI(); - // In the case the token is a JWE (encrypted token), we use the decrypted token. - string? tokenUsedToCallTheWebApi = GetActualToken(validatedToken); - string? originalTokenToCallWebApi = tokenUsedToCallTheWebApi; + // In the case the token is a JWE (encrypted token), we use the decrypted token. + string? tokenUsedToCallTheWebApi = GetActualToken(validatedToken); + string? originalTokenToCallWebApi = tokenUsedToCallTheWebApi; - AcquireTokenOnBehalfOfParameterBuilder? builder = null; - TokenAcquisitionExtensionOptions? addInOptions = tokenAcquisitionExtensionOptionsMonitor?.CurrentValue; + AcquireTokenOnBehalfOfParameterBuilder? builder = null; + TokenAcquisitionExtensionOptions? addInOptions = tokenAcquisitionExtensionOptionsMonitor?.CurrentValue; - // Case of web APIs: we need to do an on-behalf-of flow, with the token used to call the API - if (tokenUsedToCallTheWebApi != null) + // Case of web APIs: we need to do an on-behalf-of flow, with the token used to call the API + if (tokenUsedToCallTheWebApi != null) + { + if (addInOptions != null && addInOptions.InvokeOnBeforeOnBehalfOfInitializedAsync != null) { - if (addInOptions != null && addInOptions.InvokeOnBeforeOnBehalfOfInitializedAsync != null) + var oboInitEventArgs = new OnBehalfOfEventArgs { - var oboInitEventArgs = new OnBehalfOfEventArgs - { - UserAssertionToken = tokenUsedToCallTheWebApi, - User = userHint - }; - await addInOptions.InvokeOnBeforeOnBehalfOfInitializedAsync(oboInitEventArgs).ConfigureAwait(false); - - if (oboInitEventArgs.UserAssertionToken != null) - { - tokenUsedToCallTheWebApi = oboInitEventArgs.UserAssertionToken; - } - } + UserAssertionToken = tokenUsedToCallTheWebApi, + User = userHint + }; + await addInOptions.InvokeOnBeforeOnBehalfOfInitializedAsync(oboInitEventArgs).ConfigureAwait(false); - if (string.IsNullOrEmpty(tokenAcquisitionOptions?.LongRunningWebApiSessionKey)) + if (oboInitEventArgs.UserAssertionToken != null) { - builder = application - .AcquireTokenOnBehalfOf( - scopes.Except(_scopesRequestedByMsal), - new UserAssertion(tokenUsedToCallTheWebApi)); + tokenUsedToCallTheWebApi = oboInitEventArgs.UserAssertionToken; } - else - { - string? sessionKey = tokenAcquisitionOptions!.LongRunningWebApiSessionKey; - if (sessionKey == Abstractions.AcquireTokenOptions.LongRunningWebApiSessionKeyAuto) - { - sessionKey = null; - } + } - builder = (application as ILongRunningWebApi)? - .InitiateLongRunningProcessInWebApi( - scopes.Except(_scopesRequestedByMsal), - tokenUsedToCallTheWebApi, - ref sessionKey); - tokenAcquisitionOptions.LongRunningWebApiSessionKey = sessionKey; - } + if (string.IsNullOrEmpty(tokenAcquisitionOptions?.LongRunningWebApiSessionKey)) + { + builder = application + .AcquireTokenOnBehalfOf( + scopes.Except(_scopesRequestedByMsal), + new UserAssertion(tokenUsedToCallTheWebApi)); } - else if (!string.IsNullOrEmpty(tokenAcquisitionOptions?.LongRunningWebApiSessionKey)) + else { - string sessionKey = tokenAcquisitionOptions!.LongRunningWebApiSessionKey!; + string? sessionKey = tokenAcquisitionOptions!.LongRunningWebApiSessionKey; + if (sessionKey == Abstractions.AcquireTokenOptions.LongRunningWebApiSessionKeyAuto) + { + sessionKey = null; + } + builder = (application as ILongRunningWebApi)? - .AcquireTokenInLongRunningProcess( + .InitiateLongRunningProcessInWebApi( scopes.Except(_scopesRequestedByMsal), - sessionKey); + tokenUsedToCallTheWebApi, + ref sessionKey); + tokenAcquisitionOptions.LongRunningWebApiSessionKey = sessionKey; } + } + else if (!string.IsNullOrEmpty(tokenAcquisitionOptions?.LongRunningWebApiSessionKey)) + { + string sessionKey = tokenAcquisitionOptions!.LongRunningWebApiSessionKey!; + builder = (application as ILongRunningWebApi)? + .AcquireTokenInLongRunningProcess( + scopes.Except(_scopesRequestedByMsal), + sessionKey); + } - if (builder != null) - { - builder.WithSendX5C(mergedOptions.SendX5C); + if (builder != null) + { + builder.WithSendX5C(mergedOptions.SendX5C); - ClaimsPrincipal? userForCcsRouting = _tokenAcquisitionHost.GetUserFromRequest(); - var userTenant = string.Empty; - if (userForCcsRouting != null) - { - userTenant = userForCcsRouting.GetTenantId(); - builder.WithCcsRoutingHint(userForCcsRouting.GetObjectId(), userTenant); - } - if (!string.IsNullOrEmpty(tenantId)) + ClaimsPrincipal? userForCcsRouting = _tokenAcquisitionHost.GetUserFromRequest(); + var userTenant = string.Empty; + if (userForCcsRouting != null) + { + userTenant = userForCcsRouting.GetTenantId(); + builder.WithCcsRoutingHint(userForCcsRouting.GetObjectId(), userTenant); + } + if (!string.IsNullOrEmpty(tenantId)) + { + builder.WithTenantId(tenantId); + } + else + { + if (!string.IsNullOrEmpty(userTenant)) { - builder.WithTenantId(tenantId); + builder.WithTenantId(userTenant); } - else + } + if (tokenAcquisitionOptions != null) + { + if (addInOptions != null && addInOptions.InvokeOnBeforeTokenAcquisitionForOnBehalfOfAsync != null) { - if (!string.IsNullOrEmpty(userTenant)) + var eventArgs = new OnBehalfOfEventArgs { - builder.WithTenantId(userTenant); - } + User = userHint, + UserAssertionToken = originalTokenToCallWebApi + }; + + await addInOptions.InvokeOnBeforeTokenAcquisitionForOnBehalfOfAsync(builder, tokenAcquisitionOptions, eventArgs).ConfigureAwait(false); } - if (tokenAcquisitionOptions != null) - { - if (addInOptions != null && addInOptions.InvokeOnBeforeTokenAcquisitionForOnBehalfOfAsync != null) - { - var eventArgs = new OnBehalfOfEventArgs - { - User = userHint, - UserAssertionToken = originalTokenToCallWebApi - }; - await addInOptions.InvokeOnBeforeTokenAcquisitionForOnBehalfOfAsync(builder, tokenAcquisitionOptions, eventArgs).ConfigureAwait(false); - } + AddFmiPathForSignedAssertionIfNeeded(tokenAcquisitionOptions, builder); - AddFmiPathForSignedAssertionIfNeeded(tokenAcquisitionOptions, builder); + var dict = MergeExtraQueryParameters(mergedOptions, tokenAcquisitionOptions); + if (dict != null) + { + const string assertionConstant = "assertion"; + const string subAssertionConstant = "sub_assertion"; - var dict = MergeExtraQueryParameters(mergedOptions, tokenAcquisitionOptions); - if (dict != null) + // Special case when the OBO inbound token is composite (for instance PFT) + if (dict.ContainsKey(assertionConstant) && dict.ContainsKey(subAssertionConstant)) { - const string assertionConstant = "assertion"; - const string subAssertionConstant = "sub_assertion"; + string assertion = dict[assertionConstant].value; + string subAssertion = dict[subAssertionConstant].value; - // Special case when the OBO inbound token is composite (for instance PFT) - if (dict.ContainsKey(assertionConstant) && dict.ContainsKey(subAssertionConstant)) - { - string assertion = dict[assertionConstant].value; - string subAssertion = dict[subAssertionConstant].value; + // Check assertion and sub_assertion passed from merging extra query parameters to ensure they do not contain unsupported character(s). + CheckAssertionsForInjectionAttempt(assertion, subAssertion); - // Check assertion and sub_assertion passed from merging extra query parameters to ensure they do not contain unsupported character(s). - CheckAssertionsForInjectionAttempt(assertion, subAssertion); - - builder.OnBeforeTokenRequest((data) => - { - // Replace the assertion and adds sub_assertion with the values from the extra query parameters - data.BodyParameters[assertionConstant] = assertion; - data.BodyParameters.Add(subAssertionConstant, subAssertion); - return Task.CompletedTask; - }); - - // Remove the assertion and sub_assertion from the extra query parameters - // as they are already handled as body parameters. - dict.Remove(assertionConstant); - dict.Remove(subAssertionConstant); - } - - builder.WithExtraQueryParameters(dict); - } - if (tokenAcquisitionOptions.ExtraHeadersParameters != null) - { - builder.WithExtraHttpHeaders(tokenAcquisitionOptions.ExtraHeadersParameters); - } - if (tokenAcquisitionOptions.CorrelationId != null) - { - builder.WithCorrelationId(tokenAcquisitionOptions.CorrelationId.Value); - } - builder.WithForceRefresh(tokenAcquisitionOptions.ForceRefresh); - builder.WithClaims(tokenAcquisitionOptions.Claims); - var clientClaims = GetClientClaimsIfExist(tokenAcquisitionOptions); - if (clientClaims != null) - { - builder.WithExtraClientAssertionClaims(clientClaims); - } - if (tokenAcquisitionOptions.PoPConfiguration != null) - { - builder.WithSignedHttpRequestProofOfPossession(tokenAcquisitionOptions.PoPConfiguration); + builder.OnBeforeTokenRequest((data) => + { + // Replace the assertion and adds sub_assertion with the values from the extra query parameters + data.BodyParameters[assertionConstant] = assertion; + data.BodyParameters.Add(subAssertionConstant, subAssertion); + return Task.CompletedTask; + }); + + // Remove the assertion and sub_assertion from the extra query parameters + // as they are already handled as body parameters. + dict.Remove(assertionConstant); + dict.Remove(subAssertionConstant); } - } - return await builder.ExecuteAsync(tokenAcquisitionOptions != null ? tokenAcquisitionOptions.CancellationToken : CancellationToken.None) - .ConfigureAwait(false); + builder.WithExtraQueryParameters(dict); + } + if (tokenAcquisitionOptions.ExtraHeadersParameters != null) + { + builder.WithExtraHttpHeaders(tokenAcquisitionOptions.ExtraHeadersParameters); + } + if (tokenAcquisitionOptions.CorrelationId != null) + { + builder.WithCorrelationId(tokenAcquisitionOptions.CorrelationId.Value); + } + builder.WithForceRefresh(tokenAcquisitionOptions.ForceRefresh); + builder.WithClaims(tokenAcquisitionOptions.Claims); + var clientClaims = GetClientClaimsIfExist(tokenAcquisitionOptions); + if (clientClaims != null) + { + builder.WithExtraClientAssertionClaims(clientClaims); + } + if (tokenAcquisitionOptions.PoPConfiguration != null) + { + builder.WithSignedHttpRequestProofOfPossession(tokenAcquisitionOptions.PoPConfiguration); + } } - return null; - } - catch (MsalUiRequiredException ex) - { - Logger.TokenAcquisitionError( - _logger, - LogMessages.ErrorAcquiringTokenForDownstreamWebApi + ex.Message, - ex); - throw; + return await builder.ExecuteAsync(tokenAcquisitionOptions != null ? tokenAcquisitionOptions.CancellationToken : CancellationToken.None) + .ConfigureAwait(false); } + + return null; } /// diff --git a/tests/Microsoft.Identity.Web.Test/TokenAcquisitionTests.cs b/tests/Microsoft.Identity.Web.Test/TokenAcquisitionTests.cs index ef66fa427..bcb699e54 100644 --- a/tests/Microsoft.Identity.Web.Test/TokenAcquisitionTests.cs +++ b/tests/Microsoft.Identity.Web.Test/TokenAcquisitionTests.cs @@ -4,15 +4,18 @@ using System; using System.Collections.Generic; using System.Net.Http; +using System.Security.Claims; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Identity.Abstractions; using Microsoft.Identity.Client; using Microsoft.Identity.Web.Extensibility; using Microsoft.Identity.Web.Test.Common; using Microsoft.Identity.Web.Test.Common.Mocks; using Microsoft.Identity.Web.TestOnly; +using NSubstitute; using Xunit; @@ -160,6 +163,44 @@ public async Task ExtraBodyParametersAreSentToEndpointTest() Assert.Equal("Bearer header.payload.signature", result); } + /// + /// Tests that a caught is not re-logged by Microsoft.Identity.Web, + /// since MSAL.NET already logs it. Re-logging produced duplicate log entries. + /// This addresses issue #3528. + /// + [Fact] + public async Task GetAuthenticationResultForUserAsync_DoesNotDuplicateLog_WhenMsalUiRequiredExceptionIsThrown() + { + // Arrange + var tokenAcquirerFactory = InitTokenAcquirerFactory(); + + var innerLogger = Substitute.For>(); + innerLogger.IsEnabled(Arg.Any()).Returns(true); + tokenAcquirerFactory.Services.AddSingleton>( + new LoggerMock(innerLogger)); + + IServiceProvider serviceProvider = tokenAcquirerFactory.Build(); + IAuthorizationHeaderProvider authorizationHeaderProvider = serviceProvider.GetRequiredService(); + + // No account/login-hint claims, so MSAL's AcquireTokenSilent throws MsalUiRequiredException + // synchronously (the exact scenario reported in issue #3528). + var user = new ClaimsPrincipal(new Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity()); + + // Act & Assert + await Assert.ThrowsAsync(() => + authorizationHeaderProvider.CreateAuthorizationHeaderForUserAsync( + new[] { "https://graph.microsoft.com/.default" }, + authorizationHeaderProviderOptions: null, + claimsPrincipal: user)); + + innerLogger.DidNotReceive().Log( + Microsoft.Extensions.Logging.LogLevel.Information, + Arg.Is(e => e.Id == 300), // LoggingEventId.TokenAcquisitionError + Arg.Any(), + Arg.Any(), + Arg.Any>()); + } + private TokenAcquirerFactory InitTokenAcquirerFactory() { TokenAcquirerFactoryTesting.ResetTokenAcquirerFactoryInTest();