feat(db): enforce row-level security on workspace-scoped transactions (P2-1b-iii) - #407
Draft
jwvanderstam wants to merge 5 commits into
Draft
jwvanderstam wants to merge 5 commits into
jwvanderstam wants to merge 5 commits into
Conversation
…urity (P2-1b-i) Records ADR-5: workspace isolation is enforced in the database. The scope is passed to get_connection() by the method that already holds it, not bound in a contextvar; ALL_WORKSPACES stays on the owner role. The ADR states what this commits to (shared-schema tenancy as the boundary, operator = installation owner) and what it does not (a single node). - The 18 by-id methods hand their scope to get_connection(scope=). - test_object_authorization_matrix fails any scoped method that does not, and asserts the scan finds exactly SCOPED_METHODS. - Migration 0018 grants the application identity SET on localchat_scoped; without it a managed database would refuse the role switch. No behaviour change: get_connection accepts the scope and does not act on it until the 34 methods still taking workspace_id: str | None are converted (P2-1b-ii), so retrieval is covered from the first day RLS enforces anything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kspace Found while sorting the workspace_id: str | None methods for P2-1b-ii. - POST /api/chat: additional_workspace_ids from the request body reached document and memory retrieval unauthorised. Any user who knew a workspace's id could retrieve from it, a workspace API key included. Each extra id is now authorised like the primary one (member at viewer+, admin any, API key none); one unauthorised id refuses the request with 403. - POST /api/documents/test: retrieval ran with no workspace, returning chunk previews from every workspace to any viewer. Now uses the authorised one. - list_documents LLM tool: called get_all_documents() bare, listing every workspace's documents. Now reads the request scope like search_documents. Each has a regression test that fails with its fix reverted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sed, never defaulted (P2-1b-ii) Fifteen database methods took workspace_id: str | None = None and read None as every workspace - the listings, the counts, and retrieval itself. Each now takes a mandatory keyword-only scope: Scope, builds SQL through scope_predicate (which refuses None and ""), and hands the scope to get_connection(scope=). The retrieval chain above them (retrieve_context, MemoryRetriever.retrieve, suggest_documents, chat's retrieve_contexts) takes a Scope too, so the translation happens once, at the route, via get_scope(). Behaviour-preserving by construction: every former None is an explicit ALL_WORKSPACES (SyncWorker, connector loading, boot count, admin stats, the two token-authenticated MCP list_sources calls). SCOPED_METHODS now lists 33. One behaviour change, a disclosure the conversion surfaced: GET /api/status counted documents for whatever X-Workspace-ID named. It now counts only a workspace the caller is authorised for, and 0 otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… (P2-1b-iii)
get_connection(scope=<workspace>) now opens the transaction with
SET LOCAL ROLE localchat_scoped and set_config('app.workspace_id', ..., true),
so Postgres refuses other workspaces' rows even when a query omits its WHERE
clause. ALL_WORKSPACES and None stay on the owner role (ADR-5).
- Migration 0019: default privileges, so tables a later release adds are not
"permission denied" on the scoped path only.
- search_memories runs one scoped transaction per authorised workspace; under
RLS a single ANY(...) query silently dropped the additional ones.
- Scoped transactions set hnsw.iterative_scan = strict_order and
hnsw.ef_search = 400. Without them the policy turned exact per-workspace
vector search into a filtered HNSW scan returning 19 of 40 rows on average,
as few as 0. With them: 40 of 40, 92% overlap with exact, 3.8 ms median vs
9.1 ms. Requires pgvector 0.8+.
- test_row_level_security: fixture connection is autocommit, so owner checks
really run as the owner; new tests for the app connection, default
privileges, and scoped top_k recall, each failing with its fix removed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a new cluster restore-proof failed on migration 0019: a dump carrying ALTER DEFAULT PRIVILEGES cannot be restored by a non-superuser. Looking closer showed the managed-Postgres restore recipe had been broken since 0017 for any new cluster: a role is a cluster object pg_dump never carries, so the dump's GRANT ... TO localchat_scoped fails, and even a restore that got through would leave 0017 recorded with its role missing - every scoped query failing once RLS is enforced. - Drop 0019. _ensure_extensions_and_tables() now creates localchat_scoped if missing, grants it to the app identity and grants it all current tables and sequences, every boot; bootstrap re-applies it after Alembic so tables a migration adds are covered on the same boot. - OPERATIONS.md: the managed recipe gains --no-privileges; the app restores the grants on start. Needs CREATEROLE, or the role created once. - restore-proof restores that way and asserts the grant is absent before boot and present after. Verified by hand in a second, empty cluster: the old recipe fails with role "localchat_scoped" does not exist; with the new one a non-superuser identity restored, booted, created the role and served a scoped query. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This was referenced Sep 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last of three PRs for the application half of P2-1b (ADR-5). This one changes behaviour: row-level security is now enforced.
What changes
get_connection(scope=<workspace>)opens the transaction withSET LOCAL ROLE localchat_scopedandset_config('app.workspace_id', %s, true). The workspace is bound as a parameter, never interpolated into the SQL.ALL_WORKSPACESandNonestay on the owner role. It refuses an autocommit connection, where both settings would lapse at once, and an empty workspace._apply_scoped_role, inside_ensure_extensions_and_tables(), and again after Alembic). This also covers tables a later release adds. See "Restore" below for why this isn't a migration.search_memoriesruns one scoped transaction per authorised workspace and merges the results by similarity. Under RLS, a singleANY([...])query silently dropped the additional workspaces.hnsw.iterative_scan = strict_orderandhnsw.ef_search = 400. The next section explains why.The benchmark found a recall regression, not a latency one
Under the policy, Postgres swaps the exact per-workspace vector scan for the HNSW index filtered afterwards. The benchmark used 10,000 clustered chunks across five workspaces, the application's own
search_similar_chunks, 200 interleaved queries, and a top-40 request:test_a_scoped_vector_search_still_returns_top_kpins the fix. With the two settings removed, 20 queries returned[34, 25, 38, 0, 0, 0, …]rows. It needs 10,000 chunks: at 4,000 the planner keeps the exact plan and the test passes either way.This requires pgvector 0.8 or later. An older version refuses the setting and scoped queries fail loudly. The shipped image is
pgvector/pgvector:pg16, which runs 0.8.2. The Scaleway managed database's pgvector version has not been checked.Tests, each confirmed to fail with its fix removed
test_the_application_connection_enforces_the_scope: the realDatabasepool runs aslocalchat_scopedwith the workspace set, and an unfilteredSELECT count(*) FROM documentssees 1 row in its own workspace and 0 in a foreign one. So the database is doing the hiding, not the query.test_a_table_added_later_is_granted_to_the_scoped_role_at_boot: a new table is unreachable on the scoped path before boot and reachable after it.test_a_scoped_vector_search_still_returns_top_kcovers the recall fix.test_scoped_connection.pypins the exact statements in the fast suite, including thatALL_WORKSPACESandNoneissue nothing.top_k(two workspaces, interleaved scores).conn.transaction()nested as a savepoint andSET LOCAL ROLEoutlived it, so "the owner can see the seeded row" could run as the restricted role.Verification (local, isolated from the live stack)
ruff,mypy,bandit: clean.33f2695with the same result.Restore:
restore-proofcaught a problem, and it predates this PRThe first version of this PR used migration
0019(ALTER DEFAULT PRIVILEGES).restore-prooffailed on it, because a non-superuser cannot restore a dump that carries it. Looking closer showed that the managed-Postgres restore recipe has been broken since0017(#400) for any new cluster:pg_dumpnever carries it.GRANT … TO localchat_scopedfails withrole "localchat_scoped" does not exist.0017recorded as applied with its role missing. With RLS enforced, every scoped query would then fail.The fix (
33f2695):OPERATIONS.md: the recipe gains--no-privileges, plus a new row in its "what fails and why" table.restore-proofrestores that way and asserts the grant is absent before boot and present after.Verified by hand in a second, empty cluster:
CREATEROLErestored the dump, booted, created the role itself, and served a scoped query aslocalchat_scoped. This also settles feat(db): ADR-5 and the get_connection(scope=) seam for row-level security (P2-1b-i) #404's open item about switching roles as a non-superuser.Requirement: the app identity needs
CREATEROLE, or the provider must createlocalchat_scoped(NOLOGIN) once.0017already needed that.One deviation from what was agreed
We agreed that the integration test database would run the Alembic chain. That turned out not to be needed. Every fixture that reaches a scoped path already migrates:
test_ingest_ask_citeexplicitly, and the live-server fixtures through the app's bootstrap. A missing role also fails loudly ("role does not exist") instead of passing. So there's no conftest change in this PR. It's easy to add if you still want it.Not verified
🤖 Generated with Claude Code