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

Documentation/intro-user.txt

@@ -548,7 +548,8 @@ request.
 ----
 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]]

java/com/google/gerrit/common/data/Permission.java

@@ -46,6 +46,7 @@ public class Permission implements Comparable<Permission> {
   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 @@ public class Permission implements Comparable<Permission> {
     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);

java/com/google/gerrit/server/change/WorkInProgressOp.java

@@ -18,20 +18,14 @@ import com.google.common.base.Strings;
 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 @@ public class WorkInProgressOp implements BatchUpdateOp {
     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;

java/com/google/gerrit/server/permissions/ChangeControl.java

@@ -176,6 +176,14 @@ class ChangeControl {
     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 @@ class ChangeControl {
             return canRestore();
           case SUBMIT:
             return refControl.canSubmit(isOwner());
+          case TOGGLE_WORK_IN_PROGRESS_STATE:
+            return canToggleWorkInProgressState();
 
           case REMOVE_REVIEWER:
           case SUBMIT_AS:

java/com/google/gerrit/server/permissions/ChangePermission.java

@@ -55,7 +55,8 @@ public enum ChangePermission implements ChangePermissionOrLabel {
    */
   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;
 

java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java

@@ -97,6 +97,9 @@ public class DefaultPermissionMappings {
           .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(

java/com/google/gerrit/server/restapi/change/PostReview.java

@@ -340,8 +340,10 @@ public class PostReview
           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;

java/com/google/gerrit/server/restapi/change/SetReadyForReview.java

@@ -16,7 +16,6 @@ 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.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.restapi.RestApiException;
 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 @@ public class SetReadyForReview extends RetryingRestModifyView<ChangeResource, In
         .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)));
   }
 }

java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java

@@ -16,7 +16,6 @@ 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.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.restapi.RestApiException;
 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 @@ public class SetWorkInProgress extends RetryingRestModifyView<ChangeResource, In
         .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)));
   }
 }

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -277,7 +277,7 @@ public class ChangeIT extends AbstractDaemonTest {
 
     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 @@ public class ChangeIT extends AbstractDaemonTest {
 
     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();
   }
 
@@ -499,6 +499,37 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_READY);
   }
 
+  @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();
@@ -588,17 +619,33 @@ public class ChangeIT extends AbstractDaemonTest {
     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);
   }
 

polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html

@@ -120,6 +120,10 @@ limitations under the License.
             id: 'submitAs',
             name: 'Submit (On Behalf Of)',
           },
+          toggleWipState: {
+            id: 'toggleWipState',
+            name: 'Toggle Work In Progress State',
+          },
           viewDrafts: {
             id: 'viewDrafts',
             name: 'View Drafts',