Add new change permission: Toggle Work In Progress state
Allow to grant the ACL to arbitrary users using new permission to
control who is able to flip the Work In Progress bit in addition to
three different groups allowed to toggle Work In Progress state: change
owner, server administrators and project owners.
Feature: Issue 10385
Change-Id: I66f6078936c4fae20f3a12dbd3a5d9a244eb75cb
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 544039b..17c9a61 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -548,7 +548,8 @@
----
Alternatively, click *Ready* from the Change screen.
-Only change owners, project owners and site administrators can mark changes as
+Change owners, project owners, site administrators and members of a group that
+was granted "Toggle Work In Progress state" permission can mark changes as
`work-in-progress` and `ready`.
[[wip-polygerrit]]
diff --git a/java/com/google/gerrit/common/data/Permission.java b/java/com/google/gerrit/common/data/Permission.java
index 2e9c2d6..3ba0ba7 100644
--- a/java/com/google/gerrit/common/data/Permission.java
+++ b/java/com/google/gerrit/common/data/Permission.java
@@ -46,6 +46,7 @@
public static final String REMOVE_REVIEWER = "removeReviewer";
public static final String SUBMIT = "submit";
public static final String SUBMIT_AS = "submitAs";
+ public static final String TOGGLE_WORK_IN_PROGRESS_STATE = "toggleWipState";
public static final String VIEW_PRIVATE_CHANGES = "viewPrivateChanges";
private static final List<String> NAMES_LC;
@@ -78,6 +79,7 @@
NAMES_LC.add(REMOVE_REVIEWER.toLowerCase());
NAMES_LC.add(SUBMIT.toLowerCase());
NAMES_LC.add(SUBMIT_AS.toLowerCase());
+ NAMES_LC.add(TOGGLE_WORK_IN_PROGRESS_STATE.toLowerCase());
NAMES_LC.add(VIEW_PRIVATE_CHANGES.toLowerCase());
LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
diff --git a/java/com/google/gerrit/server/change/WorkInProgressOp.java b/java/com/google/gerrit/server/change/WorkInProgressOp.java
index 02870fb..e5fdd8f 100644
--- a/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -18,20 +18,14 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.ChangeMessagesUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.extensions.events.WorkInProgressStateChanged;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -57,34 +51,6 @@
WorkInProgressOp create(boolean workInProgress, Input in);
}
- public static void checkPermissions(
- PermissionBackend permissionBackend, CurrentUser user, Change change)
- throws PermissionBackendException, AuthException {
- if (!user.isIdentifiedUser()) {
- throw new AuthException("Authentication required");
- }
-
- if (change.getOwner().equals(user.asIdentifiedUser().getAccountId())) {
- return;
- }
-
- try {
- permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
- return;
- } catch (AuthException e) {
- // Skip.
- }
-
- try {
- permissionBackend
- .user(user)
- .project(change.getProject())
- .check(ProjectPermission.WRITE_CONFIG);
- } catch (AuthException exp) {
- throw new AuthException("not allowed to toggle work in progress");
- }
- }
-
private final ChangeMessagesUtil cmUtil;
private final EmailReviewComments.Factory email;
private final PatchSetUtil psUtil;
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 250ca03..309bf51 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -176,6 +176,14 @@
return refControl.canForceEditTopicName();
}
+ /** Can this user toggle WorkInProgress state? */
+ private boolean canToggleWorkInProgressState() {
+ return isOwner()
+ || getProjectControl().isOwner()
+ || refControl.canPerform(Permission.TOGGLE_WORK_IN_PROGRESS_STATE)
+ || getProjectControl().isAdmin();
+ }
+
/** Can this user edit the description? */
private boolean canEditDescription() {
if (getChange().isNew()) {
@@ -299,6 +307,8 @@
return canRestore();
case SUBMIT:
return refControl.canSubmit(isOwner());
+ case TOGGLE_WORK_IN_PROGRESS_STATE:
+ return canToggleWorkInProgressState();
case REMOVE_REVIEWER:
case SUBMIT_AS:
diff --git a/java/com/google/gerrit/server/permissions/ChangePermission.java b/java/com/google/gerrit/server/permissions/ChangePermission.java
index ca1c460..2fba4ef 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -55,7 +55,8 @@
*/
REBASE,
SUBMIT,
- SUBMIT_AS("submit on behalf of other users");
+ SUBMIT_AS("submit on behalf of other users"),
+ TOGGLE_WORK_IN_PROGRESS_STATE;
private final String description;
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index ece29df..d56bc78 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -97,6 +97,9 @@
.put(ChangePermission.REBASE, Permission.REBASE)
.put(ChangePermission.SUBMIT, Permission.SUBMIT)
.put(ChangePermission.SUBMIT_AS, Permission.SUBMIT_AS)
+ .put(
+ ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE)
.build();
private static <T extends Enum<T>> void checkMapContainsAllEnumValues(
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 8afaa45..ff27bfa 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -340,8 +340,10 @@
return Response.withStatusCode(SC_BAD_REQUEST, output);
}
- WorkInProgressOp.checkPermissions(
- permissionBackend, revision.getUser(), revision.getChange());
+ revision
+ .getChangeResource()
+ .permissions()
+ .check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
if (input.ready) {
output.ready = true;
diff --git a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
index 94914b0..aacf58b 100644
--- a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
+++ b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -25,48 +24,36 @@
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.change.WorkInProgressOp.Input;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
@Singleton
public class SetReadyForReview extends RetryingRestModifyView<ChangeResource, Input, Response<?>>
implements UiAction<ChangeResource> {
private final WorkInProgressOp.Factory opFactory;
- private final PermissionBackend permissionBackend;
- private final Provider<CurrentUser> user;
@Inject
- SetReadyForReview(
- RetryHelper retryHelper,
- WorkInProgressOp.Factory opFactory,
- PermissionBackend permissionBackend,
- Provider<CurrentUser> user) {
+ SetReadyForReview(RetryHelper retryHelper, WorkInProgressOp.Factory opFactory) {
super(retryHelper);
this.opFactory = opFactory;
- this.permissionBackend = permissionBackend;
- this.user = user;
}
@Override
protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException {
- WorkInProgressOp.checkPermissions(permissionBackend, user.get(), rsrc.getChange());
+ rsrc.permissions().check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
Change change = rsrc.getChange();
if (!change.isNew()) {
@@ -94,15 +81,6 @@
.setVisible(
and(
rsrc.getChange().isNew() && rsrc.getChange().isWorkInProgress(),
- or(
- rsrc.isUserOwner(),
- or(
- permissionBackend
- .currentUser()
- .testCond(GlobalPermission.ADMINISTRATE_SERVER),
- permissionBackend
- .currentUser()
- .project(rsrc.getProject())
- .testCond(ProjectPermission.WRITE_CONFIG)))));
+ rsrc.permissions().testCond(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE)));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java b/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
index d087e47..852813e 100644
--- a/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
+++ b/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -25,48 +24,36 @@
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.change.WorkInProgressOp.Input;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
@Singleton
public class SetWorkInProgress extends RetryingRestModifyView<ChangeResource, Input, Response<?>>
implements UiAction<ChangeResource> {
private final WorkInProgressOp.Factory opFactory;
- private final PermissionBackend permissionBackend;
- private final Provider<CurrentUser> user;
@Inject
- SetWorkInProgress(
- WorkInProgressOp.Factory opFactory,
- RetryHelper retryHelper,
- PermissionBackend permissionBackend,
- Provider<CurrentUser> user) {
+ SetWorkInProgress(WorkInProgressOp.Factory opFactory, RetryHelper retryHelper) {
super(retryHelper);
this.opFactory = opFactory;
- this.permissionBackend = permissionBackend;
- this.user = user;
}
@Override
protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException {
- WorkInProgressOp.checkPermissions(permissionBackend, user.get(), rsrc.getChange());
+ rsrc.permissions().check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
Change change = rsrc.getChange();
if (!change.isNew()) {
@@ -94,15 +81,6 @@
.setVisible(
and(
rsrc.getChange().isNew() && !rsrc.getChange().isWorkInProgress(),
- or(
- rsrc.isUserOwner(),
- or(
- permissionBackend
- .currentUser()
- .testCond(GlobalPermission.ADMINISTRATE_SERVER),
- permissionBackend
- .currentUser()
- .project(rsrc.getProject())
- .testCond(ProjectPermission.WRITE_CONFIG)))));
+ rsrc.permissions().testCond(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE)));
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d3e41f7..c38f686 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -277,7 +277,7 @@
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(changeId).setWorkInProgress();
}
@@ -323,7 +323,7 @@
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(changeId).setReadyForReview();
}
@@ -500,6 +500,37 @@
}
@Test
+ public void toggleWorkInProgressStateByNonOwnerWithPermission() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String refactor = "Needs some refactoring";
+ String ptal = "PTAL";
+
+ grant(
+ project,
+ "refs/heads/master",
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ false,
+ REGISTERED_USERS);
+
+ requestScopeOperations.setApiUser(user.getId());
+ gApi.changes().id(changeId).setWorkInProgress(refactor);
+
+ ChangeInfo info = gApi.changes().id(changeId).get();
+
+ assertThat(info.workInProgress).isTrue();
+ assertThat(Iterables.getLast(info.messages).message).contains(refactor);
+ assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_WIP);
+
+ gApi.changes().id(changeId).setReadyForReview(ptal);
+
+ info = gApi.changes().id(changeId).get();
+ assertThat(info.workInProgress).isNull();
+ assertThat(Iterables.getLast(info.messages).message).contains(ptal);
+ assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_READY);
+ }
+
+ @Test
public void reviewAndStartReview() throws Exception {
PushOneCommit.Result r = createWorkInProgressChange();
r.assertOkStatus();
@@ -588,17 +619,33 @@
ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(r.getChangeId()).current().review(in);
}
@Test
+ public void reviewWithWorkInProgressByNonOwnerWithPermission() throws Exception {
+ PushOneCommit.Result r = createChange();
+ ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+ grant(
+ project,
+ "refs/heads/master",
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ false,
+ REGISTERED_USERS);
+ requestScopeOperations.setApiUser(user.getId());
+ gApi.changes().id(r.getChangeId()).current().review(in);
+ ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+ assertThat(info.workInProgress).isTrue();
+ }
+
+ @Test
public void reviewWithReadyByNonOwnerReturnsError() throws Exception {
PushOneCommit.Result r = createChange();
ReviewInput in = ReviewInput.noScore().setReady(true);
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(r.getChangeId()).current().review(in);
}
diff --git a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
index fb0c685..e949a87 100644
--- a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
+++ b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
@@ -120,6 +120,10 @@
id: 'submitAs',
name: 'Submit (On Behalf Of)',
},
+ toggleWipState: {
+ id: 'toggleWipState',
+ name: 'Toggle Work In Progress State',
+ },
viewDrafts: {
id: 'viewDrafts',
name: 'View Drafts',
