#930 Change captcha storage to change code internally upon failure

- Within CaptchaStorage#checkCode, a player's captcha code is overridden with a new one on failure or cleared on success
- Fixes inconsistencies in the retrieval / regeneration of codes

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

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;
     }

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.
+     * 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.
-     * @param name the name of the player to generate a code for
+     * <p>
-     * @return the generated code
+     * On success, the code associated with the player is cleared; on failure, a new code is generated.
-     */
-    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.
      *
      * @param player the player to check
      * @param code the supplied code

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();

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;
     }
 

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");
     }
 

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 result = manager.checkCode(player, captchaCode);
-        boolean goodResult = manager.checkCode(player, captchaCode);
 
         // then
-        assertThat(captchaCode.length(), equalTo(4));
+        assertThat(captchaCode, stringWithLength(4));
-        assertThat(badResult, equalTo(false));
+        assertThat(result, equalTo(true));
-        assertThat(goodResult, 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

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")