diff --git a/backend/internal/application/auth/mfa_setup_reenrol_test.go b/backend/internal/application/auth/mfa_setup_reenrol_test.go index c5f4819d..ecf594dc 100644 --- a/backend/internal/application/auth/mfa_setup_reenrol_test.go +++ b/backend/internal/application/auth/mfa_setup_reenrol_test.go @@ -19,23 +19,10 @@ import ( "github.com/opendefender/openrisk/pkg/otp" ) -// uniqueMFARepository enforces mfa_secrets.user_id UNIQUE, which the plain mock -// does not: it overwrote on Create, so the defect behind #889 never showed. -type uniqueMFARepository struct{ *MockMFARepository } - -func (r uniqueMFARepository) CreateMFASecret(ctx context.Context, s *domain.MFASecret) error { - for _, existing := range r.secrets { - if existing.UserID == s.UserID { - return errors.New("duplicated key not allowed") - } - } - return r.MockMFARepository.CreateMFASecret(ctx, s) -} - func TestSetupMFA_ReplacesAnUnverifiedSecret(t *testing.T) { ctx := context.Background() key := make([]byte, 32) - repo := uniqueMFARepository{NewMockMFARepository()} + repo := NewMockMFARepository() uc := NewSetupMFAUseCase(repo, key) in := SetupMFAInput{UserID: uuid.New(), TenantID: uuid.New(), Email: "awa@example.test"} @@ -77,10 +64,10 @@ func TestSetupMFA_ReplacesAnUnverifiedSecret(t *testing.T) { func TestSetupMFA_VerifiedSecretIsKept(t *testing.T) { ctx := context.Background() key := make([]byte, 32) - repo := uniqueMFARepository{NewMockMFARepository()} + repo := NewMockMFARepository() userID, tenantID := uuid.New(), uuid.New() enc, _ := crypto.EncryptAES256GCM("KEEPME", key) - _ = repo.MockMFARepository.CreateMFASecret(ctx, &domain.MFASecret{ + _ = repo.CreateMFASecret(ctx, &domain.MFASecret{ ID: uuid.New(), UserID: userID, TenantID: tenantID, SecretEncrypted: enc, IsVerified: true, }) diff --git a/backend/internal/application/auth/mfa_usecase.go b/backend/internal/application/auth/mfa_usecase.go index 4ad2c7d1..7e7e3f6b 100644 --- a/backend/internal/application/auth/mfa_usecase.go +++ b/backend/internal/application/auth/mfa_usecase.go @@ -83,52 +83,20 @@ func (uc *SetupMFAUseCase) Execute(ctx context.Context, input SetupMFAInput) (*S return nil, fmt.Errorf("failed to encrypt secret: %w", err) } - // Store encrypted secret (not yet verified). An unverified secret left by - // an enrolment that was never finished (tab closed, token expired) is - // replaced: inserting a second row hit the unique user_id, and the account - // could never enrol again — locked out for good if its role requires MFA - // (#889). A verified secret was refused above and is never touched. - if existingSecret != nil { - replaced, err := uc.mfaRepo.ReplaceUnverifiedMFASecret(ctx, input.UserID, input.TenantID, encryptedSecret) - if err != nil { - return nil, fmt.Errorf("failed to replace unverified MFA secret: %w", err) - } - if !replaced { - // Verified between the read above and this write. - return nil, domain.NewConflictError("MFA", "already_enabled") - } - } else { - mfaSecret := &domain.MFASecret{ - UserID: input.UserID, - TenantID: input.TenantID, - SecretEncrypted: encryptedSecret, - IsVerified: false, - } - if err := uc.mfaRepo.CreateMFASecret(ctx, mfaSecret); err != nil { - return nil, fmt.Errorf("failed to store MFA secret: %w", err) - } - } - // Generate backup codes (CSPRNG, unique per user — see otp.GenerateBackupCodes). backupCodes, err := otp.GenerateBackupCodes() if err != nil { return nil, fmt.Errorf("failed to generate backup codes: %w", err) } - // Replace any previously-issued codes for this user so re-enrolment - // invalidates the old set (defence in depth against a stale/compromised set). - if err := uc.mfaRepo.DeleteBackupCodes(ctx, input.UserID, input.TenantID); err != nil { - return nil, fmt.Errorf("failed to clear previous backup codes: %w", err) - } - - // Hash and store backup codes - var hashedCodes []*domain.MFABackupCode + // Hashed before anything is written: bcrypt is slow, and the write below + // holds a row lock. + hashedCodes := make([]*domain.MFABackupCode, 0, len(backupCodes)) for _, code := range backupCodes { hash, err := bcrypt.GenerateFromPassword([]byte(code), bcrypt.DefaultCost) if err != nil { return nil, fmt.Errorf("failed to hash backup code: %w", err) } - hashedCodes = append(hashedCodes, &domain.MFABackupCode{ UserID: input.UserID, TenantID: input.TenantID, @@ -136,8 +104,20 @@ func (uc *SetupMFAUseCase) Execute(ctx context.Context, input SetupMFAInput) (*S }) } - if err := uc.mfaRepo.SaveBackupCodes(ctx, hashedCodes); err != nil { - return nil, fmt.Errorf("failed to store backup codes: %w", err) + // Store the unverified secret and the new codes together, or neither + // (#714): a secret saved without its codes leaves the user no way back if + // they lose the phone. An unverified secret left by an enrolment that was + // never finished (tab closed, token expired) is replaced, so the account + // can always start over (#889). Earlier codes are dropped, so re-enrolment + // invalidates a stale or compromised set. + stored, err := uc.mfaRepo.StartMFAEnrolment(ctx, input.UserID, input.TenantID, encryptedSecret, hashedCodes) + if err != nil { + return nil, fmt.Errorf("failed to store MFA enrolment: %w", err) + } + if !stored { + // Verified between the read above and this write, or the user's one + // secret (user_id is unique) belongs to another organisation. + return nil, domain.NewConflictError("MFA", "already_enabled") } return &SetupMFAOutput{ diff --git a/backend/internal/application/auth/mfa_usecase_test.go b/backend/internal/application/auth/mfa_usecase_test.go index ad509ff1..d3e85f47 100644 --- a/backend/internal/application/auth/mfa_usecase_test.go +++ b/backend/internal/application/auth/mfa_usecase_test.go @@ -7,12 +7,14 @@ package auth import ( "context" + "errors" "testing" "time" "github.com/google/uuid" "github.com/opendefender/openrisk/internal/domain" "github.com/stretchr/testify/assert" + "golang.org/x/crypto/bcrypt" ) // Mock MFA Repository @@ -20,6 +22,9 @@ type MockMFARepository struct { secrets map[string]*domain.MFASecret codes map[string][]*domain.MFABackupCode oauthProviders map[string]*domain.OAuthProvider + // enrolmentErr makes StartMFAEnrolment fail; enrolments counts its calls. + enrolmentErr error + enrolments int } func NewMockMFARepository() *MockMFARepository { @@ -56,15 +61,28 @@ func (m *MockMFARepository) ConsumeTOTPStep(ctx context.Context, userID, tenantI return true, nil } -func (m *MockMFARepository) ReplaceUnverifiedMFASecret(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string) (bool, error) { +// StartMFAEnrolment honours what the GORM version guarantees: user_id is +// unique across tenants, a verified secret is never replaced, and the secret +// and codes are written together or not at all. +func (m *MockMFARepository) StartMFAEnrolment(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string, codes []*domain.MFABackupCode) (bool, error) { + m.enrolments++ + if m.enrolmentErr != nil { + return false, m.enrolmentErr + } key := userID.String() + ":" + tenantID.String() - s, ok := m.secrets[key] - if !ok || s.IsVerified { - return false, nil + for k, s := range m.secrets { + if s.UserID == userID && (k != key || s.IsVerified) { + return false, nil + } } - s.SecretEncrypted = secretEncrypted - s.LastTOTPStep = nil - s.LastUsedAt = nil + if s, ok := m.secrets[key]; ok { + s.SecretEncrypted = secretEncrypted + s.LastTOTPStep = nil + s.LastUsedAt = nil + } else { + m.secrets[key] = &domain.MFASecret{ID: uuid.New(), UserID: userID, TenantID: tenantID, SecretEncrypted: secretEncrypted} + } + m.codes[key] = codes return true, nil } @@ -182,6 +200,67 @@ func TestSetupMFA_Success(t *testing.T) { assert.NotEmpty(t, output.Secret) assert.NotEmpty(t, output.QRCode) assert.Len(t, output.BackupCodes, 8) + + // The secret and the codes go to the store in one call (#714), each code + // hashed and scoped to the caller. + assert.Equal(t, 1, mfaRepo.enrolments) + stored := mfaRepo.codes[userID.String()+":"+tenantID.String()] + assert.Len(t, stored, 8) + for i, c := range stored { + assert.Equal(t, userID, c.UserID) + assert.Equal(t, tenantID, c.TenantID) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(c.CodeHash), []byte(output.BackupCodes[i]))) + } +} + +// No secret yet for this user and tenant: setup creates one rather than +// failing on the missing row. +func TestSetupMFA_NotFound(t *testing.T) { + ctx := context.Background() + mfaRepo := NewMockMFARepository() + userID, tenantID := uuid.New(), uuid.New() + + _, err := NewSetupMFAUseCase(mfaRepo, make([]byte, 32)). + Execute(ctx, SetupMFAInput{UserID: userID, TenantID: tenantID, Email: "user@example.com"}) + + assert.NoError(t, err) + secret, _ := mfaRepo.GetMFASecret(ctx, userID, tenantID) + if assert.NotNil(t, secret) { + assert.False(t, secret.IsVerified, "a new secret awaits verification") + } +} + +// Without a session identity nothing is generated or written. +func TestSetupMFA_Unauthorized(t *testing.T) { + for name, in := range map[string]SetupMFAInput{ + "no user": {TenantID: uuid.New(), Email: "user@example.com"}, + "no tenant": {UserID: uuid.New(), Email: "user@example.com"}, + } { + t.Run(name, func(t *testing.T) { + mfaRepo := NewMockMFARepository() + _, err := NewSetupMFAUseCase(mfaRepo, make([]byte, 32)).Execute(context.Background(), in) + + var appErr *domain.AppError + assert.True(t, errors.As(err, &appErr) && errors.Is(appErr.Err, domain.ErrValidation), "got %v", err) + assert.Zero(t, mfaRepo.enrolments) + assert.Empty(t, mfaRepo.secrets) + }) + } +} + +// A store failure is a server fault, not a conflict or a validation error: +// it stays untyped so the handler answers 500 without its text (#714). +func TestSetupMFA_StoreFailureIsNotAConflict(t *testing.T) { + mfaRepo := NewMockMFARepository() + mfaRepo.enrolmentErr = errors.New(`ERROR: duplicate key value violates unique constraint "idx_mfa_secrets_user_id"`) + + out, err := NewSetupMFAUseCase(mfaRepo, make([]byte, 32)). + Execute(context.Background(), SetupMFAInput{UserID: uuid.New(), TenantID: uuid.New(), Email: "user@example.com"}) + + assert.Nil(t, out) + assert.ErrorIs(t, err, mfaRepo.enrolmentErr) + var appErr *domain.AppError + assert.False(t, errors.As(err, &appErr), "a store failure must not be a typed error, got %v", err) } func TestSetupMFA_InvalidInput(t *testing.T) { diff --git a/backend/internal/handler/auth/mfa_handler.go b/backend/internal/handler/auth/mfa_handler.go index b01719e6..f56a6530 100644 --- a/backend/internal/handler/auth/mfa_handler.go +++ b/backend/internal/handler/auth/mfa_handler.go @@ -7,6 +7,7 @@ package auth import ( "errors" + "log" "strconv" "time" @@ -394,6 +395,10 @@ func (h *MFAHandler) challengeFailed(c *fiber.Ctx, userID, tenantID uuid.UUID, e } // mapAuthError maps typed domain errors to HTTP status codes. +// +// Anything untyped is a server fault — a store, decryption or hashing failure — +// that the caller can neither cause nor fix. Its text (a driver message, a +// constraint name) is logged, never sent (#714). func mapAuthError(c *fiber.Ctx, err error) error { if appErr, ok := err.(*domain.AppError); ok { switch appErr.Err { @@ -407,5 +412,6 @@ func mapAuthError(c *fiber.Ctx, err error) error { return c.Status(fiber.StatusForbidden).JSON(fiber.Map{"error": appErr.Message}) } } - return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": err.Error()}) + log.Printf("auth: %s failed: %v", c.Path(), err) + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": genericFailure(resolveLocale(c, ""))}) } diff --git a/backend/internal/handler/auth/mfa_setup_http_test.go b/backend/internal/handler/auth/mfa_setup_http_test.go new file mode 100644 index 00000000..6de4ab45 --- /dev/null +++ b/backend/internal/handler/auth/mfa_setup_http_test.go @@ -0,0 +1,104 @@ +// 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 ( + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gofiber/fiber/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" + + appauth "github.com/opendefender/openrisk/internal/application/auth" + "github.com/opendefender/openrisk/internal/domain" + authhandler "github.com/opendefender/openrisk/internal/handler/auth" + "github.com/opendefender/openrisk/internal/infrastructure/repository" +) + +// #714 — POST /auth/mfa/setup through the handler, the use case and the GORM +// repository: started over it answers 200 each time, and a store failure +// answers 500 with no database text. + +const setupPath = "/api/v1/auth/mfa/setup" + +func newSetupApp(t *testing.T) (*deferredFixture, *fiber.App) { + t.Helper() + f := newDeferredFixture(t) + // Production's unique user_id (domain.MFASecret), which the upsert targets. + require.NoError(t, f.db.Exec(`CREATE UNIQUE INDEX idx_mfa_secrets_user_id ON mfa_secrets (user_id)`).Error) + + setupUC := appauth.NewSetupMFAUseCase(repository.NewGormMFARepository(f.db), deferredTOTPKey) + h := authhandler.NewMFAHandler(setupUC, nil, nil, nil, nil, repository.NewGormUserRepository(f.db), nil) + + app := fiber.New() + // What the session middleware leaves behind for an authenticated caller. + app.Post(setupPath, func(c *fiber.Ctx) error { + c.Locals("user_id", f.adminA.UserID.String()) + c.Locals("tenant_id", f.tenantA.String()) + return c.Next() + }, h.Setup) + return f, app +} + +func postSetup(t *testing.T, app *fiber.App) (int, string) { + t.Helper() + req := httptest.NewRequest(http.MethodPost, setupPath, nil) + req.Header.Set("Accept-Language", "en") + resp, err := app.Test(req, -1) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp.StatusCode, string(body) +} + +func TestMFASetupHTTP_StartedOverAnswers200(t *testing.T) { + f, app := newSetupApp(t) + + var secrets []string + for range 3 { + status, body := postSetup(t, app) + require.Equal(t, http.StatusOK, status, body) + var out appauth.SetupMFAOutput + require.NoError(t, json.Unmarshal([]byte(body), &out)) + secrets = append(secrets, out.Secret) + } + assert.NotEqual(t, secrets[0], secrets[2], "each attempt gets a new key") + + var rows, codes int64 + require.NoError(t, f.db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", f.adminA.UserID).Count(&rows).Error) + require.NoError(t, f.db.Model(&domain.MFABackupCode{}).Where("user_id = ? AND tenant_id = ?", f.adminA.UserID, f.tenantA).Count(&codes).Error) + assert.EqualValues(t, 1, rows) + assert.EqualValues(t, 8, codes, "only the latest set of codes") +} + +func TestMFASetupHTTP_StoreFailureIs500WithoutItsText(t *testing.T) { + f, app := newSetupApp(t) + const dbText = `pq: insert on table "mfa_backup_codes" violates constraint` + require.NoError(t, f.db.Callback().Create().Before("gorm:create").Register("test:fail_codes", func(tx *gorm.DB) { + if tx.Statement.Table == "mfa_backup_codes" { + _ = tx.AddError(errors.New(dbText)) + } + })) + + status, body := postSetup(t, app) + + assert.Equal(t, http.StatusInternalServerError, status) + assert.JSONEq(t, `{"error":"Something went wrong. Please try again shortly."}`, body) + for _, leak := range []string{"mfa_", "constraint", "pq:", "failed to"} { + assert.False(t, strings.Contains(body, leak), "the body must not carry %q: %s", leak, body) + } + var rows int64 + require.NoError(t, f.db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", f.adminA.UserID).Count(&rows).Error) + assert.Zero(t, rows, "no secret without its backup codes") +} diff --git a/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go new file mode 100644 index 00000000..af9c91e0 --- /dev/null +++ b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go @@ -0,0 +1,304 @@ +// 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" + "fmt" + "os" + "sync" + "testing" + "time" + + "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" +) + +// #889, #714 — StartMFAEnrolment creates a secret or gives an unfinished +// enrolment a new key, replaces the backup codes, and does both or neither. + +func newCodes(n int) []*domain.MFABackupCode { + codes := make([]*domain.MFABackupCode, n) + for i := range codes { + codes[i] = &domain.MFABackupCode{CodeHash: uuid.NewString()} + } + return codes +} + +func codeHashes(t *testing.T, db *gorm.DB, userID, tenantID uuid.UUID) []string { + t.Helper() + var hashes []string + require.NoError(t, db.Model(&domain.MFABackupCode{}). + Where("user_id = ? AND tenant_id = ?", userID, tenantID). + Order("code_hash").Pluck("code_hash", &hashes).Error) + return hashes +} + +func hashesOf(codes []*domain.MFABackupCode) []string { + out := make([]string, len(codes)) + for i, c := range codes { + out[i] = c.CodeHash + } + return out +} + +func secretOf(t *testing.T, db *gorm.DB, userID uuid.UUID) *domain.MFASecret { + t.Helper() + var s domain.MFASecret + err := db.Where("user_id = ?", userID).First(&s).Error + if errors.Is(err, gorm.ErrRecordNotFound) { + return nil + } + require.NoError(t, err) + return &s +} + +func checkStartEnrolment(t *testing.T, db *gorm.DB) { + t.Helper() + repo := NewGormMFARepository(db) + ctx := context.Background() + user, tenant := uuid.New(), uuid.New() + + // First enrolment creates the row and the codes. + first := newCodes(8) + ok, err := repo.StartMFAEnrolment(ctx, user, tenant, "k1", first) + require.NoError(t, err) + assert.True(t, ok) + s := secretOf(t, db, user) + require.NotNil(t, s) + assert.Equal(t, "k1", s.SecretEncrypted) + assert.Equal(t, tenant, s.TenantID) + assert.False(t, s.IsVerified) + assert.ElementsMatch(t, hashesOf(first), codeHashes(t, db, user, tenant)) + + // The abandoned key had been used: its replay step must not carry over. + step, used := int64(42), time.Now() + require.NoError(t, db.Model(&domain.MFASecret{}).Where("user_id = ? AND tenant_id = ?", user, tenant). + Updates(map[string]any{"last_totp_step": step, "last_used_at": used}).Error) + + // Started over N times: one row, the newest key, only the newest codes. + var last []*domain.MFABackupCode + for i := range 3 { + last = newCodes(8) + ok, err = repo.StartMFAEnrolment(ctx, user, tenant, fmt.Sprintf("k%d", i+2), last) + require.NoError(t, err) + assert.True(t, ok) + } + var rows int64 + require.NoError(t, db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ?", user).Count(&rows).Error) + assert.EqualValues(t, 1, rows) + s = secretOf(t, db, user) + assert.Equal(t, "k4", s.SecretEncrypted) + assert.Equal(t, s.TenantID, tenant) + assert.Nil(t, s.LastTOTPStep, "the abandoned key's replay step must not carry over") + assert.Nil(t, s.LastUsedAt) + assert.ElementsMatch(t, hashesOf(last), codeHashes(t, db, user, tenant), "only the latest set of codes survives") + + // Another tenant cannot reach the row, nor the codes. + ok, err = repo.StartMFAEnrolment(ctx, user, uuid.New(), "intruder", newCodes(8)) + require.NoError(t, err) + assert.False(t, ok, "tenant-scoped: another tenant replaces nothing") + assert.Equal(t, "k4", secretOf(t, db, user).SecretEncrypted) + assert.ElementsMatch(t, hashesOf(last), codeHashes(t, db, user, tenant)) + + // A verified secret is never replaced, and its codes stay. + require.NoError(t, db.Model(&domain.MFASecret{}).Where("user_id = ? AND tenant_id = ?", user, tenant). + Update("is_verified", true).Error) + ok, err = repo.StartMFAEnrolment(ctx, user, tenant, "attack", newCodes(8)) + require.NoError(t, err) + assert.False(t, ok, "a verified secret is never replaced") + s = secretOf(t, db, user) + assert.Equal(t, "k4", s.SecretEncrypted) + assert.True(t, s.IsVerified) + assert.ElementsMatch(t, hashesOf(last), codeHashes(t, db, user, tenant), "a refused enrolment must not touch the codes") +} + +// checkStartEnrolmentReusesATombstone covers accounts whose MFA was turned +// off before #754, when disabling soft-deleted the secret: the tombstone +// keeps the unique user_id, verified or not. +func checkStartEnrolmentReusesATombstone(t *testing.T, db *gorm.DB) { + t.Helper() + repo := NewGormMFARepository(db) + ctx := context.Background() + tenant := uuid.New() + verifiedAt := time.Now() + + for name, verified := range map[string]bool{"unverified": false, "verified": true} { + user := uuid.New() + row := &domain.MFASecret{ID: uuid.New(), UserID: user, TenantID: tenant, SecretEncrypted: "old", IsVerified: verified} + if verified { + row.VerifiedAt = &verifiedAt + } + require.NoError(t, db.Create(row).Error) + require.NoError(t, db.Delete(row).Error) + require.Nil(t, secretOf(t, db, user), "%s: the tombstone is invisible to reads", name) + + codes := newCodes(8) + ok, err := repo.StartMFAEnrolment(ctx, user, tenant, "new", codes) + require.NoError(t, err) + assert.True(t, ok, "%s: a disabled factor must not block a new enrolment", name) + + s := secretOf(t, db, user) + require.NotNil(t, s, "%s: the new secret must be visible, or it can never be verified", name) + assert.Equal(t, "new", s.SecretEncrypted) + assert.False(t, s.IsVerified, "%s: the new secret awaits verification", name) + assert.Nil(t, s.VerifiedAt) + assert.ElementsMatch(t, hashesOf(codes), codeHashes(t, db, user, tenant)) + } + + // Another tenant cannot take over a tombstone either. + user := uuid.New() + row := &domain.MFASecret{ID: uuid.New(), UserID: user, TenantID: tenant, SecretEncrypted: "old"} + require.NoError(t, db.Create(row).Error) + require.NoError(t, db.Delete(row).Error) + ok, err := repo.StartMFAEnrolment(ctx, user, uuid.New(), "intruder", newCodes(8)) + require.NoError(t, err) + assert.False(t, ok) + var keys []string + require.NoError(t, db.Unscoped().Model(&domain.MFASecret{}). + Where("user_id = ? AND deleted_at IS NOT NULL", user).Pluck("secret_encrypted", &keys).Error) + assert.Equal(t, []string{"old"}, keys, "the tombstone is left as it was") +} + +// checkStartEnrolmentRollsBack fails the backup-code insert, the last write of +// the transaction, and expects every earlier write undone. +func checkStartEnrolmentRollsBack(t *testing.T, db *gorm.DB) { + t.Helper() + repo := NewGormMFARepository(db) + ctx := context.Background() + + pending, tenant := uuid.New(), uuid.New() + kept := newCodes(8) + ok, err := repo.StartMFAEnrolment(ctx, pending, tenant, "before", kept) + require.NoError(t, err) + require.True(t, ok) + + boom := errors.New("backup code insert failed") + require.NoError(t, db.Callback().Create().Before("gorm:create").Register("test:fail_codes", func(tx *gorm.DB) { + if tx.Statement.Table == "mfa_backup_codes" { + _ = tx.AddError(boom) + } + })) + t.Cleanup(func() { _ = db.Callback().Create().Remove("test:fail_codes") }) + + // Replacing an unfinished enrolment: the old key and codes survive. + _, err = repo.StartMFAEnrolment(ctx, pending, tenant, "after", newCodes(8)) + require.ErrorIs(t, err, boom) + assert.Equal(t, "before", secretOf(t, db, pending).SecretEncrypted, "the new key must roll back with the codes") + assert.ElementsMatch(t, hashesOf(kept), codeHashes(t, db, pending, tenant), "the code delete must roll back") + + // A first enrolment: no secret is left behind. + fresh := uuid.New() + _, err = repo.StartMFAEnrolment(ctx, fresh, tenant, "orphan", newCodes(8)) + require.ErrorIs(t, err, boom) + assert.Nil(t, secretOf(t, db, fresh), "a secret without its codes must not be stored") +} + +// sqliteEnrolmentRepo adds the unique user_id index production has +// (domain.MFASecret's uniqueIndex): the upsert targets it. +func sqliteEnrolmentRepo(t *testing.T) *gorm.DB { + t.Helper() + _, db := setupMFARepo(t) + require.NoError(t, db.Exec(`CREATE UNIQUE INDEX idx_mfa_secrets_user_id ON mfa_secrets (user_id)`).Error) + return db +} + +func TestGormMFARepository_StartMFAEnrolment(t *testing.T) { + checkStartEnrolment(t, sqliteEnrolmentRepo(t)) +} + +func TestGormMFARepository_StartMFAEnrolment_ReusesATombstone(t *testing.T) { + checkStartEnrolmentReusesATombstone(t, sqliteEnrolmentRepo(t)) +} + +func TestGormMFARepository_StartMFAEnrolment_RollsBackWhenTheCodesFail(t *testing.T) { + checkStartEnrolmentRollsBack(t, sqliteEnrolmentRepo(t)) +} + +func openMFAPostgres(t *testing.T) *gorm.DB { + t.Helper() + 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) + return db +} + +// inTempMFATables runs check against session-private copies of the MFA tables, +// inside a transaction that is always rolled back. +func inTempMFATables(t *testing.T, db *gorm.DB, check func(*testing.T, *gorm.DB)) { + t.Helper() + rollback := errors.New("rollback") + err := db.Transaction(func(tx *gorm.DB) error { + require.NoError(t, tx.Exec(`CREATE TEMP TABLE mfa_secrets (LIKE public.mfa_secrets INCLUDING ALL) ON COMMIT DROP`).Error) + require.NoError(t, tx.Exec(`CREATE TEMP TABLE mfa_backup_codes (LIKE public.mfa_backup_codes INCLUDING ALL) ON COMMIT DROP`).Error) + check(t, tx) + return rollback + }) + require.ErrorIs(t, err, rollback) +} + +func TestGormMFARepository_StartMFAEnrolment_Postgres(t *testing.T) { + inTempMFATables(t, openMFAPostgres(t), checkStartEnrolment) +} + +func TestGormMFARepository_StartMFAEnrolment_ReusesATombstone_Postgres(t *testing.T) { + inTempMFATables(t, openMFAPostgres(t), checkStartEnrolmentReusesATombstone) +} + +func TestGormMFARepository_StartMFAEnrolment_RollsBackWhenTheCodesFail_Postgres(t *testing.T) { + inTempMFATables(t, openMFAPostgres(t), checkStartEnrolmentRollsBack) +} + +// Two setups racing on an account with no secret yet both land, and leave one +// row and one set of codes — before #714 the second hit the unique user_id. +// This needs separate connections, so like the #849 race test it writes to the +// real tables (migrated schema required) and deletes its rows afterwards. +func TestGormMFARepository_StartMFAEnrolment_ConcurrentFirstSetup_Postgres(t *testing.T) { + db := openMFAPostgres(t) + sqlDB, err := db.DB() + require.NoError(t, err) + sqlDB.SetMaxOpenConns(10) + + repo := NewGormMFARepository(db) + ctx := context.Background() + user, tenant := uuid.New(), uuid.New() + t.Cleanup(func() { + // Scoped to the user and tenant this test made up. + db.Unscoped().Where("user_id = ? AND tenant_id = ?", user, tenant).Delete(&domain.MFASecret{}) + db.Where("user_id = ? AND tenant_id = ?", user, tenant).Delete(&domain.MFABackupCode{}) + }) + + const racers = 10 + var wg sync.WaitGroup + start := make(chan struct{}) + for i := range racers { + wg.Add(1) + go func() { + defer wg.Done() + <-start + ok, err := repo.StartMFAEnrolment(ctx, user, tenant, "k"+uuid.NewString(), newCodes(8)) + assert.NoError(t, err, "racer %d", i) + assert.True(t, ok, "racer %d", i) + }() + } + close(start) + wg.Wait() + + var rows int64 + require.NoError(t, db.Unscoped().Model(&domain.MFASecret{}).Where("user_id = ? AND tenant_id = ?", user, tenant).Count(&rows).Error) + assert.EqualValues(t, 1, rows) + assert.Len(t, codeHashes(t, db, user, tenant), 8, "one set of codes, not a mix of several") +} diff --git a/backend/internal/infrastructure/repository/gorm_mfa_reenrol_test.go b/backend/internal/infrastructure/repository/gorm_mfa_reenrol_test.go deleted file mode 100644 index fa6e0373..00000000 --- a/backend/internal/infrastructure/repository/gorm_mfa_reenrol_test.go +++ /dev/null @@ -1,88 +0,0 @@ -// 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" - "time" - - "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" -) - -// #889 — an unfinished enrolment can be started over: the unverified secret's -// key is replaced in place, and a verified one is never touched. - -func checkReplaceUnverified(t *testing.T, db *gorm.DB) { - t.Helper() - repo := NewGormMFARepository(db) - ctx := context.Background() - step := int64(42) - used := time.Now() - - pending, tenant := uuid.New(), uuid.New() - require.NoError(t, db.Create(&domain.MFASecret{ - ID: uuid.New(), UserID: pending, TenantID: tenant, SecretEncrypted: "old", - LastTOTPStep: &step, LastUsedAt: &used, - }).Error) - enrolled := uuid.New() - require.NoError(t, db.Create(&domain.MFASecret{ - ID: uuid.New(), UserID: enrolled, TenantID: tenant, SecretEncrypted: "kept", IsVerified: true, - }).Error) - - // Another tenant's request cannot reach the row. - ok, err := repo.ReplaceUnverifiedMFASecret(ctx, pending, uuid.New(), "intruder") - require.NoError(t, err) - assert.False(t, ok, "tenant-scoped: another tenant replaces nothing") - - ok, err = repo.ReplaceUnverifiedMFASecret(ctx, pending, tenant, "new") - require.NoError(t, err) - assert.True(t, ok) - var got domain.MFASecret - require.NoError(t, db.Where("user_id = ?", pending).First(&got).Error) - assert.Equal(t, "new", got.SecretEncrypted) - assert.False(t, got.IsVerified) - assert.Nil(t, got.LastTOTPStep, "the abandoned key's replay step must not carry over") - assert.Nil(t, got.LastUsedAt) - - ok, err = repo.ReplaceUnverifiedMFASecret(ctx, enrolled, tenant, "attack") - require.NoError(t, err) - assert.False(t, ok, "a verified secret is never replaced") - var kept domain.MFASecret - require.NoError(t, db.Where("user_id = ?", enrolled).First(&kept).Error) - assert.Equal(t, "kept", kept.SecretEncrypted) -} - -func TestGormMFARepository_ReplaceUnverifiedMFASecret(t *testing.T) { - _, db := setupMFARepo(t) - checkReplaceUnverified(t, db) -} - -func TestGormMFARepository_ReplaceUnverifiedMFASecret_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 { - require.NoError(t, tx.Exec(`CREATE TEMP TABLE mfa_secrets (LIKE public.mfa_secrets INCLUDING ALL) ON COMMIT DROP`).Error) - checkReplaceUnverified(t, tx) - return rollback - }) - require.ErrorIs(t, err, rollback) -} diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository.go b/backend/internal/infrastructure/repository/gorm_mfa_repository.go index 2977499b..25c40577 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_repository.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository.go @@ -13,6 +13,7 @@ import ( "github.com/google/uuid" "github.com/opendefender/openrisk/internal/domain" "gorm.io/gorm" + "gorm.io/gorm/clause" ) // GormMFARepository implements MFARepository using GORM @@ -71,27 +72,90 @@ func (r *GormMFARepository) ConsumeTOTPStep(ctx context.Context, userID, tenantI return res.RowsAffected == 1, nil } -// ReplaceUnverifiedMFASecret gives an unfinished enrolment a new key (#889). +// StartMFAEnrolment stores a new unverified secret and replaces every backup +// code of the user, in one transaction (#714). // -// One conditional UPDATE, not a read and a Save: the is_verified = false guard -// makes it lose cleanly against a verification that lands at the same moment, -// and last_totp_step is reset here explicitly rather than by a Save, which -// ConsumeTOTPStep's contract forbids (#849). The step and last use belonged to -// the abandoned key, so they start over with the new one. -func (r *GormMFARepository) ReplaceUnverifiedMFASecret(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string) (bool, error) { - res := r.db.WithContext(ctx).Model(&domain.MFASecret{}). - Where("user_id = ? AND tenant_id = ? AND is_verified = ?", userID, tenantID, false). - Updates(map[string]any{ - "secret_encrypted": secretEncrypted, - "last_totp_step": nil, - "last_used_at": nil, - }) - if res.Error != nil { - return false, res.Error +// The secret is written by one upsert, not a read and then a write: a first +// enrolment creates the row, and an unfinished one has its key replaced +// (#889). Two setups racing on an account with no row therefore both land, +// the later key winning, instead of the second hitting the unique user_id. +// The update only applies to an unverified row of the same tenant; a +// verified secret makes it affect nothing, and the call reports false and +// rolls back, leaving that secret and its codes untouched. +// +// A soft-deleted row counts as no secret, as it does for every read. Before +// #754, turning MFA off soft-deleted the secret, and that tombstone still +// holds the unique user_id: it is brought back as a fresh, unverified +// enrolment rather than updated out of sight or refused as "already enabled". +// +// last_totp_step and last_used_at are reset here explicitly rather than by a +// Save, which ConsumeTOTPStep's contract forbids (#849): they belonged to the +// abandoned key. +// +// The codes arrive already hashed. Hashing is slow, and must not happen while +// the row is locked. +func (r *GormMFARepository) StartMFAEnrolment(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string, codes []*domain.MFABackupCode) (bool, error) { + err := r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + secret := &domain.MFASecret{ + ID: uuid.New(), + UserID: userID, + TenantID: tenantID, + SecretEncrypted: secretEncrypted, + } + res := tx.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "user_id"}}, + DoUpdates: clause.Assignments(map[string]any{ + "secret_encrypted": secretEncrypted, + "is_verified": false, + "verified_at": nil, + "last_totp_step": nil, + "last_used_at": nil, + "deleted_at": nil, + "updated_at": time.Now(), + }), + Where: clause.Where{Exprs: []clause.Expression{ + clause.Expr{ + SQL: "(mfa_secrets.is_verified = ? OR mfa_secrets.deleted_at IS NOT NULL) AND mfa_secrets.tenant_id = ?", + Vars: []any{false, tenantID}, + }, + }}, + }).Create(secret) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return errEnrolmentRefused + } + if err := tx.Where("user_id = ? AND tenant_id = ?", userID, tenantID). + Delete(&domain.MFABackupCode{}).Error; err != nil { + return err + } + for _, c := range codes { + c.UserID, c.TenantID = userID, tenantID + if c.ID == uuid.Nil { + c.ID = uuid.New() + } + } + if len(codes) > 0 { + if err := tx.CreateInBatches(codes, 100).Error; err != nil { + return err + } + } + return nil + }) + if errors.Is(err, errEnrolmentRefused) { + return false, nil } - return res.RowsAffected == 1, nil + if err != nil { + return false, err + } + return true, nil } +// errEnrolmentRefused rolls StartMFAEnrolment back when a verified secret is +// already in place. It never leaves the repository. +var errEnrolmentRefused = errors.New("mfa enrolment refused: secret already verified") + // DisableMFA removes the TOTP secret and every backup code of one user, in one // transaction (#754). // diff --git a/backend/internal/infrastructure/repository/mfa_repository_interface.go b/backend/internal/infrastructure/repository/mfa_repository_interface.go index 7a46344f..29258db4 100644 --- a/backend/internal/infrastructure/repository/mfa_repository_interface.go +++ b/backend/internal/infrastructure/repository/mfa_repository_interface.go @@ -21,10 +21,10 @@ type MFARepository interface { // ConsumeTOTPStep marks a TOTP step as used; false means it (or a later one) // already was, i.e. a replay (#849). ConsumeTOTPStep(ctx context.Context, userID, tenantID uuid.UUID, step int64) (bool, error) - // ReplaceUnverifiedMFASecret swaps the key material of a secret that was - // never verified, for an enrolment started over (#889). false means there - // is no unverified secret to replace, e.g. it was verified meanwhile. - ReplaceUnverifiedMFASecret(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string) (bool, error) + // StartMFAEnrolment stores a new unverified secret (created, or replacing + // an unverified one) and replaces the backup codes, in one transaction + // (#714). false means a verified secret exists; nothing was written. + StartMFAEnrolment(ctx context.Context, userID, tenantID uuid.UUID, secretEncrypted string, codes []*domain.MFABackupCode) (bool, error) // DisableMFA deletes the secret AND the backup codes, atomically. DisableMFA(ctx context.Context, userID, tenantID uuid.UUID) error diff --git a/docs/714_MFA_SETUP_ATOMIC.md b/docs/714_MFA_SETUP_ATOMIC.md new file mode 100644 index 00000000..edfc1955 --- /dev/null +++ b/docs/714_MFA_SETUP_ATOMIC.md @@ -0,0 +1,167 @@ +# Spec — #714 MFA setup: one transaction, typed errors + +Status: approved by owner 2026-10-07 (both proposals below accepted) · done 2026-10-07 · Branch: `fix/714-mfa-setup-atomic` · Milestone: trust-v1 + +## Objective + +Starting MFA enrolment must never leave an account half-enrolled, and it must never +show a database message to the user. + +#889 (merged in PR #892, 2026-10-02) already fixed the lockout in #714's title: +an unverified secret is now replaced in place, so setup can be retried. Three of +#714's criteria are still open on `origin/master` (cefe453d): + +- The secret write, `DeleteBackupCodes` and `SaveBackupCodes` are three separate + statements. If the third fails, the user keeps a new secret and has **no + backup codes**. ABSOLUTE RULE 7. +- Every untyped error falls through `mapAuthError` to + `400 {"error": err.Error()}`, which sends the raw storage text to the client. + ABSOLUTE RULE 3. +- Two concurrent *first* setups (no row yet) both take the `CreateMFASecret` + branch. The second one still hits `idx_mfa_secrets_user_id` and answers 400 + "duplicated key". This is the read-then-write race that #889 left. + +The frontend comments still say setup "answers 400". That is no longer true. The +guard itself is still needed, for a new reason: each call now **replaces** the key, +so a second call would invalidate the QR code already on screen. + +## Acceptance criteria, mapped to #714 + +| #714 AC | State on master | Work here | +|---|---|---| +| 1 Unverified → 200 with a new secret | ✅ #889 (`TestSetupMFA_ReplacesAnUnverifiedSecret`) | keep it passing | +| 2 Verified → `ErrConflict already_enabled` | ✅ #889 (`TestSetupMFA_VerifiedSecretIsKept`) | keep it passing | +| 3 N calls → one row, one set of unused codes | ⚠️ one row yes; one code set only if nothing fails between the delete and the insert | covered by the transaction | +| 4 Code-storage failure → no new secret persisted | ❌ | **transaction** | +| 5 Storage failure → typed error, no DB text | ❌ | **error mapping** | +| 6 Every query scoped by `tenant_id` | ✅ | keep it, and test it | +| DoD: `TestSetupMFA_{Success,NotFound,Unauthorized,ReplacesUnverifiedSecret,VerifiedConflict}` | `Success` only. #889 named the other two `…AnUnverifiedSecret` / `…VerifiedSecretIsKept` | add `NotFound` and `Unauthorized`; keep #889's names, see Open questions | +| DoD: frontend guard comments | ❌ stale | rewrite both comments | + +## Design + +### 1. One repository call, one transaction (AC 3, 4) + +New method on `MFARepository`: + +```go +// StartMFAEnrolment stores a new unverified secret and replaces every backup +// code of the user, in one transaction (#714). It creates the row, or replaces +// the key of an unverified one; false means a verified secret exists, which it +// leaves untouched. +StartMFAEnrolment(ctx context.Context, userID, tenantID uuid.UUID, + secretEncrypted string, codes []*domain.MFABackupCode) (bool, error) +``` + +Inside `db.Transaction`: + +1. **Upsert the secret.** It is one statement, so it has no read-then-write race: + `INSERT … ON CONFLICT (user_id) DO UPDATE SET secret_encrypted = excluded.secret_encrypted, last_totp_step = NULL, last_used_at = NULL, updated_at = … WHERE mfa_secrets.is_verified = false AND mfa_secrets.tenant_id = excluded.tenant_id` + (GORM `clause.OnConflict{Columns, DoUpdates, Where}`; works on Postgres and SQLite). + 0 rows affected means a verified secret, or a row of another tenant. In both cases + it returns `false` and rolls back. #889's reset of `last_totp_step` and + `last_used_at` is kept, as #849 requires. +2. `DELETE FROM mfa_backup_codes WHERE user_id = ? AND tenant_id = ?` +3. `CreateInBatches(codes)` + +The use case still bcrypt-hashes the 8 codes **before** it calls the repository. +About 8 × 60 ms of hashing must not hold a row lock. + +`SetupMFAUseCase.Execute` becomes: validate → `GetMFASecret` (keeps the fast 409 +for a verified secret) → generate the secret, QR code and codes → hash → +`StartMFAEnrolment` → `false` ⇒ `NewConflictError("MFA", "already_enabled")`. + +`ReplaceUnverifiedMFASecret` loses its only caller. It is removed together with its +mock and its tests, and the new method's repository tests take over its cases. +`CreateMFASecret`, `DeleteBackupCodes` and `SaveBackupCodes` stay. They have other +callers or tests, and removing them is outside this issue. + +### 2. No database text in a response (AC 5) + +`mapAuthError` (`backend/internal/handler/auth/mfa_handler.go:397`) currently ends +with `400 {"error": err.Error()}`. It becomes +`500 {"error": genericFailure(locale)}`, the same answer `challengeFailed` already +gives for a store failure. The handler logs the underlying error at `error` level +with zerolog. No secret is in it: it is a GORM or driver message. + +This also changes `Verify`. Its untyped errors are a lookup failure, a decryption +failure, a `ConsumeTOTPStep` failure and an update failure. Today they answer 400 +with internal text; afterwards they answer 500 with generic text. Every error a +user can cause is already typed (`ErrValidation`, `ErrNotFound`, `ErrConflict`) +and is unchanged. + +The setup use case also wraps its own internal failures without the storage text +(`fmt.Errorf("store MFA enrolment: %w", err)` stays internal; it is never +rendered). + +### 3. Frontend comments (DoD) + +`frontend/src/features/auth/MFAEnrollmentDialog.tsx:42` and +`frontend/src/features/auth/AuthScreen.tsx:530`: the guards stay and the comments +are rewritten. Each call issues a new key and invalidates the previous one, so a +second call caused by React's double effect would leave a QR code on screen that +can no longer verify. There is no code change, so no behaviour changes and no new +strings are needed. + +## Tests + +Use case (`backend/internal/application/auth/`, mock repository): + +- `TestSetupMFA_Success`: already exists. Additionally asserts that exactly one `StartMFAEnrolment` call carries 8 hashed codes, each scoped to the user and tenant. +- `TestSetupMFA_NotFound`: the user and tenant have no secret, so the setup **creates** one. This is the use case's not-found path; the account itself is not looked up here. +- `TestSetupMFA_Unauthorized`: a nil user or tenant → `ErrValidation`, and the repository is never touched. This is the same convention as `TestMFAChallenge_Unauthorized`. +- `TestSetupMFA_ReplacesAnUnverifiedSecret` and `TestSetupMFA_VerifiedSecretIsKept`: they already exist and must stay green. +- `TestSetupMFA_StoreFailureIsNotAConflict`: the repository returns an error, so the use case returns an untyped error, not an `AppError`. + +Repository (`backend/internal/infrastructure/repository/`, SQLite plus a `_Postgres` twin gated on `DATABASE_URL`): + +- creates on first call; replaces an unverified key and resets `last_totp_step`/`last_used_at`; refuses a verified one and leaves the row and codes untouched; +- after N calls: 1 row in `mfa_secrets` and exactly the last 8 codes; +- **rollback**: when the code insert fails (forced by a `BEFORE INSERT` trigger on `mfa_backup_codes`, as `DisableMFA_RollsBackWhenTheSecondWriteFails` does), the secret is unchanged (unverified case) or absent (first-call case), and the old codes are still there; +- tenant scope: a row in tenant A is not replaced by a call for tenant B, and B's codes delete leaves A's codes alone; +- Postgres: two concurrent first setups for one user both return `true` and leave one row. No duplicate-key error. + +Handler (`backend/internal/handler/auth/`): + +- `Setup` with a repository failure → 500, and the body does not contain the store's error text. + +Live check (per [[always-verify-live]]): local stack with real Postgres: +register → login → setup → setup → verify with the **second** QR code's code → 200; +the first QR code's code → refused. Then force a failure (trigger) → the response +is 500 with the generic text, and `mfa_secrets` is unchanged. + +## Commands + +``` +cd backend && go build ./... && go vet ./internal/application/auth/ ./internal/infrastructure/repository/ ./internal/handler/auth/ +cd backend && go test -race ./internal/application/auth/ ./internal/infrastructure/repository/ ./internal/handler/auth/ +cd backend && DATABASE_URL=postgres://…spare-port… go test -race -run 'MFA' ./internal/infrastructure/repository/ +cd backend && go test ./... # full suite before the PR (route/authz ratchets) +cd frontend && npx tsc -b --noEmit # comments only, but cheap +``` + +## Boundaries + +- **Always:** keep `tenant_id` in every WHERE; one commit per step; format only touched files; paste test output into the issue comment. +- **Ask first:** any schema or index change (none planned); changing what the frontend does on a setup error. +- **Never:** change the TOTP, encryption or backup-code scheme; touch the MFA policy or grace period; merge the PR. + +## Out of scope, noted + +- `idx_mfa_secrets_user_id` is unique on `user_id` alone, but every read is per `(user_id, tenant_id)`. A user who belongs to two tenants and has a secret in tenant A can never enrol in tenant B. Before this change that failed with 400 "duplicated key"; it now fails with 409 `already_enabled`, which is clearer but still a refusal. Whether MFA belongs to the person or to each membership is tenant-isolation **design**: escalated as D-066 in `docs/DECISIONS.md`, tracked in #897. + +## Decisions taken at review + +1. Test names: #889's `TestSetupMFA_ReplacesAnUnverifiedSecret` and `TestSetupMFA_VerifiedSecretIsKept` are kept. They are the tests #714's DoD calls `…ReplacesUnverifiedSecret` and `…VerifiedConflict`. +2. The `mapAuthError` fallback becomes 500 with a generic message, for `Verify` too. + +## Tasks + +- [x] `StartMFAEnrolment`: upsert, code delete and code insert in one transaction; `ReplaceUnverifiedMFASecret` removed — `gorm_mfa_repository.go`, `mfa_repository_interface.go` +- [x] Setup use case hashes first, then makes one repository call — `mfa_usecase.go` +- [x] Repository tests on SQLite and Postgres: create, replace, N calls, verified refusal, tenant scope, rollback, concurrent first setup — `gorm_mfa_enrolment_test.go` +- [x] Use case tests: `Success` (strengthened), `NotFound`, `Unauthorized`, `StoreFailureIsNotAConflict`; mock updated — `mfa_usecase_test.go`, `mfa_setup_reenrol_test.go` +- [x] `mapAuthError`: untyped error → 500, generic FR/EN text, logged — `mfa_handler.go`, `mfa_setup_http_test.go` +- [x] Frontend guard comments rewritten — `AuthScreen.tsx`, `MFAEnrollmentDialog.tsx` +- [x] Found while testing: a secret soft-deleted before #754 (a tombstone) still holds the unique `user_id`. The upsert now treats it as no secret and brings it back as a fresh, unverified enrolment, still tenant-scoped — `gorm_mfa_repository.go`, `gorm_mfa_enrolment_test.go` (`…ReusesATombstone`, SQLite and Postgres). On master such an account got 400 forever. +- [x] Live check on a throwaway Postgres: setup ×3 → one row, 8 codes; first QR code refused, last accepted; verified → 409; forced code-insert failure → 500 generic, secret and codes unchanged; verified tombstone → setup 200, verify 200 diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 2f7c7a31..27e117d2 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -5,6 +5,39 @@ recommends, and surfaces these in the daily brief. Run `/decide` to clear them. ## Open +### D-066 — MFA: one authenticator per person, or one per organisation? · raised 2026-10-07 + +**Raised by** — #714, tracked in #897. This is tenant-isolation design, so it is escalated rather +than fixed in place. + +**Context** +- `mfa_secrets.user_id` is unique on its own, but every read and write is scoped by + `(user_id, tenant_id)`: sign-in, the MFA gate, the status resolver and setup. +- So a person enrolled in organisation A has, seen from organisation B, no factor. B's + setup can still never store one, because the `user_id` row already exists under A. If + their role in B requires MFA, they are locked out of B when the grace period ends. Since #714 setup answers + `409 already_enabled` there instead of a `400` with database text, but still refuses. +- This comes from reading the code; it has not been reproduced live. + +**Options** +- **A — per person.** One secret per user, valid in every organisation. Reads drop + `tenant_id` on `mfa_secrets` (gated by the user's own identity instead). The schema is + simplest, but an organisation's admin then relies on a factor that another + organisation's policy governs. +- **B — per membership.** The unique index becomes `(user_id, tenant_id)`. Each + organisation enrols its own factor; the queries stay as they are. Cost: one migration, + and a person carries one authenticator entry per organisation. +- **C — keep as is.** Multi-organisation members whose role requires MFA stay stuck. + +**Recommendation** — **B.** It matches every query already written and keeps each +tenant's security posture its own. The migration relaxes one unique index, and +`StartMFAEnrolment`'s upsert target moves from `(user_id)` to `(user_id, tenant_id)`. + +**Cost of delay** — Low today (multi-organisation accounts are rare), high for any MSP-style +customer. + +**Blocks** — #897. + ### D-064 — Sign-in lock: anyone can keep an address locked · raised 2026-10-02 **Raised by** — #688 (PR #874), tracked in #879. Since #688, 10 failed sign-ins on one diff --git a/frontend/src/features/auth/AuthScreen.tsx b/frontend/src/features/auth/AuthScreen.tsx index 276ec680..4964cc45 100644 --- a/frontend/src/features/auth/AuthScreen.tsx +++ b/frontend/src/features/auth/AuthScreen.tsx @@ -527,11 +527,10 @@ function MFAEnrollment({ const [busy, setBusy] = useState(false); const [error, setError] = useState(''); const [nonce, setNonce] = useState(0); - // POST /auth/mfa/setup is NOT idempotent: a second call for the same account - // answers 400 "duplicated key not allowed". React runs effects twice in - // development, so the second call failed and painted "la création du compte a - // échoué" over a screen that had already loaded its QR code perfectly — the - // account existed, nothing had failed, and the user was told to start over. + // Each POST /auth/mfa/setup issues a new key and voids the previous one + // (#714). React runs effects twice in development, and the two responses can + // land in either order, so a second call could leave on screen a QR code the + // server no longer accepts: the user scans it and every code is refused. // One call per token, tracked outside render so a re-render cannot repeat it. const requested = useRef(null); diff --git a/frontend/src/features/auth/MFAEnrollmentDialog.tsx b/frontend/src/features/auth/MFAEnrollmentDialog.tsx index edc6d661..ea99646f 100644 --- a/frontend/src/features/auth/MFAEnrollmentDialog.tsx +++ b/frontend/src/features/auth/MFAEnrollmentDialog.tsx @@ -39,10 +39,10 @@ export function MFAEnrollmentDialog({ const [error, setError] = useState(''); const [copied, setCopied] = useState(false); const dialogRef = useRef(null); - // POST /auth/mfa/setup is not idempotent — a second call for the same account - // answers 400. React runs effects twice in development, so without this guard - // a screen that had already loaded its QR code perfectly would paint an error - // over itself. Tracked outside render so a re-render cannot repeat it. + // Each POST /auth/mfa/setup issues a new key and voids the previous one + // (#714). React runs effects twice in development, so without this guard a + // second call could leave on screen a QR code the server no longer accepts. + // Tracked outside render so a re-render cannot repeat it. const requested = useRef(false); useEffect(() => {