test(isolation): automate PMS-236 tenant-isolation QA across the API #216

Merged

nrupard merged 2 commits from feat/pms-236-tenant-isolation-integration-test into main

2026-06-13 19:19:14 +02:00

nrupard commented

2026-06-13 19:09:48 +02:00

Owner

Summary

Automates the PMS-236 tenant-isolation QA plan as a Rust integration suite (tests/tenant_isolation.rs): 8 #[sqlx::test] cases that drive the real HTTP API with a Bearer token per tenant, against two tenants each carrying the full seeded lookup set and a tenant-scoped admin user. Closes PMS-236.

The issue was framed as a manual QA pass; the chosen form is automation so the invariants are regression-protected in CI.

Coverage (AC mapping)

cross_tenant_idor_reads_return_404 (AC #4): B gets 404 for A's company/contact/site/ticket ids (existence-hiding, never 403); A reads its own.
cross_tenant_admin_cannot_read_other_tenant (AC #5): a tenant-scoped admin (not super_admin) is denied another tenant's row with 403. Actors are admin because super_admin is the cross-tenant system role and would mask the guard.
search_and_list_do_not_leak_across_tenants (AC #2/#6): full and partial (trigram) q= searches on tickets/companies/contacts return zero of A's rows from B.
ticket_number_sequence_is_per_tenant (AC #2): both tenants' first ticket share a number; a global sequence would offset B by A's volume.
deletion_is_tenant_scoped (AC #3).
settings_and_branding_are_tenant_scoped (AC #8): tenant_settings, tenants.branding, and module_config stay at B's defaults when A writes.
users_are_tenant_scoped (AC #9 users): A's admin cannot list or manage B's users; same email in both tenants scopes the session to one tenant_id.
rls_read_scoping_with_guc (AC #9 RLS): two real tenants under a NOBYPASSRLS role with app.current_tenant set to B - B's row visible, A's hidden by the policy, so the result does not depend on the fail-open default.

Relationship to the sibling suites (not a duplicate)

Documented in the module header: per_user_isolation.rs (PMS-264) pins cross-module read/write denial between technician actors; rls_isolation.rs (PMS-257) pins the fail-closed + WITH CHECK DB-engine face. This file adds the admin-role cross-tenant guard, search scoping, ticket_number, settings/branding/module isolation, and user scoping.

Findings for follow-up (filed separately, not in this PR)

The issue's "billing / notifications / assets / KB return 501" Background is stale: those modules have real handlers now (the E2E suite exercises them), so there is no 501 surface left to defer. The issue text should be updated.
TenantService::create_tenant omits the NOT-NULL tenants.kind column and 500s with SQLSTATE 23502; no existing test exercises that INSERT, so the defect is latent (POST /api/v1/tenants would fail). The suite sidesteps it by provisioning via ensure_personal_tenant.

Verification

cargo test -p mokosh-server --test tenant_isolation: 8 passed, 0 failed (against postgres:18.2-alpine, the CI image).
cargo fmt --all --check clean; cargo clippy --test tenant_isolation clean.

## Summary Automates the PMS-236 tenant-isolation QA plan as a Rust integration suite (`tests/tenant_isolation.rs`): 8 `#[sqlx::test]` cases that drive the real HTTP API with a Bearer token per tenant, against two tenants each carrying the full seeded lookup set and a tenant-scoped `admin` user. Closes PMS-236. The issue was framed as a manual QA pass; the chosen form is automation so the invariants are regression-protected in CI. ## Coverage (AC mapping) - `cross_tenant_idor_reads_return_404` (AC #4): B gets 404 for A's company/contact/site/ticket ids (existence-hiding, never 403); A reads its own. - `cross_tenant_admin_cannot_read_other_tenant` (AC #5): a tenant-scoped `admin` (not super_admin) is denied another tenant's row with 403. Actors are `admin` because super_admin is the cross-tenant system role and would mask the guard. - `search_and_list_do_not_leak_across_tenants` (AC #2/#6): full and partial (trigram) `q=` searches on tickets/companies/contacts return zero of A's rows from B. - `ticket_number_sequence_is_per_tenant` (AC #2): both tenants' first ticket share a number; a global sequence would offset B by A's volume. - `deletion_is_tenant_scoped` (AC #3). - `settings_and_branding_are_tenant_scoped` (AC #8): tenant_settings, `tenants.branding`, and `module_config` stay at B's defaults when A writes. - `users_are_tenant_scoped` (AC #9 users): A's admin cannot list or manage B's users; same email in both tenants scopes the session to one `tenant_id`. - `rls_read_scoping_with_guc` (AC #9 RLS): two real tenants under a `NOBYPASSRLS` role with `app.current_tenant` set to B - B's row visible, A's hidden by the policy, so the result does not depend on the fail-open default. ## Relationship to the sibling suites (not a duplicate) Documented in the module header: `per_user_isolation.rs` (PMS-264) pins cross-module read/write denial between `technician` actors; `rls_isolation.rs` (PMS-257) pins the fail-closed + `WITH CHECK` DB-engine face. This file adds the admin-role cross-tenant guard, search scoping, `ticket_number`, settings/branding/module isolation, and user scoping. ## Findings for follow-up (filed separately, not in this PR) - The issue's "billing / notifications / assets / KB return 501" Background is stale: those modules have real handlers now (the E2E suite exercises them), so there is no 501 surface left to defer. The issue text should be updated. - `TenantService::create_tenant` omits the NOT-NULL `tenants.kind` column and 500s with SQLSTATE 23502; no existing test exercises that INSERT, so the defect is latent (`POST /api/v1/tenants` would fail). The suite sidesteps it by provisioning via `ensure_personal_tenant`. ## Verification - `cargo test -p mokosh-server --test tenant_isolation`: 8 passed, 0 failed (against postgres:18.2-alpine, the CI image). - `cargo fmt --all --check` clean; `cargo clippy --test tenant_isolation` clean.

nrupard added 1 commit

2026-06-13 19:09:48 +02:00

test(isolation): automate PMS-236 tenant-isolation QA across the API

E2E / Playwright against staging (pull_request) Failing after 45s

Details

Check / fmt + clippy + compile + tests (pull_request) Failing after 3m35s

Details

2c69eb9872

Encode the PMS-236 manual tenant-isolation QA plan as a Rust integration suite (tests/tenant_isolation.rs), 8 #[sqlx::test] cases driving the real HTTP API with a Bearer token per tenant. Two tenants are provisioned with the full seeded lookup set (via ensure_personal_tenant, same copy_default_config a normal tenant gets) and a tenant-scoped admin user each.

Coverage, mapped to the acceptance criteria:

- cross_tenant_idor_reads_return_404 (AC #4): tenant B gets 404 (existence-hiding, never 403) for A's company/contact/site/ticket ids; A reads its own.
- cross_tenant_admin_cannot_read_other_tenant (AC #5): a tenant-scoped admin (NOT super_admin) is denied another tenant's row with 403. The actors are admin precisely because super_admin is the cross-tenant system role and would mask the guard - the gap per_user_isolation (technician actors) leaves.
- search_and_list_do_not_leak_across_tenants (AC #2/#6): full and partial (trigram surface) q= searches on tickets/companies/contacts return zero of A's rows from B; B's own rows are present.
- ticket_number_sequence_is_per_tenant (AC #2): both tenants' first ticket share a number; a global sequence would offset B by A's volume.
- deletion_is_tenant_scoped (AC #3).
- settings_and_branding_are_tenant_scoped (AC #8): tenant_settings, tenants.branding, and module_config stay at B's defaults when A writes its own.
- users_are_tenant_scoped (AC #9 users): A's admin cannot list or manage B's users; the same email in both tenants yields a session scoped to exactly one tenant_id.
- rls_read_scoping_with_guc (AC #9 RLS): two real tenants under a NOBYPASSRLS role with app.current_tenant set to B - B's row is visible, A's is hidden by the policy, proving the result does not depend on the fail-open default.

Relationship to siblings (documented in the module header so coverage reads as one set, not a duplicate): per_user_isolation.rs pins cross-module read/write denial between technician actors; rls_isolation.rs pins the fail-closed + WITH CHECK DB-engine face. This adds the admin-role guard, search, ticket_number, settings, and user scoping.

Two findings noted for follow-up (filed separately):
- The issue's "billing/notifications/assets/KB return 501" Background is stale; those modules have real handlers now, so there is no 501 surface to defer.
- TenantService::create_tenant omits the NOT-NULL tenants.kind column and 500s; no existing test exercises that INSERT. The suite sidesteps it via ensure_personal_tenant.

#PMS-236

nrupard added 1 commit

2026-06-13 19:17:43 +02:00

test(isolation): harden module-config and ticket_number checks

E2E / Playwright against staging (pull_request) Failing after 44s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

Check / fmt + clippy + compile + tests (pull_request) Failing after 3m57s

Details

a3224a8586

Address two false-green risks from review of the PMS-236 suite:

- settings_and_branding_are_tenant_scoped: the module_config check compared B's is_enabled before/after A's write but never confirmed A's write took effect. A silently no-op'd write (or a default that already equalled A's target) would leave both A and B unchanged and pass vacuously. Now assert A's own assets module reads back enabled after A's PUT, so "A changed, B did not" is proven both ways.

- ticket_number_sequence_is_per_tenant: parse the numeric sequence value (new ticket_seq helper, tolerant of a bare number or a prefixed string) instead of comparing raw JSON. Assert the sequence advances by exactly one within a tenant and that B's first equals A's first. This fails loudly on a null/missing field and no longer breaks spuriously if a display prefix is added to ticket_number later.

8 tests pass; cargo fmt + clippy clean.

#PMS-236