fix(auth): report an expired MFA enrollment as expired and pin the MFA write guards

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))
 			}
 		})
 	}

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

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)
+		}
+	})
 }

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)

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 {