OutputStreamQuery: Only return current patch set when visible to user

When querying changes with the 'gerrit query' ssh command, and
passing the --current-patch-set option, the current patch set is
included even when it is not visible to the caller (for example when
the patch set is a draft, and the caller cannot see drafts).

This causes problems for example when the caller runs a query and
then tries to perform some operation on the revisions of the current
patchset revisions that were returned. For those revisions that are
not visible, the operation will fail.

The same is true when using the --patch-sets option.

Add a check for patch set visibility, and do not add it in the
results.

Bug: Issue 4070
Change-Id: Id68969bc49a9412ae81f252fd2d846539d9022fa

5 files changed, 68 insertions, 15 deletions

diff --git a/Documentation/cmd-query.txt b/Documentation/cmd-query.txt
index 0ff59d4d3c..090781b1bc 100644
--- a/Documentation/cmd-query.txt
+++ b/Documentation/cmd-query.txt
@@ -54,15 +54,17 @@ command line parser in the server).
 
 --current-patch-set::
 	Include information about the current patch set in the results.
+	Note that the information will only be included when the current
+	patch set is visible to the caller.
 
 --patch-sets::
-	Include information about all patch sets.  If combined with
-	the --current-patch-set flag then the current patch set
-	information will be output twice, once in each field.
+	Include information about all patch sets visible to the caller.
+        If combined with the --current-patch-set flag then the current patch
+	set information will be output twice, once in each field.
 
 --all-approvals::
-	Include information about all patch sets along with the
-	approval information for each patch set.  If combined with
+	Include information about all patch sets visible to the caller along
+	with the approval information for each patch set.  If combined with
 	the --current-patch-set flag then the current patch set
 	information will be output twice, once in each field.
 
@@ -76,7 +78,7 @@ command line parser in the server).
 --comments::
 	Include comments for all changes. If combined with the
 	--patch-sets flag then all inline/file comments are included for
-	each patch set.
+	each patch set that is visible to the caller.
 
 --commit-message::
 	Include the full commit message in the change description.
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index d890fa4488..056b4ed22f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -236,6 +236,11 @@ public abstract class AbstractDaemonTest {
     return push.to(git, ref);
   }
 
+  protected PushOneCommit.Result amendChangeAsDraft(String changeId)
+      throws GitAPIException, IOException {
+    return amendChange(changeId, "refs/drafts/master");
+  }
+
   protected ChangeInfo getChange(String changeId, ListChangesOption... options)
       throws IOException {
     return getChange(adminSession, changeId, options);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
index 066d3628d7..0e381c192e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
@@ -16,11 +16,13 @@ package com.google.gerrit.acceptance.ssh;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assert_;
+import static com.google.gerrit.acceptance.GitUtil.initSsh;
 
 import com.google.common.collect.Lists;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.SshSession;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.Side;
@@ -300,15 +302,41 @@ public class QueryIT extends AbstractDaemonTest {
     assertThat(changes.get(0).submitRecords.size()).isEqualTo(1);
   }
 
-  private List<ChangeAttribute> executeSuccessfulQuery(String params)
-      throws Exception {
+  @Test
+  public void testQueryWithNonVisibleCurrentPatchSet() throws Exception {
+    String changeId = createChange().getChangeId();
+    amendChangeAsDraft(changeId);
+    String query = "--current-patch-set --patch-sets " + changeId;
+    List<ChangeAttribute> changes = executeSuccessfulQuery(query);
+    assertThat(changes.size()).isEqualTo(1);
+    assertThat(changes.get(0).patchSets).isNotNull();
+    assertThat(changes.get(0).patchSets).hasSize(2);
+    assertThat(changes.get(0).currentPatchSet).isNotNull();
+
+    SshSession userSession = new SshSession(server, user);
+    initSsh(user);
+    userSession.open();
+    changes = executeSuccessfulQuery(query, userSession);
+    assertThat(changes.size()).isEqualTo(1);
+    assertThat(changes.get(0).patchSets).hasSize(1);
+    assertThat(changes.get(0).currentPatchSet).isNull();
+    userSession.close();
+  }
+
+  private List<ChangeAttribute> executeSuccessfulQuery(String params,
+      SshSession session) throws Exception {
     String rawResponse =
-        sshSession.exec("gerrit query --format=JSON " + params);
-    assert_().withFailureMessage(sshSession.getError())
-        .that(sshSession.hasError()).isFalse();
+        session.exec("gerrit query --format=JSON " + params);
+    assert_().withFailureMessage(session.getError())
+        .that(session.hasError()).isFalse();
     return getChanges(rawResponse);
   }
 
+  private List<ChangeAttribute> executeSuccessfulQuery(String params)
+      throws Exception {
+    return executeSuccessfulQuery(params, sshSession);
+  }
+
   private static List<ChangeAttribute> getChanges(String rawResponse) {
     String[] lines = rawResponse.split("\\n");
     List<ChangeAttribute> changes = new ArrayList<>(lines.length - 1);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index e4e94a12f8..c565ab2327 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -17,6 +17,8 @@ package com.google.gerrit.server.query.change;
 import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
 
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Predicate;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
@@ -545,6 +547,22 @@ public class ChangeData {
   }
 
   /**
+   * @return patches for the change visible to the current user.
+   * @throws OrmException an error occurred reading the database.
+   */
+  public Collection<PatchSet> visiblePatches() throws OrmException {
+    return FluentIterable.from(patches()).filter(new Predicate<PatchSet>() {
+      @Override
+      public boolean apply(PatchSet input) {
+        try {
+          return changeControl().isPatchVisible(input, db);
+        } catch (OrmException e) {
+          return false;
+        }
+      }}).toList();
+  }
+
+  /**
    * @return patch with the given ID, or null if it does not exist.
    * @throws OrmException an error occurred reading the database.
    */
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index d594f8acdf..301273564c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -209,11 +209,11 @@ public class OutputStreamQuery {
 
           if (includePatchSets) {
             if (includeFiles) {
-              eventFactory.addPatchSets(c, d.patches(),
+              eventFactory.addPatchSets(c, d.visiblePatches(),
                   includeApprovals ? d.approvals().asMap() : null,
                   includeFiles, d.change(), labelTypes);
             } else {
-              eventFactory.addPatchSets(c, d.patches(),
+              eventFactory.addPatchSets(c, d.visiblePatches(),
                   includeApprovals ? d.approvals().asMap() : null,
                   labelTypes);
             }
@@ -221,7 +221,7 @@ public class OutputStreamQuery {
 
           if (includeCurrentPatchSet) {
             PatchSet current = d.currentPatchSet();
-            if (current != null) {
+            if (current != null && cc.isPatchVisible(current, d.db())) {
               c.currentPatchSet = eventFactory.asPatchSetAttribute(current);
               eventFactory.addApprovals(c.currentPatchSet,
                   d.currentApprovals(), labelTypes);
@@ -240,7 +240,7 @@ public class OutputStreamQuery {
           if (includeComments) {
             eventFactory.addComments(c, d.messages());
             if (includePatchSets) {
-              eventFactory.addPatchSets(c, d.patches(),
+              eventFactory.addPatchSets(c, d.visiblePatches(),
                   includeApprovals ? d.approvals().asMap() : null,
                   includeFiles, d.change(), labelTypes);
               for (PatchSetAttribute attribute : c.patchSets) {