Merge branch 'stable-3.3-2021-07' * stable-3.3-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: I6ca32916b861e64583c30e7147556a12e93f0f49
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 e30ce3a..8ecc7bc 100644 --- a/Documentation/rest-api-projects.txt +++ b/Documentation/rest-api-projects.txt
@@ -2561,6 +2561,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 bdb03d2..18e192d 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 com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.R_TAGS; @@ -28,6 +31,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; @@ -99,6 +103,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 cherryPickWithoutMessageSameBranch() throws Exception { String destBranch = "master"; @@ -390,4 +441,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 b196dd5..02d2f35 160000 --- a/plugins/gitiles +++ b/plugins/gitiles
@@ -1 +1 @@ -Subproject commit b196dd5b6fcfd50518a6625a64cb93424c084620 +Subproject commit 02d2f357ded829277a317081d905a52337dcc9ac