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)
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index c1349aa..63c3279 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2504,6 +2504,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..a0b64da 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.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 @@
@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 @@
}
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 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
new file mode 100644
index 0000000..5b107a6
--- /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 04625c5..c7955a56 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.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 @@
}
@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 @@
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");
+ }
}
