From 771111c5211b0f0c9717c4439851a553cb3dbe67 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 18 Aug 2016 15:56:27 +0200 Subject: [PATCH] PostReviewers: Fail if designated reviewer cannot see the change If a user tries to add a user as reviewer to a change that is not visible to that user, a proper error message is displayed now. Before the request didn't fail, but the user was not added as reviewer. And without feedback the calling user didn't know why the reviewer was not added. Change-Id: Ib364d1a862e75eda492c6bf3f80a0be0341072e6 Signed-off-by: Edwin Kempin --- .../acceptance/api/change/ChangeIT.java | 41 +++++++++++++++++++ .../gerrit/server/change/PostReviewers.java | 11 ++--- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index b59a93d298..006d8a47a1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -28,6 +28,7 @@ import static com.google.gerrit.server.project.Util.blockLabel; import static com.google.gerrit.server.project.Util.category; import static com.google.gerrit.server.project.Util.value; import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.Assert.fail; import com.google.common.base.Function; import com.google.common.collect.ImmutableSet; @@ -63,6 +64,7 @@ import com.google.gerrit.extensions.common.RevisionInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; +import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; @@ -535,6 +537,45 @@ public class ChangeIT extends AbstractDaemonTest { .rebase(ri); } + @Test + public void addReviewerThatCannotSeeChange() throws Exception { + // create hidden project that is only visible to administrators + Project.NameKey p = createProject("p"); + ProjectConfig cfg = projectCache.checkedGet(p).getConfig(); + Util.allow(cfg, + Permission.READ, + groupCache.get(new AccountGroup.NameKey("Administrators")) + .getGroupUUID(), + "refs/*"); + Util.block(cfg, Permission.READ, REGISTERED_USERS, "refs/*"); + saveProjectConfig(p, cfg); + + // create change + TestRepository repo = cloneProject(p, admin); + PushOneCommit push = pushFactory.create(db, admin.getIdent(), repo); + PushOneCommit.Result result = push.to("refs/for/master"); + result.assertOkStatus(); + + // check the user cannot see the change + setApiUser(user); + try { + gApi.changes().id(result.getChangeId()).get(); + fail("Expected ResourceNotFoundException"); + } catch (ResourceNotFoundException e) { + // Expected. + } + + // try to add user as reviewer + setApiUser(admin); + AddReviewerInput in = new AddReviewerInput(); + in.reviewer = user.email; + exception.expect(UnprocessableEntityException.class); + exception.expectMessage("Change not visible to " + user.email); + gApi.changes() + .id(result.getChangeId()) + .addReviewer(in); + } + @Test public void addReviewer() throws Exception { TestTimeUtil.resetWithClockStep(1, SECONDS); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 399c3bae30..fb37d9d8c1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -161,10 +161,9 @@ public class PostReviewers public Addition prepareApplication(ChangeResource rsrc, AddReviewerInput input) throws OrmException, RestApiException, IOException { + Account.Id accountId; try { - Account.Id accountId = accounts.parse(input.reviewer).getAccountId(); - return putAccount(input.reviewer, reviewerFactory.create(rsrc, accountId), - input.state()); + accountId = accounts.parse(input.reviewer).getAccountId(); } catch (UnprocessableEntityException e) { try { return putGroup(rsrc, input); @@ -173,17 +172,19 @@ public class PostReviewers .format(ChangeMessages.get().reviewerNotFound, input.reviewer)); } } + return putAccount(input.reviewer, reviewerFactory.create(rsrc, accountId), + input.state()); } private Addition putAccount(String reviewer, ReviewerResource rsrc, - ReviewerState state) { + ReviewerState state) throws UnprocessableEntityException { Account member = rsrc.getReviewerUser().getAccount(); ChangeControl control = rsrc.getReviewerControl(); if (isValidReviewer(member, control)) { return new Addition(reviewer, rsrc.getChangeResource(), ImmutableMap.of(member.getId(), control), state); } - return new Addition(reviewer); + throw new UnprocessableEntityException("Change not visible to " + reviewer); } private Addition putGroup(ChangeResource rsrc, AddReviewerInput input)