Meishin/LoginSystem
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

#1005 Improve restricted user feature (performance, error handling)

Browse Source 
- 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
This commit is contained in:
ljacqu 2017-03-14 22:26:19 +01:00
parent 62368b1cda
commit 1da74cb987
6 changed files with 148 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java
View File

@ -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.

48
src/main/java/fr/xephi/authme/service/ValidationService.java
View File

@ -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<String> unrestrictedNames;
private Multimap<String, String> 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<String> 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<String, String> loadNameRestrictions(List<String> configuredRestrictions) {
Multimap<String, String> 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;

2
src/main/java/fr/xephi/authme/settings/properties/RestrictionSettings.java
View File

@ -81,7 +81,7 @@ public final class RestrictionSettings implements SettingsHolder {
"Example:",
" AllowedRestrictedUser:",
" - playername;127.0.0.1"})
public static final Property<List<String>> ALLOWED_RESTRICTED_USERS =
public static final Property<List<String>> RESTRICTED_USERS =
newLowercaseListProperty("settings.restrictions.AllowedRestrictedUser");
@Comment("Ban unknown IPs trying to log in with a restricted username?")

16
src/main/java/fr/xephi/authme/util/StringUtils.java
View File

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

81
src/test/java/fr/xephi/authme/service/ValidationServiceTest.java
View File

@ -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.<String>emptyList());
given(settings.getProperty(EmailSettings.DOMAIN_BLACKLIST)).willReturn(Collections.<String>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.<String>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.<String>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.<String>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.<String>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.<String>emptyList());
given(settings.getProperty(ProtectionSettings.COUNTRIES_BLACKLIST)).willReturn(Collections.<String>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.<String>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.<String>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.<String>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.<String>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<String> 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));

10
src/test/java/fr/xephi/authme/util/StringUtilsTest.java
View File

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