Prevent creating a group with the same name as a system group

Prevent the SSH create-group command from creating groups
with the same name as a Gerrit system group[1]

[1] https://gerrit-review.googlesource.com/
     Documentation/access-control.html#system_groups

Bug: Issue 3831
Change-Id: I5dbb4806c2a38c8da87c75481ee747812dbf4f10

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

@@ -139,6 +139,45 @@ public class GroupsIT extends AbstractDaemonTest {
     assertGroupInfo(getFromCache(newGroupName), g);
   }
 
+  @Test
+  public void testCreateDuplicateInternalGroupCaseSensitiveName_Conflict()
+      throws Exception {
+    String dupGroupName = name("dupGroup");
+    gApi.groups().create(dupGroupName);
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage("group '" + dupGroupName + "' already exists");
+    gApi.groups().create(dupGroupName);
+  }
+
+  @Test
+  public void testCreateDuplicateInternalGroupCaseInsensitiveName()
+      throws Exception {
+    String dupGroupName = name("dupGroupA");
+    String dupGroupNameLowerCase = name("dupGroupA").toLowerCase();
+    gApi.groups().create(dupGroupName);
+    gApi.groups().create(dupGroupNameLowerCase);
+    assertThat(gApi.groups().list().getAsMap().keySet().contains(dupGroupName));
+    assertThat(gApi.groups().list().getAsMap().keySet().contains(dupGroupNameLowerCase));
+  }
+
+  @Test
+  public void testCreateDuplicateSystemGroupCaseSensitiveName_Conflict()
+      throws Exception {
+    String newGroupName = "Registered Users";
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage("group 'Registered Users' already exists");
+    gApi.groups().create(newGroupName);
+  }
+
+  @Test
+  public void testCreateDuplicateSystemGroupCaseInsensitiveName_Conflict()
+      throws Exception {
+    String newGroupName = "registered users";
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage("group 'Registered Users' already exists");
+    gApi.groups().create(newGroupName);
+  }
+
   @Test
   public void testCreateGroupWithProperties() throws Exception {
     GroupInput in = new GroupInput();
@@ -159,13 +198,6 @@ public class GroupsIT extends AbstractDaemonTest {
     gApi.groups().create(name("newGroup"));
   }
 
-  @Test
-  public void testCreateGroupWhenGroupAlreadyExists_Conflict()
-      throws Exception {
-    exception.expect(ResourceConflictException.class);
-    gApi.groups().create("Administrators");
-  }
-
   @Test
   public void testGetGroup() throws Exception {
     AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/CreateGroupIT.java

@@ -0,0 +1,82 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.ssh;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+
+import org.junit.Test;
+
+public class CreateGroupIT extends AbstractDaemonTest {
+
+  @Test
+  public void withDuplicateInternalGroupCaseSensitiveName_Conflict()
+      throws Exception {
+    String newGroupName = "dupGroupA";
+    adminSession.put("/groups/" + newGroupName);
+    sshSession.exec("gerrit create-group " + newGroupName);
+    assert_().withFailureMessage(sshSession.getError())
+        .that(sshSession.hasError()).isTrue();
+  }
+
+  @Test
+  public void withDuplicateInternalGroupCaseInsensitiveName()
+      throws Exception {
+    String newGroupName = "dupGroupB";
+    String newGroupNameLowerCase = newGroupName.toLowerCase();
+
+    adminSession.put("/groups/" + newGroupName);
+    sshSession.exec("gerrit create-group " + newGroupNameLowerCase);
+    assert_().withFailureMessage(sshSession.getError())
+        .that(sshSession.hasError()).isFalse();
+    assertThat(groupCache.get(new AccountGroup.NameKey(newGroupName)))
+      .isNotNull();
+    assertThat(groupCache.get(new AccountGroup.NameKey(newGroupNameLowerCase)))
+      .isNotNull();
+  }
+
+  @Test
+  public void withDuplicateSystemGroupCaseSensitiveName_Conflict()
+      throws Exception {
+    String newGroupName = "Registered Users";
+    sshSession.exec("gerrit create-group " + newGroupName);
+    assert_().withFailureMessage(sshSession.getError())
+        .that(sshSession.hasError()).isTrue();
+  }
+
+  @Test
+  public void withDuplicateSystemGroupCaseInsensitiveName_Conflict()
+      throws Exception {
+    String newGroupName = "Registered Users";
+    sshSession.exec("gerrit create-group " + newGroupName);
+    assert_().withFailureMessage(sshSession.getError())
+        .that(sshSession.hasError()).isTrue();
+  }
+
+  @Test
+  public void withNonDuplicateGroupName() throws Exception {
+    String newGroupName = "newGroupB";
+    sshSession.exec("gerrit create-group " + newGroupName);
+    assert_().withFailureMessage(sshSession.getError())
+        .that(sshSession.hasError()).isFalse();
+    AccountGroup accountGroup =
+        groupCache.get(new AccountGroup.NameKey(newGroupName));
+    assertThat(accountGroup).isNotNull();
+    assertThat(accountGroup.getName()).isEqualTo(newGroupName);
+  }
+}

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

@@ -51,6 +51,8 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
 
 import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
 
 @RequiresCapability(GlobalCapability.CREATE_GROUP)
 public class CreateGroup implements RestModifyView<TopLevelResource, GroupInput> {
@@ -137,6 +139,17 @@ public class CreateGroup implements RestModifyView<TopLevelResource, GroupInput>
 
   private AccountGroup createGroup(CreateGroupArgs createGroupArgs)
       throws OrmException, ResourceConflictException {
+
+    // Do not allow creating groups with the same name as system groups
+    List<String> sysGroupNames = SystemGroupBackend.getNames();
+    for (String name : sysGroupNames) {
+      if (name.toLowerCase(Locale.US).equals(
+          createGroupArgs.getGroupName().toLowerCase(Locale.US))) {
+        throw new ResourceConflictException("group '" + name
+            + "' already exists");
+      }
+    }
+
     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

@@ -37,35 +37,38 @@ import java.util.SortedMap;
 import java.util.TreeMap;
 
 public class SystemGroupBackend extends AbstractGroupBackend {
+  public static final String SYSTEM_GROUP_SCHEME = "global:";
+
   /** Common UUID assigned to the "Anonymous Users" group. */
   public static final AccountGroup.UUID ANONYMOUS_USERS =
-      new AccountGroup.UUID("global:Anonymous-Users");
+      new AccountGroup.UUID(SYSTEM_GROUP_SCHEME + "Anonymous-Users");
 
   /** Common UUID assigned to the "Registered Users" group. */
   public static final AccountGroup.UUID REGISTERED_USERS =
-      new AccountGroup.UUID("global:Registered-Users");
+      new AccountGroup.UUID(SYSTEM_GROUP_SCHEME + "Registered-Users");
 
   /** Common UUID assigned to the "Project Owners" placeholder group. */
   public static final AccountGroup.UUID PROJECT_OWNERS =
-      new AccountGroup.UUID("global:Project-Owners");
+      new AccountGroup.UUID(SYSTEM_GROUP_SCHEME + "Project-Owners");
 
   /** Common UUID assigned to the "Change Owner" placeholder group. */
   public static final AccountGroup.UUID CHANGE_OWNER =
-      new AccountGroup.UUID("global:Change-Owner");
+      new AccountGroup.UUID(SYSTEM_GROUP_SCHEME + "Change-Owner");
 
   private static final SortedMap<String, GroupReference> names;
   private static final ImmutableMap<AccountGroup.UUID, GroupReference> uuids;
+  private static final AccountGroup.UUID[] all = {
+      ANONYMOUS_USERS,
+      REGISTERED_USERS,
+      PROJECT_OWNERS,
+      CHANGE_OWNER,
+  };
 
   static {
     SortedMap<String, GroupReference> n = new TreeMap<>();
     ImmutableMap.Builder<AccountGroup.UUID, GroupReference> u =
         ImmutableMap.builder();
-    AccountGroup.UUID[] all = {
-        ANONYMOUS_USERS,
-        REGISTERED_USERS,
-        PROJECT_OWNERS,
-        CHANGE_OWNER,
-    };
+
     for (AccountGroup.UUID uuid : all) {
       int c = uuid.get().indexOf(':');
       String name = uuid.get().substring(c + 1).replace('-', ' ');
@@ -78,7 +81,7 @@ public class SystemGroupBackend extends AbstractGroupBackend {
   }
 
   public static boolean isSystemGroup(AccountGroup.UUID uuid) {
-    return uuid.get().startsWith("global:");
+    return uuid.get().startsWith(SYSTEM_GROUP_SCHEME);
   }
 
   public static boolean isAnonymousOrRegistered(GroupReference ref) {
@@ -93,6 +96,15 @@ public class SystemGroupBackend extends AbstractGroupBackend {
     return checkNotNull(uuids.get(uuid), "group %s not found", uuid.get());
   }
 
+  public static List<String> getNames() {
+    List<String> names = new ArrayList<>();
+    for (AccountGroup.UUID uuid : all) {
+      int c = uuid.get().indexOf(':');
+      names.add(uuid.get().substring(c + 1).replace('-', ' '));
+    }
+    return names;
+  }
+
   @Override
   public boolean handles(AccountGroup.UUID uuid) {
     return isSystemGroup(uuid);