From 12be15136287bcfe705d38647690045daee2bbd3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 18:04:17 +0200 Subject: [PATCH 1/3] fix(auth): close last-admin TOCTOU with deferred DB trigger (closes #919) Add migration 000058 (deferred constraint trigger trg_min_one_admin) that enforces at least one Administrators-group member at commit time, making the count-and-mutate sequence atomic at the DB level. The existing application-level soft check in checkLastAdminConstraint and DeleteUser is retained as a fast early-exit that returns a user-friendly 409; the trigger is the hard race-free backstop that catches the narrow window where two concurrent requests both pass the soft check. isLastAdminConstraintViolation in store_postgres.go maps the P0001 trigger exception back to ErrLastAdmin so DeleteUser and UpdateUser surface the same sentinel regardless of which guard fires. Regression tests added: soft-check unit tests, trigger-simulation tests (plain error with sentinel prefix), and a concurrent goroutine test that runs both deletes simultaneously and asserts exactly one returns ErrLastAdmin. --- internal/auth/service_user.go | 19 +- internal/auth/service_user_test.go | 177 ++++++++++++++++++ internal/auth/store_postgres.go | 25 +++ .../000058_enforce_min_one_admin.down.sql | 4 + .../000058_enforce_min_one_admin.up.sql | 47 +++++ 5 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql create mode 100644 internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index dc581362c..3c70c632c 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -315,6 +315,13 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re } if err := s.store.UpdateUser(ctx, user); err != nil { + // The deferred DB trigger (migration 000058) fires at commit time and + // can reject writes that the application-level soft check missed due to + // concurrent requests. Surface the trigger violation as ErrLastAdmin so + // callers receive the same sentinel regardless of which guard caught it. + if isLastAdminConstraintViolation(err) { + return nil, ErrLastAdmin + } return nil, fmt.Errorf("failed to update user: %w", err) } @@ -430,7 +437,17 @@ func (s *Service) DeleteUser(ctx context.Context, userID string) error { logging.Warnf("Failed to delete user sessions: %v", err) } - return s.store.DeleteUser(ctx, userID) + if err := s.store.DeleteUser(ctx, userID); err != nil { + // The deferred DB trigger (migration 000058) fires at commit time and + // can reject deletes that the application-level soft check missed due + // to concurrent requests. Surface as ErrLastAdmin so the handler maps + // it to the same 409 regardless of which guard caught it. + if isLastAdminConstraintViolation(err) { + return ErrLastAdmin + } + return err + } + return nil } // GetUser returns user info. Returns (nil, pgx.ErrNoRows) if the user does not exist. diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index f8c110843..e813e6b8b 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sync" "testing" "time" @@ -329,6 +330,129 @@ func TestService_DeleteUser(t *testing.T) { mockStore.AssertExpectations(t) }) + + t.Run("blocks delete of last admin member (soft check)", func(t *testing.T) { + // Verifies that the application-level guard in DeleteUser returns + // ErrLastAdmin when CountGroupMembers reports only one admin remains. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + GroupIDs: []string{DefaultAdminGroupID}, + } + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(1, nil).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + err := service.DeleteUser(ctx, "admin-1") + assert.ErrorIs(t, err, ErrLastAdmin) + }) + + t.Run("maps DB trigger violation to ErrLastAdmin", func(t *testing.T) { + // Verifies that DeleteUser surfaces ErrLastAdmin when the deferred DB + // trigger (migration 000058) fires because a concurrent request already + // removed the other admin between the soft check and the DELETE. The + // soft check saw count >= 2 and passed, but the trigger rejects the + // commit. We simulate this by having the store return the raw pgconn + // error text that the real trigger would produce. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + GroupIDs: []string{DefaultAdminGroupID}, + } + // Soft check passes: two admins visible at read time. + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Once() + mockStore.On("DeleteUserSessions", ctx, "admin-1").Return(nil).Once() + // The DELETE statement triggers the deferred constraint at commit. We + // simulate this with a plain error whose text matches the trigger + // sentinel; the real DB would return a pgconn.PgError with code P0001. + // isLastAdminConstraintViolation falls back to a string check, so this + // plain error lets us exercise the mapping path in unit tests. + triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one member of the Administrators group must remain") + mockStore.On("DeleteUser", ctx, "admin-1").Return(triggerErr).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + err := service.DeleteUser(ctx, "admin-1") + assert.ErrorIs(t, err, ErrLastAdmin) + }) +} + +// TestService_DeleteUser_ConcurrentLastTwoAdmins is a regression test for the +// TOCTOU race described in issue #919. Two goroutines simultaneously attempt +// to delete the last two members of the Administrators group. Both pass the +// soft CountGroupMembers check (each sees count == 2). The DB-level deferred +// trigger (migration 000058) rejects one of the commits. We simulate the race +// by running both DeleteUser calls concurrently and configuring the mock so +// that the second DELETE returns the trigger violation error. +// +// Invariant asserted: at least one of the two deletes returns ErrLastAdmin, +// meaning the system never ends up with zero administrators. +func TestService_DeleteUser_ConcurrentLastTwoAdmins(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminA := &User{ID: "admin-a", GroupIDs: []string{DefaultAdminGroupID}} + adminB := &User{ID: "admin-b", GroupIDs: []string{DefaultAdminGroupID}} + + // Both goroutines read their respective user records and see count == 2. + mockStore.On("GetUserByID", ctx, "admin-a").Return(adminA, nil).Once() + mockStore.On("GetUserByID", ctx, "admin-b").Return(adminB, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Twice() + mockStore.On("DeleteUserSessions", ctx, "admin-a").Return(nil).Once() + mockStore.On("DeleteUserSessions", ctx, "admin-b").Return(nil).Once() + + // admin-a's delete succeeds; admin-b's hits the deferred constraint trigger. + mockStore.On("DeleteUser", ctx, "admin-a").Return(nil).Once() + triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one member of the Administrators group must remain") + mockStore.On("DeleteUser", ctx, "admin-b").Return(triggerErr).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + // Use a WaitGroup and a ready channel to maximise concurrency: both + // goroutines block at the barrier before calling DeleteUser. + ready := make(chan struct{}) + errCh := make(chan error, 2) + var wg sync.WaitGroup + wg.Add(2) + + start := func(userID string) { + defer wg.Done() + <-ready // synchronise start + errCh <- service.DeleteUser(ctx, userID) + } + + go start("admin-a") + go start("admin-b") + + close(ready) // release both goroutines simultaneously + wg.Wait() + close(errCh) + + var errs []error + for err := range errCh { + errs = append(errs, err) + } + + // Exactly one delete must return ErrLastAdmin; the other succeeds. + lastAdminErrors := 0 + for _, err := range errs { + if errors.Is(err, ErrLastAdmin) { + lastAdminErrors++ + } + } + assert.Equal(t, 1, lastAdminErrors, + "exactly one concurrent delete should be rejected with ErrLastAdmin; got errors: %v", errs) } func TestService_ListUsers(t *testing.T) { @@ -557,6 +681,59 @@ func TestService_UpdateUser(t *testing.T) { mockStore.AssertExpectations(t) }) + + t.Run("blocks removing admin group from last admin member (soft check)", func(t *testing.T) { + // guardGroupChange should return ErrLastAdmin when CountGroupMembers + // reports only one admin-group member and the update removes that group. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + Email: "admin@example.com", + GroupIDs: []string{DefaultAdminGroupID}, + } + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(1, nil).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + req := UpdateUserRequest{ + GroupIDs: []string{"other-group"}, + } + _, err := service.UpdateUser(ctx, "", "admin-1", req) + assert.ErrorIs(t, err, ErrLastAdmin) + }) + + t.Run("maps DB trigger violation to ErrLastAdmin on UpdateUser", func(t *testing.T) { + // Verifies that UpdateUser surfaces ErrLastAdmin when the deferred DB + // trigger fires because the soft check saw count >= 2 but a concurrent + // request removed the other admin before this transaction committed. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + Email: "admin@example.com", + GroupIDs: []string{DefaultAdminGroupID}, + } + // Soft check passes: two admins visible at read time. + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Once() + // UpdateUser hits the deferred trigger at commit time. + triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one member of the Administrators group must remain") + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(triggerErr).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + req := UpdateUserRequest{ + GroupIDs: []string{"other-group"}, + } + _, err := service.UpdateUser(ctx, "", "admin-1", req) + assert.ErrorIs(t, err, ErrLastAdmin) + }) } func TestService_CreateUser_EdgeCases(t *testing.T) { diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index efdf1bfca..61adfb272 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "strings" "time" "github.com/LeanerCloud/CUDly/pkg/logging" @@ -160,6 +161,30 @@ func isEmailDuplicateError(err error) bool { return false } +// isLastAdminConstraintViolation reports whether err is the deferred trigger +// exception from migration 000058 (trg_min_one_admin). The trigger uses +// RAISE EXCEPTION with a message prefixed by "last_admin_constraint_violation" +// and PostgreSQL error code P0001 (raise_exception). Detecting by message +// prefix rather than code alone avoids false positives from other user-raised +// exceptions in the same codebase. +// +// The function also handles plain errors whose message contains the sentinel +// prefix, which allows unit tests to simulate the trigger violation without a +// live PostgreSQL connection. +func isLastAdminConstraintViolation(err error) bool { + if err == nil { + return false + } + const sentinel = "last_admin_constraint_violation" + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) { + return pgErr.Code == "P0001" && strings.HasPrefix(pgErr.Message, sentinel) + } + // Fallback for unit-test stubs that return a plain error with the sentinel + // message (no pgconn available in mock-based tests). + return strings.HasPrefix(err.Error(), sentinel) +} + // UpdateUser updates an existing user func (s *PostgresStore) UpdateUser(ctx context.Context, user *User) error { user.UpdatedAt = time.Now() diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql b/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql new file mode 100644 index 000000000..f854b56f9 --- /dev/null +++ b/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql @@ -0,0 +1,4 @@ +-- Reverse of 000058: remove the deferred constraint trigger and its +-- supporting function that enforce at least one Administrators-group member. +DROP TRIGGER IF EXISTS trg_min_one_admin ON users; +DROP FUNCTION IF EXISTS check_min_one_admin(); diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql b/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql new file mode 100644 index 000000000..e28c1aa1a --- /dev/null +++ b/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql @@ -0,0 +1,47 @@ +-- Issue #919: close the TOCTOU window in the last-admin protection. +-- +-- The application layer (service_user.go) calls CountGroupMembers and then +-- writes in a separate statement. 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 guard, and leave zero admins. +-- +-- This migration adds a deferred constraint trigger that fires at COMMIT +-- time for writes that could reduce the Administrators-group membership. +-- If the group has zero members when the transaction commits, the trigger +-- raises an exception and rolls back the whole transaction. Combined with +-- the existing application-level soft check (which rejects early with a +-- user-friendly 409), the DB trigger is the hard, race-free backstop. +-- +-- The WHEN clause limits execution to rows that previously held the +-- Administrators-group UUID, so unrelated user writes (e.g. updating +-- last_login_at or failed_login_attempts) never pay the COUNT(*) cost. +-- +-- Constraint triggers must be FOR EACH ROW in PostgreSQL. DEFERRABLE +-- INITIALLY DEFERRED means the function runs at commit time, after all +-- writes in the transaction are visible, giving the correct post-commit +-- view of the admin count. + +CREATE OR REPLACE FUNCTION check_min_one_admin() +RETURNS TRIGGER LANGUAGE plpgsql AS $$ +DECLARE + admin_count INTEGER; +BEGIN + SELECT COUNT(*) INTO admin_count + FROM users + WHERE group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]; + + IF admin_count < 1 THEN + RAISE EXCEPTION 'last_admin_constraint_violation: at least one member of the Administrators group must remain'; + END IF; + + RETURN NULL; +END; +$$; + +DROP TRIGGER IF EXISTS trg_min_one_admin ON users; +CREATE CONSTRAINT TRIGGER trg_min_one_admin + AFTER UPDATE OR DELETE ON users + DEFERRABLE INITIALLY DEFERRED + FOR EACH ROW + WHEN (OLD.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]) + EXECUTE FUNCTION check_min_one_admin(); From 9fd7623eb96375b42be98084147a6f46918a22c5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 2 Jun 2026 19:53:33 +0200 Subject: [PATCH 2/3] fix(auth): make last-admin trigger race-safe, active-aware, and narrowly scoped CodeRabbit review on #921 found the deferred constraint trigger added for the #919 TOCTOU fix was not actually race-safe, ignored the active column, and fired on every admin-row UPDATE including the login hot path. Address all three: - Concurrency (critical): a deferred trigger with an unlocked COUNT(*) runs against each transaction's own MVCC snapshot, so two concurrent deletes / deactivations can both pass and leave zero admins. Take a transaction-scoped pg_advisory_xact_lock at the start of check_min_one_admin() so the checks serialize: the second waiter blocks until the first commits, then re-counts the post-commit state. The lock auto-releases at COMMIT/ROLLBACK. - Active semantics (minor): count only active members (AND active = true) so the trigger mirrors AdminExists. Deactivating the last admin (active=false with group_ids unchanged) is now blocked, not just deletes and group removal. - Trigger scope (major): split the single AFTER UPDATE OR DELETE trigger into a DELETE-only and an UPDATE-only constraint trigger. The UPDATE trigger's WHEN clause fires only when an active admin loses admin standing (drops the group or is deactivated), so login-path writes (last_login_at, failed_login_attempts, lockout) on an admin that stays active never pay the COUNT(*) cost. Comment rewritten to match. Add an application-level deactivation soft guard in UpdateUser mirroring the existing group-removal guard, so deactivating the last admin rejects early with ErrLastAdmin (friendly 409) instead of relying solely on the trigger. Tests: deactivation soft-block, allow-when-others-exist, trigger-violation mapping, and a concurrent-deactivation race regression (WaitGroup + ready channel, require only on the test goroutine). go test -race green. Migration stays 000058 (base tops out at 000057, no collision). down migration drops both new triggers plus the legacy single-trigger name. --- internal/auth/service_user.go | 28 +++- internal/auth/service_user_test.go | 144 ++++++++++++++++++ .../000058_enforce_min_one_admin.down.sql | 8 +- .../000058_enforce_min_one_admin.up.sql | 85 ++++++++--- 4 files changed, 239 insertions(+), 26 deletions(-) diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 3c70c632c..a1f536b49 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -290,9 +290,10 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re return nil, fmt.Errorf("user not found") } - // Snapshot the prior membership before mutating so the guards below can - // reason about what is being added/removed. + // Snapshot the prior membership/active state before mutating so the guards + // below can reason about what is being added/removed/deactivated. priorGroups := append([]string(nil), user.GroupIDs...) + priorActive := user.Active if err := applyUpdateUserRequest(user, req); err != nil { return nil, err @@ -304,6 +305,19 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re } } + // Deactivation guard: deactivating the last active Administrators-group + // member would lock the deployment out of admin functionality, the same + // hazard as removing the group. AdminExists and the 000058 trigger both + // count only active members, so this soft check mirrors that invariant and + // rejects early with a friendly 409. The deferred trigger is the race-free + // backstop when concurrent requests slip past this read-then-write check. + if priorActive && req.Active != nil && !*req.Active && + containsGroup(user.GroupIDs, DefaultAdminGroupID) { + if err := s.checkLastAdminConstraint(ctx); err != nil { + return nil, err + } + } + // Email is mutated through updateUserEmail rather than applyUpdateUserRequest // because it requires a DB lookup (uniqueness check) and format validation // (same rules as the self-edit profile path; see updateUserEmail and #868 @@ -362,10 +376,12 @@ func (s *Service) guardGroupChange(ctx context.Context, actorUserID, targetUserI return nil } -// checkLastAdminConstraint returns ErrLastAdmin if removing the -// Administrators group from its current holder would leave the group with -// zero members. Pulled out of guardGroupChange to keep that function under -// the cyclomatic limit. +// checkLastAdminConstraint returns ErrLastAdmin if removing or deactivating +// the Administrators group's current sole holder would leave the group with +// no other member to fall back on. Pulled out of guardGroupChange to keep that +// function under the cyclomatic limit and reused by the deactivation guard. +// This is a best-effort early reject; the 000058 deferred trigger is the +// race-free backstop that also accounts for already-inactive members. func (s *Service) checkLastAdminConstraint(ctx context.Context) error { count, err := s.store.CountGroupMembers(ctx, DefaultAdminGroupID) if err != nil { diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index e813e6b8b..8525cfb81 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -455,6 +455,69 @@ func TestService_DeleteUser_ConcurrentLastTwoAdmins(t *testing.T) { "exactly one concurrent delete should be rejected with ErrLastAdmin; got errors: %v", errs) } +// TestService_UpdateUser_ConcurrentDeactivateLastTwoAdmins is the deactivation +// analogue of the delete race in issue #919 / CR #921. Two goroutines +// simultaneously deactivate the last two active admins. Both read count == 2 +// and pass the soft check. The 000058 deferred trigger now counts only *active* +// members and serializes via an advisory xact lock, so exactly one commit is +// rejected. We simulate the DB outcome by having the second UpdateUser return +// the trigger violation, and assert the invariant: never zero active admins. +func TestService_UpdateUser_ConcurrentDeactivateLastTwoAdmins(t *testing.T) { + ctx := context.Background() + + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminA := &User{ID: "admin-a", Email: "a@example.com", GroupIDs: []string{DefaultAdminGroupID}, Active: true} + adminB := &User{ID: "admin-b", Email: "b@example.com", GroupIDs: []string{DefaultAdminGroupID}, Active: true} + + mockStore.On("GetUserByID", ctx, "admin-a").Return(adminA, nil).Once() + mockStore.On("GetUserByID", ctx, "admin-b").Return(adminB, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Twice() + + // admin-a's deactivation commits; admin-b's hits the deferred trigger. + mockStore.On("UpdateUser", ctx, mock.MatchedBy(func(u *User) bool { return u.ID == "admin-a" })).Return(nil).Once() + triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one active member of the Administrators group must remain") + mockStore.On("UpdateUser", ctx, mock.MatchedBy(func(u *User) bool { return u.ID == "admin-b" })).Return(triggerErr).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + ready := make(chan struct{}) + errCh := make(chan error, 2) + var wg sync.WaitGroup + wg.Add(2) + + start := func(userID string) { + defer wg.Done() + <-ready + inactive := false + _, err := service.UpdateUser(ctx, "", userID, UpdateUserRequest{Active: &inactive}) + errCh <- err + } + + go start("admin-a") + go start("admin-b") + + close(ready) + wg.Wait() + close(errCh) + + var errs []error + for err := range errCh { + errs = append(errs, err) + } + + lastAdminErrors := 0 + for _, err := range errs { + if errors.Is(err, ErrLastAdmin) { + lastAdminErrors++ + } + } + assert.Equal(t, 1, lastAdminErrors, + "exactly one concurrent deactivation should be rejected with ErrLastAdmin; got errors: %v", errs) +} + func TestService_ListUsers(t *testing.T) { ctx := context.Background() @@ -734,6 +797,87 @@ func TestService_UpdateUser(t *testing.T) { _, err := service.UpdateUser(ctx, "", "admin-1", req) assert.ErrorIs(t, err, ErrLastAdmin) }) + + t.Run("blocks deactivating last admin member (soft check)", func(t *testing.T) { + // Deactivating (active=false) the sole Administrators-group member must + // be rejected with ErrLastAdmin: it would leave zero active admins, the + // same lockout hazard as removing the group. The group is unchanged, so + // this exercises the deactivation guard, not guardGroupChange. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + Email: "admin@example.com", + GroupIDs: []string{DefaultAdminGroupID}, + Active: true, + } + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(1, nil).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + inactive := false + req := UpdateUserRequest{Active: &inactive} + _, err := service.UpdateUser(ctx, "", "admin-1", req) + assert.ErrorIs(t, err, ErrLastAdmin) + }) + + t.Run("allows deactivating an admin when others remain", func(t *testing.T) { + // With more than one admin-group member the deactivation guard passes + // and the update proceeds; the DB trigger would catch the concurrent + // edge case but the soft check sees count >= 2 and allows it. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + Email: "admin@example.com", + GroupIDs: []string{DefaultAdminGroupID}, + Active: true, + } + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Once() + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + inactive := false + req := UpdateUserRequest{Active: &inactive} + user, err := service.UpdateUser(ctx, "", "admin-1", req) + require.NoError(t, err) + assert.False(t, user.Active) + }) + + t.Run("maps DB trigger violation to ErrLastAdmin on deactivation", func(t *testing.T) { + // Concurrent deactivations: the soft check saw count >= 2 but a parallel + // request deactivated the other admin first. The deferred trigger (which + // counts only active members) rejects this commit; UpdateUser must map + // the violation to ErrLastAdmin so deactivation and delete behave alike. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + adminUser := &User{ + ID: "admin-1", + Email: "admin@example.com", + GroupIDs: []string{DefaultAdminGroupID}, + Active: true, + } + mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() + mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Once() + triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one active member of the Administrators group must remain") + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(triggerErr).Once() + + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + inactive := false + req := UpdateUserRequest{Active: &inactive} + _, err := service.UpdateUser(ctx, "", "admin-1", req) + assert.ErrorIs(t, err, ErrLastAdmin) + }) } func TestService_CreateUser_EdgeCases(t *testing.T) { diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql b/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql index f854b56f9..085aa650c 100644 --- a/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql +++ b/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql @@ -1,4 +1,8 @@ --- Reverse of 000058: remove the deferred constraint trigger and its --- supporting function that enforce at least one Administrators-group member. +-- Reverse of 000058: remove the deferred constraint triggers and their +-- supporting function that enforce at least one active Administrators-group +-- member. The legacy single-trigger name is dropped too so re-runs across the +-- up/down boundary stay clean. DROP TRIGGER IF EXISTS trg_min_one_admin ON users; +DROP TRIGGER IF EXISTS trg_min_one_admin_delete ON users; +DROP TRIGGER IF EXISTS trg_min_one_admin_update ON users; DROP FUNCTION IF EXISTS check_min_one_admin(); diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql b/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql index e28c1aa1a..f63f3fb9d 100644 --- a/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql +++ b/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql @@ -2,36 +2,63 @@ -- -- The application layer (service_user.go) calls CountGroupMembers and then -- writes in a separate statement. 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 guard, and leave zero admins. +-- removing/deactivating a different one of the last two admin-group members +-- can both observe count == 2, both pass the soft guard, and leave zero +-- active admins. -- --- This migration adds a deferred constraint trigger that fires at COMMIT --- time for writes that could reduce the Administrators-group membership. --- If the group has zero members when the transaction commits, the trigger --- raises an exception and rolls back the whole transaction. Combined with --- the existing application-level soft check (which rejects early with a --- user-friendly 409), the DB trigger is the hard, race-free backstop. +-- This migration adds deferred constraint triggers that fire at COMMIT time +-- for writes that could reduce the number of *active* Administrators-group +-- members. If the group would have zero active members when the transaction +-- commits, the trigger raises an exception and rolls back the whole +-- transaction. Combined with the existing application-level soft check (which +-- rejects early with a user-friendly 409), the DB triggers are the hard, +-- race-free backstop. -- --- The WHEN clause limits execution to rows that previously held the --- Administrators-group UUID, so unrelated user writes (e.g. updating --- last_login_at or failed_login_attempts) never pay the COUNT(*) cost. +-- Concurrency safety (CR #921, critical): a deferred trigger alone is NOT +-- race-free. Under READ COMMITTED/REPEATABLE READ, each transaction's +-- COUNT(*) runs against its own MVCC snapshot and cannot see a concurrent +-- transaction's uncommitted delete, so two transactions can each observe +-- count >= 1 and both commit, leaving zero admins. To serialize the check we +-- take a transaction-scoped advisory lock on a fixed key at the start of the +-- function. Concurrent admin-affecting commits then run the COUNT one at a +-- time: the second waiter blocks until the first commits (releasing the lock), +-- then re-counts and sees the post-commit state. The lock auto-releases at +-- COMMIT or ROLLBACK, so it cannot leak. +-- +-- Active semantics (CR #921, minor): the count mirrors AdminExists +-- (AND active = true), so deactivating the last admin (a plain UPDATE with +-- group_ids unchanged) is also blocked -- not just deletes and group removals. +-- +-- Trigger scope (CR #921, major): UPDATE and DELETE are handled by separate +-- triggers so the COUNT(*) only runs when an admin is actually losing admin +-- standing. The DELETE trigger fires when the deleted row was an admin. The +-- UPDATE trigger fires only when the row was an active admin and the new row +-- either drops the Administrators group or is deactivated -- so the +-- high-frequency login path (updating last_login_at / failed_login_attempts on +-- an admin that stays an active admin) never pays the COUNT(*) cost. -- -- Constraint triggers must be FOR EACH ROW in PostgreSQL. DEFERRABLE --- INITIALLY DEFERRED means the function runs at commit time, after all --- writes in the transaction are visible, giving the correct post-commit --- view of the admin count. +-- INITIALLY DEFERRED means the function runs at commit time, after all writes +-- in the transaction are visible, giving the correct post-commit view. CREATE OR REPLACE FUNCTION check_min_one_admin() RETURNS TRIGGER LANGUAGE plpgsql AS $$ DECLARE admin_count INTEGER; BEGIN + -- Serialize the count across concurrent admin-affecting transactions. + -- Without this, two deferred checks run against independent MVCC + -- snapshots and both can pass while jointly removing the last admins. + -- The key is an arbitrary fixed constant scoped to this invariant. + PERFORM pg_advisory_xact_lock(8059058058580001); + SELECT COUNT(*) INTO admin_count FROM users - WHERE group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]; + WHERE group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[] + AND active = true; IF admin_count < 1 THEN - RAISE EXCEPTION 'last_admin_constraint_violation: at least one member of the Administrators group must remain'; + RAISE EXCEPTION 'last_admin_constraint_violation: at least one active member of the Administrators group must remain'; END IF; RETURN NULL; @@ -39,9 +66,31 @@ END; $$; DROP TRIGGER IF EXISTS trg_min_one_admin ON users; -CREATE CONSTRAINT TRIGGER trg_min_one_admin - AFTER UPDATE OR DELETE ON users +DROP TRIGGER IF EXISTS trg_min_one_admin_delete ON users; +DROP TRIGGER IF EXISTS trg_min_one_admin_update ON users; + +-- DELETE: fire when the removed row was an Administrators-group member. +CREATE CONSTRAINT TRIGGER trg_min_one_admin_delete + AFTER DELETE ON users DEFERRABLE INITIALLY DEFERRED FOR EACH ROW WHEN (OLD.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]) EXECUTE FUNCTION check_min_one_admin(); + +-- UPDATE: fire only when an active admin loses admin standing, i.e. the old +-- row was an active admin and the new row either drops the Administrators +-- group or is no longer active. Updates that keep an admin active and in the +-- group (e.g. last_login_at / failed_login_attempts) do not fire. +CREATE CONSTRAINT TRIGGER trg_min_one_admin_update + AFTER UPDATE ON users + DEFERRABLE INITIALLY DEFERRED + FOR EACH ROW + WHEN ( + OLD.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[] + AND OLD.active = true + AND ( + NOT (NEW.group_ids @> ARRAY['00000000-0000-5000-8000-000000000001']::UUID[]) + OR NEW.active = false + ) + ) + EXECUTE FUNCTION check_min_one_admin(); From 435320d5bba8c12b19b86a0e6cea839922815cf4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 13:57:14 +0200 Subject: [PATCH 3/3] fix(auth): renumber migration, cut UpdateUser complexity, add DB-level last-admin race test (refs #919) Adversarial review of PR #921 surfaced three blockers; all three are addressed here on top of the rebased branch. 1. Migration number collision. The branch forked before the base gained migrations 000058..000064, so its 000058_enforce_min_one_admin pair collided with the base's 000058_purchase_executions_direct_execute_audit and would sort below already-applied migrations on merge, breaking golang-migrate. Rebased onto feat/multicloud-web-frontend and renumbered the migration to 000065 (1 + the highest existing number) via git mv for both the up and down SQL. Updated the migration-number references in the comments across service_user.go, store_postgres.go, the down SQL, and the service tests to match. 2. gocyclo on UpdateUser. UpdateUser was at cyclomatic complexity 16 (limit 10) after the deactivation guard landed. Extracted the deactivation guard into guardDeactivation and the shared lookup-or-not-found block into loadUser (also reused by DeleteUser), mirroring the existing checkLastAdminConstraint extraction style. UpdateUser is now 9. No behaviour change. 3. The fix was untested at the DB level. The existing tests are pure mocks that hardcode the trigger error and would pass even if the SQL were wrong. Added a testcontainers-backed integration test (000065_enforce_min_one_admin_test.go) that applies the real migrations and, with a commit barrier, runs two concurrent transactions each stripping admin standing from a different one of two active admins (deactivate / delete / group-removal). It asserts the invariant issue #919 actually requires: the deferred trigger never lets both commit and at least one active admin always remains. Verified the concurrent test FAILS when the trigger SQL is stubbed out and PASSES with it in place. A second test pins the single-transaction guarantee (rejecting removal of the sole admin, allowing removal of one of two). --- internal/auth/service_user.go | 72 +++--- internal/auth/service_user_test.go | 6 +- internal/auth/store_postgres.go | 2 +- ... => 000065_enforce_min_one_admin.down.sql} | 2 +- ...ql => 000065_enforce_min_one_admin.up.sql} | 0 .../000065_enforce_min_one_admin_test.go | 222 ++++++++++++++++++ 6 files changed, 271 insertions(+), 33 deletions(-) rename internal/database/postgres/migrations/{000058_enforce_min_one_admin.down.sql => 000065_enforce_min_one_admin.down.sql} (86%) rename internal/database/postgres/migrations/{000058_enforce_min_one_admin.up.sql => 000065_enforce_min_one_admin.up.sql} (100%) create mode 100644 internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index a1f536b49..28eefb64f 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -271,6 +271,24 @@ func (s *Service) sendInviteEmail(ctx context.Context, user *User, inviteToken s logging.Infof("User invited: id=%s, groups=%d, email_sent=%t", user.ID, len(user.GroupIDs), sent) } +// loadUser fetches a user by ID, normalising a missing row (either pgx.ErrNoRows +// or a nil user) into a uniform "user not found" error. Extracted so the mutating +// service methods share one lookup-or-not-found path and stay under gocyclo's +// complexity threshold. +func (s *Service) loadUser(ctx context.Context, userID string) (*User, error) { + user, err := s.store.GetUserByID(ctx, userID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, fmt.Errorf("user not found") + } + return nil, err + } + if user == nil { + return nil, fmt.Errorf("user not found") + } + return user, nil +} + // UpdateUser updates user details (requires manage-users permission). // // actorUserID is the authenticated user performing the change (from the @@ -279,16 +297,10 @@ func (s *Service) sendInviteEmail(ctx context.Context, user *User, inviteToken s // they hold the manage-users permission. Pass "" for trusted internal callers // (e.g. the stateless admin API key) that have already been authorised. func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, req UpdateUserRequest) (*User, error) { - user, err := s.store.GetUserByID(ctx, userID) + user, err := s.loadUser(ctx, userID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, fmt.Errorf("user not found") - } return nil, err } - if user == nil { - return nil, fmt.Errorf("user not found") - } // Snapshot the prior membership/active state before mutating so the guards // below can reason about what is being added/removed/deactivated. @@ -305,17 +317,8 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re } } - // Deactivation guard: deactivating the last active Administrators-group - // member would lock the deployment out of admin functionality, the same - // hazard as removing the group. AdminExists and the 000058 trigger both - // count only active members, so this soft check mirrors that invariant and - // rejects early with a friendly 409. The deferred trigger is the race-free - // backstop when concurrent requests slip past this read-then-write check. - if priorActive && req.Active != nil && !*req.Active && - containsGroup(user.GroupIDs, DefaultAdminGroupID) { - if err := s.checkLastAdminConstraint(ctx); err != nil { - return nil, err - } + if err := s.guardDeactivation(ctx, user, priorActive, req.Active); err != nil { + return nil, err } // Email is mutated through updateUserEmail rather than applyUpdateUserRequest @@ -329,7 +332,7 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re } if err := s.store.UpdateUser(ctx, user); err != nil { - // The deferred DB trigger (migration 000058) fires at commit time and + // The deferred DB trigger (migration 000065) fires at commit time and // can reject writes that the application-level soft check missed due to // concurrent requests. Surface the trigger violation as ErrLastAdmin so // callers receive the same sentinel regardless of which guard caught it. @@ -342,6 +345,25 @@ func (s *Service) UpdateUser(ctx context.Context, actorUserID, userID string, re return user, nil } +// guardDeactivation rejects deactivating the last active Administrators-group +// member, which would lock the deployment out of admin functionality, the same +// hazard as removing the group. AdminExists and the 000065 trigger both count +// only active members, so this soft check mirrors that invariant and rejects +// early with a friendly 409. The deferred trigger is the race-free backstop +// when concurrent requests slip past this read-then-write check. Extracted from +// UpdateUser to keep that function under gocyclo's complexity threshold. +// +// user carries the post-applyUpdateUserRequest group membership; priorActive is +// the active state before the update and reqActive the requested change (nil +// when the request does not touch Active). +func (s *Service) guardDeactivation(ctx context.Context, user *User, priorActive bool, reqActive *bool) error { + deactivating := priorActive && reqActive != nil && !*reqActive + if deactivating && containsGroup(user.GroupIDs, DefaultAdminGroupID) { + return s.checkLastAdminConstraint(ctx) + } + return nil +} + // guardGroupChange enforces the issue #907 invariants for a group-membership // change: at least one group remains, the last Administrators-group member is // not removed, and a non-privileged actor cannot escalate their own access. @@ -380,7 +402,7 @@ func (s *Service) guardGroupChange(ctx context.Context, actorUserID, targetUserI // the Administrators group's current sole holder would leave the group with // no other member to fall back on. Pulled out of guardGroupChange to keep that // function under the cyclomatic limit and reused by the deactivation guard. -// This is a best-effort early reject; the 000058 deferred trigger is the +// This is a best-effort early reject; the 000065 deferred trigger is the // race-free backstop that also accounts for already-inactive members. func (s *Service) checkLastAdminConstraint(ctx context.Context) error { count, err := s.store.CountGroupMembers(ctx, DefaultAdminGroupID) @@ -427,16 +449,10 @@ func applyUpdateUserRequest(user *User, req UpdateUserRequest) error { // delete the last remaining Administrators-group member so the deployment can // never be locked out of admin functionality (issue #907). func (s *Service) DeleteUser(ctx context.Context, userID string) error { - user, err := s.store.GetUserByID(ctx, userID) + user, err := s.loadUser(ctx, userID) if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return fmt.Errorf("user not found") - } return err } - if user == nil { - return fmt.Errorf("user not found") - } if containsGroup(user.GroupIDs, DefaultAdminGroupID) { count, err := s.store.CountGroupMembers(ctx, DefaultAdminGroupID) @@ -454,7 +470,7 @@ func (s *Service) DeleteUser(ctx context.Context, userID string) error { } if err := s.store.DeleteUser(ctx, userID); err != nil { - // The deferred DB trigger (migration 000058) fires at commit time and + // The deferred DB trigger (migration 000065) fires at commit time and // can reject deletes that the application-level soft check missed due // to concurrent requests. Surface as ErrLastAdmin so the handler maps // it to the same 409 regardless of which guard caught it. diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index 8525cfb81..7afa7f755 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -353,7 +353,7 @@ func TestService_DeleteUser(t *testing.T) { t.Run("maps DB trigger violation to ErrLastAdmin", func(t *testing.T) { // Verifies that DeleteUser surfaces ErrLastAdmin when the deferred DB - // trigger (migration 000058) fires because a concurrent request already + // trigger (migration 000065) fires because a concurrent request already // removed the other admin between the soft check and the DELETE. The // soft check saw count >= 2 and passed, but the trigger rejects the // commit. We simulate this by having the store return the raw pgconn @@ -389,7 +389,7 @@ func TestService_DeleteUser(t *testing.T) { // TOCTOU race described in issue #919. Two goroutines simultaneously attempt // to delete the last two members of the Administrators group. Both pass the // soft CountGroupMembers check (each sees count == 2). The DB-level deferred -// trigger (migration 000058) rejects one of the commits. We simulate the race +// trigger (migration 000065) rejects one of the commits. We simulate the race // by running both DeleteUser calls concurrently and configuring the mock so // that the second DELETE returns the trigger violation error. // @@ -458,7 +458,7 @@ func TestService_DeleteUser_ConcurrentLastTwoAdmins(t *testing.T) { // TestService_UpdateUser_ConcurrentDeactivateLastTwoAdmins is the deactivation // analogue of the delete race in issue #919 / CR #921. Two goroutines // simultaneously deactivate the last two active admins. Both read count == 2 -// and pass the soft check. The 000058 deferred trigger now counts only *active* +// and pass the soft check. The 000065 deferred trigger now counts only *active* // members and serializes via an advisory xact lock, so exactly one commit is // rejected. We simulate the DB outcome by having the second UpdateUser return // the trigger violation, and assert the invariant: never zero active admins. diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 61adfb272..e8c54ecde 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -162,7 +162,7 @@ func isEmailDuplicateError(err error) bool { } // isLastAdminConstraintViolation reports whether err is the deferred trigger -// exception from migration 000058 (trg_min_one_admin). The trigger uses +// exception from migration 000065 (trg_min_one_admin). The trigger uses // RAISE EXCEPTION with a message prefixed by "last_admin_constraint_violation" // and PostgreSQL error code P0001 (raise_exception). Detecting by message // prefix rather than code alone avoids false positives from other user-raised diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql b/internal/database/postgres/migrations/000065_enforce_min_one_admin.down.sql similarity index 86% rename from internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql rename to internal/database/postgres/migrations/000065_enforce_min_one_admin.down.sql index 085aa650c..cf1a14467 100644 --- a/internal/database/postgres/migrations/000058_enforce_min_one_admin.down.sql +++ b/internal/database/postgres/migrations/000065_enforce_min_one_admin.down.sql @@ -1,4 +1,4 @@ --- Reverse of 000058: remove the deferred constraint triggers and their +-- Reverse of 000065: remove the deferred constraint triggers and their -- supporting function that enforce at least one active Administrators-group -- member. The legacy single-trigger name is dropped too so re-runs across the -- up/down boundary stay clean. diff --git a/internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql b/internal/database/postgres/migrations/000065_enforce_min_one_admin.up.sql similarity index 100% rename from internal/database/postgres/migrations/000058_enforce_min_one_admin.up.sql rename to internal/database/postgres/migrations/000065_enforce_min_one_admin.up.sql diff --git a/internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go b/internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go new file mode 100644 index 000000000..1b2e9668a --- /dev/null +++ b/internal/database/postgres/migrations/000065_enforce_min_one_admin_test.go @@ -0,0 +1,222 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "sync" + "testing" + + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // adminGroupIDForMinAdminTest mirrors DefaultAdminGroupID / the UUID baked + // into the 000065 trigger. Copied here so the test is self-contained without + // depending on declaration order across test files in the same package. + adminGroupIDForMinAdminTest = "00000000-0000-5000-8000-000000000001" +) + +// seedActiveAdmin inserts an active user that is a member of the Administrators +// group and returns its UUID. +func seedActiveAdmin(t *testing.T, ctx context.Context, pool *pgxpool.Pool, email string) string { + t.Helper() + var id string + err := pool.QueryRow(ctx, ` + INSERT INTO users (id, email, password_hash, salt, active, group_ids, created_at, updated_at) + VALUES (gen_random_uuid(), $1, '', '', true, ARRAY[$2::uuid], NOW(), NOW()) + RETURNING id + `, email, adminGroupIDForMinAdminTest).Scan(&id) + require.NoError(t, err, "seeding admin %s must succeed", email) + return id +} + +// countActiveAdmins returns the number of active members of the Administrators +// group, the exact invariant the 000065 trigger protects. +func countActiveAdmins(t *testing.T, ctx context.Context, pool *pgxpool.Pool) int { + t.Helper() + var n int + err := pool.QueryRow(ctx, ` + SELECT COUNT(*) FROM users + WHERE group_ids @> ARRAY[$1::uuid] AND active = true + `, adminGroupIDForMinAdminTest).Scan(&n) + require.NoError(t, err) + return n +} + +// TestMigration_EnforceMinOneAdmin_ConcurrentRace covers issue #919: the real +// TOCTOU race that the deferred constraint triggers + pg_advisory_xact_lock in +// migration 000065 are meant to close. Two concurrent transactions each remove +// admin standing from a DIFFERENT one of the last two active admins. Without +// the advisory lock, each deferred check runs against its own MVCC snapshot, +// both observe count >= 1, and both commit -- leaving zero active admins. The +// lock serializes the count at commit time so exactly one commits and at least +// one active admin always remains. +// +// Unlike the unit tests in service_user_test.go (which hardcode the trigger +// error string and would pass even if the SQL were wrong), this test runs the +// actual migration SQL against a real PostgreSQL instance. +func TestMigration_EnforceMinOneAdmin_ConcurrentRace(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + // runConcurrentRace seeds exactly two active admins, then runs two + // concurrent transactions that each strip admin standing from a DIFFERENT + // admin using the supplied mutateSQL (parameterised by the target user ID). + // A commit barrier holds both transactions at COMMIT until both have done + // their write, which is what exercises the deferred-trigger + advisory-lock + // arbitration. It returns how many of the two transactions committed and the + // number of active admins left afterwards. + runConcurrentRace := func(t *testing.T, mutateSQL string) (commits, remainingAdmins int) { + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + + adminA := seedActiveAdmin(t, ctx, pool, "race-admin-a@test.example") + adminB := seedActiveAdmin(t, ctx, pool, "race-admin-b@test.example") + require.Equal(t, 2, countActiveAdmins(t, ctx, pool), "test setup: two active admins expected") + + // Release both goroutines into their COMMIT only after both have + // completed their mutating statement, maximising the overlap the + // deferred triggers must arbitrate. + var writesReady sync.WaitGroup + writesReady.Add(2) + commitGate := make(chan struct{}) + + results := make([]error, 2) + targets := []string{adminA, adminB} + var wg sync.WaitGroup + wg.Add(2) + + for i := 0; i < 2; i++ { + go func(idx int) { + defer wg.Done() + tx, err := pool.Begin(ctx) + if err != nil { + results[idx] = err + writesReady.Done() + return + } + if _, err := tx.Exec(ctx, mutateSQL, targets[idx]); err != nil { + _ = tx.Rollback(ctx) + results[idx] = err + writesReady.Done() + return + } + // Signal this write is done, then wait for both before committing. + writesReady.Done() + <-commitGate + results[idx] = tx.Commit(ctx) + }(i) + } + + writesReady.Wait() + close(commitGate) + wg.Wait() + + for _, err := range results { + if err == nil { + commits++ + } + } + return commits, countActiveAdmins(t, ctx, pool) + } + + // assertRaceSafe encodes the invariant issue #919 actually demands and that + // the trigger guarantees: the deferred check must never let BOTH admin- + // stripping transactions through (commits <= 1), and at least one active + // admin must always survive (remainingAdmins >= 1). Asserting an exact + // commits == 1 would be wrong: depending on commit interleaving the advisory + // lock can legitimately reject BOTH transactions (commits == 0, two admins + // untouched), which is still safe. The bug this guards against is the + // pre-trigger behaviour where both committed and zero admins remained. + assertRaceSafe := func(t *testing.T, commits, remainingAdmins int, op string) { + t.Helper() + assert.LessOrEqual(t, commits, 1, + "the deferred trigger must never let both concurrent %s commit (would leave zero admins)", op) + assert.GreaterOrEqual(t, remainingAdmins, 1, + "at least one active admin must remain after the concurrent %s race", op) + } + + t.Run("concurrent deactivation never removes the last admin", func(t *testing.T) { + commits, remaining := runConcurrentRace(t, `UPDATE users SET active = false WHERE id = $1`) + assertRaceSafe(t, commits, remaining, "deactivations") + }) + + t.Run("concurrent deletion never removes the last admin", func(t *testing.T) { + commits, remaining := runConcurrentRace(t, `DELETE FROM users WHERE id = $1`) + assertRaceSafe(t, commits, remaining, "deletions") + }) + + t.Run("concurrent group removal never removes the last admin", func(t *testing.T) { + commits, remaining := runConcurrentRace(t, `UPDATE users SET group_ids = '{}'::uuid[] WHERE id = $1`) + assertRaceSafe(t, commits, remaining, "group removals") + }) +} + +// TestMigration_EnforceMinOneAdmin_BlocksLastAdminRemoval verifies the trigger's +// single-transaction guarantee: removing the only remaining active admin must +// fail at COMMIT with the last_admin_constraint_violation exception, while a +// removal that leaves another active admin must succeed. This pins the +// trigger's COUNT/active semantics against the real migration SQL. +func TestMigration_EnforceMinOneAdmin_BlocksLastAdminRemoval(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + // freshPool spins up an isolated migrated database so each subtest starts + // from a clean admin count. + freshPool := func(t *testing.T) *pgxpool.Pool { + t.Helper() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(ctx) }) + pool := container.DB.Pool() + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, "", "")) + return pool + } + + t.Run("deactivating the sole admin is rejected", func(t *testing.T) { + pool := freshPool(t) + id := seedActiveAdmin(t, ctx, pool, "sole-admin@test.example") + require.Equal(t, 1, countActiveAdmins(t, ctx, pool)) + + _, err := pool.Exec(ctx, `UPDATE users SET active = false WHERE id = $1`, id) + require.Error(t, err, "deactivating the last active admin must be rejected at commit") + assert.Contains(t, err.Error(), "last_admin_constraint_violation", + "the rejection must come from the 000065 trigger") + assert.Equal(t, 1, countActiveAdmins(t, ctx, pool), + "the rejected transaction must roll back, leaving the admin active") + }) + + t.Run("deleting the sole admin is rejected", func(t *testing.T) { + pool := freshPool(t) + id := seedActiveAdmin(t, ctx, pool, "sole-admin-del@test.example") + require.Equal(t, 1, countActiveAdmins(t, ctx, pool)) + + _, err := pool.Exec(ctx, `DELETE FROM users WHERE id = $1`, id) + require.Error(t, err, "deleting the last active admin must be rejected at commit") + assert.Contains(t, err.Error(), "last_admin_constraint_violation", + "the rejection must come from the 000065 trigger") + assert.Equal(t, 1, countActiveAdmins(t, ctx, pool), + "the rejected transaction must roll back, leaving the admin active") + }) + + t.Run("removing one of two admins succeeds", func(t *testing.T) { + pool := freshPool(t) + a := seedActiveAdmin(t, ctx, pool, "two-admins-a@test.example") + _ = seedActiveAdmin(t, ctx, pool, "two-admins-b@test.example") + require.Equal(t, 2, countActiveAdmins(t, ctx, pool)) + + _, err := pool.Exec(ctx, `UPDATE users SET active = false WHERE id = $1`, a) + require.NoError(t, err, "deactivating one of two admins must succeed") + assert.Equal(t, 1, countActiveAdmins(t, ctx, pool)) + }) +}