From 2b1a97e959921fc165971a22a677fbe3ec41dbdf Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 5 Feb 2017 13:12:04 +0100 Subject: [PATCH] #761 Fix removal and restoration of primary permission group - Improve how a player is being switched between permission groups (add new group before removing old one) - Remove group handling logic from LimboCache: AuthGroupHandler is now solely responsible for changing the player's permission group --- .../xephi/authme/data/limbo/LimboCache.java | 22 +---- .../authme/permission/AuthGroupHandler.java | 93 ++++++++++++------- .../process/login/ProcessSyncPlayerLogin.java | 24 ++--- .../xephi/authme/service/CommonService.java | 5 +- .../authme/data/limbo/LimboCacheTest.java | 14 --- .../authme/service/CommonServiceTest.java | 4 +- 6 files changed, 78 insertions(+), 84 deletions(-) diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboCache.java b/src/main/java/fr/xephi/authme/data/limbo/LimboCache.java index 622ed481..b6fa2944 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/LimboCache.java +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboCache.java @@ -1,10 +1,8 @@ package fr.xephi.authme.data.limbo; +import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.permission.PermissionsManager; -import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.SpawnLoader; -import fr.xephi.authme.settings.properties.PluginSettings; -import fr.xephi.authme.util.StringUtils; import org.bukkit.Location; import org.bukkit.entity.Player; @@ -22,14 +20,11 @@ public class LimboCache { private final Map cache = new ConcurrentHashMap<>(); private LimboPlayerStorage limboPlayerStorage; - private Settings settings; private PermissionsManager permissionsManager; private SpawnLoader spawnLoader; @Inject - LimboCache(Settings settings, PermissionsManager permissionsManager, - SpawnLoader spawnLoader, LimboPlayerStorage limboPlayerStorage) { - this.settings = settings; + LimboCache(PermissionsManager permissionsManager, SpawnLoader spawnLoader, LimboPlayerStorage limboPlayerStorage) { this.permissionsManager = permissionsManager; this.spawnLoader = spawnLoader; this.limboPlayerStorage = limboPlayerStorage; @@ -51,6 +46,7 @@ public class LimboCache { if (permissionsManager.hasGroupSupport()) { playerGroup = permissionsManager.getPrimaryGroup(player); } + ConsoleLogger.debug("Player `{0}` has primary group `{1}`", player.getName(), playerGroup); if (limboPlayerStorage.hasData(player)) { LimboPlayer cache = limboPlayerStorage.readData(player); @@ -83,15 +79,14 @@ public class LimboCache { float walkSpeed = data.getWalkSpeed(); float flySpeed = data.getFlySpeed(); // Reset the speed value if it was 0 - if(walkSpeed < 0.01f) { + if (walkSpeed < 0.01f) { walkSpeed = 0.2f; } - if(flySpeed < 0.01f) { + if (flySpeed < 0.01f) { flySpeed = 0.2f; } player.setWalkSpeed(walkSpeed); player.setFlySpeed(flySpeed); - restoreGroup(player, data.getGroup()); data.clearTasks(); } } @@ -153,11 +148,4 @@ public class LimboCache { removeFromCache(player); addPlayerData(player); } - - private void restoreGroup(Player player, String group) { - if (!StringUtils.isEmpty(group) && permissionsManager.hasGroupSupport() - && settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) { - permissionsManager.setGroup(player, group); - } - } } diff --git a/src/main/java/fr/xephi/authme/permission/AuthGroupHandler.java b/src/main/java/fr/xephi/authme/permission/AuthGroupHandler.java index 050b908a..5e14e986 100644 --- a/src/main/java/fr/xephi/authme/permission/AuthGroupHandler.java +++ b/src/main/java/fr/xephi/authme/permission/AuthGroupHandler.java @@ -13,6 +13,15 @@ import javax.inject.Inject; /** * Changes the permission group according to the auth status of the player and the configuration. + *

+ * If this feature is enabled, the primary permissions group of a player is replaced until he has + * logged in. Some permission plugins have a notion of a primary group; for other permission plugins the + * first group is simply taken. + *

+ * The groups that are used as replacement until the player logs in is configurable and depends on if + * the player is registered or not. Note that some (all?) permission systems require the group to actually + * exist for the replacement to take place. Furthermore, since some permission groups require that players + * be in at least one group, this will mean that the player is not removed from his primary group. */ public class AuthGroupHandler implements Reloadable { @@ -36,11 +45,49 @@ public class AuthGroupHandler implements Reloadable { * * @param player the player * @param groupType the group type - * - * @return True upon success, false otherwise. False is also returned if groups aren't supported - * with the current permissions system. */ - public boolean setGroup(Player player, AuthGroupType groupType) { + public void setGroup(Player player, AuthGroupType groupType) { + if (!useAuthGroups()) { + return; + } + + String primaryGroup = ""; + LimboPlayer limboPlayer = limboCache.getPlayerData(player.getName()); + if (limboPlayer != null) { + primaryGroup = limboPlayer.getGroup(); + } + + switch (groupType) { + // Implementation note: some permission systems don't support players not being in any group, + // so add the new group before removing the old ones + case UNREGISTERED: + permissionsManager.addGroup(player, unregisteredGroup); + permissionsManager.removeGroups(player, registeredGroup, primaryGroup); + break; + + case REGISTERED_UNAUTHENTICATED: + permissionsManager.addGroup(player, registeredGroup); + permissionsManager.removeGroups(player, unregisteredGroup, primaryGroup); + break; + + case LOGGED_IN: + restoreGroup(player); + break; + + default: + throw new IllegalStateException("Encountered unhandled auth group type '" + groupType + "'"); + } + + ConsoleLogger.debug( + () -> player.getName() + " changed to " + groupType + ": has groups " + permissionsManager.getGroups(player)); + } + + /** + * Returns whether the auth permissions group function should be used. + * + * @return true if should be used, false otherwise + */ + private boolean useAuthGroups() { // Check whether the permissions check is enabled if (!settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) { return false; @@ -51,39 +98,21 @@ public class AuthGroupHandler implements Reloadable { ConsoleLogger.warning("The current permissions system doesn't have group support, unable to set group!"); return false; } - - switch (groupType) { - case UNREGISTERED: - // Remove the other group, set the current group - permissionsManager.removeGroups(player, registeredGroup); - return permissionsManager.addGroup(player, unregisteredGroup); - - case REGISTERED_UNAUTHENTICATED: - // Remove the other group, set the current group - permissionsManager.removeGroups(player, unregisteredGroup); - return permissionsManager.addGroup(player, registeredGroup); - - case LOGGED_IN: - return restoreGroup(player); - - default: - throw new IllegalStateException("Encountered unhandled auth group type '" + groupType + "'"); - } + return true; } - private boolean restoreGroup(Player player) { - // Get the player's LimboPlayer + /** + * Restores the player's original primary group (taken from LimboPlayer). + * + * @param player the player to process + */ + private void restoreGroup(Player player) { LimboPlayer limbo = limboCache.getPlayerData(player.getName()); - if (limbo == null) { - return false; + if (limbo != null) { + String primaryGroup = limbo.getGroup(); + permissionsManager.addGroup(player, primaryGroup); } - - // Get the players group - String realGroup = limbo.getGroup(); - - // Remove the other group types groups, set the real group permissionsManager.removeGroups(player, unregisteredGroup, registeredGroup); - return permissionsManager.addGroup(player, realGroup); } @Override diff --git a/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java b/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java index 6fb5fa8b..70402e53 100644 --- a/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java +++ b/src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java @@ -1,6 +1,5 @@ package fr.xephi.authme.process.login; -import fr.xephi.authme.AuthMe; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.data.limbo.LimboCache; import fr.xephi.authme.data.limbo.LimboPlayer; @@ -8,17 +7,17 @@ import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.events.LoginEvent; import fr.xephi.authme.events.RestoreInventoryEvent; import fr.xephi.authme.listener.PlayerListener; +import fr.xephi.authme.permission.AuthGroupType; import fr.xephi.authme.process.SynchronousProcess; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.BungeeService; +import fr.xephi.authme.service.CommonService; import fr.xephi.authme.service.TeleportationService; -import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.WelcomeMessageConfiguration; import fr.xephi.authme.settings.commandconfig.CommandManager; import fr.xephi.authme.settings.properties.RegistrationSettings; import fr.xephi.authme.util.StringUtils; import org.bukkit.entity.Player; -import org.bukkit.plugin.PluginManager; import org.bukkit.potion.PotionEffectType; import javax.inject.Inject; @@ -28,9 +27,6 @@ import static fr.xephi.authme.settings.properties.RestrictionSettings.PROTECT_IN public class ProcessSyncPlayerLogin implements SynchronousProcess { - @Inject - private AuthMe plugin; - @Inject private BungeeService bungeeService; @@ -40,9 +36,6 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { @Inject private BukkitService bukkitService; - @Inject - private PluginManager pluginManager; - @Inject private TeleportationService teleportationService; @@ -53,7 +46,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { private CommandManager commandManager; @Inject - private Settings settings; + private CommonService commonService; @Inject private WelcomeMessageConfiguration welcomeMessageConfiguration; @@ -63,7 +56,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { private void restoreInventory(Player player) { RestoreInventoryEvent event = new RestoreInventoryEvent(player); - pluginManager.callEvent(event); + bukkitService.callEvent(event); if (!event.isCancelled()) { player.updateInventory(); } @@ -80,8 +73,9 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { // do we really need to use location from database for now? // because LimboCache#restoreData teleport player to last location. } + commonService.setGroup(player, AuthGroupType.LOGGED_IN); - if (settings.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) { + if (commonService.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) { restoreInventory(player); } @@ -98,7 +92,7 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { } } - if (settings.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)) { + if (commonService.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)) { player.removePotionEffect(PotionEffectType.BLINDNESS); } @@ -108,8 +102,8 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess { // Login is done, display welcome message List welcomeMessage = welcomeMessageConfiguration.getWelcomeMessage(player); - if (settings.getProperty(RegistrationSettings.USE_WELCOME_MESSAGE)) { - if (settings.getProperty(RegistrationSettings.BROADCAST_WELCOME_MESSAGE)) { + if (commonService.getProperty(RegistrationSettings.USE_WELCOME_MESSAGE)) { + if (commonService.getProperty(RegistrationSettings.BROADCAST_WELCOME_MESSAGE)) { welcomeMessage.forEach(bukkitService::broadcastMessage); } else { welcomeMessage.forEach(player::sendMessage); diff --git a/src/main/java/fr/xephi/authme/service/CommonService.java b/src/main/java/fr/xephi/authme/service/CommonService.java index c181f83b..cea51ea0 100644 --- a/src/main/java/fr/xephi/authme/service/CommonService.java +++ b/src/main/java/fr/xephi/authme/service/CommonService.java @@ -101,10 +101,9 @@ public class CommonService { * * @param player the player to process * @param group the group to add the player to - * @return true on success, false otherwise */ - public boolean setGroup(Player player, AuthGroupType group) { - return authGroupHandler.setGroup(player, group); + public void setGroup(Player player, AuthGroupType group) { + authGroupHandler.setGroup(player, group); } } diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboCacheTest.java b/src/test/java/fr/xephi/authme/data/limbo/LimboCacheTest.java index fcc45451..152f2e35 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/LimboCacheTest.java +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboCacheTest.java @@ -2,9 +2,7 @@ package fr.xephi.authme.data.limbo; import fr.xephi.authme.ReflectionTestUtils; import fr.xephi.authme.permission.PermissionsManager; -import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.SpawnLoader; -import fr.xephi.authme.settings.properties.PluginSettings; import org.bukkit.Location; import org.bukkit.entity.Player; import org.junit.Test; @@ -33,9 +31,6 @@ public class LimboCacheTest { @InjectMocks private LimboCache limboCache; - @Mock - private Settings settings; - @Mock private PermissionsManager permissionsManager; @@ -118,11 +113,7 @@ public class LimboCacheTest { given(limboPlayer.isCanFly()).willReturn(true); float flySpeed = 1.0f; given(limboPlayer.getFlySpeed()).willReturn(flySpeed); - String group = "primary-group"; - given(limboPlayer.getGroup()).willReturn(group); getCache().put(name.toLowerCase(), limboPlayer); - given(settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)).willReturn(true); - given(permissionsManager.hasGroupSupport()).willReturn(true); // when limboCache.restoreData(player); @@ -132,7 +123,6 @@ public class LimboCacheTest { verify(player).setWalkSpeed(walkSpeed); verify(player).setAllowFlight(true); verify(player).setFlySpeed(flySpeed); - verify(permissionsManager).setGroup(player, group); verify(limboPlayer).clearTasks(); } @@ -147,11 +137,7 @@ public class LimboCacheTest { given(limboPlayer.getWalkSpeed()).willReturn(0f); given(limboPlayer.isCanFly()).willReturn(true); given(limboPlayer.getFlySpeed()).willReturn(0f); - String group = "primary-group"; - given(limboPlayer.getGroup()).willReturn(group); getCache().put(name.toLowerCase(), limboPlayer); - given(settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)).willReturn(true); - given(permissionsManager.hasGroupSupport()).willReturn(true); // when limboCache.restoreData(player); diff --git a/src/test/java/fr/xephi/authme/service/CommonServiceTest.java b/src/test/java/fr/xephi/authme/service/CommonServiceTest.java index ff992bfe..7927f826 100644 --- a/src/test/java/fr/xephi/authme/service/CommonServiceTest.java +++ b/src/test/java/fr/xephi/authme/service/CommonServiceTest.java @@ -134,13 +134,11 @@ public class CommonServiceTest { // given Player player = mock(Player.class); AuthGroupType type = AuthGroupType.LOGGED_IN; - given(authGroupHandler.setGroup(player, type)).willReturn(true); // when - boolean result = commonService.setGroup(player, type); + commonService.setGroup(player, type); // then verify(authGroupHandler).setGroup(player, type); - assertThat(result, equalTo(true)); } }