diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index fd47317e..2d2f087c 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -692,7 +692,13 @@ 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 (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[:]) // 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 +707,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 +1121,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 +2254,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..4e757afb --- /dev/null +++ b/backend/internal/application/auth/mfa_disable_test.go @@ -0,0 +1,276 @@ +// 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" + "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 +// 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")) +} + +// 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 468d93dc..c863f2d0 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,59 @@ 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. 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. +// --------------------------------------------------------------------------- + +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") + // 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 +// *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) + // 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 + Locale string } // DisableMFAOutput represents MFA disable response @@ -220,32 +269,117 @@ type DisableMFAOutput struct { // DisableMFAUseCase handles MFA disable type DisableMFAUseCase struct { mfaRepo repository.MFARepository + users DisableMFAUserLookup passwordHasher PasswordHasher + 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 -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 +} + +// 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 + 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) + } + 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 hasPassword && (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 !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) + 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 +387,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 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 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) type ChallengeMFAInput struct { UserID uuid.UUID 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/auth/audit.go b/backend/internal/auth/audit.go index d10b333f..277a78ad 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", "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 // ("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/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 176b4686..5a729d27 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 @@ -70,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() @@ -173,6 +178,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 +190,14 @@ 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{}). + WithTOTPKey(deferredTOTPKey). + 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..e01bfcb9 --- /dev/null +++ b/backend/internal/handler/auth/mfa_disable_e2e_test.go @@ -0,0 +1,235 @@ +// 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" + "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 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"]) +} + +// 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. + 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..9f371eb4 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,12 @@ 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, +// 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") @@ -168,18 +195,86 @@ 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"` } - _ = 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, + Code: req.Code, + 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.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, + "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..ce4eff9a 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 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, 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{ + 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 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 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.", + } +} + func newSignInCopy(locale, name, ip, userAgent string, when time.Time) copyBlock { stamp := when.UTC().Format("2006-01-02 15:04 UTC") if locale == "en" { 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_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) +} 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 diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 7e175a53..1ed2ff33 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -9,6 +9,29 @@ Nothing open. ## Resolved +### D-061 — SSO accounts turn MFA off with an authenticator code · decided 2026-09-30 +**Decided (owner)** — An account with no local password (identity-provider sign-in) +confirms turning MFA off with a **current TOTP code** from its authenticator app. Backup +codes are not accepted for this. Accounts with a password keep confirming with the +password, and a code does not replace it. + +**Why it came here** — #754 made the disable endpoint re-verify the password. An SSO account +has no password to re-verify, so the first version refused it outright (409 +`no_local_password`). Letting it through some other way changes the auth design, which +CLAUDE.md says the owner decides. The owner asked for it directly on 2026-09-30. + +**Consequence** — `DisableMFAUseCase.WithTOTPKey` decrypts the stored secret and checks the +code. A wrong or missing code gets 401 `wrong_code` and counts against the same 5-per-15-minute +per-account budget. Without the key wired, SSO accounts are still refused, so a +misconfiguration fails closed. `/auth/me` now returns `has_password` (a boolean, never the +hash) so the dialog asks for the right proof. Backup codes are left out because they are the +factor most often written down next to the workstation, which is the threat #754 is about. +A TOTP code is valid for about 90 s (±1 step) and is not marked as used, the same as at login: +someone who watched a code being typed could replay it inside that window. That matches the +existing login challenge and is not made worse here. + +**Unblocked** — #754, PR #848. + ### D-060 — animated KPI counters: an in-house rolling counter on the three KPIs of #751 · decided 2026-09-28 **Decided (owner)** — **A, against the recommendation** (B, change once). An in-house slot-reel counter (transitions.dev `spinning-counter`) goes on the three KPIs of diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 8129689b..137ea348 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -365,6 +365,101 @@ paths: '429': description: Rate limited + /auth/me: + get: + tags: + - Authentication + summary: The signed-in user, their MFA state and how they can prove who they are + description: > + Re-read on every call, never taken from the token alone. `mfa` is + omitted when the server cannot resolve it (read that as unknown, not as + "fine"). `has_password` is false for an account that signs in through + an identity provider; such an account confirms sensitive changes, like + turning MFA off, with an authenticator code instead (#754, D-061). It is + a boolean, never the hash. + operationId: getMe + security: + - bearerAuth: [] + responses: + '200': + description: The current session's user + content: + application/json: + schema: + $ref: '#/components/schemas/MeResponse' + '401': + description: No session + '404': + description: The session's user no longer exists + + /auth/mfa/disable: + post: + tags: + - Authentication + summary: Turn off two-factor authentication for the signed-in user + description: > + An open session is not enough (#754). The body carries the current + `password`, or, when `/auth/me` says `has_password: false`, a current + six-digit `code` from the authenticator app; backup codes are not + accepted, and a code never replaces the password of an account that has + one. Refused for roles the deployment requires MFA for. Each attempt + counts against a per-account budget of 5 per 15 minutes (on top of the + per-IP limit). On success the TOTP secret and every backup code are + deleted in one transaction, the attempt is audited, and the owner is + notified by email and in-app. Neither the password nor the code is ever + logged or echoed. Errors carry a stable `code` (see DisableMFAError). + operationId: disableMfa + security: + - bearerAuth: [] + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAInput' + responses: + '200': + description: MFA is off + content: + application/json: + schema: + type: object + required: [message] + properties: + message: { type: string } + '400': + description: The body is not JSON + '401': + description: "`wrong_password` or `wrong_code`. Not a session error: the session stays valid." + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAError' + '403': + description: "`mfa_required_by_role`" + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAError' + '404': + description: "`not_enrolled`" + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAError' + '409': + description: "`no_local_password`: no password and no way to check a code on this deployment" + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAError' + '429': + description: "`too_many_attempts`, or the per-IP limit" + content: + application/json: + schema: + $ref: '#/components/schemas/DisableMFAError' + # ==================== ORGANIZATION ==================== /organization: get: @@ -6053,6 +6148,47 @@ components: transferred_at: type: string format: date-time + MeResponse: + type: object + required: [user, organization_id, has_password] + properties: + user: + $ref: '#/components/schemas/User' + organization_id: + type: string + format: uuid + has_password: + type: boolean + description: False for an identity-provider account with no local password. + mfa: + $ref: '#/components/schemas/MFAStatus' + business_role: + type: string + description: The caller's current business role in this organization, when they have one. + DisableMFAInput: + type: object + properties: + password: + type: string + format: password + description: Required when the account has a password. + code: + type: string + pattern: '^[0-9]{6}$' + description: Current authenticator code. Required instead of `password` when `has_password` is false. + locale: + type: string + enum: [fr, en] + DisableMFAError: + type: object + required: [error, code] + properties: + error: + type: string + description: Localised message, safe to show as is. + code: + type: string + enum: [wrong_password, wrong_code, mfa_required_by_role, no_local_password, not_enrolled, too_many_attempts, rejected, internal] ErrorResponse: type: object properties: diff --git a/frontend/src/features/auth/MFADisableDialog.tsx b/frontend/src/features/auth/MFADisableDialog.tsx new file mode 100644 index 00000000..cb27f760 --- /dev/null +++ b/frontend/src/features/auth/MFADisableDialog.tsx @@ -0,0 +1,250 @@ +// 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 — 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 { Controller, 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, OtpField } from '../../shared/ds'; +import { SkeletonRows } from '../../shared/ui'; +import { useUIStore } from '../../store/uiStore'; +import { useAuthStore } from '../../hooks/useAuthStore'; +import { useDisableMFA, useDisableMFAProof } from './useMfa'; +import type { DisableMFAErrorBody, DisableMFAProof } from './authService'; + +type Tr = (fr: string, en: string) => string; + +function schema(tr: Tr, proof: DisableMFAProof) { + return z.object({ + 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(), + }); +} + +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(); + 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, proof)), + defaultValues: { password: '', code: '' }, + }); + + const onSubmit = (v: Values) => { + setBlocked(null); + disable.mutate( + { proof: proof === 'code' ? { code: v.code } : { 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 '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': + 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; + const resolving = proofQuery.isLoading && corrected === null; + + return ( + + + ); +} diff --git a/frontend/src/features/auth/authService.ts b/frontend/src/features/auth/authService.ts index 9e109a9f..7ceeb5a7 100644 --- a/frontend/src/features/auth/authService.ts +++ b/frontend/src/features/auth/authService.ts @@ -218,6 +218,46 @@ export async function verifyMFA( return data; } +/** Why the server refused to turn MFA off (#754). */ +export type DisableMFAErrorCode = + | 'wrong_password' + | 'wrong_code' + | 'mfa_required_by_role' + | 'no_local_password' + | 'not_enrolled' + | 'too_many_attempts'; + +export interface DisableMFAErrorBody { + error?: string; + code?: DisableMFAErrorCode; +} + +/** + * What the server will accept as proof when turning MFA off (#754): the + * password, or — for an account that signs in through an identity provider and + * has none — a current authenticator code. Unknown reads as "password"; the + * server answers `wrong_code` if that was wrong, and the dialog switches. + */ +export type DisableMFAProof = 'password' | 'code'; + +export async function fetchDisableMFAProof(): Promise { + const { data } = await api.get<{ has_password?: boolean }>('/auth/me'); + return data?.has_password === false ? 'code' : 'password'; +} + +/** + * Turns MFA off for the signed-in user. The session is not enough: the server + * re-checks the password (or the authenticator code for an account without + * one) and refuses roles that require MFA (#754). + */ +export async function disableMFA( + proof: { password: string } | { code: string }, + locale: Lang, +): Promise<{ message: string }> { + const { data } = await api.post<{ message: string }>('/auth/mfa/disable', { ...proof, 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..b55fbc27 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, fetchDisableMFAProof } 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,44 @@ 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: ({ + proof, + locale, + }: { + proof: { password: string } | { code: string }; + locale: Lang; + }) => disableMFA(proof, 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 }); + }, + }); +} + +/** Which proof the disable dialog asks for (#754). Read fresh on every open. */ +export function useDisableMFAProof() { + return useQuery({ + queryKey: ['auth', 'mfa-disable-proof'], + queryFn: fetchDisableMFAProof, + staleTime: 0, + retry: 1, + }); +} 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..37cf202a --- /dev/null +++ b/frontend/src/features/settings/__tests__/mfaDisable.test.tsx @@ -0,0 +1,237 @@ +// #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(); +const fetchDisableMFAProof = vi.fn(); +vi.mock('../../auth/authService', async () => { + const actual = + await vi.importActual('../../auth/authService'); + return { + ...actual, + disableMFA: (...a: unknown[]) => disableMFA(...a), + fetchDisableMFAProof: (...a: unknown[]) => fetchDisableMFAProof(...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()); + fetchDisableMFAProof.mockResolvedValue('password'); +}); + +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( + { password: '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(); + }); + + describe('an account that signs in through an identity provider', () => { + it('asks for an authenticator code, not a password, and sends the code', async () => { + fetchDisableMFAProof.mockResolvedValue('code'); + disableMFA.mockResolvedValue({ message: 'Two-factor authentication turned off.' }); + renderPanel(); + await userEvent.click(await screen.findByTestId('mfa-disable-open')); + + const code = await screen.findByTestId('mfa-disable-code'); + expect(screen.queryByTestId('mfa-disable-password')).not.toBeInTheDocument(); + await userEvent.type(code, '123456'); + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + await waitFor(() => + expect(disableMFA).toHaveBeenCalledWith({ code: '123456' }, expect.any(String)), + ); + }); + + it('sends nothing until the code has six digits', async () => { + fetchDisableMFAProof.mockResolvedValue('code'); + renderPanel(); + await userEvent.click(await screen.findByTestId('mfa-disable-open')); + await userEvent.type(await screen.findByTestId('mfa-disable-code'), '123'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect((await screen.findByRole('alert')).textContent).toMatch(/6-digit code|6 chiffres/i); + expect(disableMFA).not.toHaveBeenCalled(); + }); + + it('reports a wrong code on the code field', async () => { + fetchDisableMFAProof.mockResolvedValue('code'); + disableMFA.mockRejectedValue(refusal('wrong_code', 'Incorrect code.', 401)); + renderPanel(); + await userEvent.click(await screen.findByTestId('mfa-disable-open')); + await userEvent.type(await screen.findByTestId('mfa-disable-code'), '000000'); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect(await screen.findByText('Incorrect code.')).toBeInTheDocument(); + expect(disableMFA).toHaveBeenCalledTimes(1); + }); + + it('switches to the code when the server says the account has no password', async () => { + // /auth/me could not be read, so the dialog guessed "password". + fetchDisableMFAProof.mockRejectedValue(new Error('offline')); + disableMFA.mockRejectedValue(refusal('wrong_code', 'Incorrect code.', 401)); + renderPanel(); + await userEvent.click(await screen.findByTestId('mfa-disable-open')); + // One retry (1 s) before the dialog falls back to asking for a password. + await userEvent.type( + await screen.findByTestId('mfa-disable-password', {}, { timeout: 3000 }), + 'not-mine', + ); + + await userEvent.click(screen.getByTestId('mfa-disable-submit')); + + expect(await screen.findByTestId('mfa-disable-code')).toBeInTheDocument(); + expect(screen.queryByTestId('mfa-disable-password')).not.toBeInTheDocument(); + }); + }); +}); 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) ?? 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';