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

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));
     }
 
     /**

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<String> 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