IncludedIn: filter out non-visible branches

The project's commit endpoint "Get Included In" returns the list of
branches and tags in which a change is included. This endpoint only
checked if the input commit is visible to the caller (see
CommitsCollections), but didn't check for caller's visibility on
branches and tags that are returned by this endpoint.

In this change, we filter out branches and tags that are not visibile to
the caller.

Note: The NullAwareCorrespondence class was copied from master because
we need it for the new test.

Bug: Issue 14732
Change-Id: I66f5b72926501fe2482a0f7fc91032bbf2eac43b
(cherry picked from commit 993378f07dad5e4fbcb4482ac97a98461e1b9743)

8 files changed, 228 insertions, 19 deletions

diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index c1349aa4ae..63c3279410 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2504,6 +2504,9 @@ is returned that describes the commit.
 Retrieves the branches and tags in which a change is included. As result
 an link:rest-api-changes.html#included-in-info[IncludedInInfo] entity is returned.
 
+Branches that are not visible to the calling user according to the project's
+read permissions are filtered out from the result.
+
 .Request
 ----
   GET /projects/work%2Fmy-project/commits/a8a477efffbbf3b44169bb9a1d3a334cbbd9aa96/in HTTP/1.0
diff --git a/java/com/google/gerrit/server/change/IncludedIn.java b/java/com/google/gerrit/server/change/IncludedIn.java
index 3c66c2c8bb..a0b64dab34 100644
--- a/java/com/google/gerrit/server/change/IncludedIn.java
+++ b/java/com/google/gerrit/server/change/IncludedIn.java
@@ -14,6 +14,11 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
+import static java.util.Comparator.naturalOrder;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.entities.Project;
@@ -23,13 +28,19 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.plugincontext.PluginSetContext;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Map;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -37,17 +48,21 @@ import org.eclipse.jgit.revwalk.RevWalk;
 @Singleton
 public class IncludedIn {
   private final GitRepositoryManager repoManager;
+  private final PermissionBackend permissionBackend;
   private final PluginSetContext<ExternalIncludedIn> externalIncludedIn;
 
   @Inject
   IncludedIn(
-      GitRepositoryManager repoManager, PluginSetContext<ExternalIncludedIn> externalIncludedIn) {
+      GitRepositoryManager repoManager,
+      PermissionBackend permissionBackend,
+      PluginSetContext<ExternalIncludedIn> externalIncludedIn) {
     this.repoManager = repoManager;
+    this.permissionBackend = permissionBackend;
     this.externalIncludedIn = externalIncludedIn;
   }
 
   public IncludedInInfo apply(Project.NameKey project, String revisionId)
-      throws RestApiException, IOException {
+      throws RestApiException, IOException, PermissionBackendException {
     try (Repository r = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(r)) {
       rw.setRetainBody(false);
@@ -61,18 +76,45 @@ public class IncludedIn {
       }
 
       IncludedInResolver.Result d = IncludedInResolver.resolve(r, rw, rev);
+
+      // Filter branches and tags according to their visbility by the user
+      ImmutableSortedSet<String> filteredBranches =
+          sortedShortNames(filterReadableRefs(project, d.branches()).keySet());
+      ImmutableSortedSet<String> filteredTags =
+          sortedShortNames(filterReadableRefs(project, d.tags()).keySet());
+
       ListMultimap<String, String> external = MultimapBuilder.hashKeys().arrayListValues().build();
       externalIncludedIn.runEach(
           ext -> {
             ListMultimap<String, String> extIncludedIns =
-                ext.getIncludedIn(project.get(), rev.name(), d.tags(), d.branches());
+                ext.getIncludedIn(project.get(), rev.name(), filteredBranches, filteredTags);
             if (extIncludedIns != null) {
               external.putAll(extIncludedIns);
             }
           });
 
       return new IncludedInInfo(
-          d.branches(), d.tags(), (!external.isEmpty() ? external.asMap() : null));
+          filteredBranches, filteredTags, (!external.isEmpty() ? external.asMap() : null));
+    }
+  }
+
+  /**
+   * Filter readable branches or tags according to the caller's refs visibility.
+   *
+   * @param project specific Gerrit project.
+   * @param inputRefs a list of branches (in short name) as strings
+   */
+  private Map<String, Ref> filterReadableRefs(Project.NameKey project, ImmutableList<Ref> inputRefs)
+      throws IOException, PermissionBackendException {
+    PermissionBackend.ForProject perm = permissionBackend.currentUser().project(project);
+    try (Repository repo = repoManager.openRepository(project)) {
+      return perm.filter(inputRefs, repo, RefFilterOptions.defaults());
     }
   }
+
+  private ImmutableSortedSet<String> sortedShortNames(Collection<String> refs) {
+    return refs.stream()
+        .map(Repository::shortenRefName)
+        .collect(toImmutableSortedSet(naturalOrder()));
+  }
 }
diff --git a/java/com/google/gerrit/server/change/IncludedInResolver.java b/java/com/google/gerrit/server/change/IncludedInResolver.java
index 09ca258103..38917006e1 100644
--- a/java/com/google/gerrit/server/change/IncludedInResolver.java
+++ b/java/com/google/gerrit/server/change/IncludedInResolver.java
@@ -14,13 +14,11 @@
 
 package com.google.gerrit.server.change;
 
-import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
 import static java.util.Comparator.comparing;
-import static java.util.Comparator.naturalOrder;
 import static java.util.stream.Collectors.toList;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.LinkedListMultimap;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
@@ -171,13 +169,12 @@ public class IncludedInResolver {
    * Returns the short names of refs which are as well in the matchingRefs list as well as in the
    * allRef list.
    */
-  private static ImmutableSortedSet<String> getMatchingRefNames(
+  private static ImmutableList<Ref> getMatchingRefNames(
       Set<String> matchingRefs, Collection<Ref> allRefs) {
     return allRefs.stream()
-        .map(Ref::getName)
-        .filter(matchingRefs::contains)
-        .map(Repository::shortenRefName)
-        .collect(toImmutableSortedSet(naturalOrder()));
+        .filter(r -> matchingRefs.contains(r.getName()))
+        .distinct()
+        .collect(ImmutableList.toImmutableList());
   }
 
   /** Parse commit of ref and store the relation between ref and commit. */
@@ -211,8 +208,8 @@ public class IncludedInResolver {
 
   @AutoValue
   public abstract static class Result {
-    public abstract ImmutableSortedSet<String> branches();
+    public abstract ImmutableList<Ref> branches();
 
-    public abstract ImmutableSortedSet<String> tags();
+    public abstract ImmutableList<Ref> tags();
   }
 }
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java b/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
index 67b5870ba7..517fbdf9a8 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
@@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.IncludedIn;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
@@ -38,7 +39,8 @@ public class ChangeIncludedIn implements RestReadView<ChangeResource> {
   }
 
   @Override
-  public Response<IncludedInInfo> apply(ChangeResource rsrc) throws RestApiException, IOException {
+  public Response<IncludedInInfo> apply(ChangeResource rsrc)
+      throws RestApiException, IOException, PermissionBackendException {
     PatchSet ps = psUtil.current(rsrc.getNotes());
     return Response.ok(includedIn.apply(rsrc.getProject(), ps.commitId().name()));
   }
diff --git a/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java b/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
index a4a82ce2d7..e566858782 100644
--- a/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
+++ b/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.change.IncludedIn;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.CommitResource;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -36,7 +37,8 @@ public class CommitIncludedIn implements RestReadView<CommitResource> {
   }
 
   @Override
-  public Response<IncludedInInfo> apply(CommitResource rsrc) throws RestApiException, IOException {
+  public Response<IncludedInInfo> apply(CommitResource rsrc)
+      throws RestApiException, IOException, PermissionBackendException {
     RevCommit commit = rsrc.getCommit();
     Project.NameKey project = rsrc.getProjectState().getNameKey();
     return Response.ok(includedIn.apply(project, commit.getId().getName()));
diff --git a/java/com/google/gerrit/truth/NullAwareCorrespondence.java b/java/com/google/gerrit/truth/NullAwareCorrespondence.java
new file mode 100644
index 0000000000..5b107a6929
--- /dev/null
+++ b/java/com/google/gerrit/truth/NullAwareCorrespondence.java
@@ -0,0 +1,83 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.truth;
+
+import com.google.common.base.Function;
+import com.google.common.truth.Correspondence;
+import java.util.Optional;
+
+/** Utility class for constructing null aware {@link Correspondence}s. */
+public class NullAwareCorrespondence {
+  /**
+   * Constructs a {@link Correspondence} that compares elements by transforming the actual elements
+   * using the given function and testing for equality with the expected elements.
+   *
+   * <p>If the actual element is null, it will correspond to a null expected element. This is
+   * different to {@link Correspondence#transforming(Function, String)} which would invoke the
+   * function with a {@code null} argument, requiring the function being able to handle {@code
+   * null}.
+   *
+   * @param actualTransform a {@link Function} taking an actual value and returning a new value
+   *     which will be compared with an expected value to determine whether they correspond
+   * @param description should fill the gap in a failure message of the form {@code "not true that
+   *     <some actual element> is an element that <description> <some expected element>"}, e.g.
+   *     {@code "has an ID of"}
+   */
+  public static <A, E> Correspondence<A, E> transforming(
+      Function<A, ? extends E> actualTransform, String description) {
+    return Correspondence.transforming(
+        actualValue -> Optional.ofNullable(actualValue).map(actualTransform).orElse(null),
+        description);
+  }
+
+  /**
+   * Constructs a {@link Correspondence} that compares elements by transforming the actual elements
+   * using the given function and testing for equality with the expected elements.
+   *
+   * <p>If the actual element is null, it will correspond to a null expected element. This is
+   * different to {@link Correspondence#transforming(Function, Function, String)} which would invoke
+   * the function with a {@code null} argument, requiring the function being able to handle {@code
+   * null}.
+   *
+   * <p>If the expected element is null, it will correspond to a new null expected element. This is
+   * different to {@link Correspondence#transforming(Function, Function, String)} which would invoke
+   * the function with a {@code null} argument, requiring the function being able to handle {@code
+   * null}.
+   *
+   * @param actualTransform a {@link Function} taking an actual value and returning a new value
+   *     which will be compared with an expected value to determine whether they correspond
+   * @param expectedTransform a {@link Function} taking an expected value and returning a new value
+   *     which will be compared with a transformed actual value
+   * @param description should fill the gap in a failure message of the form {@code "not true that
+   *     <some actual element> is an element that <description> <some expected element>"}, e.g.
+   *     {@code "has an ID of"}
+   */
+  public static <A, E> Correspondence<A, E> transforming(
+      Function<A, ? extends E> actualTransform,
+      Function<E, ?> expectedTransform,
+      String description) {
+    return Correspondence.transforming(
+        actualValue -> Optional.ofNullable(actualValue).map(actualTransform).orElse(null),
+        expectedValue -> Optional.ofNullable(expectedValue).map(expectedTransform).orElse(null),
+        description);
+  }
+
+  /**
+   * Private constructor to prevent instantiation of this class.
+   *
+   * <p>This class contains only static method and hence never needs to be instantiated.
+   */
+  private NullAwareCorrespondence() {}
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
index 04625c58df..c7955a56ad 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
@@ -15,7 +15,10 @@
 package com.google.gerrit.acceptance.api.project;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
 import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static java.util.stream.Collectors.toList;
 import static org.eclipse.jgit.lib.Constants.R_TAGS;
 
@@ -26,6 +29,7 @@ import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.RefNames;
 import com.google.gerrit.extensions.api.changes.CherryPickInput;
 import com.google.gerrit.extensions.api.changes.IncludedInInfo;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -96,6 +100,53 @@ public class CommitIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void includedInMergedChange_filtersOutNonVisibleBranches() throws Exception {
+    Result baseChange = createAndSubmitChange("refs/for/master");
+
+    createBranch(BranchNameKey.create(project, "test-branch-1"));
+    createBranch(BranchNameKey.create(project, "test-branch-2"));
+    createAndSubmitChange("refs/for/test-branch-1");
+    createAndSubmitChange("refs/for/test-branch-2");
+
+    assertThat(getIncludedIn(baseChange.getCommit().getId()).branches)
+        .containsExactly("master", "test-branch-1", "test-branch-2");
+
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(block(Permission.READ).ref("refs/heads/test-branch-1").group(REGISTERED_USERS))
+        .update();
+
+    assertThat(getIncludedIn(baseChange.getCommit().getId()).branches)
+        .containsExactly("master", "test-branch-2");
+  }
+
+  @Test
+  public void includedInMergedChange_filtersOutNonVisibleTags() throws Exception {
+    String tagBase = "tag_base";
+    String tagBranch1 = "tag_1";
+
+    Result baseChange = createAndSubmitChange("refs/for/master");
+    createLightWeightTag(tagBase);
+    assertThat(getIncludedIn(baseChange.getCommit().getId()).tags).containsExactly(tagBase);
+
+    createBranch(BranchNameKey.create(project, "test-branch-1"));
+    createAndSubmitChange("refs/for/test-branch-1");
+    createLightWeightTag(tagBranch1);
+    assertThat(getIncludedIn(baseChange.getCommit().getId()).tags)
+        .containsExactly(tagBase, tagBranch1);
+
+    projectOperations
+        .project(project)
+        .forUpdate()
+        // Tag permissions are controlled by read permissions on branches. Blocking read permission
+        // on test-branch-1 so that tagBranch1 becomes non-visible
+        .add(block(Permission.READ).ref("refs/heads/test-branch-1").group(REGISTERED_USERS))
+        .update();
+    assertThat(getIncludedIn(baseChange.getCommit().getId()).tags).containsExactly(tagBase);
+  }
+
+  @Test
   public void cherryPickWithoutMessage() throws Exception {
     String branch = "foo";
 
@@ -177,4 +228,15 @@ public class CommitIT extends AbstractDaemonTest {
     assertThat(actual.email).isEqualTo(expected.email());
     assertThat(actual.name).isEqualTo(expected.fullName());
   }
+
+  private Result createAndSubmitChange(String branch) throws Exception {
+    Result r = createChange(branch);
+    approve(r.getChangeId());
+    gApi.changes().id(r.getChangeId()).current().submit();
+    return r;
+  }
+
+  private void createLightWeightTag(String tagName) throws Exception {
+    pushHead(testRepo, RefNames.REFS_TAGS + tagName, false, false);
+  }
 }
diff --git a/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java b/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
index 19c479d3b8..b69a894b14 100644
--- a/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
+++ b/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
@@ -17,9 +17,13 @@ package com.google.gerrit.server.change;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.entities.RefNames.REFS_TAGS;
 
+import com.google.common.truth.Correspondence;
+import com.google.gerrit.truth.NullAwareCorrespondence;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevTag;
@@ -112,8 +116,12 @@ public class IncludedInResolverTest {
     IncludedInResolver.Result detail = resolve(commit_v2_5);
 
     // Check that only tags and branches which refer the tip are returned
-    assertThat(detail.tags()).containsExactly(TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
-    assertThat(detail.branches()).containsExactly(BRANCH_2_5);
+    assertThat(detail.tags())
+        .comparingElementsUsing(hasShortName())
+        .containsExactly(TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
+    assertThat(detail.branches())
+        .comparingElementsUsing(hasShortName())
+        .containsExactly(BRANCH_2_5);
   }
 
   @Test
@@ -123,6 +131,7 @@ public class IncludedInResolverTest {
 
     // Check whether all tags and branches are returned
     assertThat(detail.tags())
+        .comparingElementsUsing(hasShortName())
         .containsExactly(
             TAG_1_0,
             TAG_1_0_1,
@@ -133,6 +142,7 @@ public class IncludedInResolverTest {
             TAG_2_5_ANNOTATED,
             TAG_2_5_ANNOTATED_TWICE);
     assertThat(detail.branches())
+        .comparingElementsUsing(hasShortName())
         .containsExactly(BRANCH_MASTER, BRANCH_1_0, BRANCH_1_3, BRANCH_2_0, BRANCH_2_5);
   }
 
@@ -143,8 +153,11 @@ public class IncludedInResolverTest {
 
     // Check whether all succeeding tags and branches are returned
     assertThat(detail.tags())
+        .comparingElementsUsing(hasShortName())
         .containsExactly(TAG_1_3, TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
-    assertThat(detail.branches()).containsExactly(BRANCH_1_3, BRANCH_2_5);
+    assertThat(detail.branches())
+        .comparingElementsUsing(hasShortName())
+        .containsExactly(BRANCH_1_3, BRANCH_2_5);
   }
 
   private IncludedInResolver.Result resolve(RevCommit commit) throws Exception {
@@ -154,4 +167,9 @@ public class IncludedInResolverTest {
   private RevTag tag(String name, RevObject dest) throws Exception {
     return tr.update(REFS_TAGS + name, tr.tag(name, dest));
   }
+
+  private static Correspondence<Ref, String> hasShortName() {
+    return NullAwareCorrespondence.transforming(
+        ref -> Repository.shortenRefName(ref.getName()), "has short name");
+  }
 }