feat: add MetadataAddress support to ClientCredentialsClient for OIDC discovery by paulomorgado · Pull Request #358 · DuendeSoftware/foss

paulomorgado · 2026-07-22T10:42:53Z

Summary

This change adds MetadataAddress support to client-credentials token acquisition so callers can rely on discovery metadata instead of always hardcoding TokenEndpoint.

It aligns ClientCredentialsClient with established ASP.NET authentication configuration patterns while preserving full backward compatibility for existing TokenEndpoint-based setups.

Motivation

Today, ClientCredentialsClient requires TokenEndpoint up front. That makes configuration less flexible in environments where endpoint details are discovered dynamically from OpenID Connect metadata.

This PR enables that dynamic discovery flow by allowing MetadataAddress as an alternative configuration source.

What changed

Public API
- Added Uri? MetadataAddress to ClientCredentialsClient.
- Updated public API verification baseline accordingly.
Validation rules
- Validation now requires:
  - ClientId, and
  - at least one of TokenEndpoint or MetadataAddress.
- Improved validation error messages to explicitly indicate the missing requirement.
Runtime token endpoint resolution
- In ClientCredentialsTokenClient, if TokenEndpoint is missing and MetadataAddress is provided, the token endpoint is resolved via discovery (GetDiscoveryDocumentAsync).
- If discovery metadata does not provide a token endpoint, a clear InvalidOperationException is raised.
- If both are configured, TokenEndpoint takes precedence.

Parallel with ASP.NET Core `MetadataAddress` semantics

This implementation intentionally mirrors the concept used by:

In both APIs, MetadataAddress is the discovery endpoint for obtaining metadata.
This PR brings the same model to client token management:

Same idea: use discovery metadata as a source of authority configuration.
Different use: OIDC/JWT bearer primarily bootstrap sign-in/validation metadata, while this client-credentials flow specifically needs discovery to resolve the token endpoint for token requests.

Behavioral notes

Existing configurations using only TokenEndpoint continue to work unchanged.
When both are set, TokenEndpoint is used directly (no discovery round-trip).
A discovered token endpoint is reused for subsequent forced renewals, avoiding repeated metadata calls.
Explicit metadata document URLs are honored as provided.

Test coverage added/updated

Missing both TokenEndpoint and MetadataAddress throws.
Missing TokenEndpoint + valid MetadataAddress discovers and requests token successfully.
TokenEndpoint precedence over MetadataAddress.
Metadata without token_endpoint throws expected exception.
Metadata request failure path throws expected exception.
Discovered token endpoint is reused.
Explicit metadata document address is used as-is.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

🟡 Changes recommended

Discovery currently weakens HTTPS enforcement, mishandles custom metadata URLs, unnecessarily depends on JWKS availability, and obscures discovery errors.

4 open findings

🧠 Review effort: Balanced

Copilot

🟢 Approval recommended

The implementation preserves compatibility and comprehensively validates and tests the discovery flow.

0 open findings

4 resolved since last review

🧠 Review effort: Balanced

bhazen · 2026-10-15T18:24:15Z

Hi @paulomorgado, thanks for this PR. Discovery support for client credentials is something we'd like to have, and we've spent some time as a team looking at how it should fit into the library.

We'd like to take it in a slightly different direction, mainly so it matches how discovery already works elsewhere in this repo. The introspection handler (OAuth2IntrospectionOptions) and OidcClient both take an Authority, expose a DiscoveryPolicy, and resolve endpoints with GetDiscoveryDocumentAsync. We'd like ClientCredentialsClient to follow the same model so users only have one way of configuring discovery to learn across our libraries.

Concretely, that means:

Authority instead of MetadataAddress. We know OpenIdConnectOptions and JwtBearerOptions have both, but none of our libraries have a MetadataAddress setting, and we'd rather stay consistent with them. Authority already accepts a full discovery URL when it ends in .well-known/openid-configuration (or the policy's DiscoveryDocumentPath), since DiscoveryEndpoint.ParseUrl treats it as the complete address.
A DiscoveryPolicy property, defaulting to new(), the same as OAuth2IntrospectionOptions.DiscoveryPolicy.
Use GetDiscoveryDocumentAsync with that policy rather than fetching and validating the document by hand. That gives us issuer validation, endpoint validation and the HTTPS requirement (with the loopback exception) from the existing policy, and lets users adjust them the same way they do elsewhere. The current code doesn't validate issuer, which is the main gap we'd want closed.
TokenEndpoint still takes precedence, and validation requires either TokenEndpoint or Authority. Your existing validation change already does this, so it should mostly be a rename.
Resolve the endpoint once per client, not on each request. At the moment ClientCredentialsTokenClient writes the discovered endpoint into the shared options object during the request, so concurrent first requests can each fetch the document and overwrite the value while another request is using it. The introspection handler avoids this by resolving lazily, once, through an AsyncLazy set up in PostConfigure. We'd like the same approach here, with the endpoint kept for the lifetime of the options, as introspection and OidcClient do.
Discovery failures throw InvalidOperationException, mapped by ResponseErrorType the same way PostConfigureOAuth2IntrospectionOptions does, so the messages are consistent.

Your tests are a good base. Most of the scenarios (precedence, missing token endpoint, failed request, endpoint reuse) still apply, and we'd add a couple for concurrent first requests and issuer validation.

Would you like to rework the PR along these lines? If you'd rather not, no problem at all. Just let us know and we'll pick it up from here and credit you in our PR.

paulomorgado · 2026-10-15T19:39:43Z

Hi @bhazen,

Let me look into this, but I think I can handle it,

paulomorgado · 2026-10-16T17:12:51Z

Hi @bhazen,

Check out the latest commit.

bhazen

Thanks @paulomorgado for making the changes based on the direction we discussed

A few things about this round of changes:

CI failure. ConventionTests.All_async_methods_should_end_with_Async_and_have_cancellation_token_as_last_parameter fails on AsyncLazy<T>.Value. Details inline.
Discovery isn't set up if Authority is configured late. Details inline on PostConfigureClientCredentialsClient.
Test organization. The discovery tests added to ClientTokenManagementTests.cs would sit better in ClientCredentialsDiscoveryTests.cs, where the existing helpers would cut most of their setup. Could you move them over, apart from Missing_tokenEndpoint_and_authority_throw_exception and TokenEndpoint_should_take_precedence_over_authority, which fit where they are?
One missing test. Nothing covers DPoP with Authority. A test that the proof's htu is the discovered token endpoint (including the nonce retry, if it's easy to include) would close that gap. The code already does the right thing, so this is just about locking it in.

Merge conflict with main. There's a conflict in ServiceCollectionExtensions.cs since main added a validator registration on the same line. Keeping both lines resolves it:

services.TryAddEnumerable(ServiceDescriptor.Singleton<IValidateOptions<ClientCredentialsTokenManagementOptions>, ClientCredentialsTokenManagementOptions.Validator>());
services.TryAddEnumerable(ServiceDescriptor.Singleton<IPostConfigureOptions<ClientCredentialsClient>, PostConfigureClientCredentialsClient>());

There are also a couple of small nits inline. Thanks again for turning this around so quickly.

bhazen · 2026-10-20T17:10:14Z

+    private readonly Lock _initializationLock = new();
+    private Task<T>? _task;
+
+    public Task<T> Value


This trips ConventionTests.All_async_methods_should_end_with_Async_and_have_cancellation_token_as_last_parameter, since the getter returns a Task without the Async suffix or a CancellationToken.

We'd like to keep the Value shape so it matches the other AsyncLazy copies in this repo (introspection and IdentityModel). Could you add an exemption for AsyncLazy<> in that convention test instead, with a short comment explaining why? The per-caller cancellation you already have via WaitAsync(ct) in ClientCredentialsTokenClient covers the token concern.

Added an exemption specifically for AsyncLazy<>, with a short comment explaining the repository’s Value pattern and per-caller cancellation through WaitAsync(ct). The property remains unchanged, and the convention checks still apply to other types. The convention test now passes locally.

bhazen · 2026-10-20T17:11:02Z

+{
+    public void PostConfigure(string? name, ClientCredentialsClient options)
+    {
+        if (options.TokenEndpoint is null && !string.IsNullOrWhiteSpace(options.Authority))


This only creates the lazy resolver if Authority is already set when this post-configure runs. Because it's registered inside AddClientCredentialsTokenManagement, it runs before any PostConfigure<ClientCredentialsClient> the app registers afterwards. If an app sets Authority there, validation passes (it runs after all post-configures) but every token request throws "No TokenEndpoint or Authority configured".

Could this create the lazy whenever TokenEndpoint is null, and check Authority inside the factory instead? The client token code would then just need to handle the missing-Authority case from the factory's exception. A test that sets Authority in a later PostConfigure would lock this in.

The post-configurer now creates the lazy resolver whenever TokenEndpoint is null. The Authority check happens inside the factory, so it sees the value after all post-configures have run.

Added a regression that sets Authority in a later PostConfigure, plus coverage confirming that an explicit TokenEndpoint supplied there still takes precedence. Missing authority is reported by the factory before making any HTTP request.

bhazen · 2026-10-20T17:11:19Z

+
+        if (string.IsNullOrWhiteSpace(disco.TokenEndpoint))
+        {
+            throw new InvalidOperationException($"No token endpoint found in discovery document for client {options.ClientId}");


Small one: the other messages in this flow identify the client by its name (for client {clientName}), but this one uses options.ClientId. Could you pass name through from PostConfigure and use that here so they're consistent?

Passed name from PostConfigure into the discovery factory and used it in the missing-token-endpoint message instead of ClientId. The regression now checks for the configured client name, which differs from the OAuth client ID. The new missing-authority error uses the same name.

bhazen · 2026-10-20T17:12:50Z

    }

-    private HttpClient GetHttpClient(ClientCredentialsClient client)
+    internal static HttpClient GetHttpClient(ClientCredentialsClient client, IHttpClientFactory httpClientFactory)


Nit, optional: reusing this so discovery picks the same HttpClient as the token request makes sense. Since it's now shared, would it read better as an internal method on ClientCredentialsClient (or a small internal helper), rather than PostConfigureClientCredentialsClient calling into ClientCredentialsTokenClient?

Moved GetHttpClient onto ClientCredentialsClient as an internal instance method. Both discovery and token requests now use it, so the post-configurer no longer calls into ClientCredentialsTokenClient.

Selection precedence is unchanged: an explicitly supplied HttpClient, then the configured named client, then the default backchannel client.

Added MetadataAddress to ClientCredentialsClient for OIDC discovery. Validation now requires TokenEndpoint or MetadataAddress. ClientCredentialsTokenClient resolves token endpoint from metadata if needed, with improved error handling. Tests updated for new scenarios and API changes.

Require an explicit metadata document URI, fetch metadata JSON directly, validate only the discovered token endpoint, enforce HTTPS except for localhost/127.0.0.1 loopback HTTP, reject malformed or cross-authority endpoints, skip JWKS/signing-key retrieval, and expand tests for explicit URLs, ignored fields, malformed JSON, loopback exceptions, and endpoint validation.

Replace MetadataAddress with Authority and expose DiscoveryPolicy. Use GetDiscoveryDocumentAsync for standard discovery and validation. Initialize shared, retryable discovery lazily during post-configuration, retaining the resolved endpoint for the options lifetime without mutating TokenEndpoint. Isolate caller cancellation from shared discovery. Preserve explicit TokenEndpoint precedence and configured backchannels. Map discovery failures to InvalidOperationException consistently with introspection, preserving underlying exceptions.

Create lazy discovery whenever TokenEndpoint is absent and defer Authority validation until discovery runs, allowing later PostConfigure callbacks. Use configured client names in discovery errors and move shared HTTP client selection onto ClientCredentialsClient. Consolidate discovery tests and cover late configuration, explicit endpoint precedence, and DPoP endpoint binding including nonce retry. Exempt AsyncLazy from async conventions while preserving its Value API.

paulomorgado

Addressed these points, including the inline comments. I moved the discovery tests into ClientCredentialsDiscoveryTests and reused its setup helpers, leaving Missing_tokenEndpoint_and_authority_throw_exception and TokenEndpoint_should_take_precedence_over_authority in ClientTokenManagementTests.

Added coverage using real DPoP proofs to check that htu is the discovered token endpoint on both the initial request and the nonce retry, including checking the retry’s nonce.

Rebased onto main and resolved the registration conflict by keeping both the management-options validator and the discovery post-configurer. All 125 targeted tests pass locally, including the convention test.

paulomorgado · 2026-10-21T07:55:21Z

+    private readonly Lock _initializationLock = new();
+    private Task<T>? _task;
+
+    public Task<T> Value


Added an exemption specifically for AsyncLazy<>, with a short comment explaining the repository’s Value pattern and per-caller cancellation through WaitAsync(ct). The property remains unchanged, and the convention checks still apply to other types. The convention test now passes locally.

paulomorgado · 2026-10-21T07:57:27Z

    }

-    private HttpClient GetHttpClient(ClientCredentialsClient client)
+    internal static HttpClient GetHttpClient(ClientCredentialsClient client, IHttpClientFactory httpClientFactory)


Moved GetHttpClient onto ClientCredentialsClient as an internal instance method. Both discovery and token requests now use it, so the post-configurer no longer calls into ClientCredentialsTokenClient.

Selection precedence is unchanged: an explicitly supplied HttpClient, then the configured named client, then the default backchannel client.

paulomorgado · 2026-10-21T07:58:10Z

+
+        if (string.IsNullOrWhiteSpace(disco.TokenEndpoint))
+        {
+            throw new InvalidOperationException($"No token endpoint found in discovery document for client {options.ClientId}");


Passed name from PostConfigure into the discovery factory and used it in the missing-token-endpoint message instead of ClientId. The regression now checks for the configured client name, which differs from the OAuth client ID. The new missing-authority error uses the same name.

paulomorgado · 2026-10-21T07:59:18Z

+{
+    public void PostConfigure(string? name, ClientCredentialsClient options)
+    {
+        if (options.TokenEndpoint is null && !string.IsNullOrWhiteSpace(options.Authority))


The post-configurer now creates the lazy resolver whenever TokenEndpoint is null. The Authority check happens inside the factory, so it sees the value after all post-configures have run.

Added a regression that sets Authority in a later PostConfigure, plus coverage confirming that an explicit TokenEndpoint supplied there still takes precedence. Missing authority is reported by the factory before making any HTTP request.

Copilot AI review requested due to automatic review settings July 22, 2026 10:42

Copilot AI reviewed Jul 22, 2026

paulomorgado requested a balanced review from Copilot October 13, 2026 07:32

Copilot started reviewing on behalf of paulomorgado October 13, 2026 07:33

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

paulomorgado force-pushed the feature/MetadataAddress branch 2 times, most recently from f9fe6ef to b8719d9 Compare October 13, 2026 09:48

paulomorgado requested a balanced review from Copilot October 13, 2026 09:49

Copilot started reviewing on behalf of paulomorgado October 13, 2026 09:50

View session

Copilot AI reviewed Oct 13, 2026

View reviewed changes

paulomorgado marked this pull request as draft October 15, 2026 19:36

paulomorgado marked this pull request as ready for review October 19, 2026 17:43

bhazen reviewed Oct 20, 2026

View reviewed changes

paulomorgado added 4 commits October 21, 2026 08:38

paulomorgado force-pushed the feature/MetadataAddress branch from ae20782 to 176ab0d Compare October 21, 2026 08:06

paulomorgado commented Oct 21, 2026

View reviewed changes

Conversation

paulomorgado commented Jul 22, 2026

Summary

Motivation

What changed

Parallel with ASP.NET Core MetadataAddress semantics

Behavioral notes

Test coverage added/updated

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

bhazen commented Oct 15, 2026

Uh oh!

paulomorgado commented Oct 15, 2026

Uh oh!

paulomorgado commented Oct 16, 2026

Uh oh!

bhazen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulomorgado left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Parallel with ASP.NET Core `MetadataAddress` semantics