diff --git a/services/graph/tests/test_graph_compliance_key_before_tenant.py b/services/graph/tests/test_graph_compliance_key_before_tenant.py index 8d9e572d8..69fb00c32 100644 --- a/services/graph/tests/test_graph_compliance_key_before_tenant.py +++ b/services/graph/tests/test_graph_compliance_key_before_tenant.py @@ -21,6 +21,13 @@ ``provisions`` route in ``routes.py`` already declared the key first. ``test_every_graph_route_checks_key_before_tenant`` walks every route served under ``services.graph.app.routes.router`` so none of them can regress. + +The order is also what makes the tenant funnel sound. ``get_current_tenant_id`` +reconciles the middleware tenant against the principal ``require_api_key`` stores +on ``request.state`` (409 ``E_TENANT_MISMATCH`` on divergence). A route that +resolves the tenant with no key dependency, or before the key, has no principal +to compare with, and the middleware tenant silently wins. Such routes fail the +walk unless their path is in ``TENANT_ONLY_ROUTE_ALLOWLIST``. """ from __future__ import annotations @@ -32,7 +39,7 @@ from unittest.mock import AsyncMock, patch import pytest -from fastapi import FastAPI +from fastapi import APIRouter, Depends, FastAPI from fastapi.routing import APIRoute from fastapi.testclient import TestClient @@ -49,6 +56,13 @@ TENANT = "11111111-1111-1111-1111-111111111111" BYPASS_TOKEN = "graph-key-before-tenant-test" # noqa: S105 — test-only env bypass value +# Effective paths (prefix applied) of graph routes allowed to depend on +# ``get_current_tenant_id`` without ``require_api_key``. Keep this empty: the +# monolith-wide walk found zero such routes, and adding one is a reviewed +# decision that the route has some other authenticated principal, not a +# convenience to make the walk pass. +TENANT_ONLY_ROUTE_ALLOWLIST: frozenset[str] = frozenset() + @pytest.fixture(autouse=True) def _rate_limiter_off(monkeypatch): @@ -188,12 +202,14 @@ def _effective_api_routes(router) -> list: return [ctx for ctx in iter_route_contexts(app.routes) if isinstance(ctx.route, APIRoute)] -def test_every_graph_route_checks_key_before_tenant(): - from services.graph.app import routes as graph_routes - - routes = _effective_api_routes(graph_routes.router) - assert routes, "graph router has no routes" +def _key_before_tenant_offenders(routes, allowlist: frozenset[str]) -> tuple[list[str], list[str]]: + """Split the tenant-bound routes in ``routes`` into (checked paths, offenders). + An offender depends on ``get_current_tenant_id`` without ``require_api_key`` + (unless its effective path is in ``allowlist``) or resolves the tenant before + the key. Either way the funnel has no principal to reconcile against when it + runs, so the middleware tenant would silently win. + """ checked: list[str] = [] offenders: list[str] = [] for route in routes: @@ -202,11 +218,49 @@ def test_every_graph_route_checks_key_before_tenant(): continue # open route (health), metrics-key route, or key-only route checked.append(route.path) if require_api_key not in order: - # Graph routes are API-key only: a tenant-bound route with no key - # dependency has no principal to resolve the tenant from. + if route.path in allowlist: + continue offenders.append(f"{sorted(route.methods)} {route.path}: no API-key dependency") elif order.index(require_api_key) > order.index(get_current_tenant_id): offenders.append(f"{sorted(route.methods)} {route.path}") + return checked, offenders + + +def test_tenant_only_route_is_reported_unless_allowlisted(): + """The guard itself fires: no real graph route trips it, so prove it on a synthetic router.""" + synthetic = APIRouter() + + @synthetic.get("/tenant-only") + async def _tenant_only(tenant_id: uuid.UUID = Depends(get_current_tenant_id)): + return {"tenant_id": str(tenant_id)} + + @synthetic.get("/tenant-before-key") + async def _tenant_before_key( + tenant_id: uuid.UUID = Depends(get_current_tenant_id), + api_key=Depends(require_api_key), + ): + return {"tenant_id": str(tenant_id)} + + routes = _effective_api_routes(synthetic) + checked, offenders = _key_before_tenant_offenders(routes, allowlist=frozenset()) + assert checked == ["/api/v1/tenant-only", "/api/v1/tenant-before-key"] + assert offenders == [ + "['GET'] /api/v1/tenant-only: no API-key dependency", + "['GET'] /api/v1/tenant-before-key", + ] + + # The allowlist excuses only the tenant-only route, never a wrong order. + _, allowed = _key_before_tenant_offenders(routes, allowlist=frozenset({"/api/v1/tenant-only"})) + assert allowed == ["['GET'] /api/v1/tenant-before-key"] + + +def test_every_graph_route_checks_key_before_tenant(): + from services.graph.app import routes as graph_routes + + routes = _effective_api_routes(graph_routes.router) + assert routes, "graph router has no routes" + + checked, offenders = _key_before_tenant_offenders(routes, allowlist=TENANT_ONLY_ROUTE_ALLOWLIST) # The walk must reach the sub-routers, not just the operational routes in routes.py. for expected in ( diff --git a/services/graph/tests/test_graph_tenant_key_is_source_of_truth.py b/services/graph/tests/test_graph_tenant_key_is_source_of_truth.py new file mode 100644 index 000000000..9c6ea7d1a --- /dev/null +++ b/services/graph/tests/test_graph_tenant_key_is_source_of_truth.py @@ -0,0 +1,251 @@ +"""The API key's tenant is the single source of truth on every graph route. + +Two dependencies resolve a tenant on the graph routers. ``require_api_key`` +stores the authenticated principal on ``request.state.api_key``; +``get_current_tenant_id`` reads ``request.state.tenant_id`` (set by +``TenantContextMiddleware`` from a JWT claim or the internal header pair) and +falls back to that principal. Both seed the RLS contextvar that the SQLAlchemy +begin-hook turns into the Postgres tenant GUC, so the last writer decides which +tenant's rows RLS exposes. Since #3698 the key resolves first at router level, +which made the middleware tenant the last writer: a request carrying key tenant +A and middleware tenant B operated on B's rows on every router that did not +call ``reconcile_tenant_scope`` (recall, traceability, lineage). + +``get_current_tenant_id`` now treats the principal's tenant as authoritative and +the middleware tenant as an assertion that must match it. A divergence is +rejected with 409 ``E_TENANT_MISMATCH`` (the ``reconcile_tenant_scope`` +contract, #2511) before the handler runs. These tests drive the real middleware +and the real ``require_api_key`` (env bypass), with no dependency overrides, +against the router tree the monolith mounts. +""" + +from __future__ import annotations + +import sys +from contextlib import ExitStack +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient +from starlette.middleware.base import BaseHTTPMiddleware + +REPO_ROOT = Path(__file__).resolve().parents[3] +for _p in (str(REPO_ROOT), str(REPO_ROOT / "services")): + if _p not in sys.path: + sys.path.insert(0, _p) + +from shared.middleware.tenant_context import TenantContextMiddleware # noqa: E402 +from shared.tenant_context import get_rls_tenant_id # noqa: E402 + +TENANT_A = "11111111-1111-1111-1111-111111111111" +TENANT_B = "22222222-2222-2222-2222-222222222222" +BYPASS_TOKEN = "graph-key-tenant-truth-test" # noqa: S105 — test-only env bypass value +INTERNAL_SECRET = "graph-key-tenant-truth-internal" # noqa: S105 — test-only header secret # pragma: allowlist secret + +KEY_ONLY = {"X-RegEngine-API-Key": BYPASS_TOKEN} + +RECALL_ENGINE = "services.graph.app.routers.fsma.recall.get_recall_engine" +TRACE_NEO4J = "services.graph.app.routers.fsma.traceability.Neo4jClient" +LINEAGE_NEO4J = "services.graph.app.routers.lineage_traversal.Neo4jClient" +LABELS_NEO4J = "services.graph.app.routers.labels.Neo4jClient" + +# A body that passes the labels request model, so the only thing that can stop +# the write is the tenant check. +LABEL_BATCH = { + "packer_gln": "0614141000005", + "product": {"gtin": "00614141000012", "description": "Romaine hearts", "expected_units": 10}, + "traceability": {"lot_number": "LOT-1", "pack_date": "2026-10-01"}, + "quantity": 1, +} + + +class _JwtClaimStub(BaseHTTPMiddleware): + """Stand-in for the JWT layer: a validated claims dict on ``request.state.user``. + + Runs ahead of ``TenantContextMiddleware`` (added after it, so outermost), which + reads the claim as its first tenant source. ``request.state`` is backed by the + ASGI scope, so the value is visible to every layer below. + """ + + def __init__(self, app, tenant: str): + super().__init__(app) + self._tenant = tenant + + async def dispatch(self, request, call_next): + request.state.user = {"tenant_id": self._tenant} + return await call_next(request) + + +@pytest.fixture(autouse=True) +def _rate_limiter_off(monkeypatch): + """Keep these GETs out of the shared 10/min quotas without touching Redis.""" + from shared.rate_limit import limiter + + monkeypatch.setattr(limiter, "enabled", False) + + +@pytest.fixture +def _env(monkeypatch): + """Real ``require_api_key`` via its test bypass, real internal-header secret.""" + monkeypatch.setenv("REGENGINE_ENV", "test") + monkeypatch.setenv("AUTH_TEST_BYPASS_TOKEN", BYPASS_TOKEN) + monkeypatch.setenv("AUTH_TEST_BYPASS_TENANT_ID", TENANT_A) + monkeypatch.setenv("REGENGINE_INTERNAL_SECRET", INTERNAL_SECRET) + + +def _app(jwt_tenant: str | None = None) -> FastAPI: + """The whole graph router tree behind the real tenant middleware, as the monolith mounts it.""" + from services.graph.app import routes as graph_routes + + app = FastAPI() + app.add_middleware(TenantContextMiddleware) + if jwt_tenant is not None: + app.add_middleware(_JwtClaimStub, tenant=jwt_tenant) + app.include_router(graph_routes.router, prefix="/api/v1") + return app + + +def _with_middleware_tenant(tenant: str) -> dict[str, str]: + """Key plus the authenticated internal header pair that resolves ``tenant`` in the middleware.""" + return {**KEY_ONLY, "X-RegEngine-Tenant-ID": tenant, "X-RegEngine-Internal-Secret": INTERNAL_SECRET} + + +def _recall_engine_stub(seen: list[tuple[str, str | None]]) -> MagicMock: + """A recall engine whose SLA lookup records the tenant it was asked for and the RLS tenant in effect.""" + engine = MagicMock() + + def _sla(tenant_str: str) -> dict: + seen.append((tenant_str, get_rls_tenant_id())) + return {"sla_compliance_rate": 100, "total_drills": 0, "avg_trace_time": 0} + + engine.get_sla_metrics.side_effect = _sla + return engine + + +def _neo4j_stub(seen: list[str | None]) -> MagicMock: + """A Neo4jClient double whose construction records the RLS tenant in effect.""" + cls = MagicMock() + cls.get_tenant_database_name.return_value = "tenant-db" + instance = MagicMock() + instance.__aenter__ = AsyncMock(return_value=MagicMock()) + instance.__aexit__ = AsyncMock(return_value=False) + + def _construct(**_kwargs): + seen.append(get_rls_tenant_id()) + return instance + + cls.side_effect = _construct + return cls + + +# ── Negative path: a middleware tenant that differs from the key tenant ───────── + + +@pytest.mark.parametrize( + "method, url, body, data_layer", + [ + ("GET", "/api/v1/fsma/recall/sla", None, RECALL_ENGINE), + ("POST", "/api/v1/fsma/recall/schedule", {}, RECALL_ENGINE), + ("GET", "/api/v1/fsma/traceability/timeline/LOT-1", None, TRACE_NEO4J), + ("GET", "/api/v1/lineage/LOT-1/history", None, LINEAGE_NEO4J), + ("POST", "/api/v1/labels/batch/init", LABEL_BATCH, LABELS_NEO4J), + # The science handlers answer 501 themselves; the 409 must still come first. + ("GET", "/api/v1/fsma/science/mass-balance/LOT-1", None, None), + ], +) +def test_middleware_tenant_that_differs_from_key_tenant_is_rejected(_env, method, url, body, data_layer): + """Key tenant A + authenticated internal header for tenant B: 409, handler never reached.""" + client = TestClient(_app(), raise_server_exceptions=False) + with ExitStack() as stack: + entry = stack.enter_context(patch(data_layer)) if data_layer else None + resp = client.request(method, url, json=body, headers=_with_middleware_tenant(TENANT_B)) + + assert resp.status_code == 409, resp.text + assert resp.json()["detail"] == "E_TENANT_MISMATCH" + if entry is not None: + assert entry.mock_calls == [], f"{method} {url} reached its data layer with a divergent tenant" + + +def test_jwt_claim_tenant_that_differs_from_key_tenant_is_rejected(_env): + """The middleware's first tenant source, a validated JWT claim, is held to the same check.""" + client = TestClient(_app(jwt_tenant=TENANT_B), raise_server_exceptions=False) + with patch(RECALL_ENGINE) as entry: + resp = client.get("/api/v1/fsma/recall/sla", headers=KEY_ONLY) + + assert resp.status_code == 409, resp.text + assert resp.json()["detail"] == "E_TENANT_MISMATCH" + assert entry.mock_calls == [] + + +# ── Happy paths: the key tenant drives the handler and the RLS contextvar ─────── + + +def test_key_only_recall_request_runs_as_the_key_tenant(_env): + seen: list[tuple[str, str | None]] = [] + client = TestClient(_app(), raise_server_exceptions=False) + with patch(RECALL_ENGINE, return_value=_recall_engine_stub(seen)): + resp = client.get("/api/v1/fsma/recall/sla", headers=KEY_ONLY) + + assert resp.status_code == 200, resp.text + assert resp.json()["tenant_id"] == TENANT_A + # The handler scoped the engine call to the key tenant, and the RLS + # contextvar the Postgres begin-hook would read held the same tenant. + assert seen == [(TENANT_A, TENANT_A)] + + +def test_middleware_tenant_that_matches_key_tenant_is_accepted(_env): + """Agreement between the two sources is not a mismatch: no 409 on legitimate requests.""" + seen: list[tuple[str, str | None]] = [] + client = TestClient(_app(), raise_server_exceptions=False) + with patch(RECALL_ENGINE, return_value=_recall_engine_stub(seen)): + resp = client.get("/api/v1/fsma/recall/sla", headers=_with_middleware_tenant(TENANT_A)) + + assert resp.status_code == 200, resp.text + assert seen == [(TENANT_A, TENANT_A)] + + +def test_jwt_claim_tenant_that_matches_key_tenant_is_accepted(_env): + """A JWT claim naming the key's own tenant is agreement, not a mismatch.""" + seen: list[tuple[str, str | None]] = [] + client = TestClient(_app(jwt_tenant=TENANT_A), raise_server_exceptions=False) + with patch(RECALL_ENGINE, return_value=_recall_engine_stub(seen)): + resp = client.get("/api/v1/fsma/recall/sla", headers=KEY_ONLY) + + assert resp.status_code == 200, resp.text + assert seen == [(TENANT_A, TENANT_A)] + + +def test_rejected_divergent_request_does_not_taint_the_next_request(_env): + """Same client, two requests: a 409 for a tenant-B assertion leaves nothing behind for the key-only request that follows.""" + seen: list[tuple[str, str | None]] = [] + client = TestClient(_app(), raise_server_exceptions=False) + with patch(RECALL_ENGINE, return_value=_recall_engine_stub(seen)): + rejected = client.get("/api/v1/fsma/recall/sla", headers=_with_middleware_tenant(TENANT_B)) + accepted = client.get("/api/v1/fsma/recall/sla", headers=KEY_ONLY) + + assert rejected.status_code == 409, rejected.text + assert accepted.status_code == 200, accepted.text + assert accepted.json()["tenant_id"] == TENANT_A + # Only the second request reached the engine, and it ran as A with RLS seeded A. + assert seen == [(TENANT_A, TENANT_A)] + + +def test_key_only_traceability_request_runs_as_the_key_tenant(_env): + seen: list[str | None] = [] + client = TestClient(_app(), raise_server_exceptions=False) + with ExitStack() as stack: + stack.enter_context(patch(TRACE_NEO4J, _neo4j_stub(seen))) + timeline = AsyncMock(return_value=[]) + stack.enter_context(patch("services.graph.app.routers.fsma.traceability.get_lot_timeline", timeline)) + audit = MagicMock() + audit.alog = AsyncMock() + stack.enter_context(patch("services.graph.app.routers.fsma.traceability.get_audit_log", return_value=audit)) + resp = client.get("/api/v1/fsma/traceability/timeline/LOT-1", headers=KEY_ONLY) + + assert resp.status_code == 200, resp.text + assert timeline.await_count == 1 + assert timeline.await_args.args[2] == TENANT_A + assert audit.alog.await_args.kwargs["tenant_id"] == TENANT_A + assert seen == [TENANT_A] diff --git a/services/shared/middleware/tenant_context.py b/services/shared/middleware/tenant_context.py index b6b532732..369fdd43e 100644 --- a/services/shared/middleware/tenant_context.py +++ b/services/shared/middleware/tenant_context.py @@ -290,16 +290,33 @@ def _tenant_id_from_authenticated_principal(request: Request) -> Optional[uuid.U return None +def _as_uuid(value: object) -> Optional[uuid.UUID]: + """Parse ``value`` as a UUID; None when it is not one.""" + try: + return uuid.UUID(str(value).strip()) + except (ValueError, AttributeError, TypeError): + return None + + async def get_current_tenant_id(request: Request) -> uuid.UUID: """ FastAPI dependency to get the current request's tenant_id. Resolution order: - 1. ``request.state.tenant_id`` set by ``TenantContextMiddleware`` - (JWT or authenticated internal header). - 2. Fallback to the tenant on the authenticated principal that - ``require_api_key`` attaches to ``request.state.api_key`` — this is how - per-key DB-issued API keys resolve their own tenant (#2524). + 1. The tenant on the authenticated principal that ``require_api_key`` + attaches to ``request.state.api_key`` — this is how per-key DB-issued + API keys resolve their own tenant (#2524), and when it is present it is + the single source of truth. + 2. ``request.state.tenant_id`` set by ``TenantContextMiddleware`` (JWT or + authenticated internal header). When a principal tenant is also present + the middleware tenant is only an assertion that must match it: a + divergence is rejected with 409 ``E_TENANT_MISMATCH`` (the + ``shared.auth.reconcile_tenant_scope`` contract, #2511) before any data + access. It is never resolved in favour of the middleware — since #3698 + the key resolves first on every graph router, which had made the + middleware tenant the last writer of the RLS contextvar, so key tenant A + plus middleware tenant B read and wrote B's rows on the routers that do + not call ``reconcile_tenant_scope`` themselves. Raises HTTPException(401) if no tenant_id is available (fail closed). @@ -310,9 +327,36 @@ async def get_reports( ): return await fetch_reports(tenant_id) """ - tenant_id = getattr(request.state, "tenant_id", None) - if tenant_id is None: - tenant_id = _tenant_id_from_authenticated_principal(request) + middleware_tenant = getattr(request.state, "tenant_id", None) + api_key = getattr(request.state, "api_key", None) + principal_tenant = _tenant_id_from_authenticated_principal(request) + + if api_key is not None and principal_tenant is None: + # An authenticated principal with no usable tenant (missing, empty or + # unparseable) is a malformed credential, never a reason to fall back + # to the middleware tenant: that would let a header or claim pick the + # tenant with no principal authority behind it. + raise HTTPException(status_code=401, detail="Tenant ID not found in authentication context") + + # Compare parsed UUIDs, so equivalent spellings (case, braces, no hyphens) + # never 409 and a middleware value that is not a UUID never matches. + if ( + principal_tenant is not None + and middleware_tenant is not None + and _as_uuid(middleware_tenant) != principal_tenant + ): + logger.warning( + "graph_tenant_scope_mismatch: middleware tenant differs from the authenticated API key's tenant", + extra={ + "authenticated_tenant": str(principal_tenant), + "middleware_tenant": str(middleware_tenant), + "key_id": getattr(api_key, "key_id", None), + "path": request.url.path if getattr(request, "url", None) is not None else None, + }, + ) + raise HTTPException(status_code=409, detail="E_TENANT_MISMATCH") + + tenant_id = principal_tenant if principal_tenant is not None else middleware_tenant if tenant_id is None: raise HTTPException(status_code=401, detail="Tenant ID not found in authentication context") @@ -320,7 +364,12 @@ async def get_reports( # #2461: seed the RLS contextvar — this dependency is the tenant # funnel for all graph routers (traceability, recall, lineage, # compliance, metrics, labels), whose fsma_recall / fsma_audit - # private engines are covered by the class-level begin-hook. + # private engines are covered by the class-level begin-hook. The + # comparison above only works when require_api_key has already run: + # every graph route resolves the key first (router-level dependency + # or declaration order), and test_graph_compliance_key_before_tenant + # fails any route that does not, because a tenant resolved before the + # key would hand the handler the middleware tenant with no 409. set_rls_tenant_id(tenant_id) return tenant_id diff --git a/services/shared/tests/test_tenant_isolation.py b/services/shared/tests/test_tenant_isolation.py index ebacaaedc..d03c677a6 100644 --- a/services/shared/tests/test_tenant_isolation.py +++ b/services/shared/tests/test_tenant_isolation.py @@ -151,9 +151,13 @@ async def test_two_db_keys_resolve_to_own_tenants(self): assert tenant_a != tenant_b @pytest.mark.asyncio - async def test_middleware_tenant_wins_over_principal(self): - """When the middleware already resolved a tenant (JWT/internal-header/ - env key), it is authoritative and the principal fallback is not used.""" + async def test_middleware_tenant_must_match_principal(self): + """When the middleware resolved a tenant (JWT/internal header) and the + authenticated API key carries a different one, fail closed with 409 + ``E_TENANT_MISMATCH`` (the ``reconcile_tenant_scope`` contract, #2511). + The middleware tenant never overrides the principal: #3698 made it the + last writer of the RLS contextvar on the graph routers, so letting it + win meant key tenant A operated on tenant B's rows.""" from shared.middleware import get_current_tenant_id mw_tenant = uuid.uuid4() @@ -165,7 +169,52 @@ async def test_middleware_tenant_wins_over_principal(self): ) ) - assert await get_current_tenant_id(request) == mw_tenant + with pytest.raises(HTTPException) as exc_info: + await get_current_tenant_id(request) + assert exc_info.value.status_code == 409 + assert exc_info.value.detail == "E_TENANT_MISMATCH" + + @pytest.mark.asyncio + @pytest.mark.parametrize("bad_principal", ["not-a-uuid", " {mw} ", "", None]) + async def test_unusable_principal_tenant_with_middleware_tenant_fails_closed(self, bad_principal): + """A principal whose tenant is unparseable, empty or missing is a + malformed credential: 401, never a fall-through that resolves the + middleware tenant — even when that tenant is the same UUID with + surrounding spaces.""" + from shared.middleware import get_current_tenant_id + + mw_tenant = uuid.uuid4() + principal_tenant = bad_principal.format(mw=mw_tenant) if isinstance(bad_principal, str) else bad_principal + request = SimpleNamespace( + state=SimpleNamespace( + tenant_id=mw_tenant, + api_key=SimpleNamespace(key_id="k", tenant_id=principal_tenant), + ) + ) + + with pytest.raises(HTTPException) as exc_info: + await get_current_tenant_id(request) + assert exc_info.value.status_code == 401 + assert "Tenant ID not found" in str(exc_info.value.detail) + + @pytest.mark.asyncio + async def test_matching_middleware_and_principal_tenant_resolves(self): + """Agreement between the two sources resolves to that tenant and seeds + the RLS contextvar with it — never a 409, whatever the spelling (the + principal here is upper-case with no hyphens).""" + from shared.middleware import get_current_tenant_id + from shared.tenant_context import get_rls_tenant_id + + tenant = uuid.uuid4() + request = SimpleNamespace( + state=SimpleNamespace( + tenant_id=tenant, + api_key=SimpleNamespace(key_id="k", tenant_id=str(tenant).upper().replace("-", "")), + ) + ) + + assert await get_current_tenant_id(request) == tenant + assert get_rls_tenant_id() == str(tenant) @pytest.mark.asyncio async def test_api_key_without_tenant_fails_closed(self):