Fix duplicate logging of MsalUiRequiredException#3910
Conversation
@microsoft-github-policy-service agree |
4cb3ff3 to
ecac618
Compare
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 AzureAD#3528
ecac618 to
1ec99d2
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run Microsoft-identity-web PR (YAML) |
|
No pipelines are associated with this pull request. |
|
Opened #3941 as an in-repo copy of this PR so the �zure-pipelines PR pipeline runs natively (fork PRs don't trigger it, and the required check is enforced by an enterprise ruleset that admin merge can't bypass). Your commits and authorship are preserved unchanged in #3941. This PR will be closed as landed once #3941 merges. |
…) (#3941) * 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 * Removing try-catch block to allow the exception propagate naturally --------- Co-authored-by: armin-azar <armin@azars.io>
|
Landed via in-repo copy #3941 (squash-merged), which preserves your original commits and authorship. Thanks @armin-azar! Closing this PR as completed. |
Fix duplicate logging of MsalUiRequiredException
Summary of the changes (Less than 80 chars)
Stop double-logging MsalUiRequiredException
Description
MsalUiRequiredException is already logged internally by MSAL.NET at error
level. Microsoft.Identity.Web additionally logged the same exception via
Logger.TokenAcquisitionErrorin two places in TokenAcquisition.cs, bothof which only rethrow/wrap the exception (no recovery logic depends on the
log call). This produced two log entries for a single MsalUiRequiredException,
as described in the issue.
This PR removes the redundant logging calls in those two catch blocks and
adds a comment explaining why nothing is logged there. All other
Logger.TokenAcquisitionError call sites (which handle exceptions MSAL does
not already log, e.g. certificate retry) are unchanged.
Added a unit test that reproduces the scenario from the issue (AcquireTokenSilent
called with no account/login hint, which MSAL throws MsalUiRequiredException for
synchronously) and asserts the log is not emitted.
Fixes #3528