count login failures by ip address and not by name

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<String, Integer> playerCounts;
+    private final ConcurrentHashMap<String, Integer> 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 = ipLoginFailureCounts.get(address);
-            Integer count = playerCounts.get(nameLower);
 
             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
+     * @param address The player's IP address
-     * @return True if the player should be tempbanned
+     * @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.
+     * Tempban a player's IP address for failing to log in too many times.
-     * This bans the player's IP address, and calculates the expire
+     * This calculates the expire time based on the time the method was called.
-     * 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);
         }
     }
 

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

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));
+        assertThat(manager.shouldTempban(address), equalTo(false));
-        manager.increaseCount(player);
+        manager.increaseCount(address);
-        assertThat(manager.shouldTempban(player), equalTo(true));
+        assertThat(manager.shouldTempban(address), equalTo(true));
-        assertThat(manager.shouldTempban("otherPlayer"), equalTo(false));
+        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(address);
-        manager.increaseCount(player);
+        manager.increaseCount(address);
-        manager.increaseCount(player);
+        manager.increaseCount(address);
 
         // then
-        assertThat(manager.shouldTempban(player), equalTo(true));
+        assertThat(manager.shouldTempban(address), equalTo(true));
-        assertHasCount(manager, player, 3);
+        assertHasCount(manager, address, 3);
 
         // when 2
-        manager.resetCount(player);
+        manager.resetCount(address);
 
         // then 2
-        assertThat(manager.shouldTempban(player), equalTo(false));
+        assertThat(manager.shouldTempban(address), equalTo(false));
-        assertHasCount(manager, player, null);
+        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));
+        assertThat(manager.shouldTempban(address), equalTo(false));
-        assertHasCount(manager, player, null);
+        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));
+        assertThat(manager.shouldTempban(address), equalTo(true));
-        assertHasCount(manager, player, 1);
+        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<String, Integer> playerCounts = (Map<String, Integer>) ReflectionTestUtils
-            .getFieldValue(TempbanManager.class, manager, "playerCounts");
+            .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
-        assertThat(playerCounts.get(player.toLowerCase()), equalTo(count));
+        assertThat(playerCounts.get(address), equalTo(count));
     }
 }