From 5cbb83e15337a2c88ce4cdb52878f8e64f605ec6 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 14 Jun 2016 21:52:43 +0200 Subject: [PATCH] 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 8042b6039..0a55353c7 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 27fe17dea..5d1c2fca4 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 d040d49f0..28304e005 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 cf936257d..d8f790e58 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 ac8a408b8..8b94fe49e 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 e7c67763b..99cc3a2b9 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 907cb8e20..bef656219 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 e960a084a..b3c6fc03c 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 d3d5c3760..65ed998a7 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; } }