Merge branch 'stable-3.2-2021-07'

* stable-3.2-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: Iaa9a86a527af846b096cb404150ce2eb705db306

.gitmodules

@@ -24,7 +24,7 @@
 
 [submodule "plugins/gitiles"]
 	path = plugins/gitiles
-	url = ../plugins/gitiles
+	url = ../plugins/gitiles-security-fixes
 	branch = .
 
 [submodule "plugins/hooks"]

Documentation/rest-api-projects.txt

@@ -2556,6 +2556,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

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,31 +29,40 @@
 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;
 
 @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 IncludedInInfo apply(Project.NameKey project, String revisionId)
       }
 
       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()));
+  }
 }

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 @@ private void partition(List<RevCommit> before, List<RevCommit> after) {
    * 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 @@ private void parseCommits(Collection<Ref> refs) throws IOException {
 
   @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();
   }
 }

java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java

@@ -22,6 +22,7 @@
 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()));
   }

java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java

@@ -20,6 +20,7 @@
 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()));

java/com/google/gerrit/truth/NullAwareCorrespondence.java

@@ -7,15 +7,6 @@
 // http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
-// 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

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.testsuite.project.ProjectOperations;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Permission;
+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;
@@ -95,6 +99,53 @@ public void includedInMergedChange() throws Exception {
         .containsExactly("master", "test-branch");
   }
 
+  @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 @@ private static void assertPerson(GitPerson actual, TestAccount expected) {
     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);
+  }
 }

javatests/com/google/gerrit/server/change/IncludedInResolverTest.java

@@ -17,9 +17,13 @@
 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 void resolveLatestCommit() throws Exception {
     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 void resolveFirstCommit() throws Exception {
 
     // 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 void resolveFirstCommit() throws Exception {
             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 void resolveBetwixtCommit() throws Exception {
 
     // 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 @@ private IncludedInResolver.Result resolve(RevCommit commit) throws Exception {
   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");
+  }
 }