From 4da44439c4aae6f2b28fcdd62e8bb43f814ca425 Mon Sep 17 00:00:00 2001 From: Christopher Sellers <1@christophersellers.com> Date: Thu, 29 Oct 2026 23:57:00 -0700 Subject: [PATCH 1/3] fix(graph): make the API key's tenant the single source of truth in the tenant funnel Two dependencies resolve a tenant on the graph routers and both seed the RLS contextvar that the SQLAlchemy begin-hook turns into the Postgres tenant GUC: require_api_key (the authenticated principal) and get_current_tenant_id (request.state.tenant_id from TenantContextMiddleware, a JWT claim or the internal header pair, with the principal as fallback). The last writer decides which tenant's rows RLS exposes. PR #3698 moved require_api_key to a router-level dependency so the key resolves first, which made the middleware tenant the last writer: a request carrying key tenant A and middleware tenant B read and wrote B's rows on every router that does not call reconcile_tenant_scope (recall, traceability, lineage; science and labels are Neo4j-only here). Compliance, metrics and regulations reconcile per handler. Mechanism: the funnel (option B), not per-handler reconcile calls. get_current_tenant_id now takes the principal's tenant as authoritative and treats the middleware tenant as an assertion that must match it; a divergence raises 409 E_TENANT_MISMATCH (the reconcile_tenant_scope contract, #2511) before any data access, and the RLS contextvar is seeded with the key tenant. The comparison uses the principal's tenant as stored (string, case-insensitive, as reconcile_tenant_scope does), so a key tenant that is not a UUID can never let the middleware tenant win by falling through to the parsed-None path. Chosen because get_current_tenant_id has no callers outside the graph routers (grep over services/ and server/), so one change covers all 20 handlers on the five routers plus any future route, where per-handler calls would have to be remembered on each new handler. The funnel can only compare against a principal that require_api_key already stored, so the structural walk in test_graph_compliance_key_before_tenant.py is the companion guard: a route that depends on get_current_tenant_id without require_api_key, or resolves the tenant first, fails it unless its path is in TENANT_ONLY_ROUTE_ALLOWLIST (empty; the monolith-wide walk found none), and a synthetic-router test proves the guard fires. services/shared/tests/test_tenant_isolation.py pinned the old behaviour ("middleware tenant wins over principal"); it now asserts the 409, with companion tests that agreement resolves and seeds RLS and that an unparseable principal tenant with a middleware tenant present fails closed. Tests (real TenantContextMiddleware, real require_api_key via the env bypass, no dependency overrides, the router tree as the monolith mounts it): key A plus the authenticated internal header for tenant B returns 409 without reaching the data layer on GET /api/v1/fsma/recall/sla, POST /api/v1/fsma/recall/schedule, GET /api/v1/fsma/traceability/timeline/{tlc}, GET /api/v1/lineage/{tlc}/history and POST /api/v1/labels/batch/init; a JWT-claim tenant B (stub middleware setting request.state.user) is held to the same check; key only and a matching header return 200 with the key tenant in the handler call and in the RLS contextvar. The 409 tests fail on main fb4e5df6a (200, handler ran as tenant B). Runtime, local monolith against local Postgres/Redis (no Neo4j; the SLA route does not use it): main returns 200 with tenant_id B for the divergent request, this branch returns 409 E_TENANT_MISMATCH; key-only and matching requests return 200 as tenant A on both. Graph suite: 2 failed (test_recall_drill_queue_isolation.py, ADMIN_DATABASE_URL unset locally, pre-existing), 817 passed, 8 skipped, 3 xfailed. Jev sidecar on the mechanism choice: funnel (confidence 0.99), request req_01a18b984181764994454b3be057dd9b. Muse read-only review: the write-route, JWT-source and unparseable-principal findings are applied above; the rest describe pre-existing paths (get_optional_tenant_id is unused by graph routers; require_api_key rejects tenantless keys with 403 before the funnel runs) or the ordering constraint the structural test guards. Co-Authored-By: Claude Fable 5.1 --- ...test_graph_compliance_key_before_tenant.py | 70 +++++- ...est_graph_tenant_key_is_source_of_truth.py | 221 ++++++++++++++++++ services/shared/middleware/tenant_context.py | 54 ++++- .../shared/tests/test_tenant_isolation.py | 50 +++- 4 files changed, 374 insertions(+), 21 deletions(-) create mode 100644 services/graph/tests/test_graph_tenant_key_is_source_of_truth.py 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..f27249714 --- /dev/null +++ b/services/graph/tests/test_graph_tenant_key_is_source_of_truth.py @@ -0,0 +1,221 @@ +"""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), + ], +) +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 patch(data_layer) as entry: + 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" + 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_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..dc394d317 100644 --- a/services/shared/middleware/tenant_context.py +++ b/services/shared/middleware/tenant_context.py @@ -295,11 +295,20 @@ 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 +319,31 @@ 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) + principal_tenant = _tenant_id_from_authenticated_principal(request) + + # Compare the principal's tenant as stored, not only its parsed form: a + # key tenant that is not a UUID can never equal the middleware's UUID, and + # must not let the middleware tenant win by falling through to None. + api_key = getattr(request.state, "api_key", None) + raw_principal_tenant = getattr(api_key, "tenant_id", None) if api_key is not None else None + if ( + raw_principal_tenant + and middleware_tenant is not None + and str(middleware_tenant).strip().lower() != str(raw_principal_tenant).strip().lower() + ): + logger.warning( + "graph_tenant_scope_mismatch: middleware tenant differs from the authenticated API key's tenant", + extra={ + "authenticated_tenant": str(raw_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 +351,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..cbd3d3b75 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,45 @@ 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 + async def test_unparseable_principal_tenant_with_middleware_tenant_fails_closed(self): + """A key tenant that is not a UUID can never match the middleware's UUID: + 409, never a fall-through that lets the middleware tenant win.""" + from shared.middleware import get_current_tenant_id + + request = SimpleNamespace( + state=SimpleNamespace( + tenant_id=uuid.uuid4(), + api_key=SimpleNamespace(key_id="k", tenant_id="not-a-uuid"), + ) + ) + + with pytest.raises(HTTPException) as exc_info: + await get_current_tenant_id(request) + assert exc_info.value.status_code == 409 + + @pytest.mark.asyncio + async def test_matching_middleware_and_principal_tenant_resolves(self): + """Agreement between the two sources (case-insensitively) resolves to + that tenant and seeds the RLS contextvar with it — never a 409.""" + 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()), + ) + ) + + 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): From bb52b4ed8c6a4049636c81cd8edcf2208ad0091c Mon Sep 17 00:00:00 2001 From: Christopher Sellers <1@christophersellers.com> Date: Fri, 30 Oct 2026 00:37:48 -0700 Subject: [PATCH 2/3] fix(graph): reject unparseable principal tenants and compare parsed UUIDs in the tenant funnel CodeRabbit on #3762: a principal tenant with surrounding spaces fails to parse, the string comparison stripped it to a match, and the middleware tenant was then selected for a malformed credential. get_current_tenant_id now fails closed with 401 when the principal carries a tenant that does not parse, and compares parsed UUIDs so equivalent spellings never 409 and a non-UUID middleware value never matches. Also adds the same-client sequence test CodeRabbit asked for: a rejected tenant-B assertion followed by a key-only request on the same TestClient, asserting the second runs as tenant A with RLS seeded A. Co-Authored-By: Claude Fable 5.1 --- ...est_graph_tenant_key_is_source_of_truth.py | 15 ++++++++++ services/shared/middleware/tenant_context.py | 28 +++++++++++++------ .../shared/tests/test_tenant_isolation.py | 23 +++++++++------ 3 files changed, 49 insertions(+), 17 deletions(-) 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 index f27249714..4fdb68b02 100644 --- 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 @@ -202,6 +202,21 @@ def test_middleware_tenant_that_matches_key_tenant_is_accepted(_env): 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) diff --git a/services/shared/middleware/tenant_context.py b/services/shared/middleware/tenant_context.py index dc394d317..6ff602200 100644 --- a/services/shared/middleware/tenant_context.py +++ b/services/shared/middleware/tenant_context.py @@ -290,6 +290,14 @@ 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. @@ -320,22 +328,26 @@ async def get_reports( return await fetch_reports(tenant_id) """ middleware_tenant = getattr(request.state, "tenant_id", None) - principal_tenant = _tenant_id_from_authenticated_principal(request) - - # Compare the principal's tenant as stored, not only its parsed form: a - # key tenant that is not a UUID can never equal the middleware's UUID, and - # must not let the middleware tenant win by falling through to None. api_key = getattr(request.state, "api_key", None) raw_principal_tenant = getattr(api_key, "tenant_id", None) if api_key is not None else None + principal_tenant = _tenant_id_from_authenticated_principal(request) + + if raw_principal_tenant and principal_tenant is None: + # The principal carries a tenant that does not parse: a malformed + # credential, never a reason to fall back to the middleware tenant. + 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 ( - raw_principal_tenant + principal_tenant is not None and middleware_tenant is not None - and str(middleware_tenant).strip().lower() != str(raw_principal_tenant).strip().lower() + 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(raw_principal_tenant), + "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, diff --git a/services/shared/tests/test_tenant_isolation.py b/services/shared/tests/test_tenant_isolation.py index cbd3d3b75..b41b49949 100644 --- a/services/shared/tests/test_tenant_isolation.py +++ b/services/shared/tests/test_tenant_isolation.py @@ -175,26 +175,31 @@ async def test_middleware_tenant_must_match_principal(self): assert exc_info.value.detail == "E_TENANT_MISMATCH" @pytest.mark.asyncio - async def test_unparseable_principal_tenant_with_middleware_tenant_fails_closed(self): - """A key tenant that is not a UUID can never match the middleware's UUID: - 409, never a fall-through that lets the middleware tenant win.""" + @pytest.mark.parametrize("bad_principal", ["not-a-uuid", " {mw} "]) + async def test_unparseable_principal_tenant_with_middleware_tenant_fails_closed(self, bad_principal): + """A principal whose tenant does not parse is a malformed credential: + 401, never a fall-through that resolves the middleware tenant — even + when the middleware tenant is the same UUID with surrounding spaces.""" from shared.middleware import get_current_tenant_id + mw_tenant = uuid.uuid4() request = SimpleNamespace( state=SimpleNamespace( - tenant_id=uuid.uuid4(), - api_key=SimpleNamespace(key_id="k", tenant_id="not-a-uuid"), + tenant_id=mw_tenant, + api_key=SimpleNamespace(key_id="k", tenant_id=bad_principal.format(mw=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.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 (case-insensitively) resolves to - that tenant and seeds the RLS contextvar with it — never a 409.""" + """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 @@ -202,7 +207,7 @@ async def test_matching_middleware_and_principal_tenant_resolves(self): request = SimpleNamespace( state=SimpleNamespace( tenant_id=tenant, - api_key=SimpleNamespace(key_id="k", tenant_id=str(tenant).upper()), + api_key=SimpleNamespace(key_id="k", tenant_id=str(tenant).upper().replace("-", "")), ) ) From 5c25a6d28887fb3451f127512c89269da00cd958 Mon Sep 17 00:00:00 2001 From: Christopher Sellers <1@christophersellers.com> Date: Fri, 30 Oct 2026 00:56:16 -0700 Subject: [PATCH 3/3] fix(graph): fail closed on an authenticated principal with no usable tenant Muse second-opinion review on bb52b4ed8 (HIGH): a principal present on request.state.api_key whose tenant_id is None or empty skipped both the 401 (raw value falsy) and the 409 (parsed value None), so the middleware tenant drove the handler and the RLS contextvar with no principal authority behind it. require_api_key rejects tenantless keys with 403 on the key-store path, but the env-bypass path stores the principal without that check. get_current_tenant_id now returns 401 whenever a principal is present and its tenant is missing, empty or unparseable. Tests: the unit case covers None and "" alongside the unparseable forms; the route tests add the science router to the 409 matrix and a JWT claim that matches the key tenant (200). Co-Authored-By: Claude Fable 5.1 --- ...est_graph_tenant_key_is_source_of_truth.py | 19 +++++++++++++++++-- services/shared/middleware/tenant_context.py | 9 +++++---- .../shared/tests/test_tenant_isolation.py | 14 ++++++++------ 3 files changed, 30 insertions(+), 12 deletions(-) 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 index 4fdb68b02..9c6ea7d1a 100644 --- 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 @@ -151,17 +151,21 @@ def _construct(**_kwargs): ("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 patch(data_layer) as entry: + 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" - assert entry.mock_calls == [], f"{method} {url} reached its data layer with a divergent tenant" + 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): @@ -202,6 +206,17 @@ def test_middleware_tenant_that_matches_key_tenant_is_accepted(_env): 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]] = [] diff --git a/services/shared/middleware/tenant_context.py b/services/shared/middleware/tenant_context.py index 6ff602200..369fdd43e 100644 --- a/services/shared/middleware/tenant_context.py +++ b/services/shared/middleware/tenant_context.py @@ -329,12 +329,13 @@ async def get_reports( """ middleware_tenant = getattr(request.state, "tenant_id", None) api_key = getattr(request.state, "api_key", None) - raw_principal_tenant = getattr(api_key, "tenant_id", None) if api_key is not None else None principal_tenant = _tenant_id_from_authenticated_principal(request) - if raw_principal_tenant and principal_tenant is None: - # The principal carries a tenant that does not parse: a malformed - # credential, never a reason to fall back to the middleware tenant. + 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) diff --git a/services/shared/tests/test_tenant_isolation.py b/services/shared/tests/test_tenant_isolation.py index b41b49949..d03c677a6 100644 --- a/services/shared/tests/test_tenant_isolation.py +++ b/services/shared/tests/test_tenant_isolation.py @@ -175,18 +175,20 @@ async def test_middleware_tenant_must_match_principal(self): assert exc_info.value.detail == "E_TENANT_MISMATCH" @pytest.mark.asyncio - @pytest.mark.parametrize("bad_principal", ["not-a-uuid", " {mw} "]) - async def test_unparseable_principal_tenant_with_middleware_tenant_fails_closed(self, bad_principal): - """A principal whose tenant does not parse is a malformed credential: - 401, never a fall-through that resolves the middleware tenant — even - when the middleware tenant is the same UUID with surrounding spaces.""" + @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=bad_principal.format(mw=mw_tenant)), + api_key=SimpleNamespace(key_id="k", tenant_id=principal_tenant), ) )