diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 88d31a2a..9127e342 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -92,6 +92,8 @@ obo_grant_profile = "entra" # "entra" | "rfc8693" — how tokens are In the admin MCP form, choose **Sign-in passthrough** and set **Audience** (required — the downstream resource the token is minted for, e.g. `api://` on Entra or the client id on Keycloak). The client-id / secret / registration fields do not apply and are hidden. +`oauth_obo` servers are accepted only when **OIDC sign-in is configured and enabled** and `[oidc] obo_grant_profile` is a valid profile — the write is rejected otherwise, since a row that can never mint would surface to users as a permanent "please retry" that never heals. + ### Identity-provider setup **Entra (`obo_grant_profile = "entra"`):** @@ -150,7 +152,7 @@ Additional indicators (circuit-breaker state, encryption-key mismatch) are expos | `none` / `static` → `oauth_user` | — | New code path activates for this server. Existing static headers (if any) are no longer sent. Users must authorize on first use. | | `oauth_user` → `none` / `static` | — | Existing `mcp_user_tokens` rows are **deleted**: the tokens are bound to the auth model + URL active at consent time, and rows left behind could silently rebind if a row with the old name/URL reappears. Switching back to `oauth_user` later starts clean — users re-consent on next use. This is **not reversible**; the AS-side grants are untouched (revoke upstream via the AS if needed). | | OAuth `client_id` or `client_secret` rotated | — | Existing tokens may stop refreshing if the AS treats them as bound to the previous client. Bulk-revoke after rotation. | -| `oauth_user` ↔ `oauth_obo` | — | The per-user rows are **deleted** on the flip (they mean different things: per-server AS refresh tokens vs. minted cache). A flip into `oauth_obo` clears carried-over `oauth_scopes` under the `entra` profile (they cannot apply there); under `rfc8693` a scopes value submitted with the flip is kept as the token-exchange scope and the column is cleared only when the request omits it. A flip into `oauth_user` clears the obo-era `oauth_audience` (an IdP-side app identifier, not the resource indicator `oauth_user` needs) unless the request sets a new one. | +| `oauth_user` ↔ `oauth_obo` | — | The per-user rows are **deleted** on the flip (they mean different things: per-server AS refresh tokens vs. minted cache). `oauth_audience` and `oauth_scopes` mean different things in each model (a resource indicator vs. an IdP app identifier; AS-consent scopes vs. an rfc8693 exchange scope), so on a flip they **never carry** — each is taken from the request for the target model or set NULL. The admin console clears these fields when you change the auth type, so re-enter the correct values for the new mode; via the API, supply them explicitly (a flip into `oauth_obo` with no `oauth_audience` is rejected, and a non-empty `oauth_scopes` under the `entra` profile is rejected since that leg pins `/.default`). | | `oauth_obo` → `none` / `static` | — | Minted cache rows are deleted. | | `oauth_obo` **audience**, **URL**, or **`oauth_scopes`** changed | — | Minted cache rows are **deleted** (tokens are bound to the audience/URL/scopes at mint time), forcing a fresh mint — so an audience or scope narrowing takes effect immediately, not at token expiry. | diff --git a/tests/test_app_js.py b/tests/test_app_js.py index cd2df7c0..ae4c2e73 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -876,6 +876,30 @@ def test_phase8_xss_safe_render_in_build_mcp_error_embed() -> None: ) +def test_mcp_error_button_gated_on_consent_url_not_code_alone() -> None: + """Review finding: the chat error card rendered a Connect / Re-consent + button from the error CODE alone, so an oauth_obo error (consent_url=None, + since sign-in passthrough has no per-server consent flow and /start rejects + obo rows) produced a button that dead-ended in a 'no consent URL' toast. + The button must render only when a valid per-server consent URL is present — + obo errors show the card's honest detail text without a broken affordance.""" + body = _INTERACTIVE_JS.read_text(encoding="utf-8") + start = body.index("function buildMcpErrorEmbed(") + rest = body[start:] + end_match = re.search(r"\n}\n", rest) + assert end_match is not None + fn = rest[: end_match.end()] + # The render gate combines the category with a consent-URL presence check. + assert "hasConsentAffordance" in fn, ( + "buildMcpErrorEmbed must gate the action button on the presence of a " + "consent URL, not on the error category alone." + ) + assert 'category === "actionable" && hasConsentAffordance' in fn, ( + "the button-render condition must require BOTH an actionable category " + "and a real consent URL" + ) + + def test_phase8_css_classes_present_in_stylesheet() -> None: """The MCP error-embed + connections classes app.js/interactive.js reference must keep their CSS rules (else the consent / connections UX silently loses diff --git a/tests/test_mcp_admin_api.py b/tests/test_mcp_admin_api.py index 374f550f..fe226e6a 100644 --- a/tests/test_mcp_admin_api.py +++ b/tests/test_mcp_admin_api.py @@ -5,6 +5,7 @@ from __future__ import annotations import base64 import json import uuid +from types import SimpleNamespace from typing import TYPE_CHECKING, Any from unittest.mock import AsyncMock, MagicMock, patch @@ -191,6 +192,19 @@ def _install_token_store(app, storage) -> None: ) +def _enabled_oidc(profile: str = "entra") -> SimpleNamespace: + """An OIDC config that satisfies the oauth_obo write-time gate. + + oauth_obo mints from the user's captured sign-in, so the write choke point + requires OIDC enabled + a valid ``obo_grant_profile``. Tests exercising obo + writes install one of these; the finding-C tests install a disabled / + bad-profile config instead to assert the rejection. + """ + return SimpleNamespace( + enabled=True, issuer="https://idp.example.com", obo_grant_profile=profile + ) + + @pytest.fixture def client(storage): """TestClient wired to console admin MCP endpoints with full permissions.""" @@ -200,6 +214,10 @@ def client(storage): ) app.state.auth_storage = storage _install_token_store(app, storage) + # Default: OIDC enabled under the entra profile so oauth_obo writes pass the + # requirement gate. Per-test overrides install rfc8693 / disabled / bad + # profile as needed. + app.state.oidc_config = _enabled_oidc("entra") return TestClient(app) @@ -746,6 +764,68 @@ class TestUpdateMcpServer: assert data["auth_type"] == "oauth_obo" assert data["oauth_audience"] == "api://mcp-a" + def test_create_obo_rejected_when_oidc_disabled(self, client): + """Review finding: oauth_obo mints from the user's OIDC sign-in, so an + install with OIDC disabled can NEVER mint. Reject at write time (a + permanent misconfig otherwise surfaces per-dispatch as a retryable + transient that never heals).""" + client.app.state.oidc_config = SimpleNamespace(enabled=False, obo_grant_profile="entra") + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "obo-no-oidc", + "transport": "streamable-http", + "url": "https://mcp.example.com/sse", + "auth_type": "oauth_obo", + "oauth_audience": "api://mcp-a", + }, + ) + assert r.status_code == 400, r.text + assert "OIDC" in r.json()["error"] + + def test_create_obo_rejected_on_invalid_grant_profile(self, client): + """Review finding: a typo'd deployment obo_grant_profile leaves the mint + leg unresolved (obo_misconfigured per dispatch), so reject it at the + write choke point rather than as a runtime transient.""" + client.app.state.oidc_config = _enabled_oidc("bogus-profile") + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "obo-bad-profile", + "transport": "streamable-http", + "url": "https://mcp.example.com/sse", + "auth_type": "oauth_obo", + "oauth_audience": "api://mcp-a", + }, + ) + assert r.status_code == 400, r.text + assert "obo_grant_profile" in r.json()["error"] + + def test_flip_user_to_obo_via_api_without_audience_is_rejected(self, client, storage): + """Review finding: a flip into obo must NOT carry the oauth_user-era + oauth_audience (a resource indicator, conventionally the MCP URL) — it + would pass the audience-required check and then fail every mint. An API + PUT of just {auth_type: oauth_obo} recomputes audience from the body + (absent → NULL) and is rejected loudly, not saved with the stale value.""" + r = client.post( + "/v1/api/admin/mcp-servers", + json={ + "name": "flip-api-noaud", + "transport": "streamable-http", + "url": "https://mcp.example.com/sse", + "auth_type": "oauth_user", + "oauth_client_id": "cli_x", + "oauth_audience": "https://mcp.example.com/sse", # resource indicator + }, + ) + sid = r.json()["server_id"] + r2 = client.put( + f"/v1/api/admin/mcp-servers/{sid}", + json={"auth_type": "oauth_obo"}, # no audience in body + ) + assert r2.status_code == 400, r2.text + assert "oauth_audience" in r2.json()["error"] + def test_update_flip_oauth_user_to_obo_keeps_audience_and_purges_tokens(self, client, storage): """#551 (findings 10344 + 10326): flipping oauth_user→oauth_obo must NOT null oauth_audience (the mint engine needs it), and MUST purge the old @@ -844,17 +924,17 @@ class TestUpdateMcpServer: sid: str = r.json()["server_id"] return sid - def test_flip_to_obo_resent_scopes_cleared_under_entra(self, client): - """Review finding: on a flip into obo the carried-over oauth_user - scopes are profile-dependent. Under entra they can never apply (the - leg pins /.default), so an equal-value re-send is treated - as the stale pre-fill and cleared — NOT rejected (the flip must not - 400 on a value the operator never typed).""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="entra") + def test_flip_to_obo_under_entra_rejects_explicit_scopes_but_omit_clears(self, client): + """Redesign: a flip into obo recomputes scopes from the body (never + carries the old row's value across the semantic boundary). Under entra, + an EXPLICIT non-empty scopes value is rejected 400 — an honest visible + snap rather than a silent drop — while the console-realistic flip (the + form clears the semantic field on the auth-type switch, so scopes is + omitted/empty) succeeds with scopes NULL.""" + client.app.state.oidc_config = _enabled_oidc("entra") + # Explicit non-empty scopes on the flip → 400 (they can't apply on entra). sid = self._create_oauth_user_row_with_scopes(client, "flip-resend-entra") - r2 = client.put( + rejected = client.put( f"/v1/api/admin/mcp-servers/{sid}", json={ "auth_type": "oauth_obo", @@ -862,8 +942,16 @@ class TestUpdateMcpServer: "oauth_scopes": "openid profile offline_access", }, ) - assert r2.status_code == 200, r2.text - assert r2.json()["oauth_scopes"] in (None, "") # carried-over value cleared + assert rejected.status_code == 400 + assert "entra" in rejected.json()["error"] + # The realistic flip (scopes field cleared → omitted) succeeds, NULL scopes. + sid2 = self._create_oauth_user_row_with_scopes(client, "flip-omit-entra") + ok = client.put( + f"/v1/api/admin/mcp-servers/{sid2}", + json={"auth_type": "oauth_obo", "oauth_audience": "api://mcp-a"}, + ) + assert ok.status_code == 200, ok.text + assert ok.json()["oauth_scopes"] in (None, "") # not carried across the flip def test_flip_to_obo_resent_scopes_kept_under_rfc8693(self, client): """Review finding: under rfc8693 oauth_scopes IS the token-exchange @@ -871,9 +959,7 @@ class TestUpdateMcpServer: Keycloak optional-audience scope can legitimately equal the old consent scope string) must NOT have it silently nulled; only an omitted field clears (previous test).""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="rfc8693") + client.app.state.oidc_config = _enabled_oidc("rfc8693") sid = self._create_oauth_user_row_with_scopes(client, "flip-resend-rfc") r2 = client.put( f"/v1/api/admin/mcp-servers/{sid}", @@ -891,8 +977,6 @@ class TestUpdateMcpServer: entra profile must stay editable — an unrelated PUT that doesn't touch scopes must NOT be rejected (the entra-scope reject fires only on a real scopes write).""" - from types import SimpleNamespace - # Seed an obo row that already has scopes (e.g. created under rfc8693). storage.create_mcp_server( server_id="entra-edit-id", @@ -904,7 +988,7 @@ class TestUpdateMcpServer: oauth_scopes="custom.scope", ) # Now the deployment is on the entra profile. - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="entra") + client.app.state.oidc_config = _enabled_oidc("entra") # An unrelated maintenance edit (disable) — does NOT touch scopes. r = client.put( @@ -946,8 +1030,6 @@ class TestUpdateMcpServer: ) # The list handler fans out node status; no cluster nodes in this test. - from types import SimpleNamespace - client.app.state.collector = SimpleNamespace(get_all_nodes=lambda: []) client.app.state.proxy_client = MagicMock() r = client.get("/v1/api/admin/mcp-servers") @@ -1058,9 +1140,7 @@ class TestUpdateMcpServer: bearer's privileges exactly like the audience does — narrowing oauth_scopes must purge cached rows or the reduction silently waits out the token TTL (inconsistent with the audience purge).""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="rfc8693") + client.app.state.oidc_config = _enabled_oidc("rfc8693") sid = self._seed_obo_row_with_cache( client, storage, name="scope-change", scopes="api.read api.write" ) @@ -1077,9 +1157,7 @@ class TestUpdateMcpServer: """Review finding companion: the admin form re-submits the pre-filled scopes on every save — an EQUAL value is normalized out of the update and must not flush every user's minted tokens.""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="rfc8693") + client.app.state.oidc_config = _enabled_oidc("rfc8693") sid = self._seed_obo_row_with_cache(client, storage, name="scope-noop", scopes="api.read") r2 = client.put( f"/v1/api/admin/mcp-servers/{sid}", @@ -1096,9 +1174,7 @@ class TestUpdateMcpServer: carried over, every consent yields a wrong-resource token that 401s with no visible cause. The flip must clear it (and the rfc8693 exchange scopes) unless the request explicitly sets new values.""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="rfc8693") + client.app.state.oidc_config = _enabled_oidc("rfc8693") sid = self._seed_obo_row_with_cache(client, storage, name="flip-back", scopes="api.read") r2 = client.put( f"/v1/api/admin/mcp-servers/{sid}", @@ -1117,16 +1193,14 @@ class TestUpdateMcpServer: oauth_scopes, so a same-type edit of an entra-profile obo row carrying legacy scopes must accept an EQUAL value (normalized to a no-op) instead of 400ing — only a genuine scope CHANGE is rejected.""" - from types import SimpleNamespace - # The legacy-scoped entra row arises from a deployment profile switch: # the row is created while the profile is rfc8693 (scopes accepted), # then the deployment flips to entra. - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="rfc8693") + client.app.state.oidc_config = _enabled_oidc("rfc8693") sid = self._seed_obo_row_with_cache( client, storage, name="entra-resend", scopes="legacy.scope" ) - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="entra") + client.app.state.oidc_config = _enabled_oidc("entra") # Equal re-send + unrelated change → accepted, scopes untouched. r2 = client.put( f"/v1/api/admin/mcp-servers/{sid}", @@ -1146,9 +1220,7 @@ class TestUpdateMcpServer: """#551 follow-up: oauth_scopes is meaningless for the entra grant leg (it mints /.default), so the write path rejects it rather than silently ignoring it at mint time.""" - from types import SimpleNamespace - - client.app.state.oidc_config = SimpleNamespace(obo_grant_profile="entra") + client.app.state.oidc_config = _enabled_oidc("entra") r = client.post( "/v1/api/admin/mcp-servers", json={ diff --git a/tests/test_mcp_obo_mint.py b/tests/test_mcp_obo_mint.py index 34b1d8b6..3dcdf7e5 100644 --- a/tests/test_mcp_obo_mint.py +++ b/tests/test_mcp_obo_mint.py @@ -545,6 +545,47 @@ class TestCacheAndCredentialLookup: row = storage.get_mcp_user_token(USER, SERVER) assert row is not None and row["audience"] == AUDIENCE + def test_stale_scopes_cache_row_is_not_served_and_remints(self, storage: SQLiteBackend) -> None: + """Review finding: the read-side freshness gate is the AUTHORITATIVE + enforcement of a scope narrowing (the admin cache purge is best-effort). + Under rfc8693 a row minted with the OLD, wider scopes must NOT be served + after the server's scopes are narrowed — even if the purge failed — so + the privilege reduction takes effect on the next dispatch, not at TTL.""" + _seed_obo_server(storage, oauth_scopes="api.read") # server's CURRENT scopes + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock( + side_effect=[ + _mk_response(200, {"access_token": "subject-at", "expires_in": 300}), + _mk_response(200, {"access_token": "reminted-narrow", "expires_in": 3600}), + ] + ) + state = _make_app_state( + storage, + http_client=client, + oidc_config=_make_oidc_config(obo_grant_profile="rfc8693"), + ) + _seed_credential(state) + # Fresh, right-audience, refresh-less — but minted with the OLD wider scopes. + state.mcp_token_store.create_user_token( + USER, + SERVER, + access_token="wide-scope-at", + refresh_token=None, + expires_at=(datetime.now(UTC) + timedelta(seconds=3600)).strftime(_ISO), + scopes="api.read api.write", # wider than the server's current api.read + as_issuer=ISSUER, + audience=AUDIENCE, + ) + + result = _mint(state) + + # The wider-scope row is NOT served; a fresh mint for the current scopes runs. + assert result.kind == "token" + assert result.token == "reminted-narrow" + assert client.post.call_count == 2 + row = storage.get_mcp_user_token(USER, SERVER) + assert row is not None and (row["scopes"] or "") == "api.read" + def test_missing_credential_returns_missing_with_zero_http_calls( self, storage: SQLiteBackend ) -> None: @@ -683,12 +724,14 @@ class TestFailureHandling: assert "obo_mint_rejected" in blob assert "AADSTS65001" in blob # the actual IdP error text survives - def test_permanent_rejection_arms_cooldown_terminal_state(self, storage: SQLiteBackend) -> None: - """Review finding (2156): the obo permanent-rejection arm must arm the - cooldown, else — because the credential survives and a missing cache row - is mint-eligible — every subsequent dispatch re-runs the doomed - redemption and emits another token_revoked audit row forever. A second - immediate dispatch must NOT re-hit the IdP or re-audit.""" + def test_permanent_rejection_no_cache_row_emits_no_revoke_audit( + self, storage: SQLiteBackend + ) -> None: + """Review finding: a permanent mint rejection with NO cache row (the + common missing-tenant-grant case — the user never had a token for this + server) must NOT emit a token_revoked audit for a row that never + existed. The cooldown is still armed as the terminal backstop (the + credential survives), so the second dispatch short-circuits.""" _seed_obo_server(storage) client = MagicMock(spec=httpx.AsyncClient) client.post = AsyncMock( @@ -712,10 +755,43 @@ class TestFailureHandling: # Terminal: the cooldown short-circuits the second dispatch entirely. assert second.kind == "refresh_failed_transient" assert client.post.call_count == 1 # NOT re-minted - # Exactly one revoke audit — not one per dispatch. + # No row was ever deleted → no bogus revoke audit. + events = storage.list_audit_events(action="mcp_server.oauth.token_revoked") + assert len(events) == 0 + assert state.mcp_token_store.get_oidc_credential(USER, ISSUER) is not None + + def test_permanent_rejection_with_cache_row_audits_exactly_once( + self, storage: SQLiteBackend + ) -> None: + """Companion: when a cache row DID exist, the permanent rejection deletes + it and audits token_revoked exactly ONCE. A later doomed re-mint (past + the cooldown) finds no row to delete and must NOT append a second audit + row — the crux of the audit-spam finding.""" + _seed_obo_server(storage) + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock( + return_value=_mk_response(400, {"error": "invalid_grant", "error_description": "dead"}) + ) + state = _make_app_state(storage, http_client=client, oidc_config=_make_oidc_config()) + _seed_credential(state) + _seed_cache_row(state, expires_in_seconds=-1000, access_token="stale-at") # forces a mint + + from turnstone.core.mcp_oauth import _clear_refresh_backoff + + async def _run() -> None: + # First dispatch: deletes the (stale) cache row + audits once. + await get_obo_access_token_classified(app_state=state, user_id=USER, server_name=SERVER) + # Clear the cooldown so the second dispatch actually re-mints (the + # weekend-of-scheduled-runs scenario), then dispatch again. + _clear_refresh_backoff(state, USER, SERVER) + await get_obo_access_token_classified(app_state=state, user_id=USER, server_name=SERVER) + + asyncio.run(_run()) + + assert client.post.call_count == 2 # re-minted after the cooldown cleared + # But only ONE revoke audit — the second doomed mint found no row. events = storage.list_audit_events(action="mcp_server.oauth.token_revoked") assert len(events) == 1 - assert state.mcp_token_store.get_oidc_credential(USER, ISSUER) is not None def test_force_refresh_during_cooldown_falls_through_on_fresh_cache( self, storage: SQLiteBackend diff --git a/tests/test_mcp_user_pool.py b/tests/test_mcp_user_pool.py index 94155cc3..01e2af68 100644 --- a/tests/test_mcp_user_pool.py +++ b/tests/test_mcp_user_pool.py @@ -1187,6 +1187,22 @@ class TestTokenRejectedDetail: assert "consent" not in obo_detail.lower() assert "administrator" in obo_detail.lower() + def test_obo_insufficient_scope_detail_points_at_admin_not_reconsent(self) -> None: + """Review finding: the 403 insufficient-scope branch was the one + user-actionable site not made obo-aware — it told obo users to + 'Re-consent with new scopes' though no per-server consent flow exists + (consent_url is None, /start rejects obo). The obo detail names the real + remedy (an administrator widening access), the oauth_user one keeps the + step-up re-consent language.""" + from turnstone.core.mcp_client import _insufficient_scope_detail + + user_detail = _insufficient_scope_detail({"auth_type": "oauth_user"}, "tool") + assert "Re-consent" in user_detail + + obo_detail = _insufficient_scope_detail({"auth_type": "oauth_obo"}, "tool") + assert "consent" not in obo_detail.lower() + assert "administrator" in obo_detail.lower() + # --------------------------------------------------------------------------- # Background token-freshness sweep (oauth_user keep-hot, no connection warming) diff --git a/turnstone/console/server.py b/turnstone/console/server.py index d6305e8e..ca5b5f49 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -9802,6 +9802,47 @@ def _enforce_oauth_user_https(auth_type: str, url: str | None) -> JSONResponse | return None +def _obo_profile(request: Request) -> str: + """Deployment-level ``[oidc] obo_grant_profile`` (``""`` when OIDC absent). + + One reader for the profile so the create handler, the update handler, and + the write-time enforcement never diverge on how they resolve it. + """ + oidc_config = getattr(request.app.state, "oidc_config", None) + return str(getattr(oidc_config, "obo_grant_profile", "") or "") + + +def _oauth_columns_to_clear(auth_type: str | None) -> dict[str, None]: + """OAuth columns to force-NULL for a row of *auth_type* (shared write policy). + + Single source of the "which OAuth columns does this auth model use" policy, + consumed by both the create and update handlers so they cannot drift. + ``oauth_user`` uses every column (nothing cleared); ``oauth_obo`` uses + ``oauth_audience`` (+ ``oauth_scopes`` under rfc8693) but none of the + oauth_user-only columns; ``static``/``none`` use no OAuth columns at all. + ``oauth_client_secret_ct`` is owned by a dedicated write path and is not + included here. Callers that cannot persist ``oauth_as_issuer_cached`` (the + create path) drop that key. + """ + if auth_type == "oauth_user": + return {} + if auth_type == "oauth_obo": + return { + "oauth_client_id": None, + "oauth_registration_mode": None, + "oauth_authorization_server_url": None, + "oauth_as_issuer_cached": None, + } + return { # static / none + "oauth_client_id": None, + "oauth_scopes": None, + "oauth_audience": None, + "oauth_registration_mode": None, + "oauth_authorization_server_url": None, + "oauth_as_issuer_cached": None, + } + + def _enforce_oauth_obo_requirements( request: Request, auth_type: str, *, audience: str | None, scopes: str | None ) -> JSONResponse | None: @@ -9814,6 +9855,14 @@ def _enforce_oauth_obo_requirements( per-user mint-cache rows AND makes ``initialize_mcp_crypto_state`` ``SystemExit(1)`` at the next restart, so accepting one keyless plants a deferred whole-cluster boot failure. + - **OIDC not enabled / no captured-credential source** → 400. The mint + redeems the user's captured IdP credential, so an install with no + ``[oidc]`` issuer (or OIDC operator-disabled) can NEVER mint — every tool + call would return ``mcp_refresh_unavailable`` ("please retry") forever, a + permanent misconfig dressed as a transient. Reject at write time. + - **Invalid ``obo_grant_profile``** → 400. A typo'd deployment profile + leaves the mint leg unresolved (``obo_misconfigured`` per dispatch), so + surface it here rather than as a runtime transient that never heals. - **No ``oauth_audience``** → 400. The mint engine hard-requires the downstream audience; without it every tool call fails transient and logs ``obo_misconfigured``. Reject here, exactly as bad URL schemes are. @@ -9827,6 +9876,33 @@ def _enforce_oauth_obo_requirements( return None if getattr(request.app.state, "mcp_token_store", None) is None: return JSONResponse({"error": _OAUTH_TOKEN_STORE_503_MSG}, status_code=503) + oidc_config = getattr(request.app.state, "oidc_config", None) + if oidc_config is None or not getattr(oidc_config, "enabled", False): + return JSONResponse( + { + "error": ( + "auth_type=oauth_obo requires OIDC sign-in to be configured and " + "enabled (it mints per-server tokens from each user's Turnstone " + "sign-in). Configure [oidc] and capture_user_credential, or use " + "auth_type=oauth_user for per-server consent." + ) + }, + status_code=400, + ) + profile = _obo_profile(request) + from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES + + if profile not in OBO_GRANT_PROFILES: + return JSONResponse( + { + "error": ( + f"[oidc] obo_grant_profile={profile!r} is not a valid grant profile " + f"({', '.join(sorted(OBO_GRANT_PROFILES))}); fix the deployment " + "config before configuring oauth_obo servers" + ) + }, + status_code=400, + ) if not (audience or "").strip(): return JSONResponse( { @@ -9837,20 +9913,17 @@ def _enforce_oauth_obo_requirements( }, status_code=400, ) - if (scopes or "").strip(): - oidc_config = getattr(request.app.state, "oidc_config", None) - profile = str(getattr(oidc_config, "obo_grant_profile", "") or "") - if profile == "entra": - return JSONResponse( - { - "error": ( - "oauth_scopes is not used by the entra grant profile " - "(it mints /.default); leave it empty or switch " - "the deployment to the rfc8693 profile" - ) - }, - status_code=400, - ) + if (scopes or "").strip() and profile == "entra": + return JSONResponse( + { + "error": ( + "oauth_scopes is not used by the entra grant profile " + "(it mints /.default); leave it empty or switch " + "the deployment to the rfc8693 profile" + ) + }, + status_code=400, + ) return None @@ -10272,30 +10345,23 @@ async def admin_create_mcp_server(request: Request) -> JSONResponse: headers_dict = body.get("headers", {}) env_dict = body.get("env", {}) - # Column policy — mirror of the update handler's clearing rules: persist - # only the OAuth columns the target auth_type actually uses, so a later - # auth_type flip can't resurrect values the operator never intended for - # the new mode (the update path clears on TRANSITION; without the same - # policy here, a row CREATED as oauth_obo could carry a client_id / - # registration_mode / AS-URL straight into a later oauth_user flip). - oauth_client_id = _clean_oauth_text(body.get("oauth_client_id")) - oauth_scopes = _clean_oauth_text(body.get("oauth_scopes")) - oauth_audience = _clean_oauth_text(body.get("oauth_audience"), max_length=2048) - oauth_registration_mode = _clean_oauth_text(body.get("oauth_registration_mode")) - oauth_as_url = _clean_oauth_text(body.get("oauth_authorization_server_url"), max_length=2048) - if auth_type == "oauth_obo": - # obo uses audience (+ scopes under rfc8693) but none of the - # oauth_user-only columns. - oauth_client_id = None - oauth_registration_mode = None - oauth_as_url = None - elif auth_type != "oauth_user": - # static/none use no OAuth columns at all. - oauth_client_id = None - oauth_scopes = None - oauth_audience = None - oauth_registration_mode = None - oauth_as_url = None + # Column policy — the SAME ``_oauth_columns_to_clear`` rule the update + # handler applies on a flip: persist only the OAuth columns the target + # auth_type actually uses, so a row created as oauth_obo can't carry a + # client_id / registration_mode / AS-URL straight into a later oauth_user + # flip. (``oauth_as_issuer_cached`` is discovery-owned and not a create + # parameter, so it is dropped from the cleared set here.) + oauth_cols: dict[str, Any] = { + "oauth_client_id": _clean_oauth_text(body.get("oauth_client_id")), + "oauth_scopes": _clean_oauth_text(body.get("oauth_scopes")), + "oauth_audience": _clean_oauth_text(body.get("oauth_audience"), max_length=2048), + "oauth_registration_mode": _clean_oauth_text(body.get("oauth_registration_mode")), + "oauth_authorization_server_url": _clean_oauth_text( + body.get("oauth_authorization_server_url"), max_length=2048 + ), + } + oauth_cols.update(_oauth_columns_to_clear(auth_type)) + oauth_cols.pop("oauth_as_issuer_cached", None) storage.create_mcp_server( server_id=server_id, @@ -10310,11 +10376,7 @@ async def admin_create_mcp_server(request: Request) -> JSONResponse: enabled=bool(body.get("enabled", True)), created_by=audit_uid, auth_type=auth_type, - oauth_client_id=oauth_client_id, - oauth_scopes=oauth_scopes, - oauth_audience=oauth_audience, - oauth_registration_mode=oauth_registration_mode, - oauth_authorization_server_url=oauth_as_url, + **oauth_cols, ) # Encrypt + persist the operator-supplied client secret via the dedicated @@ -10467,87 +10529,44 @@ async def admin_update_mcp_server(request: Request) -> JSONResponse: old_auth = existing.get("auth_type") new_auth = updates.get("auth_type") + # A flip is any auth_type transition. ``oauth_audience`` and ``oauth_scopes`` + # keep their meaning only WITHIN an auth type — across oauth_user↔oauth_obo + # the audience is a resource indicator vs. an IdP-side app identifier, and + # the scopes are AS-consent scopes vs. an rfc8693 exchange scope — so on a + # flip they must NEVER carry from the old row; they are recomputed from the + # request body (or NULLed) for the target type below. + is_flip = new_auth is not None and new_auth != old_auth - # A request that re-sends oauth_scopes / oauth_audience equal to the row's - # current value is a no-op — drop it from ``updates``. The admin form - # pre-fills both fields and submits them on every save, and three rules - # below key on "this request SETS the column": the entra-profile scope - # rejection (without this, a pre-existing scoped row under the entra - # profile is un-editable — every unrelated save 400s), the mint-cache - # purge triggers (a no-op re-send must not flush every user's cache), and - # the flip-into-oauth_user clearing of obo-era values (a re-sent stale - # pre-fill must not masquerade as an explicit set). - for _noop_key in ("oauth_scopes", "oauth_audience"): - if _noop_key in updates and (updates[_noop_key] or None) == ( - existing.get(_noop_key) or None - ): - del updates[_noop_key] + if not is_flip: + # SAME-TYPE edit: a re-send of oauth_scopes / oauth_audience equal to the + # row's current value is a genuine no-op — drop it from ``updates`` so the + # admin form's unconditional re-send of pre-filled fields doesn't (a) + # trip the mint-cache purge triggers or (b) trip the entra scope + # rejection that would otherwise make a legacy-scoped row un-editable. + # (On a flip this comparison is meaningless — the old value is in the + # other auth model's semantics — so it is skipped.) + for _noop_key in ("oauth_scopes", "oauth_audience"): + if _noop_key in updates and (updates[_noop_key] or None) == ( + existing.get(_noop_key) or None + ): + del updates[_noop_key] - # Clear the OAuth columns the target auth_type does NOT use, so a stale - # client_id / audience can't leak back on a later flip. static/none use - # none; oauth_obo uses oauth_audience (+ oauth_scopes) but none of the - # oauth_user-only columns; oauth_user uses client_id / registration / - # AS-URL / scopes plus an audience with DIFFERENT semantics (RFC 8707 - # resource indicator, not the obo IdP-side app identifier). - # ``oauth_client_secret_ct`` is owned by a dedicated write path (see below). - if new_auth and not is_user_scoped_auth(new_auth): - # → static/none: clear every OAuth column. - updates.update( - { - "oauth_client_id": None, - "oauth_scopes": None, - "oauth_audience": None, - "oauth_registration_mode": None, - "oauth_authorization_server_url": None, - "oauth_as_issuer_cached": None, - } - ) - elif new_auth == "oauth_obo": - # obo keeps audience (it needs it); clear the oauth_user-only columns. - updates.update( - { - "oauth_client_id": None, - "oauth_registration_mode": None, - "oauth_authorization_server_url": None, - "oauth_as_issuer_cached": None, - } - ) - # A FLIP into obo carries oauth_user's AS-consent scopes, which mean - # something different in each grant profile: - # entra — scopes are never used (the leg pins /.default), - # so clear the carry-over unless this request typed a - # genuinely NEW value (which the enforce step below then - # rejects loudly rather than silently discarding); - # rfc8693 — scopes ARE the token-exchange scope, so honor the - # request verbatim: a value equal to the old consent - # scopes is a deliberate keep (the operator sees the - # field on the flip form), and only an OMITTED field - # clears the oauth_user consent scopes (different - # semantics under the new auth model). - if old_auth != "oauth_obo": - profile = str( - getattr(getattr(request.app.state, "oidc_config", None), "obo_grant_profile", "") - or "" - ) - if profile == "entra": - if "oauth_scopes" not in updates: - updates["oauth_scopes"] = None - elif "oauth_scopes" not in body: - updates["oauth_scopes"] = None - elif new_auth == "oauth_user" and old_auth != "oauth_user": - # Flipping INTO oauth_user: the obo-era oauth_audience is an IdP-side - # app identifier (api:// on Entra, a client id on Keycloak), NOT - # the RFC 8707 resource indicator oauth_user passes to its per-server - # AS — carried over, build_authorize_url requests (and the token - # validator accepts) a wrong-resource token that 401s on every - # dispatch with no visible cause. Same for rfc8693 exchange scopes - # leaking into consent scopes. Clear both unless this request - # explicitly sets them. (On main this was structurally impossible — - # any non-oauth_user row had every OAuth column nulled.) - if "oauth_audience" not in updates: - updates["oauth_audience"] = None - if "oauth_scopes" not in updates and "oauth_scopes" not in body: - updates["oauth_scopes"] = None + # Clear the OAuth columns the target auth_type does NOT use (shared + # ``_oauth_columns_to_clear`` policy). ``oauth_client_secret_ct`` is owned by + # a dedicated write path (see below). + if is_flip: + target_auth = new_auth # non-None: is_flip requires new_auth is not None + if is_user_scoped_auth(target_auth): + # FLIP into a user-scoped type: recompute the semantic columns from + # the body — present → that value, absent → NULL — so neither the + # old audience nor the old scopes can leak across the semantic + # boundary. The top-of-handler loop already copied any body value + # into ``updates``; forcing the key present here NULLs it when the + # body omitted it (e.g. an API PUT of just {"auth_type": ...}, or the + # console form clearing the field on the auth-type switch). + updates["oauth_audience"] = updates.get("oauth_audience") + updates["oauth_scopes"] = updates.get("oauth_scopes") + updates.update(_oauth_columns_to_clear(target_auth)) # Renaming a pool-backed row needs the same per-user-token purge as # delete: the tokens are keyed on the OLD ``server_name`` and a row # later created with that old name (with attacker-controlled URL) diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index 3face7d7..e5d5100b 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -5120,16 +5120,17 @@ function _wireMcpAudienceAutofill() { } function _onMcpAuthTypeChange() { - // Switching INTO sign-in passthrough: if the audience field still holds - // the URL-autofill artifact (operator typed the URL first, then picked - // this mode), blank it so the required-field check asks for a real - // identity-provider application identifier instead of silently - // persisting the MCP URL as the audience. - if (_selectedMcpAuthType() === "oauth_obo") { - const aud = document.getElementById("mcp-oauth-audience"); - const urlVal = document.getElementById("mcp-url").value.trim(); - if (aud && urlVal && aud.value.trim() === urlVal) aud.value = ""; - } + // Audience and Scopes are auth-type-specific: for oauth_user the audience is + // an RFC 8707 resource indicator (~ the MCP URL) and scopes are AS-consent + // scopes; for sign-in passthrough the audience is the identity-provider-side + // application identifier and scopes (rfc8693 only) are the token-exchange + // scope. Carrying one type's value into the other passes validation and then + // fails every mint/consent, so clear both when the auth type changes — the + // operator re-enters the correct values for the new mode (the backend + // likewise refuses to carry these columns across a flip). A same-type edit + // never fires this (the radio didn't change), so pre-filled values are kept. + document.getElementById("mcp-oauth-audience").value = ""; + document.getElementById("mcp-oauth-scopes").value = ""; toggleMcpAuthFields(); } @@ -5179,11 +5180,7 @@ function _mcpResetForm() { document.getElementById("mcp-oauth-registration").value = "preregistered"; document.getElementById("mcp-oauth-client-id").value = ""; document.getElementById("mcp-oauth-client-secret").value = ""; - const scopesReset = document.getElementById("mcp-oauth-scopes"); - scopesReset.value = ""; - // Create flow has no loaded row — the marker's absence tells - // _parseMcpForm to always include the field. - delete scopesReset.dataset.loaded; + document.getElementById("mcp-oauth-scopes").value = ""; document.getElementById("mcp-oauth-audience").value = ""; document.getElementById("mcp-create-error").classList.remove("is-visible"); toggleMcpTransport(); @@ -5256,12 +5253,7 @@ function showEditMcpModal(serverId) { s.oauth_client_id || ""; // Secret field always blank — write-only, never read back. document.getElementById("mcp-oauth-client-secret").value = ""; - const scopesInput = document.getElementById("mcp-oauth-scopes"); - scopesInput.value = s.oauth_scopes || ""; - // Remember the loaded value so _parseMcpForm can omit an untouched - // pre-fill from the payload (the server treats a present field as an - // operator-set value). - scopesInput.dataset.loaded = s.oauth_scopes || ""; + document.getElementById("mcp-oauth-scopes").value = s.oauth_scopes || ""; document.getElementById("mcp-oauth-audience").value = s.oauth_audience || ""; toggleMcpTransport(); @@ -5364,18 +5356,13 @@ function _parseMcpForm() { if (!audience) return { error: "Audience is required for sign-in passthrough servers" }; payload.oauth_audience = audience; - // Include scopes only when the operator changed them from the loaded row - // value (or on create, where no loaded marker exists): the server treats - // a present field as an operator-set value, and re-submitting the - // untouched pre-fill on every save must not read as one. - const scopesInput = document.getElementById("mcp-oauth-scopes"); - const scopesVal = scopesInput.value.trim(); - if ( - scopesInput.dataset.loaded === undefined || - scopesVal !== scopesInput.dataset.loaded.trim() - ) { - payload.oauth_scopes = scopesVal; - } + // Always send the visible Scopes value — the backend distinguishes a + // same-type no-op re-send (dropped) from a genuine change / flip on its + // side, so the form doesn't need omit-when-unchanged logic (which used to + // collide with the backend's flip handling and silently drop scopes). + payload.oauth_scopes = document + .getElementById("mcp-oauth-scopes") + .value.trim(); } return payload; diff --git a/turnstone/core/mcp_client.py b/turnstone/core/mcp_client.py index 9926f053..b2d43703 100644 --- a/turnstone/core/mcp_client.py +++ b/turnstone/core/mcp_client.py @@ -5713,8 +5713,7 @@ class MCPClientManager: return try: self._storage.delete_mcp_pending_consent(user_id, server_name) - self._prune_pending_consent_cleared(now) - self._pending_consent_cleared[key] = now + self._mark_pending_consent_cleared(key, now) except Exception: log.debug( "mcp_pool.pending_consent_clear_failed user=%s server=%s", @@ -5723,6 +5722,17 @@ class MCPClientManager: exc_info=True, ) + def _mark_pending_consent_cleared(self, key: tuple[str, str], now: float) -> None: + """Record a DB-confirmed pending-consent DELETE in the TTL map. + + The single "prune-then-stamp" step shared by the two DB-confirmed clear + sites (:meth:`_clear_pending_consent_sync` on the hot dispatch path and + :meth:`_clear_pending_consent_best_effort` from the sweep) so the + bookkeeping order can't drift between them. + """ + self._prune_pending_consent_cleared(now) + self._pending_consent_cleared[key] = now + def _prune_pending_consent_cleared(self, now: float) -> None: """Drop aged entries when the TTL map grows large (memory hygiene). @@ -6558,10 +6568,7 @@ class MCPClientManager: return _structured_error( code="mcp_insufficient_scope", server=server_name, - detail=( - f"{kind.capitalize()} requires elevated scopes. " - "Re-consent flow with new scopes required." - ), + detail=_insufficient_scope_detail(server_row, kind), scopes_required=list(scopes), consent_url=_build_consent_url(server_row, scopes_required=list(scopes)), ) @@ -7243,6 +7250,30 @@ def _refresh_failed_detail(server_row: dict[str, Any]) -> str: return "Refresh token rejected. Re-consent required." +def _insufficient_scope_detail(server_row: dict[str, Any], kind: str) -> str: + """User-facing detail for a 403 insufficient_scope, per auth model. + + ``oauth_user``: a per-server step-up re-consent with the widened scope set + is the remedy (``_build_consent_url`` attaches the affordance). ``oauth_obo``: + there is no per-server consent flow — the minted token's scopes come from + the user's captured sign-in (entra: the app's delegated permissions; + rfc8693: the server's configured ``oauth_scopes``), so a user-driven + step-up is impossible (``_build_consent_url`` returns None). Point at the + real fix — an administrator widening access — instead of a dead-end + re-consent card. + """ + if server_row.get("auth_type") == "oauth_obo": + return ( + f"This {kind} needs additional permissions your sign-in token does not " + "carry. Ask your administrator to grant the required access (add the " + "scope to the server, or widen your delegated permissions at the " + "identity provider)." + ) + return ( + f"{kind.capitalize()} requires elevated scopes. Re-consent flow with new scopes required." + ) + + def _token_rejected_detail(server_row: dict[str, Any]) -> str: """User-facing detail for a 401 that survived one forced refresh (retry ceiling). diff --git a/turnstone/core/mcp_oauth.py b/turnstone/core/mcp_oauth.py index 661f668c..daea0d16 100644 --- a/turnstone/core/mcp_oauth.py +++ b/turnstone/core/mcp_oauth.py @@ -39,7 +39,6 @@ import httpx from turnstone.core.audit import record_audit from turnstone.core.log import get_logger from turnstone.core.mcp_crypto import ( - USER_SCOPED_AUTH_TYPES, MCPTokenDecryptError, OIDCCredentialPlain, is_user_scoped_auth, @@ -1500,21 +1499,64 @@ async def _revoke_after_refresh_failure( Drops the per-key refresh lock and backoff state — both safe to call when no entry exists — and returns ``refresh_failed`` so the dispatcher surfaces re-consent. + + The ``token_revoked`` audit is emitted ONLY when a row was actually deleted. + This matters for oauth_obo: its shared credential survives a per-server + revoke, so after the first permanent rejection deletes the cache row, every + later dispatch/prime past the cooldown re-runs the doomed redemption and + lands here again with nothing to delete — auditing unconditionally would + append a fresh "token_revoked" row for a token that no longer exists on + every cycle, forever (misleading forensics + alert fatigue for operators + who alert on token_revoked). oauth_user cannot hit the empty-delete case (a + revoke there also removes the only refresh source, so no recurrence), so + this gate is a no-op for it. """ - await asyncio.to_thread(token_store.delete_user_token, user_id, server_name) - await _audit_event( - app_state, - server_id=server_id_for_audit, - user_id=user_id, - action="mcp_server.oauth.token_revoked", - server_name=server_name, - detail={"reason": reason}, - ) + deleted = await asyncio.to_thread(token_store.delete_user_token, user_id, server_name) + if deleted: + await _audit_event( + app_state, + server_id=server_id_for_audit, + user_id=user_id, + action="mcp_server.oauth.token_revoked", + server_name=server_name, + detail={"reason": reason}, + ) _drop_refresh_lock(app_state, user_id, server_name) _clear_refresh_backoff(app_state, user_id, server_name) return TokenLookupResult(kind="refresh_failed") +def _decrypt_failure_result( + app_state: Any, + user_id: str, + server_name: str, + exc: MCPTokenDecryptError, + *, + event: str | None, +) -> TokenLookupResult: + """Map an ``MCPTokenDecryptError`` to a classified ``decrypt_failure`` result. + + One construction of the (optional warning log + ``_no_token_result`` + + fingerprint-carrying ``TokenLookupResult``) shape, shared by the five + token/credential read sites in the oauth_user and oauth_obo state machines + (the raw ``get_user_token`` / ``get_oidc_credential`` calls each raise this + on a key rotated away, and each must keep the classified-result contract). + Pass the site's structured *event* name to log, or ``None`` for a re-read + whose first read already logged. + """ + if event is not None: + log.warning(event, user_id=user_id, server_name=server_name, exc_info=True) + return _no_token_result( + app_state, + user_id, + server_name, + TokenLookupResult( + kind="decrypt_failure", + decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), + ), + ) + + async def get_user_access_token(*, app_state: Any, user_id: str, server_name: str) -> str | None: """Return a valid plaintext access token, refreshing if needed. @@ -1786,20 +1828,12 @@ async def get_user_access_token_classified( try: plain = await asyncio.to_thread(token_store.get_user_token, user_id, server_name) except MCPTokenDecryptError as exc: - log.warning( - "mcp_server.oauth.token_decrypt_failed_classified", - user_id=user_id, - server_name=server_name, - exc_info=True, - ) - return _no_token_result( + return _decrypt_failure_result( app_state, user_id, server_name, - TokenLookupResult( - kind="decrypt_failure", - decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), - ), + exc, + event="mcp_server.oauth.token_decrypt_failed_classified", ) if plain is None: return _no_token_result(app_state, user_id, server_name, TokenLookupResult(kind="missing")) @@ -1856,20 +1890,12 @@ async def get_user_access_token_classified( try: plain2 = await asyncio.to_thread(token_store.get_user_token, user_id, server_name) except MCPTokenDecryptError as exc: - log.warning( - "mcp_server.oauth.token_decrypt_failed_classified", - user_id=user_id, - server_name=server_name, - exc_info=True, - ) - return _no_token_result( + return _decrypt_failure_result( app_state, user_id, server_name, - TokenLookupResult( - kind="decrypt_failure", - decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), - ), + exc, + event="mcp_server.oauth.token_decrypt_failed_classified", ) if plain2 is None: return _no_token_result( @@ -2185,20 +2211,12 @@ async def _read_obo_credential( try: credential = await asyncio.to_thread(token_store.get_oidc_credential, user_id, issuer) except MCPTokenDecryptError as exc: - log.warning( - "mcp_server.oauth.obo_credential_decrypt_failed", - user_id=user_id, - server_name=server_name, - exc_info=True, - ) - return _no_token_result( + return _decrypt_failure_result( app_state, user_id, server_name, - TokenLookupResult( - kind="decrypt_failure", - decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), - ), + exc, + event="mcp_server.oauth.obo_credential_decrypt_failed", ) if credential is None: # No captured credential → the consent affordance is a re-login. @@ -2206,10 +2224,12 @@ async def _read_obo_credential( return credential -def _is_fresh_obo_cache_row(plain: MCPUserTokenPlain | None, current_audience: str) -> bool: +def _is_fresh_obo_cache_row( + plain: MCPUserTokenPlain | None, current_audience: str, current_scopes: str +) -> bool: """True when a cache row may be served as a minted obo access token. - Three conditions, all required (single source of truth for the pre-lock read + Four conditions, all required (single source of truth for the pre-lock read AND the post-lock re-read so they can't drift): - refresh_token is NULL — minted rows carry no refresh token; a @@ -2219,12 +2239,20 @@ def _is_fresh_obo_cache_row(plain: MCPUserTokenPlain | None, current_audience: s for a since-narrowed audience must NOT be served, so an operator's privilege reduction takes effect immediately rather than at token TTL (the audience-change purge is best-effort; this is the authoritative gate); + - the row's scopes equal the server's CURRENT scopes — the same authoritative + gate for the rfc8693 exchange scope (which shapes the minted bearer's + privileges just like the audience): a scope NARROWING must take effect on + the next dispatch even if the admin's best-effort cache purge failed, + rather than serving the wider-privilege bearer until its TTL. Under the + entra leg scopes are inert, so the stored and current values track the + same server column and this term is a no-op there; - not at/near expiry. """ return ( plain is not None and plain["refresh_token"] is None and (plain.get("audience") or "") == current_audience + and (plain.get("scopes") or "") == current_scopes and not _token_needs_refresh(plain["expires_at"]) ) @@ -2295,25 +2323,18 @@ async def get_obo_access_token_classified( try: plain = await asyncio.to_thread(token_store.get_user_token, user_id, server_name) except MCPTokenDecryptError as exc: - log.warning( - "mcp_server.oauth.obo_cache_decrypt_failed", - user_id=user_id, - server_name=server_name, - exc_info=True, - ) - return _no_token_result( + return _decrypt_failure_result( app_state, user_id, server_name, - TokenLookupResult( - kind="decrypt_failure", - decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), - ), + exc, + event="mcp_server.oauth.obo_cache_decrypt_failed", ) - # Serve the cache only when it is a fresh, right-audience, refresh-less row - # (see _is_fresh_obo_cache_row). A stale-audience or refresh-bearing row - # falls through to a fresh mint (which overwrites it via _persist_obo_cache_row). - fresh = _is_fresh_obo_cache_row(plain, audience) + # Serve the cache only when it is a fresh, right-audience, right-scopes, + # refresh-less row (see _is_fresh_obo_cache_row). A stale-audience/-scopes or + # refresh-bearing row falls through to a fresh mint (which overwrites it via + # _persist_obo_cache_row). + fresh = _is_fresh_obo_cache_row(plain, audience, scopes) if fresh and not force_refresh and plain is not None: return _token_result(app_state, user_id, server_name, plain["access_token"]) @@ -2384,19 +2405,12 @@ async def get_obo_access_token_classified( try: plain2 = await asyncio.to_thread(token_store.get_user_token, user_id, server_name) except MCPTokenDecryptError as exc: - return _no_token_result( - app_state, - user_id, - server_name, - TokenLookupResult( - kind="decrypt_failure", - decrypt_fingerprints=tuple(exc.key_fingerprints_attempted), - ), - ) + # First read already logged obo_cache_decrypt_failed; this re-read + # under the lock stays silent (event=None) to avoid a double line. + return _decrypt_failure_result(app_state, user_id, server_name, exc, event=None) # Same servability gate as the pre-lock read (refresh-less, right-audience, - # not-expired) — a stale-audience or refresh-bearing row falls through and - # re-mints. - if _is_fresh_obo_cache_row(plain2, audience) and plain2 is not None: + # right-scopes, not-expired) — a stale row falls through and re-mints. + if _is_fresh_obo_cache_row(plain2, audience, scopes) and plain2 is not None: if not force_refresh: return _token_result(app_state, user_id, server_name, plain2["access_token"]) # Obo cache rows are delete+create'd per mint and never touched by @@ -4008,7 +4022,6 @@ __all__ = [ "MCP_OAUTH_STATE_TTL_SECONDS", "OBO_GRANT_PROFILES", "TokenLookupResult", - "USER_SCOPED_AUTH_TYPES", "build_authorize_url", "close_mcp_oauth_state", "create_pending_state", diff --git a/turnstone/core/oidc.py b/turnstone/core/oidc.py index e31ca4f3..aa6a92a7 100644 --- a/turnstone/core/oidc.py +++ b/turnstone/core/oidc.py @@ -224,8 +224,11 @@ def load_oidc_config() -> OIDCConfig: "TURNSTONE_OIDC_OBO_GRANT_PROFILE", cfg, "obo_grant_profile", "entra" ).strip() # Validate against the operative mint-leg registry (single source of truth, - # so a new leg needs no second edit here). Lazy import: mcp_oauth pulls in - # the MCP stack, and it does not import this module, so there is no cycle. + # so a new leg needs no second edit here). Function-level import: oidc and + # mcp_oauth reference each other's runtime helpers (mcp_oauth's mint path + # imports this module's ``maybe_rediscover_oidc`` likewise lazily), so BOTH + # directions stay off the module-import graph to keep the cycle unrealised — + # neither module may import the other at module scope. from turnstone.core.mcp_oauth import OBO_GRANT_PROFILES if obo_grant_profile not in OBO_GRANT_PROFILES: diff --git a/turnstone/shared_static/interactive.js b/turnstone/shared_static/interactive.js index 1954530b..c4e4b666 100644 --- a/turnstone/shared_static/interactive.js +++ b/turnstone/shared_static/interactive.js @@ -4453,7 +4453,20 @@ function buildMcpErrorEmbed(err, rawJson, onConsent) { body.appendChild(scopesLine); } - if (category === "actionable") { + // Render the Connect / Re-consent button only when the dispatcher actually + // supplied a per-server consent URL. An "actionable"-category code with no + // consent_url means there is no per-server consent flow for this server — + // sign-in passthrough (oauth_obo) mints from the user's Turnstone sign-in and + // is deliberately absent from the Settings connections list and rejected by + // /start — so a button here would dead-end ("no consent URL; open Settings" + // pointing at a panel with nothing to connect). In that case the honest + // remedy is the detail text (sign in again / ask your administrator), so we + // show the card without a broken affordance. + const consentUrl = err.consent_url; + const hasConsentAffordance = + typeof consentUrl === "string" && + consentUrl.startsWith("/v1/api/mcp/oauth/start"); + if (category === "actionable" && hasConsentAffordance) { const btn = document.createElement("button"); btn.type = "button"; btn.className = "mcp-error-action-btn"; @@ -4469,20 +4482,10 @@ function buildMcpErrorEmbed(err, rawJson, onConsent) { : "Connect to " + serverLabel, ); btn.addEventListener("click", function () { - const consentUrl = err.consent_url; - if (!consentUrl || typeof consentUrl !== "string") { - // Defensive: should always be present per the dispatcher. If a - // path forgets to include it the user can still connect via the - // Settings panel (gear icon). - showToast("No consent URL available; open Settings to connect."); - return; - } - // Defence-in-depth: reject anything that isn't path-relative to - // the dispatcher's known prefix. ``_build_consent_url`` always - // emits ``/v1/api/mcp/oauth/start?...`` — a non-prefix value - // would indicate a future producer drift or a compromised - // dispatcher, and ``window.open("javascript:...")`` would be - // catastrophic. Never rely on the producer-side guarantee alone. + // Defence-in-depth: the render gate already proved the prefix, but + // re-check at click time — a non-prefix value would indicate producer + // drift or a compromised dispatcher, and window.open("javascript:...") + // would be catastrophic. Never rely on the producer-side guarantee alone. if (!consentUrl.startsWith("/v1/api/mcp/oauth/start")) { showToast("Invalid consent URL"); return;