fix(rls): set the tenant GUC on serving reads of RLS-covered tables (PMS-692) #488

Merged

longjacksonle merged 5 commits from fix/PMS-692-tenant-guc-rls-reads into main 2026-08-03 05:28:27 +02:00

Conversation 0 Commits 5 Files changed 15 +320 -83

longjacksonle commented 2026-08-03 04:13:33 +02:00

Owner

Copy link

PMS-692: set the tenant GUC on serving reads of RLS-covered tables

With the PMS-285 role split active (the dev stack already wires it), Database::pool() returns the unprivileged mokosh_app (NOBYPASSRLS) connection. Migration 038 put a fail-closed tenant_isolation policy on every tenant_id table, so any serving query that runs on the bare pool WITHOUT setting the app.current_tenant GUC (via begin_with_tenant) matches tenant_id = NULL - zero rows on reads, WITH CHECK rejection on writes. This fixes every cited call site.

The concrete breakages fixed

• A - portal password setup: the setup-token candidate lookup (keyed by contact_id, tenant resolved from the row) moves to migrator_pool() with a SAFETY note. It fail-closed on the app pool, so no customer could ever set a portal password.
• B - global search: SearchService held a bare PgPool; GET /search returned an empty 200. Now takes Database and runs all ten scans in one begin_with_tenant transaction.
• C - saved reports: already ran through begin_with_tenant (converted by earlier PMS-256/285 work); covered by a regression assertion.
• D - attachment upload: AttachmentService held a bare PgPool; the INSERT's WITH CHECK was rejected. Now takes Database (reads use an inline tenant tx, writes commit explicitly).
• E - tombstone probe: is_user_tombstoned (tenant-unscoped by design) moves to migrator_pool() with a SAFETY note - on the app pool it always read "not tombstoned", making the MAPPS-348 410 branch dead code.

Also swept

• email_intake cross-table helpers (contacts / users / ticket_notes / tenant_settings) now run through begin_with_tenant; settings::read_email_intake_default_company takes the tenant-GUC connection like its read_ci_impact_max_depth sibling. The bunyip account-deleted webhook (pre-auth, cross-tenant) moves to migrator_pool().
• tickets (portal note-author + visibility), tenants (ticket_sequences probe), sla-worker accessor renamed pool() -> migrator_pool(), and the unused AuthService::pool() accessor removed.
• Every remaining .pool() call (RLS-exempt tenants reads, pre-auth paths) carries an explicit // SAFETY (PMS-285) note.

Guardrail + regression

• scripts/check-pool-safety.nu (wired into check.yml) fails any new serving .pool() call lacking an adjacent // SAFETY (PMS-285 note.
• tests/rls_serving_reads.rs uses the boot_rls / build_app_role_pool harness (request pool as a real NOSUPERUSER NOBYPASSRLS role) to assert GET /search returns the seeded row and is_user_tombstoned reads true for a soft-deleted user - both of which fail-closed before this change.

Acceptance

• grep -rn '.pool()' src/ | grep -v migrator_pool returns only SAFETY-noted sites, each naming an RLS-exempt (tenants) or pre-auth read.
• No **/service.rs / routes.rs holds a bare PgPool field for tenant-scoped queries (Search + Attachment converted).
• Regression on from_pools + NOBYPASSRLS asserts non-empty GET /search and is_user_tombstoned == true (saved-reports/portal set-password covered by existing suites).
• CI fails a new tenant-scoped db.pool() without the SAFETY note.

Verification

clippy --all-targets -D warnings, fmt, all four nu gates, and the auth / RLS / portal / email_intake / tickets / saved-reports suites (13 suites incl. the new one) pass against a throwaway Postgres. Not run against live staging.

## PMS-692: set the tenant GUC on serving reads of RLS-covered tables With the PMS-285 role split active (the dev stack already wires it), `Database::pool()` returns the unprivileged `mokosh_app` (`NOBYPASSRLS`) connection. Migration 038 put a fail-closed `tenant_isolation` policy on every `tenant_id` table, so any serving query that runs on the bare pool WITHOUT setting the `app.current_tenant` GUC (via `begin_with_tenant`) matches `tenant_id = NULL` - zero rows on reads, `WITH CHECK` rejection on writes. This fixes every cited call site. ### The concrete breakages fixed - **A - portal password setup**: the setup-token candidate lookup (keyed by `contact_id`, tenant resolved from the row) moves to `migrator_pool()` with a SAFETY note. It fail-closed on the app pool, so no customer could ever set a portal password. - **B - global search**: `SearchService` held a bare `PgPool`; `GET /search` returned an empty 200. Now takes `Database` and runs all ten scans in one `begin_with_tenant` transaction. - **C - saved reports**: already ran through `begin_with_tenant` (converted by earlier PMS-256/285 work); covered by a regression assertion. - **D - attachment upload**: `AttachmentService` held a bare `PgPool`; the INSERT's `WITH CHECK` was rejected. Now takes `Database` (reads use an inline tenant tx, writes commit explicitly). - **E - tombstone probe**: `is_user_tombstoned` (tenant-unscoped by design) moves to `migrator_pool()` with a SAFETY note - on the app pool it always read "not tombstoned", making the MAPPS-348 410 branch dead code. ### Also swept - **email_intake** cross-table helpers (contacts / users / ticket_notes / tenant_settings) now run through `begin_with_tenant`; `settings::read_email_intake_default_company` takes the tenant-GUC connection like its `read_ci_impact_max_depth` sibling. The bunyip account-deleted webhook (pre-auth, cross-tenant) moves to `migrator_pool()`. - tickets (portal note-author + visibility), tenants (`ticket_sequences` probe), sla-worker accessor renamed `pool()` -> `migrator_pool()`, and the unused `AuthService::pool()` accessor removed. - Every remaining `.pool()` call (RLS-exempt `tenants` reads, pre-auth paths) carries an explicit `// SAFETY (PMS-285)` note. ### Guardrail + regression - `scripts/check-pool-safety.nu` (wired into check.yml) fails any new serving `.pool()` call lacking an adjacent `// SAFETY (PMS-285` note. - `tests/rls_serving_reads.rs` uses the `boot_rls` / `build_app_role_pool` harness (request pool as a real `NOSUPERUSER NOBYPASSRLS` role) to assert `GET /search` returns the seeded row and `is_user_tombstoned` reads `true` for a soft-deleted user - both of which fail-closed before this change. ### Acceptance - [x] `grep -rn '.pool()' src/ | grep -v migrator_pool` returns only SAFETY-noted sites, each naming an RLS-exempt (`tenants`) or pre-auth read. - [x] No `**/service.rs` / `routes.rs` holds a bare `PgPool` field for tenant-scoped queries (Search + Attachment converted). - [x] Regression on `from_pools` + `NOBYPASSRLS` asserts non-empty `GET /search` and `is_user_tombstoned == true` (saved-reports/portal set-password covered by existing suites). - [x] CI fails a new tenant-scoped `db.pool()` without the SAFETY note. ### Verification `clippy --all-targets -D warnings`, `fmt`, all four nu gates, and the auth / RLS / portal / email_intake / tickets / saved-reports suites (13 suites incl. the new one) pass against a throwaway Postgres. Not run against live staging.

longjacksonle added 5 commits 2026-08-03 04:13:33 +02:00

fix(rls): route Search + Attachment services through the tenant GUC (PMS-692) ... 16a069dca7

SearchService and AttachmentService held a bare `PgPool` and read/wrote RLS-covered tables (tickets, contacts, companies, assets, projects, ticket_attachments, ticket_notes) directly on it. On the unprivileged NOBYPASSRLS serving connection those queries fail the `tenant_isolation` policy closed: global search returned an empty 200 (silently wrong) and attachment upload's INSERT was rejected. Both services now take `Database` and run every statement inside a `begin_with_tenant` transaction (search runs all ten in one; attachment reads use an inline tenant tx, writes commit explicitly). The bunyip account-deleted webhook, which is pre-auth and resolves the tenant from the row it reads, moves to `migrator_pool()` with a SAFETY note.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V9DhtRyWubFuzzKohE3JBt

fix(rls): scope portal/auth/tickets/tenants reads by the tenant GUC (PMS-692) ... ceb759c8b4

The individual missed call sites from the audit:

- portal `setup_password`: the setup-token candidate lookup keyed by contact_id (tenant resolved from the row) moves to migrator_pool with a SAFETY note - on the app pool it fail-closed, so no customer could ever set a portal password (case A).
- portal `contact_names`: wraps the `contacts` read in begin_with_tenant.
- auth `is_user_tombstoned`: deliberately tenant-unscoped probe by sub moves to migrator_pool with a SAFETY note - on the app pool it always read "not tombstoned", making the 410 ACCOUNT_DELETED branch dead code (case E).
- tickets: the portal note-author `users` lookup and `assert_portal_ticket_visible` `tickets` read run under begin_with_tenant.
- tenants: the `ticket_sequences` presence probe runs under begin_with_tenant (was a permanent false negative); the two RLS-exempt `tenants` reads carry an explicit SAFETY (PMS-285) marker.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V9DhtRyWubFuzzKohE3JBt

fix(rls): scope email_intake + SLA-worker reads; drop dead pool accessor (PMS-692) ... dd55704ec3

email_intake read/wrote RLS-covered contacts / users / ticket_notes / tenant_settings on the bare app pool - under NOBYPASSRLS the creator lookup found no admin (intake errored) and the contact lookup came back empty (duplicate contacts). Its cross-table helpers now run through begin_with_tenant (the contact resolve+create is one tx), and `settings::read_email_intake_default_company` takes the tenant-GUC connection like its `read_ci_impact_max_depth` sibling.

Also: the SLA sweep worker's cross-tenant accessor is renamed `pool()` -> `migrator_pool()` (it already returned the BYPASSRLS pool; the old name read as a per-tenant serving pool) with a SAFETY note at the call site; the unused `AuthService::pool()` accessor is removed; and the remaining RLS-exempt `tenants` reads (seed try_claim, auth tenant-status, tenants guards) carry explicit SAFETY (PMS-285) markers. `grep '.pool()' src | grep -v migrator_pool` now returns only SAFETY-noted, RLS-exempt/pre-auth sites.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V9DhtRyWubFuzzKohE3JBt

ci(rls): gate new bare .pool() serving calls without a SAFETY note (PMS-692) ... d54256c3e6

scripts/check-pool-safety.nu flags any request-serving `.pool()` call (excluding
migrator_pool and the Database accessor defs) that lacks an adjacent
`// SAFETY (PMS-285` note, so a new tenant-scoped query on the app pool that would
fail-close RLS cannot land silently. Wired into check.yml alongside the other nu
source-hygiene gates.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V9DhtRyWubFuzzKohE3JBt

test(rls): serving reads resolve through the tenant GUC under NOBYPASSRLS (PMS-692) ... 4a937179aa

New regression built on the boot_rls / build_app_role_pool harness (request-serving pool as a real NOSUPERUSER NOBYPASSRLS role): GET /api/v1/search returns the seeded company (case B - fail-closed to an empty 200 before the SearchService conversion), and is_user_tombstoned reads true for a soft-deleted user through the NOBYPASSRLS-app Database (case E - always read false on the app pool, making the 410 branch dead code).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V9DhtRyWubFuzzKohE3JBt

longjacksonle merged commit 7d8164d90d into main 2026-08-03 05:28:27 +02:00

longjacksonle deleted branch fix/PMS-692-tenant-guc-rls-reads 2026-08-03 05:28:27 +02:00

longjacksonle referenced this pull request from a commit 2026-08-03 05:28:28 +02:00

Merge pull request 'fix(rls): set the tenant GUC on serving reads of RLS-covered tables (PMS-692)' (#488) from fix/PMS-692-tenant-guc-rls-reads into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/mokosh-server!488

No description provided.