#761 Restore permission group in sync with limbo players

- Couple AuthGroupHandler closer to the LimboService: whenever a limbo player is restored, the auth group should be restored as well. This fixes some consistency issues.
- Move AuthGroupHandler into limbo package and make it package-private
- Change permission handler to skip any empty groups (prevents odd command output e.g. for BukkitPermissions)

src/main/java/fr/xephi/authme/data/limbo/AuthGroupHandler.java

@@ -1,16 +1,14 @@
-package fr.xephi.authme.permission;
+package fr.xephi.authme.data.limbo;
 
 import fr.xephi.authme.ConsoleLogger;
-import fr.xephi.authme.data.limbo.LimboPlayer;
-import fr.xephi.authme.data.limbo.LimboService;
 import fr.xephi.authme.initialization.Reloadable;
+import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.properties.PluginSettings;
 import org.bukkit.entity.Player;
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
-import java.util.Optional;
 
 /**
  * Changes the permission group according to the auth status of the player and the configuration.
@@ -24,7 +22,7 @@ import java.util.Optional;
  * 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 {
+class AuthGroupHandler implements Reloadable {
 
     @Inject
     private PermissionsManager permissionsManager;
@@ -32,9 +30,6 @@ public class AuthGroupHandler implements Reloadable {
     @Inject
     private Settings settings;
 
-    @Inject
-    private LimboService limboService;
-
     private String unregisteredGroup;
     private String registeredGroup;
 
@@ -45,17 +40,15 @@ public class AuthGroupHandler implements Reloadable {
      * Sets the group of a player by its authentication status.
      *
      * @param player the player
+     * @param limbo the associated limbo player (nullable)
      * @param groupType the group type
      */
-    public void setGroup(Player player, AuthGroupType groupType) {
+    void setGroup(Player player, LimboPlayer limbo, AuthGroupType groupType) {
         if (!useAuthGroups()) {
             return;
         }
 
-        String primaryGroup = Optional
-            .ofNullable(limboService.getLimboPlayer(player.getName()))
-            .map(LimboPlayer::getGroup)
-            .orElse("");
+        String primaryGroup = limbo == null ? "" : limbo.getGroup();
 
         switch (groupType) {
             // Implementation note: some permission systems don't support players not being in any group,

src/main/java/fr/xephi/authme/data/limbo/AuthGroupType.java

@@ -1,9 +1,9 @@
-package fr.xephi.authme.permission;
+package fr.xephi.authme.data.limbo;
 
 /**
  * Represents the group type based on the user's auth status.
  */
-public enum AuthGroupType {
+enum AuthGroupType {
 
     /** Player does not have an account. */
     UNREGISTERED,

src/main/java/fr/xephi/authme/data/limbo/LimboService.java

@@ -34,6 +34,9 @@ public class LimboService {
     @Inject
     private LimboPersistence persistence;
 
+    @Inject
+    private AuthGroupHandler authGroupHandler;
+
     LimboService() {
     }
 
@@ -63,6 +66,8 @@ public class LimboService {
         taskManager.registerMessageTask(player, limboPlayer, isRegistered);
         taskManager.registerTimeoutTask(player, limboPlayer);
         helper.revokeLimboStates(player);
+        authGroupHandler.setGroup(player, limboPlayer,
+            isRegistered ? AuthGroupType.REGISTERED_UNAUTHENTICATED : AuthGroupType.UNREGISTERED);
         entries.put(name, limboPlayer);
         persistence.saveLimboPlayer(player, limboPlayer);
     }
@@ -91,7 +96,7 @@ public class LimboService {
      * Restores the limbo data and subsequently deletes the entry.
      * <p>
      * Note that teleportation on the player is performed by {@link fr.xephi.authme.service.TeleportationService} and
-     * changing the permission group is handled by {@link fr.xephi.authme.permission.AuthGroupHandler}.
+     * changing the permission group is handled by {@link fr.xephi.authme.data.limbo.AuthGroupHandler}.
      *
      * @param player the player whose data should be restored
      */
@@ -110,6 +115,7 @@ public class LimboService {
             ConsoleLogger.debug("Restored LimboPlayer stats for `{0}`", lowerName);
             persistence.removeLimboPlayer(player);
         }
+        authGroupHandler.setGroup(player, limbo, AuthGroupType.LOGGED_IN);
     }
 
     /**
@@ -119,11 +125,12 @@ public class LimboService {
      * @param player the player to reset the tasks for
      */
     public void replaceTasksAfterRegistration(Player player) {
-        getLimboOrLogError(player, "reset tasks")
-            .ifPresent(limbo -> {
-                taskManager.registerTimeoutTask(player, limbo);
-                taskManager.registerMessageTask(player, limbo, true);
-            });
+        Optional<LimboPlayer> limboPlayer = getLimboOrLogError(player, "reset tasks");
+        limboPlayer.ifPresent(limbo -> {
+            taskManager.registerTimeoutTask(player, limbo);
+            taskManager.registerMessageTask(player, limbo, true);
+        });
+        authGroupHandler.setGroup(player, limboPlayer.orElse(null), AuthGroupType.REGISTERED_UNAUTHENTICATED);
     }
 
     /**

src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java

@@ -49,7 +49,7 @@ class LimboServiceHelper {
      * Removes the data that is saved in a LimboPlayer from the player.
      * <p>
      * Note that teleportation on the player is performed by {@link fr.xephi.authme.service.TeleportationService} and
-     * changing the permission group is handled by {@link fr.xephi.authme.permission.AuthGroupHandler}.
+     * changing the permission group is handled by {@link fr.xephi.authme.data.limbo.AuthGroupHandler}.
      *
      * @param player the player to set defaults to
      */

src/main/java/fr/xephi/authme/permission/PermissionsManager.java

@@ -342,19 +342,20 @@ public class PermissionsManager implements Reloadable {
      * @param player     The player
      * @param groupNames The name of the groups to add.
      *
-     * @return True if succeed, false otherwise.
-     * False is also returned if this feature isn't supported for the current permissions system.
+     * @return True if at least one group was removed, false otherwise.
+     *         False is also returned if this feature isn't supported for the current permissions system.
      */
     public boolean removeGroups(Player player, String... groupNames) {
         // If no permissions system is used, return false
-        if (!isEnabled())
+        if (!isEnabled()) {
             return false;
+        }
 
         // Add each group to the user
-        boolean result = true;
+        boolean result = false;
         for (String groupName : groupNames) {
-            if (!handler.removeFromGroup(player, groupName)) {
-                result = false;
+            if (!groupName.isEmpty()) {
+                result |= handler.removeFromGroup(player, groupName);
             }
         }
 

src/main/java/fr/xephi/authme/process/join/AsynchronousJoin.java

@@ -9,7 +9,6 @@ import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.events.ProtectInventoryEvent;
 import fr.xephi.authme.events.RestoreSessionEvent;
 import fr.xephi.authme.message.MessageKey;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.permission.PlayerStatePermission;
 import fr.xephi.authme.process.AsynchronousProcess;
 import fr.xephi.authme.process.login.AsynchronousLogin;
@@ -111,8 +110,6 @@ public class AsynchronousJoin implements AsynchronousProcess {
         final boolean isAuthAvailable = database.isAuthAvailable(name);
 
         if (isAuthAvailable) {
-            service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
-
             // Protect inventory
             if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) {
                 ProtectInventoryEvent ev = bukkitService.createAndCallEvent(
@@ -129,14 +126,9 @@ public class AsynchronousJoin implements AsynchronousProcess {
                 bukkitService.runTaskOptionallyAsync(() -> asynchronousLogin.forceLogin(player));
                 return;
             }
-        } else {
-            // Groups logic
-            service.setGroup(player, AuthGroupType.UNREGISTERED);
-
+        } else if (!service.getProperty(RegistrationSettings.FORCE)) {
             // Skip if registration is optional
-            if (!service.getProperty(RegistrationSettings.FORCE)) {
-                return;
-            }
+            return;
         }
 
         final int registrationTimeout = service.getProperty(RestrictionSettings.TIMEOUT) * TICKS_PER_SECOND;

src/main/java/fr/xephi/authme/process/login/ProcessSyncPlayerLogin.java

@@ -6,8 +6,6 @@ import fr.xephi.authme.data.limbo.LimboService;
 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;
@@ -17,7 +15,6 @@ import fr.xephi.authme.service.TeleportationService;
 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.potion.PotionEffectType;
 
@@ -68,9 +65,8 @@ public class ProcessSyncPlayerLogin implements SynchronousProcess {
 
     public void processPlayerLogin(Player player) {
         final String name = player.getName().toLowerCase();
-
-        commonService.setGroup(player, AuthGroupType.LOGGED_IN);
         final LimboPlayer limbo = limboService.getLimboPlayer(name);
+
         // Limbo contains the State of the Player before /login
         if (limbo != null) {
             limboService.restoreData(player);

src/main/java/fr/xephi/authme/process/logout/ProcessSynchronousPlayerLogout.java

@@ -6,7 +6,6 @@ import fr.xephi.authme.data.limbo.LimboService;
 import fr.xephi.authme.events.LogoutEvent;
 import fr.xephi.authme.listener.protocollib.ProtocolLibService;
 import fr.xephi.authme.message.MessageKey;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.process.SynchronousProcess;
 import fr.xephi.authme.service.BukkitService;
 import fr.xephi.authme.service.CommonService;
@@ -75,7 +74,6 @@ public class ProcessSynchronousPlayerLogout implements SynchronousProcess {
 
         // Set player's data to unauthenticated
         limboService.createLimboPlayer(player, true);
-        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
     }
 
 }

src/main/java/fr/xephi/authme/process/register/ProcessSyncEmailRegister.java

@@ -3,7 +3,6 @@ package fr.xephi.authme.process.register;
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.data.limbo.LimboService;
 import fr.xephi.authme.message.MessageKey;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.process.SynchronousProcess;
 import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.util.PlayerUtils;
@@ -24,9 +23,7 @@ public class ProcessSyncEmailRegister implements SynchronousProcess {
     }
 
     public void processEmailRegister(Player player) {
-        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
         service.send(player, MessageKey.ACCOUNT_NOT_ACTIVATED);
-
         limboService.replaceTasksAfterRegistration(player);
 
         player.saveData();

src/main/java/fr/xephi/authme/process/register/ProcessSyncPasswordRegister.java

@@ -3,7 +3,6 @@ package fr.xephi.authme.process.register;
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.data.limbo.LimboService;
 import fr.xephi.authme.message.MessageKey;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.process.SynchronousProcess;
 import fr.xephi.authme.service.BungeeService;
 import fr.xephi.authme.service.CommonService;
@@ -48,7 +47,6 @@ public class ProcessSyncPasswordRegister implements SynchronousProcess {
     }
 
     public void processPasswordRegister(Player player) {
-        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
         service.send(player, MessageKey.REGISTER_SUCCESS);
 
         if (!service.getProperty(EmailSettings.MAIL_ACCOUNT).isEmpty()) {

src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java

@@ -8,8 +8,6 @@ import fr.xephi.authme.datasource.DataSource;
 import fr.xephi.authme.events.UnregisterByAdminEvent;
 import fr.xephi.authme.events.UnregisterByPlayerEvent;
 import fr.xephi.authme.message.MessageKey;
-import fr.xephi.authme.permission.AuthGroupHandler;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.process.AsynchronousProcess;
 import fr.xephi.authme.security.PasswordSecurity;
 import fr.xephi.authme.service.BukkitService;
@@ -50,9 +48,6 @@ public class AsynchronousUnregister implements AsynchronousProcess {
     @Inject
     private TeleportationService teleportationService;
 
-    @Inject
-    private AuthGroupHandler authGroupHandler;
-
     @Inject
     private CommandManager commandManager;
 
@@ -123,7 +118,6 @@ public class AsynchronousUnregister implements AsynchronousProcess {
                 applyBlindEffect(player);
             });
         }
-        authGroupHandler.setGroup(player, AuthGroupType.UNREGISTERED);
         service.send(player, MessageKey.UNREGISTERED_SUCCESS);
     }
 

src/main/java/fr/xephi/authme/service/CommonService.java

@@ -3,8 +3,6 @@ package fr.xephi.authme.service;
 import ch.jalu.configme.properties.Property;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.message.Messages;
-import fr.xephi.authme.permission.AuthGroupHandler;
-import fr.xephi.authme.permission.AuthGroupType;
 import fr.xephi.authme.permission.PermissionNode;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.settings.Settings;
@@ -27,9 +25,6 @@ public class CommonService {
     @Inject
     private PermissionsManager permissionsManager;
 
-    @Inject
-    private AuthGroupHandler authGroupHandler;
-
     CommonService() {
     }
 
@@ -85,16 +80,4 @@ public class CommonService {
     public boolean hasPermission(Player player, PermissionNode node) {
         return permissionsManager.hasPermission(player, node);
     }
-
-    /**
-     * Sets the permission group of the given player.
-     *
-     * @param player the player to process
-     * @param group the group to add the player to
-     */
-    // TODO ljacqu 20170304: Move this out of CommonService
-    public void setGroup(Player player, AuthGroupType group) {
-        authGroupHandler.setGroup(player, group);
-    }
-
 }