Structure checks done in PlayerJoinListener as individual methods

src/main/java/fr/xephi/authme/AntiBot.java

@@ -29,8 +29,8 @@ public class AntiBot {
     private AntiBotStatus antiBotStatus = AntiBotStatus.DISABLED;
 
     @Inject
-    public AntiBot(NewSetting settings, Messages messages, PermissionsManager permissionsManager,
+    AntiBot(NewSetting settings, Messages messages, PermissionsManager permissionsManager,
-                   BukkitService bukkitService) {
+            BukkitService bukkitService) {
         this.settings = settings;
         this.messages = messages;
         this.permissionsManager = permissionsManager;
@@ -86,7 +86,12 @@ public class AntiBot {
         }, duration * TICKS_PER_MINUTE);
     }
 
-    public void checkAntiBot(final Player player) {
+    /**
+     * Handles a player joining the server and checks if AntiBot needs to be activated.
+     *
+     * @param player the player who joined the server
+     */
+    public void handlePlayerJoin(final Player player) {
         if (antiBotStatus == AntiBotStatus.ACTIVE || antiBotStatus == AntiBotStatus.DISABLED) {
             return;
         }

src/main/java/fr/xephi/authme/listener/AuthMePlayerJoinListener.java

@@ -22,6 +22,7 @@ import fr.xephi.authme.settings.properties.ProtectionSettings;
 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.entity.Player;
@@ -60,79 +61,42 @@ public class AuthMePlayerJoinListener implements Listener, Reloadable {
     private ValidationService validationService;
     @Inject
     private AuthMe plugin;
+    @Inject
+    private LimboCache limboCache;
 
     private Pattern nicknamePattern;
 
     @EventHandler(priority = EventPriority.LOW)
     public void onPlayerJoin(PlayerJoinEvent event) {
         final Player player = event.getPlayer();
-        if (player == null) {
+        if (player != null) {
-            return;
+            // Schedule login task so works after the prelogin
+            // (Fix found by Koolaid5000)
+            bukkitService.runTask(new Runnable() {
+                @Override
+                public void run() {
+                    management.performJoin(player);
+                }
+            });
         }
-
-        // Schedule login task so works after the prelogin
-        // (Fix found by Koolaid5000)
-        bukkitService.runTask(new Runnable() {
-            @Override
-            public void run() {
-                management.performJoin(player);
-            }
-        });
     }
 
     // Note ljacqu 20160528: AsyncPlayerPreLoginEvent is not fired by all servers in offline mode
+    // e.g. CraftBukkit does not. So we need to run crucial things in onPlayerLogin, too
     @EventHandler(priority = EventPriority.HIGHEST)
     public void onPreLogin(AsyncPlayerPreLoginEvent event) {
-        PlayerAuth auth = dataSource.getAuth(event.getName());
-        if (auth == null && antiBot.getAntiBotStatus() == AntiBot.AntiBotStatus.ACTIVE) {
-            event.setKickMessage(m.retrieveSingle(MessageKey.KICK_ANTIBOT));
-            event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
-            antiBot.antibotKicked.addIfAbsent(event.getName());
-            return;
-        }
-        if (auth == null && settings.getProperty(RestrictionSettings.KICK_NON_REGISTERED)) {
-            event.setKickMessage(m.retrieveSingle(MessageKey.MUST_REGISTER_MESSAGE));
-            event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
-            return;
-        }
         final String name = event.getName().toLowerCase();
-        if (name.length() > settings.getProperty(RestrictionSettings.MAX_NICKNAME_LENGTH) || name.length() < settings.getProperty(RestrictionSettings.MIN_NICKNAME_LENGTH)) {
+        final boolean isAuthAvailable = dataSource.isAuthAvailable(event.getName());
-            event.setKickMessage(m.retrieveSingle(MessageKey.INVALID_NAME_LENGTH));
-            event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
-            return;
-        }
-        if (settings.getProperty(RegistrationSettings.PREVENT_OTHER_CASE) && auth != null && auth.getRealName() != null) {
-            String realName = auth.getRealName();
-            if (!realName.isEmpty() && !"Player".equals(realName) && !realName.equals(event.getName())) {
-                event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
-                event.setKickMessage(m.retrieveSingle(MessageKey.INVALID_NAME_CASE, realName, event.getName()));
-                return;
-            }
-            if (realName.isEmpty() || "Player".equals(realName)) {
-                dataSource.updateRealName(event.getName().toLowerCase(), event.getName());
-            }
-        }
 
-        if (auth == null && settings.getProperty(ProtectionSettings.ENABLE_PROTECTION)) {
+        try {
-            String playerIp = event.getAddress().getHostAddress();
+            // Potential performance improvement: make checkAntiBot not require `isAuthAvailable` info and use
-            if (!validationService.isCountryAdmitted(playerIp)) {
+            // "checkKickNonRegistered" as last -> no need to query the DB before checking antibot / name
-                event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
+            checkAntibot(name, isAuthAvailable);
-                event.setKickMessage(m.retrieveSingle(MessageKey.COUNTRY_BANNED_ERROR));
+            checkKickNonRegistered(isAuthAvailable);
-                return;
+            checkIsValidName(name);
-            }
+        } catch (VerificationFailedException e) {
-        }
+            event.setKickMessage(m.retrieveSingle(e.getReason(), e.getArgs()));
-
-        final Player player = bukkitService.getPlayerExact(name);
-        // Check if forceSingleSession is set to true, so kick player that has
-        // joined with same nick of online player
-        if (player != null && settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)) {
             event.setLoginResult(AsyncPlayerPreLoginEvent.Result.KICK_OTHER);
-            event.setKickMessage(m.retrieveSingle(MessageKey.USERNAME_ALREADY_ONLINE_ERROR));
-            LimboPlayer limbo = LimboCache.getInstance().getLimboPlayer(name);
-            if (limbo != null && PlayerCache.getInstance().isAuthenticated(name)) {
-                Utils.addNormal(player, limbo.getGroup());
-                LimboCache.getInstance().deleteLimboPlayer(name);
-            }
         }
     }
 
@@ -141,65 +105,30 @@ public class AuthMePlayerJoinListener implements Listener, Reloadable {
         final Player player = event.getPlayer();
         if (player == null || Utils.isUnrestricted(player)) {
             return;
-        }
+        } else if (refusePlayerForFullServer(event)) {
-
+            return;
-        if (event.getResult() == PlayerLoginEvent.Result.KICK_FULL) {
+        } else if (event.getResult() != PlayerLoginEvent.Result.ALLOWED) {
-            if (permissionsManager.hasPermission(player, PlayerStatePermission.IS_VIP)) {
-                int playersOnline = bukkitService.getOnlinePlayers().size();
-                if (playersOnline > plugin.getServer().getMaxPlayers()) {
-                    event.allow();
-                } else {
-                    Player pl = generateKickPlayer(bukkitService.getOnlinePlayers());
-                    if (pl != null) {
-                        pl.kickPlayer(m.retrieveSingle(MessageKey.KICK_FOR_VIP));
-                        event.allow();
-                    } else {
-                        ConsoleLogger.info("The player " + event.getPlayer().getName() + " tried to join, but the server was full");
-                        event.setKickMessage(m.retrieveSingle(MessageKey.KICK_FULL_SERVER));
-                        event.setResult(PlayerLoginEvent.Result.KICK_FULL);
-                    }
-                }
-            } else {
-                event.setKickMessage(m.retrieveSingle(MessageKey.KICK_FULL_SERVER));
-                event.setResult(PlayerLoginEvent.Result.KICK_FULL);
-                return;
-            }
-        }
-
-        if (event.getResult() != PlayerLoginEvent.Result.ALLOWED) {
             return;
         }
 
         final String name = player.getName().toLowerCase();
-        boolean isAuthAvailable = dataSource.isAuthAvailable(name);
+        final PlayerAuth auth = dataSource.getAuth(player.getName());
+        final boolean isAuthAvailable = auth != null;
 
-        if (antiBot.getAntiBotStatus() == AntiBot.AntiBotStatus.ACTIVE && !isAuthAvailable) {
+        try {
-            event.setKickMessage(m.retrieveSingle(MessageKey.KICK_ANTIBOT));
+            checkAntibot(name, isAuthAvailable);
-            event.setResult(PlayerLoginEvent.Result.KICK_OTHER);
+            checkKickNonRegistered(isAuthAvailable);
-            antiBot.antibotKicked.addIfAbsent(player.getName());
+            checkIsValidName(name);
-            return;
+            checkNameCasing(player, auth);
-        }
+            checkSingleSession(player);
-
+            checkPlayerCountry(isAuthAvailable, event);
-        if (settings.getProperty(RestrictionSettings.KICK_NON_REGISTERED) && !isAuthAvailable) {
+        } catch (VerificationFailedException e) {
-            event.setKickMessage(m.retrieveSingle(MessageKey.MUST_REGISTER_MESSAGE));
+            event.setKickMessage(m.retrieveSingle(e.getReason(), e.getArgs()));
             event.setResult(PlayerLoginEvent.Result.KICK_OTHER);
             return;
         }
 
-        if (name.length() > settings.getProperty(RestrictionSettings.MAX_NICKNAME_LENGTH) || name.length() < settings.getProperty(RestrictionSettings.MIN_NICKNAME_LENGTH)) {
+        antiBot.handlePlayerJoin(player);
-            event.setKickMessage(m.retrieveSingle(MessageKey.INVALID_NAME_LENGTH));
-            event.setResult(PlayerLoginEvent.Result.KICK_OTHER);
-            return;
-        }
-
-        if (name.equalsIgnoreCase("Player") || !nicknamePattern.matcher(player.getName()).matches()) {
-            event.setKickMessage(m.retrieveSingle(MessageKey.INVALID_NAME_CHARACTERS)
-                .replace("REG_EX", nicknamePattern.pattern()));
-            event.setResult(PlayerLoginEvent.Result.KICK_OTHER);
-            return;
-        }
-
-        antiBot.checkAntiBot(player);
 
         if (settings.getProperty(HooksSettings.BUNGEECORD)) {
             ByteArrayDataOutput out = ByteStreams.newDataOutput();
@@ -221,13 +150,174 @@ public class AuthMePlayerJoinListener implements Listener, Reloadable {
         }
     }
 
-    // Select the player to kick when a vip player joins the server when full
+    /**
-    private Player generateKickPlayer(Collection<? extends Player> collection) {
+     * Selects a non-VIP player to kick when a VIP player joins the server when full.
-        for (Player player : collection) {
+     *
+     * @param onlinePlayers list of online players
+     * @return the player to kick, or null if none applicable
+     */
+    private Player generateKickPlayer(Collection<? extends Player> onlinePlayers) {
+        for (Player player : onlinePlayers) {
             if (!permissionsManager.hasPermission(player, PlayerStatePermission.IS_VIP)) {
                 return player;
             }
         }
         return null;
     }
+
+    /**
+     * Checks if Antibot is enabled.
+     *
+     * @param playerName the name of the player (lowercase)
+     * @param isAuthAvailable whether or not the player is registered
+     */
+    private void checkAntibot(String playerName, boolean isAuthAvailable) throws VerificationFailedException {
+        if (antiBot.getAntiBotStatus() == AntiBot.AntiBotStatus.ACTIVE && !isAuthAvailable) {
+            antiBot.antibotKicked.addIfAbsent(playerName);
+            throw new VerificationFailedException(MessageKey.KICK_ANTIBOT);
+        }
+    }
+
+    /**
+     * Checks whether non-registered players should be kicked, and if so, whether the player should be kicked.
+     *
+     * @param isAuthAvailable whether or not the player is registered
+     */
+    private void checkKickNonRegistered(boolean isAuthAvailable) throws VerificationFailedException {
+        if (!isAuthAvailable && settings.getProperty(RestrictionSettings.KICK_NON_REGISTERED)) {
+            throw new VerificationFailedException(MessageKey.MUST_REGISTER_MESSAGE);
+        }
+    }
+
+    /**
+     * Checks that the name adheres to the configured username restrictions.
+     *
+     * @param name the name to verify
+     */
+    private void checkIsValidName(String name) throws VerificationFailedException {
+        if (name.length() > settings.getProperty(RestrictionSettings.MAX_NICKNAME_LENGTH)
+            || name.length() < settings.getProperty(RestrictionSettings.MIN_NICKNAME_LENGTH)) {
+            throw new VerificationFailedException(MessageKey.INVALID_NAME_LENGTH);
+        }
+        if (!nicknamePattern.matcher(name).matches()) {
+            throw new VerificationFailedException(MessageKey.INVALID_NAME_CHARACTERS, nicknamePattern.pattern());
+        }
+    }
+
+    /**
+     * Handles the case of a full server and verifies if the user's connection should really be refused
+     * by adjusting the event object accordingly. Attempts to kick a non-VIP player to make room if the
+     * joining player is a VIP.
+     *
+     * @param event the login event to verify
+     * @return true if the player's connection should be refused (i.e. the event does not need to be processed
+     * further), false if the player is not refused
+     */
+    private boolean refusePlayerForFullServer(PlayerLoginEvent event) {
+        final Player player = event.getPlayer();
+        if (event.getResult() != PlayerLoginEvent.Result.KICK_FULL) {
+            // Server is not full, no need to do anything
+            return false;
+        } else if (!permissionsManager.hasPermission(player, PlayerStatePermission.IS_VIP)) {
+            // Server is full and player is NOT VIP; set kick message and proceed with kick
+            event.setKickMessage(m.retrieveSingle(MessageKey.KICK_FULL_SERVER));
+            return true;
+        }
+
+        // Server is full and player is VIP; attempt to kick a non-VIP player to make room
+        Collection<? extends Player> onlinePlayers = bukkitService.getOnlinePlayers();
+        if (onlinePlayers.size() < plugin.getServer().getMaxPlayers()) {
+            event.allow();
+            return false;
+        }
+        Player nonVipPlayer = generateKickPlayer(onlinePlayers);
+        if (nonVipPlayer != null) {
+            nonVipPlayer.kickPlayer(m.retrieveSingle(MessageKey.KICK_FOR_VIP));
+            event.allow();
+            return false;
+        } else {
+            ConsoleLogger.info("VIP player " + player.getName() + " tried to join, but the server was full");
+            event.setKickMessage(m.retrieveSingle(MessageKey.KICK_FULL_SERVER));
+            return true;
+        }
+    }
+
+    /**
+     * Checks that the casing in the username corresponds to the one in the database, if so configured.
+     *
+     * @param player the player to verify
+     * @param auth the auth object associated with the player
+     */
+    private void checkNameCasing(Player player, PlayerAuth auth) throws VerificationFailedException {
+        if (auth != null && settings.getProperty(RegistrationSettings.PREVENT_OTHER_CASE)) {
+            String realName = auth.getRealName(); // might be null or "Player"
+            String connectingName = player.getName();
+
+            if (StringUtils.isEmpty(realName) || "Player".equals(realName)) {
+                dataSource.updateRealName(connectingName.toLowerCase(), connectingName);
+            } else if (!realName.equals(connectingName)) {
+                throw new VerificationFailedException(MessageKey.INVALID_NAME_CASE, realName, connectingName);
+            }
+        }
+    }
+
+    /**
+     * Checks that the player's country is admitted if he is not registered.
+     *
+     * @param isAuthAvailable whether or not the user is registered
+     * @param event the login event of the player
+     */
+    private void checkPlayerCountry(boolean isAuthAvailable,
+                                    PlayerLoginEvent event) throws VerificationFailedException {
+        if (!isAuthAvailable && settings.getProperty(ProtectionSettings.ENABLE_PROTECTION)) {
+            String playerIp = event.getAddress().getHostAddress();
+            if (!validationService.isCountryAdmitted(playerIp)) {
+                throw new VerificationFailedException(MessageKey.COUNTRY_BANNED_ERROR);
+            }
+        }
+    }
+
+    /**
+     * 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
+     */
+    private void checkSingleSession(Player player) throws VerificationFailedException {
+        if (!settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)) {
+            return;
+        }
+
+        Player onlinePlayer = bukkitService.getPlayerExact(player.getName());
+        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 VerificationFailedException(MessageKey.USERNAME_ALREADY_ONLINE_ERROR);
+        }
+    }
+
+    /**
+     * Exception thrown when a verification has failed and the player should be kicked.
+     */
+    private static final class VerificationFailedException extends Exception {
+        private final MessageKey reason;
+        private final String[] args;
+
+        public VerificationFailedException(MessageKey reason, String... args) {
+            this.reason = reason;
+            this.args = args;
+        }
+
+        public MessageKey getReason() {
+            return reason;
+        }
+
+        public String[] getArgs() {
+            return args;
+        }
+    }
 }

src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java

@@ -1,12 +1,11 @@
 package fr.xephi.authme.listener;
 
 import fr.xephi.authme.AntiBot;
-import fr.xephi.authme.AuthMe;
 import fr.xephi.authme.cache.auth.PlayerAuth;
 import fr.xephi.authme.datasource.DataSource;
+import fr.xephi.authme.hooks.PluginHooks;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.output.Messages;
-import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.process.Management;
 import fr.xephi.authme.settings.NewSetting;
 import fr.xephi.authme.settings.SpawnLoader;
@@ -15,8 +14,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.Utils;
-import fr.xephi.authme.util.ValidationService;
-import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
@@ -59,8 +56,6 @@ public class AuthMePlayerListener implements Listener {
 
     public static final ConcurrentHashMap<String, String> joinMessage = new ConcurrentHashMap<>();
 
-    @Inject
-    private AuthMe plugin;
     @Inject
     private NewSetting settings;
     @Inject
@@ -76,9 +71,7 @@ public class AuthMePlayerListener implements Listener {
     @Inject
     private SpawnLoader spawnLoader;
     @Inject
-    private ValidationService validationService;
+    private PluginHooks pluginHooks;
-    @Inject
-    private PermissionsManager permissionsManager;
 
     private void sendLoginOrRegisterMessage(final Player player) {
         bukkitService.runTaskAsynchronously(new Runnable() {
@@ -249,7 +242,7 @@ public class AuthMePlayerListener implements Listener {
         }
 
         if (!antiBot.antibotKicked.contains(player.getName())) {
-            plugin.getManagement().performQuit(player, true);
+            management.performQuit(player, true);
         }
     }
 
@@ -287,7 +280,7 @@ public class AuthMePlayerListener implements Listener {
          * @note little hack cause InventoryOpenEvent cannot be cancelled for
          * real, cause no packet is send to server by client for the main inv
          */
-        Bukkit.getScheduler().scheduleSyncDelayedTask(plugin, new Runnable() {
+        bukkitService.scheduleSyncDelayedTask(new Runnable() {
             @Override
             public void run() {
                 player.closeInventory();
@@ -307,7 +300,7 @@ public class AuthMePlayerListener implements Listener {
         if (Utils.checkAuth(player)) {
             return;
         }
-        if (plugin.getPluginHooks().isNpc(player)) {
+        if (pluginHooks.isNpc(player)) {
             return;
         }
         event.setCancelled(true);

src/test/java/fr/xephi/authme/AntiBotTest.java

@@ -164,7 +164,7 @@ public class AntiBotTest {
         AntiBot antiBot = createListeningAntiBot();
 
         // when
-        antiBot.checkAntiBot(player);
+        antiBot.handlePlayerJoin(player);
 
         // then
         @SuppressWarnings("unchecked")
@@ -194,7 +194,7 @@ public class AntiBotTest {
         AntiBot antiBot = createListeningAntiBot();
 
         // when
-        antiBot.checkAntiBot(player);
+        antiBot.handlePlayerJoin(player);
 
         // then
         @SuppressWarnings("rawtypes")