From 2417bf4c3fb523a0d31bfc6ba79a0824dc097627 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 27 Aug 2016 21:28:11 +0200 Subject: [PATCH] #876 Keep track of wrong logins by (ip, username) and implement threshold --- .../fr/xephi/authme/cache/TempbanManager.java | 94 +++++++++++++++---- .../process/login/AsynchronousLogin.java | 3 +- .../authme/cache/TempbanManagerTest.java | 54 ++++++----- 3 files changed, 112 insertions(+), 39 deletions(-) diff --git a/src/main/java/fr/xephi/authme/cache/TempbanManager.java b/src/main/java/fr/xephi/authme/cache/TempbanManager.java index c5f0ee6f..71f1ed90 100644 --- a/src/main/java/fr/xephi/authme/cache/TempbanManager.java +++ b/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 ipLoginFailureCounts; + private final Map> 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 countsByName = ipLoginFailureCounts.get(address); + if (countsByName == null) { + countsByName = new ConcurrentHashMap<>(); + ipLoginFailureCounts.put(address, countsByName); + } - if (count == null) { - ipLoginFailureCounts.put(address, 1); + TimedCounter counter = countsByName.get(name); + 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 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); - return count != null && count >= threshold; + Map countsByName = ipLoginFailureCounts.get(address); + 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(); + } + } } diff --git a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java b/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java index cb4df02c..b672a24b 100644 --- a/src/main/java/fr/xephi/authme/process/login/AsynchronousLogin.java +++ b/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) diff --git a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java index 911900e4..44eb4b0c 100644 --- a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java +++ b/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); - } + manager.increaseCount(address, "Bob"); + 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); - manager.increaseCount(address); + manager.increaseCount(address, "test"); + manager.increaseCount(address, "test"); + 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); - manager.increaseCount(ip); + manager.increaseCount(ip, "user"); + manager.increaseCount(ip, "name2"); + 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") - Map playerCounts = (Map) ReflectionTestUtils + @SuppressWarnings("unchecked") + private static void assertHasNoEntries(TempbanManager manager, String address) { + Map> playerCounts = (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> playerCounts = (Map>) + ReflectionTestUtils.getFieldValue(TempbanManager.class, manager, "ipLoginFailureCounts"); + assertThat(playerCounts.get(address).get(name).getCount(10000L), equalTo(count)); } }