diff --git a/internal/app/account/service.go b/internal/app/account/service.go index 653641f8..2a5facaf 100644 --- a/internal/app/account/service.go +++ b/internal/app/account/service.go @@ -411,13 +411,47 @@ func (s *Service) RequestPasswordRecovery(ctx context.Context, userID int64) (st if !settings.HasPassword || settings.RecoveryEmail == "" { return "", domain.ErrPasswordRecoveryNA } - settings.RecoveryCode = recoveryCode - settings.RecoveryCodeExpiresAt = time.Now().Unix() + recoveryCodeTTL + // A fixed recovery code was used here regardless of whether it was + // actually emailed anywhere, which let anyone reset 2FA on any account + // with a recovery email set. Recovery now requires a real sender and a + // freshly generated code delivered to it -- no sender, no recovery. + if s.loginEmailSender == nil { + return "", domain.ErrPasswordRecoveryNA + } + code, err := randomDigits(s.loginEmailCodeLength) + if err != nil { + return "", err + } + deliveryID, err := otpdelivery.NewDeliveryID() + if err != nil { + return "", err + } + expiresAtUnix := time.Now().Unix() + recoveryCodeTTL + expiresAt := time.Unix(expiresAtUnix, 0) + settings.RecoveryCode = code + settings.RecoveryCodeExpiresAt = expiresAtUnix if s.passwords != nil { if err := s.passwords.Save(ctx, userID, settings); err != nil { return "", err } } + if err := deliverOTP(ctx, s.loginEmailSender, otpdelivery.Request{ + DeliveryID: deliveryID, + Purpose: otpdelivery.PurposePasswordRecovery, + Channel: otpdelivery.ChannelEmail, + Recipient: settings.RecoveryEmail, + Code: code, + ExpiresAt: expiresAt, + }); err != nil { + if s.passwords != nil { + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Second) + defer cancel() + settings.RecoveryCode = "" + settings.RecoveryCodeExpiresAt = 0 + _ = s.passwords.Save(cleanupCtx, userID, settings) + } + return "", err + } return emailPattern(settings.RecoveryEmail), nil } @@ -535,10 +569,9 @@ func (s *Service) CancelPasswordEmail(ctx context.Context, userID int64) error { } func checkRecoveryCode(settings domain.PasswordSettings, code string) error { + // No standing fixed-code fallback: an unrequested (or already consumed) + // recovery must not be satisfiable by any code at all. if settings.RecoveryCode == "" { - if code == recoveryCode { - return nil - } return domain.ErrPasswordRecoveryNA } if settings.RecoveryCodeExpiresAt > 0 && time.Now().Unix() > settings.RecoveryCodeExpiresAt { diff --git a/internal/app/account/service_test.go b/internal/app/account/service_test.go index 0d5b2831..d41a93a7 100644 --- a/internal/app/account/service_test.go +++ b/internal/app/account/service_test.go @@ -69,7 +69,9 @@ func TestPasswordSRPRoundTrip(t *testing.T) { func TestRecoverPasswordClearsTwoFactorPassword(t *testing.T) { ctx := context.Background() const userID int64 = 1002 - svc := NewService(memory.NewPasswordStore()) + sender := &captureMailSender{} + svc := NewService(memory.NewPasswordStore(), + WithLoginEmailVerification(memory.NewCodeStore(), sender, time.Minute, 3, 6)) initial, err := svc.GetPassword(ctx, userID) if err != nil { @@ -93,7 +95,10 @@ func TestRecoverPasswordClearsTwoFactorPassword(t *testing.T) { if pattern != "b***b@example.com" { t.Fatalf("recovery pattern = %q, want masked email", pattern) } - if err := svc.RecoverPassword(ctx, userID, recoveryCode, nil); err != nil { + if sender.to != "bob@example.com" || sender.code == "" { + t.Fatalf("sender = %+v, want delivered code to bob@example.com", sender) + } + if err := svc.RecoverPassword(ctx, userID, sender.code, nil); err != nil { t.Fatalf("RecoverPassword clear: %v", err) } cleared, err := svc.GetPassword(ctx, userID) diff --git a/internal/app/account/srp.go b/internal/app/account/srp.go index accd809b..3c169813 100644 --- a/internal/app/account/srp.go +++ b/internal/app/account/srp.go @@ -12,7 +12,6 @@ import ( const ( passwordHashSize = 256 - recoveryCode = "12345" recoveryCodeTTL = 15 * 60 ) diff --git a/internal/otpdelivery/delivery.go b/internal/otpdelivery/delivery.go index 9a681c2f..7ad7eafb 100644 --- a/internal/otpdelivery/delivery.go +++ b/internal/otpdelivery/delivery.go @@ -30,6 +30,7 @@ const ( PurposeLoginEmailChange Purpose = "login_email_change" PurposeChangePhone Purpose = "change_phone" PurposeConfirmPhone Purpose = "confirm_phone" + PurposePasswordRecovery Purpose = "password_recovery" ) type Request struct { @@ -47,7 +48,7 @@ func (r Request) Validate(now time.Time) error { return fmt.Errorf("delivery id is empty or too long") } switch r.Purpose { - case PurposeLoginEmail, PurposeLoginSMS, PurposeLoginEmailSetup, PurposeLoginEmailChange, PurposeChangePhone, PurposeConfirmPhone: + case PurposeLoginEmail, PurposeLoginSMS, PurposeLoginEmailSetup, PurposeLoginEmailChange, PurposeChangePhone, PurposeConfirmPhone, PurposePasswordRecovery: default: return fmt.Errorf("unsupported delivery purpose %q", r.Purpose) } diff --git a/internal/rpc/auth_password_pending_test.go b/internal/rpc/auth_password_pending_test.go index 69fb57c5..9ac599e6 100644 --- a/internal/rpc/auth_password_pending_test.go +++ b/internal/rpc/auth_password_pending_test.go @@ -3,6 +3,7 @@ package rpc import ( "context" "testing" + "time" "github.com/iamxvbaba/td/clock" "github.com/iamxvbaba/td/tg" @@ -10,9 +11,21 @@ import ( appaccount "telesrv/internal/app/account" "telesrv/internal/domain" + "telesrv/internal/otpdelivery" "telesrv/internal/store/memory" ) +type capturePasswordRecoveryMailSender struct { + to string + code string +} + +func (s *capturePasswordRecoveryMailSender) Deliver(_ context.Context, req otpdelivery.Request) (otpdelivery.Result, error) { + s.to = req.Recipient + s.code = req.Code + return otpdelivery.Result{}, nil +} + func TestAccountGetPasswordUsesPendingPasswordUser(t *testing.T) { ctx := pendingPasswordContext() const userID int64 = 42 @@ -65,9 +78,11 @@ func TestAuthRecoverPasswordCompletesPendingSignIn(t *testing.T) { pendingPassword: true, } sessions := &captureSessions{} + sender := &capturePasswordRecoveryMailSender{} router := New(Config{}, Deps{ - Auth: auth, - Account: appaccount.NewService(passwords), + Auth: auth, + Account: appaccount.NewService(passwords, + appaccount.WithLoginEmailVerification(memory.NewCodeStore(), sender, time.Minute, 3, 6)), Users: staticUsersService{user: domain.User{ID: userID, AccessHash: 7, Phone: "15550000042", FirstName: "Alice"}}, Sessions: sessions, }, zaptest.NewLogger(t), clock.System) @@ -75,7 +90,10 @@ func TestAuthRecoverPasswordCompletesPendingSignIn(t *testing.T) { if _, err := router.onAuthRequestPasswordRecovery(ctx); err != nil { t.Fatalf("auth.requestPasswordRecovery: %v", err) } - if _, err := router.onAuthRecoverPassword(ctx, &tg.AuthRecoverPasswordRequest{Code: "12345"}); err != nil { + if sender.to != "alice@example.com" || sender.code == "" { + t.Fatalf("sender = %+v, want delivered recovery code to alice@example.com", sender) + } + if _, err := router.onAuthRecoverPassword(ctx, &tg.AuthRecoverPasswordRequest{Code: sender.code}); err != nil { t.Fatalf("auth.recoverPassword: %v", err) } if auth.completePasswordCount != 1 || auth.completedPasswordKey != authKeyID {