From 143babc748f3ff85c936003de48c534673f7ab29 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 22:57:33 -0700 Subject: [PATCH 1/2] fix(auth): close admin bootstrap race and cover store SQL paths Adding the DB-backed coverage requested by review finding TEST-01 (issue #1153) exposed a real bug: the CreateAdminIfNone conditional INSERT ... WHERE NOT EXISTS is not race-free on its own. Under READ COMMITTED each statement snapshots at statement start, so two concurrent bootstrap calls can both see "no admin" and both insert, producing two admins (reproduced by the new concurrent integration test, which failed with 2 winners against the old code). Fix: run the guarded insert inside a transaction that first takes the transaction-scoped advisory lock already used by the min-one-admin trigger (migration 000065). The second caller blocks until the first commits and its INSERT then sees the committed admin, so the NOT EXISTS guard suppresses the duplicate. DBConnection gains Begin(); database.Connection and pgxmock already provide it. Test coverage added per the report's recommendation: - pgxmock tests pinning the actual SQL for CreateAdminIfNone (19 column INSERT, NOT EXISTS predicate on $20, Administrators group forcing, ErrEmailInUse mapping, begin/lock/commit error paths), AdminExists predicate agreement, CountGroupMembers, ListUsers, ListAPIKeysByUser, UpdateAPIKeyLastUsed and Ping, taking these paths from 0% to 85-100% coverage. - testcontainers integration tests (integration build tag) proving bootstrap-insert-once semantics under two concurrent calls across 25 barrier-synced iterations, plus sequential no-op, inactive-admin and email-collision semantics against the real migrated schema. Closes #1153 --- internal/auth/store_postgres.go | 45 +- internal/auth/store_postgres_db_test.go | 215 ++++++++ internal/auth/store_postgres_pgxmock_test.go | 539 +++++++++++++++++++ internal/auth/store_postgres_test.go | 8 + 4 files changed, 802 insertions(+), 5 deletions(-) create mode 100644 internal/auth/store_postgres_db_test.go create mode 100644 internal/auth/store_postgres_pgxmock_test.go diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index e8c54ecde..3ff5103bc 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -20,6 +20,7 @@ type DBConnection interface { QueryRow(ctx context.Context, sql string, args ...any) pgx.Row Query(ctx context.Context, sql string, args ...any) (pgx.Rows, error) Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) + Begin(ctx context.Context) (pgx.Tx, error) Ping(ctx context.Context) error } @@ -353,6 +354,14 @@ func (s *PostgresStore) CountGroupMembers(ctx context.Context, groupID string) ( return count, nil } +// adminInvariantAdvisoryLockKey is the transaction-scoped advisory lock key +// serializing writes that affect the "at least/at most the right number of +// admins" invariants. It MUST stay equal to the key used by +// check_min_one_admin() in migration 000065 so bootstrap inserts and +// admin-demoting commits serialize against each other as well as among +// themselves. +const adminInvariantAdvisoryLockKey = 8059058058580001 + // 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 @@ -361,10 +370,18 @@ func (s *PostgresStore) CountGroupMembers(ctx context.Context, groupID string) ( // for any other failure. // // The conditional INSERT closes the bootstrap race without the -// users_one_admin partial unique index (dropped in migration 000050). -// Postgres guarantees atomicity of the SELECT … WHERE NOT EXISTS … -// INSERT in a single statement — no advisory lock or transaction is -// needed. +// users_one_admin partial unique index (dropped in migration 000050), +// but a single INSERT … WHERE NOT EXISTS statement is NOT race-free on +// its own: under READ COMMITTED each statement's snapshot is taken at +// statement start, so two concurrent calls can both see "no admin" and +// both insert (reproduced by +// TestIntegration_CreateAdminIfNone_ConcurrentBootstrapOnce). To close +// that window the insert runs in a transaction that first takes the +// transaction-scoped advisory lock shared with the min-one-admin +// trigger (migration 000065): the second caller blocks until the first +// commits, and its INSERT statement then takes a fresh snapshot that +// sees the committed admin, so the NOT EXISTS guard suppresses the +// duplicate. The lock auto-releases at COMMIT or ROLLBACK. func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool, error) { if user.ID == "" { user.ID = uuid.New().String() @@ -411,7 +428,21 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool groupIDs = append(append([]string(nil), groupIDs...), DefaultAdminGroupID) } - tag, err := s.db.Exec(ctx, query, + tx, err := s.db.Begin(ctx) + if err != nil { + return false, fmt.Errorf("failed to begin admin bootstrap transaction: %w", err) + } + // Rollback is a no-op after a successful Commit; on any earlier return + // it also releases the advisory lock. + defer func() { _ = tx.Rollback(ctx) }() + + // Serialize against concurrent bootstrap calls and against the + // min-one-admin deferred trigger (see adminInvariantAdvisoryLockKey). + if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", adminInvariantAdvisoryLockKey); err != nil { + return false, fmt.Errorf("failed to acquire admin bootstrap lock: %w", err) + } + + tag, err := tx.Exec(ctx, query, user.ID, user.Email, user.PasswordHash, user.Salt, groupIDs, user.Active, user.MFAEnabled, user.MFASecret, user.MFAPendingSecret, user.MFAPendingSecretExpiresAt, recoveryCodes, @@ -431,6 +462,10 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool return false, fmt.Errorf("failed to create admin: %w", err) } + if err := tx.Commit(ctx); err != nil { + return false, fmt.Errorf("failed to commit admin bootstrap transaction: %w", err) + } + return tag.RowsAffected() > 0, nil } diff --git a/internal/auth/store_postgres_db_test.go b/internal/auth/store_postgres_db_test.go new file mode 100644 index 000000000..cbd2b9c01 --- /dev/null +++ b/internal/auth/store_postgres_db_test.go @@ -0,0 +1,215 @@ +//go:build integration +// +build integration + +package auth + +// store_postgres_db_test.go - DB-backed integration tests for the +// security-critical PostgresStore bootstrap paths (TEST-01, issue #1153). +// These run the real SQL against a migrated PostgreSQL testcontainer, +// proving the 19-column bootstrap INSERT, the NOT EXISTS guard and its +// agreement with AdminExists (active-membership semantics), and the +// insert-once guarantee under two concurrent CreateAdminIfNone calls. + +import ( + "context" + "fmt" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/database" + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// getAuthTestMigrationsPath returns the absolute path to the migrations directory. +func getAuthTestMigrationsPath() string { + _, filename, _, _ := runtime.Caller(0) + return filepath.Join(filepath.Dir(filename), "..", "database", "postgres", "migrations") +} + +// setupAuthTestDB starts a PostgreSQL container and runs all migrations +// without seeding a bootstrap admin (empty adminEmail), so tests start from +// the "fresh deployment, no admin yet" state CreateAdminIfNone exists for. +func setupAuthTestDB(t *testing.T) *database.Connection { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping DB test: cannot start PostgreSQL container: %v", err) + return nil + } + + if err := migrations.RunMigrations(ctx, container.DB.Pool(), getAuthTestMigrationsPath(), "", ""); err != nil { + container.Cleanup(ctx) + t.Skipf("Skipping DB test: cannot run migrations: %v", err) + return nil + } + + t.Cleanup(func() { + container.Cleanup(context.Background()) + }) + + return container.DB +} + +// resetUsers wipes the users table between scenarios. TRUNCATE is used +// deliberately: the trg_min_one_admin_delete row trigger (migration 000065) +// would otherwise veto deleting the last admin, and TRUNCATE does not fire +// row-level triggers. CASCADE clears dependent rows (sessions, api_keys). +func resetUsers(t *testing.T, db *database.Connection) { + t.Helper() + _, err := db.Exec(context.Background(), "TRUNCATE users CASCADE") + require.NoError(t, err) +} + +func newBootstrapCandidate(email string) *User { + return &User{ + Email: email, + PasswordHash: "test-hash", + Salt: "test-salt", + Active: true, + } +} + +// TestIntegration_CreateAdminIfNone_Bootstrap exercises the real bootstrap +// INSERT end to end: column ordering, the Administrators-group forcing logic, +// and the NOT EXISTS guard's agreement with AdminExists. +func TestIntegration_CreateAdminIfNone_Bootstrap(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + ctx := context.Background() + + t.Run("inserts first admin and forces Administrators group", func(t *testing.T) { + resetUsers(t, db) + + user := newBootstrapCandidate("first-admin@example.com") + created, err := store.CreateAdminIfNone(ctx, user) + require.NoError(t, err) + assert.True(t, created) + + // The inserted row must satisfy the membership predicate AdminExists uses. + exists, err := store.AdminExists(ctx) + require.NoError(t, err) + assert.True(t, exists, "bootstrap admin must be visible to AdminExists") + + stored, err := store.GetUserByEmail(ctx, "first-admin@example.com") + require.NoError(t, err) + assert.Contains(t, stored.GroupIDs, DefaultAdminGroupID, + "bootstrap admin must carry the Administrators group even though the caller did not supply it") + + count, err := store.CountGroupMembers(ctx, DefaultAdminGroupID) + require.NoError(t, err) + assert.Equal(t, 1, count) + }) + + t.Run("second sequential call is a no-op", func(t *testing.T) { + resetUsers(t, db) + + created, err := store.CreateAdminIfNone(ctx, newBootstrapCandidate("admin-a@example.com")) + require.NoError(t, err) + require.True(t, created) + + created, err = store.CreateAdminIfNone(ctx, newBootstrapCandidate("admin-b@example.com")) + require.NoError(t, err) + assert.False(t, created, "an active admin already exists, so the guarded insert must not fire") + + count, err := store.CountGroupMembers(ctx, DefaultAdminGroupID) + require.NoError(t, err) + assert.Equal(t, 1, count, "the losing call must not have inserted a row") + }) + + t.Run("inactive admin counts as no admin, matching AdminExists", func(t *testing.T) { + resetUsers(t, db) + + // Insert an INACTIVE Administrators-group member through the regular, + // unguarded create path (INSERT does not fire the min-one-admin trigger). + inactive := newBootstrapCandidate("inactive-admin@example.com") + inactive.Active = false + inactive.GroupIDs = []string{DefaultAdminGroupID} + require.NoError(t, store.CreateUser(ctx, inactive)) + + exists, err := store.AdminExists(ctx) + require.NoError(t, err) + require.False(t, exists, "inactive admin must not count as an existing admin") + + // The NOT EXISTS guard must agree: bootstrap proceeds. + created, err := store.CreateAdminIfNone(ctx, newBootstrapCandidate("real-admin@example.com")) + require.NoError(t, err) + assert.True(t, created, + "CreateAdminIfNone must treat an inactive-only admin set as 'no admin', mirroring AdminExists") + + count, err := store.CountGroupMembers(ctx, DefaultAdminGroupID) + require.NoError(t, err) + assert.Equal(t, 2, count, "CountGroupMembers counts members regardless of active flag") + }) + + t.Run("email collision with existing non-admin surfaces ErrEmailInUse", func(t *testing.T) { + resetUsers(t, db) + + regular := newBootstrapCandidate("taken@example.com") + // users_min_one_group (migration 000057) requires at least one group; + // any seeded non-admin group satisfies it. + regular.GroupIDs = []string{DefaultPurchaserGroupID} + require.NoError(t, store.CreateUser(ctx, regular)) + + created, err := store.CreateAdminIfNone(ctx, newBootstrapCandidate("taken@example.com")) + assert.False(t, created) + assert.ErrorIs(t, err, ErrEmailInUse) + }) +} + +// TestIntegration_CreateAdminIfNone_ConcurrentBootstrapOnce proves the +// insert-once semantics under two concurrent CreateAdminIfNone calls: exactly +// one caller wins and exactly one admin row exists afterwards, across +// repeated barrier-synchronized attempts. +func TestIntegration_CreateAdminIfNone_ConcurrentBootstrapOnce(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + ctx := context.Background() + + const iterations = 25 + + for i := 0; i < iterations; i++ { + resetUsers(t, db) + + type outcome struct { + created bool + err error + } + results := make(chan outcome, 2) + start := make(chan struct{}) + + for n := 0; n < 2; n++ { + user := newBootstrapCandidate(fmt.Sprintf("admin-%d-%d@example.com", i, n)) + go func(u *User) { + <-start + created, err := store.CreateAdminIfNone(ctx, u) + results <- outcome{created: created, err: err} + }(user) + } + close(start) + + winners := 0 + for n := 0; n < 2; n++ { + r := <-results + require.NoError(t, r.err, "iteration %d", i) + if r.created { + winners++ + } + } + require.Equal(t, 1, winners, + "iteration %d: exactly one of two concurrent bootstrap calls must win", i) + + count, err := store.CountGroupMembers(ctx, DefaultAdminGroupID) + require.NoError(t, err) + require.Equal(t, 1, count, + "iteration %d: exactly one admin row must exist after concurrent bootstrap", i) + } +} diff --git a/internal/auth/store_postgres_pgxmock_test.go b/internal/auth/store_postgres_pgxmock_test.go new file mode 100644 index 000000000..12269c7c5 --- /dev/null +++ b/internal/auth/store_postgres_pgxmock_test.go @@ -0,0 +1,539 @@ +package auth + +// store_postgres_pgxmock_test.go - pgxmock tests for the PostgresStore SQL +// paths that the MockDBConnection suite never executes (TEST-01, issue #1153): +// CreateAdminIfNone (bootstrap-race guard), CountGroupMembers, ListUsers, +// ListAPIKeysByUser, UpdateAPIKeyLastUsed and Ping. These tests pin the actual +// SQL text (argument count and order, the NOT EXISTS predicate) so that +// column drift or predicate divergence between AdminExists and +// CreateAdminIfNone fails a test instead of shipping green. + +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" +) + +// newAuthPgxMock creates a pgxmock pool with regexp query matching and +// registers the expectations check as cleanup so no test can forget it. +func newAuthPgxMock(t *testing.T) pgxmock.PgxPoolIface { + t.Helper() + mock, err := pgxmock.NewPool(pgxmock.QueryMatcherOption(pgxmock.QueryMatcherRegexp)) + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, mock.ExpectationsWereMet()) + mock.Close() + }) + return mock +} + +// createAdminIfNoneSQLRegex matches the full conditional bootstrap INSERT: +// 19 insert columns fed by SELECT $1..$19, guarded by the NOT EXISTS +// predicate bound to $20. +const createAdminIfNoneSQLRegex = `(?s)INSERT INTO users \(\s*` + + `id, email, password_hash, salt, group_ids, active,\s*` + + `mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at,\s*` + + `mfa_recovery_codes, password_reset_token, password_reset_expiry,\s*` + + `failed_login_attempts, locked_until, password_history,\s*` + + `created_at, updated_at, last_login_at\s*\)\s*` + + `SELECT \$1, \$2, \$3, \$4, \$5, \$6, \$7, \$8, \$9, \$10, \$11, \$12, \$13, \$14, \$15, \$16, \$17, \$18, \$19\s*` + + `WHERE NOT EXISTS \(SELECT 1 FROM users WHERE group_ids @> ARRAY\[\$20::uuid\] AND active = true\)` + +// bootstrapAdminUser returns a minimal valid first-admin candidate. +func bootstrapAdminUser() *User { + return &User{ + ID: "11111111-2222-4333-8444-555555555555", + Email: "bootstrap-admin@example.com", + PasswordHash: "hash", + Salt: "salt", + Active: true, + } +} + +// createAdminArgs builds the 20 expected Exec arguments for CreateAdminIfNone: +// exact matches for the security-relevant ones (id, email, group_ids, active +// and the $20 predicate binding) and AnyArg for incidental fields whose +// values the function fills in (timestamps) or passes through untouched. +func createAdminArgs(user *User, wantGroupIDs []string) []any { + return []any{ + user.ID, // $1 + user.Email, // $2 + user.PasswordHash, // $3 + user.Salt, // $4 + wantGroupIDs, // $5 - must contain the Administrators group + user.Active, // $6 + user.MFAEnabled, // $7 + pgxmock.AnyArg(), // $8 mfa_secret + pgxmock.AnyArg(), // $9 mfa_pending_secret + pgxmock.AnyArg(), // $10 mfa_pending_secret_expires_at + pgxmock.AnyArg(), // $11 mfa_recovery_codes + pgxmock.AnyArg(), // $12 password_reset_token + pgxmock.AnyArg(), // $13 password_reset_expiry + user.FailedLoginAttempts, // $14 + pgxmock.AnyArg(), // $15 locked_until + pgxmock.AnyArg(), // $16 password_history + pgxmock.AnyArg(), // $17 created_at (set inside) + pgxmock.AnyArg(), // $18 updated_at (set inside) + pgxmock.AnyArg(), // $19 last_login_at + DefaultAdminGroupID, // $20 NOT EXISTS predicate binding + } +} + +// advisoryLockSQLRegex matches the bootstrap serialization lock statement. +const advisoryLockSQLRegex = `SELECT pg_advisory_xact_lock\(\$1\)` + +// expectBootstrapTxPrefix registers the transaction-open and advisory-lock +// expectations that precede the guarded bootstrap INSERT. +func expectBootstrapTxPrefix(mock pgxmock.PgxPoolIface) { + mock.ExpectBegin() + mock.ExpectExec(advisoryLockSQLRegex). + WithArgs(adminInvariantAdvisoryLockKey). + WillReturnResult(pgxmock.NewResult("SELECT", 1)) +} + +// ---- CreateAdminIfNone ------------------------------------------------------ + +func TestPGXMock_CreateAdminIfNone_InsertsAndForcesAdminGroup(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + user.GroupIDs = []string{"99999999-aaaa-4bbb-8ccc-dddddddddddd"} + + // The Administrators group must be appended to the caller-supplied slice. + wantGroups := []string{"99999999-aaaa-4bbb-8ccc-dddddddddddd", DefaultAdminGroupID} + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, wantGroups)...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + created, err := store.CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + assert.True(t, created) + assert.False(t, user.CreatedAt.IsZero(), "CreatedAt must be set by the store") + assert.False(t, user.UpdatedAt.IsZero(), "UpdatedAt must be set by the store") +} + +func TestPGXMock_CreateAdminIfNone_DoesNotDuplicateAdminGroup(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + user.GroupIDs = []string{DefaultAdminGroupID} + + // Caller already supplied the Administrators group: pass through as-is. + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, []string{DefaultAdminGroupID})...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + created, err := store.CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + assert.True(t, created) +} + +func TestPGXMock_CreateAdminIfNone_GeneratesIDWhenEmpty(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + user.ID = "" + + args := createAdminArgs(user, []string{DefaultAdminGroupID}) + args[0] = pgxmock.AnyArg() // generated UUID + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(args...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + created, err := store.CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + assert.True(t, created) + assert.NotEmpty(t, user.ID, "an ID must be generated when none is provided") +} + +func TestPGXMock_CreateAdminIfNone_AdminAlreadyExists(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + + // The NOT EXISTS guard suppressed the insert: zero rows affected means + // another caller won the bootstrap race. Not an error. + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, []string{DefaultAdminGroupID})...). + WillReturnResult(pgxmock.NewResult("INSERT", 0)) + mock.ExpectCommit() + + created, err := store.CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + assert.False(t, created) +} + +func TestPGXMock_CreateAdminIfNone_EmailCollision(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, []string{DefaultAdminGroupID})...). + WillReturnError(&pgconn.PgError{Code: "23505", ConstraintName: "users_email_key"}) + mock.ExpectRollback() + + created, err := store.CreateAdminIfNone(context.Background(), user) + assert.False(t, created) + assert.ErrorIs(t, err, ErrEmailInUse) +} + +func TestPGXMock_CreateAdminIfNone_OtherDBError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, []string{DefaultAdminGroupID})...). + WillReturnError(errors.New("connection refused")) + mock.ExpectRollback() + + created, err := store.CreateAdminIfNone(context.Background(), user) + assert.False(t, created) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to create admin") + assert.NotErrorIs(t, err, ErrEmailInUse) +} + +func TestPGXMock_CreateAdminIfNone_BeginError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectBegin().WillReturnError(errors.New("pool exhausted")) + + created, err := store.CreateAdminIfNone(context.Background(), bootstrapAdminUser()) + assert.False(t, created) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to begin admin bootstrap transaction") +} + +func TestPGXMock_CreateAdminIfNone_LockError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectBegin() + mock.ExpectExec(advisoryLockSQLRegex). + WithArgs(adminInvariantAdvisoryLockKey). + WillReturnError(errors.New("connection reset")) + mock.ExpectRollback() + + created, err := store.CreateAdminIfNone(context.Background(), bootstrapAdminUser()) + assert.False(t, created) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to acquire admin bootstrap lock") +} + +func TestPGXMock_CreateAdminIfNone_CommitError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + user := bootstrapAdminUser() + + expectBootstrapTxPrefix(mock) + mock.ExpectExec(createAdminIfNoneSQLRegex). + WithArgs(createAdminArgs(user, []string{DefaultAdminGroupID})...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit().WillReturnError(errors.New("connection lost")) + mock.ExpectRollback() + + created, err := store.CreateAdminIfNone(context.Background(), user) + assert.False(t, created, "an unconfirmed insert must not be reported as created") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to commit admin bootstrap transaction") +} + +// ---- AdminExists predicate consistency -------------------------------------- + +// TestPGXMock_AdminExists_PredicateMatchesBootstrapGuard pins the AdminExists +// SQL to the same membership predicate the CreateAdminIfNone guard uses. The +// two must agree (see the comment in store_postgres.go); each side is pinned +// by its own regex so drift in either fails a test. +func TestPGXMock_AdminExists_PredicateMatchesBootstrapGuard(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`SELECT EXISTS\(SELECT 1 FROM users WHERE group_ids @> ARRAY\[\$1::uuid\] AND active = true\)`). + WithArgs(DefaultAdminGroupID). + WillReturnRows(pgxmock.NewRows([]string{"exists"}).AddRow(true)) + + exists, err := store.AdminExists(context.Background()) + require.NoError(t, err) + assert.True(t, exists) +} + +// ---- CountGroupMembers ------------------------------------------------------- + +func TestPGXMock_CountGroupMembers_Success(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM users WHERE group_ids @> ARRAY\[\$1::uuid\]`). + WithArgs(DefaultAdminGroupID). + WillReturnRows(pgxmock.NewRows([]string{"count"}).AddRow(3)) + + count, err := store.CountGroupMembers(context.Background(), DefaultAdminGroupID) + require.NoError(t, err) + assert.Equal(t, 3, count) +} + +func TestPGXMock_CountGroupMembers_QueryError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM users`). + WithArgs(DefaultAdminGroupID). + WillReturnError(errors.New("db down")) + + count, err := store.CountGroupMembers(context.Background(), DefaultAdminGroupID) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to count group members") + assert.Equal(t, 0, count) +} + +// ---- ListUsers --------------------------------------------------------------- + +// userColumns matches the SELECT column order in ListUsers / scanUser. +var userColumns = []string{ + "id", "email", "password_hash", "salt", "group_ids", "active", + "mfa_enabled", "mfa_secret", "mfa_pending_secret", "mfa_pending_secret_expires_at", + "mfa_recovery_codes", "password_reset_token", "password_reset_expiry", + "failed_login_attempts", "locked_until", "password_history", + "created_at", "updated_at", "last_login_at", +} + +func TestPGXMock_ListUsers_Success(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + created := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) + lastLogin := time.Date(2026, 2, 3, 4, 5, 6, 0, time.UTC) + + rows := pgxmock.NewRows(userColumns). + AddRow( + "user-1", "admin@example.com", "hash1", "salt1", + []string{DefaultAdminGroupID}, true, + true, "mfa-secret", nil, nil, + []string{"code1"}, nil, nil, + 0, nil, []string{"old-hash"}, + created, created, lastLogin, + ). + AddRow( + "user-2", "user@example.com", "hash2", "salt2", + []string{}, false, + false, nil, nil, nil, + []string{}, nil, nil, + 2, nil, []string{}, + created, created, nil, + ) + + mock.ExpectQuery(`(?s)SELECT id, email, password_hash, salt, group_ids, active,.*FROM users\s+ORDER BY created_at DESC\s+LIMIT 10000`). + WillReturnRows(rows) + + users, err := store.ListUsers(context.Background()) + require.NoError(t, err) + require.Len(t, users, 2) + + assert.Equal(t, "user-1", users[0].ID) + assert.Equal(t, "admin@example.com", users[0].Email) + assert.Equal(t, []string{DefaultAdminGroupID}, users[0].GroupIDs) + assert.True(t, users[0].Active) + assert.Equal(t, "mfa-secret", users[0].MFASecret) + require.NotNil(t, users[0].LastLoginAt) + assert.Equal(t, lastLogin, *users[0].LastLoginAt) + + assert.Equal(t, "user-2", users[1].ID) + assert.False(t, users[1].Active) + assert.Empty(t, users[1].MFASecret) + assert.Nil(t, users[1].LastLoginAt) + assert.Equal(t, 2, users[1].FailedLoginAttempts) +} + +func TestPGXMock_ListUsers_Empty(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`(?s)SELECT id, email,.*FROM users`). + WillReturnRows(pgxmock.NewRows(userColumns)) + + users, err := store.ListUsers(context.Background()) + require.NoError(t, err) + assert.NotNil(t, users) + assert.Empty(t, users) +} + +func TestPGXMock_ListUsers_QueryError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`(?s)SELECT id, email,.*FROM users`). + WillReturnError(errors.New("db down")) + + users, err := store.ListUsers(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to list users") + assert.Nil(t, users) +} + +func TestPGXMock_ListUsers_RowError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + created := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) + rows := pgxmock.NewRows(userColumns). + AddRow( + "user-1", "admin@example.com", "hash1", "salt1", + []string{}, true, + false, nil, nil, nil, + []string{}, nil, nil, + 0, nil, []string{}, + created, created, nil, + ). + RowError(0, errors.New("connection reset mid-iteration")) + + mock.ExpectQuery(`(?s)SELECT id, email,.*FROM users`).WillReturnRows(rows) + + _, err := store.ListUsers(context.Background()) + require.Error(t, err) +} + +// ---- ListAPIKeysByUser --------------------------------------------------------- + +// apiKeyColumns matches the SELECT column order in ListAPIKeysByUser / scanAPIKey. +var apiKeyColumns = []string{ + "id", "user_id", "name", "key_prefix", "key_hash", "permissions", + "is_active", "expires_at", "created_at", "last_used_at", +} + +func TestPGXMock_ListAPIKeysByUser_Success(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + created := time.Date(2026, 3, 4, 5, 6, 7, 0, time.UTC) + expires := created.Add(24 * time.Hour) + lastUsed := created.Add(time.Hour) + + rows := pgxmock.NewRows(apiKeyColumns). + AddRow( + "key-1", "user-1", "ci key", "cudly_ab", "hash-1", + []byte(`[{"action":"view","resource":"recommendations"}]`), true, expires, created, lastUsed, + ). + AddRow( + "key-2", "user-1", "old key", "cudly_cd", "hash-2", + []byte(`[]`), false, nil, created, nil, + ) + + mock.ExpectQuery(`(?s)SELECT id, user_id, name, key_prefix, key_hash, permissions,.*FROM api_keys\s+WHERE user_id = \$1\s+ORDER BY created_at DESC`). + WithArgs("user-1"). + WillReturnRows(rows) + + keys, err := store.ListAPIKeysByUser(context.Background(), "user-1") + require.NoError(t, err) + require.Len(t, keys, 2) + + assert.Equal(t, "key-1", keys[0].ID) + assert.Equal(t, "user-1", keys[0].UserID) + assert.Equal(t, []Permission{{Action: "view", Resource: "recommendations"}}, keys[0].Permissions) + assert.True(t, keys[0].IsActive) + require.NotNil(t, keys[0].ExpiresAt) + assert.Equal(t, expires, *keys[0].ExpiresAt) + require.NotNil(t, keys[0].LastUsedAt) + assert.Equal(t, lastUsed, *keys[0].LastUsedAt) + + assert.Equal(t, "key-2", keys[1].ID) + assert.False(t, keys[1].IsActive) + assert.Nil(t, keys[1].ExpiresAt) + assert.Nil(t, keys[1].LastUsedAt) +} + +func TestPGXMock_ListAPIKeysByUser_QueryError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectQuery(`(?s)SELECT id, user_id,.*FROM api_keys`). + WithArgs("user-1"). + WillReturnError(errors.New("db down")) + + keys, err := store.ListAPIKeysByUser(context.Background(), "user-1") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to list API keys") + assert.Nil(t, keys) +} + +// ---- UpdateAPIKeyLastUsed ------------------------------------------------------ + +func TestPGXMock_UpdateAPIKeyLastUsed_Success(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectExec(`UPDATE api_keys SET last_used_at = NOW\(\) WHERE id = \$1`). + WithArgs("key-1"). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + + err := store.UpdateAPIKeyLastUsed(context.Background(), "key-1") + assert.NoError(t, err) +} + +func TestPGXMock_UpdateAPIKeyLastUsed_NotFound(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectExec(`UPDATE api_keys SET last_used_at = NOW\(\) WHERE id = \$1`). + WithArgs("missing-key"). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + + err := store.UpdateAPIKeyLastUsed(context.Background(), "missing-key") + require.Error(t, err) + assert.Contains(t, err.Error(), "API key not found") +} + +func TestPGXMock_UpdateAPIKeyLastUsed_ExecError(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectExec(`UPDATE api_keys SET last_used_at = NOW\(\) WHERE id = \$1`). + WithArgs("key-1"). + WillReturnError(errors.New("db down")) + + err := store.UpdateAPIKeyLastUsed(context.Background(), "key-1") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to update API key last used") +} + +// ---- Ping ---------------------------------------------------------------------- + +func TestPGXMock_Ping(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectPing() + assert.NoError(t, store.Ping(context.Background())) +} + +func TestPGXMock_Ping_Error(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + + mock.ExpectPing().WillReturnError(errors.New("connection lost")) + assert.Error(t, store.Ping(context.Background())) +} diff --git a/internal/auth/store_postgres_test.go b/internal/auth/store_postgres_test.go index 567547b03..f1c1b6069 100644 --- a/internal/auth/store_postgres_test.go +++ b/internal/auth/store_postgres_test.go @@ -37,6 +37,14 @@ func (m *MockDBConnection) Exec(ctx context.Context, sql string, args ...interfa return mockArgs.Get(0).(pgconn.CommandTag), mockArgs.Error(1) } +func (m *MockDBConnection) Begin(ctx context.Context) (pgx.Tx, error) { + mockArgs := m.Called(ctx) + if mockArgs.Get(0) == nil { + return nil, mockArgs.Error(1) + } + return mockArgs.Get(0).(pgx.Tx), mockArgs.Error(1) +} + func (m *MockDBConnection) Ping(ctx context.Context) error { args := m.Called(ctx) return args.Error(0) From feaa190ff7e9201d1ce59af4434d64f28d7c52aa Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 18:13:28 +0200 Subject: [PATCH 2/2] fix(auth): satisfy errcheck and shadow lints in bootstrap tx scaffolding The bootstrap-race fix introduced two new golangci-lint failures on the new transaction scaffolding in CreateAdminIfNone (Lint Code is failing on this PR for these reasons): - `_ = tx.Rollback(ctx)` does not satisfy errcheck under this project's `check-blank: true` setting. Replace with the project-standard `defer tx.Rollback(ctx) //nolint:errcheck` pattern used in internal/config/store_postgres.go. - `if _, err := tx.Exec(...)` on the advisory-lock acquire shadowed the outer `err` (govet shadow). Drop the short-declaration so the lock-exec assigns to the outer variable. No behavior change on the success or known-error paths. go test ./internal/auth/ -race: 577 passed. golangci-lint on store_postgres.go: no new findings on the touched lines. --- internal/auth/store_postgres.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 3ff5103bc..11dfc582b 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -433,12 +433,13 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool return false, fmt.Errorf("failed to begin admin bootstrap transaction: %w", err) } // Rollback is a no-op after a successful Commit; on any earlier return - // it also releases the advisory lock. - defer func() { _ = tx.Rollback(ctx) }() + // it also releases the advisory lock. Matches the project convention + // (e.g. internal/config/store_postgres.go) for deferred rollback. + defer tx.Rollback(ctx) //nolint:errcheck // Serialize against concurrent bootstrap calls and against the // min-one-admin deferred trigger (see adminInvariantAdvisoryLockKey). - if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", adminInvariantAdvisoryLockKey); err != nil { + if _, err = tx.Exec(ctx, "SELECT pg_advisory_xact_lock($1)", adminInvariantAdvisoryLockKey); err != nil { return false, fmt.Errorf("failed to acquire admin bootstrap lock: %w", err) }