-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Add client registration handling to OpenIdConnect authentication flow #65367
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: main
Are you sure you want to change the base?
Conversation
|
Thanks for your PR, @@glucaci. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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
Adds per-request extension points to OpenIdConnectHandler to support multi-tenant OpenID Connect scenarios by dynamically resolving client registration and allowing events to take over token endpoint client authentication.
Changes:
- Introduces
OpenIdConnectClientRegistrationandOpenIdConnectHandler.ResolveClientRegistrationAsync(...)for per-request client settings. - Adds
AuthorizationCodeReceivedContext.HandleClientAuthentication()/HandledClientAuthenticationto allow alternative token endpoint authentication (e.g., client assertions). - Adds a comprehensive new test suite covering default and overridden behaviors across challenge, PAR, token redemption, and protocol validation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Security/Authentication/test/OpenIdConnect/OpenIdConnectExtensionTests.cs | Adds tests validating dynamic client registration and handled client authentication behavior. |
| src/Security/Authentication/OpenIdConnect/src/PublicAPI.Unshipped.txt | Records newly added public APIs for the OpenIdConnect package. |
| src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs | Wires dynamic client registration through challenge, PAR, token redemption, and protocol validation; honors handled client authentication. |
| src/Security/Authentication/OpenIdConnect/src/OpenIdConnectClientRegistration.cs | Introduces the per-request client registration data container (id/secret/TVP). |
| src/Security/Authentication/OpenIdConnect/src/Events/AuthorizationCodeReceivedContext.cs | Adds event-controlled client authentication handling for code redemption. |
| JwtSecurityToken? jwt = null; | ||
| string? nonce = null; | ||
| var validationParameters = Options.TokenValidationParameters.Clone(); | ||
| var registration = await ResolveClientRegistrationAsync(properties); |
Copilot
AI
Feb 9, 2026
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.
properties is used elsewhere in this method as nullable (e.g., passed as properties! later), but here it's passed as non-null to ResolveClientRegistrationAsync(AuthenticationProperties properties). This is a nullability mismatch and may not compile (or can introduce a nullability warning-as-error). Use properties! here (consistent with the existing usage) or ensure properties is non-nullable by construction before this point.
| var registration = await ResolveClientRegistrationAsync(properties); | |
| var registration = await ResolveClientRegistrationAsync(properties!); |
| Assert.NotNull(capturedTokenParams); | ||
| // ClientSecret was null in the registration, so no client_secret should be sent | ||
| // (the OpenIdConnectMessage won't include a null/empty parameter) | ||
| Assert.False(capturedTokenParams!.ContainsKey("client_secret") && !string.IsNullOrEmpty(capturedTokenParams["client_secret"])); |
Copilot
AI
Feb 9, 2026
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 assertion would still pass if client_secret is present but empty, which weakens the test and could mask a regression (the comment says it should be omitted). Prefer asserting that the key is not present at all (e.g., Assert.DoesNotContain(\"client_secret\", capturedTokenParams.Keys)).
| Assert.False(capturedTokenParams!.ContainsKey("client_secret") && !string.IsNullOrEmpty(capturedTokenParams["client_secret"])); | |
| Assert.DoesNotContain("client_secret", capturedTokenParams!.Keys); |
| var server = CreateServerWithEvents(options => | ||
| { | ||
| options.ClientId = "default-client-id"; | ||
| options.Events.OnRedirectToIdentityProvider = ctx => | ||
| { | ||
| capturedClientId = ctx.ProtocolMessage.ClientId; | ||
| ctx.HandleResponse(); | ||
| ctx.Response.StatusCode = StatusCodes.Status200OK; | ||
| return Task.CompletedTask; | ||
| }; | ||
| }); |
Copilot
AI
Feb 9, 2026
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.
The tests create TestServer instances but never dispose them. This can leak resources across the test run and cause flakiness. Update the tests to dispose servers (e.g., await using var server = ...; / using var server = ...;) consistently after each test.
| foreach (string key in query) | ||
| { | ||
| if (key is not null) | ||
| { | ||
| parameters[key] = query[key]!; | ||
| } | ||
| } |
Copilot
AI
Feb 9, 2026
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.
HttpUtility.ParseQueryString(...) returns a NameValueCollection whose enumerator can yield null keys. Declaring the loop variable as non-nullable string makes the key is not null check ineffective (the assignment would already have failed). Use foreach (string? key in query) or iterate query.AllKeys to accurately reflect nullability and simplify the code.
Add client registration handling to OpenIdConnect authentication flow
Summary
Adds per-request extension points to
OpenIdConnectHandlerto support multi-tenant scenarios where the OIDC client identity, credentials, and token validation settings vary per request — without requiring a full handler fork.Motivation
Applications that serve multiple tenants through a single OIDC authentication scheme (e.g., resolving Entra ID app registrations per-request) currently have no way to override
client_id,client_secret, orTokenValidationParameterson a per-request basis. The only option today is to fork the entire handler. This PR adds minimal, non-breaking extension points that close that gap.Changes
New type:
OpenIdConnectClientRegistrationA simple, unsealed class grouping the three per-request client values:
ClientId— used in authorize, token, PAR requests and protocol validationClientSecret— used in token and PAR requests (nullable for public clients / assertion-based auth)TokenValidationParameters— used for ID token validationNew virtual method:
ResolveClientRegistrationAsyncDefault implementation returns values from
OpenIdConnectOptions(with a clonedTokenValidationParameters), preserving existing behavior. Override to resolve a different client registration per request.New event method:
AuthorizationCodeReceivedContext.HandleClientAuthentication()Mirrors the existing
PushedAuthorizationContext.HandleClientAuthentication()pattern. When called inOnAuthorizationCodeReceived, the handler removes client_secret from the token endpoint request, allowing the event to provide alternative credentials (e.g., client_assertion / private_key_jwt).Design notes
ValueTask<T>— The default implementation is synchronous (ValueTask.FromResult), avoiding aTaskallocation on the hot path.HandleClientAuthenticationonAuthorizationCodeReceivedContext— Follows the identical pattern already shipped onPushedAuthorizationContextAuthenticationProperties.GetParameter<T>()