diff --git a/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java b/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java index b568f305..d8e9e4a7 100644 --- a/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java @@ -53,7 +53,7 @@ public class CaptchaCommand extends PlayerCommand { commonService.send(player, MessageKey.LOGIN_MESSAGE); limboService.unmuteMessageTask(player); } else { - String newCode = loginCaptchaManager.generateCode(player.getName()); + String newCode = loginCaptchaManager.getCaptchaCodeOrGenerateNew(player.getName()); commonService.send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode); } } @@ -64,7 +64,7 @@ public class CaptchaCommand extends PlayerCommand { commonService.send(player, MessageKey.CAPTCHA_SUCCESS); commonService.send(player, MessageKey.REGISTER_MESSAGE); } else { - String newCode = registrationCaptchaManager.generateCode(player.getName()); + String newCode = registrationCaptchaManager.getCaptchaCodeOrGenerateNew(player.getName()); commonService.send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode); } } diff --git a/src/main/java/fr/xephi/authme/data/captcha/CaptchaCodeStorage.java b/src/main/java/fr/xephi/authme/data/captcha/CaptchaCodeStorage.java index ba4ec470..713edd69 100644 --- a/src/main/java/fr/xephi/authme/data/captcha/CaptchaCodeStorage.java +++ b/src/main/java/fr/xephi/authme/data/captcha/CaptchaCodeStorage.java @@ -61,7 +61,7 @@ public class CaptchaCodeStorage { * @param name the name of the player to generate a code for * @return the generated code */ - public String generateCode(String name) { + private String generateCode(String name) { String code = RandomStringUtils.generate(captchaLength); captchaCodes.put(name.toLowerCase(), code); return code; @@ -69,6 +69,7 @@ public class CaptchaCodeStorage { /** * Checks the given code against the existing one. Upon success, the saved captcha code is removed from storage. + * Upon failure, a new code is generated. * * @param name the name of the player to check * @param code the supplied code @@ -80,6 +81,8 @@ public class CaptchaCodeStorage { if (savedCode != null && savedCode.equalsIgnoreCase(code)) { captchaCodes.remove(nameLowerCase); return true; + } else { + generateCode(name); } return false; } diff --git a/src/main/java/fr/xephi/authme/data/captcha/CaptchaManager.java b/src/main/java/fr/xephi/authme/data/captcha/CaptchaManager.java index b5b01d85..c3daeb6c 100644 --- a/src/main/java/fr/xephi/authme/data/captcha/CaptchaManager.java +++ b/src/main/java/fr/xephi/authme/data/captcha/CaptchaManager.java @@ -24,16 +24,10 @@ public interface CaptchaManager { String getCaptchaCodeOrGenerateNew(String name); /** - * Generates a code for the player and returns it. - * - * @param name the name of the player to generate a code for - * @return the generated code - */ - String generateCode(String name); - - /** - * Checks the given code against the existing one. This method may perform additional state changes - * on success or failure, such as modifying some counter or setting a player as verified. + * Checks the given code against the existing one. This method is not reentrant, i.e. it performs additional + * state changes on success or failure, such as modifying some counter or setting a player as verified. + *

+ * On success, the code associated with the player is cleared; on failure, a new code is generated. * * @param player the player to check * @param code the supplied code diff --git a/src/main/java/fr/xephi/authme/data/captcha/LoginCaptchaManager.java b/src/main/java/fr/xephi/authme/data/captcha/LoginCaptchaManager.java index 8bf21441..cf3958a1 100644 --- a/src/main/java/fr/xephi/authme/data/captcha/LoginCaptchaManager.java +++ b/src/main/java/fr/xephi/authme/data/captcha/LoginCaptchaManager.java @@ -51,11 +51,6 @@ public class LoginCaptchaManager implements CaptchaManager, SettingsDependent, H return captchaCodeStorage.getCodeOrGenerateNew(name); } - @Override - public String generateCode(String name) { - return captchaCodeStorage.generateCode(name); - } - @Override public boolean checkCode(Player player, String code) { String nameLower = player.getName().toLowerCase(); diff --git a/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java b/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java index a5961241..bce95429 100644 --- a/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java +++ b/src/main/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManager.java @@ -45,20 +45,14 @@ public class RegistrationCaptchaManager return captchaCodeStorage.getCodeOrGenerateNew(name); } - @Override - public String generateCode(String name) { - return captchaCodeStorage.generateCode(name); - } - @Override public boolean checkCode(Player player, String code) { String nameLower = player.getName().toLowerCase(); boolean isCodeCorrect = captchaCodeStorage.checkCode(nameLower, code); if (isCodeCorrect) { verifiedNamesForRegistration.add(nameLower); - } else { - limboService.resetMessageTask(player, false); } + limboService.resetMessageTask(player, false); return isCodeCorrect; } diff --git a/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java index 7ec74411..8c139508 100644 --- a/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java @@ -111,7 +111,7 @@ public class CaptchaCommandTest { String captchaCode = "2468"; given(loginCaptchaManager.checkCode(player, captchaCode)).willReturn(false); String newCode = "1337"; - given(loginCaptchaManager.generateCode(name)).willReturn(newCode); + given(loginCaptchaManager.getCaptchaCodeOrGenerateNew(name)).willReturn(newCode); // when command.executeCommand(player, Collections.singletonList(captchaCode)); @@ -119,7 +119,7 @@ public class CaptchaCommandTest { // then verify(loginCaptchaManager).isCaptchaRequired(name); verify(loginCaptchaManager).checkCode(player, captchaCode); - verify(loginCaptchaManager).generateCode(name); + verify(loginCaptchaManager).getCaptchaCodeOrGenerateNew(name); verifyNoMoreInteractions(loginCaptchaManager); verify(commonService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, newCode); verifyNoMoreInteractions(commonService); @@ -153,14 +153,14 @@ public class CaptchaCommandTest { given(registrationCaptchaManager.isCaptchaRequired(name)).willReturn(true); String captchaCode = "SFL3"; given(registrationCaptchaManager.checkCode(player, captchaCode)).willReturn(false); - given(registrationCaptchaManager.generateCode(name)).willReturn("new code"); + given(registrationCaptchaManager.getCaptchaCodeOrGenerateNew(name)).willReturn("new code"); // when command.executeCommand(player, Collections.singletonList(captchaCode)); // then verify(registrationCaptchaManager).checkCode(player, captchaCode); - verify(registrationCaptchaManager).generateCode(name); + verify(registrationCaptchaManager).getCaptchaCodeOrGenerateNew(name); verify(commonService).send(player, MessageKey.CAPTCHA_WRONG_ERROR, "new code"); } diff --git a/src/test/java/fr/xephi/authme/data/captcha/LoginCaptchaManagerTest.java b/src/test/java/fr/xephi/authme/data/captcha/LoginCaptchaManagerTest.java index 7fdd109f..e5c2c63f 100644 --- a/src/test/java/fr/xephi/authme/data/captcha/LoginCaptchaManagerTest.java +++ b/src/test/java/fr/xephi/authme/data/captcha/LoginCaptchaManagerTest.java @@ -7,7 +7,9 @@ import fr.xephi.authme.util.expiring.TimedCounter; import org.bukkit.entity.Player; import org.junit.Test; +import static fr.xephi.authme.AuthMeMatchers.stringWithLength; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -47,17 +49,34 @@ public class LoginCaptchaManagerTest { String captchaCode = manager.getCaptchaCodeOrGenerateNew(name); // when - boolean badResult = manager.checkCode(player, "wrong_code"); - boolean goodResult = manager.checkCode(player, captchaCode); + boolean result = manager.checkCode(player, captchaCode); // then - assertThat(captchaCode.length(), equalTo(4)); - assertThat(badResult, equalTo(false)); - assertThat(goodResult, equalTo(true)); + assertThat(captchaCode, stringWithLength(4)); + assertThat(result, equalTo(true)); // Supplying correct code should clear the entry, and a code should be invalid if no entry is present assertThat(manager.checkCode(player, "bogus"), equalTo(false)); } + @Test + public void shouldGenerateNewCodeOnFailure() { + // given + String name = "Tarheel"; + Player player = mock(Player.class); + given(player.getName()).willReturn(name); + Settings settings = mockSettings(1, 9); + LoginCaptchaManager manager = new LoginCaptchaManager(settings); + String captchaCode = manager.getCaptchaCodeOrGenerateNew(name); + + // when + boolean result = manager.checkCode(player, "wrongcode"); + + // then + assertThat(captchaCode, stringWithLength(9)); + assertThat(result, equalTo(false)); + assertThat(manager.getCaptchaCodeOrGenerateNew(name), not(equalTo(captchaCode))); + } + @Test public void shouldHaveSameCodeAfterGeneration() { // given diff --git a/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java b/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java index 4a71ab2c..abd7d295 100644 --- a/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java +++ b/src/test/java/fr/xephi/authme/data/captcha/RegistrationCaptchaManagerTest.java @@ -1,23 +1,28 @@ package fr.xephi.authme.data.captcha; import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.expiring.ExpiringMap; import org.bukkit.entity.Player; import org.junit.Test; +import org.mockito.Mockito; import static fr.xephi.authme.AuthMeMatchers.stringWithLength; 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; /** * Test for {@link RegistrationCaptchaManager}. */ public class RegistrationCaptchaManagerTest { + private LimboService limboService = Mockito.mock(LimboService.class); + @Test public void shouldBeDisabled() { // given @@ -45,6 +50,7 @@ public class RegistrationCaptchaManagerTest { String captcha = "abc3"; RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings); + captchaManager.setLimboService(limboService); getCodeMap(captchaManager).put("test", captcha); Player player = mock(Player.class); @@ -57,6 +63,7 @@ public class RegistrationCaptchaManagerTest { assertThat(isSuccessful, equalTo(true)); assertThat(getCodeMap(captchaManager).isEmpty(), equalTo(true)); assertThat(captchaManager.isCaptchaRequired("test"), equalTo(false)); + verify(limboService).resetMessageTask(player, false); } @Test @@ -67,6 +74,7 @@ public class RegistrationCaptchaManagerTest { int captchaLength = 9; given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(captchaLength); RegistrationCaptchaManager captchaManager = new RegistrationCaptchaManager(settings); + captchaManager.setLimboService(limboService); // when String captcha1 = captchaManager.getCaptchaCodeOrGenerateNew("toast"); @@ -82,6 +90,7 @@ public class RegistrationCaptchaManagerTest { // when (2) / then (2) assertThat(captchaManager.checkCode(player, captcha1), equalTo(true)); + verify(limboService).resetMessageTask(player, false); } @SuppressWarnings("unchecked")