From 1b5be5d694a34705d26d6d7c6c1f087fc77ffb14 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:04:40 +0100 Subject: [PATCH 1/7] docs(auth): spec for an atomic MFA setup (#714) #889 already lets an unfinished enrolment start over. What #714 still asks for is the transaction across the secret and the backup codes, and a response that never carries database text. The spec records what master already covers and what this branch adds. Signed-off-by: alex-dembele --- ...rol_test.go => gorm_mfa_enrolment_test.go} | 0 docs/714_MFA_SETUP_ATOMIC.md | 166 ++++++++++++++++++ 2 files changed, 166 insertions(+) rename backend/internal/infrastructure/repository/{gorm_mfa_reenrol_test.go => gorm_mfa_enrolment_test.go} (100%) create mode 100644 docs/714_MFA_SETUP_ATOMIC.md diff --git a/backend/internal/infrastructure/repository/gorm_mfa_reenrol_test.go b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go similarity index 100% rename from backend/internal/infrastructure/repository/gorm_mfa_reenrol_test.go rename to backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go diff --git a/docs/714_MFA_SETUP_ATOMIC.md b/docs/714_MFA_SETUP_ATOMIC.md new file mode 100644 index 00000000..0dcc7726 --- /dev/null +++ b/docs/714_MFA_SETUP_ATOMIC.md @@ -0,0 +1,166 @@ +# 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] 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 From 38f6a9b44ffc8909ab82f55e6f59cb12a5c1cbf5 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:04:40 +0100 Subject: [PATCH 2/7] fix(auth): store the MFA secret and its backup codes in one transaction (#714) Setup wrote the secret, deleted the old backup codes and inserted the new ones as three statements. If the insert failed, the user kept a new secret and no backup codes at all. StartMFAEnrolment now does all three in one transaction. The secret is written by an upsert on user_id that only applies to an unverified row of the same tenant, so it also closes the race #889 left: two first setups at the same moment both land instead of the second hitting the unique index. A verified secret affects no row, the call rolls back and setup answers 409 as before. The codes are hashed before the transaction opens, so bcrypt never runs under the row lock. ReplaceUnverifiedMFASecret had no other caller and is gone; its cases move to the new repository tests, which run on SQLite and, with DATABASE_URL, on Postgres, including a concurrent first setup. Signed-off-by: alex-dembele --- .../auth/mfa_setup_reenrol_test.go | 19 +- .../internal/application/auth/mfa_usecase.go | 54 ++-- .../application/auth/mfa_usecase_test.go | 93 ++++++- .../repository/gorm_mfa_enrolment_test.go | 233 +++++++++++++++--- .../repository/gorm_mfa_repository.go | 87 +++++-- .../repository/mfa_repository_interface.go | 8 +- 6 files changed, 377 insertions(+), 117 deletions(-) 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/infrastructure/repository/gorm_mfa_enrolment_test.go b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go index fa6e0373..a08fa91f 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go @@ -8,7 +8,9 @@ package repository import ( "context" "errors" + "fmt" "os" + "sync" "testing" "time" @@ -22,67 +24,226 @@ import ( "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. +// #889, #714 — StartMFAEnrolment creates a secret or gives an unfinished +// enrolment a new key, replaces the backup codes, and does both or neither. -func checkReplaceUnverified(t *testing.T, db *gorm.DB) { +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() - step := int64(42) - used := time.Now() + user, tenant := uuid.New(), uuid.New() - 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") + // 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.False(t, ok, "tenant-scoped: another tenant replaces nothing") + 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) - ok, err = repo.ReplaceUnverifiedMFASecret(ctx, pending, tenant, "new") + // 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.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") + 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") - var kept domain.MFASecret - require.NoError(t, db.Where("user_id = ?", enrolled).First(&kept).Error) - assert.Equal(t, "kept", kept.SecretEncrypted) + 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") +} + +// 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") } -func TestGormMFARepository_ReplaceUnverifiedMFASecret(t *testing.T) { +// 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) - checkReplaceUnverified(t, db) + require.NoError(t, db.Exec(`CREATE UNIQUE INDEX idx_mfa_secrets_user_id ON mfa_secrets (user_id)`).Error) + return db } -func TestGormMFARepository_ReplaceUnverifiedMFASecret_Postgres(t *testing.T) { +func TestGormMFARepository_StartMFAEnrolment(t *testing.T) { + checkStartEnrolment(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 { + 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) + 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_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_repository.go b/backend/internal/infrastructure/repository/gorm_mfa_repository.go index 2977499b..b52f7fdc 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,79 @@ 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. +// +// 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, + "last_totp_step": nil, + "last_used_at": nil, + "updated_at": time.Now(), + }), + Where: clause.Where{Exprs: []clause.Expression{ + clause.Expr{SQL: "mfa_secrets.is_verified = ? 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 From ea334aefaae5bc22113694be70b16f8abf510a6f Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:04:40 +0100 Subject: [PATCH 3/7] fix(auth): answer 500 without database text when an MFA call fails (#714) mapAuthError sent any untyped error back as 400 with err.Error(), so a store failure showed the user a driver message and a constraint name. Untyped errors are server faults the caller cannot fix: they now answer 500 with the generic FR/EN message, and the real error goes to the log. This covers setup and verify. Errors a user can cause are typed and answer as before. Signed-off-by: alex-dembele --- backend/internal/handler/auth/mfa_handler.go | 8 +- .../handler/auth/mfa_setup_http_test.go | 104 ++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 backend/internal/handler/auth/mfa_setup_http_test.go 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") +} From 1e289d6026dcef3ef2b1d048d0be9cea30ec3312 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:04:40 +0100 Subject: [PATCH 4/7] docs(auth): say why MFA setup still runs once per screen (#714) Both guards said a second setup call answers 400. It no longer does: each call issues a new key and voids the previous one. The guards stay, because a second call could now leave a QR code on screen that the server no longer accepts. Signed-off-by: alex-dembele --- frontend/src/features/auth/AuthScreen.tsx | 9 ++++----- frontend/src/features/auth/MFAEnrollmentDialog.tsx | 8 ++++---- 2 files changed, 8 insertions(+), 9 deletions(-) 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(() => { From ca4c77f0145a056ff5633c1ec6de5c4cac57eb5c Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:04:40 +0100 Subject: [PATCH 5/7] docs(decisions): raise D-066, MFA per person or per organisation (#714) mfa_secrets.user_id is unique on its own while every query is scoped by tenant, so a member of two organisations can enrol in only one. Choosing the model is tenant-isolation design, so it goes to the owner. Tracked in #897. Signed-off-by: alex-dembele --- docs/DECISIONS.md | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) 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 From ef67b6586f0c4e67e23634eb92c3a7ae4e5763c1 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:06:30 +0100 Subject: [PATCH 6/7] fix(auth): let an account whose MFA was turned off before #754 enrol again (#714) Before #754, turning MFA off soft-deleted the secret. Such a tombstone is invisible to every read but still holds the unique user_id, so the setup upsert landed on it: an unverified tombstone got a new key nobody could see, and setup answered 200 with a QR code that could never verify; a verified one answered 409 to a user with MFA off. A soft-deleted row is now replaced like an unverified one and brought back as a fresh enrolment, still only within its own tenant. Signed-off-by: alex-dembele --- .../repository/gorm_mfa_enrolment_test.go | 55 +++++++++++++++++++ .../repository/gorm_mfa_repository.go | 13 ++++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go index a08fa91f..af9c91e0 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_enrolment_test.go @@ -123,6 +123,53 @@ func checkStartEnrolment(t *testing.T, db *gorm.DB) { 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) { @@ -170,6 +217,10 @@ 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)) } @@ -203,6 +254,10 @@ 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) } diff --git a/backend/internal/infrastructure/repository/gorm_mfa_repository.go b/backend/internal/infrastructure/repository/gorm_mfa_repository.go index b52f7fdc..25c40577 100644 --- a/backend/internal/infrastructure/repository/gorm_mfa_repository.go +++ b/backend/internal/infrastructure/repository/gorm_mfa_repository.go @@ -83,6 +83,11 @@ func (r *GormMFARepository) ConsumeTOTPStep(ctx context.Context, userID, tenantI // 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. @@ -101,12 +106,18 @@ func (r *GormMFARepository) StartMFAEnrolment(ctx context.Context, userID, tenan 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 = ? AND mfa_secrets.tenant_id = ?", Vars: []any{false, tenantID}}, + 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 { From 3fe842b68d0b444cb2af9552fd931aab81aac88e Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Wed, 7 Oct 2026 10:07:30 +0100 Subject: [PATCH 7/7] docs(auth): record the tombstone case in the #714 spec (#714) Signed-off-by: alex-dembele --- docs/714_MFA_SETUP_ATOMIC.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/714_MFA_SETUP_ATOMIC.md b/docs/714_MFA_SETUP_ATOMIC.md index 0dcc7726..edfc1955 100644 --- a/docs/714_MFA_SETUP_ATOMIC.md +++ b/docs/714_MFA_SETUP_ATOMIC.md @@ -163,4 +163,5 @@ cd frontend && npx tsc -b --noEmit # comments only, but cheap - [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] 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 +- [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