From eae19ebf447d5198dc92f7507be9bd6197b16dd4 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 08:31:38 +0100 Subject: [PATCH 1/9] fix(auth): delete the MFA secret and backup codes in one transaction (#754) Disabling MFA ran two writes outside a transaction: a failure on the second left backup codes alive after their secret was gone. Both now go in one transaction, and the secret is hard-deleted so re-enrolment does not trip the unique user_id constraint on a tombstone. Signed-off-by: alex-dembele --- .../application/auth/mfa_usecase_test.go | 1 + .../repository/gorm_mfa_repository.go | 23 +++- .../repository/gorm_mfa_repository_test.go | 123 ++++++++++++++++++ .../repository/mfa_repository_interface.go | 1 + 4 files changed, 144 insertions(+), 4 deletions(-) create mode 100644 backend/internal/infrastructure/repository/gorm_mfa_repository_test.go diff --git a/backend/internal/application/auth/mfa_usecase_test.go b/backend/internal/application/auth/mfa_usecase_test.go index ec3bc47c..9356cf72 100644 --- a/backend/internal/application/auth/mfa_usecase_test.go +++ b/backend/internal/application/auth/mfa_usecase_test.go @@ -50,6 +50,7 @@ func (m *MockMFARepository) UpdateMFASecret(ctx context.Context, secret *domain. func (m *MockMFARepository) DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error { key := userID.String() + ":" + tenantID.String() delete(m.secrets, key) + delete(m.codes, key) return nil } diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository.go b/backend/internal/infrastructure/repository/gorm_mfa_repository.go index 93ffcd09..9fd29ec4 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_repository.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository.go @@ -51,11 +51,26 @@ func (r *GormMFARepository) UpdateMFASecret(ctx context.Context, secret *domain. return r.db.WithContext(ctx).Save(secret).Error } -// DisableMFA disables MFA for user (soft delete) +// DisableMFA removes the TOTP secret and every backup code of one user, in one +// transaction (#754). +// +// Both or neither: backup codes that survive their secret are a second factor +// nobody can see or revoke from the UI, and a secret that survives its codes +// leaves an account the user believes is unprotected still demanding a code. +// +// The secret is hard-deleted. It is key material, so a soft-deleted copy has no +// business lingering, and mfa_secrets.user_id is UNIQUE without a deleted_at +// clause — a tombstone would make the next enrolment fail on that constraint. func (r *GormMFARepository) DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error { - return r.db.WithContext(ctx). - Where("user_id = ? AND tenant_id = ?", userID, tenantID). - Delete(&domain.MFASecret{}).Error + return r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + if err := tx.Unscoped(). + Where("user_id = ? AND tenant_id = ?", userID, tenantID). + Delete(&domain.MFASecret{}).Error; err != nil { + return err + } + return tx.Where("user_id = ? AND tenant_id = ?", userID, tenantID). + Delete(&domain.MFABackupCode{}).Error + }) } // SaveBackupCodes saves backup codes in batch diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository_test.go b/backend/internal/infrastructure/repository/gorm_mfa_repository_test.go new file mode 100644 index 00000000..3fa3268a --- /dev/null +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository_test.go @@ -0,0 +1,123 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package repository + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/testsupport/sqliteschema" +) + +// #754 — DisableMFA removes the secret and the backup codes in one transaction. + +func setupMFARepo(t *testing.T) (*GormMFARepository, *gorm.DB) { + t.Helper() + dsn := "file:mfa_" + uuid.New().String() + "?mode=memory&cache=private" + db, err := gorm.Open(sqlite.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + for _, m := range []struct { + table string + model any + }{ + {"mfa_secrets", &domain.MFASecret{}}, + {"mfa_backup_codes", &domain.MFABackupCode{}}, + } { + require.NoError(t, db.Exec(`CREATE TABLE `+m.table+` (id TEXT PRIMARY KEY)`).Error) + require.NoError(t, sqliteschema.Reconcile(db, m.table, m.model)) + } + return NewGormMFARepository(db), db +} + +func seedMFA(t *testing.T, db *gorm.DB, userID, tenantID uuid.UUID) { + t.Helper() + require.NoError(t, db.Create(&domain.MFASecret{ID: uuid.New(), UserID: userID, TenantID: tenantID, SecretEncrypted: "x", IsVerified: true}).Error) + for i := 0; i < 3; i++ { + require.NoError(t, db.Create(&domain.MFABackupCode{ID: uuid.New(), UserID: userID, TenantID: tenantID, CodeHash: uuid.NewString()}).Error) + } +} + +// countMFA counts rows including soft-deleted ones: a tombstone is not a deletion. +func countMFA(t *testing.T, db *gorm.DB, userID uuid.UUID) (secrets, codes int64) { + t.Helper() + require.NoError(t, db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", userID).Count(&secrets).Error) + require.NoError(t, db.Model(&domain.MFABackupCode{}).Where("user_id = ?", userID).Count(&codes).Error) + return secrets, codes +} + +func TestGormMFARepository_DisableMFA_DeletesSecretAndCodes(t *testing.T) { + repo, db := setupMFARepo(t) + user, tenant, other := uuid.New(), uuid.New(), uuid.New() + seedMFA(t, db, user, tenant) + seedMFA(t, db, other, tenant) + + require.NoError(t, repo.DisableMFA(context.Background(), user, tenant)) + + secrets, codes := countMFA(t, db, user) + assert.Zero(t, secrets, "the secret must be gone, not soft-deleted") + assert.Zero(t, codes) + secrets, codes = countMFA(t, db, other) + assert.EqualValues(t, 1, secrets, "another user's factor is untouched") + assert.EqualValues(t, 3, codes) +} + +func TestGormMFARepository_DisableMFA_IsTenantScoped(t *testing.T) { + repo, db := setupMFARepo(t) + user, tenant := uuid.New(), uuid.New() + seedMFA(t, db, user, tenant) + + require.NoError(t, repo.DisableMFA(context.Background(), user, uuid.New())) + + secrets, codes := countMFA(t, db, user) + assert.EqualValues(t, 1, secrets) + assert.EqualValues(t, 3, codes) +} + +func TestGormMFARepository_DisableMFA_RollsBackWhenTheSecondWriteFails(t *testing.T) { + repo, db := setupMFARepo(t) + user, tenant := uuid.New(), uuid.New() + seedMFA(t, db, user, tenant) + + // Fail the backup-code delete, which runs after the secret delete. + boom := errors.New("backup code delete failed") + require.NoError(t, db.Callback().Delete().Before("gorm:delete").Register("test:fail_codes", func(tx *gorm.DB) { + if tx.Statement.Table == "mfa_backup_codes" { + _ = tx.AddError(boom) + } + })) + + err := repo.DisableMFA(context.Background(), user, tenant) + require.ErrorIs(t, err, boom) + + secrets, codes := countMFA(t, db, user) + assert.EqualValues(t, 1, secrets, "the secret delete must roll back") + assert.EqualValues(t, 3, codes) + + var s domain.MFASecret + require.NoError(t, db.Where("user_id = ?", user).First(&s).Error, "the secret is still live, not a tombstone") +} + +func TestGormMFARepository_DisableMFA_AllowsReenrolment(t *testing.T) { + repo, db := setupMFARepo(t) + user, tenant := uuid.New(), uuid.New() + seedMFA(t, db, user, tenant) + require.NoError(t, db.Exec(`CREATE UNIQUE INDEX ux_mfa_user ON mfa_secrets (user_id)`).Error) + + require.NoError(t, repo.DisableMFA(context.Background(), user, tenant)) + + // Production's mfa_secrets.user_id is UNIQUE with no deleted_at clause. + require.NoError(t, repo.CreateMFASecret(context.Background(), + &domain.MFASecret{ID: uuid.New(), UserID: user, TenantID: tenant, SecretEncrypted: "y"})) +} diff --git a/backend/internal/infrastructure/repository/mfa_repository_interface.go b/backend/internal/infrastructure/repository/mfa_repository_interface.go index ba76870e..c69e10e5 100644 --- a/backend/internal/infrastructure/repository/mfa_repository_interface.go +++ b/backend/internal/infrastructure/repository/mfa_repository_interface.go @@ -18,6 +18,7 @@ type MFARepository interface { CreateMFASecret(ctx context.Context, secret *domain.MFASecret) error GetMFASecret(ctx context.Context, userID, tenantID uuid.UUID) (*domain.MFASecret, error) UpdateMFASecret(ctx context.Context, secret *domain.MFASecret) error + // DisableMFA deletes the secret AND the backup codes, atomically. DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error // Backup Codes From 48d0b5173a91fa93ee425748374abfdd9dd70ef9 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 08:31:38 +0100 Subject: [PATCH 2/9] fix(auth): require the password to turn MFA off (#754) An open session was enough to remove the second factor. The disable endpoint now re-verifies the current password (wrong or missing: 401, generic body), counts every attempt against a per-account budget of 5 per 15 minutes on top of the per-IP limiter, refuses roles that login requires MFA for (403), writes an mfa_disable audit entry with a reason code on failure, and notifies the owner by email and in-app. Signed-off-by: alex-dembele --- backend/cmd/server/main.go | 20 +- .../application/auth/mfa_disable_test.go | 202 ++++++++++++++++++ .../internal/application/auth/mfa_usecase.go | 132 +++++++++++- backend/internal/auth/audit.go | 6 + backend/internal/domain/notification.go | 4 + .../handler/auth/mfa_deferred_e2e_test.go | 11 + .../handler/auth/mfa_disable_e2e_test.go | 180 ++++++++++++++++ backend/internal/handler/auth/mfa_handler.go | 101 ++++++++- .../internal/infrastructure/authmail/async.go | 7 + .../infrastructure/authmail/reset_mailer.go | 36 ++++ 10 files changed, 681 insertions(+), 18 deletions(-) create mode 100644 backend/internal/application/auth/mfa_disable_test.go create mode 100644 backend/internal/handler/auth/mfa_disable_e2e_test.go diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fd47317e..600489c5 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -692,7 +692,11 @@ func main() { // MFA use cases + handler. setupMFAUseCase := auth.NewSetupMFAUseCase(mfaRepo, mfaKey[:]) verifyMFAUseCase := auth.NewVerifyMFAUseCase(mfaRepo, *userRepo, mfaKey[:]) - disableMFAUseCase := auth.NewDisableMFAUseCase(mfaRepo, passwordHasher) + // #754 — removing the factor re-proves the password, is refused for the + // roles login requires MFA for, and mails the owner. + disableMFAUseCase := auth.NewDisableMFAUseCase(mfaRepo, userRepo, passwordHasher). + RequireMFAForRoles(mfaRequiredRoles, mfaRequiredBusinessRoles). + WithMailer(securityMailer) challengeMFAUseCase := auth.NewChallengeMFAUseCase(mfaRepo, mfaKey[:]) // OR26-03 — one resolver answers "must this member enrol now?" for /auth/me // and for the request-time guard, so the banner and the enforcement can never @@ -701,7 +705,9 @@ func main() { mfaStatusResolver := auth.NewMFAStatusResolver(mfaRepo, userRepo, mfaRequiredRoles, mfaRequiredBusinessRoles). WithPolicies(mfaPolicyRepo) mfaHandler := authhandler.NewMFAHandler(setupMFAUseCase, verifyMFAUseCase, disableMFAUseCase, challengeMFAUseCase, tokenManager, userRepo, authAudit). - WithMFAStatus(mfaStatusResolver) + WithMFAStatus(mfaStatusResolver). + // #754 — per-account budget on disabling MFA, shared across instances. + WithDisableAttemptLimit(middleware.NewRedisRateLimitStore(redisClientInstance)) mfaPolicyHandler := authhandler.NewMFAPolicyHandler( auth.NewGetMFAPolicyUseCase(mfaPolicyRepo, mfaRequiredRoles, mfaRequiredBusinessRoles), auth.NewUpdateMFAPolicyUseCase(mfaPolicyRepo, mfaRequiredRoles, mfaRequiredBusinessRoles). @@ -1113,7 +1119,8 @@ func main() { mfaEnrollmentGuard := middleware.MFAEnrollmentMiddleware(rsaKeys, jtiBlacklistChecker) api.Post("/auth/mfa/setup", mfaEnrollmentGuard, mfaHandler.Setup) api.Post("/auth/mfa/verify", mfaEnrollmentGuard, mfaHandler.Verify) - protected.Post("/auth/mfa/disable", mfaHandler.Disable) + // Throttled like /auth/password/change: the body carries a password guess. + protected.Post("/auth/mfa/disable", authRateLimit, mfaHandler.Disable) // --- MFA policy (OR26-03) — "force MFA after N days" ----------------------- // Reading is open to any authenticated member: everyone subject to a deadline @@ -2245,6 +2252,13 @@ func main() { // --- Notifications (Protected routes) --- notificationRepo := repository.NewNotificationRepository(database.DB) notificationUseCase := notificationapp.NewUseCase(notificationRepo) + // #754 — the MFA deactivation notice lands in the bell as well as the inbox. + disableMFAUseCase.WithInAppNotifier(func(ctx context.Context, tenantID, userID uuid.UUID, subject, message string) { + if err := notificationUseCase.NotifyInApp(userID, tenantID, + domain.NotificationTypeMFADisabled, subject, message, nil, ""); err != nil && !errors.Is(err, notificationapp.ErrSuppressed) { + zeroLogger.Warn().Err(err).Msg("mfa disable: in-app notification failed") + } + }) notificationHandler := handlers.NewNotificationHandler(notificationUseCase) // Attack Surface §4: tell the tenant's admins when the vuln→risk rule diff --git a/backend/internal/application/auth/mfa_disable_test.go b/backend/internal/application/auth/mfa_disable_test.go new file mode 100644 index 00000000..8ab65a69 --- /dev/null +++ b/backend/internal/application/auth/mfa_disable_test.go @@ -0,0 +1,202 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opendefender/openrisk/internal/domain" +) + +// #754 — disabling MFA re-proves the password, is refused for roles that +// require MFA, and removes the secret and the backup codes together. + +type disableHasher struct{} + +func (disableHasher) Hash(p string) (string, error) { return "hashed:" + p, nil } +func (disableHasher) Verify(hash, plain string) bool { return hash == "hashed:"+plain } + +type disableUsers struct { + users map[uuid.UUID]*domain.User + members map[uuid.UUID]*domain.OrganizationMember + err error +} + +func (u *disableUsers) GetByID(_ context.Context, id uuid.UUID) (*domain.User, error) { + return u.users[id], nil +} + +func (u *disableUsers) GetOrganizationMember(_ context.Context, userID, _ uuid.UUID) (*domain.OrganizationMember, error) { + if u.err != nil { + return nil, u.err + } + return u.members[userID], nil +} + +type disableMailer struct { + to, locale string + calls int +} + +func (m *disableMailer) SendMFADisabled(_ context.Context, to, _, locale string) error { + m.to, m.locale = to, locale + m.calls++ + return nil +} + +const disablePassword = "Ancre-Vitrail7-Cobalt" + +type disableFixture struct { + uc *DisableMFAUseCase + repo *MockMFARepository + users *disableUsers + mailer *disableMailer + inApp []string + user *domain.User + tenant uuid.UUID + codeKey string +} + +func newDisableFixture(t *testing.T, role domain.MemberRole, business domain.BusinessRoleKey) *disableFixture { + t.Helper() + tenant := uuid.New() + user := &domain.User{ID: uuid.New(), Email: "rssi@banque.cm", FullName: "R. Ssi", IsActive: true, Password: "hashed:" + disablePassword} + users := &disableUsers{ + users: map[uuid.UUID]*domain.User{user.ID: user}, + members: map[uuid.UUID]*domain.OrganizationMember{user.ID: { + UserID: user.ID, OrganizationID: tenant, Role: role, BusinessRole: business, + }}, + } + repo := NewMockMFARepository() + require.NoError(t, repo.CreateMFASecret(context.Background(), &domain.MFASecret{UserID: user.ID, TenantID: tenant, IsVerified: true})) + require.NoError(t, repo.SaveBackupCodes(context.Background(), []*domain.MFABackupCode{{UserID: user.ID, TenantID: tenant, CodeHash: "h"}})) + + mailer := &disableMailer{} + orgRoles, businessRoles := domain.DefaultMFAPrivilegeRoles() + uc := NewDisableMFAUseCase(repo, users, disableHasher{}). + RequireMFAForRoles(orgRoles, businessRoles). + WithMailer(mailer) + f := &disableFixture{uc: uc, repo: repo, users: users, mailer: mailer, user: user, tenant: tenant, + codeKey: user.ID.String() + ":" + tenant.String()} + uc.WithInAppNotifier(func(_ context.Context, tenantID, userID uuid.UUID, subject, _ string) { + if tenantID == f.tenant && userID == f.user.ID { + f.inApp = append(f.inApp, subject) + } + }) + return f +} + +func (f *disableFixture) run(password, roleHint string) error { + _, err := f.uc.Execute(context.Background(), DisableMFAInput{ + UserID: f.user.ID, TenantID: f.tenant, Password: password, OrgRoleHint: roleHint, Locale: "en", + }) + return err +} + +func (f *disableFixture) assertStillEnrolled(t *testing.T) { + t.Helper() + secret, _ := f.repo.GetMFASecret(context.Background(), f.user.ID, f.tenant) + assert.NotNil(t, secret, "the secret must survive a refused disable") + assert.Len(t, f.repo.codes[f.codeKey], 1, "the backup codes must survive a refused disable") + assert.Zero(t, f.mailer.calls, "no notice for a disable that did not happen") + assert.Empty(t, f.inApp, "no in-app notice for a disable that did not happen") +} + +func TestDisableMFA_Success(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + + require.NoError(t, f.run(disablePassword, "user")) + + secret, _ := f.repo.GetMFASecret(context.Background(), f.user.ID, f.tenant) + assert.Nil(t, secret) + assert.Empty(t, f.repo.codes[f.codeKey]) + assert.Equal(t, 1, f.mailer.calls) + assert.Equal(t, "rssi@banque.cm", f.mailer.to) + assert.Equal(t, "en", f.mailer.locale) + assert.Equal(t, []string{"Two-factor authentication turned off"}, f.inApp) +} + +func TestDisableMFA_NotFound(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + require.NoError(t, f.repo.DisableMFA(context.Background(), f.user.ID, f.tenant)) + + err := f.run(disablePassword, "") + + var appErr *domain.AppError + require.True(t, errors.As(err, &appErr), "got %v", err) + assert.ErrorIs(t, appErr.Err, domain.ErrNotFound) + assert.Zero(t, f.mailer.calls) +} + +func TestDisableMFA_Unauthorized(t *testing.T) { + for name, password := range map[string]string{"wrong password": "guess-1234", "missing password": ""} { + t.Run(name, func(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + assert.ErrorIs(t, f.run(password, ""), ErrMFADisablePasswordIncorrect) + f.assertStillEnrolled(t) + }) + } +} + +func TestDisableMFA_UnauthorizedWithoutSession(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + _, err := f.uc.Execute(context.Background(), DisableMFAInput{TenantID: f.tenant, Password: disablePassword}) + + var appErr *domain.AppError + require.True(t, errors.As(err, &appErr), "got %v", err) + assert.ErrorIs(t, appErr.Err, domain.ErrUnauthorized) + f.assertStillEnrolled(t) +} + +func TestDisableMFA_RoleRequiringMFAIsRefused(t *testing.T) { + cases := map[string]struct { + role domain.MemberRole + business domain.BusinessRoleKey + hint string + }{ + "admin": {domain.RoleAdmin, "", ""}, + "root": {domain.RoleRoot, "", ""}, + "security officer as org user": {domain.RoleUser, domain.BusinessRoleRSSI, ""}, + "token says admin, row says user": {domain.RoleUser, "", "admin"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + f := newDisableFixture(t, tc.role, tc.business) + assert.ErrorIs(t, f.run(disablePassword, tc.hint), ErrMFARequiredByRole) + f.assertStillEnrolled(t) + }) + } +} + +func TestDisableMFA_MembershipLookupFailureFailsClosed(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + f.users.err = errors.New("db down") + + require.Error(t, f.run(disablePassword, "")) + f.assertStillEnrolled(t) +} + +func TestDisableMFA_NoRolePolicyAllowsAnAdmin(t *testing.T) { + f := newDisableFixture(t, domain.RoleAdmin, "") + // MFA_REQUIRED_ROLES="" — the deployment made MFA optional for everyone. + f.uc.RequireMFAForRoles(nil, nil) + + require.NoError(t, f.run(disablePassword, "admin")) +} + +func TestDisableMFA_AccountWithoutLocalPasswordIsRefused(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + f.user.Password = "" + + assert.ErrorIs(t, f.run("", ""), ErrNoLocalPassword) + f.assertStillEnrolled(t) +} diff --git a/backend/internal/application/auth/mfa_usecase.go b/backend/internal/application/auth/mfa_usecase.go index 468d93dc..a78e60ec 100644 --- a/backend/internal/application/auth/mfa_usecase.go +++ b/backend/internal/application/auth/mfa_usecase.go @@ -7,6 +7,7 @@ package auth import ( "context" + "errors" "fmt" "time" @@ -205,11 +206,50 @@ func (uc *VerifyMFAUseCase) Execute(ctx context.Context, input VerifyMFAInput) ( }, nil } -// DisableMFAInput represents MFA disable request +// --------------------------------------------------------------------------- +// Disabling MFA (#754). +// +// Removing a factor is the one MFA operation an attacker at an unlocked +// workstation wants most, so the session alone is not enough: the caller must +// prove the password again. Privileged roles cannot remove it at all — the +// login policy would demand it back on the next sign-in, and in between the +// account would sit on a password alone. +// --------------------------------------------------------------------------- + +var ( + // ErrMFADisablePasswordIncorrect covers a wrong and a missing password alike. + // Deliberately unspecific. + ErrMFADisablePasswordIncorrect = errors.New("password is incorrect") + // ErrMFARequiredByRole refuses the removal for a role the deployment requires + // MFA for. + ErrMFARequiredByRole = errors.New("two-factor authentication is required for this role") +) + +// DisableMFAUserLookup reads the account and its membership. Satisfied by +// *repository.GormUserRepository. +type DisableMFAUserLookup interface { + GetByID(ctx context.Context, id uuid.UUID) (*domain.User, error) + GetOrganizationMember(ctx context.Context, userID, orgID uuid.UUID) (*domain.OrganizationMember, error) +} + +// MFADisabledMailer sends the deactivation notice. +type MFADisabledMailer interface { + SendMFADisabled(ctx context.Context, to, fullName, locale string) error +} + +// MFADisabledInAppNotifier records the in-app deactivation notice. +type MFADisabledInAppNotifier func(ctx context.Context, tenantID, userID uuid.UUID, subject, message string) + +// DisableMFAInput represents MFA disable request. UserID, TenantID and +// OrgRoleHint come from the session, never from the body. type DisableMFAInput struct { UserID uuid.UUID TenantID uuid.UUID Password string // Current password (required for security) + // OrgRoleHint is the org role in the caller's signed token. It can only + // widen the privileged check, never narrow it (same rule as MFAStatusResolver). + OrgRoleHint string + Locale string } // DisableMFAOutput represents MFA disable response @@ -220,32 +260,97 @@ type DisableMFAOutput struct { // DisableMFAUseCase handles MFA disable type DisableMFAUseCase struct { mfaRepo repository.MFARepository + users DisableMFAUserLookup passwordHasher PasswordHasher + privileged domain.MFAPrivilegeSet + mailer MFADisabledMailer + inApp MFADisabledInAppNotifier } // NewDisableMFAUseCase creates a new disable MFA use case -func NewDisableMFAUseCase(mfaRepo repository.MFARepository, passwordHasher PasswordHasher) *DisableMFAUseCase { +func NewDisableMFAUseCase(mfaRepo repository.MFARepository, users DisableMFAUserLookup, passwordHasher PasswordHasher) *DisableMFAUseCase { return &DisableMFAUseCase{ mfaRepo: mfaRepo, + users: users, passwordHasher: passwordHasher, } } -// Execute disables MFA for user +// RequireMFAForRoles names the roles that may not remove their factor. Pass the +// same lists login enforces, so the two can never disagree. +func (uc *DisableMFAUseCase) RequireMFAForRoles(orgRoles, businessRoles []string) *DisableMFAUseCase { + uc.privileged = domain.NewMFAPrivilegeSet(orgRoles, businessRoles) + return uc +} + +// WithMailer wires the deactivation notice. Optional. +func (uc *DisableMFAUseCase) WithMailer(m MFADisabledMailer) *DisableMFAUseCase { + uc.mailer = m + return uc +} + +// WithInAppNotifier wires the in-app deactivation notice. Optional. +func (uc *DisableMFAUseCase) WithInAppNotifier(n MFADisabledInAppNotifier) *DisableMFAUseCase { + uc.inApp = n + return uc +} + +// Execute verifies the password, refuses privileged roles, then deletes the +// secret and the backup codes in one transaction and notifies the owner. func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) (*DisableMFAOutput, error) { if input.UserID == uuid.Nil || input.TenantID == uuid.Nil { - return nil, domain.NewValidationError("user_id and tenant_id required") + return nil, domain.NewUnauthorizedError("authentication required") } - // TODO: Verify password before disabling (requires user repo + password verification) + user, err := uc.users.GetByID(ctx, input.UserID) + if err != nil { + return nil, fmt.Errorf("auth.DisableMFA: load user: %w", err) + } + if user == nil || !user.IsActive { + return nil, domain.NewNotFoundError("user", input.UserID) + } + if user.Password == "" { + // An identity-provider account has no password to prove. Refusing is the + // safe answer until it can prove a factor another way. + return nil, ErrNoLocalPassword + } + if input.Password == "" || !uc.passwordHasher.Verify(user.Password, input.Password) { + return nil, ErrMFADisablePasswordIncorrect + } + + secret, err := uc.mfaRepo.GetMFASecret(ctx, input.UserID, input.TenantID) + if err != nil { + return nil, fmt.Errorf("auth.DisableMFA: load secret: %w", err) + } + if secret == nil { + return nil, domain.NewNotFoundError("MFA secret", input.UserID) + } + + if !uc.privileged.Empty() { + member, err := uc.users.GetOrganizationMember(ctx, input.UserID, input.TenantID) + if err != nil { + // Cannot tell whether the role requires MFA: refuse rather than guess. + return nil, fmt.Errorf("auth.DisableMFA: load membership: %w", err) + } + privileged := uc.privileged.Includes(domain.MemberRole(input.OrgRoleHint), "") + if member != nil && uc.privileged.Includes(member.Role, member.BusinessRole) { + privileged = true + } + if privileged { + return nil, ErrMFARequiredByRole + } + } - // Delete MFA secret and backup codes if err := uc.mfaRepo.DisableMFA(ctx, input.UserID, input.TenantID); err != nil { - return nil, fmt.Errorf("failed to disable MFA: %w", err) + return nil, fmt.Errorf("auth.DisableMFA: %w", err) } - if err := uc.mfaRepo.DeleteBackupCodes(ctx, input.UserID, input.TenantID); err != nil { - return nil, fmt.Errorf("failed to delete backup codes: %w", err) + if uc.mailer != nil { + _ = uc.mailer.SendMFADisabled(ctx, user.Email, user.FullName, normaliseLocale(input.Locale)) + } + if uc.inApp != nil { + subject, message := mfaDisabledInAppCopy(normaliseLocale(input.Locale)) + uc.inApp(ctx, input.TenantID, input.UserID, subject, message) } return &DisableMFAOutput{ @@ -253,6 +358,15 @@ func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) }, nil } +func mfaDisabledInAppCopy(locale string) (string, string) { + if locale == "en" { + return "Two-factor authentication turned off", + "Two-factor authentication was turned off on your account after your password was confirmed. If this wasn't you, change your password and turn it back on from Settings → Security." + } + return "Double authentification désactivée", + "La double authentification a été désactivée sur votre compte après confirmation de votre mot de passe. Si ce n'est pas vous, changez votre mot de passe et réactivez-la depuis Paramètres → Sécurité." +} + // ChallengeMFAInput represents MFA challenge request (after login) type ChallengeMFAInput struct { UserID uuid.UUID diff --git a/backend/internal/auth/audit.go b/backend/internal/auth/audit.go index d10b333f..fc5b1f2b 100644 --- a/backend/internal/auth/audit.go +++ b/backend/internal/auth/audit.go @@ -34,6 +34,12 @@ const ( AuditActionPatRevoke AuditAction = "pat_revoke" AuditActionPatUse AuditAction = "pat_use" + // AuditActionMfaDisable records a request to remove the second factor + // (#754). Failures carry a reason ("wrong_password", "mfa_required_by_role", + // "no_local_password", "not_enrolled"): repeated wrong passwords here are a + // session thief guessing. + AuditActionMfaDisable AuditAction = "mfa_disable" + // Password reset. Both halves are recorded, and failures carry a reason // ("rate_limited", "invalid_token", "weak_password"), because a burst of // invalid-token attempts is what a reset-link brute force looks like. diff --git a/backend/internal/domain/notification.go b/backend/internal/domain/notification.go index 966d82a9..f2549121 100644 --- a/backend/internal/domain/notification.go +++ b/backend/internal/domain/notification.go @@ -29,6 +29,10 @@ const ( // only, with no per-event preference column: Allows lets it through unless // the user silenced everything, as it does risk_review and automation. NotificationTypeVendorAssessmentReminder NotificationType = "vendor_assessment_reminder" + // NotificationTypeMFADisabled tells the owner their second factor was + // removed (#754). A security notice: someone at an unlocked workstation who + // knew the password is exactly who would do this. + NotificationTypeMFADisabled NotificationType = "mfa_disabled" ) const ( diff --git a/backend/internal/handler/auth/mfa_deferred_e2e_test.go b/backend/internal/handler/auth/mfa_deferred_e2e_test.go index 176b4686..4849729a 100644 --- a/backend/internal/handler/auth/mfa_deferred_e2e_test.go +++ b/backend/internal/handler/auth/mfa_deferred_e2e_test.go @@ -54,6 +54,8 @@ type deferredFixture struct { keys *authpkg.RSAKeys policyRepo *repository.GormMFAPolicyRepository resolver *appauth.MFAStatusResolver + // trail captures what the tamper-evident audit middleware would chain. + trail *capturedTrail now time.Time @@ -173,6 +175,8 @@ func newDeferredFixture(t *testing.T) *deferredFixture { // then the MFA guard reads it, then everything else. protected := api.Use(middleware.Protected(keys, nil)) protected.Use(middleware.MFAPolicyGuard(f.resolver)) + f.trail = &capturedTrail{} + protected.Use(middleware.AuditMutations(f.trail)) protected.Get("/auth/me", h.Me) protected.Get("/security/mfa-policy", policyHandler.Get) @@ -183,6 +187,13 @@ func newDeferredFixture(t *testing.T) *deferredFixture { protected.Post("/risks", ok) protected.Post("/auth/pat", ok) protected.Post("/auth/mfa/setup", ok) + // #754 — the real disable path: use case, handler, repository. + disableUC := appauth.NewDisableMFAUseCase(mfaRepo, userRepo, deferredHasher{}). + RequireMFAForRoles(orgRoles, businessRoles) + mfaHandler := authhandler.NewMFAHandler(nil, nil, disableUC, nil, tokens, userRepo, nil). + WithMFAStatus(f.resolver). + WithDisableAttemptLimit(middleware.NewRateLimitStore()) + protected.Post("/auth/mfa/disable", mfaHandler.Disable) f.app = app return f diff --git a/backend/internal/handler/auth/mfa_disable_e2e_test.go b/backend/internal/handler/auth/mfa_disable_e2e_test.go new file mode 100644 index 00000000..5a5c0fd0 --- /dev/null +++ b/backend/internal/handler/auth/mfa_disable_e2e_test.go @@ -0,0 +1,180 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth_test + +import ( + "context" + "net/http" + "sync" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opendefender/openrisk/internal/domain" +) + +// --------------------------------------------------------------------------- +// #754 — disabling MFA through the real HTTP stack: RS256 auth middleware, MFA +// guard, audit middleware, handler, use case and GORM repository. A live +// session is not enough to remove the second factor. +// --------------------------------------------------------------------------- + +type capturedTrail struct { + mu sync.Mutex + events []*domain.AuditEvent +} + +func (c *capturedTrail) Append(_ context.Context, e *domain.AuditEvent) error { + c.mu.Lock() + defer c.mu.Unlock() + c.events = append(c.events, e) + return nil +} + +func (c *capturedTrail) pathCount(path string) int { + c.mu.Lock() + defer c.mu.Unlock() + n := 0 + for _, e := range c.events { + if e.Path == path { + n++ + } + } + return n +} + +const disablePath = "/api/v1/auth/mfa/disable" + +// enrol gives a member a verified secret and backup codes, as a completed +// enrolment would. +func (f *deferredFixture) enrol(t *testing.T, m *domain.OrganizationMember) { + t.Helper() + require.NoError(t, f.db.Create(&domain.MFASecret{ + ID: uuid.New(), UserID: m.UserID, TenantID: m.OrganizationID, SecretEncrypted: "x", IsVerified: true, + }).Error) + for i := 0; i < 8; i++ { + require.NoError(t, f.db.Create(&domain.MFABackupCode{ + ID: uuid.New(), UserID: m.UserID, TenantID: m.OrganizationID, CodeHash: uuid.NewString(), + }).Error) + } +} + +func (f *deferredFixture) factorRows(t *testing.T, m *domain.OrganizationMember) (secrets, codes int64) { + t.Helper() + require.NoError(t, f.db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", m.UserID).Count(&secrets).Error) + require.NoError(t, f.db.Model(&domain.MFABackupCode{}).Where("user_id = ?", m.UserID).Count(&codes).Error) + return secrets, codes +} + +// A session opened before enrolment, so the login itself asks for no code. +func (f *deferredFixture) sessionThenEnrol(t *testing.T, m *domain.OrganizationMember) string { + t.Helper() + _, token := f.login(t, m.User.Email) + require.NotEmpty(t, token) + f.enrol(t, m) + return token +} + +func TestMFADisableE2E_LiveSessionWithoutPasswordCannotDisable(t *testing.T) { + f := newDeferredFixture(t) + token := f.sessionThenEnrol(t, f.memberA) + + for name, body := range map[string]any{ + "no body": nil, + "empty password": jsonBody{"password": ""}, + "wrong password": jsonBody{"password": "guess-1234"}, + } { + t.Run(name, func(t *testing.T) { + status, resp := f.do(t, http.MethodPost, disablePath, token, body) + if body == nil { + // No JSON at all is refused before the password is even read. + assert.Equal(t, http.StatusBadRequest, status) + } else { + assert.Equal(t, http.StatusUnauthorized, status) + assert.Equal(t, "wrong_password", resp["code"]) + // Must not read as a dead session to the SPA. + assert.NotContains(t, []any{"TOKEN_EXPIRED", "TOKEN_REVOKED", "TOKEN_INVALID", "UNAUTHORIZED"}, resp["code"]) + } + secrets, codes := f.factorRows(t, f.memberA) + assert.EqualValues(t, 1, secrets, "the secret must survive") + assert.EqualValues(t, 8, codes, "the backup codes must survive") + }) + } + assert.Zero(t, f.trail.pathCount(disablePath), "a refused disable changes nothing and is not chained") +} + +func TestMFADisableE2E_CorrectPasswordDisablesAndIsChained(t *testing.T) { + f := newDeferredFixture(t) + token := f.sessionThenEnrol(t, f.memberA) + + status, resp := f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": deferredPassword}) + require.Equal(t, http.StatusOK, status, "%v", resp) + + secrets, codes := f.factorRows(t, f.memberA) + assert.Zero(t, secrets) + assert.Zero(t, codes) + assert.Equal(t, 1, f.trail.pathCount(disablePath), "the deactivation is on the chained trail") + + // Disabling twice finds nothing to disable. + status, resp = f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": deferredPassword}) + assert.Equal(t, http.StatusNotFound, status) + assert.Equal(t, "not_enrolled", resp["code"]) +} + +func TestMFADisableE2E_PrivilegedRoleIsRefused(t *testing.T) { + f := newDeferredFixture(t) + // Inside the grace window the admin holds a session without a code. + token := f.sessionThenEnrol(t, f.adminA) + f.resolver.Invalidate(f.adminA.UserID, f.tenantA) + + status, resp := f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": deferredPassword}) + assert.Equal(t, http.StatusForbidden, status, "%v", resp) + assert.Equal(t, "mfa_required_by_role", resp["code"]) + + secrets, codes := f.factorRows(t, f.adminA) + assert.EqualValues(t, 1, secrets) + assert.EqualValues(t, 8, codes) +} + +func TestMFADisableE2E_AnotherTenantsFactorIsOutOfReach(t *testing.T) { + f := newDeferredFixture(t) + token := f.sessionThenEnrol(t, f.memberA) + f.enrol(t, f.adminB) + + status, _ := f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": deferredPassword}) + require.Equal(t, http.StatusOK, status) + + secrets, codes := f.factorRows(t, f.adminB) + assert.EqualValues(t, 1, secrets) + assert.EqualValues(t, 8, codes) +} + +func TestMFADisableE2E_AttemptsAreLimitedPerAccount(t *testing.T) { + f := newDeferredFixture(t) + token := f.sessionThenEnrol(t, f.memberA) + + for i := 0; i < 5; i++ { + status, _ := f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": "guess-" + uuid.NewString()}) + require.Equal(t, http.StatusUnauthorized, status, "attempt %d", i+1) + } + // The sixth try is refused even with the right password: the budget is the + // account's, and a guesser who finally hits it must still wait. + status, resp := f.do(t, http.MethodPost, disablePath, token, jsonBody{"password": deferredPassword}) + assert.Equal(t, http.StatusTooManyRequests, status) + assert.Equal(t, "too_many_attempts", resp["code"]) + + secrets, codes := f.factorRows(t, f.memberA) + assert.EqualValues(t, 1, secrets) + assert.EqualValues(t, 8, codes) + + // Another account's budget is untouched. + other := f.seedMember(t, f.tenantA, "other@a.io", domain.RoleUser, "", f.now) + otherToken := f.sessionThenEnrol(t, other) + status, _ = f.do(t, http.MethodPost, disablePath, otherToken, jsonBody{"password": deferredPassword}) + assert.Equal(t, http.StatusOK, status) +} diff --git a/backend/internal/handler/auth/mfa_handler.go b/backend/internal/handler/auth/mfa_handler.go index 06fbe685..094fc039 100644 --- a/backend/internal/handler/auth/mfa_handler.go +++ b/backend/internal/handler/auth/mfa_handler.go @@ -6,6 +6,9 @@ package auth import ( + "errors" + "time" + "github.com/gofiber/fiber/v2" "github.com/google/uuid" @@ -29,6 +32,25 @@ type MFAHandler struct { // mfaStatus is the cache the request-time guard reads. Optional; when set, // a completed enrolment drops the caller's entry immediately (OR26-03). mfaStatus *appauth.MFAStatusResolver + // disableAttempts counts disable attempts per account (#754). Optional. + disableAttempts middleware.RateLimitBackend +} + +// Per-account budget for POST /auth/mfa/disable. The per-IP limiter on the +// route does not stop a session thief who rotates addresses; this does, because +// the key is the account the stolen session belongs to. Five tries in fifteen +// minutes is ample for someone who knows their password. +const ( + mfaDisableMaxAttempts = 5 + mfaDisableWindow = 15 * time.Minute +) + +// WithDisableAttemptLimit counts every disable attempt against the caller's +// account. Pass the shared (Redis-backed) store so the budget holds across +// instances. +func (h *MFAHandler) WithDisableAttemptLimit(store middleware.RateLimitBackend) *MFAHandler { + h.disableAttempts = store + return h } // WithMFAStatus lets a completed enrolment take effect on the very next request @@ -158,7 +180,11 @@ func (h *MFAHandler) issueSessionResponse(c *fiber.Ctx, userID uuid.UUID, device return c.JSON(LoginResponse{TokenPair: pair, CSRFToken: csrfToken}) } -// Disable turns MFA off for the current user. +// Disable turns MFA off for the current user (#754). +// +// The session is not proof enough: the body must carry the current password. +// Neither the password nor anything derived from it is logged or echoed; audit +// failures carry a reason code only. func (h *MFAHandler) Disable(c *fiber.Ctx) error { userID := ctxUUID(c, "user_id") tenantID := ctxUUID(c, "tenant_id") @@ -168,18 +194,81 @@ func (h *MFAHandler) Disable(c *fiber.Ctx) error { var req struct { Password string `json:"password"` + Locale string `json:"locale,omitempty"` } - _ = c.BodyParser(&req) + if err := c.BodyParser(&req); err != nil { + return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid request body"}) + } + locale := resolveLocale(c, req.Locale) - out, err := h.disable.Execute(c.UserContext(), appauth.DisableMFAInput{UserID: userID, TenantID: tenantID, Password: req.Password}) - if err != nil { - return mapAuthError(c, err) + orgRole := "" + if claims := middleware.GetUserClaims(c); claims != nil { + orgRole = claims.OrgRoles[tenantID] + } + + fail := func(status int, reason, message string) error { + h.logDisable(c, userID, tenantID, false, &reason) + return c.Status(status).JSON(fiber.Map{"error": message, "code": reason}) + } + + // Checked before the password is: a refused attempt must not reach the hasher. + if h.disableAttempts != nil && + !h.disableAttempts.IsAllowed("mfa-disable:"+userID.String(), mfaDisableMaxAttempts, mfaDisableWindow) { + return fail(fiber.StatusTooManyRequests, "too_many_attempts", + pick(locale, + "Trop de tentatives. Réessayez dans quelques minutes.", + "Too many attempts. Try again in a few minutes.")) + } + + _, err := h.disable.Execute(c.UserContext(), appauth.DisableMFAInput{ + UserID: userID, + TenantID: tenantID, + Password: req.Password, + OrgRoleHint: orgRole, + Locale: locale, + }) + + var appErr *domain.AppError + switch { + case errors.Is(err, appauth.ErrMFADisablePasswordIncorrect): + // 401 without a token error code: the SPA reads that as "this request + // was refused", not "your session is over". + return fail(fiber.StatusUnauthorized, "wrong_password", + pick(locale, "Mot de passe incorrect.", "Incorrect password.")) + case errors.Is(err, appauth.ErrMFARequiredByRole): + return fail(fiber.StatusForbidden, "mfa_required_by_role", + pick(locale, + "Votre rôle impose la double authentification : elle ne peut pas être désactivée.", + "Your role requires two-factor authentication: it cannot be turned off.")) + case errors.Is(err, appauth.ErrNoLocalPassword): + return fail(fiber.StatusConflict, "no_local_password", + pick(locale, + "Ce compte se connecte via votre fournisseur d'identité et n'a pas de mot de passe à confirmer.", + "This account signs in through your identity provider and has no password to confirm.")) + case errors.As(err, &appErr) && errors.Is(appErr.Err, domain.ErrNotFound): + return fail(fiber.StatusNotFound, "not_enrolled", + pick(locale, "La double authentification n'est pas activée.", "Two-factor authentication is not enabled.")) + case errors.As(err, &appErr): + return fail(appErr.Code, "rejected", domain.MessageFromError(err)) + case err != nil: + return fail(fiber.StatusInternalServerError, "internal", genericFailure(locale)) } + + h.logDisable(c, userID, tenantID, true, nil) // Turning MFA off must re-arm the requirement immediately: a privileged // account that disables its authenticator past the deadline has to be // stopped on its next request, not after the cache expires. h.mfaStatus.Invalidate(userID, tenantID) - return c.JSON(out) + return c.JSON(fiber.Map{ + "message": pick(locale, "Double authentification désactivée.", "Two-factor authentication turned off."), + }) +} + +func (h *MFAHandler) logDisable(c *fiber.Ctx, userID, tenantID uuid.UUID, success bool, reason *string) { + if h.audit == nil { + return + } + _ = h.audit.LogFiber(c, &userID, &tenantID, coreauth.AuditActionMfaDisable, success, reason) } // Challenge is the second leg of an MFA login. It is reached with an diff --git a/backend/internal/infrastructure/authmail/async.go b/backend/internal/infrastructure/authmail/async.go index 55362274..97a3a926 100644 --- a/backend/internal/infrastructure/authmail/async.go +++ b/backend/internal/infrastructure/authmail/async.go @@ -33,6 +33,7 @@ type ResetMailerLike interface { SendResetConfirmation(ctx context.Context, to, fullName, locale string) error SendNewSignInAlert(ctx context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error SendPasswordChanged(ctx context.Context, to, fullName, locale string) error + SendMFADisabled(ctx context.Context, to, fullName, locale string) error } // NewAsync wraps a mailer so every send returns immediately. @@ -58,6 +59,12 @@ func (a *Async) SendPasswordChanged(_ context.Context, to, fullName, locale stri return nil } +// SendMFADisabled queues the two-factor deactivation notice. +func (a *Async) SendMFADisabled(_ context.Context, to, fullName, locale string) error { + a.dispatch(func(ctx context.Context) { _ = a.inner.SendMFADisabled(ctx, to, fullName, locale) }) + return nil +} + // SendNewSignInAlert queues the new-device notice. func (a *Async) SendNewSignInAlert(_ context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error { a.dispatch(func(ctx context.Context) { diff --git a/backend/internal/infrastructure/authmail/reset_mailer.go b/backend/internal/infrastructure/authmail/reset_mailer.go index d7aa6e76..3533f8f4 100644 --- a/backend/internal/infrastructure/authmail/reset_mailer.go +++ b/backend/internal/infrastructure/authmail/reset_mailer.go @@ -70,6 +70,19 @@ func (m *Mailer) SendPasswordChanged(ctx context.Context, to, fullName, locale s return m.sender.SendEmail(ctx, to, c.subject, m.render(c)) } +// SendMFADisabled tells the owner that two-factor authentication was turned off +// (#754). +// +// Losing a factor is exactly what someone at an unlocked workstation would do +// first, so the notice goes to the mailbox, which that person does not hold. +func (m *Mailer) SendMFADisabled(ctx context.Context, to, fullName, locale string) error { + if m == nil || m.sender == nil { + return nil + } + c := mfaDisabledCopy(locale, displayName(fullName, locale)) + return m.sender.SendEmail(ctx, to, c.subject, m.render(c)) +} + // SendNewSignInAlert warns about a sign-in from an unrecognised device. func (m *Mailer) SendNewSignInAlert(ctx context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error { if m == nil || m.sender == nil { @@ -170,6 +183,29 @@ func passwordChangedCopy(locale, name string) copyBlock { } } +func mfaDisabledCopy(locale, name string) copyBlock { + if locale == "en" { + return copyBlock{ + subject: "Two-factor authentication was turned off on your OpenRisk account", + heading: "Two-factor authentication is off", + paragraphs: []string{ + fmt.Sprintf("Hello %s,", name), + "Two-factor authentication was just turned off on this OpenRisk account, after the password was confirmed. Your authenticator app and your backup codes no longer work. A password alone now opens the account.", + }, + footnote: "If this wasn't you, your password is known to someone else: reset it immediately from the sign-in page, turn two-factor authentication back on, and contact your OpenRisk administrator.", + } + } + return copyBlock{ + subject: "La double authentification a été désactivée sur votre compte OpenRisk", + heading: "La double authentification est désactivée", + paragraphs: []string{ + fmt.Sprintf("Bonjour %s,", name), + "La double authentification vient d'être désactivée sur ce compte OpenRisk, après confirmation du mot de passe. Votre application d'authentification et vos codes de secours ne fonctionnent plus. Le mot de passe seul suffit désormais pour ouvrir le compte.", + }, + footnote: "Si vous n'êtes pas à l'origine de ce changement, quelqu'un d'autre connaît votre mot de passe : réinitialisez-le immédiatement depuis la page de connexion, réactivez la double authentification et contactez votre administrateur OpenRisk.", + } +} + func newSignInCopy(locale, name, ip, userAgent string, when time.Time) copyBlock { stamp := when.UTC().Format("2006-01-02 15:04 UTC") if locale == "en" { From b3d9016f147d29c1801da1a6af0fae42fb5c394b Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 08:31:38 +0100 Subject: [PATCH 3/9] fix(ds): leave focus on a field that autofocused inside a Modal (#754) The deferred initial focus moved focus to the first focusable element, the close button, a frame after an autoFocus field had taken it. A user typing straight away typed into nothing. It now stays put when focus is already in the panel. Signed-off-by: alex-dembele --- .../src/shared/ds/__tests__/primitives.test.tsx | 14 ++++++++++++++ frontend/src/shared/ds/useDismissableLayer.ts | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/frontend/src/shared/ds/__tests__/primitives.test.tsx b/frontend/src/shared/ds/__tests__/primitives.test.tsx index 8ae3a9c1..9fd9ea15 100644 --- a/frontend/src/shared/ds/__tests__/primitives.test.tsx +++ b/frontend/src/shared/ds/__tests__/primitives.test.tsx @@ -464,6 +464,20 @@ describe('Modal', () => { expect(document.activeElement).toBe(trigger); }); + it('leaves focus on a field that autofocused instead of pulling it to the first button', async () => { + render( + {}} title="Confirm" closeLabel="Close"> + + , + ); + const field = screen.getByLabelText('Password'); + expect(document.activeElement).toBe(field); + + // The deferred initial focus has run by now; it must not have moved. + await act(() => new Promise((r) => requestAnimationFrame(() => r(undefined)))); + expect(document.activeElement).toBe(field); + }); + it('keeps Tab inside the dialog', async () => { const user = userEvent.setup(); render(); diff --git a/frontend/src/shared/ds/useDismissableLayer.ts b/frontend/src/shared/ds/useDismissableLayer.ts index 8e626bf3..9dec33a9 100644 --- a/frontend/src/shared/ds/useDismissableLayer.ts +++ b/frontend/src/shared/ds/useDismissableLayer.ts @@ -102,6 +102,10 @@ export function useDismissableLayer( // Focus after paint: the panel animates in, and focusing an element that // is still mid-transform makes some browsers scroll the container. const focusFrame = requestAnimationFrame(() => { + // A field inside the panel that took focus itself (autoFocus) is where the + // user expects to type; pulling focus to the close button would swallow + // their first keystrokes. + if (panelRef.current?.contains(document.activeElement)) return; const target = initialFocusRef?.current ?? panelRef.current?.querySelector(FOCUSABLE) ?? From 6f0eb5bda885010e1d511f971bd8e75dce51bb77 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 08:31:38 +0100 Subject: [PATCH 4/9] feat(settings): turn MFA off from Settings with password confirmation (#754) A dialog asks for the current password, reports a wrong one on the field and refusals it cannot fix plainly, and hides the button for roles that require MFA. The mutation never retries: every request is a password guess counted against the server's budget, and the app-wide retry of 3 spent four of five attempts on one typo. Signed-off-by: alex-dembele --- .../src/features/auth/MFADisableDialog.tsx | 169 ++++++++++++++++++ frontend/src/features/auth/authService.ts | 22 +++ frontend/src/features/auth/useMfa.ts | 30 +++- .../notifications/notificationService.ts | 4 +- .../src/features/settings/MFAPolicyPanel.tsx | 31 +++- .../settings/__tests__/mfaDisable.test.tsx | 167 +++++++++++++++++ frontend/src/shared/notificationCategory.ts | 1 + 7 files changed, 419 insertions(+), 5 deletions(-) create mode 100644 frontend/src/features/auth/MFADisableDialog.tsx create mode 100644 frontend/src/features/settings/__tests__/mfaDisable.test.tsx diff --git a/frontend/src/features/auth/MFADisableDialog.tsx b/frontend/src/features/auth/MFADisableDialog.tsx new file mode 100644 index 00000000..ada496ba --- /dev/null +++ b/frontend/src/features/auth/MFADisableDialog.tsx @@ -0,0 +1,169 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// +// #754 — turning MFA off from Settings › Security. +// +// An open session is not proof of who is at the keyboard, so the server asks +// for the password again. The dialog says what is lost (the authenticator AND +// the backup codes), and a wrong password is reported on the field itself. + +import { useForm } from 'react-hook-form'; +import { zodResolver } from '@hookform/resolvers/zod'; +import { isAxiosError } from 'axios'; +import { z } from 'zod'; +import { ShieldOff } from 'lucide-react'; +import { toast } from 'sonner'; +import { useState } from 'react'; + +import { Button, Field, Input, Modal } from '../../shared/ds'; +import { useUIStore } from '../../store/uiStore'; +import { useAuthStore } from '../../hooks/useAuthStore'; +import { useDisableMFA } from './useMfa'; +import type { DisableMFAErrorBody } from './authService'; + +type Tr = (fr: string, en: string) => string; + +function schema(tr: Tr) { + return z.object({ + password: z.string().min(1, tr('Saisissez votre mot de passe.', 'Enter your password.')), + }); +} + +type Values = z.infer>; + +export function MFADisableDialog({ onClose }: { onClose: () => void }) { + const lang = useUIStore((s) => s.lang); + const tr: Tr = (fr, en) => (lang === 'fr' ? fr : en); + const email = useAuthStore((s) => s.user?.email ?? ''); + const disable = useDisableMFA(); + // A refusal the password field cannot fix (role, identity provider, throttle). + const [blocked, setBlocked] = useState(null); + + const { + register, + handleSubmit, + setError, + formState: { errors }, + } = useForm({ + resolver: zodResolver(schema(tr)), + defaultValues: { password: '' }, + }); + + const onSubmit = (v: Values) => { + setBlocked(null); + disable.mutate( + { password: v.password, locale: lang }, + { + onSuccess: (res) => { + toast.success(res.message); + onClose(); + }, + onError: (err) => { + const body = (isAxiosError(err) ? err.response?.data : undefined) as + DisableMFAErrorBody | undefined; + switch (body?.code) { + case 'wrong_password': + setError('password', { message: body.error }); + return; + case 'mfa_required_by_role': + case 'no_local_password': + case 'too_many_attempts': + setBlocked(body.error ?? ''); + return; + case 'not_enrolled': + // Already off (another tab, another device): nothing left to do. + toast.success(body.error ?? ''); + onClose(); + return; + default: + toast.error( + body?.error || + tr( + 'La désactivation a échoué. Réessayez dans un instant.', + 'Could not turn it off. Try again in a moment.', + ), + ); + } + }, + }, + ); + }; + + const busy = disable.isPending; + + return ( + + + ); +} diff --git a/frontend/src/features/auth/authService.ts b/frontend/src/features/auth/authService.ts index 9e109a9f..325f33cd 100644 --- a/frontend/src/features/auth/authService.ts +++ b/frontend/src/features/auth/authService.ts @@ -218,6 +218,28 @@ export async function verifyMFA( return data; } +/** Why the server refused to turn MFA off (#754). */ +export type DisableMFAErrorCode = + | 'wrong_password' + | 'mfa_required_by_role' + | 'no_local_password' + | 'not_enrolled' + | 'too_many_attempts'; + +export interface DisableMFAErrorBody { + error?: string; + code?: DisableMFAErrorCode; +} + +/** + * Turns MFA off for the signed-in user. The session is not enough: the server + * re-checks the current password and refuses roles that require MFA (#754). + */ +export async function disableMFA(password: string, locale: Lang): Promise<{ message: string }> { + const { data } = await api.post<{ message: string }>('/auth/mfa/disable', { password, locale }); + return data; +} + export interface MFAChallengeResult { token_pair?: { access_token: string; refresh_token: string }; csrf_token?: string; diff --git a/frontend/src/features/auth/useMfa.ts b/frontend/src/features/auth/useMfa.ts index 495532b3..2d11f8ae 100644 --- a/frontend/src/features/auth/useMfa.ts +++ b/frontend/src/features/auth/useMfa.ts @@ -4,7 +4,9 @@ // OR26-03 — MFA state and policy hooks. import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; -import { fetchMFAStatus, fetchMFAPolicy, saveMFAPolicy } from './mfaPolicyService'; +import { fetchMFAStatus, fetchMFAPolicy, saveMFAPolicy, type MFAStatus } from './mfaPolicyService'; +import { disableMFA } from './authService'; +import type { Lang } from '../../store/uiStore'; /** Shared key so any flow that changes MFA state can invalidate the banner. */ export const MFA_STATUS_KEY = ['auth', 'mfa-status'] as const; @@ -51,3 +53,29 @@ export function useInvalidateMFAStatus() { const qc = useQueryClient(); return () => qc.invalidateQueries({ queryKey: MFA_STATUS_KEY }); } + +/** + * Turns MFA off (#754). + * + * Not optimistic: the server has to check the password first, and showing + * "MFA off" before it agrees would tell the user an unprotected account is + * what they have when it is not. Once the server says yes, the panel changes + * at once and a refetch confirms it. + */ +export function useDisableMFA() { + const qc = useQueryClient(); + return useMutation({ + mutationFn: ({ password, locale }: { password: string; locale: Lang }) => + disableMFA(password, locale), + // Never replay: every request is a password guess the server counts against + // a five-per-quarter-hour budget. The app-wide retry of 3 would spend four + // of them on one wrong password and lock the user out on their second try. + retry: false, + onSuccess: () => { + qc.setQueryData(MFA_STATUS_KEY, (prev) => + prev ? { ...prev, state: 'recommended', configured: false } : prev, + ); + void qc.invalidateQueries({ queryKey: MFA_STATUS_KEY }); + }, + }); +} diff --git a/frontend/src/features/notifications/notificationService.ts b/frontend/src/features/notifications/notificationService.ts index 7285f4ec..1cdf2797 100644 --- a/frontend/src/features/notifications/notificationService.ts +++ b/frontend/src/features/notifications/notificationService.ts @@ -19,7 +19,9 @@ export type NotificationType = | 'automation' | 'sla_breach' // A J-7 / J-3 / J-1 questionnaire reminder went to a vendor, or could not (#672). - | 'vendor_assessment_reminder'; + | 'vendor_assessment_reminder' + // Two-factor authentication was turned off on the account (#754). + | 'mfa_disabled'; export interface Notification { id: string; diff --git a/frontend/src/features/settings/MFAPolicyPanel.tsx b/frontend/src/features/settings/MFAPolicyPanel.tsx index 5a5023e0..34818d82 100644 --- a/frontend/src/features/settings/MFAPolicyPanel.tsx +++ b/frontend/src/features/settings/MFAPolicyPanel.tsx @@ -20,6 +20,7 @@ import { useUIStore } from '../../store/uiStore'; import { useAuthStore } from '../../hooks/useAuthStore'; import { useMFAPolicy, useSaveMFAPolicy, useMFAStatus } from '../auth/useMfa'; import { MFAEnrollmentDialog } from '../auth/MFAEnrollmentDialog'; +import { MFADisableDialog } from '../auth/MFADisableDialog'; import { useI18n } from '../../hooks/useI18n'; export function MFAPolicyPanel() { @@ -210,6 +211,7 @@ export function MFAAccountPanel() { const tr = (fr: string, en: string) => (lang === 'fr' ? fr : en); const { data: status, isLoading, isError, refetch } = useMFAStatus(); const [enrolling, setEnrolling] = useState(false); + const [disabling, setDisabling] = useState(false); const body = () => { if (isLoading) return ; @@ -233,9 +235,31 @@ export function MFAAccountPanel() { } if (status?.state === 'configured') { return ( -
-
{body()} {enrolling && setEnrolling(false)} />} + {disabling && setDisabling(false)} />} ); } diff --git a/frontend/src/features/settings/__tests__/mfaDisable.test.tsx b/frontend/src/features/settings/__tests__/mfaDisable.test.tsx new file mode 100644 index 00000000..484a0ba1 --- /dev/null +++ b/frontend/src/features/settings/__tests__/mfaDisable.test.tsx @@ -0,0 +1,167 @@ +// #754 — turning MFA off from Settings › Security. +// +// What matters: the button is only offered to an account the server would let +// through, the password is required before anything is sent, a wrong password +// lands on the field, a refusal the field cannot fix is said plainly, and the +// panel only shows "off" once the server agreed. + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { AxiosError, AxiosHeaders, type AxiosResponse } from 'axios'; + +import { MFAAccountPanel } from '../MFAPolicyPanel'; +import type { MFAStatus } from '../../auth/mfaPolicyService'; + +const fetchMFAStatus = vi.fn(); +vi.mock('../../auth/mfaPolicyService', async () => { + const actual = await vi.importActual( + '../../auth/mfaPolicyService', + ); + return { ...actual, fetchMFAStatus: (...a: unknown[]) => fetchMFAStatus(...a) }; +}); + +const disableMFA = vi.fn(); +vi.mock('../../auth/authService', async () => { + const actual = + await vi.importActual('../../auth/authService'); + return { ...actual, disableMFA: (...a: unknown[]) => disableMFA(...a) }; +}); + +vi.mock('../../../hooks/useAuthStore', () => ({ + useAuthStore: (sel: (s: { user: { email: string } }) => unknown) => + sel({ user: { email: 'rssi@banque.cm' } }), +})); + +const toastSuccess = vi.fn(); +vi.mock('sonner', () => ({ + toast: { success: (...a: unknown[]) => toastSuccess(...a), error: vi.fn() }, +})); + +function status(over: Partial = {}): MFAStatus { + return { + state: 'configured', + configured: true, + required: false, + privileged: false, + grace_period_active: false, + grace_days: 7, + ...over, + } as MFAStatus; +} + +function refusal(code: string, error: string, httpStatus: number): AxiosError { + const response = { + status: httpStatus, + data: { code, error }, + statusText: '', + headers: {}, + config: { headers: new AxiosHeaders() }, + } as AxiosResponse; + return new AxiosError(error, String(httpStatus), undefined, undefined, response); +} + +function renderPanel() { + // Mutation retries as in src/main.tsx, so a replayed password guess shows up. + const qc = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: 3, retryDelay: 0 } }, + }); + return render( + + + , + ); +} + +async function openDialog() { + await userEvent.click(await screen.findByTestId('mfa-disable-open')); + return screen.findByTestId('mfa-disable-password'); +} + +beforeEach(() => { + vi.clearAllMocks(); + fetchMFAStatus.mockResolvedValue(status()); +}); + +describe('turning MFA off', () => { + it('offers no button to a role the server would refuse', async () => { + fetchMFAStatus.mockResolvedValue(status({ privileged: true })); + renderPanel(); + + expect(await screen.findByTestId('mfa-disable-locked')).toBeInTheDocument(); + expect(screen.queryByTestId('mfa-disable-open')).not.toBeInTheDocument(); + }); + + it('sends nothing without a password', async () => { + renderPanel(); + await openDialog(); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect( + await screen.findByText(/enter your password|saisissez votre mot de passe/i), + ).toBeInTheDocument(); + expect(disableMFA).not.toHaveBeenCalled(); + }); + + it('reports a wrong password on the field and keeps MFA shown as on', async () => { + disableMFA.mockRejectedValue(refusal('wrong_password', 'Incorrect password.', 401)); + renderPanel(); + await userEvent.type(await openDialog(), 'guess-1234'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect(await screen.findByText('Incorrect password.')).toBeInTheDocument(); + expect(screen.getByTestId('mfa-disable-password')).toHaveAttribute('aria-invalid', 'true'); + expect(document.body.textContent).toMatch(/MFA is enabled|Le MFA est activé/); + }); + + it('sends a wrong password once, never replays it against the attempt budget', async () => { + disableMFA.mockRejectedValue(refusal('wrong_password', 'Incorrect password.', 401)); + renderPanel(); + await userEvent.type(await openDialog(), 'guess-1234'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect(await screen.findByText('Incorrect password.')).toBeInTheDocument(); + expect(disableMFA).toHaveBeenCalledTimes(1); + }); + + it('says plainly when the server refuses for a reason the password cannot fix', async () => { + disableMFA.mockRejectedValue( + refusal('too_many_attempts', 'Too many attempts. Try again in a few minutes.', 429), + ); + renderPanel(); + await userEvent.type(await openDialog(), 'Ancre-Vitrail7-Cobalt'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect((await screen.findByTestId('mfa-disable-blocked')).textContent).toMatch( + /too many attempts/i, + ); + expect(screen.getByTestId('mfa-disable-submit')).toBeDisabled(); + }); + + it('sends the password and flips the panel once the server agrees', async () => { + disableMFA.mockResolvedValue({ message: 'Two-factor authentication turned off.' }); + fetchMFAStatus + .mockResolvedValueOnce(status()) + .mockResolvedValue(status({ state: 'recommended', configured: false })); + renderPanel(); + await userEvent.type(await openDialog(), 'Ancre-Vitrail7-Cobalt'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + await waitFor(() => + expect(disableMFA).toHaveBeenCalledWith('Ancre-Vitrail7-Cobalt', expect.any(String)), + ); + await waitFor(() => + expect(screen.queryByTestId('mfa-disable-password')).not.toBeInTheDocument(), + ); + expect(toastSuccess).toHaveBeenCalledWith('Two-factor authentication turned off.'); + expect( + await screen.findByRole('button', { name: /enable mfa|activer le mfa/i }), + ).toBeInTheDocument(); + }); +}); diff --git a/frontend/src/shared/notificationCategory.ts b/frontend/src/shared/notificationCategory.ts index a69cc5df..1798a685 100644 --- a/frontend/src/shared/notificationCategory.ts +++ b/frontend/src/shared/notificationCategory.ts @@ -89,6 +89,7 @@ export function categoryForType(type: string): NotifCategory { case 'risk_resolved': case 'scan_complete': case 'sla_breach': + case 'mfa_disabled': case 'automation': default: return 'security'; From e125f6c5b0a5cea2d61b4e3073645fe81fd90a62 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 09:00:38 +0100 Subject: [PATCH 5/9] test(auth): prove the MFA disable rollback on a real Postgres (#754) The sqlite tests fail the second write through a GORM callback. This one, gated on DATABASE_URL, makes Postgres refuse it with a trigger and checks the secret delete rolls back, the tenant scope holds, and re-enrolment works after a hard delete. Signed-off-by: alex-dembele --- .../repository/gorm_mfa_repository_pg_test.go | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 backend/internal/infrastructure/repository/gorm_mfa_repository_pg_test.go diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository_pg_test.go b/backend/internal/infrastructure/repository/gorm_mfa_repository_pg_test.go new file mode 100644 index 00000000..5c639250 --- /dev/null +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository_pg_test.go @@ -0,0 +1,90 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package repository + +import ( + "context" + "errors" + "os" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + "github.com/opendefender/openrisk/internal/domain" +) + +// #754 — the sqlite tests fail the second write with a GORM callback; this one +// makes Postgres itself refuse it, with a trigger, and checks the secret delete +// is rolled back by the database. Temporary tables shadow the real ones for this +// connection only, and the outer transaction is rolled back at the end. +func TestGormMFARepository_DisableMFA_Postgres(t *testing.T) { + dsn := os.Getenv("DATABASE_URL") + if dsn == "" { + t.Skip("DATABASE_URL not set") + } + db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + + rollback := errors.New("rollback") + err = db.Transaction(func(tx *gorm.DB) error { + for _, sql := range []string{ + `CREATE TEMP TABLE mfa_secrets (LIKE public.mfa_secrets INCLUDING ALL) ON COMMIT DROP`, + `CREATE TEMP TABLE mfa_backup_codes (LIKE public.mfa_backup_codes INCLUDING ALL) ON COMMIT DROP`, + `CREATE FUNCTION pg_temp.refuse_code_delete() RETURNS trigger LANGUAGE plpgsql AS + $$ BEGIN IF current_setting('or754.refuse', true) = 'on' THEN + RAISE EXCEPTION 'backup code delete refused'; END IF; RETURN OLD; END $$`, + `CREATE TRIGGER refuse BEFORE DELETE ON pg_temp.mfa_backup_codes + FOR EACH ROW EXECUTE FUNCTION pg_temp.refuse_code_delete()`, + } { + require.NoError(t, tx.Exec(sql).Error, sql) + } + repo := NewGormMFARepository(tx) + ctx := context.Background() + user, tenant := uuid.New(), uuid.New() + seed := func() { + require.NoError(t, tx.Create(&domain.MFASecret{ID: uuid.New(), UserID: user, TenantID: tenant, SecretEncrypted: "x", IsVerified: true}).Error) + for range 3 { + require.NoError(t, tx.Create(&domain.MFABackupCode{ID: uuid.New(), UserID: user, TenantID: tenant, CodeHash: uuid.NewString()}).Error) + } + } + count := func() (secrets, codes int64) { + // Tenant-agnostic on purpose: proves no row survives under ANY tenant. + require.NoError(t, tx.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", user).Count(&secrets).Error) + // Same: every tenant's backup codes for this user. + require.NoError(t, tx.Model(&domain.MFABackupCode{}).Where("user_id = ?", user).Count(&codes).Error) + return + } + seed() + + // Postgres refuses the second write: nothing may be deleted. + require.NoError(t, tx.Exec(`SET LOCAL or754.refuse = 'on'`).Error) + require.Error(t, repo.DisableMFA(ctx, user, tenant)) + s, c := count() + assert.EqualValues(t, 1, s, "the secret delete must roll back with the failed code delete") + assert.EqualValues(t, 3, c) + + // Another tenant's id deletes nothing. + require.NoError(t, tx.Exec(`SET LOCAL or754.refuse = 'off'`).Error) + require.NoError(t, repo.DisableMFA(ctx, user, uuid.New())) + s, c = count() + assert.EqualValues(t, 1, s) + assert.EqualValues(t, 3, c) + + // The real call removes both, and the unique user_id lets the user enrol again. + require.NoError(t, repo.DisableMFA(ctx, user, tenant)) + s, c = count() + assert.Zero(t, s, "hard delete, no tombstone") + assert.Zero(t, c) + seed() + return rollback + }) + require.ErrorIs(t, err, rollback) +} From 94d4dacc1f0b5ce185e46f3d9f30baf23ec791a0 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 09:11:21 +0100 Subject: [PATCH 6/9] feat(auth): let an SSO account turn MFA off with an authenticator code (#754) An account that signs in through an identity provider has no local password, so the disable endpoint refused it outright. It now confirms with a current TOTP code instead (401 wrong_code otherwise, same per-account budget). Backup codes do not count, and a code never replaces the password of an account that has one. Without the key wired, the refusal stays: it fails closed. /auth/me returns has_password so the client asks for the right proof, and the deactivation notice no longer claims a password was confirmed. Owner decision D-061. Signed-off-by: alex-dembele --- backend/cmd/server/main.go | 6 +- .../application/auth/mfa_disable_test.go | 76 ++++++++++++++++++- .../internal/application/auth/mfa_usecase.go | 43 +++++++++-- backend/internal/auth/audit.go | 6 +- backend/internal/handler/auth/handler.go | 4 + .../handler/auth/mfa_deferred_e2e_test.go | 4 + .../handler/auth/mfa_disable_e2e_test.go | 55 ++++++++++++++ backend/internal/handler/auth/mfa_handler.go | 12 ++- .../infrastructure/authmail/reset_mailer.go | 8 +- 9 files changed, 194 insertions(+), 20 deletions(-) diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index 600489c5..2d2f087c 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -692,9 +692,11 @@ func main() { // MFA use cases + handler. setupMFAUseCase := auth.NewSetupMFAUseCase(mfaRepo, mfaKey[:]) verifyMFAUseCase := auth.NewVerifyMFAUseCase(mfaRepo, *userRepo, mfaKey[:]) - // #754 — removing the factor re-proves the password, is refused for the - // roles login requires MFA for, and mails the owner. + // #754 — removing the factor re-proves the password (or, with no local + // password, a current authenticator code), is refused for the roles login + // requires MFA for, and mails the owner. disableMFAUseCase := auth.NewDisableMFAUseCase(mfaRepo, userRepo, passwordHasher). + WithTOTPKey(mfaKey[:]). RequireMFAForRoles(mfaRequiredRoles, mfaRequiredBusinessRoles). WithMailer(securityMailer) challengeMFAUseCase := auth.NewChallengeMFAUseCase(mfaRepo, mfaKey[:]) diff --git a/backend/internal/application/auth/mfa_disable_test.go b/backend/internal/application/auth/mfa_disable_test.go index 8ab65a69..4e757afb 100644 --- a/backend/internal/application/auth/mfa_disable_test.go +++ b/backend/internal/application/auth/mfa_disable_test.go @@ -9,12 +9,16 @@ import ( "context" "errors" "testing" + "time" "github.com/google/uuid" + "github.com/pquerna/otp/totp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" ) // #754 — disabling MFA re-proves the password, is refused for roles that @@ -193,10 +197,80 @@ func TestDisableMFA_NoRolePolicyAllowsAnAdmin(t *testing.T) { require.NoError(t, f.run(disablePassword, "admin")) } -func TestDisableMFA_AccountWithoutLocalPasswordIsRefused(t *testing.T) { +// An account that signs in through an identity provider has no password to +// re-prove; it proves possession of the factor it removes instead. + +var disableTOTPKey = []byte("0123456789abcdef0123456789abcdef") + +// ssoAccount turns the fixture's user into an identity-provider account whose +// stored secret is real, and returns that secret. +func (f *disableFixture) ssoAccount(t *testing.T) string { + t.Helper() + f.user.Password = "" + plain, err := otp.GenerateTOTPSecret() + require.NoError(t, err) + enc, err := crypto.EncryptAES256GCM(plain, disableTOTPKey) + require.NoError(t, err) + secret, _ := f.repo.GetMFASecret(context.Background(), f.user.ID, f.tenant) + secret.SecretEncrypted = enc + f.uc.WithTOTPKey(disableTOTPKey) + return plain +} + +func (f *disableFixture) runWithCode(password, code string) error { + _, err := f.uc.Execute(context.Background(), DisableMFAInput{ + UserID: f.user.ID, TenantID: f.tenant, Password: password, Code: code, Locale: "en", + }) + return err +} + +func TestDisableMFA_AccountWithoutLocalPasswordConfirmsWithACode(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + plain := f.ssoAccount(t) + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + + require.NoError(t, f.runWithCode("", code)) + + secret, _ := f.repo.GetMFASecret(context.Background(), f.user.ID, f.tenant) + assert.Nil(t, secret) + assert.Empty(t, f.repo.codes[f.codeKey]) + assert.Equal(t, 1, f.mailer.calls) +} + +func TestDisableMFA_AccountWithoutLocalPasswordRejectsAWrongOrMissingCode(t *testing.T) { + for name, code := range map[string]string{"wrong": "000000", "missing": ""} { + t.Run(name, func(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + plain := f.ssoAccount(t) + if name == "wrong" { + // Make sure "000000" is not, by chance, the current code. + if ok, _ := totp.ValidateCustom(code, plain, time.Now(), totp.ValidateOpts{Period: 30, Skew: 1, Digits: 6}); ok { + t.Skip("000000 happens to be valid right now") + } + } + + assert.ErrorIs(t, f.runWithCode("anything", code), ErrMFADisableCodeIncorrect) + f.assertStillEnrolled(t) + }) + } +} + +func TestDisableMFA_AccountWithoutLocalPasswordIsRefusedWithoutAKey(t *testing.T) { f := newDisableFixture(t, domain.RoleUser, "") f.user.Password = "" assert.ErrorIs(t, f.run("", ""), ErrNoLocalPassword) f.assertStillEnrolled(t) } + +func TestDisableMFA_ACodeDoesNotReplaceThePasswordOfAPasswordAccount(t *testing.T) { + f := newDisableFixture(t, domain.RoleUser, "") + plain := f.ssoAccount(t) + f.user.Password = "hashed:" + disablePassword // back to a password account + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + + assert.ErrorIs(t, f.runWithCode("", code), ErrMFADisablePasswordIncorrect) + f.assertStillEnrolled(t) +} diff --git a/backend/internal/application/auth/mfa_usecase.go b/backend/internal/application/auth/mfa_usecase.go index a78e60ec..c863f2d0 100644 --- a/backend/internal/application/auth/mfa_usecase.go +++ b/backend/internal/application/auth/mfa_usecase.go @@ -211,7 +211,10 @@ func (uc *VerifyMFAUseCase) Execute(ctx context.Context, input VerifyMFAInput) ( // // Removing a factor is the one MFA operation an attacker at an unlocked // workstation wants most, so the session alone is not enough: the caller must -// prove the password again. Privileged roles cannot remove it at all — the +// prove the password again. An account that signs in through an identity +// provider has no password here, so it proves possession of the factor it is +// removing instead: a current code from the authenticator app. Backup codes do +// not count — they are the factor most often written down next to the desk. Privileged roles cannot remove it at all — the // login policy would demand it back on the next sign-in, and in between the // account would sit on a password alone. // --------------------------------------------------------------------------- @@ -223,6 +226,9 @@ var ( // ErrMFARequiredByRole refuses the removal for a role the deployment requires // MFA for. ErrMFARequiredByRole = errors.New("two-factor authentication is required for this role") + // ErrMFADisableCodeIncorrect covers a wrong and a missing authenticator code + // alike, for an account without a local password. + ErrMFADisableCodeIncorrect = errors.New("authentication code is incorrect") ) // DisableMFAUserLookup reads the account and its membership. Satisfied by @@ -246,6 +252,9 @@ type DisableMFAInput struct { UserID uuid.UUID TenantID uuid.UUID Password string // Current password (required for security) + // Code is a current TOTP code. Required instead of Password when the account + // has no local password (identity-provider sign-in); ignored otherwise. + Code string // OrgRoleHint is the org role in the caller's signed token. It can only // widen the privileged check, never narrow it (same rule as MFAStatusResolver). OrgRoleHint string @@ -265,6 +274,9 @@ type DisableMFAUseCase struct { privileged domain.MFAPrivilegeSet mailer MFADisabledMailer inApp MFADisabledInAppNotifier + // totpKey decrypts the stored secret to check Code. Without it an account + // with no local password cannot prove anything and is refused. + totpKey []byte } // NewDisableMFAUseCase creates a new disable MFA use case @@ -283,6 +295,13 @@ func (uc *DisableMFAUseCase) RequireMFAForRoles(orgRoles, businessRoles []string return uc } +// WithTOTPKey lets an account without a local password confirm with a current +// authenticator code. Pass the same key the secrets were encrypted with. +func (uc *DisableMFAUseCase) WithTOTPKey(key []byte) *DisableMFAUseCase { + uc.totpKey = key + return uc +} + // WithMailer wires the deactivation notice. Optional. func (uc *DisableMFAUseCase) WithMailer(m MFADisabledMailer) *DisableMFAUseCase { uc.mailer = m @@ -309,12 +328,13 @@ func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) if user == nil || !user.IsActive { return nil, domain.NewNotFoundError("user", input.UserID) } - if user.Password == "" { - // An identity-provider account has no password to prove. Refusing is the - // safe answer until it can prove a factor another way. + hasPassword := user.Password != "" + if !hasPassword && uc.totpKey == nil { + // Nothing this account could prove. Refuse rather than let the session + // alone remove the factor. return nil, ErrNoLocalPassword } - if input.Password == "" || !uc.passwordHasher.Verify(user.Password, input.Password) { + if hasPassword && (input.Password == "" || !uc.passwordHasher.Verify(user.Password, input.Password)) { return nil, ErrMFADisablePasswordIncorrect } @@ -325,6 +345,15 @@ func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) if secret == nil { return nil, domain.NewNotFoundError("MFA secret", input.UserID) } + if !hasPassword { + plain, err := crypto.DecryptAES256GCM(secret.SecretEncrypted, uc.totpKey) + if err != nil { + return nil, fmt.Errorf("auth.DisableMFA: decrypt secret: %w", err) + } + if input.Code == "" || !otp.VerifyTOTP(plain, input.Code) { + return nil, ErrMFADisableCodeIncorrect + } + } if !uc.privileged.Empty() { member, err := uc.users.GetOrganizationMember(ctx, input.UserID, input.TenantID) @@ -361,10 +390,10 @@ func (uc *DisableMFAUseCase) Execute(ctx context.Context, input DisableMFAInput) func mfaDisabledInAppCopy(locale string) (string, string) { if locale == "en" { return "Two-factor authentication turned off", - "Two-factor authentication was turned off on your account after your password was confirmed. If this wasn't you, change your password and turn it back on from Settings → Security." + "Two-factor authentication was turned off on your account after your identity was confirmed. If this wasn't you, secure your account and turn it back on from Settings → Security." } return "Double authentification désactivée", - "La double authentification a été désactivée sur votre compte après confirmation de votre mot de passe. Si ce n'est pas vous, changez votre mot de passe et réactivez-la depuis Paramètres → Sécurité." + "La double authentification a été désactivée sur votre compte après confirmation de votre identité. Si ce n'est pas vous, sécurisez votre compte et réactivez-la depuis Paramètres → Sécurité." } // ChallengeMFAInput represents MFA challenge request (after login) diff --git a/backend/internal/auth/audit.go b/backend/internal/auth/audit.go index fc5b1f2b..277a78ad 100644 --- a/backend/internal/auth/audit.go +++ b/backend/internal/auth/audit.go @@ -35,9 +35,9 @@ const ( AuditActionPatUse AuditAction = "pat_use" // AuditActionMfaDisable records a request to remove the second factor - // (#754). Failures carry a reason ("wrong_password", "mfa_required_by_role", - // "no_local_password", "not_enrolled"): repeated wrong passwords here are a - // session thief guessing. + // (#754). Failures carry a reason ("wrong_password", "wrong_code", + // "mfa_required_by_role", "no_local_password", "not_enrolled"): repeated + // wrong passwords or codes here are a session thief guessing. AuditActionMfaDisable AuditAction = "mfa_disable" // Password reset. Both halves are recorded, and failures carry a reason diff --git a/backend/internal/handler/auth/handler.go b/backend/internal/handler/auth/handler.go index 99f06ef8..6989452e 100644 --- a/backend/internal/handler/auth/handler.go +++ b/backend/internal/handler/auth/handler.go @@ -548,6 +548,10 @@ func (h *Handler) Me(c *fiber.Ctx) error { if mfaBlock != nil { body["mfa"] = mfaBlock } + // #754 — whether the account can confirm with a password at all. An + // identity-provider account confirms sensitive changes (turning MFA + // off) with an authenticator code instead. A fact, not the hash. + body["has_password"] = user.Password != "" // The CURRENT business role, re-read per call like everything else // here. The client picks its persona and its landing route from // this; before #338 the field did not exist on this response, so diff --git a/backend/internal/handler/auth/mfa_deferred_e2e_test.go b/backend/internal/handler/auth/mfa_deferred_e2e_test.go index 4849729a..5a729d27 100644 --- a/backend/internal/handler/auth/mfa_deferred_e2e_test.go +++ b/backend/internal/handler/auth/mfa_deferred_e2e_test.go @@ -72,6 +72,9 @@ func (deferredHasher) NeedsRehash(string) bool { return false } const deferredPassword = "Ancre-Vitrail7-Cobalt" +// deferredTOTPKey encrypts the secrets of the identity-provider accounts (#754). +var deferredTOTPKey = []byte("0123456789abcdef0123456789abcdef") + func newDeferredFixture(t *testing.T) *deferredFixture { t.Helper() @@ -189,6 +192,7 @@ func newDeferredFixture(t *testing.T) *deferredFixture { protected.Post("/auth/mfa/setup", ok) // #754 — the real disable path: use case, handler, repository. disableUC := appauth.NewDisableMFAUseCase(mfaRepo, userRepo, deferredHasher{}). + WithTOTPKey(deferredTOTPKey). RequireMFAForRoles(orgRoles, businessRoles) mfaHandler := authhandler.NewMFAHandler(nil, nil, disableUC, nil, tokens, userRepo, nil). WithMFAStatus(f.resolver). diff --git a/backend/internal/handler/auth/mfa_disable_e2e_test.go b/backend/internal/handler/auth/mfa_disable_e2e_test.go index 5a5c0fd0..e01bfcb9 100644 --- a/backend/internal/handler/auth/mfa_disable_e2e_test.go +++ b/backend/internal/handler/auth/mfa_disable_e2e_test.go @@ -10,12 +10,16 @@ import ( "net/http" "sync" "testing" + "time" "github.com/google/uuid" + "github.com/pquerna/otp/totp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" ) // --------------------------------------------------------------------------- @@ -126,6 +130,57 @@ func TestMFADisableE2E_CorrectPasswordDisablesAndIsChained(t *testing.T) { assert.Equal(t, "not_enrolled", resp["code"]) } +// ssoSessionThenEnrol opens a session, then turns the member into an +// identity-provider account (no local password) enrolled with a real secret. +func (f *deferredFixture) ssoSessionThenEnrol(t *testing.T, m *domain.OrganizationMember) (token, plain string) { + t.Helper() + _, token = f.login(t, m.User.Email) + require.NotEmpty(t, token) + require.NoError(t, f.db.Model(&domain.User{}).Where("id = ?", m.UserID).Update("password", "").Error) + plain, err := otp.GenerateTOTPSecret() + require.NoError(t, err) + enc, err := crypto.EncryptAES256GCM(plain, deferredTOTPKey) + require.NoError(t, err) + require.NoError(t, f.db.Create(&domain.MFASecret{ + ID: uuid.New(), UserID: m.UserID, TenantID: m.OrganizationID, SecretEncrypted: enc, IsVerified: true, + }).Error) + return token, plain +} + +func TestMFADisableE2E_AccountWithoutPasswordConfirmsWithACode(t *testing.T) { + f := newDeferredFixture(t) + token, plain := f.ssoSessionThenEnrol(t, f.memberA) + + status, me := f.do(t, http.MethodGet, "/api/v1/auth/me", token, nil) + require.Equal(t, http.StatusOK, status) + assert.Equal(t, false, me["has_password"], "the dialog must know to ask for a code") + + status, resp := f.do(t, http.MethodPost, disablePath, token, jsonBody{"code": "000000"}) + if status != http.StatusOK { // "000000" could, once in a million, be the live code + assert.Equal(t, http.StatusUnauthorized, status) + assert.Equal(t, "wrong_code", resp["code"]) + secrets, _ := f.factorRows(t, f.memberA) + assert.EqualValues(t, 1, secrets, "a wrong code removes nothing") + } + + code, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + status, resp = f.do(t, http.MethodPost, disablePath, token, jsonBody{"code": code}) + require.Equal(t, http.StatusOK, status, "%v", resp) + secrets, codes := f.factorRows(t, f.memberA) + assert.Zero(t, secrets) + assert.Zero(t, codes) +} + +func TestMFADisableE2E_MeSaysAPasswordAccountHasAPassword(t *testing.T) { + f := newDeferredFixture(t) + token := f.sessionThenEnrol(t, f.memberA) + + status, me := f.do(t, http.MethodGet, "/api/v1/auth/me", token, nil) + require.Equal(t, http.StatusOK, status) + assert.Equal(t, true, me["has_password"]) +} + func TestMFADisableE2E_PrivilegedRoleIsRefused(t *testing.T) { f := newDeferredFixture(t) // Inside the grace window the admin holds a session without a code. diff --git a/backend/internal/handler/auth/mfa_handler.go b/backend/internal/handler/auth/mfa_handler.go index 094fc039..9f371eb4 100644 --- a/backend/internal/handler/auth/mfa_handler.go +++ b/backend/internal/handler/auth/mfa_handler.go @@ -182,9 +182,10 @@ func (h *MFAHandler) issueSessionResponse(c *fiber.Ctx, userID uuid.UUID, device // Disable turns MFA off for the current user (#754). // -// The session is not proof enough: the body must carry the current password. -// Neither the password nor anything derived from it is logged or echoed; audit -// failures carry a reason code only. +// The session is not proof enough: the body must carry the current password, +// or, for an account that signs in through an identity provider, a current +// authenticator code. Neither is logged or echoed; audit failures carry a +// reason code only. func (h *MFAHandler) Disable(c *fiber.Ctx) error { userID := ctxUUID(c, "user_id") tenantID := ctxUUID(c, "tenant_id") @@ -194,6 +195,7 @@ func (h *MFAHandler) Disable(c *fiber.Ctx) error { var req struct { Password string `json:"password"` + Code string `json:"code,omitempty"` Locale string `json:"locale,omitempty"` } if err := c.BodyParser(&req); err != nil { @@ -224,6 +226,7 @@ func (h *MFAHandler) Disable(c *fiber.Ctx) error { UserID: userID, TenantID: tenantID, Password: req.Password, + Code: req.Code, OrgRoleHint: orgRole, Locale: locale, }) @@ -235,6 +238,9 @@ func (h *MFAHandler) Disable(c *fiber.Ctx) error { // was refused", not "your session is over". return fail(fiber.StatusUnauthorized, "wrong_password", pick(locale, "Mot de passe incorrect.", "Incorrect password.")) + case errors.Is(err, appauth.ErrMFADisableCodeIncorrect): + return fail(fiber.StatusUnauthorized, "wrong_code", + pick(locale, "Code incorrect.", "Incorrect code.")) case errors.Is(err, appauth.ErrMFARequiredByRole): return fail(fiber.StatusForbidden, "mfa_required_by_role", pick(locale, diff --git a/backend/internal/infrastructure/authmail/reset_mailer.go b/backend/internal/infrastructure/authmail/reset_mailer.go index 3533f8f4..ce4eff9a 100644 --- a/backend/internal/infrastructure/authmail/reset_mailer.go +++ b/backend/internal/infrastructure/authmail/reset_mailer.go @@ -190,9 +190,9 @@ func mfaDisabledCopy(locale, name string) copyBlock { heading: "Two-factor authentication is off", paragraphs: []string{ fmt.Sprintf("Hello %s,", name), - "Two-factor authentication was just turned off on this OpenRisk account, after the password was confirmed. Your authenticator app and your backup codes no longer work. A password alone now opens the account.", + "Two-factor authentication was just turned off on this OpenRisk account, after the account holder's identity was confirmed (password, or a code from the authenticator app for accounts that sign in through an identity provider). Your authenticator app and your backup codes no longer work. A password alone now opens the account.", }, - footnote: "If this wasn't you, your password is known to someone else: reset it immediately from the sign-in page, turn two-factor authentication back on, and contact your OpenRisk administrator.", + footnote: "If this wasn't you, someone else can prove they are you: reset your password (or alert your identity provider's administrator), turn two-factor authentication back on, and contact your OpenRisk administrator.", } } return copyBlock{ @@ -200,9 +200,9 @@ func mfaDisabledCopy(locale, name string) copyBlock { heading: "La double authentification est désactivée", paragraphs: []string{ fmt.Sprintf("Bonjour %s,", name), - "La double authentification vient d'être désactivée sur ce compte OpenRisk, après confirmation du mot de passe. Votre application d'authentification et vos codes de secours ne fonctionnent plus. Le mot de passe seul suffit désormais pour ouvrir le compte.", + "La double authentification vient d'être désactivée sur ce compte OpenRisk, après confirmation de l'identité du titulaire (mot de passe, ou code de l'application d'authentification pour les comptes connectés via un fournisseur d'identité). Votre application d'authentification et vos codes de secours ne fonctionnent plus. Le mot de passe seul suffit désormais pour ouvrir le compte.", }, - footnote: "Si vous n'êtes pas à l'origine de ce changement, quelqu'un d'autre connaît votre mot de passe : réinitialisez-le immédiatement depuis la page de connexion, réactivez la double authentification et contactez votre administrateur OpenRisk.", + footnote: "Si vous n'êtes pas à l'origine de ce changement, quelqu'un d'autre peut se faire passer pour vous : réinitialisez votre mot de passe (ou alertez l'administrateur de votre fournisseur d'identité), réactivez la double authentification et contactez votre administrateur OpenRisk.", } } From eb5f8e3fa25b52dbd09224336b6d527bbdb4f249 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 30 Sep 2026 09:11:21 +0100 Subject: [PATCH 7/9] feat(settings): ask an SSO account for its authenticator code to turn MFA off (#754) The dialog reads has_password when it opens and shows either the password field or a six-digit code field, with copy that matches. If it guessed wrong (the read failed), a wrong_code answer to a password switches it to the code. Signed-off-by: alex-dembele --- .../src/features/auth/MFADisableDialog.tsx | 153 +++++++++++++----- frontend/src/features/auth/authService.ts | 24 ++- frontend/src/features/auth/useMfa.ts | 21 ++- .../settings/__tests__/mfaDisable.test.tsx | 74 ++++++++- 4 files changed, 228 insertions(+), 44 deletions(-) diff --git a/frontend/src/features/auth/MFADisableDialog.tsx b/frontend/src/features/auth/MFADisableDialog.tsx index ada496ba..cb27f760 100644 --- a/frontend/src/features/auth/MFADisableDialog.tsx +++ b/frontend/src/features/auth/MFADisableDialog.tsx @@ -4,10 +4,12 @@ // #754 — turning MFA off from Settings › Security. // // An open session is not proof of who is at the keyboard, so the server asks -// for the password again. The dialog says what is lost (the authenticator AND -// the backup codes), and a wrong password is reported on the field itself. +// for the password again — or, for an account that signs in through an +// identity provider and has no password here, a current code from the +// authenticator app. The dialog says what is lost (the authenticator AND the +// backup codes), and a wrong answer is reported on the field itself. -import { useForm } from 'react-hook-form'; +import { Controller, useForm } from 'react-hook-form'; import { zodResolver } from '@hookform/resolvers/zod'; import { isAxiosError } from 'axios'; import { z } from 'zod'; @@ -15,17 +17,27 @@ import { ShieldOff } from 'lucide-react'; import { toast } from 'sonner'; import { useState } from 'react'; -import { Button, Field, Input, Modal } from '../../shared/ds'; +import { Button, Field, Input, Modal, OtpField } from '../../shared/ds'; +import { SkeletonRows } from '../../shared/ui'; import { useUIStore } from '../../store/uiStore'; import { useAuthStore } from '../../hooks/useAuthStore'; -import { useDisableMFA } from './useMfa'; -import type { DisableMFAErrorBody } from './authService'; +import { useDisableMFA, useDisableMFAProof } from './useMfa'; +import type { DisableMFAErrorBody, DisableMFAProof } from './authService'; type Tr = (fr: string, en: string) => string; -function schema(tr: Tr) { +function schema(tr: Tr, proof: DisableMFAProof) { return z.object({ - password: z.string().min(1, tr('Saisissez votre mot de passe.', 'Enter your password.')), + password: + proof === 'password' + ? z.string().min(1, tr('Saisissez votre mot de passe.', 'Enter your password.')) + : z.string(), + code: + proof === 'code' + ? z + .string() + .regex(/^\d{6}$/, tr('Saisissez les 6 chiffres du code.', 'Enter the 6-digit code.')) + : z.string(), }); } @@ -36,23 +48,30 @@ export function MFADisableDialog({ onClose }: { onClose: () => void }) { const tr: Tr = (fr, en) => (lang === 'fr' ? fr : en); const email = useAuthStore((s) => s.user?.email ?? ''); const disable = useDisableMFA(); + const proofQuery = useDisableMFAProof(); + // The server can correct the guess: `wrong_code` to a password means this + // account has no password and must confirm with its authenticator. + const [corrected, setCorrected] = useState(null); + const proof: DisableMFAProof = corrected ?? proofQuery.data ?? 'password'; // A refusal the password field cannot fix (role, identity provider, throttle). const [blocked, setBlocked] = useState(null); const { register, + control, handleSubmit, setError, + clearErrors, formState: { errors }, } = useForm({ - resolver: zodResolver(schema(tr)), - defaultValues: { password: '' }, + resolver: zodResolver(schema(tr, proof)), + defaultValues: { password: '', code: '' }, }); const onSubmit = (v: Values) => { setBlocked(null); disable.mutate( - { password: v.password, locale: lang }, + { proof: proof === 'code' ? { code: v.code } : { password: v.password }, locale: lang }, { onSuccess: (res) => { toast.success(res.message); @@ -65,6 +84,15 @@ export function MFADisableDialog({ onClose }: { onClose: () => void }) { case 'wrong_password': setError('password', { message: body.error }); return; + case 'wrong_code': + if (proof !== 'code') { + // We asked for a password this account does not have. + clearErrors(); + setCorrected('code'); + return; + } + setError('code', { message: body.error }); + return; case 'mfa_required_by_role': case 'no_local_password': case 'too_many_attempts': @@ -90,6 +118,7 @@ export function MFADisableDialog({ onClose }: { onClose: () => void }) { }; const busy = disable.isPending; + const resolving = proofQuery.isLoading && corrected === null; return ( void }) { dismissable={!busy} closeLabel={tr('Fermer', 'Close')} title={tr('Désactiver le MFA', 'Turn off MFA')} - subtitle={tr( - 'Confirmez votre mot de passe pour continuer.', - 'Confirm your password to continue.', - )} + subtitle={ + proof === 'code' + ? tr( + "Confirmez avec un code de votre application d'authentification.", + 'Confirm with a code from your authenticator app.', + ) + : tr('Confirmez votre mot de passe pour continuer.', 'Confirm your password to continue.') + } leading={