Skip to content

Commit 2bc618a

Browse files
committed
OAuth client: refresh before re-authorizing, and discover before refreshing
The 401 branch of OAuthClientProvider discovered metadata but never tried the stored refresh token, and the pre-request refresh tried the refresh token but never discovered metadata. After a restart nothing restores an expiry, so the pre-request path is skipped, the stale bearer draws a 401, and the flow went straight to interactive authorization with a usable refresh token in hand; headless clients failed outright. When an application forced the pre-request path, the refresh was posted to a path guessed from the server origin, which 404s against an authorization server under a path, and the refresh token was dropped. The 401 branch now tries the refresh_token grant after discovery and registration and runs the full authorization only when there is no refresh token or the server rejects it. The pre-request refresh runs only when authorization server metadata is already known, so it never guesses an endpoint; a cold start takes the 401 and refreshes there. A fresh dynamic registration clears any held tokens, which belonged to a previous client. Closes #3240, #3250, #1318.
1 parent 0d92192 commit 2bc618a

7 files changed

Lines changed: 237 additions & 8 deletions

File tree

docs/client/oauth-clients.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ The first time `Client` sends a request, the server answers `401`. The provider
8181
3. **Authorization.** It generates the PKCE pair and a `state`, builds the authorization URL, awaits your `redirect_handler`, then awaits your `callback_handler` for the code.
8282
4. **Exchange.** It trades the code for an `OAuthToken`, stores it, and replays your original request with `Authorization: Bearer ...`.
8383

84-
After that it is quiet. Tokens come out of storage, an expired access token is refreshed with the refresh token, and only when none of that works does it run the flow again.
84+
After that it is quiet. Tokens come out of storage, an expired access token is refreshed with the refresh token, and only when none of that works does it run the flow again. That holds across restarts: a new process that finds a refresh token in storage answers the first `401` by rediscovering the authorization server and refreshing, not by sending anyone back to the browser.
8585

8686
You wrote none of it. Two keyword arguments remain (`client_metadata_url` and `validate_resource_url`), and this file needs neither. `client_metadata_url` is the one worth knowing about; it gets its own section below.
8787

src/mcp/client/auth/oauth2.py

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -586,8 +586,13 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
586586
# Capture protocol version from request headers
587587
self.context.protocol_version = request.headers.get(MCP_PROTOCOL_VERSION_HEADER)
588588

589-
if not self.context.is_token_valid() and self.context.can_refresh_token():
590-
# Try to refresh token
589+
# Refresh ahead of the request only when the token endpoint is already known; on a cold
590+
# start the request goes out and the 401 branch discovers, then refreshes.
591+
if (
592+
not self.context.is_token_valid()
593+
and self.context.can_refresh_token()
594+
and self.context.oauth_metadata is not None
595+
):
591596
refresh_request = await self._refresh_token()
592597
refresh_response = yield refresh_request
593598

@@ -741,10 +746,18 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx
741746
client_information.issuer = discovered_issuer
742747
self.context.client_info = client_information
743748
await self.context.storage.set_client_info(client_information)
744-
745-
# Step 5: Perform authorization and complete token exchange
746-
token_response = yield await self._perform_authorization()
747-
await self._handle_token_response(token_response)
749+
# Held tokens belong to a previous client and cannot be refreshed by this one.
750+
self.context.clear_tokens()
751+
752+
# Step 5: Refresh with the stored refresh token first (RFC 6749 §6); run the full
753+
# authorization only when there is none or the server rejects it.
754+
refreshed = False
755+
if self.context.can_refresh_token():
756+
refresh_response = yield await self._refresh_token()
757+
refreshed = await self._handle_refresh_response(refresh_response)
758+
if not refreshed:
759+
token_response = yield await self._perform_authorization()
760+
await self._handle_token_response(token_response)
748761
except Exception:
749762
logger.exception("OAuth flow error")
750763
raise

tests/client/test_auth.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3253,3 +3253,29 @@ async def echo_callback() -> AuthorizationCodeResult:
32533253
await auth_flow.asend(httpx2.Response(200, request=final_req))
32543254
except StopAsyncIteration:
32553255
pass
3256+
3257+
3258+
@pytest.mark.anyio
3259+
async def test_expired_token_is_not_refreshed_ahead_of_the_request_before_metadata_is_discovered(
3260+
oauth_provider: OAuthClientProvider, valid_tokens: OAuthToken
3261+
) -> None:
3262+
"""With no authorization-server metadata yet, an expired token is not refreshed at a guessed endpoint.
3263+
3264+
The request goes out unauthenticated instead, so the 401 branch discovers the real token
3265+
endpoint before the refresh token is presented anywhere (#3240).
3266+
"""
3267+
oauth_provider.context.current_tokens = valid_tokens
3268+
oauth_provider.context.token_expiry_time = time.time() - 60
3269+
oauth_provider.context.client_info = OAuthClientInformationFull(client_id="c", redirect_uris=None)
3270+
oauth_provider.context.oauth_metadata = None
3271+
oauth_provider._initialized = True
3272+
3273+
request = httpx2.Request("POST", "https://api.example.com/v1/mcp")
3274+
auth_flow = oauth_provider.async_auth_flow(request)
3275+
first = await auth_flow.__anext__()
3276+
3277+
assert first is request
3278+
assert "Authorization" not in first.headers
3279+
3280+
with pytest.raises(StopAsyncIteration):
3281+
await auth_flow.asend(httpx2.Response(200, request=request))

tests/interaction/_requirements.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3883,6 +3883,26 @@ def __post_init__(self) -> None:
38833883
transports=("streamable-http",),
38843884
note="OAuth is HTTP-only.",
38853885
),
3886+
"client-auth:refresh:on-401": Requirement(
3887+
source="issue:#3250",
3888+
behavior=(
3889+
"A 401 received while a refresh token is held is answered, after rediscovery, with a "
3890+
"refresh_token grant before any interactive authorization, so a client constructed over "
3891+
"persisted tokens and client registration recovers from an expired access token headlessly."
3892+
),
3893+
transports=("streamable-http",),
3894+
note="OAuth is HTTP-only. RFC 6749 §1.5 (E)-(H); matches the TypeScript, C# and Rust SDKs.",
3895+
),
3896+
"client-auth:refresh:discovered-endpoint": Requirement(
3897+
source="issue:#3240",
3898+
behavior=(
3899+
"A refresh in a process that has not yet discovered the authorization server happens only after "
3900+
"protected-resource and authorization-server metadata discovery and posts to the advertised "
3901+
"token endpoint, never to a path guessed from the server origin."
3902+
),
3903+
transports=("streamable-http",),
3904+
note="OAuth is HTTP-only.",
3905+
),
38863906
"client-auth:resource-parameter": Requirement(
38873907
source=f"{SPEC_BASE_URL}/basic/authorization#resource-parameter-implementation",
38883908
behavior=(

tests/interaction/auth/_harness.py

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,14 @@
2626
from mcp.server import Server
2727
from mcp.server.auth.provider import AccessToken, ProviderTokenVerifier
2828
from mcp.server.auth.settings import AuthSettings, ClientRegistrationOptions, RevocationOptions
29-
from mcp.shared.auth import AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthToken
29+
from mcp.shared.auth import (
30+
AuthorizationCodeResult,
31+
OAuthClientInformationFull,
32+
OAuthClientMetadata,
33+
OAuthMetadata,
34+
OAuthToken,
35+
ProtectedResourceMetadata,
36+
)
3037
from tests.interaction._connect import BASE_URL, NO_DNS_REBINDING_PROTECTION
3138
from tests.interaction.auth._provider import InMemoryAuthorizationServerProvider
3239
from tests.interaction.transports._bridge import StreamingASGITransport
@@ -273,6 +280,53 @@ def shim(
273280
return lambda app: shimmed_app(app, not_found=not_found, serve=serve)
274281

275282

283+
def path_prefixed_as_shim(prefix: str) -> AppShim:
284+
"""Build an `app_shim` that presents the co-hosted authorization server as living under `prefix`.
285+
286+
The SDK server mounts `/authorize`, `/token` and `/register` at the origin root whatever the
287+
issuer, so an AS whose endpoints sit under a path cannot be configured natively. This serves
288+
PRM naming `{BASE_URL}{prefix}` as the AS, serves that issuer's metadata at the RFC 8414
289+
path-inserted well-known URL with every endpoint under the prefix, forwards `{prefix}/x` to the
290+
real `/x`, and 404s the bare root endpoints and root metadata so a client guessing origin-root
291+
paths fails as it would against such a server. Pair with
292+
`InMemoryAuthorizationServerProvider(issuer=f"{BASE_URL}{prefix}")` so the redirect `iss` matches.
293+
"""
294+
issuer = f"{BASE_URL}{prefix}"
295+
prm = ProtectedResourceMetadata(resource=AnyHttpUrl(f"{BASE_URL}/mcp"), authorization_servers=[AnyHttpUrl(issuer)])
296+
asm = OAuthMetadata(
297+
issuer=AnyHttpUrl(issuer),
298+
authorization_endpoint=AnyHttpUrl(f"{issuer}/authorize"),
299+
token_endpoint=AnyHttpUrl(f"{issuer}/token"),
300+
registration_endpoint=AnyHttpUrl(f"{issuer}/register"),
301+
scopes_supported=["mcp"],
302+
response_types_supported=["code"],
303+
grant_types_supported=["authorization_code", "refresh_token"],
304+
token_endpoint_auth_methods_supported=["client_secret_post", "client_secret_basic", "none"],
305+
code_challenge_methods_supported=["S256"],
306+
)
307+
308+
def factory(app: ASGIApp) -> ASGIApp:
309+
inner = shimmed_app(
310+
app,
311+
not_found=frozenset({"/token", "/authorize", "/register", "/.well-known/oauth-authorization-server"}),
312+
serve={
313+
"/.well-known/oauth-protected-resource/mcp": metadata_body(prm),
314+
f"/.well-known/oauth-authorization-server{prefix}": metadata_body(asm),
315+
},
316+
)
317+
318+
async def wrapped(scope: Scope, receive: Receive, send: Send) -> None:
319+
if scope["type"] == "http" and scope["path"].startswith(f"{prefix}/"):
320+
path = scope["path"][len(prefix) :]
321+
await app({**scope, "path": path, "raw_path": path.encode()}, receive, send)
322+
return
323+
await inner(scope, receive, send)
324+
325+
return wrapped
326+
327+
return factory
328+
329+
276330
@dataclass
277331
class _FirstChallenge:
278332
"""ASGI shim that answers the first request to a path with 401 + a given WWW-Authenticate.

tests/interaction/auth/_provider.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ def mint_access_token(self, *, client_id: str, scopes: list[str], resource: str
103103
)
104104
return access
105105

106+
def expire_access_token(self, token: str) -> None:
107+
"""Move an issued access token's server-side expiry into the past so the bearer middleware 401s it."""
108+
self.access_tokens[token] = self.access_tokens[token].model_copy(update={"expires_at": int(time.time()) - 1})
109+
106110
async def get_client(self, client_id: str) -> OAuthClientInformationFull | None:
107111
return self.clients.get(client_id)
108112

tests/interaction/auth/test_lifecycle.py

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,23 @@
1818
from pydantic import AnyHttpUrl, AnyUrl
1919

2020
from mcp import MCPError
21+
from mcp.client.auth import OAuthClientProvider
2122
from mcp.client.auth.extensions.client_credentials import ClientCredentialsOAuthProvider, PrivateKeyJWTOAuthProvider
2223
from mcp.server import Server, ServerRequestContext
2324
from mcp.shared.auth import OAuthClientInformationFull, OAuthMetadata
2425
from tests.interaction._connect import BASE_URL
2526
from tests.interaction._requirements import requirement
2627
from tests.interaction.auth._harness import (
2728
REDIRECT_URI,
29+
AppShim,
2830
InMemoryTokenStorage,
2931
RecordedRequest,
3032
auth_settings,
3133
connect_with_oauth,
3234
m2m_token_shim,
3335
metadata_body,
36+
oauth_client_metadata,
37+
path_prefixed_as_shim,
3438
record_requests,
3539
shim,
3640
step_up_shim,
@@ -98,6 +102,29 @@ def seeded_client(provider: InMemoryAuthorizationServerProvider, **kwargs: objec
98102
return info
99103

100104

105+
async def first_process_login(
106+
provider: InMemoryAuthorizationServerProvider, storage: InMemoryTokenStorage, *, app_shim: AppShim | None = None
107+
) -> str:
108+
"""Run one interactive connect so `storage` holds what a first process leaves behind; return its access token.
109+
110+
The restart tests then build a fresh `OAuthClientProvider` over the same storage, as a second
111+
process would, so the registration and tokens carry exactly what the SDK persists.
112+
"""
113+
server = Server("guarded", on_list_tools=list_tools)
114+
async with connect_with_oauth(server, provider=provider, storage=storage, app_shim=app_shim) as (client, _):
115+
await client.list_tools()
116+
assert storage.tokens is not None and storage.tokens.refresh_token is not None
117+
return storage.tokens.access_token
118+
119+
120+
def restarted_headless_provider(storage: InMemoryTokenStorage) -> OAuthClientProvider:
121+
"""A provider as a second, headless process constructs it: same storage, fresh state, no handlers.
122+
123+
Reaching the interactive step raises rather than opening a browser the scenario says is absent.
124+
"""
125+
return OAuthClientProvider(server_url=f"{BASE_URL}/mcp", client_metadata=oauth_client_metadata(), storage=storage)
126+
127+
101128
@requirement("client-auth:refresh:transparent")
102129
async def test_an_expired_access_token_is_transparently_refreshed_before_the_next_request() -> None:
103130
"""An access token the client considers expired is refreshed and the new bearer is used.
@@ -354,6 +381,91 @@ async def test_a_failed_refresh_clears_stored_tokens_and_restarts_the_full_flow(
354381
assert storage.tokens.access_token in provider.access_tokens
355382

356383

384+
@requirement("client-auth:refresh:on-401")
385+
async def test_a_restarted_client_answers_a_401_with_its_stored_refresh_token() -> None:
386+
"""A second process holding only persisted tokens and registration refreshes on 401 instead of re-authorizing.
387+
388+
Steps: (1) a first process logs in and its storage keeps the registration and a refresh token;
389+
(2) the server-side access token lapses; (3) a fresh provider over the same storage, with no
390+
browser, connects. The recording proves the stale bearer drew a 401, discovery ran, one
391+
`refresh_token` grant followed, and neither `/authorize` nor `/register` was touched.
392+
SDK behaviour per RFC 6749 §1.5; regression bar for #3250 / #1318.
393+
"""
394+
provider = InMemoryAuthorizationServerProvider()
395+
storage = InMemoryTokenStorage()
396+
with anyio.fail_after(5):
397+
stale_access_token = await first_process_login(provider, storage)
398+
provider.expire_access_token(stale_access_token)
399+
400+
recorded, on_request = record_requests()
401+
server = Server("guarded", on_list_tools=list_tools)
402+
with anyio.fail_after(5):
403+
async with connect_with_oauth(
404+
server, provider=provider, auth=restarted_headless_provider(storage), on_request=on_request
405+
) as (client, _):
406+
result = await client.list_tools()
407+
408+
assert result.tools[0].name == "echo"
409+
assert [(r.method, r.path) for r in recorded[:5]] == snapshot(
410+
[
411+
("POST", "/mcp"),
412+
("GET", "/.well-known/oauth-protected-resource/mcp"),
413+
("GET", "/.well-known/oauth-authorization-server"),
414+
("POST", "/token"),
415+
("POST", "/mcp"),
416+
]
417+
)
418+
assert recorded[0].headers["authorization"] == f"Bearer {stale_access_token}"
419+
assert [form_body(r)["grant_type"] for r in find(recorded, "POST", "/token")] == ["refresh_token"]
420+
assert find(recorded, "GET", "/authorize") == [] and find(recorded, "POST", "/register") == []
421+
assert storage.tokens is not None and storage.tokens.access_token != stale_access_token
422+
assert storage.tokens.access_token in provider.access_tokens
423+
424+
425+
@requirement("client-auth:refresh:discovered-endpoint")
426+
async def test_a_restarted_client_refreshes_at_the_token_endpoint_advertised_under_a_path() -> None:
427+
"""Against an authorization server under `/oauth2/v1`, a second process refreshes at `/oauth2/v1/token`.
428+
429+
The bare `/token` 404s here. Nothing is discovered yet in the second process, so no refresh is
430+
attempted before the request; the 401 drives discovery and the single refresh POST goes to the
431+
advertised endpoint. Regression bar for #3240, where the guessed `{origin}/token` 404ed and the
432+
refresh token was discarded.
433+
"""
434+
prefix = "/oauth2/v1"
435+
provider = InMemoryAuthorizationServerProvider(issuer=f"{BASE_URL}{prefix}")
436+
storage = InMemoryTokenStorage()
437+
app_shim = path_prefixed_as_shim(prefix)
438+
with anyio.fail_after(5):
439+
stale_access_token = await first_process_login(provider, storage, app_shim=app_shim)
440+
provider.expire_access_token(stale_access_token)
441+
442+
recorded, on_request = record_requests()
443+
server = Server("guarded", on_list_tools=list_tools)
444+
with anyio.fail_after(5):
445+
async with connect_with_oauth(
446+
server,
447+
provider=provider,
448+
auth=restarted_headless_provider(storage),
449+
app_shim=app_shim,
450+
on_request=on_request,
451+
) as (client, _):
452+
result = await client.list_tools()
453+
454+
assert result.tools[0].name == "echo"
455+
assert [(r.method, r.path) for r in recorded[:5]] == snapshot(
456+
[
457+
("POST", "/mcp"),
458+
("GET", "/.well-known/oauth-protected-resource/mcp"),
459+
("GET", "/.well-known/oauth-authorization-server/oauth2/v1"),
460+
("POST", "/oauth2/v1/token"),
461+
("POST", "/mcp"),
462+
]
463+
)
464+
token_posts = [r for r in recorded if r.method == "POST" and r.path.endswith("/token")]
465+
assert [(r.path, form_body(r)["grant_type"]) for r in token_posts] == [("/oauth2/v1/token", "refresh_token")]
466+
assert not any(r.path.endswith("/authorize") for r in recorded)
467+
468+
357469
@requirement("client-auth:client-credentials")
358470
async def test_client_credentials_provider_obtains_a_token_without_an_authorize_step() -> None:
359471
"""The client-credentials provider connects with no authorize step and a `client_credentials` grant.

0 commit comments

Comments
 (0)