ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-12 23:12:23 -06:00
Code Issues Packages Projects Releases Wiki Activity
Files
5a3f46a1fa7313fdd1dd0372dbcb38b6b2a1617d
turnstone/tests/test_mcp_oauth_revoke.py
T
Patrick Buckley 5a3f46a1fa feat(mcp): per-user MCP server consent UX (Phase 8) 
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.
2026-05-07 13:59:50 -07:00

400 lines
15 KiB
Python
Raw Blame History

"""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
Reference in New Issue View Git Blame Copy Permalink