From 3411450ff1a6b4c051aa344842c310166363b360 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 14 Jun 2016 21:03:32 +0200 Subject: [PATCH 1/2] #760 Fix single session feature - Move the check from PlayerLoginEvent to AsyncPlayerPreLoginEvent. Single session can only be implemented with PreLoginEvent; it is already to late to check this in the PlayerLoginEvent. Ergo, we cannot offer this for CraftBukkit. - Remove interactions with LimboCache - no interactions with LimboCache expected until after OnJoinVerification checks. (Thanks sgdc3!) --- .../authme/listener/AuthMePlayerListener.java | 4 +++- .../xephi/authme/listener/OnJoinVerifier.java | 18 +++--------------- .../authme/listener/OnJoinVerifierTest.java | 19 +++++++------------ 3 files changed, 13 insertions(+), 28 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java index 67e57ee8..84fff695 100644 --- a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java +++ b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java @@ -217,6 +217,9 @@ public class AuthMePlayerListener implements Listener { onJoinVerifier.checkAntibot(name, isAuthAvailable); onJoinVerifier.checkKickNonRegistered(isAuthAvailable); onJoinVerifier.checkIsValidName(name); + // Note #760: Single session must be checked here - checking with PlayerLoginEvent is too late and + // the first connection will have been kicked. This means this feature doesn't work on CraftBukkit. + onJoinVerifier.checkSingleSession(name); } catch (FailedVerificationException e) { event.setKickMessage(m.retrieveSingle(e.getReason(), e.getArgs())); event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER); @@ -243,7 +246,6 @@ public class AuthMePlayerListener implements Listener { onJoinVerifier.checkKickNonRegistered(isAuthAvailable); onJoinVerifier.checkIsValidName(name); onJoinVerifier.checkNameCasing(player, auth); - onJoinVerifier.checkSingleSession(player); onJoinVerifier.checkPlayerCountry(isAuthAvailable, event); } catch (FailedVerificationException e) { event.setKickMessage(m.retrieveSingle(e.getReason(), e.getArgs())); diff --git a/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java b/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java index 64f8c3f7..297e74c6 100644 --- a/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java +++ b/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java @@ -3,9 +3,6 @@ package fr.xephi.authme.listener; import fr.xephi.authme.AntiBot; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.cache.auth.PlayerAuth; -import fr.xephi.authme.cache.auth.PlayerCache; -import fr.xephi.authme.cache.limbo.LimboCache; -import fr.xephi.authme.cache.limbo.LimboPlayer; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.Reloadable; import fr.xephi.authme.output.MessageKey; @@ -18,7 +15,6 @@ import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.util.BukkitService; import fr.xephi.authme.util.StringUtils; -import fr.xephi.authme.util.Utils; import fr.xephi.authme.util.ValidationService; import org.bukkit.Server; import org.bukkit.entity.Player; @@ -49,8 +45,6 @@ class OnJoinVerifier implements Reloadable { @Inject private BukkitService bukkitService; @Inject - private LimboCache limboCache; - @Inject private Server server; private Pattern nicknamePattern; @@ -187,21 +181,15 @@ class OnJoinVerifier implements Reloadable { * Checks if a player with the same name (case-insensitive) is already playing and refuses the * connection if so configured. * - * @param player the player to verify + * @param name the player name to check */ - public void checkSingleSession(Player player) throws FailedVerificationException { + public void checkSingleSession(String name) throws FailedVerificationException { if (!settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)) { return; } - Player onlinePlayer = bukkitService.getPlayerExact(player.getName()); + Player onlinePlayer = bukkitService.getPlayerExact(name); if (onlinePlayer != null) { - String name = player.getName().toLowerCase(); - LimboPlayer limbo = limboCache.getLimboPlayer(name); - if (limbo != null && PlayerCache.getInstance().isAuthenticated(name)) { - Utils.addNormal(player, limbo.getGroup()); - limboCache.deleteLimboPlayer(name); - } throw new FailedVerificationException(MessageKey.USERNAME_ALREADY_ONLINE_ERROR); } } diff --git a/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java b/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java index 1cf9d0c1..6d5d27e4 100644 --- a/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java +++ b/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java @@ -3,7 +3,6 @@ package fr.xephi.authme.listener; import fr.xephi.authme.AntiBot; import fr.xephi.authme.TestHelper; import fr.xephi.authme.cache.auth.PlayerAuth; -import fr.xephi.authme.cache.limbo.LimboCache; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.output.Messages; @@ -67,8 +66,6 @@ public class OnJoinVerifierTest { @Mock private BukkitService bukkitService; @Mock - private LimboCache limboCache; - @Mock private Server server; @Rule @@ -341,21 +338,21 @@ public class OnJoinVerifierTest { @Test public void shouldAcceptNameThatIsNotOnline() throws FailedVerificationException { // given - Player player = newPlayerWithName("bobby"); + String name = "bobby"; given(settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)).willReturn(true); given(bukkitService.getPlayerExact("bobby")).willReturn(null); // when - onJoinVerifier.checkSingleSession(player); + onJoinVerifier.checkSingleSession(name); // then - verifyZeroInteractions(limboCache); + verify(bukkitService).getPlayerExact(name); } @Test public void shouldRejectNameAlreadyOnline() throws FailedVerificationException { // given - Player player = newPlayerWithName("Charlie"); + String name = "Charlie"; Player onlinePlayer = newPlayerWithName("charlie"); given(bukkitService.getPlayerExact("Charlie")).willReturn(onlinePlayer); given(settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)).willReturn(true); @@ -364,22 +361,20 @@ public class OnJoinVerifierTest { expectValidationExceptionWith(MessageKey.USERNAME_ALREADY_ONLINE_ERROR); // when / then - onJoinVerifier.checkSingleSession(player); - verify(limboCache).getLimboPlayer("charlie"); + onJoinVerifier.checkSingleSession(name); } @Test public void shouldAcceptAlreadyOnlineNameForDisabledSetting() throws FailedVerificationException { // given - Player player = newPlayerWithName("Felipe"); + String name = "Felipe"; given(settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)).willReturn(false); // when - onJoinVerifier.checkSingleSession(player); + onJoinVerifier.checkSingleSession(name); // then verifyZeroInteractions(bukkitService); - verifyZeroInteractions(limboCache); } private static Player newPlayerWithName(String name) { From 5cbb83e15337a2c88ce4cdb52878f8e64f605ec6 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 14 Jun 2016 21:52:43 +0200 Subject: [PATCH 2/2] Code householding, add tests to TempbanManager - Delegate event firing to BukkitService - Write tests for IP banning function - Update comments on tempban properties in config.yml --- .../fr/xephi/authme/cache/TempbanManager.java | 14 ++-- .../authme/process/join/AsynchronousJoin.java | 2 +- .../process/login/ProcessSyncPlayerLogin.java | 2 +- .../register/ProcessSyncPasswordRegister.java | 2 +- .../fr/xephi/authme/util/BukkitService.java | 19 +++++ src/main/resources/config.yml | 6 +- src/test/java/fr/xephi/authme/TestHelper.java | 18 ++++ .../authme/cache/TempbanManagerTest.java | 83 ++++++++++++++++++- .../executable/authme/GetIpCommandTest.java | 8 +- 9 files changed, 130 insertions(+), 24 deletions(-) diff --git a/src/main/java/fr/xephi/authme/cache/TempbanManager.java b/src/main/java/fr/xephi/authme/cache/TempbanManager.java index 8042b603..0a55353c 100644 --- a/src/main/java/fr/xephi/authme/cache/TempbanManager.java +++ b/src/main/java/fr/xephi/authme/cache/TempbanManager.java @@ -7,8 +7,6 @@ import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.BukkitService; import fr.xephi.authme.util.Utils; -import org.bukkit.BanList; -import org.bukkit.Bukkit; import org.bukkit.entity.Player; import javax.inject.Inject; @@ -21,13 +19,11 @@ import java.util.concurrent.ConcurrentHashMap; // TODO Gnat008 20160613: Figure out the best way to remove entries based on time public class TempbanManager implements SettingsDependent { + private static final long MINUTE_IN_MILLISECONDS = 60000; + private final ConcurrentHashMap ipLoginFailureCounts; - - private final long MINUTE_IN_MILLISECONDS = 60000; - - private BukkitService bukkitService; - - private Messages messages; + private final BukkitService bukkitService; + private final Messages messages; private boolean isEnabled; private int threshold; @@ -102,7 +98,7 @@ public class TempbanManager implements SettingsDependent { bukkitService.scheduleSyncDelayedTask(new Runnable() { @Override public void run() { - Bukkit.getServer().getBanList(BanList.Type.IP).addBan(ip, reason, expires, "AuthMe"); + bukkitService.banIp(ip, reason, expires, "AuthMe"); player.kickPlayer(reason); } }); diff --git a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java index 27fe17de..5d1c2fca 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -124,7 +124,7 @@ public class AsynchronousJoin implements AsynchronousProcess { // Protect inventory if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN) && plugin.inventoryProtector != null) { ProtectInventoryEvent ev = new ProtectInventoryEvent(player); - plugin.getServer().getPluginManager().callEvent(ev); + bukkitService.callEvent(ev); if (ev.isCancelled()) { plugin.inventoryProtector.sendInventoryPacket(player); if (!service.getProperty(SecuritySettings.REMOVE_SPAM_FROM_CONSOLE)) { diff --git a/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java b/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java index d040d49f..28304e00 100644 --- a/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java +++ b/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java @@ -133,7 +133,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { } // The Login event now fires (as intended) after everything is processed - Bukkit.getServer().getPluginManager().callEvent(new LoginEvent(player)); + bukkitService.callEvent(new LoginEvent(player)); player.saveData(); if (service.getProperty(HooksSettings.BUNGEECORD)) { sendBungeeMessage(player); diff --git a/src/main/java/fr/xephi/authme/process/register/ProcessSyncPasswordRegister.java b/src/main/java/fr/xephi/authme/process/register/ProcessSyncPasswordRegister.java index cf936257..d8f790e5 100644 --- a/src/main/java/fr/xephi/authme/process/register/ProcessSyncPasswordRegister.java +++ b/src/main/java/fr/xephi/authme/process/register/ProcessSyncPasswordRegister.java @@ -124,7 +124,7 @@ public class ProcessSyncPasswordRegister implements SynchronousProcess { } // The LoginEvent now fires (as intended) after everything is processed - plugin.getServer().getPluginManager().callEvent(new LoginEvent(player)); + bukkitService.callEvent(new LoginEvent(player)); player.saveData(); if (!service.getProperty(SecuritySettings.REMOVE_SPAM_FROM_CONSOLE)) { diff --git a/src/main/java/fr/xephi/authme/util/BukkitService.java b/src/main/java/fr/xephi/authme/util/BukkitService.java index ac8a408b..8b94fe49 100644 --- a/src/main/java/fr/xephi/authme/util/BukkitService.java +++ b/src/main/java/fr/xephi/authme/util/BukkitService.java @@ -2,6 +2,8 @@ package fr.xephi.authme.util; import fr.xephi.authme.AuthMe; import fr.xephi.authme.ConsoleLogger; +import org.bukkit.BanEntry; +import org.bukkit.BanList; import org.bukkit.Bukkit; import org.bukkit.OfflinePlayer; import org.bukkit.World; @@ -15,6 +17,7 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Date; import java.util.Set; /** @@ -206,4 +209,20 @@ public class BukkitService { return false; } + /** + * Adds a ban to the this list. If a previous ban exists, this will + * update the previous entry. + * + * @param ip the ip of the ban + * @param reason reason for the ban, null indicates implementation default + * @param expires date for the ban's expiration (unban), or null to imply + * forever + * @param source source of the ban, null indicates implementation default + * @return the entry for the newly created ban, or the entry for the + * (updated) previous ban + */ + public BanEntry banIp(String ip, String reason, Date expires, String source) { + return Bukkit.getServer().getBanList(BanList.Type.IP).addBan(ip, reason, expires, source); + } + } diff --git a/src/main/resources/config.yml b/src/main/resources/config.yml index e7c67763..99cc3a2b 100644 --- a/src/main/resources/config.yml +++ b/src/main/resources/config.yml @@ -332,11 +332,11 @@ Security: # information correctly without any corruption. kickPlayersBeforeStopping: true tempban: - # Tempban users if they enter the wrong password too many times + # Tempban a user's IP address if they enter the wrong password too many times enableTempban: false - # How many times a user can attempt to login before being tempbanned + # How many times a user can attempt to login before their IP being tempbanned maxLoginTries: 10 - # The length of time a player will be tempbanned in minutes + # The length of time a IP address will be tempbanned in minutes # Default: 480 minutes, or 8 hours tempbanLength: 480 Converter: diff --git a/src/test/java/fr/xephi/authme/TestHelper.java b/src/test/java/fr/xephi/authme/TestHelper.java index 907cb8e2..bef65621 100644 --- a/src/test/java/fr/xephi/authme/TestHelper.java +++ b/src/test/java/fr/xephi/authme/TestHelper.java @@ -1,6 +1,7 @@ package fr.xephi.authme; import fr.xephi.authme.util.BukkitService; +import org.bukkit.entity.Player; import org.mockito.ArgumentCaptor; import org.mockito.Mockito; @@ -8,6 +9,8 @@ import java.io.File; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Modifier; +import java.net.InetAddress; +import java.net.InetSocketAddress; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; @@ -15,7 +18,9 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.logging.Logger; +import static org.mockito.BDDMockito.given; import static org.mockito.Matchers.anyLong; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; /** @@ -144,4 +149,17 @@ public final class TestHelper { } } + /** + * Configures the player mock to return the given IP address. + * + * @param player the player mock + * @param ip the ip address it should return + */ + public static void mockPlayerIp(Player player, String ip) { + InetAddress inetAddress = mock(InetAddress.class); + given(inetAddress.getHostAddress()).willReturn(ip); + InetSocketAddress inetSocketAddress = new InetSocketAddress(inetAddress, 8093); + given(player.getAddress()).willReturn(inetSocketAddress); + } + } diff --git a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java index e960a084..b3c6fc03 100644 --- a/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java +++ b/src/test/java/fr/xephi/authme/cache/TempbanManagerTest.java @@ -1,21 +1,31 @@ package fr.xephi.authme.cache; import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.TestHelper; +import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.output.Messages; import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.BukkitService; +import org.bukkit.entity.Player; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import java.util.Calendar; +import java.util.Date; import java.util.Map; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.lessThan; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; /** * Test for {@link TempbanManager}. @@ -23,11 +33,13 @@ import static org.mockito.Mockito.mock; @RunWith(MockitoJUnitRunner.class) public class TempbanManagerTest { - @Mock - BukkitService bukkitService; + private static final long DATE_TOLERANCE_MILLISECONDS = 100L; @Mock - Messages messages; + private BukkitService bukkitService; + + @Mock + private Messages messages; @Test public void shouldAddCounts() { @@ -109,6 +121,71 @@ public class TempbanManagerTest { assertThat(result, equalTo(false)); } + @Test + public void shouldNotIssueBanIfDisabled() { + // given + NewSetting settings = mockSettings(0, 0); + given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(false); + Player player = mock(Player.class); + TempbanManager manager = new TempbanManager(bukkitService, messages, settings); + + // when + manager.tempbanPlayer(player); + + // then + verifyZeroInteractions(player, bukkitService); + } + + @Test + public void shouldBanPlayerIp() { + // given + Player player = mock(Player.class); + String ip = "123.45.67.89"; + TestHelper.mockPlayerIp(player, ip); + String banReason = "IP ban too many logins"; + given(messages.retrieveSingle(MessageKey.TEMPBAN_MAX_LOGINS)).willReturn(banReason); + NewSetting settings = mockSettings(2, 100); + TempbanManager manager = new TempbanManager(bukkitService, messages, settings); + + // when + manager.tempbanPlayer(player); + TestHelper.runSyncDelayedTask(bukkitService); + + // then + verify(player).kickPlayer(banReason); + ArgumentCaptor captor = ArgumentCaptor.forClass(Date.class); + verify(bukkitService).banIp(eq(ip), eq(banReason), captor.capture(), eq("AuthMe")); + + // Compute the expected expiration date and check that the actual date is within the difference tolerance + Calendar cal = Calendar.getInstance(); + cal.add(Calendar.MINUTE, 100); + long expectedExpiration = cal.getTime().getTime(); + assertThat(Math.abs(captor.getValue().getTime() - expectedExpiration), lessThan(DATE_TOLERANCE_MILLISECONDS)); + } + + @Test + public void shouldResetCountAfterBan() { + // given + Player player = mock(Player.class); + String ip = "22.44.66.88"; + TestHelper.mockPlayerIp(player, ip); + String banReason = "kick msg"; + given(messages.retrieveSingle(MessageKey.TEMPBAN_MAX_LOGINS)).willReturn(banReason); + NewSetting settings = mockSettings(10, 60); + TempbanManager manager = new TempbanManager(bukkitService, messages, settings); + manager.increaseCount(ip); + manager.increaseCount(ip); + manager.increaseCount(ip); + + // when + manager.tempbanPlayer(player); + TestHelper.runSyncDelayedTask(bukkitService); + + // then + verify(player).kickPlayer(banReason); + assertHasCount(manager, ip, null); + } + private static NewSetting mockSettings(int maxTries, int tempbanLength) { NewSetting settings = mock(NewSetting.class); given(settings.getProperty(SecuritySettings.TEMPBAN_ON_MAX_LOGINS)).willReturn(true); diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/GetIpCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/GetIpCommandTest.java index d3d5c376..65ed998a 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/GetIpCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/GetIpCommandTest.java @@ -1,5 +1,6 @@ package fr.xephi.authme.command.executable.authme; +import fr.xephi.authme.TestHelper; import fr.xephi.authme.util.BukkitService; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; @@ -9,8 +10,6 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; -import java.net.InetAddress; -import java.net.InetSocketAddress; import java.util.Collections; import static org.hamcrest.Matchers.allOf; @@ -68,10 +67,7 @@ public class GetIpCommandTest { private static Player mockPlayer(String name, String ip) { Player player = mock(Player.class); given(player.getName()).willReturn(name); - InetAddress inetAddress = mock(InetAddress.class); - given(inetAddress.getHostAddress()).willReturn(ip); - InetSocketAddress inetSocketAddress = new InetSocketAddress(inetAddress, 8093); - given(player.getAddress()).willReturn(inetSocketAddress); + TestHelper.mockPlayerIp(player, ip); return player; } }