Make names of system groups configurable

By setting 'groups.<UUID>.name' in gerrit.config administrators can
configure the display name for system groups. This is e.g. useful to
rename the 'Anonymous Users' group which is often misunderstood by
users. When configuring a new name for a system group the
administrator must make sure that this name is not used any existing
group. Gerrit prevents creation of groups with names that are used by
system groups (case insensitive). In addition Gerrit keeps the default
names of system groups reserved so that administrators can always
switch back to the default names. This also avoids confusion with
having a normal group with the default name of a system group, but
having the system group available under a different name.

When configuring a name for a system group, the UUID of the group
stays intact. Hence there is no need to modify any group references in
refs/meta/config branches. In the refs/meta/config branches still the
old name will be used until there is an update, which will update the
names of renamed groups automatically.

Change-Id: Ida61027a7ec6d1d30f2a945dfd1fbb2e0c6121ea
Signed-off-by: Edwin Kempin <ekempin@google.com>

Documentation/config-gerrit.txt

@@ -2201,6 +2201,29 @@ all registered users.
 +
 By default, false.
 
+[[groups.uuid.name]]groups.<uuid>.name::
++
+Display name for group with the given UUID.
++
+This option is only supported for system groups (scheme 'global').
++
+E.g. this parameter can be used to configure another name for the
+`Anonymous Users` group:
++
+----
+[groups "global:Anonymous-Users"]
+  name = All Users
+----
++
+When setting this parameter it should be verified that there is no
+existing group with the same name (case-insensitive). Configuring an
+ambiguous name makes Gerrit fail on startup. Once set Gerrit ensures
+that it is not possible to create a group with this name. Gerrit also
+keeps the default name reserved so that it cannot be used for new
+groups either. This means there is no danger of ambiguous group names
+when this parameter is removed and the system group uses the default
+name again.
+
 [[http]]
 === Section http
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java

@@ -17,12 +17,15 @@ package com.google.gerrit.acceptance.api.group;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.acceptance.api.group.GroupAssert.assertGroupInfo;
 import static com.google.gerrit.acceptance.rest.account.AccountAssert.assertAccountInfos;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.extensions.api.groups.GroupApi;
 import com.google.gerrit.extensions.api.groups.GroupInput;
 import com.google.gerrit.extensions.common.AccountInfo;
@@ -176,6 +179,24 @@ public class GroupsIT extends AbstractDaemonTest {
     gApi.groups().create(newGroupName);
   }
 
+  @Test
+  @GerritConfig(name = "groups.global:Anonymous-Users.name", value = "All Users")
+  public void createGroupWithConfiguredNameOfSystemGroup_Conflict()
+      throws Exception {
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage("group 'All Users' already exists");
+    gApi.groups().create("all users");
+  }
+
+  @Test
+  @GerritConfig(name = "groups.global:Anonymous-Users.name", value = "All Users")
+  public void createGroupWithDefaultNameOfSystemGroup_Conflict()
+      throws Exception {
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage("group name 'Anonymous Users' is reserved");
+    gApi.groups().create("anonymous users");
+  }
+
   @Test
   public void createGroupWithProperties() throws Exception {
     GroupInput in = new GroupInput();
@@ -210,6 +231,41 @@ public class GroupsIT extends AbstractDaemonTest {
     assertGroupInfo(expectedGroup, group);
   }
 
+  @Test
+  @GerritConfig(name = "groups.global:Anonymous-Users.name",
+      value = "All Users")
+  public void getSystemGroupByConfiguredName() throws Exception {
+    GroupReference anonymousUsersGroup =
+        systemGroupBackend.getGroup(ANONYMOUS_USERS);
+    assertThat(anonymousUsersGroup.getName()).isEqualTo("All Users");
+
+    GroupInfo group =
+        gApi.groups().id(anonymousUsersGroup.getUUID().get()).get();
+    assertThat(group.name).isEqualTo(anonymousUsersGroup.getName());
+
+    group = gApi.groups().id(anonymousUsersGroup.getName()).get();
+    assertThat(group.id)
+        .isEqualTo(Url.encode((anonymousUsersGroup.getUUID().get())));
+  }
+
+  @Test
+  public void getSystemGroupByDefaultName() throws Exception {
+    GroupReference anonymousUsersGroup =
+        systemGroupBackend.getGroup(ANONYMOUS_USERS);
+    GroupInfo group = gApi.groups().id("Anonymous Users").get();
+    assertThat(group.name).isEqualTo(anonymousUsersGroup.getName());
+    assertThat(group.id)
+        .isEqualTo(Url.encode((anonymousUsersGroup.getUUID().get())));
+  }
+
+  @Test
+  @GerritConfig(name = "groups.global:Anonymous-Users.name",
+      value = "All Users")
+  public void getSystemGroupByDefaultName_NotFound() throws Exception {
+    exception.expect(ResourceNotFoundException.class);
+    gApi.groups().id("Anonymous-Users").get();
+  }
+
   @Test
   public void groupName() throws Exception {
     String name = name("group");

gerrit-server/src/main/java/com/google/gerrit/server/StartupChecks.java

@@ -17,6 +17,8 @@ package com.google.gerrit.server;
 import com.google.gerrit.extensions.events.LifecycleListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.server.account.UniversalGroupBackend;
+import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
@@ -27,6 +29,10 @@ public class StartupChecks implements LifecycleListener {
     protected void configure() {
       DynamicSet.setOf(binder(), StartupCheck.class);
       listener().to(StartupChecks.class);
+      DynamicSet.bind(binder(), StartupCheck.class)
+          .to(UniversalGroupBackend.ConfigCheck.class);
+      DynamicSet.bind(binder(), StartupCheck.class)
+          .to(SystemGroupBackend.NameCheck.class);
     }
   }
 

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.account;
 
 import static com.google.gerrit.server.account.GroupBackends.GROUP_REF_NAME_COMPARATOR;
+import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
@@ -27,10 +28,14 @@ import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.StartupCheck;
+import com.google.gerrit.server.StartupException;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
+import org.eclipse.jgit.lib.Config;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -208,4 +213,38 @@ public class UniversalGroupBackend implements GroupBackend {
     }
     return false;
   }
+
+  public static class ConfigCheck implements StartupCheck {
+    private final Config cfg;
+    private final UniversalGroupBackend universalGroupBackend;
+
+    @Inject
+    ConfigCheck(@GerritServerConfig Config cfg,
+        UniversalGroupBackend groupBackend) {
+      this.cfg = cfg;
+      this.universalGroupBackend = groupBackend;
+    }
+
+    @Override
+    public void check() throws StartupException {
+      String invalid = cfg.getSubsections("groups").stream()
+          .filter(
+              sub -> {
+                AccountGroup.UUID uuid = new AccountGroup.UUID(sub);
+                GroupBackend groupBackend = universalGroupBackend.backend(uuid);
+                return groupBackend == null || groupBackend.get(uuid) == null;
+              })
+          .map(u -> "'" + u + "'")
+          .collect(joining(","));
+
+      if (!invalid.isEmpty()) {
+        throw new StartupException(String.format(
+            "Subsections for 'groups' in gerrit.config must be valid group"
+                + " UUIDs. The following group UUIDs could not be resolved: "
+                + invalid
+                + " Please remove/fix these 'groups' subsections in"
+                + " gerrit.config."));
+      }
+    }
+  }
 }

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

@@ -180,6 +180,14 @@ public class CreateGroup implements RestModifyView<TopLevelResource, GroupInput>
       }
     }
 
+    for (String name : systemGroupBackend.getReservedNames()) {
+      if (name.toLowerCase(Locale.US).equals(
+          createGroupArgs.getGroupName().toLowerCase(Locale.US))) {
+        throw new ResourceConflictException("group name '" + name
+            + "' is reserved");
+      }
+    }
+
     AccountGroup.Id groupId = new AccountGroup.Id(db.nextAccountGroupId());
     AccountGroup.UUID uuid =
         GroupUUID.make(

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

@@ -18,22 +18,30 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.StartupCheck;
+import com.google.gerrit.server.StartupException;
 import com.google.gerrit.server.account.AbstractGroupBackend;
+import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupMembership;
 import com.google.gerrit.server.account.ListGroupMembership;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
+import org.eclipse.jgit.lib.Config;
+
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -80,23 +88,29 @@ public class SystemGroupBackend extends AbstractGroupBackend {
     return ANONYMOUS_USERS.equals(uuid) || REGISTERED_USERS.equals(uuid);
   }
 
+  private final ImmutableSet<String> reservedNames;
   private final SortedMap<String, GroupReference> names;
   private final ImmutableMap<AccountGroup.UUID, GroupReference> uuids;
 
   @Inject
   @VisibleForTesting
-  public SystemGroupBackend() {
+  public SystemGroupBackend(@GerritServerConfig Config cfg) {
     SortedMap<String, GroupReference> n = new TreeMap<>();
     ImmutableMap.Builder<AccountGroup.UUID, GroupReference> u =
         ImmutableMap.builder();
 
+    ImmutableSet.Builder<String> reservedNamesBuilder = ImmutableSet.builder();
     for (AccountGroup.UUID uuid : all) {
       int c = uuid.get().indexOf(':');
-      String name = uuid.get().substring(c + 1).replace('-', ' ');
-      GroupReference ref = new GroupReference(uuid, name);
+      String defaultName = uuid.get().substring(c + 1).replace('-', ' ');
+      reservedNamesBuilder.add(defaultName);
+      String configuredName = cfg.getString("groups", uuid.get(), "name");
+      GroupReference ref = new GroupReference(uuid,
+          MoreObjects.firstNonNull(configuredName, defaultName));
       n.put(ref.getName().toLowerCase(Locale.US), ref);
       u.put(ref.getUUID(), ref);
     }
+    reservedNames = reservedNamesBuilder.build();
     names = Collections.unmodifiableSortedMap(n);
     uuids = u.build();
   }
@@ -109,6 +123,10 @@ public class SystemGroupBackend extends AbstractGroupBackend {
     return names.values().stream().map(r -> r.getName()).collect(toSet());
   }
 
+  public Set<String> getReservedNames() {
+    return reservedNames;
+  }
+
   @Override
   public boolean handles(AccountGroup.UUID uuid) {
     return isSystemGroup(uuid);
@@ -168,4 +186,48 @@ public class SystemGroupBackend extends AbstractGroupBackend {
         ANONYMOUS_USERS,
         REGISTERED_USERS));
   }
+
+  public static class NameCheck implements StartupCheck {
+    private final Config cfg;
+    private final GroupCache groupCache;
+
+    @Inject
+    NameCheck(@GerritServerConfig Config cfg,
+        GroupCache groupCache) {
+      this.cfg = cfg;
+      this.groupCache = groupCache;
+    }
+
+    @Override
+    public void check() throws StartupException {
+      Map<AccountGroup.UUID, String> configuredNames = new HashMap<>();
+      Map<String, AccountGroup.UUID> byLowerCaseConfiguredName =
+          new HashMap<>();
+      for (AccountGroup.UUID uuid : all) {
+        String configuredName = cfg.getString("groups", uuid.get(), "name");
+        if (configuredName != null) {
+          configuredNames.put(uuid, configuredName);
+          byLowerCaseConfiguredName.put(configuredName.toLowerCase(Locale.US),
+              uuid);
+        }
+      }
+      if (configuredNames.isEmpty()) {
+        return;
+      }
+      for (AccountGroup g : groupCache.all()) {
+        String name = g.getName().toLowerCase(Locale.US);
+        if (byLowerCaseConfiguredName.keySet().contains(name)) {
+          AccountGroup.UUID uuidSystemGroup =
+              byLowerCaseConfiguredName.get(name);
+          throw new StartupException(String.format(
+              "The configured name '%s' for system group '%s' is ambiguous"
+                  + " with the name '%s' of existing group '%s'."
+                  + " Please remove/change the value for groups.%s.name in"
+                  + " gerrit.config.",
+              configuredNames.get(uuidSystemGroup), uuidSystemGroup.get(),
+              g.getName(), g.getGroupUUID().get(), uuidSystemGroup.get()));
+        }
+      }
+    }
+  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/account/UniversalGroupBackendTest.java

@@ -38,6 +38,7 @@ import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.testutil.GerritBaseTests;
 
 import org.easymock.IAnswer;
+import org.eclipse.jgit.lib.Config;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -57,7 +58,7 @@ public class UniversalGroupBackendTest extends GerritBaseTests {
     user = createNiceMock(IdentifiedUser.class);
     replay(user);
     backends = new DynamicSet<>();
-    backends.add(new SystemGroupBackend());
+    backends.add(new SystemGroupBackend(new Config()));
     backend = new UniversalGroupBackend(backends);
   }