diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index a04853e5..ac7714e1 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -687,7 +687,9 @@ func main() { // Organization + owner + root membership in one transaction (#687). WithAccounts(repository.NewGormRegistrationRepository(database.DB)) refreshUseCase := auth.NewRefreshTokenUseCase(tokenManager) - logoutUseCase := auth.NewLogoutUseCase(tokenManager) + // #689 — logout also blacklists the access token it was called with, so the + // session ends now instead of at the end of the access TTL. + logoutUseCase := auth.NewLogoutUseCase(tokenManager).WithAccessTokenRevocation(tokenBlacklistManager) // MFA use cases + handler. setupMFAUseCase := auth.NewSetupMFAUseCase(mfaRepo, mfaKey[:]) @@ -699,7 +701,12 @@ func main() { WithTOTPKey(mfaKey[:]). RequireMFAForRoles(mfaRequiredRoles, mfaRequiredBusinessRoles). WithMailer(securityMailer) - challengeMFAUseCase := auth.NewChallengeMFAUseCase(mfaRepo, mfaKey[:]) + // #689 — five codes per challenge token, ten failures per account in + // fifteen minutes before a fixed fifteen-minute lock, counted in Redis so + // the budget holds across instances. The owner is mailed when a lock is set. + challengeMFAUseCase := auth.NewChallengeMFAUseCase(mfaRepo, mfaKey[:]). + WithAttemptLimits(authmfa.NewAttemptStore(redisClientInstance), tokenBlacklistManager). + WithLockNotice(userRepo, securityMailer) // OR26-03 — one resolver answers "must this member enrol now?" for /auth/me // and for the request-time guard, so the banner and the enforcement can never // disagree. Cached per (user, tenant) for a minute; enrolment, disabling and @@ -759,7 +766,8 @@ func main() { // The same repository resolveSessionForOrg reads at token mint time, so // /auth/me's business role and the token's permissions come from one // membership row and cannot disagree (#338). - WithMemberLookup(userRepo) + WithMemberLookup(userRepo). + WithAccessTokenRevocation(rsaKeys) // OAuth identity resolution: known link → verified-email link → provision. // No provisioner is wired, so an identity with no OpenRisk account is refused @@ -895,7 +903,19 @@ func main() { // on `api` BEFORE the Protected group: MFATokenMiddleware validates the special // token itself and rejects full/absent tokens. On a valid code the handler // mints the real access+refresh pair. - api.Post("/auth/mfa/challenge", middleware.MFATokenMiddleware(rsaKeys, jtiBlacklistChecker), mfaHandler.Challenge) + // + // #689 — the per-IP limit runs first, in its own bucket: a flood is refused + // before any token is parsed, and it never spends the login budget. The + // per-token and per-account limits live in the use case. + mfaChallengeRateLimit := middleware.RateLimit(middleware.RateLimitConfig{ + MaxRequests: 20, + WindowSize: 5 * time.Minute, + Store: handlers.PrefixedRateLimitBackend{ + Prefix: "mfa-challenge-ip:", + Inner: authLimiterStore, + }, + }) + api.Post("/auth/mfa/challenge", mfaChallengeRateLimit, middleware.MFATokenMiddleware(rsaKeys, jtiBlacklistChecker), mfaHandler.Challenge) // Scanner AGENT endpoints (register/stream/push) are mounted on `app` HERE — // deliberately BEFORE the /api/v1 user-token middleware below — so they are diff --git a/backend/internal/application/auth/logout.go b/backend/internal/application/auth/logout.go index d84c9fae..b22147ff 100644 --- a/backend/internal/application/auth/logout.go +++ b/backend/internal/application/auth/logout.go @@ -7,39 +7,70 @@ package auth import ( "context" + "errors" "fmt" - - "github.com/opendefender/openrisk/internal/auth" + "time" ) // LogoutInput represents the input for user logout type LogoutInput struct { RefreshToken string + // AccessJTI and AccessExpiresAt identify the access token the request came + // with, already verified by the caller. Revoking it is what makes logout + // end the session now rather than when the token expires (#689). + AccessJTI string + AccessExpiresAt time.Time +} + +// RefreshTokenRevoker deletes a refresh token. Satisfied by *auth.TokenManager. +type RefreshTokenRevoker interface { + RevokeRefreshToken(ctx context.Context, refreshTokenValue string) error } // LogoutUseCase handles user logout type LogoutUseCase struct { - tokenManager *auth.TokenManager + tokenManager RefreshTokenRevoker + accessTokens TokenRevoker } // NewLogoutUseCase creates a new logout use case -func NewLogoutUseCase(tokenManager *auth.TokenManager) *LogoutUseCase { +func NewLogoutUseCase(tokenManager RefreshTokenRevoker) *LogoutUseCase { return &LogoutUseCase{ tokenManager: tokenManager, } } -// Execute performs user logout +// WithAccessTokenRevocation blacklists the access token on logout. Without it +// the access token stays valid until it expires, up to AccessTokenTTL. +func (uc *LogoutUseCase) WithAccessTokenRevocation(r TokenRevoker) *LogoutUseCase { + uc.accessTokens = r + return uc +} + +// Execute performs user logout. +// +// The access token is revoked first: a refresh token that is already gone (a +// second logout, a rotated cookie) must not leave the access token working. +// Both revocations are always attempted, so a Redis outage on the first never +// spares the refresh token. func (uc *LogoutUseCase) Execute(ctx context.Context, input LogoutInput) error { - // Validate input - if input.RefreshToken == "" { + if input.RefreshToken == "" && input.AccessJTI == "" { return fmt.Errorf("refresh token is required") } - // Revoke the refresh token - if err := uc.tokenManager.RevokeRefreshToken(ctx, input.RefreshToken); err != nil { - return fmt.Errorf("failed to revoke refresh token: %w", err) + var accessErr error + if input.AccessJTI != "" && uc.accessTokens != nil { + if ttl := time.Until(input.AccessExpiresAt); ttl > 0 { + if err := uc.accessTokens.BlacklistJTI(ctx, input.AccessJTI, ttl); err != nil { + accessErr = fmt.Errorf("failed to revoke access token: %w", err) + } + } } - return nil + if input.RefreshToken != "" { + if err := uc.tokenManager.RevokeRefreshToken(ctx, input.RefreshToken); err != nil { + return errors.Join(fmt.Errorf("failed to revoke refresh token: %w", err), accessErr) + } + } + return accessErr } diff --git a/backend/internal/application/auth/logout_test.go b/backend/internal/application/auth/logout_test.go new file mode 100644 index 00000000..5df5590c --- /dev/null +++ b/backend/internal/application/auth/logout_test.go @@ -0,0 +1,107 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "errors" + "testing" + "time" +) + +type fakeRefreshRevoker struct { + known map[string]bool + revoked []string +} + +func (f *fakeRefreshRevoker) RevokeRefreshToken(_ context.Context, v string) error { + if !f.known[v] { + return errors.New("refresh token not found") + } + f.revoked = append(f.revoked, v) + return nil +} + +type failingJTIRevoker struct{ calls int } + +func (f *failingJTIRevoker) BlacklistJTI(context.Context, string, time.Duration) error { + f.calls++ + return errors.New("redis down") +} + +func newLogoutFixture() (*LogoutUseCase, *fakeRefreshRevoker, *fakeJTIRevoker) { + refresh := &fakeRefreshRevoker{known: map[string]bool{"rt-1": true}} + access := &fakeJTIRevoker{jtis: map[string]time.Duration{}} + return NewLogoutUseCase(refresh).WithAccessTokenRevocation(access), refresh, access +} + +func TestLogout_Success(t *testing.T) { + uc, refresh, access := newLogoutFixture() + err := uc.Execute(context.Background(), LogoutInput{ + RefreshToken: "rt-1", AccessJTI: "jti-1", AccessExpiresAt: time.Now().Add(10 * time.Minute), + }) + if err != nil { + t.Fatal(err) + } + if len(refresh.revoked) != 1 { + t.Fatal("the refresh token must be revoked") + } + ttl, ok := access.jtis["jti-1"] + if !ok || ttl <= 9*time.Minute || ttl > 10*time.Minute { + t.Fatalf("the access token must be blacklisted for what it has left, got %v %v", ok, ttl) + } +} + +// A refresh token that is already gone still lets the access token die. +func TestLogout_NotFound(t *testing.T) { + uc, _, access := newLogoutFixture() + err := uc.Execute(context.Background(), LogoutInput{ + RefreshToken: "rt-unknown", AccessJTI: "jti-1", AccessExpiresAt: time.Now().Add(time.Minute), + }) + if err == nil { + t.Fatal("an unknown refresh token must be reported") + } + if !access.revoked("jti-1") { + t.Fatal("the access token must be revoked even when the refresh token is unknown") + } +} + +func TestLogout_Unauthorized(t *testing.T) { + uc, refresh, access := newLogoutFixture() + if err := uc.Execute(context.Background(), LogoutInput{}); err == nil { + t.Fatal("a logout with no credential must fail") + } + if len(refresh.revoked) != 0 || len(access.jtis) != 0 { + t.Fatal("nothing may be revoked without a credential") + } +} + +func TestLogout_ExpiredAccessTokenNeedsNoBlacklist(t *testing.T) { + uc, _, access := newLogoutFixture() + _ = uc.Execute(context.Background(), LogoutInput{ + RefreshToken: "rt-1", AccessJTI: "jti-1", AccessExpiresAt: time.Now().Add(-time.Second), + }) + if access.revoked("jti-1") { + t.Fatal("an expired token is already dead; blacklisting it wastes a key") + } +} + +// A Redis outage on the blacklist must not spare the refresh token. +func TestLogout_BlacklistOutageStillRevokesRefreshToken(t *testing.T) { + refresh := &fakeRefreshRevoker{known: map[string]bool{"rt-1": true}} + broken := &failingJTIRevoker{} + uc := NewLogoutUseCase(refresh).WithAccessTokenRevocation(broken) + + err := uc.Execute(context.Background(), LogoutInput{ + RefreshToken: "rt-1", AccessJTI: "jti-1", AccessExpiresAt: time.Now().Add(time.Minute), + }) + if err == nil { + t.Fatal("the outage must be reported") + } + if broken.calls != 1 || len(refresh.revoked) != 1 { + t.Fatalf("both revocations must be attempted: blacklist=%d refresh=%d", broken.calls, len(refresh.revoked)) + } +} diff --git a/backend/internal/application/auth/mfa_challenge_limits_test.go b/backend/internal/application/auth/mfa_challenge_limits_test.go new file mode 100644 index 00000000..e86a6fd1 --- /dev/null +++ b/backend/internal/application/auth/mfa_challenge_limits_test.go @@ -0,0 +1,323 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + "time" + + "github.com/google/uuid" + "github.com/pquerna/otp/totp" + "golang.org/x/crypto/bcrypt" + + "github.com/opendefender/openrisk/internal/domain" + "github.com/opendefender/openrisk/internal/infrastructure/authmfa" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" +) + +// challengeFixture is one enrolled account behind a challenge use case with +// the #689 limits wired to in-memory fakes. +type challengeFixture struct { + uc *ChallengeMFAUseCase + userID uuid.UUID + tenantID uuid.UUID + secret string + backup string + store *authmfa.MemoryAttemptStore + revoker *fakeJTIRevoker + mailer *fakeLockMailer +} + +type fakeJTIRevoker struct { + mu sync.Mutex + jtis map[string]time.Duration +} + +func (r *fakeJTIRevoker) BlacklistJTI(_ context.Context, jti string, ttl time.Duration) error { + r.mu.Lock() + defer r.mu.Unlock() + r.jtis[jti] = ttl + return nil +} + +func (r *fakeJTIRevoker) revoked(jti string) bool { + r.mu.Lock() + defer r.mu.Unlock() + _, ok := r.jtis[jti] + return ok +} + +type fakeLockMailer struct { + mu sync.Mutex + sent []string +} + +func (m *fakeLockMailer) SendMFALocked(_ context.Context, to, _, locale string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.sent = append(m.sent, to+"|"+locale) + return nil +} + +func (m *fakeLockMailer) count() int { + m.mu.Lock() + defer m.mu.Unlock() + return len(m.sent) +} + +type fakeLockUsers struct{ user *domain.User } + +func (f fakeLockUsers) GetByID(_ context.Context, id uuid.UUID) (*domain.User, error) { + if f.user != nil && f.user.ID == id { + return f.user, nil + } + return nil, nil +} + +func newChallengeFixture(t *testing.T) *challengeFixture { + t.Helper() + key := make([]byte, 32) + repo := NewMockMFARepository() + f := &challengeFixture{ + userID: uuid.New(), + tenantID: uuid.New(), + store: authmfa.NewMemoryAttemptStore(), + revoker: &fakeJTIRevoker{jtis: map[string]time.Duration{}}, + mailer: &fakeLockMailer{}, + backup: "K7M2Q9XT", + } + + secret, err := otp.GenerateTOTPSecret() + if err != nil { + t.Fatal(err) + } + f.secret = secret + enc, err := crypto.EncryptAES256GCM(secret, key) + if err != nil { + t.Fatal(err) + } + _ = repo.CreateMFASecret(context.Background(), &domain.MFASecret{ + ID: uuid.New(), UserID: f.userID, TenantID: f.tenantID, SecretEncrypted: enc, IsVerified: true, + }) + hash, _ := bcrypt.GenerateFromPassword([]byte(f.backup), bcrypt.MinCost) + _ = repo.SaveBackupCodes(context.Background(), []*domain.MFABackupCode{ + {ID: uuid.New(), UserID: f.userID, TenantID: f.tenantID, CodeHash: string(hash)}, + }) + + user := &domain.User{ID: f.userID, Email: "awa.diallo@example.test", FullName: "Awa Diallo", Locale: "en"} + f.uc = NewChallengeMFAUseCase(repo, key). + WithAttemptLimits(f.store, f.revoker). + WithLockNotice(fakeLockUsers{user: user}, f.mailer) + return f +} + +func (f *challengeFixture) try(jti, code string) error { + _, err := f.uc.Execute(context.Background(), ChallengeMFAInput{ + UserID: f.userID, TenantID: f.tenantID, Code: code, + ChallengeJTI: jti, ChallengeExpiresAt: time.Now().Add(5 * time.Minute), + }) + return err +} + +func (f *challengeFixture) goodCode(t *testing.T) string { + t.Helper() + code, err := totp.GenerateCode(f.secret, time.Now()) + if err != nil { + t.Fatal(err) + } + return code +} + +// badCode is a six-digit code the authenticator would not accept right now. +func (f *challengeFixture) badCode(t *testing.T, seed int) string { + t.Helper() + for i := seed; ; i++ { + c := fmt.Sprintf("%06d", (i*7919+13)%1000000) + if !otp.VerifyTOTP(f.secret, c) { + return c + } + } +} + +func isInvalidCode(err error) bool { + var appErr *domain.AppError + return errors.As(err, &appErr) && errors.Is(appErr.Err, domain.ErrValidation) +} + +func TestMFAChallenge_Success(t *testing.T) { + f := newChallengeFixture(t) + if err := f.try("jti-1", f.goodCode(t)); err != nil { + t.Fatalf("a correct code must pass: %v", err) + } +} + +func TestMFAChallenge_NotFound(t *testing.T) { + f := newChallengeFixture(t) + _, err := f.uc.Execute(context.Background(), ChallengeMFAInput{ + UserID: uuid.New(), TenantID: f.tenantID, Code: "123456", ChallengeJTI: "jti-x", + }) + var appErr *domain.AppError + if !errors.As(err, &appErr) || !errors.Is(appErr.Err, domain.ErrNotFound) { + t.Fatalf("an account without a factor must be NotFound, got %v", err) + } +} + +// A locked account is refused even with the right code: the lock is checked +// before the code is. +func TestMFAChallenge_Unauthorized(t *testing.T) { + f := newChallengeFixture(t) + if _, err := f.store.Lock(context.Background(), userLockKey(f.userID), time.Minute); err != nil { + t.Fatal(err) + } + var locked *MFAChallengeLockedError + if err := f.try("jti-1", f.goodCode(t)); !errors.As(err, &locked) { + t.Fatalf("a locked account must refuse a correct code, got %v", err) + } + if locked.JustLocked { + t.Fatal("an existing lock must not read as newly set") + } +} + +func TestMFAChallenge_TokenDiesAfterFiveFailures(t *testing.T) { + f := newChallengeFixture(t) + for i := 1; i < MFAChallengeMaxAttemptsPerToken; i++ { + if err := f.try("jti-1", f.badCode(t, i)); !isInvalidCode(err) { + t.Fatalf("failure %d: want invalid code, got %v", i, err) + } + } + if f.revoker.revoked("jti-1") { + t.Fatal("the token must survive four failures") + } + + if err := f.try("jti-1", f.badCode(t, 99)); !errors.Is(err, ErrMFAChallengeExhausted) { + t.Fatalf("fifth failure: want ErrMFAChallengeExhausted, got %v", err) + } + if !f.revoker.revoked("jti-1") { + t.Fatal("the fifth failure must blacklist the token") + } + + // Even with the blacklist failing open, the counter refuses the token, + // and it refuses it before looking at the code. + if err := f.try("jti-1", f.goodCode(t)); !errors.Is(err, ErrMFAChallengeExhausted) { + t.Fatalf("a spent token must refuse a correct code, got %v", err) + } + // A fresh login gets a fresh token. + if err := f.try("jti-2", f.goodCode(t)); err != nil { + t.Fatalf("a new token must work: %v", err) + } +} + +func TestMFAChallenge_UserBackoffAcrossTokens(t *testing.T) { + f := newChallengeFixture(t) + // Nine failures over three tokens: 4 + 4 + 1. No token is spent, the + // account is one failure short of the lock. + tokens := []string{"a", "a", "a", "a", "b", "b", "b", "b", "c"} + for i, jti := range tokens { + if err := f.try(jti, f.badCode(t, i)); !isInvalidCode(err) { + t.Fatalf("failure %d: want invalid code, got %v", i+1, err) + } + } + + var locked *MFAChallengeLockedError + if err := f.try("c", f.badCode(t, 50)); !errors.As(err, &locked) { + t.Fatalf("the tenth failure must lock the account, got %v", err) + } + if !locked.JustLocked || locked.RetryAfter != MFAChallengeLockDuration { + t.Fatalf("lock: JustLocked=%v RetryAfter=%v", locked.JustLocked, locked.RetryAfter) + } + + // Another token, another address: still locked, with what remains. + if err := f.try("d", f.goodCode(t)); !errors.As(err, &locked) { + t.Fatalf("a correct code on a new token must be refused during the lock, got %v", err) + } + if locked.JustLocked || locked.RetryAfter <= 0 || locked.RetryAfter > MFAChallengeLockDuration+time.Second { + t.Fatalf("refusal under lock: JustLocked=%v RetryAfter=%v", locked.JustLocked, locked.RetryAfter) + } +} + +func TestMFAChallenge_BackupCodesCounted(t *testing.T) { + f := newChallengeFixture(t) + // Wrong backup-shaped codes spend the token exactly like wrong TOTP codes. + wrong := []string{"AAAAAAAA", "BBBBBBBB", "CCCCCCCC", "DDDDDDDD"} + for _, c := range wrong { + if err := f.try("jti-1", c); !isInvalidCode(err) { + t.Fatalf("%s: want invalid code, got %v", c, err) + } + } + if err := f.try("jti-1", "EEEEEEEE"); !errors.Is(err, ErrMFAChallengeExhausted) { + t.Fatalf("fifth wrong backup code: want ErrMFAChallengeExhausted, got %v", err) + } + // The real backup code cannot rescue a spent token. + if err := f.try("jti-1", f.backup); !errors.Is(err, ErrMFAChallengeExhausted) { + t.Fatalf("a valid backup code on a spent token: got %v", err) + } + // And the five backup failures count towards the account too. + if n, _ := f.store.Count(context.Background(), userFailuresKey(f.userID)); n < 5 { + t.Fatalf("account failures: got %d, want at least 5", n) + } + // On a fresh token the real backup code still works. + if err := f.try("jti-2", f.backup); err != nil { + t.Fatalf("a valid backup code on a fresh token: %v", err) + } +} + +func TestMFAChallenge_SuccessClearsFailureCount(t *testing.T) { + f := newChallengeFixture(t) + for i := range 4 { + _ = f.try("a", f.badCode(t, i)) + } + for i := range 4 { + _ = f.try("b", f.badCode(t, 10+i)) + } + if err := f.try("c", f.goodCode(t)); err != nil { + t.Fatalf("correct code: %v", err) + } + // Eight earlier failures are forgiven: eight more do not lock. + for i := range 4 { + _ = f.try("d", f.badCode(t, 20+i)) + } + for i := range 4 { + if err := f.try("e", f.badCode(t, 30+i)); !isInvalidCode(err) { + t.Fatalf("after a success the count restarts, got %v", err) + } + } +} + +func TestMFAChallenge_LockSendsOneEmail(t *testing.T) { + f := newChallengeFixture(t) + for i := range MFAChallengeMaxFailuresPerUser { + _ = f.try(fmt.Sprintf("t%d", i/4), f.badCode(t, i)) + } + for i := range 20 { + _ = f.try(fmt.Sprintf("x%d", i), f.goodCode(t)) + } + if got := f.mailer.count(); got != 1 { + t.Fatalf("one lock must send one email, got %d", got) + } + if f.mailer.sent[0] != "awa.diallo@example.test|en" { + t.Fatalf("email sent to %q", f.mailer.sent[0]) + } +} + +type brokenStore struct{ *authmfa.MemoryAttemptStore } + +func (brokenStore) LockedFor(context.Context, string) (time.Duration, error) { + return 0, errors.New("store down") +} + +// If the limiter cannot answer, the challenge must not go ahead unchecked. +func TestMFAChallenge_StoreErrorFailsClosed(t *testing.T) { + f := newChallengeFixture(t) + f.uc.WithAttemptLimits(brokenStore{authmfa.NewMemoryAttemptStore()}, f.revoker) + if err := f.try("jti-1", f.goodCode(t)); err == nil { + t.Fatal("a store error must refuse the attempt") + } +} diff --git a/backend/internal/application/auth/mfa_limits.go b/backend/internal/application/auth/mfa_limits.go new file mode 100644 index 00000000..cd1cc5b0 --- /dev/null +++ b/backend/internal/application/auth/mfa_limits.go @@ -0,0 +1,249 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + + coreauth "github.com/opendefender/openrisk/internal/auth" + "github.com/opendefender/openrisk/internal/domain" +) + +// Limits on the MFA challenge (#689). A 6-digit code with a ±1 step tolerance +// leaves about 3 valid values in 10^6, so the budget has to be counted in +// single digits per token and in tens per account, not per minute. +const ( + // MFAChallengeMaxAttemptsPerToken is how many codes one MFA_REQUIRED token + // may try. The token is blacklisted when the last one fails. + MFAChallengeMaxAttemptsPerToken = 5 + // MFAChallengeMaxFailuresPerUser is how many failures an account takes, + // across every token and source address, before it is locked. + MFAChallengeMaxFailuresPerUser = 10 + // MFAChallengeFailureWindow is counted from the first failure. + MFAChallengeFailureWindow = 15 * time.Minute + // MFAChallengeLockDuration is fixed: attempts during the lock do not + // extend it, so a lock can never outlast this. + MFAChallengeLockDuration = 15 * time.Minute +) + +// ErrMFAChallengeExhausted means this challenge token has used all its +// attempts. The caller must sign in again with the password. +var ErrMFAChallengeExhausted = errors.New("this sign-in attempt has no codes left") + +// MFAChallengeLockedError means the account refuses challenges for a while. +type MFAChallengeLockedError struct { + RetryAfter time.Duration + // JustLocked is true only for the request that set the lock, so the + // caller audits a lock once rather than once per refused attempt. + JustLocked bool +} + +func (e *MFAChallengeLockedError) Error() string { + return "two-factor sign-in is locked for this account" +} + +// MFALockedMailer tells the owner that their account was locked after repeated +// second-factor failures, which means someone else holds their password. +type MFALockedMailer interface { + SendMFALocked(ctx context.Context, to, fullName, locale string) error +} + +// MFALockUserLookup finds the address to warn. Satisfied by +// *repository.GormUserRepository. +type MFALockUserLookup interface { + GetByID(ctx context.Context, id uuid.UUID) (*domain.User, error) +} + +// MFAAttemptStore holds the counters and locks that bound guessing at the MFA +// challenge (#689). It must be shared across instances: a per-instance counter +// lets an attacker multiply the budget by the number of replicas. +type MFAAttemptStore interface { + // Increment adds one to key and returns the new count. The key expires ttl + // after its first increment; later increments do not move that deadline. + Increment(ctx context.Context, key string, ttl time.Duration) (int64, error) + // Count returns the current value of key, 0 when it does not exist. + Count(ctx context.Context, key string) (int64, error) + // Lock sets key for ttl unless it is already set, and reports whether this + // call set it. Only the call that sets the lock audits it and mails the + // owner, so a burst of refused attempts produces one notice, not fifty. + Lock(ctx context.Context, key string, ttl time.Duration) (bool, error) + // LockedFor returns how long the lock on key still holds, 0 when unlocked. + LockedFor(ctx context.Context, key string) (time.Duration, error) + // Delete removes key. Deleting a missing key is not an error. + Delete(ctx context.Context, key string) error +} + +// TokenRevoker blacklists a token's JTI until the token would have expired +// anyway. *pkg/auth.TokenBlacklistManager satisfies it. +type TokenRevoker interface { + BlacklistJTI(ctx context.Context, jti string, ttl time.Duration) error +} + +// WithAttemptLimits turns the limits on. Without it the challenge behaves as it +// did before #689; main.go always wires it. +func (uc *ChallengeMFAUseCase) WithAttemptLimits(store MFAAttemptStore, revoker TokenRevoker) *ChallengeMFAUseCase { + if uc.limits == nil { + uc.limits = &challengeLimits{} + } + uc.limits.store = store + uc.limits.revoker = revoker + return uc +} + +// WithLockNotice mails the owner when their account gets locked. Optional. +func (uc *ChallengeMFAUseCase) WithLockNotice(users MFALockUserLookup, mailer MFALockedMailer) *ChallengeMFAUseCase { + if uc.limits == nil { + uc.limits = &challengeLimits{} + } + uc.limits.users = users + uc.limits.mailer = mailer + return uc +} + +type challengeLimits struct { + store MFAAttemptStore + revoker TokenRevoker + users MFALockUserLookup + mailer MFALockedMailer +} + +// The account keys are per user, not per (user, tenant): the lock protects the +// identity, whichever organisation the login targets. +func tokenAttemptsKey(jti string) string { return "challenge:token:" + jti } +func userFailuresKey(userID uuid.UUID) string { return "challenge:user:" + userID.String() } +func userLockKey(userID uuid.UUID) string { return "challenge:lock:" + userID.String() } + +// reserve refuses a locked account or a spent token, then counts this attempt +// against both before the code is looked at. +func (l *challengeLimits) reserve(ctx context.Context, input ChallengeMFAInput) error { + if l.store == nil { + return nil + } + locked, err := l.store.LockedFor(ctx, userLockKey(input.UserID)) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: read lock: %w", err) + } + if locked > 0 { + return &MFAChallengeLockedError{RetryAfter: locked} + } + + if input.ChallengeJTI != "" { + n, err := l.store.Increment(ctx, tokenAttemptsKey(input.ChallengeJTI), tokenRemaining(input)) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: count token attempt: %w", err) + } + if n > MFAChallengeMaxAttemptsPerToken { + l.revoke(ctx, input) + return ErrMFAChallengeExhausted + } + } + + n, err := l.store.Increment(ctx, userFailuresKey(input.UserID), MFAChallengeFailureWindow) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: count account attempt: %w", err) + } + if n > MFAChallengeMaxFailuresPerUser { + // Only reachable when parallel requests raced past the lock check. + return l.lock(ctx, input.UserID) + } + return nil +} + +// recordFailure acts on the counts reserve already raised: the account lock +// first, because it outranks the token, then the token. +func (l *challengeLimits) recordFailure(ctx context.Context, input ChallengeMFAInput) error { + if l.store == nil { + return nil + } + failures, err := l.store.Count(ctx, userFailuresKey(input.UserID)) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: read account failures: %w", err) + } + if failures >= MFAChallengeMaxFailuresPerUser { + l.revoke(ctx, input) + return l.lock(ctx, input.UserID) + } + + if input.ChallengeJTI == "" { + return nil + } + attempts, err := l.store.Count(ctx, tokenAttemptsKey(input.ChallengeJTI)) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: read token attempts: %w", err) + } + if attempts >= MFAChallengeMaxAttemptsPerToken { + l.revoke(ctx, input) + return ErrMFAChallengeExhausted + } + return nil +} + +// recordSuccess forgives earlier mistypes. An active lock is not touched: a +// success cannot happen under one, reserve refuses first. +func (l *challengeLimits) recordSuccess(ctx context.Context, input ChallengeMFAInput) { + if l.store == nil { + return + } + _ = l.store.Delete(ctx, userFailuresKey(input.UserID)) +} + +func (l *challengeLimits) lock(ctx context.Context, userID uuid.UUID) error { + set, err := l.store.Lock(ctx, userLockKey(userID), MFAChallengeLockDuration) + if err != nil { + return fmt.Errorf("auth.ChallengeMFA: set lock: %w", err) + } + // A fresh window starts when the lock ends. + _ = l.store.Delete(ctx, userFailuresKey(userID)) + + retry := MFAChallengeLockDuration + if !set { + if d, err := l.store.LockedFor(ctx, userLockKey(userID)); err == nil && d > 0 { + retry = d + } + } else { + l.notify(ctx, userID) + } + return &MFAChallengeLockedError{RetryAfter: retry, JustLocked: set} +} + +// revoke kills the challenge token. The counter already refuses it; the +// blacklist makes the middleware refuse it too, before any code is read. +func (l *challengeLimits) revoke(ctx context.Context, input ChallengeMFAInput) { + if l.revoker == nil || input.ChallengeJTI == "" { + return + } + _ = l.revoker.BlacklistJTI(ctx, input.ChallengeJTI, tokenRemaining(input)) +} + +func (l *challengeLimits) notify(ctx context.Context, userID uuid.UUID) { + if l.mailer == nil || l.users == nil { + return + } + user, err := l.users.GetByID(ctx, userID) + if err != nil || user == nil || user.Email == "" { + return + } + _ = l.mailer.SendMFALocked(ctx, user.Email, user.FullName, normaliseLocale(user.Locale)) +} + +// tokenRemaining is how long the challenge token has left, which is how long +// its counter and its blacklist entry need to live. A token without a known +// expiry gets the full challenge lifetime. +func tokenRemaining(input ChallengeMFAInput) time.Duration { + if input.ChallengeExpiresAt.IsZero() { + return coreauth.MFAChallengeTTL + } + d := time.Until(input.ChallengeExpiresAt) + if d < time.Second { + return time.Second + } + return d +} diff --git a/backend/internal/application/auth/mfa_usecase.go b/backend/internal/application/auth/mfa_usecase.go index c863f2d0..1a7465ba 100644 --- a/backend/internal/application/auth/mfa_usecase.go +++ b/backend/internal/application/auth/mfa_usecase.go @@ -401,6 +401,10 @@ type ChallengeMFAInput struct { UserID uuid.UUID TenantID uuid.UUID Code string // TOTP code OR backup code + // ChallengeJTI and ChallengeExpiresAt identify the MFA_REQUIRED token the + // request came with. Attempts are counted against it (#689). + ChallengeJTI string + ChallengeExpiresAt time.Time } // ChallengeMFAOutput represents MFA challenge response @@ -413,6 +417,7 @@ type ChallengeMFAOutput struct { type ChallengeMFAUseCase struct { mfaRepo repository.MFARepository encKey []byte + limits *challengeLimits } // NewChallengeMFAUseCase creates a new challenge MFA use case @@ -423,7 +428,11 @@ func NewChallengeMFAUseCase(mfaRepo repository.MFARepository, encKey []byte) *Ch } } -// Execute verifies TOTP or backup code during login +// Execute verifies TOTP or backup code during login. +// +// With limits wired, every attempt is reserved against the challenge token and +// the account before the code is checked, so parallel requests cannot all slip +// under the limit together. A TOTP code and a backup code are the same attempt. func (uc *ChallengeMFAUseCase) Execute(ctx context.Context, input ChallengeMFAInput) (*ChallengeMFAOutput, error) { if input.UserID == uuid.Nil || input.TenantID == uuid.Nil { return nil, domain.NewValidationError("user_id and tenant_id required") @@ -432,6 +441,35 @@ func (uc *ChallengeMFAUseCase) Execute(ctx context.Context, input ChallengeMFAIn return nil, domain.NewValidationError("code required") } + if uc.limits != nil { + if err := uc.limits.reserve(ctx, input); err != nil { + return nil, err + } + } + + out, err := uc.verify(ctx, input) + if errors.Is(err, errMFACodeMismatch) { + if uc.limits != nil { + if limitErr := uc.limits.recordFailure(ctx, input); limitErr != nil { + return nil, limitErr + } + } + return nil, domain.NewValidationError("invalid MFA code") + } + if err != nil { + return nil, err + } + + if uc.limits != nil { + uc.limits.recordSuccess(ctx, input) + } + return out, nil +} + +// errMFACodeMismatch is the one outcome that counts as a failed guess. +var errMFACodeMismatch = errors.New("mfa code mismatch") + +func (uc *ChallengeMFAUseCase) verify(ctx context.Context, input ChallengeMFAInput) (*ChallengeMFAOutput, error) { // Get MFA secret mfaSecret, err := uc.mfaRepo.GetMFASecret(ctx, input.UserID, input.TenantID) if err != nil { @@ -480,5 +518,5 @@ func (uc *ChallengeMFAUseCase) Execute(ctx context.Context, input ChallengeMFAIn } } - return nil, domain.NewValidationError("invalid MFA code") + return nil, errMFACodeMismatch } diff --git a/backend/internal/handler/auth/handler.go b/backend/internal/handler/auth/handler.go index 6989452e..aa5b788d 100644 --- a/backend/internal/handler/auth/handler.go +++ b/backend/internal/handler/auth/handler.go @@ -8,6 +8,7 @@ package auth import ( "context" "errors" + "strings" "github.com/gofiber/fiber/v2" "github.com/google/uuid" @@ -15,6 +16,7 @@ import ( coreauth "github.com/opendefender/openrisk/internal/auth" "github.com/opendefender/openrisk/internal/domain" "github.com/opendefender/openrisk/internal/middleware" + authpkg "github.com/opendefender/openrisk/pkg/auth" ) // Handler handles authentication endpoints @@ -39,6 +41,17 @@ type Handler struct { // CURRENT business role. Optional; without it the field is omitted rather // than guessed (#338). memberLookup OrganizationMemberReader + // accessTokenKeys verifies the access token presented at logout so its + // JTI can be revoked (#689). Optional; without it logout ends only the + // refresh token, and the access token lives out its TTL. + accessTokenKeys *authpkg.RSAKeys +} + +// WithAccessTokenRevocation lets logout verify and revoke the access token it +// was called with. The use case must be wired with a revoker too. +func (h *Handler) WithAccessTokenRevocation(keys *authpkg.RSAKeys) *Handler { + h.accessTokenKeys = keys + return h } // UserByIDReader resolves a user by id for /auth/me. @@ -490,13 +503,19 @@ func (h *Handler) Logout(c *fiber.Ctx) error { // cookies behind is worse than one that reports an error. middleware.ClearSessionCookies(c) - if req.RefreshToken == "" { + input := auth.LogoutInput{RefreshToken: req.RefreshToken} + if claims := h.presentedAccessToken(c); claims != nil { + input.AccessJTI = claims.JTI + if claims.ExpiresAt != nil { + input.AccessExpiresAt = claims.ExpiresAt.Time + } + } + + if input.RefreshToken == "" && input.AccessJTI == "" { return c.JSON(fiber.Map{"message": "Logged out"}) } - err := h.logoutUseCase.Execute(c.UserContext(), auth.LogoutInput{ - RefreshToken: req.RefreshToken, - }) + err := h.logoutUseCase.Execute(c.UserContext(), input) if err != nil { reason := "logout failed" @@ -512,6 +531,34 @@ func (h *Handler) Logout(c *fiber.Ctx) error { }) } +// presentedAccessToken returns the verified claims of the access token sent +// with the request, from the Authorization header or the session cookie, the +// same order the auth middleware reads them. A missing, forged or expired +// token yields nil: logout still clears the cookies, there is just nothing +// left to revoke. A valid signature is required, so nobody can blacklist a +// JTI they do not hold. +func (h *Handler) presentedAccessToken(c *fiber.Ctx) *authpkg.Claims { + if h.accessTokenKeys == nil { + return nil + } + token := "" + if header := c.Get("Authorization"); header != "" { + if parts := strings.Split(header, " "); len(parts) == 2 && parts[0] == "Bearer" { + token = parts[1] + } + } else { + token = c.Cookies(middleware.AccessTokenCookie) + } + if token == "" { + return nil + } + claims, err := authpkg.ValidateAccessToken(h.accessTokenKeys, token, nil) + if err != nil || claims.JTI == "" { + return nil + } + return claims +} + // Me godoc // @Summary Get current user profile // @Description Get profile of currently authenticated user diff --git a/backend/internal/handler/auth/logout_e2e_test.go b/backend/internal/handler/auth/logout_e2e_test.go new file mode 100644 index 00000000..164a7b43 --- /dev/null +++ b/backend/internal/handler/auth/logout_e2e_test.go @@ -0,0 +1,70 @@ +// 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 ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/opendefender/openrisk/internal/middleware" +) + +// #689 — logout ends the access token too, not just the refresh token. + +func TestLogout_AccessTokenRejectedAfterLogout(t *testing.T) { + s := newChallengeStack(t, 100) + _, access := s.login(t, s.memberA.User.Email) + require.NotEmpty(t, access) + + status, _ := s.do(t, http.MethodGet, "/api/v1/auth/me", access, nil) + require.Equal(t, http.StatusOK, status, "the token works before logout") + + status, body := s.do(t, http.MethodPost, "/api/v1/auth/logout", access, nil) + require.Equal(t, http.StatusOK, status, "%v", body) + + status, body = s.do(t, http.MethodGet, "/api/v1/auth/me", access, nil) + assert.Equal(t, http.StatusUnauthorized, status) + assert.Equal(t, "TOKEN_REVOKED", body["code"]) +} + +// A browser holds the token in the session cookie, not in a header. +func TestLogout_CookieSessionAccessTokenRejectedAfterLogout(t *testing.T) { + s := newChallengeStack(t, 100) + _, access := s.login(t, s.memberA.User.Email) + require.NotEmpty(t, access) + + withCookie := func(method, path string) *http.Response { + req := httptest.NewRequest(method, path, strings.NewReader("")) + req.AddCookie(&http.Cookie{Name: middleware.AccessTokenCookie, Value: access}) + resp, err := s.app.Test(req, -1) + require.NoError(t, err) + _ = resp.Body.Close() + return resp + } + + require.Equal(t, http.StatusOK, withCookie(http.MethodGet, "/api/v1/auth/me").StatusCode) + require.Equal(t, http.StatusOK, withCookie(http.MethodPost, "/api/v1/auth/logout").StatusCode) + assert.Equal(t, http.StatusUnauthorized, withCookie(http.MethodGet, "/api/v1/auth/me").StatusCode) +} + +// A forged token at logout revokes nothing and still answers 200: the caller +// loses their cookies either way. +func TestLogout_ForgedTokenIsIgnored(t *testing.T) { + s := newChallengeStack(t, 100) + _, access := s.login(t, s.memberA.User.Email) + + forged := access[:len(access)-4] + "AAAA" + status, _ := s.do(t, http.MethodPost, "/api/v1/auth/logout", forged, nil) + assert.Equal(t, http.StatusOK, status) + + status, _ = s.do(t, http.MethodGet, "/api/v1/auth/me", access, nil) + assert.Equal(t, http.StatusOK, status, "the real token was not revoked by a forgery") +} diff --git a/backend/internal/handler/auth/mfa_challenge_e2e_test.go b/backend/internal/handler/auth/mfa_challenge_e2e_test.go new file mode 100644 index 00000000..1cf11fae --- /dev/null +++ b/backend/internal/handler/auth/mfa_challenge_e2e_test.go @@ -0,0 +1,250 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// This program is free software: you can redistribute it and/or modify it under +// the terms of the GNU Affero General Public License v3.0 (see LICENSE). + +package auth_test + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + appauth "github.com/opendefender/openrisk/internal/application/auth" + coreauth "github.com/opendefender/openrisk/internal/auth" + "github.com/opendefender/openrisk/internal/domain" + authhandler "github.com/opendefender/openrisk/internal/handler/auth" + "github.com/opendefender/openrisk/internal/infrastructure/authmfa" + "github.com/opendefender/openrisk/internal/infrastructure/repository" + "github.com/opendefender/openrisk/internal/middleware" + authpkg "github.com/opendefender/openrisk/pkg/auth" + "github.com/opendefender/openrisk/pkg/crypto" + "github.com/opendefender/openrisk/pkg/otp" +) + +// --------------------------------------------------------------------------- +// #689 — the MFA challenge through the real HTTP stack: per-IP limiter, the +// MFA_REQUIRED token middleware with a JTI blacklist, handler, use case and +// GORM repository, mounted in the order main.go uses. +// --------------------------------------------------------------------------- + +const challengePath = "/api/v1/auth/mfa/challenge" + +// memoryBlacklist stands in for Redis behind authpkg.TokenBlacklistManager. +type memoryBlacklist struct { + mu sync.Mutex + keys map[string]string +} + +func (m *memoryBlacklist) Set(_ context.Context, key, value string, _ time.Duration) error { + m.mu.Lock() + defer m.mu.Unlock() + m.keys[key] = value + return nil +} + +func (m *memoryBlacklist) Get(_ context.Context, key string) (string, error) { + m.mu.Lock() + defer m.mu.Unlock() + return m.keys[key], nil +} + +func (m *memoryBlacklist) Exists(_ context.Context, key string) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + _, ok := m.keys[key] + return ok, nil +} + +// challengeStack adds the challenge (and logout) routes to a fixture's +// database and keys, on a fresh app. +type challengeStack struct { + *deferredFixture + ipLimit int +} + +func newChallengeStack(t *testing.T, ipLimit int) *challengeStack { + t.Helper() + f := newDeferredFixture(t) + userRepo := repository.NewGormUserRepository(f.db) + mfaRepo := repository.NewGormMFARepository(f.db) + tokens := coreauth.NewTokenManager(f.db, f.keys) + // What main.go's resolver derives from the default membership, reduced to + // the one account these tests sign in with. + tokens.SetSessionResolver(func(_ context.Context, userID uuid.UUID) (*coreauth.SessionClaims, error) { + return &coreauth.SessionClaims{ + TenantID: f.tenantA, + OrgRoles: map[uuid.UUID]string{f.tenantA: string(domain.RoleUser)}, + }, nil + }) + orgRoles, businessRoles := domain.DefaultMFAPrivilegeRoles() + + blacklist := authpkg.NewTokenBlacklistManager(&memoryBlacklist{keys: map[string]string{}}) + checker := blacklist.CheckJTIBlacklist(context.Background()) + + loginUC := appauth.NewLoginUseCase(userRepo, tokens, deferredHasher{}). + WithMFA(mfaRepo). + RequireMFAForRoles(orgRoles, businessRoles). + WithMFAPolicies(f.policyRepo). + WithClock(func() time.Time { return f.now }) + logoutUC := appauth.NewLogoutUseCase(tokens).WithAccessTokenRevocation(blacklist) + h := authhandler.NewHandler(loginUC, nil, nil, logoutUC, deferredHasher{}, nil). + WithUserLookup(userRepo). + WithMFAStatus(f.resolver). + WithAccessTokenRevocation(f.keys) + + challengeUC := appauth.NewChallengeMFAUseCase(mfaRepo, deferredTOTPKey). + WithAttemptLimits(authmfa.NewMemoryAttemptStore(), blacklist) + mfaHandler := authhandler.NewMFAHandler(nil, nil, nil, challengeUC, tokens, userRepo, nil) + + app := fiber.New() + api := app.Group("/api/v1") + api.Post("/auth/login", h.Login) + api.Post("/auth/logout", h.Logout) + ipLimiter := middleware.RateLimit(middleware.RateLimitConfig{ + MaxRequests: ipLimit, + WindowSize: 5 * time.Minute, + Store: middleware.NewRateLimitStore(), + }) + api.Post("/auth/mfa/challenge", ipLimiter, middleware.MFATokenMiddleware(f.keys, checker), mfaHandler.Challenge) + protected := api.Use(middleware.Protected(f.keys, checker)) + protected.Get("/auth/me", h.Me) + + f.app = app + return &challengeStack{deferredFixture: f, ipLimit: ipLimit} +} + +// enrolWithSecret gives a member a working authenticator and returns its seed. +func (s *challengeStack) enrolWithSecret(t *testing.T, m *domain.OrganizationMember) string { + t.Helper() + plain, err := otp.GenerateTOTPSecret() + require.NoError(t, err) + enc, err := crypto.EncryptAES256GCM(plain, deferredTOTPKey) + require.NoError(t, err) + require.NoError(t, s.db.Create(&domain.MFASecret{ + ID: uuid.New(), UserID: m.UserID, TenantID: m.OrganizationID, SecretEncrypted: enc, IsVerified: true, + }).Error) + return plain +} + +// mfaToken signs in with the password and returns the MFA_REQUIRED token. +func (s *challengeStack) mfaToken(t *testing.T, email string) string { + t.Helper() + body, _ := s.login(t, email) + tok, _ := body["mfa_token"].(string) + require.NotEmpty(t, tok, "the login must stop at the code step: %v", body) + return tok +} + +func wrongCode(t *testing.T, plain string, seed int) string { + t.Helper() + for i := seed; ; i++ { + c := fmt.Sprintf("%06d", (i*104729+7)%1000000) + if !otp.VerifyTOTP(plain, c) { + return c + } + } +} + +func liveCode(t *testing.T, plain string) string { + t.Helper() + c, err := totp.GenerateCode(plain, time.Now()) + require.NoError(t, err) + return c +} + +func TestMFAChallengeE2E_CorrectCodeOpensASession(t *testing.T) { + s := newChallengeStack(t, 100) + plain := s.enrolWithSecret(t, s.memberA) + + status, body := s.do(t, http.MethodPost, challengePath, s.mfaToken(t, s.memberA.User.Email), jsonBody{"code": liveCode(t, plain)}) + require.Equal(t, http.StatusOK, status, "%v", body) + assert.NotNil(t, body["token_pair"]) +} + +func TestMFAChallengeE2E_TokenIsRevokedAfterFiveWrongCodes(t *testing.T) { + s := newChallengeStack(t, 100) + plain := s.enrolWithSecret(t, s.memberA) + tok := s.mfaToken(t, s.memberA.User.Email) + + for i := 1; i <= 4; i++ { + status, _ := s.do(t, http.MethodPost, challengePath, tok, jsonBody{"code": wrongCode(t, plain, i)}) + require.Equal(t, http.StatusBadRequest, status, "wrong code %d", i) + } + status, body := s.do(t, http.MethodPost, challengePath, tok, jsonBody{"code": wrongCode(t, plain, 40)}) + require.Equal(t, http.StatusUnauthorized, status) + assert.Equal(t, "MFA_CHALLENGE_EXHAUSTED", body["code"]) + + // Sixth attempt, with the right code: the middleware refuses the token + // before the handler reads anything. + status, body = s.do(t, http.MethodPost, challengePath, tok, jsonBody{"code": liveCode(t, plain)}) + require.Equal(t, http.StatusUnauthorized, status) + assert.Equal(t, "TOKEN_REVOKED", body["code"]) +} + +func TestMFAChallengeE2E_AccountLocksAcrossTokensWithRetryAfter(t *testing.T) { + s := newChallengeStack(t, 100) + plain := s.enrolWithSecret(t, s.memberA) + + n := 0 + for range 2 { + tok := s.mfaToken(t, s.memberA.User.Email) + for range 4 { + n++ + status, _ := s.do(t, http.MethodPost, challengePath, tok, jsonBody{"code": wrongCode(t, plain, n)}) + require.Equal(t, http.StatusBadRequest, status, "failure %d", n) + } + } + tok := s.mfaToken(t, s.memberA.User.Email) + status, _ := s.do(t, http.MethodPost, challengePath, tok, jsonBody{"code": wrongCode(t, plain, 90)}) + require.Equal(t, http.StatusBadRequest, status, "failure 9") + + resp, body := s.raw(t, tok, wrongCode(t, plain, 91)) + require.Equal(t, http.StatusTooManyRequests, resp.StatusCode, "the tenth failure locks: %v", body) + assert.Equal(t, "MFA_LOCKED", body["code"]) + assert.Equal(t, "900", resp.Header.Get("Retry-After")) + + // A fresh login and the right code: still refused while the lock holds. + resp, body = s.raw(t, s.mfaToken(t, s.memberA.User.Email), liveCode(t, plain)) + require.Equal(t, http.StatusTooManyRequests, resp.StatusCode) + assert.Equal(t, "MFA_LOCKED", body["code"]) + assert.NotEmpty(t, resp.Header.Get("Retry-After")) +} + +func TestMFAChallengeE2E_IPLimitRunsBeforeTheToken(t *testing.T) { + s := newChallengeStack(t, 3) + for i := range 3 { + status, _ := s.do(t, http.MethodPost, challengePath, "not-a-token", jsonBody{"code": "123456"}) + require.Equal(t, http.StatusUnauthorized, status, "request %d reaches the token check", i+1) + } + status, _ := s.do(t, http.MethodPost, challengePath, "not-a-token", jsonBody{"code": "123456"}) + assert.Equal(t, http.StatusTooManyRequests, status, "the fourth is refused before the token is parsed") +} + +// raw posts a code and keeps the response headers. +func (s *challengeStack) raw(t *testing.T, token, code string) (*http.Response, jsonBody) { + t.Helper() + raw, err := json.Marshal(jsonBody{"code": code}) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPost, challengePath, strings.NewReader(string(raw))) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+token) + resp, err := s.app.Test(req, -1) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + out := jsonBody{} + _ = json.NewDecoder(resp.Body).Decode(&out) + return resp, out +} diff --git a/backend/internal/handler/auth/mfa_handler.go b/backend/internal/handler/auth/mfa_handler.go index 9f371eb4..b01719e6 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" + "strconv" "time" "github.com/gofiber/fiber/v2" @@ -17,6 +18,7 @@ import ( "github.com/opendefender/openrisk/internal/domain" "github.com/opendefender/openrisk/internal/infrastructure/repository" "github.com/opendefender/openrisk/internal/middleware" + authpkg "github.com/opendefender/openrisk/pkg/auth" ) // MFAHandler exposes the /auth/mfa/* endpoints: setup + verify (enrollment, under @@ -296,12 +298,17 @@ func (h *MFAHandler) Challenge(c *fiber.Ctx) error { return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid request body"}) } - if _, err := h.challenge.Execute(c.UserContext(), appauth.ChallengeMFAInput{UserID: userID, TenantID: tenantID, Code: req.Code}); err != nil { - if h.audit != nil { - reason := "invalid MFA code" - _ = h.audit.LogFiber(c, &userID, &tenantID, coreauth.AuditActionMfaVerify, false, &reason) - } - return mapAuthError(c, err) + input := appauth.ChallengeMFAInput{UserID: userID, TenantID: tenantID, Code: req.Code} + // The token's own identity, so attempts are counted against it (#689). + if jti, ok := c.Locals("jti").(string); ok { + input.ChallengeJTI = jti + } + if claims, ok := c.Locals("user").(*authpkg.Claims); ok && claims.ExpiresAt != nil { + input.ChallengeExpiresAt = claims.ExpiresAt.Time + } + + if _, err := h.challenge.Execute(c.UserContext(), input); err != nil { + return h.challengeFailed(c, userID, tenantID, err) } fp := req.DeviceFingerprint @@ -336,6 +343,56 @@ func (h *MFAHandler) Challenge(c *fiber.Ctx) error { return c.JSON(LoginResponse{TokenPair: pair, CSRFToken: csrfToken}) } +// challengeFailed answers a refused challenge and audits it. The codes let the +// sign-in screen tell "wrong code" from "start again" from "wait". +func (h *MFAHandler) challengeFailed(c *fiber.Ctx, userID, tenantID uuid.UUID, err error) error { + locale := resolveLocale(c, "") + audit := func(reason string) { + if h.audit != nil { + _ = h.audit.LogFiber(c, &userID, &tenantID, coreauth.AuditActionMfaVerify, false, &reason) + } + } + + var locked *appauth.MFAChallengeLockedError + var appErr *domain.AppError + switch { + case errors.As(err, &locked): + // Audited once, when the lock is set. The refusals that follow are the + // lock working, and would drown the event that matters. + if locked.JustLocked { + audit("mfa_locked") + } + seconds := int(locked.RetryAfter.Round(time.Second) / time.Second) + if seconds < 1 { + seconds = 1 + } + c.Set(fiber.HeaderRetryAfter, strconv.Itoa(seconds)) + return c.Status(fiber.StatusTooManyRequests).JSON(fiber.Map{ + "code": "MFA_LOCKED", + "retry_after": seconds, + "error": pick(locale, + "Trop de codes erronés. La connexion à deux facteurs est suspendue pour ce compte, réessayez plus tard.", + "Too many wrong codes. Two-factor sign-in is paused for this account, try again later."), + }) + case errors.Is(err, appauth.ErrMFAChallengeExhausted): + audit("mfa_challenge_exhausted") + return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ + "code": "MFA_CHALLENGE_EXHAUSTED", + "error": pick(locale, + "Trop de codes erronés pour cette connexion. Reconnectez-vous avec votre mot de passe.", + "Too many wrong codes for this sign-in. Sign in again with your password."), + }) + case errors.As(err, &appErr): + audit("invalid MFA code") + return mapAuthError(c, err) + default: + // A store or decryption failure: nothing the caller can fix, and its + // text is not theirs to read. + audit("internal") + return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{"error": genericFailure(locale)}) + } +} + // mapAuthError maps typed domain errors to HTTP status codes. func mapAuthError(c *fiber.Ctx, err error) error { if appErr, ok := err.(*domain.AppError); ok { diff --git a/backend/internal/infrastructure/authmail/async.go b/backend/internal/infrastructure/authmail/async.go index 97a3a926..e363886e 100644 --- a/backend/internal/infrastructure/authmail/async.go +++ b/backend/internal/infrastructure/authmail/async.go @@ -34,6 +34,7 @@ type ResetMailerLike interface { SendNewSignInAlert(ctx context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error SendPasswordChanged(ctx context.Context, to, fullName, locale string) error SendMFADisabled(ctx context.Context, to, fullName, locale string) error + SendMFALocked(ctx context.Context, to, fullName, locale string) error } // NewAsync wraps a mailer so every send returns immediately. @@ -65,6 +66,12 @@ func (a *Async) SendMFADisabled(_ context.Context, to, fullName, locale string) return nil } +// SendMFALocked queues the second-factor lock notice (#689). +func (a *Async) SendMFALocked(_ context.Context, to, fullName, locale string) error { + a.dispatch(func(ctx context.Context) { _ = a.inner.SendMFALocked(ctx, to, fullName, locale) }) + return nil +} + // SendNewSignInAlert queues the new-device notice. func (a *Async) SendNewSignInAlert(_ context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error { a.dispatch(func(ctx context.Context) { diff --git a/backend/internal/infrastructure/authmail/mfa_locked_mailer_test.go b/backend/internal/infrastructure/authmail/mfa_locked_mailer_test.go new file mode 100644 index 00000000..fde886b0 --- /dev/null +++ b/backend/internal/infrastructure/authmail/mfa_locked_mailer_test.go @@ -0,0 +1,46 @@ +// 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 authmail + +import ( + "context" + "strings" + "testing" +) + +func TestSendMFALocked_SaysWhatHappenedInBothLanguages(t *testing.T) { + cases := []struct { + locale, subject, mustSay string + }{ + {"en", "two-factor sign-in", "reset your password"}, + {"fr", "connexion à deux facteurs", "réinitialisez votre mot de passe"}, + } + for _, tc := range cases { + t.Run(tc.locale, func(t *testing.T) { + s := &capturingSender{} + if err := New(s).SendMFALocked(context.Background(), "awa@example.test", "Awa Diallo", tc.locale); err != nil { + t.Fatal(err) + } + if s.to != "awa@example.test" { + t.Fatalf("sent to %q", s.to) + } + if !strings.Contains(s.subject, tc.subject) { + t.Fatalf("subject %q lacks %q", s.subject, tc.subject) + } + for _, want := range []string{"Awa Diallo", "15 minutes", tc.mustSay} { + if !strings.Contains(s.body, want) { + t.Fatalf("body lacks %q", want) + } + } + }) + } +} + +func TestSendMFALocked_NoSenderIsANoOp(t *testing.T) { + if err := New(nil).SendMFALocked(context.Background(), "a@example.test", "", "en"); err != nil { + t.Fatalf("an unwired mailer must not fail the challenge: %v", err) + } +} diff --git a/backend/internal/infrastructure/authmail/reset_mailer.go b/backend/internal/infrastructure/authmail/reset_mailer.go index ce4eff9a..e793e81f 100644 --- a/backend/internal/infrastructure/authmail/reset_mailer.go +++ b/backend/internal/infrastructure/authmail/reset_mailer.go @@ -83,6 +83,17 @@ func (m *Mailer) SendMFADisabled(ctx context.Context, to, fullName, locale strin return m.sender.SendEmail(ctx, to, c.subject, m.render(c)) } +// SendMFALocked tells the owner that two-factor sign-in was locked after +// repeated wrong codes (#689). Reaching the code step takes the password, so +// the message says so plainly: the remedy is a password reset. +func (m *Mailer) SendMFALocked(ctx context.Context, to, fullName, locale string) error { + if m == nil || m.sender == nil { + return nil + } + c := mfaLockedCopy(locale, displayName(fullName, locale)) + return m.sender.SendEmail(ctx, to, c.subject, m.render(c)) +} + // SendNewSignInAlert warns about a sign-in from an unrecognised device. func (m *Mailer) SendNewSignInAlert(ctx context.Context, to, fullName, ip, userAgent string, when time.Time, locale string) error { if m == nil || m.sender == nil { @@ -206,6 +217,32 @@ func mfaDisabledCopy(locale, name string) copyBlock { } } +func mfaLockedCopy(locale, name string) copyBlock { + minutes := 15 // mirrors application/auth.MFAChallengeLockDuration + if locale == "en" { + return copyBlock{ + subject: "Too many wrong codes: two-factor sign-in to your OpenRisk account is paused", + heading: "Two-factor sign-in is paused", + paragraphs: []string{ + fmt.Sprintf("Hello %s,", name), + fmt.Sprintf("Someone entered your password correctly, then a wrong two-factor code too many times. To stop them guessing, OpenRisk refuses codes for this account for %d minutes. Your account was not opened.", minutes), + "Getting to the code step takes your password. If these attempts were not yours, someone else knows it.", + }, + footnote: "If this wasn't you, reset your password now and tell your OpenRisk administrator. If it was you, wait a few minutes and sign in again.", + } + } + return copyBlock{ + subject: "Trop de codes erronés : la connexion à deux facteurs de votre compte OpenRisk est suspendue", + heading: "La connexion à deux facteurs est suspendue", + paragraphs: []string{ + fmt.Sprintf("Bonjour %s,", name), + fmt.Sprintf("Quelqu'un a saisi votre mot de passe correctement, puis un code de double authentification erroné trop de fois. Pour l'empêcher de deviner, OpenRisk refuse les codes pour ce compte pendant %d minutes. Votre compte n'a pas été ouvert.", minutes), + "Pour arriver à l'étape du code, il faut votre mot de passe. Si ces tentatives ne viennent pas de vous, quelqu'un d'autre le connaît.", + }, + footnote: "Si ce n'était pas vous, réinitialisez votre mot de passe dès maintenant et prévenez votre administrateur OpenRisk. Si c'était vous, patientez quelques minutes et reconnectez-vous.", + } +} + func newSignInCopy(locale, name, ip, userAgent string, when time.Time) copyBlock { stamp := when.UTC().Format("2006-01-02 15:04 UTC") if locale == "en" { diff --git a/backend/internal/infrastructure/authmfa/attempts.go b/backend/internal/infrastructure/authmfa/attempts.go new file mode 100644 index 00000000..7026a668 --- /dev/null +++ b/backend/internal/infrastructure/authmfa/attempts.go @@ -0,0 +1,192 @@ +// 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 authmfa + +import ( + "context" + "strconv" + "sync" + "time" +) + +// attemptRedis is the slice of the Redis wrapper the store needs. Declared here +// so this package does not import the wrapper and tests can drive failures. +type attemptRedis interface { + Incr(ctx context.Context, key string) (int64, error) + Expire(ctx context.Context, key string, ttl time.Duration) error + Get(ctx context.Context, key string) (string, error) + SetNX(ctx context.Context, key, value string, ttl time.Duration) (bool, error) + Del(ctx context.Context, keys ...string) error +} + +// AttemptStore implements application/auth.MFAAttemptStore on Redis, so the +// MFA challenge counters hold across every instance (#689). +// +// When Redis errors, each operation falls back to a per-instance memory store. +// The limits then hold per instance instead of globally. That is weaker, but it +// never fails open, which is what an outage must not do to a brute-force guard. +type AttemptStore struct { + redis attemptRedis + fallback *MemoryAttemptStore +} + +// NewAttemptStore wraps a Redis client. +func NewAttemptStore(redis attemptRedis) *AttemptStore { + return &AttemptStore{redis: redis, fallback: NewMemoryAttemptStore()} +} + +const attemptKeyPrefix = "mfa-attempts:" + +func (s *AttemptStore) Increment(ctx context.Context, key string, ttl time.Duration) (int64, error) { + k := attemptKeyPrefix + key + n, err := s.redis.Incr(ctx, k) + if err != nil { + return s.fallback.Increment(ctx, key, ttl) + } + if n == 1 { + // Only the first increment sets the deadline, so the window is fixed + // from the first failure. A key left without a TTL by a failed Expire + // would count forever; drop it instead and let the memory store count. + if err := s.redis.Expire(ctx, k, ttl); err != nil { + _ = s.redis.Del(ctx, k) + return s.fallback.Increment(ctx, key, ttl) + } + } + return n, nil +} + +func (s *AttemptStore) Count(ctx context.Context, key string) (int64, error) { + v, err := s.redis.Get(ctx, attemptKeyPrefix+key) + if err != nil { + return s.fallback.Count(ctx, key) + } + if v == "" { + // Redis may have been down when the failures were counted. + return s.fallback.Count(ctx, key) + } + n, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return 0, nil + } + return n, nil +} + +// Lock stores the unlock time as the value, so LockedFor can answer without a +// TTL command the wrapper does not expose. +func (s *AttemptStore) Lock(ctx context.Context, key string, ttl time.Duration) (bool, error) { + until := time.Now().Add(ttl).UnixNano() + set, err := s.redis.SetNX(ctx, attemptKeyPrefix+key, strconv.FormatInt(until, 10), ttl) + if err != nil { + return s.fallback.Lock(ctx, key, ttl) + } + return set, nil +} + +func (s *AttemptStore) LockedFor(ctx context.Context, key string) (time.Duration, error) { + v, err := s.redis.Get(ctx, attemptKeyPrefix+key) + if err != nil || v == "" { + return s.fallback.LockedFor(ctx, key) + } + until, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return 0, nil + } + return remaining(time.Unix(0, until)), nil +} + +func (s *AttemptStore) Delete(ctx context.Context, key string) error { + _ = s.fallback.Delete(ctx, key) + // If Redis is down the copy there survives, but it expires on its own. + _ = s.redis.Del(ctx, attemptKeyPrefix+key) + return nil +} + +func remaining(until time.Time) time.Duration { + d := time.Until(until) + if d <= 0 { + return 0 + } + // Round up so a caller told "retry in 0s" is never told that too early. + return d.Truncate(time.Second) + time.Second +} + +// MemoryAttemptStore is the per-instance store. It backs AttemptStore when +// Redis is unreachable and serves tests. +type MemoryAttemptStore struct { + mu sync.Mutex + entries map[string]memoryEntry + now func() time.Time +} + +type memoryEntry struct { + count int64 + expires time.Time +} + +// NewMemoryAttemptStore returns an empty store. +func NewMemoryAttemptStore() *MemoryAttemptStore { + return &MemoryAttemptStore{entries: map[string]memoryEntry{}, now: time.Now} +} + +// live returns the entry for key if it has not expired. Caller holds mu. +func (m *MemoryAttemptStore) live(key string) (memoryEntry, bool) { + e, ok := m.entries[key] + if !ok { + return memoryEntry{}, false + } + if !m.now().Before(e.expires) { + delete(m.entries, key) + return memoryEntry{}, false + } + return e, true +} + +func (m *MemoryAttemptStore) Increment(_ context.Context, key string, ttl time.Duration) (int64, error) { + m.mu.Lock() + defer m.mu.Unlock() + e, ok := m.live(key) + if !ok { + e = memoryEntry{expires: m.now().Add(ttl)} + } + e.count++ + m.entries[key] = e + return e.count, nil +} + +func (m *MemoryAttemptStore) Count(_ context.Context, key string) (int64, error) { + m.mu.Lock() + defer m.mu.Unlock() + e, _ := m.live(key) + return e.count, nil +} + +func (m *MemoryAttemptStore) Lock(_ context.Context, key string, ttl time.Duration) (bool, error) { + m.mu.Lock() + defer m.mu.Unlock() + if _, ok := m.live(key); ok { + return false, nil + } + m.entries[key] = memoryEntry{count: 1, expires: m.now().Add(ttl)} + return true, nil +} + +func (m *MemoryAttemptStore) LockedFor(_ context.Context, key string) (time.Duration, error) { + m.mu.Lock() + defer m.mu.Unlock() + e, ok := m.live(key) + if !ok { + return 0, nil + } + d := e.expires.Sub(m.now()) + return d.Truncate(time.Second) + time.Second, nil +} + +func (m *MemoryAttemptStore) Delete(_ context.Context, key string) error { + m.mu.Lock() + defer m.mu.Unlock() + delete(m.entries, key) + return nil +} diff --git a/backend/internal/infrastructure/authmfa/attempts_test.go b/backend/internal/infrastructure/authmfa/attempts_test.go new file mode 100644 index 00000000..51e94e68 --- /dev/null +++ b/backend/internal/infrastructure/authmfa/attempts_test.go @@ -0,0 +1,182 @@ +// 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 authmfa + +import ( + "context" + "errors" + "strconv" + "sync" + "testing" + "time" + + appauth "github.com/opendefender/openrisk/internal/application/auth" +) + +var ( + _ appauth.MFAAttemptStore = (*AttemptStore)(nil) + _ appauth.MFAAttemptStore = (*MemoryAttemptStore)(nil) +) + +// fakeRedis is a minimal in-process Redis with switchable failure. +type fakeRedis struct { + mu sync.Mutex + vals map[string]string + ttls map[string]time.Duration + down bool + expires int +} + +func newFakeRedis() *fakeRedis { + return &fakeRedis{vals: map[string]string{}, ttls: map[string]time.Duration{}} +} + +var errDown = errors.New("redis down") + +func (f *fakeRedis) Incr(_ context.Context, key string) (int64, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.down { + return 0, errDown + } + n, _ := strconv.ParseInt(f.vals[key], 10, 64) + n++ + f.vals[key] = strconv.FormatInt(n, 10) + return n, nil +} + +func (f *fakeRedis) Expire(_ context.Context, key string, ttl time.Duration) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.down { + return errDown + } + f.expires++ + f.ttls[key] = ttl + return nil +} + +func (f *fakeRedis) Get(_ context.Context, key string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.down { + return "", errDown + } + return f.vals[key], nil +} + +func (f *fakeRedis) SetNX(_ context.Context, key, value string, ttl time.Duration) (bool, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.down { + return false, errDown + } + if _, ok := f.vals[key]; ok { + return false, nil + } + f.vals[key] = value + f.ttls[key] = ttl + return true, nil +} + +func (f *fakeRedis) Del(_ context.Context, keys ...string) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.down { + return errDown + } + for _, k := range keys { + delete(f.vals, k) + delete(f.ttls, k) + } + return nil +} + +func TestAttemptStore_CountsAndSetsTheWindowOnce(t *testing.T) { + ctx := context.Background() + r := newFakeRedis() + s := NewAttemptStore(r) + + for want := int64(1); want <= 3; want++ { + n, err := s.Increment(ctx, "k", time.Minute) + if err != nil || n != want { + t.Fatalf("increment %d: got %d, %v", want, n, err) + } + } + if r.expires != 1 { + t.Fatalf("the window must be set on the first failure only, got %d Expire calls", r.expires) + } + if got, _ := s.Count(ctx, "k"); got != 3 { + t.Fatalf("count: got %d, want 3", got) + } + _ = s.Delete(ctx, "k") + if got, _ := s.Count(ctx, "k"); got != 0 { + t.Fatalf("count after delete: got %d, want 0", got) + } +} + +func TestAttemptStore_LockIsSetOnceAndReportsWhatRemains(t *testing.T) { + ctx := context.Background() + s := NewAttemptStore(newFakeRedis()) + + set, err := s.Lock(ctx, "lock", 15*time.Minute) + if err != nil || !set { + t.Fatalf("first lock: set=%v err=%v", set, err) + } + if set, _ := s.Lock(ctx, "lock", 15*time.Minute); set { + t.Fatal("a second Lock while the first holds must report false") + } + d, _ := s.LockedFor(ctx, "lock") + if d <= 14*time.Minute || d > 15*time.Minute+time.Second { + t.Fatalf("LockedFor: got %v, want about 15m", d) + } + if d, _ := s.LockedFor(ctx, "other"); d != 0 { + t.Fatalf("an absent lock must read 0, got %v", d) + } +} + +func TestAttemptStore_FallsBackToMemoryWhenRedisIsDown(t *testing.T) { + ctx := context.Background() + r := newFakeRedis() + r.down = true + s := NewAttemptStore(r) + + for range 5 { + if _, err := s.Increment(ctx, "k", time.Minute); err != nil { + t.Fatalf("increment must not fail when Redis is down: %v", err) + } + } + if got, _ := s.Count(ctx, "k"); got != 5 { + t.Fatalf("fallback must keep counting, got %d", got) + } + if set, _ := s.Lock(ctx, "lock", time.Minute); !set { + t.Fatal("fallback must lock") + } + if d, _ := s.LockedFor(ctx, "lock"); d == 0 { + t.Fatal("fallback lock must hold") + } +} + +func TestMemoryAttemptStore_ExpiresEntries(t *testing.T) { + ctx := context.Background() + m := NewMemoryAttemptStore() + now := time.Now() + m.now = func() time.Time { return now } + + _, _ = m.Increment(ctx, "k", time.Minute) + _, _ = m.Lock(ctx, "lock", time.Minute) + + now = now.Add(time.Minute) + if got, _ := m.Count(ctx, "k"); got != 0 { + t.Fatalf("count after the window: got %d, want 0", got) + } + if d, _ := m.LockedFor(ctx, "lock"); d != 0 { + t.Fatalf("lock after its ttl: got %v, want 0", d) + } + if n, _ := m.Increment(ctx, "k", time.Minute); n != 1 { + t.Fatalf("a new window starts at 1, got %d", n) + } +} diff --git a/docs/689_MFA_CHALLENGE_LIMITS.md b/docs/689_MFA_CHALLENGE_LIMITS.md new file mode 100644 index 00000000..23238a24 --- /dev/null +++ b/docs/689_MFA_CHALLENGE_LIMITS.md @@ -0,0 +1,196 @@ +# Spec — #689 MFA challenge limits and logout revocation + +Status: approved by owner 2026-10-02 (limits kept · lock email yes · frontend copy in a follow-up issue) · Branch: `689-securityauth-mfa-challenge-codes-can-be-guessed-without-limit-no-attempt-counter-no-rate-limit` · Milestone: trust-v1 + +## Objective + +Someone who holds a user's password can currently submit TOTP or backup codes to +`POST /api/v1/auth/mfa/challenge` without limit: no attempt counter, no rate +limit, and a challenge token reusable for 5 minutes. A second factor that can be +guessed is not a second factor. Separately, `POST /auth/logout` leaves the access +token valid for up to 15 minutes. + +Success means: + +- a challenge token dies after 5 wrong codes; +- an account stops accepting challenges for a bounded time after repeated failures, whatever the token or source IP; +- the route has its own per-IP bucket; +- backup codes count the same as TOTP codes; +- a logged-out access token is refused on the next request. + +## Current state (read 2026-10-02) + +| Piece | File | Fact | +|---|---|---| +| Route | `backend/cmd/server/main.go:898` | `MFATokenMiddleware` only, no limiter | +| Handler | `backend/internal/handler/auth/mfa_handler.go` `Challenge` | audits each failure, nothing else | +| Use case | `backend/internal/application/auth/mfa_usecase.go` `ChallengeMFAUseCase.Execute` | TOTP, then bcrypt over unused backup codes; returns `ErrValidation` on a miss | +| Challenge token | `backend/internal/auth/token.go` | `MFAChallengeTTL = 5m`, carries a JTI; middleware sets `c.Locals("jti")` and `c.Locals("user")` (claims, with `ExpiresAt`) | +| Blacklist | `backend/pkg/auth/blacklist.go` | `BlacklistJTI(ctx, jti, ttl)`; read by every token middleware; fail-open if Redis is down | +| Shared counters | `backend/internal/middleware/ratelimit.go`, `backend/internal/infrastructure/redis/client.go` | `AllowRate` is a **fixed, wall-clock-aligned** window. A 5-minute token can straddle two buckets, so it cannot give an exact "5 per token" | +| Precedent | #754, `WithDisableAttemptLimit` | per-account budget on `/auth/mfa/disable` via the shared store | +| Logout | `backend/internal/handler/auth/handler.go:480`, `application/auth/logout.go` | revokes the refresh token and clears cookies; the access token is never looked at | + +## Design + +### 1. Per-token limit (AC 1) + +- Key `mfa:challenge:token:`. It counts **failed** codes only and expires with the token's remaining lifetime. +- Before verifying, the use case checks the count. At 5 or more it refuses without touching TOTP or bcrypt. +- The 5th failure blacklists the JTI for `claims.RemainingTTL()`. From then on the middleware itself answers `401 TOKEN_REVOKED`. +- The 5th failure answers `401 {"code":"MFA_CHALLENGE_EXHAUSTED"}`, so the screen can say "sign in again". +- The counter check is what enforces the limit. The blacklist is a second barrier. So a Redis outage, where the blacklist fails open, still leaves the per-instance counter refusing the token. + +### 2. Per-account limit (AC 2) + +- Key `mfa:challenge:user:`. It counts failures across all tokens and IPs, over a 15-minute window. +- At **10 failures** the lock key `mfa:challenge:lock:` is set for **15 minutes**. + - While the lock is set, every challenge for that user answers `429 {"code":"MFA_LOCKED"}` with `Retry-After`. The code is not evaluated. + - The lock is not extended by attempts made while it holds, so it is bounded. +- Setting the lock is audited once, as `mfa_verify` with success=false and reason `mfa_locked`. Reusing the existing action avoids a migration. +- Setting the lock also emails the account owner, once per lock, in their locale (FR/EN). The email says that someone passed their password and then failed the second factor repeatedly, and it recommends a password reset. It reuses the async `securityMailer` (`internal/infrastructure/authmail`) through a new `MFALockedMailer { SendMFALocked(ctx, to, fullName, locale string) error }` port, on the model of `MFADisabledMailer`. A mail failure never changes the response. +- A successful challenge clears the user's failure counter. It does not clear an active lock. +- The key is the user, not (user, tenant). The lock covers the identity across organisations, which is the stricter choice. No DB query is added, so the tenant-filter rule is not engaged. + +### 3. Per-IP limit (AC 3) + +The existing `middleware.RateLimit` runs over `authLimiterStore`, wrapped in `handlers.PrefixedRateLimitBackend{Prefix: "mfa-challenge"}`: + +- **20 requests per 5 minutes per IP**, its own bucket, separate from login's; +- mounted **before** `MFATokenMiddleware`, so a flood never reaches token validation. + +This is the pattern #688 asks for. #688 itself (login, sign-up and reset buckets) is not touched. + +### 4. Backup codes (AC 4) + +Already satisfied structurally: one `Execute` call is one attempt, whether the input matched as TOTP, as a backup code, or as neither. The tests prove it explicitly. + +### 5. Logout revokes the access token (AC 5) + +- `Logout` reads the access token from `Authorization: Bearer` or the `or_access` cookie, the same order as `extractAccessToken`. +- It validates the token's signature and expiry with the server's RSA keys. An invalid or expired token is ignored: the response is still 200 and the cookies are still cleared, as today. +- It then blacklists the JTI for the token's remaining TTL. +- This happens whether or not a refresh token was supplied. +- The revocation lives in `LogoutUseCase`, through a `TokenRevoker` port. The handler only extracts the JTI and expiry. + +### Layering + +- **`application/auth` port:** + `MFAAttemptStore { Increment(ctx, key, ttl) (int64, error); Exists(ctx, key) (bool, error); Set(ctx, key, ttl) error; Delete(ctx, key) error }` + It sits next to `TokenRevoker { BlacklistJTI(ctx, jti, ttl) error }`, which `*authpkg.TokenBlacklistManager` already satisfies. +- **`infrastructure/authmfa/attempts.go`:** a Redis implementation (`INCR`, plus `EXPIRE` on the first increment) with an in-memory fallback, mirroring `RedisRateLimitStore`. A Redis outage degrades to per-instance limits and never fails open. +- **`ChallengeMFAUseCase.WithAttemptLimits(store, revoker)`:** + - `ChallengeMFAInput` gains `ChallengeJTI` and `ChallengeExpiresAt`; + - new sentinel errors `ErrMFAChallengeExhausted` and `ErrMFAChallengeLocked` (the latter carries the retry duration), following the `ErrMFADisable*` precedent; + - unwired means today's behaviour, so the existing tests are unaffected. +- **Handler:** `Challenge` maps the two errors to 401 and 429. `Logout` gains `WithAccessTokenRevocation(rsaKeys)`. +- **`main.go`:** wiring only. + +## Commands + +``` +cd backend && go build ./... && go vet ./... && go test ./... -race +cd backend && go test ./internal/application/auth/ ./internal/handler/auth/ ./internal/infrastructure/authmfa/ -run 'MFAChallenge|Logout' -race -v +``` +Live check: throwaway pg + redis on spare ports, run the server from the repo root (live-boot recipe), then curl through login → challenge. + +## Testing strategy + +**Use case** (`application/auth/mfa_challenge_limits_test.go`), with a fake store and a fake revoker: + +- `TestMFAChallenge_TokenDiesAfterFiveFailures` +- `TestMFAChallenge_UserBackoffAcrossTokens` +- `TestMFAChallenge_BackupCodesCounted` +- `TestMFAChallenge_Success` +- `TestMFAChallenge_NotFound` +- `TestMFAChallenge_Unauthorized` (a locked account is refused even with the right code) +- `TestMFAChallenge_SuccessClearsFailureCount` +- `TestMFAChallenge_LockSendsOneEmail` (one email per lock, not per refused attempt) + +**Store** (`infrastructure/authmfa/attempts_test.go`): counting, expiry, and fallback when Redis errors. + +**Handler e2e** (`handler/auth/mfa_challenge_e2e_test.go`), through real middleware with a real RSA key and an in-memory blacklist: + +- the 6th request with the same token gets 401 `TOKEN_REVOKED`; +- a lock gets 429 with `Retry-After`; +- the IP limit gets 429 before token validation. + +**Logout:** + +- `TestLogout_AccessTokenRejectedAfterLogout`: log in, call logout with the bearer token, then `/auth/me` with the same token gets 401 `TOKEN_REVOKED`; +- a cookie-only logout gets the same result. + +**Live:** criteria 1, 2 and 5 against a local instance with an enrolled account. The output goes in the issue comment. + +## Boundaries + +- **Always:** + - typed errors; + - no code and no token in logs or audit, only reason codes; + - the full `go test ./...` before the PR (route changes trip the authz registry tests). +- **Ask first:** thresholds other than those below, a new audit action (that means a migration). +- **Never:** + - touch the login, sign-up or reset buckets (#688); + - change the TOTP tolerance window; + - make the blacklist fail-closed globally. That is a design change to auth and goes to escalation. + +## Success criteria + +1. A 6th call with a token that has had 5 wrong codes answers 401, and the JTI is in the blacklist. +2. 10 failures over two or more tokens lock the account. During the lock a correct code answers 429 with `Retry-After`, the lock audit row exists, and after the lock expires a correct code succeeds. +3. 21 challenge requests from one IP within 5 minutes: the 21st answers 429, and `/auth/login` from that IP is still allowed. +4. Wrong backup codes increment the same counters as wrong TOTP codes. +5. After logout, the old access token answers 401 on `/auth/me`. +6. The four named tests, plus Success/NotFound/Unauthorized, pass under `-race`. The full build, vet and test run is green. + +## Known limits (stated, not hidden) + +- **Deliberate lockout:** an attacker who already holds the password can re-trigger the 15-minute lock every 15 minutes. That denies the owner, but grants the attacker nothing. The remedy is a password reset, which kills the attacker's first factor. The owner is emailed on each lock, so they learn the password is known. +- **Redis down:** + - the limits become per-instance; + - the JTI blacklist fails open, which is existing behaviour, so a logged-out access token stays valid until it expires; + - the per-token counter still kills challenge tokens on the instance that counted them. + +## Decisions (owner, 2026-10-02) + +1. Limits kept: 5 per token · 10 failures / 15 min → 15-min lock per account · 20 per 5 min per IP. +2. Email the owner when a lock is set: yes. +3. Frontend copy for `MFA_CHALLENGE_EXHAUSTED` and `MFA_LOCKED`: a separate follow-up issue. This PR stays backend. + +## Plan — tasks in dependency order + +Each task ends green on its own `go test` and is one commit (`fix(auth): … (#689)`). + +- [x] **T1 — Attempt store.** Adds the `MFAAttemptStore` port in `application/auth`, and `infrastructure/authmfa/attempts.go`: Redis (`INCR` + `EXPIRE` on the first increment, `SET` with TTL, `EXISTS`, `DEL`) with an in-memory fallback. + - Accept: counts, expires, falls back when Redis errors. + - Verify: `go test ./internal/infrastructure/authmfa/ -race` + - Files: `application/auth/mfa_limits.go` (new), `infrastructure/authmfa/attempts.go`, `attempts_test.go` + +- [x] **T2 — Per-token and per-account limits in the use case.** + - `WithAttemptLimits(store, revoker)`; input `ChallengeJTI` and `ChallengeExpiresAt`; `ErrMFAChallengeExhausted` and `ErrMFAChallengeLocked{RetryAfter}`. + - Checks the lock and the token count before verifying. Counts failures, blacklists the token at 5, locks at 10, clears the user counter on success. + - Accept: success criteria 1, 2 and 4 at the use-case level. + - Verify: `go test ./internal/application/auth/ -run MFAChallenge -race -v` + - Files: `mfa_usecase.go`, `mfa_limits.go`, `mfa_challenge_limits_test.go` + +- [x] **T3 — Lock email.** `MFALockedMailer` port, sent once when the lock is set; FR/EN template in `authmail`. + - Accept: one email per lock, not per refused attempt; a mail failure does not change the result. + - Verify: `go test ./internal/application/auth/ ./internal/infrastructure/authmail/ -race` + - Files: `mfa_limits.go`, `authmail/` mailer and its test + +- [x] **T4 — Handler and route.** `Challenge` passes the JTI and expiry and maps the errors (401 `MFA_CHALLENGE_EXHAUSTED`, 429 `MFA_LOCKED` with `Retry-After`). In `main.go`: wiring plus the per-IP `RateLimit` with the `mfa-challenge` prefix, mounted before `MFATokenMiddleware`. + - Accept: success criteria 1, 2 and 3 over HTTP. + - Verify: `go test ./internal/handler/auth/ -run MFAChallenge -race -v` + - Files: `mfa_handler.go`, `main.go`, `mfa_challenge_e2e_test.go` + +- [x] **T5 — Logout revokes the access token.** A `TokenRevoker` in `LogoutUseCase` (input `AccessJTI` and `AccessExpiresAt`); the handler extracts and validates the bearer or cookie token; wiring in `main.go`. + - Accept: success criterion 5, for both bearer and cookie. + - Verify: `go test ./internal/application/auth/ ./internal/handler/auth/ -run Logout -race -v` + - Files: `logout.go`, `handler.go`, `main.go`, tests + +- [x] **T6 — Full gate and live proof.** + - Run `go build ./... && go vet ./... && go test ./... -race`. + - Live boot with an enrolled account: criteria 1, 2 and 5 by curl, plus the lock email in the mail log. + - Open the frontend follow-up issue, post the progress comment, open the PR with `Closes #689`, set `status:in-review`. + +Risks: route registry and authz ratchet tests may trip on the new middleware order (T4). That is why the full `go test ./...` runs before the PR.