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
diff --git a/.gitmodules b/.gitmodules
index e5eef1e..6f493f0 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 92759b6..d53bd9c 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2556,6 +2556,9 @@
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 3c66c2c..c06ce82 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.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 @@
@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 @@
}
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 09ca258..3891700 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 @@
* 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 @@
@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 67b5870..517fbdf 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.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 @@
}
@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 a4a82ce..e566858 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.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 @@
}
@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
index 687ad94..5b107a6 100644
--- a/java/com/google/gerrit/truth/NullAwareCorrespondence.java
+++ b/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
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
index 80e04c0..2356327 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.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;
@@ -96,6 +100,53 @@
}
@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 @@
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 19c479d..b69a894 100644
--- a/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
+++ b/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 @@
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 @@
// 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 @@
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 @@
// 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 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
index 584360d..709df89 160000
--- a/plugins/gitiles
+++ b/plugins/gitiles
@@ -1 +1 @@
-Subproject commit 584360d050f5adf8317ebc078a874da9c4316bd0
+Subproject commit 709df89717a08f5d8b8728076d608f30894a11bc
