Expand on_behalf_of tests
Increase coverage of conditions involving restricted labels, invisible
users, etc. In a few cases this exposes inconsistencies between
PostReview and Submit.
Change-Id: Ifa69ad95c7ce07748fb72148d3602d802b46e583
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
index 8196885..540656f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java
+++ b/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 @@
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,210 @@
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 voteOnBehalfOfPublishesDrafts() 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;
+
+ // TODO(dborowitz): This doesn't seem appropriate, disallow it.
+ 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
+ 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 voteOnBehalfOfSucceedsWhenUserCannotSeeDestinationRef()
+ 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);
+
+ // TODO(dborowitz): Make this fail instead.
+ revision.review(in);
+ ChangeInfo c = gApi.changes()
+ .id(r.getChangeId())
+ .get();
+
+ LabelInfo codeReview = c.labels.get("Code-Review");
+ assertThat(codeReview.all).hasSize(1);
+ ApprovalInfo approval = codeReview.all.get(0);
+ assertThat(approval._accountId).isEqualTo(user.id.get());
+ assertThat(approval.value).isEqualTo(1);
+ }
+
+ @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 +353,82 @@
.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 submitOnBehalfOfInvisibleUserIsAllowed() 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;
+ // TODO(dborowitz): Make this fail instead.
+ gApi.changes()
+ .id(changeId)
+ .current()
+ .submit(in);
+ assertThat(gApi.changes().id(changeId).get().status)
+ .isEqualTo(ChangeStatus.MERGED);
+ }
+
+ 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);
+ }
}
