From 1da74cb987c0c8c472d0373f2a730eea549c430a Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 14 Mar 2017 22:26:19 +0100 Subject: [PATCH] #1005 Improve restricted user feature (performance, error handling) - Move check for restricted user into validation service - Keep restrictions in a map by name for fast lookup, avoid splitting Strings on every call - Gracefully handle case when entry does not have the expected ';' and log exception --- .../authme/process/join/AsynchronousJoin.java | 36 ++------- .../authme/service/ValidationService.java | 48 +++++++++++ .../properties/RestrictionSettings.java | 2 +- .../fr/xephi/authme/util/StringUtils.java | 16 ++++ .../authme/service/ValidationServiceTest.java | 81 ++++++++++++++++--- .../fr/xephi/authme/util/StringUtilsTest.java | 10 +++ 6 files changed, 148 insertions(+), 45 deletions(-) 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 3dcfe8e0..73bef42a 100644 --- a/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java +++ b/src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java @@ -15,6 +15,7 @@ import fr.xephi.authme.process.login.AsynchronousLogin; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.CommonService; import fr.xephi.authme.service.PluginHookService; +import fr.xephi.authme.service.ValidationService; import fr.xephi.authme.settings.commandconfig.CommandManager; import fr.xephi.authme.settings.properties.HooksSettings; import fr.xephi.authme.settings.properties.PluginSettings; @@ -71,6 +72,9 @@ public class AsynchronousJoin implements AsynchronousProcess { @Inject private CommandManager commandManager; + @Inject + private ValidationService validationService; + AsynchronousJoin() { } @@ -91,7 +95,7 @@ public class AsynchronousJoin implements AsynchronousProcess { pluginHookService.setEssentialsSocialSpyStatus(player, false); } - if (isNameRestricted(name, ip, player.getAddress().getHostName())) { + if (!validationService.fulfillsNameRestrictions(player)) { bukkitService.scheduleSyncTaskFromOptionallyAsyncTask(new Runnable() { @Override public void run() { @@ -180,36 +184,6 @@ public class AsynchronousJoin implements AsynchronousProcess { limboPlayerTaskManager.registerMessageTask(name, isAuthAvailable); } - /** - * Returns whether the name is restricted based on the restriction settings. - * - * @param name The name to check - * @param ip The IP address of the player - * @param domain The hostname of the IP address - * - * @return True if the name is restricted (IP/domain is not allowed for the given name), - * false if the restrictions are met or if the name has no restrictions to it - */ - private boolean isNameRestricted(String name, String ip, String domain) { - if (!service.getProperty(RestrictionSettings.ENABLE_RESTRICTED_USERS)) { - return false; - } - - boolean nameFound = false; - for (String entry : service.getProperty(RestrictionSettings.ALLOWED_RESTRICTED_USERS)) { - String[] args = entry.split(";"); - String testName = args[0]; - String testIp = args[1]; - if (testName.equalsIgnoreCase(name)) { - nameFound = true; - if ((ip != null && testIp.equals(ip)) || (domain != null && testIp.equalsIgnoreCase(domain))) { - return false; - } - } - } - return nameFound; - } - /** * Checks whether the maximum number of accounts has been exceeded for the given IP address (according to * settings and permissions). If this is the case, the player is kicked. diff --git a/src/main/java/fr/xephi/authme/service/ValidationService.java b/src/main/java/fr/xephi/authme/service/ValidationService.java index dd1f6e51..9511c6e3 100644 --- a/src/main/java/fr/xephi/authme/service/ValidationService.java +++ b/src/main/java/fr/xephi/authme/service/ValidationService.java @@ -1,6 +1,8 @@ package fr.xephi.authme.service; import ch.jalu.configme.properties.Property; +import com.google.common.collect.HashMultimap; +import com.google.common.collect.Multimap; import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.initialization.Reloadable; @@ -12,8 +14,10 @@ import fr.xephi.authme.settings.properties.EmailSettings; import fr.xephi.authme.settings.properties.ProtectionSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; +import fr.xephi.authme.util.PlayerUtils; import fr.xephi.authme.util.Utils; import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; import javax.annotation.PostConstruct; import javax.inject.Inject; @@ -23,6 +27,8 @@ import java.util.List; import java.util.Set; import java.util.regex.Pattern; +import static fr.xephi.authme.util.StringUtils.isInsideString; + /** * Validation service. */ @@ -39,6 +45,7 @@ public class ValidationService implements Reloadable { private Pattern passwordRegex; private Set unrestrictedNames; + private Multimap restrictedNames; ValidationService() { } @@ -49,6 +56,9 @@ public class ValidationService implements Reloadable { passwordRegex = Utils.safePatternCompile(settings.getProperty(RestrictionSettings.ALLOWED_PASSWORD_REGEX)); // Use Set for more efficient contains() lookup unrestrictedNames = new HashSet<>(settings.getProperty(RestrictionSettings.UNRESTRICTED_NAMES)); + restrictedNames = settings.getProperty(RestrictionSettings.ENABLE_RESTRICTED_USERS) + ? loadNameRestrictions(settings.getProperty(RestrictionSettings.RESTRICTED_USERS)) + : HashMultimap.create(); } /** @@ -132,6 +142,24 @@ public class ValidationService implements Reloadable { return unrestrictedNames.contains(name.toLowerCase()); } + /** + * Checks that the player meets any name restriction if present (IP/domain-based). + * + * @param player the player to check + * @return true if the player may join, false if the player does not satisfy the name restrictions + */ + public boolean fulfillsNameRestrictions(Player player) { + Collection restrictions = restrictedNames.get(player.getName().toLowerCase()); + if (Utils.isCollectionEmpty(restrictions)) { + return true; + } + + String ip = PlayerUtils.getPlayerIp(player); + String domain = player.getAddress().getHostName(); + return restrictions.stream() + .anyMatch(restriction -> ip.equals(restriction) || domain.equalsIgnoreCase(restriction)); + } + /** * Verifies whether the given value is allowed according to the given whitelist and blacklist settings. * Whitelist has precedence over blacklist: if a whitelist is set, the value is rejected if not present @@ -161,6 +189,26 @@ public class ValidationService implements Reloadable { return false; } + /** + * Loads the configured name restrictions into a Multimap by player name (all-lowercase). + * + * @param configuredRestrictions the restriction rules to convert to a map + * @return map of allowed IPs/domain names by player name + */ + private Multimap loadNameRestrictions(List configuredRestrictions) { + Multimap restrictions = HashMultimap.create(); + for (String restriction : configuredRestrictions) { + if (isInsideString(';', restriction)) { + String[] data = restriction.split(";"); + restrictions.put(data[0].toLowerCase(), data[1]); + } else { + ConsoleLogger.warning("Restricted user rule must have a ';' separating name from restriction," + + " but found: '" + restriction + "'"); + } + } + return restrictions; + } + public static final class ValidationResult { private final MessageKey messageKey; private final String[] args; diff --git a/src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java b/src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java index 5a007b32..40de8ca6 100644 --- a/src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java +++ b/src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java @@ -81,7 +81,7 @@ public final class RestrictionSettings implements SettingsHolder { "Example:", " AllowedRestrictedUser:", " - playername;127.0.0.1"}) - public static final Property> ALLOWED_RESTRICTED_USERS = + public static final Property> RESTRICTED_USERS = newLowercaseListProperty("settings.restrictions.AllowedRestrictedUser"); @Comment("Ban unknown IPs trying to log in with a restricted username?") diff --git a/src/main/java/fr/xephi/authme/util/StringUtils.java b/src/main/java/fr/xephi/authme/util/StringUtils.java index 5e0696af..1f200c0f 100644 --- a/src/main/java/fr/xephi/authme/util/StringUtils.java +++ b/src/main/java/fr/xephi/authme/util/StringUtils.java @@ -76,4 +76,20 @@ public final class StringUtils { public static String formatException(Throwable th) { return "[" + th.getClass().getSimpleName() + "]: " + th.getMessage(); } + + /** + * Check that the given needle is in the middle of the haystack, i.e. that the haystack + * contains the needle and that it is not at the very start or end. + * + * @param needle the needle to search for + * @param haystack the haystack to search in + * + * @return true if the needle is in the middle of the word, false otherwise + */ + // Note ljacqu 20170314: `needle` is restricted to char type intentionally because something like + // isInsideString("11", "2211") would unexpectedly return true... + public static boolean isInsideString(char needle, String haystack) { + int index = haystack.indexOf(needle); + return index > 0 && index < haystack.length() - 1; + } } diff --git a/src/test/java/fr/xephi/authme/service/ValidationServiceTest.java b/src/test/java/fr/xephi/authme/service/ValidationServiceTest.java index 21ffa952..3953d4ef 100644 --- a/src/test/java/fr/xephi/authme/service/ValidationServiceTest.java +++ b/src/test/java/fr/xephi/authme/service/ValidationServiceTest.java @@ -4,24 +4,30 @@ import ch.jalu.injector.testing.BeforeInjecting; import ch.jalu.injector.testing.DelayedInjectionRunner; import ch.jalu.injector.testing.InjectDelayed; import com.google.common.base.Strings; +import fr.xephi.authme.TestHelper; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.permission.PlayerStatePermission; +import fr.xephi.authme.service.ValidationService.ValidationResult; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.properties.EmailSettings; import fr.xephi.authme.settings.properties.ProtectionSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; -import fr.xephi.authme.service.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import java.util.Arrays; import java.util.Collections; +import java.util.logging.Logger; import static java.util.Arrays.asList; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; @@ -55,6 +61,7 @@ public class ValidationServiceTest { .willReturn(asList("unsafe", "other-unsafe")); given(settings.getProperty(EmailSettings.MAX_REG_PER_EMAIL)).willReturn(3); given(settings.getProperty(RestrictionSettings.UNRESTRICTED_NAMES)).willReturn(asList("name01", "npc")); + given(settings.getProperty(RestrictionSettings.ENABLE_RESTRICTED_USERS)).willReturn(false); } @Test @@ -115,8 +122,8 @@ public class ValidationServiceTest { @Test public void shouldAcceptEmailWithEmptyLists() { // given - given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); - given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); // when boolean result = validationService.validateEmail("test@example.org"); @@ -130,7 +137,7 @@ public class ValidationServiceTest { // given given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)) .willReturn(asList("domain.tld", "example.com")); - given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); // when boolean result = validationService.validateEmail("TesT@Example.com"); @@ -144,7 +151,7 @@ public class ValidationServiceTest { // given given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)) .willReturn(asList("domain.tld", "example.com")); - given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.emptyList()); // when boolean result = validationService.validateEmail("email@other-domain.abc"); @@ -156,7 +163,7 @@ public class ValidationServiceTest { @Test public void shouldAcceptEmailNotInBlacklist() { // given - given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)) .willReturn(asList("Example.org", "a-test-name.tld")); @@ -170,7 +177,7 @@ public class ValidationServiceTest { @Test public void shouldRejectEmailInBlacklist() { // given - given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(EmailSettings.DOMAIN_WHITELIST)).willReturn(Collections.emptyList()); given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)) .willReturn(asList("Example.org", "a-test-name.tld")); @@ -263,8 +270,8 @@ public class ValidationServiceTest { @Test public void shouldNotInvokeGeoLiteApiIfCountryListsAreEmpty() { // given - given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); - given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); // when boolean result = validationService.isCountryAdmitted("addr"); @@ -278,7 +285,7 @@ public class ValidationServiceTest { public void shouldAcceptCountryInWhitelist() { // given given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(asList("ch", "it")); - given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); String ip = "127.0.0.1"; given(geoIpService.getCountryCode(ip)).willReturn("CH"); @@ -294,7 +301,7 @@ public class ValidationServiceTest { public void shouldRejectCountryMissingFromWhitelist() { // given given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(asList("ch", "it")); - given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.emptyList()); String ip = "123.45.67.89"; given(geoIpService.getCountryCode(ip)).willReturn("BR"); @@ -309,7 +316,7 @@ public class ValidationServiceTest { @Test public void shouldAcceptCountryAbsentFromBlacklist() { // given - given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(asList("ch", "it")); String ip = "127.0.0.1"; given(geoIpService.getCountryCode(ip)).willReturn("BR"); @@ -325,7 +332,7 @@ public class ValidationServiceTest { @Test public void shouldRejectCountryInBlacklist() { // given - given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); + given(settings.getProperty(ProtectionSettings.COUNTRIES_WHITELIST)).willReturn(Collections.emptyList()); given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(asList("ch", "it")); String ip = "123.45.67.89"; given(geoIpService.getCountryCode(ip)).willReturn("IT"); @@ -338,6 +345,54 @@ public class ValidationServiceTest { verify(geoIpService).getCountryCode(ip); } + @Test + public void shouldCheckNameRestrictions() { + // given + given(settings.getProperty(RestrictionSettings.ENABLE_RESTRICTED_USERS)).willReturn(true); + given(settings.getProperty(RestrictionSettings.RESTRICTED_USERS)) + .willReturn(Arrays.asList("Bobby;127.0.0.4", "Tamara;32.24.16.8")); + validationService.reload(); + + Player bobby = mockPlayer("bobby", "127.0.0.4"); + Player tamara = mockPlayer("taMARA", "8.8.8.8"); + Player notRestricted = mockPlayer("notRestricted", "0.0.0.0"); + + // when + boolean isBobbyAdmitted = validationService.fulfillsNameRestrictions(bobby); + boolean isTamaraAdmitted = validationService.fulfillsNameRestrictions(tamara); + boolean isNotRestrictedAdmitted = validationService.fulfillsNameRestrictions(notRestricted); + + // then + assertThat(isBobbyAdmitted, equalTo(true)); + assertThat(isTamaraAdmitted, equalTo(false)); + assertThat(isNotRestrictedAdmitted, equalTo(true)); + } + + @Test + public void shouldLogWarningForInvalidRestrictionRule() { + // given + Logger logger = TestHelper.setupLogger(); + given(settings.getProperty(RestrictionSettings.ENABLE_RESTRICTED_USERS)).willReturn(true); + given(settings.getProperty(RestrictionSettings.RESTRICTED_USERS)) + .willReturn(Arrays.asList("Bobby;127.0.0.4", "Tamara;")); + + // when + validationService.reload(); + + // then + ArgumentCaptor stringCaptor = ArgumentCaptor.forClass(String.class); + verify(logger).warning(stringCaptor.capture()); + assertThat(stringCaptor.getValue(), containsString("Tamara;")); + } + + private static Player mockPlayer(String name, String ip) { + Player player = mock(Player.class); + given(player.getName()).willReturn(name); + TestHelper.mockPlayerIp(player, ip); + given(player.getAddress().getHostName()).willReturn("--"); + return player; + } + private static void assertErrorEquals(ValidationResult validationResult, MessageKey messageKey, String... args) { assertThat(validationResult.hasError(), equalTo(true)); assertThat(validationResult.getMessageKey(), equalTo(messageKey)); diff --git a/src/test/java/fr/xephi/authme/util/StringUtilsTest.java b/src/test/java/fr/xephi/authme/util/StringUtilsTest.java index 629adbd4..7111f81b 100644 --- a/src/test/java/fr/xephi/authme/util/StringUtilsTest.java +++ b/src/test/java/fr/xephi/authme/util/StringUtilsTest.java @@ -95,4 +95,14 @@ public class StringUtilsTest { public void shouldHaveHiddenConstructor() { TestHelper.validateHasOnlyPrivateEmptyConstructor(StringUtils.class); } + + @Test + public void shouldCheckIfHasNeedleInWord() { + // given/when/then + assertThat(StringUtils.isInsideString('@', "@hello"), equalTo(false)); + assertThat(StringUtils.isInsideString('?', "absent"), equalTo(false)); + assertThat(StringUtils.isInsideString('-', "abcd-"), equalTo(false)); + assertThat(StringUtils.isInsideString('@', "hello@example"), equalTo(true)); + assertThat(StringUtils.isInsideString('@', "D@Z"), equalTo(true)); + } }