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 969a1953cf)

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java

@@ -30,6 +30,7 @@ import com.google.common.collect.Lists;
 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 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
   }
 
   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 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
 
   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)

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java

@@ -17,11 +17,14 @@ package com.google.gerrit.acceptance.rest.change;
 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.Change;
 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 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
   @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 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit {
     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,

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -95,6 +95,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
   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 @@ public class PatchSetInserter extends BatchUpdate.Op {
     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 @@ public class PatchSetInserter extends BatchUpdate.Op {
     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");
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java

@@ -65,6 +65,7 @@ public class RebaseChangeOp extends BatchUpdate.Op {
   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 @@ public class RebaseChangeOp extends BatchUpdate.Op {
     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 @@ public class RebaseChangeOp extends BatchUpdate.Op {
         .setSendMail(false)
         .setFireRevisionCreated(fireRevisionCreated)
         .setCopyApprovals(copyApprovals)
+        .setCheckAddPatchSetPermission(checkAddPatchSetPermission)
         .setMessage(
           "Patch Set " + rebasedPatchSetId.get()
           + ": Patch Set " + originalPatchSet.getId().get() + " was rebased");

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java

@@ -124,7 +124,8 @@ public class RebaseIfNecessary extends SubmitStrategy {
           // 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) {