Merge "Catch invalid property values for groups"

java/com/google/gerrit/server/group/db/GroupConfig.java

@@ -146,11 +146,13 @@ public class GroupConfig extends VersionedMetaData {
       ImmutableSet<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> subgroups,
       Timestamp createdOn,
-      ObjectId refState) {
+      ObjectId refState)
+      throws ConfigInvalidException {
     InternalGroup.Builder group = InternalGroup.builder();
     group.setGroupUUID(groupUuid);
-    Arrays.stream(GroupConfigEntry.values())
+    for (GroupConfigEntry configEntry : GroupConfigEntry.values()) {
-        .forEach(configEntry -> configEntry.readFromConfig(group, config));
+      configEntry.readFromConfig(groupUuid, group, config);
+    }
     group.setMembers(members);
     group.setSubgroups(subgroups);
     group.setCreatedOn(createdOn);

java/com/google/gerrit/server/group/db/GroupConfigEntry.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.group.db;
 import com.google.common.base.Strings;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.group.InternalGroup;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 
 // TODO(aliceks): Add Javadoc descriptions to this file. Mention that this class must only be used
@@ -24,9 +25,14 @@ import org.eclipse.jgit.lib.Config;
 enum GroupConfigEntry {
   ID("id") {
     @Override
-    void readFromConfig(InternalGroup.Builder group, Config config) {
+    void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config)
-      AccountGroup.Id id = new AccountGroup.Id(config.getInt(SECTION_NAME, super.keyName, 0));
+        throws ConfigInvalidException {
-      group.setId(id);
+      int id = config.getInt(SECTION_NAME, super.keyName, -1);
+      if (id < 0) {
+        throw new ConfigInvalidException(
+            String.format("ID of the group %s must not be negative", groupUuid.get()));
+      }
+      group.setId(new AccountGroup.Id(id));
     }
 
     @Override
@@ -46,10 +52,14 @@ enum GroupConfigEntry {
   },
   NAME("name") {
     @Override
-    void readFromConfig(InternalGroup.Builder group, Config config) {
+    void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config)
-      AccountGroup.NameKey name =
+        throws ConfigInvalidException {
-          new AccountGroup.NameKey(config.getString(SECTION_NAME, null, super.keyName));
+      String name = config.getString(SECTION_NAME, null, super.keyName);
-      group.setNameKey(name);
+      if (Strings.isNullOrEmpty(name)) {
+        throw new ConfigInvalidException(
+            String.format("Name of the group %s must be defined", groupUuid.get()));
+      }
+      group.setNameKey(new AccountGroup.NameKey(name));
     }
 
     @Override
@@ -67,9 +77,9 @@ enum GroupConfigEntry {
   },
   DESCRIPTION("description") {
     @Override
-    void readFromConfig(InternalGroup.Builder group, Config config) {
+    void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config) {
       String description = config.getString(SECTION_NAME, null, super.keyName);
-      group.setDescription(description);
+      group.setDescription(Strings.emptyToNull(description));
     }
 
     @Override
@@ -89,8 +99,13 @@ enum GroupConfigEntry {
   },
   OWNER_GROUP_UUID("ownerGroupUuid") {
     @Override
-    void readFromConfig(InternalGroup.Builder group, Config config) {
+    void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config)
+        throws ConfigInvalidException {
       String ownerGroupUuid = config.getString(SECTION_NAME, null, super.keyName);
+      if (Strings.isNullOrEmpty(ownerGroupUuid)) {
+        throw new ConfigInvalidException(
+            String.format("Owner UUID of the group %s must be defined", groupUuid.get()));
+      }
       group.setOwnerGroupUUID(new AccountGroup.UUID(ownerGroupUuid));
     }
 
@@ -110,7 +125,7 @@ enum GroupConfigEntry {
   },
   VISIBLE_TO_ALL("visibleToAll") {
     @Override
-    void readFromConfig(InternalGroup.Builder group, Config config) {
+    void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config) {
       boolean visibleToAll = config.getBoolean(SECTION_NAME, super.keyName, false);
       group.setVisibleToAll(visibleToAll);
     }
@@ -137,7 +152,9 @@ enum GroupConfigEntry {
     this.keyName = keyName;
   }
 
-  abstract void readFromConfig(InternalGroup.Builder group, Config config);
+  abstract void readFromConfig(
+      AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config)
+      throws ConfigInvalidException;
 
   abstract void initNewConfig(Config config, InternalGroupCreation group);
 

javatests/com/google/gerrit/server/group/db/GroupConfigTest.java

@@ -0,0 +1,256 @@
+// Copyright (C) 2017 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.server.group.db;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import java.util.TimeZone;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+public class GroupConfigTest {
+  @Rule public ExpectedException expectedException = ExpectedException.none();
+
+  private Repository repository;
+  private TestRepository<?> testRepository;
+  private final AccountGroup.UUID groupUuid = new AccountGroup.UUID("users-XYZ");
+  private final AccountGroup.NameKey groupName = new AccountGroup.NameKey("users");
+  private final AccountGroup.Id groupId = new AccountGroup.Id(123);
+
+  @Before
+  public void setUp() throws Exception {
+    repository = new InMemoryRepository(new DfsRepositoryDescription("Test Repository"));
+    testRepository = new TestRepository<>(repository);
+  }
+
+  @Test
+  public void nameOfNewGroupMustNotBeNull() throws Exception {
+    InternalGroupCreation groupCreation =
+        getPrefilledGroupCreationBuilder().setNameKey(new AccountGroup.NameKey(null)).build();
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(repository, groupCreation);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Name of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void nameOfNewGroupMustNotBeEmpty() throws Exception {
+    InternalGroupCreation groupCreation =
+        getPrefilledGroupCreationBuilder().setNameKey(new AccountGroup.NameKey("")).build();
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(repository, groupCreation);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Name of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void idOfNewGroupMustNotBeNegative() throws Exception {
+    InternalGroupCreation groupCreation =
+        getPrefilledGroupCreationBuilder().setId(new AccountGroup.Id(-2)).build();
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(repository, groupCreation);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("ID of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void ownerUuidOfNewGroupMustNotBeNull() throws Exception {
+    InternalGroupCreation groupCreation = getPrefilledGroupCreationBuilder().build();
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setOwnerGroupUUID(new AccountGroup.UUID(null)).build();
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(repository, groupCreation);
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Owner UUID of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void ownerUuidOfNewGroupMustNotBeEmpty() throws Exception {
+    InternalGroupCreation groupCreation = getPrefilledGroupCreationBuilder().build();
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setOwnerGroupUUID(new AccountGroup.UUID("")).build();
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(repository, groupCreation);
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Owner UUID of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void nameInConfigMustBeDefined() throws Exception {
+    populateGroupConfig(groupUuid, "[group]\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    expectedException.expect(ConfigInvalidException.class);
+    expectedException.expectMessage("Name of the group users-XYZ");
+    GroupConfig.loadForGroup(repository, groupUuid);
+  }
+
+  @Test
+  public void idInConfigMustBeDefined() throws Exception {
+    populateGroupConfig(groupUuid, "[group]\n\tname = users\n\townerGroupUuid = owners\n");
+
+    expectedException.expect(ConfigInvalidException.class);
+    expectedException.expectMessage("ID of the group users-XYZ");
+    GroupConfig.loadForGroup(repository, groupUuid);
+  }
+
+  @Test
+  public void idInConfigMustNotBeNegative() throws Exception {
+    populateGroupConfig(
+        groupUuid, "[group]\n\tname = users\n\tid = -5\n\townerGroupUuid = owners\n");
+
+    expectedException.expect(ConfigInvalidException.class);
+    expectedException.expectMessage("ID of the group users-XYZ");
+    GroupConfig.loadForGroup(repository, groupUuid);
+  }
+
+  @Test
+  public void ownerUuidInConfigMustBeDefined() throws Exception {
+    populateGroupConfig(groupUuid, "[group]\n\tname = users\n\tid = 42\n");
+
+    expectedException.expect(ConfigInvalidException.class);
+    expectedException.expectMessage("Owner UUID of the group users-XYZ");
+    GroupConfig.loadForGroup(repository, groupUuid);
+  }
+
+  @Test
+  public void nameCannotBeUpdatedToNull() throws Exception {
+    populateGroupConfig(
+        groupUuid, "[group]\n\tname = users\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setName(new AccountGroup.NameKey(null)).build();
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Name of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void nameCannotBeUpdatedToEmptyString() throws Exception {
+    populateGroupConfig(
+        groupUuid, "[group]\n\tname = users\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setName(new AccountGroup.NameKey("")).build();
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Name of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void ownerUuidCannotBeUpdatedToNull() throws Exception {
+    populateGroupConfig(
+        groupUuid, "[group]\n\tname = users\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setOwnerGroupUUID(new AccountGroup.UUID(null)).build();
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Owner UUID of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  @Test
+  public void ownerUuidCannotBeUpdatedToEmptyString() throws Exception {
+    populateGroupConfig(
+        groupUuid, "[group]\n\tname = users\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder().setOwnerGroupUUID(new AccountGroup.UUID("")).build();
+    groupConfig.setGroupUpdate(groupUpdate, Account.Id::toString, AccountGroup.UUID::get);
+
+    try (MetaDataUpdate metaDataUpdate = createMetaDataUpdate()) {
+      expectedException.expectCause(instanceOf(ConfigInvalidException.class));
+      expectedException.expectMessage("Owner UUID of the group users-XYZ");
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  private InternalGroupCreation.Builder getPrefilledGroupCreationBuilder() {
+    return InternalGroupCreation.builder()
+        .setGroupUUID(groupUuid)
+        .setNameKey(groupName)
+        .setId(groupId)
+        .setCreatedOn(TimeUtil.nowTs());
+  }
+
+  private void populateGroupConfig(AccountGroup.UUID uuid, String fileContent) throws Exception {
+    testRepository
+        .branch(RefNames.refsGroups(uuid))
+        .commit()
+        .message("Prepopulate group.config")
+        .add(GroupConfig.GROUP_CONFIG_FILE, fileContent)
+        .create();
+  }
+
+  private MetaDataUpdate createMetaDataUpdate() {
+    TimeZone tz = TimeZone.getTimeZone("America/Los_Angeles");
+    PersonIdent serverIdent =
+        new PersonIdent("Gerrit Server", "noreply@gerritcodereview.com", TimeUtil.nowTs(), tz);
+
+    MetaDataUpdate metaDataUpdate =
+        new MetaDataUpdate(
+            GitReferenceUpdated.DISABLED, new Project.NameKey("Test Repository"), repository);
+    metaDataUpdate.getCommitBuilder().setCommitter(serverIdent);
+    metaDataUpdate.getCommitBuilder().setAuthor(serverIdent);
+    return metaDataUpdate;
+  }
+}