diff --git a/internal/auth/service_login_db_test.go b/internal/auth/service_login_db_test.go index 0774bbf2..7c93cb0b 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 9c32c58b..38ac1c9b 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 81c7f91c..1e1c9c4a 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 dccb8dfd..8414f87e 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 e0dfd632..53284502 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 00000000..0395250f --- /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"})) +}