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