Merge branch 'stable-3.0' into stable-3.1

* stable-3.0:
  ChangeQueryBuilder: Use ChangeIsVisibleToPredicate.Factory
  ChangeQueryProcessor: Use ChangeIsVisibleToPredicate.Factory
  Don't inject CurrentUser to ChangeIsVisibleToPredicate

Change-Id: I2edad75127bd8425073fdb28ddfa1dff933e1177

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

@@ -20,9 +20,13 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
+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.entities.Patch.COMMIT_MSG;
 import static com.google.gerrit.entities.Patch.MERGE_LIST;
 import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES;
+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.TestLabels.label;
 import static com.google.gerrit.server.project.testing.TestLabels.value;
 import static java.nio.charset.StandardCharsets.UTF_8;
@@ -40,6 +44,7 @@ import com.google.common.jimfs.Jimfs;
 import com.google.common.primitives.Chars;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
 import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.AccessSection;
@@ -290,6 +295,7 @@ public abstract class AbstractDaemonTest {
   @Inject private ProjectIndexCollection projectIndexes;
   @Inject private RequestScopeOperations requestScopeOperations;
   @Inject private SitePaths sitePaths;
+  @Inject private ProjectOperations projectOperations;
 
   private ProjectResetter resetter;
   private List<Repository> toClose;
@@ -970,6 +976,16 @@ public abstract class AbstractDaemonTest {
     }
   }
 
+  protected void blockAnonymousRead() throws Exception {
+    String allRefs = RefNames.REFS + "*";
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(block(Permission.READ).ref(allRefs).group(ANONYMOUS_USERS))
+        .add(allow(Permission.READ).ref(allRefs).group(REGISTERED_USERS))
+        .update();
+  }
+
   protected PushOneCommit.Result pushTo(String ref) throws Exception {
     PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
     return push.to(ref);

java/com/google/gerrit/pgm/util/BatchProgramModule.java

@@ -74,6 +74,7 @@ import com.google.gerrit.server.project.ProjectCacheImpl;
 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.restapi.group.GroupModule;
 import com.google.gerrit.server.rules.DefaultSubmitRule;
 import com.google.gerrit.server.rules.IgnoreSelfApprovalRule;
@@ -164,6 +165,7 @@ public class BatchProgramModule extends FactoryModule {
     install(PureRevertCache.module());
     factory(CapabilityCollection.Factory.class);
     factory(ChangeData.AssistedFactory.class);
+    factory(ChangeIsVisibleToPredicate.Factory.class);
     factory(ProjectState.Factory.class);
 
     // Submit rules

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

@@ -167,6 +167,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;
@@ -256,6 +257,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

@@ -46,7 +46,6 @@ import com.google.gerrit.index.query.QueryBuilder;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.index.query.QueryRequiresAuthException;
 import com.google.gerrit.mail.Address;
-import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.CommentsUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
@@ -68,7 +67,6 @@ import com.google.gerrit.server.index.change.ChangeField;
 import com.google.gerrit.server.index.change.ChangeIndex;
 import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeIndexRewriter;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -202,7 +200,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     final ChangeData.Factory changeDataFactory;
     final ChangeIndex index;
     final ChangeIndexRewriter rewriter;
-    final ChangeNotes.Factory notesFactory;
     final CommentsUtil commentsUtil;
     final ConflictsCache conflictsCache;
     final DynamicMap<ChangeHasOperandFactory> hasOperands;
@@ -218,7 +215,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     final StarredChangesUtil starredChangesUtil;
     final SubmitDryRun submitDryRun;
     final GroupMembers groupMembers;
-    final Provider<AnonymousUser> anonymousUserProvider;
+    final ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory;
 
     private final Provider<CurrentUser> self;
 
@@ -232,7 +229,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
         IdentifiedUser.GenericFactory userFactory,
         Provider<CurrentUser> self,
         PermissionBackend permissionBackend,
-        ChangeNotes.Factory notesFactory,
         ChangeData.Factory changeDataFactory,
         CommentsUtil commentsUtil,
         AccountResolver accountResolver,
@@ -250,7 +246,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
         StarredChangesUtil starredChangesUtil,
         AccountCache accountCache,
         GroupMembers groupMembers,
-        Provider<AnonymousUser> anonymousUserProvider) {
+        ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory) {
       this(
           queryProvider,
           rewriter,
@@ -259,7 +255,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
           userFactory,
           self,
           permissionBackend,
-          notesFactory,
           changeDataFactory,
           commentsUtil,
           accountResolver,
@@ -277,7 +272,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
           starredChangesUtil,
           accountCache,
           groupMembers,
-          anonymousUserProvider);
+          changeIsVisbleToPredicateFactory);
     }
 
     private Arguments(
@@ -288,7 +283,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
         IdentifiedUser.GenericFactory userFactory,
         Provider<CurrentUser> self,
         PermissionBackend permissionBackend,
-        ChangeNotes.Factory notesFactory,
         ChangeData.Factory changeDataFactory,
         CommentsUtil commentsUtil,
         AccountResolver accountResolver,
@@ -306,14 +300,13 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
         StarredChangesUtil starredChangesUtil,
         AccountCache accountCache,
         GroupMembers groupMembers,
-        Provider<AnonymousUser> anonymousUserProvider) {
+        ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory) {
       this.queryProvider = queryProvider;
       this.rewriter = rewriter;
       this.opFactories = opFactories;
       this.userFactory = userFactory;
       this.self = self;
       this.permissionBackend = permissionBackend;
-      this.notesFactory = notesFactory;
       this.changeDataFactory = changeDataFactory;
       this.commentsUtil = commentsUtil;
       this.accountResolver = accountResolver;
@@ -332,7 +325,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
       this.accountCache = accountCache;
       this.hasOperands = hasOperands;
       this.groupMembers = groupMembers;
-      this.anonymousUserProvider = anonymousUserProvider;
+      this.changeIsVisbleToPredicateFactory = changeIsVisbleToPredicateFactory;
     }
 
     Arguments asUser(CurrentUser otherUser) {
@@ -344,7 +337,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
           userFactory,
           Providers.of(otherUser),
           permissionBackend,
-          notesFactory,
           changeDataFactory,
           commentsUtil,
           accountResolver,
@@ -362,7 +354,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
           starredChangesUtil,
           accountCache,
           groupMembers,
-          anonymousUserProvider);
+          changeIsVisbleToPredicateFactory);
     }
 
     Arguments asUser(Account.Id otherId) {
@@ -974,12 +966,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   }
 
   public Predicate<ChangeData> visibleto(CurrentUser user) {
-    return new ChangeIsVisibleToPredicate(
+    return args.changeIsVisbleToPredicateFactory.forUser(user);
-        args.notesFactory,
-        user,
-        args.permissionBackend,
-        args.projectCache,
-        args.anonymousUserProvider);
   }
 
   public Predicate<ChangeData> isVisible() throws QueryParseException {

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

@@ -28,7 +28,6 @@ import com.google.gerrit.index.query.IndexPredicate;
 import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.QueryProcessor;
 import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.DynamicOptions;
 import com.google.gerrit.server.DynamicOptions.DynamicBean;
@@ -40,9 +39,6 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeIndexRewriter;
 import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
 import com.google.gerrit.server.index.change.IndexedChangeQuery;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.project.ProjectCache;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import java.util.HashMap;
@@ -58,11 +54,8 @@ import java.util.Set;
 public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
     implements DynamicOptions.BeanReceiver, DynamicOptions.BeanProvider {
   private final Provider<CurrentUser> userProvider;
-  private final ChangeNotes.Factory notesFactory;
   private final ImmutableListMultimap<String, ChangeAttributeFactory> attributeFactoriesByPlugin;
-  private final PermissionBackend permissionBackend;
+  private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory;
-  private final ProjectCache projectCache;
-  private final Provider<AnonymousUser> anonymousUserProvider;
   private final Map<String, DynamicBean> dynamicBeans = new HashMap<>();
 
   static {
@@ -80,11 +73,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
       IndexConfig indexConfig,
       ChangeIndexCollection indexes,
       ChangeIndexRewriter rewriter,
-      ChangeNotes.Factory notesFactory,
       DynamicSet<ChangeAttributeFactory> attributeFactories,
-      PermissionBackend permissionBackend,
+      ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory) {
-      ProjectCache projectCache,
-      Provider<AnonymousUser> anonymousUserProvider) {
     super(
         metricMaker,
         ChangeSchemaDefinitions.INSTANCE,
@@ -94,10 +84,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
         FIELD_LIMIT,
         () -> limitsFactory.create(userProvider.get()).getQueryLimit());
     this.userProvider = userProvider;
-    this.notesFactory = notesFactory;
+    this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory;
-    this.permissionBackend = permissionBackend;
-    this.projectCache = projectCache;
-    this.anonymousUserProvider = anonymousUserProvider;
 
     ImmutableListMultimap.Builder<String, ChangeAttributeFactory> factoriesBuilder =
         ImmutableListMultimap.builder();
@@ -144,14 +131,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
   @Override
   protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) {
     return new AndChangeSource(
-        pred,
+        pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start);
-        new ChangeIsVisibleToPredicate(
-            notesFactory,
-            userProvider.get(),
-            permissionBackend,
-            projectCache,
-            anonymousUserProvider),
-        start);
   }
 
   @Override

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

@@ -89,20 +89,20 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   private final Map<QueryKey, ImmutableList<ChangeData>> queryCache;
   private final Map<BranchNameKey, 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
@@ -150,7 +150,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());
     }
@@ -209,13 +210,19 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation {
   }
 
   private ChangeSet byCommitsOnBranchNotMerged(
-      OpenRepo or, BranchNameKey branch, Set<String> visibleHashes, Set<String> nonVisibleHashes)
+      OpenRepo or,
+      BranchNameKey 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

@@ -22,7 +22,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.acceptance.TestAccount;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -53,13 +52,18 @@ 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 abstract class AbstractSubmitOnPush extends AbstractDaemonTest {
-public class SubmitOnPushIT extends AbstractDaemonTest {
   @Inject private ApprovalsUtil approvalsUtil;
   @Inject private ProjectOperations projectOperations;
 
+  @Before
+  public void blockAnonymous() throws Exception {
+    blockAnonymousRead();
+  }
+
   @Test
   public void submitOnPush() throws Exception {
     projectOperations

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);
+  }
+}

javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java

@@ -27,7 +27,7 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
         new ChangeQueryBuilder.Definition<>(FakeQueryBuilder.class),
         new ChangeQueryBuilder.Arguments(
             null, null, null, null, null, null, null, null, null, null, null, null, null, null,
-            null, null, null, null, indexes, null, null, null, null, null, null, null));
+            null, null, null, indexes, null, null, null, null, null, null, null));
   }
 
   @Operator