From b061476743e5d0b8c4c312fbd164d19c8e0e7912 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Lievremont Date: Tue, 23 Jun 2015 10:20:02 +0200 Subject: [PATCH] SONAR-6468 Check old password when changing password on self --- .../org/sonar/server/user/UserUpdater.java | 19 ++++++++- .../server/user/ws/ChangePasswordAction.java | 11 ++++- .../user/ws/ChangePasswordActionTest.java | 41 +++++++++++++++---- .../org/sonar/server/user/ws/UsersWsTest.java | 2 +- 4 files changed, 63 insertions(+), 10 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java index 6e849793bbc..e0e1080911e 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/UserUpdater.java @@ -134,6 +134,19 @@ public class UserUpdater { userIndexer.index(); } + public void checkCurrentPassword(String login, String password) { + DbSession dbSession = dbClient.openSession(false); + try { + UserDto user = dbClient.userDao().selectByLogin(dbSession, login); + String cryptedPassword = encryptPassword(password, user.getSalt()); + if (!cryptedPassword.equals(user.getCryptedPassword())) { + throw new IllegalArgumentException("Incorrect password"); + } + } finally { + dbSession.close(); + } + } + private UserDto createNewUserDto(DbSession dbSession, NewUser newUser) { UserDto userDto = new UserDto(); List messages = newArrayList(); @@ -301,7 +314,11 @@ public class UserUpdater { random.nextBytes(salt); String saltHex = DigestUtils.sha1Hex(salt); userDto.setSalt(saltHex); - userDto.setCryptedPassword(DigestUtils.sha1Hex("--" + saltHex + "--" + password + "--")); + userDto.setCryptedPassword(encryptPassword(password, saltHex)); + } + + private static String encryptPassword(String password, String salt) { + return DigestUtils.sha1Hex("--" + salt + "--" + password + "--"); } private void notifyNewUser(String login, String name, String email) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java b/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java index c59560a786b..0fcb4ee58b2 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/ws/ChangePasswordAction.java @@ -32,6 +32,7 @@ public class ChangePasswordAction implements UsersWsAction { private static final String PARAM_LOGIN = "login"; private static final String PARAM_PASSWORD = "password"; + private static final String PARAM_PREVIOUS_PASSWORD = "previousPassword"; private final UserUpdater userUpdater; private final UserSession userSession; @@ -60,6 +61,11 @@ public class ChangePasswordAction implements UsersWsAction { .setDescription("New password") .setRequired(true) .setExampleValue("mypassword"); + + action.createParam(PARAM_PREVIOUS_PASSWORD) + .setDescription("Previous password. Required when changing one's own password.") + .setRequired(false) + .setExampleValue("oldpassword"); } @Override @@ -67,7 +73,10 @@ public class ChangePasswordAction implements UsersWsAction { userSession.checkLoggedIn(); String login = request.mandatoryParam(PARAM_LOGIN); - if (!login.equals(userSession.getLogin())) { + if (login.equals(userSession.getLogin())) { + String previousPassword = request.mandatoryParam(PARAM_PREVIOUS_PASSWORD); + userUpdater.checkCurrentPassword(login, previousPassword); + } else { userSession.checkGlobalPermission(GlobalPermissions.SYSTEM_ADMIN); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java index 83b484554cb..c0c419bbd95 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/ChangePasswordActionTest.java @@ -32,13 +32,13 @@ import org.sonar.core.permission.GlobalPermissions; import org.sonar.core.persistence.DbSession; import org.sonar.core.persistence.DbTester; import org.sonar.core.user.GroupDto; -import org.sonar.core.user.UserDto; import org.sonar.server.db.DbClient; import org.sonar.server.es.EsTester; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; import org.sonar.server.tester.UserSessionRule; +import org.sonar.server.user.NewUser; import org.sonar.server.user.NewUserNotifier; import org.sonar.server.user.SecurityRealmFactory; import org.sonar.server.user.UserUpdater; @@ -76,6 +76,8 @@ public class ChangePasswordActionTest { DbClient dbClient; + UserUpdater userUpdater; + UserIndexer userIndexer; DbSession session; @@ -98,8 +100,8 @@ public class ChangePasswordActionTest { userIndexer = (UserIndexer) new UserIndexer(dbClient, esTester.client()).setEnabled(true); index = new UserIndex(esTester.client()); - tester = new WsTester( - new UsersWs(new ChangePasswordAction(new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, realmFactory), userSessionRule))); + userUpdater = new UserUpdater(mock(NewUserNotifier.class), settings, dbClient, userIndexer, system2, realmFactory); + tester = new WsTester(new UsersWs(new ChangePasswordAction(userUpdater, userSessionRule))); controller = tester.controller("api/users"); } @@ -152,6 +154,7 @@ public class ChangePasswordActionTest { userSessionRule.login("john"); tester.newPostRequest("api/users", "change_password") .setParam("login", "john") + .setParam("previousPassword", "Valar Dohaeris") .setParam("password", "Valar Morghulis") .execute() .assertNoContent(); @@ -161,6 +164,31 @@ public class ChangePasswordActionTest { assertThat(newPassword).isNotEqualTo(originalPassword); } + @Test(expected = IllegalArgumentException.class) + public void fail_to_update_password_on_self_without_old_password() throws Exception { + createUser(); + session.clearCache(); + + userSessionRule.login("john"); + tester.newPostRequest("api/users", "change_password") + .setParam("login", "john") + .setParam("password", "Valar Morghulis") + .execute(); + } + + @Test(expected = IllegalArgumentException.class) + public void fail_to_update_password_on_self_with_bad_old_password() throws Exception { + createUser(); + session.clearCache(); + + userSessionRule.login("john"); + tester.newPostRequest("api/users", "change_password") + .setParam("login", "john") + .setParam("previousPassword", "I dunno") + .setParam("password", "Valar Morghulis") + .execute(); + } + @Test(expected = BadRequestException.class) public void fail_to_update_password_on_external_auth() throws Exception { createUser(); @@ -174,13 +202,12 @@ public class ChangePasswordActionTest { } private void createUser() { - dbClient.userDao().insert(session, new UserDto() + userUpdater.create(NewUser.create() .setEmail("john@email.com") .setLogin("john") .setName("John") .setScmAccounts(newArrayList("jn")) - .setActive(true)); - session.commit(); - userIndexer.index(); + .setPassword("Valar Dohaeris") + .setPasswordConfirmation("Valar Dohaeris")); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java index fdbe1d595f7..d40dfe87961 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ws/UsersWsTest.java @@ -89,7 +89,7 @@ public class UsersWsTest { WebService.Action action = controller.action("change_password"); assertThat(action).isNotNull(); assertThat(action.isPost()).isTrue(); - assertThat(action.params()).hasSize(2); + assertThat(action.params()).hasSize(3); } @Test -- 2.39.5