Add option to disable performance shortcut for world-readable repos

Gerrit's ref filter contains a performance shortcut that skips the
costly, full evaluation of refs in case all refs are readable by a user.

This commit adds a config option to disable the performance shortcut.
This will require more resources to filter refs for git-upload and
git-receive, but enables the host owner to make guarantees about
the visibility of refs.

Change-Id: I52fd61218fcb84cb71c90bf25b551323786c792c
This commit is contained in:
Patrick Hiesel
2018-10-09 13:39:54 +02:00
parent 4eac09b97a
commit 8d0770eb49
3 changed files with 30 additions and 1 deletions

View File

@@ -664,6 +664,13 @@ link:#accountDeactivation[accountDeactivation] section appropriately.
+ +
By default, false. By default, false.
[[auth.skipFullRefEvaluationIfAllRefsAreVisible]]auth.skipFullRefEvaluationIfAllRefsAreVisible::
+
Whether to skip the full ref visibility checks as a performance shortcut when all refs are
visible to a user. Full ref filtering would filter out things like pending edits.
+
By default, true.
[[cache]] [[cache]]
=== Section cache === Section cache

View File

@@ -39,6 +39,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupCache; import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.SearchingChangeCacheImpl; import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TagMatcher; import com.google.gerrit.server.git.TagMatcher;
@@ -59,6 +60,7 @@ import java.util.HashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.stream.Stream; import java.util.stream.Stream;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.Repository;
@@ -83,6 +85,7 @@ class DefaultRefFilter {
private final PermissionBackend.ForProject permissionBackendForProject; private final PermissionBackend.ForProject permissionBackendForProject;
private final Counter0 fullFilterCount; private final Counter0 fullFilterCount;
private final Counter0 skipFilterCount; private final Counter0 skipFilterCount;
private final boolean skipFullRefEvaluationIfAllRefsAreVisible;
private Map<Change.Id, Branch.NameKey> visibleChanges; private Map<Change.Id, Branch.NameKey> visibleChanges;
@@ -94,6 +97,7 @@ class DefaultRefFilter {
Provider<ReviewDb> db, Provider<ReviewDb> db,
GroupCache groupCache, GroupCache groupCache,
PermissionBackend permissionBackend, PermissionBackend permissionBackend,
@GerritServerConfig Config config,
MetricMaker metricMaker, MetricMaker metricMaker,
@Assisted ProjectControl projectControl) { @Assisted ProjectControl projectControl) {
this.tagCache = tagCache; this.tagCache = tagCache;
@@ -102,6 +106,8 @@ class DefaultRefFilter {
this.db = db; this.db = db;
this.groupCache = groupCache; this.groupCache = groupCache;
this.permissionBackend = permissionBackend; this.permissionBackend = permissionBackend;
this.skipFullRefEvaluationIfAllRefsAreVisible =
config.getBoolean("auth", "skipFullRefEvaluationIfAllRefsAreVisible", true);
this.projectControl = projectControl; this.projectControl = projectControl;
this.user = projectControl.getUser(); this.user = projectControl.getUser();
@@ -127,7 +133,7 @@ class DefaultRefFilter {
refs = addUsersSelfSymref(refs); refs = addUsersSelfSymref(refs);
} }
if (!projectState.isAllUsers()) { if (skipFullRefEvaluationIfAllRefsAreVisible && !projectState.isAllUsers()) {
if (projectState.statePermitsRead() if (projectState.statePermitsRead()
&& checkProjectPermission(permissionBackendForProject, ProjectPermission.READ)) { && checkProjectPermission(permissionBackendForProject, ProjectPermission.READ)) {
skipFilterCount.increment(); skipFilterCount.increment();

View File

@@ -588,6 +588,22 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
} }
} }
@Test
@GerritConfig(name = "auth.skipFullRefEvaluationIfAllRefsAreVisible", value = "false")
public void advertisedReferencesOmitPrivateChangesOfOtherUsersWhenShortcutDisabled()
throws Exception {
allow("refs/*", Permission.READ, REGISTERED_USERS);
TestRepository<?> userTestRepository = cloneProject(project, user);
try (Git git = userTestRepository.git()) {
String change3RefName = c3.currentPatchSet().getRefName();
assertWithMessage("Precondition violated").that(getRefs(git)).contains(change3RefName);
gApi.changes().id(c3.getId().get()).setPrivate(true, null);
assertThat(getRefs(git)).doesNotContain(change3RefName);
}
}
@Test @Test
public void advertisedReferencesOmitDraftCommentRefsOfOtherUsers() throws Exception { public void advertisedReferencesOmitDraftCommentRefsOfOtherUsers() throws Exception {
assume().that(notesMigration.commitChangeWrites()).isTrue(); assume().that(notesMigration.commitChangeWrites()).isTrue();