diff --git a/docs/guides/plugins.md b/docs/guides/plugins.md index 05d56203..6b56f151 100644 --- a/docs/guides/plugins.md +++ b/docs/guides/plugins.md @@ -283,6 +283,32 @@ Analytics event schemas register through `register_event_schema(self, MyEvent)`: the plugin instance's declared name. The router above is reachable at `http://localhost:7727/api/v1/my-app/`. +### Who can reach plugin routes? + +Every route you register is gated by `PluginAccessMiddleware`. A request from a caller who +has turned your plugin off in their settings — or a request to a plugin disabled +system-wide — is answered with `403` before it reaches your handler, so a handler never has +to check plugin access itself. + +The gate identifies the caller from the request's bearer token. A request carrying no +readable token passes straight through it: a per-user preference is meaningless without a +user, so endpoints that are called by someone other than a logged-in user — an OAuth +callback, an inbound webhook — keep working. Those endpoints are reachable regardless of +anyone's plugin settings, so authenticate them the way you would any other public endpoint +(a signed state parameter, a request signature) rather than relying on the gate. + +That applies to the system-wide switch too: disabling a plugin turns its endpoints off for +every *identified* caller, but its unauthenticated endpoints stay reachable, because the +gate has no caller to check them against. Treat "disabled" as a per-caller answer, not as a +kill switch for the plugin's HTTP surface. + +The gate covers HTTP routes and nothing else — **a plugin's MCP tools are not gated at +all**. `/ai/mcp` is a mount rather than a plugin route, so no plugin name resolves for a +request to it, and the MCP surface carries no caller identity to check a preference +against. Disabling a plugin, for one user or system-wide, does not stop its tools being +called over MCP. If a tool must not run for someone, enforce that in the tool itself; do +not rely on the plugin being switched off. + ## Frontend Metadata The backend is the single source of truth for what the frontend shows about a plugin. Declare it through three per-concern hooks from `sparkth.lib.frontend.hooks`, registered in `__init__` like every other hook. Human-facing names are passed positionally: diff --git a/pyproject.toml b/pyproject.toml index 1a5cd227..769f3703 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,7 @@ requires-python = ">=3.14" dependencies = [ "aiohttp>=3.13.2", "alembic>=1.17.2", - "fastapi[standard]>=0.121.2", + "fastapi[standard]>=0.138.0", "fastmcp>=2.13.1", "pydantic>=2.12.4", "sqlmodel>=0.0.27", diff --git a/sparkth/core/plugins/constants.py b/sparkth/core/plugins/constants.py index 1ffa3625..10230e36 100644 --- a/sparkth/core/plugins/constants.py +++ b/sparkth/core/plugins/constants.py @@ -10,3 +10,7 @@ # ``/dashboard/``) and is the key joining the backend plugin, its DB row, # and its frontend counterpart. PLUGIN_NAME_PATTERN = re.compile(r"[a-z0-9]+(-[a-z0-9]+)*") + +# The Authorization scheme the plugin access gate reads the caller's token from, compared +# against the header's scheme lowercased (RFC 7235 makes it case-insensitive). +BEARER_SCHEME = "bearer" diff --git a/sparkth/core/plugins/middleware.py b/sparkth/core/plugins/middleware.py index 4ad97794..41d8e462 100644 --- a/sparkth/core/plugins/middleware.py +++ b/sparkth/core/plugins/middleware.py @@ -1,7 +1,8 @@ from typing import Any, Awaitable, Callable, cast -from fastapi import Request, Response, status +from fastapi import FastAPI, Request, Response, status from fastapi.responses import JSONResponse +from fastapi.routing import RouteContext, iter_route_contexts from sqlalchemy.exc import DatabaseError, OperationalError from sqlmodel import Session, select from sqlmodel.ext.asyncio.session import AsyncSession @@ -9,21 +10,61 @@ from starlette.routing import Match from sparkth.core.models.plugin import Plugin, UserPlugin +from sparkth.core.models.user import User +from sparkth.core.plugins.constants import BEARER_SCHEME from sparkth.core.routes import get_route_plugin_name -from sparkth.lib.db import get_async_session +from sparkth.lib.auth import decode_token_username, get_user_by_username +from sparkth.lib.db import session_scope from sparkth.lib.log import get_logger logger = get_logger(__name__) +def _bearer_token_username(request: Request) -> str | None: + """Username carried by the request's bearer token, or None when it carries no readable one. + + Reads the header directly rather than through the ``HTTPBearer`` security scheme, which + is a FastAPI dependency and so only resolves once routing has picked a handler — after + this middleware has already run. + """ + header = request.headers.get("authorization") + if header is None: + return None + + scheme, _, token = header.partition(" ") + if scheme.lower() != BEARER_SCHEME or not token: + return None + + return decode_token_username(token) + + class PluginAccessMiddleware(BaseHTTPMiddleware): + """Reject requests to plugins the caller has turned off, before they reach the route. + + Runs ahead of routing, so it resolves both facts it needs itself: which plugin owns the + requested URL (from the name ``register_router`` stamps on plugin endpoints) and who is + asking (from the request's bearer token, via the helpers in ``sparkth.lib.auth`` that + ``get_current_user`` is built from). + + Anonymous requests fail open. Plugin routers carry unauthenticated endpoints — Slack's + OAuth callback is called by Slack itself, with no token — and a per-user preference is + meaningless without a user; endpoints that do require a caller are still rejected by + their own auth dependency. + """ + def __init__(self, app: Any, exclude_paths: list[str] | None = None) -> None: super().__init__(app) + # Filled on the first request rather than here: ``app`` is the next ASGI app in the + # chain, not the FastAPI instance holding the routes, and ``assemble_app`` adds this + # middleware before it registers the plugin routers anyway. See _flattened_routes. + self._route_contexts: list[RouteContext] | None = None + # Entries are matched with startswith, so every one of them must be a real path + # prefix: a bare "/" would exclude every path there is and leave the gate + # enforcing nothing. self.exclude_paths = exclude_paths or [ "/docs", "/redoc", "/openapi.json", - "/", "/api/v1/auth", ] @@ -37,15 +78,20 @@ async def dispatch(self, request: Request, call_next: Callable[[Request], Awaita response = await call_next(request) return response - user = getattr(request.state, "user", None) - if not user: + username = _bearer_token_username(request) + if username is None: response = await call_next(request) return response - has_access = await self._check_plugin_access(user.id, plugin_name) + user, has_access = await self._resolve_caller_access(username, plugin_name) + if user is not None: + # Hand the route the user already loaded here: get_current_user reuses it rather + # than decoding the same token and re-reading the same row a second time. + request.state.user = user + if not has_access: logger.warning( - f"User {user.id} attempted to access disabled plugin '{plugin_name}' at path {request.url.path}" + f"User '{username}' attempted to access disabled plugin '{plugin_name}' at path {request.url.path}" ) return JSONResponse( status_code=status.HTTP_403_FORBIDDEN, @@ -65,20 +111,68 @@ def _is_excluded_path(self, path: str) -> bool: return False def _get_route_plugin_name(self, request: Request) -> str | None: - for route in request.app.routes: - match, _ = route.matches(request.scope) + """Name of the plugin owning the route this request targets, or None for a core route. + + Since FastAPI 0.137 include_router() no longer copies the sub-routes into + app.routes: it appends a single lazy _IncludedRouter branch, which has no .endpoint + carrying the plugin name. iter_route_contexts flattens those branches into contexts + that match on the *prefixed* path — the underlying route's own .path is unprefixed + and would never match the request — while original_route is the real route whose + endpoint holds the stamp. + """ + for context in self._flattened_routes(request.app): + match, _ = context.matches(request.scope) if match == Match.FULL: - return get_route_plugin_name(route) + return get_route_plugin_name(context.original_route) return None - async def _check_plugin_access(self, user_id: int, plugin_name: str) -> bool: + def _flattened_routes(self, app: FastAPI) -> list[RouteContext]: + """The app's route table, flattened on first use and kept. + + The table is fixed once ``assemble_app`` returns — routes are registered at import + time, never per request — so flattening it again on every request is repeated work + on a path every request takes. The matching itself still runs per request: request + paths carry parameters (``/api/v1/canvas/courses/{course_id}``), so there is no + path-keyed answer to cache, only the flattened list to match against. + + Args: + app: The FastAPI instance serving the request, taken from its scope. + + Returns: + Every route on the app, with nested includes flattened into their own contexts. + """ + if self._route_contexts is None: + self._route_contexts = list(iter_route_contexts(app.routes)) + return self._route_contexts + + async def _resolve_caller_access(self, username: str, plugin_name: str) -> tuple[User | None, bool]: + """The user this token names, and whether they may still use the plugin. + + Both answers come out of one session because the caller needs both and the route + needs the user again: returning it lets ``get_current_user`` skip a second lookup. + + A token naming a user that no longer exists passes: there is no preference to + enforce, and the route's own auth dependency rejects the request anyway. A database + failure blocks — the gate cannot confirm access, so it must not grant it. + + Args: + username: The username the request's bearer token names. + plugin_name: The plugin owning the route being requested. + + Returns: + The user, or None when the token names nobody or the lookup failed, paired with + whether the request may proceed. + """ try: - async for session in get_async_session(): - return await _check_plugin_access_async(user_id, plugin_name, session, check_system_enabled=True) - return False # Fallback if session generator doesn't yield + async with session_scope() as session: + user = await get_user_by_username(username, session) + if user is None or user.id is None: + return None, True + allowed = await _check_plugin_access_async(user.id, plugin_name, session, check_system_enabled=True) + return user, allowed except (DatabaseError, OperationalError) as e: - logger.error(f"Database error checking plugin access for user {user_id} and plugin '{plugin_name}': {e}") - return False + logger.error(f"Database error checking plugin access for user '{username}' and plugin '{plugin_name}': {e}") + return None, False async def _check_plugin_access_async( @@ -87,6 +181,15 @@ async def _check_plugin_access_async( """ Shared async logic for checking plugin access. + Reads the system switch and the user's own preference in one query. The join is a + LEFT OUTER one, and it is keyed on the user as well as the plugin: an inner join would + hide a plugin nobody has expressed a preference on, and a join keyed on the plugin + alone would let one user's disabled plugin answer for every other user. + + Both "no plugin row" and "no preference row" mean access is allowed. A plugin the + registry has never seen is not a disabled plugin, and a user who has never expressed a + preference has not opted out. + Args: user_id: The user ID to check access for plugin_name: The name of the plugin @@ -96,34 +199,35 @@ async def _check_plugin_access_async( Returns: bool: True if user has access, False otherwise """ - plugin_statement = select(Plugin).where( - Plugin.name == plugin_name, - Plugin.deleted_at == None, + # cast: SQLModel types a column comparison as bool, so the composed ON clause does not + # satisfy outerjoin's signature without it — the same reason joins elsewhere cast. + on_clause = cast( + Any, + (UserPlugin.plugin_id == Plugin.id) & (UserPlugin.user_id == user_id) & (UserPlugin.deleted_at == None), + ) + statement = ( + select(Plugin.enabled, UserPlugin.enabled) + .outerjoin(UserPlugin, on_clause) + .where(Plugin.name == plugin_name, Plugin.deleted_at == None) ) - result = await session.exec(plugin_statement) - plugin = result.one_or_none() + result = await session.exec(statement) + row = result.one_or_none() - if plugin is None: + if row is None: logger.debug(f"Plugin '{plugin_name}' not found in database. Allowing access by default.") return True - if check_system_enabled and not plugin.enabled: + system_enabled, user_enabled = row + + if check_system_enabled and not system_enabled: logger.debug(f"Plugin '{plugin_name}' is disabled at system level") return False - statement = select(UserPlugin).where( - UserPlugin.user_id == user_id, - UserPlugin.plugin_id == plugin.id, - UserPlugin.deleted_at == None, - ) - user_plugin_result = await session.exec(statement) - user_plugin = user_plugin_result.one_or_none() - - if user_plugin is None: + if user_enabled is None: logger.debug(f"No UserPlugin record for user {user_id} and plugin '{plugin_name}'. Allowing access by default.") return True - return bool(user_plugin.enabled) + return bool(user_enabled) def _check_plugin_access(user_id: int, plugin_name: str, session: Session, check_system_enabled: bool = False) -> bool: diff --git a/sparkth/core/routes/__init__.py b/sparkth/core/routes/__init__.py index ea6bceaa..1b401873 100644 --- a/sparkth/core/routes/__init__.py +++ b/sparkth/core/routes/__init__.py @@ -1,6 +1,7 @@ from typing import cast from fastapi import APIRouter +from fastapi.routing import iter_route_contexts from starlette.routing import BaseRoute from sparkth.lib.plugins import SparkthPlugin @@ -24,10 +25,17 @@ def register_router(plugin: SparkthPlugin, router: APIRouter) -> None: tags=[f"plugin:{plugin.name}", plugin.name], ) - # Associate each route to the plugin - for route in prefixed_router.routes: - if hasattr(route, "endpoint"): - setattr(route.endpoint, PLUGIN_NAME_ATTRIBUTE, plugin.name) + # Associate each route to the plugin. + # + # Since FastAPI 0.137 include_router() no longer copies the sub-routes into + # prefixed_router.routes: it appends a single lazy _IncludedRouter branch, which has no + # .endpoint to stamp. iter_route_contexts (added in 0.138, hence that dependency floor) + # flattens those branches back into the underlying routes, so the attribute lands on the + # endpoints the app actually serves. + for context in iter_route_contexts(prefixed_router.routes): + endpoint = getattr(context.original_route, "endpoint", None) + if endpoint is not None: + setattr(endpoint, PLUGIN_NAME_ATTRIBUTE, plugin.name) hooks.PLUGIN_ROUTERS.add_item(plugin, prefixed_router) diff --git a/sparkth/lib/auth.py b/sparkth/lib/auth.py index bc383bfa..f4691d15 100644 --- a/sparkth/lib/auth.py +++ b/sparkth/lib/auth.py @@ -1,12 +1,20 @@ -"""Authentication dependency for resolving the current user from a bearer token. +"""Bearer-token authentication: the token-reading helpers and the current-user dependency. The single canonical home for ``get_current_user``: every caller (routes, the permission gate, plugins, and the test harness) imports it from here. It is deliberately not re-exported from ``sparkth.api.v1.auth`` — one object keeps FastAPI dependency overrides working. + +``get_current_user`` is a FastAPI dependency, so it is only available to code that runs +*inside* a route. Code that must identify the caller earlier — ``PluginAccessMiddleware`` +runs before routing, so no dependency has resolved yet — composes the same two helpers this +module builds the dependency from, :func:`decode_token_username` and +:func:`get_user_by_username`, rather than decoding tokens or querying users of its own. One +implementation of "who is this request from" keeps the security gate from drifting away from +the dependency as tokens or user lookup change. """ import jwt -from fastapi import Depends, HTTPException, status +from fastapi import Depends, HTTPException, Request, status from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from sqlmodel import select from sqlmodel.ext.asyncio.session import AsyncSession @@ -14,34 +22,75 @@ from sparkth.core import security from sparkth.core.models.user import User from sparkth.lib.db import get_async_session +from sparkth.lib.log import get_logger + +logger = get_logger(__name__) security_scheme = HTTPBearer() +def decode_token_username(token: str) -> str | None: + """Return the username a bearer token identifies, or ``None`` if it identifies nobody. + + ``None`` covers every way a token can fail to name a user — malformed, signed with the + wrong key, expired, or carrying no ``sub`` claim — because callers treat them alike: + an unreadable token is an unauthenticated request. Callers decide what that means; + this helper never raises. + + Args: + token: The raw JWT from the ``Authorization`` header, without the ``Bearer`` prefix. + """ + try: + payload = security.decode_access_token(token) + except jwt.InvalidTokenError as e: + # Debug, not warning: expired tokens are routine (every session eventually reaches + # this path), so logging louder would bury real failures in noise. + logger.debug(f"Rejected an unreadable access token: {e}") + return None + + username = payload.get("sub") + if not isinstance(username, str) or not username: + return None + return username + + +async def get_user_by_username(username: str, session: AsyncSession) -> User | None: + """Return the user with this username, or ``None`` when no user has it.""" + result = await session.exec(select(User).where(User.username == username)) + return result.one_or_none() + + async def get_current_user( + request: Request, credentials: HTTPAuthorizationCredentials = Depends(security_scheme), session: AsyncSession = Depends(get_async_session), ) -> User: - token = credentials.credentials + """Resolve the authenticated user, rejecting the request when the token names no one. - try: - payload = security.decode_access_token(token) - username = payload.get("sub") - if not isinstance(username, str) or username is None: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Could not validate credentials", - headers={"WWW-Authenticate": "Bearer"}, - ) - except jwt.InvalidTokenError: + Reuses the user ``PluginAccessMiddleware`` left on ``request.state`` when it has already + resolved this request's caller, rather than decoding the same token and re-reading the + same row. Only that gate writes the attribute, and only from this request's own token, + so the answer is the one this dependency would have computed. Requests it never + identified — core routes, which it does not gate — fall through to the full lookup. + + The reused instance is detached: the gate's session has closed by the time the route + runs. That is safe because ``User`` maps only columns, which stay readable on a detached + instance; a test in ``tests/core/plugins/test_middleware.py`` pins that, since adding a + relationship to ``User`` is what would make this unsafe. + """ + cached_user = getattr(request.state, "user", None) + if isinstance(cached_user, User): + return cached_user + + username = decode_token_username(credentials.credentials) + if username is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials", headers={"WWW-Authenticate": "Bearer"}, ) - result = await session.exec(select(User).where(User.username == username)) - user = result.one_or_none() + user = await get_user_by_username(username, session) if user is None: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, diff --git a/tests/core/plugins/test_middleware.py b/tests/core/plugins/test_middleware.py new file mode 100644 index 00000000..0a540b44 --- /dev/null +++ b/tests/core/plugins/test_middleware.py @@ -0,0 +1,405 @@ +"""Behavioural tests for the per-user plugin access gate (PluginAccessMiddleware). + +The gate answers one question before a request reaches its route: does the caller still +have this plugin turned on? Each test drives a real request through the assembled app so +the whole chain is exercised — resolving which plugin owns the URL, resolving the caller +from the bearer token, and the access lookup itself. +""" + +import re +from collections.abc import Iterator, Sequence +from contextlib import contextmanager +from typing import Any, cast + +import pytest +from fastapi import FastAPI, Request +from fastapi.routing import RouteContext, iter_route_contexts +from httpx import AsyncClient +from sqlalchemy import event, inspect +from sqlalchemy.exc import OperationalError +from sqlmodel.ext.asyncio.session import AsyncSession +from starlette.routing import BaseRoute + +from sparkth.core.db import get_engine +from sparkth.core.models.plugin import Plugin, UserPlugin +from sparkth.core.models.user import User +from sparkth.core.plugins.middleware import PluginAccessMiddleware +from sparkth.core.security import create_access_token +from sparkth.main import assemble_app + +CHAT_COMPLETIONS_PATH = "/api/v1/chat/completions" +SLACK_CALLBACK_PATH = "/api/v1/slack/oauth/callback" + + +class _CountingFlattener: + """Stands in for ``iter_route_contexts``, recording how often the table is flattened.""" + + def __init__(self) -> None: + self.calls = 0 + + def __call__(self, routes: Sequence[BaseRoute]) -> Iterator[RouteContext]: + self.calls += 1 + return iter_route_contexts(routes) + + +# The user table, quoted or not, but never the user_plugins table beside it. +_USER_SELECT = re.compile(r'^select\b.*\bfrom\s+"?user"?(\s|$)') + + +class _UserSelectCounter: + """Counts the SELECTs issued against the user table on the shared engine.""" + + def __init__(self) -> None: + self.count = 0 + + def __call__(self, conn: Any, cursor: Any, statement: str, *args: Any) -> None: + if _USER_SELECT.match(" ".join(statement.split()).lower()): + self.count += 1 + + +@contextmanager +def _counting_user_selects() -> Iterator[_UserSelectCounter]: + """Count user-table reads for the duration of the block.""" + counter = _UserSelectCounter() + engine = get_engine().sync_engine + event.listen(engine, "before_cursor_execute", counter) + try: + yield counter + finally: + event.remove(engine, "before_cursor_execute", counter) + + +def _request(app: FastAPI, method: str, path: str) -> Request: + return Request( + { + "type": "http", + "method": method, + "path": path, + "root_path": "", + "headers": [], + "query_string": b"", + "app": app, + } + ) + + +def _auth_headers(username: str) -> dict[str, str]: + return {"Authorization": f"Bearer {create_access_token({'sub': username})}"} + + +async def _seed_user(session: AsyncSession, username: str) -> User: + user = User( + name="Gate Test", + username=username, + email=f"{username}@example.com", + hashed_password="not-a-real-hash", + ) + session.add(user) + await session.commit() + await session.refresh(user) + return user + + +async def _seed_plugin(session: AsyncSession, name: str, enabled: bool) -> Plugin: + plugin = Plugin(name=name, enabled=enabled) + session.add(plugin) + await session.commit() + await session.refresh(plugin) + return plugin + + +async def _raise_operational_error(username: str, session: AsyncSession) -> User | None: + """Stand in for the user lookup when the database is unreachable.""" + raise OperationalError("SELECT 1", {}, Exception("connection lost")) + + +async def _seed_user_plugin(session: AsyncSession, user: User, plugin: Plugin, enabled: bool) -> None: + session.add(UserPlugin(user_id=cast(int, user.id), plugin_id=cast(int, plugin.id), enabled=enabled)) + await session.commit() + + +class TestRoutePluginResolution: + """Which plugin owns the requested URL.""" + + @pytest.mark.parametrize( + "method, path, plugin_name", + [ + ("POST", CHAT_COMPLETIONS_PATH, "chat"), + ("GET", "/api/v1/slack/oauth/status", "slack"), + ("GET", "/api/v1/google-drive/oauth/status", "google-drive"), + ], + ) + def test_resolves_the_plugin_that_owns_a_plugin_route(self, method: str, path: str, plugin_name: str) -> None: + # Every plugin that mounts routes, not just one: these routers nest their includes to + # different depths, and a plugin the gate cannot name is a plugin it cannot police. + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, method, path)) == plugin_name + + def test_returns_none_for_a_core_route(self) -> None: + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, "GET", "/api/v1/user/me")) is None + + def test_returns_none_for_an_unknown_path(self) -> None: + app = assemble_app() + middleware = PluginAccessMiddleware(app) + + assert middleware._get_route_plugin_name(_request(app, "GET", "/api/v1/nothing-here")) is None + + def test_flattens_the_route_table_once_and_reuses_it(self, monkeypatch: pytest.MonkeyPatch) -> None: + """The table is fixed once the app is assembled, so flattening it per request is + repeated work on a path every request takes. Resolution must stay correct across + both requests — a cache that answers the second one wrongly is worse than no cache.""" + app = assemble_app() + middleware = PluginAccessMiddleware(app) + flattener = _CountingFlattener() + monkeypatch.setattr("sparkth.core.plugins.middleware.iter_route_contexts", flattener) + + first = middleware._get_route_plugin_name(_request(app, "POST", CHAT_COMPLETIONS_PATH)) + second = middleware._get_route_plugin_name(_request(app, "GET", "/api/v1/slack/oauth/status")) + + assert (first, second) == ("chat", "slack") + assert flattener.calls == 1 + + +class TestExcludedPaths: + """Which paths the gate declines to police.""" + + def test_does_not_exclude_plugin_paths_by_default(self) -> None: + # exclude_paths is matched with startswith, so a "/" entry would exclude every path + # there is and leave the gate enforcing nothing at all. + middleware = PluginAccessMiddleware(assemble_app()) + + assert middleware._is_excluded_path(CHAT_COMPLETIONS_PATH) is False + + def test_excludes_the_configured_paths(self) -> None: + middleware = PluginAccessMiddleware(assemble_app(), ["/api/v1/auth"]) + + assert middleware._is_excluded_path("/api/v1/auth/login") is True + + +class TestPluginAccessGate: + """What the gate does with a real request.""" + + async def test_blocks_a_plugin_the_user_disabled(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "disabled-chat-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_blocks_a_plugin_disabled_system_wide(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "system-disabled-user") + plugin = await _seed_plugin(session, "chat", False) + await _seed_user_plugin(session, user, plugin, True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_lets_through_a_plugin_the_user_enabled(self, client: AsyncClient, session: AsyncSession) -> None: + # The inverse of the block: without it, a gate that rejects everything would pass the + # tests above while breaking every plugin endpoint in the product. + user = await _seed_user(session, "enabled-chat-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code != 403 + + async def test_lets_through_a_plugin_with_no_user_preference( + self, client: AsyncClient, session: AsyncSession + ) -> None: + user = await _seed_user(session, "no-preference-user") + await _seed_plugin(session, "chat", True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code != 403 + + async def test_does_not_gate_core_routes(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "core-route-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.get("/api/v1/user/me", headers=_auth_headers(user.username)) + + assert response.status_code == 200 + + async def test_does_not_gate_anonymous_requests(self, client: AsyncClient, session: AsyncSession) -> None: + # Unauthenticated plugin endpoints exist — Slack's OAuth callback is called by Slack + # itself, with no bearer token. Per-user access is meaningless without a user, so the + # gate must fail open rather than block the callback. + user = await _seed_user(session, "slack-callback-user") + plugin = await _seed_plugin(session, "slack", True) + await _seed_user_plugin(session, user, plugin, False) + + response = await client.get(SLACK_CALLBACK_PATH) + + assert response.status_code == 422 + + async def test_lets_through_a_token_naming_a_user_that_does_not_exist( + self, client: AsyncClient, session: AsyncSession + ) -> None: + # Nobody to hold a preference, so there is nothing to enforce. The route's own auth + # dependency is what rejects the request. + await _seed_plugin(session, "chat", True) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers("no-such-user"), + ) + + assert response.status_code != 403 + assert response.json()["detail"] == "User not found" + + async def test_blocks_a_plugin_disabled_system_wide_for_a_user_with_no_preference( + self, client: AsyncClient, session: AsyncSession + ) -> None: + """The system switch does not depend on the caller having a preference row, and most + callers have none. Resolving the switch and the preference together must keep the + plugin's own row even when nothing joins to it — otherwise the administrative + control silently stops applying to exactly the users who never opened their + settings.""" + user = await _seed_user(session, "no-preference-system-disabled") + await _seed_plugin(session, "chat", False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_lets_through_a_plugin_only_another_user_disabled( + self, client: AsyncClient, session: AsyncSession + ) -> None: + """The preference read must be the caller's own. Resolving the plugin and the + preference together means keying that lookup on the user as well — miss it, and one + user turning a plugin off would turn it off for everybody.""" + caller = await _seed_user(session, "unaffected-caller") + other = await _seed_user(session, "opted-out-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, other, plugin, False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(caller.username), + ) + + assert response.status_code != 403 + + async def test_lets_through_a_plugin_the_registry_has_never_seen( + self, client: AsyncClient, session: AsyncSession + ) -> None: + """No row is not the same as a disabled row: a plugin absent from the registry + stays reachable rather than being treated as switched off.""" + user = await _seed_user(session, "unregistered-plugin-user") + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code != 403 + + async def test_blocks_when_the_access_lookup_fails( + self, client: AsyncClient, session: AsyncSession, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The gate fails closed: a lookup it could not complete is not permission to proceed. + user = await _seed_user(session, "db-error-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, True) + monkeypatch.setattr("sparkth.core.plugins.middleware.get_user_by_username", _raise_operational_error) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert response.status_code == 403 + assert "chat" in response.json()["detail"] + + async def test_lets_through_a_request_carrying_an_invalid_token( + self, client: AsyncClient, session: AsyncSession + ) -> None: + # An unreadable token identifies nobody, so there is no per-user preference to enforce. + # The route's own auth dependency still rejects the request. + plugin = await _seed_plugin(session, "chat", True) + user = await _seed_user(session, "invalid-token-user") + await _seed_user_plugin(session, user, plugin, False) + + response = await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers={"Authorization": "Bearer not-a-real-token"}, + ) + + assert response.json()["detail"] == "Could not validate credentials" + + +class TestCallerIsResolvedOnce: + """The gate and ``get_current_user`` answer the same question about the same request, + so the second one reuses the first one's answer instead of repeating the work.""" + + async def test_a_gated_route_reads_the_user_once(self, client: AsyncClient, session: AsyncSession) -> None: + user = await _seed_user(session, "reuse-user") + plugin = await _seed_plugin(session, "chat", True) + await _seed_user_plugin(session, user, plugin, True) + + with _counting_user_selects() as counter: + await client.post( + CHAT_COMPLETIONS_PATH, + json={"messages": []}, + headers=_auth_headers(user.username), + ) + + assert counter.count == 1 + + async def test_an_ungated_route_still_reads_the_user(self, client: AsyncClient, session: AsyncSession) -> None: + """A core route never reaches the gate, so nothing has been loaded for the + dependency to reuse and it must still resolve the caller itself.""" + user = await _seed_user(session, "core-route-user") + + with _counting_user_selects() as counter: + response = await client.get("/api/v1/user/me", headers=_auth_headers(user.username)) + + assert response.status_code == 200 + assert counter.count == 1 + + def test_the_user_model_carries_no_relationships(self) -> None: + """What makes handing the loaded user to the route safe: the gate's session has + closed by then, so the instance the route receives is detached. Detached is + harmless for plain columns and raises on a relationship — adding one to ``User`` + would make this reuse unsafe, so the guard fails loudly rather than at runtime.""" + assert list(inspect(User).relationships) == [] diff --git a/tests/core/test_assemble_app.py b/tests/core/test_assemble_app.py index b7d940da..cd192c2a 100644 --- a/tests/core/test_assemble_app.py +++ b/tests/core/test_assemble_app.py @@ -21,7 +21,7 @@ def _route_paths(application: FastAPI) -> set[str]: """Every APIRoute path, prefixes applied. - Since FastAPI 0.140 include_router() no longer copies the sub-routes into + Since FastAPI 0.137 include_router() no longer copies the sub-routes into application.routes: it appends a single lazy _IncludedRouter branch instead. iter_route_contexts flattens those branches back into per-route contexts whose .path carries the include prefix. diff --git a/tests/core/test_routes.py b/tests/core/test_routes.py new file mode 100644 index 00000000..4d3aec12 --- /dev/null +++ b/tests/core/test_routes.py @@ -0,0 +1,53 @@ +"""Tests for plugin route registration in ``sparkth.core.routes``.""" + +from typing import Awaitable, Callable + +from fastapi import APIRouter +from fastapi.routing import iter_route_contexts + +from sparkth.core.routes import get_route_plugin_name, register_router +from sparkth.core.routes.hooks import PLUGIN_ROUTERS +from sparkth.lib.plugins import SparkthPlugin + + +async def _ping() -> dict[str, str]: + """Endpoint stood up purely so the router has a route to stamp.""" + return {"status": "ok"} + + +async def _pong() -> dict[str, str]: + """Second endpoint so each test stamps its own function object.""" + return {"status": "ok"} + + +def _plugin_router(endpoint: Callable[[], Awaitable[dict[str, str]]]) -> APIRouter: + router = APIRouter() + router.add_api_route("/ping", endpoint, methods=["GET"]) + return router + + +def _registered_router(plugin: SparkthPlugin) -> APIRouter: + routers = {registered: router for registered, router in PLUGIN_ROUTERS.iter_items()} + return routers[plugin] + + +def test_register_router_stamps_the_plugin_name_on_its_routes() -> None: + # The stamp is what PluginAccessMiddleware reads to decide which plugin owns a URL, so a + # router whose routes carry no plugin name leaves the per-user access gate with nothing + # to enforce. iter_route_contexts flattens the lazy _IncludedRouter branch include_router + # appends (FastAPI 0.137+) back into the underlying routes. + plugin = SparkthPlugin("stamp-test") + register_router(plugin, _plugin_router(_ping)) + + contexts = list(iter_route_contexts(_registered_router(plugin).routes)) + + assert [get_route_plugin_name(context.original_route) for context in contexts] == ["stamp-test"] + + +def test_register_router_prefixes_routes_with_the_plugin_namespace() -> None: + plugin = SparkthPlugin("prefix-test") + register_router(plugin, _plugin_router(_pong)) + + paths = [context.path for context in iter_route_contexts(_registered_router(plugin).routes)] + + assert paths == ["/api/v1/prefix-test/ping"] diff --git a/tests/lib/test_auth.py b/tests/lib/test_auth.py index d12be06d..a5d99d03 100644 --- a/tests/lib/test_auth.py +++ b/tests/lib/test_auth.py @@ -1,5 +1,24 @@ +from datetime import timedelta + +from sqlmodel.ext.asyncio.session import AsyncSession + import sparkth.api.v1.auth as api_auth import sparkth.lib.auth as lib_auth +from sparkth.core.models.user import User +from sparkth.core.security import create_access_token + + +async def _seed_user(session: AsyncSession, username: str) -> User: + user = User( + name="Auth Test", + username=username, + email=f"{username}@example.com", + hashed_password="not-a-real-hash", + ) + session.add(user) + await session.commit() + await session.refresh(user) + return user def test_get_current_user_lives_in_lib_auth() -> None: @@ -12,3 +31,41 @@ def test_get_current_user_not_reexported_from_api_auth() -> None: # so they all share one object. sparkth.api.v1.auth must NOT re-export it — a compat shim would # split the canonical location and let a dependency_overrides key silently miss. assert not hasattr(api_auth, "get_current_user") + + +class TestDecodeTokenUsername: + """Reading the subject out of a bearer token. + + Shared by get_current_user and PluginAccessMiddleware so both read tokens the same way. + """ + + def test_returns_the_token_subject(self) -> None: + token = create_access_token({"sub": "tokenuser"}) + + assert lib_auth.decode_token_username(token) == "tokenuser" + + def test_returns_none_for_a_malformed_token(self) -> None: + assert lib_auth.decode_token_username("not-a-real-token") is None + + def test_returns_none_for_an_expired_token(self) -> None: + token = create_access_token({"sub": "tokenuser"}, expires_delta=timedelta(minutes=-1)) + + assert lib_auth.decode_token_username(token) is None + + def test_returns_none_when_the_token_carries_no_subject(self) -> None: + assert lib_auth.decode_token_username(create_access_token({})) is None + + +class TestGetUserByUsername: + """Looking a user up by username.""" + + async def test_returns_the_matching_user(self, session: AsyncSession) -> None: + await _seed_user(session, "lookupuser") + + found = await lib_auth.get_user_by_username("lookupuser", session) + + assert found is not None + assert found.username == "lookupuser" + + async def test_returns_none_when_no_user_matches(self, session: AsyncSession) -> None: + assert await lib_auth.get_user_by_username("nobody-here", session) is None diff --git a/uv.lock b/uv.lock index 8e42b891..ad67cd1f 100644 --- a/uv.lock +++ b/uv.lock @@ -3063,7 +3063,7 @@ requires-dist = [ { name = "authlib", specifier = ">=1.3.0" }, { name = "beautifulsoup4", specifier = ">=4.0" }, { name = "cryptography", specifier = ">=46.0.3" }, - { name = "fastapi", extras = ["standard"], specifier = ">=0.121.2" }, + { name = "fastapi", extras = ["standard"], specifier = ">=0.138.0" }, { name = "fastmcp", specifier = ">=2.13.1" }, { name = "google-api-core", specifier = ">=2.10.0" }, { name = "greenlet", specifier = ">=3.2.4" },