diff --git a/backend/fluksio/api/deps.py b/backend/fluksio/api/deps.py index da2223c..069e539 100644 --- a/backend/fluksio/api/deps.py +++ b/backend/fluksio/api/deps.py @@ -18,9 +18,9 @@ from fluksio.core.db import engine from fluksio.flow import panels from fluksio.flow.artifacts import is_reference from fluksio.flow.controller import FlowController -from fluksio.flow.dashboards import DashboardStore +from fluksio.flow.dashboards import DashboardDef, DashboardStore from fluksio.flow.workers import PythonWorkerPool -from fluksio.models import TokenPayload, User +from fluksio.models import OAuthClient, TokenPayload, User reusable_oauth2 = OAuth2PasswordBearer( tokenUrl=f"{settings.API_V1_STR}/login/access-token" @@ -80,6 +80,60 @@ def _panel_messages(panel_id: str, request: Request) -> set[str]: return panels.messages_for(panel_id, store) +def _locked(defn: DashboardDef, live: dict[str, Any]) -> bool: + """Whether this dashboard is read-only right now. + + Read the way the client reads it (``Dashboard/settings.tsx``): the bound + message if it is carrying something, and the stored value otherwise. Live, + because a lock a flow drives is the case the binding exists for — judging + it from the stored fallback alone would refuse every control on a + dashboard its flow has unlocked. + """ + setting = defn.settings.get("locked") + if setting is None: + return False + value = live.get(setting.message) if setting.message else None + return (setting.value if value is None else value) is True + + +def _panel_writable(panel_id: str, request: Request) -> set[str]: + """The messages this panel may publish to. + + The same walk as the read allowlist, minus whatever a locked dashboard + contributes: ``locked`` is a dashboard saying it is there to be looked at, + and until this it stopped only the client drawing the control — the server + took the publish from a screen that asked anyway. + + Not quite all of it: a querying chart publishes the request it reads by, + and a locked dashboard whose charts cannot ask goes blank rather than + read-only. That request is the exception ``panels.requests_of`` names. + + Union, exactly as the allowlist itself is: a message one dashboard on this + panel displays and another controls stays writable, because the unlocked + one is what entitles the screen to it. Reads are untouched — read-only is + not blind, and a locked dashboard has to keep drawing live data. + """ + store: DashboardStore | None = getattr(request.app.state, "dashboard_store", None) + if store is None: + return set() + defns = panels.dashboards_for(panel_id, store) + bound = sorted( + {s.message for d in defns if (s := d.settings.get("locked")) and s.message} + ) + controller: FlowController | None = getattr( + request.app.state, "flow_controller", None + ) + live = controller.state.get_present(bound) if bound and controller else {} + writable: set[str] = set() + for defn in defns: + writable |= ( + panels.requests_of(defn) + if _locked(defn, live) + else panels.messages_of(defn) + ) + return writable + + def _panel_digests(panel_id: str, request: Request) -> set[str]: """The artifacts this panel's messages are pointing at right now. @@ -116,7 +170,10 @@ def _panel_may(payload: dict[str, Any], request: Request) -> None: asks for its window by publishing a request, which is why that request counts as one of its widget's messages. Bounded to those, though: a screen on a wall has no business reaching a message no tile on it draws, and the - catalogue behind ``GET /messages/`` is the whole namespace at once. + catalogue behind ``GET /messages/`` is the whole namespace at once. A + dashboard that says it is locked is bounded further still — it entitles a + panel to read every message it names and to publish to none of them but + the requests its own charts ask by. # ponytail: the panel file and the published dashboards are re-read per # request. Cache them behind the store's version if this shows up in a @@ -146,13 +203,16 @@ def _panel_may(payload: dict[str, Any], request: Request) -> None: # value in. Percent-decoded, because a message name is a path segment # here and the client encodes it as one. rest = unquote(path[len(f"{api}/messages/") :]) + scope = _panel_messages if method == "GET" and rest.endswith(_HISTORY): name = rest[: -len(_HISTORY)] elif method == "POST": name = rest + # A locked dashboard entitles the panel to nothing it can publish. + scope = _panel_writable else: name = "" - allowed = bool(name) and name in _panel_messages(panel.id, request) + allowed = bool(name) and name in scope(panel.id, request) elif method == "GET" and path.startswith(f"{api}/artifacts/"): # The bytes behind a media message a tile on this panel is drawing. # Scoped to what those messages hold *now*, which is exactly what a @@ -182,6 +242,10 @@ def decode_token(token: str, request: Request | None = None) -> dict[str, Any]: refuses it outright, so the only door it fits is the one ``_panel_may`` guards. + An agent's token is held to one thing more: the client it names has to + still be registered, which is what makes revoking an agent take effect now + rather than whenever its stateless token happens to expire. + The last branch is the seam a hosted deployment widens: a portal this instance was enrolled with signs tokens with a key pinned at enrolment, and they name the portal account holding them, which resolves @@ -209,9 +273,38 @@ def decode_token(token: str, request: Request | None = None) -> dict[str, Any]: raise InvalidTokenError("a panel token must name its panel") return _gate_panel(panel_token, request) try: - return security.decode_oauth_token(token) + agent = security.decode_oauth_token(token) except InvalidTokenError: return _gate_panel(cloud_config.decode_portal_token(token), request) + return _gate_agent(agent) + + +def oauth_client_lives(client_id: str) -> bool: + """Is the agent a token names still a registered client? Blocking. + + An MCP access token is a stateless JWT good until it expires, so deleting + the client row revoked nothing that had already been handed out. The + client id is a claim the token has always carried, so this holds the ones + already in circulation just as well as the next one minted. + + One primary-key read per agent request, against a table with a row per + registered agent. The request it gates goes on to look its user up the + same way, and the panel gate next door re-reads the panels file and every + published dashboard from disk, so this is the cheapest check in here. + """ + try: + key = uuid.UUID(client_id) + except ValueError: + return False + with Session(engine) as session: + return session.get(OAuthClient, key) is not None + + +def _gate_agent(payload: dict[str, Any]) -> dict[str, Any]: + """Refuse a token whose agent has been revoked.""" + if not oauth_client_lives(str(payload.get("client_id") or "")): + raise InvalidTokenError("this agent's registration was withdrawn") + return payload def _gate_panel(payload: dict[str, Any], request: Request | None) -> dict[str, Any]: diff --git a/backend/fluksio/api/routes/oauth.py b/backend/fluksio/api/routes/oauth.py index ce77e21..f69f512 100644 --- a/backend/fluksio/api/routes/oauth.py +++ b/backend/fluksio/api/routes/oauth.py @@ -555,9 +555,11 @@ def revoke_client(client_id: uuid.UUID, session: SessionDep) -> Any: """Withdraw one agent's access, leaving every other agent alone. Deleting the client cascades to its codes and refresh tokens, so it can - get nothing new and cannot come back without registering again. An access - token already in its hands keeps working until it expires - (``MCP_TOKEN_EXPIRE_MINUTES``) — those are stateless by design. + get nothing new and cannot come back without registering again. The access + token already in its hands stops working too, even though it is a + stateless JWT nobody can reach into: every door that takes one looks the + client up first (``api.deps.oauth_client_lives``), so the row deleted here + is the whole of the revocation. """ client = session.get(OAuthClient, client_id) if client is None: diff --git a/backend/fluksio/flow/panels.py b/backend/fluksio/flow/panels.py index 9c999c5..dc3bc19 100644 --- a/backend/fluksio/flow/panels.py +++ b/backend/fluksio/flow/panels.py @@ -18,7 +18,7 @@ from pathlib import Path from pydantic import BaseModel, Field, field_validator from fluksio.core.config import settings -from fluksio.flow.dashboards import DashboardNotFound, DashboardStore +from fluksio.flow.dashboards import DashboardDef, DashboardNotFound, DashboardStore from fluksio.flow.schemas import _validate_name @@ -101,29 +101,62 @@ def find(panel_id: str) -> PanelDef | None: return None -def messages_for(panel_id: str, store: DashboardStore) -> set[str]: - """Every message this panel's dashboards read or write. +def dashboards_for(panel_id: str, store: DashboardStore) -> list[DashboardDef]: + """The published documents this panel shows, in rail order. - What a screen is entitled to see, as its own dashboards define it. Read - from the published documents, since that is what a panel draws, and empty - for a panel that is gone — which is the same answer as "nothing". + Published, since that is what a panel draws. A name that no longer + resolves is a dashboard someone deleted and is skipped, and a panel that + is gone shows nothing. + + Handed back whole rather than walked here, because what a panel may do + with a message depends on the document it came from — a dashboard that + says it is locked entitles a screen to read it and not to touch it. + """ + panel = find(panel_id) + if panel is None: + return [] + found: list[DashboardDef] = [] + for name in panel.dashboards: + try: + found.append(store.read(name)) + except DashboardNotFound: + continue + return found + + +def messages_of(defn: DashboardDef) -> set[str]: + """Every message one dashboard reads or writes. A dashboard's own bound settings count, not only its widgets': the theme a panel is driven to is a message no tile on it draws, and a wall panel refused its own theme message is the one surface the setting exists for. """ - panel = find(panel_id) - if panel is None: - return set() - names: set[str] = set() - for dashboard in panel.dashboards: - try: - defn = store.read(dashboard) - except DashboardNotFound: - continue - names.update(defn.setting_messages) - for widget in defn.widgets: - names.update(widget.messages) - if widget.target: - names.add(widget.target) + names = set(defn.setting_messages) + for widget in defn.widgets: + names.update(widget.messages) + if widget.target: + names.add(widget.target) return names + + +def requests_of(defn: DashboardDef) -> set[str]: + """The publishes this dashboard makes in order to read. + + A querying chart asks a flow for the series it draws by publishing a + request, so that publish is how the tile reads rather than something + anyone touched. Every other message a dashboard sends comes from a + control, which is what marking it read-only turns off — so this is what a + locked dashboard is still entitled to send. + """ + return {w.target for w in defn.widgets if w.type == "chart" and w.target} + + +def messages_for(panel_id: str, store: DashboardStore) -> set[str]: + """Every message this panel's dashboards read or write. + + What a screen is entitled to see, as its own dashboards define it, and + empty for a panel that is gone — which is the same answer as "nothing". + """ + return { + name for defn in dashboards_for(panel_id, store) for name in messages_of(defn) + } diff --git a/backend/fluksio/mcp/http.py b/backend/fluksio/mcp/http.py index 4a3800e..6f77fc0 100644 --- a/backend/fluksio/mcp/http.py +++ b/backend/fluksio/mcp/http.py @@ -13,12 +13,14 @@ from urllib.parse import urlparse import httpx import jwt +from fastapi.concurrency import run_in_threadpool from mcp.server.auth.provider import AccessToken from mcp.server.auth.settings import AuthSettings from mcp.server.transport_security import TransportSecuritySettings from pydantic import AnyHttpUrl from starlette.applications import Starlette +from fluksio.api.deps import oauth_client_lives from fluksio.core import security from fluksio.core.config import settings from fluksio.mcp import server @@ -30,11 +32,18 @@ _INTERNAL_BASE = "http://fluksio-mcp.internal" class _JWTVerifier: - """Accept only tokens minted for the MCP channel. + """Accept only tokens minted for the MCP channel, by agents still registered. A perfectly valid browser token is refused: it was issued for a person's session, and honouring it here would make agent traffic indistinguishable from theirs. + + The client row is looked up because the token itself cannot be withdrawn: + it is stateless and good until ``MCP_TOKEN_EXPIRE_MINUTES`` runs out, so + the registration it names is the thing revoking an agent actually removes. + Refusing at the door rather than leaving it to the API the tools call means + a revoked agent gets the 401 that sends it back to authorize, instead of a + tool listing that works and a tool call that does not. """ async def verify_token(self, token: str) -> AccessToken | None: @@ -46,9 +55,17 @@ class _JWTVerifier: if payload.get("mcp") is not True: logger.debug("MCP token rejected: not an MCP-channel token") return None + client_id = str(payload.get("client_id", "")) + # Off the event loop: the lookup is SQLite, like every other read this + # process makes, and the session it opens is blocking. + if not await run_in_threadpool(oauth_client_lives, client_id): + logger.debug( + "MCP token rejected: agent %s is no longer registered", client_id + ) + return None return AccessToken( token=token, - client_id=str(payload.get("client_id", "")), + client_id=client_id, scopes=[security.MCP_SCOPE], subject=str(payload.get("sub", "")), ) diff --git a/backend/tests/api/routes/test_oauth.py b/backend/tests/api/routes/test_oauth.py index c6dcdaf..100e05c 100644 --- a/backend/tests/api/routes/test_oauth.py +++ b/backend/tests/api/routes/test_oauth.py @@ -270,6 +270,17 @@ def test_one_agent_can_be_revoked_without_touching_the_others( assert revoked.status_code == 200, revoked.text assert all(c["id"] != client_id for c in listed()) + # And the access token already in its hands stops working, rather than + # outliving the revocation by up to MCP_TOKEN_EXPIRE_MINUTES: the token is + # stateless, so the client row it names is what withdrawing it removes. + assert ( + client.post( + f"{settings.API_V1_STR}/login/test-token", + headers={"Authorization": f"Bearer {tokens['access_token']}"}, + ).status_code + == 401 + ) + # Its refresh token went with it, so it cannot mint itself a new one. refreshed = client.post( f"{PREFIX}/token", diff --git a/backend/tests/api/routes/test_panels.py b/backend/tests/api/routes/test_panels.py index 0efdda0..fd29d89 100644 --- a/backend/tests/api/routes/test_panels.py +++ b/backend/tests/api/routes/test_panels.py @@ -345,6 +345,88 @@ def test_a_panel_speaks_only_its_own_widgets_messages( ) +def _publish_settings( + client: TestClient, headers: dict[str, str], name: str, settings_: dict +) -> None: + """Give a published dashboard these settings and publish it again.""" + saved = client.get(f"{DASHBOARDS}/{name}?draft=true", headers=headers).json() + saved["settings"] = settings_ + written = client.put(f"{DASHBOARDS}/{name}", headers=headers, json=saved) + assert written.status_code == 200, written.text + published = client.post( + f"{DASHBOARDS}/{name}/publish", + headers=headers, + json={"version": written.json()["version"]}, + ) + assert published.status_code == 200, published.text + + +def test_a_locked_dashboard_entitles_a_panel_to_read_and_not_to_publish( + client: TestClient, superuser_token_headers: dict[str, str] +) -> None: + """`locked` bounds what a screen may send, not what it may draw. + + It used to bound neither: the client stopped offering the control and the + server took the publish from anything that asked anyway. + + Unions like the allowlist itself, which is what the second dashboard here + is for — a message a locked dashboard shows and an unlocked one controls + stays writable, because the unlocked one is what entitles the panel to it. + """ + _dashboard_with(client, superuser_token_headers, "panel_locked", PANEL_WIDGETS) + _publish_settings( + client, superuser_token_headers, "panel_locked", {"locked": {"value": True}} + ) + _dashboard_with( + client, + superuser_token_headers, + "panel_open", + [{"id": "w_button", "type": "button", "config": {"target": "demo.button"}}], + ) + _panels( + client, + superuser_token_headers, + {"panels": [{"id": "foyer", "dashboards": ["panel_locked", "panel_open"]}]}, + ) + panel_headers = _pair(client, superuser_token_headers, "foyer") + messages = f"{settings.API_V1_STR}/messages" + + # Only the locked dashboard names it, so there is nothing left to send it. + assert ( + client.post( + f"{messages}/demo.slider", headers=panel_headers, json={"value": 1} + ).status_code + == 403 + ) + # 404, not 403: past the gate, and refused by an engine that knows no such + # message. The unlocked dashboard binds this one too. + assert ( + client.post( + f"{messages}/demo.button", headers=panel_headers, json={"value": 1} + ).status_code + == 404 + ) + # A querying chart's request survives the lock: publishing it is how that + # tile *reads*, and a locked dashboard whose charts cannot ask goes blank + # rather than read-only. + assert ( + client.post( + f"{messages}/demo.query_request", + headers=panel_headers, + json={"value": {"range_s": 3600, "interval_s": 60}}, + ).status_code + == 404 + ) + # And what the locked dashboard draws is still readable — read-only is not + # blind, and a wall panel showing stale numbers is the failure to avoid. + assert ( + client.get( + f"{messages}/demo.temperature/history", headers=panel_headers + ).status_code + == 200 + ) + + def test_a_panel_may_read_its_dashboards_own_settings( client: TestClient, superuser_token_headers: dict[str, str] ) -> None: @@ -854,8 +936,9 @@ def test_a_socket_pushes_only_the_frames_it_was_asked_for(tmp_path) -> None: store = ArtifactStore(tmp_path / "artifacts") store.volatile = VolatileStore(tmp_path / "ring", limit_bytes=1_000_000) - frame = store.put([b"\x89PNG..."], name="f.png", media_type="image/png", - volatile=True) + frame = store.put( + [b"\x89PNG..."], name="f.png", media_type="image/png", volatile=True + ) kept = store.put([b"checkpoint"], name="w.pt") asking = orjson.dumps({"type": "media", "names": ["cam.frame", "other.frame"]}) diff --git a/backend/tests/mcp/test_mcp_http.py b/backend/tests/mcp/test_mcp_http.py index da04760..8a6f89a 100644 --- a/backend/tests/mcp/test_mcp_http.py +++ b/backend/tests/mcp/test_mcp_http.py @@ -2,25 +2,38 @@ import asyncio import uuid -from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Generator from datetime import timedelta from typing import Any import httpx import pytest from fastapi.testclient import TestClient +from sqlmodel import Session from fluksio.core import security from fluksio.core.config import settings from fluksio.main import app +from fluksio.models import OAuthClient MCP_HEADERS = {"Accept": "application/json, text/event-stream"} -def mcp_token(user_id: uuid.UUID) -> str: - return security.create_oauth_access_token( - user_id, uuid.uuid4(), timedelta(minutes=5) - ) +def mcp_token(user_id: uuid.UUID, client_id: uuid.UUID) -> str: + return security.create_oauth_access_token(user_id, client_id, timedelta(minutes=5)) + + +@pytest.fixture +def agent(db: Session) -> Generator[uuid.UUID, None, None]: + """A registered agent, because a token naming one that is gone is refused.""" + row = OAuthClient(client_name="Test agent", redirect_uris=[]) + db.add(row) + db.commit() + yield row.id + left = db.get(OAuthClient, row.id) + if left is not None: + db.delete(left) + db.commit() @pytest.fixture @@ -96,12 +109,15 @@ def test_a_browser_token_is_not_an_agent_token( def test_an_agent_can_list_and_call_tools( - over_mcp, client: TestClient, superuser_token_headers: dict[str, str] + over_mcp, + client: TestClient, + superuser_token_headers: dict[str, str], + agent: uuid.UUID, ) -> None: me = client.get( f"{settings.API_V1_STR}/users/me", headers=superuser_token_headers ).json() - token = mcp_token(uuid.UUID(me["id"])) + token = mcp_token(uuid.UUID(me["id"]), agent) async def block(http: httpx.AsyncClient) -> tuple[Any, Any]: listed = await http.post("/mcp", **rpc(token, "tools/list")) @@ -121,3 +137,36 @@ def test_an_agent_can_list_and_call_tools( assert called.status_code == 200 assert "error" not in called.json() assert called.json()["result"]["isError"] is False + + +def test_revoking_an_agent_stops_the_token_it_already_holds( + over_mcp, + client: TestClient, + db: Session, + superuser_token_headers: dict[str, str], + agent: uuid.UUID, +) -> None: + """Withdrawing an agent has to bite now, not whenever its token expires. + + An access token is a stateless JWT valid for its whole life, so the + registration it names is the only thing deleting a client takes away. + + Both calls share one block because the session manager runs once. + """ + me = client.get( + f"{settings.API_V1_STR}/users/me", headers=superuser_token_headers + ).json() + token = mcp_token(uuid.UUID(me["id"]), agent) + + async def block(http: httpx.AsyncClient) -> tuple[Any, Any]: + before = await http.post("/mcp", **rpc(token, "tools/list")) + row = db.get(OAuthClient, agent) + assert row is not None + db.delete(row) + db.commit() + after = await http.post("/mcp", **rpc(token, "tools/list")) + return before, after + + before, after = over_mcp(block) + assert before.status_code == 200 + assert after.status_code == 401 diff --git a/frontend/src/components/Dashboard/publish.tsx b/frontend/src/components/Dashboard/publish.tsx index 4e26ae0..e389f80 100644 --- a/frontend/src/components/Dashboard/publish.tsx +++ b/frontend/src/components/Dashboard/publish.tsx @@ -43,10 +43,10 @@ function confirms(live: unknown, sent: unknown): boolean { * disabled — a dashboard that silently swallows a press looks broken rather * than locked. * - * ponytail: this is a read-only surface, not an authorisation boundary. The - * server still takes a publish from a panel credential whose dashboard says - * locked, because the credential's own allowlist is what bounds it. Making it - * a real lock means carrying the flag into `_panel_may`. + * Not the boundary, though — the affordance. A panel credential is bounded by + * the same flag on the server (`api/deps.py`, `_panel_writable`), which is + * what refuses a publish from a screen that asks anyway; this is what keeps a + * control from looking pressable when it is not. * * Its own module rather than `widgets.tsx`, which every widget file is * imported *by*: a control drawn in a file of its own can only reach this