From 152d1dc2167089fb33da951e2e2e780b85e9e565 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 18 Feb 2017 22:50:30 +0100 Subject: [PATCH] #949 Created TimedCounter + implement it in TempbanManager --- .../fr/xephi/authme/data/TempbanManager.java | 90 ++++--------------- .../fr/xephi/authme/util/ExpiringMap.java | 15 +++- .../fr/xephi/authme/util/TimedCounter.java | 50 +++++++++++ .../xephi/authme/data/TempbanManagerTest.java | 64 +++++-------- .../fr/xephi/authme/util/ExpiringMapTest.java | 33 +++---- .../xephi/authme/util/TimedCounterTest.java | 55 ++++++++++++ 6 files changed, 172 insertions(+), 135 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/util/TimedCounter.java create mode 100644 src/test/java/fr/xephi/authme/util/TimedCounterTest.java diff --git a/src/main/java/fr/xephi/authme/data/TempbanManager.java b/src/main/java/fr/xephi/authme/data/TempbanManager.java index e5d31ed1a..07019a725 100644 --- a/src/main/java/fr/xephi/authme/data/TempbanManager.java +++ b/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> ipLoginFailureCounts; + private final Map> 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 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 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 map = ipLoginFailureCounts.get(address); - if (map != null) { - map.remove(name); + TimedCounter 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 countsByName = ipLoginFailureCounts.get(address); + TimedCounter 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 countsByIp : ipLoginFailureCounts.values()) { - Iterator 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 countsByIp : ipLoginFailureCounts.values()) { + countsByIp.removeExpiredEntries(); } + ipLoginFailureCounts.entrySet().removeIf(e -> e.getValue().isEmpty()); } } diff --git a/src/main/java/fr/xephi/authme/util/ExpiringMap.java b/src/main/java/fr/xephi/authme/util/ExpiringMap.java index 83beb37d6..519946f89 100644 --- a/src/main/java/fr/xephi/authme/util/ExpiringMap.java +++ b/src/main/java/fr/xephi/authme/util/ExpiringMap.java @@ -24,7 +24,7 @@ import java.util.concurrent.TimeUnit; */ public class ExpiringMap { - private final Map> entries = new ConcurrentHashMap<>(); + protected final Map> entries = new ConcurrentHashMap<>(); private long expirationMillis; /** @@ -88,12 +88,23 @@ public class ExpiringMap { 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 the value type */ - private static final class ExpiringEntry { + protected static final class ExpiringEntry { private final V value; private final long expiration; diff --git a/src/main/java/fr/xephi/authme/util/TimedCounter.java b/src/main/java/fr/xephi/authme/util/TimedCounter.java new file mode 100644 index 000000000..59c54d4d6 --- /dev/null +++ b/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. + *

+ * 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 extends ExpiringMap { + + /** + * 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); + } +} diff --git a/src/test/java/fr/xephi/authme/data/TempbanManagerTest.java b/src/test/java/fr/xephi/authme/data/TempbanManagerTest.java index 28c2ab9ee..4fd8ad117 100644 --- a/src/test/java/fr/xephi/authme/data/TempbanManagerTest.java +++ b/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 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)); + Map> counts = new HashMap<>(); + TimedCounter counter1 = mockCounter(); + given(counter1.isEmpty()).willReturn(true); + counts.put("11.11.11.11", counter1); + TimedCounter 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> 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> playerCounts = ReflectionTestUtils + Map> playerCounts = ReflectionTestUtils .getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts"); - Map map = playerCounts.get(address); - assertThat(map == null || map.isEmpty(), equalTo(true)); + TimedCounter counter = playerCounts.get(address); + assertThat(counter == null || counter.isEmpty(), equalTo(true)); } private static void assertHasCount(TempbanManager manager, String address, String name, int count) { - Map> playerCounts = ReflectionTestUtils + Map> 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 TimedCounter mockCounter() { + return mock(TimedCounter.class); } } diff --git a/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java b/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java index d4a3f8684..cdc9c36a5 100644 --- a/src/test/java/fr/xephi/authme/util/ExpiringMapTest.java +++ b/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 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 map = new ExpiringMap<>(-3, TimeUnit.MINUTES); - map.put(3, "trois"); - - // then - assertThat(map.get(3), nullValue()); - } - @Test public void shouldCleanUpExpiredEntries() throws InterruptedException { // given - ExpiringMap map = new ExpiringMap<>(400, TimeUnit.MILLISECONDS); + ExpiringMap 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 internalMap = ReflectionTestUtils.getFieldValue(ExpiringMap.class, map, "entries"); + Map internalMap = map.entries; assertThat(internalMap.keySet(), containsInAnyOrder(64, 25)); } + @Test + public void shouldReturnIfIsEmpty() { + // given + ExpiringMap 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)); + } } diff --git a/src/test/java/fr/xephi/authme/util/TimedCounterTest.java b/src/test/java/fr/xephi/authme/util/TimedCounterTest.java new file mode 100644 index 000000000..9903842de --- /dev/null +++ b/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 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 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 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)); + } +}