opendev/gerrit
Code Issues Proposed changes

Allow admins to see all groups (including external ones) of a user

Browse Source 
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
This commit is contained in:
Alice Kober-Sotzek
2018-03-15 12:18:49 +01:00
parent ba6851811a
commit 656384bbef
2 changed files with 8 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/server/account/GroupControl.java
View File

@@ -177,6 +177,6 @@ public class GroupControl {
if (group instanceof GroupDescription.Internal) {
return ((GroupDescription.Internal) group).isVisibleToAll() || isOwner();
}
return false;
return canAdministrateServer();
}
}

13
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -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) {