From abc6cf79edadf86782472c34ad55acf39c23d307 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 26 Oct 2026 20:27:03 +1100 Subject: [PATCH] feat(auth): enforce the role matrix on every data route (M2c, #94) PERMISSIONS in api/auth.py maps each action to the roles allowed it; allow(action) returns 403 for any other signed-in role. docs/ROLES.md shows the same table and tests/test_roles.py fails if they drift, then tries every role against every route. Requests without a session keep the shared-key access. Isolation tests now use tenant admins, so they check isolation at the highest privilege. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019YPoXzBSj89ZHgw7nfTmbB --- api/auth.py | 28 ++++++++ api/routers/audit_routes.py | 4 +- api/routers/csv_routes.py | 12 ++-- api/routers/te_routes.py | 4 +- docs/ROLES.md | 20 ++++++ scripts/lint.sh | 1 + tests/test_roles.py | 128 +++++++++++++++++++++++++++++++++ tests/test_tenant_isolation.py | 4 +- 8 files changed, 189 insertions(+), 12 deletions(-) create mode 100644 docs/ROLES.md create mode 100644 tests/test_roles.py diff --git a/api/auth.py b/api/auth.py index 489909f..90a3001 100644 --- a/api/auth.py +++ b/api/auth.py @@ -73,3 +73,31 @@ async def tenant_scope(authorization: str = Header(default=""), session = await run_in_threadpool(_live_session, bearer_token(authorization)) with scoped(schema_for(session["tenant"])): yield session + + +# M2c (#94): which roles may perform each action. docs/ROLES.md shows the same +# table, and tests/test_roles.py checks that the two agree. +PERMISSIONS: dict[str, frozenset[str]] = { + "preview": frozenset({"uploader", "tenant_admin"}), + "upload": frozenset({"uploader", "tenant_admin"}), + "list_files": frozenset({"uploader", "reviewer", "tenant_admin", "auditor"}), + "read_rows": frozenset({"uploader", "reviewer", "tenant_admin", "auditor"}), + "list_te": frozenset({"uploader", "reviewer", "tenant_admin", "auditor"}), + "delete_file": frozenset({"tenant_admin"}), + "read_audit": frozenset({"tenant_admin", "auditor"}), +} + + +def allow(action: str): + """Route dependency: 403 unless the signed-in role may perform ``action``. + + Requests without a session (API key or dev) are not role-checked; they keep + today's single-operator access until the shared key is retired. + """ + roles = PERMISSIONS[action] + + def check(session: dict | None = Depends(tenant_scope)) -> None: + if session is not None and session["role"] not in roles: + raise HTTPException(status_code=403, + detail=f"Role '{session['role']}' may not {action.replace('_', ' ')}") + return check diff --git a/api/routers/audit_routes.py b/api/routers/audit_routes.py index e00acde..3650304 100644 --- a/api/routers/audit_routes.py +++ b/api/routers/audit_routes.py @@ -12,7 +12,7 @@ from fastapi import APIRouter, Depends from psycopg2 import sql -from api.auth import tenant_scope +from api.auth import allow, tenant_scope from api.tenancy import uploads_schema from api.db import Conn @@ -23,7 +23,7 @@ ) -@router.get("/log") +@router.get("/log", dependencies=[Depends(allow("read_audit"))]) def audit_log(limit: int = 50, offset: int = 0) -> dict: limit = max(1, min(limit, 200)) offset = max(0, offset) diff --git a/api/routers/csv_routes.py b/api/routers/csv_routes.py index 8498a22..377f633 100644 --- a/api/routers/csv_routes.py +++ b/api/routers/csv_routes.py @@ -8,7 +8,7 @@ from psycopg2 import sql from pydantic import BaseModel, Field -from api.auth import tenant_scope +from api.auth import allow, tenant_scope from api.config import settings from api.tenancy import current_schema, uploads_schema from api.db import Conn @@ -42,7 +42,7 @@ class UploadRequest(BaseModel): targetTable: str | None = None # required when mode == "te" -@router.post("/preview") +@router.post("/preview", dependencies=[Depends(allow("preview"))]) def preview(req: PreviewRequest) -> dict: if len(req.content.encode("utf-8")) > settings.MAX_UPLOAD_BYTES: raise HTTPException(413, "File too large") @@ -67,7 +67,7 @@ def preview(req: PreviewRequest) -> dict: return result -@router.post("/upload") +@router.post("/upload", dependencies=[Depends(allow("upload"))]) def upload(req: UploadRequest) -> dict: if len(req.content.encode("utf-8")) > settings.MAX_UPLOAD_BYTES: raise HTTPException(413, "File too large") @@ -98,7 +98,7 @@ def upload(req: UploadRequest) -> dict: return result -@router.get("/files") +@router.get("/files", dependencies=[Depends(allow("list_files"))]) def list_files() -> list[dict]: with Conn() as conn: with conn.cursor() as cur: @@ -126,7 +126,7 @@ def list_files() -> list[dict]: ] -@router.get("/tables/{table_name}/rows") +@router.get("/tables/{table_name}/rows", dependencies=[Depends(allow("read_rows"))]) def table_rows(table_name: str, limit: int = 50) -> dict: if not table_name.startswith("csv_") or len(table_name) > 64: raise HTTPException(422, "Invalid table name") @@ -158,7 +158,7 @@ def table_rows(table_name: str, limit: int = 50) -> dict: return {"rows": rows} -@router.delete("/files/{file_id}") +@router.delete("/files/{file_id}", dependencies=[Depends(allow("delete_file"))]) def delete_file(file_id: int) -> dict: if not settings.allow_destructive: raise HTTPException( diff --git a/api/routers/te_routes.py b/api/routers/te_routes.py index edbd0af..dab6bce 100644 --- a/api/routers/te_routes.py +++ b/api/routers/te_routes.py @@ -2,7 +2,7 @@ from fastapi import APIRouter, Depends -from api.auth import tenant_scope +from api.auth import allow, tenant_scope from api.config import TE_TABLES from api.tenancy import te_schema from api.db import Conn @@ -15,7 +15,7 @@ ) -@router.get("/tables") +@router.get("/tables", dependencies=[Depends(allow("list_te"))]) def te_tables() -> list[dict]: # BUG-042: this used to run 1-2 queries per table (24 for the 12-table # whitelist) — a COUNT(*) existence check plus a full COUNT(*) scan. diff --git a/docs/ROLES.md b/docs/ROLES.md new file mode 100644 index 0000000..2ee8c21 --- /dev/null +++ b/docs/ROLES.md @@ -0,0 +1,20 @@ +# Roles and permissions (BR-36, #94) + +Every member of a tenant has one role (ADR-0003). This table is enforced by `PERMISSIONS` in `api/auth.py`. `tests/test_roles.py` fails if the two disagree, and it tries every role against every route. + +| Action | Route | uploader | reviewer | tenant_admin | auditor | +|---|---|---|---|---|---| +| preview | `POST /api/csv/preview` | ✓ | | ✓ | | +| upload | `POST /api/csv/upload` | ✓ | | ✓ | | +| list_files | `GET /api/csv/files` | ✓ | ✓ | ✓ | ✓ | +| read_rows | `GET /api/csv/tables/{table}/rows` | ✓ | ✓ | ✓ | ✓ | +| list_te | `GET /api/te/tables` | ✓ | ✓ | ✓ | ✓ | +| delete_file | `DELETE /api/csv/files/{id}` | | | ✓ | | +| read_audit | `GET /api/audit/log` | | | ✓ | ✓ | + +A role that isn't allowed gets **403**. Two kinds of request are outside this table: + +- **Sign-in routes** (`/api/auth/*`) are open to anyone, so people can sign in. +- **Requests with no session** use the shared API key, or nothing in `dev`. They keep full access until the shared key is retired. + +Approving and editing data arrive with M4 and M6. The reviewer gets those permissions then. diff --git a/scripts/lint.sh b/scripts/lint.sh index e533339..1e995fe 100644 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -38,6 +38,7 @@ PY_TESTS=( tests/test_import_counts.py tests/test_accounts.py tests/test_tenant_isolation.py + tests/test_roles.py ) # Collect scripts/*.py dynamically diff --git a/tests/test_roles.py b/tests/test_roles.py new file mode 100644 index 0000000..fb60dd7 --- /dev/null +++ b/tests/test_roles.py @@ -0,0 +1,128 @@ +"""Role-based authorisation matrix (M2c, #94, BR-36). + +The unit test keeps docs/ROLES.md and api/auth.PERMISSIONS in step. The +integration test tries every role against every protected route. +""" + +from __future__ import annotations + +import os +import unittest +import uuid +from pathlib import Path + +import pytest +from fastapi.testclient import TestClient +from psycopg2 import sql + +from api.auth import PERMISSIONS +from api.config import settings +from api.db import Conn +from api.main import app +from api.services import accounts +from api.services.accounts import ROLES +from api.tenancy import migrate_tenant, schema_for + +_PW = "correct horse battery staple" +_DOC = Path(__file__).resolve().parent.parent / "docs" / "ROLES.md" +# action -> (method, path); {table} and {id} are filled per run. +_ROUTES = { + "preview": ("POST", "/api/csv/preview"), + "upload": ("POST", "/api/csv/upload"), + "list_files": ("GET", "/api/csv/files"), + "read_rows": ("GET", "/api/csv/tables/{table}/rows"), + "list_te": ("GET", "/api/te/tables"), + "delete_file": ("DELETE", "/api/csv/files/{id}"), + "read_audit": ("GET", "/api/audit/log"), +} + + +def _doc_matrix() -> dict[str, frozenset[str]]: + rows = [ln for ln in _DOC.read_text(encoding="utf-8").splitlines() if ln.startswith("| ")] + header = [c.strip() for c in rows[0].strip("|").split("|")] + out = {} + for ln in rows[1:]: # the |---| separator does not start with "| " + cells = [c.strip() for c in ln.strip("|").split("|")] + out[cells[0]] = frozenset(r for r, c in zip(header[2:], cells[2:]) if c == "✓") + return out + + +@pytest.mark.unit +class MatrixIsDocumented(unittest.TestCase): + def test_docs_table_matches_the_enforced_permissions(self): + self.assertEqual(_doc_matrix(), PERMISSIONS) + + def test_every_action_has_a_route_and_only_known_roles(self): + self.assertEqual(set(PERMISSIONS), set(_ROUTES)) + for action, roles in PERMISSIONS.items(): + self.assertTrue(roles <= set(ROLES), action) + + +@pytest.mark.integration +class RoleMatrixOverTheApi(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls._orig_allow_destructive = settings.allow_destructive + settings.allow_destructive = True + cls.ctx = TestClient(app) + cls.client = cls.ctx.__enter__() + cls.slug = f"roles_{os.getpid()}_{uuid.uuid4().hex[:6]}" + cls.headers = {} + with Conn() as conn: + with conn.cursor() as cur: + tid = accounts.create_tenant(cur, cls.slug, "Roles test") + for role in ROLES: + uid = accounts.create_user(cur, f"{cls.slug}_{role}", _PW) + accounts.add_membership(cur, uid, tid, role) + conn.commit() + migrate_tenant(cls.slug) + for role in ROLES: + res = cls.client.post("/api/auth/login", json={ + "tenant": cls.slug, "username": f"{cls.slug}_{role}", "password": _PW}) + assert res.status_code == 200, res.text + cls.headers[role] = {"Authorization": f"Bearer {res.json()['token']}"} + + @classmethod + def tearDownClass(cls): + settings.allow_destructive = cls._orig_allow_destructive + with Conn() as conn: + with conn.cursor() as cur: + cur.execute(sql.SQL("DROP SCHEMA IF EXISTS {} CASCADE").format(sql.Identifier(schema_for(cls.slug)))) + cur.execute(sql.SQL("DELETE FROM {}.tenants WHERE slug = %s").format( + sql.Identifier(settings.CONTROL_SCHEMA)), (cls.slug,)) + cur.execute(sql.SQL("DELETE FROM {}.users WHERE username LIKE %s").format( + sql.Identifier(settings.CONTROL_SCHEMA)), (f"{cls.slug}_%",)) + conn.commit() + cls.ctx.__exit__(None, None, None) + + def _file(self) -> tuple[str, str]: + tag = uuid.uuid4().hex[:10] + res = self.client.post("/api/csv/upload", headers=self.headers["tenant_admin"], + json={"fileName": f"r_{tag}.csv", "content": f"k\n{tag}\n"}).json() + assert res["status"] == "ok", res + return res["tableName"], res["fileId"] + + def _call(self, role: str, action: str) -> int: + method, path = _ROUTES[action] + table, file_id = self._file() + body = None + if action in ("preview", "upload"): + tag = uuid.uuid4().hex[:10] + body = {"fileName": f"p_{tag}.csv", "content": f"k\n{tag}\n"} + res = self.client.request(method, path.format(table=table, id=file_id), + headers=self.headers[role], json=body) + return res.status_code + + def test_every_role_against_every_route(self): + for action, allowed in PERMISSIONS.items(): + for role in ROLES: + with self.subTest(action=action, role=role): + code = self._call(role, action) + if role in allowed: + self.assertNotIn(code, (401, 403), f"{role} should be allowed to {action}") + self.assertLess(code, 500) + else: + self.assertEqual(code, 403, f"{role} must be refused {action}") + + def test_no_session_requests_are_not_role_checked(self): + self.assertEqual(self.client.get("/api/audit/log").status_code, 200) diff --git a/tests/test_tenant_isolation.py b/tests/test_tenant_isolation.py index bc4c3d7..b5d3a3f 100644 --- a/tests/test_tenant_isolation.py +++ b/tests/test_tenant_isolation.py @@ -1,6 +1,6 @@ """Standing cross-tenant negative tests (M2b, #93, BR-36; ADR-0003). -Two tenants, each with an uploader. Nothing one tenant does may be visible to +Two tenants, each with a tenant admin (the most privileged role). Nothing one tenant does may be visible to or changeable by the other, or by a request that has no session. Integration: needs a live PostgreSQL. Fails (never skips) without one. """ @@ -46,7 +46,7 @@ def setUpClass(cls): for key, slug in cls.slugs.items(): tid = accounts.create_tenant(cur, slug, f"Tenant {key}") uid = accounts.create_user(cur, f"{slug}_user", _PW) - accounts.add_membership(cur, uid, tid, "uploader") + accounts.add_membership(cur, uid, tid, "tenant_admin") # full rights: isolation must still hold conn.commit() for slug in cls.slugs.values(): migrate_tenant(slug)