opendev/gerrit
Code Issues Proposed changes

TestProjectUpdate: Only allow updating capabilities on All-Projects

Browse Source 
In order to implement this logic in TestProjectUpdate, we need to pass
the injected AllProjectsName and the current project name into
TestProjectUpdate. This is an implementation detail handled entirely
in-package, and is not part of the public TestProjectUpdate API
(although it does leak into the toString representation).

This requires changing the tests, which were incorrectly updating
global capabilities on arbitrary repos. A side effect is that we can't
check for the exact contents of the [capabilities] section, because the
site is initialized with more capability entries in All-Projects. Also,
the group names referenced in project.config are different, because the
groups file in All-Projects is also initialized during setup. (This is
the kind of brittleness we signed up for when phrasing the test
assertions over the contents of project.config.)

Change-Id: Ib96dd12612ff7e4bcc95f483e1ed53a803ba3972
This commit is contained in:
Dave Borowitz
2019-05-21 15:12:58 +02:00
parent f4f100d788
commit 5d9f8c9080
4 changed files with 149 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
View File

@@ -32,6 +32,7 @@ import com.google.gerrit.common.data.PermissionRule;
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.config.AllProjectsName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.project.CreateProjectArgs;
@@ -55,6 +56,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk;
public class ProjectOperationsImpl implements ProjectOperations {
private final AllProjectsName allProjectsName;
private final GitRepositoryManager repoManager;
private final MetaDataUpdate.Server metaDataUpdateFactory;
private final ProjectCache projectCache;
@@ -63,11 +65,13 @@ public class ProjectOperationsImpl implements ProjectOperations {
@Inject
ProjectOperationsImpl(
AllProjectsName allProjectsName,
GitRepositoryManager repoManager,
MetaDataUpdate.Server metaDataUpdateFactory,
ProjectCache projectCache,
ProjectConfig.Factory projectConfigFactory,
ProjectCreator projectCreator) {
this.allProjectsName = allProjectsName;
this.repoManager = repoManager;
this.metaDataUpdateFactory = metaDataUpdateFactory;
this.projectCache = projectCache;
@@ -119,7 +123,7 @@ public class ProjectOperationsImpl implements ProjectOperations {
@Override
public TestProjectUpdate.Builder forUpdate() {
return TestProjectUpdate.builder(this::updateProject);
return TestProjectUpdate.builder(nameKey, allProjectsName, this::updateProject);
}
private void updateProject(TestProjectUpdate projectUpdate)

48
java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
View File

@@ -15,19 +15,21 @@
package com.google.gerrit.acceptance.testsuite.project;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.common.data.AccessSection.GLOBAL_CAPABILITIES;
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.testsuite.ThrowingConsumer;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllProjectsName;
import java.util.Optional;
import org.eclipse.jgit.lib.Constants;
@@ -243,7 +245,7 @@ public abstract class TestProjectUpdate {
/** Starts a builder for describing a capability key for deletion. */
public static TestPermissionKey.Builder capabilityKey(String name) {
return TestPermissionKey.builder().name(name).section(AccessSection.GLOBAL_CAPABILITIES);
return TestPermissionKey.builder().name(name).section(GLOBAL_CAPABILITIES);
}
/** Records the key of a permission (of any type) for deletion. */
@@ -270,7 +272,7 @@ public abstract class TestProjectUpdate {
requireNonNull(ref);
checkArgument(ref.startsWith(Constants.R_REFS), "must be a ref: %s", ref);
checkArgument(
!section().isPresent() || !section().get().equals(AccessSection.GLOBAL_CAPABILITIES),
!section().isPresent() || !section().get().equals(GLOBAL_CAPABILITIES),
"can't set ref on global capability");
return section(ref);
}
@@ -285,13 +287,23 @@ public abstract class TestProjectUpdate {
}
}
static Builder builder(ThrowingConsumer<TestProjectUpdate> projectUpdater) {
return new AutoValue_TestProjectUpdate.Builder().projectUpdater(projectUpdater);
static Builder builder(
Project.NameKey nameKey,
AllProjectsName allProjectsName,
ThrowingConsumer<TestProjectUpdate> projectUpdater) {
return new AutoValue_TestProjectUpdate.Builder()
.nameKey(nameKey)
.allProjectsName(allProjectsName)
.projectUpdater(projectUpdater);
}
/** Builder for {@link TestProjectUpdate}. */
@AutoValue.Builder
public abstract static class Builder {
abstract Builder nameKey(Project.NameKey project);
abstract Builder allProjectsName(AllProjectsName allProjects);
abstract ImmutableList.Builder<TestPermission> addedPermissionsBuilder();
abstract ImmutableList.Builder<TestLabelPermission> addedLabelPermissionsBuilder();
@@ -359,7 +371,7 @@ public abstract class TestProjectUpdate {
"do not specify group for setExclusiveGroup: %s",
testPermissionKey);
checkArgument(
!testPermissionKey.section().equals(AccessSection.GLOBAL_CAPABILITIES),
!testPermissionKey.section().equals(GLOBAL_CAPABILITIES),
"setExclusiveGroup not valid for global capabilities: %s",
testPermissionKey);
exclusiveGroupPermissionsBuilder().put(testPermissionKey, exclusive);
@@ -368,7 +380,20 @@ public abstract class TestProjectUpdate {
abstract Builder projectUpdater(ThrowingConsumer<TestProjectUpdate> projectUpdater);
abstract TestProjectUpdate build();
abstract TestProjectUpdate autoBuild();
TestProjectUpdate build() {
TestProjectUpdate projectUpdate = autoBuild();
if (projectUpdate.hasCapabilityUpdates()) {
checkArgument(
projectUpdate.nameKey().equals(projectUpdate.allProjectsName()),
"cannot update global capabilities on %s, only %s: %s",
projectUpdate.nameKey(),
projectUpdate.allProjectsName(),
projectUpdate);
}
return projectUpdate;
}
/** Executes the update, updating the underlying project. */
public void update() {
@@ -377,6 +402,10 @@ public abstract class TestProjectUpdate {
}
}
abstract Project.NameKey nameKey();
abstract AllProjectsName allProjectsName();
abstract ImmutableList<TestPermission> addedPermissions();
abstract ImmutableList<TestLabelPermission> addedLabelPermissions();
@@ -389,6 +418,11 @@ public abstract class TestProjectUpdate {
abstract ThrowingConsumer<TestProjectUpdate> projectUpdater();
boolean hasCapabilityUpdates() {
return !addedCapabilities().isEmpty()
|| removedPermissions().stream().anyMatch(k -> k.section().equals(GLOBAL_CAPABILITIES));
}
private static void checkLabelName(String name) {
// "label-Code-Review" is technically a valid label name, and we don't prevent users from
// using it in production, but specifying it in a test is programmer error.

82
javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
View File

@@ -30,6 +30,7 @@ import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static com.google.gerrit.server.group.SystemGroupBackend.PROJECT_OWNERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.ConfigSubject.assertThat;
import static java.util.stream.Collectors.toList;
@@ -412,54 +413,52 @@ public class ProjectOperationsImplTest extends AbstractDaemonTest {
@Test
public void addAllowCapability() throws Exception {
Project.NameKey key = projectOperations.newProject().create();
Config config = projectOperations.project(allProjects).getConfig();
assertThat(config)
.sectionValues("capability")
.doesNotContainEntry("administrateServer", "group Registered Users");
projectOperations
.project(key)
.project(allProjects)
.forUpdate()
.add(allowCapability(ADMINISTRATE_SERVER).group(REGISTERED_USERS))
.update();
Config config = projectOperations.project(key).getConfig();
assertThat(config).sections().containsExactly("capability");
assertThat(config).subsections("capability").isEmpty();
assertThat(config)
assertThat(projectOperations.project(allProjects).getConfig())
.sectionValues("capability")
.containsExactly("administrateServer", "group global:Registered-Users");
.containsEntry("administrateServer", "group Registered Users");
}
@Test
public void addAllowCapabilityWithRange() throws Exception {
Project.NameKey key = projectOperations.newProject().create();
Config config = projectOperations.project(allProjects).getConfig();
assertThat(config).sectionValues("capability").doesNotContainKey("queryLimit");
projectOperations
.project(key)
.project(allProjects)
.forUpdate()
.add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 5000))
.update();
Config config = projectOperations.project(key).getConfig();
assertThat(config).sections().containsExactly("capability");
assertThat(config).subsections("capability").isEmpty();
assertThat(config)
assertThat(projectOperations.project(allProjects).getConfig())
.sectionValues("capability")
.containsExactly("queryLimit", "+0..+5000 group global:Registered-Users");
.containsEntry("queryLimit", "+0..+5000 group Registered Users");
}
@Test
public void addAllowCapabilityWithDefaultRange() throws Exception {
Project.NameKey key = projectOperations.newProject().create();
Config config = projectOperations.project(allProjects).getConfig();
assertThat(config).sectionValues("capability").doesNotContainKey("queryLimit");
projectOperations
.project(key)
.project(allProjects)
.forUpdate()
.add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS))
.update();
Config config = projectOperations.project(key).getConfig();
assertThat(config).sections().containsExactly("capability");
assertThat(config).subsections("capability").isEmpty();
assertThat(config)
assertThat(projectOperations.project(allProjects).getConfig())
.sectionValues("capability")
.containsExactly(
"queryLimit", "+0..+" + DEFAULT_MAX_QUERY_LIMIT + " group global:Registered-Users");
.containsEntry("queryLimit", "+0..+" + DEFAULT_MAX_QUERY_LIMIT + " group Registered Users");
}
@Test
@@ -514,27 +513,27 @@ public class ProjectOperationsImplTest extends AbstractDaemonTest {
@Test
public void removeCapability() throws Exception {
Project.NameKey key = projectOperations.newProject().create();
projectOperations
.project(key)
.project(allProjects)
.forUpdate()
.add(allowCapability(ADMINISTRATE_SERVER).group(REGISTERED_USERS))
.add(allowCapability(ADMINISTRATE_SERVER).group(PROJECT_OWNERS))
.update();
assertThat(projectOperations.project(key).getConfig())
assertThat(projectOperations.project(allProjects).getConfig())
.sectionValues("capability")
.containsExactly(
"administrateServer", "group global:Registered-Users",
"administrateServer", "group global:Project-Owners");
.containsAtLeastEntriesIn(
ImmutableListMultimap.of(
"administrateServer", "group Registered Users",
"administrateServer", "group Project Owners"));
projectOperations
.project(key)
.project(allProjects)
.forUpdate()
.remove(capabilityKey(ADMINISTRATE_SERVER).group(REGISTERED_USERS))
.update();
assertThat(projectOperations.project(key).getConfig())
assertThat(projectOperations.project(allProjects).getConfig())
.sectionValues("capability")
.containsExactly("administrateServer", "group global:Project-Owners");
.doesNotContainEntry("administrateServer", "group Registered Users");
}
@Test
@@ -567,6 +566,27 @@ public class ProjectOperationsImplTest extends AbstractDaemonTest {
.containsEntry("create", "group global:Registered-Users");
}
@Test
public void updatingCapabilitiesNotAllowedForNonAllProjects() throws Exception {
Project.NameKey key = projectOperations.newProject().create();
assertThrows(
RuntimeException.class,
() ->
projectOperations
.project(key)
.forUpdate()
.add(allowCapability(ADMINISTRATE_SERVER).group(REGISTERED_USERS))
.update());
assertThrows(
RuntimeException.class,
() ->
projectOperations
.project(key)
.forUpdate()
.remove(capabilityKey(ADMINISTRATE_SERVER))
.update());
}
private void deleteRefsMetaConfig(Project.NameKey key) throws Exception {
try (Repository repo = repoManager.openRepository(key)) {
new TestRepository<>(repo).delete(REFS_CONFIG);

53
javatests/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdateTest.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.testsuite.project;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.capabilityKey;
@@ -32,10 +33,14 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.TestCapability;
import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.TestLabelPermission;
import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.TestPermissionKey;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllProjectsName;
import java.util.function.Function;
import org.junit.Test;
public class TestProjectUpdateTest {
private static final AllProjectsName ALL_PROJECTS_NAME = new AllProjectsName("All-Projects");
@Test
public void testCapabilityDisallowsZeroRangeOnCapabilityThatHasNoRange() throws Exception {
assertThrows(
@@ -140,7 +145,7 @@ public class TestProjectUpdateTest {
public void testProjectUpdateDisallowsGroupOnExclusiveGroupPermissionKey() throws Exception {
TestPermissionKey.Builder b = permissionKey(ABANDON).ref("refs/*");
Function<TestPermissionKey.Builder, TestProjectUpdate.Builder> updateBuilder =
kb -> TestProjectUpdate.builder(u -> {}).setExclusiveGroup(kb, true);
kb -> builder().setExclusiveGroup(kb, true);
assertThat(updateBuilder.apply(b).build().exclusiveGroupPermissions())
.containsExactly(b.build(), true);
@@ -148,4 +153,50 @@ public class TestProjectUpdateTest {
b.group(REGISTERED_USERS);
assertThrows(RuntimeException.class, () -> updateBuilder.apply(b).build());
}
@Test
public void hasCapabilityUpdates() throws Exception {
assertThat(builder().build().hasCapabilityUpdates()).isFalse();
assertThat(
builder()
.add(allow(ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(0, 1))
.remove(permissionKey(ABANDON).ref("refs/foo"))
.remove(labelPermissionKey("Code-Review").ref("refs/foo"))
.setExclusiveGroup(permissionKey(ABANDON).ref("refs/bar"), true)
.setExclusiveGroup(labelPermissionKey(ABANDON).ref("refs/bar"), true)
.build()
.hasCapabilityUpdates())
.isFalse();
assertThat(
builder(ALL_PROJECTS_NAME)
.add(allowCapability(ADMINISTRATE_SERVER).group(REGISTERED_USERS))
.build()
.hasCapabilityUpdates())
.isTrue();
assertThat(
builder(ALL_PROJECTS_NAME)
.remove(capabilityKey(ADMINISTRATE_SERVER))
.build()
.hasCapabilityUpdates())
.isTrue();
}
@Test
public void updatingCapabilitiesNotAllowedForNonAllProjects() throws Exception {
assertThrows(
RuntimeException.class,
() -> builder().add(allowCapability(ADMINISTRATE_SERVER).group(REGISTERED_USERS)).update());
assertThrows(
RuntimeException.class,
() -> builder().remove(capabilityKey(ADMINISTRATE_SERVER)).update());
}
private static TestProjectUpdate.Builder builder() {
return builder(Project.nameKey("test-project"));
}
private static TestProjectUpdate.Builder builder(Project.NameKey nameKey) {
return TestProjectUpdate.builder(nameKey, ALL_PROJECTS_NAME, u -> {});
}
}