From d0441d1a719bf43f16495c6f882ebf99666449c3 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 15 May 2017 11:14:24 +0900 Subject: [PATCH] Allow administrators to mark/unmark merged changes as private The UI shows the "Unmark Private" button for merged changes, but then the operation fails because SetPrivateOp has a check for the change's status and rejects the operation when it's merged. Remove the check from SetPrivateOp and modify the existing checks in PostPrivate and DeletePrivate to allow administrators to perform the operation on merged changes. Split DeletePrivateByPost out to a separate class rather than being a static subclass of DeletePrivate. This allows to reuse the utility method canDeletePrivate, which was not possible from a static class. Bug: Issue 6211 Change-Id: I507f2d8b09dfe0a2726e0a7bede26a5be8cc7fc1 --- .../acceptance/api/change/ChangeIT.java | 58 +++++++++++++++++++ .../gerrit/server/change/DeletePrivate.java | 33 +++++------ .../server/change/DeletePrivateByPost.java | 44 ++++++++++++++ .../google/gerrit/server/change/Module.java | 1 - .../gerrit/server/change/PostPrivate.java | 8 +-- .../gerrit/server/change/SetPrivateOp.java | 3 - 6 files changed, 120 insertions(+), 27 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivateByPost.java 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 5d0f0baaa0..08b0f675bc 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 @@ -279,6 +279,64 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(gApi.changes().id(result.getChangeId()).get().isPrivate).isTrue(); } + @Test + public void administratorCanUnmarkPrivateAfterMerging() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + gApi.changes().id(changeId).setPrivate(true, null); + assertThat(gApi.changes().id(changeId).get().isPrivate).isTrue(); + merge(result); + gApi.changes().id(changeId).setPrivate(false, null); + assertThat(gApi.changes().id(changeId).get().isPrivate).isNull(); + } + + @Test + public void administratorCanMarkPrivateAfterMerging() throws Exception { + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + assertThat(gApi.changes().id(changeId).get().isPrivate).isNull(); + merge(result); + gApi.changes().id(changeId).setPrivate(true, null); + assertThat(gApi.changes().id(changeId).get().isPrivate).isTrue(); + } + + @Test + public void userCannotMarkPrivateAfterMerging() throws Exception { + TestRepository userRepo = cloneProject(project, user); + PushOneCommit.Result result = + pushFactory.create(db, user.getIdent(), userRepo).to("refs/for/master"); + + String changeId = result.getChangeId(); + assertThat(gApi.changes().id(changeId).get().isPrivate).isNull(); + + merge(result); + + setApiUser(user); + exception.expect(AuthException.class); + exception.expectMessage("not allowed to mark private"); + gApi.changes().id(changeId).setPrivate(true, null); + } + + @Test + public void userCannotUnmarkPrivateAfterMerging() throws Exception { + TestRepository userRepo = cloneProject(project, user); + PushOneCommit.Result result = + pushFactory.create(db, user.getIdent(), userRepo).to("refs/for/master"); + + String changeId = result.getChangeId(); + assertThat(gApi.changes().id(changeId).get().isPrivate).isNull(); + gApi.changes().id(changeId).addReviewer(admin.getId().toString()); + gApi.changes().id(changeId).setPrivate(true, null); + assertThat(gApi.changes().id(changeId).get().isPrivate).isTrue(); + + merge(result); + + setApiUser(user); + exception.expect(AuthException.class); + exception.expectMessage("not allowed to unmark private"); + gApi.changes().id(changeId).setPrivate(false, null); + } + @Test public void setWorkInProgressNotAllowedWithoutPermission() throws Exception { PushOneCommit.Result rwip = createChange(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivate.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivate.java index 7819a29f1f..71c940b8c3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivate.java @@ -19,9 +19,11 @@ 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; -import com.google.gerrit.extensions.webui.UiAction; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; @@ -36,19 +38,25 @@ public class DeletePrivate extends RetryingRestModifyView> { private final ChangeMessagesUtil cmUtil; private final Provider dbProvider; + private final PermissionBackend permissionBackend; @Inject - DeletePrivate(Provider dbProvider, RetryHelper retryHelper, ChangeMessagesUtil cmUtil) { + DeletePrivate( + Provider dbProvider, + RetryHelper retryHelper, + ChangeMessagesUtil cmUtil, + PermissionBackend permissionBackend) { super(retryHelper); this.dbProvider = dbProvider; this.cmUtil = cmUtil; + this.permissionBackend = permissionBackend; } @Override protected Response applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, SetPrivateOp.Input input) throws RestApiException, UpdateException { - if (!rsrc.isUserOwner()) { + if (!canDeletePrivate(rsrc)) { throw new AuthException("not allowed to unmark private"); } @@ -70,20 +78,9 @@ public class DeletePrivate return Response.none(); } - public static class DeletePrivateByPost extends DeletePrivate - implements UiAction { - @Inject - DeletePrivateByPost( - Provider dbProvider, RetryHelper retryHelper, ChangeMessagesUtil cmUtil) { - super(dbProvider, retryHelper, cmUtil); - } - - @Override - public Description getDescription(ChangeResource rsrc) { - return new UiAction.Description() - .setLabel("Unmark private") - .setTitle("Unmark change as private") - .setVisible(rsrc.getChange().isPrivate() && rsrc.isUserOwner()); - } + protected boolean canDeletePrivate(ChangeResource rsrc) { + PermissionBackend.WithUser user = permissionBackend.user(rsrc.getUser()); + return user.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER) + || (rsrc.isUserOwner() && rsrc.getChange().getStatus() != Change.Status.MERGED); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivateByPost.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivateByPost.java new file mode 100644 index 0000000000..9cf85d1d90 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivateByPost.java @@ -0,0 +1,44 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import com.google.gerrit.extensions.webui.UiAction; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.update.RetryHelper; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +@Singleton +public class DeletePrivateByPost extends DeletePrivate implements UiAction { + @Inject + DeletePrivateByPost( + Provider dbProvider, + RetryHelper retryHelper, + ChangeMessagesUtil cmUtil, + PermissionBackend permissionBackend) { + super(dbProvider, retryHelper, cmUtil, permissionBackend); + } + + @Override + public Description getDescription(ChangeResource rsrc) { + return new UiAction.Description() + .setLabel("Unmark private") + .setTitle("Unmark change as private") + .setVisible(rsrc.getChange().isPrivate() && canDeletePrivate(rsrc)); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index 43a487bd6d..db31c17bab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -28,7 +28,6 @@ import static com.google.gerrit.server.change.VoteResource.VOTE_KIND; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.RestApiModule; import com.google.gerrit.server.account.AccountLoader; -import com.google.gerrit.server.change.DeletePrivate.DeletePrivateByPost; import com.google.gerrit.server.change.Reviewed.DeleteReviewed; import com.google.gerrit.server.change.Reviewed.PutReviewed; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostPrivate.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostPrivate.java index a1e673ffa0..771e669442 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostPrivate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostPrivate.java @@ -85,14 +85,12 @@ public class PostPrivate return new UiAction.Description() .setLabel("Mark private") .setTitle("Mark change as private") - .setVisible( - !change.isPrivate() - && change.getStatus() != Change.Status.MERGED - && canSetPrivate(rsrc)); + .setVisible(!change.isPrivate() && canSetPrivate(rsrc)); } private boolean canSetPrivate(ChangeResource rsrc) { PermissionBackend.WithUser user = permissionBackend.user(rsrc.getUser()); - return rsrc.isUserOwner() || user.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER); + return user.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER) + || (rsrc.isUserOwner() && rsrc.getChange().getStatus() != Change.Status.MERGED); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetPrivateOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetPrivateOp.java index 7008ecab42..d0bb70b979 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SetPrivateOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SetPrivateOp.java @@ -48,9 +48,6 @@ public class SetPrivateOp implements BatchUpdateOp { @Override public boolean updateChange(ChangeContext ctx) throws ResourceConflictException, OrmException { Change change = ctx.getChange(); - if (change.getStatus() == Change.Status.MERGED) { - throw new ResourceConflictException("change is merged"); - } ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId()); change.setPrivate(isPrivate); change.setLastUpdatedOn(ctx.getWhen());