-
Notifications
You must be signed in to change notification settings - Fork 249
Extract shared WWW-Authenticate challenge handling as internal helper #3545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: jmprieur <13203188+jmprieur@users.noreply.github.com>
…ngeHelper and fix ForceRefresh behavior Co-authored-by: jmprieur <13203188+jmprieur@users.noreply.github.com>
jmprieur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks copilot
|
@jmprieur - comment from Vega: ✅ What's Strong: The bug fix removing unnecessary ForceRefresh is correct—MSAL.NET handles cache bypass automatically via CacheRefreshReason.ForceRefreshOrClaims DownstreamApi uses WwwAuthenticateChallengeHelper.CloneHttpContentAsync() directly |
|
|
||
| // Read the content into a byte array to ensure it can be reused | ||
| #if NET | ||
| byte[] contentBytes = await originalContent.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there worries along the same thoughts as #3532 ?
| /// and creating new ByteArrayContent. This ensures the content can be sent multiple times, | ||
| /// even if the original stream was non-seekable. | ||
| /// </remarks> | ||
| public static async Task<HttpContent?> CloneHttpContentAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this method at all. Why can't we just use:
await originalContent.LoadIntoBufferAsync(cancellationToken);
This should buffer the content into memory allowing multiple reads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also provide an upper bound of the size of the buffer
| /// A 401 Unauthorized response may include a WWW-Authenticate header with a claims challenge. | ||
| /// The actual claims extraction should be done using <see cref="ExtractClaimsChallenge"/>. | ||
| /// </remarks> | ||
| public static bool ShouldAttemptClaimsChallengeRetry(HttpResponseMessage response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider a merge it with ExtractClaimsChallenge(), e.g., in form of TryExtractClaimsChallengeToRetry, returning bool with claims as string out parameter? It'd provide a slightly better user experience such as a single call/check would be needed considering that claims are used straight away.
Overview
This PR extracts duplicate WWW-Authenticate challenge handling logic from
DownstreamApiandMicrosoftIdentityMessageHandlerinto a shared internal helper, improving maintainability and fixing a bug whereForceRefreshwas incorrectly set during claims challenges.Problem
Both
DownstreamApiandMicrosoftIdentityMessageHandlerimplemented nearly identical logic for handling WWW-Authenticate header challenges, including:This duplication made maintenance difficult, and bug fixes had to be applied in multiple places. Additionally,
MicrosoftIdentityMessageHandlerwas incorrectly settingForceRefresh = truewhen handling claims challenges, which is unnecessary since MSAL.NET automatically bypasses the cache when claims are present (see CacheRefreshReason.ForceRefreshOrClaims).Solution
Created
WwwAuthenticateChallengeHelperin the TokenAcquisition project with three core methods:Both
DownstreamApiandMicrosoftIdentityMessageHandlernow use this helper, ensuring consistent behavior across entry points.Key Changes
1. WwwAuthenticateChallengeHelper (New)
2. DownstreamApi (Refactored)
WwwAuthenticateChallengeHelper.ExtractClaimsChallenge()instead of directly callingWwwAuthenticateParametersWwwAuthenticateChallengeHelper.CloneHttpContentAsync()3. MicrosoftIdentityMessageHandler (Refactored & Bug Fixed)
WwwAuthenticateChallengeHelpermethods for consistencyForceRefresh = truesetting when claims are present4. Unit Tests (New)
Benefits
✅ Eliminates code duplication - Shared logic in one place makes maintenance easier
✅ Fixes ForceRefresh bug - Corrects unnecessary cache bypass in MicrosoftIdentityMessageHandler
✅ Defensive content cloning - Properly handles non-seekable streams in retry scenarios
✅ Enhanced maintainability - Inline comments document design decisions
✅ Consistent behavior - Both entry points now handle challenges identically
✅ No breaking changes - All 644 existing tests pass without modification
Testing
Files Changed
WwwAuthenticateChallengeHelper.cs(new, 84 lines)WwwAuthenticateChallengeHelperTests.cs(new, 191 lines)DownstreamApi.cs(18 line changes)MicrosoftIdentityMessageHandler.cs(20 line changes)InternalsVisibleTo.cs(1 line added)InternalAPI.Unshipped.txtfiles for all 5 target frameworks (4 lines each)Total: 10 files changed, +323/-11 lines
Co-created by Jean-Marc and Copilot, per our collaborative process.
Original prompt
Fixes #3541
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.