-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix telemetry for redirect + custom ICredentials #115939
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
Fix telemetry for redirect + custom ICredentials #115939
Conversation
Tagging subscribers to this area: @dotnet/ncl |
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.
Pull Request Overview
This PR refactors authentication handling in SocketsHttpHandler
by consolidating two handler types into one with a configurable auth flag, updates redirect logic to disable auth via a request flag, and extends tests to cover different credential modes.
- Merged
HttpConnectionHandler
andHttpAuthenticatedConnectionHandler
into a single handler using adoRequestAuth
parameter. - Simplified
RedirectHandler
to flip an auth-disabled flag onHttpRequestMessage
instead of swapping handlers. - Converted redirect span tests to a
[ConditionalTheory]
with inline data for null,CredentialCache
, and custom credentials.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
MetricsTest.cs | Updated redirect metrics test to a [ConditionalTheory] with three credential modes and added CustomCredentials . |
SocketsHttpHandler.cs | Always instantiate HttpConnectionHandler with doRequestAuth flag and pass disableAuthOnRedirect to RedirectHandler . |
RedirectHandler.cs | Removed dual handlers, introduced _disableAuthOnRedirect , and call request.DisableAuth() for redirects. |
HttpConnectionHandler.cs | Added _doRequestAuth field and have SendAsync respect the request’s disabled-auth flag. |
HttpAuthenticatedConnectionHandler.cs | Removed obsolete authenticated handler and its project entry. |
HttpRequestMessage.cs | Added AuthDisabled bit flag and internal methods DisableAuth /IsAuthDisabled . |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs:22
- Add unit tests for
HttpRequestMessage.DisableAuth
andIsAuthDisabled
to verify that the auth-disabled flag is correctly set and interpreted during redirects.
private const int AuthDisabled = 8;
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs
Show resolved
Hide resolved
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.
This is a great change! I'm glad to see HttpAuthenticatedConnectionHandler
gone!
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #115938.
Instead of having two separated handler types, this PR proposes to store a bool in
HttpConnectionHandler
and manages the disabling of the auth by flipping a flag onHttpRequestMessage
.