Reject uploading project config changes if they contain duplicate SRs
This is to prevent project owners and admins from accidentally adding
multiple definitions for the same submit requirement name. The current
behaviour is that one of the definitions override the other, but it's
better to reject such changes.
Existing setups that contain duplicate submit requirement definitions
will continue to work just fine. This is because the config parsing
errors are only checked on the upload config change path, but not during
the server's parsing of configs (e.g. after a server restart). A test is
added for this case.
Google-Bug-Id: b/240133667
Release-Notes: Reject uploading project config changes if they contain duplicate submit requirement definitions
Change-Id: Ib554f921a6683fc688b70dc0186f9b3ec509314a
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 47b0a53..654b3a4 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -977,6 +977,7 @@
Map<String, String> lowerNames = new HashMap<>();
submitRequirementSections = new LinkedHashMap<>();
for (String name : rc.getSubsections(SUBMIT_REQUIREMENT)) {
+ checkDuplicateSrDefinition(rc, name);
String lower = name.toLowerCase();
if (lowerNames.containsKey(lower)) {
error(
@@ -1034,6 +1035,40 @@
}
}
+ private void checkDuplicateSrDefinition(Config rc, String srName) {
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_DESCRIPTION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_DESCRIPTION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_APPLICABILITY_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_APPLICABILITY_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_SUBMITTABILITY_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_SUBMITTABILITY_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_OVERRIDE_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_OVERRIDE_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_OVERRIDE_IN_CHILD_PROJECTS).length
+ > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_OVERRIDE_IN_CHILD_PROJECTS, srName));
+ }
+ }
+
/**
* Report unsupported submit requirement parameters as errors.
*
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 168819c..52207db 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -25,6 +25,8 @@
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.entities.LabelFunction;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -36,6 +38,7 @@
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
public class ProjectConfigIT extends AbstractDaemonTest {
@@ -389,6 +392,168 @@
}
@Test
+ public void rejectSubmitRequirement_duplicateDescriptionKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " description = description 1\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " description = description 2\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of description"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateApplicableIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n "
+ + " applicableIf = is:true\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " applicableIf = is:false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of applicableif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateSubmittableIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " submittableIf = label:Code-Review=MIN\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of submittableif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateOverrideIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " overrideIf = is:true\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " overrideIf = is:false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of overrideif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateCanOverrideInChildProjectsKey() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " canOverrideInChildProjects = true\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n "
+ + " canOverrideInChildProjects = false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of canoverrideinchildprojects"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void submitRequirementsAreParsed_forExistingDuplicateDefinitions() throws Exception {
+ // Duplicate submit requirement definitions are rejected on config change uploads. For setups
+ // already containing duplicate SR definitions, the server is able to parse the "submit
+ // requirements correctly"
+
+ RevCommit revision;
+ // Commit a change to the project config, bypassing server validation.
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ revision =
+ testRepo
+ .branch(RefNames.REFS_CONFIG)
+ .commit()
+ .add(
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " canOverrideInChildProjects = true\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n "
+ + " canOverrideInChildProjects = false\n")
+ .parent(projectOperations.project(project).getHead(RefNames.REFS_CONFIG))
+ .create();
+ }
+
+ try (Repository git = repoManager.openRepository(project)) {
+ // Server is able to parse the config.
+ ProjectConfig cfg = projectConfigFactory.create(project);
+ cfg.load(git, revision);
+
+ // One of the two definitions takes precedence and overrides the other.
+ assertThat(cfg.getSubmitRequirementSections())
+ .containsExactly(
+ "Foo",
+ SubmitRequirement.builder()
+ .setName("Foo")
+ .setAllowOverrideInChildProjects(false)
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .build());
+ }
+ }
+
+ @Test
public void testRejectChangingLabelFunction_toMaxWithBlock() throws Exception {
testChangingLabelFunction(
/* initialLabelFunction= */ LabelFunction.NO_BLOCK,
