#949 Created TimedCounter + implement it in TempbanManager

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

@@ -1,21 +1,21 @@
 package fr.xephi.authme.data;
 
-import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.initialization.HasCleanup;
 import fr.xephi.authme.initialization.SettingsDependent;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
+import fr.xephi.authme.service.BukkitService;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
-import fr.xephi.authme.service.BukkitService;
+import fr.xephi.authme.util.TimedCounter;
 import fr.xephi.authme.util.PlayerUtils;
 import org.bukkit.entity.Player;
 
 import javax.inject.Inject;
 import java.util.Date;
-import java.util.Iterator;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
 
 import static fr.xephi.authme.settings.properties.SecuritySettings.TEMPBAN_MINUTES_BEFORE_RESET;
 import static fr.xephi.authme.util.Utils.MILLIS_PER_MINUTE;
@@ -25,7 +25,7 @@ import static fr.xephi.authme.util.Utils.MILLIS_PER_MINUTE;
  */
 public class TempbanManager implements SettingsDependent, HasCleanup {
 
-    private final Map<String, Map<String, TimedCounter>> ipLoginFailureCounts;
+    private final Map<String, TimedCounter<String>> ipLoginFailureCounts;
     private final BukkitService bukkitService;
     private final Messages messages;
 
@@ -50,18 +50,9 @@ public class TempbanManager implements SettingsDependent, HasCleanup {
      */
     public void increaseCount(String address, String name) {
         if (isEnabled) {
-            Map<String, TimedCounter> countsByName = ipLoginFailureCounts.get(address);
-            if (countsByName == null) {
-                countsByName = new ConcurrentHashMap<>();
-                ipLoginFailureCounts.put(address, countsByName);
-            }
-
-            TimedCounter counter = countsByName.get(name);
-            if (counter == null) {
-                countsByName.put(name, new TimedCounter(1));
-            } else {
-                counter.increment(resetThreshold);
-            }
+            TimedCounter<String> countsByName = ipLoginFailureCounts.computeIfAbsent(
+                address, k -> new TimedCounter<>(resetThreshold, TimeUnit.MINUTES));
+            countsByName.increment(name);
         }
     }
 
@@ -73,9 +64,9 @@ public class TempbanManager implements SettingsDependent, HasCleanup {
      */
     public void resetCount(String address, String name) {
         if (isEnabled) {
-            Map<String, TimedCounter> map = ipLoginFailureCounts.get(address);
-            if (map != null) {
-                map.remove(name);
+            TimedCounter<String> counter = ipLoginFailureCounts.get(address);
+            if (counter != null) {
+                counter.remove(name);
             }
         }
     }
@@ -88,13 +79,9 @@ public class TempbanManager implements SettingsDependent, HasCleanup {
      */
     public boolean shouldTempban(String address) {
         if (isEnabled) {
-            Map<String, TimedCounter> countsByName = ipLoginFailureCounts.get(address);
+            TimedCounter<String> countsByName = ipLoginFailureCounts.get(address);
             if (countsByName != null) {
-                int total = 0;
-                for (TimedCounter counter : countsByName.values()) {
-                    total += counter.getCount(resetThreshold);
-                }
-                return total >= threshold;
+                return countsByName.total() >= threshold;
             }
         }
         return false;
@@ -137,56 +124,9 @@ public class TempbanManager implements SettingsDependent, HasCleanup {
 
     @Override
     public void performCleanup() {
-        for (Map<String, TimedCounter> countsByIp : ipLoginFailureCounts.values()) {
-            Iterator<TimedCounter> it = countsByIp.values().iterator();
-            while (it.hasNext()) {
-                TimedCounter counter = it.next();
-                if (counter.getCount(resetThreshold) == 0) {
-                    it.remove();
-                }
-            }
-        }
-    }
-
-    /**
-     * 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();
+        for (TimedCounter<String> countsByIp : ipLoginFailureCounts.values()) {
+            countsByIp.removeExpiredEntries();
         }
+        ipLoginFailureCounts.entrySet().removeIf(e -> e.getValue().isEmpty());
     }
 }

src/main/java/fr/xephi/authme/util/ExpiringMap.java

@@ -24,7 +24,7 @@ import java.util.concurrent.TimeUnit;
  */
 public class ExpiringMap<K, V> {
 
-    private final Map<K, ExpiringEntry<V>> entries = new ConcurrentHashMap<>();
+    protected final Map<K, ExpiringEntry<V>> entries = new ConcurrentHashMap<>();
     private long expirationMillis;
 
     /**
@@ -88,12 +88,23 @@ public class ExpiringMap<K, V> {
         this.expirationMillis = unit.toMillis(duration);
     }
 
+    /**
+     * Returns whether this map is empty. This reflects the state of the
+     * internal map, which may contain expired entries only. The result
+     * may change after running {@link #removeExpiredEntries()}.
+     *
+     * @return true if map is really empty, false otherwise
+     */
+    public boolean isEmpty() {
+        return entries.isEmpty();
+    }
+
     /**
      * Class holding a value paired with an expiration timestamp.
      *
      * @param <V> the value type
      */
-    private static final class ExpiringEntry<V> {
+    protected static final class ExpiringEntry<V> {
 
         private final V value;
         private final long expiration;

src/main/java/fr/xephi/authme/util/TimedCounter.java

@@ -0,0 +1,50 @@
+package fr.xephi.authme.util;
+
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Keeps a count per key which expires after a configurable amount of time.
+ * <p>
+ * Once the expiration of an entry has been reached, the counter resets
+ * to 0. The counter returns 0 rather than {@code null} for any given key.
+ */
+public class TimedCounter<K> extends ExpiringMap<K, Integer> {
+
+    /**
+     * Constructor.
+     *
+     * @param duration the duration of time after which entries expire
+     * @param unit the time unit in which {@code duration} is expressed
+     */
+    public TimedCounter(long duration, TimeUnit unit) {
+        super(duration, unit);
+    }
+
+    @Override
+    public Integer get(K key) {
+        Integer value = super.get(key);
+        return value == null ? 0 : value;
+    }
+
+    /**
+     * Increments the value stored for the provided key.
+     *
+     * @param key the key to increment the counter for
+     */
+    public void increment(K key) {
+        put(key, get(key) + 1);
+    }
+
+    /**
+     * Calculates the total of all non-expired entries in this counter.
+     *
+     * @return the total of all valid entries
+     */
+    public int total() {
+        return entries.values().stream()
+            .map(ExpiringEntry::getValue)
+            .filter(Objects::nonNull)
+            .reduce(0, Integer::sum);
+    }
+}

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

@@ -2,12 +2,12 @@ package fr.xephi.authme.data;
 
 import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.TestHelper;
-import fr.xephi.authme.data.TempbanManager.TimedCounter;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
 import fr.xephi.authme.service.BukkitService;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.util.TimedCounter;
 import org.bukkit.entity.Player;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -20,8 +20,7 @@ import java.util.Date;
 import java.util.HashMap;
 import java.util.Map;
 
-import static org.hamcrest.Matchers.aMapWithSize;
-import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.lessThan;
 import static org.junit.Assert.assertThat;
@@ -195,42 +194,24 @@ public class TempbanManagerTest {
     @Test
     public void shouldPerformCleanup() {
         // given
-        // `expirationPoint` is the approximate timestamp until which entries should be considered, so subtracting
-        // from it will create expired entries, and adding a reasonably large number makes it still valid
-        final long expirationPoint = System.currentTimeMillis() - TEST_EXPIRATION_THRESHOLD;
-        // 2 current entries with total 6 failed tries
-        Map<String, TimedCounter> map1 = new HashMap<>();
-        map1.put("name", newTimedCounter(4, expirationPoint + 20_000));
-        map1.put("other", newTimedCounter(2, expirationPoint + 40_000));
-        // 0 current entries
-        Map<String, TimedCounter> map2 = new HashMap<>();
-        map2.put("someone", newTimedCounter(10, expirationPoint - 5_000));
-        map2.put("somebody", newTimedCounter(10, expirationPoint - 8_000));
-        // 1 current entry with total 4 failed tries
-        Map<String, TimedCounter> map3 = new HashMap<>();
-        map3.put("some", newTimedCounter(5, expirationPoint - 12_000));
-        map3.put("test", newTimedCounter(4, expirationPoint + 8_000));
-        map3.put("values", newTimedCounter(2, expirationPoint - 80_000));
+        Map<String, TimedCounter<String>> counts = new HashMap<>();
+        TimedCounter<String> counter1 = mockCounter();
+        given(counter1.isEmpty()).willReturn(true);
+        counts.put("11.11.11.11", counter1);
+        TimedCounter<String> counter2 = mockCounter();
+        given(counter2.isEmpty()).willReturn(false);
+        counts.put("33.33.33.33", counter2);
 
-        String[] addresses = {"123.45.67.89", "127.0.0.1", "192.168.0.1"};
-        Map<String, Map<String, TimedCounter>> counterMap = new HashMap<>();
-        counterMap.put(addresses[0], map1);
-        counterMap.put(addresses[1], map2);
-        counterMap.put(addresses[2], map3);
-
-        TempbanManager manager = new TempbanManager(bukkitService, messages, mockSettings(5, 250));
-        ReflectionTestUtils.setField(TempbanManager.class, manager, "ipLoginFailureCounts", counterMap);
+        TempbanManager manager = new TempbanManager(bukkitService, messages, mockSettings(3, 10));
+        ReflectionTestUtils.setField(TempbanManager.class, manager, "ipLoginFailureCounts", counts);
 
         // when
         manager.performCleanup();
 
         // then
-        assertThat(counterMap.get(addresses[0]), aMapWithSize(2));
-        assertHasCount(manager, addresses[0], "name", 4);
-        assertHasCount(manager, addresses[0], "other", 2);
-        assertThat(counterMap.get(addresses[1]), anEmptyMap());
-        assertThat(counterMap.get(addresses[2]), aMapWithSize(1));
-        assertHasCount(manager, addresses[2], "test", 4);
+        verify(counter1).removeExpiredEntries();
+        verify(counter2).removeExpiredEntries();
+        assertThat(counts.keySet(), contains("33.33.33.33"));
     }
 
     private static Settings mockSettings(int maxTries, int tempbanLength) {
@@ -244,21 +225,20 @@ public class TempbanManagerTest {
     }
 
     private static void assertHasNoEntries(TempbanManager manager, String address) {
-        Map<String, Map<String, TimedCounter>> playerCounts = ReflectionTestUtils
+        Map<String, TimedCounter<String>> playerCounts = ReflectionTestUtils
             .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
-        Map<String, TimedCounter> map = playerCounts.get(address);
-        assertThat(map == null || map.isEmpty(), equalTo(true));
+        TimedCounter<String> counter = playerCounts.get(address);
+        assertThat(counter == null || counter.isEmpty(), equalTo(true));
     }
 
     private static void assertHasCount(TempbanManager manager, String address, String name, int count) {
-        Map<String, Map<String, TimedCounter>> playerCounts = ReflectionTestUtils
+        Map<String, TimedCounter<String>> playerCounts = ReflectionTestUtils
             .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
-        assertThat(playerCounts.get(address).get(name).getCount(TEST_EXPIRATION_THRESHOLD), equalTo(count));
+        assertThat(playerCounts.get(address).get(name), equalTo(count));
     }
 
-    private static TimedCounter newTimedCounter(int count, long timestamp) {
-        TimedCounter counter = new TimedCounter(count);
-        ReflectionTestUtils.setField(TimedCounter.class, counter, "lastIncrementTimestamp", timestamp);
-        return counter;
+    @SuppressWarnings("unchecked")
+    private static <T> TimedCounter<T> mockCounter() {
+        return mock(TimedCounter.class);
     }
 }

src/test/java/fr/xephi/authme/util/ExpiringMapTest.java

@@ -1,6 +1,5 @@
 package fr.xephi.authme.util;
 
-import fr.xephi.authme.ReflectionTestUtils;
 import org.junit.Test;
 
 import java.util.Map;
@@ -45,14 +44,14 @@ public class ExpiringMapTest {
     }
 
     @Test
-    public void shouldUpdateExpiration() {
+    public void shouldUpdateExpirationAndSupportNegativeValues() {
         // given
         ExpiringMap<Integer, Integer> map = new ExpiringMap<>(2, TimeUnit.DAYS);
         map.put(2, 4);
         map.put(3, 9);
 
         // when
-        map.setExpiration(0, TimeUnit.SECONDS);
+        map.setExpiration(-100, TimeUnit.MILLISECONDS);
 
         // then
         map.put(5, 25);
@@ -61,20 +60,10 @@ public class ExpiringMapTest {
         assertThat(map.get(5), nullValue());
     }
 
-    @Test
-    public void shouldAcceptNegativeExpiration() {
-        // given / when
-        ExpiringMap<Integer, String> map = new ExpiringMap<>(-3, TimeUnit.MINUTES);
-        map.put(3, "trois");
-
-        // then
-        assertThat(map.get(3), nullValue());
-    }
-
     @Test
     public void shouldCleanUpExpiredEntries() throws InterruptedException {
         // given
-        ExpiringMap<Integer, Integer> map = new ExpiringMap<>(400, TimeUnit.MILLISECONDS);
+        ExpiringMap<Integer, Integer> map = new ExpiringMap<>(200, TimeUnit.MILLISECONDS);
         map.put(144, 12);
         map.put(121, 11);
         map.put(81, 9);
@@ -83,12 +72,24 @@ public class ExpiringMapTest {
         map.put(25, 5);
 
         // when
-        Thread.sleep(400);
+        Thread.sleep(300);
         map.removeExpiredEntries();
 
         // then
-        Map<Integer, ?> internalMap = ReflectionTestUtils.getFieldValue(ExpiringMap.class, map, "entries");
+        Map<Integer, ?> internalMap = map.entries;
         assertThat(internalMap.keySet(), containsInAnyOrder(64, 25));
     }
 
+    @Test
+    public void shouldReturnIfIsEmpty() {
+        // given
+        ExpiringMap<String, String> map = new ExpiringMap<>(-8, TimeUnit.SECONDS);
+
+        // when / then
+        assertThat(map.isEmpty(), equalTo(true));
+        map.put("hoi", "Welt");
+        assertThat(map.isEmpty(), equalTo(false));
+        map.removeExpiredEntries();
+        assertThat(map.isEmpty(), equalTo(true));
+    }
 }

src/test/java/fr/xephi/authme/util/TimedCounterTest.java

@@ -0,0 +1,55 @@
+package fr.xephi.authme.util;
+
+import org.junit.Test;
+
+import java.util.concurrent.TimeUnit;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.Assert.assertThat;
+
+/**
+ * Test for {@link TimedCounter}.
+ */
+public class TimedCounterTest {
+
+    @Test
+    public void shouldReturnZeroForAnyKey() {
+        // given
+        TimedCounter<Double> counter = new TimedCounter<>(1, TimeUnit.DAYS);
+
+        // when / then
+        assertThat(counter.get(2.0), equalTo(0));
+        assertThat(counter.get(-3.14159), equalTo(0));
+    }
+
+    @Test
+    public void shouldIncrementCount() {
+        // given
+        TimedCounter<String> counter = new TimedCounter<>(10, TimeUnit.MINUTES);
+        counter.put("moto", 12);
+
+        // when
+        counter.increment("hello");
+        counter.increment("moto");
+
+        // then
+        assertThat(counter.get("hello"), equalTo(1));
+        assertThat(counter.get("moto"), equalTo(13));
+    }
+
+    @Test
+    public void shouldSumUpEntries() {
+        // given
+        TimedCounter<String> counter = new TimedCounter<>(90, TimeUnit.SECONDS);
+        counter.entries.put("expired", new ExpiringMap.ExpiringEntry<>(800, 0));
+        counter.entries.put("expired2", new ExpiringMap.ExpiringEntry<>(24, System.currentTimeMillis() - 100));
+        counter.put("other", 10);
+        counter.put("Another", 4);
+
+        // when
+        int totals = counter.total();
+
+        // then
+        assertThat(totals, equalTo(14));
+    }
+}