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 a1e160329d..7bad2613d8 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 @@ -41,6 +41,7 @@ import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.AddReviewerInput; @@ -611,6 +612,76 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(sender.getMessages()).isEmpty(); } + @Test + public void pushCommitWithFooterOfOtherUser() throws Exception { + // admin pushes commit that references 'user' in a footer + PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo, + PushOneCommit.SUBJECT + "\n\n" + + FooterConstants.REVIEWED_BY.getName() + ": " + + user.getIdent().toExternalString(), + PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT); + PushOneCommit.Result result = push.to("refs/for/master"); + result.assertOkStatus(); + + // check that 'user' was added as reviewer + ChangeInfo change = gApi.changes().id(result.getChangeId()).get(); + Collection reviewers = change.reviewers.get(REVIEWER); + assertThat(reviewers).isNotNull(); + assertThat(reviewers).hasSize(1); + assertThat(reviewers.iterator().next()._accountId) + .isEqualTo(user.getId().get()); + assertThat(change.reviewers.get(CC)).isNull(); + + List messages = sender.getMessages(); + assertThat(messages).hasSize(1); + Message m = messages.get(0); + assertThat(m.rcpt()).containsExactly(user.emailAddress); + assertThat(m.body()).contains("Hello " + user.fullName + ",\n"); + assertThat(m.body()).contains("I'd like you to do a code review."); + assertThat(m.body()) + .contains("Change subject: " + PushOneCommit.SUBJECT + "\n"); + assertMailFrom(m, admin.email); + } + + @Test + public void pushCommitWithFooterOfOtherUserThatCannotSeeChange() + 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); + + // admin pushes commit that references 'user' in a footer + TestRepository repo = cloneProject(p, admin); + PushOneCommit push = pushFactory.create(db, admin.getIdent(), repo, + PushOneCommit.SUBJECT + "\n\n" + FooterConstants.REVIEWED_BY.getName() + + ": " + user.getIdent().toExternalString(), + PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT); + PushOneCommit.Result result = push.to("refs/for/master"); + result.assertOkStatus(); + + // check that 'user' cannot see the change + setApiUser(user); + try { + gApi.changes().id(result.getChangeId()).get(); + fail("Expected ResourceNotFoundException"); + } catch (ResourceNotFoundException e) { + // Expected. + } + + // check that 'user' was NOT added as cc ('user' can't see the change) + setApiUser(admin); + ChangeInfo change = gApi.changes().id(result.getChangeId()).get(); + assertThat(change.reviewers.get(REVIEWER)).isNull(); + assertThat(change.reviewers.get(CC)).isNull(); + assertThat(sender.getMessages()).isEmpty(); + } + @Test public void addReviewerThatCannotSeeChange() throws Exception { // create hidden project that is only visible to administrators diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 6333809451..84781f58e6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -19,6 +19,8 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID; import com.google.common.base.MoreObjects; +import com.google.common.base.Predicate; +import com.google.common.collect.Sets; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; @@ -34,6 +36,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.extensions.events.CommentAdded; @@ -48,6 +51,7 @@ import com.google.gerrit.server.git.SendEmailExecutor; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.mail.CreateChangeSender; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; @@ -86,6 +90,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { LoggerFactory.getLogger(ChangeInserter.class); private final ProjectControl.GenericFactory projectControlFactory; + private final IdentifiedUser.GenericFactory userFactory; private final ChangeControl.GenericFactory changeControlFactory; private final PatchSetInfoFactory patchSetInfoFactory; private final PatchSetUtil psUtil; @@ -128,6 +133,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { @Inject ChangeInserter(ProjectControl.GenericFactory projectControlFactory, + IdentifiedUser.GenericFactory userFactory, ChangeControl.GenericFactory changeControlFactory, PatchSetInfoFactory patchSetInfoFactory, PatchSetUtil psUtil, @@ -142,6 +148,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { @Assisted RevCommit commit, @Assisted String refName) { this.projectControlFactory = projectControlFactory; + this.userFactory = userFactory; this.changeControlFactory = changeControlFactory; this.patchSetInfoFactory = patchSetInfoFactory; this.psUtil = psUtil; @@ -353,8 +360,10 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { update.fixStatus(change.getStatus()); LabelTypes labelTypes = ctl.getProjectControl().getLabelTypes(); - approvalsUtil.addReviewers(db, update, labelTypes, change, - patchSet, patchSetInfo, reviewers, Collections. emptySet()); + approvalsUtil.addReviewers(db, update, labelTypes, change, patchSet, + patchSetInfo, + filterOnChangeVisibility(db, ctx.getNotes(), reviewers), + Collections. emptySet()); approvalsUtil.addApprovals(db, update, labelTypes, patchSet, ctx.getControl(), approvals); if (message != null) { @@ -368,6 +377,25 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { return true; } + private Set filterOnChangeVisibility(final ReviewDb db, + final ChangeNotes notes, Set accounts) { + return Sets.filter(accounts, new Predicate() { + @Override + public boolean apply(Account.Id accountId) { + try { + IdentifiedUser user = userFactory.create(accountId); + return changeControlFactory.controlFor(notes, user).isVisible(db); + } catch (OrmException | NoSuchChangeException e) { + log.warn( + String.format("Failed to check if account %d can see change %d", + accountId.get(), notes.getChangeId().get()), + e); + return false; + } + } + }); + } + @Override public void postUpdate(Context ctx) throws OrmException, NoSuchChangeException { if (sendMail) {