Skip to content

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

Merged
merged 4 commits into from
Jun 9, 2025

Conversation

antonfirsov
Copy link
Member

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 on HttpRequestMessage.

@Copilot Copilot AI review requested due to automatic review settings May 23, 2025 12:09
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a 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 and HttpAuthenticatedConnectionHandler into a single handler using a doRequestAuth parameter.
  • Simplified RedirectHandler to flip an auth-disabled flag on HttpRequestMessage 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 and IsAuthDisabled to verify that the auth-disabled flag is correctly set and interpreted during redirects.
private const int AuthDisabled = 8;

Copy link
Member

@ManickaP ManickaP left a 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!
:shipit:

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov antonfirsov merged commit 01bf094 into dotnet:main Jun 9, 2025
84 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributed tracing and metrics are not emitted on redirects with custom ICredentials
3 participants