#949 Add configurable timeout for captcha count

src/main/java/fr/xephi/authme/data/CaptchaManager.java

@@ -1,19 +1,22 @@
 package fr.xephi.authme.data;
 
+import fr.xephi.authme.initialization.HasCleanup;
 import fr.xephi.authme.initialization.SettingsDependent;
-import fr.xephi.authme.util.RandomStringUtils;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.util.RandomStringUtils;
+import fr.xephi.authme.util.TimedCounter;
 
 import javax.inject.Inject;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Manager for the handling of captchas.
  */
-public class CaptchaManager implements SettingsDependent {
+public class CaptchaManager implements SettingsDependent, HasCleanup {
 
-    private final ConcurrentHashMap<String, Integer> playerCounts;
+    private final TimedCounter<String> playerCounts;
     private final ConcurrentHashMap<String, String> captchaCodes;
 
     private boolean isEnabled;
@@ -22,8 +25,9 @@ public class CaptchaManager implements SettingsDependent {
 
     @Inject
     CaptchaManager(Settings settings) {
-        this.playerCounts = new ConcurrentHashMap<>();
         this.captchaCodes = new ConcurrentHashMap<>();
+        long countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
+        this.playerCounts = new TimedCounter<>(countTimeout, TimeUnit.MINUTES);
         reload(settings);
     }
 
@@ -35,12 +39,7 @@ public class CaptchaManager implements SettingsDependent {
     public void increaseCount(String name) {
         if (isEnabled) {
             String playerLower = name.toLowerCase();
-            Integer currentCount = playerCounts.get(playerLower);
+            playerCounts.increment(playerLower);
-            if (currentCount == null) {
-                playerCounts.put(playerLower, 1);
-            } else {
-                playerCounts.put(playerLower, currentCount + 1);
-            }
         }
     }
 
@@ -51,21 +50,7 @@ public class CaptchaManager implements SettingsDependent {
      * @return true if the player has to solve a captcha, false otherwise
      */
     public boolean isCaptchaRequired(String name) {
-        if (isEnabled) {
+        return isEnabled && playerCounts.get(name.toLowerCase()) >= threshold;
-            Integer count = playerCounts.get(name.toLowerCase());
-            return count != null && count >= threshold;
-        }
-        return false;
-    }
-
-    /**
-     * Returns the stored captcha code for the player.
-     *
-     * @param name the player's name
-     * @return the code the player is required to enter, or null if none registered
-     */
-    public String getCaptchaCode(String name) {
-        return captchaCodes.get(name.toLowerCase());
     }
 
     /**
@@ -75,7 +60,7 @@ public class CaptchaManager implements SettingsDependent {
      * @return the code the player is required to enter
      */
     public String getCaptchaCodeOrGenerateNew(String name) {
-        String code = getCaptchaCode(name);
+        String code = captchaCodes.get(name.toLowerCase());
         return code == null ? generateCode(name) : code;
     }
 
@@ -127,6 +112,13 @@ public class CaptchaManager implements SettingsDependent {
         this.isEnabled = settings.getProperty(SecuritySettings.USE_CAPTCHA);
         this.threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TRIES_BEFORE_CAPTCHA);
         this.captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
+        long countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
+        playerCounts.setExpiration(countTimeout, TimeUnit.MINUTES);
+    }
+
+    @Override
+    public void performCleanup() {
+        playerCounts.removeExpiredEntries();
     }
 
 }

src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java

@@ -40,6 +40,10 @@ public class SecuritySettings implements SettingsHolder {
     public static final Property<Integer> CAPTCHA_LENGTH =
         newProperty("Security.captcha.captchaLength", 5);
 
+    @Comment("Minutes after which login attempts count is reset for a player")
+    public static final Property<Integer> CAPTCHA_COUNT_MINUTES_BEFORE_RESET =
+        newProperty("Security.captcha.captchaCountReset", 60);
+
     @Comment("Minimum length of password")
     public static final Property<Integer> MIN_PASSWORD_LENGTH =
         newProperty("settings.security.minPasswordLength", 5);

src/test/java/fr/xephi/authme/data/CaptchaManagerTest.java

@@ -3,12 +3,10 @@ package fr.xephi.authme.data;
 import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.util.TimedCounter;
 import org.junit.Test;
 
-import java.util.Map;
-
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertThat;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
@@ -57,11 +55,6 @@ public class CaptchaManagerTest {
         assertThat(manager.checkCode(player, "bogus"), equalTo(true));
     }
 
-    /**
-     * Tests {@link CaptchaManager#getCaptchaCode} and {@link CaptchaManager#getCaptchaCodeOrGenerateNew}.
-     * The former method should never change the code (and so return {@code null} for no code) while the latter should
-     * generate a new code if no code is yet present. If a code is saved, it should never generate a new one.
-     */
     @Test
     public void shouldHaveSameCodeAfterGeneration() {
         // given
@@ -70,18 +63,14 @@ public class CaptchaManagerTest {
         CaptchaManager manager = new CaptchaManager(settings);
 
         // when
-        String code1 = manager.getCaptchaCode(player);
+        String code1 = manager.getCaptchaCodeOrGenerateNew(player);
         String code2 = manager.getCaptchaCodeOrGenerateNew(player);
-        String code3 = manager.getCaptchaCode(player);
+        String code3 = manager.getCaptchaCodeOrGenerateNew(player);
-        String code4 = manager.getCaptchaCodeOrGenerateNew(player);
-        String code5 = manager.getCaptchaCode(player);
 
         // then
-        assertThat(code1, nullValue());
+        assertThat(code1.length(), equalTo(5));
-        assertThat(code2.length(), equalTo(5));
+        assertThat(code2, equalTo(code1));
-        assertThat(code3, equalTo(code2));
+        assertThat(code3, equalTo(code1));
-        assertThat(code4, equalTo(code2));
-        assertThat(code5, equalTo(code2));
     }
 
     @Test
@@ -104,7 +93,7 @@ public class CaptchaManagerTest {
 
         // then 2
         assertThat(manager.isCaptchaRequired(player), equalTo(false));
-        assertHasCount(manager, player, null);
+        assertHasCount(manager, player, 0);
     }
 
     @Test
@@ -120,7 +109,7 @@ public class CaptchaManagerTest {
 
         // then
         assertThat(manager.isCaptchaRequired(player), equalTo(false));
-        assertHasCount(manager, player, null);
+        assertHasCount(manager, player, 0);
     }
 
     @Test
@@ -149,11 +138,12 @@ public class CaptchaManagerTest {
         given(settings.getProperty(SecuritySettings.USE_CAPTCHA)).willReturn(true);
         given(settings.getProperty(SecuritySettings.MAX_LOGIN_TRIES_BEFORE_CAPTCHA)).willReturn(maxTries);
         given(settings.getProperty(SecuritySettings.CAPTCHA_LENGTH)).willReturn(captchaLength);
+        given(settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET)).willReturn(30);
         return settings;
     }
 
     private static void assertHasCount(CaptchaManager manager, String player, Integer count) {
-        Map<String, Integer> playerCounts = ReflectionTestUtils
+        TimedCounter<String> playerCounts = ReflectionTestUtils
             .getFieldValue(CaptchaManager.class, manager, "playerCounts");
         assertThat(playerCounts.get(player.toLowerCase()), equalTo(count));
     }