PostReview: Fix permission check for setting wip/ready
The work-in-progress flag on a change can be toggled by the change
owner, the project owner and admins.
The SetWorkInProgress and SetReadyForReview REST endpoints properly
checked the permissions and also the corresponding UI actions were
enabled/disabled based on these permission. However when the
work-in-progress flag was toggled from PostReview the request was denied
if the user was a non-change-owner. As result of this project owners and
admins could see the action in PolyGerrit but clicking on it failed.
To fix this the permission checks are now implemented by a static method
in WorkInProgressOp that can be reused from all the places.
This changes the response code for PostReview if a user is not allowed
to toggle the work-in-progress flag to 403 Forbidden. Previously it was
returning 400 Bad Request which was wrong since 400 Bad Request should
only be returned if the request has a malformed syntax, required input
fields are missing or mutally exclusive input fields are used together,
but not if permissions are missing.
Bug: Issue 9787
Change-Id: Ica169002e36c1d8070c403a3d7103a6f70060c28
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index da8f4a7..a8199ac 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -393,7 +393,7 @@
setApiUser(user);
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to set work in progress");
+ exception.expectMessage("not allowed to toggle work in progress");
gApi.changes().id(changeId).setWorkInProgress();
}
@@ -439,7 +439,7 @@
setApiUser(user);
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to set ready for review");
+ exception.expectMessage("not allowed to toggle work in progress");
gApi.changes().id(changeId).setReadyForReview();
}
@@ -651,12 +651,53 @@
}
@Test
+ @TestProjectInput(cloneAs = "user")
+ public void reviewWithWorkInProgressChangeOwner() throws Exception {
+ PushOneCommit push = pushFactory.create(db, user.getIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+ assertThat(r.getChange().change().getOwner()).isEqualTo(user.id);
+
+ setApiUser(user);
+ ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+ gApi.changes().id(r.getChangeId()).current().review(in);
+ ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+ assertThat(info.workInProgress).isTrue();
+ }
+
+ @Test
+ @TestProjectInput(cloneAs = "user")
+ public void reviewWithWithWorkInProgressAdmin() throws Exception {
+ PushOneCommit push = pushFactory.create(db, user.getIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+ assertThat(r.getChange().change().getOwner()).isEqualTo(user.id);
+
+ setApiUser(admin);
+ ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+ gApi.changes().id(r.getChangeId()).current().review(in);
+ ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+ assertThat(info.workInProgress).isTrue();
+ }
+
+ @Test
public void reviewWithWorkInProgressByNonOwnerReturnsError() throws Exception {
PushOneCommit.Result r = createChange();
ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
setApiUser(user);
- ReviewResult result = gApi.changes().id(r.getChangeId()).revision("current").review(in);
- assertThat(result.error).isEqualTo(PostReview.ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS);
+ exception.expect(AuthException.class);
+ exception.expectMessage("not allowed to toggle work in progress");
+ gApi.changes().id(r.getChangeId()).current().review(in);
+ }
+
+ @Test
+ public void reviewWithReadyByNonOwnerReturnsError() throws Exception {
+ PushOneCommit.Result r = createChange();
+ ReviewInput in = ReviewInput.noScore().setReady(true);
+ setApiUser(user);
+ exception.expect(AuthException.class);
+ exception.expectMessage("not allowed to toggle work in progress");
+ gApi.changes().id(r.getChangeId()).current().review(in);
}
@Test
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 2ccbe30..8c32039 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -102,7 +102,9 @@
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
@@ -140,8 +142,6 @@
public class PostReview
extends RetryingRestModifyView<RevisionResource, ReviewInput, Response<ReviewResult>> {
public static final String ERROR_ADDING_REVIEWER = "error adding reviewer";
- public static final String ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS =
- "only change owner can specify work_in_progress or ready";
public static final String ERROR_WIP_READY_MUTUALLY_EXCLUSIVE =
"work_in_progress and ready are mutually exclusive";
@@ -167,6 +167,8 @@
private final Config gerritConfig;
private final WorkInProgressOp.Factory workInProgressOpFactory;
private final ProjectCache projectCache;
+ private final PermissionBackend permissionBackend;
+ private final ProjectControl.GenericFactory projectControlFactory;
private final boolean strictLabels;
@Inject
@@ -188,7 +190,9 @@
NotifyUtil notifyUtil,
@GerritServerConfig Config gerritConfig,
WorkInProgressOp.Factory workInProgressOpFactory,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ PermissionBackend permissionBackend,
+ ProjectControl.GenericFactory projectControlFactory) {
super(retryHelper);
this.db = db;
this.changeResourceFactory = changeResourceFactory;
@@ -207,6 +211,8 @@
this.gerritConfig = gerritConfig;
this.workInProgressOpFactory = workInProgressOpFactory;
this.projectCache = projectCache;
+ this.permissionBackend = permissionBackend;
+ this.projectControlFactory = projectControlFactory;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
}
@@ -214,14 +220,16 @@
protected Response<ReviewResult> applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input)
throws RestApiException, UpdateException, OrmException, IOException,
- PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException {
+ PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException,
+ NoSuchProjectException {
return apply(updateFactory, revision, input, TimeUtil.nowTs());
}
public Response<ReviewResult> apply(
BatchUpdate.Factory updateFactory, RevisionResource revision, ReviewInput input, Timestamp ts)
throws RestApiException, UpdateException, OrmException, IOException,
- PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException {
+ PermissionBackendException, ConfigInvalidException, PatchListNotAvailableException,
+ NoSuchProjectException {
// Respect timestamp, but truncate at change created-on time.
ts = Ordering.natural().max(ts, revision.getChange().getCreatedOn());
if (revision.getEdit().isPresent()) {
@@ -342,10 +350,13 @@
output.error = ERROR_WIP_READY_MUTUALLY_EXCLUSIVE;
return Response.withStatusCode(SC_BAD_REQUEST, output);
}
- if (!revision.getChange().getOwner().equals(revision.getUser().getAccountId())) {
- output.error = ERROR_ONLY_OWNER_CAN_MODIFY_WORK_IN_PROGRESS;
- return Response.withStatusCode(SC_BAD_REQUEST, output);
- }
+
+ WorkInProgressOp.checkPermissions(
+ permissionBackend,
+ revision.getUser(),
+ revision.getChange(),
+ projectControlFactory.controlFor(revision.getProject(), revision.getUser()));
+
if (input.ready) {
output.ready = true;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
index e62b89d..ca89cc9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetReadyForReview.java
@@ -77,6 +77,11 @@
throws RestApiException, UpdateException, PermissionBackendException, NoSuchProjectException,
IOException {
Change change = rsrc.getChange();
+ WorkInProgressOp.checkPermissions(
+ permissionBackend,
+ self.get(),
+ change,
+ projectControlFactory.controlFor(rsrc.getProject(), rsrc.getUser()));
if (!rsrc.isUserOwner()
&& !permissionBackend.user(self).test(GlobalPermission.ADMINISTRATE_SERVER)
&& !projectControlFactory.controlFor(rsrc.getProject(), rsrc.getUser()).isOwner()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetWorkInProgress.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetWorkInProgress.java
index 9f3b3d9..bd412d7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetWorkInProgress.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetWorkInProgress.java
@@ -18,7 +18,6 @@
import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import com.google.gerrit.common.TimeUtil;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -77,11 +76,11 @@
throws RestApiException, UpdateException, PermissionBackendException, NoSuchProjectException,
IOException {
Change change = rsrc.getChange();
- if (!rsrc.isUserOwner()
- && !permissionBackend.user(rsrc.getUser()).test(GlobalPermission.ADMINISTRATE_SERVER)
- && !projectControlFactory.controlFor(rsrc.getProject(), rsrc.getUser()).isOwner()) {
- throw new AuthException("not allowed to set work in progress");
- }
+ WorkInProgressOp.checkPermissions(
+ permissionBackend,
+ self.get(),
+ change,
+ projectControlFactory.controlFor(rsrc.getProject(), rsrc.getUser()));
if (change.getStatus() != Status.NEW) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java
index bd4b2cd..28226a7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -20,14 +20,20 @@
import com.google.common.collect.ImmutableListMultimap;
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.project.ProjectControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -53,6 +59,30 @@
WorkInProgressOp create(boolean workInProgress, Input in);
}
+ public static void checkPermissions(
+ PermissionBackend permissionBackend,
+ CurrentUser user,
+ Change change,
+ ProjectControl projectControl)
+ throws PermissionBackendException, AuthException {
+ if (!user.isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
+ if (change.getOwner().equals(user.asIdentifiedUser().getAccountId())) {
+ return;
+ }
+
+ try {
+ permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
+ return;
+ } catch (AuthException e) {
+ // Skip.
+ }
+ if (!projectControl.isOwner()) {
+ throw new AuthException("not allowed to toggle work in progress");
+ }
+ }
+
private final ChangeMessagesUtil cmUtil;
private final EmailReviewComments.Factory email;
private final PatchSetUtil psUtil;
