opendev/gerrit
Code Issues Proposed changes

Create "Revert" permission

Browse Source 
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
This commit is contained in:
Gal Paikin
2020-01-22 10:36:35 +01:00
parent 037a4680f5
commit 6c9ed9550d
13 changed files with 99 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
Documentation/access-control.txt
View File

@@ -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

18
Documentation/rest-api-changes.txt
View File

@@ -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
----

8
java/com/google/gerrit/server/permissions/ChangeControl.java
View File

@@ -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:

1
java/com/google/gerrit/server/permissions/ChangePermission.java
View File

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

1
java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
View File

@@ -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(

4
java/com/google/gerrit/server/permissions/RefControl.java
View File

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

6
java/com/google/gerrit/server/restapi/change/Revert.java
View File

@@ -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();
@@ -124,11 +126,13 @@ public class Revert
.setLabel("Revert")
.setTitle("Revert the change")
.setVisible(
and(
and(
change.isMerged() && projectStatePermitsWrite,
permissionBackend
.user(rsrc.getUser())
.ref(change.getDest())
.testCond(CREATE_CHANGE)));
.testCond(CREATE_CHANGE)),
permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT)));
}
}

6
java/com/google/gerrit/server/restapi/change/RevertSubmission.java
View File

@@ -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())
@@ -524,6 +526,7 @@ public class RevertSubmission
.setTitle(
"Revert this change and all changes that have been submitted together with this change")
.setVisible(
and(
and(
change.isMerged()
&& change.getSubmissionId() != null
@@ -532,7 +535,8 @@ public class RevertSubmission
permissionBackend
.user(rsrc.getUser())
.ref(change.getDest())
.testCond(CREATE_CHANGE)));
.testCond(CREATE_CHANGE)),
permissionBackend.user(rsrc.getUser()).change(rsrc.getNotes()).testCond(REVERT)));
}
/**

1
java/com/google/gerrit/server/schema/AllProjectsCreator.java
View File

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

1
java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
View File

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

2
javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
View File

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

35
javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
View File

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

4
polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.js
View File

@@ -102,6 +102,10 @@ export const AccessBehavior = {
id: 'rebase',
name: 'Rebase',
},
revert: {
id: 'revert',
name: 'Revert',
},
removeReviewer: {
id: 'removeReviewer',
name: 'Remove Reviewer',