Skip to content

Commit e283b48

Browse files
committed
Fix duplicate logging of MsalUiRequiredException
MsalUiRequiredException is already logged by MSAL.NET internally, so re-logging it via Logger.TokenAcquisitionError in TokenAcquisition.cs produced a duplicate log entry for every exception. Removed the redundant logging in the two catch blocks that only rethrow/wrap the exception. Fixes #3528
1 parent a3a339f commit e283b48

2 files changed

Lines changed: 44 additions & 7 deletions

File tree

src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -387,8 +387,7 @@ private async Task<AuthenticationResult> GetAuthenticationResultForUserInternalA
387387
catch (MsalUiRequiredException ex)
388388
{
389389
// GetAccessTokenForUserAsync is an abstraction that can be called from a web app or a web API
390-
Logger.TokenAcquisitionError(_logger, ex.Message, ex);
391-
390+
// MsalUiRequiredException is already logged by MSAL. Re-logging here would produce duplicates.
392391
// Case of the web app: we let the MsalUiRequiredException be caught by the
393392
// AuthorizeForScopesAttribute exception filter so that the user can consent, do 2FA, etc ...
394393
throw new MicrosoftIdentityWebChallengeUserException(ex, scopes.ToArray(), userFlow);
@@ -1426,12 +1425,9 @@ private void NotifyCertificateSelection(
14261425

14271426
return null;
14281427
}
1429-
catch (MsalUiRequiredException ex)
1428+
catch (MsalUiRequiredException)
14301429
{
1431-
Logger.TokenAcquisitionError(
1432-
_logger,
1433-
LogMessages.ErrorAcquiringTokenForDownstreamWebApi + ex.Message,
1434-
ex);
1430+
// MsalUiRequiredException is already logged by MSAL. Re-logging here would produce duplicates.
14351431
throw;
14361432
}
14371433
}

tests/Microsoft.Identity.Web.Test/TokenAcquisitionTests.cs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,18 @@
44
using System;
55
using System.Collections.Generic;
66
using System.Net.Http;
7+
using System.Security.Claims;
78
using System.Threading;
89
using System.Threading.Tasks;
910
using Microsoft.Extensions.DependencyInjection;
11+
using Microsoft.Extensions.Logging;
1012
using Microsoft.Identity.Abstractions;
1113
using Microsoft.Identity.Client;
1214
using Microsoft.Identity.Web.Extensibility;
1315
using Microsoft.Identity.Web.Test.Common;
1416
using Microsoft.Identity.Web.Test.Common.Mocks;
1517
using Microsoft.Identity.Web.TestOnly;
18+
using NSubstitute;
1619
using Xunit;
1720

1821

@@ -160,6 +163,44 @@ public async Task ExtraBodyParametersAreSentToEndpointTest()
160163
Assert.Equal("Bearer header.payload.signature", result);
161164
}
162165

166+
/// <summary>
167+
/// Tests that a caught <see cref="MsalUiRequiredException"/> is not re-logged by Microsoft.Identity.Web,
168+
/// since MSAL.NET already logs it. Re-logging produced duplicate log entries.
169+
/// This addresses issue #3528.
170+
/// </summary>
171+
[Fact]
172+
public async Task GetAuthenticationResultForUserAsync_DoesNotDuplicateLog_WhenMsalUiRequiredExceptionIsThrown()
173+
{
174+
// Arrange
175+
var tokenAcquirerFactory = InitTokenAcquirerFactory();
176+
177+
var innerLogger = Substitute.For<ILogger<TokenAcquisition>>();
178+
innerLogger.IsEnabled(Arg.Any<Microsoft.Extensions.Logging.LogLevel>()).Returns(true);
179+
tokenAcquirerFactory.Services.AddSingleton<ILogger<TokenAcquisition>>(
180+
new LoggerMock<TokenAcquisition>(innerLogger));
181+
182+
IServiceProvider serviceProvider = tokenAcquirerFactory.Build();
183+
IAuthorizationHeaderProvider authorizationHeaderProvider = serviceProvider.GetRequiredService<IAuthorizationHeaderProvider>();
184+
185+
// No account/login-hint claims, so MSAL's AcquireTokenSilent throws MsalUiRequiredException
186+
// synchronously (the exact scenario reported in issue #3528).
187+
var user = new ClaimsPrincipal(new Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity());
188+
189+
// Act & Assert
190+
await Assert.ThrowsAsync<MicrosoftIdentityWebChallengeUserException>(() =>
191+
authorizationHeaderProvider.CreateAuthorizationHeaderForUserAsync(
192+
new[] { "https://graph.microsoft.com/.default" },
193+
authorizationHeaderProviderOptions: null,
194+
claimsPrincipal: user));
195+
196+
innerLogger.DidNotReceive().Log(
197+
Microsoft.Extensions.Logging.LogLevel.Information,
198+
Arg.Is<EventId>(e => e.Id == 300), // LoggingEventId.TokenAcquisitionError
199+
Arg.Any<object>(),
200+
Arg.Any<Exception?>(),
201+
Arg.Any<Func<object, Exception?, string>>());
202+
}
203+
163204
private TokenAcquirerFactory InitTokenAcquirerFactory()
164205
{
165206
TokenAcquirerFactoryTesting.ResetTokenAcquirerFactoryInTest();

0 commit comments

Comments
 (0)