Merge "Create "Revert" permission" into stable-3.2

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

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
+allow reverting the change the response is "`409 Conflict`", and the error
-the error message is contained in the response body.
+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
+If the user doesn't have revert permission on the change or upload permission on
-allow reverting the change, the response is "`409 Conflict`" and
+the destination, the response is "`403 Forbidden`", and the error message is
-the error message is contained in the response body.
+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
 ----

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:

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;

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(

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);

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,
+                and(
-                permissionBackend
+                    change.isMerged() && projectStatePermitsWrite,
-                    .user(rsrc.getUser())
+                    permissionBackend
-                    .ref(change.getDest())
+                        .user(rsrc.getUser())
-                    .testCond(CREATE_CHANGE)));
+                        .ref(change.getDest())
+                        .testCond(CREATE_CHANGE)),
+                permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT)));
   }
 }

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()
+                and(
-                    && change.getSubmissionId() != null
+                    change.isMerged()
-                    && isChangePartOfSubmission(change.getSubmissionId())
+                        && change.getSubmissionId() != null
-                    && projectStatePermitsWrite,
+                        && isChangePartOfSubmission(change.getSubmissionId())
-                permissionBackend
+                        && projectStatePermitsWrite,
-                    .user(rsrc.getUser())
+                    permissionBackend
-                    .ref(change.getDest())
+                        .user(rsrc.getUser())
-                    .testCond(CREATE_CHANGE)));
+                        .ref(change.getDest())
+                        .testCond(CREATE_CHANGE)),
+                permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT)));
   }
 
   /**

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);
   }

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",

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)) {

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();

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',