Merge branch 'stable-3.1-2021-07'

* stable-3.1-2021-07:
  Update Gitiles plugin
  IncludedIn: filter out non-visible branches
  Point gitiles submodule to gitiles security fix repo

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I7298bc114f89564dbf019ea0bebbc10bc5575002

10 files changed, 232 insertions, 20 deletions

diff --git a/.gitmodules b/.gitmodules
index e5eef1e538..6f493f0933 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -24,7 +24,7 @@
 
 [submodule "plugins/gitiles"]
 	path = plugins/gitiles
-	url = ../plugins/gitiles
+	url = ../plugins/gitiles-security-fixes
 	branch = .
 
 [submodule "plugins/hooks"]
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index a4e27b3b52..de2c550fc5 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2505,6 +2505,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..c06ce82c05 100644
--- a/java/com/google/gerrit/server/change/IncludedIn.java
+++ b/java/com/google/gerrit/server/change/IncludedIn.java
@@ -14,6 +14,12 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+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 +29,18 @@ 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 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,48 @@ 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()));
+      ImmutableSortedSet<String> filteredTags =
+          sortedShortNames(filterReadableRefs(project, d.tags()));
+
       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 Collection<String> 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()).stream()
+          .map(Ref::getName)
+          .collect(toImmutableList());
     }
   }
+
+  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 e67770c3ec..5ee934071a 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";
 
@@ -192,4 +243,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");
+  }
 }
diff --git a/plugins/gitiles b/plugins/gitiles
-Subproject b8ea789876c0661e1aef9fa3f54a6ac3b8abd62
+Subproject 0dee9e42fe5716383ef87df4a256de63e3c1fd2