Don't require Add Patch Set permission for submit by rebase
When the submit strategy was Rebase Always or Rebase If Necessary and
a rebase was needed for the submit, the submit failed if the user
didn't have the Add Patch Set permission. However for submitting a
change the Submit permission alone should be sufficient.
The behavior is now consistent with the Cherry-Pick submit strategy
which also doesn't require the Add Patch Set permission if a
cherry-pick is done on submit.
Change-Id: Id9c484ff51f9dfa7ea77216fbf9b06a799676412
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit 969a1953cf3889baa663fe59e421ba9cffcbabe3)
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 6d7a665..8ab7c2a 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -30,6 +30,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
@@ -460,11 +461,17 @@
}
protected void assertApproved(String changeId) throws Exception {
+ assertApproved(changeId, admin);
+ }
+
+ protected void assertApproved(String changeId, TestAccount user)
+ throws Exception {
ChangeInfo c = get(changeId, DETAILED_LABELS);
LabelInfo cr = c.labels.get("Code-Review");
assertThat(cr.all).hasSize(1);
assertThat(cr.all.get(0).value).isEqualTo(2);
- assertThat(new Account.Id(cr.all.get(0)._accountId)).isEqualTo(admin.getId());
+ assertThat(new Account.Id(cr.all.get(0)._accountId))
+ .isEqualTo(user.getId());
}
protected void assertMerged(String changeId) throws RestApiException {
@@ -484,14 +491,19 @@
protected void assertSubmitter(String changeId, int psId)
throws Exception {
+ assertSubmitter(changeId, psId, admin);
+ }
+
+ protected void assertSubmitter(String changeId, int psId, TestAccount user)
+ throws Exception {
Change c =
getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change();
ChangeNotes cn = notesFactory.createChecked(db, c);
- PatchSetApproval submitter = approvalsUtil.getSubmitter(
- db, cn, new PatchSet.Id(cn.getChangeId(), psId));
+ PatchSetApproval submitter = approvalsUtil.getSubmitter(db, cn,
+ new PatchSet.Id(cn.getChangeId(), psId));
assertThat(submitter).isNotNull();
assertThat(submitter.isLegacySubmit()).isTrue();
- assertThat(submitter.getAccountId()).isEqualTo(admin.getId());
+ assertThat(submitter.getAccountId()).isEqualTo(user.getId());
}
protected void assertNoSubmitter(String changeId, int psId)
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java
index 9b3fd15..aa5386f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java
@@ -17,11 +17,14 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.getChangeId;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -33,6 +36,8 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.Submit.TestSubmitInput;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.project.Util;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -68,6 +73,24 @@
@Test
@TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
public void submitWithRebase() throws Exception {
+ submitWithRebase(admin);
+ }
+
+ @Test
+ @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
+ public void submitWithRebaseWithoutAddPatchSetPermission() throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ Util.block(cfg, Permission.ADD_PATCH_SET, REGISTERED_USERS, "refs/*");
+ Util.allow(cfg, Permission.SUBMIT, REGISTERED_USERS, "refs/heads/*");
+ Util.allow(cfg, Permission.forLabel(Util.codeReview().getName()), -2, 2,
+ REGISTERED_USERS, "refs/heads/*");
+ saveProjectConfig(project, cfg);
+
+ submitWithRebase(user);
+ }
+
+ private void submitWithRebase(TestAccount submitter) throws Exception {
+ setApiUser(submitter);
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change =
createChange("Change 1", "a.txt", "content");
@@ -82,13 +105,13 @@
RevCommit headAfterSecondSubmit = getRemoteHead();
assertThat(headAfterSecondSubmit.getParent(0))
.isEqualTo(headAfterFirstSubmit);
- assertApproved(change2.getChangeId());
+ assertApproved(change2.getChangeId(), submitter);
assertCurrentRevision(change2.getChangeId(), 2, headAfterSecondSubmit);
- assertSubmitter(change2.getChangeId(), 1);
- assertSubmitter(change2.getChangeId(), 2);
+ assertSubmitter(change2.getChangeId(), 1, submitter);
+ assertSubmitter(change2.getChangeId(), 2, submitter);
assertPersonEquals(admin.getIdent(),
headAfterSecondSubmit.getAuthorIdent());
- assertPersonEquals(admin.getIdent(),
+ assertPersonEquals(submitter.getIdent(),
headAfterSecondSubmit.getCommitterIdent());
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 5bc3a36..f41d41d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -95,6 +95,7 @@
private String message;
private CommitValidators.Policy validatePolicy =
CommitValidators.Policy.GERRIT;
+ private boolean checkAddPatchSetPermission = true;
private boolean draft;
private List<String> groups = Collections.emptyList();
private boolean fireRevisionCreated = true;
@@ -154,6 +155,12 @@
return this;
}
+ public PatchSetInserter setCheckAddPatchSetPermission(
+ boolean checkAddPatchSetPermission) {
+ this.checkAddPatchSetPermission = checkAddPatchSetPermission;
+ return this;
+ }
+
public PatchSetInserter setDraft(boolean draft) {
this.draft = draft;
return this;
@@ -294,7 +301,7 @@
CommitValidators cv = commitValidatorsFactory.create(
origCtl.getRefControl(), sshInfo, ctx.getRepository());
- if (!origCtl.canAddPatchSet(ctx.getDb())) {
+ if (checkAddPatchSetPermission && !origCtl.canAddPatchSet(ctx.getDb())) {
throw new AuthException("cannot add patch set");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 8909e60..20dbfb3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -65,6 +65,7 @@
private PersonIdent committerIdent;
private boolean fireRevisionCreated = true;
private CommitValidators.Policy validate;
+ private boolean checkAddPatchSetPermission = true;
private boolean forceContentMerge;
private boolean copyApprovals = true;
@@ -101,6 +102,12 @@
return this;
}
+ public RebaseChangeOp setCheckAddPatchSetPermission(
+ boolean checkAddPatchSetPermission) {
+ this.checkAddPatchSetPermission = checkAddPatchSetPermission;
+ return this;
+ }
+
public RebaseChangeOp setFireRevisionCreated(boolean fireRevisionCreated) {
this.fireRevisionCreated = fireRevisionCreated;
return this;
@@ -153,6 +160,7 @@
.setSendMail(false)
.setFireRevisionCreated(fireRevisionCreated)
.setCopyApprovals(copyApprovals)
+ .setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setMessage(
"Patch Set " + rebasedPatchSetId.get()
+ ": Patch Set " + originalPatchSet.getId().get() + " was rebased");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
index a309e6e..7892a4a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
@@ -124,7 +124,8 @@
// Bypass approval copier since SubmitStrategyOp copy all approvals
// later anyway.
.setCopyApprovals(false)
- .setValidatePolicy(CommitValidators.Policy.NONE);
+ .setValidatePolicy(CommitValidators.Policy.NONE)
+ .setCheckAddPatchSetPermission(false);
try {
rebaseOp.updateRepo(ctx);
} catch (MergeConflictException | NoSuchChangeException e) {
