Don't inject CurrentUser to ChangeIsVisibleToPredicate

CurrentUser is not set to IdentifiedUser in ReceiveCommits when
using HTTP.
This causes ISE when pushing to refs/for/target%submit over HTTP when
Anonymous Users cannot read target.

Tests:
* Run submit on push tests in both SSH and HTTP.
* Block READ for Anonymous Users in submit on push tests.

Bug: Issue 12561
Change-Id: I1c79f18a65ad9816a182dc00e403dc93c3e748b6

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -22,6 +22,7 @@ import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
 import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
 import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 import static com.google.gerrit.server.project.testing.Util.category;
 import static com.google.gerrit.server.project.testing.Util.value;
@@ -1059,6 +1060,17 @@ public abstract class AbstractDaemonTest {
     block(ref, Permission.READ, REGISTERED_USERS);
   }
 
+  protected void blockAnonymousRead() throws Exception {
+    AccountGroup.UUID anonymous = systemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
+    AccountGroup.UUID registered = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+    String allRefs = RefNames.REFS + "*";
+    try (ProjectConfigUpdate u = updateProject(project)) {
+      Util.block(u.getConfig(), Permission.READ, anonymous, allRefs);
+      Util.allow(u.getConfig(), Permission.READ, registered, allRefs);
+      u.save();
+    }
+  }
+
   protected PushOneCommit.Result pushTo(String ref) throws Exception {
     PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
     return push.to(ref);

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -160,6 +160,7 @@ import com.google.gerrit.server.project.ProjectNameLockManager;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeIsVisibleToPredicate;
 import com.google.gerrit.server.query.change.ChangeQueryBuilder;
 import com.google.gerrit.server.query.change.ConflictsCacheImpl;
 import com.google.gerrit.server.quota.QuotaEnforcer;
@@ -249,6 +250,7 @@ public class GerritGlobalModule extends FactoryModule {
     factory(CapabilityCollection.Factory.class);
     factory(ChangeData.AssistedFactory.class);
     factory(ChangeJson.AssistedFactory.class);
+    factory(ChangeIsVisibleToPredicate.Factory.class);
     factory(LabelsJson.Factory.class);
     factory(MergeUtil.Factory.class);
     factory(PatchScriptFactory.Factory.class);

java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java

@@ -30,12 +30,17 @@ import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 
 public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData> {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  public interface Factory {
+    ChangeIsVisibleToPredicate forUser(CurrentUser user);
+  }
+
   protected final ChangeNotes.Factory notesFactory;
   protected final CurrentUser user;
   protected final PermissionBackend permissionBackend;
@@ -45,10 +50,10 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
   @Inject
   public ChangeIsVisibleToPredicate(
       ChangeNotes.Factory notesFactory,
-      CurrentUser user,
       PermissionBackend permissionBackend,
       ProjectCache projectCache,
-      Provider<AnonymousUser> anonymousUserProvider) {
+      Provider<AnonymousUser> anonymousUserProvider,
+      @Assisted CurrentUser user) {
     super(ChangeQueryBuilder.FIELD_VISIBLETO, IndexUtils.describe(user));
     this.notesFactory = notesFactory;
     this.user = user;

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -989,10 +989,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   public Predicate<ChangeData> visibleto(CurrentUser user) {
     return new ChangeIsVisibleToPredicate(
         args.notesFactory,
-        user,
         args.permissionBackend,
         args.projectCache,
-        args.anonymousUserProvider);
+        args.anonymousUserProvider,
+        user);
   }
 
   public Predicate<ChangeData> isVisible() throws QueryParseException {

java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java

@@ -147,10 +147,10 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
         pred,
         new ChangeIsVisibleToPredicate(
             notesFactory,
-            userProvider.get(),
             permissionBackend,
             projectCache,
-            anonymousUserProvider),
+            anonymousUserProvider,
+            userProvider.get()),
         start);
   }
 

java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java

@@ -90,20 +90,20 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   private final Map<QueryKey, ImmutableList<ChangeData>> queryCache;
   private final Map<Branch.NameKey, Optional<RevCommit>> heads;
   private final ProjectCache projectCache;
-  private final ChangeIsVisibleToPredicate changeIsVisibleToPredicate;
+  private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory;
 
   @Inject
   LocalMergeSuperSetComputation(
       PermissionBackend permissionBackend,
       Provider<InternalChangeQuery> queryProvider,
       ProjectCache projectCache,
-      ChangeIsVisibleToPredicate changeIsVisibleToPredicate) {
+      ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory) {
     this.projectCache = projectCache;
     this.permissionBackend = permissionBackend;
     this.queryProvider = queryProvider;
     this.queryCache = new HashMap<>();
     this.heads = new HashMap<>();
-    this.changeIsVisibleToPredicate = changeIsVisibleToPredicate;
+    this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory;
   }
 
   @Override
@@ -152,7 +152,8 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
           walkChangesByHashes(visibleCommits, Collections.emptySet(), or, b);
       Set<String> nonVisibleHashes = walkChangesByHashes(nonVisibleCommits, visibleHashes, or, b);
 
-      ChangeSet partialSet = byCommitsOnBranchNotMerged(or, b, visibleHashes, nonVisibleHashes);
+      ChangeSet partialSet =
+          byCommitsOnBranchNotMerged(or, b, visibleHashes, nonVisibleHashes, user);
       Iterables.addAll(visibleChanges, partialSet.changes());
       Iterables.addAll(nonVisibleChanges, partialSet.nonVisibleChanges());
     }
@@ -211,13 +212,19 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   }
 
   private ChangeSet byCommitsOnBranchNotMerged(
-      OpenRepo or, Branch.NameKey branch, Set<String> visibleHashes, Set<String> nonVisibleHashes)
+      OpenRepo or,
+      Branch.NameKey branch,
+      Set<String> visibleHashes,
+      Set<String> nonVisibleHashes,
+      CurrentUser user)
       throws IOException {
     List<ChangeData> potentiallyVisibleChanges =
         byCommitsOnBranchNotMerged(or, branch, visibleHashes);
     List<ChangeData> invisibleChanges =
         new ArrayList<>(byCommitsOnBranchNotMerged(or, branch, nonVisibleHashes));
     List<ChangeData> visibleChanges = new ArrayList<>(potentiallyVisibleChanges.size());
+    ChangeIsVisibleToPredicate changeIsVisibleToPredicate =
+        changeIsVisibleToPredicateFactory.forUser(user);
     for (ChangeData cd : potentiallyVisibleChanges) {
       if (changeIsVisibleToPredicate.match(cd)) {
         visibleChanges.add(cd);

javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java

@@ -21,7 +21,6 @@ import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.reviewdb.client.Change;
@@ -44,12 +43,17 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.RefSpec;
 import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.junit.Before;
 import org.junit.Test;
 
-@NoHttpd
-public class SubmitOnPushIT extends AbstractDaemonTest {
+public abstract class AbstractSubmitOnPush extends AbstractDaemonTest {
   @Inject private ApprovalsUtil approvalsUtil;
 
+  @Before
+  public void blockAnonymous() throws Exception {
+    blockAnonymousRead();
+  }
+
   @Test
   public void submitOnPush() throws Exception {
     grant(project, "refs/for/refs/heads/master", Permission.SUBMIT);

javatests/com/google/gerrit/acceptance/git/HttpSubmitOnPushIT.java

@@ -0,0 +1,30 @@
+// 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.acceptance.git;
+
+import com.google.gerrit.acceptance.GitUtil;
+import org.eclipse.jgit.transport.CredentialsProvider;
+import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider;
+import org.junit.Before;
+
+public class HttpSubmitOnPushIT extends AbstractSubmitOnPush {
+
+  @Before
+  public void cloneProjectOverHttp() throws Exception {
+    CredentialsProvider.setDefault(
+        new UsernamePasswordCredentialsProvider(admin.username(), admin.httpPassword()));
+    testRepo = GitUtil.cloneProject(project, admin.getHttpUrl(server) + "/a/" + project.get());
+  }
+}

javatests/com/google/gerrit/acceptance/git/SshSubmitOnPushIT.java

@@ -0,0 +1,27 @@
+// 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.acceptance.git;
+
+import com.google.gerrit.acceptance.NoHttpd;
+import org.junit.Before;
+
+@NoHttpd
+public class SshSubmitOnPushIT extends AbstractSubmitOnPush {
+
+  @Before
+  public void cloneProjectOverSsh() throws Exception {
+    testRepo = cloneProject(project, admin);
+  }
+}