From ce0417358cfc03f1bf0639e132d354a29a8966aa Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 06:21:10 +0000 Subject: [PATCH] fix(auth): report an expired MFA enrollment as expired and pin the MFA write guards Follow-ups from the #567 review: - (d) When EnableMFA's guarded write is refused, re-check the row on the database clock: if MFA is still off under the same pending secret and its expiry has passed, return ErrMFAEnrollmentExpired (400) instead of ErrUserChanged (409). Every other refusal, including the losers of concurrent enables, stays ErrUserChanged. - (a) Pin each conjunct of the DisableMFA and ReplaceMFARecoveryCodes guards on its own, and add the zero-recovery-codes stale regenerate-after-disable subtest. - (c) Say why MFADisable clones the recovery codes before spending one, and pin it with a unit test that spends a non-last code. - (e) Pin that DisableMFA clears the pending secret and its expiry. - (f) Bound LoginLockoutExpiry's timestamps on the database clock (clock_timestamp()) instead of the test process clock. Closes #568 Co-Authored-By: Claude Opus 5.5 --- internal/auth/service_login_db_test.go | 19 +- internal/auth/service_mfa.go | 8 +- internal/auth/service_mfa_cas_db_test.go | 189 +++++++++++++++++- internal/auth/service_mfa_test.go | 28 +++ internal/auth/store_postgres_credentials.go | 25 ++- ...store_postgres_credentials_pgxmock_test.go | 86 ++++++++ 6 files changed, 347 insertions(+), 8 deletions(-) create mode 100644 internal/auth/store_postgres_credentials_pgxmock_test.go diff --git a/internal/auth/service_login_db_test.go b/internal/auth/service_login_db_test.go index 0774bbf20..7c93cb0b9 100644 --- a/internal/auth/service_login_db_test.go +++ b/internal/auth/service_login_db_test.go @@ -307,6 +307,18 @@ func TestIntegration_LoginMixedOrdering(t *testing.T) { } } +// dbClock reads the database's own clock. last_login_at and locked_until are +// stamped with NOW() on the server, so bounds taken from the test process's +// clock compare two clocks and fail when they disagree by a few milliseconds +// (issue #568 (f)); bounds read here share the clock and precision of the +// stamped value. +func dbClock(t *testing.T, store *PostgresStore) time.Time { + t.Helper() + var now time.Time + require.NoError(t, store.db.QueryRow(t.Context(), "SELECT clock_timestamp()").Scan(&now)) + return now +} + func TestIntegration_LoginLockoutExpiry(t *testing.T) { store := NewPostgresStore(setupAuthTestDB(t)) service := NewService(ServiceConfig{Store: store}) @@ -323,8 +335,9 @@ func TestIntegration_LoginLockoutExpiry(t *testing.T) { if success { password = "OriginalPassword123!" } - before := time.Now() + before := dbClock(t, store) response, loginErr := service.Login(t.Context(), LoginRequest{Email: user.Email, Password: password}) + after := dbClock(t, store) stored, err := store.GetUserByID(t.Context(), user.ID) require.NoError(t, err) if success { @@ -333,13 +346,13 @@ func TestIntegration_LoginLockoutExpiry(t *testing.T) { assert.Zero(t, stored.FailedLoginAttempts) assert.Nil(t, stored.LockedUntil) require.NotNil(t, stored.LastLoginAt) - assert.WithinRange(t, *stored.LastLoginAt, before, time.Now()) + assert.WithinRange(t, *stored.LastLoginAt, before, after) } else { require.EqualError(t, loginErr, genericLoginError) assert.Nil(t, response) assert.Equal(t, MaxFailedLoginAttempts+1, stored.FailedLoginAttempts) require.NotNil(t, stored.LockedUntil) - assert.WithinRange(t, *stored.LockedUntil, before.Add(AccountLockoutDuration), time.Now().Add(AccountLockoutDuration)) + assert.WithinRange(t, *stored.LockedUntil, before.Add(AccountLockoutDuration), after.Add(AccountLockoutDuration)) } }) } diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 9c32c58bf..38ac1c9bd 100644 --- a/internal/auth/service_mfa.go +++ b/internal/auth/service_mfa.go @@ -243,7 +243,9 @@ func (s *Service) hashRecoveryCode(code string) (string, error) { // consumeRecoveryCode returns true and removes the matching hash from // user.MFARecoveryCodes if entered matches any stored hash. The // removal is what makes recovery codes single-use; the caller must -// persist the user afterwards via UpdateUser. +// persist the shortened slice through its own compare-and-swap write. +// The removal shifts the slice in place, so a caller that still needs +// the codes as read must clone them first. // // Fails closed on empty input: an empty string normalizes to "", which // bcrypt.CompareHashAndPassword would compare against every stored hash. @@ -507,7 +509,9 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove // Try TOTP first (cheap), then fall back to recovery code (bcrypt // compare, ~constant-time per slot). Either path counts as a - // fresh proof-of-possession. + // fresh proof-of-possession. The codes are cloned because + // consumeRecoveryCode removes a matched hash in place, which would + // otherwise shift readCodes and fail DisableMFA's codes guard. readSecret, readCodes := user.MFASecret, slices.Clone(user.MFARecoveryCodes) if err := s.verifyMFAProof(ctx, user, codeOrRecovery); err != nil { return err diff --git a/internal/auth/service_mfa_cas_db_test.go b/internal/auth/service_mfa_cas_db_test.go index 81c7f91cb..1e1c9c4a7 100644 --- a/internal/auth/service_mfa_cas_db_test.go +++ b/internal/auth/service_mfa_cas_db_test.go @@ -4,6 +4,9 @@ package auth import ( "context" + "errors" + "fmt" + "sync" "testing" "time" @@ -89,6 +92,30 @@ func TestIntegration_MFADisableAndRegenerateAreColumnScoped(t *testing.T) { }) }) + t.Run("stale-regenerate-after-disable-with-zero-recovery-codes", func(t *testing.T) { + // Issue #568 (a): with every recovery code spent, the disabled row's + // codes ('{}') still match the stale read, so only the mfa_enabled and + // mfa_secret guards refuse the write. Spending the codes is stood in + // for by emptying the column. + f := newCredentialRaceFixture(t, store, "mfa-cas-regen-nocodes@example.com") + f.enrollMFA() + _, err := store.db.Exec(ctx, "UPDATE users SET mfa_recovery_codes = '{}' WHERE id = $1", f.user.ID) + require.NoError(t, err) + secret := f.stored().MFASecret + step := time.Now().Unix() / 30 + winner := f.onRead(func(ctx context.Context, other *Service) { + require.NoError(t, other.MFADisable(ctx, f.user.ID, credentialRacePassword, generateTOTP(secret, step))) + }) + fresh, err := f.svc.MFARegenerateRecoveryCodes(ctx, f.user.ID, generateTOTP(secret, step+1)) + require.ErrorIs(t, err, ErrUserChanged) + assert.Nil(t, fresh) + stored := f.stored() + assert.False(t, stored.MFAEnabled) + assert.Empty(t, stored.MFASecret) + assert.Empty(t, stored.MFARecoveryCodes, "the stale regenerate must not give a disabled account recovery codes") + assertOnlyChanged(t, winner(), stored, func(*User) {}) + }) + for name, act := range map[string]func(f *credentialRaceFixture) error{ "disable": func(f *credentialRaceFixture) error { return f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, totpNow(f.stored().MFASecret)) @@ -119,7 +146,8 @@ func TestIntegration_MFADisableAndRegenerateAreColumnScoped(t *testing.T) { } // Issue #527: the pending-secret expiry is enforced by the write itself, not -// only on the value read before it. +// only on the value read before it. Issue #568 (d): the caller is told the +// enrollment expired, not that the account changed. func TestIntegration_MFAEnableExpiryEnforcedInWrite(t *testing.T) { store := NewPostgresStore(setupAuthTestDB(t)) ctx := t.Context() @@ -132,9 +160,166 @@ func TestIntegration_MFAEnableExpiryEnforcedInWrite(t *testing.T) { require.NoError(t, execErr) }) codes, err := f.svc.MFAEnable(ctx, f.user.ID, totpNow(setup.Secret)) - require.ErrorIs(t, err, ErrUserChanged) + require.ErrorIs(t, err, ErrMFAEnrollmentExpired) + assert.NotErrorIs(t, err, ErrUserChanged) assert.Nil(t, codes) stored := f.stored() assert.False(t, stored.MFAEnabled, "an expired enrollment must not be promoted") assert.Empty(t, stored.MFASecret) + assert.Equal(t, setup.Secret, stored.MFAPendingSecret, "a refused enable must not clear the pending enrollment") +} + +// mfaColumns is a users row's MFA state, written directly so a test can put +// the row in exactly the state one guard is meant to refuse. expiresIn is a +// Postgres interval added to NOW(); nil leaves the pending expiry NULL. +type mfaColumns struct { + enabled bool + secret string + codes []string + pending string + expiresIn *string +} + +func pgInterval(s string) *string { return &s } + +func newMFAColumnsUser(t *testing.T, store *PostgresStore, email string, row mfaColumns) *User { + t.Helper() + u := &User{Email: email, PasswordHash: "unused-hash", Active: true, GroupIDs: []string{DefaultPurchaserGroupID}} + require.NoError(t, store.CreateUser(t.Context(), u)) + if row.codes == nil { + row.codes = []string{} // the column is NOT NULL DEFAULT '{}' + } + _, err := store.db.Exec(t.Context(), ` + UPDATE users SET mfa_enabled = $2, mfa_secret = $3, mfa_recovery_codes = $4, + mfa_pending_secret = $5, mfa_pending_secret_expires_at = NOW() + $6::interval + WHERE id = $1 + `, u.ID, row.enabled, row.secret, row.codes, row.pending, row.expiresIn) + require.NoError(t, err) + stored, err := store.GetUserByID(t.Context(), u.ID) + require.NoError(t, err) + return stored +} + +// Issue #568 (a), (e): each conjunct of the DisableMFA and +// ReplaceMFARecoveryCodes guards refuses a stale write on its own (the other +// two still match the read), and DisableMFA clears the pending enrollment too. +func TestIntegration_MFAWriteGuardsArePinnedIndividually(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + ctx := t.Context() + const readSecret = "READ-SECRET" + readCodes := []string{"read-code-1", "read-code-2"} + writes := map[string]func(userID string) error{ + "disable": func(id string) error { return store.DisableMFA(ctx, id, readSecret, readCodes) }, + "regenerate": func(id string) error { + return store.ReplaceMFARecoveryCodes(ctx, id, readSecret, readCodes, []string{"new-code"}) + }, + } + stale := map[string]mfaColumns{ + "mfa-enabled-false": {enabled: false, secret: readSecret, codes: readCodes}, + "mfa-secret-replaced": {enabled: true, secret: "OTHER-SECRET", codes: readCodes}, + "mfa-recovery-codes-changed": {enabled: true, secret: readSecret, codes: readCodes[1:]}, + } + for write, act := range writes { + for guard, row := range stale { + t.Run(write+"-refused-on-"+guard, func(t *testing.T) { + before := newMFAColumnsUser(t, store, "guard-"+write+"-"+guard+"@example.com", row) + require.ErrorIs(t, act(before.ID), ErrUserChanged) + after, err := store.GetUserByID(ctx, before.ID) + require.NoError(t, err) + assert.Equal(t, before, after, "a refused write must leave the row untouched") + }) + } + } + + matching := mfaColumns{enabled: true, secret: readSecret, codes: readCodes, pending: "PENDING-SECRET", expiresIn: pgInterval("10 minutes")} + t.Run("disable-clears-every-mfa-column", func(t *testing.T) { + before := newMFAColumnsUser(t, store, "guard-disable-match@example.com", matching) + require.NotNil(t, before.MFAPendingSecretExpiresAt) + require.NoError(t, writes["disable"](before.ID)) + after, err := store.GetUserByID(ctx, before.ID) + require.NoError(t, err) + assert.Empty(t, after.MFAPendingSecret, "DisableMFA must clear mfa_pending_secret") + assert.Nil(t, after.MFAPendingSecretExpiresAt, "DisableMFA must clear mfa_pending_secret_expires_at") + assertOnlyChanged(t, before, after, func(w *User) { + w.MFAEnabled, w.MFASecret, w.MFARecoveryCodes = false, "", []string{} + w.MFAPendingSecret, w.MFAPendingSecretExpiresAt = "", nil + }) + }) + t.Run("regenerate-replaces-only-the-codes", func(t *testing.T) { + before := newMFAColumnsUser(t, store, "guard-regenerate-match@example.com", matching) + require.NoError(t, writes["regenerate"](before.ID)) + after, err := store.GetUserByID(ctx, before.ID) + require.NoError(t, err) + assertOnlyChanged(t, before, after, func(w *User) { w.MFARecoveryCodes = []string{"new-code"} }) + }) +} + +// Issue #568 (d): EnableMFA reports ErrMFAEnrollmentExpired only when the +// write was refused by the expiry check, and ErrUserChanged for every other +// refusal, including the losers of concurrent enables of one enrollment. +func TestIntegration_EnableMFARefusalReason(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + ctx := t.Context() + const pending = "PENDING-SECRET" + enable := func(id string) error { return store.EnableMFA(ctx, id, pending, 1, []string{"new-code"}) } + + refusals := map[string]struct { + row mfaColumns + want error + }{ + "expired": {mfaColumns{pending: pending, expiresIn: pgInterval("-1 minute")}, ErrMFAEnrollmentExpired}, + "replaced-by-expired-setup": {mfaColumns{pending: "OTHER-PENDING", expiresIn: pgInterval("-1 minute")}, ErrUserChanged}, + "replaced-by-live-setup": {mfaColumns{pending: "OTHER-PENDING", expiresIn: pgInterval("10 minutes")}, ErrUserChanged}, + "enrollment-cleared": {mfaColumns{}, ErrUserChanged}, + "no-expiry": {mfaColumns{pending: pending}, ErrUserChanged}, + "mfa-on-with-expired-pending": {mfaColumns{enabled: true, secret: "ACTIVE-SECRET", codes: []string{"c"}, + pending: pending, expiresIn: pgInterval("-1 minute")}, ErrUserChanged}, + } + for name, tc := range refusals { + t.Run(name, func(t *testing.T) { + before := newMFAColumnsUser(t, store, "enable-refusal-"+name+"@example.com", tc.row) + err := enable(before.ID) + require.ErrorIs(t, err, tc.want) + assert.Equal(t, errors.Is(tc.want, ErrMFAEnrollmentExpired), errors.Is(err, ErrMFAEnrollmentExpired)) + assert.Equal(t, errors.Is(tc.want, ErrUserChanged), errors.Is(err, ErrUserChanged)) + after, getErr := store.GetUserByID(ctx, before.ID) + require.NoError(t, getErr) + assert.Equal(t, before, after, "a refused enable must leave the row untouched") + }) + } + + t.Run("concurrent-enables-of-one-live-enrollment", func(t *testing.T) { + const racers, rounds = 8, 5 + for round := range rounds { + u := newMFAColumnsUser(t, store, fmt.Sprintf("enable-race-%d@example.com", round), + mfaColumns{pending: pending, expiresIn: pgInterval("10 minutes")}) + errs := make([]error, racers) + var wg sync.WaitGroup + start := make(chan struct{}) + for i := range racers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + errs[i] = enable(u.ID) + }() + } + close(start) + wg.Wait() + wins := 0 + for i, err := range errs { + if err == nil { + wins++ + continue + } + require.ErrorIs(t, err, ErrUserChanged, "round %d racer %d", round, i) + require.NotErrorIs(t, err, ErrMFAEnrollmentExpired, "round %d racer %d", round, i) + } + require.Equal(t, 1, wins, "round %d: exactly one enable must win", round) + stored, err := store.GetUserByID(ctx, u.ID) + require.NoError(t, err) + assert.True(t, stored.MFAEnabled) + assert.Equal(t, pending, stored.MFASecret) + } + }) } diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index dccb8dfd3..8414f87e2 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -3,6 +3,7 @@ package auth import ( "context" "errors" + "slices" "strings" "testing" "time" @@ -351,6 +352,33 @@ func TestMFADisable_WithRecoveryCode(t *testing.T) { require.NoError(t, err) } +// Issue #568 (c): spending a recovery code that is not the last one shifts the +// user's slice in place, so DisableMFA must still be guarded by the codes as +// read, not by the shifted slice. +func TestMFADisable_WithRecoveryCodePassesCodesAsRead(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + codes := make([]string, 0, 3) + for _, plaintext := range []string{"ABCD-2345", "EFGH-6789", "JKLM-2345"} { + hash, err := service.hashRecoveryCode(plaintext) + require.NoError(t, err) + codes = append(codes, hash) + } + user.MFARecoveryCodes = codes + asRead := slices.Clone(codes) + + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + mockStore.On("DisableMFA", ctx, user.ID, user.MFASecret, asRead).Return(nil).Once() + + require.NoError(t, service.MFADisable(ctx, user.ID, "SecurePass@123", "ABCD-2345")) + mockStore.AssertExpectations(t) +} + func TestMFADisable_WrongPassword(t *testing.T) { ctx := context.Background() mockStore := new(MockStore) diff --git a/internal/auth/store_postgres_credentials.go b/internal/auth/store_postgres_credentials.go index e0dfd6324..532845029 100644 --- a/internal/auth/store_postgres_credentials.go +++ b/internal/auth/store_postgres_credentials.go @@ -135,11 +135,34 @@ func (s *PostgresStore) EnableMFA(ctx context.Context, userID, pendingSecret str return fmt.Errorf("failed to enable MFA: %w", err) } if result.RowsAffected() == 0 { - return ErrUserChanged + return s.enableMFARefusal(ctx, userID, pendingSecret) } return nil } +// enableMFARefusal names why EnableMFA's guarded write matched no row: the +// enrollment expired if MFA is still off under the same pending secret and its +// expiry has passed on the database clock, otherwise the row changed (issue +// #568). Pending secrets are random and leave the row only by being cleared or +// replaced, so a row still holding this one was refused by the expiry check. +// If the check itself fails, the write's own verdict, ErrUserChanged, stands. +func (s *PostgresStore) enableMFARefusal(ctx context.Context, userID, pendingSecret string) error { + var expired bool + err := s.db.QueryRow(ctx, ` + SELECT (mfa_pending_secret_expires_at <= NOW()) IS TRUE FROM users + WHERE id = $1 AND mfa_enabled = false AND mfa_pending_secret = $2 + `, userID, pendingSecret).Scan(&expired) + switch { + case errors.Is(err, pgx.ErrNoRows): + return ErrUserChanged + case err != nil: + return fmt.Errorf("%w (checking enrollment expiry: %w)", ErrUserChanged, err) + case expired: + return ErrMFAEnrollmentExpired + } + return ErrUserChanged +} + // ReplaceMFARecoveryCodes writes only the recovery codes, and only while MFA is // on under the secret and codes the caller read (issue #527). func (s *PostgresStore) ReplaceMFARecoveryCodes(ctx context.Context, userID, readSecret string, readCodes, newHashes []string) error { diff --git a/internal/auth/store_postgres_credentials_pgxmock_test.go b/internal/auth/store_postgres_credentials_pgxmock_test.go new file mode 100644 index 000000000..0395250ff --- /dev/null +++ b/internal/auth/store_postgres_credentials_pgxmock_test.go @@ -0,0 +1,86 @@ +package auth + +// pgxmock tests for how EnableMFA names a refused enrollment (issue #568 (d)): +// an expired pending secret is ErrMFAEnrollmentExpired, anything else stays +// ErrUserChanged. The real-Postgres behavior is pinned by +// TestIntegration_EnableMFARefusalReason. + +import ( + "errors" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/pashagolub/pgxmock/v4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + enableMFAUpdateRegex = `(?s)UPDATE users SET mfa_enabled = true, mfa_secret = \$2.*WHERE id = \$1 AND mfa_enabled = false AND mfa_pending_secret = \$2\s+AND mfa_pending_secret_expires_at > NOW\(\)` + enableMFARefusalRegex = `(?s)SELECT \(mfa_pending_secret_expires_at <= NOW\(\)\) IS TRUE FROM users\s+WHERE id = \$1 AND mfa_enabled = false AND mfa_pending_secret = \$2` +) + +func TestPGXMock_EnableMFA_RefusalReason(t *testing.T) { + hashes := []string{"hash-1"} + tests := []struct { + name string + diagnose func(pgxmock.PgxPoolIface) + want error + wantExpired bool + }{ + { + name: "expired-pending-secret", + diagnose: func(m pgxmock.PgxPoolIface) { + m.ExpectQuery(enableMFARefusalRegex).WithArgs("user-1", "PENDING"). + WillReturnRows(pgxmock.NewRows([]string{"expired"}).AddRow(true)) + }, + want: ErrMFAEnrollmentExpired, + wantExpired: true, + }, + { + name: "same-pending-secret-not-expired", + diagnose: func(m pgxmock.PgxPoolIface) { + m.ExpectQuery(enableMFARefusalRegex).WithArgs("user-1", "PENDING"). + WillReturnRows(pgxmock.NewRows([]string{"expired"}).AddRow(false)) + }, + want: ErrUserChanged, + }, + { + name: "pending-secret-gone-or-mfa-on", + diagnose: func(m pgxmock.PgxPoolIface) { + m.ExpectQuery(enableMFARefusalRegex).WithArgs("user-1", "PENDING").WillReturnError(pgx.ErrNoRows) + }, + want: ErrUserChanged, + }, + { + name: "diagnosis-query-fails", + diagnose: func(m pgxmock.PgxPoolIface) { + m.ExpectQuery(enableMFARefusalRegex).WithArgs("user-1", "PENDING").WillReturnError(errors.New("connection reset")) + }, + want: ErrUserChanged, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + mock.ExpectExec(enableMFAUpdateRegex).WithArgs("user-1", "PENDING", hashes, int64(7)). + WillReturnResult(pgxmock.NewResult("UPDATE", 0)) + tt.diagnose(mock) + + err := store.EnableMFA(t.Context(), "user-1", "PENDING", 7, hashes) + require.ErrorIs(t, err, tt.want) + assert.Equal(t, tt.wantExpired, errors.Is(err, ErrMFAEnrollmentExpired)) + assert.Equal(t, !tt.wantExpired, errors.Is(err, ErrUserChanged)) + }) + } +} + +func TestPGXMock_EnableMFA_SuccessSkipsDiagnosis(t *testing.T) { + mock := newAuthPgxMock(t) + store := NewPostgresStore(mock) + mock.ExpectExec(enableMFAUpdateRegex).WithArgs("user-1", "PENDING", []string{"hash-1"}, int64(7)). + WillReturnResult(pgxmock.NewResult("UPDATE", 1)) + + require.NoError(t, store.EnableMFA(t.Context(), "user-1", "PENDING", 7, []string{"hash-1"})) +}