Skip to content

Issue a new password reset token on each request - #14655

Open
MitchDrage wants to merge 1 commit into
apache:4.22from
MitchDrage:password-reset-new-token
Open

MitchDrage wants to merge 1 commit into
apache:4.22from
MitchDrage:password-reset-new-token

Conversation

@MitchDrage

Copy link
Copy Markdown
Contributor

Description

This PR fixes a second forgot-password request silently doing nothing while the first token is still valid.

  • Each forgot-password request now issues a new token with a full TTL. The new token replaces the previous one, so only the latest email's link works.
  • Adds user.password.reset.email.cooldown (seconds, default 300, 0 disables). Requests for the same user within the cooldown send no email and are logged at INFO. The cooldown is claimed before the email is sent and kept if the send fails, so a mail outage does not remove the limit. It is best effort: requests arriving at the same instant can each pass the check. A per-user GlobalLock would close that gap, but each held lock opens its own database connection outside the pool. The lock would be held for the whole SMTP send on an unauthenticated endpoint, so I chose the cooldown claim instead.
  • Each reset detail write now removes all existing rows for that detail first. Duplicate rows left by parallel requests previously survived every later request and locked the user out, since no emailed link would work.
  • Adds SMTPMailSender.trySendMail, which returns whether the SMTP server accepted the message.
    • Why: sendMail logs and swallows every failure, so the reset code cannot tell when an email never went out. Saving a new token after a failed send would invalidate the user's working link with nothing delivered. The token is now saved only after trySendMail returns true.
    • Other callers: sendMail keeps its void signature and now calls trySendMail, ignoring the result. The send logic, logging and exception handling are unchanged. Its other callers (AlertManagerImpl, ProjectManagerImpl, QuotaAlertManagerImpl, UsageAlertManagerImpl) need no changes, and their existing tests pass.
  • The forgot-password success message now holds for every outcome, including the cooldown, and does not confirm the account exists. Only en.json is updated; other locales are left for translators.

The wait between reset emails was previously tied to user.password.reset.ttl. It is now set separately by user.password.reset.email.cooldown. Since anyone can request a reset for a known username, repeated requests can replace a user's pending link once per cooldown. The 300-second default keeps that to a nuisance, and the user can always request a fresh link.

Fixes: #13604

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

  • Unit tests for the cooldown (first request, within and after the cooldown, disabled, future issue time, invalid value, large value), the send path (new token saved, send failure keeps the existing token and cooldown), duplicate cleanup and cleanup after a reset.
  • SMTPMailSenderTest covers trySendMail returning true and false. The existing tests for the other sendMail callers (alerts, quota, usage, projects) pass unchanged.
  • End-to-end on a simulator management server with Mailpit as the SMTP server (cooldown 30s): first request and reset via the link, repeat requests within and after the cooldown, old and reused links, an expired link, an SMTP outage, cooldown disabled, a token from before the upgrade, and bursts of 10 parallel requests.

How did you try to break this feature and the system with this change?

  • Parallel bursts, an SMTP outage mid-flow, expired and reused links, clock skew between management servers, and a malformed or very large cooldown value.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

User password reset request responds as successful, but fails silently - TTL Issue

1 participant