Merge "Create "Revert" permission" into stable-3.2
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 846b80c..88db66e 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())
@@ -518,14 +520,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',
