Union previously requested scopes on step-up re-authorization (SEP-2350) (#2931)
This commit is contained in:
parent
4573e4ac33
commit
1331131650
3 changed files with 155 additions and 8 deletions
|
|
@ -26,6 +26,7 @@ from mcp.client.auth.utils import (
|
|||
handle_registration_response,
|
||||
is_valid_client_metadata_url,
|
||||
should_use_client_metadata_url,
|
||||
union_scopes,
|
||||
validate_authorization_response_iss,
|
||||
validate_metadata_issuer,
|
||||
)
|
||||
|
|
@ -1387,10 +1388,9 @@ class TestAuthFlow:
|
|||
async def capture_redirect(url: str) -> None:
|
||||
nonlocal redirect_captured, captured_state
|
||||
redirect_captured = True
|
||||
# Verify the new scope is included in authorization URL
|
||||
assert "scope=admin%3Awrite+admin%3Adelete" in url or "scope=admin:write+admin:delete" in url.replace(
|
||||
"%3A", ":"
|
||||
).replace("+", " ")
|
||||
# SEP-2350: the authorization URL carries the union of the prior and challenged scopes
|
||||
scope = parse_qs(urlparse(url).query)["scope"][0]
|
||||
assert scope == "read write admin:write admin:delete"
|
||||
# Extract state from redirect URL
|
||||
parsed = urlparse(url)
|
||||
params = parse_qs(parsed.query)
|
||||
|
|
@ -1420,8 +1420,8 @@ class TestAuthFlow:
|
|||
# Trigger step-up - should get token exchange request
|
||||
token_exchange_request = await auth_flow.asend(response_403)
|
||||
|
||||
# Verify scope was updated
|
||||
assert oauth_provider.context.client_metadata.scope == "admin:write admin:delete"
|
||||
# Verify scope was updated to the union of prior and challenged scopes (SEP-2350)
|
||||
assert oauth_provider.context.client_metadata.scope == "read write admin:write admin:delete"
|
||||
assert redirect_captured
|
||||
|
||||
# Complete the flow with successful token response
|
||||
|
|
@ -1447,6 +1447,65 @@ class TestAuthFlow:
|
|||
except StopAsyncIteration:
|
||||
pass # Expected
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_403_step_up_preserves_scope_from_stored_token(
|
||||
self, oauth_provider: OAuthClientProvider, mock_storage: MockTokenStorage
|
||||
):
|
||||
"""SEP-2350: a restart-loaded token's scope is folded into the step-up union.
|
||||
|
||||
On restart only the token is reloaded (not client_metadata.scope), so the stored token's
|
||||
granted scope must seed the union, or the challenge would re-authorize for less.
|
||||
"""
|
||||
client_info = OAuthClientInformationFull(
|
||||
client_id="test_client_id",
|
||||
client_secret="test_client_secret",
|
||||
redirect_uris=[AnyUrl("http://localhost:3030/callback")],
|
||||
)
|
||||
# Simulate a restart: a token granted "read" is loaded, but client_metadata carries no scope.
|
||||
oauth_provider.context.current_tokens = OAuthToken(access_token="t", scope="read")
|
||||
oauth_provider.context.token_expiry_time = time.time() + 1800
|
||||
oauth_provider.context.client_info = client_info
|
||||
oauth_provider.context.client_metadata.scope = None
|
||||
oauth_provider._initialized = True
|
||||
|
||||
captured_state: str | None = None
|
||||
reauthorize_scope: str | None = None
|
||||
|
||||
async def capture_redirect(url: str) -> None:
|
||||
nonlocal captured_state, reauthorize_scope
|
||||
params = parse_qs(urlparse(url).query)
|
||||
reauthorize_scope = params["scope"][0]
|
||||
captured_state = params.get("state", [None])[0]
|
||||
|
||||
async def mock_callback() -> AuthorizationCodeResult:
|
||||
return AuthorizationCodeResult(code="auth_code", state=captured_state)
|
||||
|
||||
oauth_provider.context.redirect_handler = capture_redirect
|
||||
oauth_provider.context.callback_handler = mock_callback
|
||||
|
||||
auth_flow = oauth_provider.async_auth_flow(httpx.Request("GET", "https://api.example.com/mcp"))
|
||||
request = await auth_flow.__anext__()
|
||||
response_403 = httpx.Response(
|
||||
403,
|
||||
headers={"WWW-Authenticate": 'Bearer error="insufficient_scope", scope="write"'},
|
||||
request=request,
|
||||
)
|
||||
token_exchange_request = await auth_flow.asend(response_403)
|
||||
|
||||
assert reauthorize_scope == "read write"
|
||||
|
||||
# Drive the flow to completion so the context lock is released cleanly
|
||||
token_response = httpx.Response(
|
||||
200,
|
||||
json={"access_token": "new", "token_type": "Bearer", "expires_in": 3600, "scope": "read write"},
|
||||
request=token_exchange_request,
|
||||
)
|
||||
final_request = await auth_flow.asend(token_response)
|
||||
try:
|
||||
await auth_flow.asend(httpx.Response(200, request=final_request))
|
||||
except StopAsyncIteration:
|
||||
pass
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
(
|
||||
|
|
@ -2717,3 +2776,20 @@ def test_validate_metadata_issuer_accepts_match():
|
|||
def test_validate_metadata_issuer_rejects_mismatch():
|
||||
with pytest.raises(OAuthFlowError, match="metadata issuer mismatch"):
|
||||
validate_metadata_issuer(_issuer_metadata(issuer="https://attacker.example.com"), _ISSUER)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("previous", "new", "expected"),
|
||||
[
|
||||
pytest.param("mcp:basic", "mcp:write", "mcp:basic mcp:write", id="disjoint-union-order"),
|
||||
pytest.param(
|
||||
"mcp:basic offline_access", "mcp:write mcp:basic", "mcp:basic offline_access mcp:write", id="dedup"
|
||||
),
|
||||
pytest.param(None, "mcp:write", "mcp:write", id="no-previous"),
|
||||
pytest.param("mcp:basic", None, "mcp:basic", id="no-new"),
|
||||
pytest.param(None, None, None, id="both-empty"),
|
||||
],
|
||||
)
|
||||
def test_union_scopes(previous: str | None, new: str | None, expected: str | None):
|
||||
"""SEP-2350: union merges previous and new scopes, dedups, and preserves order."""
|
||||
assert union_scopes(previous, new) == expected
|
||||
|
|
|
|||
|
|
@ -3265,6 +3265,15 @@ REQUIREMENTS: dict[str, Requirement] = {
|
|||
transports=("streamable-http",),
|
||||
note="OAuth is HTTP-only.",
|
||||
),
|
||||
"client-auth:403-scope-union": Requirement(
|
||||
source=f"{SPEC_BASE_URL}/basic/authorization#step-up-authorization-flow",
|
||||
behavior=(
|
||||
"On a 403 insufficient_scope step-up, the re-authorization request carries the union of the "
|
||||
"previously requested scopes and the newly challenged scopes (SEP-2350)."
|
||||
),
|
||||
transports=("streamable-http",),
|
||||
note="OAuth is HTTP-only.",
|
||||
),
|
||||
"client-auth:as-metadata-discovery:priority-order": Requirement(
|
||||
source=f"{SPEC_BASE_URL}/basic/authorization#authorization-server-metadata-discovery",
|
||||
behavior=(
|
||||
|
|
|
|||
|
|
@ -178,6 +178,35 @@ async def test_a_403_insufficient_scope_triggers_one_reauthorize_with_the_challe
|
|||
assert counts[("POST", "/token")] == 2
|
||||
|
||||
|
||||
@requirement("client-auth:403-scope-union")
|
||||
async def test_a_403_step_up_re_authorizes_with_the_union_of_prior_and_challenged_scopes() -> None:
|
||||
"""The step-up re-authorize requests the union of the previously requested and challenged scopes.
|
||||
|
||||
The first authorization requests `mcp`; the 403 challenges a disjoint `write` (not naming
|
||||
`mcp`). Per SEP-2350 the client must re-authorize with `mcp write`, not drop `mcp`. The client
|
||||
is pre-registered with both scopes so the server's authorize handler accepts the wider request.
|
||||
"""
|
||||
provider = InMemoryAuthorizationServerProvider()
|
||||
storage = InMemoryTokenStorage(client_info=seeded_client(provider, scope="mcp write"))
|
||||
server = Server("guarded", on_list_tools=list_tools)
|
||||
settings = auth_settings(required_scopes=["mcp"], valid_scopes=["mcp", "write"])
|
||||
challenge = 'Bearer error="insufficient_scope", scope="write"'
|
||||
|
||||
with anyio.fail_after(5):
|
||||
async with connect_with_oauth(
|
||||
server,
|
||||
provider=provider,
|
||||
storage=storage,
|
||||
settings=settings,
|
||||
app_shim=step_up_shim(challenge),
|
||||
) as (client, headless):
|
||||
await client.list_tools()
|
||||
|
||||
assert len(headless.authorize_urls) == 2
|
||||
assert authorize_params(headless.authorize_urls[0])["scope"] == "mcp"
|
||||
assert authorize_params(headless.authorize_urls[1])["scope"] == "mcp write"
|
||||
|
||||
|
||||
@requirement("client-auth:401-after-auth-throws")
|
||||
async def test_a_second_401_after_a_completed_oauth_flow_surfaces_without_looping() -> None:
|
||||
"""A 401 on the post-auth retry surfaces as an error rather than re-entering discovery.
|
||||
|
|
|
|||
Loading…
Reference in a new issue