From fc0b7c46ac52fc356a6c80f99b09aba3834447e1 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 9 Jan 2016 21:57:20 +0100 Subject: [PATCH] #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 | 4 +- .../xephi/authme/command/CommandHandler.java | 77 +++++++++++++++-- .../xephi/authme/command/CommandMapper.java | 85 +------------------ .../xephi/authme/command/CommandService.java | 32 +++---- .../authme/security/PasswordSecurity.java | 2 +- .../authme/command/CommandHandlerTest.java | 5 +- .../authme/command/CommandMapperTest.java | 4 +- .../authme/command/CommandServiceTest.java | 27 ++---- 8 files changed, 101 insertions(+), 135 deletions(-) diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index e9de7d941..5e7c383f1 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/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 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); } diff --git a/src/main/java/fr/xephi/authme/command/CommandHandler.java b/src/main/java/fr/xephi/authme/command/CommandHandler.java index e3ab671b4..37c1a2a16 100644 --- a/src/main/java/fr/xephi/authme/command/CommandHandler.java +++ b/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())) { - executeCommand(sender, result); - } else { - commandService.outputMappingError(sender, result); - } + handleCommandResult(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 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!"); + } } diff --git a/src/main/java/fr/xephi/authme/command/CommandMapper.java b/src/main/java/fr/xephi/authme/command/CommandMapper.java index 69d31fe5f..8bc5dccdd 100644 --- a/src/main/java/fr/xephi/authme/command/CommandMapper.java +++ b/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} - * or to display help messages for erroneous invocations (unknown command, no permission, etc.). + * The AuthMe command handler, responsible for mapping incoming + * 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 HELP_COMMAND_CLASS = HelpCommand.class; private final Set baseCommands; - private final Messages messages; private final PermissionsManager permissionsManager; - private final HelpProvider helpProvider; - public CommandMapper(Set baseCommands, Messages messages, - PermissionsManager permissionsManager, HelpProvider helpProvider) { + public CommandMapper(Set baseCommands, PermissionsManager permissionsManager) { 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 lines = helpProvider.printHelp(sender, result, HelpProvider.SHOW_ARGUMENTS); - for (String line : lines) { - sender.sendMessage(line); - } - - List 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. diff --git a/src/main/java/fr/xephi/authme/command/CommandService.java b/src/main/java/fr/xephi/authme/command/CommandService.java index 8a986b0ee..8dd1da599 100644 --- a/src/main/java/fr/xephi/authme/command/CommandService.java +++ b/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 authMe.getPermissionsManager(); + return permissionsManager; } /** diff --git a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java b/src/main/java/fr/xephi/authme/security/PasswordSecurity.java index 552b64dd2..ddeb979d3 100644 --- a/src/main/java/fr/xephi/authme/security/PasswordSecurity.java +++ b/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) { diff --git a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java index 018721c0d..ed3943a2b 100644 --- a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java +++ b/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 captor = ArgumentCaptor.forClass(String.class); + verify(sender).sendMessage(captor.capture()); + assertThat(captor.getValue(), containsString("don't have permission")); } @Test diff --git a/src/test/java/fr/xephi/authme/command/CommandMapperTest.java b/src/test/java/fr/xephi/authme/command/CommandMapperTest.java index e65c08c68..a7d233050 100644 --- a/src/test/java/fr/xephi/authme/command/CommandMapperTest.java +++ b/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); } // ----------- diff --git a/src/test/java/fr/xephi/authme/command/CommandServiceTest.java b/src/test/java/fr/xephi/authme/command/CommandServiceTest.java index a08e18efd..e3e67e3ed 100644 --- a/src/test/java/fr/xephi/authme/command/CommandServiceTest.java +++ b/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 - PermissionsManager manager = mock(PermissionsManager.class); - given(authMe.getPermissionsManager()).willReturn(manager); - - // when + // given / when PermissionsManager result = commandService.getPermissionsManager(); // then - assertThat(result, equalTo(manager)); - verify(authMe).getPermissionsManager(); + assertThat(result, equalTo(permissionsManager)); } @Test