#930 Refactor captcha managers to have a crude captcha storage class instead of inheritance

- Remove abstract captcha manager in favor of a primitive captcha code storage (composition over inheritance) - Supply player when checking captcha code for further usage (fixes open point from previous commit)
2018-01-05 01:26:25 +01:00
parent 0494886518
commit 180bbbf0be
18 changed files with 361 additions and 308 deletions
@@ -1,9 +1,9 @@
 package fr.xephi.authme.command.executable.captcha;

 import fr.xephi.authme.command.PlayerCommand;
-import fr.xephi.authme.data.LoginCaptchaManager;
-import fr.xephi.authme.data.RegistrationCaptchaManager;
 import fr.xephi.authme.data.auth.PlayerCache;
+import fr.xephi.authme.data.captcha.LoginCaptchaManager;
+import fr.xephi.authme.data.captcha.RegistrationCaptchaManager;
 import fr.xephi.authme.data.limbo.LimboService;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.service.CommonService;
@@ -47,7 +47,7 @@ public class CaptchaCommand extends PlayerCommand {
    }

    private void checkLoginCaptcha(Player player, String captchaCode) {
-        final boolean isCorrectCode = loginCaptchaManager.checkCode(player.getName(), captchaCode);
+        final boolean isCorrectCode = loginCaptchaManager.checkCode(player, captchaCode);
        if (isCorrectCode) {
            commonService.send(player, MessageKey.CAPTCHA_SUCCESS);
            commonService.send(player, MessageKey.LOGIN_MESSAGE);
@@ -59,7 +59,7 @@ public class CaptchaCommand extends PlayerCommand {
    }

    private void checkRegisterCaptcha(Player player, String captchaCode) {
-        final boolean isCorrectCode = registrationCaptchaManager.checkCode(player.getName(), captchaCode);
+        final boolean isCorrectCode = registrationCaptchaManager.checkCode(player, captchaCode);
        if (isCorrectCode) {
            commonService.send(player, MessageKey.CAPTCHA_SUCCESS);
            commonService.send(player, MessageKey.REGISTER_MESSAGE);
@@ -2,7 +2,7 @@ package fr.xephi.authme.command.executable.register;

 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.command.PlayerCommand;
-import fr.xephi.authme.data.RegistrationCaptchaManager;
+import fr.xephi.authme.data.captcha.RegistrationCaptchaManager;
 import fr.xephi.authme.mail.EmailService;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.process.Management;
@@ -1,126 +0,0 @@
-package fr.xephi.authme.data;
-
-import fr.xephi.authme.initialization.HasCleanup;
-import fr.xephi.authme.initialization.SettingsDependent;
-import fr.xephi.authme.settings.Settings;
-import fr.xephi.authme.settings.properties.SecuritySettings;
-import fr.xephi.authme.util.RandomStringUtils;
-import fr.xephi.authme.util.expiring.ExpiringMap;
-
-import java.util.concurrent.TimeUnit;
-
-/**
- * Manages captcha codes.
- */
-public abstract class AbstractCaptchaManager implements SettingsDependent, HasCleanup {
-
-    // Note: Proper expiration is set in reload(), which is also called on initialization
-    private final ExpiringMap<String, String> captchaCodes = new ExpiringMap<>(0, TimeUnit.MINUTES);
-    private int captchaLength;
-
-    /**
-     * Constructor.
-     *
-     * @param settings the settings instance
-     */
-    public AbstractCaptchaManager(Settings settings) {
-        initialize(settings);
-    }
-
-    /**
-     * Returns whether the given player is required to solve a captcha.
-     *
-     * @param name the name of the player to verify
-     * @return true if the player has to solve a captcha, false otherwise
-     */
-    public abstract boolean isCaptchaRequired(String name);
-
-    /**
-     * Returns the stored captcha for the player or generates and saves a new one.
-     *
-     * @param name the player's name
-     * @return the code the player is required to enter
-     */
-    public String getCaptchaCodeOrGenerateNew(String name) {
-        String code = captchaCodes.get(name.toLowerCase());
-        return code == null ? generateCode(name) : code;
-    }
-
-    /**
-     * 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
-     */
-    public String generateCode(String name) {
-        String code = RandomStringUtils.generate(captchaLength);
-        captchaCodes.put(name.toLowerCase(), code);
-        return code;
-    }
-
-    /**
-     * Checks the given code against the existing one and resets the player's auth failure count upon success.
-     *
-     * @param name the name of the player to check
-     * @param code the supplied code
-     * @return true if the code matches or if no captcha is required for the player, false otherwise
-     */
-    public boolean checkCode(String name, String code) {
-        final String nameLowerCase = name.toLowerCase();
-        String savedCode = captchaCodes.get(nameLowerCase);
-        if (savedCode != null && savedCode.equalsIgnoreCase(code)) {
-            captchaCodes.remove(nameLowerCase);
-            processSuccessfulCode(nameLowerCase);
-            return true;
-        } else {
-            processUnsuccessfulCode(nameLowerCase);
-        }
-        return false;
-    }
-
-    private void initialize(Settings settings) {
-        captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
-        captchaCodes.setExpiration(minutesBeforeCodeExpires(settings), TimeUnit.MINUTES);
-    }
-
-    /**
-     * Called on initialization and on reload.
-     *
-     * @param settings the settings instance
-     */
-    @Override
-    public void reload(Settings settings) {
-        // Note ljacqu 20171201: Use initialize() as an in-between method so that we can call it in the constructor
-        // without causing any trouble to a child that may extend reload -> at the point of calling, the child's fields
-        // would not yet be initialized.
-        initialize(settings);
-    }
-
-    @Override
-    public void performCleanup() {
-        captchaCodes.removeExpiredEntries();
-    }
-
-    /**
-     * Called when a player has successfully solved the captcha.
-     *
-     * @param nameLower the player's name (all lowercase)
-     */
-    protected abstract void processSuccessfulCode(String nameLower);
-
-    /**
-     * Called when a player has failed the captcha code.
-     *
-     * @param nameLower the player's name (all lowercase)
-     */
-    protected void processUnsuccessfulCode(String nameLower) {
-    }
-
-    /**
-     * Returns the number of minutes a generated captcha code should live for before it may expire.
-     *
-     * @param settings the settings instance
-     * @return number of minutes that the code is valid for
-     */
-    protected abstract int minutesBeforeCodeExpires(Settings settings);
-}
@@ -1,80 +0,0 @@
-package fr.xephi.authme.data;
-
-import fr.xephi.authme.settings.Settings;
-import fr.xephi.authme.settings.properties.SecuritySettings;
-import fr.xephi.authme.util.expiring.TimedCounter;
-
-import javax.inject.Inject;
-import java.util.concurrent.TimeUnit;
-
-/**
- * Manager for the handling of captchas after too many failed login attempts.
- */
-public class LoginCaptchaManager extends AbstractCaptchaManager {
-
-    // Note: proper expiration is set in reload(), which is also called on initialization by the parent
-    private final TimedCounter<String> playerCounts = new TimedCounter<>(0, TimeUnit.MINUTES);
-
-    private boolean isEnabled;
-    private int threshold;
-
-    @Inject
-    LoginCaptchaManager(Settings settings) {
-        super(settings);
-        reload(settings);
-    }
-
-    /**
-     * Increases the failure count for the given player.
-     *
-     * @param name the player's name
-     */
-    public void increaseLoginFailureCount(String name) {
-        if (isEnabled) {
-            String playerLower = name.toLowerCase();
-            playerCounts.increment(playerLower);
-        }
-    }
-
-    @Override
-    public boolean isCaptchaRequired(String playerName) {
-        return isEnabled && playerCounts.get(playerName.toLowerCase()) >= threshold;
-    }
-
-    /**
-     * Resets the login count of the given player to 0.
-     *
-     * @param name the player's name
-     */
-    public void resetLoginFailureCount(String name) {
-        if (isEnabled) {
-            playerCounts.remove(name.toLowerCase());
-        }
-    }
-
-    @Override
-    public void reload(Settings settings) {
-        super.reload(settings);
-
-        this.isEnabled = settings.getProperty(SecuritySettings.ENABLE_LOGIN_FAILURE_CAPTCHA);
-        this.threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TRIES_BEFORE_CAPTCHA);
-        long countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
-        playerCounts.setExpiration(countTimeout, TimeUnit.MINUTES);
-    }
-
-    @Override
-    public void performCleanup() {
-        super.performCleanup();
-        playerCounts.removeExpiredEntries();
-    }
-
-    @Override
-    protected void processSuccessfulCode(String nameLower) {
-        playerCounts.remove(nameLower);
-    }
-
-    @Override
-    protected int minutesBeforeCodeExpires(Settings settings) {
-        return settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
-    }
-}
@@ -1,71 +0,0 @@
-package fr.xephi.authme.data;
-
-import fr.xephi.authme.data.limbo.LimboService;
-import fr.xephi.authme.initialization.circulardependency.HasCircularDependency;
-import fr.xephi.authme.initialization.circulardependency.InjectAfterInitialization;
-import fr.xephi.authme.settings.Settings;
-import fr.xephi.authme.settings.properties.SecuritySettings;
-import fr.xephi.authme.util.expiring.ExpiringSet;
-import org.bukkit.Bukkit;
-import org.bukkit.entity.Player;
-
-import javax.inject.Inject;
-import java.util.concurrent.TimeUnit;
-
-/**
- * Captcha manager for registration.
- */
-public class RegistrationCaptchaManager extends AbstractCaptchaManager implements HasCircularDependency {
-
-    private static final int MINUTES_VALID_FOR_REGISTRATION = 30;
-
-    private LimboService limboService;
-
-    private final ExpiringSet<String> verifiedNamesForRegistration =
-        new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES);
-    private boolean isEnabled;
-
-    @Inject
-    RegistrationCaptchaManager(Settings settings) {
-        super(settings);
-        reload(settings);
-    }
-
-    @Override
-    public boolean isCaptchaRequired(String name) {
-        return isEnabled && !verifiedNamesForRegistration.contains(name.toLowerCase());
-    }
-
-    @Override
-    public void reload(Settings settings) {
-        super.reload(settings);
-        this.isEnabled = settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION);
-    }
-
-    @Override
-    public void performCleanup() {
-        super.performCleanup();
-        verifiedNamesForRegistration.removeExpiredEntries();
-    }
-
-    @Override
-    protected void processSuccessfulCode(String nameLower) {
-        verifiedNamesForRegistration.add(nameLower);
-    }
-
-    @Override
-    protected void processUnsuccessfulCode(String nameLower) {
-        final Player player = Bukkit.getPlayerExact(nameLower); // TODO #930: Pass in player!
-        limboService.resetMessageTask(player, false);
-    }
-
-    @Override
-    protected int minutesBeforeCodeExpires(Settings settings) {
-        return MINUTES_VALID_FOR_REGISTRATION;
-    }
-
-    @InjectAfterInitialization
-    public void setLimboService(LimboService limboService) {
-        this.limboService = limboService;
-    }
-}
@@ -0,0 +1,90 @@
+package fr.xephi.authme.data.captcha;
+
+import fr.xephi.authme.util.RandomStringUtils;
+import fr.xephi.authme.util.expiring.ExpiringMap;
+
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Primitive service for storing captcha codes.
+ */
+public class CaptchaCodeStorage {
+
+    /** Map of captcha codes (with player name as key, case-insensitive). */
+    private ExpiringMap<String, String> captchaCodes;
+    /** Number of characters newly generated captcha codes should have. */
+    private int captchaLength;
+
+    /**
+     * Constructor.
+     *
+     * @param expirationInMinutes minutes after which a saved captcha code expires
+     * @param captchaLength the number of characters a captcha code should have
+     */
+    public CaptchaCodeStorage(long expirationInMinutes, int captchaLength) {
+        this.captchaCodes = new ExpiringMap<>(expirationInMinutes, TimeUnit.MINUTES);
+        this.captchaLength = captchaLength;
+    }
+
+    /**
+     * Sets the expiration of captcha codes.
+     *
+     * @param expirationInMinutes minutes after which a saved captcha code expires
+     */
+    public void setExpirationInMinutes(long expirationInMinutes) {
+        captchaCodes.setExpiration(expirationInMinutes, TimeUnit.MINUTES);
+    }
+
+    /**
+     * Sets the captcha length.
+     *
+     * @param captchaLength number of characters a captcha code should have
+     */
+    public void setCaptchaLength(int captchaLength) {
+        this.captchaLength = captchaLength;
+    }
+
+    /**
+     * Returns the stored captcha for the player or generates and saves a new one.
+     *
+     * @param name the player's name
+     * @return the code the player is required to enter
+     */
+    public String getCodeOrGenerateNew(String name) {
+        String code = captchaCodes.get(name.toLowerCase());
+        return code == null ? generateCode(name) : code;
+    }
+
+    /**
+     * 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
+     */
+    public String generateCode(String name) {
+        String code = RandomStringUtils.generate(captchaLength);
+        captchaCodes.put(name.toLowerCase(), code);
+        return code;
+    }
+
+    /**
+     * Checks the given code against the existing one. Upon success, the saved captcha code is removed from storage.
+     *
+     * @param name the name of the player to check
+     * @param code the supplied code
+     * @return true if the code matches, false otherwise
+     */
+    public boolean checkCode(String name, String code) {
+        String nameLowerCase = name.toLowerCase();
+        String savedCode = captchaCodes.get(nameLowerCase);
+        if (savedCode != null && savedCode.equalsIgnoreCase(code)) {
+            captchaCodes.remove(nameLowerCase);
+            return true;
+        }
+        return false;
+    }
+
+    public void removeExpiredEntries() {
+        captchaCodes.removeExpiredEntries();
+    }
+}
@@ -0,0 +1,44 @@
+package fr.xephi.authme.data.captcha;
+
+import org.bukkit.entity.Player;
+
+/**
+ * Manages captcha codes.
+ */
+public interface CaptchaManager {
+
+    /**
+     * Returns whether the given player is required to solve a captcha.
+     *
+     * @param name the name of the player to verify
+     * @return true if the player has to solve a captcha, false otherwise
+     */
+    boolean isCaptchaRequired(String name);
+
+    /**
+     * Returns the stored captcha for the player or generates and saves a new one.
+     *
+     * @param name the player's name
+     * @return the code the player is required to enter
+     */
+    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.
+     *
+     * @param player the player to check
+     * @param code the supplied code
+     * @return true if the code matches, false otherwise
+     */
+    boolean checkCode(Player player, String code);
+
+}
@@ -0,0 +1,99 @@
+package fr.xephi.authme.data.captcha;
+
+import fr.xephi.authme.initialization.HasCleanup;
+import fr.xephi.authme.initialization.SettingsDependent;
+import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.util.expiring.TimedCounter;
+import org.bukkit.entity.Player;
+
+import javax.inject.Inject;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Manager for the handling of captchas after too many failed login attempts.
+ */
+public class LoginCaptchaManager implements CaptchaManager, SettingsDependent, HasCleanup {
+
+    private final TimedCounter<String> playerCounts;
+    private final CaptchaCodeStorage captchaCodeStorage;
+
+    private boolean isEnabled;
+    private int threshold;
+
+    @Inject
+    LoginCaptchaManager(Settings settings) {
+        // Note: Proper values are set in reload()
+        this.captchaCodeStorage = new CaptchaCodeStorage(30, 4);
+        this.playerCounts = new TimedCounter<>(9, TimeUnit.MINUTES);
+        reload(settings);
+    }
+
+    /**
+     * Increases the failure count for the given player.
+     *
+     * @param name the player's name
+     */
+    public void increaseLoginFailureCount(String name) {
+        if (isEnabled) {
+            String playerLower = name.toLowerCase();
+            playerCounts.increment(playerLower);
+        }
+    }
+
+    @Override
+    public boolean isCaptchaRequired(String playerName) {
+        return isEnabled && playerCounts.get(playerName.toLowerCase()) >= threshold;
+    }
+
+    @Override
+    public String getCaptchaCodeOrGenerateNew(String name) {
+        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) {
+            playerCounts.remove(nameLower);
+        }
+        return isCodeCorrect;
+    }
+
+    /**
+     * Resets the login count of the given player to 0.
+     *
+     * @param name the player's name
+     */
+    public void resetLoginFailureCount(String name) {
+        if (isEnabled) {
+            playerCounts.remove(name.toLowerCase());
+        }
+    }
+
+    @Override
+    public void reload(Settings settings) {
+        int expirationInMinutes = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
+        captchaCodeStorage.setExpirationInMinutes(expirationInMinutes);
+        int captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
+        captchaCodeStorage.setCaptchaLength(captchaLength);
+
+        int countTimeout = settings.getProperty(SecuritySettings.CAPTCHA_COUNT_MINUTES_BEFORE_RESET);
+        playerCounts.setExpiration(countTimeout, TimeUnit.MINUTES);
+
+        isEnabled = settings.getProperty(SecuritySettings.ENABLE_LOGIN_FAILURE_CAPTCHA);
+        threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TRIES_BEFORE_CAPTCHA);
+    }
+
+    @Override
+    public void performCleanup() {
+        playerCounts.removeExpiredEntries();
+        captchaCodeStorage.removeExpiredEntries();
+    }
+}
@@ -0,0 +1,83 @@
+package fr.xephi.authme.data.captcha;
+
+import fr.xephi.authme.data.limbo.LimboService;
+import fr.xephi.authme.initialization.HasCleanup;
+import fr.xephi.authme.initialization.SettingsDependent;
+import fr.xephi.authme.initialization.circulardependency.HasCircularDependency;
+import fr.xephi.authme.initialization.circulardependency.InjectAfterInitialization;
+import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.util.expiring.ExpiringSet;
+import org.bukkit.entity.Player;
+
+import javax.inject.Inject;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Captcha manager for registration.
+ */
+public class RegistrationCaptchaManager
+    implements CaptchaManager, SettingsDependent, HasCleanup, HasCircularDependency {
+
+    private static final int MINUTES_VALID_FOR_REGISTRATION = 30;
+
+    private LimboService limboService;
+
+    private final ExpiringSet<String> verifiedNamesForRegistration;
+    private final CaptchaCodeStorage captchaCodeStorage;
+    private boolean isEnabled;
+
+    @Inject
+    RegistrationCaptchaManager(Settings settings) {
+        // NOTE: proper captcha length is set in reload()
+        this.captchaCodeStorage = new CaptchaCodeStorage(MINUTES_VALID_FOR_REGISTRATION, 4);
+        this.verifiedNamesForRegistration = new ExpiringSet<>(MINUTES_VALID_FOR_REGISTRATION, TimeUnit.MINUTES);
+        reload(settings);
+    }
+
+    @Override
+    public boolean isCaptchaRequired(String name) {
+        return isEnabled && !verifiedNamesForRegistration.contains(name.toLowerCase());
+    }
+
+    @Override
+    public String getCaptchaCodeOrGenerateNew(String name) {
+        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);
+        }
+        return isCodeCorrect;
+    }
+
+    @Override
+    public void reload(Settings settings) {
+        int captchaLength = settings.getProperty(SecuritySettings.CAPTCHA_LENGTH);
+        captchaCodeStorage.setCaptchaLength(captchaLength);
+
+        isEnabled = settings.getProperty(SecuritySettings.ENABLE_CAPTCHA_FOR_REGISTRATION);
+    }
+
+    @Override
+    public void performCleanup() {
+        verifiedNamesForRegistration.removeExpiredEntries();
+        captchaCodeStorage.removeExpiredEntries();
+    }
+
+    @InjectAfterInitialization
+    public void setLimboService(LimboService limboService) {
+        this.limboService = limboService;
+    }
+}
@@ -1,7 +1,7 @@
 package fr.xephi.authme.data.limbo;

-import fr.xephi.authme.data.RegistrationCaptchaManager;
 import fr.xephi.authme.data.auth.PlayerCache;
+import fr.xephi.authme.data.captcha.RegistrationCaptchaManager;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
 import fr.xephi.authme.service.BukkitService;
@@ -2,7 +2,7 @@ package fr.xephi.authme.process.login;

 import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.ConsoleLogger;
-import fr.xephi.authme.data.LoginCaptchaManager;
+import fr.xephi.authme.data.captcha.LoginCaptchaManager;
 import fr.xephi.authme.data.TempbanManager;
 import fr.xephi.authme.data.auth.PlayerAuth;
 import fr.xephi.authme.data.auth.PlayerCache;