diff --git a/gradle/changelog/unified_password_validation.yaml b/gradle/changelog/unified_password_validation.yaml new file mode 100644 index 0000000000..3d039d4626 --- /dev/null +++ b/gradle/changelog/unified_password_validation.yaml @@ -0,0 +1,2 @@ +- type: changed + description: Unify password validation ([#2077](https://github.com/scm-manager/scm-manager/pull/2077)) diff --git a/scm-core/src/main/java/sonia/scm/SCMContext.java b/scm-core/src/main/java/sonia/scm/SCMContext.java index 1a383bc64e..0bb06bca42 100644 --- a/scm-core/src/main/java/sonia/scm/SCMContext.java +++ b/scm-core/src/main/java/sonia/scm/SCMContext.java @@ -26,6 +26,7 @@ package sonia.scm; //~--- non-JDK imports -------------------------------------------------------- +import sonia.scm.security.CipherUtil; import sonia.scm.user.User; import sonia.scm.util.ServiceUtil; @@ -51,7 +52,10 @@ public final class SCMContext public static final User ANONYMOUS = new User( USER_ANONYMOUS, "SCM Anonymous", - "scm-anonymous@scm-manager.org" + "scm-anonymous@scm-manager.org", + CipherUtil.getInstance().encode("__not_necessary_password__"), + "xml", + true ); /** Singleton instance of {@link SCMContextProvider} */ diff --git a/scm-core/src/main/java/sonia/scm/user/User.java b/scm-core/src/main/java/sonia/scm/user/User.java index 6cb86a420b..9ebd1ee51d 100644 --- a/scm-core/src/main/java/sonia/scm/user/User.java +++ b/scm-core/src/main/java/sonia/scm/user/User.java @@ -78,6 +78,7 @@ public class User extends BasicPropertiesAware implements Principal, ModelObject /** * The user type is replaced by {@link #external} flag + * * @deprecated Use {@link #external} instead. */ @Deprecated @@ -216,6 +217,8 @@ public class User extends BasicPropertiesAware implements Principal, ModelObject public boolean isValid() { return ValidationUtil.isNameValid(name) && Util.isNotEmpty(displayName) && ((Util.isEmpty(mail)) || ValidationUtil.isMailAddressValid(mail)); + //TODO Ensure that passwords are encrypted + // && (external || ValidationUtil.isPasswordValid(password)); } @Override diff --git a/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java b/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java index 34779e9ffa..553fd09f96 100644 --- a/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java +++ b/scm-core/src/main/java/sonia/scm/util/ValidationUtil.java @@ -24,6 +24,7 @@ package sonia.scm.util; +import com.google.common.base.Strings; import sonia.scm.Validateable; import java.util.regex.Pattern; @@ -87,6 +88,17 @@ public final class ValidationUtil { return Util.isNotEmpty(name) && name.matches(REGEX_NAME) && !name.equals(".."); } + /** + * Returns {@code true} if the user password is valid. + * + * @param password password to be validated + * @return {@code true} if password is valid + */ + public static boolean isPasswordValid(String password) { + String pw = Strings.nullToEmpty(password); + return pw.length() >= 6 && pw.length() <= 1024; + } + /** * Returns {@code true} if the object is valid. * diff --git a/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java b/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java index cd4aaf5121..2569e62f0c 100644 --- a/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java +++ b/scm-core/src/test/java/sonia/scm/util/ValidationUtilTest.java @@ -155,6 +155,16 @@ class ValidationUtilTest { assertFalse(ValidationUtil.isNameValid(value)); } + @ParameterizedTest + @ValueSource(strings = { + "1", + " ", + "asdf" + }) + void shouldRejectPassword(String value) { + assertFalse(ValidationUtil.isPasswordValid(value)); + } + @Test void testIsNotContaining() { diff --git a/scm-plugins/scm-legacy-plugin/src/test/java/sonia/scm/legacy/LegacyRealmTest.java b/scm-plugins/scm-legacy-plugin/src/test/java/sonia/scm/legacy/LegacyRealmTest.java index 1eacfd12c1..d804db54c9 100644 --- a/scm-plugins/scm-legacy-plugin/src/test/java/sonia/scm/legacy/LegacyRealmTest.java +++ b/scm-plugins/scm-legacy-plugin/src/test/java/sonia/scm/legacy/LegacyRealmTest.java @@ -102,7 +102,9 @@ public class LegacyRealmTest { @Test public void testDoGetAuthenticationInfoWithNullPassword() { - when(userDAO.get("tricia")).thenReturn(UserTestData.createTrillian()); + User trillian = UserTestData.createTrillian(); + trillian.setPassword(null); + when(userDAO.get("tricia")).thenReturn(trillian); AuthenticationToken token = new UsernamePasswordToken("tricia", "secret"); diff --git a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java index 4905bfe4fe..1afc9cc443 100644 --- a/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java +++ b/scm-test/src/main/java/sonia/scm/user/UserManagerTestBase.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.user; //~--- non-JDK imports -------------------------------------------------------- @@ -282,6 +282,7 @@ public abstract class UserManagerTestBase extends ManagerTestBase { String id = UUID.randomUUID().toString(); User user = new User(id, id.concat(" displayName"), id.concat("@mail.com")); + user.setPassword("$shiro1$secret"); manager.create(user); diff --git a/scm-test/src/main/java/sonia/scm/user/UserTestData.java b/scm-test/src/main/java/sonia/scm/user/UserTestData.java index c1559aa269..48d92a2b5d 100644 --- a/scm-test/src/main/java/sonia/scm/user/UserTestData.java +++ b/scm-test/src/main/java/sonia/scm/user/UserTestData.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.user; /** @@ -103,6 +103,7 @@ public final class UserTestData public static User createTrillian() { User user = new User("trillian", "Tricia McMillan", "tricia.mcmillan@hitchhiker.com"); + user.setPassword("$shiro1$trillisSecret"); user.setType("xml"); return user; } @@ -117,6 +118,7 @@ public final class UserTestData { User user = new User("zaphod", "Zaphod Beeblebrox", "zaphod.beeblebrox@hitchhiker.com"); user.setType("xml"); + user.setPassword("$shiro1$zaphodsSecret"); return user; } } diff --git a/scm-ui/ui-components/src/forms/PasswordConfirmation.tsx b/scm-ui/ui-components/src/forms/PasswordConfirmation.tsx index 8933744461..688ce55353 100644 --- a/scm-ui/ui-components/src/forms/PasswordConfirmation.tsx +++ b/scm-ui/ui-components/src/forms/PasswordConfirmation.tsx @@ -50,7 +50,7 @@ const PasswordConfirmation: FC = ({ passwordChanged, passwordValidat return passwordValidator(newPassword); } - return newPassword.length >= 6 && newPassword.length < 32; + return newPassword.length >= 6 && newPassword.length < 1024; }; const handlePasswordValidationChange = (newConfirmedPassword: string) => { @@ -76,7 +76,7 @@ const PasswordConfirmation: FC = ({ passwordChanged, passwordValidat handlePasswordChange(event)} + onChange={(event) => handlePasswordChange(event)} value={password} validationError={!passwordValid} errorMessage={t("password.passwordInvalid")} diff --git a/scm-ui/ui-webapp/public/locales/de/commons.json b/scm-ui/ui-webapp/public/locales/de/commons.json index fb1c740342..113d84aa59 100644 --- a/scm-ui/ui-webapp/public/locales/de/commons.json +++ b/scm-ui/ui-webapp/public/locales/de/commons.json @@ -125,7 +125,7 @@ "currentPassword": "Aktuelles Passwort", "currentPasswordHelpText": "Dieses Passwort wird momentan bereits verwendet.", "confirmPassword": "Passwort wiederholen", - "passwordInvalid": "Das Passwort muss zwischen 6 und 32 Zeichen lang sein!", + "passwordInvalid": "Das Passwort muss zwischen 6 und 1024 Zeichen lang sein!", "passwordConfirmFailed": "Passwörter müssen identisch sein!", "submit": "Speichern", "changedSuccessfully": "Passwort erfolgreich geändert!" diff --git a/scm-ui/ui-webapp/public/locales/en/commons.json b/scm-ui/ui-webapp/public/locales/en/commons.json index 8665937830..c99b757adc 100644 --- a/scm-ui/ui-webapp/public/locales/en/commons.json +++ b/scm-ui/ui-webapp/public/locales/en/commons.json @@ -126,7 +126,7 @@ "currentPassword": "Current Password", "currentPasswordHelpText": "The password currently in use", "confirmPassword": "Confirm Password", - "passwordInvalid": "Password has to be between 6 and 32 characters", + "passwordInvalid": "Password has to be between 6 and 1024 characters", "passwordConfirmFailed": "Passwords have to be identical", "submit": "Submit", "changedSuccessfully": "Password changed successfully" diff --git a/scm-ui/ui-webapp/public/locales/es/commons.json b/scm-ui/ui-webapp/public/locales/es/commons.json index 33c8b70dd5..1d25d7d768 100644 --- a/scm-ui/ui-webapp/public/locales/es/commons.json +++ b/scm-ui/ui-webapp/public/locales/es/commons.json @@ -85,7 +85,7 @@ "currentPassword": "Contraseña actual", "currentPasswordHelpText": "La contraseña ya está en uso", "confirmPassword": "Confirme la contraseña", - "passwordInvalid": "La contraseña debe tener entre 6 y 32 caracteres", + "passwordInvalid": "La contraseña debe tener entre 6 y 1024 caracteres", "passwordConfirmFailed": "Las contraseñas deben ser identicas", "submit": "Guardar", "changedSuccessfully": "Contraseña cambiada correctamente" diff --git a/scm-ui/ui-webapp/src/users/components/userValidation.test.ts b/scm-ui/ui-webapp/src/users/components/userValidation.test.ts index a0ba6dacc3..63267212cc 100644 --- a/scm-ui/ui-webapp/src/users/components/userValidation.test.ts +++ b/scm-ui/ui-webapp/src/users/components/userValidation.test.ts @@ -36,7 +36,7 @@ describe("test displayName validation", () => { "Tricia.McMillan@hitchhiker.com", "Ford Prefect (ford.prefect@hitchhiker.com)", "Zaphod Beeblebrox ", - "Marvin, der depressive Roboter" + "Marvin, der depressive Roboter", ]; for (const name of validNames) { expect(validator.isDisplayNameValid(name)).toBe(true); @@ -47,7 +47,7 @@ describe("test displayName validation", () => { describe("test password validation", () => { it("should return false", () => { // invalid taken from ValidationUtilTest.java - const invalid = ["", "abc", "aaabbbcccdddeeefffggghhhiiijjjkkk"]; + const invalid = ["", "abc"]; for (const password of invalid) { expect(validator.isPasswordValid(password)).toBe(false); } @@ -55,7 +55,7 @@ describe("test password validation", () => { it("should return true", () => { // valid taken from ValidationUtilTest.java - const valid = ["secret123", "mySuperSecretPassword"]; + const valid = ["secret123", "mySuperSecretPassword", "mySuperSecretPasswordWithMoreThan1024Characters"]; for (const password of valid) { expect(validator.isPasswordValid(password)).toBe(true); } diff --git a/scm-ui/ui-webapp/src/users/components/userValidation.ts b/scm-ui/ui-webapp/src/users/components/userValidation.ts index 808f8fe126..95e4bccd64 100644 --- a/scm-ui/ui-webapp/src/users/components/userValidation.ts +++ b/scm-ui/ui-webapp/src/users/components/userValidation.ts @@ -35,5 +35,5 @@ export const isDisplayNameValid = (displayName: string) => { return false; }; export const isPasswordValid = (password: string) => { - return password.length >= 6 && password.length < 32; + return password.length >= 6 && password.length < 1024; }; diff --git a/scm-webapp/src/main/java/sonia/scm/repository/cli/RepositoryTemplateRenderer.java b/scm-webapp/src/main/java/sonia/scm/repository/cli/RepositoryTemplateRenderer.java index 78f6123552..aa6e3cfc38 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/cli/RepositoryTemplateRenderer.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/cli/RepositoryTemplateRenderer.java @@ -71,11 +71,13 @@ class RepositoryTemplateRenderer extends TemplateRenderer { public void renderInvalidInputError() { renderToStderr(INVALID_INPUT_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.USAGE); } public void renderNotFoundError() { renderToStderr(NOT_FOUND_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.NOT_FOUND); } diff --git a/scm-webapp/src/main/java/sonia/scm/user/cli/UserConvertToInternalCommand.java b/scm-webapp/src/main/java/sonia/scm/user/cli/UserConvertToInternalCommand.java index 5491d1c488..71ce75452f 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/cli/UserConvertToInternalCommand.java +++ b/scm-webapp/src/main/java/sonia/scm/user/cli/UserConvertToInternalCommand.java @@ -30,6 +30,7 @@ import picocli.CommandLine; import sonia.scm.cli.ParentCommand; import sonia.scm.user.User; import sonia.scm.user.UserManager; +import sonia.scm.util.ValidationUtil; import javax.inject.Inject; @@ -60,6 +61,9 @@ class UserConvertToInternalCommand implements Runnable { User user = manager.get(username); if (user != null) { + if (!ValidationUtil.isPasswordValid(password)) { + templateRenderer.renderPasswordError(); + } user.setExternal(false); user.setPassword(passwordService.encryptPassword(password)); manager.modify(user); diff --git a/scm-webapp/src/main/java/sonia/scm/user/cli/UserCreateCommand.java b/scm-webapp/src/main/java/sonia/scm/user/cli/UserCreateCommand.java index 59960fd693..e94fa45a28 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/cli/UserCreateCommand.java +++ b/scm-webapp/src/main/java/sonia/scm/user/cli/UserCreateCommand.java @@ -31,6 +31,7 @@ import sonia.scm.cli.CommandValidator; import sonia.scm.cli.ParentCommand; import sonia.scm.user.User; import sonia.scm.user.UserManager; +import sonia.scm.util.ValidationUtil; import javax.inject.Inject; import javax.validation.constraints.Email; @@ -78,6 +79,9 @@ class UserCreateCommand implements Runnable { @Override public void run() { validator.validate(); + if (!external && password != null && !ValidationUtil.isPasswordValid(password)) { + templateRenderer.renderPasswordError(); + } User newUser = new User(); newUser.setName(username); newUser.setDisplayName(displayName); diff --git a/scm-webapp/src/main/java/sonia/scm/user/cli/UserModifyCommand.java b/scm-webapp/src/main/java/sonia/scm/user/cli/UserModifyCommand.java index cd487602eb..afdbcc5ad9 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/cli/UserModifyCommand.java +++ b/scm-webapp/src/main/java/sonia/scm/user/cli/UserModifyCommand.java @@ -31,6 +31,7 @@ import sonia.scm.cli.CommandValidator; import sonia.scm.cli.ParentCommand; import sonia.scm.user.User; import sonia.scm.user.UserManager; +import sonia.scm.util.ValidationUtil; import javax.inject.Inject; import javax.validation.constraints.Email; @@ -70,6 +71,9 @@ class UserModifyCommand implements Runnable { @Override public void run() { validator.validate(); + if (password != null && !ValidationUtil.isPasswordValid(password)) { + templateRenderer.renderPasswordError(); + } User user = manager.get(username); @@ -80,7 +84,7 @@ class UserModifyCommand implements Runnable { if (email != null) { user.setMail(email); } - if (password != null) { + if (!user.isExternal() && password != null) { user.setPassword(passwordService.encryptPassword(password)); } manager.modify(user); diff --git a/scm-webapp/src/main/java/sonia/scm/user/cli/UserTemplateRenderer.java b/scm-webapp/src/main/java/sonia/scm/user/cli/UserTemplateRenderer.java index 66d7a1f42d..bd23f325ea 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/cli/UserTemplateRenderer.java +++ b/scm-webapp/src/main/java/sonia/scm/user/cli/UserTemplateRenderer.java @@ -75,21 +75,25 @@ class UserTemplateRenderer extends TemplateRenderer { public void renderPasswordError() { renderToStderr(PASSWORD_ERROR_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.USAGE); } public void renderExternalActivateError() { renderToStderr(EXTERNAL_ACTIVATE_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.USAGE); } public void renderExternalDeactivateError() { renderToStderr(EXTERNAL_DEACTIVATE_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.USAGE); } public void renderNotFoundError() { renderToStderr(NOT_FOUND_TEMPLATE, Collections.emptyMap()); + context.getStderr().println(); context.exit(ExitCode.NOT_FOUND); } } diff --git a/scm-webapp/src/main/resources/sonia/scm/cli/i18n.properties b/scm-webapp/src/main/resources/sonia/scm/cli/i18n.properties index ab058233d0..a346eacff0 100644 --- a/scm-webapp/src/main/resources/sonia/scm/cli/i18n.properties +++ b/scm-webapp/src/main/resources/sonia/scm/cli/i18n.properties @@ -100,7 +100,7 @@ scm.user.password = Password scm.user.active = Active ### User error -scmUserErrorPassword = Password is required for internal users +scmUserErrorPassword = Valid password is required for internal users scmUserErrorExternalActivate = External users cannot be activated scmUserErrorExternalDeactivate = External users cannot be deactivated scmUserErrorNotFound = Could not find user diff --git a/scm-webapp/src/main/resources/sonia/scm/cli/i18n_de.properties b/scm-webapp/src/main/resources/sonia/scm/cli/i18n_de.properties index 8e9727a825..828ceb5efb 100644 --- a/scm-webapp/src/main/resources/sonia/scm/cli/i18n_de.properties +++ b/scm-webapp/src/main/resources/sonia/scm/cli/i18n_de.properties @@ -110,7 +110,7 @@ scm.user.password = Passwort scm.user.active = Aktiv ### User error -scmUserErrorPassword = Für interne Benutzer muss ein Passwort gesetzt werden +scmUserErrorPassword = Für interne Benutzer muss ein gültiges Passwort gesetzt werden scmUserErrorExternalActivate = Externe Benutzer können nicht aktiviert werden scmUserErrorExternalDeactivate = Externe Benutzer können nicht deaktiviert werden scmUserErrorNotFound= Benutzer konnte nicht gefunden werden diff --git a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java index 68fd09affb..66c4f78f6d 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java @@ -78,7 +78,7 @@ public class DefaultUserManagerTest extends UserManagerTestBase { public void shouldFailChangePasswordForWrongOldPassword() { UserManager userManager = new DefaultUserManager(userDAO); - userManager.changePasswordForLoggedInUser("wrongPassword", "---"); + userManager.changePasswordForLoggedInUser("wrongPassword", "$shiro1$secret"); } @Test diff --git a/scm-webapp/src/test/java/sonia/scm/user/cli/UserConvertToInternalCommandTest.java b/scm-webapp/src/test/java/sonia/scm/user/cli/UserConvertToInternalCommandTest.java index 715ae1fa37..e933d6698c 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/cli/UserConvertToInternalCommandTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/cli/UserConvertToInternalCommandTest.java @@ -97,6 +97,7 @@ class UserConvertToInternalCommandTest { @Test void shouldPrintUserAfterActivationInEnglish() { testRenderer.setLocale("en"); + command.setPassword("havelock123"); command.run(); @@ -116,6 +117,7 @@ class UserConvertToInternalCommandTest { @Test void shouldPrintUserAfterActivationInGerman() { testRenderer.setLocale("de"); + command.setPassword("havelock123"); command.run(); diff --git a/scm-webapp/src/test/java/sonia/scm/user/cli/UserCreateCommandTest.java b/scm-webapp/src/test/java/sonia/scm/user/cli/UserCreateCommandTest.java index f57f273be9..e07aec29d9 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/cli/UserCreateCommandTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/cli/UserCreateCommandTest.java @@ -218,7 +218,7 @@ class UserCreateCommandTest { verifyNoInteractions(manager); assertThat(testRenderer.getStdOut()).isEmpty(); - assertThat(testRenderer.getStdErr()).contains("Password is required for internal users"); + assertThat(testRenderer.getStdErr()).contains("Valid password is required for internal users"); } @Test @@ -229,12 +229,13 @@ class UserCreateCommandTest { verifyNoInteractions(manager); assertThat(testRenderer.getStdOut()).isEmpty(); - assertThat(testRenderer.getStdErr()).contains("Für interne Benutzer muss ein Passwort gesetzt werden"); + assertThat(testRenderer.getStdErr()).contains("Für interne Benutzer muss ein gültiges Passwort gesetzt werden"); } @Test void shouldFailWithEnglishMsgIfExternalUserAndInactive() { testRenderer.setLocale("en"); + command.setPassword("123456"); command.setExternal(true); command.setInactive(true); @@ -248,6 +249,7 @@ class UserCreateCommandTest { @Test void shouldFailWithGermanMsgIfExternalUserWithInactive() { testRenderer.setLocale("de"); + command.setPassword("123456"); command.setExternal(true); command.setInactive(true); diff --git a/scm-webapp/src/test/java/sonia/scm/user/cli/UserModifyCommandTest.java b/scm-webapp/src/test/java/sonia/scm/user/cli/UserModifyCommandTest.java index d4a301d618..1efb7ed72e 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/cli/UserModifyCommandTest.java +++ b/scm-webapp/src/test/java/sonia/scm/user/cli/UserModifyCommandTest.java @@ -158,6 +158,7 @@ class UserModifyCommandTest { @Test void shouldPrintUserAfterModificationInEnglish() { testRenderer.setLocale("en"); + command.setPassword("havelock"); command.setDisplayName("Lord Vetinari"); command.setEmail("patrician@discworld"); @@ -179,6 +180,7 @@ class UserModifyCommandTest { @Test void shouldPrintUserAfterModificationInGerman() { testRenderer.setLocale("de"); + command.setPassword("havelock"); command.setDisplayName("Lord Vetinari"); command.setEmail("patrician@discworld"); @@ -216,6 +218,7 @@ class UserModifyCommandTest { @Test void shouldFailWithEnglishMsgIfUserNotFound() { testRenderer.setLocale("en"); + command.setPassword("havelock"); when(manager.get(any())).thenReturn(null); assertThrows(CliExitException.class, () -> command.run()); @@ -228,6 +231,7 @@ class UserModifyCommandTest { @Test void shouldFailWithGermanMsgIfUserNotFound() { testRenderer.setLocale("de"); + command.setPassword("havelock"); when(manager.get(any())).thenReturn(null); assertThrows(CliExitException.class, () -> command.run());