Merge changes from topic 'impersonation-2'

* changes:
  PostReview: Disallow modifying other people's drafts
  More consistency in on_behalf_of implementation
  Expand on_behalf_of tests

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

@@ -15,50 +15,65 @@
 package com.google.gerrit.acceptance.rest.account;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.server.group.SystemGroupBackend.CHANGE_OWNER;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
+import com.google.gerrit.extensions.api.groups.GroupInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.client.Side;
 import com.google.gerrit.extensions.common.ApprovalInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.GroupInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Patch;
+import com.google.gerrit.server.account.AccountControl;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.project.Util;
+import com.google.inject.Inject;
 
 import org.junit.Before;
 import org.junit.Test;
 
 public class ImpersonationIT extends AbstractDaemonTest {
+  @Inject
+  private AccountControl.Factory accountControlFactory;
+
   private TestAccount admin2;
+  private GroupInfo newGroup;
 
   @Before
   public void setUp() throws Exception {
     admin2 = accounts.admin2();
+    GroupInput gi = new GroupInput();
+    gi.name = name("New-Group");
+    gi.members = ImmutableList.of(user.id.toString());
+    newGroup = gApi.groups().create(gi).get();
   }
 
   @Test
   public void voteOnBehalfOf() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
-    LabelType codeReviewType = Util.codeReview();
-    String forCodeReviewAs = Permission.forLabelAs(codeReviewType.getName());
-    String heads = "refs/heads/*";
-    AccountGroup.UUID owner =
-        SystemGroupBackend.getGroup(CHANGE_OWNER).getUUID();
-    Util.allow(cfg, forCodeReviewAs, -1, 1, owner, heads);
-    saveProjectConfig(project, cfg);
-
+    allowCodeReviewOnBehalfOf();
     PushOneCommit.Result r = createChange();
     RevisionApi revision = gApi.changes()
         .id(r.getChangeId())
@@ -66,6 +81,7 @@ public class ImpersonationIT extends AbstractDaemonTest {
 
     ReviewInput in = ReviewInput.recommend();
     in.onBehalfOf = user.id.toString();
+    in.message = "Message on behalf of";
     revision.review(in);
 
     ChangeInfo c = gApi.changes()
@@ -77,15 +93,196 @@ public class ImpersonationIT extends AbstractDaemonTest {
     ApprovalInfo approval = codeReview.all.get(0);
     assertThat(approval._accountId).isEqualTo(user.id.get());
     assertThat(approval.value).isEqualTo(1);
+
+    ChangeMessageInfo m = Iterables.getLast(c.messages);
+    assertThat(m.message).endsWith(in.message);
+    assertThat(m.author._accountId).isEqualTo(user.id.get());
   }
 
-  private void allowSubmitOnBehalfOf() throws Exception {
+  @Test
+  public void voteOnBehalfOfRequiresLabel() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.message = "Message on behalf of";
+
+    exception.expect(AuthException.class);
+    exception.expectMessage(
+        "label required to post review on behalf of \"" + in.onBehalfOf + '"');
+    revision.review(in);
+  }
+
+  @Test
+  public void voteOnBehalfOfInvalidLabel() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.strictLabels = true;
+    in.label("Not-A-Label", 5);
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage(
+        "label \"Not-A-Label\" is not a configured label");
+    revision.review(in);
+  }
+
+  @Test
+  public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels()
+      throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.strictLabels = false;
+    in.label("Code-Review", 1);
+    in.label("Not-A-Label", 5);
+
+    revision.review(in);
+
+    assertThat(gApi.changes().id(r.getChangeId()).get().labels)
+        .doesNotContainKey("Not-A-Label");
+  }
+
+  @Test
+  public void voteOnBehalfOfLabelNotPermitted() throws Exception {
     ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
-    Util.allow(cfg,
-        Permission.SUBMIT_AS,
-        SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID(),
-        "refs/heads/*");
+    LabelType verified = Util.verified();
+    cfg.getLabelSections().put(verified.getName(), verified);
     saveProjectConfig(project, cfg);
+
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.label("Verified", 1);
+
+    exception.expect(AuthException.class);
+    exception.expectMessage(
+        "not permitted to modify label \"Verified\" on behalf of \""
+            + in.onBehalfOf + '"');
+    revision.review(in);
+  }
+
+  @Test
+  public void voteOnBehalfOfWithComment() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.label("Code-Review", 1);
+    CommentInput ci = new CommentInput();
+    ci.path = Patch.COMMIT_MSG;
+    ci.side = Side.REVISION;
+    ci.line = 1;
+    ci.message = "message";
+    in.comments = ImmutableMap.of(ci.path, ImmutableList.of(ci));
+
+    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(ci.message);
+  }
+
+  @Test
+  public void voteOnBehalfOfCannotModifyDrafts() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+
+    setApiUser(user);
+    DraftInput di = new DraftInput();
+    di.path = Patch.COMMIT_MSG;
+    di.side = Side.REVISION;
+    di.line = 1;
+    di.message = "message";
+    gApi.changes().id(r.getChangeId()).current().createDraft(di);
+
+    setApiUser(admin);
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.label("Code-Review", 1);
+    in.drafts = DraftHandling.PUBLISH;
+
+    exception.expect(AuthException.class);
+    exception.expectMessage("not allowed to modify other user's drafts");
+    gApi.changes().id(r.getChangeId()).current().review(in);
+  }
+
+  @Test
+  public void voteOnBehalfOfMissingUser() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = "doesnotexist";
+    in.label("Code-Review", 1);
+
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage("Account Not Found: doesnotexist");
+    revision.review(in);
+  }
+
+  @Test
+  public void voteOnBehalfOfFailsWhenUserCannotSeeDestinationRef()
+      throws Exception {
+    blockRead(newGroup);
+
+    allowCodeReviewOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.label("Code-Review", 1);
+
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage(
+        "on_behalf_of account " + user.id + " cannot see destination ref");
+    revision.review(in);
+  }
+
+  @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+  @Test
+  public void voteOnBehalfOfInvisibleUserNotAllowed() throws Exception {
+    allowCodeReviewOnBehalfOf();
+    setApiUser(accounts.user2());
+    assertThat(accountControlFactory.get().canSee(user.id)).isFalse();
+
+    PushOneCommit.Result r = createChange();
+    RevisionApi revision = gApi.changes()
+        .id(r.getChangeId())
+        .current();
+
+    ReviewInput in = new ReviewInput();
+    in.onBehalfOf = user.id.toString();
+    in.label("Code-Review", 1);
+
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage("Account Not Found: " + in.onBehalfOf);
+    revision.review(in);
   }
 
   @Test
@@ -142,4 +339,81 @@ public class ImpersonationIT extends AbstractDaemonTest {
         .current()
         .submit(in);
   }
+
+  @Test
+  public void submitOnBehalfOfFailsWhenUserCannotSeeDestinationRef()
+      throws Exception {
+    blockRead(newGroup);
+
+    allowSubmitOnBehalfOf();
+    PushOneCommit.Result r = createChange();
+    String changeId = project.get() + "~master~" + r.getChangeId();
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .review(ReviewInput.approve());
+    SubmitInput in = new SubmitInput();
+    in.onBehalfOf = user.email;
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage(
+        "on_behalf_of account " + user.id + " cannot see destination ref");
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .submit(in);
+  }
+
+  @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+  @Test
+  public void submitOnBehalfOfInvisibleUserNotAllowed() throws Exception {
+    allowSubmitOnBehalfOf();
+    setApiUser(accounts.user2());
+    assertThat(accountControlFactory.get().canSee(user.id)).isFalse();
+
+    PushOneCommit.Result r = createChange();
+    String changeId = project.get() + "~master~" + r.getChangeId();
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .review(ReviewInput.approve());
+    SubmitInput in = new SubmitInput();
+    in.onBehalfOf = user.email;
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage("Account Not Found: " + in.onBehalfOf);
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .submit(in);
+  }
+
+  private void allowCodeReviewOnBehalfOf() throws Exception {
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    LabelType codeReviewType = Util.codeReview();
+    String forCodeReviewAs = Permission.forLabelAs(codeReviewType.getName());
+    String heads = "refs/heads/*";
+    AccountGroup.UUID uuid =
+        SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+    Util.allow(cfg, forCodeReviewAs, -1, 1, uuid, heads);
+    saveProjectConfig(project, cfg);
+  }
+
+  private void allowSubmitOnBehalfOf() throws Exception {
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    String heads = "refs/heads/*";
+    AccountGroup.UUID uuid =
+        SystemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+    Util.allow(cfg, Permission.SUBMIT_AS, uuid, heads);
+    Util.allow(cfg, Permission.SUBMIT, uuid, heads);
+    LabelType codeReviewType = Util.codeReview();
+    Util.allow(cfg, Permission.forLabel(codeReviewType.getName()),
+        -2, 2, uuid, heads);
+    saveProjectConfig(project, cfg);
+  }
+
+  private void blockRead(GroupInfo group) throws Exception {
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    Util.block(
+        cfg, Permission.READ, new AccountGroup.UUID(group.id), "refs/heads/master");
+    saveProjectConfig(project, cfg);
+  }
 }

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();
@@ -276,6 +284,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     }
 
     ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
+    if (!target.getRefControl().isVisible()) {
+      throw new UnprocessableEntityException(String.format(
+          "on_behalf_of account %s cannot see destination ref",
+          target.getUser().getAccountId()));
+    }
     return new RevisionResource(changes.parse(target), rev.getPatchSet());
   }
 
@@ -540,7 +553,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         }
       }
 
-      switch (firstNonNull(in.drafts, DraftHandling.DELETE)) {
+      switch (in.drafts) {
         case KEEP:
         default:
           break;

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

@@ -485,16 +485,11 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     if (!caller.canSubmitAs()) {
       throw new AuthException("submit on behalf of not permitted");
     }
-    IdentifiedUser targetUser = accounts.parseId(in.onBehalfOf);
-    if (targetUser == null) {
-      throw new UnprocessableEntityException(String.format(
-          "Account Not Found: %s", in.onBehalfOf));
-    }
-    ChangeControl target = caller.forUser(targetUser);
+    ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
     if (!target.getRefControl().isVisible()) {
       throw new UnprocessableEntityException(String.format(
           "on_behalf_of account %s cannot see destination ref",
-          targetUser.getAccountId()));
+          target.getUser().getAccountId()));
     }
     return new RevisionResource(changes.parse(target), rsrc.getPatchSet());
   }