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