Make code to list groups for a user reuasble from SSH commands

The motivation for this refactoring is to be able to implement in a
later change a new option for the 'ls-groups' command that allows to
list the groups for a specific user.

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

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountSecurityImpl.java

@@ -27,7 +27,6 @@ import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.AccountAgreement;
 import com.google.gerrit.reviewdb.AccountExternalId;
-import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.reviewdb.AccountSshKey;
 import com.google.gerrit.reviewdb.AuthType;
 import com.google.gerrit.reviewdb.ContactInformation;
@@ -43,7 +42,6 @@ import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.account.ChangeUserName;
 import com.google.gerrit.server.account.ClearPassword;
 import com.google.gerrit.server.account.GeneratePassword;
-import com.google.gerrit.server.account.GroupDetailFactory;
 import com.google.gerrit.server.account.Realm;
 import com.google.gerrit.server.config.AuthConfig;
 import com.google.gerrit.server.contact.ContactStore;
@@ -63,7 +61,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.UnsupportedEncodingException;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -88,7 +85,6 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
   private final DeleteExternalIds.Factory deleteExternalIdsFactory;
   private final ExternalIdDetailFactory.Factory externalIdDetailFactory;
   private final MyGroupsFactory.Factory myGroupsFactory;
-  private final GroupDetailFactory.Factory groupDetailFactory;
 
   private final ChangeHookRunner hooks;
 
@@ -105,7 +101,6 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
       final DeleteExternalIds.Factory deleteExternalIdsFactory,
       final ExternalIdDetailFactory.Factory externalIdDetailFactory,
       final MyGroupsFactory.Factory myGroupsFactory,
-      final GroupDetailFactory.Factory groupDetailFactory,
       final ChangeHookRunner hooks) {
     super(schema, currentUser);
     contactStore = cs;
@@ -126,7 +121,6 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
     this.deleteExternalIdsFactory = deleteExternalIdsFactory;
     this.externalIdDetailFactory = externalIdDetailFactory;
     this.myGroupsFactory = myGroupsFactory;
-    this.groupDetailFactory = groupDetailFactory;
     this.hooks = hooks;
   }
 
@@ -215,11 +209,7 @@ class AccountSecurityImpl extends BaseServiceImplementation implements
     run(callback, new Action<List<GroupDetail>>() {
       public List<GroupDetail> run(final ReviewDb db) throws OrmException,
           NoSuchGroupException, Failure {
-        List<GroupDetail> groupDetails = new ArrayList<GroupDetail>();
+        return myGroupsFactory.create().call();
-        for(AccountGroup group : myGroupsFactory.create().call()) {
-          groupDetails.add(groupDetailFactory.create(group.getId()).call());
-        }
-        return groupDetails;
       }
     });
   }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/MyGroupsFactory.java

@@ -14,46 +14,33 @@
 
 package com.google.gerrit.httpd.rpc.account;
 
+import com.google.gerrit.common.data.GroupDetail;
+import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.httpd.rpc.Handler;
-import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.VisibleGroups;
+import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
 import java.util.List;
-import java.util.Set;
 
-class MyGroupsFactory extends Handler<List<AccountGroup>> {
+class MyGroupsFactory extends Handler<List<GroupDetail>> {
   interface Factory {
     MyGroupsFactory create();
   }
 
-  private final GroupCache groupCache;
+  private final VisibleGroups.Factory visibleGroupsFactory;
   private final IdentifiedUser user;
 
   @Inject
-  MyGroupsFactory(final GroupCache groupCache, final IdentifiedUser user) {
+  MyGroupsFactory(final VisibleGroups.Factory visibleGroupsFactory, final IdentifiedUser user) {
-    this.groupCache = groupCache;
+    this.visibleGroupsFactory = visibleGroupsFactory;
     this.user = user;
   }
 
   @Override
-  public List<AccountGroup> call() {
+  public List<GroupDetail> call() throws OrmException, NoSuchGroupException {
-    final Set<AccountGroup.UUID> effective = user.getEffectiveGroups();
+    final VisibleGroups visibleGroups = visibleGroupsFactory.create();
-    final int cnt = effective.size();
+    return visibleGroups.get(user).getGroups();
-    final List<AccountGroup> groupList = new ArrayList<AccountGroup>(cnt);
-    for (final AccountGroup.UUID groupId : effective) {
-      groupList.add(groupCache.get(groupId));
-    }
-    Collections.sort(groupList, new Comparator<AccountGroup>() {
-      @Override
-      public int compare(AccountGroup a, AccountGroup b) {
-        return a.getName().compareTo(b.getName());
-      }
-    });
-    return groupList;
   }
 }

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

@@ -30,7 +30,6 @@ import java.util.Collection;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
-import java.util.SortedSet;
 import java.util.TreeSet;
 
 public class VisibleGroups {
@@ -44,7 +43,6 @@ public class VisibleGroups {
   private final GroupControl.Factory groupControlFactory;
   private final GroupDetailFactory.Factory groupDetailFactory;
 
-  private Collection<ProjectControl> projects;
   private boolean onlyVisibleToAll;
   private AccountGroup.Type groupType;
 
@@ -59,10 +57,6 @@ public class VisibleGroups {
     this.groupDetailFactory = groupDetailFactory;
   }
 
-  public void setProjects(final Collection<ProjectControl> projects) {
-    this.projects = projects;
-  }
-
   public void setOnlyVisibleToAll(final boolean onlyVisibleToAll) {
     this.onlyVisibleToAll = onlyVisibleToAll;
   }
@@ -72,17 +66,13 @@ public class VisibleGroups {
   }
 
   public GroupList get() throws OrmException, NoSuchGroupException {
-    final Iterable<AccountGroup> groups;
+    final Iterable<AccountGroup> groups = groupCache.all();
-    if (projects != null && !projects.isEmpty()) {
-      groups = getGroupsForProjects();
-    } else {
-      groups = groupCache.all();
-    }
     return createGroupList(filterGroups(groups));
   }
 
-  private Set<AccountGroup> getGroupsForProjects() throws NoSuchGroupException {
+  public GroupList get(final Collection<ProjectControl> projects)
-    final SortedSet<AccountGroup> groups =
+      throws OrmException, NoSuchGroupException {
+    final Set<AccountGroup> groups =
         new TreeSet<AccountGroup>(new GroupComparator());
     for (final ProjectControl projectControl : projects) {
       final Set<GroupReference> groupsRefs = projectControl.getAllGroups();
@@ -94,7 +84,24 @@ public class VisibleGroups {
         groups.add(group);
       }
     }
-    return groups;
+    return createGroupList(filterGroups(groups));
+  }
+
+  public GroupList get(final IdentifiedUser user) throws OrmException,
+      NoSuchGroupException {
+    if (identifiedUser.get().getAccountId().equals(user.getAccountId())
+        || identifiedUser.get().getCapabilities().canAdministrateServer()) {
+      final Set<AccountGroup.UUID> effective = user.getEffectiveGroups();
+      final Set<AccountGroup> groups =
+          new TreeSet<AccountGroup>(new GroupComparator());
+      for (final AccountGroup.UUID groupId : effective) {
+        groups.add(groupCache.get(groupId));
+      }
+      return createGroupList(filterGroups(groups));
+    } else {
+      throw new NoSuchGroupException("Groups of user '" + user.getAccountId()
+          + "' are not visible.");
+    }
   }
 
   private List<AccountGroup> filterGroups(final Iterable<AccountGroup> groups) {

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ListGroupsCommand.java

@@ -63,10 +63,14 @@ public class ListGroupsCommand extends BaseCommand {
     final PrintWriter stdout = toPrintWriter(out);
     try {
       final VisibleGroups visibleGroups = visibleGroupsFactory.create();
-      visibleGroups.setProjects(projects);
       visibleGroups.setOnlyVisibleToAll(visibleToAll);
       visibleGroups.setGroupType(groupType);
-      final GroupList groupList = visibleGroups.get();
+      final GroupList groupList;
+      if (!projects.isEmpty()) {
+        groupList = visibleGroups.get(projects);
+      } else {
+        groupList = visibleGroups.get();
+      }
       for (final GroupDetail groupDetail : groupList.getGroups()) {
         stdout.print(groupDetail.group.getName() + "\n");
       }