#1557 Disallow player from using /email setpassword more than once

src/main/java/fr/xephi/authme/command/CommandInitializer.java

@@ -33,9 +33,9 @@ import fr.xephi.authme.command.executable.changepassword.ChangePasswordCommand;
 import fr.xephi.authme.command.executable.email.AddEmailCommand;
 import fr.xephi.authme.command.executable.email.ChangeEmailCommand;
 import fr.xephi.authme.command.executable.email.EmailBaseCommand;
+import fr.xephi.authme.command.executable.email.EmailSetPasswordCommand;
 import fr.xephi.authme.command.executable.email.ProcessCodeCommand;
 import fr.xephi.authme.command.executable.email.RecoverEmailCommand;
-import fr.xephi.authme.command.executable.email.SetPasswordCommand;
 import fr.xephi.authme.command.executable.email.ShowEmailCommand;
 import fr.xephi.authme.command.executable.login.LoginCommand;
 import fr.xephi.authme.command.executable.logout.LogoutCommand;
@@ -540,7 +540,7 @@ public class CommandInitializer {
             .detailedDescription("Set a new password after successfully recovering your account.")
             .withArgument("password", "New password", MANDATORY)
             .permission(PlayerPermission.RECOVER_EMAIL)
-            .executableCommand(SetPasswordCommand.class)
+            .executableCommand(EmailSetPasswordCommand.class)
             .register();
 
         return emailBase;

src/main/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommand.java

@@ -18,7 +18,7 @@ import java.util.List;
 /**
  * Command for changing password following successful recovery.
  */
-public class SetPasswordCommand extends PlayerCommand {
+public class EmailSetPasswordCommand extends PlayerCommand {
 
     @Inject
     private DataSource dataSource;
@@ -45,11 +45,14 @@ public class SetPasswordCommand extends PlayerCommand {
             if (!result.hasError()) {
                 HashedPassword hashedPassword = passwordSecurity.computeHash(password, name);
                 dataSource.updatePassword(name, hashedPassword);
+                recoveryService.removeFromSuccessfulRecovery(player);
                 ConsoleLogger.info("Player '" + name + "' has changed their password from recovery");
                 commonService.send(player, MessageKey.PASSWORD_CHANGED_SUCCESS);
             } else {
                 commonService.send(player, result.getMessageKey(), result.getArgs());
             }
+        } else {
+            commonService.send(player, MessageKey.CHANGE_PASSWORD_EXPIRED);
         }
     }
 }

src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java

@@ -121,6 +121,16 @@ public class PasswordRecoveryService implements Reloadable, HasCleanup {
         commonService.send(player, MessageKey.RECOVERY_CHANGE_PASSWORD);
     }
 
+    /**
+     * Removes a player from the list of successful recovers so that he can
+     * no longer use the /email setpassword command.
+     *
+     * @param player The player to remove.
+     */
+    public void removeFromSuccessfulRecovery(Player player) {
+        successfulRecovers.remove(player.getName());
+    }
+
     /**
      * Check if a player is able to have emails sent.
      *
@@ -149,12 +159,7 @@ public class PasswordRecoveryService implements Reloadable, HasCleanup {
         String playerAddress = PlayerUtils.getPlayerIp(player);
         String storedAddress = successfulRecovers.get(name);
 
-        if (storedAddress == null || !playerAddress.equals(storedAddress)) {
-            messages.send(player, MessageKey.CHANGE_PASSWORD_EXPIRED);
-            return false;
-        }
-
-        return true;
+        return storedAddress != null && playerAddress.equals(storedAddress);
     }
 
     @Override

src/test/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommandTest.java

@@ -24,13 +24,13 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
 
 /**
- * Tests for {@link SetPasswordCommand}.
+ * Tests for {@link EmailSetPasswordCommand}.
  */
 @RunWith(MockitoJUnitRunner.class)
-public class SetPasswordCommandTest {
+public class EmailSetPasswordCommandTest {
 
     @InjectMocks
-    private SetPasswordCommand command;
+    private EmailSetPasswordCommand command;
 
     @Mock
     private DataSource dataSource;
@@ -70,6 +70,7 @@ public class SetPasswordCommandTest {
         // then
         verify(validationService).validatePassword("abc123", name);
         verify(dataSource).updatePassword(name, hashedPassword);
+        verify(recoveryService).removeFromSuccessfulRecovery(player);
         verify(commonService).send(player, MessageKey.PASSWORD_CHANGED_SUCCESS);
     }
 
@@ -101,7 +102,7 @@ public class SetPasswordCommandTest {
         command.runCommand(player, Collections.singletonList("abc123"));
 
         // then
-        verifyZeroInteractions(validationService);
-        verifyZeroInteractions(dataSource);
+        verifyZeroInteractions(validationService, dataSource);
+        verify(commonService).send(player, MessageKey.CHANGE_PASSWORD_EXPIRED);
     }
 }

src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java

@@ -3,6 +3,7 @@ package fr.xephi.authme.service;
 import ch.jalu.injector.testing.BeforeInjecting;
 import ch.jalu.injector.testing.DelayedInjectionRunner;
 import ch.jalu.injector.testing.InjectDelayed;
+import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.mail.EmailService;
 import fr.xephi.authme.message.MessageKey;
@@ -14,6 +15,8 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -70,4 +73,50 @@ public class PasswordRecoveryServiceTest {
         verify(emailService).sendRecoveryCode(name, email, code);
         verify(commonService).send(player, MessageKey.RECOVERY_CODE_SENT);
     }
+
+    @Test
+    public void shouldKeepTrackOfSuccessfulRecoversByIp() {
+        // given
+        Player bobby = mock(Player.class);
+        TestHelper.mockPlayerIp(bobby, "192.168.8.8");
+        given(bobby.getName()).willReturn("bobby");
+
+        Player bobby2 = mock(Player.class);
+        TestHelper.mockPlayerIp(bobby2, "127.0.0.1");
+        given(bobby2.getName()).willReturn("bobby");
+
+        Player other = mock(Player.class);
+        TestHelper.mockPlayerIp(other, "192.168.8.8");
+        given(other.getName()).willReturn("other");
+
+        // when
+        recoveryService.addSuccessfulRecovery(bobby);
+
+        // then
+        assertThat(recoveryService.canChangePassword(bobby), equalTo(true));
+        assertThat(recoveryService.canChangePassword(bobby2), equalTo(false));
+        assertThat(recoveryService.canChangePassword(other), equalTo(false));
+    }
+
+    @Test
+    public void shouldRemovePlayerFromSuccessfulRecovers() {
+        // given
+        Player bobby = mock(Player.class);
+        TestHelper.mockPlayerIp(bobby, "192.168.8.8");
+        given(bobby.getName()).willReturn("bobby");
+        recoveryService.addSuccessfulRecovery(bobby);
+
+        Player other = mock(Player.class);
+        TestHelper.mockPlayerIp(other, "8.8.8.8");
+        given(other.getName()).willReturn("other");
+        recoveryService.addSuccessfulRecovery(other);
+
+        // when
+        recoveryService.removeFromSuccessfulRecovery(other);
+
+
+        // then
+        assertThat(recoveryService.canChangePassword(bobby), equalTo(true));
+        assertThat(recoveryService.canChangePassword(other), equalTo(false));
+    }
 }