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());