Merge changes I8ec3c7d4,I4b8d69fd

* changes:
  Don't add users from footers as CC if they can't see the change
  Add tests for pushing commit with footer of other user

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<AccountInfo> 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<Message> 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<InMemoryRepository> 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

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,
+    approvalsUtil.addReviewers(db, update, labelTypes, change, patchSet,
-        patchSet, patchSetInfo, reviewers, Collections.<Account.Id> emptySet());
+        patchSetInfo,
+        filterOnChangeVisibility(db, ctx.getNotes(), reviewers),
+        Collections.<Account.Id> 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<Account.Id> filterOnChangeVisibility(final ReviewDb db,
+      final ChangeNotes notes, Set<Account.Id> accounts) {
+    return Sets.filter(accounts, new Predicate<Account.Id>() {
+      @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) {