#761 Improve permissions group support

- Drop one auth group type in favor of three: logged in, registered but not logged in, and unregistered
- Move  properties to same parent path

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

@@ -5,14 +5,11 @@ import fr.xephi.authme.data.limbo.LimboCache;
 import fr.xephi.authme.data.limbo.LimboPlayer;
 import fr.xephi.authme.initialization.Reloadable;
 import fr.xephi.authme.settings.Settings;
-import fr.xephi.authme.settings.properties.HooksSettings;
 import fr.xephi.authme.settings.properties.PluginSettings;
-import fr.xephi.authme.settings.properties.SecuritySettings;
 import org.bukkit.entity.Player;
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
-import java.util.Arrays;
 
 /**
  * Changes the permission group according to the auth status of the player and the configuration.
@@ -28,7 +25,6 @@ public class AuthGroupHandler implements Reloadable {
     @Inject
     private LimboCache limboCache;
 
-    private String unloggedInGroup;
     private String unregisteredGroup;
     private String registeredGroup;
 
@@ -36,15 +32,15 @@ public class AuthGroupHandler implements Reloadable {
     }
 
     /**
-     * Set the group of a player, by its AuthMe group type.
+     * Sets the group of a player by its authentication status.
      *
-     * @param player The player.
+     * @param player the player
-     * @param group  The group type.
+     * @param groupType the group type
      *
-     * @return True if succeeded, false otherwise. False is also returned if groups aren't supported
+     * @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 group) {
+    public boolean setGroup(Player player, AuthGroupType groupType) {
         // Check whether the permissions check is enabled
         if (!settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) {
             return false;
@@ -56,71 +52,45 @@ public class AuthGroupHandler implements Reloadable {
             return false;
         }
 
-        switch (group) {
+        switch (groupType) {
             case UNREGISTERED:
-                // Remove the other group type groups, set the current group
+                // Remove the other group, set the current group
-                permissionsManager.removeGroups(player, Arrays.asList(registeredGroup, unloggedInGroup));
+                permissionsManager.removeGroups(player, registeredGroup);
                 return permissionsManager.addGroup(player, unregisteredGroup);
 
-            case REGISTERED:
+            case REGISTERED_UNAUTHENTICATED:
-                // Remove the other group type groups, set the current group
+                // Remove the other group, set the current group
-                permissionsManager.removeGroups(player, Arrays.asList(unregisteredGroup, unloggedInGroup));
+                permissionsManager.removeGroups(player, unregisteredGroup);
                 return permissionsManager.addGroup(player, registeredGroup);
 
-            case NOT_LOGGED_IN:
-                // Remove the other group type groups, set the current group
-                permissionsManager.removeGroups(player, Arrays.asList(unregisteredGroup, registeredGroup));
-                return permissionsManager.addGroup(player, unloggedInGroup);
-
             case LOGGED_IN:
-                // Get the player data
+                return restoreGroup(player);
-                LimboPlayer data = limboCache.getPlayerData(player.getName().toLowerCase());
-                if (data == null) {
-                    return false;
-                }
 
-                // Get the players group
-                String realGroup = data.getGroup();
-
-                // Remove the other group types groups, set the real group
-                permissionsManager.removeGroups(player,
-                    Arrays.asList(unregisteredGroup, registeredGroup, unloggedInGroup)
-                );
-                return permissionsManager.addGroup(player, realGroup);
             default:
-                return false;
+                throw new IllegalStateException("Encountered unhandled auth group type '" + groupType + "'");
         }
     }
 
-    /**
+    private boolean restoreGroup(Player player) {
-     * TODO: This method requires better explanation.
+        // Get the player's LimboPlayer
-     * <p>
+        LimboPlayer limbo = limboCache.getPlayerData(player.getName());
-     * Set the normal group of a player.
+        if (limbo == null) {
-     *
-     * @param player The player.
-     * @param group  The normal group.
-     *
-     * @return True on success, false on failure.
-     */
-    public boolean addNormal(Player player, String group) {
-        // Check whether the permissions check is enabled
-        if (!settings.getProperty(PluginSettings.ENABLE_PERMISSION_CHECK)) {
             return false;
         }
 
-        // Remove old groups
+        // Get the players group
-        permissionsManager.removeGroups(player, Arrays.asList(unregisteredGroup, registeredGroup, unloggedInGroup));
+        String realGroup = limbo.getGroup();
 
-        // Add the normal group, return the result
+        // Remove the other group types groups, set the real group
-        return permissionsManager.addGroup(player, group);
+        permissionsManager.removeGroups(player, unregisteredGroup, registeredGroup);
+        return permissionsManager.addGroup(player, realGroup);
     }
 
     @Override
     @PostConstruct
     public void reload() {
-        unloggedInGroup = settings.getProperty(SecuritySettings.UNLOGGEDIN_GROUP);
+        unregisteredGroup = settings.getProperty(PluginSettings.UNREGISTERED_GROUP);
-        unregisteredGroup = settings.getProperty(HooksSettings.UNREGISTERED_GROUP);
+        registeredGroup = settings.getProperty(PluginSettings.REGISTERED_GROUP);
-        registeredGroup = settings.getProperty(HooksSettings.REGISTERED_GROUP);
     }
 
 }

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

@@ -8,11 +8,8 @@ public enum AuthGroupType {
     /** Player does not have an account. */
     UNREGISTERED,
 
-    /** Registered? */
+    /** Player is registered but not logged in. */
-    REGISTERED, // TODO #761: Remove this or the NOT_LOGGED_IN one
+    REGISTERED_UNAUTHENTICATED,
-
-    /** Player is registered and not logged in. */
-    NOT_LOGGED_IN,
 
     /** Player is logged in. */
     LOGGED_IN

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

@@ -289,7 +289,7 @@ public class PermissionsManager implements Reloadable {
      * @return True if the player is in the specified group, false otherwise.
      * False is also returned if groups aren't supported by the used permissions system.
      */
-    public boolean inGroup(Player player, String groupName) {
+    public boolean isInGroup(Player player, String groupName) {
         // If no permissions system is used, return false
         if (!isEnabled())
             return false;
@@ -307,42 +307,12 @@ public class PermissionsManager implements Reloadable {
      * False is also returned if this feature isn't supported for the current permissions system.
      */
     public boolean addGroup(Player player, String groupName) {
-        if (StringUtils.isEmpty(groupName)) {
+        if (!isEnabled() || StringUtils.isEmpty(groupName)) {
             return false;
         }
-
-        // If no permissions system is used, return false
-        if (!isEnabled()) {
-            return false;
-        }
-
         return handler.addToGroup(player, groupName);
     }
 
-    /**
-     * Add the permission groups of a player, if supported.
-     *
-     * @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.
-     */
-    public boolean addGroups(Player player, List<String> groupNames) {
-        // If no permissions system is used, return false
-        if (!isEnabled())
-            return false;
-
-        // Add each group to the user
-        boolean result = true;
-        for (String groupName : groupNames)
-            if (!addGroup(player, groupName))
-                result = false;
-
-        // Return the result
-        return result;
-    }
-
     /**
      * Remove the permission group of a player, if supported.
      *
@@ -352,8 +322,7 @@ public class PermissionsManager implements Reloadable {
      * @return True if succeed, false otherwise.
      * False is also returned if this feature isn't supported for the current permissions system.
      */
-    public boolean removeGroup(Player player, String groupName) {
+    public boolean removeGroups(Player player, String groupName) {
-        // If no permissions system is used, return false
         if (!isEnabled())
             return false;
 
@@ -369,16 +338,18 @@ public class PermissionsManager implements Reloadable {
      * @return True if succeed, false otherwise.
      * False is also returned if this feature isn't supported for the current permissions system.
      */
-    public boolean removeGroups(Player player, List<String> groupNames) {
+    public boolean removeGroups(Player player, String... groupNames) {
         // If no permissions system is used, return false
         if (!isEnabled())
             return false;
 
         // Add each group to the user
         boolean result = true;
-        for (String groupName : groupNames)
+        for (String groupName : groupNames) {
-            if (!removeGroup(player, groupName))
+            if (!handler.removeFromGroup(player, groupName)) {
                 result = false;
+            }
+        }
 
         // Return the result
         return result;
@@ -402,41 +373,6 @@ public class PermissionsManager implements Reloadable {
         return handler.setGroup(player, groupName);
     }
 
-    /**
-     * Set the permission groups of a player, if supported.
-     * This clears the current groups of the player.
-     *
-     * @param player     The player
-     * @param groupNames The name of the groups to set.
-     *
-     * @return True if succeed, false otherwise.
-     * False is also returned if this feature isn't supported for the current permissions system.
-     */
-    public boolean setGroups(Player player, List<String> groupNames) {
-        // If no permissions system is used or if there's no group supplied, return false
-        if (!isEnabled() || groupNames.isEmpty())
-            return false;
-
-        // Set the main group
-        if (!setGroup(player, groupNames.get(0)))
-            return false;
-
-        // Add the rest of the groups
-        boolean result = true;
-        for (int i = 1; i < groupNames.size(); i++) {
-            // Get the group name
-            String groupName = groupNames.get(i);
-
-            // Add this group
-            if (!addGroup(player, groupName)) {
-                result = false;
-            }
-        }
-
-        // Return the result
-        return result;
-    }
-
     /**
      * Remove all groups of the specified player, if supported.
      * Systems like Essentials GroupManager don't allow all groups to be removed from a player, thus the user will stay
@@ -456,6 +392,6 @@ public class PermissionsManager implements Reloadable {
         List<String> groupNames = getGroups(player);
 
         // Remove each group
-        return removeGroups(player, groupNames);
+        return removeGroups(player, groupNames.toArray(new String[groupNames.size()]));
     }
 }

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

@@ -112,7 +112,7 @@ public class AsynchronousJoin implements AsynchronousProcess {
 
         if (isAuthAvailable) {
             limboCache.addPlayerData(player);
-            service.setGroup(player, AuthGroupType.NOT_LOGGED_IN);
+            service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
 
             // Protect inventory
             if (service.getProperty(PROTECT_INVENTORY_BEFORE_LOGIN)) {

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

@@ -77,7 +77,7 @@ public class ProcessSynchronousPlayerLogout implements SynchronousProcess {
         }
 
         // Set player's data to unauthenticated
-        service.setGroup(player, AuthGroupType.NOT_LOGGED_IN);
+        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
         player.setOp(false);
         player.setAllowFlight(false);
         // Remove speed

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

@@ -3,9 +3,8 @@ package fr.xephi.authme.process.register;
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.permission.AuthGroupType;
-import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.process.SynchronousProcess;
-import fr.xephi.authme.settings.properties.HooksSettings;
+import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.task.LimboPlayerTaskManager;
 import fr.xephi.authme.util.PlayerUtils;
 import org.bukkit.entity.Player;
@@ -25,12 +24,10 @@ public class ProcessSyncEmailRegister implements SynchronousProcess {
     }
 
     public void processEmailRegister(Player player) {
-        final String name = player.getName().toLowerCase();
+        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
-        if (!service.getProperty(HooksSettings.REGISTERED_GROUP).isEmpty()) {
-            service.setGroup(player, AuthGroupType.REGISTERED);
-        }
         service.send(player, MessageKey.ACCOUNT_NOT_ACTIVATED);
 
+        final String name = player.getName().toLowerCase();
         limboPlayerTaskManager.registerTimeoutTask(player);
         limboPlayerTaskManager.registerMessageTask(name, true);
 

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

@@ -4,12 +4,11 @@ import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.data.limbo.LimboCache;
 import fr.xephi.authme.message.MessageKey;
 import fr.xephi.authme.permission.AuthGroupType;
-import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.process.SynchronousProcess;
 import fr.xephi.authme.service.BungeeService;
+import fr.xephi.authme.service.CommonService;
 import fr.xephi.authme.settings.commandconfig.CommandManager;
 import fr.xephi.authme.settings.properties.EmailSettings;
-import fr.xephi.authme.settings.properties.HooksSettings;
 import fr.xephi.authme.settings.properties.RegistrationSettings;
 import fr.xephi.authme.task.LimboPlayerTaskManager;
 import fr.xephi.authme.util.PlayerUtils;
@@ -56,10 +55,7 @@ public class ProcessSyncPasswordRegister implements SynchronousProcess {
     }
 
     public void processPasswordRegister(Player player) {
-        if (!service.getProperty(HooksSettings.REGISTERED_GROUP).isEmpty()) {
+        service.setGroup(player, AuthGroupType.REGISTERED_UNAUTHENTICATED);
-            service.setGroup(player, AuthGroupType.REGISTERED);
-        }
-
         service.send(player, MessageKey.REGISTER_SUCCESS);
 
         if (!service.getProperty(EmailSettings.MAIL_ACCOUNT).isEmpty()) {

src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java

@@ -71,6 +71,7 @@ public class SettingsMigrationService extends PlainMigrationService {
             | hasOldHelpHeaderProperty(resource)
             | hasSupportOldPasswordProperty(resource)
             | convertToRegistrationType(resource)
+            | mergeAndMovePermissionGroupSettings(resource)
             || hasDeprecatedProperties(resource);
     }
 
@@ -251,6 +252,26 @@ public class SettingsMigrationService extends PlainMigrationService {
         return true;
     }
 
+    private static boolean mergeAndMovePermissionGroupSettings(PropertyResource resource) {
+        boolean performedChanges;
+
+        // We have two old settings replaced by only one: move the first non-empty one
+        Property<String> oldUnloggedInGroup = newProperty("settings.security.unLoggedinGroup", "");
+        Property<String> oldRegisteredGroup = newProperty("GroupOptions.RegisteredPlayerGroup", "");
+        if (!oldUnloggedInGroup.getValue(resource).isEmpty()) {
+            performedChanges = moveProperty(oldUnloggedInGroup, PluginSettings.REGISTERED_GROUP, resource);
+        } else {
+            performedChanges = moveProperty(oldRegisteredGroup, PluginSettings.REGISTERED_GROUP, resource);
+        }
+
+        // Move paths of other old options
+        performedChanges |= moveProperty(newProperty("GroupOptions.UnregisteredPlayerGroup", ""),
+            PluginSettings.UNREGISTERED_GROUP, resource);
+        performedChanges |= moveProperty(newProperty("permission.EnablePermissionCheck", false),
+            PluginSettings.ENABLE_PERMISSION_CHECK, resource);
+        return performedChanges;
+    }
+
     /**
      * Checks for an old property path and moves it to a new path if present.
      *
@@ -264,9 +285,10 @@ public class SettingsMigrationService extends PlainMigrationService {
                                             Property<T> newProperty,
                                             PropertyResource resource) {
         if (resource.contains(oldProperty.getPath())) {
-            ConsoleLogger.info("Detected deprecated property " + oldProperty.getPath());
+            if (resource.contains(newProperty.getPath())) {
-            if (!resource.contains(newProperty.getPath())) {
+                ConsoleLogger.info("Detected deprecated property " + oldProperty.getPath());
-                ConsoleLogger.info("Renamed " + oldProperty.getPath() + " to " + newProperty.getPath());
+            } else {
+                ConsoleLogger.info("Renaming " + oldProperty.getPath() + " to " + newProperty.getPath());
                 resource.setValue(newProperty.getPath(), oldProperty.getValue(resource));
             }
             return true;

src/main/java/fr/xephi/authme/settings/properties/HooksSettings.java

@@ -58,13 +58,6 @@ public class HooksSettings implements SettingsHolder {
     public static final Property<String> WORDPRESS_TABLE_PREFIX =
         newProperty("ExternalBoardOptions.wordpressTablePrefix", "wp_");
 
-    @Comment("Unregistered permission group")
-    public static final Property<String> UNREGISTERED_GROUP =
-        newProperty("GroupOptions.UnregisteredPlayerGroup", "");
-
-    @Comment("Registered permission group")
-    public static final Property<String> REGISTERED_GROUP =
-        newProperty("GroupOptions.RegisteredPlayerGroup", "");
 
     private HooksSettings() {
     }

src/main/java/fr/xephi/authme/settings/properties/PluginSettings.java

@@ -44,13 +44,33 @@ public class PluginSettings implements SettingsHolder {
         newProperty("settings.messagesLanguage", "en");
 
     @Comment({
-        "Take care with this option; if you want",
+        "Enables switching a player to defined permission groups before they log in.",
-        "to use group switching of AuthMe",
+        "See below for a detailed explanation."
-        "for unloggedIn players, set this setting to true.",
-        "Default is false."
     })
     public static final Property<Boolean> ENABLE_PERMISSION_CHECK =
-        newProperty("permission.EnablePermissionCheck", false);
+        newProperty("GroupOptions.enablePermissionCheck", false);
+
+    @Comment({
+        "This is a very important option: if a registered player joins the server",
+        "AuthMe will switch him to unLoggedInGroup. This should prevent all major exploits.",
+        "You can set up your permission plugin with this special group to have no permissions,",
+        "or only permission to chat (or permission to send private messages etc.).",
+        "The better way is to set up this group with few permissions, so if a player",
+        "tries to exploit an account they can do only what you've defined for the group.",
+        "After login, the player will be moved to his correct permissions group!",
+        "Please note that the group name is case-sensitive, so 'admin' is different from 'Admin'",
+        "Otherwise your group will be wiped and the player will join in the default group []!",
+        "Example: registeredPlayerGroup: 'NotLogged'"
+    })
+    public static final Property<String> REGISTERED_GROUP =
+        newProperty("GroupOptions.registeredPlayerGroup", "");
+
+    @Comment({
+        "Similar to above, unregistered players can be set to the following",
+        "permissions group"
+    })
+    public static final Property<String> UNREGISTERED_GROUP =
+        newProperty("GroupOptions.unregisteredPlayerGroup", "");
 
     @Comment({
         "Log level: INFO, FINE, DEBUG. Use INFO for general messages,",

src/main/java/fr/xephi/authme/settings/properties/SecuritySettings.java

@@ -48,22 +48,6 @@ public class SecuritySettings implements SettingsHolder {
     public static final Property<Integer> MAX_PASSWORD_LENGTH =
         newProperty("settings.security.passwordMaxLength", 30);
 
-    @Comment({
-        "This is a very important option: every time a player joins the server,",
-        "if they are registered, AuthMe will switch him to unLoggedInGroup.",
-        "This should prevent all major exploits.",
-        "You can set up your permission plugin with this special group to have no permissions,",
-        "or only permission to chat (or permission to send private messages etc.).",
-        "The better way is to set up this group with few permissions, so if a player",
-        "tries to exploit an account they can do only what you've defined for the group.",
-        "After, a logged in player will be moved to his correct permissions group!",
-        "Please note that the group name is case-sensitive, so 'admin' is different from 'Admin'",
-        "Otherwise your group will be wiped and the player will join in the default group []!",
-        "Example unLoggedinGroup: NotLogged"
-    })
-    public static final Property<String> UNLOGGEDIN_GROUP =
-        newProperty("settings.security.unLoggedinGroup", "unLoggedinGroup");
-
     @Comment({
         "Possible values: SHA256, BCRYPT, BCRYPT2Y, PBKDF2, SALTEDSHA512, WHIRLPOOL,",
         "MYBB, IPB3, PHPBB, PHPFUSION, SMF, XENFORO, XAUTH, JOOMLA, WBB3, WBB4, MD5VB,",

src/test/java/fr/xephi/authme/settings/SettingsMigrationServiceTest.java

@@ -18,7 +18,10 @@ import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 
 import static fr.xephi.authme.TestHelper.getJarFile;
+import static fr.xephi.authme.settings.properties.PluginSettings.ENABLE_PERMISSION_CHECK;
 import static fr.xephi.authme.settings.properties.PluginSettings.LOG_LEVEL;
+import static fr.xephi.authme.settings.properties.PluginSettings.REGISTERED_GROUP;
+import static fr.xephi.authme.settings.properties.PluginSettings.UNREGISTERED_GROUP;
 import static fr.xephi.authme.settings.properties.RegistrationSettings.DELAY_JOIN_MESSAGE;
 import static fr.xephi.authme.settings.properties.RegistrationSettings.REGISTER_SECOND_ARGUMENT;
 import static fr.xephi.authme.settings.properties.RegistrationSettings.REGISTRATION_TYPE;
@@ -65,6 +68,9 @@ public class SettingsMigrationServiceTest {
         assertThat(settings.getProperty(LOG_LEVEL), equalTo(LogLevel.INFO));
         assertThat(settings.getProperty(REGISTRATION_TYPE), equalTo(RegistrationType.EMAIL));
         assertThat(settings.getProperty(REGISTER_SECOND_ARGUMENT), equalTo(RegisterSecondaryArgument.CONFIRMATION));
+        assertThat(settings.getProperty(ENABLE_PERMISSION_CHECK), equalTo(true));
+        assertThat(settings.getProperty(REGISTERED_GROUP), equalTo("unLoggedinGroup"));
+        assertThat(settings.getProperty(UNREGISTERED_GROUP), equalTo(""));
 
         // Check migration of old setting to email.html
         assertThat(Files.readLines(new File(dataFolder, "email.html"), StandardCharsets.UTF_8),

src/test/resources/fr/xephi/authme/settings/config-old.yml

@@ -290,7 +290,7 @@ permission:
     # to use Vault and Group Switching of
     # AuthMe for unloggedIn players put true
     # below, default is false.
-    EnablePermissionCheck: false
+    EnablePermissionCheck: true
 BackupSystem:
    # Enable or Disable Automatic Backup
     ActivateBackup: false