diff --git a/src/main/java/fr/xephi/authme/cache/TempbanManager.java b/src/main/java/fr/xephi/authme/cache/TempbanManager.java index 71f1ed90..f2d9cc5b 100644 --- a/src/main/java/fr/xephi/authme/cache/TempbanManager.java +++ b/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 { +public class TempbanManager implements SettingsDependent, HasCleanup { 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> 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 countsByIp : ipLoginFailureCounts.values()) { + Iterator it = countsByIp.values().iterator(); + while (it.hasNext()) { + TimedCounter counter = it.next(); + if (counter.getCount(resetThreshold) == 0) { + it.remove(); + } + } + } } /** diff --git a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java index d9806086..2134a312 100644 --- a/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java +++ b/src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java @@ -104,6 +104,11 @@ public class SecuritySettings implements SettingsClass { public static final Property 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 TEMPBAN_MINUTES_BEFORE_RESET = + newProperty("Security.tempban.minutesBeforeCounterReset", 480); + private SecuritySettings() { } diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index 8e8efad7..bfa09d74 100644 --- a/src/main/resources/config.yml +++ b/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 diff --git a/src/test/java/fr/xephi/authme/ReflectionTestUtils.java b/src/test/java/fr/xephi/authme/ReflectionTestUtils.java index 5724fb4e..fdd716bd 100644 --- a/src/test/java/fr/xephi/authme/ReflectionTestUtils.java +++ b/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 Field getField(Class 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 V getFieldValue(Class clazz, T instance, String fieldName) { Field field = getField(clazz, instance, fieldName); diff --git a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java index 8e2091bf..11540ddd 100644 --- a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java +++ b/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 map1 = new HashMap<>(); + map1.put("name", newTimedCounter(4, expirationPoint + 20_000)); + map1.put("other", newTimedCounter(2, expirationPoint + 40_000)); + // 0 current entries + Map 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 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> 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> 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; } }