From 43fe972939560029f5918bd50ad3e3e287c48620 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 19 Dec 2016 18:06:00 +0100 Subject: 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 (cherry picked from commit 969a1953cf3889baa663fe59e421ba9cffcbabe3) --- .../acceptance/rest/change/AbstractSubmit.java | 20 +++++++++++--- .../rest/change/SubmitByRebaseIfNecessaryIT.java | 31 +++++++++++++++++++--- .../gerrit/server/change/PatchSetInserter.java | 9 ++++++- .../gerrit/server/change/RebaseChangeOp.java | 8 ++++++ .../server/git/strategy/RebaseIfNecessary.java | 3 ++- 5 files changed, 61 insertions(+), 10 deletions(-) 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 6d7a66584e..8ab7c2a13f 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.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) 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 9b3fd15e17..aa5386f2a1 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 @@ 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, 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 5bc3a362b7..f41d41d12e 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 @@ 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 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"); } 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 8909e609c6..20dbfb335e 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 @@ 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"); 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 a309e6ec89..7892a4ac5e 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 @@ 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) { -- cgit v1.2.3