#876 Make expiration configurable and implement cleanup for old entries

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

@@ -1,6 +1,7 @@
 package fr.xephi.authme.cache;
 
 import com.google.common.annotations.VisibleForTesting;
+import fr.xephi.authme.initialization.HasCleanup;
 import fr.xephi.authme.initialization.SettingsDependent;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.output.Messages;
@@ -12,18 +13,18 @@ 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 static fr.xephi.authme.settings.properties.SecuritySettings.TEMPBAN_MINUTES_BEFORE_RESET;
+
 /**
  * Manager for handling temporary bans.
  */
-// TODO #876: Implement HasCleanup interface
+public class TempbanManager implements SettingsDependent, HasCleanup {
-public class TempbanManager implements SettingsDependent {
 
     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 Map<String, Map<String, TimedCounter>> ipLoginFailureCounts;
     private final BukkitService bukkitService;
@@ -32,6 +33,7 @@ public class TempbanManager implements SettingsDependent {
     private boolean isEnabled;
     private int threshold;
     private int length;
+    private long resetThreshold;
 
     @Inject
     TempbanManager(BukkitService bukkitService, Messages messages, Settings settings) {
@@ -59,7 +61,7 @@ public class TempbanManager implements SettingsDependent {
             if (counter == null) {
                 countsByName.put(name, new TimedCounter(1));
             } else {
-                counter.increment(COUNTER_RETENTION_MILLIS);
+                counter.increment(resetThreshold);
             }
         }
     }
@@ -91,12 +93,11 @@ public class TempbanManager implements SettingsDependent {
             if (countsByName != null) {
                 int total = 0;
                 for (TimedCounter counter : countsByName.values()) {
-                    total += counter.getCount(COUNTER_RETENTION_MILLIS);
+                    total += counter.getCount(resetThreshold);
                 }
                 return total >= threshold;
             }
         }
-
         return false;
     }
 
@@ -132,6 +133,20 @@ public class TempbanManager implements SettingsDependent {
         this.isEnabled = settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS);
         this.threshold = settings.getProperty(SecuritySettings.MAX_LOGIN_TEMPBAN);
         this.length = settings.getProperty(SecuritySettings.TEMPBAN_LENGTH);
+        this.resetThreshold = settings.getProperty(TEMPBAN_MINUTES_BEFORE_RESET) * MINUTE_IN_MILLISECONDS;
+    }
+
+    @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();
+                }
+            }
+        }
     }
 
     /**

src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java

@@ -104,6 +104,11 @@ public class SecuritySettings implements SettingsClass {
     public static final Property<Integer> TEMPBAN_LENGTH =
         newProperty("Security.tempban.tempbanLength", 480);
 
+    @Comment({"How many minutes before resetting the count for failed logins by IP and username",
+        "Default: 480 minutes (8 hours)"})
+    public static final Property<Integer> TEMPBAN_MINUTES_BEFORE_RESET =
+        newProperty("Security.tempban.minutesBeforeCounterReset", 480);
+
     private SecuritySettings() {
     }
 

src/main/resources/config.yml

@@ -334,6 +334,9 @@ Security:
         # The length of time a IP address will be tempbanned in minutes
         # Default: 480 minutes, or 8 hours
         tempbanLength: 480
+        # How many minutes before resetting the count for failed logins by IP and username
+        # Default: 480 minutes (8 hours)
+        minutesBeforeCounterReset: 480
 Converter:
     Rakamak:
         # Rakamak file name

src/test/java/fr/xephi/authme/ReflectionTestUtils.java

@@ -34,15 +34,6 @@ public final class ReflectionTestUtils {
         }
     }
 
-    public static void setField(Field field, Object instance, Object value) {
-        try {
-            field.setAccessible(true);
-            field.set(instance, value);
-        } catch (IllegalAccessException e) {
-            throw new UnsupportedOperationException(e);
-        }
-    }
-
     private static <T> Field getField(Class<T> clazz, T instance, String fieldName) {
         try {
             Field field = clazz.getDeclaredField(fieldName);
@@ -54,16 +45,6 @@ public final class ReflectionTestUtils {
         }
     }
 
-    public static Object getFieldValue(Field field, Object instance) {
-        try {
-            field.setAccessible(true);
-            return field.get(instance);
-        } catch (IllegalAccessException e) {
-            throw new UnsupportedOperationException("Cannot get value of field '"
-                + field + "' for '" + instance + "'", e);
-        }
-    }
-
     @SuppressWarnings("unchecked")
     public static <T, V> V getFieldValue(Class<T> clazz, T instance, String fieldName) {
         Field field = getField(clazz, instance, fieldName);

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

@@ -17,8 +17,11 @@ import org.mockito.runners.MockitoJUnitRunner;
 
 import java.util.Calendar;
 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.equalTo;
 import static org.hamcrest.Matchers.lessThan;
 import static org.junit.Assert.assertThat;
@@ -35,6 +38,7 @@ import static org.mockito.Mockito.verifyZeroInteractions;
 public class TempbanManagerTest {
 
     private static final long DATE_TOLERANCE_MILLISECONDS = 200L;
+    private static final long TEST_EXPIRATION_THRESHOLD = 120_000L;
 
     @Mock
     private BukkitService bukkitService;
@@ -188,11 +192,54 @@ public class TempbanManagerTest {
         assertHasNoEntries(manager, ip);
     }
 
+    @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));
+
+        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);
+
+        // 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);
+    }
+
     private static Settings mockSettings(int maxTries, int tempbanLength) {
         Settings settings = mock(Settings.class);
         given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(true);
         given(settings.getProperty(SecuritySettings.MAX_LOGIN_TEMPBAN)).willReturn(maxTries);
         given(settings.getProperty(SecuritySettings.TEMPBAN_LENGTH)).willReturn(tempbanLength);
+        given(settings.getProperty(SecuritySettings.TEMPBAN_MINUTES_BEFORE_RESET))
+            .willReturn((int) TEST_EXPIRATION_THRESHOLD / 60_000);
         return settings;
     }
 
@@ -206,6 +253,12 @@ public class TempbanManagerTest {
     private static void assertHasCount(TempbanManager manager, String address, String name, int count) {
         Map<String, Map<String, TimedCounter>> playerCounts = ReflectionTestUtils
             .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts");
-        assertThat(playerCounts.get(address).get(name).getCount(10000L), equalTo(count));
+        assertThat(playerCounts.get(address).get(name).getCount(TEST_EXPIRATION_THRESHOLD), equalTo(count));
+    }
+
+    private static TimedCounter newTimedCounter(int count, long timestamp) {
+        TimedCounter counter = new TimedCounter(count);
+        ReflectionTestUtils.setField(TimedCounter.class, counter, "lastIncrementTimestamp", timestamp);
+        return counter;
     }
 }