Skip to content

fix(email): save the recovery password only after the mail is sent - #3185

Open
rishabhvenu wants to merge 1 commit into
AuthMe:masterfrom
rishabhvenu:fix/recovery-save-after-send
Open

rishabhvenu wants to merge 1 commit into
AuthMe:masterfrom
rishabhvenu:fix/recovery-save-after-send

Conversation

@rishabhvenu

Copy link
Copy Markdown
Contributor

With recovery codes disabled (Security.recoveryCode.length: 0), PasswordRecoveryService.generateAndSendNewPassword saves the new password with dataSource.updatePassword and only then sends the mail. If sending fails, the player gets "The email could not be sent. Please contact an administrator.", but the old password has already been replaced by one they never received, so they cannot log in any more (#3153).

The mail is now sent first, and the new hash is only saved once sendPasswordMail returns true. On a send failure nothing is written and the player gets the same email.send_failure message as before. The return value of updatePassword was ignored until now; if saving fails after the mail went out, the mailed password would not work, so the player gets error.unexpected_error instead of the success message, a warning is logged, and the cooldown is not started. The mailed password becomes valid one database write after the SMTP server accepts the mail, which is far shorter than the time it takes the player to read it.

Testing:

  • Added three tests to PasswordRecoveryServiceTest: on success the mail is sent before the password is saved, and with the password from the mail; on a send failure updatePassword is never called; when saving fails the player gets the error message. All three fail on master and pass with the change.
  • mvn -V -B clean package on JDK 21: build passes, 1507 tests in authme-core. PurgeServiceTest.shouldRunAutoPurge failed in two earlier runs on a loaded machine (601 ms and 107 ms against its 100 ms tolerance) and passed alone and in the third full run; it is unrelated.
  • In game, SQLite, recovery codes disabled, SMTP on 127.0.0.1:25 without TLS. An offline-mode bot registers, gets an email set with /authme setemail, rejoins, runs /email recover <email>, then rejoins and logs in:
    • Nothing listening on port 25 (the send fails). Master, Paper 1.21.11 and Folia 1.21.11: the player gets "The email could not be sent", and /login <old password> is kicked with "Wrong password!". With the change, on both: same message, and /login <old password> succeeds.
    • A small SMTP sink on port 25. With the change, on Paper and Folia: "Recovery email sent successfully!", the old password is rejected and the password from the received mail logs in. Master behaves the same on Paper.

Not tested: a failing database write after a successful send (only covered by the unit test), MySQL and PostgreSQL, and the pre-join dialog recovery path in AsynchronousJoin, which calls the same method.

Fixes #3153

With recovery codes disabled, /email recover wrote the new password to
the database before sending it. When the mail could not be sent the
player got "The email could not be sent", but the old password had
already been replaced by one they never received, so they could no
longer log in.

Send the mail first and only save the new password once it went out.
If saving fails after the mail was sent, report an error instead of
the success message, since the mailed password would not work.

Fixes AuthMe#3153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Password recovery changes password in database BEFORE sending email -- users permanently locked out if email fails

1 participant