SONAR-10949 Add membership check in api/organizations/search_members

server/sonar-server/src/main/java/org/sonar/server/organization/ws/SearchMembersAction.java

@@ -24,6 +24,7 @@ import com.google.common.collect.Ordering;
 import java.util.List;
 import java.util.Optional;
 import javax.annotation.Nullable;
+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;
@@ -33,7 +34,6 @@ import org.sonar.core.util.stream.MoreCollectors;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
 import org.sonar.db.organization.OrganizationDto;
-import org.sonar.db.permission.OrganizationPermission;
 import org.sonar.db.user.UserDto;
 import org.sonar.server.es.SearchOptions;
 import org.sonar.server.es.SearchResult;
@@ -49,8 +49,11 @@ import org.sonarqube.ws.Organizations.User;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Strings.emptyToNull;
+import static org.sonar.api.server.ws.WebService.SelectionMode.SELECTED;
 import static org.sonar.core.util.Protobuf.setNullable;
+import static org.sonar.db.permission.OrganizationPermission.ADMINISTER;
 import static org.sonar.server.es.SearchOptions.MAX_LIMIT;
+import static org.sonar.server.organization.ws.OrganizationsWsSupport.PARAM_ORGANIZATION;
 import static org.sonar.server.ws.WsUtils.checkFoundWithOptional;
 import static org.sonar.server.ws.WsUtils.writeProtobuf;
 
@@ -73,22 +76,25 @@ public class SearchMembersAction implements OrganizationsWsAction {
   @Override
   public void define(WebService.NewController context) {
     WebService.NewAction action = context.createAction("search_members")
-      .setDescription("Search members of an organization")
+      .setDescription("Search members of an organization.<br/>" +
+        "Require organization membership.")
       .setResponseExample(getClass().getResource("search_members-example.json"))
       .setSince("6.4")
       .setInternal(true)
+      .setChangelog(new Change("7.3", "This action now requires organization membership"))
       .setHandler(this);
 
-    action.addSearchQuery("orwe", "names", "logins");
+    action.createSearchQuery("orwe", "names", "logins")
+      .setMinimumLength(2);
     action.addPagingParams(50, MAX_LIMIT);
 
     action.createParam(Param.SELECTED)
       .setDescription("Depending on the value, show only selected items (selected=selected) or deselected items (selected=deselected).")
       .setInternal(true)
-      .setDefaultValue(SelectionMode.SELECTED.value())
-      .setPossibleValues(SelectionMode.SELECTED.value(), SelectionMode.DESELECTED.value());
+      .setDefaultValue(SELECTED.value())
+      .setPossibleValues(SELECTED.value(), SelectionMode.DESELECTED.value());
 
-    action.createParam("organization")
+    action.createParam(PARAM_ORGANIZATION)
       .setDescription("Organization key")
       .setInternal(true)
       .setRequired(false);
@@ -98,6 +104,7 @@ public class SearchMembersAction implements OrganizationsWsAction {
   public void handle(Request request, Response response) throws Exception {
     try (DbSession dbSession = dbClient.openSession(false)) {
       OrganizationDto organization = getOrganization(dbSession, request.param("organization"));
+      userSession.checkMembership(organization);
 
       UserQuery.Builder userQuery = buildUserQuery(request, organization);
       SearchOptions searchOptions = buildSearchOptions(request);
@@ -110,7 +117,7 @@ public class SearchMembersAction implements OrganizationsWsAction {
         .collect(MoreCollectors.toList());
 
       Multiset<String> groupCountByLogin = null;
-      if (userSession.hasPermission(OrganizationPermission.ADMINISTER, organization)) {
+      if (userSession.hasPermission(ADMINISTER, organization)) {
         groupCountByLogin = dbClient.groupMembershipDao().countGroupByLoginsAndOrganization(dbSession, orderedLogins, organization.getUuid());
       }
 

server/sonar-server/src/test/java/org/sonar/server/organization/ws/SearchMembersActionTest.java

@@ -20,8 +20,6 @@
 package org.sonar.server.organization.ws;
 
 import java.util.stream.IntStream;
-import javax.annotation.CheckForNull;
-import javax.annotation.Nullable;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -35,6 +33,7 @@ import org.sonar.db.permission.OrganizationPermission;
 import org.sonar.db.user.GroupDto;
 import org.sonar.db.user.UserDto;
 import org.sonar.server.es.EsTester;
+import org.sonar.server.exceptions.ForbiddenException;
 import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.issue.ws.AvatarResolverImpl;
 import org.sonar.server.organization.DefaultOrganizationProvider;
@@ -42,15 +41,15 @@ import org.sonar.server.organization.TestDefaultOrganizationProvider;
 import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.user.index.UserIndex;
 import org.sonar.server.user.index.UserIndexer;
-import org.sonar.server.ws.TestRequest;
 import org.sonar.server.ws.WsActionTester;
 import org.sonarqube.ws.Common.Paging;
 import org.sonarqube.ws.Organizations.SearchMembersWsResponse;
 import org.sonarqube.ws.Organizations.User;
 
+import static java.lang.String.format;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.tuple;
-import static org.sonar.core.util.Protobuf.setNullable;
+import static org.sonar.api.server.ws.WebService.SelectionMode.DESELECTED;
 import static org.sonar.test.JsonAssert.assertJson;
 
 public class SearchMembersActionTest {
@@ -69,13 +68,17 @@ public class SearchMembersActionTest {
   private DefaultOrganizationProvider organizationProvider = TestDefaultOrganizationProvider.from(db);
   private UserIndexer indexer = new UserIndexer(dbClient, es.client());
 
-  private WsActionTester ws = new WsActionTester(new SearchMembersAction(dbClient, new UserIndex(es.client(), System2.INSTANCE), organizationProvider, userSession, new AvatarResolverImpl()));
-
-  private SearchMembersRequest request = new SearchMembersRequest();
+  private WsActionTester ws = new WsActionTester(
+    new SearchMembersAction(dbClient, new UserIndex(es.client(), System2.INSTANCE), organizationProvider, userSession, new AvatarResolverImpl()));
 
   @Test
   public void empty_response() {
-    SearchMembersWsResponse result = call();
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
+
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList()).isEmpty();
     assertThat(result.getPaging())
@@ -85,13 +88,17 @@ public class SearchMembersActionTest {
 
   @Test
   public void search_members_of_default_organization() {
+    OrganizationDto defaultOrganization = db.getDefaultOrganization();
+    logAsOrganizationMember(defaultOrganization);
     UserDto user = db.users().insertUser();
-    db.organizations().addMember(db.getDefaultOrganization(), user);
+    db.organizations().addMember(defaultOrganization, user);
     UserDto anotherUser = db.users().insertUser();
-    db.organizations().addMember(db.getDefaultOrganization(), anotherUser);
+    db.organizations().addMember(defaultOrganization, anotherUser);
     indexAllUsers();
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", defaultOrganization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList())
       .extracting(User::getLogin, User::getName)
@@ -103,17 +110,18 @@ public class SearchMembersActionTest {
   @Test
   public void search_members_of_specified_organization() {
     OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     OrganizationDto anotherOrganization = db.organizations().insert();
     UserDto user = db.users().insertUser();
     UserDto anotherUser = db.users().insertUser();
     UserDto userInAnotherOrganization = db.users().insertUser();
-    db.organizations().addMember(organization, user);
-    db.organizations().addMember(organization, anotherUser);
+    db.organizations().addMember(organization, user, anotherUser);
     db.organizations().addMember(anotherOrganization, userInAnotherOrganization);
     indexAllUsers();
-    request.setOrganization(organization.getKey());
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList())
       .extracting(User::getLogin, User::getName)
@@ -124,81 +132,106 @@ public class SearchMembersActionTest {
 
   @Test
   public void return_avatar() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     UserDto user = db.users().insertUser(u -> u.setEmail("email@domain.com"));
-    db.organizations().addMember(db.getDefaultOrganization(), user);
+    db.organizations().addMember(organization, user);
     indexer.commitAndIndex(db.getSession(), user);
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsers(0).getAvatar()).isEqualTo("7328fddefd53de471baeb6e2b764f78a");
   }
 
   @Test
   public void do_not_return_group_count_if_no_admin_permission() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     UserDto user = db.users().insertUser();
-    db.organizations().addMember(db.getDefaultOrganization(), user);
+    db.organizations().addMember(organization, user);
     GroupDto group = db.users().insertGroup();
     db.users().insertMember(group, user);
     indexAllUsers();
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsers(0).hasGroupCount()).isFalse();
   }
 
   @Test
   public void return_group_counts_if_org_admin() {
-    userSession.addPermission(OrganizationPermission.ADMINISTER, db.getDefaultOrganization());
+    OrganizationDto organization = db.organizations().insert();
+    UserDto currentUser = db.users().insertUser();
+    userSession.logIn(currentUser)
+      .addPermission(OrganizationPermission.ADMINISTER, organization)
+      .addMembership(organization);
     UserDto user = db.users().insertUser();
-    db.organizations().addMember(db.getDefaultOrganization(), user);
+    db.organizations().addMember(organization, user);
     UserDto anotherUser = db.users().insertUser();
-    db.organizations().addMember(db.getDefaultOrganization(), anotherUser);
+    db.organizations().addMember(organization, anotherUser);
     IntStream.range(0, 10)
-      .mapToObj(i -> db.users().insertGroup())
+      .mapToObj(i -> db.users().insertGroup(organization))
       .forEach(g -> db.users().insertMembers(g, user));
     OrganizationDto anotherOrganization = db.organizations().insert();
     GroupDto anotherGroup = db.users().insertGroup(anotherOrganization);
     db.users().insertMember(anotherGroup, user);
     indexAllUsers();
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
-    assertThat(result.getUsersList()).extracting(User::getLogin, User::getGroupCount).containsOnly(
-      tuple(user.getLogin(), 10),
-      tuple(anotherUser.getLogin(), 0));
+    assertThat(result.getUsersList()).extracting(User::getLogin, User::getGroupCount, User::hasGroupCount)
+      .containsExactlyInAnyOrder(
+        tuple(user.getLogin(), 10, true),
+        tuple(anotherUser.getLogin(), 0, true));
   }
 
   @Test
   public void search_non_members() {
-    OrganizationDto anotherOrganization = db.organizations().insert();
-    UserDto user = db.users().insertUser();
+    OrganizationDto organization = db.organizations().insert();
+    UserDto currentUser = db.users().insertUser();
+    userSession.logIn(currentUser).addMembership(organization);
     UserDto anotherUser = db.users().insertUser();
+    db.organizations().addMember(organization, currentUser, anotherUser);
+
+    OrganizationDto anotherOrganization = db.organizations().insert();
+    UserDto userInNoOrganization = db.users().insertUser();
     UserDto userInAnotherOrganization = db.users().insertUser();
     db.organizations().addMember(anotherOrganization, userInAnotherOrganization);
     indexAllUsers();
-    request
-      .setOrganization(anotherOrganization.getKey())
-      .setSelected(WebService.SelectionMode.DESELECTED.value());
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .setParam("selected", DESELECTED.value())
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList())
       .extracting(User::getLogin, User::getName)
       .containsOnly(
-        tuple(user.getLogin(), user.getName()),
-        tuple(anotherUser.getLogin(), anotherUser.getName()));
+        tuple(userInNoOrganization.getLogin(), userInNoOrganization.getName()),
+        tuple(userInAnotherOrganization.getLogin(), userInAnotherOrganization.getName()));
   }
 
   @Test
   public void search_members_pagination() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     IntStream.range(0, 10).forEach(i -> {
       UserDto userDto = db.users().insertUser(user -> user.setName("USER_" + i));
-      db.organizations().addMember(db.getDefaultOrganization(), userDto);
+      db.organizations().addMember(organization, userDto);
     });
     indexAllUsers();
-    request.setPage(2).setPageSize(3);
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .setParam("p", "2")
+      .setParam("ps", "3")
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList()).extracting(User::getName)
       .containsExactly("USER_3", "USER_4", "USER_5");
@@ -209,87 +242,101 @@ public class SearchMembersActionTest {
 
   @Test
   public void search_members_by_name() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     IntStream.range(0, 10).forEach(i -> {
       UserDto userDto = db.users().insertUser(user -> user.setName("USER_" + i));
-      db.organizations().addMember(db.getDefaultOrganization(), userDto);
+      db.organizations().addMember(organization, userDto);
     });
     indexAllUsers();
-    request.setQuery("_9");
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .setParam("q", "_9")
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList()).extracting(User::getName).containsExactly("USER_9");
   }
 
   @Test
   public void search_members_by_login() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     IntStream.range(0, 10).forEach(i -> {
       UserDto userDto = db.users().insertUser(user -> user.setLogin("USER_" + i));
-      db.organizations().addMember(db.getDefaultOrganization(), userDto);
+      db.organizations().addMember(organization, userDto);
     });
     indexAllUsers();
-    request.setQuery("_9");
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .setParam("q", "_9")
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList()).extracting(User::getLogin).containsExactly("USER_9");
   }
 
   @Test
   public void search_members_by_email() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     IntStream.range(0, 10).forEach(i -> {
       UserDto userDto = db.users().insertUser(user -> user
         .setLogin("L" + i)
         .setEmail("USER_" + i + "@email.com"));
-      db.organizations().addMember(db.getDefaultOrganization(), userDto);
+      db.organizations().addMember(organization, userDto);
     });
     indexAllUsers();
-    request.setQuery("_9");
 
-    SearchMembersWsResponse result = call();
+    SearchMembersWsResponse result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .setParam("q", "_9")
+      .executeProtobuf(SearchMembersWsResponse.class);
 
     assertThat(result.getUsersList()).extracting(User::getLogin).containsExactly("L9");
   }
 
   @Test
   public void fail_if_organization_is_unknown() {
-    request.setOrganization("ORGA 42");
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
 
     expectedException.expect(NotFoundException.class);
-    expectedException.expectMessage("No organization with key 'ORGA 42'");
+    expectedException.expectMessage("No organization with key 'unknown'");
 
-    call();
+    ws.newRequest()
+      .setParam("organization", "unknown")
+      .execute();
   }
 
   @Test
-  public void fail_if_page_size_greater_than_500() {
-    request.setPageSize(501);
-
-    expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("'ps' value (501) must be less than 500");
-
-    call();
-  }
-
-  @Test
-  public void fail_if_query_length_lower_than_2() {
-    request.setQuery("a");
+  public void fail_when_not_member_of_organization() {
+    OrganizationDto organization = db.organizations().insert();
+    UserDto user = db.users().insertUser();
+    userSession.logIn(user);
 
-    expectedException.expect(IllegalArgumentException.class);
-    expectedException.expectMessage("Query length must be greater than or equal to 2");
+    expectedException.expect(ForbiddenException.class);
+    expectedException.expectMessage(format("You're not member of organization '%s'", organization.getKey()));
 
-    call();
+    ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .execute();
   }
 
   @Test
   public void json_example() {
+    OrganizationDto organization = db.organizations().insert();
+    logAsOrganizationMember(organization);
     UserDto user1 = db.users().insertUser(u -> u.setLogin("ada.lovelace").setName("Ada Lovelace").setEmail("ada@lovelace.com"));
-    db.organizations().addMember(db.getDefaultOrganization(), user1);
+    db.organizations().addMember(organization, user1);
     UserDto user2 = db.users().insertUser(u -> u.setLogin("grace.hopper").setName("Grace Hopper").setEmail("grace@hopper.com"));
-    db.organizations().addMember(db.getDefaultOrganization(), user2);
+    db.organizations().addMember(organization, user2);
     indexAllUsers();
 
-    String result = ws.newRequest().execute().getInput();
+    String result = ws.newRequest()
+      .setParam("organization", organization.getKey())
+      .execute()
+      .getInput();
 
     assertJson(result).isSimilarTo(ws.getDef().responseExampleAsString());
   }
@@ -311,78 +358,17 @@ public class SearchMembersActionTest {
     assertThat(selected.possibleValues()).containsOnly("selected", "deselected");
     assertThat(selected.isInternal()).isTrue();
     assertThat(selected.defaultValue()).isEqualTo("selected");
+
+    assertThat(action.param("ps").maximumValue()).isEqualTo(500);
+    assertThat(action.param("q").minimumLength()).isEqualTo(2);
   }
 
   private void indexAllUsers() {
     indexer.indexOnStartup(indexer.getIndexTypes());
   }
 
-  private SearchMembersWsResponse call() {
-    TestRequest wsRequest = ws.newRequest();
-    setNullable(request.getOrganization(), o -> wsRequest.setParam("organization", o));
-    setNullable(request.getQuery(), q -> wsRequest.setParam(Param.TEXT_QUERY, q));
-    setNullable(request.getPage(), p -> wsRequest.setParam(Param.PAGE, String.valueOf(p)));
-    setNullable(request.getPageSize(), ps -> wsRequest.setParam(Param.PAGE_SIZE, String.valueOf(ps)));
-    setNullable(request.getSelected(), s -> wsRequest.setParam(Param.SELECTED, s));
-
-    return wsRequest.executeProtobuf(SearchMembersWsResponse.class);
+  private void logAsOrganizationMember(OrganizationDto organization) {
+    userSession.logIn(db.users().insertUser()).addMembership(organization);
   }
 
-  private static class SearchMembersRequest {
-    private String organization;
-    private String selected;
-    private String query;
-    private Integer page;
-    private Integer pageSize;
-
-    @CheckForNull
-    public String getOrganization() {
-      return organization;
-    }
-
-    public SearchMembersRequest setOrganization(@Nullable String organization) {
-      this.organization = organization;
-      return this;
-    }
-
-    @CheckForNull
-    public String getSelected() {
-      return selected;
-    }
-
-    public SearchMembersRequest setSelected(@Nullable String selected) {
-      this.selected = selected;
-      return this;
-    }
-
-    @CheckForNull
-    public String getQuery() {
-      return query;
-    }
-
-    public SearchMembersRequest setQuery(@Nullable String query) {
-      this.query = query;
-      return this;
-    }
-
-    @CheckForNull
-    public Integer getPage() {
-      return page;
-    }
-
-    public SearchMembersRequest setPage(@Nullable Integer page) {
-      this.page = page;
-      return this;
-    }
-
-    @CheckForNull
-    public Integer getPageSize() {
-      return pageSize;
-    }
-
-    public SearchMembersRequest setPageSize(@Nullable Integer pageSize) {
-      this.pageSize = pageSize;
-      return this;
-    }
-  }
 }