diff --git a/src/main/java/fr/xephi/authme/AntiBot.java b/src/main/java/fr/xephi/authme/AntiBot.java index d85cc7843..37f048724 100644 --- a/src/main/java/fr/xephi/authme/AntiBot.java +++ b/src/main/java/fr/xephi/authme/AntiBot.java @@ -24,7 +24,7 @@ public class AntiBot { private final Messages messages; private final PermissionsManager permissionsManager; private final BukkitService bukkitService; - public final CopyOnWriteArrayList antibotKicked = new CopyOnWriteArrayList(); + private final CopyOnWriteArrayList antibotKicked = new CopyOnWriteArrayList(); private final CopyOnWriteArrayList antibotPlayers = new CopyOnWriteArrayList(); private AntiBotStatus antiBotStatus = AntiBotStatus.DISABLED; @@ -112,6 +112,27 @@ public class AntiBot { }, 15 * TICKS_PER_SECOND); } + /** + * Returns whether the player was kicked because of activated antibot. The list is reset + * when antibot is deactivated. + * + * @param name the name to check + * @return true if the given name has been kicked because of Antibot + */ + public boolean wasPlayerKicked(String name) { + return antibotKicked.contains(name.toLowerCase()); + } + + /** + * Adds a name to the list of players kicked by antibot. Should only be used when a player + * is determined to be kicked because of failed antibot verification. + * + * @param name the name to add + */ + public void addPlayerKick(String name) { + antibotKicked.addIfAbsent(name.toLowerCase()); + } + public enum AntiBotStatus { LISTENING, DISABLED, diff --git a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java index ec0368a6e..fba9e6fc7 100644 --- a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java +++ b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java @@ -23,7 +23,6 @@ import org.bukkit.event.entity.EntityDamageByEntityEvent; import org.bukkit.event.inventory.InventoryClickEvent; import org.bukkit.event.inventory.InventoryOpenEvent; import org.bukkit.event.player.AsyncPlayerChatEvent; -import org.bukkit.event.player.AsyncPlayerPreLoginEvent; import org.bukkit.event.player.PlayerBedEnterEvent; import org.bukkit.event.player.PlayerCommandPreprocessEvent; import org.bukkit.event.player.PlayerDropItemEvent; @@ -195,34 +194,10 @@ public class AuthMePlayerListener implements Listener { management.performJoin(player); } - // Note ljacqu 20160528: AsyncPlayerPreLoginEvent is not fired by all servers in offline mode + // Note: 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 - - // Note sgdc3 20160619: No performance improvements if we do the same thing on the Sync method - // let's try to remove this, about the single session issue, - // i tried to use the low priority to the sync handler - - /* - @EventHandler(priority = EventPriority.HIGHEST) - public void onPreLogin(AsyncPlayerPreLoginEvent event) { - final String name = event.getName().toLowerCase(); - //final boolean isAuthAvailable = dataSource.isAuthAvailable(event.getName()); - - try { - // Potential performance improvement: make checkAntiBot not require `isAuthAvailable` info and use - // "checkKickNonRegistered" as last -> no need to query the DB before checking antibot / name - // 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); - } - } - */ + // We have no performance improvements if we do the same thing on two different events + // Important: the single session feature works if we use the low priority to the sync handler @EventHandler(priority = EventPriority.LOW) public void onPlayerLogin(PlayerLoginEvent event) { @@ -264,7 +239,7 @@ public class AuthMePlayerListener implements Listener { event.setQuitMessage(null); } - if (antiBot.antibotKicked.contains(player.getName())) { + if (antiBot.wasPlayerKicked(player.getName())) { return; } @@ -275,7 +250,7 @@ public class AuthMePlayerListener implements Listener { public void onPlayerKick(PlayerKickEvent event) { Player player = event.getPlayer(); - if (!antiBot.antibotKicked.contains(player.getName())) { + if (!antiBot.wasPlayerKicked(player.getName())) { management.performQuit(player, true); } } diff --git a/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java b/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java index 297e74c6c..73bf93e38 100644 --- a/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java +++ b/src/main/java/fr/xephi/authme/listener/OnJoinVerifier.java @@ -15,6 +15,7 @@ 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; @@ -56,13 +57,7 @@ class OnJoinVerifier implements Reloadable { @Override public void reload() { String nickRegEx = settings.getProperty(RestrictionSettings.ALLOWED_NICKNAME_CHARACTERS); - try { - nicknamePattern = Pattern.compile(nickRegEx); - } catch (Exception e) { - nicknamePattern = Pattern.compile(".*?"); - ConsoleLogger.showError("Nickname pattern is not a valid regular expression! " - + "Fallback to allowing all nicknames"); - } + nicknamePattern = Utils.safePatternCompile(nickRegEx); } /** @@ -73,7 +68,7 @@ class OnJoinVerifier implements Reloadable { */ public void checkAntibot(String playerName, boolean isAuthAvailable) throws FailedVerificationException { if (antiBot.getAntiBotStatus() == AntiBot.AntiBotStatus.ACTIVE && !isAuthAvailable) { - antiBot.antibotKicked.addIfAbsent(playerName); + antiBot.addPlayerKick(playerName); throw new FailedVerificationException(MessageKey.KICK_ANTIBOT); } } diff --git a/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java b/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java index 44f0fd0b4..9c792fe66 100644 --- a/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java +++ b/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java @@ -13,7 +13,6 @@ import fr.xephi.authme.security.HashAlgorithm; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.security.crypts.TwoFactor; -import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.EmailSettings; import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; @@ -159,10 +158,7 @@ public class AsyncRegister implements AsynchronousProcess { return; } - if (!Settings.forceRegLogin && autoLogin) { - //PlayerCache.getInstance().addPlayer(auth); - //database.setLogged(name); - // TODO: check this... + if (!service.getProperty(RegistrationSettings.FORCE_LOGIN_AFTER_REGISTER) && autoLogin) { plugin.getManagement().performLogin(player, "dontneed", true); } syncProcessManager.processSyncPasswordRegister(player); diff --git a/src/main/java/fr/xephi/authme/settings/Settings.java b/src/main/java/fr/xephi/authme/settings/Settings.java index 235801558..6e8430010 100644 --- a/src/main/java/fr/xephi/authme/settings/Settings.java +++ b/src/main/java/fr/xephi/authme/settings/Settings.java @@ -3,7 +3,6 @@ package fr.xephi.authme.settings; import fr.xephi.authme.AuthMe; import fr.xephi.authme.settings.domain.Property; import fr.xephi.authme.settings.properties.PluginSettings; -import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; import org.bukkit.configuration.file.FileConfiguration; @@ -25,7 +24,6 @@ public final class Settings { public static boolean protectInventoryBeforeLogInEnabled; public static boolean isStopEnabled; public static boolean reloadSupport; - public static boolean forceRegLogin; public static boolean noTeleport; public static boolean isRemoveSpeedEnabled; public static String getUnloggedinGroup; @@ -64,7 +62,6 @@ public final class Settings { isStopEnabled = configFile.getBoolean("Security.SQLProblem.stopServer", true); reloadSupport = configFile.getBoolean("Security.ReloadCommand.useReloadCommandSupport", true); defaultWorld = configFile.getString("Purge.defaultWorld", "world"); - forceRegLogin = load(RegistrationSettings.FORCE_LOGIN_AFTER_REGISTER); noTeleport = load(RestrictionSettings.NO_TELEPORT); crazyloginFileName = configFile.getString("Converter.CrazyLogin.fileName", "accounts.db"); } diff --git a/src/main/java/fr/xephi/authme/util/Utils.java b/src/main/java/fr/xephi/authme/util/Utils.java index ae9406569..d26ae366e 100644 --- a/src/main/java/fr/xephi/authme/util/Utils.java +++ b/src/main/java/fr/xephi/authme/util/Utils.java @@ -10,6 +10,7 @@ import org.bukkit.OfflinePlayer; import org.bukkit.entity.Player; import java.util.Arrays; +import java.util.regex.Pattern; /** * Utility class for various operations used in the codebase. @@ -79,6 +80,15 @@ public final class Utils { } } + public static Pattern safePatternCompile(String pattern) { + try { + return Pattern.compile(pattern); + } catch (Exception e) { + ConsoleLogger.showError("Failed to compile pattern '" + pattern + "' - defaulting to allowing everything"); + return Pattern.compile(".*?"); + } + } + /** * Returns the IP of the given player. * diff --git a/src/main/java/fr/xephi/authme/util/ValidationService.java b/src/main/java/fr/xephi/authme/util/ValidationService.java index 8e72f852a..615095d00 100644 --- a/src/main/java/fr/xephi/authme/util/ValidationService.java +++ b/src/main/java/fr/xephi/authme/util/ValidationService.java @@ -38,7 +38,7 @@ public class ValidationService implements Reloadable { @Override public void reload() { - passwordRegex = Pattern.compile(settings.getProperty(RestrictionSettings.ALLOWED_PASSWORD_REGEX)); + passwordRegex = Utils.safePatternCompile(settings.getProperty(RestrictionSettings.ALLOWED_PASSWORD_REGEX)); } /** diff --git a/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java b/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java index 6d5d27e44..436f62f58 100644 --- a/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java +++ b/src/test/java/fr/xephi/authme/listener/OnJoinVerifierTest.java @@ -36,6 +36,7 @@ import java.util.List; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -377,6 +378,51 @@ public class OnJoinVerifierTest { verifyZeroInteractions(bukkitService); } + @Test + public void shouldCheckAntiBot() throws FailedVerificationException { + // given + String name = "user123"; + boolean hasAuth = false; + given(antiBot.getAntiBotStatus()).willReturn(AntiBot.AntiBotStatus.LISTENING); + + // when + onJoinVerifier.checkAntibot(name, hasAuth); + + // then + verify(antiBot).getAntiBotStatus(); + } + + @Test + public void shouldAllowUserWithAuth() throws FailedVerificationException { + // given + String name = "Bobby"; + boolean hasAuth = true; + given(antiBot.getAntiBotStatus()).willReturn(AntiBot.AntiBotStatus.ACTIVE); + + // when + onJoinVerifier.checkAntibot(name, hasAuth); + + // then + verify(antiBot).getAntiBotStatus(); + } + + @Test + public void shouldThrowForActiveAntiBot() { + // given + String name = "Bobby"; + boolean hasAuth = false; + given(antiBot.getAntiBotStatus()).willReturn(AntiBot.AntiBotStatus.ACTIVE); + + // when / then + try { + onJoinVerifier.checkAntibot(name, hasAuth); + fail("Expected exception to be thrown"); + } catch (FailedVerificationException e) { + assertThat(e, exceptionWithData(MessageKey.KICK_ANTIBOT)); + verify(antiBot).addPlayerKick(name); + } + } + private static Player newPlayerWithName(String name) { Player player = mock(Player.class); given(player.getName()).willReturn(name); diff --git a/src/test/java/fr/xephi/authme/util/UtilsTest.java b/src/test/java/fr/xephi/authme/util/UtilsTest.java new file mode 100644 index 000000000..58af9b7c9 --- /dev/null +++ b/src/test/java/fr/xephi/authme/util/UtilsTest.java @@ -0,0 +1,93 @@ +package fr.xephi.authme.util; + +import fr.xephi.authme.TestHelper; +import org.bukkit.entity.Player; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.UUID; +import java.util.regex.Pattern; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; + +/** + * Test for {@link Utils}. + */ +public class UtilsTest { + + @BeforeClass + public static void setAuthmeInstance() { + TestHelper.setupLogger(); + } + + @Test + public void shouldCompilePattern() { + // given + String pattern = "gr(a|e)ys?"; + + // when + Pattern result = Utils.safePatternCompile(pattern); + + // then + assertThat(result.toString(), equalTo(pattern)); + } + + @Test + public void shouldDefaultToAllAllowedPattern() { + // given + String invalidPattern = "gr(a|eys?"; // missing closing ')' + + // when + Pattern result = Utils.safePatternCompile(invalidPattern); + + // then + assertThat(result.toString(), equalTo(".*?")); + } + + @Test + public void shouldGetPlayerIp() { + // given + Player player = mock(Player.class); + String ip = "124.86.248.62"; + TestHelper.mockPlayerIp(player, ip); + + // when + String result = Utils.getPlayerIp(player); + + // then + assertThat(result, equalTo(ip)); + } + + @Test + public void shouldGetUuid() { + // given + UUID uuid = UUID.randomUUID(); + Player player = mock(Player.class); + given(player.getUniqueId()).willReturn(uuid); + + // when + String result = Utils.getUUIDorName(player); + + // then + assertThat(result, equalTo(uuid.toString())); + } + + @Test + public void shouldFallbackToName() { + // given + Player player = mock(Player.class); + doThrow(RuntimeException.class).when(player).getUniqueId(); + String name = "Bobby12"; + given(player.getName()).willReturn(name); + + // when + String result = Utils.getUUIDorName(player); + + // then + assertThat(result, equalTo(name)); + } +}