Create "Revert" permission
Create a dedicated revert permission to only allow specific groups to
revert changes. Groups that don't have the "Revert" permission will not
see the Revert button and will not be allowed to revert changes from
the GUI.
This change is created due to spammers that easily use the "Revert
Submission" button to create many changes quickly.
Change-Id: If5180bc982659ab5c9ddaa096ac455823484c50c
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 88edde2..47afdba 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -780,6 +780,14 @@
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 8930bc9..0badced 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1475,9 +1475,13 @@
}
----
+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 @@
]
----
-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 253f50c..143547b 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -117,6 +117,12 @@
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 @@
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 2fba4ef..63b0378 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -54,6 +54,7 @@
* 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 8215083..8479f02 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -96,6 +96,7 @@
.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 2c633ba..7c5d6bd 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -162,6 +162,10 @@
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 a1624df..fd4a13e 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 @@
.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 @@
.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 7a04f05..1055b00 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 @@
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 @@
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())
@@ -525,14 +527,16 @@
"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 c15efba..c19be5e 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
@@ -178,6 +178,7 @@
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 a6424b9..606d851 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 @@
" 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 11ca391..7d97934 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.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 @@
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 50f20c8..24d08db 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -447,6 +447,22 @@
}
@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 {
String secondProject = "secondProject";
@@ -565,6 +581,25 @@
}
@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 0899d4c..7b48ecc 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 @@
id: 'rebase',
name: 'Rebase',
},
+ revert: {
+ id: 'revert',
+ name: 'Revert',
+ },
removeReviewer: {
id: 'removeReviewer',
name: 'Remove Reviewer',
