Merge "Allow administrators to mark/unmark merged changes as private"

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

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<ChangeResource, SetPrivateOp.Input, Response<String>> {
   private final ChangeMessagesUtil cmUtil;
   private final Provider<ReviewDb> dbProvider;
+  private final PermissionBackend permissionBackend;
 
   @Inject
-  DeletePrivate(Provider<ReviewDb> dbProvider, RetryHelper retryHelper, ChangeMessagesUtil cmUtil) {
+  DeletePrivate(
+      Provider<ReviewDb> dbProvider,
+      RetryHelper retryHelper,
+      ChangeMessagesUtil cmUtil,
+      PermissionBackend permissionBackend) {
     super(retryHelper);
     this.dbProvider = dbProvider;
     this.cmUtil = cmUtil;
+    this.permissionBackend = permissionBackend;
   }
 
   @Override
   protected Response<String> 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<ChangeResource> {
-    @Inject
-    DeletePrivateByPost(
-        Provider<ReviewDb> 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);
   }
 }

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<ChangeResource> {
+  @Inject
+  DeletePrivateByPost(
+      Provider<ReviewDb> 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));
+  }
+}

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;
 

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

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