From 39395836b44730257b6b8288108ad9c76e418277 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 19 Feb 2017 11:50:06 +0100 Subject: [PATCH] #949 Add configurable timeout for captcha count --- .../fr/xephi/authme/data/CaptchaManager.java | 44 ++++++++----------- .../settings/properties/SecuritySettings.java | 4 ++ .../xephi/authme/data/CaptchaManagerTest.java | 30 +++++-------- 3 files changed, 32 insertions(+), 46 deletions(-) diff --git a/src/main/java/fr/xephi/authme/data/CaptchaManager.java b/src/main/java/fr/xephi/authme/data/CaptchaManager.java index 36f33a3c..7e83ec23 100644 --- a/src/main/java/fr/xephi/authme/data/CaptchaManager.java +++ b/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 playerCounts; + private final TimedCounter playerCounts; private final ConcurrentHashMap 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); - if (currentCount == null) { - playerCounts.put(playerLower, 1); - } else { - playerCounts.put(playerLower, currentCount + 1); - } + playerCounts.increment(playerLower); } } @@ -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) { - 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()); + return isEnabled && playerCounts.get(name.toLowerCase()) >= threshold; } /** @@ -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(); } } diff --git a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java index 054651ad..daed1a8c 100644 --- a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java +++ b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java @@ -40,6 +40,10 @@ public class SecuritySettings implements SettingsHolder { public static final Property CAPTCHA_LENGTH = newProperty("Security.captcha.captchaLength", 5); + @Comment("Minutes after which login attempts count is reset for a player") + public static final Property CAPTCHA_COUNT_MINUTES_BEFORE_RESET = + newProperty("Security.captcha.captchaCountReset", 60); + @Comment("Minimum length of password") public static final Property MIN_PASSWORD_LENGTH = newProperty("settings.security.minPasswordLength", 5); diff --git a/src/test/java/fr/xephi/authme/data/CaptchaManagerTest.java b/src/test/java/fr/xephi/authme/data/CaptchaManagerTest.java index 92be450d..d53091db 100644 --- a/src/test/java/fr/xephi/authme/data/CaptchaManagerTest.java +++ b/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 code4 = manager.getCaptchaCodeOrGenerateNew(player); - String code5 = manager.getCaptchaCode(player); + String code3 = manager.getCaptchaCodeOrGenerateNew(player); // then - assertThat(code1, nullValue()); - assertThat(code2.length(), equalTo(5)); - assertThat(code3, equalTo(code2)); - assertThat(code4, equalTo(code2)); - assertThat(code5, equalTo(code2)); + assertThat(code1.length(), equalTo(5)); + assertThat(code2, equalTo(code1)); + assertThat(code3, equalTo(code1)); } @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 playerCounts = ReflectionTestUtils + TimedCounter playerCounts = ReflectionTestUtils .getFieldValue(CaptchaManager.class, manager, "playerCounts"); assertThat(playerCounts.get(player.toLowerCase()), equalTo(count)); }