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

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<ChangeData>
   protected final ProjectCache projectCache;
   private final Provider<AnonymousUser> anonymousUserProvider;
 
+  @Inject
   public ChangeIsVisibleToPredicate(
       Provider<ReviewDb> db,
       ChangeNotes.Factory notesFactory,

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<InternalChangeQuery> queryProvider;
-  private final Map<QueryKey, List<ChangeData>> queryCache;
+  private final Map<QueryKey, ImmutableList<ChangeData>> queryCache;
   private final Map<Branch.NameKey, Optional<RevCommit>> heads;
   private final ProjectCache projectCache;
+  private final ChangeIsVisibleToPredicate changeIsVisibleToPredicate;
 
   @Inject
   LocalMergeSuperSetComputation(
       PermissionBackend permissionBackend,
       Provider<InternalChangeQuery> 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<String> visibleHashes =
           walkChangesByHashes(visibleCommits, Collections.emptySet(), or, b);
-      Iterables.addAll(visibleChanges, byCommitsOnBranchNotMerged(or, db, b, visibleHashes));
-
       Set<String> 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<ChangeData> byCommitsOnBranchNotMerged(
+  private ChangeSet byCommitsOnBranchNotMerged(
+      OpenRepo or,
+      ReviewDb db,
+      Branch.NameKey branch,
+      Set<String> visibleHashes,
+      Set<String> nonVisibleHashes)
+      throws OrmException, IOException {
+    List<ChangeData> potentiallyVisibleChanges =
+        byCommitsOnBranchNotMerged(or, db, branch, visibleHashes);
+    List<ChangeData> invisibleChanges =
+        new ArrayList<>(byCommitsOnBranchNotMerged(or, db, branch, nonVisibleHashes));
+    List<ChangeData> 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<ChangeData> byCommitsOnBranchNotMerged(
       OpenRepo or, ReviewDb db, Branch.NameKey branch, Set<String> hashes)
       throws OrmException, IOException {
     if (hashes.isEmpty()) {
       return ImmutableList.of();
     }
     QueryKey k = QueryKey.create(branch, hashes);
-    List<ChangeData> cached = queryCache.get(k);
+    if (queryCache.containsKey(k)) {
-    if (cached != null) {
+      return queryCache.get(k);
-      return cached;
-    }
-
-    List<ChangeData> result = new ArrayList<>();
-    Iterable<ChangeData> destChanges =
-        queryProvider.get().byCommitsOnBranchNotMerged(or.repo, db, branch, hashes);
-    for (ChangeData chd : destChanges) {
-      result.add(chd);
     }
+    ImmutableList<ChangeData> result =
+        ImmutableList.copyOf(
+            queryProvider.get().byCommitsOnBranchNotMerged(or.repo, db, branch, hashes));
     queryCache.put(k, result);
     return result;
   }

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
+    // Submit is expected to fail.
-    // the user. We would expect the submit to fail, but instead the submit succeeds and the hidden
+    try {
-    // change gets submitted too.
+      gApi.changes().id(change2Result.getChangeId()).current().submit();
-    // TODO(ekempin): Make this submit fail.
+      fail("expected failure");
-    gApi.changes().id(change2Result.getChangeId()).current().submit(new SubmitInput());
+    } 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.
+  @Test
-    setApiUser(admin);
+  public void dependencyOnHiddenChangeUsingTopicPreventsMerge() throws Exception {
-    assertThat(gApi.changes().id(changeResult.getChangeId()).get().status)
+    // Construct a topic where a change included by topic depends on a private change that is not
-        .isEqualTo(ChangeStatus.MERGED);
+    // visible to the submitting user
-    assertThat(gApi.changes().id(change2Result.getChangeId()).get().status)
+    // (c1) --- topic --- (c2b)
-        .isEqualTo(ChangeStatus.MERGED);
+    //                      |
+    //                    (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