From: Julien Lancelot Date: Thu, 26 Nov 2020 07:56:50 +0000 (+0100) Subject: SONAR-14175 Prevent using same password as before in api/users/change_password X-Git-Tag: 8.6.0.39681~44 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=344a9c94aaf3478f9c0362b5977fbda84352440a;p=sonarqube.git SONAR-14175 Prevent using same password as before in api/users/change_password * Prevent using same password as before in api/users/change_password * Improve UT - Replace usage of ExpectedException by assertThatThrownBy - Add expected message when exceptions are thrown (help me to detect that some UTs were not covering the correct use case) - Use generated values as much as possible --- diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java index ee272d12f8d..85ce54b18a5 100644 --- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java +++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java @@ -19,6 +19,7 @@ */ package org.sonar.server.user.ws; +import org.sonar.api.server.ws.Change; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.server.ws.WebService; @@ -34,6 +35,7 @@ import org.sonar.server.user.UserSession; import org.sonar.server.user.UserUpdater; import static java.lang.String.format; +import static org.sonarqube.ws.WsUtils.checkArgument; public class ChangePasswordAction implements UsersWsAction { @@ -61,7 +63,8 @@ public class ChangePasswordAction implements UsersWsAction { "Administer System permission is required to change another user's password.") .setSince("5.2") .setPost(true) - .setHandler(this); + .setHandler(this) + .setChangelog(new Change("8.6", "It's no more possible for the password to be the same as the previous one")); action.createParam(PARAM_LOGIN) .setDescription("User login") @@ -85,15 +88,15 @@ public class ChangePasswordAction implements UsersWsAction { try (DbSession dbSession = dbClient.openSession(false)) { String login = request.mandatoryParam(PARAM_LOGIN); + String password = request.mandatoryParam(PARAM_PASSWORD); UserDto user = getUser(dbSession, login); if (login.equals(userSession.getLogin())) { String previousPassword = request.mandatoryParam(PARAM_PREVIOUS_PASSWORD); - checkCurrentPassword(dbSession, user, previousPassword); + checkPreviousPassword(dbSession, user, previousPassword); + checkArgument(!previousPassword.equals(password), "Password must be different from old password"); } else { userSession.checkIsSystemAdministrator(); } - - String password = request.mandatoryParam(PARAM_PASSWORD); UpdateUser updateUser = new UpdateUser().setPassword(password); userUpdater.updateAndCommit(dbSession, user, updateUser, u -> { }); @@ -109,7 +112,7 @@ public class ChangePasswordAction implements UsersWsAction { return user; } - private void checkCurrentPassword(DbSession dbSession, UserDto user, String password) { + private void checkPreviousPassword(DbSession dbSession, UserDto user, String password) { try { localAuthentication.authenticate(dbSession, user, password, AuthenticationEvent.Method.BASIC); } catch (AuthenticationException ex) { diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java index 9c4b1ef0cea..62d6b550aa1 100644 --- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java +++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java @@ -22,12 +22,10 @@ package org.sonar.server.user.ws; import org.junit.Before; import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.sonar.api.config.internal.MapSettings; -import org.sonar.api.impl.utils.AlwaysIncreasingSystem2; import org.sonar.api.server.ws.WebService; -import org.sonar.api.utils.System2; import org.sonar.db.DbTester; +import org.sonar.db.user.UserDto; import org.sonar.server.authentication.CredentialsLocalAuthentication; import org.sonar.server.es.EsTester; import org.sonar.server.exceptions.BadRequestException; @@ -35,27 +33,27 @@ import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.organization.TestDefaultOrganizationProvider; import org.sonar.server.tester.UserSessionRule; -import org.sonar.server.user.NewUser; import org.sonar.server.user.NewUserNotifier; +import org.sonar.server.user.UpdateUser; import org.sonar.server.user.UserUpdater; +import org.sonar.server.user.index.UserIndexDefinition; import org.sonar.server.user.index.UserIndexer; import org.sonar.server.usergroups.DefaultGroupFinder; +import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.TestResponse; import org.sonar.server.ws.WsActionTester; +import static java.lang.String.format; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; -import static org.sonar.db.user.UserTesting.newExternalUser; -import static org.sonar.db.user.UserTesting.newLocalUser; public class ChangePasswordActionTest { - @Rule - public ExpectedException expectedException = ExpectedException.none(); @Rule public DbTester db = DbTester.create(); @Rule - public EsTester es = EsTester.create(); + public EsTester es = EsTester.createCustom(UserIndexDefinition.createForTest()); @Rule public UserSessionRule userSessionRule = UserSessionRule.standalone().logIn(); @@ -78,118 +76,140 @@ public class ChangePasswordActionTest { @Test public void a_user_can_update_his_password() { - userUpdater.createAndCommit(db.getSession(), NewUser.builder() - .setEmail("john@email.com") - .setLogin("john") - .setName("John") - .setPassword("Valar Dohaeris") - .build(), u -> { - }); - String oldCryptedPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), "john").getCryptedPassword(); - userSessionRule.logIn("john"); + String oldPassword = "Valar Dohaeris"; + UserDto user = createLocalUser(oldPassword); + String oldCryptedPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), user.getLogin()).getCryptedPassword(); + userSessionRule.logIn(user); TestResponse response = tester.newRequest() - .setParam("login", "john") + .setParam("login", user.getLogin()) .setParam("previousPassword", "Valar Dohaeris") .setParam("password", "Valar Morghulis") .execute(); assertThat(response.getStatus()).isEqualTo(204); - String newCryptedPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), "john").getCryptedPassword(); + String newCryptedPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), user.getLogin()).getCryptedPassword(); assertThat(newCryptedPassword).isNotEqualTo(oldCryptedPassword); } @Test public void system_administrator_can_update_password_of_user() { - userSessionRule.logIn().setSystemAdministrator(); - createLocalUser(); - String originalPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), "john").getCryptedPassword(); + UserDto admin = createLocalUser(); + userSessionRule.logIn(admin).setSystemAdministrator(); + UserDto user = createLocalUser(); + String originalPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), user.getLogin()).getCryptedPassword(); tester.newRequest() - .setParam("login", "john") + .setParam("login", user.getLogin()) .setParam("password", "Valar Morghulis") .execute(); - String newPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), "john").getCryptedPassword(); + String newPassword = db.getDbClient().userDao().selectByLogin(db.getSession(), user.getLogin()).getCryptedPassword(); assertThat(newPassword).isNotEqualTo(originalPassword); } @Test - public void fail_on_missing_permission() { - createLocalUser(); - userSessionRule.logIn("polop"); + public void fail_to_update_someone_else_password_if_not_admin() { + UserDto user = createLocalUser(); + userSessionRule.logIn(user); + UserDto anotherUser = createLocalUser(); - expectedException.expect(ForbiddenException.class); - tester.newRequest() - .setParam("login", "john") - .execute(); + TestRequest request = tester.newRequest() + .setParam("login", anotherUser.getLogin()) + .setParam("previousPassword", "I dunno") + .setParam("password", "Valar Morghulis"); + + assertThatThrownBy(request::execute) + .isInstanceOf(ForbiddenException.class); } @Test - public void fail_on_unknown_user() { - userSessionRule.logIn().setSystemAdministrator(); - - expectedException.expect(NotFoundException.class); - expectedException.expectMessage("User with login 'polop' has not been found"); + public void fail_to_update_unknown_user() { + UserDto admin = createLocalUser(); + userSessionRule.logIn(admin).setSystemAdministrator(); - tester.newRequest() + TestRequest request = tester.newRequest() .setParam("login", "polop") - .setParam("password", "polop") - .execute(); + .setParam("password", "polop"); + + assertThatThrownBy(request::execute) + .isInstanceOf(NotFoundException.class) + .hasMessage("User with login 'polop' has not been found"); } @Test public void fail_on_disabled_user() { - db.users().insertUser(u -> u.setLogin("polop").setActive(false)); - userSessionRule.logIn().setSystemAdministrator(); + UserDto user = db.users().insertUser(u -> u.setActive(false)); + userSessionRule.logIn(user); - expectedException.expect(NotFoundException.class); - expectedException.expectMessage("User with login 'polop' has not been found"); + TestRequest request = tester.newRequest() + .setParam("login", user.getLogin()) + .setParam("password", "polop"); - tester.newRequest() - .setParam("login", "polop") - .setParam("password", "polop") - .execute(); + assertThatThrownBy(request::execute) + .isInstanceOf(NotFoundException.class) + .hasMessage(format("User with login '%s' has not been found", user.getLogin())); } @Test public void fail_to_update_password_on_self_without_old_password() { - createLocalUser(); - userSessionRule.logIn("john"); + UserDto user = createLocalUser(); + userSessionRule.logIn(user); - expectedException.expect(IllegalArgumentException.class); + TestRequest request = tester.newRequest() + .setParam("login", user.getLogin()) + .setParam("password", "Valar Morghulis"); - tester.newRequest() - .setParam("login", "john") - .setParam("password", "Valar Morghulis") - .execute(); + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("The 'previousPassword' parameter is missing"); } @Test public void fail_to_update_password_on_self_with_bad_old_password() { - createLocalUser(); - userSessionRule.logIn("john"); + UserDto user = createLocalUser(); + userSessionRule.logIn(user); - expectedException.expect(IllegalArgumentException.class); - - tester.newRequest() - .setParam("login", "john") + TestRequest request = tester.newRequest() + .setParam("login", user.getLogin()) .setParam("previousPassword", "I dunno") - .setParam("password", "Valar Morghulis") - .execute(); + .setParam("password", "Valar Morghulis"); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Incorrect password"); } @Test public void fail_to_update_password_on_external_auth() { - userSessionRule.logIn().setSystemAdministrator(); - db.users().insertUser(newExternalUser("john", "John", "john@email.com")); + UserDto admin = db.users().insertUser(); + userSessionRule.logIn(admin).setSystemAdministrator(); + UserDto user = db.users().insertUser(u -> u.setLocal(false)); - expectedException.expect(BadRequestException.class); + TestRequest request = tester.newRequest() + .setParam("login", user.getLogin()) + .setParam("previousPassword", "I dunno") + .setParam("password", "Valar Morghulis"); - tester.newRequest() - .setParam("login", "john") - .setParam("password", "Valar Morghulis") - .execute(); + assertThatThrownBy(request::execute) + .isInstanceOf(BadRequestException.class) + .hasMessage("Password cannot be changed when external authentication is used"); + } + + @Test + public void fail_to_update_to_same_password() { + String oldPassword = "Valar Dohaeris"; + UserDto user = createLocalUser(oldPassword); + userSessionRule.logIn(user); + + TestRequest request = tester.newRequest() + .setParam("login", user.getLogin()) + .setParam("previousPassword", oldPassword) + .setParam("password", oldPassword); + + assertThatThrownBy(request::execute) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Password must be different from old password"); } @Test @@ -200,7 +220,15 @@ public class ChangePasswordActionTest { assertThat(action.params()).hasSize(3); } - private void createLocalUser() { - db.users().insertUser(newLocalUser("john", "John", "john@email.com")); + private UserDto createLocalUser() { + return db.users().insertUser(u -> u.setLocal(true)); } + + private UserDto createLocalUser(String password) { + UserDto user = createLocalUser(); + userUpdater.updateAndCommit(db.getSession(), user, new UpdateUser().setLogin(user.getLogin()).setPassword(password), u -> { + }); + return user; + } + }