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 78c5ee62d..3e0fb4286 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 5c0aad621..3e4e1df74 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