From cad1a4b6a7e8d036e959fc970bda5239119ad95c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 14:22:43 +0100 Subject: [PATCH] fix(auth): take the admin invariant lock before the row on demote, deactivate and delete UpdateUser, UpdateUserAdminFields and DeleteUser now run in a transaction whose first statement takes adminInvariantAdvisoryLockKey, the same point CreateAdminIfNone takes it, so every write that can lower the active-admin count orders the advisory lock before the users row lock. Before, these writes locked the row first and only reached the lock in the deferred min-one-admin trigger at commit. The trigger already kept the floor, so this is defense in depth: a mix of the two orders would deadlock, and the new real-Postgres deadlock test catches a partial conversion (40P01 in round 0). Closes #63 Co-Authored-By: Claude Opus 5.5 --- internal/auth/store_postgres.go | 127 ++++--- .../auth/store_postgres_admin_lock_db_test.go | 349 ++++++++++++++++++ .../auth/store_postgres_admin_lock_test.go | 239 ++++++++++++ internal/auth/store_postgres_credentials.go | 29 +- internal/auth/store_postgres_test.go | 110 ------ 5 files changed, 689 insertions(+), 165 deletions(-) create mode 100644 internal/auth/store_postgres_admin_lock_db_test.go create mode 100644 internal/auth/store_postgres_admin_lock_test.go diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 052ac8131..1253f4b75 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -186,7 +186,8 @@ func isLastAdminConstraintViolation(err error) bool { return strings.HasPrefix(err.Error(), sentinel) } -// UpdateUser updates an existing user. +// UpdateUser updates an existing user. It rewrites group_ids and active, so it +// takes the admin-invariant lock first (see adminInvariantAdvisoryLockKey). func (s *PostgresStore) UpdateUser(ctx context.Context, user *User) error { user.UpdatedAt = time.Now() @@ -219,53 +220,54 @@ func (s *PostgresStore) UpdateUser(ctx context.Context, user *User) error { recoveryCodes = []string{} } - result, err := s.db.Exec(ctx, query, - user.ID, - user.Email, - user.PasswordHash, - user.Salt, - user.GroupIDs, - user.Active, - user.MFAEnabled, - user.MFASecret, - user.MFAPendingSecret, - user.MFAPendingSecretExpiresAt, - recoveryCodes, - user.PasswordResetToken, - user.PasswordResetExpiry, - user.FailedLoginAttempts, - user.LockedUntil, - user.PasswordHistory, - user.UpdatedAt, - user.LastLoginAt, - user.DeactivatedAt, - ) - - if err != nil { - return fmt.Errorf("failed to update user: %w", err) - } - - if result.RowsAffected() == 0 { - return fmt.Errorf("user not found: %s", user.ID) - } - - return nil + return s.withAdminInvariantLock(ctx, "update user", func(tx pgx.Tx) error { + result, err := tx.Exec(ctx, query, + user.ID, + user.Email, + user.PasswordHash, + user.Salt, + user.GroupIDs, + user.Active, + user.MFAEnabled, + user.MFASecret, + user.MFAPendingSecret, + user.MFAPendingSecretExpiresAt, + recoveryCodes, + user.PasswordResetToken, + user.PasswordResetExpiry, + user.FailedLoginAttempts, + user.LockedUntil, + user.PasswordHistory, + user.UpdatedAt, + user.LastLoginAt, + user.DeactivatedAt, + ) + if err != nil { + return fmt.Errorf("failed to update user: %w", err) + } + if result.RowsAffected() == 0 { + return fmt.Errorf("user not found: %s", user.ID) + } + return nil + }) } -// DeleteUser deletes a user. +// DeleteUser deletes a user. Deleting an Administrators-group member lowers +// the admin count, so it takes the admin-invariant lock first (see +// adminInvariantAdvisoryLockKey). func (s *PostgresStore) DeleteUser(ctx context.Context, userID string) error { query := `DELETE FROM users WHERE id = $1` - result, err := s.db.Exec(ctx, query, userID) - if err != nil { - return fmt.Errorf("failed to delete user: %w", err) - } - - if result.RowsAffected() == 0 { - return fmt.Errorf("user not found: %s", userID) - } - - return nil + return s.withAdminInvariantLock(ctx, "delete user", func(tx pgx.Tx) error { + result, err := tx.Exec(ctx, query, userID) + if err != nil { + return fmt.Errorf("failed to delete user: %w", err) + } + if result.RowsAffected() == 0 { + return fmt.Errorf("user not found: %s", userID) + } + return nil + }) } // ListUsers lists all users. @@ -362,8 +364,47 @@ func (s *PostgresStore) CountGroupMembers(ctx context.Context, groupID string) ( // check_min_one_admin() in migration 000065 so bootstrap inserts and // admin-demoting commits serialize against each other as well as among // themselves. +// +// Convention (issue #63): any transaction that may demote, deactivate or +// delete an Administrators-group member, or create the first admin, must +// acquire this lock as its first statement, before it locks any users row. +// Use withAdminInvariantLock. The deferred trigger takes the same key again at +// commit, which is then a no-op re-acquire. A write that locked a users row +// first and only reached this lock in the trigger would hold the row while +// waiting for the lock, and deadlock against a writer holding the lock while +// waiting for that row. Writes that cannot take admin standing away (login +// counters, MFA and password columns) do not take the lock; they only ever +// hold the row lock, so they cannot complete such a cycle. const adminInvariantAdvisoryLockKey = 8059058058580001 +// withAdminInvariantLock runs fn in a transaction whose first statement takes +// adminInvariantAdvisoryLockKey, then commits. The min-one-admin trigger runs +// at that commit, so its last_admin_constraint_violation comes back wrapped +// from here. op names the write in begin, lock and commit errors; errors from +// fn are returned unchanged. The lock auto-releases at COMMIT or ROLLBACK. +func (s *PostgresStore) withAdminInvariantLock(ctx context.Context, op string, fn func(tx pgx.Tx) error) error { + tx, err := s.db.Begin(ctx) + if err != nil { + return fmt.Errorf("failed to %s: begin transaction: %w", op, err) + } + defer func() { + if rbErr := tx.Rollback(ctx); rbErr != nil { + _ = rbErr // no-op after Commit; Postgres logs real rollback failures + } + }() + + if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", adminInvariantAdvisoryLockKey); err != nil { + return fmt.Errorf("failed to %s: acquire admin invariant lock: %w", op, err) + } + if err := fn(tx); err != nil { + return err + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("failed to %s: commit: %w", op, err) + } + return nil +} + // CreateAdminIfNone atomically inserts user as the first admin in the // system. Returns (true, nil) when the insert succeeded; (false, nil) // when an admin already existed (TOCTOU race — both callers passed diff --git a/internal/auth/store_postgres_admin_lock_db_test.go b/internal/auth/store_postgres_admin_lock_db_test.go new file mode 100644 index 000000000..547b1eca8 --- /dev/null +++ b/internal/auth/store_postgres_admin_lock_db_test.go @@ -0,0 +1,349 @@ +//go:build integration + +package auth + +// store_postgres_admin_lock_db_test.go - real-PostgreSQL tests for the +// admin-invariant advisory lock ordering (issue #63). Every write that can +// lower the active-admin count must take adminInvariantAdvisoryLockKey as the +// first statement of its transaction, before any users row lock, the same as +// CreateAdminIfNone. The min-one-admin deferred trigger (migration 000065) +// takes the same key again at commit, which is then a no-op re-acquire. + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" +) + +// adminDemotingWrite is one store-level write that can lower the active-admin +// count. target is the row as the caller read it. +type adminDemotingWrite struct { + name string + run func(ctx context.Context, store *PostgresStore, target *User) error +} + +func adminDemotingWrites() []adminDemotingWrite { + return []adminDemotingWrite{ + {"UpdateUserAdminFields deactivate", func(ctx context.Context, store *PostgresStore, target *User) error { + next := *target + now := time.Now() + next.Active, next.DeactivatedAt = false, &now + return store.UpdateUserAdminFields(ctx, &next, target.Email, target.GroupIDs, target.Active) + }}, + {"UpdateUserAdminFields drop Administrators group", func(ctx context.Context, store *PostgresStore, target *User) error { + next := *target + next.GroupIDs = []string{DefaultPurchaserGroupID} + return store.UpdateUserAdminFields(ctx, &next, target.Email, target.GroupIDs, target.Active) + }}, + {"UpdateUser deactivate", func(ctx context.Context, store *PostgresStore, target *User) error { + next := *target + next.Active = false + return store.UpdateUser(ctx, &next) + }}, + {"DeleteUser", func(ctx context.Context, store *PostgresStore, target *User) error { + return store.DeleteUser(ctx, target.ID) + }}, + } +} + +// seedActiveAdmins inserts n active Administrators-group members. CreateUser +// is a plain INSERT, which the min-one-admin triggers do not watch. +func seedActiveAdmins(t *testing.T, store *PostgresStore, prefix string, n int) []*User { + t.Helper() + admins := make([]*User, n) + for i := range admins { + u := newBootstrapCandidate(fmt.Sprintf("%s-%d@example.com", prefix, i)) + u.GroupIDs = []string{DefaultAdminGroupID} + require.NoError(t, store.CreateUser(context.Background(), u)) + admins[i] = u + } + return admins +} + +func countActiveAdmins(t *testing.T, db DBConnection) int { + t.Helper() + var n int + require.NoError(t, db.QueryRow(context.Background(), + `SELECT COUNT(*) FROM users WHERE `+adminGroupContainsClause+` AND active = true`, + DefaultAdminGroupID).Scan(&n)) + return n +} + +// waitForAdminLockWaiter blocks until some backend is queued on the +// admin-invariant advisory lock. A bigint advisory key is reported in +// pg_locks split into classid (high 32 bits) and objid (low 32 bits). +func waitForAdminLockWaiter(t *testing.T, db DBConnection) { + t.Helper() + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + var waiting int + require.NoError(t, db.QueryRow(context.Background(), ` + SELECT COUNT(*) FROM pg_locks + WHERE locktype = 'advisory' AND NOT granted + AND ((classid::bigint << 32) | objid::bigint) = $1`, + int64(adminInvariantAdvisoryLockKey)).Scan(&waiting)) + if waiting > 0 { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatal("no backend queued on the admin-invariant advisory lock") +} + +// TestIntegration_AdminDemotingWrites_LockBeforeRowLock pins the lock order. +// A test transaction holds the admin-invariant lock, as a bootstrap would, +// while each write runs. The write must queue on that lock before touching +// the target row, so the holder can still lock the row with NOWAIT. Before +// issue #63 the write updated or deleted the row first and only reached the +// lock in the deferred trigger at commit, so NOWAIT failed with 55P03. +func TestIntegration_AdminDemotingWrites_LockBeforeRowLock(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + + for _, write := range adminDemotingWrites() { + t.Run(write.name, func(t *testing.T) { + resetUsers(t, db) + // Two admins, so the write is allowed once the holder lets go. + target := seedActiveAdmins(t, store, "lock-order", 2)[0] + ctx := context.Background() + + holder, err := db.Pool().Begin(ctx) + require.NoError(t, err) + defer func() { _ = holder.Rollback(ctx) }() + _, err = holder.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", adminInvariantAdvisoryLockKey) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- write.run(ctx, store, target) }() + waitForAdminLockWaiter(t, db) + + _, err = holder.Exec(ctx, "SELECT 1 FROM users WHERE id = $1 FOR UPDATE NOWAIT", target.ID) + require.NoError(t, err, + "the write locked the users row before the admin-invariant advisory lock") + require.NoError(t, holder.Commit(ctx)) + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(30 * time.Second): + t.Fatal("write did not finish after the advisory lock was released") + } + assert.Equal(t, 1, countActiveAdmins(t, db)) + }) + } +} + +// serviceDemotion is one service-level way of taking admin standing away. +type serviceDemotion struct { + name string + run func(ctx context.Context, svc *Service, userID string) error +} + +func serviceDemotions() []serviceDemotion { + inactive := false + return []serviceDemotion{ + {"deactivate", func(ctx context.Context, svc *Service, id string) error { + _, err := svc.UpdateUser(ctx, "", id, UpdateUserRequest{Active: &inactive}) + return err + }}, + {"drop group", func(ctx context.Context, svc *Service, id string) error { + _, err := svc.UpdateUser(ctx, "", id, UpdateUserRequest{GroupIDs: []string{DefaultPurchaserGroupID}}) + return err + }}, + {"delete", func(ctx context.Context, svc *Service, id string) error { + return svc.DeleteUser(ctx, id) + }}, + } +} + +// runConcurrently starts every fn at once behind a barrier and returns their +// errors in order. +func runConcurrently(fns ...func() error) []error { + errs := make([]error, len(fns)) + start := make(chan struct{}) + var wg sync.WaitGroup + for i, fn := range fns { + wg.Add(1) + go func(i int, fn func() error) { + defer wg.Done() + <-start + errs[i] = fn() + }(i, fn) + } + close(start) + wg.Wait() + return errs +} + +// TestIntegration_ConcurrentDemotionOfLastTwoAdmins races every pair of +// demote, deactivate and delete against the two remaining admins: exactly one +// of the pair succeeds, the other gets ErrLastAdmin, and one active admin is +// left. +func TestIntegration_ConcurrentDemotionOfLastTwoAdmins(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + svc := NewService(ServiceConfig{Store: store}) + ctx := context.Background() + const iterations = 15 + + for _, first := range serviceDemotions() { + for _, second := range serviceDemotions() { + t.Run(first.name+" vs "+second.name, func(t *testing.T) { + for i := 0; i < iterations; i++ { + resetUsers(t, db) + admins := seedActiveAdmins(t, store, fmt.Sprintf("last-two-%d", i), 2) + + errs := runConcurrently( + func() error { return first.run(ctx, svc, admins[0].ID) }, + func() error { return second.run(ctx, svc, admins[1].ID) }, + ) + + succeeded := 0 + for _, err := range errs { + if err == nil { + succeeded++ + continue + } + require.ErrorIs(t, err, ErrLastAdmin, "iteration %d", i) + } + require.Equal(t, 1, succeeded, "iteration %d: exactly one demotion must win", i) + require.Equal(t, 1, countActiveAdmins(t, db), "iteration %d", i) + } + }) + } + } +} + +// TestIntegration_ConcurrentBootstrapVersusDemotion races CreateAdminIfNone +// against demoting an existing admin. An admin exists throughout, so the +// bootstrap must never insert; the demotion succeeds only when another admin +// remains. +func TestIntegration_ConcurrentBootstrapVersusDemotion(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + svc := NewService(ServiceConfig{Store: store}) + ctx := context.Background() + const iterations = 15 + + for _, admins := range []int{1, 2} { + for _, demotion := range serviceDemotions() { + t.Run(fmt.Sprintf("%d admins, %s", admins, demotion.name), func(t *testing.T) { + for i := 0; i < iterations; i++ { + resetUsers(t, db) + seeded := seedActiveAdmins(t, store, fmt.Sprintf("bootstrap-race-%d", i), admins) + + var created bool + errs := runConcurrently( + func() error { + var err error + created, err = store.CreateAdminIfNone(ctx, newBootstrapCandidate(fmt.Sprintf("bootstrap-%d@example.com", i))) + return err + }, + func() error { return demotion.run(ctx, svc, seeded[0].ID) }, + ) + + require.NoError(t, errs[0], "iteration %d", i) + assert.False(t, created, "iteration %d: an admin existed, the bootstrap must not insert", i) + if admins == 1 { + require.ErrorIs(t, errs[1], ErrLastAdmin, "iteration %d", i) + } else { + require.NoError(t, errs[1], "iteration %d", i) + } + require.Equal(t, 1, countActiveAdmins(t, db), "iteration %d", i) + } + }) + } + } +} + +// lockTimeoutPool opens a second pool on the test database whose sessions +// give up on any lock wait after lockTimeout, so a hang fails fast instead of +// stalling the suite. +func lockTimeoutPool(t *testing.T, db *database.Connection, lockTimeout string) *pgxpool.Pool { + t.Helper() + cfg := db.Pool().Config().Copy() + cfg.MaxConns = 16 + cfg.ConnConfig.RuntimeParams["lock_timeout"] = lockTimeout + pool, err := pgxpool.NewWithConfig(context.Background(), cfg) + require.NoError(t, err) + t.Cleanup(pool.Close) + return pool +} + +// expectedContentionError reports whether err is an outcome the mixed +// workload can legitimately produce: the min-one-admin trigger refusing the +// last admin, a compare-and-set losing to another write, or the row having +// been deleted by another write. +func expectedContentionError(err error) bool { + if err == nil || isLastAdminConstraintViolation(err) || errors.Is(err, ErrUserChanged) { + return true + } + var pgErr *pgconn.PgError + return !errors.As(err, &pgErr) && strings.HasPrefix(err.Error(), "user not found: ") +} + +// mixedAdminWorkload returns one round of concurrent writes against three +// admins: every admin-demoting store path, plus the bootstrap and the login +// paths that lock users rows without the advisory lock. +func mixedAdminWorkload(ctx context.Context, store *PostgresStore, admins []*User) []func() error { + var fns []func() error + for i, write := range adminDemotingWrites() { + target := admins[i%len(admins)] + other := admins[(i+1)%len(admins)] + fns = append(fns, + func() error { return write.run(ctx, store, target) }, + func() error { return write.run(ctx, store, other) }, + ) + } + for _, a := range admins { + id := a.ID + fns = append(fns, + func() error { return store.RecordSuccessfulLogin(ctx, id) }, + func() error { return store.RecordFailedLogin(ctx, id) }, + ) + } + fns = append(fns, func() error { + _, err := store.CreateAdminIfNone(ctx, newBootstrapCandidate("mixed-bootstrap@example.com")) + return err + }) + return fns +} + +// TestIntegration_AdminWrites_NoDeadlock runs many rounds of the mixed +// workload with a lock_timeout and asserts no transaction ever deadlocks +// (40P01) or times out on a lock (55P03), and that an active admin always +// remains. A write path that still locked the row before the advisory lock +// would deadlock against one that takes the advisory lock first. +func TestIntegration_AdminWrites_NoDeadlock(t *testing.T) { + db := setupAuthTestDB(t) + pool := lockTimeoutPool(t, db, "10s") + store := NewPostgresStore(pool) + ctx := context.Background() + const rounds = 60 + + for round := 0; round < rounds; round++ { + resetUsers(t, db) + admins := seedActiveAdmins(t, store, fmt.Sprintf("mixed-%d", round), 3) + + for _, err := range runConcurrently(mixedAdminWorkload(ctx, store, admins)...) { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) { + require.NotEqual(t, "40P01", pgErr.Code, "round %d: deadlock: %v", round, err) + require.NotEqual(t, "55P03", pgErr.Code, "round %d: lock wait timed out: %v", round, err) + } + require.True(t, expectedContentionError(err), "round %d: unexpected error: %v", round, err) + } + require.GreaterOrEqual(t, countActiveAdmins(t, pool), 1, "round %d", round) + } +} diff --git a/internal/auth/store_postgres_admin_lock_test.go b/internal/auth/store_postgres_admin_lock_test.go new file mode 100644 index 000000000..4ab953f65 --- /dev/null +++ b/internal/auth/store_postgres_admin_lock_test.go @@ -0,0 +1,239 @@ +package auth + +// store_postgres_admin_lock_test.go - pgxmock tests pinning that every +// PostgresStore write able to lower the active-admin count runs in a +// transaction whose first statement takes adminInvariantAdvisoryLockKey +// (issue #63). The lock-order and concurrency behavior itself is proven on a +// real PostgreSQL in store_postgres_admin_lock_db_test.go. + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/pashagolub/pgxmock/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + updateUserSQLRegex = `(?s)UPDATE users SET\s+email = \$2,.*deactivated_at = \$19\s+WHERE id = \$1` + + updateUserAdminFieldsSQLRegex = `(?s)UPDATE users SET email = \$2, group_ids = \$3, active = \$4, ` + + `deactivated_at = \$5, updated_at = NOW\(\)\s+` + + `WHERE id = \$1 AND email = \$6 AND group_ids = \$7 AND active = \$8\s+RETURNING updated_at` + + deleteUserSQLRegex = `DELETE FROM users WHERE id = \$1` +) + +const lockedWriteUserID = "22222222-3333-4444-8555-666666666666" + +// writeOutcome selects what the mocked write statement returns. +type writeOutcome int + +const ( + writeOK writeOutcome = iota + writeNoRow + writeFails +) + +// adminLockedWrite describes one admin-affecting store write: how to mock its +// statement for an outcome, how to call it, and the error a missing row gives. +type adminLockedWrite struct { + name string + expect func(mock pgxmock.PgxPoolIface, outcome writeOutcome) + call func(ctx context.Context, store *PostgresStore) error + assertNoRow func(t *testing.T, err error) +} + +func expectLockedExec(mock pgxmock.PgxPoolIface, sqlRegex, verb string, args []any, outcome writeOutcome) { + e := mock.ExpectExec(sqlRegex).WithArgs(args...) + switch outcome { + case writeOK: + e.WillReturnResult(pgxmock.NewResult(verb, 1)) + case writeNoRow: + e.WillReturnResult(pgxmock.NewResult(verb, 0)) + case writeFails: + e.WillReturnError(errors.New("connection reset")) + } +} + +func anyArgs(n int) []any { + args := make([]any, n) + for i := range args { + args[i] = pgxmock.AnyArg() + } + return args +} + +func adminLockedWrites() []adminLockedWrite { + assertUserNotFound := func(t *testing.T, err error) { + require.Error(t, err) + assert.Contains(t, err.Error(), "user not found: "+lockedWriteUserID) + } + return []adminLockedWrite{ + { + name: "UpdateUser", + expect: func(mock pgxmock.PgxPoolIface, outcome writeOutcome) { + args := append([]any{lockedWriteUserID}, anyArgs(18)...) + expectLockedExec(mock, updateUserSQLRegex, "UPDATE", args, outcome) + }, + call: func(ctx context.Context, store *PostgresStore) error { + return store.UpdateUser(ctx, &User{ID: lockedWriteUserID, GroupIDs: []string{DefaultPurchaserGroupID}}) + }, + assertNoRow: assertUserNotFound, + }, + { + name: "UpdateUserAdminFields", + expect: func(mock pgxmock.PgxPoolIface, outcome writeOutcome) { + e := mock.ExpectQuery(updateUserAdminFieldsSQLRegex). + WithArgs(lockedWriteUserID, "new@example.com", []string{DefaultPurchaserGroupID}, false, + pgxmock.AnyArg(), "old@example.com", []string{DefaultAdminGroupID}, true) + switch outcome { + case writeOK: + e.WillReturnRows(mock.NewRows([]string{"updated_at"}).AddRow(time.Unix(1700000000, 0))) + case writeNoRow: + e.WillReturnRows(mock.NewRows([]string{"updated_at"})) + case writeFails: + e.WillReturnError(errors.New("connection reset")) + } + }, + call: func(ctx context.Context, store *PostgresStore) error { + user := &User{ID: lockedWriteUserID, Email: "new@example.com", GroupIDs: []string{DefaultPurchaserGroupID}} + return store.UpdateUserAdminFields(ctx, user, "old@example.com", []string{DefaultAdminGroupID}, true) + }, + assertNoRow: func(t *testing.T, err error) { assert.ErrorIs(t, err, ErrUserChanged) }, + }, + { + name: "DeleteUser", + expect: func(mock pgxmock.PgxPoolIface, outcome writeOutcome) { + expectLockedExec(mock, deleteUserSQLRegex, "DELETE", []any{lockedWriteUserID}, outcome) + }, + call: func(ctx context.Context, store *PostgresStore) error { + return store.DeleteUser(ctx, lockedWriteUserID) + }, + assertNoRow: assertUserNotFound, + }, + } +} + +// expectAdminLock registers the transaction open and the advisory lock that +// must precede every admin-affecting write. pgxmock matches in order, so a +// write issued before the lock fails the test. +func expectAdminLock(mock pgxmock.PgxPoolIface) { + mock.ExpectBegin() + mock.ExpectExec(advisoryLockSQLRegex). + WithArgs(adminInvariantAdvisoryLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) +} + +func TestPGXMock_AdminWrites_TakeLockBeforeWriteAndCommit(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + expectAdminLock(mock) + w.expect(mock, writeOK) + mock.ExpectCommit() + + require.NoError(t, w.call(context.Background(), NewPostgresStore(mock))) + }) + } +} + +func TestPGXMock_AdminWrites_BeginError(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + mock.ExpectBegin().WillReturnError(errors.New("pool exhausted")) + + err := w.call(context.Background(), NewPostgresStore(mock)) + require.Error(t, err) + assert.Contains(t, err.Error(), "begin transaction") + }) + } +} + +func TestPGXMock_AdminWrites_LockErrorSkipsWrite(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + mock.ExpectBegin() + mock.ExpectExec(advisoryLockSQLRegex). + WithArgs(adminInvariantAdvisoryLockKey). + WillReturnError(errors.New("canceling statement due to lock timeout")) + mock.ExpectRollback() + + err := w.call(context.Background(), NewPostgresStore(mock)) + require.Error(t, err) + assert.Contains(t, err.Error(), "acquire admin invariant lock") + }) + } +} + +func TestPGXMock_AdminWrites_NoRowRollsBack(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + expectAdminLock(mock) + w.expect(mock, writeNoRow) + mock.ExpectRollback() + + w.assertNoRow(t, w.call(context.Background(), NewPostgresStore(mock))) + }) + } +} + +func TestPGXMock_AdminWrites_WriteErrorRollsBack(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + expectAdminLock(mock) + w.expect(mock, writeFails) + mock.ExpectRollback() + + err := w.call(context.Background(), NewPostgresStore(mock)) + require.Error(t, err) + assert.Contains(t, err.Error(), "connection reset") + }) + } +} + +// TestPGXMock_AdminWrites_CommitTriggerViolationStaysDetectable covers the +// deferred min-one-admin trigger, which now raises at the explicit COMMIT. The +// service maps it to ErrLastAdmin through isLastAdminConstraintViolation, so +// the PgError must survive the wrapping. +func TestPGXMock_AdminWrites_CommitTriggerViolationStaysDetectable(t *testing.T) { + for _, w := range adminLockedWrites() { + t.Run(w.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + expectAdminLock(mock) + w.expect(mock, writeOK) + mock.ExpectCommit().WillReturnError(&pgconn.PgError{ + Code: "P0001", + Message: "last_admin_constraint_violation: at least one active member of the Administrators group must remain", + }) + mock.ExpectRollback() + + err := w.call(context.Background(), NewPostgresStore(mock)) + require.Error(t, err) + assert.True(t, isLastAdminConstraintViolation(err), "lost the trigger error: %v", err) + assert.Contains(t, err.Error(), "commit") + }) + } +} + +func TestPGXMock_UpdateUserAdminFields_ReturnsStoredUpdatedAt(t *testing.T) { + mock := newAuthPgxMock(t) + stored := time.Unix(1700000000, 0) + expectAdminLock(mock) + mock.ExpectQuery(updateUserAdminFieldsSQLRegex). + WithArgs(anyArgs(8)...). + WillReturnRows(mock.NewRows([]string{"updated_at"}).AddRow(stored)) + mock.ExpectCommit() + + user := &User{ID: lockedWriteUserID} + require.NoError(t, NewPostgresStore(mock).UpdateUserAdminFields(context.Background(), user, "", nil, true)) + assert.True(t, user.UpdatedAt.Equal(stored)) +} diff --git a/internal/auth/store_postgres_credentials.go b/internal/auth/store_postgres_credentials.go index e0dfd6324..ca716bbf9 100644 --- a/internal/auth/store_postgres_credentials.go +++ b/internal/auth/store_postgres_credentials.go @@ -14,19 +14,24 @@ import ( // the row still holds the email, groups and active state the caller read, so a // stale admin save cannot erase MFA fields or restore an old password hash // (issue #493). +// +// The write can deactivate a member or drop the Administrators group, so it +// takes the admin-invariant lock first (see adminInvariantAdvisoryLockKey). func (s *PostgresStore) UpdateUserAdminFields(ctx context.Context, user *User, readEmail string, readGroupIDs []string, readActive bool) error { - err := s.db.QueryRow(ctx, ` - UPDATE users SET email = $2, group_ids = $3, active = $4, deactivated_at = $5, updated_at = NOW() - WHERE id = $1 AND email = $6 AND group_ids = $7 AND active = $8 - RETURNING updated_at - `, user.ID, user.Email, user.GroupIDs, user.Active, user.DeactivatedAt, readEmail, readGroupIDs, readActive).Scan(&user.UpdatedAt) - if errors.Is(err, pgx.ErrNoRows) { - return ErrUserChanged - } - if err != nil { - return fmt.Errorf("failed to update user: %w", err) - } - return nil + return s.withAdminInvariantLock(ctx, "update user", func(tx pgx.Tx) error { + err := tx.QueryRow(ctx, ` + UPDATE users SET email = $2, group_ids = $3, active = $4, deactivated_at = $5, updated_at = NOW() + WHERE id = $1 AND email = $6 AND group_ids = $7 AND active = $8 + RETURNING updated_at + `, user.ID, user.Email, user.GroupIDs, user.Active, user.DeactivatedAt, readEmail, readGroupIDs, readActive).Scan(&user.UpdatedAt) + if errors.Is(err, pgx.ErrNoRows) { + return ErrUserChanged + } + if err != nil { + return fmt.Errorf("failed to update user: %w", err) + } + return nil + }) } // UpdateUserCredentials writes only the email and password columns, and only diff --git a/internal/auth/store_postgres_test.go b/internal/auth/store_postgres_test.go index 5e21b0de1..b270bc97a 100644 --- a/internal/auth/store_postgres_test.go +++ b/internal/auth/store_postgres_test.go @@ -275,93 +275,6 @@ func TestPostgresStore_CreateUser(t *testing.T) { }) } -func TestPostgresStore_UpdateUser(t *testing.T) { - ctx := context.Background() - - t.Run("successfully update user", func(t *testing.T) { - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - user := &User{ - ID: "user-123", - Email: "updated@example.com", - GroupIDs: []string{}, - } - - // Create a CommandTag that indicates 1 row was affected - tag := pgconn.NewCommandTag("UPDATE 1") - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(tag, nil) - - err := store.UpdateUser(ctx, user) - require.NoError(t, err) - assert.False(t, user.UpdatedAt.IsZero()) - - mockDB.AssertExpectations(t) - }) - - t.Run("return error when update fails", func(t *testing.T) { - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - user := &User{ - ID: "user-123", - GroupIDs: []string{}, - } - - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(pgconn.CommandTag{}, fmt.Errorf("update failed")) - - err := store.UpdateUser(ctx, user) - assert.Error(t, err) - - mockDB.AssertExpectations(t) - }) -} - -func TestPostgresStore_DeleteUser(t *testing.T) { - ctx := context.Background() - - t.Run("successfully delete user", func(t *testing.T) { - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - // Create a CommandTag that indicates 1 row was affected - tag := pgconn.NewCommandTag("DELETE 1") - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(tag, nil) - - err := store.DeleteUser(ctx, "user-123") - require.NoError(t, err) - - mockDB.AssertExpectations(t) - }) - - t.Run("return error when delete fails", func(t *testing.T) { - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(pgconn.CommandTag{}, fmt.Errorf("delete failed")) - - err := store.DeleteUser(ctx, "user-123") - assert.Error(t, err) - - mockDB.AssertExpectations(t) - }) - - t.Run("return error when user not found", func(t *testing.T) { - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - // Return 0 rows affected - tag := pgconn.NewCommandTag("DELETE 0") - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(tag, nil) - - err := store.DeleteUser(ctx, "nonexistent") - assert.Error(t, err) - assert.Contains(t, err.Error(), "user not found") - - mockDB.AssertExpectations(t) - }) -} - func TestNewPostgresStore(t *testing.T) { mockDB := new(MockDBConnection) store := NewPostgresStore(mockDB) @@ -470,29 +383,6 @@ func TestPostgresStore_AdminExists(t *testing.T) { }) } -func TestPostgresStore_UpdateUser_NotFound(t *testing.T) { - ctx := context.Background() - - mockDB := new(MockDBConnection) - store := &PostgresStore{db: mockDB} - - user := &User{ - ID: "nonexistent", - Email: "test@example.com", - GroupIDs: []string{}, - } - - // Return 0 rows affected - tag := pgconn.NewCommandTag("UPDATE 0") - mockDB.On("Exec", ctx, mock.AnythingOfType("string"), mock.Anything).Return(tag, nil) - - err := store.UpdateUser(ctx, user) - assert.Error(t, err) - assert.Contains(t, err.Error(), "user not found") - - mockDB.AssertExpectations(t) -} - // ========================================== // SESSION TESTS // ==========================================