diff --git a/src/main/java/fr/xephi/authme/cache/TempbanManager.java b/src/main/java/fr/xephi/authme/cache/TempbanManager.java index eacbd183a..8042b6039 100644 --- a/src/main/java/fr/xephi/authme/cache/TempbanManager.java +++ b/src/main/java/fr/xephi/authme/cache/TempbanManager.java @@ -18,9 +18,10 @@ import java.util.concurrent.ConcurrentHashMap; /** * Manager for handling tempbans */ +// TODO Gnat008 20160613: Figure out the best way to remove entries based on time public class TempbanManager implements SettingsDependent { - private final ConcurrentHashMap playerCounts; + private final ConcurrentHashMap ipLoginFailureCounts; private final long MINUTE_IN_MILLISECONDS = 60000; @@ -34,45 +35,49 @@ public class TempbanManager implements SettingsDependent { @Inject TempbanManager(BukkitService bukkitService, Messages messages, NewSetting settings) { - this.playerCounts = new ConcurrentHashMap<>(); + this.ipLoginFailureCounts = new ConcurrentHashMap<>(); this.bukkitService = bukkitService; this.messages = messages; loadSettings(settings); } /** - * Increases the failure count for the given player. + * Increases the failure count for the given IP address. * - * @param name the player's name + * @param address The player's IP address */ - public void increaseCount(String name) { + public void increaseCount(String address) { if (isEnabled) { - String nameLower = name.toLowerCase(); - Integer count = playerCounts.get(nameLower); + Integer count = ipLoginFailureCounts.get(address); if (count == null) { - playerCounts.put(nameLower, 1); + ipLoginFailureCounts.put(address, 1); } else { - playerCounts.put(nameLower, count + 1); + ipLoginFailureCounts.put(address, count + 1); } } } - public void resetCount(String name) { + /** + * Set the failure count for a given IP address to 0. + * + * @param address The IP address + */ + public void resetCount(String address) { if (isEnabled) { - playerCounts.remove(name.toLowerCase()); + ipLoginFailureCounts.remove(address); } } /** - * Return whether the player should be tempbanned. + * Return whether the IP address should be tempbanned. * - * @param name The player's name - * @return True if the player should be tempbanned + * @param address The player's IP address + * @return True if the IP should be tempbanned */ - public boolean shouldTempban(String name) { + public boolean shouldTempban(String address) { if (isEnabled) { - Integer count = playerCounts.get(name.toLowerCase()); + Integer count = ipLoginFailureCounts.get(address); return count != null && count >= threshold; } @@ -80,16 +85,13 @@ public class TempbanManager implements SettingsDependent { } /** - * Tempban a player for failing to log in too many times. - * This bans the player's IP address, and calculates the expire - * time based on the time the method was called. + * Tempban a player's IP address for failing to log in too many times. + * This calculates the expire time based on the time the method was called. * * @param player The player to tempban */ public void tempbanPlayer(final Player player) { if (isEnabled) { - resetCount(player.getName()); - final String ip = Utils.getPlayerIp(player); final String reason = messages.retrieveSingle(MessageKey.TEMPBAN_MAX_LOGINS); @@ -104,6 +106,8 @@ public class TempbanManager implements SettingsDependent { player.kickPlayer(reason); } }); + + resetCount(ip); } } diff --git a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java index e60851e3a..33b6defad 100644 --- a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java +++ b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java @@ -90,19 +90,6 @@ public class AsynchronousLogin implements AsynchronousProcess { return captchaManager.isCaptchaRequired(playerName); } - /** - * Queries the {@link fr.xephi.authme.cache.TempbanManager} to - * see if the player has reached the tempban threshold. - * - * @param player The player to check - * @return True if the player needs to be tempbanned - */ - private boolean shouldTempban(Player player) { - final String playerName = player.getName(); - - return tempbanManager.shouldTempban(playerName); - } - /** * Checks the precondition for authentication (like user known) and returns * the playerAuth-State @@ -168,9 +155,9 @@ public class AsynchronousLogin implements AsynchronousProcess { final String ip = Utils.getPlayerIp(player); // Increase the counts here before knowing the result of the login. - // If the login is successful, we clear the count for the player. + // If the login is successful, we clear the captcha count for the player. captchaManager.increaseCount(name); - tempbanManager.increaseCount(name); + tempbanManager.increaseCount(ip); if ("127.0.0.1".equals(pAuth.getIp()) && !pAuth.getIp().equals(ip)) { pAuth.setIp(ip); @@ -191,7 +178,6 @@ public class AsynchronousLogin implements AsynchronousProcess { database.updateSession(auth); captchaManager.resetCounts(name); - tempbanManager.resetCount(name); player.setNoDamageTicks(0); if (!forceLogin) @@ -237,7 +223,7 @@ public class AsynchronousLogin implements AsynchronousProcess { player.kickPlayer(service.retrieveSingleMessage(MessageKey.WRONG_PASSWORD)); } }); - } else if (shouldTempban(player)) { + } else if (tempbanManager.shouldTempban(ip)) { tempbanManager.tempbanPlayer(player); } else { service.send(player, MessageKey.WRONG_PASSWORD); diff --git a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java index bf02253b5..e960a084a 100644 --- a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java +++ b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java @@ -34,76 +34,76 @@ public class TempbanManagerTest { // given NewSetting settings = mockSettings(3, 60); TempbanManager manager = new TempbanManager(bukkitService, messages, settings); - String player = "Tester"; + String address = "192.168.1.1"; // when for (int i = 0; i < 2; ++i) { - manager.increaseCount(player); + manager.increaseCount(address); } // then - assertThat(manager.shouldTempban(player), equalTo(false)); - manager.increaseCount(player); - assertThat(manager.shouldTempban(player), equalTo(true)); - assertThat(manager.shouldTempban("otherPlayer"), equalTo(false)); + assertThat(manager.shouldTempban(address), equalTo(false)); + manager.increaseCount(address); + assertThat(manager.shouldTempban(address), equalTo(true)); + assertThat(manager.shouldTempban("10.0.0.1"), equalTo(false)); } @Test public void shouldIncreaseAndResetCount() { // given - String player = "plaYah"; + String address = "192.168.1.2"; NewSetting settings = mockSettings(3, 60); TempbanManager manager = new TempbanManager(bukkitService, messages, settings); // when - manager.increaseCount(player); - manager.increaseCount(player); - manager.increaseCount(player); + manager.increaseCount(address); + manager.increaseCount(address); + manager.increaseCount(address); // then - assertThat(manager.shouldTempban(player), equalTo(true)); - assertHasCount(manager, player, 3); + assertThat(manager.shouldTempban(address), equalTo(true)); + assertHasCount(manager, address, 3); // when 2 - manager.resetCount(player); + manager.resetCount(address); // then 2 - assertThat(manager.shouldTempban(player), equalTo(false)); - assertHasCount(manager, player, null); + assertThat(manager.shouldTempban(address), equalTo(false)); + assertHasCount(manager, address, null); } @Test public void shouldNotIncreaseCountForDisabledTempban() { // given - String player = "playah"; + String address = "192.168.1.3"; NewSetting settings = mockSettings(1, 5); given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(false); TempbanManager manager = new TempbanManager(bukkitService, messages, settings); // when - manager.increaseCount(player); + manager.increaseCount(address); // then - assertThat(manager.shouldTempban(player), equalTo(false)); - assertHasCount(manager, player, null); + assertThat(manager.shouldTempban(address), equalTo(false)); + assertHasCount(manager, address, null); } @Test public void shouldNotCheckCountIfTempbanIsDisabled() { // given - String player = "playah"; + String address = "192.168.1.4"; NewSetting settings = mockSettings(1, 5); TempbanManager manager = new TempbanManager(bukkitService, messages, settings); given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(false); // when - manager.increaseCount(player); + manager.increaseCount(address); // assumptions - assertThat(manager.shouldTempban(player), equalTo(true)); - assertHasCount(manager, player, 1); + assertThat(manager.shouldTempban(address), equalTo(true)); + assertHasCount(manager, address, 1); // end assumptions manager.loadSettings(settings); - boolean result = manager.shouldTempban(player); + boolean result = manager.shouldTempban(address); // then assertThat(result, equalTo(false)); @@ -117,10 +117,10 @@ public class TempbanManagerTest { return settings; } - private static void assertHasCount(TempbanManager manager, String player, Integer count) { + private static void assertHasCount(TempbanManager manager, String address, Integer count) { @SuppressWarnings("unchecked") Map playerCounts = (Map) ReflectionTestUtils - .getFieldValue(TempbanManager.class, manager, "playerCounts"); - assertThat(playerCounts.get(player.toLowerCase()), equalTo(count)); + .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts"); + assertThat(playerCounts.get(address), equalTo(count)); } }