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 ee272d12f8df805efed6da93d9e9a67143826063..85ce54b18a5a0f9676dec844f657cdeaa2a7c217 100644 (file)
--- 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 9c4b1ef0cea133a408f80f44ee8c15ac78c8b6a4..62d6b550aa19b5c18fc07df4442458d0ffc120fa 100644 (file)
--- 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;
+  }
+
 }