feat: expose discoverOAuthServerInfo() and add provider caching for auth server URL#1527
Conversation
🦋 Changeset detectedLatest commit: cb7b55c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
generally like the idea, but i'd prefer if we made it flexible to store more discovery state: this would make it easier to squirrel away the PRM URL, OASM URL, scope, resource parameter. Also makes it easier to provide those things out of band, e.g. if you've discovered them previously and want to just load them, or if there's something wrong with discovery (e.g. a 503 is de-railing the discovery process). |
ochafik
left a comment
There was a problem hiding this comment.
I'll defer to @pcarleton for approval but added a few questions :-)
Sure! We can cajole #1350 in potentially? @ochafik thoughts? |
…State Address PR review feedback from pcarleton and ochafik: - Replace saveAuthorizationServerUrl/authorizationServerUrl with unified saveDiscoveryState/discoveryState that stores all discovery results - Add OAuthDiscoveryState type covering auth server URL, resource metadata URL, resource metadata, and auth server metadata - Add 'discovery' scope to invalidateCredentials for cache clearing - Subsumes the use case from PR #1350 (persisting resourceMetadataUrl across browser redirects) - Cached path now restores full discovery state including resource metadata, fixing the resource parameter regression Co-authored-by: hassan123789 <49031989+hassan123789@users.noreply.github.com>
| /** | ||
| * Result of {@linkcode discoverOAuthServerInfo}. | ||
| */ | ||
| export interface OAuthServerInfo { |
There was a problem hiding this comment.
Can this type and OAuthDiscoveryState be the same?
There was a problem hiding this comment.
I think they're a little bit different right, they just happen to look similar right now - but in future the OAuthDiscoveryState (what gets saved) could diverge meaningfully from OAuthServerInfo?
To avoid duplication though makes sense for OAuthDiscoveryState to extend OAuthServerInfo though, so changed that here - wdyt?
Address PR feedback: - OAuthDiscoveryState now extends OAuthServerInfo instead of redeclaring the same fields, keeping them linked while allowing divergence for cache-specific fields like resourceMetadataUrl. - Add TODO for potential authorizationServerMetadataUrl field to capture the exact well-known URL where AS metadata was discovered. - Add TODO noting resourceMetadataUrl is only populated when explicitly provided, not when derived internally.
Backport of PR #1527 from main to v1.x: - Add discoverOAuthServerInfo() combining RFC 9728 + AS metadata discovery - Add OAuthServerInfo and OAuthDiscoveryState interfaces - Add saveDiscoveryState()/discoveryState() to OAuthClientProvider - Add 'discovery' scope to invalidateCredentials() - Update auth() orchestrator to use cached discovery state - Add comprehensive tests for new functionality
Backport of PR #1527 from main to v1.x: - Add discoverOAuthServerInfo() combining RFC 9728 + AS metadata discovery - Add OAuthServerInfo and OAuthDiscoveryState interfaces - Add saveDiscoveryState()/discoveryState() to OAuthClientProvider - Add 'discovery' scope to invalidateCredentials() - Update auth() orchestrator to use cached discovery state - Add comprehensive tests for new functionality
Backport of PR #1527 from main to v1.x: - Add discoverOAuthServerInfo() combining RFC 9728 + AS metadata discovery - Add OAuthServerInfo and OAuthDiscoveryState interfaces - Add saveDiscoveryState()/discoveryState() to OAuthClientProvider - Add 'discovery' scope to invalidateCredentials() - Update auth() orchestrator to use cached discovery state - Add comprehensive tests for new functionality
Backport of PR #1527 from main to v1.x: - Add discoverOAuthServerInfo() combining RFC 9728 + AS metadata discovery - Add OAuthServerInfo and OAuthDiscoveryState interfaces - Add saveDiscoveryState()/discoveryState() to OAuthClientProvider - Add 'discovery' scope to invalidateCredentials() - Update auth() orchestrator to use cached discovery state - Add comprehensive tests for new functionality
Summary
Extracts the RFC 9728 + authorization server metadata discovery logic from
authInternal()into a new publicdiscoverOAuthServerInfo()function, and adds unified discovery state caching toOAuthClientProvider.Motivation and Context
MCP clients need the authorization server URL for operations outside the
auth()orchestrator — specifically token refresh and token revocation. Currently, the SDK discovers this URL via RFC 9728 insideauthInternal()but never exposes it, forcing consumers to reimplement the discovery logic.Additionally, browser-based OAuth flows lose discovery state (including the
resourceMetadataUrlfromWWW-Authenticateheaders) during redirects, causing token exchange failures (#1234, #1350).Key Changes
discoverOAuthServerInfo(serverUrl, opts?)(new export)Combines RFC 9728 protected resource metadata discovery with authorization server metadata discovery into a single call. Returns
OAuthServerInfo { authorizationServerUrl, authorizationServerMetadata?, resourceMetadata? }.OAuthDiscoveryState(new type)Unified type for persisting all discovery results: auth server URL, resource metadata URL, resource metadata, and auth server metadata.
OAuthClientProviderchanges (non-breaking)saveDiscoveryState?(state)— called byauth()after discovery so providers can persist all discovery resultsdiscoveryState?()— returns cached state; when present,auth()restores discovery results instead of re-discoveringinvalidateCredentialsscope union extended with'discovery'for clearing cached discovery stateauthInternal()refactored to usediscoverOAuthServerInfo()and the unified caching. Cached path restores full discovery state including resource metadata, preserving theresourceparameter in token requests.This subsumes the use case from #1350 (persisting
resourceMetadataUrlacross browser redirects) — the URL is now part ofOAuthDiscoveryState.How Has This Been Tested?
discoverOAuthServerInfo()and discovery state cachingBreaking Changes
None. All additions are optional interface methods, new exports, and a new union member for an optional method parameter.
Types of changes
Checklist
Additional context
Co-authored with @hassan123789 whose #1350 identified the
resourceMetadataUrlpersistence problem that this unified approach addresses.