SONAR-17467 fix SSF-149

diff --git a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/AddUserAction.java b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
index 04e23f42c99b6b7e6a236e8c4868a763004008e1..1ce2d17101987cb410de18919579b55e85fb6a45 100644 (file)
--- a/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
+++ b/server/sonar-webserver-webapi/src/main/java/org/sonar/server/permission/ws/AddUserAction.java
@@ -54,7 +54,7 @@ public class AddUserAction implements PermissionsWsAction {
   private final Configuration configuration;
 
   public AddUserAction(DbClient dbClient, UserSession userSession, PermissionUpdater permissionUpdater, PermissionWsSupport wsSupport,
-                       WsParameters wsParameters, PermissionService permissionService, Configuration configuration) {
+    WsParameters wsParameters, PermissionService permissionService, Configuration configuration) {
     this.dbClient = dbClient;
     this.userSession = userSession;
     this.permissionUpdater = permissionUpdater;
@@ -86,9 +86,10 @@ public class AddUserAction implements PermissionsWsAction {
   @Override
   public void handle(Request request, Response response) throws Exception {
     try (DbSession dbSession = dbClient.openSession(false)) {
-      UserId user = wsSupport.findUser(dbSession, request.mandatoryParam(PARAM_USER_LOGIN));
+      String userLogin = request.mandatoryParam(PARAM_USER_LOGIN);
       Optional<ComponentDto> project = wsSupport.findProject(dbSession, request);
       checkProjectAdmin(userSession, configuration, project.orElse(null));
+      UserId user = wsSupport.findUser(dbSession, userLogin);
 
       PermissionChange change = new UserPermissionChange(
         PermissionChange.Operation.ADD,

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 b28a6d53d11e0137e924753776ec214feb27a386..aeee16cca695993682f0a635e94351edd5caf0da 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
@@ -47,6 +47,7 @@ import static java.lang.String.format;
 import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
 import static java.net.HttpURLConnection.HTTP_NO_CONTENT;
 import static org.sonarqube.ws.WsUtils.checkArgument;
+import static org.sonarqube.ws.WsUtils.isNullOrEmpty;
 import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_LOGIN;
 import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_PASSWORD;
 import static org.sonarqube.ws.client.user.UsersWsParameters.PARAM_PREVIOUS_PASSWORD;
@@ -110,28 +111,29 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc
     try (DbSession dbSession = dbClient.openSession(false)) {
       String login = getParamOrThrow(request, PARAM_LOGIN);
       String newPassword = getParamOrThrow(request, PARAM_PASSWORD);
-      UserDto user = getUserOrThrow(dbSession, login);
+      UserDto user;
+
       if (login.equals(userSession.getLogin())) {
+        user = getUserOrThrow(dbSession, login);
         String previousPassword = getParamOrThrow(request, PARAM_PREVIOUS_PASSWORD);
         checkPreviousPassword(dbSession, user, previousPassword);
         checkArgument(!previousPassword.equals(newPassword), "Password must be different from old password");
         deleteTokensAndRefreshSession(request, response, dbSession, user);
       } else {
         userSession.checkIsSystemAdministrator();
+        user = getUserOrThrow(dbSession, login);
         dbClient.sessionTokensDao().deleteByUser(dbSession, user);
       }
-      UpdateUser updateUser = new UpdateUser().setPassword(newPassword);
-      userUpdater.updateAndCommit(dbSession, user, updateUser, u -> {});
+      updatePassword(dbSession, user, newPassword);
       setResponseStatus(response, HTTP_NO_CONTENT);
     } catch (BadRequestException badRequestException) {
       setResponseStatus(response, HTTP_BAD_REQUEST);
     }
   }
 
-
   private static String getParamOrThrow(ServletRequest request, String key) {
     String value = request.getParameter(key);
-    checkArgument(value != null && !value.isEmpty(), MSG_PARAMETER_MISSING, key);
+    checkArgument(!isNullOrEmpty(value), MSG_PARAMETER_MISSING, key);
     return value;
   }
 
@@ -163,6 +165,12 @@ public class ChangePasswordAction extends ServletFilter implements BaseUsersWsAc
     jwtHttpHandler.generateToken(user, httpRequest, httpResponse);
   }
 
+  private void updatePassword(DbSession dbSession, UserDto user, String newPassword) {
+    UpdateUser updateUser = new UpdateUser().setPassword(newPassword);
+    userUpdater.updateAndCommit(dbSession, user, updateUser, u -> {
+    });
+  }
+
   private static void setResponseStatus(ServletResponse response, int newStatusCode) {
     ((HttpServletResponse) response).setStatus(newStatusCode);
   }

diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java
index 589c75cd621cb35fca5b0c1ecf7d9ead70f1d7dd..fb36fb3eb4caa29f55f05fdfc19b5d39da735d90 100644 (file)
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/permission/ws/AddUserActionTest.java
@@ -35,6 +35,7 @@ import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.exceptions.ServerException;
 import org.sonar.server.permission.PermissionService;
 import org.sonar.server.permission.PermissionServiceImpl;
+import org.sonar.server.ws.TestRequest;
 
 import static java.lang.String.format;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -133,7 +134,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void fail_when_project_uuid_is_unknown() {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, user.getLogin())
         .setParam(PARAM_PROJECT_ID, "unknown-project-uuid")
@@ -178,7 +179,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   private void failIfComponentIsNotAProjectOrView(ComponentDto file) {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, user.getLogin())
         .setParam(PARAM_PROJECT_ID, file.uuid())
@@ -193,7 +194,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void fail_when_project_permission_without_project() {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, user.getLogin())
         .setParam(PARAM_PERMISSION, UserRole.ISSUE_ADMIN)
@@ -208,7 +209,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
     db.components().insertComponent(newFileDto(project, null, "file-uuid"));
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, user.getLogin())
         .setParam(PARAM_PROJECT_ID, "file-uuid")
@@ -222,7 +223,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void fail_when_get_request() {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setMethod("GET")
         .setParam(PARAM_USER_LOGIN, "george.orwell")
@@ -236,7 +237,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void fail_when_user_login_is_missing() {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
         .execute();
@@ -248,7 +249,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void fail_when_permission_is_missing() {
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, "jrr.tolkien")
         .execute();
@@ -261,7 +262,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
     db.components().insertPrivateProject();
     loginAsAdmin();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
         .setParam(PARAM_USER_LOGIN, user.getLogin())
@@ -277,7 +278,7 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
   public void adding_global_permission_fails_if_not_system_administrator() {
     userSession.logIn();
 
-    assertThatThrownBy(() ->  {
+    assertThatThrownBy(() -> {
       newRequest()
         .setParam(PARAM_USER_LOGIN, user.getLogin())
         .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
@@ -291,16 +292,41 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
     ComponentDto project = db.components().insertPrivateProject();
     userSession.logIn();
 
-    assertThatThrownBy(() ->  {
-      newRequest()
-        .setParam(PARAM_USER_LOGIN, user.getLogin())
-        .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
-        .setParam(PARAM_PROJECT_KEY, project.getKey())
-        .execute();
-    })
+    TestRequest request = newRequest()
+      .setParam(PARAM_USER_LOGIN, user.getLogin())
+      .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
+      .setParam(PARAM_PROJECT_KEY, project.getKey());
+
+    assertThatThrownBy(() -> request.execute())
+      .isInstanceOf(ForbiddenException.class);
+  }
+
+  @Test
+  public void adding_project_permission_fails_if_user_doesnt_exist_and_not_administrator_of_project() {
+    ComponentDto project = db.components().insertPrivateProject();
+    userSession.logIn();
+
+    TestRequest request = newRequest()
+      .setParam(PARAM_USER_LOGIN, "unknown")
+      .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
+      .setParam(PARAM_PROJECT_KEY, project.getKey());
+    assertThatThrownBy(() -> request.execute())
       .isInstanceOf(ForbiddenException.class);
   }
 
+  @Test
+  public void adding_project_permission_fails_if_not_administrator_of_project_and_login_param_is_missing() {
+    ComponentDto project = db.components().insertPrivateProject();
+    userSession.logIn();
+
+    TestRequest request = newRequest()
+      .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
+      .setParam(PARAM_PROJECT_KEY, project.getKey());
+
+    assertThatThrownBy(() -> request.execute())
+      .isInstanceOf(IllegalArgumentException.class);
+  }
+
   /**
    * User is project administrator but not system administrator
    */
@@ -353,13 +379,12 @@ public class AddUserActionTest extends BasePermissionWsTest<AddUserAction> {
     userSession.logIn().addProjectPermission(UserRole.ADMIN, project);
     ComponentDto branch = db.components().insertProjectBranch(project);
 
-    assertThatThrownBy(() ->  {
-      newRequest()
-        .setParam(PARAM_PROJECT_ID, branch.uuid())
-        .setParam(PARAM_USER_LOGIN, user.getLogin())
-        .setParam(PARAM_PERMISSION, SYSTEM_ADMIN)
-        .execute();
-    })
+    TestRequest request = newRequest()
+      .setParam(PARAM_PROJECT_ID, branch.uuid())
+      .setParam(PARAM_USER_LOGIN, user.getLogin())
+      .setParam(PARAM_PERMISSION, SYSTEM_ADMIN);
+
+    assertThatThrownBy(() -> request.execute())
       .isInstanceOf(NotFoundException.class)
       .hasMessage(format("Project id '%s' not found", branch.uuid()));
   }

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 7d755135e7783006e9207844a9477c4e6ecef39d..f9b7895b2be1aaf7c2ad0aa9160cff88e2d240c8 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
@@ -154,6 +154,19 @@ public class ChangePasswordActionTest {
     verifyNoInteractions(jwtHttpHandler);
   }
 
+  @Test
+  public void fail_to_update_someone_else_password_if_not_admin_and_user_doesnt_exist() throws ServletException, IOException {
+    UserTestData user = createLocalUser();
+    userSessionRule.logIn(user.getLogin());
+
+    assertThatThrownBy(() -> executeTest("unknown", "I dunno", NEW_PASSWORD))
+      .isInstanceOf(ForbiddenException.class);
+    verifyNoInteractions(jwtHttpHandler);
+
+    assertThat(findSessionTokenDto(db.getSession(), user.getSessionTokenUuid())).isPresent();
+    verifyNoInteractions(jwtHttpHandler);
+  }
+
   @Test
   public void fail_to_update_unknown_user() {
     UserTestData admin = createLocalUser();

diff --git a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/GroupsActionTest.java b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/GroupsActionTest.java
index 9adaeafcd5a8fd0ca806ce8ff08de91fef981b5b..32aa85e56619fa7e055a95404c5be7af4a27913a 100644 (file)
--- a/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/GroupsActionTest.java
+++ b/server/sonar-webserver-webapi/src/test/java/org/sonar/server/user/ws/GroupsActionTest.java
@@ -255,15 +255,24 @@ public class GroupsActionTest {
   }
 
   @Test
-  public void fail_when_no_permission() {
-    userSession.logIn("not-admin");
+  public void fail_on_missing_permission_and_non_existent_user() {
+    userSession.logIn().addPermission(SCAN);
 
     assertThatThrownBy(() -> {
-      call(ws.newRequest().setParam("login", USER_LOGIN));
+      call(ws.newRequest().setParam("login", "unknown"));
     })
       .isInstanceOf(ForbiddenException.class);
   }
 
+  @Test
+  public void fail_when_no_permission() {
+    userSession.logIn("not-admin");
+
+    TestRequest request = ws.newRequest().setParam("login", USER_LOGIN);
+    assertThatThrownBy(() -> call(request))
+      .isInstanceOf(ForbiddenException.class);
+  }
+
   @Test
   public void test_json_example() {
     UserDto user = insertUser();