From 656384bbef72e490fd03a770a99637a66e88cf54 Mon Sep 17 00:00:00 2001
From: Alice Kober-Sotzek <aliceks@google.com>
Date: Thu, 15 Mar 2018 12:18:49 +0100
Subject: [PATCH] Allow admins to see all groups (including external ones) of a
 user

Previously, admins were only allowed to see the internal groups a user
belongs to. System groups as well as those from other GroupBackends were
excluded from the result. This impeded investigations in case of ACL
issues.

Allowing admins to see all groups shouldn't create a new security issue.
Even previously, admins had ways to track down any external groups which
were mentioned in Gerrit ACLs. If implemented correctly, external
GroupBackends should only provide groups which are mentioned in the
Gerrit ACLs of the corresponding project. In that case, listing all
groups for another user as admin shouldn't disclose any new information
to admins.

This change doesn't only allow admins to see all groups of a user but
also allows them to see all users who belong to an internal group even
when users are listed as members on subgroups. There is no apparent
reason to allow one direction without the other and that's why we don't
restrict it.

Change-Id: Ia71c59af6035ea23bad5a3156d1522f7dac6424b
---
 .../google/gerrit/server/account/GroupControl.java  |  2 +-
 .../gerrit/acceptance/api/accounts/AccountIT.java   | 13 +++++++------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/java/com/google/gerrit/server/account/GroupControl.java b/java/com/google/gerrit/server/account/GroupControl.java
index 4b223bff1e..5649629ad3 100644
--- a/java/com/google/gerrit/server/account/GroupControl.java
+++ b/java/com/google/gerrit/server/account/GroupControl.java
@@ -177,6 +177,6 @@ public class GroupControl {
     if (group instanceof GroupDescription.Internal) {
       return ((GroupDescription.Internal) group).isVisibleToAll() || isOwner();
     }
-    return false;
+    return canAdministrateServer();
   }
 }
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index f8781b5b00..83d166a61e 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -1982,13 +1982,11 @@ public class AccountIT extends AbstractDaemonTest {
     assertGroups(
         admin.username, ImmutableList.of("Anonymous Users", "Registered Users", "Administrators"));
 
-    // TODO: update when test user is fixed to be included in "Anonymous Users" and
-    //      "Registered Users" groups
-    assertGroups(user.username, ImmutableList.of());
+    assertGroups(user.username, ImmutableList.of("Anonymous Users", "Registered Users"));
 
     String group = createGroup("group");
     String newUser = createAccount("user1", group);
-    assertGroups(newUser, ImmutableList.of(group));
+    assertGroups(newUser, ImmutableList.of("Anonymous Users", "Registered Users", group));
   }
 
   @Test
@@ -2367,11 +2365,14 @@ public class AccountIT extends AbstractDaemonTest {
   }
 
   private void assertGroups(String user, List<String> expected) throws Exception {
-    List<String> actual =
-        gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+    List<String> actual = getNamesOfGroupsOfUser(user);
     assertThat(actual).containsExactlyElementsIn(expected);
   }
 
+  private List<String> getNamesOfGroupsOfUser(String user) throws RestApiException {
+    return gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList());
+  }
+
   private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
     int seq = 1;
     for (SshKeyInfo key : sshKeys) {