diff --git a/gradle/changelog/encrypt_cli_passwords.yaml b/gradle/changelog/encrypt_cli_passwords.yaml new file mode 100644 index 0000000000..c2215850a8 --- /dev/null +++ b/gradle/changelog/encrypt_cli_passwords.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Encrypt passwords stored via cli commands ([#2080](https://github.com/scm-manager/scm-manager/pull/2080)) 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 90158a3616..5491d1c488 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 @@ -25,6 +25,7 @@ package sonia.scm.user.cli; import com.google.common.annotations.VisibleForTesting; +import org.apache.shiro.authc.credential.PasswordService; import picocli.CommandLine; import sonia.scm.cli.ParentCommand; import sonia.scm.user.User; @@ -39,6 +40,7 @@ class UserConvertToInternalCommand implements Runnable { @CommandLine.Mixin private final UserTemplateRenderer templateRenderer; private final UserManager manager; + private final PasswordService passwordService; @CommandLine.Parameters(index = "0", paramLabel = "", descriptionKey = "scm.user.username") private String username; @@ -47,9 +49,10 @@ class UserConvertToInternalCommand implements Runnable { private String password; @Inject - UserConvertToInternalCommand(UserTemplateRenderer templateRenderer, UserManager manager) { + UserConvertToInternalCommand(UserTemplateRenderer templateRenderer, UserManager manager, PasswordService passwordService) { this.templateRenderer = templateRenderer; this.manager = manager; + this.passwordService = passwordService; } @Override @@ -58,7 +61,7 @@ class UserConvertToInternalCommand implements Runnable { if (user != null) { user.setExternal(false); - user.setPassword(password); + user.setPassword(passwordService.encryptPassword(password)); manager.modify(user); templateRenderer.render(user); } else { 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 bdbd20a2a2..59960fd693 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 @@ -25,6 +25,7 @@ package sonia.scm.user.cli; import com.google.common.annotations.VisibleForTesting; +import org.apache.shiro.authc.credential.PasswordService; import picocli.CommandLine; import sonia.scm.cli.CommandValidator; import sonia.scm.cli.ParentCommand; @@ -43,6 +44,7 @@ class UserCreateCommand implements Runnable { @CommandLine.Mixin private final CommandValidator validator; private final UserManager manager; + private final PasswordService passwordService; @CommandLine.Parameters(index = "0", paramLabel = "", descriptionKey = "scm.user.username") private String username; @@ -66,10 +68,11 @@ class UserCreateCommand implements Runnable { @Inject public UserCreateCommand(UserTemplateRenderer templateRenderer, CommandValidator validator, - UserManager manager) { + UserManager manager, PasswordService passwordService) { this.templateRenderer = templateRenderer; this.validator = validator; this.manager = manager; + this.passwordService = passwordService; } @Override @@ -84,7 +87,7 @@ class UserCreateCommand implements Runnable { if (password == null) { templateRenderer.renderPasswordError(); } - newUser.setPassword(password); + newUser.setPassword(passwordService.encryptPassword(password)); newUser.setActive(!inactive); } else { if (inactive) { 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 cf4c617bac..cd487602eb 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 @@ -25,6 +25,7 @@ package sonia.scm.user.cli; import com.google.common.annotations.VisibleForTesting; +import org.apache.shiro.authc.credential.PasswordService; import picocli.CommandLine; import sonia.scm.cli.CommandValidator; import sonia.scm.cli.ParentCommand; @@ -43,6 +44,7 @@ class UserModifyCommand implements Runnable { @CommandLine.Mixin private final CommandValidator validator; private final UserManager manager; + private final PasswordService passwordService; @CommandLine.Parameters(index = "0", paramLabel = "", descriptionKey = "scm.user.username") private String username; @@ -58,10 +60,11 @@ class UserModifyCommand implements Runnable { private String password; @Inject - UserModifyCommand(UserTemplateRenderer templateRenderer, CommandValidator validator, UserManager manager) { + UserModifyCommand(UserTemplateRenderer templateRenderer, CommandValidator validator, UserManager manager, PasswordService passwordService) { this.templateRenderer = templateRenderer; this.validator = validator; this.manager = manager; + this.passwordService = passwordService; } @Override @@ -78,7 +81,7 @@ class UserModifyCommand implements Runnable { user.setMail(email); } if (password != null) { - user.setPassword(password); + user.setPassword(passwordService.encryptPassword(password)); } manager.modify(user); templateRenderer.render(user); 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 f7309754ba..715ae1fa37 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 @@ -24,6 +24,7 @@ package sonia.scm.user.cli; +import org.apache.shiro.authc.credential.PasswordService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -49,12 +50,14 @@ class UserConvertToInternalCommandTest { @Mock private UserManager manager; + @Mock + private PasswordService passwordService; private UserConvertToInternalCommand command; @BeforeEach void initCommand() { - command = new UserConvertToInternalCommand(testRenderer.getTemplateRenderer(), manager); + command = new UserConvertToInternalCommand(testRenderer.getTemplateRenderer(), manager, passwordService); } @Nested @@ -73,7 +76,10 @@ class UserConvertToInternalCommandTest { @Test void shouldActivateInternalUser() { - command.setPassword("havelock123"); + String password = "havelock123"; + String enc_password = "enc_havelock123"; + when(passwordService.encryptPassword(password)).thenReturn(enc_password); + command.setPassword(password); command.run(); @@ -81,7 +87,7 @@ class UserConvertToInternalCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Havelock Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("havelock123"); + assertThat(argument.getPassword()).isEqualTo(enc_password); assertThat(argument.getMail()).isEqualTo("havelock.vetinari@discworld"); assertThat(argument.isActive()).isTrue(); return true; 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 42ac25b8f2..f57f273be9 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 @@ -24,6 +24,7 @@ package sonia.scm.user.cli; +import org.apache.shiro.authc.credential.PasswordService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -40,6 +41,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -53,16 +55,20 @@ class UserCreateCommandTest { private CommandValidator validator; @Mock private UserManager manager; + @Mock + private PasswordService passwordService; private UserCreateCommand command; @BeforeEach void initCommand() { - command = new UserCreateCommand(testRenderer.getTemplateRenderer(), validator, manager); + command = new UserCreateCommand(testRenderer.getTemplateRenderer(), validator, manager, passwordService); } @Nested class ForSuccessfulCreationTest { + private static final String PASSWORD = "patrician"; + private static final String ENC_PASSWORD = "enc_patrician"; @BeforeEach void mockCreation() { @@ -76,6 +82,7 @@ class UserCreateCommandTest { command.setUsername("havelock"); command.setDisplayName("Havelock Vetinari"); command.setEmail("havelock.vetinari@discworld"); + lenient().when(passwordService.encryptPassword(PASSWORD)).thenReturn(ENC_PASSWORD); } @Test @@ -88,7 +95,7 @@ class UserCreateCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Havelock Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("patrician"); + assertThat(argument.getPassword()).isEqualTo(ENC_PASSWORD); assertThat(argument.getMail()).isEqualTo("havelock.vetinari@discworld"); assertThat(argument.isActive()).isTrue(); return true; @@ -114,7 +121,7 @@ class UserCreateCommandTest { @Test void shouldCreateInactiveUser() { - command.setPassword("patrician"); + command.setPassword(PASSWORD); command.setInactive(true); command.run(); @@ -123,7 +130,7 @@ class UserCreateCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Havelock Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("patrician"); + assertThat(argument.getPassword()).isEqualTo(ENC_PASSWORD); assertThat(argument.getMail()).isEqualTo("havelock.vetinari@discworld"); assertThat(argument.isActive()).isFalse(); return true; @@ -133,7 +140,7 @@ class UserCreateCommandTest { @Test void shouldPrintUserAfterCreationInEnglish() { testRenderer.setLocale("en"); - command.setPassword("patrician"); + command.setPassword(PASSWORD); command.run(); @@ -153,7 +160,7 @@ class UserCreateCommandTest { @Test void shouldPrintUserAfterCreationInGerman() { testRenderer.setLocale("de"); - command.setPassword("patrician"); + command.setPassword(PASSWORD); command.run(); @@ -169,6 +176,23 @@ class UserCreateCommandTest { ); assertThat(testRenderer.getStdErr()).isEmpty(); } + + @Test + void shouldEncryptPassword() { + command.setPassword(PASSWORD); + + command.run(); + + verify(manager).create(argThat(argument -> { + assertThat(argument.getName()).isEqualTo("havelock"); + assertThat(argument.getDisplayName()).isEqualTo("Havelock Vetinari"); + assertThat(argument.isExternal()).isFalse(); + assertThat(argument.getPassword()).isEqualTo(ENC_PASSWORD); + assertThat(argument.getMail()).isEqualTo("havelock.vetinari@discworld"); + assertThat(argument.isActive()).isTrue(); + return true; + })); + } } @Nested 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 39eb883c7a..d4a301d618 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 @@ -24,6 +24,7 @@ package sonia.scm.user.cli; +import org.apache.shiro.authc.credential.PasswordService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -40,6 +41,7 @@ import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -53,16 +55,20 @@ class UserModifyCommandTest { private CommandValidator validator; @Mock private UserManager manager; + @Mock + private PasswordService passwordService; private UserModifyCommand command; @BeforeEach void initCommand() { - command = new UserModifyCommand(testRenderer.getTemplateRenderer(), validator, manager); + command = new UserModifyCommand(testRenderer.getTemplateRenderer(), validator, manager, passwordService); } @Nested class ForSuccessfulModificationTest { + private static final String NEW_PASSWORD = "havelock"; + private static final String NEW_ENC_PASSWORD = "enc_havelock"; @BeforeEach void mockGet() { @@ -73,13 +79,14 @@ class UserModifyCommandTest { user.setCreationDate(1649262000000L); user.setLastModified(1649272000000L); when(manager.get(any())).thenReturn(user); + lenient().when(passwordService.encryptPassword(NEW_PASSWORD)).thenReturn(NEW_ENC_PASSWORD); } @Test void shouldModifyUser() { command.setDisplayName("Lord Vetinari"); command.setEmail("patrician@discworld"); - command.setPassword("havelock"); + command.setPassword(NEW_PASSWORD); command.run(); @@ -87,7 +94,7 @@ class UserModifyCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Lord Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("havelock"); + assertThat(argument.getPassword()).isEqualTo(NEW_ENC_PASSWORD); assertThat(argument.getMail()).isEqualTo("patrician@discworld"); assertThat(argument.isActive()).isTrue(); return true; @@ -97,7 +104,7 @@ class UserModifyCommandTest { @Test void shouldNotModifyDisplayNameIfNotSet() { command.setEmail("patrician@discworld"); - command.setPassword("havelock"); + command.setPassword(NEW_PASSWORD); command.run(); @@ -105,7 +112,7 @@ class UserModifyCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Havelock Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("havelock"); + assertThat(argument.getPassword()).isEqualTo(NEW_ENC_PASSWORD); assertThat(argument.getMail()).isEqualTo("patrician@discworld"); assertThat(argument.isActive()).isTrue(); return true; @@ -115,7 +122,7 @@ class UserModifyCommandTest { @Test void shouldNotModifyEmailIfNotSet() { command.setDisplayName("Lord Vetinari"); - command.setPassword("havelock"); + command.setPassword(NEW_PASSWORD); command.run(); @@ -123,7 +130,7 @@ class UserModifyCommandTest { assertThat(argument.getName()).isEqualTo("havelock"); assertThat(argument.getDisplayName()).isEqualTo("Lord Vetinari"); assertThat(argument.isExternal()).isFalse(); - assertThat(argument.getPassword()).isEqualTo("havelock"); + assertThat(argument.getPassword()).isEqualTo(NEW_ENC_PASSWORD); assertThat(argument.getMail()).isEqualTo("havelock.vetinari@discworld"); assertThat(argument.isActive()).isTrue(); return true;