#306 Add permission manager to command service

- Inject permission manager into command service explicitly via constructor
- Change command mapper to only care about generating FoundCommandResult objects, and command handler to worry about dealing with it later on

src/main/java/fr/xephi/authme/AuthMe.java

@@ -422,9 +422,9 @@ public class AuthMe extends JavaPlugin {
                                                     PasswordSecurity passwordSecurity, NewSetting settings) {
         HelpProvider helpProvider = new HelpProvider(permissionsManager);
         Set<CommandDescription> baseCommands = CommandInitializer.buildCommands();
-        CommandMapper mapper = new CommandMapper(baseCommands, messages, permissionsManager, helpProvider);
+        CommandMapper mapper = new CommandMapper(baseCommands, permissionsManager);
         CommandService commandService = new CommandService(
-            this, mapper, helpProvider, messages, passwordSecurity, settings);
+            this, mapper, helpProvider, messages, passwordSecurity, permissionsManager, settings);
         return new CommandHandler(commandService);
     }
 

src/main/java/fr/xephi/authme/command/CommandHandler.java

@@ -3,6 +3,9 @@ package fr.xephi.authme.command;
 import java.util.ArrayList;
 import java.util.List;
 
+import fr.xephi.authme.AuthMe;
+import fr.xephi.authme.command.help.HelpProvider;
+import org.bukkit.ChatColor;
 import org.bukkit.command.CommandSender;
 
 import fr.xephi.authme.util.StringUtils;
@@ -13,6 +16,12 @@ import fr.xephi.authme.util.StringUtils;
  */
 public class CommandHandler {
 
+    /**
+     * The threshold for suggesting a similar command. If the difference is below this value, we will
+     * ask the player whether he meant the similar command.
+     */
+    private static final double SUGGEST_COMMAND_THRESHOLD = 0.75;
+
     private final CommandService commandService;
 
     /**
@@ -40,14 +49,32 @@ public class CommandHandler {
         parts.add(0, bukkitCommandLabel);
 
         FoundCommandResult result = commandService.mapPartsToCommand(sender, parts);
-        if (FoundResultStatus.SUCCESS.equals(result.getResultStatus())) {
+        handleCommandResult(sender, result);
-            executeCommand(sender, result);
-        } else {
-            commandService.outputMappingError(sender, result);
-        }
         return !FoundResultStatus.MISSING_BASE_COMMAND.equals(result.getResultStatus());
     }
 
+    private void handleCommandResult(CommandSender sender, FoundCommandResult result) {
+        switch (result.getResultStatus()) {
+            case SUCCESS:
+                executeCommand(sender, result);
+                break;
+            case MISSING_BASE_COMMAND:
+                sender.sendMessage(ChatColor.DARK_RED + "Failed to parse " + AuthMe.getPluginName() + " command!");
+                break;
+            case INCORRECT_ARGUMENTS:
+                sendImproperArgumentsMessage(sender, result);
+                break;
+            case UNKNOWN_LABEL:
+                sendUnknownCommandMessage(sender, result);
+                break;
+            case NO_PERMISSION:
+                sendPermissionDeniedError(sender);
+                break;
+            default:
+                throw new IllegalStateException("Unknown result status '" + result.getResultStatus() + "'");
+        }
+    }
+
     /**
      * Execute the command for the given command sender.
      *
@@ -76,6 +103,46 @@ public class CommandHandler {
         return cleanArguments;
     }
 
+    /**
+     * Show an "unknown command" message to the user and suggest an existing command if its similarity is within
+     * the defined threshold.
+     *
+     * @param sender The command sender
+     * @param result The command that was found during the mapping process
+     */
+    private static void sendUnknownCommandMessage(CommandSender sender, FoundCommandResult result) {
+        sender.sendMessage(ChatColor.DARK_RED + "Unknown command!");
 
+        // Show a command suggestion if available and the difference isn't too big
+        if (result.getDifference() <= SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) {
+            sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD
+                + CommandUtils.constructCommandPath(result.getCommandDescription()) + ChatColor.YELLOW + "?");
+        }
+
+        sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + result.getLabels().get(0)
+            + " help" + ChatColor.YELLOW + " to view help.");
+    }
+
+    private void sendImproperArgumentsMessage(CommandSender sender, FoundCommandResult result) {
+        CommandDescription command = result.getCommandDescription();
+        if (!commandService.getPermissionsManager().hasPermission(sender, command)) {
+            sendPermissionDeniedError(sender);
+            return;
+        }
+
+        // Show the command argument help
+        sender.sendMessage(ChatColor.DARK_RED + "Incorrect command arguments!");
+        commandService.outputHelp(sender, result, HelpProvider.SHOW_ARGUMENTS);
+
+        List<String> labels = result.getLabels();
+        String childLabel = labels.size() >= 2 ? labels.get(1) : "";
+        sender.sendMessage(ChatColor.GOLD + "Detailed help: " + ChatColor.WHITE
+            + "/" + labels.get(0) + " help " + childLabel);
+    }
+
+    // TODO ljacqu 20151212: Remove me once I am a MessageKey
+    private static void sendPermissionDeniedError(CommandSender sender) {
+        sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!");
+    }
 
 }

src/main/java/fr/xephi/authme/command/CommandMapper.java

@@ -1,13 +1,11 @@
 package fr.xephi.authme.command;
 
-import fr.xephi.authme.AuthMe;
 import fr.xephi.authme.command.executable.HelpCommand;
 import fr.xephi.authme.command.help.HelpProvider;
 import fr.xephi.authme.output.Messages;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.util.CollectionUtils;
 import fr.xephi.authme.util.StringUtils;
-import org.bukkit.ChatColor;
 import org.bukkit.command.CommandSender;
 
 import java.util.ArrayList;
@@ -19,101 +17,24 @@ import static fr.xephi.authme.command.FoundResultStatus.MISSING_BASE_COMMAND;
 import static fr.xephi.authme.command.FoundResultStatus.UNKNOWN_LABEL;
 
 /**
- * The AuthMe command handler, responsible for mapping incoming command parts to the correct {@link CommandDescription}
+ * The AuthMe command handler, responsible for mapping incoming
- * or to display help messages for erroneous invocations (unknown command, no permission, etc.).
+ * command parts to the correct {@link CommandDescription}.
  */
 public class CommandMapper {
 
-    /**
-     * The threshold for suggesting a similar command. If the difference is below this value, we will
-     * ask the player whether he meant the similar command.
-     */
-    private static final double SUGGEST_COMMAND_THRESHOLD = 0.75;
-
     /**
      * The class of the help command, to which the base label should also be passed in the arguments.
      */
     private static final Class<? extends ExecutableCommand> HELP_COMMAND_CLASS = HelpCommand.class;
 
     private final Set<CommandDescription> baseCommands;
-    private final Messages messages;
     private final PermissionsManager permissionsManager;
-    private final HelpProvider helpProvider;
 
-    public CommandMapper(Set<CommandDescription> baseCommands, Messages messages,
+    public CommandMapper(Set<CommandDescription> baseCommands, PermissionsManager permissionsManager) {
-                         PermissionsManager permissionsManager, HelpProvider helpProvider) {
         this.baseCommands = baseCommands;
-        this.messages = messages;
         this.permissionsManager = permissionsManager;
-        this.helpProvider = helpProvider;
     }
 
-    public void outputStandardError(CommandSender sender, FoundCommandResult result) {
-        switch (result.getResultStatus()) {
-            case SUCCESS:
-                // Successful mapping, so no error to output
-                break;
-            case MISSING_BASE_COMMAND:
-                sender.sendMessage(ChatColor.DARK_RED + "Failed to parse " + AuthMe.getPluginName() + " command!");
-                break;
-            case INCORRECT_ARGUMENTS:
-                sendImproperArgumentsMessage(sender, result);
-                break;
-            case UNKNOWN_LABEL:
-                sendUnknownCommandMessage(sender, result);
-                break;
-            case NO_PERMISSION:
-                sendPermissionDeniedError(sender);
-                break;
-            default:
-                throw new IllegalStateException("Unknown result status '" + result.getResultStatus() + "'");
-        }
-    }
-
-    /**
-     * Show an "unknown command" message to the user and suggest an existing command if its similarity is within
-     * the defined threshold.
-     *
-     * @param sender The command sender
-     * @param result The command that was found during the mapping process
-     */
-    private static void sendUnknownCommandMessage(CommandSender sender, FoundCommandResult result) {
-        sender.sendMessage(ChatColor.DARK_RED + "Unknown command!");
-
-        // Show a command suggestion if available and the difference isn't too big
-        if (result.getDifference() <= SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) {
-            sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD
-                + CommandUtils.constructCommandPath(result.getCommandDescription()) + ChatColor.YELLOW + "?");
-        }
-
-        sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + result.getLabels().get(0)
-            + " help" + ChatColor.YELLOW + " to view help.");
-    }
-
-    private void sendImproperArgumentsMessage(CommandSender sender, FoundCommandResult result) {
-        CommandDescription command = result.getCommandDescription();
-        if (!permissionsManager.hasPermission(sender, command)) {
-            sendPermissionDeniedError(sender);
-            return;
-        }
-
-        // Show the command argument help
-        sender.sendMessage(ChatColor.DARK_RED + "Incorrect command arguments!");
-        List<String> lines = helpProvider.printHelp(sender, result, HelpProvider.SHOW_ARGUMENTS);
-        for (String line : lines) {
-            sender.sendMessage(line);
-        }
-
-        List<String> labels = result.getLabels();
-        String childLabel = labels.size() >= 2 ? labels.get(1) : "";
-        sender.sendMessage(ChatColor.GOLD + "Detailed help: " + ChatColor.WHITE
-            + "/" + labels.get(0) + " help " + childLabel);
-    }
-
-    // TODO ljacqu 20151212: Remove me once I am a MessageKey
-    private static void sendPermissionDeniedError(CommandSender sender) {
-        sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!");
-    }
 
     /**
      * Map incoming command parts to a command. This processes all parts and distinguishes the labels from arguments.

src/main/java/fr/xephi/authme/command/CommandService.java

@@ -1,11 +1,5 @@
 package fr.xephi.authme.command;
 
-import java.util.List;
-
-import fr.xephi.authme.settings.custom.NewSetting;
-import fr.xephi.authme.settings.domain.Property;
-import org.bukkit.command.CommandSender;
-
 import fr.xephi.authme.AuthMe;
 import fr.xephi.authme.command.help.HelpProvider;
 import fr.xephi.authme.datasource.DataSource;
@@ -14,6 +8,11 @@ import fr.xephi.authme.output.Messages;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.process.Management;
 import fr.xephi.authme.security.PasswordSecurity;
+import fr.xephi.authme.settings.custom.NewSetting;
+import fr.xephi.authme.settings.domain.Property;
+import org.bukkit.command.CommandSender;
+
+import java.util.List;
 
 /**
  * Service for implementations of {@link ExecutableCommand} to execute some common tasks.
@@ -26,6 +25,7 @@ public class CommandService {
     private final HelpProvider helpProvider;
     private final CommandMapper commandMapper;
     private final PasswordSecurity passwordSecurity;
+    private final PermissionsManager permissionsManager;
     private final NewSetting settings;
 
     /**
@@ -36,14 +36,18 @@ public class CommandService {
      * @param helpProvider Help provider
      * @param messages Messages instance
      * @param passwordSecurity The Password Security instance
+     * @param permissionsManager The permissions manager
+     * @param settings The settings manager
      */
     public CommandService(AuthMe authMe, CommandMapper commandMapper, HelpProvider helpProvider, Messages messages,
-                          PasswordSecurity passwordSecurity, NewSetting settings) {
+                          PasswordSecurity passwordSecurity, PermissionsManager permissionsManager,
+                          NewSetting settings) {
         this.authMe = authMe;
         this.messages = messages;
         this.helpProvider = helpProvider;
         this.commandMapper = commandMapper;
         this.passwordSecurity = passwordSecurity;
+        this.permissionsManager = permissionsManager;
         this.settings = settings;
     }
 
@@ -79,17 +83,6 @@ public class CommandService {
         return commandMapper.mapPartsToCommand(sender, commandParts);
     }
 
-    /**
-     * Output the standard error message for the status in the provided {@link FoundCommandResult} object.
-     * Does not output anything for successful mappings.
-     *
-     * @param sender The sender to output the error to
-     * @param result The mapping result to process
-     */
-    public void outputMappingError(CommandSender sender, FoundCommandResult result) {
-        commandMapper.outputStandardError(sender, result);
-    }
-
     /**
      * Run the given task asynchronously with the Bukkit scheduler.
      *
@@ -146,8 +139,7 @@ public class CommandService {
      * @return the permissions manager
      */
     public PermissionsManager getPermissionsManager() {
-        // TODO ljacqu 20151226: Might be nicer to pass the perm manager via constructor
+        return permissionsManager;
-        return authMe.getPermissionsManager();
     }
 
     /**

src/main/java/fr/xephi/authme/security/PasswordSecurity.java

@@ -62,7 +62,7 @@ public class PasswordSecurity {
      * @param hashedPassword The encrypted password to test the clear-text password against
      * @param playerName     The name of the player
      *
-     * @return True if the
+     * @return True if there was a password match with another encryption method, false otherwise
      */
     private boolean compareWithAllEncryptionMethods(String password, HashedPassword hashedPassword,
                                                     String playerName) {

src/test/java/fr/xephi/authme/command/CommandHandlerTest.java

@@ -14,6 +14,7 @@ import static fr.xephi.authme.command.FoundResultStatus.SUCCESS;
 import static java.util.Arrays.asList;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsString;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyListOf;
@@ -89,7 +90,9 @@ public class CommandHandlerTest {
         assertThat(captor.getValue(), contains("unreg", "testPlayer"));
 
         verify(command, never()).getExecutableCommand();
-        verify(serviceMock).outputMappingError(eq(sender), any(FoundCommandResult.class));
+        ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
+        verify(sender).sendMessage(captor.capture());
+        assertThat(captor.getValue(), containsString("don't have permission"));
     }
 
     @Test

src/test/java/fr/xephi/authme/command/CommandMapperTest.java

@@ -1,7 +1,5 @@
 package fr.xephi.authme.command;
 
-import fr.xephi.authme.command.help.HelpProvider;
-import fr.xephi.authme.output.Messages;
 import fr.xephi.authme.permission.PermissionsManager;
 import org.bukkit.command.CommandSender;
 import org.junit.Before;
@@ -43,7 +41,7 @@ public class CommandMapperTest {
     @Before
     public void setUpMocks() {
         permissionsManagerMock = mock(PermissionsManager.class);
-        mapper = new CommandMapper(commands, mock(Messages.class), permissionsManagerMock, mock(HelpProvider.class));
+        mapper = new CommandMapper(commands, permissionsManagerMock);
     }
 
     // -----------

src/test/java/fr/xephi/authme/command/CommandServiceTest.java

@@ -39,6 +39,7 @@ public class CommandServiceTest {
     private Messages messages;
     private PasswordSecurity passwordSecurity;
     private CommandService commandService;
+    private PermissionsManager permissionsManager;
     private NewSetting settings;
 
     @Before
@@ -48,8 +49,10 @@ public class CommandServiceTest {
         helpProvider = mock(HelpProvider.class);
         messages = mock(Messages.class);
         passwordSecurity = mock(PasswordSecurity.class);
+        permissionsManager = mock(PermissionsManager.class);
         settings = mock(NewSetting.class);
-        commandService = new CommandService(authMe, commandMapper, helpProvider, messages, passwordSecurity, settings);
+        commandService = new CommandService(
+            authMe, commandMapper, helpProvider, messages, passwordSecurity, permissionsManager, settings);
     }
 
     @Test
@@ -92,19 +95,6 @@ public class CommandServiceTest {
         verify(commandMapper).mapPartsToCommand(sender, commandParts);
     }
 
-    @Test
-    public void shouldOutputMappingError() {
-        // given
-        CommandSender sender = mock(CommandSender.class);
-        FoundCommandResult result = mock(FoundCommandResult.class);
-
-        // when
-        commandService.outputMappingError(sender, result);
-
-        // then
-        verify(commandMapper).outputStandardError(sender, result);
-    }
-
     @Test
     @Ignore
     public void shouldRunTaskInAsync() {
@@ -174,16 +164,11 @@ public class CommandServiceTest {
 
     @Test
     public void shouldReturnPermissionsManager() {
-        // given
+        // given / when
-        PermissionsManager manager = mock(PermissionsManager.class);
-        given(authMe.getPermissionsManager()).willReturn(manager);
-
-        // when
         PermissionsManager result = commandService.getPermissionsManager();
 
         // then
-        assertThat(result, equalTo(manager));
+        assertThat(result, equalTo(permissionsManager));
-        verify(authMe).getPermissionsManager();
     }
 
     @Test