From b9943675ba02d21de5a443e92b0377a30a1f1e4b Mon Sep 17 00:00:00 2001 From: ljacqu Date: Mon, 21 May 2018 13:29:34 +0200 Subject: [PATCH] #1557 Disallow player from using /email setpassword more than once --- .../authme/command/CommandInitializer.java | 4 +- ...mand.java => EmailSetPasswordCommand.java} | 5 +- .../service/PasswordRecoveryService.java | 17 ++++--- ....java => EmailSetPasswordCommandTest.java} | 11 +++-- .../service/PasswordRecoveryServiceTest.java | 49 +++++++++++++++++++ 5 files changed, 72 insertions(+), 14 deletions(-) rename src/main/java/fr/xephi/authme/command/executable/email/{SetPasswordCommand.java => EmailSetPasswordCommand.java} (89%) rename src/test/java/fr/xephi/authme/command/executable/email/{SetPasswordCommandTest.java => EmailSetPasswordCommandTest.java} (90%) diff --git a/src/main/java/fr/xephi/authme/command/CommandInitializer.java b/src/main/java/fr/xephi/authme/command/CommandInitializer.java index 6dbbcea59..ba48e0112 100644 --- a/src/main/java/fr/xephi/authme/command/CommandInitializer.java +++ b/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; diff --git a/src/main/java/fr/xephi/authme/command/executable/email/SetPasswordCommand.java b/src/main/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommand.java similarity index 89% rename from src/main/java/fr/xephi/authme/command/executable/email/SetPasswordCommand.java rename to src/main/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommand.java index d5d084aa6..376e8db29 100644 --- a/src/main/java/fr/xephi/authme/command/executable/email/SetPasswordCommand.java +++ b/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); } } } diff --git a/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java b/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java index f24b45f35..200aa11c7 100644 --- a/src/main/java/fr/xephi/authme/service/PasswordRecoveryService.java +++ b/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 diff --git a/src/test/java/fr/xephi/authme/command/executable/email/SetPasswordCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommandTest.java similarity index 90% rename from src/test/java/fr/xephi/authme/command/executable/email/SetPasswordCommandTest.java rename to src/test/java/fr/xephi/authme/command/executable/email/EmailSetPasswordCommandTest.java index 33acaf3f6..bf7fba782 100644 --- a/src/test/java/fr/xephi/authme/command/executable/email/SetPasswordCommandTest.java +++ b/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); } } diff --git a/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java b/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java index 48973bbfa..f0fc9b87c 100644 --- a/src/test/java/fr/xephi/authme/service/PasswordRecoveryServiceTest.java +++ b/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)); + } }