#784 Perform bypass purge permission check with OfflinePlayer objects

- Move permission check inside PurgeTask to perform it with OfflinePlayer objects instead of lowercase names
- Move purge members into child "purge" package
- Unify online and offline default permission behavior in DefaultPermission
This commit is contained in:
ljacqu 2016-07-17 11:54:22 +02:00
parent b439a0391c
commit 2a4cda0709
18 changed files with 102 additions and 149 deletions

View File

@ -47,7 +47,7 @@ import fr.xephi.authme.settings.properties.RestrictionSettings;
import fr.xephi.authme.settings.properties.SecuritySettings;
import fr.xephi.authme.settings.properties.SettingsFieldRetriever;
import fr.xephi.authme.settings.propertymap.PropertyMap;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import fr.xephi.authme.util.BukkitService;
import fr.xephi.authme.util.FileUtils;
import fr.xephi.authme.util.GeoLiteAPI;

View File

@ -1,7 +1,7 @@
package fr.xephi.authme.command.executable.authme;
import fr.xephi.authme.command.ExecutableCommand;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import fr.xephi.authme.util.BukkitService;
import org.bukkit.OfflinePlayer;
import org.bukkit.command.CommandSender;

View File

@ -1,7 +1,7 @@
package fr.xephi.authme.command.executable.authme;
import fr.xephi.authme.command.ExecutableCommand;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import org.bukkit.ChatColor;
import org.bukkit.command.CommandSender;

View File

@ -1,6 +1,6 @@
package fr.xephi.authme.permission;
import org.bukkit.command.CommandSender;
import org.bukkit.permissions.ServerOperator;
/**
* The default permission to fall back to if there is no support for permission nodes.
@ -10,12 +10,7 @@ public enum DefaultPermission {
/** No one has permission. */
NOT_ALLOWED("No permission") {
@Override
public boolean evaluate(CommandSender sender) {
return false;
}
@Override
public boolean evaluateOffline(String name) {
public boolean evaluate(ServerOperator sender) {
return false;
}
},
@ -23,26 +18,15 @@ public enum DefaultPermission {
/** Only players with OP status have permission. */
OP_ONLY("OP's only") {
@Override
public boolean evaluate(CommandSender sender) {
return sender.isOp();
}
@Override
public boolean evaluateOffline(String name) {
// TODO #784: Check if there is an elegant way to evaluate OP status
return false;
public boolean evaluate(ServerOperator sender) {
return sender != null && sender.isOp();
}
},
/** Everyone is granted permission. */
ALLOWED("Everyone allowed") {
@Override
public boolean evaluate(CommandSender sender) {
return true;
}
@Override
public boolean evaluateOffline(String name) {
public boolean evaluate(ServerOperator sender) {
return true;
}
};
@ -64,15 +48,7 @@ public enum DefaultPermission {
* @param sender the sender to process
* @return true if the sender has permission, false otherwise
*/
public abstract boolean evaluate(CommandSender sender);
/**
* Evaluate whether permission is granted to an offline user.
*
* @param name The name to check
* @return True if the user has permission, false otherwise
*/
public abstract boolean evaluateOffline(String name);
public abstract boolean evaluate(ServerOperator sender);
/**
* Return the textual representation.

View File

@ -12,6 +12,7 @@ import fr.xephi.authme.permission.handlers.VaultHandler;
import fr.xephi.authme.permission.handlers.ZPermissionsHandler;
import fr.xephi.authme.util.StringUtils;
import org.anjocaido.groupmanager.GroupManager;
import org.bukkit.OfflinePlayer;
import org.bukkit.Server;
import org.bukkit.command.CommandSender;
import org.bukkit.entity.Player;
@ -210,22 +211,33 @@ public class PermissionsManager implements Reloadable {
* Check if a player has permission for the given permission node. This is for offline player checks. If no permissions
* system is used, then the player will not have permission.
*
* @param name The name of the player.
* @param permissionNode The permission node to verify.
* @param player The offline player
* @param permissionNode The permission node to verify
*
* @return
* @return true if the player has permission, false otherwise
*/
public boolean hasPermissionOffline(String name, PermissionNode permissionNode) {
public boolean hasPermissionOffline(OfflinePlayer player, PermissionNode permissionNode) {
// Check if the permission node is null
if (permissionNode == null) {
return true;
}
if (!isEnabled()) {
return permissionNode.getDefaultPermission().evaluateOffline(name);
return permissionNode.getDefaultPermission().evaluate(player);
}
return handler.hasPermission(name, permissionNode);
return handler.hasPermissionOffline(player.getName(), permissionNode);
}
public boolean hasPermissionOffline(String name, PermissionNode permissionNode) {
if (permissionNode == null) {
return true;
}
if (!isEnabled()) {
return permissionNode.getDefaultPermission().evaluate(null);
}
return handler.hasPermissionOffline(name, permissionNode);
}
/**

View File

@ -28,7 +28,7 @@ public class BPermissionsHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
return ApiLayer.hasPermission(null, CalculableType.USER, name, node.getNode());
}

View File

@ -36,7 +36,7 @@ public class GroupManagerHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
final AnjoPermissionsHandler handler = groupManager.getWorldsHolder().getWorldPermissionsByPlayerName(name);
List<String> perms = handler.getAllPlayersPermissions(name);
return perms.contains(node.getNode());

View File

@ -47,7 +47,7 @@ public interface PermissionHandler {
*
* @return True if the player has permission.
*/
boolean hasPermission(String name, PermissionNode node);
boolean hasPermissionOffline(String name, PermissionNode node);
/**
* Check whether the player is in the specified group.

View File

@ -26,7 +26,7 @@ public class PermissionsBukkitHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
return false;
}

View File

@ -44,7 +44,7 @@ public class PermissionsExHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
PermissionUser user = permissionManager.getUser(name);
return user.has(node.getNode());
}

View File

@ -50,7 +50,7 @@ public class VaultHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
return vaultProvider.has("", name, node.getNode());
}

View File

@ -43,7 +43,7 @@ public class ZPermissionsHandler implements PermissionHandler {
}
@Override
public boolean hasPermission(String name, PermissionNode node) {
public boolean hasPermissionOffline(String name, PermissionNode node) {
Map<String, Boolean> perms = zPermissionsService.getPlayerPermissions(null, null, name);
if (perms.containsKey(node.getNode()))
return perms.get(node.getNode());

View File

@ -1,11 +1,9 @@
package fr.xephi.authme.task;
package fr.xephi.authme.task.purge;
import fr.xephi.authme.ConsoleLogger;
import fr.xephi.authme.datasource.DataSource;
import fr.xephi.authme.hooks.PluginHooks;
import fr.xephi.authme.initialization.Reloadable;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.permission.PlayerStatePermission;
import fr.xephi.authme.settings.NewSetting;
import fr.xephi.authme.settings.properties.PurgeSettings;
import fr.xephi.authme.util.BukkitService;
@ -17,16 +15,14 @@ import org.bukkit.Server;
import org.bukkit.command.CommandSender;
import org.bukkit.command.ConsoleCommandSender;
import javax.annotation.PostConstruct;
import javax.inject.Inject;
import java.io.File;
import java.util.Calendar;
import java.util.HashSet;
import java.util.Set;
import static fr.xephi.authme.util.StringUtils.makePath;
public class PurgeService implements Reloadable {
public class PurgeService {
@Inject
private BukkitService bukkitService;
@ -48,17 +44,6 @@ public class PurgeService implements Reloadable {
private boolean isPurging = false;
// Settings
private int daysBeforePurge;
/**
* Return whether a purge is in progress.
*
* @return True if purging.
*/
public boolean isPurging() {
return this.isPurging;
}
/**
* Set if a purge is currently in progress.
@ -70,9 +55,10 @@ public class PurgeService implements Reloadable {
}
/**
* Purges players from the database. Run on startup if enabled.
* Purges players from the database. Runs on startup if enabled.
*/
public void runAutoPurge() {
int daysBeforePurge = settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER);
if (!settings.getProperty(PurgeSettings.USE_AUTO_PURGE)) {
return;
} else if (daysBeforePurge <= 0) {
@ -89,29 +75,27 @@ public class PurgeService implements Reloadable {
}
/**
* Run a purge with a specified time.
* Runs a purge with a specified last login threshold. Players who haven't logged in since the threshold
* will be purged.
*
* @param sender Sender running the command.
* @param until The minimum last login.
* @param sender Sender running the command
* @param until The last login threshold in milliseconds
*/
public void runPurge(CommandSender sender, long until) {
//todo: note this should may run async because it may executes a SQL-Query
Set<String> initialPurge = dataSource.getRecordsToPurge(until);
if (CollectionUtils.isEmpty(initialPurge)) {
Set<String> toPurge = dataSource.getRecordsToPurge(until);
if (CollectionUtils.isEmpty(toPurge)) {
logAndSendMessage(sender, "No players to purge");
return;
}
Set<String> toPurge = getFinalPurgeList(initialPurge);
purgePlayers(sender, toPurge, bukkitService.getOfflinePlayers());
}
/**
* Purges the given list of player names.
* Purges all banned players.
*
* @param sender Sender running the command.
* @param names The names to remove.
* @param players Collection of OfflinePlayers (including those with the given names).
* @param sender Sender running the command
*/
public void purgePlayers(CommandSender sender, Set<String> names, OfflinePlayer[] players) {
//todo: note this should may run async because it may executes a SQL-Query
@ -120,35 +104,17 @@ public class PurgeService implements Reloadable {
return;
}
// FIXME #784: We can no longer delete records here -> permission check happens inside PurgeTask
dataSource.purgeRecords(names);
// TODO ljacqu 20160717: We shouldn't output namedBanned.size() but the actual total that was deleted
logAndSendMessage(sender, ChatColor.GOLD + "Deleted " + names.size() + " user accounts");
logAndSendMessage(sender, ChatColor.GOLD + "Purging user accounts...");
isPurging = true;
PurgeTask purgeTask = new PurgeTask(this, sender, names, players);
PurgeTask purgeTask = new PurgeTask(this, permissionsManager, sender, names, players);
bukkitService.runTaskAsynchronously(purgeTask);
}
/**
* Check each name in the initial purge findings to remove any player from the purge list
* that has the bypass permission.
*
* @param initial The initial list of players to purge.
*
* @return The list of players to purge after permission check.
*/
private Set<String> getFinalPurgeList(Set<String> initial) {
Set<String> toPurge = new HashSet<>();
for (String name : initial) {
if (!permissionsManager.hasPermissionOffline(name, PlayerStatePermission.BYPASS_PURGE)) {
toPurge.add(name);
}
}
return toPurge;
}
synchronized void purgeAntiXray(Set<String> cleared) {
if (!settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE)) {
return;
@ -287,10 +253,4 @@ public class PurgeService implements Reloadable {
sender.sendMessage(message);
}
}
@PostConstruct
@Override
public void reload() {
this.daysBeforePurge = settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER);
}
}

View File

@ -1,6 +1,8 @@
package fr.xephi.authme.task;
package fr.xephi.authme.task.purge;
import fr.xephi.authme.ConsoleLogger;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.permission.PlayerStatePermission;
import org.bukkit.Bukkit;
import org.bukkit.ChatColor;
import org.bukkit.OfflinePlayer;
@ -15,9 +17,10 @@ import java.util.UUID;
public class PurgeTask extends BukkitRunnable {
//how many players we should check for each tick
private static final int INTERVALL_CHECK = 5;
private static final int INTERVAL_CHECK = 5;
private final PurgeService purgeService;
private final PermissionsManager permissionsManager;
private final UUID sender;
private final Set<String> toPurge;
@ -26,8 +29,20 @@ public class PurgeTask extends BukkitRunnable {
private int currentPage = 0;
public PurgeTask(PurgeService service, CommandSender sender, Set<String> toPurge, OfflinePlayer[] offlinePlayers) {
/**
* Constructor.
*
* @param service the purge service
* @param permissionsManager the permissions manager
* @param sender the sender who initiated the purge, or null
* @param toPurge lowercase names to purge
* @param offlinePlayers offline players to map to the names
*/
public PurgeTask(PurgeService service, PermissionsManager permissionsManager, CommandSender sender,
Set<String> toPurge, OfflinePlayer[] offlinePlayers) {
this.purgeService = service;
this.permissionsManager = permissionsManager;
if (sender instanceof Player) {
this.sender = ((Player) sender).getUniqueId();
} else {
@ -47,20 +62,21 @@ public class PurgeTask extends BukkitRunnable {
return;
}
Set<OfflinePlayer> playerPortion = new HashSet<OfflinePlayer>(INTERVALL_CHECK);
Set<String> namePortion = new HashSet<String>(INTERVALL_CHECK);
for (int i = 0; i < INTERVALL_CHECK; i++) {
int nextPosition = (currentPage * INTERVALL_CHECK) + i;
Set<OfflinePlayer> playerPortion = new HashSet<>(INTERVAL_CHECK);
Set<String> namePortion = new HashSet<>(INTERVAL_CHECK);
for (int i = 0; i < INTERVAL_CHECK; i++) {
int nextPosition = (currentPage * INTERVAL_CHECK) + i;
if (offlinePlayers.length <= nextPosition) {
//no more offline players on this page
break;
}
OfflinePlayer offlinePlayer = offlinePlayers[nextPosition];
//remove to speed up later lookups
if (toPurge.remove(offlinePlayer.getName().toLowerCase())) {
playerPortion.add(offlinePlayer);
namePortion.add(offlinePlayer.getName());
if (!permissionsManager.hasPermissionOffline(offlinePlayer, PlayerStatePermission.BYPASS_PURGE)) {
playerPortion.add(offlinePlayer);
namePortion.add(offlinePlayer.getName());
}
}
}
@ -68,7 +84,11 @@ public class PurgeTask extends BukkitRunnable {
ConsoleLogger.info("Finished lookup up offlinePlayers. Begin looking purging player names only");
//we went through all offlineplayers but there are still names remaining
namePortion.addAll(toPurge);
for (String name : toPurge) {
if (!permissionsManager.hasPermissionOffline(name, PlayerStatePermission.BYPASS_PURGE)) {
namePortion.add(name);
}
}
toPurge.clear();
}

View File

@ -12,7 +12,7 @@ import fr.xephi.authme.process.Management;
import fr.xephi.authme.process.login.ProcessSyncPlayerLogin;
import fr.xephi.authme.security.PasswordSecurity;
import fr.xephi.authme.settings.NewSetting;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import org.bukkit.Bukkit;
import org.bukkit.Server;
import org.bukkit.plugin.PluginDescriptionFile;
@ -93,7 +93,7 @@ public class AuthMeInitializationTest {
// given
NewSetting settings = new NewSetting(settingsFile, dataFolder, getAllPropertyFields(), alwaysFulfilled());
// TODO ljacqu 20160619: At some point setting the "plugin" field should not longer be necessary
// TODO ljacqu 20160619: At some point setting the "plugin" field should no longer be necessary
// We only require it right now because of usages of AuthMe#getInstance()
ReflectionTestUtils.setField(AuthMe.class, null, "plugin", authMe);

View File

@ -1,6 +1,6 @@
package fr.xephi.authme.command.executable.authme;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import fr.xephi.authme.util.BukkitService;
import org.bukkit.OfflinePlayer;
import org.bukkit.command.CommandSender;

View File

@ -1,6 +1,6 @@
package fr.xephi.authme.command.executable.authme;
import fr.xephi.authme.task.PurgeService;
import fr.xephi.authme.task.purge.PurgeService;
import org.bukkit.command.CommandSender;
import org.junit.Test;
import org.junit.runner.RunWith;

View File

@ -1,14 +1,13 @@
package fr.xephi.authme.task;
package fr.xephi.authme.task.purge;
import fr.xephi.authme.ReflectionTestUtils;
import fr.xephi.authme.TestHelper;
import fr.xephi.authme.datasource.DataSource;
import fr.xephi.authme.hooks.PluginHooks;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.permission.PlayerStatePermission;
import fr.xephi.authme.runner.BeforeInjecting;
import fr.xephi.authme.runner.InjectDelayed;
import fr.xephi.authme.runner.DelayedInjectionRunner;
import fr.xephi.authme.runner.InjectDelayed;
import fr.xephi.authme.settings.NewSetting;
import fr.xephi.authme.settings.properties.PurgeSettings;
import fr.xephi.authme.util.BukkitService;
@ -32,7 +31,6 @@ import static com.google.common.collect.Sets.newHashSet;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.equalToIgnoringCase;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertThat;
@ -40,7 +38,6 @@ import static org.mockito.BDDMockito.given;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Matchers.anySet;
import static org.mockito.Matchers.argThat;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@ -95,7 +92,6 @@ public class PurgeServiceTest {
// given
given(settings.getProperty(PurgeSettings.USE_AUTO_PURGE)).willReturn(true);
given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(0);
purgeService.reload();
// when
purgeService.runAutoPurge();
@ -109,10 +105,9 @@ public class PurgeServiceTest {
// given
given(settings.getProperty(PurgeSettings.USE_AUTO_PURGE)).willReturn(true);
given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60);
String[] playerNames = {"alpha", "bravo", "charlie", "delta"};
given(dataSource.getRecordsToPurge(anyLong())).willReturn(newHashSet(playerNames));
Set<String> playerNames = newHashSet("alpha", "bravo", "charlie", "delta");
given(dataSource.getRecordsToPurge(anyLong())).willReturn(playerNames);
mockReturnedOfflinePlayers();
mockHasBypassPurgePermission("bravo", "delta");
// when
purgeService.runAutoPurge();
@ -121,13 +116,14 @@ public class PurgeServiceTest {
ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
verify(dataSource).getRecordsToPurge(captor.capture());
assertCorrectPurgeTimestamp(captor.getValue(), 60);
verify(dataSource).purgeRecords(newHashSet("alpha", "charlie"));
assertThat(purgeService.isPurging(), equalTo(true));
verifyScheduledPurgeTask(null, "alpha", "charlie");
verify(dataSource).purgeRecords(playerNames);
assertThat(Boolean.TRUE.equals(
ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")), equalTo(true));
verifyScheduledPurgeTask(null, playerNames);
}
@SuppressWarnings("unchecked")
@Test
@SuppressWarnings("unchecked")
public void shouldRecognizeNoPlayersToPurge() {
// given
long delay = 123012301L;
@ -148,9 +144,9 @@ public class PurgeServiceTest {
public void shouldRunPurge() {
// given
long delay = 1809714L;
given(dataSource.getRecordsToPurge(delay)).willReturn(newHashSet("charlie", "delta", "echo", "foxtrot"));
Set<String> playerNames = newHashSet("charlie", "delta", "echo", "foxtrot");
given(dataSource.getRecordsToPurge(delay)).willReturn(playerNames);
mockReturnedOfflinePlayers();
mockHasBypassPurgePermission("echo");
Player sender = mock(Player.class);
UUID uuid = UUID.randomUUID();
given(sender.getUniqueId()).willReturn(uuid);
@ -160,13 +156,14 @@ public class PurgeServiceTest {
// then
verify(dataSource).getRecordsToPurge(delay);
verify(dataSource).purgeRecords(newHashSet("charlie", "delta", "foxtrot"));
verify(sender).sendMessage(argThat(containsString("Deleted 3 user accounts")));
verifyScheduledPurgeTask(uuid, "charlie", "delta", "foxtrot");
verify(dataSource).purgeRecords(playerNames);
// FIXME #784: Deleting accounts needs to be handled differently
verify(sender).sendMessage(argThat(containsString("Deleted 4 user accounts")));
verifyScheduledPurgeTask(uuid, playerNames);
}
@Test
public void shouldRunPurgeIfProcessIsAlreadyRunning() {
public void shouldNotRunPurgeIfProcessIsAlreadyRunning() {
// given
purgeService.setPurging(true);
CommandSender sender = mock(CommandSender.class);
@ -198,18 +195,6 @@ public class PurgeServiceTest {
return players;
}
/**
* Mocks the permission manager to say that the given names have the bypass purge permission.
*
* @param names the names
*/
private void mockHasBypassPurgePermission(String... names) {
for (String name : names) {
given(permissionsManager.hasPermissionOffline(
argThat(equalToIgnoringCase(name)), eq(PlayerStatePermission.BYPASS_PURGE))).willReturn(true);
}
}
private void assertCorrectPurgeTimestamp(long timestamp, int configuredDays) {
final long toleranceMillis = 100L;
Calendar cal = Calendar.getInstance();
@ -221,7 +206,7 @@ public class PurgeServiceTest {
}
@SuppressWarnings("unchecked")
private void verifyScheduledPurgeTask(UUID uuid, String... names) {
private void verifyScheduledPurgeTask(UUID uuid, Set<String> names) {
ArgumentCaptor<PurgeTask> captor = ArgumentCaptor.forClass(PurgeTask.class);
verify(bukkitService).runTaskAsynchronously(captor.capture());
PurgeTask task = captor.getValue();
@ -229,6 +214,6 @@ public class PurgeServiceTest {
Object senderInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "sender");
Set<String> namesInTask = (Set<String>) ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge");
assertThat(senderInTask, Matchers.<Object>equalTo(uuid));
assertThat(namesInTask, containsInAnyOrder(names));
assertThat(namesInTask, containsInAnyOrder(names.toArray()));
}
}