PostReview: Disallow modifying other people's drafts

Having permission to review on behalf of another user should not give
you permission to publish or delete their draft comments, as they were
not visible to you. And it's just not very nice.

Change-Id: I76247a055d43a6d68d16cba871f262a07063d9af

Documentation/rest-api-changes.txt

@@ -5579,7 +5579,9 @@ input. +
 Allowed values are `DELETE`, `PUBLISH`, `PUBLISH_ALL_REVISIONS` and
 `KEEP`. All values except `PUBLISH_ALL_REVISIONS` operate only on drafts
 for a single revision. +
-If not set, the default is `DELETE`.
+Only `KEEP` is allowed when used in conjunction with `on_behalf_of`. +
+If not set, the default is `DELETE`, unless `on_behalf_of` is set, in
+which case the default is `KEEP` and any other value is disallowed.
 |`notify`                 |optional|
 Notify handling that defines to whom email notifications should be sent
 after the review is stored. +

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java

@@ -203,7 +203,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void voteOnBehalfOfPublishesDrafts() throws Exception {
+  public void voteOnBehalfOfCannotModifyDrafts() throws Exception {
     allowCodeReviewOnBehalfOf();
     PushOneCommit.Result r = createChange();
 
@@ -221,16 +221,9 @@ public class ImpersonationIT extends AbstractDaemonTest {
     in.label("Code-Review", 1);
     in.drafts = DraftHandling.PUBLISH;
 
-    // TODO(dborowitz): This doesn't seem appropriate, disallow it.
+    exception.expect(AuthException.class);
+    exception.expectMessage("not allowed to modify other user's drafts");
     gApi.changes().id(r.getChangeId()).current().review(in);
-
-    CommentInfo c = Iterables.getOnlyElement(
-        gApi.changes().id(r.getChangeId()).current().commentsAsList());
-    assertThat(c.author._accountId).isEqualTo(user.id.get());
-    assertThat(c.message).isEqualTo(di.message);
-
-    setApiUser(user);
-    assertThat(gApi.changes().id(r.getChangeId()).current().drafts()).isEmpty();
   }
 
   @Test

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

@@ -49,8 +49,11 @@ public class ReviewInput {
   /**
    * How to process draft comments already in the database that were not also
    * described in this input request.
+   * <p>
+   * Defaults to DELETE, unless {@link #onBehalfOf} is set, in which case it
+   * defaults to KEEP and any other value is disallowed.
    */
-  public DraftHandling drafts = DraftHandling.DELETE;
+  public DraftHandling drafts;
 
   /** Who to send email notifications to after review is stored. */
   public NotifyHandling notify = NotifyHandling.ALL;

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

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
@@ -173,6 +172,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     }
     if (input.onBehalfOf != null) {
       revision = onBehalfOf(revision, input);
+    } else if (input.drafts == null) {
+      input.drafts = DraftHandling.DELETE;
     }
     if (input.labels != null) {
       checkLabels(revision, input.strictLabels, input.labels);
@@ -248,6 +249,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
           "label required to post review on behalf of \"%s\"",
           in.onBehalfOf));
     }
+    if (in.drafts == null) {
+      in.drafts = DraftHandling.KEEP;
+    }
+    if (in.drafts != DraftHandling.KEEP) {
+      throw new AuthException("not allowed to modify other user's drafts");
+    }
+
 
     ChangeControl caller = rev.getControl();
     Iterator<Map.Entry<String, Short>> itr = in.labels.entrySet().iterator();
@@ -545,7 +553,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         }
       }
 
-      switch (firstNonNull(in.drafts, DraftHandling.DELETE)) {
+      switch (in.drafts) {
         case KEEP:
         default:
           break;