diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index f18e93692..b764724ab 100644 --- a/internal/api/handler_auth.go +++ b/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 diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 4e90e60ea..c25be6b1b 100644 --- a/internal/api/handler_auth_test.go +++ b/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) { diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 806dd844f..a6868026a 100644 --- a/internal/auth/errors.go +++ b/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. diff --git a/internal/auth/interfaces.go b/internal/auth/interfaces.go index 4b90874cd..fea2069d4 100644 --- a/internal/auth/interfaces.go +++ b/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 diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index bacd7f71a..b41431d96 100644 --- a/internal/auth/service_mfa.go +++ b/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 diff --git a/internal/auth/service_mfa_db_test.go b/internal/auth/service_mfa_db_test.go new file mode 100644 index 000000000..b74a0cb38 --- /dev/null +++ b/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()) + }) +} diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 67168c8ef..4b6cf5412 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -211,16 +211,15 @@ func TestMFASetup_HappyPath(t *testing.T) { user := createTestUser(t, "SecurePass@123") user.MFAEnabled = false mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) - mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Run(func(args mock.Arguments) { - u := args.Get(1).(*User) - assert.NotEmpty(t, u.MFAPendingSecret, "setup should persist a pending secret") - assert.NotNil(t, u.MFAPendingSecretExpiresAt) - assert.False(t, u.MFAEnabled, "setup must not flip MFAEnabled") + var pending string + mockStore.On("SetPendingMFASecret", ctx, user.ID, mock.AnythingOfType("string"), mock.AnythingOfType("time.Time")).Run(func(args mock.Arguments) { + pending = args.String(2) + assert.WithinDuration(t, time.Now().Add(mfaPendingExpiry), args.Get(3).(time.Time), time.Minute) }).Return(nil).Once() res, err := service.MFASetup(ctx, user.ID, "SecurePass@123") require.NoError(t, err) - assert.NotEmpty(t, res.Secret) + assert.Equal(t, pending, res.Secret, "the returned secret must be the persisted pending one") assert.True(t, strings.HasPrefix(res.ProvisioningURI, "otpauth://totp/")) mockStore.AssertExpectations(t) } @@ -251,13 +250,8 @@ func TestMFAEnable_HappyPath(t *testing.T) { user.MFAPendingSecretExpiresAt = &expiresAt mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) - mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Run(func(args mock.Arguments) { - u := args.Get(1).(*User) - assert.True(t, u.MFAEnabled) - assert.Equal(t, secret, u.MFASecret) - assert.Empty(t, u.MFAPendingSecret, "pending secret must be cleared on enable") - assert.Nil(t, u.MFAPendingSecretExpiresAt) - assert.Len(t, u.MFARecoveryCodes, recoveryCodeCount) + mockStore.On("EnableMFA", ctx, user.ID, secret, mock.AnythingOfType("[]string")).Run(func(args mock.Arguments) { + assert.Len(t, args.Get(3).([]string), recoveryCodeCount) }).Return(nil).Once() codes, err := service.MFAEnable(ctx, user.ID, totpFor(secret)) @@ -277,12 +271,11 @@ func TestMFAEnable_ExpiredPending(t *testing.T) { user.MFAPendingSecret = secret user.MFAPendingSecretExpiresAt = &expiresAt mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) - // Stale pending fields get wiped on expiry; allow but don't require. - mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() _, err := service.MFAEnable(ctx, user.ID, totpFor(secret)) require.Error(t, err) assert.Contains(t, err.Error(), "expired") + mockStore.AssertExpectations(t) // an expired enrollment is rejected without a write } func TestMFAEnable_NoPending(t *testing.T) { @@ -385,10 +378,47 @@ func TestMFADisable_IdempotentWhenAlreadyDisabled(t *testing.T) { user := createTestUser(t, "SecurePass@123") user.MFAEnabled = false mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) - mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + mockStore.On("ClearPendingMFASecret", ctx, user.ID).Return(nil).Once() err := service.MFADisable(ctx, user.ID, "SecurePass@123", "") require.NoError(t, err) + mockStore.AssertExpectations(t) +} + +func TestMFASetupAndEnable_RefusedWhileEnabled(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + pending := "GEZDGNBVGY3TQOJQ" + expiresAt := time.Now().Add(mfaPendingExpiry) + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + user.MFASecret = "JBSWY3DPEHPK3PXP" + user.MFAPendingSecret = pending + user.MFAPendingSecretExpiresAt = &expiresAt + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + res, err := service.MFASetup(ctx, user.ID, "SecurePass@123") + require.ErrorIs(t, err, ErrMFAAlreadyEnabled) + assert.Nil(t, res) + codes, err := service.MFAEnable(ctx, user.ID, totpFor(pending)) + require.ErrorIs(t, err, ErrMFAAlreadyEnabled) + assert.Nil(t, codes) + mockStore.AssertExpectations(t) // no write reached the store +} + +func TestMFASetup_WrongPasswordWhileEnabled_ReportsPassword(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + user := createTestUser(t, "SecurePass@123") + user.MFAEnabled = true + mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) + + _, err := service.MFASetup(ctx, user.ID, "WrongPassword!@#") + require.ErrorIs(t, err, ErrMFAInvalidPassword) } func TestMFARegenerateRecoveryCodes_HappyPath(t *testing.T) { @@ -550,7 +580,6 @@ func TestMFAEnable_ExpiredPending_ReturnsSentinel(t *testing.T) { user.MFAPendingSecret = secret user.MFAPendingSecretExpiresAt = &expiresAt mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil) - mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() _, err := service.MFAEnable(ctx, user.ID, totpFor(secret)) require.Error(t, err) diff --git a/internal/auth/store_postgres_credentials.go b/internal/auth/store_postgres_credentials.go index 234b027de..c2d1b2927 100644 --- a/internal/auth/store_postgres_credentials.go +++ b/internal/auth/store_postgres_credentials.go @@ -73,6 +73,55 @@ func (s *PostgresStore) ConsumeMFARecoveryCode(ctx context.Context, userID strin return nil } +// SetPendingMFASecret writes only the pending enrollment, and only while MFA is +// off, so a stale setup cannot overwrite a concurrent enrollment (issue #227). +func (s *PostgresStore) SetPendingMFASecret(ctx context.Context, userID, secret string, expiresAt time.Time) error { + result, err := s.db.Exec(ctx, ` + UPDATE users SET mfa_pending_secret = $2, mfa_pending_secret_expires_at = $3, updated_at = NOW() + WHERE id = $1 AND mfa_enabled = false + `, userID, secret, expiresAt) + if err != nil { + return fmt.Errorf("failed to persist pending MFA secret: %w", err) + } + if result.RowsAffected() == 0 { + return ErrUserChanged + } + return nil +} + +// EnableMFA promotes the pending secret the caller verified, only while MFA is +// still off and that secret is still the pending one (issue #227). +func (s *PostgresStore) EnableMFA(ctx context.Context, userID, pendingSecret string, recoveryHashes []string) error { + result, err := s.db.Exec(ctx, ` + UPDATE users SET mfa_enabled = true, mfa_secret = $2, mfa_recovery_codes = $3, + mfa_pending_secret = '', mfa_pending_secret_expires_at = NULL, updated_at = NOW() + WHERE id = $1 AND mfa_enabled = false AND mfa_pending_secret = $2 + `, userID, pendingSecret, recoveryHashes) + if err != nil { + return fmt.Errorf("failed to enable MFA: %w", err) + } + if result.RowsAffected() == 0 { + return ErrUserChanged + } + return nil +} + +// ClearPendingMFASecret cancels a pending enrollment, only while MFA is off, so +// a password-only cancel cannot erase a concurrent enrollment (issue #227). +func (s *PostgresStore) ClearPendingMFASecret(ctx context.Context, userID string) error { + result, err := s.db.Exec(ctx, ` + UPDATE users SET mfa_pending_secret = '', mfa_pending_secret_expires_at = NULL, updated_at = NOW() + WHERE id = $1 AND mfa_enabled = false + `, userID) + if err != nil { + return fmt.Errorf("failed to clear pending MFA secret: %w", err) + } + if result.RowsAffected() == 0 { + return ErrUserChanged + } + return nil +} + // SetPasswordResetToken writes only the reset token columns, and only while the // account is still active under the same email with the reset expiry the caller // read, so a concurrent deactivation or reset issuance wins. diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index d06b2ef9f..56ab4f1b4 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -70,6 +70,18 @@ func (m *MockStore) ConsumeMFARecoveryCode(ctx context.Context, userID string, r return m.Called(ctx, userID, readCodes, remaining).Error(0) } +func (m *MockStore) SetPendingMFASecret(ctx context.Context, userID, secret string, expiresAt time.Time) error { + return m.Called(ctx, userID, secret, expiresAt).Error(0) +} + +func (m *MockStore) EnableMFA(ctx context.Context, userID, pendingSecret string, recoveryHashes []string) error { + return m.Called(ctx, userID, pendingSecret, recoveryHashes).Error(0) +} + +func (m *MockStore) ClearPendingMFASecret(ctx context.Context, userID string) error { + return m.Called(ctx, userID).Error(0) +} + func (m *MockStore) RecordFailedLogin(ctx context.Context, userID string) error { return m.Called(ctx, userID).Error(0) } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index e2ee4b802..36ea87d6b 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -817,6 +817,18 @@ func (m *MockAuthStore) ConsumeMFARecoveryCode(ctx context.Context, userID strin return m.Called(ctx, userID, readCodes, remaining).Error(0) } +func (m *MockAuthStore) SetPendingMFASecret(ctx context.Context, userID, secret string, expiresAt time.Time) error { + return m.Called(ctx, userID, secret, expiresAt).Error(0) +} + +func (m *MockAuthStore) EnableMFA(ctx context.Context, userID, pendingSecret string, recoveryHashes []string) error { + return m.Called(ctx, userID, pendingSecret, recoveryHashes).Error(0) +} + +func (m *MockAuthStore) ClearPendingMFASecret(ctx context.Context, userID string) error { + return m.Called(ctx, userID).Error(0) +} + func (m *MockAuthStore) RecordFailedLogin(ctx context.Context, userID string) error { return m.Called(ctx, userID).Error(0) } diff --git a/internal/server/health_test.go b/internal/server/health_test.go index 67192af7c..0c15ddd99 100644 --- a/internal/server/health_test.go +++ b/internal/server/health_test.go @@ -52,6 +52,18 @@ func (m *mockAuthStoreForHealth) ConsumeMFARecoveryCode(context.Context, string, return nil } +func (m *mockAuthStoreForHealth) SetPendingMFASecret(context.Context, string, string, time.Time) error { + return nil +} + +func (m *mockAuthStoreForHealth) EnableMFA(context.Context, string, string, []string) error { + return nil +} + +func (m *mockAuthStoreForHealth) ClearPendingMFASecret(context.Context, string) error { + return nil +} + func (m *mockAuthStoreForHealth) RecordFailedLogin(context.Context, string) error { return nil }