fix(rls): complete the fail-closed backstop on 11 deferred tables (PMS-683) #467

Merged

David merged 2 commits from fix/PMS-683-rls-tail into main

2026-08-01 01:32:35 +02:00

nrupard commented

2026-07-24 21:23:55 +02:00

Owner

What

Complete the fail-closed RLS backstop that PR #464 began: migrate the 6 feature services behind the 11 deferred tenant-scoped tables onto Database::begin_with_tenant, enable RLS on all 11 tables in migration 095_rls_deferred_tables.sql, and shrink tests/rls_coverage.rs's ALLOWED_WITHOUT_RLS allowlist to empty. Every tenant-scoped public table now has RLS enabled, forced, and a tenant_isolation policy.

Why

The request-serving pool (mokosh_app) is NOBYPASSRLS, so enabling RLS on a table actively constrains every app query against it: a query that does not set the app.current_tenant GUC via begin_with_tenant fail-closes to zero rows. PR #464 could only safely cover quotes / quote_sequences because the other 11 tables' services still queried the raw pool. This change migrates those services so the tables are GUC-safe, then turns RLS on for them, restoring the tenant-isolation invariant the allowlist was tracking as debt.

Changes

Migrated services (field pool: PgPool -> db: Database; every tenant-scoped statement now runs inside begin_with_tenant, mirroring quotes/contacts): dashboards (saved_dashboards, scheduled_dashboards), approvals (ticket_approvals, change_requests), email_intake (tenant_intake_tokens, email_intake_log), saved_reports (saved_reports, scheduled_reports), workflows (workflow_rules, workflow_rule_runs), ticket_templates (ticket_templates).
tenant_intake_tokens special case: the cross-tenant resolve_token SHA-256 lookup (and its last_used_at bump) run on the BYPASSRLS migrator_pool with an explicit // SAFETY: note, since resolve-by-hash has no tenant context to set as the GUC; every other access is tenant-scoped.
migrations/095_rls_deferred_tables.sql: ENABLE + FORCE ROW LEVEL SECURITY and a fail-closed tenant_isolation policy on all 11 tables, mirroring the 038/090/091/094 shape exactly.
tests/rls_coverage.rs: ALLOWED_WITHOUT_RLS is now empty; the test asserts (both directions) that every tenant table has RLS and nothing stale remains.
Construction sites updated (src/api/router.rs, src/main.rs workers, tests/scheduled_reports.rs, tests/scheduled_dashboards.rs) to pass db.clone().
Fixed three latent raw-pool reads of already-RLS tables that the migration surfaced (they would fail-close under the production role split): the approvals create() ticket-existence check, the approvals routes parent-existence check (time_entries / change_requests / quotes, now via a new ApprovalsService::db() accessor), and saved_reports execute() running its compiled entity query (tickets etc.) - all now inside begin_with_tenant.

Tests

just pre-commit (mirrors CI check.yml): PASSED, exit 0. fmt + clippy --all-targets -D warnings + check --all-targets clean; unit tests 293 passed / 0 failed; doc tests 2 passed / 0 failed / 3 ignored.
RLS + migrated-service feature suite via the Postgres-backed docker harness (--test-threads=4, --no-fail-fast): all 18 targeted binaries PASSED, 0 failed, exit 0. Key results: rls_coverage ok (confirms all 11 tables now have RLS enabled + forced + a tenant_isolation policy and the allowlist is empty and consistent in both directions); per_user_isolation 4 ok and rls_isolation 1 ok (the NOBYPASSRLS app-role path still isolates correctly); plus dashboards_crud, scheduled_dashboards (3), ticket_approvals, approvals_polymorphic (5), email_intake, email_intake_attachments, email_intake_phase2 (3), intake_token_admin, saved_reports, saved_reports_execute, scheduled_reports (3), workflow_rules, workflow_rules_phase2, workflow_rules_phase3, ticket_templates all ok.
Harness note: #[sqlx::test]'s boot_rls path creates an unprivileged probe role, which requires CREATEROLE. The dev container's default DATABASE_URL role (mokosh_migrator) lacks it, so per_user_isolation / rls_isolation only run when DATABASE_URL is the superuser role - exactly as CI runs them (integration.yml uses the postgres superuser). The suite above was run with DATABASE_URL set to the container's MOKOSH_ADMIN_DATABASE_URL (the postgres superuser) to mirror CI; this is a pre-existing local-harness characteristic, unrelated to this change.
Fail-close reasoning (the feature tests boot the BYPASSRLS single-pool harness so they do not themselves exercise a fail-close): every statement touching the 11 tables was verified to run through begin_with_tenant (GUC set) or, for the one documented cross-tenant resolve_token path, the BYPASSRLS migrator_pool. No bare self.pool remains in any migrated service; the residual self.db.pool() calls in email_intake touch only non-target tables (users / contacts / tickets / ticket_notes / tenant_settings), whose RLS posture is unchanged by this migration.

## What Complete the fail-closed RLS backstop that PR #464 began: migrate the 6 feature services behind the 11 deferred tenant-scoped tables onto `Database::begin_with_tenant`, enable RLS on all 11 tables in migration `095_rls_deferred_tables.sql`, and shrink `tests/rls_coverage.rs`'s `ALLOWED_WITHOUT_RLS` allowlist to empty. Every tenant-scoped `public` table now has RLS enabled, forced, and a `tenant_isolation` policy. ## Why The request-serving pool (`mokosh_app`) is NOBYPASSRLS, so enabling RLS on a table actively constrains every app query against it: a query that does not set the `app.current_tenant` GUC via `begin_with_tenant` fail-closes to zero rows. PR #464 could only safely cover `quotes` / `quote_sequences` because the other 11 tables' services still queried the raw pool. This change migrates those services so the tables are GUC-safe, then turns RLS on for them, restoring the tenant-isolation invariant the allowlist was tracking as debt. ## Changes - Migrated services (field `pool: PgPool` -> `db: Database`; every tenant-scoped statement now runs inside `begin_with_tenant`, mirroring `quotes`/`contacts`): `dashboards` (saved_dashboards, scheduled_dashboards), `approvals` (ticket_approvals, change_requests), `email_intake` (tenant_intake_tokens, email_intake_log), `saved_reports` (saved_reports, scheduled_reports), `workflows` (workflow_rules, workflow_rule_runs), `ticket_templates` (ticket_templates). - `tenant_intake_tokens` special case: the cross-tenant `resolve_token` SHA-256 lookup (and its `last_used_at` bump) run on the BYPASSRLS `migrator_pool` with an explicit `// SAFETY:` note, since resolve-by-hash has no tenant context to set as the GUC; every other access is tenant-scoped. - `migrations/095_rls_deferred_tables.sql`: ENABLE + FORCE ROW LEVEL SECURITY and a fail-closed `tenant_isolation` policy on all 11 tables, mirroring the 038/090/091/094 shape exactly. - `tests/rls_coverage.rs`: `ALLOWED_WITHOUT_RLS` is now empty; the test asserts (both directions) that every tenant table has RLS and nothing stale remains. - Construction sites updated (`src/api/router.rs`, `src/main.rs` workers, `tests/scheduled_reports.rs`, `tests/scheduled_dashboards.rs`) to pass `db.clone()`. - Fixed three latent raw-pool reads of already-RLS tables that the migration surfaced (they would fail-close under the production role split): the `approvals` `create()` ticket-existence check, the `approvals` routes parent-existence check (`time_entries` / `change_requests` / `quotes`, now via a new `ApprovalsService::db()` accessor), and `saved_reports` `execute()` running its compiled entity query (tickets etc.) - all now inside `begin_with_tenant`. ## Tests - `just pre-commit` (mirrors CI check.yml): PASSED, exit 0. fmt + clippy `--all-targets -D warnings` + check `--all-targets` clean; unit tests 293 passed / 0 failed; doc tests 2 passed / 0 failed / 3 ignored. - RLS + migrated-service feature suite via the Postgres-backed docker harness (`--test-threads=4`, `--no-fail-fast`): all 18 targeted binaries PASSED, 0 failed, exit 0. Key results: `rls_coverage` ok (confirms all 11 tables now have RLS enabled + forced + a `tenant_isolation` policy and the allowlist is empty and consistent in both directions); `per_user_isolation` 4 ok and `rls_isolation` 1 ok (the NOBYPASSRLS app-role path still isolates correctly); plus `dashboards_crud`, `scheduled_dashboards` (3), `ticket_approvals`, `approvals_polymorphic` (5), `email_intake`, `email_intake_attachments`, `email_intake_phase2` (3), `intake_token_admin`, `saved_reports`, `saved_reports_execute`, `scheduled_reports` (3), `workflow_rules`, `workflow_rules_phase2`, `workflow_rules_phase3`, `ticket_templates` all ok. - Harness note: `#[sqlx::test]`'s `boot_rls` path creates an unprivileged probe role, which requires CREATEROLE. The dev container's default `DATABASE_URL` role (`mokosh_migrator`) lacks it, so `per_user_isolation` / `rls_isolation` only run when `DATABASE_URL` is the superuser role - exactly as CI runs them (integration.yml uses the `postgres` superuser). The suite above was run with `DATABASE_URL` set to the container's `MOKOSH_ADMIN_DATABASE_URL` (the postgres superuser) to mirror CI; this is a pre-existing local-harness characteristic, unrelated to this change. - Fail-close reasoning (the feature tests boot the BYPASSRLS single-pool harness so they do not themselves exercise a fail-close): every statement touching the 11 tables was verified to run through `begin_with_tenant` (GUC set) or, for the one documented cross-tenant `resolve_token` path, the BYPASSRLS `migrator_pool`. No bare `self.pool` remains in any migrated service; the residual `self.db.pool()` calls in `email_intake` touch only non-target tables (users / contacts / tickets / ticket_notes / tenant_settings), whose RLS posture is unchanged by this migration.

nrupard added 1 commit

2026-07-24 21:23:55 +02:00

fix(rls): complete the fail-closed backstop on 11 deferred tables

E2E / Playwright against staging (pull_request) Successful in 37s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 1m30s

Details

Integration / integration tests (pull_request) Successful in 5m38s

Details

e4e8a7961c

PR #464 (094_rls_quotes_backstop.sql) enabled fail-closed RLS on quotes + quote_sequences and left the other 11 tenant-scoped tables in the tests/rls_coverage.rs ALLOWED_WITHOUT_RLS allowlist, because their feature services still queried the raw NOBYPASSRLS mokosh_app pool without setting the app.current_tenant GUC. Enabling RLS on those tables before migrating their services would have fail-closed them in production. This change migrates each service onto Database::begin_with_tenant, enables RLS on all 11 tables (migration 095), and shrinks the allowlist to empty, fully restoring the tenant-isolation invariant.

Services migrated (each now routes every tenant-scoped statement through begin_with_tenant, mirroring quotes/contacts): dashboards (saved_dashboards, scheduled_dashboards), approvals (ticket_approvals, change_requests), email_intake (tenant_intake_tokens, email_intake_log), saved_reports (saved_reports, scheduled_reports), workflows (workflow_rules, workflow_rule_runs), ticket_templates (ticket_templates). The scheduled-report / scheduled-dashboard workers already scan cross-tenant on the BYPASSRLS migrator pool and call the services per-tenant, and the WorkflowExecutor writes workflow_rule_runs inside the ticket service's own begin_with_tenant transaction, so every path to these tables is GUC-safe.

Special case, tenant_intake_tokens: the email-intake resolve_token lookup matches a presented bearer's SHA-256 across every tenant and is cross-tenant by design (the bearer is the only identity, so there is no tenant context to set as the GUC). That single lookup, plus its paired last_used_at bump, now runs on the BYPASSRLS migrator pool with an explicit SAFETY note; every other tenant_intake_tokens access is tenant-scoped via begin_with_tenant.

Also fixes three latent raw-pool reads of already-RLS tables that the migration surfaced: the approvals create() ticket-existence check, the approvals routes parent-existence check (time_entries / change_requests / quotes), and saved_reports execute() running its compiled entity query (tickets etc.) all now run inside begin_with_tenant, so they no longer fail-close under the production role split.

#PMS-683

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

David added 1 commit

2026-08-01 01:09:03 +02:00