From d5df8130c7522652f25ab12930e576891b75f45e Mon Sep 17 00:00:00 2001 From: rishabhvenu Date: Wed, 28 Oct 2026 16:44:55 -0400 Subject: [PATCH] fix(email): save the recovery password only after the mail is sent 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 #3153 --- .../service/PasswordRecoveryService.java | 18 +++-- .../service/PasswordRecoveryServiceTest.java | 81 +++++++++++++++++++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/authme-core/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java b/authme-core/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java index 78c5ee62d5..3e0fb4286a 100644 --- a/authme-core/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java +++ b/authme-core/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java @@ -85,7 +85,9 @@ public void createAndSendRecoveryCode(Player player, String email) { /** * Generate a new password and send it to the player via - * email. This will update the database with the new password. + * email. The database is only updated with the new password + * once the email has been sent, so the old password keeps + * working if sending fails. * * @param player The player recovering their password. * @param email The email to send the password to. @@ -101,14 +103,18 @@ public void generateAndSendNewPassword(Player player, String email) { logger.info("Generating new password for '" + name + "'"); - dataSource.updatePassword(name, hashNew); boolean couldSendMail = emailService.sendPasswordMail(name, email, thePass); - if (couldSendMail) { - commonService.send(player, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE); - emailCooldown.add(player.getName().toLowerCase(Locale.ROOT)); - } else { + if (!couldSendMail) { commonService.send(player, MessageKey.EMAIL_SEND_FAILURE); + return; + } + if (!dataSource.updatePassword(name, hashNew)) { + logger.warning("Could not save the new password of '" + name + "' after sending it by email"); + commonService.send(player, MessageKey.ERROR); + return; } + commonService.send(player, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE); + emailCooldown.add(name.toLowerCase(Locale.ROOT)); } /** diff --git a/authme-core/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java b/authme-core/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java index 5c0aad6214..3e4e1df74f 100644 --- a/authme-core/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java @@ -7,19 +7,29 @@ import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.message.Messages; import fr.xephi.authme.security.PasswordSecurity; +import fr.xephi.authme.security.crypts.HashedPassword; +import fr.xephi.authme.settings.properties.EmailSettings; import fr.xephi.authme.settings.properties.SecuritySettings; import org.bukkit.entity.Player; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; /** @@ -49,6 +59,11 @@ class PasswordRecoveryServiceTest { @Mock private Messages messages; + @BeforeAll + static void setUpLogger() { + TestHelper.setupLogger(); + } + @BeforeEach void runPostConstructMethod() { given(commonService.getProperty(SecuritySettings.EMAIL_RECOVERY_COOLDOWN_SECONDS)).willReturn(40); @@ -76,6 +91,72 @@ void shouldSendRecoveryCode() { verify(commonService).send(player, MessageKey.RECOVERY_CODE_SENT); } + @Test + void shouldSendNewPasswordAndThenSaveIt() { + // given + Player player = mock(Player.class); + String name = "Carl"; + given(player.getName()).willReturn(name); + String email = "test@example.com"; + given(commonService.getProperty(EmailSettings.RECOVERY_PASSWORD_LENGTH)).willReturn(12); + HashedPassword hash = new HashedPassword("new-hash"); + given(passwordSecurity.computeHash(anyString(), eq(name))).willReturn(hash); + given(emailService.sendPasswordMail(eq(name), eq(email), anyString())).willReturn(true); + given(dataSource.updatePassword(name, hash)).willReturn(true); + + // when + recoveryService.generateAndSendNewPassword(player, email); + + // then + ArgumentCaptor passwordCaptor = ArgumentCaptor.forClass(String.class); + InOrder inOrder = inOrder(emailService, dataSource); + inOrder.verify(emailService).sendPasswordMail(eq(name), eq(email), passwordCaptor.capture()); + inOrder.verify(dataSource).updatePassword(name, hash); + verify(passwordSecurity).computeHash(passwordCaptor.getValue(), name); + verify(commonService).send(player, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE); + } + + @Test + void shouldNotChangePasswordIfNewPasswordMailCannotBeSent() { + // given + Player player = mock(Player.class); + String name = "Carl"; + given(player.getName()).willReturn(name); + String email = "test@example.com"; + given(commonService.getProperty(EmailSettings.RECOVERY_PASSWORD_LENGTH)).willReturn(12); + given(passwordSecurity.computeHash(anyString(), eq(name))).willReturn(new HashedPassword("new-hash")); + given(emailService.sendPasswordMail(eq(name), eq(email), anyString())).willReturn(false); + + // when + recoveryService.generateAndSendNewPassword(player, email); + + // then + verify(dataSource, never()).updatePassword(anyString(), any(HashedPassword.class)); + verify(commonService).send(player, MessageKey.EMAIL_SEND_FAILURE); + verify(commonService, never()).send(player, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE); + } + + @Test + void shouldReportErrorIfNewPasswordCannotBeSaved() { + // given + Player player = mock(Player.class); + String name = "Carl"; + given(player.getName()).willReturn(name); + String email = "test@example.com"; + given(commonService.getProperty(EmailSettings.RECOVERY_PASSWORD_LENGTH)).willReturn(12); + HashedPassword hash = new HashedPassword("new-hash"); + given(passwordSecurity.computeHash(anyString(), eq(name))).willReturn(hash); + given(emailService.sendPasswordMail(eq(name), eq(email), anyString())).willReturn(true); + given(dataSource.updatePassword(name, hash)).willReturn(false); + + // when + recoveryService.generateAndSendNewPassword(player, email); + + // then + verify(commonService).send(player, MessageKey.ERROR); + verify(commonService, never()).send(player, MessageKey.RECOVERY_EMAIL_SENT_MESSAGE); + } + @Test void shouldKeepTrackOfSuccessfulRecoversByIp() { // given