diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 76bfef8569..425bb88e80 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/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 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); diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java index 359321a79a..ce2b05df69 100644 --- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java +++ b/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 diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index a7cede66b4..e17fb02a05 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/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); diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java index 78f0c61149..14bb00c5ec 100644 --- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java +++ b/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 { 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 @Inject public ChangeIsVisibleToPredicate( ChangeNotes.Factory notesFactory, - CurrentUser user, PermissionBackend permissionBackend, ProjectCache projectCache, - Provider anonymousUserProvider) { + Provider anonymousUserProvider, + @Assisted CurrentUser user) { super(ChangeQueryBuilder.FIELD_VISIBLETO, IndexUtils.describe(user)); this.notesFactory = notesFactory; this.user = user; diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 28105f3507..df729cb63c 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/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 hasOperands; @@ -218,7 +215,7 @@ public class ChangeQueryBuilder extends QueryBuilder anonymousUserProvider; + final ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory; private final Provider self; @@ -232,7 +229,6 @@ public class ChangeQueryBuilder extends QueryBuilder self, PermissionBackend permissionBackend, - ChangeNotes.Factory notesFactory, ChangeData.Factory changeDataFactory, CommentsUtil commentsUtil, AccountResolver accountResolver, @@ -250,7 +246,7 @@ public class ChangeQueryBuilder extends QueryBuilder anonymousUserProvider) { + ChangeIsVisibleToPredicate.Factory changeIsVisbleToPredicateFactory) { this( queryProvider, rewriter, @@ -259,7 +255,6 @@ public class ChangeQueryBuilder extends QueryBuilder self, PermissionBackend permissionBackend, - ChangeNotes.Factory notesFactory, ChangeData.Factory changeDataFactory, CommentsUtil commentsUtil, AccountResolver accountResolver, @@ -306,14 +300,13 @@ public class ChangeQueryBuilder extends QueryBuilder 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 visibleto(CurrentUser user) { - return new ChangeIsVisibleToPredicate( - args.notesFactory, - user, - args.permissionBackend, - args.projectCache, - args.anonymousUserProvider); + return args.changeIsVisbleToPredicateFactory.forUser(user); } public Predicate isVisible() throws QueryParseException { diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java index f9263a9922..40c04773ed 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java +++ b/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 implements DynamicOptions.BeanReceiver, DynamicOptions.BeanProvider { private final Provider userProvider; - private final ChangeNotes.Factory notesFactory; private final ImmutableListMultimap attributeFactoriesByPlugin; - private final PermissionBackend permissionBackend; - private final ProjectCache projectCache; - private final Provider anonymousUserProvider; + private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory; private final Map dynamicBeans = new HashMap<>(); static { @@ -80,11 +73,8 @@ public class ChangeQueryProcessor extends QueryProcessor IndexConfig indexConfig, ChangeIndexCollection indexes, ChangeIndexRewriter rewriter, - ChangeNotes.Factory notesFactory, DynamicSet attributeFactories, - PermissionBackend permissionBackend, - ProjectCache projectCache, - Provider anonymousUserProvider) { + ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory) { super( metricMaker, ChangeSchemaDefinitions.INSTANCE, @@ -94,10 +84,7 @@ public class ChangeQueryProcessor extends QueryProcessor FIELD_LIMIT, () -> limitsFactory.create(userProvider.get()).getQueryLimit()); this.userProvider = userProvider; - this.notesFactory = notesFactory; - this.permissionBackend = permissionBackend; - this.projectCache = projectCache; - this.anonymousUserProvider = anonymousUserProvider; + this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory; ImmutableListMultimap.Builder factoriesBuilder = ImmutableListMultimap.builder(); @@ -144,14 +131,7 @@ public class ChangeQueryProcessor extends QueryProcessor @Override protected Predicate enforceVisibility(Predicate pred) { return new AndChangeSource( - pred, - new ChangeIsVisibleToPredicate( - notesFactory, - userProvider.get(), - permissionBackend, - projectCache, - anonymousUserProvider), - start); + pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start); } @Override diff --git a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java index 7492cf7248..ec6b35a243 100644 --- a/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java +++ b/java/com/google/gerrit/server/submit/LocalMergeSuperSetComputation.java @@ -89,20 +89,20 @@ public class LocalMergeSuperSetComputation implements MergeSuperSetComputation { private final Map> queryCache; private final Map> heads; private final ProjectCache projectCache; - private final ChangeIsVisibleToPredicate changeIsVisibleToPredicate; + private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory; @Inject LocalMergeSuperSetComputation( PermissionBackend permissionBackend, Provider 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 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 visibleHashes, Set nonVisibleHashes) + OpenRepo or, + BranchNameKey branch, + Set visibleHashes, + Set nonVisibleHashes, + CurrentUser user) throws IOException { List potentiallyVisibleChanges = byCommitsOnBranchNotMerged(or, branch, visibleHashes); List invisibleChanges = new ArrayList<>(byCommitsOnBranchNotMerged(or, branch, nonVisibleHashes)); List visibleChanges = new ArrayList<>(potentiallyVisibleChanges.size()); + ChangeIsVisibleToPredicate changeIsVisibleToPredicate = + changeIsVisibleToPredicateFactory.forUser(user); for (ChangeData cd : potentiallyVisibleChanges) { if (changeIsVisibleToPredicate.match(cd)) { visibleChanges.add(cd); diff --git a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java similarity index 99% rename from javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java rename to javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java index 28c9a9eb33..7cfb0f2855 100644 --- a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/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 class SubmitOnPushIT extends AbstractDaemonTest { +public abstract class AbstractSubmitOnPush extends AbstractDaemonTest { @Inject private ApprovalsUtil approvalsUtil; @Inject private ProjectOperations projectOperations; + @Before + public void blockAnonymous() throws Exception { + blockAnonymousRead(); + } + @Test public void submitOnPush() throws Exception { projectOperations diff --git a/javatests/com/google/gerrit/acceptance/git/HttpSubmitOnPushIT.java b/javatests/com/google/gerrit/acceptance/git/HttpSubmitOnPushIT.java new file mode 100644 index 0000000000..373341536f --- /dev/null +++ b/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()); + } +} diff --git a/javatests/com/google/gerrit/acceptance/git/SshSubmitOnPushIT.java b/javatests/com/google/gerrit/acceptance/git/SshSubmitOnPushIT.java new file mode 100644 index 0000000000..3a18257f15 --- /dev/null +++ b/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); + } +} diff --git a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java b/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java index 0753127b52..a713221a45 100644 --- a/javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java +++ b/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