From 55f7e8097aefbb7bf46aca5302d4384cd9348b4f Mon Sep 17 00:00:00 2001 From: ljacqu Date: Fri, 3 Jun 2016 12:51:49 +0200 Subject: [PATCH] #743 Add proper error message for "invalid chars in password" - Change password validation to return a ValidationResult object for passing message arguments - Remove wrapping methods in ProcessService and CommandService and use ValidationService directly --- .../xephi/authme/command/CommandService.java | 12 ---- .../authme/ChangePasswordAdminCommand.java | 11 ++- .../authme/RegisterAdminCommand.java | 11 ++- .../changepassword/ChangePasswordCommand.java | 11 ++- .../fr/xephi/authme/output/MessageKey.java | 2 + .../handlers/PermissionHandler.java | 2 + .../xephi/authme/process/ProcessService.java | 11 --- .../process/register/AsyncRegister.java | 11 ++- .../xephi/authme/util/ValidationService.java | 67 ++++++++++++++++--- .../authme/command/CommandServiceTest.java | 15 ----- .../ChangePasswordAdminCommandTest.java | 25 +++++-- .../authme/RegisterAdminCommandTest.java | 34 ++++++---- .../ChangePasswordCommandTest.java | 15 +++-- .../authme/process/ProcessServiceTest.java | 15 ----- .../authme/util/ValidationServiceTest.java | 32 +++++---- 15 files changed, 163 insertions(+), 111 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandService.java b/src/main/java/fr/xephi/authme/command/CommandService.java index c479f5477..acf150566 100644 --- a/src/main/java/fr/xephi/authme/command/CommandService.java +++ b/src/main/java/fr/xephi/authme/command/CommandService.java @@ -104,18 +104,6 @@ public class CommandService { return settings; } - - /** - * Verifies whether a password is valid according to the plugin settings. - * - * @param password the password to verify - * @param username the username the password is associated with - * @return message key with the password error, or {@code null} if password is valid - */ - public MessageKey validatePassword(String password, String username) { - return validationService.validatePassword(password, username); - } - public boolean validateEmail(String email) { return validationService.validateEmail(email); } diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java index d22423472..5f35b7df2 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommand.java @@ -10,6 +10,8 @@ import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; import javax.inject.Inject; @@ -32,6 +34,9 @@ public class ChangePasswordAdminCommand implements ExecutableCommand { @Inject private BukkitService bukkitService; + @Inject + private ValidationService validationService; + @Override public void executeCommand(final CommandSender sender, List arguments, final CommandService commandService) { @@ -40,9 +45,9 @@ public class ChangePasswordAdminCommand implements ExecutableCommand { final String playerPass = arguments.get(1); // Validate the password - MessageKey passwordError = commandService.validatePassword(playerPass, playerName); - if (passwordError != null) { - commandService.send(sender, passwordError); + ValidationResult validationResult = validationService.validatePassword(playerPass, playerName); + if (validationResult.hasError()) { + commandService.send(sender, validationResult.getMessageKey(), validationResult.getArgs()); return; } diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommand.java index 1540e4659..292180030 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommand.java @@ -9,6 +9,8 @@ import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; @@ -29,6 +31,9 @@ public class RegisterAdminCommand implements ExecutableCommand { @Inject private BukkitService bukkitService; + @Inject + private ValidationService validationService; + @Override public void executeCommand(final CommandSender sender, List arguments, final CommandService commandService) { @@ -38,9 +43,9 @@ public class RegisterAdminCommand implements ExecutableCommand { final String playerNameLowerCase = playerName.toLowerCase(); // Command logic - MessageKey passwordError = commandService.validatePassword(playerPass, playerName); - if (passwordError != null) { - commandService.send(sender, passwordError); + ValidationResult passwordValidation = validationService.validatePassword(playerPass, playerName); + if (passwordValidation.hasError()) { + commandService.send(sender, passwordValidation.getMessageKey(), passwordValidation.getArgs()); return; } diff --git a/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java b/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java index 01aea8625..8ed7b6374 100644 --- a/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommand.java @@ -8,6 +8,8 @@ import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.task.ChangePasswordTask; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.entity.Player; import javax.inject.Inject; @@ -24,6 +26,9 @@ public class ChangePasswordCommand extends PlayerCommand { @Inject private BukkitService bukkitService; + @Inject + private ValidationService validationService; + @Inject // TODO ljacqu 20160531: Remove this once change password task runs as a process (via Management) private PasswordSecurity passwordSecurity; @@ -40,9 +45,9 @@ public class ChangePasswordCommand extends PlayerCommand { } // Make sure the password is allowed - MessageKey passwordError = commandService.validatePassword(newPassword, name); - if (passwordError != null) { - commandService.send(player, passwordError); + ValidationResult passwordValidation = validationService.validatePassword(newPassword, name); + if (passwordValidation.hasError()) { + commandService.send(player, passwordValidation.getMessageKey(), passwordValidation.getArgs()); return; } diff --git a/src/main/java/fr/xephi/authme/output/MessageKey.java b/src/main/java/fr/xephi/authme/output/MessageKey.java index eb7a52381..9cfa960da 100644 --- a/src/main/java/fr/xephi/authme/output/MessageKey.java +++ b/src/main/java/fr/xephi/authme/output/MessageKey.java @@ -63,6 +63,8 @@ public enum MessageKey { PASSWORD_UNSAFE_ERROR("password_error_unsafe"), + PASSWORD_CHARACTERS_ERROR("password_error_chars", "REG_EX"), + SESSION_EXPIRED("invalid_session"), MUST_REGISTER_MESSAGE("reg_only"), diff --git a/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java b/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java index f222dac27..5d467cea0 100644 --- a/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java +++ b/src/main/java/fr/xephi/authme/permission/handlers/PermissionHandler.java @@ -5,6 +5,8 @@ import fr.xephi.authme.permission.PermissionsSystemType; import org.bukkit.entity.Player; import java.util.List; + + public interface PermissionHandler { /** diff --git a/src/main/java/fr/xephi/authme/process/ProcessService.java b/src/main/java/fr/xephi/authme/process/ProcessService.java index 753573a9e..f171e368e 100644 --- a/src/main/java/fr/xephi/authme/process/ProcessService.java +++ b/src/main/java/fr/xephi/authme/process/ProcessService.java @@ -104,17 +104,6 @@ public class ProcessService { pluginManager.callEvent(event); } - /** - * Verifies whether a password is valid according to the plugin settings. - * - * @param password the password to verify - * @param username the username the password is associated with - * @return message key with the password error, or {@code null} if password is valid - */ - public MessageKey validatePassword(String password, String username) { - return validationService.validatePassword(password, username); - } - public boolean validateEmail(String email) { return validationService.validateEmail(email); } diff --git a/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java b/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java index 2d47b5bad..44f0fd0b4 100644 --- a/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java +++ b/src/main/java/fr/xephi/authme/process/register/AsyncRegister.java @@ -20,6 +20,8 @@ import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.util.StringUtils; import fr.xephi.authme.util.Utils; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.Bukkit; import org.bukkit.entity.Player; @@ -51,6 +53,9 @@ public class AsyncRegister implements AsynchronousProcess { @Inject private PermissionsManager permissionsManager; + @Inject + private ValidationService validationService; + AsyncRegister() { } private boolean preRegisterCheck(Player player, String password) { @@ -65,9 +70,9 @@ public class AsyncRegister implements AsynchronousProcess { //check the password safety only if it's not a automatically generated password if (service.getProperty(SecuritySettings.PASSWORD_HASH) != HashAlgorithm.TWO_FACTOR) { - MessageKey passwordError = service.validatePassword(password, player.getName()); - if (passwordError != null) { - service.send(player, passwordError); + ValidationResult passwordValidation = validationService.validatePassword(password, player.getName()); + if (passwordValidation.hasError()) { + service.send(player, passwordValidation.getMessageKey(), passwordValidation.getArgs()); return false; } } diff --git a/src/main/java/fr/xephi/authme/util/ValidationService.java b/src/main/java/fr/xephi/authme/util/ValidationService.java index 40f8fd1c9..8e72f852a 100644 --- a/src/main/java/fr/xephi/authme/util/ValidationService.java +++ b/src/main/java/fr/xephi/authme/util/ValidationService.java @@ -1,6 +1,7 @@ package fr.xephi.authme.util; import fr.xephi.authme.datasource.DataSource; +import fr.xephi.authme.initialization.Reloadable; import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.permission.PlayerStatePermission; @@ -15,21 +16,29 @@ import org.bukkit.command.CommandSender; import javax.inject.Inject; import java.util.Collection; import java.util.List; +import java.util.regex.Pattern; /** * Validation service. */ -public class ValidationService { +public class ValidationService implements Reloadable { private final NewSetting settings; private final DataSource dataSource; private final PermissionsManager permissionsManager; + private Pattern passwordRegex; @Inject public ValidationService(NewSetting settings, DataSource dataSource, PermissionsManager permissionsManager) { this.settings = settings; this.dataSource = dataSource; this.permissionsManager = permissionsManager; + reload(); + } + + @Override + public void reload() { + passwordRegex = Pattern.compile(settings.getProperty(RestrictionSettings.ALLOWED_PASSWORD_REGEX)); } /** @@ -37,21 +46,21 @@ public class ValidationService { * * @param password the password to verify * @param username the username the password is associated with - * @return message key with the password error, or {@code null} if password is valid + * @return the validation result */ - public MessageKey validatePassword(String password, String username) { + public ValidationResult validatePassword(String password, String username) { String passLow = password.toLowerCase(); - if (!passLow.matches(settings.getProperty(RestrictionSettings.ALLOWED_PASSWORD_REGEX))) { - return MessageKey.PASSWORD_MATCH_ERROR; + if (!passwordRegex.matcher(passLow).matches()) { + return new ValidationResult(MessageKey.PASSWORD_CHARACTERS_ERROR, passwordRegex.pattern()); } else if (passLow.equalsIgnoreCase(username)) { - return MessageKey.PASSWORD_IS_USERNAME_ERROR; + return new ValidationResult(MessageKey.PASSWORD_IS_USERNAME_ERROR); } else if (password.length() < settings.getProperty(SecuritySettings.MIN_PASSWORD_LENGTH) || password.length() > settings.getProperty(SecuritySettings.MAX_PASSWORD_LENGTH)) { - return MessageKey.INVALID_PASSWORD_LENGTH; + return new ValidationResult(MessageKey.INVALID_PASSWORD_LENGTH); } else if (settings.getProperty(SecuritySettings.UNSAFE_PASSWORDS).contains(passLow)) { - return MessageKey.PASSWORD_UNSAFE_ERROR; + return new ValidationResult(MessageKey.PASSWORD_UNSAFE_ERROR); } - return null; + return new ValidationResult(); } /** @@ -130,4 +139,44 @@ public class ValidationService { } return false; } + + public static final class ValidationResult { + private final MessageKey messageKey; + private final String[] args; + + /** + * Constructor for a successful validation. + */ + public ValidationResult() { + this.messageKey = null; + this.args = null; + } + + /** + * Constructor for a failed validation. + * + * @param messageKey message key of the validation error + * @param args arguments for the message key + */ + public ValidationResult(MessageKey messageKey, String... args) { + this.messageKey = messageKey; + this.args = args; + } + + /** + * Returns whether an error was found during the validation, i.e. whether the validation failed. + * + * @return true if there is an error, false if the validation was successful + */ + public boolean hasError() { + return messageKey != null; + } + + public MessageKey getMessageKey() { + return messageKey; + } + public String[] getArgs() { + return args; + } + } } diff --git a/src/test/java/fr/xephi/authme/command/CommandServiceTest.java b/src/test/java/fr/xephi/authme/command/CommandServiceTest.java index c07d44d5d..334fc416f 100644 --- a/src/test/java/fr/xephi/authme/command/CommandServiceTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandServiceTest.java @@ -141,21 +141,6 @@ public class CommandServiceTest { assertThat(result, equalTo(settings)); } - @Test - public void shouldValidatePassword() { - // given - String user = "asdf"; - String password = "mySecret55"; - given(validationService.validatePassword(password, user)).willReturn(MessageKey.INVALID_PASSWORD_LENGTH); - - // when - MessageKey result = commandService.validatePassword(password, user); - - // then - assertThat(result, equalTo(MessageKey.INVALID_PASSWORD_LENGTH)); - verify(validationService).validatePassword(password, user); - } - @Test public void shouldValidateEmail() { // given diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommandTest.java index 64059b2df..5ab9ba0d8 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/ChangePasswordAdminCommandTest.java @@ -9,6 +9,8 @@ import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; import org.junit.BeforeClass; import org.junit.Test; @@ -51,6 +53,9 @@ public class ChangePasswordAdminCommandTest { @Mock private BukkitService bukkitService; + @Mock + private ValidationService validationService; + @BeforeClass public static void setUpLogger() { TestHelper.setupLogger(); @@ -60,14 +65,15 @@ public class ChangePasswordAdminCommandTest { public void shouldRejectInvalidPassword() { // given CommandSender sender = mock(CommandSender.class); - given(service.validatePassword("Bobby", "bobby")).willReturn(MessageKey.PASSWORD_IS_USERNAME_ERROR); + given(validationService.validatePassword("Bobby", "bobby")).willReturn( + new ValidationResult(MessageKey.PASSWORD_IS_USERNAME_ERROR)); // when command.executeCommand(sender, Arrays.asList("bobby", "Bobby"), service); // then - verify(service).validatePassword("Bobby", "bobby"); - verify(service).send(sender, MessageKey.PASSWORD_IS_USERNAME_ERROR); + verify(validationService).validatePassword("Bobby", "bobby"); + verify(service).send(sender, MessageKey.PASSWORD_IS_USERNAME_ERROR, new String[0]); verifyZeroInteractions(dataSource); } @@ -76,11 +82,13 @@ public class ChangePasswordAdminCommandTest { // given CommandSender sender = mock(CommandSender.class); String player = "player"; + String password = "password"; given(playerCache.isAuthenticated(player)).willReturn(false); given(dataSource.getAuth(player)).willReturn(null); + given(validationService.validatePassword(password, player)).willReturn(new ValidationResult()); // when - command.executeCommand(sender, Arrays.asList(player, "password"), service); + command.executeCommand(sender, Arrays.asList(player, password), service); runInnerRunnable(bukkitService); // then @@ -102,13 +110,14 @@ public class ChangePasswordAdminCommandTest { HashedPassword hashedPassword = mock(HashedPassword.class); given(passwordSecurity.computeHash(password, player)).willReturn(hashedPassword); given(dataSource.updatePassword(auth)).willReturn(true); + given(validationService.validatePassword(password, player)).willReturn(new ValidationResult()); // when command.executeCommand(sender, Arrays.asList(player, password), service); runInnerRunnable(bukkitService); // then - verify(service).validatePassword(password, player); + verify(validationService).validatePassword(password, player); verify(service).send(sender, MessageKey.PASSWORD_CHANGED_SUCCESS); verify(passwordSecurity).computeHash(password, player); verify(auth).setPassword(hashedPassword); @@ -126,6 +135,7 @@ public class ChangePasswordAdminCommandTest { given(dataSource.isAuthAvailable(player)).willReturn(true); given(dataSource.getAuth(player)).willReturn(auth); given(dataSource.updatePassword(auth)).willReturn(true); + given(validationService.validatePassword(password, player)).willReturn(new ValidationResult()); HashedPassword hashedPassword = mock(HashedPassword.class); given(passwordSecurity.computeHash(password, player)).willReturn(hashedPassword); @@ -135,7 +145,7 @@ public class ChangePasswordAdminCommandTest { runInnerRunnable(bukkitService); // then - verify(service).validatePassword(password, player); + verify(validationService).validatePassword(password, player); verify(service).send(sender, MessageKey.PASSWORD_CHANGED_SUCCESS); verify(passwordSecurity).computeHash(password, player); verify(auth).setPassword(hashedPassword); @@ -151,6 +161,7 @@ public class ChangePasswordAdminCommandTest { PlayerAuth auth = mock(PlayerAuth.class); given(playerCache.isAuthenticated(player)).willReturn(true); given(playerCache.getAuth(player)).willReturn(auth); + given(validationService.validatePassword(password, player)).willReturn(new ValidationResult()); HashedPassword hashedPassword = mock(HashedPassword.class); given(passwordSecurity.computeHash(password, player)).willReturn(hashedPassword); @@ -161,7 +172,7 @@ public class ChangePasswordAdminCommandTest { runInnerRunnable(bukkitService); // then - verify(service).validatePassword(password, player); + verify(validationService).validatePassword(password, player); verify(service).send(sender, MessageKey.ERROR); verify(passwordSecurity).computeHash(password, player); verify(auth).setPassword(hashedPassword); diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java index 2459bc89c..c82b7e0c2 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java @@ -8,6 +8,8 @@ import fr.xephi.authme.output.MessageKey; import fr.xephi.authme.security.PasswordSecurity; import fr.xephi.authme.security.crypts.HashedPassword; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; import org.junit.BeforeClass; @@ -49,10 +51,10 @@ public class RegisterAdminCommandTest { private BukkitService bukkitService; @Mock - private CommandSender sender; + private CommandService commandService; @Mock - private CommandService commandService; + private ValidationService validationService; @BeforeClass public static void setUpLogger() { @@ -64,14 +66,16 @@ public class RegisterAdminCommandTest { // given String user = "tester"; String password = "myPassword"; - given(commandService.validatePassword(password, user)).willReturn(MessageKey.INVALID_PASSWORD_LENGTH); + given(validationService.validatePassword(password, user)) + .willReturn(new ValidationResult(MessageKey.INVALID_PASSWORD_LENGTH)); + CommandSender sender = mock(CommandSender.class); // when command.executeCommand(sender, Arrays.asList(user, password), commandService); // then - verify(commandService).validatePassword(password, user); - verify(commandService).send(sender, MessageKey.INVALID_PASSWORD_LENGTH); + verify(validationService).validatePassword(password, user); + verify(commandService).send(sender, MessageKey.INVALID_PASSWORD_LENGTH, new String[0]); verify(bukkitService, never()).runTaskAsynchronously(any(Runnable.class)); } @@ -80,15 +84,16 @@ public class RegisterAdminCommandTest { // given String user = "my_name55"; String password = "@some-pass@"; - given(commandService.validatePassword(password, user)).willReturn(null); + given(validationService.validatePassword(password, user)).willReturn(new ValidationResult()); given(dataSource.isAuthAvailable(user)).willReturn(true); + CommandSender sender = mock(CommandSender.class); // when command.executeCommand(sender, Arrays.asList(user, password), commandService); TestHelper.runInnerRunnable(bukkitService); // then - verify(commandService).validatePassword(password, user); + verify(validationService).validatePassword(password, user); verify(commandService).send(sender, MessageKey.NAME_ALREADY_REGISTERED); verify(dataSource, never()).saveAuth(any(PlayerAuth.class)); } @@ -98,18 +103,19 @@ public class RegisterAdminCommandTest { // given String user = "test-test"; String password = "afdjhfkt"; - given(commandService.validatePassword(password, user)).willReturn(null); + given(validationService.validatePassword(password, user)).willReturn(new ValidationResult()); given(dataSource.isAuthAvailable(user)).willReturn(false); given(dataSource.saveAuth(any(PlayerAuth.class))).willReturn(false); HashedPassword hashedPassword = new HashedPassword("235sdf4w5udsgf"); given(passwordSecurity.computeHash(password, user)).willReturn(hashedPassword); + CommandSender sender = mock(CommandSender.class); // when command.executeCommand(sender, Arrays.asList(user, password), commandService); TestHelper.runInnerRunnable(bukkitService); // then - verify(commandService).validatePassword(password, user); + verify(validationService).validatePassword(password, user); verify(commandService).send(sender, MessageKey.ERROR); ArgumentCaptor captor = ArgumentCaptor.forClass(PlayerAuth.class); verify(dataSource).saveAuth(captor.capture()); @@ -121,19 +127,20 @@ public class RegisterAdminCommandTest { // given String user = "someone"; String password = "Al1O3P49S5%"; - given(commandService.validatePassword(password, user)).willReturn(null); + given(validationService.validatePassword(password, user)).willReturn(new ValidationResult()); given(dataSource.isAuthAvailable(user)).willReturn(false); given(dataSource.saveAuth(any(PlayerAuth.class))).willReturn(true); HashedPassword hashedPassword = new HashedPassword("$aea2345EW235dfsa@#R%987048"); given(passwordSecurity.computeHash(password, user)).willReturn(hashedPassword); given(bukkitService.getPlayerExact(user)).willReturn(null); + CommandSender sender = mock(CommandSender.class); // when command.executeCommand(sender, Arrays.asList(user, password), commandService); TestHelper.runInnerRunnable(bukkitService); // then - verify(commandService).validatePassword(password, user); + verify(validationService).validatePassword(password, user); verify(commandService).send(sender, MessageKey.REGISTER_SUCCESS); ArgumentCaptor captor = ArgumentCaptor.forClass(PlayerAuth.class); verify(dataSource).saveAuth(captor.capture()); @@ -146,13 +153,14 @@ public class RegisterAdminCommandTest { // given String user = "someone"; String password = "Al1O3P49S5%"; - given(commandService.validatePassword(password, user)).willReturn(null); + given(validationService.validatePassword(password, user)).willReturn(new ValidationResult()); given(dataSource.isAuthAvailable(user)).willReturn(false); given(dataSource.saveAuth(any(PlayerAuth.class))).willReturn(true); HashedPassword hashedPassword = new HashedPassword("$aea2345EW235dfsa@#R%987048"); given(passwordSecurity.computeHash(password, user)).willReturn(hashedPassword); Player player = mock(Player.class); given(bukkitService.getPlayerExact(user)).willReturn(player); + CommandSender sender = mock(CommandSender.class); // when command.executeCommand(sender, Arrays.asList(user, password), commandService); @@ -160,7 +168,7 @@ public class RegisterAdminCommandTest { runSyncDelayedTask(bukkitService); // then - verify(commandService).validatePassword(password, user); + verify(validationService).validatePassword(password, user); verify(commandService).send(sender, MessageKey.REGISTER_SUCCESS); ArgumentCaptor captor = ArgumentCaptor.forClass(PlayerAuth.class); verify(dataSource).saveAuth(captor.capture()); diff --git a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java index 289af7c6b..57b010067 100644 --- a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java @@ -8,6 +8,8 @@ import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; import fr.xephi.authme.task.ChangePasswordTask; import fr.xephi.authme.util.BukkitService; +import fr.xephi.authme.util.ValidationService; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.BlockCommandSender; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; @@ -53,6 +55,9 @@ public class ChangePasswordCommandTest { @Mock private BukkitService bukkitService; + @Mock + private ValidationService validationService; + @Before public void setSettings() { when(commandService.getProperty(SecuritySettings.MIN_PASSWORD_LENGTH)).thenReturn(2); @@ -91,26 +96,28 @@ public class ChangePasswordCommandTest { // given CommandSender sender = initPlayerWithName("abc12", true); String password = "newPW"; - given(commandService.validatePassword(password, "abc12")).willReturn(MessageKey.INVALID_PASSWORD_LENGTH); + given(validationService.validatePassword(password, "abc12")) + .willReturn(new ValidationResult(MessageKey.INVALID_PASSWORD_LENGTH)); // when command.executeCommand(sender, Arrays.asList("tester", password), commandService); // then - verify(commandService).validatePassword(password, "abc12"); - verify(commandService).send(sender, MessageKey.INVALID_PASSWORD_LENGTH); + verify(validationService).validatePassword(password, "abc12"); + verify(commandService).send(sender, MessageKey.INVALID_PASSWORD_LENGTH, new String[0]); } @Test public void shouldForwardTheDataForValidPassword() { // given CommandSender sender = initPlayerWithName("parker", true); + given(validationService.validatePassword("abc123", "parker")).willReturn(new ValidationResult()); // when command.executeCommand(sender, Arrays.asList("abc123", "abc123"), commandService); // then - verify(commandService).validatePassword("abc123", "parker"); + verify(validationService).validatePassword("abc123", "parker"); verify(commandService, never()).send(eq(sender), any(MessageKey.class)); ArgumentCaptor taskCaptor = ArgumentCaptor.forClass(ChangePasswordTask.class); verify(bukkitService).runTaskAsynchronously(taskCaptor.capture()); diff --git a/src/test/java/fr/xephi/authme/process/ProcessServiceTest.java b/src/test/java/fr/xephi/authme/process/ProcessServiceTest.java index cff6a6c9f..164494a9c 100644 --- a/src/test/java/fr/xephi/authme/process/ProcessServiceTest.java +++ b/src/test/java/fr/xephi/authme/process/ProcessServiceTest.java @@ -127,21 +127,6 @@ public class ProcessServiceTest { verify(messages).retrieveSingle(key); } - @Test - public void shouldValidatePassword() { - // given - String user = "test-user"; - String password = "passw0rd"; - given(validationService.validatePassword(password, user)).willReturn(MessageKey.PASSWORD_MATCH_ERROR); - - // when - MessageKey result = processService.validatePassword(password, user); - - // then - assertThat(result, equalTo(MessageKey.PASSWORD_MATCH_ERROR)); - verify(validationService).validatePassword(password, user); - } - @Test public void shouldValidateEmail() { // given diff --git a/src/test/java/fr/xephi/authme/util/ValidationServiceTest.java b/src/test/java/fr/xephi/authme/util/ValidationServiceTest.java index ed784510f..0d52f7758 100644 --- a/src/test/java/fr/xephi/authme/util/ValidationServiceTest.java +++ b/src/test/java/fr/xephi/authme/util/ValidationServiceTest.java @@ -9,6 +9,7 @@ import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.EmailSettings; import fr.xephi.authme.settings.properties.RestrictionSettings; import fr.xephi.authme.settings.properties.SecuritySettings; +import fr.xephi.authme.util.ValidationService.ValidationResult; import org.bukkit.command.CommandSender; import org.junit.Before; import org.junit.Test; @@ -20,7 +21,6 @@ import java.util.Arrays; import java.util.Collections; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -53,56 +53,56 @@ public class ValidationServiceTest { @Test public void shouldRejectPasswordSameAsUsername() { // given/when - MessageKey error = validationService.validatePassword("bobby", "Bobby"); + ValidationResult error = validationService.validatePassword("bobby", "Bobby"); // then - assertThat(error, equalTo(MessageKey.PASSWORD_IS_USERNAME_ERROR)); + assertErrorEquals(error, MessageKey.PASSWORD_IS_USERNAME_ERROR); } @Test public void shouldRejectPasswordNotMatchingPattern() { // given/when // service mock returns pattern a-zA-Z -> numbers should not be accepted - MessageKey error = validationService.validatePassword("invalid1234", "myPlayer"); + ValidationResult error = validationService.validatePassword("invalid1234", "myPlayer"); // then - assertThat(error, equalTo(MessageKey.PASSWORD_MATCH_ERROR)); + assertErrorEquals(error, MessageKey.PASSWORD_CHARACTERS_ERROR, "[a-zA-Z]+"); } @Test public void shouldRejectTooShortPassword() { // given/when - MessageKey error = validationService.validatePassword("ab", "tester"); + ValidationResult error = validationService.validatePassword("ab", "tester"); // then - assertThat(error, equalTo(MessageKey.INVALID_PASSWORD_LENGTH)); + assertErrorEquals(error, MessageKey.INVALID_PASSWORD_LENGTH); } @Test public void shouldRejectTooLongPassword() { // given/when - MessageKey error = validationService.validatePassword(Strings.repeat("a", 30), "player"); + ValidationResult error = validationService.validatePassword(Strings.repeat("a", 30), "player"); // then - assertThat(error, equalTo(MessageKey.INVALID_PASSWORD_LENGTH)); + assertErrorEquals(error, MessageKey.INVALID_PASSWORD_LENGTH); } @Test public void shouldRejectUnsafePassword() { // given/when - MessageKey error = validationService.validatePassword("unsafe", "playertest"); + ValidationResult error = validationService.validatePassword("unsafe", "playertest"); // then - assertThat(error, equalTo(MessageKey.PASSWORD_UNSAFE_ERROR)); + assertErrorEquals(error, MessageKey.PASSWORD_UNSAFE_ERROR); } @Test public void shouldAcceptValidPassword() { // given/when - MessageKey error = validationService.validatePassword("safePass", "some_user"); + ValidationResult error = validationService.validatePassword("safePass", "some_user"); // then - assertThat(error, nullValue()); + assertThat(error.hasError(), equalTo(false)); } @Test @@ -230,4 +230,10 @@ public class ValidationServiceTest { // then assertThat(result, equalTo(true)); } + + private static void assertErrorEquals(ValidationResult validationResult, MessageKey messageKey, String... args) { + assertThat(validationResult.hasError(), equalTo(true)); + assertThat(validationResult.getMessageKey(), equalTo(messageKey)); + assertThat(validationResult.getArgs(), equalTo(args)); + } }