diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 88edde26e4..47afdba9f9 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -780,6 +780,14 @@ Users without this access right who are able to upload new patch sets can still do the rebase locally and upload the rebased commit as a new patch set. +[[category_revert]] +=== Revert + +This category permits users to revert changes via the web UI by pushing +the `Revert Change` button. + +Users without this access right who are able to upload changes can +still do the revert locally and upload the revert commit as a new change. [[category_remove_reviewer]] === Remove Reviewer diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8930bc9a95..0badced11b 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -1475,9 +1475,13 @@ describes the reverting change. } ---- +If the user doesn't have revert permission on the change or upload permission on +the destination branch, the response is "`403 Forbidden`", and the error message is +contained in the response body. + If the change cannot be reverted because the change state doesn't -allow reverting the change, the response is "`409 Conflict`" and -the error message is contained in the response body. +allow reverting the change the response is "`409 Conflict`", and the error +message is contained in the response body. .Response ---- @@ -1584,9 +1588,13 @@ is returned. That entity describes the revert changes. ] ---- -If any of the changes cannot be reverted because the change state doesn't -allow reverting the change, the response is "`409 Conflict`" and -the error message is contained in the response body. +If the user doesn't have revert permission on the change or upload permission on +the destination, the response is "`403 Forbidden`", and the error message is +contained in the response body. + +If the change cannot be reverted because the change state doesn't +allow reverting the change the response is "`409 Conflict`", and the error +message is contained in the response body. .Response ---- diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index 253f50cba8..143547b1d3 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java @@ -117,6 +117,12 @@ class ChangeControl { return canAbandon() && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE); } + /** Can this user revert this change? */ + private boolean canRevert() { + return (refControl.canRevert()) + && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE); + } + /** The range of permitted values associated with a label permission. */ private PermissionRange getRange(String permission) { return refControl.getRange(permission, isOwner()); @@ -303,6 +309,8 @@ class ChangeControl { return canRebase(); case RESTORE: return canRestore(); + case REVERT: + return canRevert(); case SUBMIT: return refControl.canSubmit(isOwner()); case TOGGLE_WORK_IN_PROGRESS_STATE: diff --git a/java/com/google/gerrit/server/permissions/ChangePermission.java b/java/com/google/gerrit/server/permissions/ChangePermission.java index 2fba4ef039..63b03787b2 100644 --- a/java/com/google/gerrit/server/permissions/ChangePermission.java +++ b/java/com/google/gerrit/server/permissions/ChangePermission.java @@ -54,6 +54,7 @@ public enum ChangePermission implements ChangePermissionOrLabel { * change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}. */ REBASE, + REVERT, SUBMIT, SUBMIT_AS("submit on behalf of other users"), TOGGLE_WORK_IN_PROGRESS_STATE; diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java index 82150831d3..8479f02dce 100644 --- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java +++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java @@ -96,6 +96,7 @@ public class DefaultPermissionMappings { .put(ChangePermission.REMOVE_REVIEWER, Permission.REMOVE_REVIEWER) .put(ChangePermission.ADD_PATCH_SET, Permission.ADD_PATCH_SET) .put(ChangePermission.REBASE, Permission.REBASE) + .put(ChangePermission.REVERT, Permission.REVERT) .put(ChangePermission.SUBMIT, Permission.SUBMIT) .put(ChangePermission.SUBMIT_AS, Permission.SUBMIT_AS) .put( diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 2c633ba458..7c5d6bda16 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -162,6 +162,10 @@ class RefControl { return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH); } + boolean canRevert() { + return canPerform(Permission.REVERT); + } + /** @return true if this user can submit merge patch sets to this ref */ private boolean canUploadMerges() { return projectControl.controlForRef("refs/for/" + refName).canPerform(Permission.PUSH_MERGE); diff --git a/java/com/google/gerrit/server/restapi/change/Revert.java b/java/com/google/gerrit/server/restapi/change/Revert.java index a1624df666..fd4a13e2b7 100644 --- a/java/com/google/gerrit/server/restapi/change/Revert.java +++ b/java/com/google/gerrit/server/restapi/change/Revert.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.restapi.change; import static com.google.gerrit.extensions.conditions.BooleanCondition.and; +import static com.google.gerrit.server.permissions.ChangePermission.REVERT; import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; import static com.google.gerrit.server.project.ProjectCache.illegalState; @@ -94,6 +95,7 @@ public class Revert .get(rsrc.getProject()) .orElseThrow(illegalState(rsrc.getProject())) .checkStatePermitsWrite(); + rsrc.permissions().check(REVERT); ChangeNotes notes = rsrc.getNotes(); Change.Id changeIdToRevert = notes.getChangeId(); PatchSet.Id patchSetId = notes.getChange().currentPatchSetId(); @@ -125,10 +127,12 @@ public class Revert .setTitle("Revert the change") .setVisible( and( - change.isMerged() && projectStatePermitsWrite, - permissionBackend - .user(rsrc.getUser()) - .ref(change.getDest()) - .testCond(CREATE_CHANGE))); + and( + change.isMerged() && projectStatePermitsWrite, + permissionBackend + .user(rsrc.getUser()) + .ref(change.getDest()) + .testCond(CREATE_CHANGE)), + permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT))); } } diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java index 846b80cc71..88db66ec62 100644 --- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java +++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.gerrit.extensions.conditions.BooleanCondition.and; +import static com.google.gerrit.server.permissions.ChangePermission.REVERT; import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; import static com.google.gerrit.server.project.ProjectCache.illegalState; import static java.util.Objects.requireNonNull; @@ -224,6 +225,7 @@ public class RevertSubmission contributorAgreements.check(change.getProject(), changeResource.getUser()); permissionBackend.currentUser().ref(change.getDest()).check(CREATE_CHANGE); + permissionBackend.currentUser().change(changeData).check(REVERT); permissionBackend.currentUser().change(changeData).check(ChangePermission.READ); projectCache .get(change.getProject()) @@ -518,14 +520,16 @@ public class RevertSubmission "Revert this change and all changes that have been submitted together with this change") .setVisible( and( - change.isMerged() - && change.getSubmissionId() != null - && isChangePartOfSubmission(change.getSubmissionId()) - && projectStatePermitsWrite, - permissionBackend - .user(rsrc.getUser()) - .ref(change.getDest()) - .testCond(CREATE_CHANGE))); + and( + change.isMerged() + && change.getSubmissionId() != null + && isChangePartOfSubmission(change.getSubmissionId()) + && projectStatePermitsWrite, + permissionBackend + .user(rsrc.getUser()) + .ref(change.getDest()) + .testCond(CREATE_CHANGE)), + permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT))); } /** diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java index c15efbaa00..c19be5ed6f 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -178,6 +178,7 @@ public class AllProjectsCreator { grant(config, refsFor, Permission.ADD_PATCH_SET, registered); grant(config, heads, codeReviewLabel, -1, 1, registered); grant(config, heads, Permission.FORGE_AUTHOR, registered); + grant(config, heads, Permission.REVERT, registered); grant(config, magic, Permission.PUSH, registered); grant(config, magic, Permission.PUSH_MERGE, registered); } diff --git a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java index a6424b9683..606d851187 100644 --- a/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java +++ b/java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java @@ -75,6 +75,7 @@ public class AllProjectsCreatorTestUtil { " push = group Project Owners", " submit = group Administrators", " submit = group Project Owners", + " revert = group Registered Users", "[access \"refs/meta/config\"]", " exclusiveGroupPermissions = read", " create = group Administrators", diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java index 11ca391d03..7d97934a73 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java @@ -25,6 +25,7 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.UseClockStep; import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.group.GroupOperations; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.common.RawInputUtil; import com.google.gerrit.common.data.ContributorAgreement; @@ -63,6 +64,7 @@ public class AgreementsIT extends AbstractDaemonTest { private ContributorAgreement caNoAutoVerify; @Inject private GroupOperations groupOperations; @Inject private RequestScopeOperations requestScopeOperations; + @Inject private ProjectOperations projectOperations; protected void setUseContributorAgreements(InheritableBoolean value) throws Exception { try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) { diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java index 50f20c8a36..24d08db002 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java @@ -446,6 +446,22 @@ public class RevertIT extends AbstractDaemonTest { assertThat(thrown).hasMessageThat().contains("Not found: " + r.getChangeId()); } + @Test + public void revertNotAllowedForOwnerWithoutRevertPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add(block(Permission.REVERT).ref("refs/heads/master").group(REGISTERED_USERS)) + .update(); + + PushOneCommit.Result result = createChange(); + approve(result.getChangeId()); + gApi.changes().id(result.getChangeId()).current().submit(); + AuthException thrown = + assertThrows(AuthException.class, () -> gApi.changes().id(result.getChangeId()).revert()); + assertThat(thrown).hasMessageThat().contains("revert not permitted"); + } + @Test @GerritConfig(name = "change.submitWholeTopic", value = "true") public void cantCreateRevertSubmissionWithoutProjectWritePermission() throws Exception { @@ -564,6 +580,25 @@ public class RevertIT extends AbstractDaemonTest { assertThat(authException).hasMessageThat().isEqualTo("read not permitted"); } + @Test + public void revertSubmissionNotAllowedForOwnerWithoutRevertPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add(block(Permission.REVERT).ref("refs/heads/master").group(REGISTERED_USERS)) + .update(); + + PushOneCommit.Result result = createChange(); + approve(result.getChangeId()); + gApi.changes().id(result.getChangeId()).current().submit(); + requestScopeOperations.setApiUser(user.id()); + + AuthException thrown = + assertThrows( + AuthException.class, () -> gApi.changes().id(result.getChangeId()).revertSubmission()); + assertThat(thrown).hasMessageThat().contains("revert not permitted"); + } + @Test public void revertSubmissionPreservesReviewersAndCcs() throws Exception { String change = createChange("first change", "a.txt", "message").getChangeId(); diff --git a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.js b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.js index 0899d4c0d6..7b48ecc333 100644 --- a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.js +++ b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.js @@ -102,6 +102,10 @@ export const AccessBehavior = { id: 'rebase', name: 'Rebase', }, + revert: { + id: 'revert', + name: 'Revert', + }, removeReviewer: { id: 'removeReviewer', name: 'Remove Reviewer',