Merge "Allow to specify a message when the private flag on a change is set/unset"

Documentation/rest-api-changes.txt

@@ -2166,14 +2166,22 @@ if no review comment is added.
 [[mark-private]]
 === Mark Private
 --
-'PUT /changes/link:#change-id[\{change-id\}]/private'
+'POST /changes/link:#change-id[\{change-id\}]/private'
 --
 
 Marks the change to be private. Note users can only mark own changes as private.
 
+A message can be specified in the request body inside a
+link:#private-input[PrivateInput] entity.
+
 .Request
 ----
-  PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/private HTTP/1.0
+  POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/private HTTP/1.0
+  Content-Type: application/json; charset=UTF-8
+
+  {
+    "message": "After this security fix has been released we can make it public now."
+  }
 ----
 
 .Response
@@ -2192,9 +2200,17 @@ If the change was already private the response is "`200 OK`".
 Marks the change to be non-private. Note users can only unmark own private
 changes.
 
+A message can be specified in the request body inside a
+link:#private-input[PrivateInput] entity.
+
 .Request
 ----
   DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/private HTTP/1.0
+  Content-Type: application/json; charset=UTF-8
+
+  {
+    "message": "This is a security fix that must not be public."
+  }
 ----
 
 .Response
@@ -2204,6 +2220,20 @@ changes.
 
 If the change was already not private, the response is "`409 Conflict`".
 
+Please note that some proxies prohibit request bodies for DELETE
+requests. In this case, if you want to set a message options, use a
+POST request:
+
+.Request
+----
+  POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/private.delete HTTP/1.0
+  Content-Type: application/json; charset=UTF-8
+
+  {
+    "message": "This is a security fix that must not be public."
+  }
+----
+
 [[ignore]]
 === Ignore
 --
@@ -6221,6 +6251,17 @@ A list of link:rest-api-accounts.html#account-id[account IDs] that
 identify the accounts that should be should be notified.
 |=======================
 
+[[private-input]]
+=== PrivateInput
+The `PrivateInput` entity contains information for changing the private
+flag on a change.
+
+[options="header",cols="1,^1,5"]
+|=======================
+|Field Name||Description
+|`message` |optional|Message describing why the private flag was changed.
+|=======================
+
 [[problem-info]]
 === ProblemInfo
 The `ProblemInfo` entity contains a description of a potential consistency problem

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -191,17 +191,31 @@ public class ChangeIT extends AbstractDaemonTest {
     String changeId = result.getChangeId();
     assertThat(gApi.changes().id(changeId).get().isPrivate).isNull();
 
-    gApi.changes().id(changeId).setPrivate(true);
+    gApi.changes().id(changeId).setPrivate(true, null);
     ChangeInfo info = gApi.changes().id(changeId).get();
     assertThat(info.isPrivate).isTrue();
     assertThat(Iterables.getLast(info.messages).message).isEqualTo("Set private");
     assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_PRIVATE);
 
-    gApi.changes().id(changeId).setPrivate(false);
+    gApi.changes().id(changeId).setPrivate(false, null);
     info = gApi.changes().id(changeId).get();
     assertThat(info.isPrivate).isNull();
     assertThat(Iterables.getLast(info.messages).message).isEqualTo("Unset private");
     assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_UNSET_PRIVATE);
+
+    String msg = "This is a security fix that must not be public.";
+    gApi.changes().id(changeId).setPrivate(true, msg);
+    info = gApi.changes().id(changeId).get();
+    assertThat(info.isPrivate).isTrue();
+    assertThat(Iterables.getLast(info.messages).message).isEqualTo("Set private\n\n" + msg);
+    assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_PRIVATE);
+
+    msg = "After this security fix has been released we can make it public now.";
+    gApi.changes().id(changeId).setPrivate(false, msg);
+    info = gApi.changes().id(changeId).get();
+    assertThat(info.isPrivate).isNull();
+    assertThat(Iterables.getLast(info.messages).message).isEqualTo("Unset private\n\n" + msg);
+    assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_UNSET_PRIVATE);
   }
 
   @Test
@@ -213,7 +227,7 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(gApi.changes().id(result.getChangeId()).get().isPrivate).isNull();
     exception.expect(AuthException.class);
     exception.expectMessage("not allowed to mark private");
-    gApi.changes().id(result.getChangeId()).setPrivate(true);
+    gApi.changes().id(result.getChangeId()).setPrivate(true, null);
   }
 
   @Test
@@ -223,7 +237,7 @@ public class ChangeIT extends AbstractDaemonTest {
         pushFactory.create(db, user.getIdent(), userRepo).to("refs/for/master");
 
     setApiUser(user);
-    gApi.changes().id(result.getChangeId()).setPrivate(true);
+    gApi.changes().id(result.getChangeId()).setPrivate(true, null);
     // Owner can always access its private changes.
     assertThat(gApi.changes().id(result.getChangeId()).get().isPrivate).isTrue();
 
@@ -246,7 +260,7 @@ public class ChangeIT extends AbstractDaemonTest {
   @Test
   public void privateChangeOfOtherUserCanBeAccessedWithPermission() throws Exception {
     PushOneCommit.Result result = createChange();
-    gApi.changes().id(result.getChangeId()).setPrivate(true);
+    gApi.changes().id(result.getChangeId()).setPrivate(true, null);
 
     allow(Permission.VIEW_PRIVATE_CHANGES, REGISTERED_USERS, "refs/*");
     setApiUser(user);

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java

@@ -518,7 +518,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
           lsRemoteCommand.call().stream().map(Ref::getName).collect(toList());
       assertWithMessage("Precondition violated").that(initialRefNames).contains(change3RefName);
 
-      gApi.changes().id(c3.getId().get()).setPrivate(true);
+      gApi.changes().id(c3.getId().get()).setPrivate(true, null);
 
       List<String> refNames = lsRemoteCommand.call().stream().map(Ref::getName).collect(toList());
       assertThat(refNames).doesNotContain(change3RefName);
@@ -538,7 +538,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
           lsRemoteCommand.call().stream().map(Ref::getName).collect(toList());
       assertWithMessage("Precondition violated").that(initialRefNames).contains(change3RefName);
 
-      gApi.changes().id(c3.getId().get()).setPrivate(true);
+      gApi.changes().id(c3.getId().get()).setPrivate(true, null);
 
       List<String> refNames = lsRemoteCommand.call().stream().map(Ref::getName).collect(toList());
       assertThat(refNames).contains(change3RefName);

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.extensions.api.changes;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -85,7 +86,7 @@ public interface ChangeApi {
 
   void move(MoveInput in) throws RestApiException;
 
-  void setPrivate(boolean value) throws RestApiException;
+  void setPrivate(boolean value, @Nullable String message) throws RestApiException;
 
   void setWorkInProgress(String message) throws RestApiException;
 
@@ -335,7 +336,7 @@ public interface ChangeApi {
     }
 
     @Override
-    public void setPrivate(boolean value) {
+    public void setPrivate(boolean value, @Nullable String message) {
       throw new NotImplementedException();
     }
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java

@@ -122,11 +122,11 @@ public class ChangeApi {
   }
 
   public static void markPrivate(int id, AsyncCallback<JavaScriptObject> cb) {
-    change(id).view("private").put(cb);
+    change(id).view("private").post(PrivateInput.create(), cb);
   }
 
   public static void unmarkPrivate(int id, AsyncCallback<JavaScriptObject> cb) {
-    change(id).view("private").delete(cb);
+    change(id).view("private.delete").post(PrivateInput.create(), cb);
   }
 
   public static RestApi comments(int id) {
@@ -327,6 +327,16 @@ public class ChangeApi {
     protected CherryPickInput() {}
   }
 
+  private static class PrivateInput extends JavaScriptObject {
+    static PrivateInput create() {
+      return (PrivateInput) createObject();
+    }
+
+    final native void setMessage(String m) /*-{ this.message = m; }-*/;
+
+    protected PrivateInput() {}
+  }
+
   private static class RebaseInput extends JavaScriptObject {
     final native void setBase(String b) /*-{ this.base = b; }-*/;
 

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.api.changes;
 
 import static com.google.gerrit.server.api.ApiUtil.asRestApiException;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.api.changes.AbandonInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerResult;
@@ -66,16 +67,17 @@ import com.google.gerrit.server.change.ListChangeRobotComments;
 import com.google.gerrit.server.change.Move;
 import com.google.gerrit.server.change.Mute;
 import com.google.gerrit.server.change.PostHashtags;
+import com.google.gerrit.server.change.PostPrivate;
 import com.google.gerrit.server.change.PostReviewers;
 import com.google.gerrit.server.change.PublishDraftPatchSet;
 import com.google.gerrit.server.change.PutAssignee;
-import com.google.gerrit.server.change.PutPrivate;
 import com.google.gerrit.server.change.PutTopic;
 import com.google.gerrit.server.change.Rebase;
 import com.google.gerrit.server.change.Restore;
 import com.google.gerrit.server.change.Revert;
 import com.google.gerrit.server.change.Reviewers;
 import com.google.gerrit.server.change.Revisions;
+import com.google.gerrit.server.change.SetPrivateOp;
 import com.google.gerrit.server.change.SetReadyForReview;
 import com.google.gerrit.server.change.SetWorkInProgress;
 import com.google.gerrit.server.change.SubmittedTogether;
@@ -129,7 +131,7 @@ class ChangeApiImpl implements ChangeApi {
   private final Check check;
   private final Index index;
   private final Move move;
-  private final PutPrivate putPrivate;
+  private final PostPrivate postPrivate;
   private final DeletePrivate deletePrivate;
   private final Ignore ignore;
   private final Unignore unignore;
@@ -172,7 +174,7 @@ class ChangeApiImpl implements ChangeApi {
       Check check,
       Index index,
       Move move,
-      PutPrivate putPrivate,
+      PostPrivate postPrivate,
       DeletePrivate deletePrivate,
       Ignore ignore,
       Unignore unignore,
@@ -213,7 +215,7 @@ class ChangeApiImpl implements ChangeApi {
     this.check = check;
     this.index = index;
     this.move = move;
-    this.putPrivate = putPrivate;
+    this.postPrivate = postPrivate;
     this.deletePrivate = deletePrivate;
     this.ignore = ignore;
     this.unignore = unignore;
@@ -302,12 +304,13 @@ class ChangeApiImpl implements ChangeApi {
   }
 
   @Override
-  public void setPrivate(boolean value) throws RestApiException {
+  public void setPrivate(boolean value, @Nullable String message) throws RestApiException {
     try {
+      SetPrivateOp.Input input = new SetPrivateOp.Input(message);
       if (value) {
-        putPrivate.apply(change, null);
+        postPrivate.apply(change, input);
       } else {
-        deletePrivate.apply(change, null);
+        deletePrivate.apply(change, input);
       }
     } catch (Exception e) {
       throw asRestApiException("Cannot change private status", e);

gerrit-server/src/main/java/com/google/gerrit/server/change/DeletePrivate.java

@@ -33,10 +33,8 @@ import com.google.inject.Singleton;
 
 @Singleton
 public class DeletePrivate
-    extends RetryingRestModifyView<ChangeResource, DeletePrivate.Input, Response<String>>
+    extends RetryingRestModifyView<ChangeResource, SetPrivateOp.Input, Response<String>>
     implements UiAction<ChangeResource> {
-  public static class Input {}
-
   private final ChangeMessagesUtil cmUtil;
   private final Provider<ReviewDb> dbProvider;
 
@@ -49,7 +47,7 @@ public class DeletePrivate
 
   @Override
   protected Response<String> applyImpl(
-      BatchUpdate.Factory updateFactory, ChangeResource rsrc, DeletePrivate.Input input)
+      BatchUpdate.Factory updateFactory, ChangeResource rsrc, SetPrivateOp.Input input)
       throws RestApiException, UpdateException {
     if (!rsrc.isUserOwner()) {
       throw new AuthException("not allowed to unmark private");
@@ -60,7 +58,7 @@ public class DeletePrivate
     }
 
     ChangeControl control = rsrc.getControl();
-    SetPrivateOp op = new SetPrivateOp(cmUtil, false);
+    SetPrivateOp op = new SetPrivateOp(cmUtil, false, input);
     try (BatchUpdate u =
         updateFactory.create(
             dbProvider.get(),

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -85,7 +85,8 @@ public class Module extends RestApiModule {
     post(CHANGE_KIND, "index").to(Index.class);
     post(CHANGE_KIND, "rebuild.notedb").to(Rebuild.class);
     post(CHANGE_KIND, "move").to(Move.class);
-    put(CHANGE_KIND, "private").to(PutPrivate.class);
+    post(CHANGE_KIND, "private").to(PostPrivate.class);
+    post(CHANGE_KIND, "private.delete").to(DeletePrivate.class);
     delete(CHANGE_KIND, "private").to(DeletePrivate.class);
     put(CHANGE_KIND, "ignore").to(Ignore.class);
     put(CHANGE_KIND, "unignore").to(Unignore.class);

gerrit-server/src/main/java/com/google/gerrit/server/change/PostPrivate.java

@@ -32,24 +32,24 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
 @Singleton
-public class PutPrivate
-    extends RetryingRestModifyView<ChangeResource, PutPrivate.Input, Response<String>>
+public class PostPrivate
+    extends RetryingRestModifyView<ChangeResource, SetPrivateOp.Input, Response<String>>
     implements UiAction<ChangeResource> {
-  public static class Input {}
-
   private final ChangeMessagesUtil cmUtil;
   private final Provider<ReviewDb> dbProvider;
 
   @Inject
-  PutPrivate(Provider<ReviewDb> dbProvider, RetryHelper retryHelper, ChangeMessagesUtil cmUtil) {
+  PostPrivate(
+      Provider<ReviewDb> dbProvider,
+      RetryHelper retryHelper,
+      ChangeMessagesUtil cmUtil) {
     super(retryHelper);
     this.dbProvider = dbProvider;
     this.cmUtil = cmUtil;
   }
 
   @Override
-  protected Response<String> applyImpl(
-      BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
+  public Response<String> applyImpl(BatchUpdate.Factory updateFactory, ChangeResource rsrc, SetPrivateOp.Input input)
       throws RestApiException, UpdateException {
     if (!rsrc.isUserOwner()) {
       throw new AuthException("not allowed to mark private");
@@ -60,7 +60,7 @@ public class PutPrivate
     }
 
     ChangeControl control = rsrc.getControl();
-    SetPrivateOp op = new SetPrivateOp(cmUtil, true);
+    SetPrivateOp op = new SetPrivateOp(cmUtil, true, input);
     try (BatchUpdate u =
         updateFactory.create(
             dbProvider.get(),

gerrit-server/src/main/java/com/google/gerrit/server/change/SetPrivateOp.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.common.base.Strings;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -23,13 +24,25 @@ import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gwtorm.server.OrmException;
 
-class SetPrivateOp implements BatchUpdateOp {
+public class SetPrivateOp implements BatchUpdateOp {
+  public static class Input {
+    String message;
+
+    public Input() {}
+
+    public Input(String message) {
+      this.message = message;
+    }
+  }
+
   private final ChangeMessagesUtil cmUtil;
   private final boolean isPrivate;
+  private final Input input;
 
-  SetPrivateOp(ChangeMessagesUtil cmUtil, boolean isPrivate) {
+  SetPrivateOp(ChangeMessagesUtil cmUtil, boolean isPrivate, Input input) {
     this.cmUtil = cmUtil;
     this.isPrivate = isPrivate;
+    this.input = input;
   }
 
   @Override
@@ -48,10 +61,18 @@ class SetPrivateOp implements BatchUpdateOp {
 
   private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
     Change c = ctx.getChange();
+    StringBuilder buf = new StringBuilder(c.isPrivate() ? "Set private" : "Unset private");
+
+    String m = Strings.nullToEmpty(input.message).trim();
+    if (!m.isEmpty()) {
+      buf.append("\n\n");
+      buf.append(m);
+    }
+
     ChangeMessage cmsg =
         ChangeMessagesUtil.newMessage(
             ctx,
-            c.isPrivate() ? "Set private" : "Unset private",
+            buf.toString(),
             c.isPrivate()
                 ? ChangeMessagesUtil.TAG_SET_PRIVATE
                 : ChangeMessagesUtil.TAG_UNSET_PRIVATE);

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -399,7 +399,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery("is:open", change2, change1);
     assertQuery("is:private");
 
-    gApi.changes().id(change1.getChangeId()).setPrivate(true);
+    gApi.changes().id(change1.getChangeId()).setPrivate(true, null);
 
     // Change1 is not private, but should be still visible to its owner.
     assertQuery("is:open", change1, change2);