fix(auth): refuse MFA setup and enable while MFA is enabled

internal/api/handler_auth.go

@@ -547,6 +547,10 @@ func mapMFAServiceError(err error) error {
 		errors.Is(err, auth.ErrMFAEnrollmentExpired),
 		errors.Is(err, auth.ErrMFANotEnabled):
 		return NewClientError(400, err.Error())
+	case errors.Is(err, auth.ErrMFAAlreadyEnabled):
+		return NewClientError(409, auth.ErrMFAAlreadyEnabled.Error())
+	case errors.Is(err, auth.ErrUserChanged):
+		return NewClientError(409, auth.ErrUserChanged.Error())
 	case errors.Is(err, auth.ErrMFAAuthFailed):
 		// Opaque 401 to prevent user enumeration: the service returns this
 		// for both "user not found" and "DB lookup failed" paths so callers

internal/api/handler_auth_test.go

@@ -1675,6 +1675,29 @@ func TestMapMFAServiceError_AuthFailed_Is401(t *testing.T) {
 	assert.Contains(t, ce.Error(), auth.ErrMFAAuthFailed.Error())
 }
 
+// Issue #227: refusing to replace an enabled factor, and losing an enrollment
+// race, are client conflicts rather than 500s.
+func TestHandler_mfaSetupAndEnable_ConflictsAre409(t *testing.T) {
+	ctx := context.Background()
+	for _, sentinel := range []error{auth.ErrMFAAlreadyEnabled, auth.ErrUserChanged} {
+		mockAuth := new(MockAuthService)
+		mockAuth.On("ValidateSession", ctx, "tok").Return(&Session{UserID: "user-1"}, nil)
+		mockAuth.On("MFASetupAPI", ctx, "user-1", "pw").Return("", "", fmt.Errorf("persist: %w", sentinel))
+		mockAuth.On("MFAEnableAPI", ctx, "user-1", "123456").Return(nil, fmt.Errorf("persist: %w", sentinel))
+		handler := &Handler{auth: mockAuth}
+
+		_, setupErr := handler.mfaSetup(ctx, authedReq("tok", `{"password":"`+b64("pw")+`"}`))
+		_, enableErr := handler.mfaEnable(ctx, authedReq("tok", `{"code":"123456"}`))
+		for _, err := range []error{setupErr, enableErr} {
+			ce, ok := IsClientError(err)
+			require.True(t, ok, "%v must be a client error", sentinel)
+			assert.Equal(t, 409, ce.code)
+			assert.Equal(t, sentinel.Error(), ce.Error())
+		}
+		mockAuth.AssertExpectations(t)
+	}
+}
+
 // Issue #94: every MFA route that checks a password, TOTP or recovery code
 // must be throttled per IP and per session user.
 func TestHandler_mfaEndpoints_RateLimited(t *testing.T) {

internal/auth/errors.go

@@ -127,6 +127,10 @@ var (
 	ErrMFANotEnabled             = errors.New("MFA is not enabled")
 	ErrMFAAuthFailed             = errors.New("authentication failed")
 
+	// ErrMFAAlreadyEnabled: setup or enable on an account whose MFA is on.
+	// Replacing a factor needs the disable proof first (issue #227). Mapped to 409.
+	ErrMFAAlreadyEnabled = errors.New("MFA is already enabled; disable it before enrolling a new authenticator")
+
 	// API key creation sentinels — returned (wrapped via fmt.Errorf "%w") by
 	// CreateAPIKey so the API handler can map each to the right HTTP status
 	// via errors.Is rather than substring matching. See issue #102.

internal/auth/interfaces.go

@@ -17,6 +17,9 @@ type StoreInterface interface {
 	CompletePasswordReset(ctx context.Context, user *User, readResetToken, readPasswordHash string) error
 	ConsumePasswordResetToken(ctx context.Context, userID, readResetToken string) error
 	ConsumeMFARecoveryCode(ctx context.Context, userID string, readCodes, remaining []string) error
+	SetPendingMFASecret(ctx context.Context, userID, secret string, expiresAt time.Time) error
+	EnableMFA(ctx context.Context, userID, pendingSecret string, recoveryHashes []string) error
+	ClearPendingMFASecret(ctx context.Context, userID string) error
 	RecordFailedLogin(ctx context.Context, userID string) error
 	RecordSuccessfulLogin(ctx context.Context, userID string) error
 	DeleteUser(ctx context.Context, userID string) error

internal/auth/service_mfa.go

@@ -12,7 +12,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
 	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
 	"golang.org/x/crypto/bcrypt"
 )
@@ -278,10 +277,9 @@ type MFASetupResult struct {
 // that happens in MFAEnable after the user proves they have the
 // secret loaded in their authenticator.
 //
-// Safe to call repeatedly: each call overwrites the previous pending
-// secret and resets the expiry. An abandoned enrollment expires
-// harmlessly because the active MFASecret + MFAEnabled fields are
-// untouched.
+// Safe to call repeatedly while MFA is off: each call overwrites the
+// previous pending secret and resets the expiry. Refused while MFA is
+// on, so replacing a factor needs MFADisable's proof first (issue #227).
 func (s *Service) MFASetup(ctx context.Context, userID, password string) (*MFASetupResult, error) {
 	if err := s.ensureStore(); err != nil {
 		return nil, err
@@ -296,17 +294,17 @@ func (s *Service) MFASetup(ctx context.Context, userID, password string) (*MFASe
 	if !s.verifyPassword(password, user.PasswordHash) {
 		return nil, fmt.Errorf("%w", ErrMFAInvalidPassword)
 	}
+	if user.MFAEnabled {
+		return nil, fmt.Errorf("%w", ErrMFAAlreadyEnabled)
+	}
 
 	secret, err := generateMFASecret()
 	if err != nil {
 		return nil, err
 	}
 
-	expiresAt := time.Now().Add(mfaPendingExpiry)
-	user.MFAPendingSecret = secret
-	user.MFAPendingSecretExpiresAt = &expiresAt
-	if err := s.store.UpdateUser(ctx, user); err != nil {
-		return nil, fmt.Errorf("failed to persist pending MFA secret: %w", err)
+	if err := s.store.SetPendingMFASecret(ctx, user.ID, secret, time.Now().Add(mfaPendingExpiry)); err != nil {
+		return nil, err
 	}
 
 	return &MFASetupResult{
@@ -337,19 +335,13 @@ func (s *Service) generateAndHashRecoveryCodes() (plaintext, hashes []string, er
 }
 
 // validatePendingMFAEnrollment checks that a pending enrollment
-// exists, hasn't expired, and the supplied code matches. Mutates
-// user on expiry to wipe stale pending fields so a re-probe returns
-// "no enrollment in progress" rather than "expired" forever.
-func (s *Service) validatePendingMFAEnrollment(ctx context.Context, user *User, code string) error {
+// exists, hasn't expired, and the supplied code matches. It writes
+// nothing: the next setup overwrites an expired pending secret.
+func validatePendingMFAEnrollment(user *User, code string) error {
 	if user.MFAPendingSecret == "" || user.MFAPendingSecretExpiresAt == nil {
 		return fmt.Errorf("%w", ErrMFANoEnrollmentInProgress)
 	}
 	if time.Now().After(*user.MFAPendingSecretExpiresAt) {
-		user.MFAPendingSecret = ""
-		user.MFAPendingSecretExpiresAt = nil
-		if updateErr := s.store.UpdateUser(ctx, user); updateErr != nil {
-			logging.Warnf("MFAVerifyPendingCode: failed to clear expired pending secret for user: %v", updateErr)
-		}
 		return fmt.Errorf("%w", ErrMFAEnrollmentExpired)
 	}
 	if !verifyTOTP(user.MFAPendingSecret, code) {
@@ -369,9 +361,8 @@ func (s *Service) validatePendingMFAEnrollment(ctx context.Context, user *User,
 //   - the pending secret has expired
 //   - the supplied code doesn't match the pending secret
 //
-// Idempotent in the sense that a second enable on an already-enabled
-// user with no pending secret returns "no MFA enrollment in progress",
-// not a silent re-enable with new recovery codes.
+// A user whose MFA is already on gets ErrMFAAlreadyEnabled, never a
+// silent re-enable with a new secret or new recovery codes.
 func (s *Service) MFAEnable(ctx context.Context, userID, code string) ([]string, error) {
 	if err := s.ensureStore(); err != nil {
 		return nil, err
@@ -380,7 +371,10 @@ func (s *Service) MFAEnable(ctx context.Context, userID, code string) ([]string,
 	if err != nil || user == nil {
 		return nil, fmt.Errorf("%w", ErrMFAAuthFailed)
 	}
-	err = s.validatePendingMFAEnrollment(ctx, user, code)
+	if user.MFAEnabled {
+		return nil, fmt.Errorf("%w", ErrMFAAlreadyEnabled)
+	}
+	err = validatePendingMFAEnrollment(user, code)
 	if err != nil {
 		return nil, err
 	}
@@ -389,14 +383,8 @@ func (s *Service) MFAEnable(ctx context.Context, userID, code string) ([]string,
 	if err != nil {
 		return nil, err
 	}
-
-	user.MFASecret = user.MFAPendingSecret
-	user.MFAEnabled = true
-	user.MFAPendingSecret = ""
-	user.MFAPendingSecretExpiresAt = nil
-	user.MFARecoveryCodes = hashes
-	if err := s.store.UpdateUser(ctx, user); err != nil {
-		return nil, fmt.Errorf("failed to enable MFA: %w", err)
+	if err := s.store.EnableMFA(ctx, user.ID, user.MFAPendingSecret, hashes); err != nil {
+		return nil, err
 	}
 	return plaintext, nil
 }
@@ -414,14 +402,9 @@ func clearMFAFromUser(user *User) {
 
 // disableMFAAlreadyOff is the idempotent path for MFADisable: the
 // user already has MFA off, so we only need to clear any stale
-// pending fields and persist.
+// pending fields.
 func (s *Service) disableMFAAlreadyOff(ctx context.Context, user *User) error {
-	user.MFAPendingSecret = ""
-	user.MFAPendingSecretExpiresAt = nil
-	if err := s.store.UpdateUser(ctx, user); err != nil {
-		return fmt.Errorf("failed to disable MFA: %w", err)
-	}
-	return nil
+	return s.store.ClearPendingMFASecret(ctx, user.ID)
 }
 
 // MFADisable turns off MFA for a user. Requires both the current

internal/auth/service_mfa_db_test.go

@@ -0,0 +1,136 @@
+//go:build integration
+
+package auth
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Issue #227: a session plus the password must not replace an enabled factor,
+// sequentially or by racing the victim's enrollment.
+func TestIntegration_MFAReplacementNeedsCurrentFactor(t *testing.T) {
+	store := NewPostgresStore(setupAuthTestDB(t))
+	ctx := t.Context()
+
+	t.Run("setup-and-enable-refused-while-enabled", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-replace@example.com")
+		victimCodes := f.enrollMFA()
+		enrolled := f.stored()
+
+		_, err := f.svc.MFASetup(ctx, f.user.ID, credentialRacePassword)
+		require.ErrorIs(t, err, ErrMFAAlreadyEnabled)
+		assert.Equal(t, enrolled, f.stored())
+
+		// Pending material left behind (e.g. by the pre-fix setup) must not be promotable.
+		attacker, err := generateMFASecret()
+		require.NoError(t, err)
+		_, err = store.db.Exec(ctx, "UPDATE users SET mfa_pending_secret = $2, mfa_pending_secret_expires_at = NOW() + interval '5 minutes' WHERE id = $1", f.user.ID, attacker)
+		require.NoError(t, err)
+		withPending := f.stored()
+		_, err = f.svc.MFAEnable(ctx, f.user.ID, generateTOTP(attacker, time.Now().Unix()/30))
+		require.ErrorIs(t, err, ErrMFAAlreadyEnabled)
+		assert.Equal(t, withPending, f.stored())
+
+		f.requireFactorEnforced(f.user.Email, credentialRacePassword, enrolled)
+		_, err = f.svc.Login(ctx, LoginRequest{Email: f.user.Email, Password: credentialRacePassword, MFACode: victimCodes[0]})
+		require.NoError(t, err, "the victim's recovery codes must survive")
+	})
+
+	t.Run("re-enroll-after-disable-with-current-factor", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-reenroll@example.com")
+		f.enrollMFA()
+		first := f.stored()
+		require.NoError(t, f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, generateTOTP(first.MFASecret, time.Now().Unix()/30)))
+		f.enrollMFA()
+		second := f.stored()
+		assert.NotEqual(t, first.MFASecret, second.MFASecret)
+		f.requireFactorEnforced(f.user.Email, credentialRacePassword, second)
+	})
+
+	t.Run("stale-setup-after-concurrent-enrollment", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-stale-setup@example.com")
+		enrolled := f.enrollOnRead()
+		_, err := f.svc.MFASetup(ctx, f.user.ID, credentialRacePassword)
+		require.ErrorIs(t, err, ErrUserChanged)
+		assert.Equal(t, enrolled(), f.stored())
+		f.requireFactorEnforced(f.user.Email, credentialRacePassword, enrolled())
+	})
+
+	t.Run("stale-password-only-disable-after-concurrent-enrollment", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-stale-disable@example.com")
+		enrolled := f.enrollOnRead()
+		err := f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, "")
+		require.ErrorIs(t, err, ErrUserChanged)
+		assert.Equal(t, enrolled(), f.stored())
+		f.requireFactorEnforced(f.user.Email, credentialRacePassword, enrolled())
+	})
+
+	t.Run("stale-enable-after-concurrent-setup", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-stale-enable@example.com")
+		setupA, err := f.svc.MFASetup(ctx, f.user.ID, credentialRacePassword)
+		require.NoError(t, err)
+		winner := f.onRead(func(ctx context.Context, other *Service) {
+			_, setupErr := other.MFASetup(ctx, f.user.ID, credentialRacePassword)
+			require.NoError(t, setupErr)
+		})
+		codes, err := f.svc.MFAEnable(ctx, f.user.ID, generateTOTP(setupA.Secret, time.Now().Unix()/30))
+		require.ErrorIs(t, err, ErrUserChanged)
+		assert.Nil(t, codes)
+		assert.Equal(t, winner(), f.stored())
+	})
+
+	t.Run("stale-enable-after-concurrent-enable", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-double-enable@example.com")
+		setup, err := f.svc.MFASetup(ctx, f.user.ID, credentialRacePassword)
+		require.NoError(t, err)
+		code := generateTOTP(setup.Secret, time.Now().Unix()/30)
+		winner := f.onRead(func(ctx context.Context, other *Service) {
+			_, enableErr := other.MFAEnable(ctx, f.user.ID, code)
+			require.NoError(t, enableErr)
+		})
+		codes, err := f.svc.MFAEnable(ctx, f.user.ID, code)
+		require.ErrorIs(t, err, ErrUserChanged)
+		assert.Nil(t, codes)
+		assert.Equal(t, winner(), f.stored(), "the loser must not replace the winner's recovery codes")
+	})
+
+	// Only reachable with legacy data: MFA on while the verified pending secret
+	// is still stored, so the enabled predicate alone must refuse the write.
+	t.Run("stale-enable-with-enabled-row-and-matching-pending", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-enabled-pending@example.com")
+		setupA, err := f.svc.MFASetup(ctx, f.user.ID, credentialRacePassword)
+		require.NoError(t, err)
+		victim, err := generateMFASecret()
+		require.NoError(t, err)
+		winner := f.onRead(func(ctx context.Context, _ *Service) {
+			_, execErr := store.db.Exec(ctx, "UPDATE users SET mfa_enabled = true, mfa_secret = $2 WHERE id = $1", f.user.ID, victim)
+			require.NoError(t, execErr)
+		})
+		codes, err := f.svc.MFAEnable(ctx, f.user.ID, generateTOTP(setupA.Secret, time.Now().Unix()/30))
+		require.ErrorIs(t, err, ErrUserChanged)
+		assert.Nil(t, codes)
+		stored := winner()
+		assert.Equal(t, stored, f.stored())
+		assert.True(t, stored.MFAEnabled)
+		assert.Equal(t, victim, stored.MFASecret, "the victim's factor must not be overwritten")
+		assert.Equal(t, setupA.Secret, stored.MFAPendingSecret, "the write must not have touched the row")
+	})
+
+	t.Run("expired-enable-after-concurrent-enrollment", func(t *testing.T) {
+		f := newCredentialRaceFixture(t, store, "mfa-expired-enable@example.com")
+		stale, err := generateMFASecret()
+		require.NoError(t, err)
+		_, err = store.db.Exec(ctx, "UPDATE users SET mfa_pending_secret = $2, mfa_pending_secret_expires_at = NOW() - interval '1 minute' WHERE id = $1", f.user.ID, stale)
+		require.NoError(t, err)
+		enrolled := f.enrollOnRead()
+		_, err = f.svc.MFAEnable(ctx, f.user.ID, generateTOTP(stale, time.Now().Unix()/30))
+		require.ErrorIs(t, err, ErrMFAEnrollmentExpired)
+		assert.Equal(t, enrolled(), f.stored())
+		f.requireFactorEnforced(f.user.Email, credentialRacePassword, enrolled())
+	})
+}