Allow to toggle WIP flag by project owners
This change allows site administrators to pass wip and ready flags on
git push for non own changes. REST API was extended to allow also site
administrators to toggle WIP flag in this change: 812bacc7b37, but the
magic branch options were not extended to support new ACLs check.
In addition this change also allows project owners to toggle WIP flag.
New ACL check is added to both REST API and git push options.
Feature: Issue 7805
Change-Id: I259a170ed94375513741ea0123048437aefdca72
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index bcdc7d7..38af68e 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -550,6 +550,9 @@
----
Alternatively, click *Ready* from the Change screen.
+Only change owners, project owners and site administrators can mark changes as
+`work-in-progress` and `ready`.
+
[[wip-polygerrit]]
In the new PolyGerrit UI, you can mark a change as WIP, by selecting *WIP* from
the *More* menu. The Change screen updates with a yellow header, indicating that
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 6f2b5cf..7089e24 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2180,7 +2180,8 @@
'POST /changes/link:#change-id[\{change-id\}]/wip'
--
-Marks the change as not ready for review yet.
+Marks the change as not ready for review yet. Changes may only be marked not
+ready by the owner, project owners or site administrators.
The request body does not need to include a
link:#work-in-progress-input[WorkInProgressInput] entity if no review comment
@@ -2208,7 +2209,8 @@
'POST /changes/link:#change-id[\{change-id\}]/ready'
--
-Marks the change as ready for review (set WIP property to false).
+Marks the change as ready for review (set WIP property to false). Changes may
+only be marked ready by the owner, project owners or site administrators.
Activates notifications of reviewer. The request body does not need
to include a link:#work-in-progress-input[WorkInProgressInput] entity
diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt
index a95f2bc..da21809 100644
--- a/Documentation/user-upload.txt
+++ b/Documentation/user-upload.txt
@@ -306,6 +306,9 @@
git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/master%ready
----
+Only change owners, project owners and site administrators can specify
+`work-in-progress` and `ready` options on push.
+
[[message]]
==== Message
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 20b741f..7c3f461 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
@@ -416,6 +416,19 @@
}
@Test
+ public void setWorkInProgressAllowedAsProjectOwner() throws Exception {
+ setApiUser(user);
+ String changeId =
+ gApi.changes().create(new ChangeInput(project.get(), "master", "Test Change")).get().id;
+
+ com.google.gerrit.acceptance.TestAccount user2 = accountCreator.user2();
+ grant(project, "refs/*", Permission.OWNER, false, REGISTERED_USERS);
+ setApiUser(user2);
+ gApi.changes().id(changeId).setWorkInProgress();
+ assertThat(gApi.changes().id(changeId).get().workInProgress).isTrue();
+ }
+
+ @Test
public void setReadyForReviewNotAllowedWithoutPermission() throws Exception {
PushOneCommit.Result rready = createChange();
String changeId = rready.getChangeId();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index d2b7adc..cf80cd5 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -610,11 +610,11 @@
assertThat(r.getChange().change().getOwner()).isEqualTo(user.id);
assertThat(r.getChange().change().isWorkInProgress()).isTrue();
- // Other user trying to move from WIP to ready should fail.
+ // Admin user trying to move from WIP to ready should succeed.
GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps");
testRepo.reset("ps");
- r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo);
- r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP);
+ r = amendChange(r.getChangeId(), "refs/for/master%ready", user, testRepo);
+ r.assertOkStatus();
// Other user trying to move from WIP to WIP should succeed.
r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo);
@@ -626,14 +626,29 @@
r.assertOkStatus();
assertThat(r.getChange().change().isWorkInProgress()).isFalse();
- // Other user trying to move from ready to WIP should fail.
+ // Admin user trying to move from ready to WIP should succeed.
GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps");
testRepo.reset("ps");
r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo);
- r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP);
+ r.assertOkStatus();
- // Other user trying to move from ready to ready should succeed.
- r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo);
+ // Other user trying to move from wip to wip should succeed.
+ r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo);
+ r.assertOkStatus();
+
+ // Non owner, non admin and non project owner cannot flip wip bit:
+ TestAccount user2 = accountCreator.user2();
+ grant(
+ project, "refs/*", Permission.FORGE_COMMITTER, false, SystemGroupBackend.REGISTERED_USERS);
+ TestRepository<?> user2Repo = cloneProject(project, user2);
+ GitUtil.fetch(user2Repo, r.getPatchSet().getRefName() + ":ps");
+ user2Repo.reset("ps");
+ r = amendChange(r.getChangeId(), "refs/for/master%ready", user2, user2Repo);
+ r.assertErrorStatus(ReceiveConstants.ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP);
+
+ // Project owner trying to move from WIP to ready should succeed.
+ allow("refs/*", Permission.OWNER, SystemGroupBackend.REGISTERED_USERS);
+ r = amendChange(r.getChangeId(), "refs/for/master%ready", user2, user2Repo);
r.assertOkStatus();
}
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 1129e50..f012182 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
@@ -32,6 +32,8 @@
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.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -39,6 +41,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.io.IOException;
@Singleton
public class SetWorkInProgress extends RetryingRestModifyView<ChangeResource, Input, Response<?>>
@@ -47,6 +50,7 @@
private final Provider<ReviewDb> db;
private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
+ private final ProjectControl.GenericFactory projectControlFactory;
@Inject
SetWorkInProgress(
@@ -54,21 +58,25 @@
RetryHelper retryHelper,
Provider<ReviewDb> db,
Provider<CurrentUser> self,
- PermissionBackend permissionBackend) {
+ PermissionBackend permissionBackend,
+ ProjectControl.GenericFactory projectControlFactory) {
super(retryHelper);
this.opFactory = opFactory;
this.db = db;
this.self = self;
this.permissionBackend = permissionBackend;
+ this.projectControlFactory = projectControlFactory;
}
@Override
protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
- throws RestApiException, UpdateException, PermissionBackendException {
+ throws RestApiException, UpdateException, PermissionBackendException, NoSuchProjectException,
+ IOException {
Change change = rsrc.getChange();
if (!rsrc.isUserOwner()
- && !permissionBackend.user(self).test(GlobalPermission.ADMINISTRATE_SERVER)) {
+ && !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");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 0afa3bd..085f94d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -22,7 +22,7 @@
import static com.google.gerrit.server.change.HashtagsUtil.cleanupHashtag;
import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
import static com.google.gerrit.server.git.receive.ReceiveConstants.COMMAND_REJECTION_MESSAGE_FOOTER;
-import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP;
+import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP;
import static com.google.gerrit.server.git.receive.ReceiveConstants.PUSH_OPTION_SKIP_VALIDATION;
import static com.google.gerrit.server.git.receive.ReceiveConstants.SAME_CHANGE_ID_IN_MULTIPLE_CHANGES;
import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN;
@@ -2439,8 +2439,10 @@
if (magicBranch != null
&& (magicBranch.workInProgress || magicBranch.ready)
&& magicBranch.workInProgress != change.isWorkInProgress()
- && !user.getAccountId().equals(change.getOwner())) {
- reject(inputCommand, ONLY_OWNER_CAN_MODIFY_WIP);
+ && (!user.getAccountId().equals(change.getOwner())
+ && !permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER)
+ && !projectControl.isOwner())) {
+ reject(inputCommand, ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP);
return false;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java
index 92723e0..b71f01e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java
@@ -20,8 +20,8 @@
public static final String PUSH_OPTION_SKIP_VALIDATION = "skip-validation";
@VisibleForTesting
- public static final String ONLY_OWNER_CAN_MODIFY_WIP =
- "only change owner can modify Work-in-Progress";
+ public static final String ONLY_CHANGE_OWNER_OR_PROJECT_OWNER_CAN_MODIFY_WIP =
+ "only change owner or project owner can modify Work-in-Progress";
static final String COMMAND_REJECTION_MESSAGE_FOOTER =
"Please read the documentation and contact an administrator\n"
