Merge branch 'master' into 674-purge-process-refactor

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<String, Integer> ipLoginFailureCounts;
-
+    private final BukkitService bukkitService;
-    private final long MINUTE_IN_MILLISECONDS = 60000;
+    private final Messages messages;
-
-    private BukkitService bukkitService;
-
-    private 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);
                 }
             });

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()));

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);
         }
     }

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)) {

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);

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)) {

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);
+    }
+
 }

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:

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);
+    }
+
 }

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
+    private static final long DATE_TOLERANCE_MILLISECONDS = 100L;
-    BukkitService bukkitService;
 
     @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<Date> 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);

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);
+        TestHelper.mockPlayerIp(player, ip);
-        given(inetAddress.getHostAddress()).willReturn(ip);
-        InetSocketAddress inetSocketAddress = new InetSocketAddress(inetAddress, 8093);
-        given(player.getAddress()).willReturn(inetSocketAddress);
         return player;
     }
 }

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);
+        onJoinVerifier.checkSingleSession(name);
-        verify(limboCache).getLimboPlayer("charlie");
     }
 
     @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) {