fix(auth): close last-admin TOCTOU with deferred DB trigger

[cristim]

Summary

• Closes fix(auth): close last-admin TOCTOU (count + mutate admins in one transaction) #919: adds migration 000058 with a DEFERRABLE INITIALLY DEFERRED constraint trigger (trg_min_one_admin) that enforces at least one Administrators-group member at transaction commit time, making the count-and-mutate sequence atomic at the DB level
• Adds isLastAdminConstraintViolation in store_postgres.go to map the P0001 trigger exception back to ErrLastAdmin, so DeleteUser and UpdateUser surface the same sentinel regardless of which guard fires
• Retains the existing application-level soft check in checkLastAdminConstraint / DeleteUser as a fast early-exit returning a user-friendly 409; the trigger is the hard backstop

Race condition fixed

Two concurrent privileged requests each removing a different one of the last two admin-group members can both observe count == 2, both pass the soft check, and leave zero admins. The deferred trigger fires at commit time for each transaction, seeing the post-commit state (including any concurrently committed deletes), and rejects the transaction that would leave zero admins.

Test plan

• TestService_DeleteUser/blocks_delete_of_last_admin_member_(soft_check): soft check returns ErrLastAdmin when count == 1
• TestService_DeleteUser/maps_DB_trigger_violation_to_ErrLastAdmin: trigger sentinel error is mapped to ErrLastAdmin
• TestService_DeleteUser_ConcurrentLastTwoAdmins: two goroutines fire simultaneously; exactly one gets ErrLastAdmin
• TestService_UpdateUser/blocks_removing_admin_group_from_last_admin_member_(soft_check): soft check in guardGroupChange
• TestService_UpdateUser/maps_DB_trigger_violation_to_ErrLastAdmin_on_UpdateUser: UpdateUser trigger path
• go test ./internal/auth/... ./internal/api/... all pass (513 + 1385 tests)
• go vet clean; gofmt -l no output
• Race detector: go test -race ./internal/auth/... -run TestService_DeleteUser passes

closes #919

Summary by CodeRabbit

• Bug Fixes

  • Implemented enforcement preventing the system from having zero administrators, blocking deletion or modification of the last admin user with a specific error.
  • Added database-level constraint trigger for race-condition protection when multiple concurrent admin removal attempts occur.

• Tests

  • Added comprehensive test coverage for last admin protection scenarios, including concurrency regression tests.

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 40 minutes and 14 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0784bea9-49f3-45e1-9037-e76ea0baead0

📥 Commits

Reviewing files that changed from the base of the PR and between ebf9af2 and 435320d.

📒 Files selected for processing (6)

• internal/auth/service_user.go
• internal/auth/service_user_test.go
• internal/auth/store_postgres.go
• internal/database/postgres/migrations/000065_enforce_min_one_admin.down.sql
• internal/database/postgres/migrations/000065_enforce_min_one_admin.up.sql
• internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go

📝 Walkthrough

Walkthrough

This PR closes a time-of-check/time-of-use race in last-admin protection by adding a PostgreSQL deferred constraint trigger that prevents zero-admin states at commit time. The service layer detects and translates trigger violations into sentinel errors. New tests validate soft checks and exercise concurrent removal of the last two admins.

Changes

Last-admin constraint enforcement and error mapping

Layer / File(s)	Summary
Database constraint trigger and function internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql, internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql	Migration 000058 adds a deferred constraint trigger trg_min_one_admin that fires at transaction commit on UPDATE or DELETE of users in the Administrators group. The trigger calls check_min_one_admin() PL/pgSQL function to count Administrators members; if count falls below 1, the function raises an exception and rolls back the entire transaction. The trigger is DEFERRABLE INITIALLY DEFERRED and filtered by a WHEN clause to skip rows not previously in the Administrators group.
Last-admin constraint violation detection internal/auth/store_postgres.go	Helper function isLastAdminConstraintViolation() detects PostgreSQL exceptions (code P0001) with sentinel message prefix last_admin_constraint_violation, supporting both actual *pgconn.PgError and plain error stubs for unit tests.
UpdateUser and DeleteUser error translation internal/auth/service_user.go	UpdateUser and DeleteUser intercept store errors and map deferred constraint violations detected by isLastAdminConstraintViolation() into the ErrLastAdmin sentinel; otherwise return the raw store error.
Test coverage for last-admin protections internal/auth/service_user_test.go	Adds subtests for DeleteUser and UpdateUser asserting soft-check rejection and deferred-violation mapping. Introduces new concurrency regression test TestService_DeleteUser_ConcurrentLastTwoAdmins that spawns two parallel goroutines removing different admins, capturing results on a buffered channel and asserting exactly one fails with ErrLastAdmin.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Suggested labels

priority/p1, severity/high, urgency/this-sprint, impact/many, effort/m, type/security

Poem

🐰 A race once haunted the admin lane,
Where two concurrent requests could cause great pain—
But now at commit, the database does check,
And triggers ensure we never shipwreck.
With zero admins? That's locked away tight,
The carrot patch stays safe through the night! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main fix: adding a deferred DB trigger to close a TOCTOU race in last-admin protection.
Linked Issues check	✅ Passed	All coding requirements from #919 are met: deferred DB trigger enforces ≥1 Administrators, error mapping added, and concurrent regression test included.
Out of Scope Changes check	✅ Passed	All changes directly address the TOCTOU race fix with deferred trigger and error handling; no unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/auth-last-admin-toctou

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

internal/auth/service_user_test.go (1)

388-456: 🏗️ Heavy lift

Concurrency test is deterministic and does not validate the DB-level race closure.

The mock is pre-wired so admin-b's delete always returns the sentinel error and admin-a always succeeds; the outcome is independent of goroutine scheduling, so "exactly one fails" holds trivially regardless of whether the real deferred trigger works. This guards the service-layer error mapping under concurrency, but the headline claim ("proving the race is closed") rests on the DB trigger, which is never exercised here. Given the critical concern raised on migration 000058 (an unlocked COUNT(*) may not serialize concurrent commits), an integration test against real PostgreSQL spawning two real transactions is what would actually prove the invariant. Consider renaming/reframing this as a "concurrent mapping" test and adding a DB-backed regression test.

Want me to draft a PostgreSQL-backed integration test (two concurrent transactions deleting the last two admins) to validate the trigger end-to-end?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/auth/service_user_test.go` around lines 388 - 456, The test
currently claims to prove the DB-level race closure but deterministically wires
mockStore so admin-b always fails; rename
TestService_DeleteUser_ConcurrentLastTwoAdmins to
TestService_DeleteUser_ConcurrentErrorMapping (or similar), update the function
comment and assertion message to state it only verifies service-layer error
mapping under concurrent DeleteUser calls (not the DB trigger), and add a TODO
comment that an actual PostgreSQL-backed integration test should be added to
validate migration 000058 end-to-end; keep the existing mockStore setup and
assertions unchanged otherwise.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql`:
- Around line 29-35: The current migration's admin_count check ignores user
active state and thus allows deactivating the last active admin; update the
SELECT that sets admin_count (used in this IF/RAISE block) to only count users
who are active (i.e., add "AND active = true") so the predicate mirrors the
AdminExists/CountGroupMembers semantics and prevents deactivation-driven lockout
for the Administrators group UUID '00000000-0000-5000-8000-000000000001'.
- Around line 42-47: The header comment and combined constraint trigger
trg_min_one_admin are misleading and cause the expensive COUNT(*) check to run
on any UPDATE to an admin row; change the implementation so UPDATE and DELETE
are handled by separate constraint triggers on table users: create an
UPDATE-only trigger that uses a WHEN clause referencing OLD and NEW (e.g., WHEN
(OLD.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[] AND NOT
(NEW.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]))) to
only fire when admin membership is removed, keep a DELETE-only trigger that
fires when OLD.group_ids contains the admin id, both executing
check_min_one_admin(), and update the header comment to accurately describe this
behavior.
- Around line 24-47: The deferred trigger function check_min_one_admin() is
vulnerable to concurrent updates because it does an unlocked SELECT COUNT(*)—fix
by serializing access inside the function: instead of COUNT(*) use a SELECT FOR
UPDATE to lock the rows matching the Administrators group (or lock a dedicated
sentinel row) before counting, ensuring concurrent transactions cannot both see
>=1; update check_min_one_admin() to acquire that FOR UPDATE lock on users where
group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[] (or use a
sentinel lock table/row) and then re-evaluate the count, keeping the trigger
trg_min_one_admin as DEFERRABLE INITIALLY DEFERRED.

---

Nitpick comments:
In `@internal/auth/service_user_test.go`:
- Around line 388-456: The test currently claims to prove the DB-level race
closure but deterministically wires mockStore so admin-b always fails; rename
TestService_DeleteUser_ConcurrentLastTwoAdmins to
TestService_DeleteUser_ConcurrentErrorMapping (or similar), update the function
comment and assertion message to state it only verifies service-layer error
mapping under concurrent DeleteUser calls (not the DB trigger), and add a TODO
comment that an actual PostgreSQL-backed integration test should be added to
validate migration 000058 end-to-end; keep the existing mockStore setup and
assertions unchanged otherwise.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5736e9c-83ec-46ad-ba70-f46f5d78ddfb

📥 Commits

Reviewing files that changed from the base of the PR and between 1dbeef7 and ebf9af2.

📒 Files selected for processing (5)

• internal/auth/service_user.go
• internal/auth/service_user_test.go
• internal/auth/store_postgres.go
• internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql
• internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql

[cristim]

CR follow-up (commit 913e783)

All three findings on migration 000058_enforce_min_one_admin addressed, with the trigger approach kept but made genuinely race-safe rather than swapped for app-layer-only locking (the trigger covers every write path; an app-layer FOR UPDATE would only cover writes routed through UpdateUser/DeleteUser and still need a DB backstop).

Critical (concurrency): check_min_one_admin() now takes a transaction-scoped pg_advisory_xact_lock before the COUNT(*), serializing concurrent admin-affecting commits so the second waiter re-counts the post-commit state. Lock auto-releases at COMMIT/ROLLBACK.

Major (WHEN clause / heavy lift): split into separate trg_min_one_admin_delete and trg_min_one_admin_update triggers. The UPDATE trigger fires only when an active admin loses admin standing (OLD.active AND (NOT NEW admin OR NEW.active = false)), so login-path writes on an admin who stays active never pay the COUNT(*) cost. Comment rewritten.

Minor (active): COUNT now filters AND active = true to mirror AdminExists, so deactivating the last active admin is blocked too. Added an app-layer deactivation soft guard in UpdateUser and a concurrent-deactivation race regression test (WaitGroup + ready channel, require only on the test goroutine).

Migration stays 000058 (base tops out at 000057, no collision). down drops both new triggers plus the legacy single-trigger name.

Verification: go build ./... clean; go test -race ./internal/auth/... ./internal/api/... (1902 tests) green; migration package tests green; gofmt -l + go vet clean on touched files.

@coderabbitai review

[coderabbitai]

@cristim, thank you for the detailed follow-up — this is a thorough and well-reasoned resolution of all three findings:

• Advisory lock → serializes concurrent trigger executions at commit time so the second waiter always re-counts post-first-commit state; correct scope (xact-level, auto-releases).
• Trigger split → trg_min_one_admin_delete + trg_min_one_admin_update with the narrow WHEN condition means the hot admin-stays-active login path is entirely unaffected.
• active = true filter → properly mirrors AdminExists; good catch that deactivation is a semantically equivalent removal path.

Triggering a review of the updated commit now.

(⠀◕ᴗ◕⠀)🐇

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.