feat(auth): close PMS-4 - list filter, layered rate limit, MFA recovery codes, tenant hardening #83
No reviewers
Labels
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
psa-systems/mokosh-server!83
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/pms-4-closeout"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PMS-4 ('Story: Authentication and user management') had six marked-resolved subtasks but a live audit of main revealed real gaps in five of seven ACs. This PR fixes all five surgically and lands integration tests pinning each.
AC1 (list_users filter). New ListUsersFilter in crates/mokosh-types/src/auth.rs (q capped at 200 chars, optional role, optional status) deriving Validate. Route handler wires Query + calls filter.validate()? before delegating. Service rewrites list_users with two parallel dynamic WHEREs (data: $1 tenant, $2 limit, $3 offset, $4+ filters; count: $1 tenant, $2+ filters) so the count query stays in sync with the data query. Existing idx_users_role (tenant_id, role) and idx_users_status (tenant_id, status) indices serve the role/status filters; q ILIKE is a per-tenant seq scan, acceptable.
AC2 (layered rate limit + Retry-After). Rewrote rate_limit.rs as a LoginLimiter bundling two governor::RateLimiter instances: 20/min keyed on source IP (covers office NATs) + 5/min keyed on lowercased email (the account-level cap the audit asked for). Either bucket tripping returns 429 with a populated Retry-After header derived from the longer of the two refill waits. The check happens inline at the top of the login handler because tower middleware cannot read the JSON body without buffering it. Removed the old .layer(...) wiring and the from_fn_with_state middleware function.
AC3 (MFA recovery codes). New migration 027_user_mfa_recovery_codes.sql adds users.mfa_recovery_codes_hashes TEXT[] NOT NULL DEFAULT '{}' (catalog-only ACCESS EXCLUSIVE on Postgres 15+). LoginRequest gets an optional recovery_code field; MfaEnableResponse is a new Serialize type returning the 10 single-use codes ONCE. enable_mfa now mints 10 codes via mokosh_auth_crypto::recovery::generate_set() and persists hex SHA-256 hashes; login's MFA branch tries recovery_code first (with an atomic UPDATE ... SET mfa_recovery_codes_hashes = array_remove(...) WHERE $1 = ANY(...) RETURNING TRUE inside a CTE so re-using the same code on a concurrent request still fails the ANY guard on the second call); else mfa_code (TOTP); else 200 with mfa_required:true. disable_mfa zeroes the codes for symmetry. A host-side recovery_code_hex_hash helper reuses mokosh_auth_crypto::recovery::hash_code for canonicalisation so SSO and legacy paths share the same hashing.
AC6 (tenant isolation hardening, cross-cutting issue #8 closeout for the auth module). Four surgical fixes:
Related UPDATE fall-out: change_password, update_user, start_mfa_enrollment all take tenant_id now and bind it on both SELECTs and UPDATEs; reset_password fetches tenant_id alongside the candidate token_hash and scopes the user UPDATE on it. middleware.rs's HS256 path passes claims.tid into get_user_by_id; the bunyip OIDC JIT path passes default_bunyip_tenant_id().
find_user_by_email (the fifth method) still keys on email only because login does not yet hold a tenant hint. Added ORDER BY created_at ASC LIMIT 1 for determinism and a doc comment naming the residual multi-tenant risk; the real fix (subdomain-driven LoginRequest.tenant_id) is filed as a separate YT follow-up issue per the plan.
AC7 (integration tests). Two new helpers in tests/common/mod.rs (seed_user with caller-supplied email/role, seed_tenant_with_admin with a fresh tenant + admin). Nine new tests in tests/auth.rs:
Verified: cargo check --tests, cargo clippy --tests -- -Dwarnings, cargo fmt --all --check all clean; cargo test --test auth runs 10/10 green against the host postgres in 3.05s.
Plan + closeout notes: docs/mokosh-upgrade/auto-stories/PMS-4/plan.md.
Closes PMS-4. Follow-up YT issue for find_user_by_email tenant hint will be filed against PMS-4 (relates) post-merge.
PMS-4 ('Story: Authentication and user management') had six marked-resolved subtasks but a live audit of main revealed real gaps in five of seven ACs. This PR fixes all five surgically and lands integration tests pinning each. AC1 (list_users filter). New ListUsersFilter in crates/mokosh-types/src/auth.rs (q capped at 200 chars, optional role, optional status) deriving Validate. Route handler wires Query<ListUsersFilter> + calls filter.validate()? before delegating. Service rewrites list_users with two parallel dynamic WHEREs (data: \$1 tenant, \$2 limit, \$3 offset, \$4+ filters; count: \$1 tenant, \$2+ filters) so the count query stays in sync with the data query. Existing idx_users_role (tenant_id, role) and idx_users_status (tenant_id, status) indices serve the role/status filters; q ILIKE is a per-tenant seq scan, acceptable. AC2 (layered rate limit + Retry-After). Rewrote rate_limit.rs as a LoginLimiter bundling two governor::RateLimiter instances: 20/min keyed on source IP (covers office NATs) + 5/min keyed on lowercased email (the account-level cap the audit asked for). Either bucket tripping returns 429 with a populated Retry-After header derived from the longer of the two refill waits. The check happens inline at the top of the login handler because tower middleware cannot read the JSON body without buffering it. Removed the old .layer(...) wiring and the from_fn_with_state middleware function. AC3 (MFA recovery codes). New migration 027_user_mfa_recovery_codes.sql adds users.mfa_recovery_codes_hashes TEXT[] NOT NULL DEFAULT '{}' (catalog-only ACCESS EXCLUSIVE on Postgres 15+). LoginRequest gets an optional recovery_code field; MfaEnableResponse is a new Serialize type returning the 10 single-use codes ONCE. enable_mfa now mints 10 codes via mokosh_auth_crypto::recovery::generate_set() and persists hex SHA-256 hashes; login's MFA branch tries recovery_code first (with an atomic UPDATE ... SET mfa_recovery_codes_hashes = array_remove(...) WHERE \$1 = ANY(...) RETURNING TRUE inside a CTE so re-using the same code on a concurrent request still fails the ANY guard on the second call); else mfa_code (TOTP); else 200 with mfa_required:true. disable_mfa zeroes the codes for symmetry. A host-side recovery_code_hex_hash helper reuses mokosh_auth_crypto::recovery::hash_code for canonicalisation so SSO and legacy paths share the same hashing. AC6 (tenant isolation hardening, cross-cutting issue #8 closeout for the auth module). Four surgical fixes: - get_user_by_id(tenant_id, user_id) - WHERE id = \$1 AND tenant_id = \$2. - get_session(tenant_id, session_id) - same shape, called from refresh_token with claims.tid. - update_session_activity(tenant_id, session_id) - same. - update_last_login(tenant_id, user_id) - WHERE id = \$1 AND tenant_id = \$2. Related UPDATE fall-out: change_password, update_user, start_mfa_enrollment all take tenant_id now and bind it on both SELECTs and UPDATEs; reset_password fetches tenant_id alongside the candidate token_hash and scopes the user UPDATE on it. middleware.rs's HS256 path passes claims.tid into get_user_by_id; the bunyip OIDC JIT path passes default_bunyip_tenant_id(). find_user_by_email (the fifth method) still keys on email only because login does not yet hold a tenant hint. Added ORDER BY created_at ASC LIMIT 1 for determinism and a doc comment naming the residual multi-tenant risk; the real fix (subdomain-driven LoginRequest.tenant_id) is filed as a separate YT follow-up issue per the plan. AC7 (integration tests). Two new helpers in tests/common/mod.rs (seed_user with caller-supplied email/role, seed_tenant_with_admin with a fresh tenant + admin). Nine new tests in tests/auth.rs: - list_users_pagination_happy_path (15 users seeded; page=2 per_page=10 returns 5) - list_users_filter_by_role_and_q (role narrows; q + role intersects) - list_users_filter_validation_rejects_oversize_q (201-char q -> 422; F9 pin) - list_users_requires_admin (technician -> 403) - mfa_challenge_happy_path (enroll, enable, login without code -> mfa_required:true, login with code -> success) - mfa_recovery_code_login_single_use (first code -> 200, replay -> 401, second code -> 200) - login_wrong_password_returns_401 - login_rate_limit_triggers_429 (5x 401 then 6th 429 with Retry-After header + JSON body) - tenant_isolation_get_user_by_id_returns_404 (two tenants seeded; A's admin GETs B's user -> 404) Verified: cargo check --tests, cargo clippy --tests -- -Dwarnings, cargo fmt --all --check all clean; cargo test --test auth runs 10/10 green against the host postgres in 3.05s. Plan + closeout notes: docs/mokosh-upgrade/auto-stories/PMS-4/plan.md. Closes PMS-4. Follow-up YT issue for find_user_by_email tenant hint will be filed against PMS-4 (relates) post-merge.