From 2a4cda070922d604ab9a629dea438f39c3a268b0 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 17 Jul 2016 11:54:22 +0200 Subject: [PATCH 1/4] #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 --- src/main/java/fr/xephi/authme/AuthMe.java | 2 +- .../authme/PurgeBannedPlayersCommand.java | 2 +- .../executable/authme/PurgeCommand.java | 2 +- .../authme/permission/DefaultPermission.java | 36 ++-------- .../authme/permission/PermissionsManager.java | 24 +++++-- .../handlers/BPermissionsHandler.java | 2 +- .../handlers/GroupManagerHandler.java | 2 +- .../handlers/PermissionHandler.java | 2 +- .../handlers/PermissionsBukkitHandler.java | 2 +- .../handlers/PermissionsExHandler.java | 2 +- .../permission/handlers/VaultHandler.java | 2 +- .../handlers/ZPermissionsHandler.java | 2 +- .../authme/task/{ => purge}/PurgeService.java | 70 ++++--------------- .../authme/task/{ => purge}/PurgeTask.java | 42 ++++++++--- .../authme/AuthMeInitializationTest.java | 4 +- .../authme/PurgeBannedPlayersCommandTest.java | 2 +- .../executable/authme/PurgeCommandTest.java | 2 +- .../task/{ => purge}/PurgeServiceTest.java | 51 +++++--------- 18 files changed, 102 insertions(+), 149 deletions(-) rename src/main/java/fr/xephi/authme/task/{ => purge}/PurgeService.java (79%) rename src/main/java/fr/xephi/authme/task/{ => purge}/PurgeTask.java (66%) rename src/test/java/fr/xephi/authme/task/{ => purge}/PurgeServiceTest.java (82%) diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index 6c86945c..bb9d06a6 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/src/main/java/fr/xephi/authme/AuthMe.java @@ -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; diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java index 30e3bd53..b4e9e545 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java @@ -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; diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java index 1885dc76..21552d81 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java @@ -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; diff --git a/src/main/java/fr/xephi/authme/permission/DefaultPermission.java b/src/main/java/fr/xephi/authme/permission/DefaultPermission.java index d4b542cd..96e979a6 100644 --- a/src/main/java/fr/xephi/authme/permission/DefaultPermission.java +++ b/src/main/java/fr/xephi/authme/permission/DefaultPermission.java @@ -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. diff --git a/src/main/java/fr/xephi/authme/permission/PermissionsManager.java b/src/main/java/fr/xephi/authme/permission/PermissionsManager.java index efa1c66e..52ec20db 100644 --- a/src/main/java/fr/xephi/authme/permission/PermissionsManager.java +++ b/src/main/java/fr/xephi/authme/permission/PermissionsManager.java @@ -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); } /** diff --git a/src/main/java/fr/xephi/authme/permission/handlers/BPermissionsHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/BPermissionsHandler.java index 71a6772b..c448d7b7 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/BPermissionsHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/BPermissionsHandler.java @@ -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()); } diff --git a/src/main/java/fr/xephi/authme/permission/handlers/GroupManagerHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/GroupManagerHandler.java index 383f109b..d8980f87 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/GroupManagerHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/GroupManagerHandler.java @@ -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 perms = handler.getAllPlayersPermissions(name); return perms.contains(node.getNode()); diff --git a/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java index b47261bb..bf294c52 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java @@ -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. diff --git a/src/main/java/fr/xephi/authme/permission/handlers/PermissionsBukkitHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/PermissionsBukkitHandler.java index bb351c80..22edea69 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/PermissionsBukkitHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/PermissionsBukkitHandler.java @@ -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; } diff --git a/src/main/java/fr/xephi/authme/permission/handlers/PermissionsExHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/PermissionsExHandler.java index c7e40171..7b1096f9 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/PermissionsExHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/PermissionsExHandler.java @@ -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()); } diff --git a/src/main/java/fr/xephi/authme/permission/handlers/VaultHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/VaultHandler.java index b680d47e..f8e322d6 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/VaultHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/VaultHandler.java @@ -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()); } diff --git a/src/main/java/fr/xephi/authme/permission/handlers/ZPermissionsHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/ZPermissionsHandler.java index 27dca5a3..b4f3198f 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/ZPermissionsHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/ZPermissionsHandler.java @@ -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 perms = zPermissionsService.getPlayerPermissions(null, null, name); if (perms.containsKey(node.getNode())) return perms.get(node.getNode()); diff --git a/src/main/java/fr/xephi/authme/task/PurgeService.java b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java similarity index 79% rename from src/main/java/fr/xephi/authme/task/PurgeService.java rename to src/main/java/fr/xephi/authme/task/purge/PurgeService.java index 8d30d7a1..103bace5 100644 --- a/src/main/java/fr/xephi/authme/task/PurgeService.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java @@ -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 initialPurge = dataSource.getRecordsToPurge(until); - if (CollectionUtils.isEmpty(initialPurge)) { + Set toPurge = dataSource.getRecordsToPurge(until); + if (CollectionUtils.isEmpty(toPurge)) { logAndSendMessage(sender, "No players to purge"); return; } - Set 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 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 getFinalPurgeList(Set initial) { - Set toPurge = new HashSet<>(); - - for (String name : initial) { - if (!permissionsManager.hasPermissionOffline(name, PlayerStatePermission.BYPASS_PURGE)) { - toPurge.add(name); - } - } - - return toPurge; - } - synchronized void purgeAntiXray(Set 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); - } } diff --git a/src/main/java/fr/xephi/authme/task/PurgeTask.java b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java similarity index 66% rename from src/main/java/fr/xephi/authme/task/PurgeTask.java rename to src/main/java/fr/xephi/authme/task/purge/PurgeTask.java index d0636759..d356fc2a 100644 --- a/src/main/java/fr/xephi/authme/task/PurgeTask.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java @@ -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 toPurge; @@ -26,8 +29,20 @@ public class PurgeTask extends BukkitRunnable { private int currentPage = 0; - public PurgeTask(PurgeService service, CommandSender sender, Set 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 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 playerPortion = new HashSet(INTERVALL_CHECK); - Set namePortion = new HashSet(INTERVALL_CHECK); - for (int i = 0; i < INTERVALL_CHECK; i++) { - int nextPosition = (currentPage * INTERVALL_CHECK) + i; + Set playerPortion = new HashSet<>(INTERVAL_CHECK); + Set 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(); } diff --git a/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java b/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java index 72208992..c5c199db 100644 --- a/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java +++ b/src/test/java/fr/xephi/authme/AuthMeInitializationTest.java @@ -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); diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommandTest.java index 295186cf..01d9c89d 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommandTest.java @@ -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; diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java index 05074adf..68a34db7 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java @@ -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; diff --git a/src/test/java/fr/xephi/authme/task/PurgeServiceTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java similarity index 82% rename from src/test/java/fr/xephi/authme/task/PurgeServiceTest.java rename to src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java index a631476e..0b80a393 100644 --- a/src/test/java/fr/xephi/authme/task/PurgeServiceTest.java +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java @@ -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 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 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 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 names) { ArgumentCaptor 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 namesInTask = (Set) ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge"); assertThat(senderInTask, Matchers.equalTo(uuid)); - assertThat(namesInTask, containsInAnyOrder(names)); + assertThat(namesInTask, containsInAnyOrder(names.toArray())); } } From 5953bfd012b39efe210f085c8ee1460d4b7aed0e Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 17 Jul 2016 17:33:25 +0200 Subject: [PATCH 2/4] #784 Separate purge execution and purge task creation - Delete accounts in PurgeExecutor, after permission check --- .../authme/datasource/CacheDataSource.java | 3 +- .../xephi/authme/datasource/DataSource.java | 3 +- .../fr/xephi/authme/datasource/FlatFile.java | 3 +- .../fr/xephi/authme/datasource/MySQL.java | 3 +- .../fr/xephi/authme/datasource/SQLite.java | 3 +- .../authme/task/purge/PurgeExecutor.java | 211 ++++++++++++++++++ .../xephi/authme/task/purge/PurgeService.java | 169 ++------------ .../fr/xephi/authme/task/purge/PurgeTask.java | 17 +- .../AbstractResourceClosingTest.java | 7 +- .../authme/task/purge/PurgeServiceTest.java | 10 +- 10 files changed, 250 insertions(+), 179 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java diff --git a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java index 82e3a9aa..201cff60 100644 --- a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java @@ -15,6 +15,7 @@ import fr.xephi.authme.cache.auth.PlayerCache; import fr.xephi.authme.security.crypts.HashedPassword; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Set; import java.util.concurrent.Callable; @@ -184,7 +185,7 @@ public class CacheDataSource implements DataSource { } @Override - public void purgeRecords(final Set banned) { + public void purgeRecords(final Collection banned) { source.purgeRecords(banned); cachedAuths.invalidateAll(banned); } diff --git a/src/main/java/fr/xephi/authme/datasource/DataSource.java b/src/main/java/fr/xephi/authme/datasource/DataSource.java index 12ee8060..5b25fde4 100644 --- a/src/main/java/fr/xephi/authme/datasource/DataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/DataSource.java @@ -4,6 +4,7 @@ import fr.xephi.authme.cache.auth.PlayerAuth; import fr.xephi.authme.initialization.Reloadable; import fr.xephi.authme.security.crypts.HashedPassword; +import java.util.Collection; import java.util.List; import java.util.Set; @@ -82,7 +83,7 @@ public interface DataSource extends Reloadable { * * @param toPurge The players to purge */ - void purgeRecords(Set toPurge); + void purgeRecords(Collection toPurge); /** * Remove a user record from the database. diff --git a/src/main/java/fr/xephi/authme/datasource/FlatFile.java b/src/main/java/fr/xephi/authme/datasource/FlatFile.java index b9a0bc89..8b192fb1 100644 --- a/src/main/java/fr/xephi/authme/datasource/FlatFile.java +++ b/src/main/java/fr/xephi/authme/datasource/FlatFile.java @@ -17,6 +17,7 @@ import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -256,7 +257,7 @@ public class FlatFile implements DataSource { } @Override - public void purgeRecords(Set toPurge) { + public void purgeRecords(Collection toPurge) { BufferedReader br = null; BufferedWriter bw = null; ArrayList lines = new ArrayList<>(); diff --git a/src/main/java/fr/xephi/authme/datasource/MySQL.java b/src/main/java/fr/xephi/authme/datasource/MySQL.java index ed89fa6b..6678c60a 100644 --- a/src/main/java/fr/xephi/authme/datasource/MySQL.java +++ b/src/main/java/fr/xephi/authme/datasource/MySQL.java @@ -23,6 +23,7 @@ import java.sql.SQLException; import java.sql.Statement; import java.sql.Types; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -693,7 +694,7 @@ public class MySQL implements DataSource { } @Override - public void purgeRecords(Set toPurge) { + public void purgeRecords(Collection toPurge) { String sql = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (Connection con = getConnection(); PreparedStatement pst = con.prepareStatement(sql)) { for (String name : toPurge) { diff --git a/src/main/java/fr/xephi/authme/datasource/SQLite.java b/src/main/java/fr/xephi/authme/datasource/SQLite.java index 97bc4ba2..b8307ff6 100644 --- a/src/main/java/fr/xephi/authme/datasource/SQLite.java +++ b/src/main/java/fr/xephi/authme/datasource/SQLite.java @@ -16,6 +16,7 @@ import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; import java.util.ArrayList; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -317,7 +318,7 @@ public class SQLite implements DataSource { } @Override - public void purgeRecords(Set toPurge) { + public void purgeRecords(Collection toPurge) { String delete = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (PreparedStatement deletePst = con.prepareStatement(delete)) { for (String name : toPurge) { diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java new file mode 100644 index 00000000..9a822d74 --- /dev/null +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java @@ -0,0 +1,211 @@ +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.permission.PermissionsManager; +import fr.xephi.authme.settings.NewSetting; +import fr.xephi.authme.settings.properties.PurgeSettings; +import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.Utils; +import org.bukkit.ChatColor; +import org.bukkit.OfflinePlayer; +import org.bukkit.Server; + +import javax.inject.Inject; +import java.io.File; +import java.util.Collection; + +import static fr.xephi.authme.util.StringUtils.makePath; + +/** + * Executes the purge operations. + */ +class PurgeExecutor { + + @Inject + private NewSetting settings; + + @Inject + private DataSource dataSource; + + @Inject + private PermissionsManager permissionsManager; + + @Inject + private PluginHooks pluginHooks; + + @Inject + private BukkitService bukkitService; + + @Inject + private Server server; + + PurgeExecutor() { + } + + /** + * Performs the purge operations, i.e. deletes data and removes the files associated with the given + * players and names. + * + * @param players the players to purge + * @param names names to purge (lowercase) + */ + public void executePurge(Collection players, Collection names) { + // Purge other data + purgeFromAuthMe(names); + purgeEssentials(players); + purgeDat(players); + purgeLimitedCreative(names); + purgeAntiXray(names); + purgePermissions(players); + } + + synchronized void purgeAntiXray(Collection cleared) { + if (!settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE)) { + return; + } + + int i = 0; + File dataFolder = new File("." + File.separator + "plugins" + File.separator + "AntiXRayData" + + File.separator + "PlayerData"); + if (!dataFolder.exists() || !dataFolder.isDirectory()) { + return; + } + + for (String file : dataFolder.list()) { + if (cleared.contains(file.toLowerCase())) { + File playerFile = new File(dataFolder, file); + if (playerFile.exists() && playerFile.delete()) { + i++; + } + } + } + + ConsoleLogger.info("AutoPurge: Removed " + i + " AntiXRayData Files"); + } + + /** + * Deletes the given accounts from AuthMe. + * + * @param names the name of the accounts to delete + */ + synchronized void purgeFromAuthMe(Collection names) { + dataSource.purgeRecords(names); + // TODO ljacqu 20160717: We shouldn't output namedBanned.size() but the actual total that was deleted + ConsoleLogger.info(ChatColor.GOLD + "Deleted " + names.size() + " user accounts"); + } + + synchronized void purgeLimitedCreative(Collection cleared) { + if (!settings.getProperty(PurgeSettings.REMOVE_LIMITED_CREATIVE_INVENTORIES)) { + return; + } + + int i = 0; + File dataFolder = new File("." + File.separator + "plugins" + File.separator + "LimitedCreative" + + File.separator + "inventories"); + if (!dataFolder.exists() || !dataFolder.isDirectory()) { + return; + } + for (String file : dataFolder.list()) { + String name = file; + int idx; + idx = file.lastIndexOf("_creative.yml"); + if (idx != -1) { + name = name.substring(0, idx); + } else { + idx = file.lastIndexOf("_adventure.yml"); + if (idx != -1) { + name = name.substring(0, idx); + } else { + idx = file.lastIndexOf(".yml"); + if (idx != -1) { + name = name.substring(0, idx); + } + } + } + if (name.equals(file)) { + continue; + } + if (cleared.contains(name.toLowerCase())) { + File dataFile = new File(dataFolder, file); + if (dataFile.exists() && dataFile.delete()) { + i++; + } + } + } + ConsoleLogger.info("AutoPurge: Removed " + i + " LimitedCreative Survival, Creative and Adventure files"); + } + + /** + * Removes the .dat file of the given players. + * + * @param cleared list of players to clear + */ + synchronized void purgeDat(Collection cleared) { + if (!settings.getProperty(PurgeSettings.REMOVE_PLAYER_DAT)) { + return; + } + + int i = 0; + File dataFolder = new File(server.getWorldContainer() + , makePath(settings.getProperty(PurgeSettings.DEFAULT_WORLD), "players")); + + for (OfflinePlayer offlinePlayer : cleared) { + File playerFile = new File(dataFolder, Utils.getUUIDorName(offlinePlayer) + ".dat"); + if (playerFile.delete()) { + i++; + } + } + + ConsoleLogger.info("AutoPurge: Removed " + i + " .dat Files"); + } + + /** + * Removes the Essentials userdata file of each given player. + * + * @param cleared list of players to clear + */ + synchronized void purgeEssentials(Collection cleared) { + if (!settings.getProperty(PurgeSettings.REMOVE_ESSENTIALS_FILES)) { + return; + } + + int i = 0; + File essentialsDataFolder = pluginHooks.getEssentialsDataFolder(); + if (essentialsDataFolder == null) { + ConsoleLogger.info("Cannot purge Essentials: plugin is not loaded"); + return; + } + + final File userDataFolder = new File(essentialsDataFolder, "userdata"); + if (!userDataFolder.exists() || !userDataFolder.isDirectory()) { + return; + } + + for (OfflinePlayer offlinePlayer : cleared) { + File playerFile = new File(userDataFolder, Utils.getUUIDorName(offlinePlayer) + ".yml"); + if (playerFile.exists() && playerFile.delete()) { + i++; + } + } + + ConsoleLogger.info("AutoPurge: Removed " + i + " EssentialsFiles"); + } + + // TODO: What is this method for? Is it correct? + // TODO: Make it work with OfflinePlayers group data. + synchronized void purgePermissions(Collection cleared) { + if (!settings.getProperty(PurgeSettings.REMOVE_PERMISSIONS)) { + return; + } + + for (OfflinePlayer offlinePlayer : cleared) { + String name = offlinePlayer.getName(); + permissionsManager.removeAllGroups(bukkitService.getPlayerExact(name)); + } + + ConsoleLogger.info("AutoPurge: Removed permissions from " + cleared.size() + " player(s)."); + } + +} diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java index 103bace5..4b1bf40d 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java @@ -2,26 +2,20 @@ 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.permission.PermissionsManager; import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.PurgeSettings; import fr.xephi.authme.util.BukkitService; import fr.xephi.authme.util.CollectionUtils; -import fr.xephi.authme.util.Utils; -import org.bukkit.ChatColor; import org.bukkit.OfflinePlayer; -import org.bukkit.Server; import org.bukkit.command.CommandSender; import org.bukkit.command.ConsoleCommandSender; import javax.inject.Inject; -import java.io.File; import java.util.Calendar; +import java.util.Collection; import java.util.Set; -import static fr.xephi.authme.util.StringUtils.makePath; - public class PurgeService { @Inject @@ -37,21 +31,11 @@ public class PurgeService { private PermissionsManager permissionsManager; @Inject - private PluginHooks pluginHooks; - - @Inject - private Server server; + private PurgeExecutor purgeExecutor; private boolean isPurging = false; - - /** - * Set if a purge is currently in progress. - * - * @param purging True if purging. - */ - void setPurging(boolean purging) { - this.isPurging = purging; + PurgeService() { } /** @@ -93,157 +77,40 @@ public class PurgeService { } /** - * Purges all banned players. + * Purges the given list of player names. * * @param sender Sender running the command + * @param names The names to remove + * @param players Collection of OfflinePlayers (including those with the given names) */ public void purgePlayers(CommandSender sender, Set names, OfflinePlayer[] players) { - //todo: note this should may run async because it may executes a SQL-Query if (isPurging) { logAndSendMessage(sender, "Purge is already in progress! Aborting purge request"); 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, permissionsManager, sender, names, players); bukkitService.runTaskAsynchronously(purgeTask); } - synchronized void purgeAntiXray(Set cleared) { - if (!settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE)) { - return; - } - - int i = 0; - File dataFolder = new File("." + File.separator + "plugins" + File.separator + "AntiXRayData" - + File.separator + "PlayerData"); - if (!dataFolder.exists() || !dataFolder.isDirectory()) { - return; - } - - for (String file : dataFolder.list()) { - if (cleared.contains(file.toLowerCase())) { - File playerFile = new File(dataFolder, file); - if (playerFile.exists() && playerFile.delete()) { - i++; - } - } - } - - ConsoleLogger.info("AutoPurge: Removed " + i + " AntiXRayData Files"); - } - - synchronized void purgeLimitedCreative(Set cleared) { - if (!settings.getProperty(PurgeSettings.REMOVE_LIMITED_CREATIVE_INVENTORIES)) { - return; - } - - int i = 0; - File dataFolder = new File("." + File.separator + "plugins" + File.separator + "LimitedCreative" - + File.separator + "inventories"); - if (!dataFolder.exists() || !dataFolder.isDirectory()) { - return; - } - for (String file : dataFolder.list()) { - String name = file; - int idx; - idx = file.lastIndexOf("_creative.yml"); - if (idx != -1) { - name = name.substring(0, idx); - } else { - idx = file.lastIndexOf("_adventure.yml"); - if (idx != -1) { - name = name.substring(0, idx); - } else { - idx = file.lastIndexOf(".yml"); - if (idx != -1) { - name = name.substring(0, idx); - } - } - } - if (name.equals(file)) { - continue; - } - if (cleared.contains(name.toLowerCase())) { - File dataFile = new File(dataFolder, file); - if (dataFile.exists() && dataFile.delete()) { - i++; - } - } - } - ConsoleLogger.info("AutoPurge: Removed " + i + " LimitedCreative Survival, Creative and Adventure files"); - } - - synchronized void purgeDat(Set cleared) { - if (!settings.getProperty(PurgeSettings.REMOVE_PLAYER_DAT)) { - return; - } - - int i = 0; - File dataFolder = new File(server.getWorldContainer() - , makePath(settings.getProperty(PurgeSettings.DEFAULT_WORLD), "players")); - - for (OfflinePlayer offlinePlayer : cleared) { - File playerFile = new File(dataFolder, Utils.getUUIDorName(offlinePlayer) + ".dat"); - if (playerFile.delete()) { - i++; - } - } - - ConsoleLogger.info("AutoPurge: Removed " + i + " .dat Files"); + /** + * Set if a purge is currently in progress. + * + * @param purging True if purging. + */ + void setPurging(boolean purging) { + this.isPurging = purging; } /** - * Method purgeEssentials. + * Perform purge operations for the given players and names. * - * @param cleared List of String + * @param players the players (associated with the names) + * @param names the lowercase names */ - synchronized void purgeEssentials(Set cleared) { - if (!settings.getProperty(PurgeSettings.REMOVE_ESSENTIALS_FILES)) { - return; - } - - int i = 0; - File essentialsDataFolder = pluginHooks.getEssentialsDataFolder(); - if (essentialsDataFolder == null) { - ConsoleLogger.info("Cannot purge Essentials: plugin is not loaded"); - return; - } - - final File userDataFolder = new File(essentialsDataFolder, "userdata"); - if (!userDataFolder.exists() || !userDataFolder.isDirectory()) { - return; - } - - for (OfflinePlayer offlinePlayer : cleared) { - File playerFile = new File(userDataFolder, Utils.getUUIDorName(offlinePlayer) + ".yml"); - if (playerFile.exists() && playerFile.delete()) { - i++; - } - } - - ConsoleLogger.info("AutoPurge: Removed " + i + " EssentialsFiles"); - } - - // TODO: What is this method for? Is it correct? - // TODO: Make it work with OfflinePlayers group data. - synchronized void purgePermissions(Set cleared) { - if (!settings.getProperty(PurgeSettings.REMOVE_PERMISSIONS)) { - return; - } - - for (OfflinePlayer offlinePlayer : cleared) { - String name = offlinePlayer.getName(); - permissionsManager.removeAllGroups(bukkitService.getPlayerExact(name)); - } - - ConsoleLogger.info("AutoPurge: Removed permissions from " + cleared.size() + " player(s)."); + void executePurge(Collection players, Collection names) { + purgeExecutor.executePurge(players, names); } private static void logAndSendMessage(CommandSender sender, String message) { diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java index d356fc2a..fc879699 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java @@ -14,7 +14,7 @@ import java.util.HashSet; import java.util.Set; import java.util.UUID; -public class PurgeTask extends BukkitRunnable { +class PurgeTask extends BukkitRunnable { //how many players we should check for each tick private static final int INTERVAL_CHECK = 5; @@ -38,8 +38,8 @@ public class PurgeTask extends BukkitRunnable { * @param toPurge lowercase names to purge * @param offlinePlayers offline players to map to the names */ - public PurgeTask(PurgeService service, PermissionsManager permissionsManager, CommandSender sender, - Set toPurge, OfflinePlayer[] offlinePlayers) { + PurgeTask(PurgeService service, PermissionsManager permissionsManager, CommandSender sender, + Set toPurge, OfflinePlayer[] offlinePlayers) { this.purgeService = service; this.permissionsManager = permissionsManager; @@ -93,22 +93,13 @@ public class PurgeTask extends BukkitRunnable { } currentPage++; - purgeData(playerPortion, namePortion); + purgeService.executePurge(playerPortion, namePortion); if (currentPage % 20 == 0) { int completed = totalPurgeCount - toPurge.size(); sendMessage("[AuthMe] Purge progress " + completed + '/' + totalPurgeCount); } } - private void purgeData(Set playerPortion, Set namePortion) { - // Purge other data - purgeService.purgeEssentials(playerPortion); - purgeService.purgeDat(playerPortion); - purgeService.purgeLimitedCreative(namePortion); - purgeService.purgeAntiXray(namePortion); - purgeService.purgePermissions(playerPortion); - } - private void finish() { cancel(); diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java index 1ecdb637..d692d78d 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java +++ b/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java @@ -219,7 +219,7 @@ public abstract class AbstractResourceClosingTest { Object element = PARAM_VALUES.get(genericType); Preconditions.checkNotNull(element, "No sample element for list of generic type " + genericType); - if (List.class == parameterizedType.getRawType()) { + if (isAssignableFrom(parameterizedType.getRawType(), List.class)) { return Arrays.asList(element, element, element); } else if (Set.class == parameterizedType.getRawType()) { return new HashSet<>(Arrays.asList(element, element, element)); @@ -229,6 +229,11 @@ public abstract class AbstractResourceClosingTest { throw new IllegalStateException("Cannot build list for unexpected Type: " + type); } + private static boolean isAssignableFrom(Type type, Class fromType) { + return (type instanceof Class) + && ((Class) type).isAssignableFrom(fromType); + } + /* Initialize the map of test values to pass to methods to satisfy their signature. */ private static Map, Object> getDefaultParameters() { HashedPassword hash = new HashedPassword("test", "test"); diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java index 0b80a393..c709d1f0 100644 --- a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java @@ -3,7 +3,6 @@ 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.runner.BeforeInjecting; import fr.xephi.authme.runner.DelayedInjectionRunner; @@ -12,7 +11,6 @@ import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.PurgeSettings; import fr.xephi.authme.util.BukkitService; import org.bukkit.OfflinePlayer; -import org.bukkit.Server; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; import org.hamcrest.Matchers; @@ -61,9 +59,7 @@ public class PurgeServiceTest { @Mock private PermissionsManager permissionsManager; @Mock - private PluginHooks pluginHooks; - @Mock - private Server server; + private PurgeExecutor executor; @BeforeClass public static void initLogger() { @@ -116,7 +112,6 @@ public class PurgeServiceTest { ArgumentCaptor captor = ArgumentCaptor.forClass(Long.class); verify(dataSource).getRecordsToPurge(captor.capture()); assertCorrectPurgeTimestamp(captor.getValue(), 60); - verify(dataSource).purgeRecords(playerNames); assertThat(Boolean.TRUE.equals( ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")), equalTo(true)); verifyScheduledPurgeTask(null, playerNames); @@ -156,9 +151,6 @@ public class PurgeServiceTest { // then verify(dataSource).getRecordsToPurge(delay); - 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); } From ca4a64f398dd0b01fddcb5b6c2fc52e23890ec87 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 17 Jul 2016 21:12:01 +0200 Subject: [PATCH 3/4] #844 Run PurgeTask as repeating task and #784 write tests for PurgeTask --- .../authme/task/purge/PurgeExecutor.java | 2 +- .../xephi/authme/task/purge/PurgeService.java | 2 +- .../fr/xephi/authme/task/purge/PurgeTask.java | 4 +- .../fr/xephi/authme/util/BukkitService.java | 19 ++ .../authme/task/purge/PurgeServiceTest.java | 41 ++-- .../authme/task/purge/PurgeTaskTest.java | 221 ++++++++++++++++++ 6 files changed, 270 insertions(+), 19 deletions(-) create mode 100644 src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java index 9a822d74..a8805238 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java @@ -49,7 +49,7 @@ class PurgeExecutor { * players and names. * * @param players the players to purge - * @param names names to purge (lowercase) + * @param names names to purge */ public void executePurge(Collection players, Collection names) { // Purge other data diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java index 4b1bf40d..8f6f42f7 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java @@ -91,7 +91,7 @@ public class PurgeService { isPurging = true; PurgeTask purgeTask = new PurgeTask(this, permissionsManager, sender, names, players); - bukkitService.runTaskAsynchronously(purgeTask); + bukkitService.runTaskTimer(purgeTask, 0, 1); } /** diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java index fc879699..a69c5fee 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java @@ -104,9 +104,9 @@ class PurgeTask extends BukkitRunnable { cancel(); // Show a status message - sendMessage(ChatColor.GREEN + "[AuthMe] Database has been purged correctly"); + sendMessage(ChatColor.GREEN + "[AuthMe] Database has been purged successfully"); - ConsoleLogger.info("Purge Finished!"); + ConsoleLogger.info("Purge finished!"); purgeService.setPurging(false); } diff --git a/src/main/java/fr/xephi/authme/util/BukkitService.java b/src/main/java/fr/xephi/authme/util/BukkitService.java index 6913c2af..12448809 100644 --- a/src/main/java/fr/xephi/authme/util/BukkitService.java +++ b/src/main/java/fr/xephi/authme/util/BukkitService.java @@ -9,6 +9,9 @@ import org.bukkit.OfflinePlayer; import org.bukkit.World; import org.bukkit.entity.Player; import org.bukkit.event.Event; +import org.bukkit.plugin.Plugin; +import org.bukkit.scheduler.BukkitRunnable; +import org.bukkit.scheduler.BukkitScheduler; import org.bukkit.scheduler.BukkitTask; import javax.inject.Inject; @@ -123,6 +126,22 @@ public class BukkitService { return Bukkit.getScheduler().runTaskLaterAsynchronously(authMe, task, delay); } + /** + * Schedules the given task to repeatedly run until cancelled, starting after the + * specified number of server ticks. + * + * @param task the task to schedule + * @param delay the ticks to wait before running the task + * @param period the ticks to wait between runs + * @return a BukkitTask that contains the id number + * @throws IllegalArgumentException if plugin is null + * @throws IllegalStateException if this was already scheduled + * @see BukkitScheduler#runTaskTimer(Plugin, Runnable, long, long) + */ + public BukkitTask runTaskTimer(BukkitRunnable task, long delay, long period) { + return task.runTaskTimer(authMe, period, delay); + } + /** * Broadcast a message to all players. * diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java index c709d1f0..01f00b1f 100644 --- a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java @@ -4,9 +4,6 @@ import fr.xephi.authme.ReflectionTestUtils; import fr.xephi.authme.TestHelper; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.permission.PermissionsManager; -import fr.xephi.authme.runner.BeforeInjecting; -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; @@ -18,10 +15,14 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import java.util.Arrays; import java.util.Calendar; import java.util.Collections; +import java.util.List; import java.util.Set; import java.util.UUID; @@ -36,6 +37,7 @@ 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; @@ -44,10 +46,10 @@ import static org.mockito.Mockito.verifyZeroInteractions; /** * Test for {@link PurgeService}. */ -@RunWith(DelayedInjectionRunner.class) +@RunWith(MockitoJUnitRunner.class) public class PurgeServiceTest { - @InjectDelayed + @InjectMocks private PurgeService purgeService; @Mock @@ -66,15 +68,11 @@ public class PurgeServiceTest { TestHelper.setupLogger(); } - @BeforeInjecting - public void initSettingDefaults() { - given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60); - } - @Test public void shouldNotRunAutoPurge() { // given given(settings.getProperty(PurgeSettings.USE_AUTO_PURGE)).willReturn(false); + given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60); // when purgeService.runAutoPurge(); @@ -112,8 +110,8 @@ public class PurgeServiceTest { ArgumentCaptor captor = ArgumentCaptor.forClass(Long.class); verify(dataSource).getRecordsToPurge(captor.capture()); assertCorrectPurgeTimestamp(captor.getValue(), 60); - assertThat(Boolean.TRUE.equals( - ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")), equalTo(true)); + assertThat(Boolean.TRUE, equalTo( + ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging"))); verifyScheduledPurgeTask(null, playerNames); } @@ -169,6 +167,19 @@ public class PurgeServiceTest { verifyZeroInteractions(bukkitService, dataSource, permissionsManager); } + @Test + public void shouldExecutePurgeActions() { + // given + List players = Arrays.asList(mockReturnedOfflinePlayers()); + List names = Arrays.asList("alpha", "bravo", "foxtrot"); + + // when + purgeService.executePurge(players, names); + + // then + verify(executor).executePurge(players, names); + } + /** * Returns mock OfflinePlayer objects with names corresponding to A - G of the NATO phonetic alphabet, * in various casing. @@ -198,14 +209,14 @@ public class PurgeServiceTest { } @SuppressWarnings("unchecked") - private void verifyScheduledPurgeTask(UUID uuid, Set names) { + private void verifyScheduledPurgeTask(UUID senderUuid, Set names) { ArgumentCaptor captor = ArgumentCaptor.forClass(PurgeTask.class); - verify(bukkitService).runTaskAsynchronously(captor.capture()); + verify(bukkitService).runTaskTimer(captor.capture(), eq(0L), eq(1L)); PurgeTask task = captor.getValue(); Object senderInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "sender"); Set namesInTask = (Set) ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge"); - assertThat(senderInTask, Matchers.equalTo(uuid)); + assertThat(senderInTask, Matchers.equalTo(senderUuid)); assertThat(namesInTask, containsInAnyOrder(names.toArray())); } } diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java new file mode 100644 index 00000000..2168756d --- /dev/null +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java @@ -0,0 +1,221 @@ +package fr.xephi.authme.task.purge; + +import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.TestHelper; +import fr.xephi.authme.permission.PermissionNode; +import fr.xephi.authme.permission.PermissionsManager; +import fr.xephi.authme.permission.PlayerStatePermission; +import org.bukkit.Bukkit; +import org.bukkit.OfflinePlayer; +import org.bukkit.Server; +import org.bukkit.command.ConsoleCommandSender; +import org.bukkit.entity.Player; +import org.bukkit.scheduler.BukkitRunnable; +import org.bukkit.scheduler.BukkitScheduler; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.runners.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; + +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.empty; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +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.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +/** + * Test for {@link PurgeTask}. + */ +@RunWith(MockitoJUnitRunner.class) +public class PurgeTaskTest { + + private static final PermissionNode BYPASS_NODE = PlayerStatePermission.BYPASS_PURGE; + + private Map playerBypassAssignments = new HashMap<>(); + + @Mock + private PermissionsManager permissionsManager; + + @Mock + private PurgeService purgeService; + + @Captor + private ArgumentCaptor> playerCaptor; + + @Captor + private ArgumentCaptor> namesCaptor; + + @BeforeClass + public static void initLogger() { + TestHelper.setupLogger(); + } + + @Test + public void shouldRunTask() { + // given + Set names = + newHashSet("alpha", "bravo", "charlie", "delta", "echo", "foxtrot", "golf", "hotel", "india"); + // alpha and echo have bypass permission + // Foxtrot and india are not present as OfflinePlayer + // Additionally, BOGUS and 123456 are not present in the names list + OfflinePlayer[] players = asArray( + mockOfflinePlayer("Alpha", true), mockOfflinePlayer("BOGUS", false), mockOfflinePlayer("charlie", false), + mockOfflinePlayer("Delta", false), mockOfflinePlayer("BRAVO", false), mockOfflinePlayer("Echo", true), + mockOfflinePlayer("Golf", false), mockOfflinePlayer("123456", false), mockOfflinePlayer("HOTEL", false)); + reset(purgeService, permissionsManager); + setPermissionsBehavior(); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, null, names, players); + + // when (1 - first run, 5 players per run) + task.run(); + + // then (1) + // In the first run, Alpha to BRAVO (see players list above) went through. One of those players is not present + // in the names list, so expect the permission manager to have been called four times + verify(permissionsManager, times(4)).hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE)); + // Alpha has the bypass permission, so we expect charlie, Delta and BRAVO to be purged + assertRanPurgeWithPlayers(players[2], players[3], players[4]); + + // when (2) + reset(purgeService, permissionsManager); + setPermissionsBehavior(); + task.run(); + + // then (2) + // Echo, Golf, HOTEL + verify(permissionsManager, times(3)).hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE)); + assertRanPurgeWithPlayers(players[6], players[8]); + + // given (3) + // Third round: no more OfflinePlayer objects, but some names remain + reset(purgeService, permissionsManager); + given(permissionsManager.hasPermissionOffline("india", BYPASS_NODE)).willReturn(true); + + // when (3) + task.run(); + + // then (3) + // We no longer have any OfflinePlayers, so lookup of permissions was done with the names + verify(permissionsManager, times(2)).hasPermissionOffline(anyString(), eq(BYPASS_NODE)); + verify(permissionsManager, never()).hasPermissionOffline(any(OfflinePlayer.class), any(PermissionNode.class)); + assertRanPurgeWithNames("foxtrot"); + } + + @Test + public void shouldStopTaskAndInformSenderUponCompletion() { + // given + Set names = newHashSet("name1", "name2"); + Player sender = mock(Player.class); + UUID uuid = UUID.randomUUID(); + given(sender.getUniqueId()).willReturn(uuid); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, sender, names, new OfflinePlayer[0]); + + ReflectionTestUtils.setField(BukkitRunnable.class, task, "taskId", 10049); + Server server = mock(Server.class); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + given(server.getScheduler()).willReturn(scheduler); + ReflectionTestUtils.setField(Bukkit.class, null, "server", server); + given(server.getPlayer(uuid)).willReturn(sender); + + task.run(); // Run for the first time -> results in empty names list + + // when + task.run(); + + // then + verify(scheduler).cancelTask(task.getTaskId()); + verify(sender).sendMessage(argThat(containsString("Database has been purged successfully"))); + } + + @Test + public void shouldStopTaskAndInformConsoleUser() { + // given + Set names = newHashSet("name1", "name2"); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, null, names, new OfflinePlayer[0]); + + ReflectionTestUtils.setField(BukkitRunnable.class, task, "taskId", 10049); + Server server = mock(Server.class); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + given(server.getScheduler()).willReturn(scheduler); + ReflectionTestUtils.setField(Bukkit.class, null, "server", server); + ConsoleCommandSender consoleSender = mock(ConsoleCommandSender.class); + given(server.getConsoleSender()).willReturn(consoleSender); + + task.run(); // Run for the first time -> results in empty names list + + // when + task.run(); + + // then + verify(scheduler).cancelTask(task.getTaskId()); + verify(consoleSender).sendMessage(argThat(containsString("Database has been purged successfully"))); + } + + + private OfflinePlayer mockOfflinePlayer(String name, boolean hasBypassPermission) { + OfflinePlayer player = mock(OfflinePlayer.class); + given(player.getName()).willReturn(name); + playerBypassAssignments.put(player, hasBypassPermission); + return player; + } + + private OfflinePlayer[] asArray(OfflinePlayer... players) { + return players; + } + + private void setPermissionsBehavior() { + given(permissionsManager.hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE))) + .willAnswer(new Answer() { + @Override + public Boolean answer(InvocationOnMock invocationOnMock) throws Throwable { + OfflinePlayer player = (OfflinePlayer) invocationOnMock.getArguments()[0]; + Boolean hasPermission = playerBypassAssignments.get(player); + if (hasPermission == null) { + throw new IllegalStateException("Unexpected check of '" + BYPASS_NODE + + "' with player = " + player); + } + return hasPermission; + } + }); + } + + private void assertRanPurgeWithPlayers(OfflinePlayer... players) { + List names = new ArrayList<>(players.length); + for (OfflinePlayer player : players) { + names.add(player.getName()); + } + verify(purgeService).executePurge(playerCaptor.capture(), namesCaptor.capture()); + assertThat(namesCaptor.getValue(), containsInAnyOrder(names.toArray())); + assertThat(playerCaptor.getValue(), containsInAnyOrder(players)); + } + + private void assertRanPurgeWithNames(String... names) { + verify(purgeService).executePurge(playerCaptor.capture(), namesCaptor.capture()); + assertThat(namesCaptor.getValue(), containsInAnyOrder(names)); + assertThat(playerCaptor.getValue(), empty()); + } + +} From 57f90fe41012d8d4d85c3cf964e6363b8416ea30 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Mon, 18 Jul 2016 21:29:05 +0200 Subject: [PATCH 4/4] #784 Make DataSource#purgeRecords case-insensitive --- .../fr/xephi/authme/datasource/FlatFile.java | 29 +------------------ .../fr/xephi/authme/datasource/MySQL.java | 2 +- .../fr/xephi/authme/datasource/SQLite.java | 2 +- .../AbstractDataSourceIntegrationTest.java | 13 +++++++++ 4 files changed, 16 insertions(+), 30 deletions(-) diff --git a/src/main/java/fr/xephi/authme/datasource/FlatFile.java b/src/main/java/fr/xephi/authme/datasource/FlatFile.java index 8b192fb1..60a49fe0 100644 --- a/src/main/java/fr/xephi/authme/datasource/FlatFile.java +++ b/src/main/java/fr/xephi/authme/datasource/FlatFile.java @@ -258,34 +258,7 @@ public class FlatFile implements DataSource { @Override public void purgeRecords(Collection toPurge) { - BufferedReader br = null; - BufferedWriter bw = null; - ArrayList lines = new ArrayList<>(); - - try { - br = new BufferedReader(new FileReader(source)); - bw = new BufferedWriter(new FileWriter(source)); - String line; - while ((line = br.readLine()) != null) { - String[] args = line.split(":"); - if (args.length >= 4) { - if (toPurge.contains(args[0])) { - lines.add(line); - continue; - } - } - } - - for (String l : lines) { - bw.write(l + "\n"); - } - } catch (IOException ex) { - ConsoleLogger.warning(ex.getMessage()); - return; - } finally { - silentClose(br); - silentClose(bw); - } + throw new UnsupportedOperationException("Flat file no longer supported"); } @Override diff --git a/src/main/java/fr/xephi/authme/datasource/MySQL.java b/src/main/java/fr/xephi/authme/datasource/MySQL.java index 6678c60a..9a31c73c 100644 --- a/src/main/java/fr/xephi/authme/datasource/MySQL.java +++ b/src/main/java/fr/xephi/authme/datasource/MySQL.java @@ -698,7 +698,7 @@ public class MySQL implements DataSource { String sql = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (Connection con = getConnection(); PreparedStatement pst = con.prepareStatement(sql)) { for (String name : toPurge) { - pst.setString(1, name); + pst.setString(1, name.toLowerCase()); pst.executeUpdate(); } } catch (SQLException ex) { diff --git a/src/main/java/fr/xephi/authme/datasource/SQLite.java b/src/main/java/fr/xephi/authme/datasource/SQLite.java index b8307ff6..083195fa 100644 --- a/src/main/java/fr/xephi/authme/datasource/SQLite.java +++ b/src/main/java/fr/xephi/authme/datasource/SQLite.java @@ -322,7 +322,7 @@ public class SQLite implements DataSource { String delete = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (PreparedStatement deletePst = con.prepareStatement(delete)) { for (String name : toPurge) { - deletePst.setString(1, name); + deletePst.setString(1, name.toLowerCase()); deletePst.executeUpdate(); } } catch (SQLException ex) { diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java index b9474816..7fbccdd4 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java @@ -369,4 +369,17 @@ public abstract class AbstractDataSourceIntegrationTest { assertThat(dataSource.getLoggedPlayers(), empty()); } + @Test + public void shouldPerformPurgeOperation() { + // given + List names = Arrays.asList("Bobby", "USER", "DoesnotExist"); + DataSource dataSource = getDataSource(); + + // when + dataSource.purgeRecords(names); + + // then + assertThat(dataSource.getAllAuths(), empty()); + } + }