mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
464450b9e2
* feat(mcp): admin status, deferred-consent persistence, operator docs (Phase 9)
Completes the OAuth-MCP build-out (Phases 0-8 shipped) by closing the
operator + deferred-consent gaps:
1. **Per-(user, server) deferred-consent persistence** — when a
non-interactive run (scheduled / channel) hits ``mcp_consent_required``
or ``mcp_insufficient_scope``, the sync pool dispatchers now upsert a
row into a new ``mcp_pending_consent`` table. The dashboard hydrates
the gear-icon badge from this table on load, so users who weren't
online to see the in-flight SSE prompt still surface the deferred
work on next login. Cleared automatically by the OAuth callback
handler on consent completion; manual user dismiss via new DELETE
endpoints. Composite PK ``(user_id, server_name)`` collapses repeat
occurrences for the same server — no NULLs-not-distinct trap.
2. **Admin status pill + bulk-revoke** — the MCP Servers admin row now
shows ``consented_users_count`` for ``auth_type=oauth_user`` rows
when ≥1, with a two-step-confirm ``bulk-revoke`` button that drops
every user's token for the server via the existing
``delete_mcp_oauth_rows_by_server_name`` primitive. Upstream RFC
7009 revoke is intentionally NOT attempted in bulk (avoids N
upstream HTTP calls per admin click); audit detail records
``upstream_revoke_outcome=bulk_admin_no_upstream``. A "last
refresh" pill (age + outcome) renders on each row, sourced from a
new ``_last_refresh`` dict populated by ``_refresh_server`` on every
call (both manual ``refresh_sync`` and the ``_cb_auto_reconnect``
follow-up).
3. **ClientType.SCHEDULED** added to the prompts module + scheduler
passes it through to ``create_workstream``. ``ChatSession`` now
computes ``_is_interactive_for_consent`` at construction (WEB / CLI
are interactive; CHAT / SCHEDULED are not) and plumbs the flag
through ``call_tool_sync`` / ``read_resource_sync`` /
``get_prompt_sync`` to the three sync dispatchers. The wrap at the
``_is_structured_error`` gate routes consent codes to the new
``_record_pending_consent_best_effort`` helper for non-interactive
callers only; interactive sessions stay on the in-flight SSE path
Phase 8 ships unchanged.
4. **Operator docs** — ``docs/mcp-oauth.md`` (operator guide, parallel
to ``docs/oidc.md``: ``auth_type`` choice, OAuth client setup,
encryption-key rotation, troubleshooting matrix) and
``docs/operations/mcp-oauth-headless.md`` (one-paragraph runbook
per ``feedback_runbook_trust_llm.md``: pre-consent recipe for
scheduled / channel-driven runs).
Schema
- Migration 054_mcp_pending_consent.py — composite PK
``(user_id, server_name)``, ``occurrence_count`` + ``first_seen_at`` /
``last_seen_at`` for recency metadata, ``idx_mcp_pending_consent_user``
for the badge-load query. No FKs (matches the rest of the
oauth_user schema).
- Migration 055_mcp_user_tokens_server_index.py — adds
``idx_mcp_user_tokens_server`` on ``(server_name, expires_at)`` so
the admin pill's ``count_mcp_consented_users_*`` queries don't
full-scan against the leading-``user_id`` composite PK.
- Cross-backend: works on SQLite + PostgreSQL via dialect-specific
``on_conflict_do_update`` (PG ``postgresql.insert`` / SQLite
``sqlalchemy.dialects.sqlite.insert``). No ``NULLS NOT DISTINCT``
needed — the simplified PK eliminates the cross-version trap.
Endpoints
- ``GET /v1/api/mcp/oauth/pending`` — list deferred-consent records for
the authenticated user. Install-level gate via cached
``any_oauth_user_mcp_servers`` short-circuits to ``{pending: 0}`` on
installs with no oauth_user MCP servers — local-auth deployments
exercise zero new storage queries on this path. The gate result is
cached on ``app.state`` with a 60s TTL to spare repeat dashboard
loads.
- ``DELETE /v1/api/mcp/oauth/pending/{server_name}`` — single dismiss.
Returns 204 in both existed-and-deleted and never-existed cases
(no cross-tenant existence leak); audits
``mcp_server.oauth.pending_consent_dismissed`` with
``mode=single`` + ``cleared=0|1`` so a session-hijack attacker
scrubbing breadcrumbs leaves an audit trail.
- ``DELETE /v1/api/mcp/oauth/pending`` — bulk dismiss; audits
``mode=bulk`` + ``cleared=N``.
- ``POST /v1/api/admin/mcp-servers/{name}/bulk-revoke`` — admin
bulk-revoke for the named server's per-user tokens. Requires
``admin.mcp`` permission + 400s when the row isn't ``oauth_user``.
All four registered on both ``turnstone-server`` and
``turnstone-console`` (mirrors the Phase 8 ``/connections`` endpoint
shape).
Performance
- Admin list handler now uses a single ``GROUP BY`` bulk-count query
(``count_mcp_consented_users_grouped_by_server``) wrapped in
``asyncio.to_thread`` rather than N per-row sync DB round-trips
inside the async handler. Skipped entirely when no row is
oauth_user.
Frontend
- ``ui/static/app.js``: ``loadPendingConsents()`` hydrates the
existing ``_pendingConsentServers`` set on dashboard init + after
the user opens the settings modal. Endpoint failures stay silent
— the badge will be re-driven by the next in-flight tool error.
- ``console/static/admin.js``: ``consented_users_count`` pill +
``bulk-revoke`` button on each MCP row (only when ≥1 consented),
two-step confirm matching the existing delete pattern. ``last-
refresh`` age + outcome pill in the per-row status cell, sourced
from the freshest per-node entry in ``status[*].last_refresh_at`` /
``last_refresh_outcome``. CSS for the pills in ``style.css``.
Tests
- ``test_mcp_pending_consent_storage`` — 13 tests covering upsert
idempotency, list ordering, per-user isolation, single/bulk delete,
count-by-server + grouped variant, install-level gate.
- ``test_mcp_pending_consent_dispatch`` — 9 tests, including the
boundary-cross gate per ``feedback_tests_through_boundaries.md``:
drives the real ``call_tool_sync`` → ``_dispatch_pool_sync`` →
``_is_structured_error`` → ``_record_pending_consent_best_effort``
with a mocked classified-lookup so the structural plumb-through is
verified end-to-end. Includes a storage-failure test that pins
the docstring's "envelope unchanged on storage failure" promise.
- ``test_mcp_pending_consent_endpoints`` — 11 tests: install gate,
list-for-self, no-cross-user-leak, single/bulk delete, idempotent
not-found, audit emission on single + bulk + cross-tenant dismiss.
- ``test_chat_session_interactivity_flag`` — 7 tests pinning the
``ClientType`` → ``_is_interactive_for_consent`` mapping against
the module-level ``INTERACTIVE_CONSENT_CLIENT_TYPES`` frozenset.
- ``test_mcp_admin_bulk_revoke`` — 7 tests covering admin.mcp
permission gate, 404 on missing, 400 on non-oauth_user, 200 with
``rows_deleted`` + ``consented_users_before``, audit row with
``upstream_revoke_outcome=bulk_admin_no_upstream``, cross-server
isolation.
- ``test_mcp_oauth_handlers`` — 2 new callback tests pin the post-
callback ``delete_mcp_pending_consent`` invocation: success-clears
+ storage-failure-still-redirects.
- 636 tests pass on the impacted surface (47 new + Phase 0-8 OAuth-MCP
+ session + prompts + storage admin). ruff + mypy clean.
Hard invariants honored
- Static path byte-identical for ``auth_type ∈ {none, static}`` — the
flag flows only through the pool dispatchers, which only fire when
the row resolves to ``oauth_user``.
- ``asyncio.timeout`` (not ``asyncio.wait_for``) preserved on every
AS / SDK / pool-loop await — no new awaits added to the hot path.
- Install-level gate on the badge endpoint: cached
``any_oauth_user_mcp_servers`` returns False on a row-less
deployment → endpoint short-circuits without touching the pending-
consent table; 60s TTL bounds the staleness window after admin
flips ``auth_type``.
- Operator-actionable codes (key-unknown, url-insecure, *_forbidden)
explicitly filtered out of persistence — they're outside the
user-facing consent badge scope.
- Best-effort write: the structured-error envelope returned to the
agent is identical whether the persistence write succeeds or fails
(storage exception is logged with type name only — no chained
context that could carry an ``httpx.Request`` bearer header).
- No ``exc_info=True`` on any new path that can chain a bearer-bearing
``httpx.Request``.
- Defensive parsing: ``_parse_pending_consent_envelope`` mirrors
``_is_structured_error``'s ``isinstance(decoded, dict)`` guard plus
filters scope tokens through ``is_valid_scope_token`` capped at
``MAX_INSUFFICIENT_SCOPE_REPORTED`` — defense-in-depth even though
production callers already validate upstream.
- Audit events on every dismiss endpoint so a session-control attacker
scrubbing dashboard breadcrumbs still leaves a trail.
Cross-backend
- Tested on SQLite via the conftest backend fixture.
- PostgreSQL path uses ``postgresql.insert(...).on_conflict_do_update``
parallel to the existing ``mcp_user_tokens`` upsert in Phase 3.
Deferred (not Phase 9 blockers)
- Multi-node pool eviction on bulk-revoke: only local-node sessions
would be evicted if we built it, and there's no bulk-by-server
primitive on MCPClientManager today; remote nodes will surface as
a 401 on next dispatch which refreshes through the (now empty)
token row.
- RFC 8693 / Azure OBO ``auth_type=oauth_token_exchange`` — captured
in the design doc as a future architectural direction (~600 LOC +
IdP-side admin work); requires OIDC token capture and per-MCP-server
resource-trust configuration that v1 does not ship.
* docs(mcp): address Copilot review feedback on Phase 9
- Fix misleading admin.js comment that claimed the refresh pill rendered
"<short-relative> <outcome>" — the pill actually renders only the short
age, with outcome reflected via CSS class and tooltip.
- Replace broken feedback_secrets_not_in_env.md repo-root link in
mcp-oauth.md with the inlined rationale (env-borne secrets reachable
via shell tools / os.environ; TOML secrets are not).
305 lines
10 KiB
Python
305 lines
10 KiB
Python
"""Boundary tests for the Phase 9 pending-consent write path.
|
|
|
|
Drives ``MCPClientManager._dispatch_pool_sync`` (and the helper it
|
|
calls, ``_record_pending_consent_best_effort``) and asserts that
|
|
deferred-consent records reach storage only on non-interactive callers.
|
|
|
|
Per ``feedback_tests_through_boundaries.md``, at least one test must
|
|
drive the real sync dispatcher → real ``_is_structured_error`` →
|
|
real ``_record_pending_consent_best_effort`` plumb-through; the
|
|
``_helpers`` unit tests below cover the classifier in isolation, but
|
|
the end-to-end test is the structural gate that catches
|
|
plumb-through regressions.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
import contextlib
|
|
import json
|
|
import threading
|
|
from typing import Any
|
|
from unittest.mock import patch
|
|
|
|
import pytest
|
|
|
|
from tests.conftest import make_mcp_token_cipher
|
|
from turnstone.core.mcp_client import (
|
|
_PENDING_CONSENT_PERSIST_CODES,
|
|
MCPClientManager,
|
|
_parse_pending_consent_envelope,
|
|
)
|
|
from turnstone.core.mcp_crypto import MCPTokenStore
|
|
from turnstone.core.mcp_oauth import TokenLookupResult
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helper-level unit tests (cheap, no event loop)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestParseEnvelope:
|
|
def test_consent_required_no_scopes(self) -> None:
|
|
env = json.dumps({"error": {"code": "mcp_consent_required", "server": "x", "detail": "d"}})
|
|
assert _parse_pending_consent_envelope(env) == ("mcp_consent_required", None)
|
|
|
|
def test_insufficient_scope_with_scopes(self) -> None:
|
|
env = json.dumps(
|
|
{
|
|
"error": {
|
|
"code": "mcp_insufficient_scope",
|
|
"server": "x",
|
|
"detail": "d",
|
|
"scopes_required": ["read", "write"],
|
|
}
|
|
}
|
|
)
|
|
assert _parse_pending_consent_envelope(env) == (
|
|
"mcp_insufficient_scope",
|
|
["read", "write"],
|
|
)
|
|
|
|
def test_operator_codes_filtered(self) -> None:
|
|
# Key-unknown / url-insecure / *_forbidden are operator-actionable,
|
|
# NOT user-consent-shaped. They must not produce pending-consent
|
|
# rows, regardless of whether the caller is interactive.
|
|
for code in (
|
|
"mcp_token_undecryptable_key_unknown",
|
|
"mcp_oauth_url_insecure",
|
|
"mcp_tool_call_forbidden",
|
|
"mcp_resource_read_forbidden",
|
|
"mcp_prompt_get_forbidden",
|
|
):
|
|
env = json.dumps({"error": {"code": code, "server": "x", "detail": "d"}})
|
|
assert _parse_pending_consent_envelope(env) is None, code
|
|
|
|
def test_malformed_json_returns_none(self) -> None:
|
|
assert _parse_pending_consent_envelope("not json") is None
|
|
assert _parse_pending_consent_envelope("") is None
|
|
|
|
def test_persist_codes_set_is_expected(self) -> None:
|
|
# Pin the contract — adding a new persistable code here is a
|
|
# deliberate design decision and should require a test update.
|
|
assert {
|
|
"mcp_consent_required",
|
|
"mcp_insufficient_scope",
|
|
} == _PENDING_CONSENT_PERSIST_CODES
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# End-to-end plumb-through (drives _dispatch_pool_sync)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _seed_oauth_server(backend: Any, *, name: str = "pool-srv") -> None:
|
|
backend.create_mcp_server(
|
|
server_id="srv-" + name,
|
|
name=name,
|
|
transport="streamable-http",
|
|
command="",
|
|
args="[]",
|
|
url="https://example.com/mcp",
|
|
headers="{}",
|
|
env="{}",
|
|
auto_approve=False,
|
|
enabled=True,
|
|
created_by="admin",
|
|
)
|
|
backend.update_mcp_server("srv-" + name, auth_type="oauth_user")
|
|
|
|
|
|
@pytest.fixture
|
|
def running_loop_mgr():
|
|
cfg: dict[str, Any] = {}
|
|
mgr = MCPClientManager(cfg)
|
|
loop = asyncio.new_event_loop()
|
|
thread = threading.Thread(target=loop.run_forever, daemon=True, name="phase9-test-loop")
|
|
thread.start()
|
|
mgr._loop = loop
|
|
try:
|
|
yield mgr, loop, thread
|
|
finally:
|
|
|
|
async def _drain(m: MCPClientManager) -> None:
|
|
task = m._user_pool_eviction_task
|
|
if task is not None:
|
|
task.cancel()
|
|
with contextlib.suppress(BaseException):
|
|
await task
|
|
m._user_pool_eviction_task = None
|
|
|
|
with contextlib.suppress(Exception):
|
|
asyncio.run_coroutine_threadsafe(_drain(mgr), loop).result(timeout=2)
|
|
loop.call_soon_threadsafe(loop.stop)
|
|
thread.join(timeout=2)
|
|
|
|
|
|
def _wire_mgr(mgr: MCPClientManager, backend: Any) -> None:
|
|
cipher = make_mcp_token_cipher()
|
|
from types import SimpleNamespace
|
|
from unittest.mock import MagicMock
|
|
|
|
app_state = SimpleNamespace(
|
|
auth_storage=backend,
|
|
mcp_token_store=MCPTokenStore(backend, cipher, node_id="test"),
|
|
mcp_oauth_http_client=MagicMock(),
|
|
mcp_oauth_refresh_locks={},
|
|
mcp_oauth_metadata_cache={},
|
|
)
|
|
mgr.set_storage(backend)
|
|
mgr.set_app_state(app_state)
|
|
|
|
|
|
def test_dispatch_persists_pending_for_non_interactive_caller(
|
|
running_loop_mgr: Any, backend: Any
|
|
) -> None:
|
|
"""Non-interactive caller hits ``mcp_consent_required`` → a
|
|
``mcp_pending_consent`` row appears for ``(user_id, server_name)``."""
|
|
mgr, _loop, _ = running_loop_mgr
|
|
_seed_oauth_server(backend)
|
|
_wire_mgr(mgr, backend)
|
|
|
|
async def _missing_token(**kwargs: Any) -> TokenLookupResult:
|
|
return TokenLookupResult(kind="missing")
|
|
|
|
with (
|
|
patch(
|
|
"turnstone.core.mcp_client.get_user_access_token_classified",
|
|
side_effect=_missing_token,
|
|
),
|
|
pytest.raises(RuntimeError) as exc_info,
|
|
):
|
|
mgr.call_tool_sync(
|
|
"mcp__pool-srv__echo",
|
|
{"payload": "hi"},
|
|
user_id="user-a",
|
|
timeout=10,
|
|
is_interactive_for_consent=False,
|
|
)
|
|
|
|
# Structured error envelope surfaces as RuntimeError to the caller.
|
|
payload = json.loads(str(exc_info.value)).get("error", {})
|
|
assert payload.get("code") == "mcp_consent_required"
|
|
|
|
# Persistent row written for the dashboard badge.
|
|
rows = backend.list_mcp_pending_consent_by_user("user-a")
|
|
assert len(rows) == 1
|
|
r = rows[0]
|
|
assert r["user_id"] == "user-a"
|
|
assert r["server_name"] == "pool-srv"
|
|
assert r["error_code"] == "mcp_consent_required"
|
|
assert r["occurrence_count"] == 1
|
|
|
|
|
|
def test_dispatch_does_not_persist_for_interactive_caller(
|
|
running_loop_mgr: Any, backend: Any
|
|
) -> None:
|
|
"""Interactive caller hits the same error path → NO row written.
|
|
|
|
Interactive (WEB / CLI) sessions surface the consent prompt in-flight
|
|
via the Phase 8 SSE renderer; persisting would just produce
|
|
immediately-stale dashboard badges.
|
|
"""
|
|
mgr, _loop, _ = running_loop_mgr
|
|
_seed_oauth_server(backend)
|
|
_wire_mgr(mgr, backend)
|
|
|
|
async def _missing_token(**kwargs: Any) -> TokenLookupResult:
|
|
return TokenLookupResult(kind="missing")
|
|
|
|
with (
|
|
patch(
|
|
"turnstone.core.mcp_client.get_user_access_token_classified",
|
|
side_effect=_missing_token,
|
|
),
|
|
pytest.raises(RuntimeError),
|
|
):
|
|
mgr.call_tool_sync(
|
|
"mcp__pool-srv__echo",
|
|
{"payload": "hi"},
|
|
user_id="user-a",
|
|
timeout=10,
|
|
is_interactive_for_consent=True,
|
|
)
|
|
|
|
assert backend.list_mcp_pending_consent_by_user("user-a") == []
|
|
|
|
|
|
def test_dispatch_returns_envelope_unchanged_on_storage_failure(
|
|
running_loop_mgr: Any, backend: Any
|
|
) -> None:
|
|
"""When ``upsert_mcp_pending_consent`` raises, the agent-observable
|
|
contract is unchanged: the structured-error ``RuntimeError`` still
|
|
surfaces with the original ``mcp_consent_required`` code. The doc-
|
|
string promises best-effort persistence; this test pins that
|
|
promise so a regression that propagates the storage exception would
|
|
fail visibly.
|
|
"""
|
|
mgr, _loop, _ = running_loop_mgr
|
|
_seed_oauth_server(backend)
|
|
_wire_mgr(mgr, backend)
|
|
|
|
async def _missing_token(**kwargs: Any) -> TokenLookupResult:
|
|
return TokenLookupResult(kind="missing")
|
|
|
|
original_upsert = backend.upsert_mcp_pending_consent
|
|
|
|
def _raise(*_a: Any, **_kw: Any) -> None:
|
|
raise RuntimeError("storage offline")
|
|
|
|
backend.upsert_mcp_pending_consent = _raise # type: ignore[method-assign]
|
|
try:
|
|
with (
|
|
patch(
|
|
"turnstone.core.mcp_client.get_user_access_token_classified",
|
|
side_effect=_missing_token,
|
|
),
|
|
pytest.raises(RuntimeError) as exc_info,
|
|
):
|
|
mgr.call_tool_sync(
|
|
"mcp__pool-srv__echo",
|
|
{"payload": "hi"},
|
|
user_id="user-a",
|
|
timeout=10,
|
|
is_interactive_for_consent=False,
|
|
)
|
|
finally:
|
|
backend.upsert_mcp_pending_consent = original_upsert # type: ignore[method-assign]
|
|
|
|
payload = json.loads(str(exc_info.value)).get("error", {})
|
|
assert payload.get("code") == "mcp_consent_required"
|
|
|
|
|
|
def test_dispatch_does_not_persist_for_operator_actionable_code(
|
|
running_loop_mgr: Any, backend: Any
|
|
) -> None:
|
|
"""Decrypt-failure → operator-actionable; even non-interactive callers
|
|
must NOT produce a user-facing pending-consent record (the user can't
|
|
resolve this by re-consenting).
|
|
"""
|
|
mgr, _loop, _ = running_loop_mgr
|
|
_seed_oauth_server(backend)
|
|
_wire_mgr(mgr, backend)
|
|
|
|
async def _decrypt_failure(**kwargs: Any) -> TokenLookupResult:
|
|
return TokenLookupResult(kind="decrypt_failure")
|
|
|
|
with (
|
|
patch(
|
|
"turnstone.core.mcp_client.get_user_access_token_classified",
|
|
side_effect=_decrypt_failure,
|
|
),
|
|
pytest.raises(RuntimeError) as exc_info,
|
|
):
|
|
mgr.call_tool_sync(
|
|
"mcp__pool-srv__echo",
|
|
{"payload": "hi"},
|
|
user_id="user-a",
|
|
timeout=10,
|
|
is_interactive_for_consent=False,
|
|
)
|
|
|
|
payload = json.loads(str(exc_info.value)).get("error", {})
|
|
assert payload.get("code") == "mcp_token_undecryptable_key_unknown"
|
|
# The operator-actionable code does NOT produce a pending-consent row.
|
|
assert backend.list_mcp_pending_consent_by_user("user-a") == []
|