From 2f7e0e4b07de7bd6a46f477e069e39bb049e0d2e Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 8 Oct 2018 15:10:00 +0200 Subject: [PATCH] Disallow submitting of changes that depend on non-visible private changes This is a follow up on the revert of Ia57ed8ef4 that takes a slightly different approach. Instead of letting the index filter by visiblity (which is doing a post-filter using ChangeIsVisibleToPredicate) we retrieve all results and apply the predicate by hand. This way, we can add non-visible changes to the set of non-visible changes and block the submit. We could alternatively also use the PermissionBackend to filter the list, but that would require us to do exactly the same as what is done in ChangeIsVisibleToPredicate. Since the predicate is public and we apply it to a list we directly obtained from the index, reusing it seems OK. Change-Id: I102ad38e3544d92448300a2a204fe657b344faa6 --- .../change/ChangeIsVisibleToPredicate.java | 2 + .../submit/LocalMergeSuperSetComputation.java | 54 +++++++---- .../change/SubmitByMergeIfNecessaryIT.java | 90 ++++++++++++++++--- 3 files changed, 116 insertions(+), 30 deletions(-) diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java index f81ea15947..d682b93ef8 100644 --- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java +++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java @@ -29,6 +29,7 @@ import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; import com.google.inject.Provider; import java.io.IOException; import org.eclipse.jgit.errors.RepositoryNotFoundException; @@ -43,6 +44,7 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate protected final ProjectCache projectCache; private final Provider anonymousUserProvider; + @Inject public ChangeIsVisibleToPredicate( Provider db, ChangeNotes.Factory notesFactory, diff --git a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java index 9efb976a65..66463befe4 100644 --- a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java +++ b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java @@ -37,6 +37,7 @@ import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.ChangeIsVisibleToPredicate; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; import com.google.gwtorm.server.OrmException; @@ -87,20 +88,23 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { private final PermissionBackend permissionBackend; private final Provider queryProvider; - private final Map> queryCache; + private final Map> queryCache; private final Map> heads; private final ProjectCache projectCache; + private final ChangeIsVisibleToPredicate changeIsVisibleToPredicate; @Inject LocalMergeSuperSetComputation( PermissionBackend permissionBackend, Provider queryProvider, - ProjectCache projectCache) { + ProjectCache projectCache, + ChangeIsVisibleToPredicate changeIsVisibleToPredicate) { this.projectCache = projectCache; this.permissionBackend = permissionBackend; this.queryProvider = queryProvider; this.queryCache = new HashMap<>(); this.heads = new HashMap<>(); + this.changeIsVisibleToPredicate = changeIsVisibleToPredicate; } @Override @@ -147,10 +151,11 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { Set visibleHashes = walkChangesByHashes(visibleCommits, Collections.emptySet(), or, b); - Iterables.addAll(visibleChanges, byCommitsOnBranchNotMerged(or, db, b, visibleHashes)); - Set nonVisibleHashes = walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b); - Iterables.addAll(nonVisibleChanges, byCommitsOnBranchNotMerged(or, db, b, nonVisibleHashes)); + + ChangeSet partialSet = byCommitsOnBranchNotMerged(or, db, b, visibleHashes, nonVisibleHashes); + Iterables.addAll(visibleChanges, partialSet.changes()); + Iterables.addAll(nonVisibleChanges, partialSet.nonVisibleChanges()); } return new ChangeSet(visibleChanges, nonVisibleChanges); @@ -206,24 +211,41 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { return str.type; } - private List byCommitsOnBranchNotMerged( + private ChangeSet byCommitsOnBranchNotMerged( + OpenRepo or, + ReviewDb db, + Branch.NameKey branch, + Set visibleHashes, + Set nonVisibleHashes) + throws OrmException, IOException { + List potentiallyVisibleChanges = + byCommitsOnBranchNotMerged(or, db, branch, visibleHashes); + List invisibleChanges = + new ArrayList<>(byCommitsOnBranchNotMerged(or, db, branch, nonVisibleHashes)); + List visibleChanges = new ArrayList<>(potentiallyVisibleChanges.size()); + for (ChangeData cd : potentiallyVisibleChanges) { + if (changeIsVisibleToPredicate.match(cd)) { + visibleChanges.add(cd); + } else { + invisibleChanges.add(cd); + } + } + return new ChangeSet(visibleChanges, invisibleChanges); + } + + private ImmutableList byCommitsOnBranchNotMerged( OpenRepo or, ReviewDb db, Branch.NameKey branch, Set hashes) throws OrmException, IOException { if (hashes.isEmpty()) { return ImmutableList.of(); } QueryKey k = QueryKey.create(branch, hashes); - List cached = queryCache.get(k); - if (cached != null) { - return cached; - } - - List result = new ArrayList<>(); - Iterable destChanges = - queryProvider.get().byCommitsOnBranchNotMerged(or.repo, db, branch, hashes); - for (ChangeData chd : destChanges) { - result.add(chd); + if (queryCache.containsKey(k)) { + return queryCache.get(k); } + ImmutableList result = + ImmutableList.copyOf( + queryProvider.get().byCommitsOnBranchNotMerged(or.repo, db, branch, hashes)); queryCache.put(k, result); return result; } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index c1dda00aaf..229122bce3 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; @@ -24,10 +25,9 @@ import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.CherryPickInput; import com.google.gerrit.extensions.api.changes.ReviewInput; -import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.api.projects.BranchInput; -import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -657,7 +657,7 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { } @Test - public void dependencyOnHiddenChangeShouldPreventMergeButDoesnt() throws Exception { + public void dependencyOnHiddenChangePreventsMerge() throws Exception { grantLabel("Code-Review", -2, 2, project, "refs/heads/*", false, REGISTERED_USERS, false); grant(project, "refs/*", Permission.SUBMIT, false, REGISTERED_USERS); @@ -686,18 +686,80 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { assertThat(e.getMessage()).isEqualTo("Not found: " + changeResult.getChangeId()); } - // Submit the second change which has a dependency on the first change which is not visible to - // the user. We would expect the submit to fail, but instead the submit succeeds and the hidden - // change gets submitted too. - // TODO(ekempin): Make this submit fail. - gApi.changes().id(change2Result.getChangeId()).current().submit(new SubmitInput()); + // Submit is expected to fail. + try { + gApi.changes().id(change2Result.getChangeId()).current().submit(); + fail("expected failure"); + } catch (AuthException e) { + assertThat(e.getMessage()) + .isEqualTo( + "A change to be submitted with " + + change2Result.getChange().getId().id + + " is not visible"); + } + assertRefUpdatedEvents(); + assertChangeMergedEvents(); + } - // Verify that both changes have been submitted. - setApiUser(admin); - assertThat(gApi.changes().id(changeResult.getChangeId()).get().status) - .isEqualTo(ChangeStatus.MERGED); - assertThat(gApi.changes().id(change2Result.getChangeId()).get().status) - .isEqualTo(ChangeStatus.MERGED); + @Test + public void dependencyOnHiddenChangeUsingTopicPreventsMerge() throws Exception { + // Construct a topic where a change included by topic depends on a private change that is not + // visible to the submitting user + // (c1) --- topic --- (c2b) + // | + // (c2a) <= private + assume().that(isSubmitWholeTopicEnabled()).isTrue(); + + Project.NameKey p1 = createProject("project-where-we-submit"); + Project.NameKey p2 = createProject("project-impacted-via-topic"); + + grantLabel("Code-Review", -2, 2, p1, "refs/heads/*", false, REGISTERED_USERS, false); + grant(p1, "refs/*", Permission.SUBMIT, false, REGISTERED_USERS); + grantLabel("Code-Review", -2, 2, p2, "refs/heads/*", false, REGISTERED_USERS, false); + grant(p2, "refs/*", Permission.SUBMIT, false, REGISTERED_USERS); + + TestRepository repo1 = cloneProject(p1); + TestRepository repo2 = cloneProject(p2); + + PushOneCommit.Result change1 = + createChange(repo1, "master", "A fresh change in repo1", "a.txt", "1", "topic-to-submit"); + approve(change1.getChangeId()); + PushOneCommit push = + pushFactory.create( + db, admin.getIdent(), repo2, "An ancestor change in repo2", "a.txt", "2"); + PushOneCommit.Result change2a = push.to("refs/for/master"); + approve(change2a.getChangeId()); + PushOneCommit.Result change2b = + createChange( + repo2, "master", "A topic-linked change in repo2", "a.txt", "2", "topic-to-submit"); + approve(change2b.getChangeId()); + + // Mark change2a private so that it's not visible to user. + gApi.changes().id(change2a.getChangeId()).setPrivate(true, "nobody should see this"); + + setApiUser(user); + + // Verify that user cannot see change2a + try { + gApi.changes().id(change2a.getChangeId()).get(); + fail("expected failure"); + } catch (ResourceNotFoundException e) { + assertThat(e.getMessage()).isEqualTo("Not found: " + change2a.getChangeId()); + } + + // Submit is expected to fail. + try { + gApi.changes().id(change1.getChangeId()).current().submit(); + fail("expected failure"); + } catch (AuthException e) { + assertThat(e.getMessage()) + .isEqualTo( + "A change to be submitted with " + + change1.getChange().getId().id + + " is not visible"); + } + assertRefUpdatedEvents(); + assertChangeMergedEvents(); } @Test