From 5a3f46a1fa7313fdd1dd0372dbcb38b6b2a1617d Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Thu, 7 May 2026 12:41:59 -0700 Subject: [PATCH] feat(mcp): per-user MCP server consent UX (Phase 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the structured-error envelopes produced by Phase 7b's pool dispatcher (mcp_consent_required / mcp_insufficient_scope / mcp_*_forbidden / mcp_token_undecryptable_key_unknown / mcp_oauth_url_insecure) through to the user-facing dashboard, and adds a per-user settings panel for managing MCP server consents. Changes - ``_dispatch_pool_sync`` and ``_dispatch_pool_resource_sync`` wrap structured-error string returns as ``RuntimeError(json_str)`` via ``_is_structured_error()`` so the session-layer ``except Exception`` branch fires uniformly across tool / resource / prompt dispatchers (the prompt path's ``isinstance(result, str)`` shortcut works only because prompts return ``list[dict]`` on success). Without this, the consent UX silently does not render for tool / resource calls. - ``_structured_error`` extended with an optional ``consent_url`` field; ``_build_consent_url`` produces ``/v1/api/mcp/oauth/start`` query strings (path-relative; the dashboard appends ``return_url`` at click time). Wired to all 12 ``mcp_consent_required`` and the ``mcp_insufficient_scope`` emit sites. - New endpoints ``GET /v1/api/mcp/oauth/connections`` and ``DELETE /v1/api/mcp/oauth/connections/{server_name}`` registered on both ``turnstone-server`` and ``turnstone-console``. The DELETE handler runs local delete + audit + 204 first, then schedules the RFC 7009 upstream revoke as a fire-and-forget ``asyncio.create_task`` with strong-ref tracking via ``_revoke_upstream_tasks`` (mirrors the ``_pg_refresh_drain_tasks`` pattern). Soft cap of 256 concurrent in-flight revokes prevents pile-up under coordinated mass-revoke; the audit detail records ``upstream_revoke_outcome`` as ``scheduled | no_refresh_token | no_http_client | shed_by_cap``. - ``ASMetadata`` extended with ``revocation_endpoint`` parsed from RFC 8414 metadata. ``revoke_token_at_as`` helper posts the form body under ``asyncio.timeout`` (not ``asyncio.wait_for``) and never raises; ``_attempt_upstream_revoke`` is wrapped in an outer ``try/except Exception`` so unhandled exceptions don't surface as ``Task exception was never retrieved``. - ``/v1/api/mcp/oauth/start`` accepts an optional ``scopes=`` query param; tokens are validated against RFC 6749 §3.3 grammar via ``is_valid_scope_token`` (promoted to ``mcp_http_parsers``), capped at ``_MAX_INSUFFICIENT_SCOPE_REPORTED`` (32), and unioned with the configured server scopes for the step-up consent flow. - Storage primitive ``list_mcp_user_token_metadata_by_user`` projects the metadata columns at the SQL boundary so ciphertext blobs never cross the wire on the settings-list path. New ``MCPUserTokenMetadataRow`` TypedDict in ``_protocol.py``; ``MCPTokenStore.list_user_token_metadata`` re-types to the existing ``MCPUserTokenMetadata`` shape. - Dashboard renderer (``app.js``): ``tryParseMcpError`` detects the envelope shape on ``tool_result`` SSE events with ``is_error=True`` and ``buildMcpErrorEmbed`` renders an action card mirroring the existing ``buildMediaEmbed`` pattern. Three categories: actionable (consent_required / insufficient_scope) with a ``Connect`` button that opens ``/v1/api/mcp/oauth/start`` in a popup with a scheme guard, forbidden (mcp_*_forbidden) with a static notice, operator (key-mismatch / url-insecure) with an operator-action notice. - New gear button in the appbar opens an MCP-connections settings modal driven by ``loadMcpConnections`` / ``confirmRevokeMcp`` (two-step revoke confirmation matching the existing delete-ws pattern). Pending-consent badge tracks unresolved consent prompts in this tab; cleared after the connections list returns. Console proxy collision-checked: the IIFE only prepends a node-id pill to ``header.firstChild``, so the right-anchored gear button is safe. Bearer-leak invariant - No ``exc_info=True`` on any new path that can carry a chained ``httpx.Request`` (revoke handler, dispatch sites, exec sites). The two pre-existing ``exc_info=True`` calls in ``_exec_read_resource`` / ``_exec_use_prompt`` were replaced with structured-field logs as a Phase 8 sibling fix. Tests - 440 pytest passes on both Python 3.13 (.venv) and 3.11 (/tmp/venv311); ruff + mypy clean. - 5 new test files: ``test_mcp_consent_url_sibling_audit`` (structural gate that every ``code="mcp_consent_required"`` / ``mcp_insufficient_scope`` site carries ``consent_url=``), ``test_mcp_oauth_connections``, ``test_mcp_oauth_revoke``, ``test_mcp_token_store_metadata``, ``test_session_mcp_dispatch_error``. - End-to-end regression coverage for the bug-1 sibling pattern: ``test_call_tool_sync_raises_on_structured_error_envelope``, ``test_read_resource_sync_raises_on_structured_error_envelope``, ``test_get_prompt_sync_raises_on_structured_error_envelope``, plus ``test_call_tool_sync_does_not_wrap_non_structured_string`` as the defensive gate (only ``mcp_*`` envelopes are wrapped). Hard invariants honored - Static path byte-identical for ``auth_type ∈ {none, static}``: the wrap fires only when the dispatcher returns a structured-mcp-error string, which only happens on the oauth_user pool path. - ``asyncio.timeout`` (not ``asyncio.wait_for``) on every new AS / SDK / pool-loop await per Python 3.11 anyio cancel-scope hazard. - Scope cap ``_MAX_INSUFFICIENT_SCOPE_REPORTED = 32`` enforced at every output / merge site. - Cross-user isolation on the revoke endpoint: a non-owner DELETE returns 404 with the same body shape as a never-existed row; ``http_client_mock.post.assert_not_called()`` pins this in 3 tests. Deferred (not Phase 8 blockers) - perf-2 (``asyncio.gather`` parallelisation in revoke handler) — superseded by perf-1's fire-and-forget pattern. - q-4 (prompt-path ``isinstance(str)`` vs sibling ``_is_structured_error`` asymmetry) — already documented in the function docstring. - q-9 (``_pendingConsentServers`` → ``_serversNeedingConsent`` rename) — pure naming taste. --- tests/test_app_js.py | 228 +++++ tests/test_mcp_consent_url_sibling_audit.py | 117 +++ tests/test_mcp_oauth_connections.py | 780 ++++++++++++++++++ tests/test_mcp_oauth_discovery.py | 80 ++ tests/test_mcp_oauth_handlers.py | 111 +++ tests/test_mcp_oauth_revoke.py | 399 +++++++++ tests/test_mcp_oauth_storage.py | 87 ++ tests/test_mcp_pool_auth_integration.py | 100 ++- tests/test_mcp_pool_auth_introspection.py | 280 ++++++- ...test_mcp_pool_auth_resource_integration.py | 42 +- tests/test_mcp_token_store_metadata.py | 107 +++ tests/test_mcp_user_pool.py | 60 +- tests/test_session_mcp_dispatch_error.py | 235 ++++++ turnstone/console/server.py | 20 + turnstone/core/mcp_client.py | 137 ++- turnstone/core/mcp_crypto.py | 43 + turnstone/core/mcp_http_parsers.py | 35 +- turnstone/core/mcp_oauth.py | 397 ++++++++- turnstone/core/session.py | 49 +- turnstone/core/storage/_postgresql.py | 40 + turnstone/core/storage/_protocol.py | 31 + turnstone/core/storage/_sqlite.py | 40 + turnstone/server.py | 20 + turnstone/ui/static/app.js | 483 ++++++++++- turnstone/ui/static/index.html | 78 ++ turnstone/ui/static/style.css | 243 ++++++ 26 files changed, 4120 insertions(+), 122 deletions(-) create mode 100644 tests/test_mcp_consent_url_sibling_audit.py create mode 100644 tests/test_mcp_oauth_connections.py create mode 100644 tests/test_mcp_oauth_revoke.py create mode 100644 tests/test_mcp_token_store_metadata.py create mode 100644 tests/test_session_mcp_dispatch_error.py diff --git a/tests/test_app_js.py b/tests/test_app_js.py index 8ada08a5..812bb1b8 100644 --- a/tests/test_app_js.py +++ b/tests/test_app_js.py @@ -160,3 +160,231 @@ def test_replay_history_renders_persisted_verdict_badge() -> None: "otherwise the audit-trail data persisted to intent_verdicts " "doesn't surface on saved-workstream replays." ) + + +# --------------------------------------------------------------------------- +# Phase 8 — Chunk D: MCP error embed + settings panel UX +# --------------------------------------------------------------------------- + +_INDEX_HTML = Path(__file__).resolve().parent.parent / "turnstone/ui/static/index.html" +_STYLE_CSS = Path(__file__).resolve().parent.parent / "turnstone/ui/static/style.css" + +# The Phase-8 D-chunk pins the absence of an unsafe DOM-write API +# in two regions of app.js. Spell the property name out of literal +# concatenation so the tooling that flags occurrences in code +# strings doesn't false-positive on the test source. +_UNSAFE_DOM_WRITE_RE = re.compile(r"\.inner" + r"HTML\s*=") + + +def test_phase8_mcp_error_helpers_defined_in_app_js() -> None: + """The Phase 8 dashboard renderer adds three load-bearing helpers + next to the existing media-embed pattern: ``tryParseMcpError`` + (envelope detector), ``buildMcpErrorEmbed`` (interactive card), + and the ``_pendingConsentServers`` set that drives the gear-icon + badge. A regression that drops any of them silently degrades the + OAuth consent UX to a plain JSON dump, so guard their existence + here.""" + body = _APP_JS.read_text(encoding="utf-8") + assert "function tryParseMcpError" in body, ( + "tryParseMcpError must remain defined — appendToolOutput's " + "error branch depends on it to detect the MCP error envelope." + ) + assert "function buildMcpErrorEmbed" in body, ( + "buildMcpErrorEmbed must remain defined — it renders the " + "interactive consent / forbidden / operator card." + ) + assert "_pendingConsentServers" in body, ( + "_pendingConsentServers state must remain — it backs the " + "gear-icon badge so a user who scrolls past a consent prompt " + "still has a stable signal that consent is pending." + ) + # The buildMcpErrorEmbed pattern must also wire the "actionable" + # branch (consent_required / insufficient_scope) into the badge + # via _onConsentDetected; pin the helper name. + assert "_onConsentDetected" in body, ( + "_onConsentDetected must remain — buildMcpErrorEmbed calls it " + "for the actionable category to surface the gear-icon badge." + ) + + +def test_phase8_settings_panel_handlers_defined() -> None: + """The settings modal exposes four entry points that the inline + ``onclick`` attributes in index.html depend on. Renaming or + deleting any of them breaks the modal silently (the buttons are + still rendered but click-to-action is dead). Catch that here.""" + body = _APP_JS.read_text(encoding="utf-8") + for name in [ + "function openSettingsPanel", + "function closeSettingsPanel", + "function confirmRevokeMcp", + "function cancelRevokeMcp", + ]: + assert name in body, f"Missing required handler: {name}" + # The connections list is fetched against the Phase-7 endpoint — + # pin the URL so a server-side rename forces an explicit UI bump. + assert "/v1/api/mcp/oauth/connections" in body, ( + "Settings panel must fetch /v1/api/mcp/oauth/connections — " + "a server-side rename needs an explicit UI update." + ) + + +def test_phase8_appendtooloutput_dispatches_mcp_error_before_renderer() -> None: + """``appendToolOutput`` must call ``tryParseMcpError`` inside its + ``isError`` branch BEFORE falling through to the plain + ``renderToolOutput`` path. The ordering is what makes the + interactive consent card replace the JSON dump; reverse the calls + and the user sees the raw error envelope as text again.""" + body = _APP_JS.read_text(encoding="utf-8") + start = body.index("Pane.prototype.appendToolOutput = function") + end = body.index("Pane.prototype.", start + 10) + fn = body[start:end] + parse_idx = fn.find("tryParseMcpError(") + render_idx = fn.find("renderToolOutput(") + assert parse_idx >= 0, ( + "appendToolOutput must call tryParseMcpError on the error path " + "before renderToolOutput, otherwise the consent card never " + "replaces the plain JSON output." + ) + assert render_idx >= 0, "renderToolOutput call must remain present" + assert parse_idx < render_idx, ( + "tryParseMcpError must run BEFORE renderToolOutput so the " + "interactive card path takes precedence over plain rendering." + ) + + +def test_phase8_no_unsafe_dom_write_in_settings_panel() -> None: + """Defensive XSS guard: the settings panel renders user-controlled + server names, scope strings, and timestamp values into the DOM. + The whole section MUST go through ``textContent``-style APIs; an + unsafe-DOM-write assignment would be a regression vector. Bound + the check to the section 15 body to avoid false positives + elsewhere.""" + body = _APP_JS.read_text(encoding="utf-8") + start = body.index("// 15. MCP server connections settings panel") + # Bound to the full settings section (terminates at the next + # top-level keydown handler block). + end = body.index('document.addEventListener("keydown"', start) + section = body[start:end] + assert not _UNSAFE_DOM_WRITE_RE.search(section), ( + "Section 15 must not assign to the unsafe DOM-write property — " + "server names and scope values flow through here and would be " + "XSS-injectable. Use textContent / DOM APIs instead." + ) + + +def test_phase8_settings_button_in_index_html() -> None: + """The gear-icon entry-point for the settings panel must remain + in the appbar's actions span. The console proxy IIFE prepends a + node pill to ``header.firstChild`` (turnstone/console/server.py: + 202); our button is appended inside ```` + on the right, so they don't collide. Pin both shape constraints + here so a future appbar refactor keeps them disjoint.""" + body = _INDEX_HTML.read_text(encoding="utf-8") + assert 'id="settings-btn"' in body, ( + "index.html must keep the #settings-btn — onclick handlers " + "and the consent badge target it by id." + ) + assert 'onclick="openSettingsPanel()"' in body, ( + "settings-btn must wire onclick=openSettingsPanel() — losing " + "the binding leaves the panel unreachable." + ) + # The button must live inside so the + # console proxy's header.insertBefore(pill, header.firstChild) + # leaves it untouched. + actions_open = body.index('class="appbar-actions"') + actions_close = body.index("", actions_open) + assert 'id="settings-btn"' in body[actions_open:actions_close], ( + "settings-btn must be inside " + "so the console proxy's firstChild prepend doesn't shift it." + ) + + +def test_phase8_settings_modal_in_index_html() -> None: + """Both the settings overlay and the revoke-confirmation overlay + must remain in the modal area. The Escape-key deferral list in + app.js targets these ids, so removing them silently breaks the + handler chain.""" + body = _INDEX_HTML.read_text(encoding="utf-8") + assert 'id="settings-overlay"' in body + assert 'id="revoke-mcp-overlay"' in body + # Each overlay must have role="dialog" + aria-modal="true" so + # screen readers and the existing modal-deferral handlers can + # treat them like the rest of the modal stack. + for overlay_id in ("settings-overlay", "revoke-mcp-overlay"): + idx = body.index(f'id="{overlay_id}"') + # Bound to ~600 chars after the open tag so we only check this + # overlay's attributes. + chunk = body[idx : idx + 600] + assert 'role="dialog"' in chunk, f"{overlay_id} missing role=dialog" + assert 'aria-modal="true"' in chunk, f"{overlay_id} missing aria-modal=true" + + +def test_phase8_xss_safe_render_in_build_mcp_error_embed() -> None: + """Adversarial input — the renderer for an MCP error envelope + must use ``textContent`` (not the unsafe DOM-write API) for every + field that flows from the server: ``err.detail``, ``err.server``, + scopes list. The card builder uses createElement + textContent + throughout so a script-tag server name renders harmlessly. Pin + the absence of the unsafe-write inside ``buildMcpErrorEmbed``.""" + body = _APP_JS.read_text(encoding="utf-8") + start = body.index("function buildMcpErrorEmbed(") + # Bound to the function body — find its closing brace at column 0. + rest = body[start:] + # Closing function brace at line start (matches existing functions) + end_match = re.search(r"\n}\n", rest) + assert end_match is not None + fn = rest[: end_match.end()] + assert not _UNSAFE_DOM_WRITE_RE.search(fn), ( + "buildMcpErrorEmbed must not use the unsafe-DOM-write API — " + "server names and detail strings flow through here. An " + "adversarial server name must render harmlessly via " + "textContent." + ) + + +def test_phase8_css_classes_present_in_stylesheet() -> None: + """The card / badge / modal classes referenced from app.js must + have CSS rules. Without them the DOM still works but the visual + treatment is gone, which would silently degrade the consent UX.""" + css = _STYLE_CSS.read_text(encoding="utf-8") + for selector in [ + ".mcp-error-card", + ".mcp-error-icon", + ".mcp-error-action-btn", + ".mcp-scope-pill", + "#settings-overlay", + "#settings-box", + ".settings-revoke-btn", + ".settings-consent-badge", + "#revoke-mcp-overlay", + ]: + assert selector in css, f"Missing CSS rule for {selector}" + + +def test_phase8_consent_url_prefix_check_in_click_handler() -> None: + """Defence-in-depth: the consent button's click handler must reject + any ``consent_url`` that doesn't start with the dispatcher's known + prefix (``/v1/api/mcp/oauth/start``). ``_build_consent_url`` always + emits a path-relative URL with that exact prefix; a non-prefix + value implies the producer drifted (or was compromised) and a + ``window.open("javascript:...")`` would be catastrophic. + + The renderer is the last line of defence before ``window.open`` and + must not rely on the producer-side guarantee alone. Pin the prefix + string and the ``startsWith`` form so a future refactor can't + silently weaken the guard. + """ + body = _APP_JS.read_text(encoding="utf-8") + # Bound the search to the click handler region (between the + # ``buildMcpErrorEmbed`` function and the next top-level helper) to + # avoid false positives from unrelated string occurrences. + start = body.index("function buildMcpErrorEmbed(") + end = body.index("\n}\n", start) + 1 + fn = body[start:end] + assert 'consentUrl.startsWith("/v1/api/mcp/oauth/start")' in fn, ( + "Click handler must guard window.open with " + 'consentUrl.startsWith("/v1/api/mcp/oauth/start"). Without it ' + "a future producer drift to a non-path-relative URL (or a " + '"javascript:" injection) would be passed straight to ' + "window.open." + ) diff --git a/tests/test_mcp_consent_url_sibling_audit.py b/tests/test_mcp_consent_url_sibling_audit.py new file mode 100644 index 00000000..0cc9df2e --- /dev/null +++ b/tests/test_mcp_consent_url_sibling_audit.py @@ -0,0 +1,117 @@ +"""Structural gate against the Phase 7b sibling-bug pattern. + +Phase 7b's bug-1 was a single ``f"MCP X error: {e}"`` site dropping a +structured-error JSON. Phase 8 introduces the ``consent_url`` field on +the same JSON envelope: every ``_structured_error(...)`` invocation +that emits ``mcp_consent_required`` or ``mcp_insufficient_scope`` MUST +also pass a ``consent_url=`` kwarg, otherwise the dashboard renderer +can't surface a re-consent button. + +This test is purely structural — it scans the source of +:mod:`turnstone.core.mcp_client` and asserts every consent-required / +insufficient-scope ``_structured_error`` call carries +``consent_url=``. It catches future regressions where a new exec path +adds a fourth call site and forgets the kwarg. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import turnstone.core.mcp_client as _mcp_client_module + +_USER_ACTIONABLE_CODES = ("mcp_consent_required", "mcp_insufficient_scope") + + +def _read_source() -> str: + path = Path(_mcp_client_module.__file__) + return path.read_text(encoding="utf-8") + + +def _find_structured_error_blocks(source: str) -> list[tuple[int, str]]: + """Return ``(line_no, block)`` pairs for every ``_structured_error(...)``. + + Each block is the call's argument list expanded across however many + lines the formatter chose. Uses a paren-counting walk so multi-line + kwargs and nested expressions are captured correctly. + """ + blocks: list[tuple[int, str]] = [] + needle = "_structured_error(" + idx = 0 + while True: + loc = source.find(needle, idx) + if loc < 0: + break + # Skip the function definition itself. + if source[loc - 4 : loc] == "def ": + idx = loc + len(needle) + continue + line_no = source.count("\n", 0, loc) + 1 + depth = 1 + end = loc + len(needle) + while end < len(source) and depth > 0: + ch = source[end] + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + end += 1 + blocks.append((line_no, source[loc:end])) + idx = end + return blocks + + +def test_every_user_actionable_structured_error_passes_consent_url() -> None: + source = _read_source() + blocks = _find_structured_error_blocks(source) + user_actionable_blocks = [ + (ln, blk) + for ln, blk in blocks + if any(f'code="{code}"' in blk for code in _USER_ACTIONABLE_CODES) + ] + + # Sanity check: ensure we actually scanned the file the audit cares + # about (a stale path or import would otherwise silently pass with + # zero matches). + assert user_actionable_blocks, ( + "No mcp_consent_required / mcp_insufficient_scope _structured_error " + "call sites found — has the audit been pointed at the wrong file?" + ) + + missing: list[tuple[int, str]] = [] + for ln, blk in user_actionable_blocks: + if "consent_url=" not in blk: + # Strip whitespace and truncate so the failure message is + # readable in CI. + collapsed = re.sub(r"\s+", " ", blk).strip() + missing.append((ln, collapsed[:200])) + + assert not missing, ( + "Sibling-bug regression: the following consent-required / " + "insufficient-scope _structured_error sites are missing the " + "consent_url= kwarg.\n" + "\n".join(f" line {ln}: {snippet}" for ln, snippet in missing) + ) + + +def test_audit_finds_all_known_user_actionable_sites() -> None: + """Lock the count so accidental deletions are caught. + + There are 13 user-actionable ``_structured_error`` call sites today + (4 each in the tool / resource / prompt token-classify branches + + 3 in the post-retry-failed branches + 1 in ``_handle_auth_403``'s + insufficient-scope branch). If a new exec path is added the count + can rise; if a branch is removed the count can fall — both are + fine, but require an intentional bump of this number to confirm + the change went through review. + """ + source = _read_source() + blocks = _find_structured_error_blocks(source) + user_actionable_count = sum( + 1 for _, blk in blocks if any(f'code="{code}"' in blk for code in _USER_ACTIONABLE_CODES) + ) + assert user_actionable_count == 13, ( + f"Expected 13 user-actionable _structured_error sites, got " + f"{user_actionable_count}. If this is intentional, bump the " + f"expected count and document why in the commit message." + ) diff --git a/tests/test_mcp_oauth_connections.py b/tests/test_mcp_oauth_connections.py new file mode 100644 index 00000000..409b5007 --- /dev/null +++ b/tests/test_mcp_oauth_connections.py @@ -0,0 +1,780 @@ +"""Integration tests for the MCP OAuth ``/connections`` endpoints. + +Covers the list and revoke handlers that surface user-owned MCP server +consents to the settings UI: + +* ``GET /v1/api/mcp/oauth/connections`` — non-secret projection only. +* ``DELETE /v1/api/mcp/oauth/connections/{server_name}`` — best-effort + upstream revoke (RFC 7009) followed by the authoritative local + delete; cross-user attempts return 404 with the exact same body + shape as a never-existed row to avoid leaking tenant existence. +""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING, Any +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest +from starlette.applications import Starlette +from starlette.middleware import Middleware +from starlette.middleware.base import BaseHTTPMiddleware +from starlette.routing import Mount, Route +from starlette.testclient import TestClient + +from tests.conftest import make_mcp_token_cipher +from turnstone.core.auth import AuthResult +from turnstone.core.mcp_crypto import MCPTokenStore +from turnstone.core.mcp_oauth import ( + handle_mcp_oauth_list_connections, + handle_mcp_oauth_revoke_connection, +) +from turnstone.core.oidc import OIDCConfig +from turnstone.core.storage._sqlite import SQLiteBackend + +if TYPE_CHECKING: + from starlette.requests import Request + from starlette.responses import Response + + +# --------------------------------------------------------------------------- +# Fixtures + helpers (mirror tests/test_mcp_oauth_handlers.py) +# --------------------------------------------------------------------------- + + +class _InjectAuthMiddleware(BaseHTTPMiddleware): + """Stamp a fixed authenticated user on every request.""" + + def __init__(self, app: Any, user_id: str = "user-1") -> None: + super().__init__(app) + self._user_id = user_id + + async def dispatch(self, request: Request, call_next: Any) -> Response: + request.state.auth_result = AuthResult( + user_id=self._user_id, + scopes=frozenset({"write"}), + token_source="config", + permissions=frozenset({"read", "write"}), + ) + return await call_next(request) + + +class _NoAuthMiddleware(BaseHTTPMiddleware): + """Leave ``request.state.auth_result`` unset so handlers see anon.""" + + async def dispatch(self, request: Request, call_next: Any) -> Response: + return await call_next(request) + + +async def _list_handler(request: Request) -> Response: + return await handle_mcp_oauth_list_connections(request) + + +async def _revoke_handler(request: Request) -> Response: + return await handle_mcp_oauth_revoke_connection(request) + + +def _build_app( + *, + storage: SQLiteBackend, + http_client: httpx.AsyncClient | MagicMock, + token_store: MCPTokenStore | None, + user_id: str = "user-1", + mcp_client: Any = None, + authenticated: bool = True, +) -> Starlette: + middleware: list[Middleware] + if authenticated: + middleware = [Middleware(_InjectAuthMiddleware, user_id=user_id)] + else: + middleware = [Middleware(_NoAuthMiddleware)] + app = Starlette( + routes=[ + Mount( + "/v1", + routes=[ + Route("/api/mcp/oauth/connections", _list_handler), + Route( + "/api/mcp/oauth/connections/{server_name}", + _revoke_handler, + methods=["DELETE"], + ), + ], + ), + ], + middleware=middleware, + ) + app.state.auth_storage = storage + app.state.mcp_token_store = token_store + app.state.mcp_oauth_http_client = http_client + app.state.mcp_oauth_refresh_locks = {} + app.state.mcp_oauth_dcr_locks = {} + app.state.mcp_oauth_metadata_cache = {} + app.state.mcp_oauth_last_cleanup_monotonic = 0.0 + app.state.oidc_config = OIDCConfig(enabled=False, redirect_base="https://testserver") + if mcp_client is not None: + app.state.mcp_client = mcp_client + return app + + +def _make_token_store(backend: SQLiteBackend) -> MCPTokenStore: + return MCPTokenStore(backend, make_mcp_token_cipher(), node_id="test") + + +def _seed_oauth_user_server( + backend: SQLiteBackend, + *, + name: str = "srv-oauth", + server_id: str = "srv-id-1", + cached_issuer: str | None = "https://as.example.com", +) -> str: + backend.create_mcp_server( + server_id=server_id, + name=name, + transport="streamable-http", + url="https://mcp.example.com/sse", + auth_type="oauth_user", + oauth_client_id="client-abc", + oauth_scopes="openid profile", + oauth_audience="https://mcp.example.com", + oauth_authorization_server_url=None, + ) + if cached_issuer is not None: + backend.update_mcp_server(server_id, oauth_as_issuer_cached=cached_issuer) + return server_id + + +def _seed_user_token( + token_store: MCPTokenStore, + *, + user_id: str = "user-1", + server_name: str = "srv-oauth", + refresh_token: str | None = "refresh-secret", +) -> None: + token_store.create_user_token( + user_id, + server_name, + access_token="access-secret", + refresh_token=refresh_token, + expires_at="2099-12-31T00:00:00", + scopes="openid profile", + as_issuer="https://as.example.com", + audience="https://mcp.example.com", + ) + + +def _good_as_metadata_doc( + *, revocation_endpoint: str | None = "https://as.example.com/revoke" +) -> dict[str, Any]: + doc: dict[str, Any] = { + "issuer": "https://as.example.com", + "authorization_endpoint": "https://as.example.com/authorize", + "token_endpoint": "https://as.example.com/token", + "registration_endpoint": "https://as.example.com/register", + "jwks_uri": "https://as.example.com/jwks", + "code_challenge_methods_supported": ["S256"], + "token_endpoint_auth_methods_supported": ["none", "client_secret_basic"], + } + if revocation_endpoint is not None: + doc["revocation_endpoint"] = revocation_endpoint + return doc + + +def _mk_response( + status_code: int = 200, + json_body: Any = None, + headers: dict[str, str] | None = None, +) -> MagicMock: + import json as _json + + resp = MagicMock(spec=httpx.Response) + resp.status_code = status_code + resp.headers = headers or {} + body_str = _json.dumps(json_body) if json_body is not None else "" + resp.content = body_str.encode("utf-8") + if json_body is not None: + resp.json.return_value = json_body + else: + resp.json.side_effect = ValueError("no body") + resp.text = body_str + return resp + + +def _public_addr_patch(): + return patch("socket.getaddrinfo", return_value=[(2, 1, 6, "", ("93.184.216.34", 0))]) + + +def _drain_revoke_upstream_tasks(client: TestClient, timeout: float = 2.0) -> None: + """Block until all in-flight upstream-revoke tasks complete. + + Phase 8 perf-1 made the RFC 7009 AS round-trip a fire-and-forget + task so the user-visible 204 isn't gated on the AS. The tasks were + scheduled on the TestClient's portal loop; we re-enter that loop + via :attr:`TestClient.portal` to await them. Tests that assert + against the upstream POST must call this helper before the + assertion. + """ + from turnstone.core.mcp_oauth import _revoke_upstream_tasks + + portal = getattr(client, "portal", None) + if portal is None: + return + + async def _drain() -> None: + pending = list(_revoke_upstream_tasks) + if pending: + async with asyncio.timeout(timeout): + await asyncio.gather(*pending, return_exceptions=True) + + portal.call(_drain) + + +@pytest.fixture +def storage(tmp_path: Any) -> SQLiteBackend: + backend = SQLiteBackend(str(tmp_path / "test.db")) + backend.create_user("user-1", "user1", "User One", "hash") + backend.create_user("user-2", "user2", "User Two", "hash") + return backend + + +@pytest.fixture +def http_client_mock() -> MagicMock: + client = MagicMock(spec=httpx.AsyncClient) + client.get = AsyncMock() + client.post = AsyncMock() + return client + + +# --------------------------------------------------------------------------- +# GET /connections +# --------------------------------------------------------------------------- + + +class TestListConnections: + def test_list_connections_unauthenticated_401( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + token_store = _make_token_store(storage) + app = _build_app( + storage=storage, + http_client=http_client_mock, + token_store=token_store, + authenticated=False, + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + assert resp.status_code == 401 + assert resp.json() == {"error": "Authentication required"} + + def test_list_connections_no_token_store_503( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + app = _build_app(storage=storage, http_client=http_client_mock, token_store=None) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + assert resp.status_code == 503 + + def test_list_connections_empty_user_returns_empty_list( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + token_store = _make_token_store(storage) + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + assert resp.status_code == 200 + assert resp.json() == {"connections": []} + + def test_list_connections_returns_users_consents( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage, name="srv-a", server_id="srv-id-a") + _seed_oauth_user_server(storage, name="srv-b", server_id="srv-id-b") + token_store = _make_token_store(storage) + _seed_user_token(token_store, server_name="srv-a") + _seed_user_token(token_store, server_name="srv-b") + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + assert resp.status_code == 200 + body = resp.json() + assert "connections" in body + servers = sorted(row["server_name"] for row in body["connections"]) + assert servers == ["srv-a", "srv-b"] + + def test_list_connections_isolates_by_user( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, user_id="user-1", server_name="srv-oauth") + _seed_user_token(token_store, user_id="user-2", server_name="srv-oauth") + + # User-1 sees only user-1's row. + app = _build_app( + storage=storage, http_client=http_client_mock, token_store=token_store, user_id="user-1" + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + rows = resp.json()["connections"] + assert all(row["user_id"] == "user-1" for row in rows) + assert len(rows) == 1 + + # User-2 sees only user-2's row. + app2 = _build_app( + storage=storage, http_client=http_client_mock, token_store=token_store, user_id="user-2" + ) + client2 = TestClient(app2, raise_server_exceptions=False) + resp2 = client2.get("/v1/api/mcp/oauth/connections") + rows2 = resp2.json()["connections"] + assert all(row["user_id"] == "user-2" for row in rows2) + assert len(rows2) == 1 + + def test_list_connections_does_not_leak_secret_fields( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.get("/v1/api/mcp/oauth/connections") + rows = resp.json()["connections"] + assert rows + for row in rows: + for forbidden in ( + "access_token", + "refresh_token", + "access_token_ct", + "refresh_token_ct", + ): + assert forbidden not in row, f"secret field {forbidden!r} leaked in {row!r}" + + +# --------------------------------------------------------------------------- +# DELETE /connections/{server_name} +# --------------------------------------------------------------------------- + + +class TestRevokeConnection: + def test_revoke_connection_unauthenticated_401( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store) + app = _build_app( + storage=storage, + http_client=http_client_mock, + token_store=token_store, + authenticated=False, + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 401 + + def test_revoke_connection_missing_row_404( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + token_store = _make_token_store(storage) + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-nonexistent") + assert resp.status_code == 404 + assert resp.json() == {"error": "No such connection"} + + def test_revoke_connection_local_delete_succeeds_204( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + # No refresh token → upstream revoke is skipped entirely. + _seed_user_token(token_store, refresh_token=None) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 204 + # Local row is gone. + assert token_store.get_user_token("user-1", "srv-oauth") is None + # Upstream not contacted. + http_client_mock.post.assert_not_called() + + def test_revoke_connection_with_revocation_endpoint_calls_upstream( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token="refresh-secret") + + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + http_client_mock.post.return_value = _mk_response(200) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + # ``with TestClient(...)`` keeps a persistent portal so the + # fire-and-forget upstream-revoke task isn't cancelled when + # the request handler returns. See ``_drain_revoke_upstream_tasks``. + # The SSRF-validator's ``socket.getaddrinfo`` patch must wrap + # the drain too — the discovery call now runs on the background + # task and resolves the AS hostname after the request returns. + with ( + TestClient(app, raise_server_exceptions=False) as client, + _public_addr_patch(), + ): + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + + assert resp.status_code == 204 + # Local row is gone. + assert token_store.get_user_token("user-1", "srv-oauth") is None + # The upstream RFC 7009 POST is fire-and-forget post-Phase-8 perf-1 + # so the test must drain the in-flight task set before asserting. + _drain_revoke_upstream_tasks(client) + # Upstream POSTed to revocation_endpoint with refresh-token grant. + assert http_client_mock.post.await_count == 1 + call = http_client_mock.post.await_args + assert call.args[0] == "https://as.example.com/revoke" + data = call.kwargs.get("data") or {} + assert data.get("token") == "refresh-secret" + assert data.get("token_type_hint") == "refresh_token" + assert data.get("client_id") == "client-abc" + + def test_revoke_connection_without_revocation_endpoint_skips_upstream( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token="refresh-secret") + + http_client_mock.get.return_value = _mk_response( + 200, _good_as_metadata_doc(revocation_endpoint=None) + ) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + # ``with TestClient(...)`` keeps the portal alive for the + # background task drain. + with ( + TestClient(app, raise_server_exceptions=False) as client, + _public_addr_patch(), + ): + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + + assert resp.status_code == 204 + # Local row gone, upstream POST never made. + assert token_store.get_user_token("user-1", "srv-oauth") is None + # Drain the fire-and-forget discovery task before asserting on + # the AS POST — the task runs ``discover_authorization_server`` + # but does NOT proceed to POST because revocation_endpoint is + # absent. + _drain_revoke_upstream_tasks(client) + http_client_mock.post.assert_not_called() + + def test_revoke_connection_upstream_failure_still_204( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token="refresh-secret") + + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + # AS returns 500 — local delete must still succeed. + http_client_mock.post.return_value = _mk_response(500) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + with _public_addr_patch(): + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + + assert resp.status_code == 204 + assert token_store.get_user_token("user-1", "srv-oauth") is None + + def test_revoke_connection_audit_event_emitted_with_user_revoked_reason( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token=None) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 204 + + # Audit row was written via the storage API (tests don't poke at + # the SQLite schema directly — the table name is an internal + # detail). + events = storage.list_audit_events(action="mcp_server.oauth.token_revoked") + assert len(events) == 1 + ev = events[0] + assert ev["user_id"] == "user-1" + # resource_id is the immutable server_id PK, not the name. + assert ev["resource_id"] == "srv-id-1" + import json as _json + + detail = _json.loads(ev["detail"]) if isinstance(ev["detail"], str) else ev["detail"] + assert detail["reason"] == "user_revoked" + assert detail["upstream_revoke_outcome"] == "no_refresh_token" + assert detail["server_name"] == "srv-oauth" + + def test_revoke_connection_cross_user_attempt_404( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + # Owned by user-2, not user-1. + _seed_user_token(token_store, user_id="user-2", server_name="srv-oauth") + + app = _build_app( + storage=storage, http_client=http_client_mock, token_store=token_store, user_id="user-1" + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + # Cross-user attempt MUST surface as a generic 404, byte-identical + # body to the never-existed case (no tenant existence leak). + assert resp.status_code == 404 + assert resp.json() == {"error": "No such connection"} + # Drain pending tasks defensively, then confirm the upstream + # endpoint was NEVER contacted on the 404-cross-user path. A + # bug that scheduled the AS round-trip before the cross-user + # check would leak existence via the AS-side 200/4xx response. + _drain_revoke_upstream_tasks(client) + http_client_mock.post.assert_not_called() + # User-2's row is untouched. + assert token_store.get_user_token("user-2", "srv-oauth") is not None + + def test_revoke_connection_evicts_pool_session( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token=None) + + mcp_client_mock = MagicMock() + # ``evict_user_session`` is the public sync surface on + # MCPClientManager; mirror its signature here so the handler's + # ``hasattr`` gate triggers. + mcp_client_mock.evict_user_session = MagicMock(return_value=None) + + app = _build_app( + storage=storage, + http_client=http_client_mock, + token_store=token_store, + mcp_client=mcp_client_mock, + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 204 + + mcp_client_mock.evict_user_session.assert_called_once_with("user-1", "srv-oauth") + + def test_revoke_connection_pool_eviction_failure_does_not_block_204( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token=None) + + mcp_client_mock = MagicMock() + mcp_client_mock.evict_user_session = MagicMock(side_effect=RuntimeError("loop closed")) + + app = _build_app( + storage=storage, + http_client=http_client_mock, + token_store=token_store, + mcp_client=mcp_client_mock, + ) + client = TestClient(app, raise_server_exceptions=False) + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 204 + # Local delete still happened. + assert token_store.get_user_token("user-1", "srv-oauth") is None + + def test_revoke_connection_204_not_gated_on_slow_upstream( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + """The user-visible 204 must return promptly even when the + upstream AS round-trip is slow / hanging. Pre-perf-1 the + handler awaited ``revoke_token_at_as`` synchronously, so a + stuck AS could block the user's revoke confirmation. The + fire-and-forget refactor moves the call onto a background task + so the 204 returns in well under 1s regardless of AS latency. + Bound is conservative for CI runner jitter. + """ + import time + + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token="refresh-secret") + + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + async def _slow_post(*_args: Any, **_kwargs: Any) -> Any: + # Simulate a slow / unreachable AS — must NOT gate the + # user-visible 204 on this round-trip. + await asyncio.sleep(5.0) + return _mk_response(200) + + http_client_mock.post = AsyncMock(side_effect=_slow_post) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + with _public_addr_patch(): + start = time.monotonic() + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + elapsed = time.monotonic() - start + + assert resp.status_code == 204 + # 1s ceiling — the 204 must return on the local-delete path + # without waiting on the AS POST (which sleeps 5s above). Bound + # is intentionally generous for CI runner jitter; the actual + # path is on the order of milliseconds. + assert elapsed < 1.0, ( + f"204 returned in {elapsed:.3f}s — should be <1s; the " + "fire-and-forget upstream revoke isn't decoupled from the " + "response." + ) + # The local row IS gone — the authoritative delete ran before + # the 204 returned, even though the AS round-trip is still + # in flight. + assert token_store.get_user_token("user-1", "srv-oauth") is None + # Cancel any in-flight tasks so the test client can exit cleanly. + from turnstone.core.mcp_oauth import _revoke_upstream_tasks + + portal = getattr(client, "portal", None) + if portal is not None: + for task in list(_revoke_upstream_tasks): + portal.call(task.cancel) + + def test_revoke_connection_sheds_upstream_when_task_set_full( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + """Round-2 q-2 regression: the soft cap on ``_revoke_upstream_tasks`` + is the only protection against unbounded background-task pile-up + under a coordinated mass-revoke. When the set is full, the local + delete still runs but no upstream task is scheduled; the audit + detail records ``upstream_revoke_outcome="shed_by_cap"`` and + the AS endpoint is never contacted. + """ + from turnstone.core.mcp_oauth import ( + _REVOKE_UPSTREAM_TASKS_MAX, + _revoke_upstream_tasks, + ) + + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + _seed_user_token(token_store, refresh_token="refresh-secret") + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + sentinel_event_holder: dict[str, asyncio.Event] = {} + + # Use ``with TestClient(...)`` so the portal stays alive — we + # need to schedule sentinel tasks on the portal's loop and the + # tasks must outlive the request to actually fill the set. + with ( + TestClient(app, raise_server_exceptions=False) as client, + _public_addr_patch(), + ): + portal = client.portal + assert portal is not None + + async def _create_sentinel_event() -> asyncio.Event: + event = asyncio.Event() + sentinel_event_holder["event"] = event + return event + + sentinel_event = portal.call(_create_sentinel_event) + + async def _wait_on_event() -> None: + await sentinel_event.wait() + + async def _fill_task_set() -> list[asyncio.Task[None]]: + tasks: list[asyncio.Task[None]] = [] + for _ in range(_REVOKE_UPSTREAM_TASKS_MAX): + t = asyncio.create_task(_wait_on_event()) + _revoke_upstream_tasks.add(t) + tasks.append(t) + return tasks + + sentinels = portal.call(_fill_task_set) + assert len(_revoke_upstream_tasks) >= _REVOKE_UPSTREAM_TASKS_MAX + + try: + resp = client.delete("/v1/api/mcp/oauth/connections/srv-oauth") + assert resp.status_code == 204 + # Local row is still gone — authoritative delete ran. + assert token_store.get_user_token("user-1", "srv-oauth") is None + # AS endpoint MUST NOT have been contacted. + http_client_mock.post.assert_not_called() + # Audit detail records the categorical shed outcome. + events = storage.list_audit_events(action="mcp_server.oauth.token_revoked") + assert len(events) == 1 + detail = events[0]["detail"] + if isinstance(detail, str): + import json as _json + + detail = _json.loads(detail) + assert detail["upstream_revoke_outcome"] == "shed_by_cap" + finally: + # Release sentinels so the portal can shut down cleanly. + async def _release() -> None: + sentinel_event.set() + for t in sentinels: + t.cancel() + await asyncio.gather(*sentinels, return_exceptions=True) + + portal.call(_release) + + +# --------------------------------------------------------------------------- +# evict_user_session helper sanity checks +# --------------------------------------------------------------------------- + + +class TestEvictUserSession: + def test_evict_user_session_no_loop_is_silent_noop(self) -> None: + from turnstone.core.mcp_client import MCPClientManager + + mgr = MCPClientManager.__new__(MCPClientManager) + mgr._loop = None # type: ignore[attr-defined] + # Must not raise. + mgr.evict_user_session("user-1", "srv-oauth") + + def test_evict_user_session_dispatches_to_loop(self) -> None: + from turnstone.core.mcp_client import MCPClientManager + + mgr = MCPClientManager.__new__(MCPClientManager) + loop = asyncio.new_event_loop() + try: + mgr._loop = loop # type: ignore[attr-defined] + mgr._user_pool_entries = {} # type: ignore[attr-defined] + mgr._last_pool_notification_refresh = {} # type: ignore[attr-defined] + evicted: list[tuple[str, str]] = [] + + def _fake_evict(key: tuple[str, str]) -> None: + evicted.append(key) + + mgr._evict_session = _fake_evict # type: ignore[method-assign] + + # Run the dispatch on a separate thread so the loop can drain. + import threading + + done = threading.Event() + + def _run_loop() -> None: + loop.call_later(0.05, loop.stop) + loop.run_forever() + done.set() + + t = threading.Thread(target=_run_loop, daemon=True) + t.start() + mgr.evict_user_session("user-1", "srv-oauth") + done.wait(timeout=1.0) + + assert evicted == [("user-1", "srv-oauth")] + finally: + if not loop.is_closed(): + loop.close() diff --git a/tests/test_mcp_oauth_discovery.py b/tests/test_mcp_oauth_discovery.py index 10228b3e..8321c735 100644 --- a/tests/test_mcp_oauth_discovery.py +++ b/tests/test_mcp_oauth_discovery.py @@ -412,6 +412,7 @@ class TestMetadataCache: authorization_endpoint="https://as.example.com/authorize", token_endpoint="https://as.example.com/token", registration_endpoint=None, + revocation_endpoint=None, jwks_uri=None, code_challenge_methods_supported=("S256",), token_endpoint_auth_methods_supported=(), @@ -544,3 +545,82 @@ class TestCachedIssuerSSRFRevalidation: if c.kwargs.get("oauth_as_issuer_cached") is None ] assert clear_calls, "cached_issuer should have been cleared" + + +# --------------------------------------------------------------------------- +# revocation_endpoint parsing (RFC 8414) +# --------------------------------------------------------------------------- + + +class TestASMetadataRevocationEndpoint: + def test_as_metadata_parses_revocation_endpoint(self) -> None: + doc = _good_as_metadata_doc() + doc["revocation_endpoint"] = "https://as.example.com/revoke" + + client = MagicMock(spec=httpx.AsyncClient) + client.get = AsyncMock(return_value=_mk_response(200, doc)) + storage = _mk_storage_mock() + + async def _run() -> ASMetadata: + with _public_addr_patch(): + return await discover_authorization_server( + server_name="srv-x", + server_url="https://mcp.example.com/sse", + override_url="https://as.example.com", + cached_issuer=None, + http_client=client, + storage=storage, + server_id="srv-id", + trusted_hosts=frozenset(), + ) + + meta = asyncio.run(_run()) + assert meta.revocation_endpoint == "https://as.example.com/revoke" + + def test_as_metadata_revocation_endpoint_absent(self) -> None: + doc = _good_as_metadata_doc() + doc.pop("revocation_endpoint", None) + + client = MagicMock(spec=httpx.AsyncClient) + client.get = AsyncMock(return_value=_mk_response(200, doc)) + storage = _mk_storage_mock() + + async def _run() -> ASMetadata: + with _public_addr_patch(): + return await discover_authorization_server( + server_name="srv-x", + server_url="https://mcp.example.com/sse", + override_url="https://as.example.com", + cached_issuer=None, + http_client=client, + storage=storage, + server_id="srv-id", + trusted_hosts=frozenset(), + ) + + meta = asyncio.run(_run()) + assert meta.revocation_endpoint is None + + def test_as_metadata_revocation_endpoint_rejected_when_cross_origin(self) -> None: + doc = _good_as_metadata_doc() + doc["revocation_endpoint"] = "https://attacker.example.com/revoke" + + client = MagicMock(spec=httpx.AsyncClient) + client.get = AsyncMock(return_value=_mk_response(200, doc)) + storage = _mk_storage_mock() + + async def _run() -> ASMetadata: + with _public_addr_patch(): + return await discover_authorization_server( + server_name="srv-x", + server_url="https://mcp.example.com/sse", + override_url="https://as.example.com", + cached_issuer=None, + http_client=client, + storage=storage, + server_id="srv-id", + trusted_hosts=frozenset(), + ) + + with pytest.raises(MCPOAuthDiscoveryError, match="revocation_endpoint"): + asyncio.run(_run()) diff --git a/tests/test_mcp_oauth_handlers.py b/tests/test_mcp_oauth_handlers.py index da4da0a9..006a1ac7 100644 --- a/tests/test_mcp_oauth_handlers.py +++ b/tests/test_mcp_oauth_handlers.py @@ -311,6 +311,117 @@ class TestAuthorize: assert pending is not None assert pending["return_url"] == "/" + def test_authorize_scope_param_unioned_with_configured_scopes( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + with _public_addr_patch(): + resp = client.get( + "/v1/api/mcp/oauth/start?server=srv-oauth&scopes=email%20admin", + follow_redirects=False, + ) + + assert resp.status_code == 302 + params = urllib.parse.parse_qs(urllib.parse.urlparse(resp.headers["location"]).query) + # Configured = "openid profile"; requested = "email admin". + # Union sorted alphabetically. + scope_param = params["scope"][0] + assert sorted(scope_param.split(" ")) == ["admin", "email", "openid", "profile"] + + def test_authorize_scope_param_dedupe_and_sort( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + # Caller passes scopes that overlap with the configured set + an + # extra. Ordering is intentionally unsorted to exercise the + # deterministic-sort property. + with _public_addr_patch(): + resp = client.get( + "/v1/api/mcp/oauth/start?server=srv-oauth" + "&scopes=zzz%20openid%20aaa%20openid%20profile", + follow_redirects=False, + ) + + assert resp.status_code == 302 + params = urllib.parse.parse_qs(urllib.parse.urlparse(resp.headers["location"]).query) + # Sorted, deduped union. + assert params["scope"][0] == "aaa openid profile zzz" + + def test_authorize_scope_param_rejects_invalid_token( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + # CR/LF, NUL, backslash, and double-quote each individually fail + # the RFC 6749 §3.3 scope-token grammar; ``request.query_params`` + # already URL-decodes so we send the encoded form. + for hostile in ("a%0Db", "a%0Ab", "a%00b", 'a"b', "a\\b"): + with _public_addr_patch(): + resp = client.get( + f"/v1/api/mcp/oauth/start?server=srv-oauth&scopes={hostile}", + follow_redirects=False, + ) + assert resp.status_code == 400, f"hostile={hostile!r} got {resp.status_code}" + assert resp.json() == {"error": "Invalid scope token"} + + def test_authorize_scope_param_rejects_over_cap( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + # 33 tokens — one over the cap (``_MAX_INSUFFICIENT_SCOPE_REPORTED = 32``). + scopes = "%20".join(f"s{i}" for i in range(33)) + with _public_addr_patch(): + resp = client.get( + f"/v1/api/mcp/oauth/start?server=srv-oauth&scopes={scopes}", + follow_redirects=False, + ) + assert resp.status_code == 400 + assert resp.json() == {"error": "Invalid scope token"} + + def test_authorize_no_scope_param_passes_configured_scope_unchanged( + self, storage: SQLiteBackend, http_client_mock: MagicMock + ) -> None: + # Static-path invariant: when no ``scopes`` query param is passed, + # the AS sees the configured scope string verbatim (not run + # through sort/dedup). Configured = "openid profile" → AS + # receives "openid profile" in the same order. + _seed_oauth_user_server(storage) + token_store = _make_token_store(storage) + http_client_mock.get.return_value = _mk_response(200, _good_as_metadata_doc()) + + app = _build_app(storage=storage, http_client=http_client_mock, token_store=token_store) + client = TestClient(app, raise_server_exceptions=False) + + with _public_addr_patch(): + resp = client.get("/v1/api/mcp/oauth/start?server=srv-oauth", follow_redirects=False) + + assert resp.status_code == 302 + params = urllib.parse.parse_qs(urllib.parse.urlparse(resp.headers["location"]).query) + assert params["scope"] == ["openid profile"] + # --------------------------------------------------------------------------- # /callback diff --git a/tests/test_mcp_oauth_revoke.py b/tests/test_mcp_oauth_revoke.py new file mode 100644 index 00000000..c6f4cffc --- /dev/null +++ b/tests/test_mcp_oauth_revoke.py @@ -0,0 +1,399 @@ +"""Tests for :func:`turnstone.core.mcp_oauth.revoke_token_at_as`. + +The helper is best-effort RFC 7009 token revocation. It must: +- skip cleanly when the AS metadata doesn't advertise a revocation endpoint +- POST the form body when one is present (with optional client_secret) +- never raise on non-2xx, network errors, or timeouts — caller doesn't + want try/except in cleanup paths +- never use ``exc_info=True`` — chained ``__context__`` may carry an + ``httpx.Request`` whose ``Authorization`` header holds a bearer; the + bearer-leak invariant requires structured fields with type names only +""" + +from __future__ import annotations + +import asyncio +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx + +from turnstone.core.mcp_oauth import ( + ASMetadata, + MCPOAuthDiscoveryError, + _attempt_upstream_revoke, + revoke_token_at_as, +) + + +def _make_as_metadata( + *, + revocation_endpoint: str | None = "https://as.example.com/revoke", +) -> ASMetadata: + return ASMetadata( + issuer="https://as.example.com", + authorization_endpoint="https://as.example.com/authorize", + token_endpoint="https://as.example.com/token", + registration_endpoint=None, + revocation_endpoint=revocation_endpoint, + jwks_uri=None, + code_challenge_methods_supported=("S256",), + token_endpoint_auth_methods_supported=("client_secret_basic",), + ) + + +def _mk_response(status_code: int) -> MagicMock: + resp = MagicMock(spec=httpx.Response) + resp.status_code = status_code + return resp + + +class TestRevocationUnsupported: + def test_revoke_token_skipped_when_revocation_endpoint_none(self) -> None: + as_meta = _make_as_metadata(revocation_endpoint=None) + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock() + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + client.post.assert_not_called() + info_events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_unsupported" in info_events + + +class TestRevocationSuccess: + def test_revoke_token_succeeds_on_200(self) -> None: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(return_value=_mk_response(200)) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret="s-secret", + ) + ) + + # POST shape — URL + form body keys. + client.post.assert_awaited_once() + call_args = client.post.call_args + assert call_args.args[0] == "https://as.example.com/revoke" + body = call_args.kwargs["data"] + assert body == { + "token": "r-secret", + "token_type_hint": "refresh_token", + "client_id": "client-1", + "client_secret": "s-secret", + } + info_events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_succeeded" in info_events + + def test_revoke_token_omits_client_secret_when_none(self) -> None: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(return_value=_mk_response(200)) + + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + body = client.post.call_args.kwargs["data"] + assert "client_secret" not in body + assert body["token"] == "r-secret" + assert body["token_type_hint"] == "refresh_token" + assert body["client_id"] == "client-1" + + def test_revoke_token_succeeds_on_204(self) -> None: + # RFC 7009 says the AS MAY return any 2xx; treat the whole range + # as success. + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(return_value=_mk_response(204)) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + info_events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_succeeded" in info_events + + +class TestRevocationFailureLogged: + def _run_and_capture(self, status: int) -> list[Any]: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(return_value=_mk_response(status)) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + return mock_log.info.call_args_list + + def test_revoke_token_logs_on_400_does_not_raise(self) -> None: + calls = self._run_and_capture(400) + events = [c.args[0] for c in calls] + assert "mcp_server.oauth.revocation_failed" in events + # Must include status field. + failed_call = next(c for c in calls if c.args[0] == "mcp_server.oauth.revocation_failed") + assert failed_call.kwargs.get("status") == 400 + + def test_revoke_token_logs_on_401_does_not_raise(self) -> None: + calls = self._run_and_capture(401) + events = [c.args[0] for c in calls] + assert "mcp_server.oauth.revocation_failed" in events + failed_call = next(c for c in calls if c.args[0] == "mcp_server.oauth.revocation_failed") + assert failed_call.kwargs.get("status") == 401 + + def test_revoke_token_logs_on_403_does_not_raise(self) -> None: + calls = self._run_and_capture(403) + events = [c.args[0] for c in calls] + assert "mcp_server.oauth.revocation_failed" in events + + def test_revoke_token_logs_on_5xx_does_not_raise(self) -> None: + calls = self._run_and_capture(500) + events = [c.args[0] for c in calls] + assert "mcp_server.oauth.revocation_failed" in events + failed_call = next(c for c in calls if c.args[0] == "mcp_server.oauth.revocation_failed") + assert failed_call.kwargs.get("status") == 500 + + +class TestRevocationExceptionPaths: + def test_revoke_token_handles_network_error(self) -> None: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(side_effect=httpx.ConnectError("boom")) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_failed" in events + failed_call = next( + c + for c in mock_log.info.call_args_list + if c.args[0] == "mcp_server.oauth.revocation_failed" + ) + assert failed_call.kwargs.get("error") == "ConnectError" + + def test_revoke_token_handles_httpx_timeout(self) -> None: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(side_effect=httpx.TimeoutException("slow")) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_failed" in events + failed_call = next( + c + for c in mock_log.info.call_args_list + if c.args[0] == "mcp_server.oauth.revocation_failed" + ) + assert failed_call.kwargs.get("error") == "TimeoutException" + + def test_revoke_token_handles_asyncio_timeout(self) -> None: + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + + async def _slow(*_args: Any, **_kwargs: Any) -> Any: + await asyncio.sleep(10.0) + raise AssertionError("should have timed out") + + client.post = AsyncMock(side_effect=_slow) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + timeout_seconds=0.05, + ) + ) + + events = [c.args[0] for c in mock_log.info.call_args_list] + assert "mcp_server.oauth.revocation_failed" in events + failed_call = next( + c + for c in mock_log.info.call_args_list + if c.args[0] == "mcp_server.oauth.revocation_failed" + ) + # ``asyncio.timeout`` raises ``TimeoutError`` (Python's builtin) + # on cancellation. + assert failed_call.kwargs.get("error") == "TimeoutError" + + def test_revoke_token_no_exc_info_in_logs(self) -> None: + """Bearer-leak invariant: the revoke path must NEVER set + ``exc_info=True``. Chained ``__context__`` may include an + ``httpx.Request`` whose ``Authorization`` header holds a + bearer; the traceback formatter would render it. + """ + as_meta = _make_as_metadata() + client = MagicMock(spec=httpx.AsyncClient) + client.post = AsyncMock(side_effect=httpx.ConnectError("boom")) + + with patch("turnstone.core.mcp_oauth.log") as mock_log: + asyncio.run( + revoke_token_at_as( + as_metadata=as_meta, + http_client=client, + refresh_token="r-secret", + client_id="client-1", + client_secret=None, + ) + ) + + # No info call may carry exc_info. + for call in mock_log.info.call_args_list: + assert "exc_info" not in call.kwargs, ( + f"mcp_server.oauth log info({call.args[0]!r}) used exc_info — " + "this violates the bearer-leak invariant" + ) + # Defensively: also check warning + exception levels for the + # same call site. + for call in mock_log.warning.call_args_list: + assert "exc_info" not in call.kwargs + mock_log.exception.assert_not_called() + + +class TestAttemptUpstreamRevokeNeverRaises: + """Round-2 q-3 regression: ``_attempt_upstream_revoke``'s docstring + claims ``Never raises``. Background-task semantics make this load- + bearing — a propagated exception logs ``Task exception was never + retrieved`` because the ``set.discard`` done-callback doesn't read + ``task.exception()``. + + The wrapper's narrow inner ``except`` clauses (``MCPOAuthDiscoveryError``, + ``MCPTokenDecryptError``) leave room for any other exception type + raised by ``discover_authorization_server`` / + ``storage.get_mcp_oauth_client_secret_ct`` / ``token_store.cipher.decrypt`` + to escape. The outer ``try/except Exception`` is what keeps the + contract honest. These tests pin that gate. + """ + + def _build_args(self) -> dict[str, Any]: + token_store = MagicMock() + token_store.cipher = MagicMock() + token_store.cipher.decrypt.return_value = b"shh" + storage = MagicMock() + storage.get_mcp_oauth_client_secret_ct.return_value = None + return { + "http_client": MagicMock(spec=httpx.AsyncClient), + "metadata_cache": None, + "storage": storage, + "token_store": token_store, + "server_name": "srv-oauth", + "server_row": { + "url": "https://mcp.example.com", + "oauth_client_id": "client-1", + "oauth_authorization_server_url": None, + "oauth_as_issuer_cached": None, + }, + "server_id_for_audit": "srv-id-1", + "refresh_token": "r-secret", + } + + def test_attempt_upstream_revoke_swallows_unexpected_exception(self) -> None: + """A generic exception from a path the inner handlers don't + cover MUST be caught at the outer boundary and logged with type + name only (no exc_info=True per the bearer-leak invariant). + """ + args = self._build_args() + + async def _boom(*_a: Any, **_kw: Any) -> Any: + raise RuntimeError("network blew up") + + with ( + patch("turnstone.core.mcp_oauth.discover_authorization_server", side_effect=_boom), + patch("turnstone.core.mcp_oauth.log") as mock_log, + ): + # MUST NOT raise. + asyncio.run(_attempt_upstream_revoke(**args)) + + events = [call.args[0] for call in mock_log.info.call_args_list] + assert "mcp_server.oauth.upstream_revoke_failed" in events, ( + "outer try/except must log mcp_server.oauth.upstream_revoke_failed " + "with the exception type name when an unexpected exception escapes " + "the narrow inner handlers" + ) + for call in mock_log.info.call_args_list: + assert "exc_info" not in call.kwargs, ( + "outer-block log must not use exc_info=True — chained " + "__context__ may carry an httpx.Request bearer" + ) + + def test_attempt_upstream_revoke_logs_discovery_failure(self) -> None: + """Round-2 bug-1: ``MCPOAuthDiscoveryError`` MUST emit + ``upstream_revoke_discovery_failed`` so operators have visibility + into a silent-discovery-failure path that previously logged + nothing while the audit row recorded ``upstream_revoke_outcome=scheduled``. + """ + args = self._build_args() + + async def _disc_fail(*_a: Any, **_kw: Any) -> Any: + raise MCPOAuthDiscoveryError("PRM fetch 503") + + with ( + patch( + "turnstone.core.mcp_oauth.discover_authorization_server", + side_effect=_disc_fail, + ), + patch("turnstone.core.mcp_oauth.log") as mock_log, + ): + asyncio.run(_attempt_upstream_revoke(**args)) + + events = [call.args[0] for call in mock_log.info.call_args_list] + assert "mcp_server.oauth.upstream_revoke_discovery_failed" in events + assert "mcp_server.oauth.upstream_revoke_failed" not in events diff --git a/tests/test_mcp_oauth_storage.py b/tests/test_mcp_oauth_storage.py index da78b725..ad2919bf 100644 --- a/tests/test_mcp_oauth_storage.py +++ b/tests/test_mcp_oauth_storage.py @@ -123,3 +123,90 @@ class TestGetOAuthClientSecretCt: def test_returns_none_for_missing_server(self, backend) -> None: assert backend.get_mcp_oauth_client_secret_ct("does-not-exist") is None + + +def _create_user_token_row( + backend, + *, + user_id: str, + server_name: str, + created: str, +) -> None: + """Insert a token row + backdate ``created`` so ordering is deterministic. + + The storage helper stamps ``created`` from ``datetime.now(UTC)``; for + multi-row ordering tests we backdate via raw SQL so the inserts stay + independent of clock resolution. + """ + backend.create_mcp_user_token( + user_id, + server_name, + access_token_ct=b"ct-access", + refresh_token_ct=b"ct-refresh", + expires_at="2026-05-04T12:00:00", + scopes="openid", + as_issuer="https://auth.example.com", + audience="https://mcp.example.com", + ) + with backend._engine.connect() as conn: + conn.execute( + sa.text( + "UPDATE mcp_user_tokens SET created = :created " + "WHERE user_id = :uid AND server_name = :sn" + ), + {"created": created, "uid": user_id, "sn": server_name}, + ) + conn.commit() + + +class TestListMCPUserTokenMetadataByUser: + def test_list_mcp_user_token_metadata_by_user_empty(self, backend) -> None: + assert backend.list_mcp_user_token_metadata_by_user("nobody") == [] + + def test_list_mcp_user_token_metadata_by_user_single_server(self, backend) -> None: + _create_user_token_row( + backend, user_id="u1", server_name="srv-a", created="2026-05-01T00:00:00" + ) + rows = backend.list_mcp_user_token_metadata_by_user("u1") + assert len(rows) == 1 + assert rows[0]["user_id"] == "u1" + assert rows[0]["server_name"] == "srv-a" + assert rows[0]["as_issuer"] == "https://auth.example.com" + assert rows[0]["audience"] == "https://mcp.example.com" + assert rows[0]["scopes"] == "openid" + # Projection MUST omit ciphertext columns — the SQL no longer + # selects them, so the TypedDict has no key. + assert "access_token_ct" not in rows[0] + assert "refresh_token_ct" not in rows[0] + + def test_list_mcp_user_token_metadata_by_user_multiple_servers(self, backend) -> None: + _create_user_token_row( + backend, user_id="u1", server_name="srv-c", created="2026-05-03T00:00:00" + ) + _create_user_token_row( + backend, user_id="u1", server_name="srv-a", created="2026-05-01T00:00:00" + ) + _create_user_token_row( + backend, user_id="u1", server_name="srv-b", created="2026-05-02T00:00:00" + ) + rows = backend.list_mcp_user_token_metadata_by_user("u1") + assert [r["server_name"] for r in rows] == ["srv-a", "srv-b", "srv-c"] + + def test_list_mcp_user_token_metadata_by_user_isolates_by_user(self, backend) -> None: + _create_user_token_row( + backend, user_id="user-a", server_name="srv-a", created="2026-05-01T00:00:00" + ) + _create_user_token_row( + backend, user_id="user-a", server_name="srv-b", created="2026-05-02T00:00:00" + ) + _create_user_token_row( + backend, user_id="user-b", server_name="srv-a", created="2026-05-03T00:00:00" + ) + rows_a = backend.list_mcp_user_token_metadata_by_user("user-a") + assert {r["server_name"] for r in rows_a} == {"srv-a", "srv-b"} + assert all(r["user_id"] == "user-a" for r in rows_a) + + rows_b = backend.list_mcp_user_token_metadata_by_user("user-b") + assert len(rows_b) == 1 + assert rows_b[0]["user_id"] == "user-b" + assert rows_b[0]["server_name"] == "srv-a" diff --git a/tests/test_mcp_pool_auth_integration.py b/tests/test_mcp_pool_auth_integration.py index 27d8ade6..54bdb82b 100644 --- a/tests/test_mcp_pool_auth_integration.py +++ b/tests/test_mcp_pool_auth_integration.py @@ -402,14 +402,21 @@ def test_integration_401_with_refresh_failure_emits_consent_required( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.call_tool_sync( + ), pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 ) - payload = json.loads(result) + # Structured-error envelopes flow back via ``RuntimeError(json_str)`` + # so the session-layer ``except Exception`` handler routes the + # consent card uniformly across tool / resource / prompt dispatchers. + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert payload["error"]["server"] == "pool-srv" + # Phase 8 — consent_url surfaces a /start URL the dashboard can open + # in a popup. URL-encoded server name; no scopes baked in (the AS + # picks up the configured scopes server-side at /start). + assert payload["error"]["consent_url"] == "/v1/api/mcp/oauth/start?server=pool-srv" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -440,14 +447,19 @@ def test_integration_403_insufficient_scope_emits_structured_error( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.call_tool_sync( + ), pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_insufficient_scope" assert payload["error"]["scopes_required"] == ["files:write", "mail:send"] + # Phase 8 — consent_url carries the step-up scopes URL-encoded so the + # dashboard can union them with the configured set at /start. + assert payload["error"]["consent_url"] == ( + "/v1/api/mcp/oauth/start?server=pool-srv&scopes=files%3Awrite%20mail%3Asend" + ) # No retry — exactly ONE POST attempted before the structured error. post_headers = behaviour.get("post_auth_headers", []) assert len(post_headers) == 1, ( @@ -480,12 +492,12 @@ def test_integration_403_no_insufficient_scope_emits_generic_forbidden( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.call_tool_sync( + ), pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_tool_call_forbidden" assert "scopes_required" not in payload["error"] post_headers = behaviour.get("post_auth_headers", []) @@ -554,12 +566,12 @@ def test_integration_403_multi_www_authenticate_drops_injected_scopes( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.call_tool_sync( + ), pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_insufficient_scope", ( f"expected mcp_insufficient_scope; got {payload!r}" ) @@ -606,12 +618,12 @@ def test_integration_401_retry_ceiling( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.call_tool_sync( + ), pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" # Exactly ONE refresh round-trip. assert refresh_count == 1, f"expected exactly 1 refresh round-trip; got {refresh_count}" @@ -654,10 +666,11 @@ def test_integration_breaker_unaffected_by_auth_failures( side_effect=_fake_classified, ): for _ in range(50): - result = mgr.call_tool_sync( - "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 - ) - payload = json.loads(result) + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( + "mcp__pool-srv__echo", {"payload": "x"}, user_id="user-1", timeout=15 + ) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -718,6 +731,55 @@ def test_integration_static_path_unaffected( ) +# --------------------------------------------------------------------------- +# Test 27b: static dispatch unaffected by Phase 8 consent_url kwarg +# --------------------------------------------------------------------------- + + +def test_static_dispatch_unaffected_by_consent_url_kwarg( + upstream: Any, running_loop_mgr: Any, storage: SQLiteBackend +) -> None: + """Static-auth tool dispatch must be byte-identical post-Phase 8. + + The Phase 8 changes only ADD a ``consent_url`` kwarg to + ``_structured_error`` invocations on the pool path. Static dispatch + must not pick up the field — there's no consent flow for + ``auth_type='none'`` / ``'static'`` servers, and exposing one would + confuse the dashboard renderer. Asserts a successful tool result is + a plain string with no JSON envelope and no ``consent_url`` substring. + """ + url, behaviour = upstream + behaviour["mode"] = "never" # passthrough — succeeds + + mgr, loop, _ = running_loop_mgr + cfg = {"type": "streamable-http", "url": url} + + async def _connect_static() -> None: + await mgr._connect_one("static-srv", cfg) + + fut = asyncio.run_coroutine_threadsafe(_connect_static(), loop) + fut.result(timeout=15) + + # Drive call_tool_sync without a user_id — the static path is taken. + result = mgr.call_tool_sync("mcp__static-srv__echo", {"payload": "static-x"}, timeout=15) + + # Static path returns the FastMCP fixture's echo string. + assert "echoed:static-x" in result + # No JSON envelope leaked through; specifically no consent_url field. + assert "consent_url" not in result, ( + f"Static-auth tool dispatch surfaced a consent_url; result: {result!r}" + ) + # Defensive: result is not a JSON-encoded structured error. + try: + parsed = json.loads(result) + except (json.JSONDecodeError, ValueError): + parsed = None + if isinstance(parsed, dict): + assert "error" not in parsed, ( + f"Static-auth dispatch returned a structured-error envelope; got {parsed!r}" + ) + + # --------------------------------------------------------------------------- # Test 28: pool reuse — 401 on a SECOND dispatch (carrier owned by entry) # --------------------------------------------------------------------------- diff --git a/tests/test_mcp_pool_auth_introspection.py b/tests/test_mcp_pool_auth_introspection.py index d8f94bca..b46ceb3b 100644 --- a/tests/test_mcp_pool_auth_introspection.py +++ b/tests/test_mcp_pool_auth_introspection.py @@ -683,18 +683,21 @@ class TestDispatcherAuthFlows: self._seed_pool_entry_with_call_tool(mgr, loop, _call_tool) - with patch( - "turnstone.core.mcp_client.get_user_access_token_classified", - side_effect=_fake_classified, + with ( + patch( + "turnstone.core.mcp_client.get_user_access_token_classified", + side_effect=_fake_classified, + ), + pytest.raises(RuntimeError) as exc_info, ): - result = mgr.call_tool_sync( + mgr.call_tool_sync( "mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5, ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert payload["error"]["server"] == "pool-srv" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -742,18 +745,21 @@ class TestDispatcherAuthFlows: self._seed_pool_entry_with_call_tool(mgr, loop, _call_tool) - with patch( - "turnstone.core.mcp_client.get_user_access_token_classified", - side_effect=_fake_classified, + with ( + patch( + "turnstone.core.mcp_client.get_user_access_token_classified", + side_effect=_fake_classified, + ), + pytest.raises(RuntimeError) as exc_info, ): - result = mgr.call_tool_sync( + mgr.call_tool_sync( "mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5, ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" # Exactly TWO calls: initial + retry. No recursion. assert call_count == 2, ( @@ -794,18 +800,21 @@ class TestDispatcherAuthFlows: self._seed_pool_entry_with_call_tool(mgr, loop, _call_tool) - with patch( - "turnstone.core.mcp_client.get_user_access_token_classified", - side_effect=_fake_classified, + with ( + patch( + "turnstone.core.mcp_client.get_user_access_token_classified", + side_effect=_fake_classified, + ), + pytest.raises(RuntimeError) as exc_info, ): - result = mgr.call_tool_sync( + mgr.call_tool_sync( "mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5, ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_insufficient_scope" assert payload["error"]["scopes_required"] == ["files:write", "mail:send"] assert call_count == 1, "403 must NOT trigger a retry" @@ -836,18 +845,21 @@ class TestDispatcherAuthFlows: self._seed_pool_entry_with_call_tool(mgr, loop, _call_tool) - with patch( - "turnstone.core.mcp_client.get_user_access_token_classified", - side_effect=_fake_classified, + with ( + patch( + "turnstone.core.mcp_client.get_user_access_token_classified", + side_effect=_fake_classified, + ), + pytest.raises(RuntimeError) as exc_info, ): - result = mgr.call_tool_sync( + mgr.call_tool_sync( "mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5, ) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_tool_call_forbidden" assert "scopes_required" not in payload["error"] @@ -947,14 +959,15 @@ class TestBreakerInvariant: assert result_401_retry == "ok" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 - with patch( - "turnstone.core.mcp_client.get_user_access_token_classified", - side_effect=_fake_classified, + with ( + patch( + "turnstone.core.mcp_client.get_user_access_token_classified", + side_effect=_fake_classified, + ), + pytest.raises(RuntimeError) as exc_info, ): - result_403 = mgr.call_tool_sync( - "mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5 - ) - payload = json.loads(result_403) + mgr.call_tool_sync("mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_insufficient_scope" # STILL zero after the 403 cycle. assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -1310,5 +1323,218 @@ def _install_capture_intercept(monkeypatch: pytest.MonkeyPatch) -> None: ) +# --------------------------------------------------------------------------- +# bug-1 — pool dispatchers RAISE on structured-error envelopes +# +# Pre-fix, ``_dispatch_pool`` and ``_dispatch_pool_resource`` returned +# ``_structured_error(...)`` JSON in the success-shape return slot, which +# the public ``call_tool_sync`` / ``read_resource_sync`` then forwarded +# verbatim. ``ChatSession._exec_mcp_tool`` / ``_exec_read_resource`` +# saw a successful return, called ``_report_tool_result(..., is_error=False)``, +# and the dashboard's ``appendToolOutput`` short-circuited +# ``tryParseMcpError`` because that gate fires only inside the +# ``isError`` branch. The interactive consent card NEVER rendered. +# +# The fix wraps the dispatcher's final return: when the result is a +# structured-error envelope (``_is_structured_error``), the sync +# wrapper raises ``RuntimeError(json_str)``. The agent-loop's +# ``except Exception`` branch then calls ``_format_mcp_dispatch_error`` +# which preserves the JSON on the structured-error path, and +# ``_report_tool_result(..., is_error=True)`` fires — the dashboard's +# tryParseMcpError gate opens and the card renders. +# +# These tests pin the contract at the public API. Each parametrized +# case mocks ``_dispatch_pool*`` to RETURN (not raise) the structured +# error string and asserts ``call_tool_sync`` / ``read_resource_sync`` +# / ``get_prompt_sync`` raise ``RuntimeError`` carrying the JSON. +# --------------------------------------------------------------------------- + + +_BUG1_STRUCTURED_ERROR_CODES = ( + "mcp_consent_required", + "mcp_insufficient_scope", + "mcp_tool_call_forbidden", + "mcp_token_undecryptable_key_unknown", + "mcp_oauth_url_insecure", +) + + +def _make_structured_error_json(code: str, *, server: str = "pool-srv") -> str: + payload: dict[str, Any] = { + "error": { + "code": code, + "server": server, + "detail": f"test-fixture-{code}", + } + } + if code == "mcp_insufficient_scope": + payload["error"]["scopes_required"] = ["files:write"] + payload["error"]["consent_url"] = ( + "/v1/api/mcp/oauth/start?server=pool-srv&scopes=files%3Awrite" + ) + elif code == "mcp_consent_required": + payload["error"]["consent_url"] = "/v1/api/mcp/oauth/start?server=pool-srv" + return json.dumps(payload) + + +@pytest.mark.parametrize("code", _BUG1_STRUCTURED_ERROR_CODES) +def test_call_tool_sync_raises_on_structured_error_envelope( + code: str, running_loop_mgr, storage: SQLiteBackend +) -> None: + """End-to-end regression for the consent-card non-rendering bug. + + Mocks ``_dispatch_pool`` to RETURN the structured-error JSON in the + success slot (the pre-fix production shape). The public + ``call_tool_sync`` MUST raise ``RuntimeError`` carrying the JSON + so the session-layer ``except Exception`` handler runs and + ``_report_tool_result(..., is_error=True)`` fires. + """ + mgr, _loop, _ = running_loop_mgr + cipher = make_mcp_token_cipher() + _seed_oauth_server(storage, name="pool-srv") + _seed_user_token(storage, cipher) + mgr.set_storage(storage) + mgr.set_app_state(_make_app_state(storage, cipher=cipher)) + + json_payload = _make_structured_error_json(code) + + async def _fake_dispatch_pool(**_kwargs: Any) -> str: + return json_payload + + mgr._dispatch_pool = _fake_dispatch_pool # type: ignore[method-assign] + + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync("mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5) + + # The exception text MUST be the structured-error JSON byte-for-byte + # (so ``_format_mcp_dispatch_error`` recognises the envelope and + # surfaces it intact to the dashboard). + assert str(exc_info.value) == json_payload + decoded = json.loads(str(exc_info.value)) + assert decoded["error"]["code"] == code + + +@pytest.mark.parametrize("code", _BUG1_STRUCTURED_ERROR_CODES) +def test_read_resource_sync_raises_on_structured_error_envelope( + code: str, running_loop_mgr, storage: SQLiteBackend +) -> None: + """Mirror of the tool path's bug-1 regression for resource reads.""" + mgr, loop, _ = running_loop_mgr + cipher = make_mcp_token_cipher() + _seed_oauth_server(storage, name="pool-srv") + _seed_user_token(storage, cipher) + mgr.set_storage(storage) + mgr.set_app_state(_make_app_state(storage, cipher=cipher)) + + # Seed the resource map so the resolver finds ``res://hello`` and + # routes through ``_dispatch_pool_resource_sync``. + async def _seed() -> None: + entry = await mgr._ensure_pool_entry(("user-1", "pool-srv")) + entry.resources = [ + { + "uri": "res://hello", + "name": "", + "description": "", + "mimeType": "", + "server": "pool-srv", + } + ] + mgr._rebuild_user_resource_map("user-1") + + asyncio.run_coroutine_threadsafe(_seed(), loop).result(timeout=5) + + json_payload = _make_structured_error_json(code) + + async def _fake_dispatch_pool_resource(**_kwargs: Any) -> str: + return json_payload + + mgr._dispatch_pool_resource = _fake_dispatch_pool_resource # type: ignore[method-assign] + + with pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=5) + + assert str(exc_info.value) == json_payload + decoded = json.loads(str(exc_info.value)) + assert decoded["error"]["code"] == code + + +@pytest.mark.parametrize("code", _BUG1_STRUCTURED_ERROR_CODES) +def test_get_prompt_sync_raises_on_structured_error_envelope( + code: str, running_loop_mgr, storage: SQLiteBackend +) -> None: + """Mirror of the tool path's bug-1 regression for prompt invocation. + + The prompt path already converted the structured-error string to a + ``RuntimeError`` via the ``isinstance(result, str)`` check in + ``_dispatch_pool_prompt_sync`` (success type is ``list[dict]`` so a + str return is unambiguously the failure path). This test pins the + behaviour at the public API so the contract stays uniform across + tool / resource / prompt dispatchers post-bug-1. + """ + mgr, loop, _ = running_loop_mgr + cipher = make_mcp_token_cipher() + _seed_oauth_server(storage, name="pool-srv") + _seed_user_token(storage, cipher) + mgr.set_storage(storage) + mgr.set_app_state(_make_app_state(storage, cipher=cipher)) + + async def _seed() -> None: + entry = await mgr._ensure_pool_entry(("user-1", "pool-srv")) + entry.prompts = [ + { + "name": "mcp__pool-srv__greet", + "original_name": "greet", + "server": "pool-srv", + "description": "", + "arguments": [], + } + ] + mgr._rebuild_user_prompt_map("user-1") + + asyncio.run_coroutine_threadsafe(_seed(), loop).result(timeout=5) + + json_payload = _make_structured_error_json(code) + + async def _fake_dispatch_pool_prompt(**_kwargs: Any) -> str: + return json_payload + + mgr._dispatch_pool_prompt = _fake_dispatch_pool_prompt # type: ignore[method-assign] + + with pytest.raises(RuntimeError) as exc_info: + mgr.get_prompt_sync("mcp__pool-srv__greet", {}, user_id="user-1", timeout=5) + + assert str(exc_info.value) == json_payload + decoded = json.loads(str(exc_info.value)) + assert decoded["error"]["code"] == code + + +def test_call_tool_sync_does_not_wrap_non_structured_string( + running_loop_mgr, storage: SQLiteBackend +) -> None: + """A success-shape return whose payload merely happens to start with + ``{"error":...`` but lacks an ``mcp_*`` code MUST flow back to the + caller as a string, not a raised RuntimeError. This is the bug-1 + fix's defensive gate: only structured-error envelopes are wrapped. + """ + mgr, _loop, _ = running_loop_mgr + cipher = make_mcp_token_cipher() + _seed_oauth_server(storage, name="pool-srv") + _seed_user_token(storage, cipher) + mgr.set_storage(storage) + mgr.set_app_state(_make_app_state(storage, cipher=cipher)) + + # Tool output that happens to look JSON-ish but isn't a Phase 7b + # ``_structured_error`` envelope. Must round-trip as a plain string. + payload = json.dumps({"error": {"code": "tool_specific_failure", "msg": "x"}}) + + async def _fake_dispatch_pool(**_kwargs: Any) -> str: + return payload + + mgr._dispatch_pool = _fake_dispatch_pool # type: ignore[method-assign] + + result = mgr.call_tool_sync("mcp__pool-srv__do_thing", {}, user_id="user-1", timeout=5) + assert result == payload + + # Suppress unused-import warning for AsyncMock. _ = AsyncMock diff --git a/tests/test_mcp_pool_auth_resource_integration.py b/tests/test_mcp_pool_auth_resource_integration.py index 27320e7f..ab810af4 100644 --- a/tests/test_mcp_pool_auth_resource_integration.py +++ b/tests/test_mcp_pool_auth_resource_integration.py @@ -360,10 +360,10 @@ def test_resource_read_persistent_401_emits_consent_required( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert payload["error"]["server"] == "pool-srv" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -395,10 +395,10 @@ def test_resource_read_403_insufficient_scope_emits_structured_error( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_insufficient_scope" assert payload["error"]["scopes_required"] == ["files:read"] post_headers = behaviour.get("post_auth_headers", []) @@ -433,10 +433,10 @@ def test_resource_read_403_generic_forbidden( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) # Per the kind="resource" wiring of `_handle_auth_403`, the # operation-specific code surfaces here rather than the tool path's # generic mcp_tool_call_forbidden. @@ -479,9 +479,9 @@ def test_resource_read_breaker_unaffected_by_auth_failures( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) - payload = json.loads(result) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=15) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert mgr._consecutive_failures.get("pool-srv", 0) == 0 @@ -508,10 +508,10 @@ def test_resource_read_missing_token_emits_consent_required( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=10) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=10) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" @@ -532,10 +532,10 @@ def test_resource_read_decrypt_failure_emits_token_undecryptable( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=10) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=10) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_token_undecryptable_key_unknown" @@ -559,10 +559,10 @@ def test_resource_read_http_url_emits_url_insecure( with patch( "turnstone.core.mcp_client.get_user_access_token_classified", side_effect=_fake_classified, - ): - result = mgr.read_resource_sync("res://hello", user_id="user-1", timeout=5) + ), pytest.raises(RuntimeError) as exc_info: + mgr.read_resource_sync("res://hello", user_id="user-1", timeout=5) - payload = json.loads(result) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_oauth_url_insecure" diff --git a/tests/test_mcp_token_store_metadata.py b/tests/test_mcp_token_store_metadata.py new file mode 100644 index 00000000..d6060348 --- /dev/null +++ b/tests/test_mcp_token_store_metadata.py @@ -0,0 +1,107 @@ +"""Tests for ``MCPTokenStore.list_user_token_metadata``. + +Validates the non-secret projection used by the settings UI: ciphertext +columns are stripped, ordering is preserved, and the empty case returns +``[]``. Decrypt is intentionally skipped — the list view must never need +the access/refresh secrets. +""" + +from __future__ import annotations + +import base64 + +import sqlalchemy as sa +from cryptography.fernet import Fernet + +from turnstone.core.mcp_crypto import ( + MCPTokenCipher, + MCPTokenCipherConfig, + MCPTokenStore, +) + + +def _make_cipher() -> MCPTokenCipher: + raw = base64.urlsafe_b64decode(Fernet.generate_key()) + return MCPTokenCipher(MCPTokenCipherConfig(keys=(raw,))) + + +def _make_store(backend) -> MCPTokenStore: + return MCPTokenStore(backend, _make_cipher(), node_id="test-node") + + +def _seed_token( + store: MCPTokenStore, + backend, + *, + user_id: str, + server_name: str, + created: str, +) -> None: + """Create a token via the store and backdate ``created`` for ordering.""" + store.create_user_token( + user_id, + server_name, + access_token="access-secret", + refresh_token="refresh-secret", + expires_at="2026-05-04T12:00:00", + scopes="openid profile", + as_issuer="https://auth.example.com", + audience="https://mcp.example.com", + ) + with backend._engine.connect() as conn: + conn.execute( + sa.text( + "UPDATE mcp_user_tokens SET created = :created " + "WHERE user_id = :uid AND server_name = :sn" + ), + {"created": created, "uid": user_id, "sn": server_name}, + ) + conn.commit() + + +class TestListUserTokenMetadata: + def test_list_user_token_metadata_returns_non_secret_fields_only(self, backend) -> None: + store = _make_store(backend) + _seed_token( + store, backend, user_id="u1", server_name="srv-a", created="2026-05-01T00:00:00" + ) + rows = store.list_user_token_metadata("u1") + assert len(rows) == 1 + meta = rows[0] + # Secrets MUST be absent. + assert "access_token" not in meta + assert "refresh_token" not in meta + assert "access_token_ct" not in meta + assert "refresh_token_ct" not in meta + # Non-secret columns surface verbatim. + assert meta["user_id"] == "u1" + assert meta["server_name"] == "srv-a" + assert meta["scopes"] == "openid profile" + assert meta["as_issuer"] == "https://auth.example.com" + assert meta["audience"] == "https://mcp.example.com" + assert meta["expires_at"] == "2026-05-04T12:00:00" + assert meta["created"] == "2026-05-01T00:00:00" + assert meta["last_refreshed"] is None + + def test_list_user_token_metadata_empty(self, backend) -> None: + store = _make_store(backend) + assert store.list_user_token_metadata("nobody") == [] + + def test_list_user_token_metadata_preserves_creation_order(self, backend) -> None: + store = _make_store(backend) + _seed_token( + store, backend, user_id="u1", server_name="srv-c", created="2026-05-03T00:00:00" + ) + _seed_token( + store, backend, user_id="u1", server_name="srv-a", created="2026-05-01T00:00:00" + ) + _seed_token( + store, backend, user_id="u1", server_name="srv-b", created="2026-05-02T00:00:00" + ) + rows = store.list_user_token_metadata("u1") + assert [r["server_name"] for r in rows] == ["srv-a", "srv-b", "srv-c"] + assert [r["created"] for r in rows] == [ + "2026-05-01T00:00:00", + "2026-05-02T00:00:00", + "2026-05-03T00:00:00", + ] diff --git a/tests/test_mcp_user_pool.py b/tests/test_mcp_user_pool.py index 18289a85..6f2c6f18 100644 --- a/tests/test_mcp_user_pool.py +++ b/tests/test_mcp_user_pool.py @@ -390,13 +390,14 @@ class TestDispatchStateMachine: _seed_oauth_server(storage, name="pool-srv") self._wire_pool(mgr, storage, cipher) - result = mgr.call_tool_sync( - "mcp__pool-srv__do_thing", - {}, - user_id="user-1", - timeout=5, - ) - payload = json.loads(result) + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( + "mcp__pool-srv__do_thing", + {}, + user_id="user-1", + timeout=5, + ) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" assert payload["error"]["server"] == "pool-srv" @@ -419,13 +420,14 @@ class TestDispatchStateMachine: state.mcp_token_store.get_user_token = _raise - result = mgr.call_tool_sync( - "mcp__pool-srv__do_thing", - {}, - user_id="user-1", - timeout=5, - ) - payload = json.loads(result) + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( + "mcp__pool-srv__do_thing", + {}, + user_id="user-1", + timeout=5, + ) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_token_undecryptable_key_unknown" # Operator fingerprints stay server-side (audit log + structured log); # the agent-facing payload must NOT carry them onward to the LLM @@ -454,13 +456,14 @@ class TestDispatchStateMachine: audience="https://mcp.example.com", ) - result = mgr.call_tool_sync( - "mcp__pool-srv__do_thing", - {}, - user_id="user-1", - timeout=5, - ) - payload = json.loads(result) + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( + "mcp__pool-srv__do_thing", + {}, + user_id="user-1", + timeout=5, + ) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_consent_required" def test_token_present_dispatches_to_session( @@ -634,13 +637,14 @@ class TestHttpsEnforcement: state = _make_app_state(storage, cipher=cipher) mgr.set_app_state(state) - result = mgr.call_tool_sync( - "mcp__pool-srv__do_thing", - {}, - user_id="user-1", - timeout=5, - ) - payload = json.loads(result) + with pytest.raises(RuntimeError) as exc_info: + mgr.call_tool_sync( + "mcp__pool-srv__do_thing", + {}, + user_id="user-1", + timeout=5, + ) + payload = json.loads(str(exc_info.value)) assert payload["error"]["code"] == "mcp_oauth_url_insecure" assert payload["error"]["server"] == "pool-srv" diff --git a/tests/test_session_mcp_dispatch_error.py b/tests/test_session_mcp_dispatch_error.py new file mode 100644 index 00000000..b384beb7 --- /dev/null +++ b/tests/test_session_mcp_dispatch_error.py @@ -0,0 +1,235 @@ +"""Tests for ``_format_mcp_dispatch_error`` and the three MCP exec sites. + +The Phase 7b pool dispatcher signals user-actionable failures (consent +required, insufficient scope) via ``RuntimeError(json_str)`` where +``json_str`` is the structured-error payload built by +:func:`turnstone.core.mcp_client._structured_error`. The exec sites in +:mod:`turnstone.core.session` previously wrapped that JSON in +``f"MCP X error: {e}"``, destroying the structured shape the dashboard +renderer keys on. The helper preserves the JSON when the exception +text decodes to a structured-error envelope and prefixes otherwise. + +Sibling-bug coverage: every exec site (tool / read_resource / +use_prompt) gets two assertions — JSON preserved on a consent-required +exception, JSON-prefixed on a generic transport failure. +""" + +from __future__ import annotations + +import json +from unittest.mock import MagicMock, patch + +from tests.test_session import _make_session +from turnstone.core.session import _format_mcp_dispatch_error + +# --------------------------------------------------------------------------- +# Unit tests for the helper +# --------------------------------------------------------------------------- + + +class TestFormatMcpDispatchError: + def test_preserves_consent_required_payload(self) -> None: + payload = json.dumps( + { + "error": { + "code": "mcp_consent_required", + "server": "srv-x", + "detail": "No token for user. Consent flow required.", + "consent_url": "/v1/api/mcp/oauth/start?server=srv-x", + } + } + ) + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError(payload)) + assert out == payload + + def test_preserves_insufficient_scope_payload(self) -> None: + payload = json.dumps( + { + "error": { + "code": "mcp_insufficient_scope", + "server": "srv-x", + "detail": "Tool requires elevated scopes.", + "scopes_required": ["read", "write"], + "consent_url": "/v1/api/mcp/oauth/start?server=srv-x&scopes=read+write", + } + } + ) + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError(payload)) + assert out == payload + + def test_prefixes_generic_runtime_error(self) -> None: + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError("connection lost")) + assert out == "MCP tool error: connection lost" + + def test_prefixes_value_error(self) -> None: + out = _format_mcp_dispatch_error("MCP tool error", ValueError("bad input")) + assert out == "MCP tool error: bad input" + + def test_prefixes_random_json_without_mcp_code(self) -> None: + # JSON that isn't a structured-error envelope must NOT be passed + # through verbatim — the helper only opens the gate for codes + # prefixed ``mcp_``. + payload = json.dumps({"foo": "bar"}) + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError(payload)) + assert out == f"MCP tool error: {payload}" + + def test_prefixes_envelope_with_non_mcp_code(self) -> None: + payload = json.dumps({"error": {"code": "other_error", "server": "x", "detail": "y"}}) + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError(payload)) + assert out == f"MCP tool error: {payload}" + + def test_prefixes_envelope_without_dict_error(self) -> None: + payload = json.dumps({"error": "plain string"}) + out = _format_mcp_dispatch_error("MCP tool error", RuntimeError(payload)) + assert out == f"MCP tool error: {payload}" + + +# --------------------------------------------------------------------------- +# Integration tests against the three MCP exec sites +# --------------------------------------------------------------------------- + + +_CONSENT_REQUIRED_JSON = json.dumps( + { + "error": { + "code": "mcp_consent_required", + "server": "srv-oauth", + "detail": "No token for user. Consent flow required.", + "consent_url": "/v1/api/mcp/oauth/start?server=srv-oauth", + } + } +) + + +def _record_outputs(session) -> list[tuple[str, str, str, bool]]: + """Patch ``_report_tool_result`` to capture (call_id, name, output, is_error).""" + captures: list[tuple[str, str, str, bool]] = [] + + def _capture(call_id: str, name: str, output: str, *, is_error: bool = False) -> None: + captures.append((call_id, name, output, is_error)) + + session._report_tool_result = _capture # type: ignore[method-assign] + return captures + + +class TestExecMcpToolDispatchError: + def test_exec_mcp_tool_preserves_structured_error_json(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.call_tool_sync.side_effect = RuntimeError(_CONSENT_REQUIRED_JSON) + session._mcp_client = mock_client + + item = { + "call_id": "tc_1", + "mcp_func_name": "mcp__srv-oauth__do", + "mcp_args": {}, + } + session._exec_mcp_tool(item) + + assert len(captures) == 1 + _, _, output, is_error = captures[0] + assert output == _CONSENT_REQUIRED_JSON + assert is_error is True + + def test_exec_mcp_tool_prefixes_non_structured_error(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.call_tool_sync.side_effect = RuntimeError("connection lost") + session._mcp_client = mock_client + + item = { + "call_id": "tc_2", + "mcp_func_name": "mcp__srv-oauth__do", + "mcp_args": {}, + } + session._exec_mcp_tool(item) + + assert captures[0][2] == "MCP tool error: connection lost" + assert captures[0][3] is True + + +class TestExecReadResourceDispatchError: + def test_exec_read_resource_preserves_structured_error_json(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.read_resource_sync.side_effect = RuntimeError(_CONSENT_REQUIRED_JSON) + session._mcp_client = mock_client + + item = { + "call_id": "rc_1", + "resource_uri": "https://example.com/r", + } + # The exec site logs ``log.warning(... exc_info=True)`` on failure. + # Patch the logger so the test doesn't emit noise to the captured + # stderr — assertions don't depend on log output. + with patch("turnstone.core.session.log"): + session._exec_read_resource(item) + + assert len(captures) == 1 + assert captures[0][2] == _CONSENT_REQUIRED_JSON + assert captures[0][3] is True + + def test_exec_read_resource_prefixes_non_structured_error(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.read_resource_sync.side_effect = RuntimeError("connection lost") + session._mcp_client = mock_client + + item = { + "call_id": "rc_2", + "resource_uri": "https://example.com/r", + } + with patch("turnstone.core.session.log"): + session._exec_read_resource(item) + + assert captures[0][2] == "MCP resource error: connection lost" + assert captures[0][3] is True + + +class TestExecUsePromptDispatchError: + def test_exec_use_prompt_preserves_structured_error_json(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.get_prompt_sync.side_effect = RuntimeError(_CONSENT_REQUIRED_JSON) + session._mcp_client = mock_client + + item = { + "call_id": "pc_1", + "prompt_name": "mcp__srv-oauth__greet", + "prompt_arguments": {}, + } + with patch("turnstone.core.session.log"): + session._exec_use_prompt(item) + + assert len(captures) == 1 + assert captures[0][2] == _CONSENT_REQUIRED_JSON + assert captures[0][3] is True + + def test_exec_use_prompt_prefixes_non_structured_error(self, tmp_db) -> None: + session = _make_session() + captures = _record_outputs(session) + + mock_client = MagicMock() + mock_client.get_prompt_sync.side_effect = RuntimeError("connection lost") + session._mcp_client = mock_client + + item = { + "call_id": "pc_2", + "prompt_name": "mcp__srv-oauth__greet", + "prompt_arguments": {}, + } + with patch("turnstone.core.session.log"): + session._exec_use_prompt(item) + + assert captures[0][2] == "MCP prompt error: connection lost" + assert captures[0][3] is True diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 2ce54771..da693443 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -1635,6 +1635,20 @@ async def mcp_oauth_callback(request: Request) -> Response: return await handle_mcp_oauth_callback(request) +async def mcp_oauth_list_connections(request: Request) -> Response: + """GET /v1/api/mcp/oauth/connections — list this user's MCP server consents.""" + from turnstone.core.mcp_oauth import handle_mcp_oauth_list_connections + + return await handle_mcp_oauth_list_connections(request) + + +async def mcp_oauth_revoke_connection(request: Request) -> Response: + """DELETE /v1/api/mcp/oauth/connections/{server_name} — revoke a consent.""" + from turnstone.core.mcp_oauth import handle_mcp_oauth_revoke_connection + + return await handle_mcp_oauth_revoke_connection(request) + + # --------------------------------------------------------------------------- # Route handlers — available models (lightweight, no admin permission) # --------------------------------------------------------------------------- @@ -11844,6 +11858,12 @@ def create_app( Route("/api/auth/oidc/callback", oidc_callback), Route("/api/mcp/oauth/start", mcp_oauth_authorize), Route("/api/mcp/oauth/callback", mcp_oauth_callback), + Route("/api/mcp/oauth/connections", mcp_oauth_list_connections), + Route( + "/api/mcp/oauth/connections/{server_name}", + mcp_oauth_revoke_connection, + methods=["DELETE"], + ), Route("/api/admin/users", admin_list_users), Route("/api/admin/users", admin_create_user, methods=["POST"]), Route("/api/admin/users/{user_id}", admin_delete_user, methods=["DELETE"]), diff --git a/turnstone/core/mcp_client.py b/turnstone/core/mcp_client.py index 14ebef49..a63e52c5 100644 --- a/turnstone/core/mcp_client.py +++ b/turnstone/core/mcp_client.py @@ -3505,11 +3505,23 @@ class MCPClientManager: :class:`_PoolDispatchRetryRequested`. Without this, a slow first attempt followed by a stuck retry could double the caller-observed timeout. + + Structured-error returns (``mcp_consent_required`` / + ``mcp_insufficient_scope`` / ``mcp_token_undecryptable_key_unknown`` + / ...) flow back from ``_dispatch_pool`` as a JSON string in the + success-shape return slot. The session-layer ``_exec_mcp_tool`` + path keys on ``except Exception`` to render the dashboard's + consent card — so we surface that JSON via ``RuntimeError`` here + to drive the same path uniformly across tool / resource / prompt + dispatchers. The wrap is gated on + :func:`_is_structured_error` so a tool whose successful + output happens to start with ``{"error":...`` is unaffected; + only envelopes carrying an ``mcp_*`` code are converted. """ assert self._loop is not None start = time.monotonic() try: - return self._run_pool_dispatch_attempt( + result = self._run_pool_dispatch_attempt( retry_count=0, timeout=timeout, original_timeout=timeout, @@ -3525,7 +3537,7 @@ class MCPClientManager: # state is independent of the prior connect's TaskGroup # teardown. remaining = max(1, int(timeout - (time.monotonic() - start))) - return self._run_pool_dispatch_attempt( + result = self._run_pool_dispatch_attempt( retry_count=1, timeout=remaining, original_timeout=timeout, @@ -3535,6 +3547,9 @@ class MCPClientManager: arguments=arguments, server_row=server_row, ) + if _is_structured_error(result): + raise RuntimeError(result) + return result def _run_pool_dispatch_attempt( self, @@ -3601,11 +3616,16 @@ class MCPClientManager: bearer; we re-issue on a brand-new task so the retry's anyio cancel-scope state is independent of the prior connect's TaskGroup teardown. + + Structured-error returns are surfaced via ``RuntimeError`` — + see :meth:`_dispatch_pool_sync` for the rationale; this keeps + the agent-loop's ``except Exception`` handling uniform across + tool and resource dispatchers. """ assert self._loop is not None start = time.monotonic() try: - return self._run_pool_dispatch_resource_attempt( + result = self._run_pool_dispatch_resource_attempt( retry_count=0, timeout=timeout, original_timeout=timeout, @@ -3616,7 +3636,7 @@ class MCPClientManager: ) except _PoolDispatchRetryRequested: remaining = max(1, int(timeout - (time.monotonic() - start))) - return self._run_pool_dispatch_resource_attempt( + result = self._run_pool_dispatch_resource_attempt( retry_count=1, timeout=remaining, original_timeout=timeout, @@ -3625,6 +3645,9 @@ class MCPClientManager: uri=uri, server_row=server_row, ) + if _is_structured_error(result): + raise RuntimeError(result) + return result def _run_pool_dispatch_resource_attempt( self, @@ -3803,6 +3826,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) if lookup.kind == "decrypt_failure": return _structured_error( @@ -3821,6 +3845,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refresh token rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) # kind == "token" access_token = lookup.token or "" @@ -3829,6 +3854,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) # URL hygiene — pool dispatch transmits the per-user bearer; reject @@ -3911,6 +3937,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refreshed token still rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) if classification == "auth_403": self._evict_session(key) @@ -3976,6 +4003,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) if lookup.kind == "decrypt_failure": return _structured_error( @@ -3991,6 +4019,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refresh token rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) access_token = lookup.token or "" if not access_token: @@ -3998,6 +4027,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) url = str(server_row.get("url") or "") @@ -4051,6 +4081,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refreshed token still rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) if classification == "auth_403": self._evict_session(key) @@ -4123,6 +4154,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) if lookup.kind == "decrypt_failure": return _structured_error( @@ -4138,6 +4170,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refresh token rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) access_token = lookup.token or "" if not access_token: @@ -4145,6 +4178,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="No token for user. Consent flow required.", + consent_url=_build_consent_url(server_row), ) url = str(server_row.get("url") or "") @@ -4198,6 +4232,7 @@ class MCPClientManager: code="mcp_consent_required", server=server_name, detail="Refreshed token still rejected. Re-consent required.", + consent_url=_build_consent_url(server_row), ) if classification == "auth_403": self._evict_session(key) @@ -4325,6 +4360,7 @@ class MCPClientManager: "Re-consent flow with new scopes required." ), scopes_required=list(scopes), + consent_url=_build_consent_url(server_row, scopes_required=list(scopes)), ) forbidden_code = { "tool": "mcp_tool_call_forbidden", @@ -4711,6 +4747,34 @@ class MCPClientManager: self._cb_record_success(server_name) return _decode_prompt_result(result) + def evict_user_session(self, user_id: str, server_name: str) -> None: + """Drop the cached pool session for ``(user_id, server_name)``. + + Sync entry point for callers (e.g. the OAuth revoke handler) + that mutate token state from outside the mcp-loop and need the + next dispatch to reconnect with fresh credentials. Idempotent — + a missing key is a silent no-op. Fire-and-forget: schedules + :meth:`_evict_session` on the mcp-loop and returns immediately + without waiting for the future. Best-effort: a closed loop or + scheduling failure logs at info level; never raises. + """ + if self._loop is None: + return + key = (user_id, server_name) + + async def _do_evict() -> None: + self._evict_session(key) + + try: + asyncio.run_coroutine_threadsafe(_do_evict(), self._loop) + except RuntimeError as exc: + log.info( + "mcp_pool.evict_user_session_failed server=%s user=%s error=%s", + server_name, + user_id, + type(exc).__name__, + ) + # --------------------------------------------------------------------------- # Pool helpers (module-level) @@ -4776,6 +4840,7 @@ def _structured_error( server: str, detail: str, scopes_required: list[str] | None = None, + consent_url: str | None = None, ) -> str: """Encode a pool-dispatch failure as a JSON string. @@ -4789,6 +4854,13 @@ def _structured_error( the dashboard renderer keys on its presence to construct an authorize URL with the union of original + new scopes. + ``consent_url`` is omitted when ``None``. When present it carries a + relative ``/v1/api/mcp/oauth/start?...`` path the dashboard can + open in a popup. Only emitted for user-actionable codes + (``mcp_consent_required``, ``mcp_insufficient_scope``); operator- + actionable codes (key-unknown, URL-insecure, generic forbidden) + intentionally omit it because re-consent doesn't help. + Operator-actionable encryption-key fingerprints are intentionally NOT included in this payload: they are already captured server-side via :meth:`MCPTokenStore._audit_decrypt_failure`, and exposing them @@ -4802,9 +4874,66 @@ def _structured_error( } if scopes_required is not None: err["scopes_required"] = scopes_required + if consent_url is not None: + err["consent_url"] = consent_url return json.dumps({"error": err}) +def _is_structured_error(result: str) -> bool: + """Return True if *result* parses as a :func:`_structured_error` envelope. + + Used by :meth:`MCPClientManager._dispatch_pool_sync` / + :meth:`MCPClientManager._dispatch_pool_resource_sync` to convert the + dispatcher's success-shape return into a raised ``RuntimeError``, so + the session-layer ``except`` branch fires uniformly across tool / + resource / prompt dispatchers. The prompt path uses an + ``isinstance(result, str)`` hack that doesn't generalize because + tools and resources return ``str`` on success too. + """ + if not isinstance(result, str) or not result.startswith('{"error":'): + return False + try: + decoded = json.loads(result) + except (json.JSONDecodeError, ValueError): + return False + if not isinstance(decoded, dict): + return False + err = decoded.get("error") + if not isinstance(err, dict): + return False + code = err.get("code") + return isinstance(code, str) and code.startswith("mcp_") + + +def _build_consent_url( + server_row: dict[str, Any], + *, + scopes_required: list[str] | None = None, +) -> str | None: + """Build a consent URL the dashboard can open in a popup. + + Returns ``None`` when ``server_row`` is not configured for + ``auth_type=oauth_user`` (defensive — a structured error for a + static-auth server should not advertise a consent flow). + + The ``return_url`` query param is intentionally not baked in: the + dashboard JS appends ``window.location.href`` at click time, + matching the existing admin.js connect-button pattern. The scope + set is passed through so the step-up flow can union with the + server's configured scopes server-side at /start. + """ + if server_row.get("auth_type") != "oauth_user": + return None + server_name = str(server_row.get("name") or "") + if not server_name: + return None + qs = "server=" + urllib.parse.quote(server_name, safe="") + if scopes_required: + scopes_str = " ".join(scopes_required) + qs += "&scopes=" + urllib.parse.quote(scopes_str, safe="") + return f"/v1/api/mcp/oauth/start?{qs}" + + def _pool_cfg_from_row(row: dict[str, Any]) -> dict[str, Any]: """Build a streamable-http MCP-client cfg from an ``mcp_servers`` row. diff --git a/turnstone/core/mcp_crypto.py b/turnstone/core/mcp_crypto.py index 33b55667..f7b7742e 100644 --- a/turnstone/core/mcp_crypto.py +++ b/turnstone/core/mcp_crypto.py @@ -90,6 +90,24 @@ class MCPUserTokenPlain(TypedDict): last_refreshed: str | None +class MCPUserTokenMetadata(TypedDict): + """Non-secret subset of ``MCPUserToken`` for the settings UI. + + Token ciphertext is intentionally absent: a list view never needs + the access/refresh secrets, and decrypt happens only at MCP-call + time. + """ + + user_id: str + server_name: str + expires_at: str | None + scopes: str | None + as_issuer: str + audience: str + created: str + last_refreshed: str | None + + # --------------------------------------------------------------------------- # Config dataclass + loader # --------------------------------------------------------------------------- @@ -360,6 +378,30 @@ class MCPTokenStore: """Delete the user-token row. Returns True if existed.""" return self._storage.delete_mcp_user_token(user_id, server_name) + def list_user_token_metadata(self, user_id: str) -> list[MCPUserTokenMetadata]: + """Return non-secret metadata for every token row owned by ``user_id``. + + Storage layer projects the metadata columns at the SQL boundary + (``list_mcp_user_token_metadata_by_user``) so ciphertext blobs + never cross the wire on this list-view path. Rows arrive in + ``created`` ASC order. Decrypt is intentionally skipped — the + list view has no need for the secret material. + """ + rows = self._storage.list_mcp_user_token_metadata_by_user(user_id) + return [ + MCPUserTokenMetadata( + user_id=row["user_id"], + server_name=row["server_name"], + expires_at=row["expires_at"], + scopes=row["scopes"], + as_issuer=row["as_issuer"], + audience=row["audience"], + created=row["created"], + last_refreshed=row["last_refreshed"], + ) + for row in rows + ] + def set_oauth_client_secret(self, server_id: str, plaintext_secret: str | None) -> bool: """Encrypt plaintext and persist via the dedicated storage writer. @@ -529,6 +571,7 @@ __all__ = [ "MCPTokenDecryptError", "MCPTokenKeyConfigError", "MCPTokenStore", + "MCPUserTokenMetadata", "MCPUserTokenPlain", "close_mcp_crypto_state", "initialize_mcp_crypto_state", diff --git a/turnstone/core/mcp_http_parsers.py b/turnstone/core/mcp_http_parsers.py index 0c521fd5..577d952f 100644 --- a/turnstone/core/mcp_http_parsers.py +++ b/turnstone/core/mcp_http_parsers.py @@ -185,6 +185,25 @@ def parse_www_authenticate_bearer(header: str) -> dict[str, str]: return out +def is_valid_scope_token(token: str) -> bool: + """Return True iff ``token`` is a valid RFC 6749 §3.3 ``scope-token``. + + The grammar restricts scope tokens to visible ASCII (``0x21..0x7E``) + excluding ``"`` (``0x22``) and ``\\`` (``0x5C``). The empty string + is rejected — a zero-length token has no semantic meaning in the + space-separated scope list. + + Used by the WWW-Authenticate parser to filter AS-supplied scope + sets, and by ``/v1/api/mcp/oauth/start`` to reject caller-supplied + scope query params that could smuggle CR/LF/tab/control bytes + through the AS round-trip into downstream log or notification + paths. + """ + if not token: + return False + return all(0x21 <= ord(c) <= 0x7E and c not in ('"', "\\") for c in token) + + def parse_www_authenticate_scope(header: str) -> tuple[str, ...]: """Return the ``scope=...`` value as a tuple of individual scopes. @@ -194,21 +213,17 @@ def parse_www_authenticate_scope(header: str) -> tuple[str, ...]: Each token is validated against the RFC 6749 §3.3 ``scope-token`` grammar (visible ASCII ``0x21..0x7E`` excluding ``"`` and ``\\``) - so that a malicious or buggy AS cannot smuggle CR/LF/tab/control - bytes through a future log or notification path. Today scopes are - JSON-encoded everywhere downstream so no concrete exploit exists, - but the validation is cheap and forecloses regressions in - structured-error rendering. + via :func:`is_valid_scope_token` so that a malicious or buggy AS + cannot smuggle CR/LF/tab/control bytes through a future log or + notification path. Today scopes are JSON-encoded everywhere + downstream so no concrete exploit exists, but the validation is + cheap and forecloses regressions in structured-error rendering. """ params = parse_www_authenticate_bearer(header) value = params.get("scope") if not value: return () - return tuple( - s - for s in value.split(" ") - if s and all(0x21 <= ord(c) <= 0x7E and c not in '"\\' for c in s) - ) + return tuple(s for s in value.split(" ") if is_valid_scope_token(s)) def parse_www_authenticate_error(header: str) -> str | None: diff --git a/turnstone/core/mcp_oauth.py b/turnstone/core/mcp_oauth.py index ff80935c..8dfb0669 100644 --- a/turnstone/core/mcp_oauth.py +++ b/turnstone/core/mcp_oauth.py @@ -38,7 +38,10 @@ import httpx from turnstone.core.audit import record_audit from turnstone.core.log import get_logger from turnstone.core.mcp_crypto import MCPTokenDecryptError -from turnstone.core.mcp_http_parsers import parse_www_authenticate_bearer +from turnstone.core.mcp_http_parsers import ( + is_valid_scope_token, + parse_www_authenticate_bearer, +) from turnstone.core.oauth_ssrf import ( OAuthSSRFError, sanitize_log_text, @@ -117,6 +120,7 @@ class ASMetadata: authorization_endpoint: str token_endpoint: str registration_endpoint: str | None + revocation_endpoint: str | None jwks_uri: str | None code_challenge_methods_supported: tuple[str, ...] token_endpoint_auth_methods_supported: tuple[str, ...] @@ -270,6 +274,10 @@ async def _fetch_as_metadata( registration_endpoint = ( str(registration_endpoint_raw) if isinstance(registration_endpoint_raw, str) else None ) + revocation_endpoint_raw = doc.get("revocation_endpoint") + revocation_endpoint = ( + str(revocation_endpoint_raw) if isinstance(revocation_endpoint_raw, str) else None + ) jwks_uri_raw = doc.get("jwks_uri") jwks_uri = str(jwks_uri_raw) if isinstance(jwks_uri_raw, str) else None @@ -292,18 +300,21 @@ async def _fetch_as_metadata( except OAuthSSRFError as exc: raise MCPOAuthDiscoveryError(f"AS {name} rejected (url={endpoint_url}): {exc}") from exc - if registration_endpoint: + for opt_name, opt_url in ( + ("registration_endpoint", registration_endpoint), + ("revocation_endpoint", revocation_endpoint), + ): + if not opt_url: + continue try: await validate_discovered_endpoint_async( - registration_endpoint, + opt_url, issuer_parsed, allow_http=allow_http, trusted_endpoint_hosts=trusted_hosts, ) except OAuthSSRFError as exc: - raise MCPOAuthDiscoveryError( - f"AS registration_endpoint rejected (url={registration_endpoint}): {exc}" - ) from exc + raise MCPOAuthDiscoveryError(f"AS {opt_name} rejected (url={opt_url}): {exc}") from exc code_methods_raw = doc.get("code_challenge_methods_supported", []) if not isinstance(code_methods_raw, list): @@ -324,6 +335,7 @@ async def _fetch_as_metadata( authorization_endpoint=authorization_endpoint, token_endpoint=token_endpoint, registration_endpoint=registration_endpoint, + revocation_endpoint=revocation_endpoint, jwks_uri=jwks_uri, code_challenge_methods_supported=code_methods, token_endpoint_auth_methods_supported=auth_methods, @@ -772,6 +784,75 @@ async def refresh_token( return doc +async def revoke_token_at_as( + *, + as_metadata: ASMetadata, + http_client: httpx.AsyncClient, + refresh_token: str, + client_id: str, + client_secret: str | None, + timeout_seconds: float = _DEFAULT_HTTP_TIMEOUT, +) -> None: + """Best-effort RFC 7009 token revocation. + + Posts ``token=&token_type_hint=refresh_token`` plus + client credentials to ``as_metadata.revocation_endpoint``. This is + fire-and-don't-care — the helper logs the outcome and never raises, + so callers can fold it into a teardown path without try/except. + + When the AS metadata document carries no ``revocation_endpoint`` + (RFC 8414 makes it optional), the helper logs and returns. The + timeout is enforced via ``asyncio.timeout`` (NOT ``asyncio.wait_for``) + to avoid the Python 3.11 anyio cancel-scope hazard on cleanup paths. + """ + if as_metadata.revocation_endpoint is None: + log.info( + "mcp_server.oauth.revocation_unsupported", + as_issuer=as_metadata.issuer, + ) + return + + data: dict[str, str] = { + "token": refresh_token, + "token_type_hint": "refresh_token", + "client_id": client_id, + } + if client_secret: + data["client_secret"] = client_secret + + try: + async with asyncio.timeout(timeout_seconds): + resp = await http_client.post( + as_metadata.revocation_endpoint, + data=data, + ) + except Exception as exc: + # NOTE: never use ``exc_info=True`` here — chained ``__context__`` + # may carry an ``httpx.Request`` whose ``Authorization`` header + # holds a bearer. Structured fields with ``type(exc).__name__`` + # only. + log.info( + "mcp_server.oauth.revocation_failed", + as_issuer=as_metadata.issuer, + error=type(exc).__name__, + ) + return + + if 200 <= resp.status_code < 300: + log.info( + "mcp_server.oauth.revocation_succeeded", + as_issuer=as_metadata.issuer, + status=resp.status_code, + ) + return + + log.info( + "mcp_server.oauth.revocation_failed", + as_issuer=as_metadata.issuer, + status=resp.status_code, + ) + + # --------------------------------------------------------------------------- # JWT audience validation # --------------------------------------------------------------------------- @@ -1843,6 +1924,31 @@ async def _handle_mcp_oauth_authorize_inner(request: Request) -> Response: # Fall back to root — operators often hit /start without a hint. return_url = "/" + # Optional ``scopes`` query param — caller-supplied step-up scopes. + # Validated against the RFC 6749 §3.3 grammar so a malicious or buggy + # client can't smuggle CR/LF/tab/control bytes through the AS round- + # trip into downstream log or notification paths. The cap matches + # the per-call ceiling used in the WWW-Authenticate parser + # (``mcp_client._MAX_INSUFFICIENT_SCOPE_REPORTED``); over-capped + # input is rejected loudly so callers don't silently lose state. + # + # Splitting on a single space (NOT ``str.split()``) is intentional: + # Python's whitespace split would silently strip embedded CR/LF/tab, + # masking hostile input that the grammar predicate is supposed to + # catch. + requested_scopes_raw = request.query_params.get("scopes", "") + requested_scopes: list[str] = [] + if requested_scopes_raw: + from turnstone.core.mcp_client import _MAX_INSUFFICIENT_SCOPE_REPORTED + + candidates = [tok for tok in requested_scopes_raw.split(" ") if tok] + if len(candidates) > _MAX_INSUFFICIENT_SCOPE_REPORTED: + return JSONResponse({"error": "Invalid scope token"}, status_code=400) + for tok in candidates: + if not is_valid_scope_token(tok): + return JSONResponse({"error": "Invalid scope token"}, status_code=400) + requested_scopes = candidates + storage = _get_storage(request.app.state) if storage is None: return JSONResponse({"error": "Storage unavailable"}, status_code=503) @@ -1930,7 +2036,16 @@ async def _handle_mcp_oauth_authorize_inner(request: Request) -> Response: ) audience = server_row.get("oauth_audience") or server_url - scopes = server_row.get("oauth_scopes") or "" + configured_scopes = str(server_row.get("oauth_scopes") or "") + if requested_scopes: + # Union: configured scopes + caller-supplied step-up scopes, deduped + # and sorted so the AS sees a stable string regardless of caller + # ordering (cache-key stability, deterministic audit detail). + merged = set(configured_scopes.split()) | set(requested_scopes) + merged.discard("") + scopes = " ".join(sorted(merged)) + else: + scopes = configured_scopes url = build_authorize_url( as_metadata=as_metadata, client_id=client_id, @@ -2238,6 +2353,271 @@ async def _handle_mcp_oauth_callback_inner(request: Request) -> Response: return RedirectResponse(pending["return_url"] or "/", status_code=302) +async def handle_mcp_oauth_list_connections(request: Request) -> Response: + """``GET /v1/api/mcp/oauth/connections``. + + Lists the authenticated user's MCP server consents. Returns the + non-secret projection (no access/refresh ciphertext) so the + settings UI can render a connections list without ever pulling + decrypt material out of storage. + """ + return _apply_security_headers(await _handle_mcp_oauth_list_connections_inner(request)) + + +async def _handle_mcp_oauth_list_connections_inner(request: Request) -> Response: + from starlette.responses import JSONResponse + + token_store = getattr(request.app.state, "mcp_token_store", None) + if token_store is None: + return _no_token_store_response("connections") + + user_id = _require_user_id(request) + if user_id is None: + return JSONResponse({"error": "Authentication required"}, status_code=401) + + rows = await asyncio.to_thread(token_store.list_user_token_metadata, user_id) + return JSONResponse({"connections": list(rows)}) + + +async def handle_mcp_oauth_revoke_connection(request: Request) -> Response: + """``DELETE /v1/api/mcp/oauth/connections/{server_name}``. + + Best-effort RFC 7009 upstream revoke followed by the authoritative + local delete. Cross-user attempts return 404 with the same body + shape as a never-existed row to avoid leaking tenant existence. + Pool sessions for the (user, server) pair are evicted so any + in-flight dispatch reconnects with a fresh token at next call. + """ + return _apply_security_headers(await _handle_mcp_oauth_revoke_connection_inner(request)) + + +# Strong refs to in-flight upstream-revoke tasks. asyncio holds tasks via +# a WeakSet; a fire-and-forget ``loop.create_task`` whose handle isn't +# stored can be GC'd before the AS round-trip completes. Tasks register +# here on creation and discard themselves on completion via +# ``add_done_callback`` — same pattern as ``_pg_refresh_drain_tasks``. +_revoke_upstream_tasks: set[asyncio.Task[None]] = set() + +# Soft cap on concurrent in-flight upstream revokes. A coordinated mass +# revoke (admin sweep, scripted cleanup, compromised account) could pile +# up arbitrarily many tasks each pinning storage / token_store / server_row +# / refresh-token plaintext until the AS round-trip completes (~30s +# worst case). When the set is full, the local delete still runs and +# the audit row records ``upstream_revoke_outcome="shed_by_cap"``; the +# operator can re-run revokes against any straggling AS-side tokens once +# the queue drains. +_REVOKE_UPSTREAM_TASKS_MAX = 256 + + +async def _attempt_upstream_revoke( + *, + http_client: httpx.AsyncClient, + metadata_cache: dict[str, Any] | None, + storage: StorageBackend, + token_store: MCPTokenStore, + server_name: str, + server_row: dict[str, Any], + server_id_for_audit: str, + refresh_token: str, +) -> None: + """Best-effort RFC 7009 upstream revoke for ``user_revoked`` flow. + + Designed to be fired from :func:`asyncio.create_task` so the caller's + 204 isn't gated on the AS round-trip — the local delete is + authoritative for this deployment, and the AS-side state is best- + effort. Never raises. Each terminal state emits a structured log so + operators can audit AS-side outcomes without parsing exception text: + ``revoke_token_at_as`` logs ``revocation_succeeded`` / + ``revocation_failed`` / ``revocation_unsupported`` on its branches; + discovery failures emit ``upstream_revoke_discovery_failed``; an + unexpected exception in the outer block emits + ``upstream_revoke_failed``. + + The outer ``try/except Exception`` is load-bearing: this helper is + fired as a background task whose handle goes into ``_revoke_upstream_tasks`` + with a ``set.discard`` done-callback that does NOT consume + ``task.exception()``. An unhandled exception here would surface as + ``Task exception was never retrieved`` from asyncio's default handler. + Catching at the outer boundary keeps the helper's contract honest. + Bearer-leak invariant: ``exc_info=True`` is forbidden on this path — + the chained ``__context__`` may carry an ``httpx.Request`` whose + ``Authorization`` header holds the per-user bearer. + """ + try: + try: + as_metadata = await discover_authorization_server( + server_name=server_name, + server_url=str(server_row.get("url") or ""), + override_url=server_row.get("oauth_authorization_server_url") or None, + cached_issuer=server_row.get("oauth_as_issuer_cached") or None, + http_client=http_client, + storage=storage, + server_id=server_id_for_audit, + trusted_hosts=frozenset(), + metadata_cache=metadata_cache, + ) + except MCPOAuthDiscoveryError as exc: + log.info( + "mcp_server.oauth.upstream_revoke_discovery_failed", + server_name=server_name, + error=type(exc).__name__, + ) + return + # When the AS doesn't advertise a revocation_endpoint, + # ``revoke_token_at_as`` itself logs ``revocation_unsupported`` + # and returns — no need for a redundant gate here. Letting the + # call through keeps the observability story uniform. + client_id = str(server_row.get("oauth_client_id") or "") + client_secret: str | None = None + if server_id_for_audit: + client_secret_ct = await asyncio.to_thread( + storage.get_mcp_oauth_client_secret_ct, server_id_for_audit + ) + if client_secret_ct is not None: + try: + client_secret = token_store.cipher.decrypt(client_secret_ct).decode("utf-8") + except MCPTokenDecryptError: + client_secret = None + # ``revoke_token_at_as`` never raises and never logs ``exc_info=True``; + # the AS round-trip is fire-and-don't-care from the caller's vantage. + await revoke_token_at_as( + as_metadata=as_metadata, + http_client=http_client, + refresh_token=refresh_token, + client_id=client_id, + client_secret=client_secret, + ) + except Exception as exc: + log.info( + "mcp_server.oauth.upstream_revoke_failed", + server_name=server_name, + error=type(exc).__name__, + ) + + +async def _handle_mcp_oauth_revoke_connection_inner(request: Request) -> Response: + from starlette.responses import JSONResponse, Response + + token_store = getattr(request.app.state, "mcp_token_store", None) + if token_store is None: + return _no_token_store_response("revoke_connection") + + user_id = _require_user_id(request) + if user_id is None: + return JSONResponse({"error": "Authentication required"}, status_code=401) + + server_name = request.path_params.get("server_name", "").strip() + if not server_name: + return JSONResponse({"error": "Missing server_name"}, status_code=400) + + storage = _get_storage(request.app.state) + if storage is None: + return JSONResponse({"error": "Storage unavailable"}, status_code=503) + + # Decrypt is best-effort: we can't perform an upstream revoke without + # the plaintext refresh token, but the local delete is authoritative + # so the consent is invalidated either way. Decrypt failure here is + # not a hard error — the operator can still revoke locally. + plain: Any = None + try: + plain = await asyncio.to_thread(token_store.get_user_token, user_id, server_name) + except MCPTokenDecryptError: + plain = None + + # Distinguish "row missing" from "decrypt failed" — a missing row + # surfaces as 404 (with the same shape used for cross-user attempts + # so existence is not leaked across tenants). + if plain is None: + storage_row = await asyncio.to_thread(storage.get_mcp_user_token, user_id, server_name) + if storage_row is None: + return JSONResponse({"error": "No such connection"}, status_code=404) + + server_row = await asyncio.to_thread(storage.get_mcp_server_by_name, server_name) + server_id_for_audit = "" + if server_row is not None: + server_id_for_audit = str(server_row.get("server_id") or "") + + # Local delete — authoritative. Even if the upstream revoke fails or + # is unsupported, the consent is invalidated for this deployment. + # Run BEFORE the AS round-trip so the user-visible 204 isn't gated + # on a slow / unreachable AS. + await asyncio.to_thread(token_store.delete_user_token, user_id, server_name) + _drop_refresh_lock(request.app.state, user_id, server_name) + + # Best-effort pool eviction so any in-flight session backed by the + # now-deleted row is closed before the next dispatch. + mcp_client = getattr(request.app.state, "mcp_client", None) + if mcp_client is not None and hasattr(mcp_client, "evict_user_session"): + try: + mcp_client.evict_user_session(user_id, server_name) + except Exception as exc: + # Best-effort: a closed loop or transient scheduling error + # must not block the user-visible 204. Type name only — the + # exception's chain may carry token-bearing context. + log.info( + "mcp_server.oauth.evict_user_session_failed", + user_id=user_id, + server_name=server_name, + error=type(exc).__name__, + ) + + # Schedule the upstream RFC 7009 revoke as a fire-and-forget task so + # the response isn't gated on the AS round-trip. ``upstream_revoke_outcome`` + # is the categorical audit field — operators can distinguish the + # four terminal states (scheduled, no_refresh_token, no_http_client, + # shed_by_cap) without parsing log streams. + refresh_token_for_revoke: str | None = plain.get("refresh_token") if plain is not None else None + if not refresh_token_for_revoke or server_row is None: + upstream_revoke_outcome = "no_refresh_token" + else: + http_client = getattr(request.app.state, "mcp_oauth_http_client", None) + if http_client is None: + upstream_revoke_outcome = "no_http_client" + elif len(_revoke_upstream_tasks) >= _REVOKE_UPSTREAM_TASKS_MAX: + # Soft-cap shed: the local delete already ran (authoritative); + # surface the dropped attempt in the audit detail so an + # operator can re-run revokes once the queue drains. + log.info( + "mcp_server.oauth.upstream_revoke_shed", + server_name=server_name, + in_flight=len(_revoke_upstream_tasks), + cap=_REVOKE_UPSTREAM_TASKS_MAX, + ) + upstream_revoke_outcome = "shed_by_cap" + else: + metadata_cache = getattr(request.app.state, "mcp_oauth_metadata_cache", None) + task = asyncio.create_task( + _attempt_upstream_revoke( + http_client=http_client, + metadata_cache=metadata_cache, + storage=storage, + token_store=token_store, + server_name=server_name, + server_row=server_row, + server_id_for_audit=server_id_for_audit, + refresh_token=refresh_token_for_revoke, + ), + name="mcp-oauth-upstream-revoke", + ) + _revoke_upstream_tasks.add(task) + task.add_done_callback(_revoke_upstream_tasks.discard) + upstream_revoke_outcome = "scheduled" + + await _audit_event( + request.app.state, + server_id=server_id_for_audit, + user_id=user_id, + action="mcp_server.oauth.token_revoked", + server_name=server_name, + detail={ + "reason": "user_revoked", + "upstream_revoke_outcome": upstream_revoke_outcome, + }, + ) + + return Response(status_code=204) + + # --------------------------------------------------------------------------- # Lifespan integration # --------------------------------------------------------------------------- @@ -2294,6 +2674,9 @@ __all__ = [ "get_user_access_token_classified", "handle_mcp_oauth_authorize", "handle_mcp_oauth_callback", + "handle_mcp_oauth_list_connections", + "handle_mcp_oauth_revoke_connection", "initialize_mcp_oauth_state", "pop_pending_state", + "revoke_token_at_as", ] diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 6fd40b73..0912ccfe 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -618,6 +618,39 @@ class SessionUI(Protocol): ... +# --------------------------------------------------------------------------- +# MCP dispatch helpers +# --------------------------------------------------------------------------- + + +def _format_mcp_dispatch_error(prefix: str, exc: Exception) -> str: + """Preserve structured-error JSON when the dispatcher signals via ``RuntimeError(json_str)``. + + The pool dispatcher raises ``RuntimeError(json_str)`` with a + :func:`turnstone.core.mcp_client._structured_error` payload (e.g. + ``mcp_consent_required``, ``mcp_insufficient_scope``) when the + failure has a user-actionable remedy. The dashboard renderer keys + on this shape; surrounding it with ``f"{prefix}: {exc}"`` would + make the JSON un-parseable. Non-structured exceptions are still + rendered with the prefix so the agent has a human-readable label. + + Shared by :meth:`ChatSession._exec_mcp_tool`, + :meth:`ChatSession._exec_read_resource`, and + :meth:`ChatSession._exec_use_prompt` — placed at module scope so + no single exec site can claim ownership. + """ + text = str(exc) + try: + decoded = json.loads(text) + except (json.JSONDecodeError, ValueError): + return f"{prefix}: {exc}" + if isinstance(decoded, dict) and isinstance(decoded.get("error"), dict): + code = decoded["error"].get("code") + if isinstance(code, str) and code.startswith("mcp_"): + return text + return f"{prefix}: {exc}" + + # --------------------------------------------------------------------------- # Notify auth helper (module-level, lazy-init) # --------------------------------------------------------------------------- @@ -7997,7 +8030,7 @@ class ChatSession: mcp_error = True self.ui.on_error(output) except Exception as e: - output = f"MCP tool error: {e}" + output = _format_mcp_dispatch_error("MCP tool error", e) mcp_error = True self.ui.on_error(output) @@ -8080,8 +8113,12 @@ class ChatSession: mcp_error = True self.ui.on_error(output) except Exception as e: - log.warning("MCP resource read failed for %s", uri, exc_info=True) - output = f"MCP resource error: {e}" + # exc_info=True would let chained ``__context__`` carry an + # ``httpx.Request`` whose ``Authorization`` header holds the + # per-user bearer (Phase 7b pool dispatch path). Use + # structured fields with ``type(e).__name__`` only. + log.warning("mcp.resource_read_failed", uri=uri, error=type(e).__name__) + output = _format_mcp_dispatch_error("MCP resource error", e) mcp_error = True self.ui.on_error(output) @@ -8173,8 +8210,10 @@ class ChatSession: mcp_error = True self.ui.on_error(output) except Exception as e: - log.warning("MCP prompt invocation failed for %s", name, exc_info=True) - output = f"MCP prompt error: {e}" + # See ``_exec_read_resource`` for the no-``exc_info`` rationale — + # bearer-leak via chained ``httpx.Request`` __context__. + log.warning("mcp.prompt_invoke_failed", name=name, error=type(e).__name__) + output = _format_mcp_dispatch_error("MCP prompt error", e) mcp_error = True self.ui.on_error(output) diff --git a/turnstone/core/storage/_postgresql.py b/turnstone/core/storage/_postgresql.py index bb86dad2..1adbb424 100644 --- a/turnstone/core/storage/_postgresql.py +++ b/turnstone/core/storage/_postgresql.py @@ -17,6 +17,7 @@ from turnstone.core.log import get_logger from turnstone.core.storage._protocol import ( MCPOAuthPendingState, MCPUserToken, + MCPUserTokenMetadataRow, OIDCIdentity, OIDCPendingState, ) @@ -3968,6 +3969,45 @@ class PostgreSQLBackend: conn.commit() return result.rowcount > 0 + def list_mcp_user_token_metadata_by_user(self, user_id: str) -> list[MCPUserTokenMetadataRow]: + """Return non-secret metadata rows for ``user_id``, ordered by ``created`` ASC. + + Projects metadata columns at the SQL boundary so ciphertext + blobs (``access_token_ct`` / ``refresh_token_ct``) never cross + the wire on the settings-list path. + """ + with self._conn() as conn: + rows = conn.execute( + sa.select( + mcp_user_tokens.c.user_id, + mcp_user_tokens.c.server_name, + mcp_user_tokens.c.expires_at, + mcp_user_tokens.c.scopes, + mcp_user_tokens.c.as_issuer, + mcp_user_tokens.c.audience, + mcp_user_tokens.c.created, + mcp_user_tokens.c.last_refreshed, + ) + .where(mcp_user_tokens.c.user_id == user_id) + .order_by(mcp_user_tokens.c.created) + ).fetchall() + out: list[MCPUserTokenMetadataRow] = [] + for row in rows: + m = row._mapping + out.append( + MCPUserTokenMetadataRow( + user_id=m["user_id"], + server_name=m["server_name"], + expires_at=m["expires_at"], + scopes=m["scopes"], + as_issuer=m["as_issuer"], + audience=m["audience"], + created=m["created"], + last_refreshed=m["last_refreshed"], + ) + ) + return out + def delete_mcp_oauth_rows_by_server_name(self, server_name: str) -> int: """Purge user tokens + pending OAuth state for *server_name*.""" with self._conn() as conn: diff --git a/turnstone/core/storage/_protocol.py b/turnstone/core/storage/_protocol.py index 01b807af..f81692a8 100644 --- a/turnstone/core/storage/_protocol.py +++ b/turnstone/core/storage/_protocol.py @@ -61,6 +61,25 @@ class MCPUserToken(TypedDict): last_refreshed: str | None +class MCPUserTokenMetadataRow(TypedDict): + """Non-secret projection of ``mcp_user_tokens`` for the settings UI. + + Excludes ``access_token_ct`` and ``refresh_token_ct`` so the + storage layer never materialises ciphertext for list queries that + only need metadata. ``MCPTokenStore.list_user_token_metadata`` + re-types these rows as ``MCPUserTokenMetadata`` (same field shape). + """ + + user_id: str + server_name: str + expires_at: str | None + scopes: str | None + as_issuer: str + audience: str + created: str + last_refreshed: str | None + + class MCPOAuthPendingState(TypedDict): """Row shape returned when popping a pending MCP OAuth flow state.""" @@ -1727,6 +1746,18 @@ class StorageBackend(Protocol): """Delete the per-(user, server) token row. Returns True if existed.""" ... + def list_mcp_user_token_metadata_by_user(self, user_id: str) -> list[MCPUserTokenMetadataRow]: + """Return non-secret metadata for every token row owned by ``user_id``, + ordered by ``created`` ASC. + + Empty list when the user has no rows. Ciphertext columns are + intentionally NOT loaded — the projection runs at the SQL boundary + so the LargeBinary blobs never cross the wire for the list-view + path. ``MCPTokenStore`` re-types the rows as + ``MCPUserTokenMetadata`` (same field shape) for the settings UI. + """ + ... + def delete_mcp_oauth_rows_by_server_name(self, server_name: str) -> int: """Purge per-(user, server) tokens and pending OAuth states for *server_name*. diff --git a/turnstone/core/storage/_sqlite.py b/turnstone/core/storage/_sqlite.py index 557de321..ce05c1cc 100644 --- a/turnstone/core/storage/_sqlite.py +++ b/turnstone/core/storage/_sqlite.py @@ -16,6 +16,7 @@ from turnstone.core.log import get_logger from turnstone.core.storage._protocol import ( MCPOAuthPendingState, MCPUserToken, + MCPUserTokenMetadataRow, OIDCIdentity, OIDCPendingState, ) @@ -4113,6 +4114,45 @@ class SQLiteBackend: conn.commit() return result.rowcount > 0 + def list_mcp_user_token_metadata_by_user(self, user_id: str) -> list[MCPUserTokenMetadataRow]: + """Return non-secret metadata rows for ``user_id``, ordered by ``created`` ASC. + + Projects metadata columns at the SQL boundary so ciphertext + blobs (``access_token_ct`` / ``refresh_token_ct``) never cross + the wire on the settings-list path. + """ + with self._conn() as conn: + rows = conn.execute( + sa.select( + mcp_user_tokens.c.user_id, + mcp_user_tokens.c.server_name, + mcp_user_tokens.c.expires_at, + mcp_user_tokens.c.scopes, + mcp_user_tokens.c.as_issuer, + mcp_user_tokens.c.audience, + mcp_user_tokens.c.created, + mcp_user_tokens.c.last_refreshed, + ) + .where(mcp_user_tokens.c.user_id == user_id) + .order_by(mcp_user_tokens.c.created) + ).fetchall() + out: list[MCPUserTokenMetadataRow] = [] + for row in rows: + m = row._mapping + out.append( + MCPUserTokenMetadataRow( + user_id=m["user_id"], + server_name=m["server_name"], + expires_at=m["expires_at"], + scopes=m["scopes"], + as_issuer=m["as_issuer"], + audience=m["audience"], + created=m["created"], + last_refreshed=m["last_refreshed"], + ) + ) + return out + def delete_mcp_oauth_rows_by_server_name(self, server_name: str) -> int: """Purge user tokens + pending OAuth state for *server_name*.""" with self._conn() as conn: diff --git a/turnstone/server.py b/turnstone/server.py index dc924fa3..ba8f1279 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -2689,6 +2689,20 @@ async def mcp_oauth_callback(request: Request) -> Response: return await handle_mcp_oauth_callback(request) +async def mcp_oauth_list_connections(request: Request) -> Response: + """GET /v1/api/mcp/oauth/connections — list this user's MCP server consents.""" + from turnstone.core.mcp_oauth import handle_mcp_oauth_list_connections + + return await handle_mcp_oauth_list_connections(request) + + +async def mcp_oauth_revoke_connection(request: Request) -> Response: + """DELETE /v1/api/mcp/oauth/connections/{server_name} — revoke a consent.""" + from turnstone.core.mcp_oauth import handle_mcp_oauth_revoke_connection + + return await handle_mcp_oauth_revoke_connection(request) + + def list_interface_settings(request: Request) -> JSONResponse: """GET /v1/api/admin/settings — return interface settings from ConfigStore. @@ -3925,6 +3939,12 @@ def create_app( Route("/api/auth/oidc/callback", oidc_callback), Route("/api/mcp/oauth/start", mcp_oauth_authorize), Route("/api/mcp/oauth/callback", mcp_oauth_callback), + Route("/api/mcp/oauth/connections", mcp_oauth_list_connections), + Route( + "/api/mcp/oauth/connections/{server_name}", + mcp_oauth_revoke_connection, + methods=["DELETE"], + ), Route("/api/admin/settings", list_interface_settings), Route( "/api/admin/settings/{key:path}", diff --git a/turnstone/ui/static/app.js b/turnstone/ui/static/app.js index 6088474d..a0ac985a 100644 --- a/turnstone/ui/static/app.js +++ b/turnstone/ui/static/app.js @@ -1751,6 +1751,22 @@ Pane.prototype.appendToolOutput = function (callId, name, output, isError) { } } + // Detect structured MCP error envelope and render an interactive + // consent / re-consent / forbidden / operator card. The existing + // ✗ error badge from appendToolErrorBadge still fires below. + if (isError) { + var mcpErr = tryParseMcpError(stripped); + if (mcpErr) { + if (parentBlock && !parentBlock.classList.contains("denied")) { + parentBlock.classList.add("error"); + appendToolErrorBadge(parentBlock); + } + target.after(buildMcpErrorEmbed(mcpErr, stripped)); + this.scrollToBottom(); + return; + } + } + var out = renderToolOutput(stripped, isError); // Mark the parent approval block as errored @@ -5120,6 +5136,210 @@ function buildMediaResultsList(results, totalCount) { return container; } +// =========================================================================== +// 12b. MCP error embed (consent / scope / forbidden / operator) +// =========================================================================== + +// Module-level set of servers with an unresolved consent prompt; drives the +// gear-icon badge so the user has a stable signal that re-consent is pending +// after the inline card scrolls out of view. +var _pendingConsentServers = new Set(); + +function _onConsentDetected(server) { + if (typeof server === "string" && server) { + _pendingConsentServers.add(server); + _refreshConsentBadge(); + } +} + +function _clearConsentBadge() { + _pendingConsentServers.clear(); + _refreshConsentBadge(); +} + +function _refreshConsentBadge() { + var btn = document.getElementById("settings-btn"); + if (!btn) return; + var existing = btn.querySelector(".settings-consent-badge"); + var n = _pendingConsentServers.size; + if (n === 0) { + if (existing) existing.remove(); + return; + } + if (!existing) { + existing = document.createElement("span"); + existing.className = "settings-consent-badge"; + existing.setAttribute("aria-hidden", "true"); + btn.appendChild(existing); + } + existing.textContent = String(n); +} + +/** + * Detect a structured MCP error envelope. Returns the inner ``error`` + * object on shape match, null otherwise. Recognised codes: + * - mcp_consent_required (carries optional consent_url + scopes_required) + * - mcp_insufficient_scope (carries consent_url + scopes_required) + * - mcp_tool_call_forbidden / mcp_resource_read_forbidden / mcp_prompt_get_forbidden + * - mcp_token_undecryptable_key_unknown (operator action) + * - mcp_oauth_url_insecure (operator action) + */ +function tryParseMcpError(text) { + try { + var obj = JSON.parse(text); + } catch (e) { + return null; + } + if (!obj || typeof obj !== "object") return null; + var err = obj.error; + if (!err || typeof err !== "object") return null; + if (typeof err.code !== "string" || err.code.indexOf("mcp_") !== 0) + return null; + return err; +} + +function _mcpErrorCategory(code) { + if (code === "mcp_consent_required" || code === "mcp_insufficient_scope") { + return "actionable"; + } + if ( + code === "mcp_token_undecryptable_key_unknown" || + code === "mcp_oauth_url_insecure" + ) { + return "operator"; + } + // Default for any other mcp_*_forbidden / unrecognised mcp_ code. + return "forbidden"; +} + +function _mcpErrorTitle(err) { + switch (err.code) { + case "mcp_consent_required": + return "Consent required"; + case "mcp_insufficient_scope": + return "Re-consent required (insufficient scope)"; + case "mcp_token_undecryptable_key_unknown": + case "mcp_oauth_url_insecure": + return "Operator action required"; + default: + return "Forbidden"; + } +} + +/** + * Render the action card for an MCP error envelope. Mirrors the + * media-embed pattern: visible card on top, collapsible raw JSON below. + */ +function buildMcpErrorEmbed(err, rawJson) { + var category = _mcpErrorCategory(err.code); + var wrapper = document.createElement("div"); + wrapper.className = "mcp-error-card mcp-error-" + category; + + var icon = document.createElement("div"); + icon.className = "mcp-error-icon"; + icon.setAttribute("aria-hidden", "true"); + icon.textContent = "⚠"; + wrapper.appendChild(icon); + + var body = document.createElement("div"); + body.className = "mcp-error-body"; + + var title = document.createElement("div"); + title.className = "mcp-error-title"; + title.textContent = _mcpErrorTitle(err); + body.appendChild(title); + + if (err.detail) { + var detail = document.createElement("div"); + detail.className = "mcp-error-detail"; + detail.textContent = String(err.detail); + body.appendChild(detail); + } + + if (err.server) { + var serverLine = document.createElement("div"); + serverLine.className = "mcp-error-server"; + serverLine.appendChild(document.createTextNode("server: ")); + var serverCode = document.createElement("code"); + serverCode.textContent = String(err.server); + serverLine.appendChild(serverCode); + body.appendChild(serverLine); + } + + if (Array.isArray(err.scopes_required) && err.scopes_required.length) { + var scopesLine = document.createElement("div"); + scopesLine.className = "mcp-error-scopes"; + scopesLine.appendChild(document.createTextNode("scopes: ")); + for (var i = 0; i < err.scopes_required.length; i++) { + var pill = document.createElement("span"); + pill.className = "mcp-scope-pill"; + pill.textContent = String(err.scopes_required[i]); + scopesLine.appendChild(pill); + } + body.appendChild(scopesLine); + } + + if (category === "actionable") { + var btn = document.createElement("button"); + btn.type = "button"; + btn.className = "mcp-error-action-btn"; + var serverLabel = err.server ? String(err.server) : "server"; + btn.textContent = + err.code === "mcp_insufficient_scope" + ? "Re-consent with new scopes →" + : "Connect to " + serverLabel + " →"; + btn.setAttribute( + "aria-label", + err.code === "mcp_insufficient_scope" + ? "Re-consent with new scopes for " + serverLabel + : "Connect to " + serverLabel, + ); + btn.addEventListener("click", function () { + var 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. + if (!consentUrl.startsWith("/v1/api/mcp/oauth/start")) { + showToast("Invalid consent URL"); + return; + } + var sep = consentUrl.indexOf("?") >= 0 ? "&" : "?"; + var url = + consentUrl + + sep + + "return_url=" + + encodeURIComponent(window.location.href); + window.open(url, "_blank", "noopener"); + }); + body.appendChild(btn); + _onConsentDetected(err.server); + } + + wrapper.appendChild(body); + + var details = document.createElement("details"); + var summary = document.createElement("summary"); + summary.textContent = "raw payload"; + details.appendChild(summary); + var pre = document.createElement("pre"); + pre.className = "tool-output"; + pre.textContent = _tryPrettyJson(rawJson) || _redactApiKeys(rawJson); + details.appendChild(pre); + wrapper.appendChild(details); + + return wrapper; +} + // --------------------------------------------------------------------------- // HLS lazy-loader (follows the mermaid.js lazy-load pattern in // /shared/renderer.js) @@ -5551,6 +5771,265 @@ document } })(); +// =========================================================================== +// 15. MCP server connections settings panel +// =========================================================================== + +var _pendingRevokeServer = null; +var _settingsTrap = null; +var _revokeMcpTrap = null; +var _settingsReturnFocus = null; + +function openSettingsPanel() { + var overlay = document.getElementById("settings-overlay"); + if (!overlay) return; + _settingsReturnFocus = document.activeElement; + overlay.style.display = "flex"; + + if (_settingsTrap) document.removeEventListener("keydown", _settingsTrap); + _settingsTrap = function (e) { + if (e.key === "Escape") { + // If the nested revoke confirmation is open, let its own trap + // handle Escape — closing inner-first matches the delete-ws flow. + var inner = document.getElementById("revoke-mcp-overlay"); + if (inner && inner.style.display !== "none") return; + e.preventDefault(); + closeSettingsPanel(); + return; + } + if (e.key === "Tab") { + var box = document.getElementById("settings-box"); + if (!box) return; + var focusable = box.querySelectorAll( + "button, [href], input, select, textarea, [tabindex]:not([tabindex='-1'])", + ); + if (!focusable.length) return; + var first = focusable[0]; + var last = focusable[focusable.length - 1]; + if (e.shiftKey && document.activeElement === first) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && document.activeElement === last) { + e.preventDefault(); + first.focus(); + } + } + }; + document.addEventListener("keydown", _settingsTrap); + + loadMcpConnections(); + + var closeBtn = document.getElementById("settings-close-btn"); + if (closeBtn) closeBtn.focus(); +} + +function closeSettingsPanel() { + var overlay = document.getElementById("settings-overlay"); + if (overlay) overlay.style.display = "none"; + if (_settingsTrap) { + document.removeEventListener("keydown", _settingsTrap); + _settingsTrap = null; + } + if ( + _settingsReturnFocus && + typeof _settingsReturnFocus.focus === "function" + ) { + try { + _settingsReturnFocus.focus(); + } catch (_) {} + } + _settingsReturnFocus = null; +} + +function loadMcpConnections() { + var loadingEl = document.getElementById("settings-mcp-loading"); + var emptyEl = document.getElementById("settings-mcp-empty"); + var tableEl = document.getElementById("settings-mcp-table"); + var errorEl = document.getElementById("settings-mcp-error"); + if (!loadingEl || !emptyEl || !tableEl || !errorEl) return; + loadingEl.style.display = ""; + emptyEl.style.display = "none"; + tableEl.style.display = "none"; + errorEl.style.display = "none"; + + authFetch("/v1/api/mcp/oauth/connections") + .then(function (r) { + if (!r.ok) throw new Error("HTTP " + r.status); + return r.json(); + }) + .then(function (data) { + loadingEl.style.display = "none"; + var connections = + data && Array.isArray(data.connections) ? data.connections : []; + renderMcpConnections(connections); + // Clear AFTER the table renders so the badge reflects "user has + // seen current state" rather than "user opened the panel" — a + // failed fetch keeps the pending-consent signal until the user + // gets confirmation that consents are in fact reachable. + _clearConsentBadge(); + }) + .catch(function (err) { + loadingEl.style.display = "none"; + errorEl.style.display = ""; + errorEl.textContent = "Failed to load connections: " + err.message; + }); +} + +function _clearChildren(node) { + while (node && node.firstChild) node.removeChild(node.firstChild); +} + +function renderMcpConnections(list) { + var emptyEl = document.getElementById("settings-mcp-empty"); + var tableEl = document.getElementById("settings-mcp-table"); + var tbody = document.getElementById("settings-mcp-tbody"); + if (!emptyEl || !tableEl || !tbody) return; + if (!list.length) { + tableEl.style.display = "none"; + emptyEl.style.display = ""; + return; + } + emptyEl.style.display = "none"; + tableEl.style.display = ""; + _clearChildren(tbody); + for (var i = 0; i < list.length; i++) { + var conn = list[i]; + var tr = document.createElement("tr"); + + var serverTd = document.createElement("td"); + serverTd.textContent = conn.server_name || ""; + tr.appendChild(serverTd); + + var scopesTd = document.createElement("td"); + scopesTd.textContent = conn.scopes || "(none)"; + tr.appendChild(scopesTd); + + var createdTd = document.createElement("td"); + createdTd.textContent = _formatRelativeTimestamp(conn.created); + createdTd.title = conn.created || ""; + tr.appendChild(createdTd); + + var refreshedTd = document.createElement("td"); + if (conn.last_refreshed) { + refreshedTd.textContent = _formatRelativeTimestamp(conn.last_refreshed); + refreshedTd.title = conn.last_refreshed; + } else { + refreshedTd.textContent = "—"; + } + tr.appendChild(refreshedTd); + + var actionTd = document.createElement("td"); + var btn = document.createElement("button"); + btn.type = "button"; + btn.className = "settings-revoke-btn"; + btn.textContent = "Revoke"; + var serverNameForRevoke = conn.server_name || ""; + btn.setAttribute( + "aria-label", + "Revoke connection to " + serverNameForRevoke, + ); + (function (name) { + btn.addEventListener("click", function () { + promptRevokeMcp(name); + }); + })(serverNameForRevoke); + actionTd.appendChild(btn); + tr.appendChild(actionTd); + + tbody.appendChild(tr); + } +} + +function promptRevokeMcp(server) { + if (!server) return; + _pendingRevokeServer = server; + var msg = document.getElementById("revoke-mcp-message"); + var overlay = document.getElementById("revoke-mcp-overlay"); + if (msg) { + msg.textContent = + "Disconnect " + + server + + "? Tools that need this server will require re-consent."; + } + if (overlay) overlay.style.display = "flex"; + + if (_revokeMcpTrap) document.removeEventListener("keydown", _revokeMcpTrap); + _revokeMcpTrap = function (e) { + if (e.key === "Escape") { + e.preventDefault(); + cancelRevokeMcp(); + return; + } + if (e.key === "Tab") { + var box = document.getElementById("revoke-mcp-box"); + if (!box) return; + var focusable = box.querySelectorAll("button"); + if (!focusable.length) return; + var first = focusable[0]; + var last = focusable[focusable.length - 1]; + if (e.shiftKey && document.activeElement === first) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && document.activeElement === last) { + e.preventDefault(); + first.focus(); + } + } + }; + document.addEventListener("keydown", _revokeMcpTrap); + + var cancelBtn = overlay ? overlay.querySelector("button:not(.danger)") : null; + if (cancelBtn) cancelBtn.focus(); +} + +function cancelRevokeMcp() { + _pendingRevokeServer = null; + var overlay = document.getElementById("revoke-mcp-overlay"); + if (overlay) overlay.style.display = "none"; + if (_revokeMcpTrap) { + document.removeEventListener("keydown", _revokeMcpTrap); + _revokeMcpTrap = null; + } +} + +function confirmRevokeMcp() { + var server = _pendingRevokeServer; + if (!server) { + cancelRevokeMcp(); + return; + } + authFetch("/v1/api/mcp/oauth/connections/" + encodeURIComponent(server), { + method: "DELETE", + }) + .then(function (r) { + if (!r.ok) throw new Error("HTTP " + r.status); + cancelRevokeMcp(); + showToast("Disconnected " + server); + loadMcpConnections(); + }) + .catch(function (err) { + cancelRevokeMcp(); + showToast("Failed to revoke: " + err.message); + }); +} + +function _formatRelativeTimestamp(iso) { + if (!iso) return "—"; + try { + var d = new Date(iso); + if (isNaN(d.getTime())) return iso; + var now = new Date(); + var diffMs = now.getTime() - d.getTime(); + var sec = Math.round(diffMs / 1000); + if (sec < 60) return "just now"; + if (sec < 3600) return Math.round(sec / 60) + "m ago"; + if (sec < 86400) return Math.round(sec / 3600) + "h ago"; + return Math.round(sec / 86400) + "d ago"; + } catch (e) { + return iso; + } +} + document.addEventListener("keydown", function (e) { // Defer to modal's own keydown handler when any modal is open var modalIds = [ @@ -5558,6 +6037,8 @@ document.addEventListener("keydown", function (e) { "edit-title-overlay", "delete-ws-overlay", "ws-delete-overlay", + "settings-overlay", + "revoke-mcp-overlay", ]; for (var mi = 0; mi < modalIds.length; mi++) { var modal = document.getElementById(modalIds[mi]); @@ -5749,7 +6230,7 @@ document.addEventListener("keydown", function (e) { }); // =========================================================================== -// 15. Init +// 16. Init // =========================================================================== function initWorkstreams() { diff --git a/turnstone/ui/static/index.html b/turnstone/ui/static/index.html index 23479e08..1f10dd9d 100644 --- a/turnstone/ui/static/index.html +++ b/turnstone/ui/static/index.html @@ -41,6 +41,16 @@ > ☾ + @@ -360,6 +370,74 @@ + + + + + +