#876 Keep track of wrong logins by (ip, username) and implement threshold

src/main/java/fr/xephi/authme/cache/TempbanManager.java

@@ -1,5 +1,6 @@
 package fr.xephi.authme.cache;
 
+import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.initialization.SettingsDependent;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.output.Messages;
@@ -11,17 +12,20 @@ import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
 import java.util.Date;
+import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
 /**
- * Manager for handling tempbans
+ * Manager for handling temporary bans.
  */
-// TODO Gnat008 20160613: Figure out the best way to remove entries based on time
+// TODO #876: Implement HasCleanup interface
 public class TempbanManager implements SettingsDependent {
 
-    private static final long MINUTE_IN_MILLISECONDS = 60000;
+    private static final long MINUTE_IN_MILLISECONDS = 60_000;
+    // TODO #876: Make a setting out of this
+    private static final long COUNTER_RETENTION_MILLIS = 6 * 60 * MINUTE_IN_MILLISECONDS;
 
-    private final ConcurrentHashMap<String, Integer> ipLoginFailureCounts;
+    private final Map<String, Map<String, TimedCounter>> ipLoginFailureCounts;
     private final BukkitService bukkitService;
     private final Messages messages;
 
@@ -38,30 +42,40 @@ public class TempbanManager implements SettingsDependent {
     }
 
     /**
-     * Increases the failure count for the given IP address.
+     * Increases the failure count for the given IP address/username combination.
      *
      * @param address The player's IP address
+     * @param name The username
      */
-    public void increaseCount(String address) {
+    public void increaseCount(String address, String name) {
         if (isEnabled) {
-            Integer count = ipLoginFailureCounts.get(address);
+            Map<String, TimedCounter> countsByName = ipLoginFailureCounts.get(address);
+            if (countsByName == null) {
+                countsByName = new ConcurrentHashMap<>();
+                ipLoginFailureCounts.put(address, countsByName);
+            }
 
-            if (count == null) {
+            TimedCounter counter = countsByName.get(name);
-                ipLoginFailureCounts.put(address, 1);
+            if (counter == null) {
+                countsByName.put(name, new TimedCounter(1));
             } else {
-                ipLoginFailureCounts.put(address, count + 1);
+                counter.increment(COUNTER_RETENTION_MILLIS);
             }
         }
     }
 
     /**
-     * Set the failure count for a given IP address to 0.
+     * Set the failure count for a given IP address / username combination to 0.
      *
      * @param address The IP address
+     * @param name The username
      */
-    public void resetCount(String address) {
+    public void resetCount(String address, String name) {
         if (isEnabled) {
-            ipLoginFailureCounts.remove(address);
+            Map<String, TimedCounter> map = ipLoginFailureCounts.get(address);
+            if (map != null) {
+                map.remove(name);
+            }
         }
     }
 
@@ -73,8 +87,14 @@ public class TempbanManager implements SettingsDependent {
      */
     public boolean shouldTempban(String address) {
         if (isEnabled) {
-            Integer count = ipLoginFailureCounts.get(address);
+            Map<String, TimedCounter> countsByName = ipLoginFailureCounts.get(address);
-            return count != null && count >= threshold;
+            if (countsByName != null) {
+                int total = 0;
+                for (TimedCounter counter : countsByName.values()) {
+                    total += counter.getCount(COUNTER_RETENTION_MILLIS);
+                }
+                return total >= threshold;
+            }
         }
 
         return false;
@@ -103,7 +123,7 @@ public class TempbanManager implements SettingsDependent {
                 }
             });
 
-            resetCount(ip);
+            ipLoginFailureCounts.remove(ip);
         }
     }
 
@@ -113,4 +133,46 @@ public class TempbanManager implements SettingsDependent {
         this.threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TEMPBAN);
         this.length = settings.getProperty(SecuritySettings.TEMPBAN_LENGTH);
     }
+
+    /**
+     * Counter with an associated timestamp, keeping track of when the last entry has been added.
+     */
+    @VisibleForTesting
+    static final class TimedCounter {
+
+        private int counter;
+        private long lastIncrementTimestamp = System.currentTimeMillis();
+
+        /**
+         * Constructor.
+         *
+         * @param start the initial value to set the counter to
+         */
+        TimedCounter(int start) {
+            this.counter = start;
+        }
+
+        /**
+         * Returns the count, taking into account the last entry timestamp.
+         *
+         * @param threshold the threshold in milliseconds until when to consider a counter
+         * @return the counter's value, or {@code 0} if it was last incremented longer ago than the threshold
+         */
+        int getCount(long threshold) {
+            if (System.currentTimeMillis() - lastIncrementTimestamp > threshold) {
+                return 0;
+            }
+            return counter;
+        }
+
+        /**
+         * Increments the counter, taking into account the last entry timestamp.
+         *
+         * @param threshold in milliseconds, the time span until which to consider the existing number
+         */
+        void increment(long threshold) {
+            counter = getCount(threshold) + 1;
+            lastIncrementTimestamp = System.currentTimeMillis();
+        }
+    }
 }

src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java

@@ -137,7 +137,7 @@ public class AsynchronousLogin implements AsynchronousProcess {
         // Increase the counts here before knowing the result of the login.
         // If the login is successful, we clear the captcha count for the player.
         captchaManager.increaseCount(name);
-        tempbanManager.increaseCount(ip);
+        tempbanManager.increaseCount(ip, name);
 
         String email = pAuth.getEmail();
         boolean passwordVerified = forceLogin || passwordSecurity.comparePassword(
@@ -153,6 +153,7 @@ public class AsynchronousLogin implements AsynchronousProcess {
             database.updateSession(auth);
 
             captchaManager.resetCounts(name);
+            tempbanManager.resetCount(ip, name);
             player.setNoDamageTicks(0);
 
             if (!forceLogin)

src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java

@@ -2,6 +2,7 @@ package fr.xephi.authme.cache;
 
 import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.TestHelper;
+import fr.xephi.authme.cache.TempbanManager.TimedCounter;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.output.Messages;
 import fr.xephi.authme.settings.Settings;
@@ -49,13 +50,14 @@ public class TempbanManagerTest {
         String address = "192.168.1.1";
 
         // when
-        for (int i = 0; i < 2; ++i) {
+        manager.increaseCount(address, "Bob");
-            manager.increaseCount(address);
+        manager.increaseCount(address, "Todd");
-        }
 
         // then
         assertThat(manager.shouldTempban(address), equalTo(false));
-        manager.increaseCount(address);
+        assertHasCount(manager, address, "Bob", 1);
+        assertHasCount(manager, address, "Todd", 1);
+        manager.increaseCount(address, "Bob");
         assertThat(manager.shouldTempban(address), equalTo(true));
         assertThat(manager.shouldTempban("10.0.0.1"), equalTo(false));
     }
@@ -68,20 +70,20 @@ public class TempbanManagerTest {
         TempbanManager manager = new TempbanManager(bukkitService, messages, settings);
 
         // when
-        manager.increaseCount(address);
+        manager.increaseCount(address, "test");
-        manager.increaseCount(address);
+        manager.increaseCount(address, "test");
-        manager.increaseCount(address);
+        manager.increaseCount(address, "test");
 
         // then
         assertThat(manager.shouldTempban(address), equalTo(true));
-        assertHasCount(manager, address, 3);
+        assertHasCount(manager, address, "test", 3);
 
         // when 2
-        manager.resetCount(address);
+        manager.resetCount(address, "test");
 
         // then 2
         assertThat(manager.shouldTempban(address), equalTo(false));
-        assertHasCount(manager, address, null);
+        assertHasNoEntries(manager, address);
     }
 
     @Test
@@ -93,11 +95,11 @@ public class TempbanManagerTest {
         TempbanManager manager = new TempbanManager(bukkitService, messages, settings);
 
         // when
-        manager.increaseCount(address);
+        manager.increaseCount(address, "username");
 
         // then
         assertThat(manager.shouldTempban(address), equalTo(false));
-        assertHasCount(manager, address, null);
+        assertHasNoEntries(manager, address);
     }
 
     @Test
@@ -109,10 +111,10 @@ public class TempbanManagerTest {
         given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(false);
 
         // when
-        manager.increaseCount(address);
+        manager.increaseCount(address, "username");
         // assumptions
         assertThat(manager.shouldTempban(address), equalTo(true));
-        assertHasCount(manager, address, 1);
+        assertHasCount(manager, address, "username", 1);
         // end assumptions
         manager.reload(settings);
         boolean result = manager.shouldTempban(address);
@@ -173,9 +175,9 @@ public class TempbanManagerTest {
         given(messages.retrieveSingle(MessageKey.TEMPBAN_MAX_LOGINS)).willReturn(banReason);
         Settings settings = mockSettings(10, 60);
         TempbanManager manager = new TempbanManager(bukkitService, messages, settings);
-        manager.increaseCount(ip);
+        manager.increaseCount(ip, "user");
-        manager.increaseCount(ip);
+        manager.increaseCount(ip, "name2");
-        manager.increaseCount(ip);
+        manager.increaseCount(ip, "user");
 
         // when
         manager.tempbanPlayer(player);
@@ -183,7 +185,7 @@ public class TempbanManagerTest {
 
         // then
         verify(player).kickPlayer(banReason);
-        assertHasCount(manager, ip, null);
+        assertHasNoEntries(manager, ip);
     }
 
     private static Settings mockSettings(int maxTries, int tempbanLength) {
@@ -194,10 +196,18 @@ public class TempbanManagerTest {
         return settings;
     }
 
-    private static void assertHasCount(TempbanManager manager, String address, Integer count) {
+    @SuppressWarnings("unchecked")
-        @SuppressWarnings("unchecked")
+    private static void assertHasNoEntries(TempbanManager manager, String address) {
-        Map<String, Integer> playerCounts = (Map<String, Integer>) ReflectionTestUtils
+        Map<String, Map<?, ?>> playerCounts = (Map<String, Map<?, ?>>) ReflectionTestUtils
             .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
-        assertThat(playerCounts.get(address), equalTo(count));
+        Map map = playerCounts.get(address);
+        assertThat(map == null || map.isEmpty(), equalTo(true));
+    }
+
+    @SuppressWarnings("unchecked")
+    private static void assertHasCount(TempbanManager manager, String address, String name, int count) {
+        Map<String, Map<String, TimedCounter>> playerCounts = (Map<String, Map<String, TimedCounter>>)
+            ReflectionTestUtils.getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
+        assertThat(playerCounts.get(address).get(name).getCount(10000L), equalTo(count));
     }
 }