Ignore non-visible included groups when listing members recursively

When the members of a group are listed via REST with the 'recursive'
option ignore those included groups which are not visible to the
calling user. This is better than failing completely as we do
currently and is consistent with how included external groups are
handled (they are also ignored).

Change-Id: I02b2213573b2647582bddcdcab374c672308bc89
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

Documentation/rest-api-groups.txt

@@ -638,6 +638,9 @@ get::/groups/1/members/
 To resolve the included groups of a group recursively and to list all
 members the parameter `recursive` can be set.
 
+Members from included external groups and from included groups which
+are not visible to the calling user are ignored.
+
 .Request
 ----
   GET /groups/834ec36dd5e0ed21a2ff5d7e2255da082d63bbd7/members/?recursive HTTP/1.0

gerrit-server/src/main/java/com/google/gerrit/server/account/GetGroups.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.account;
 
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.errors.NoSuchGroupException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -39,8 +38,7 @@ public class GetGroups implements RestReadView<AccountResource> {
   }
 
   @Override
-  public List<GroupInfo> apply(AccountResource resource)
+  public List<GroupInfo> apply(AccountResource resource) throws OrmException {
-      throws ResourceNotFoundException, OrmException {
     IdentifiedUser user = resource.getUser();
     Account.Id userId = user.getAccountId();
     List<GroupInfo> groups = Lists.newArrayList();

gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java

@@ -82,8 +82,8 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
 
   @Override
   public List<GroupInfo> apply(GroupResource resource, Input input)
-      throws ResourceNotFoundException, MethodNotAllowedException,
+      throws MethodNotAllowedException, AuthException, BadRequestException,
-      AuthException, BadRequestException, OrmException {
+      OrmException {
     AccountGroup group = resource.toAccountGroup();
     if (group == null) {
       throw new MethodNotAllowedException();
@@ -153,8 +153,8 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
 
     @Override
     public GroupInfo apply(GroupResource resource, Input input)
-        throws ResourceNotFoundException, MethodNotAllowedException,
+        throws MethodNotAllowedException, AuthException, BadRequestException,
-        AuthException, BadRequestException, OrmException {
+        OrmException {
       AddIncludedGroups.Input in = new AddIncludedGroups.Input();
       in.groups = ImmutableList.of(id);
       List<GroupInfo> list = put.get().apply(resource, in);
@@ -178,8 +178,7 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
 
     @Override
     public Object apply(IncludedGroupResource resource,
-        PutIncludedGroup.Input input) throws ResourceNotFoundException,
+        PutIncludedGroup.Input input) throws MethodNotAllowedException, OrmException {
-        MethodNotAllowedException, OrmException {
       // Do nothing, the group is already included.
       return get.get().apply(resource);
     }

gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java

@@ -78,7 +78,7 @@ class CreateGroup implements RestModifyView<TopLevelResource, Input> {
 
   @Override
   public GroupInfo apply(TopLevelResource resource, Input input)
-      throws ResourceNotFoundException, AuthException, BadRequestException,
+      throws AuthException, BadRequestException,
       NameAlreadyUsedException, OrmException {
     if (input == null) {
       input = new Input();

gerrit-server/src/main/java/com/google/gerrit/server/group/GetDetail.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.server.group;
 
 import com.google.gerrit.common.groups.ListGroupsOption;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.group.GroupJson.GroupInfo;
 import com.google.gwtorm.server.OrmException;
@@ -31,8 +30,7 @@ public class GetDetail implements RestReadView<GroupResource> {
   }
 
   @Override
-  public GroupInfo apply(GroupResource rsrc) throws ResourceNotFoundException,
+  public GroupInfo apply(GroupResource rsrc) throws OrmException {
-      OrmException {
     return json.format(rsrc);
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/GetGroup.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.group;
 
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.group.GroupJson.GroupInfo;
 import com.google.gwtorm.server.OrmException;
@@ -29,8 +28,7 @@ class GetGroup implements RestReadView<GroupResource> {
   }
 
   @Override
-  public GroupInfo apply(GroupResource resource)
+  public GroupInfo apply(GroupResource resource) throws OrmException {
-      throws ResourceNotFoundException, OrmException {
     return json.format(resource.getGroup());
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/GetIncludedGroup.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.group;
 
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.group.GroupJson.GroupInfo;
 import com.google.gwtorm.server.OrmException;
@@ -29,8 +28,7 @@ public class GetIncludedGroup implements RestReadView<IncludedGroupResource>  {
   }
 
   @Override
-  public GroupInfo apply(IncludedGroupResource rsrc)
+  public GroupInfo apply(IncludedGroupResource rsrc) throws OrmException {
-      throws ResourceNotFoundException, OrmException {
     return json.format(rsrc.getMemberDescription());
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/GroupJson.java

@@ -22,7 +22,6 @@ import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupDescriptions;
 import com.google.gerrit.common.groups.ListGroupsOption;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.account.AccountInfo;
@@ -65,15 +64,13 @@ public class GroupJson {
     return this;
   }
 
-  public GroupInfo format(GroupResource rsrc) throws ResourceNotFoundException,
+  public GroupInfo format(GroupResource rsrc) throws OrmException {
-      OrmException {
     GroupInfo info = init(rsrc.getGroup());
     initMembersAndIncludes(rsrc, info);
     return info;
   }
 
-  public GroupInfo format(GroupDescription.Basic group)
+  public GroupInfo format(GroupDescription.Basic group) throws OrmException {
-      throws ResourceNotFoundException, OrmException {
     GroupInfo info = init(group);
     if (options.contains(MEMBERS) || options.contains(INCLUDES)) {
       GroupResource rsrc =
@@ -107,7 +104,7 @@ public class GroupJson {
   }
 
   private GroupInfo initMembersAndIncludes(GroupResource rsrc, GroupInfo info)
-      throws ResourceNotFoundException, OrmException {
+      throws OrmException {
     if (rsrc.toAccountGroup() == null) {
       return info;
     }

gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java

@@ -187,7 +187,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   }
 
   private List<GroupInfo> getGroupsOwnedBy(IdentifiedUser user)
-      throws ResourceNotFoundException, OrmException {
+      throws OrmException {
     List<GroupInfo> groups = Lists.newArrayList();
     for (AccountGroup g : filterGroups(groupCache.all())) {
       GroupControl ctl = groupControlFactory.controlFor(g);

gerrit-server/src/main/java/com/google/gerrit/server/group/ListIncludedGroups.java

@@ -19,7 +19,6 @@ import static com.google.common.base.Strings.nullToEmpty;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.AccountGroupIncludeByUuid;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -52,7 +51,7 @@ public class ListIncludedGroups implements RestReadView<GroupResource> {
 
   @Override
   public List<GroupInfo> apply(GroupResource rsrc)
-      throws MethodNotAllowedException, ResourceNotFoundException, OrmException {
+      throws MethodNotAllowedException, OrmException {
     if (rsrc.toAccountGroup() == null) {
       throw new MethodNotAllowedException();
     }

gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java

@@ -21,7 +21,6 @@ import com.google.common.collect.Ordering;
 import com.google.gerrit.common.data.GroupDetail;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -60,33 +59,28 @@ public class ListMembers implements RestReadView<GroupResource> {
 
   @Override
   public List<AccountInfo> apply(final GroupResource resource)
-      throws MethodNotAllowedException, ResourceNotFoundException, OrmException {
+      throws MethodNotAllowedException, OrmException {
     if (resource.toAccountGroup() == null) {
       throw new MethodNotAllowedException();
     }
-    try {
+    final Map<Account.Id, AccountInfo> members =
-      final Map<Account.Id, AccountInfo> members =
+        getMembers(resource.getGroupUUID(), new HashSet<AccountGroup.UUID>());
-          getMembers(resource.getGroupUUID(), new HashSet<AccountGroup.UUID>());
+    final List<AccountInfo> memberInfos = Lists.newArrayList(members.values());
-      final List<AccountInfo> memberInfos = Lists.newArrayList(members.values());
+    Collections.sort(memberInfos, new Comparator<AccountInfo>() {
-      Collections.sort(memberInfos, new Comparator<AccountInfo>() {
+      @Override
-        @Override
+      public int compare(AccountInfo a, AccountInfo b) {
-        public int compare(AccountInfo a, AccountInfo b) {
+        return ComparisonChain.start()
-          return ComparisonChain.start()
+            .compare(a.name, b.name, Ordering.natural().nullsFirst())
-              .compare(a.name, b.name, Ordering.natural().nullsFirst())
+            .compare(a.email, b.email, Ordering.natural().nullsFirst())
-              .compare(a.email, b.email, Ordering.natural().nullsFirst())
+            .compare(a._account_id, b._account_id, Ordering.natural().nullsFirst()).result();
-              .compare(a._account_id, b._account_id, Ordering.natural().nullsFirst()).result();
+      }
-        }
+    });
-      });
+    return memberInfos;
-      return memberInfos;
-    } catch (NoSuchGroupException e) {
-      throw new ResourceNotFoundException(resource.getGroupUUID().get());
-    }
   }
 
   private Map<Account.Id, AccountInfo> getMembers(
       final AccountGroup.UUID groupUUID,
-      final HashSet<AccountGroup.UUID> seenGroups) throws OrmException,
+      final HashSet<AccountGroup.UUID> seenGroups) throws OrmException {
-      NoSuchGroupException {
     seenGroups.add(groupUUID);
 
     final Map<Account.Id, AccountInfo> members = Maps.newHashMap();
@@ -96,8 +90,13 @@ public class ListMembers implements RestReadView<GroupResource> {
       return Collections.emptyMap();
     }
 
-    final GroupDetail groupDetail =
+    final GroupDetail groupDetail;
-        groupDetailFactory.create(group.getId()).call();
+    try {
+      groupDetail = groupDetailFactory.create(group.getId()).call();
+    } catch (NoSuchGroupException e) {
+      // the included group is not visible
+      return Collections.emptyMap();
+    }
 
     if (groupDetail.members != null) {
       for (final AccountGroupMember m : groupDetail.members) {