fix(auth): store the MFA enrolment in one transaction and never return database text (#714)

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,
 	})
 

backend/internal/application/auth/mfa_usecase.go

@@ -83,61 +83,41 @@ 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,
 			CodeHash: string(hash),
 		})
 	}
 
-	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{

backend/internal/application/auth/mfa_usecase_test.go

@@ -7,19 +7,24 @@ 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
 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) {

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, ""))})
 }

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")
+}