From c540430cf567407530238b5725e17dd0bf926663 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Mon, 20 Nov 2017 13:56:35 +0100 Subject: [PATCH] Catch invalid property values for groups For some properties of groups (e.g. ID, name, ownerUuid), we always assume that their values are well defined in our code. Hence, we should check early on whether those values are within the expected bounds. With NoteDb, we can check them whenever we try to load a group. Writing invalid values is implicitly covered as well since the updated group is loaded from the new config at the end of the writing process (but before creating the commit). Change-Id: Iaa862429dcdf66e00b5f699e754fc098528d7c30 --- .../gerrit/server/group/db/GroupConfig.java | 8 +- .../server/group/db/GroupConfigEntry.java | 41 ++- .../server/group/db/GroupConfigTest.java | 256 ++++++++++++++++++ 3 files changed, 290 insertions(+), 15 deletions(-) create mode 100644 javatests/com/google/gerrit/server/group/db/GroupConfigTest.java diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java index c23dc09600..fd4222b25a 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfig.java +++ b/java/com/google/gerrit/server/group/db/GroupConfig.java @@ -146,11 +146,13 @@ public class GroupConfig extends VersionedMetaData { ImmutableSet members, ImmutableSet subgroups, Timestamp createdOn, - ObjectId refState) { + ObjectId refState) + throws ConfigInvalidException { InternalGroup.Builder group = InternalGroup.builder(); group.setGroupUUID(groupUuid); - Arrays.stream(GroupConfigEntry.values()) - .forEach(configEntry -> configEntry.readFromConfig(group, config)); + for (GroupConfigEntry configEntry : GroupConfigEntry.values()) { + configEntry.readFromConfig(groupUuid, group, config); + } group.setMembers(members); group.setSubgroups(subgroups); group.setCreatedOn(createdOn); diff --git a/java/com/google/gerrit/server/group/db/GroupConfigEntry.java b/java/com/google/gerrit/server/group/db/GroupConfigEntry.java index 0daceb3f19..215f4f1a8b 100644 --- a/java/com/google/gerrit/server/group/db/GroupConfigEntry.java +++ b/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) { - AccountGroup.Id id = new AccountGroup.Id(config.getInt(SECTION_NAME, super.keyName, 0)); - group.setId(id); + void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config) + throws ConfigInvalidException { + 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) { - AccountGroup.NameKey name = - new AccountGroup.NameKey(config.getString(SECTION_NAME, null, super.keyName)); - group.setNameKey(name); + void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config) + throws ConfigInvalidException { + String name = config.getString(SECTION_NAME, null, super.keyName); + 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); diff --git a/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java b/javatests/com/google/gerrit/server/group/db/GroupConfigTest.java new file mode 100644 index 0000000000..3f384b280b --- /dev/null +++ b/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; + } +}