Fix ClientOAuthProvider "Client ID is not available" on cold start#1705
Open
halter73 wants to merge 1 commit into
Open
Fix ClientOAuthProvider "Client ID is not available" on cold start#1705halter73 wants to merge 1 commit into
halter73 wants to merge 1 commit into
Conversation
…sted On a cold start where a durable ITokenCache returns a persisted refresh token but no client ID has been assigned yet (DCR/CIMD clients), GetAccessTokenAsync attempted a token refresh before assigning a client ID, causing CreateTokenRequest -> GetClientIdOrThrow() to throw 'Client ID is not available'. Persist the client registration (client ID, secret, token endpoint auth method) alongside the tokens so a single durable ITokenCache can use the refresh token after a restart, and restore it on a cold start before the refresh attempt. Also guard the refresh block with a client-ID check so the flow falls through to client-ID assignment and the authorization-code flow instead of throwing when no credentials are available. Fixes #1658 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a cold-start OAuth flow bug where a durable ITokenCache can restore a refresh token but ClientOAuthProvider attempts refresh before a client ID is available, causing a misleading "Client ID is not available" exception. The PR persists client registration details alongside cached tokens and restores them early enough to enable refresh-token usage after process restart, with regression coverage.
Changes:
- Persist optional client registration fields (
ClientId,ClientSecret,TokenEndpointAuthMethod) inTokenContainer. - Restore cached client credentials in
ClientOAuthProviderbefore refresh decisions and guard refresh on presence of a client ID. - Add regression tests covering cold-start refresh with persisted DCR credentials and fallback reauthorization when credentials are absent.
Show a summary per file
| File | Description |
|---|---|
| tests/ModelContextProtocol.AspNetCore.Tests/OAuth/TokenCacheTests.cs | Adds regression tests for cold-start refresh behavior with/without persisted client credentials. |
| src/ModelContextProtocol.Core/Authentication/TokenContainer.cs | Extends cached token model to optionally persist client registration details. |
| src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs | Restores persisted client credentials on cold start, guards refresh, and stamps registration details when storing tokens. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Low
Comment on lines
+1175
to
+1177
| _clientId = tokens!.ClientId; | ||
| _clientSecret ??= tokens.ClientSecret; | ||
| _tokenEndpointAuthMethod ??= tokens.TokenEndpointAuthMethod; |
Comment on lines
+45
to
+47
| /// the user to re-authorize. It is populated automatically when the client ID was obtained via dynamic | ||
| /// client registration or a client-id metadata document; it is left unset for explicitly configured | ||
| /// client IDs, which are supplied via configuration instead. |
|
|
||
| // Only attempt a token refresh if we haven't attempted to already for this request. | ||
| // Also only attempt a token refresh for a 401 Unauthorized responses. Other response status codes | ||
| // should not be used for expired access tokens. This is important because 403 forbiden responses can |
| // Only attempt a token refresh if we haven't attempted to already for this request. | ||
| // Also only attempt a token refresh for a 401 Unauthorized responses. Other response status codes | ||
| // should not be used for expired access tokens. This is important because 403 forbiden responses can | ||
| // be used for incremental consent which cannot be acheived with a simple refresh. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1658.
Problem
On a cold start where a durable
ITokenCachereturns a persisted refresh token but no client ID has been assigned yet (i.e. clients relying on dynamic client registration (DCR) or a client-id metadata document (CIMD) rather than a pre-registeredClientId),ClientOAuthProvider.GetAccessTokenAsyncattempts a token refresh before assigning a client ID.RefreshTokensAsync→CreateTokenRequest→GetClientIdOrThrow()then throws:The message is misleading — DCR hasn't failed, it simply hasn't been given the chance to run yet because the refresh attempt is sequenced ahead of the client-ID assignment block. The sibling
GetAccessTokenSilentAsyncpath is already guarded against an analogous case (it only refreshes when_authServerMetadatais not null);GetAccessTokenAsynchad no equivalent guard for a missing client ID.Fix
A single durable
ITokenCacheshould be enough to survive a process restart. To make that work, the client registration is now persisted alongside the tokens and restored on a cold start:TokenContainer: added optionalClientId,ClientSecret, andTokenEndpointAuthMethodproperties. These are additive and AOT-safe (TokenContainerisn't part of the SDK's source-gen JSON context and is never serialized to the wire), soITokenCachestays unchanged — no breaking interface change.ClientOAuthProvider:_clientId/_clientSecret/_tokenEndpointAuthMethodonto theTokenContainerbefore storing tokens._clientIdis null) before the refresh decision, so a persisted refresh token can actually be used. An explicitly configuredoptions.ClientIdalways takes precedence.!string.IsNullOrEmpty(_clientId)as a safety net, so if credentials aren't available in the cache the flow falls through to client-ID assignment and the authorization-code flow instead of throwing.Resulting behavior
ITokenCacheonly → cold start restores the client ID/secret/auth method, the persisted refresh token is honored, no re-prompt and no redundant DCR.options.ClientId→ unchanged.This coexists with the existing
DynamicClientRegistration.ResponseDelegatemechanism; it just removes the requirement to use it purely to survive a cold-start refresh.Tests
Two regression tests added to
TokenCacheTests:GetTokenAsync_ColdStartWithDynamicRegistration_RefreshesUsingPersistedCredentials— a DCR client persists tokens + credentials, then a fresh provider sharing the same cache refreshes on cold start without re-prompting.GetTokenAsync_ColdStartWithoutPersistedClientId_FallsBackToReauthorization— a cache holding tokens but no persisted credentials re-authorizes instead of throwing.Full solution builds clean (warnings-as-errors) and the OAuth/Auth test suite passes.