From c7ba2ccd6f8d6a061e75c09b6eb180843aece872 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 13 Dec 2018 10:44:38 +0100 Subject: [PATCH] Return tags on full ref evaluation for users that have READ on refs/* We are working on removing the shortcut that prevents Gerrit from performing a full ref filtering if users have READ on refs/*. Gerrit's ACLs are based on refs, not on tags. Tags are being returned if they are reachable by any of the visible refs. That poses the problem of what we do with orphaned tags. Due to the shortcut, users were able to see orphaned tags in the past. In general it seems like a sane assumption to make that when one has READ on refs/* they can see all the tags as well. This is what this commit implements. We considered adding a new ACL namespace (refs/tags/*) but that would add another layer of complexity to an already hard to understand ACL system. On top, it would make it easy for users to make code accessible by adding a tag to a commit on a hidden ref when the administrator granted READ on refs/tags/* to a broader audience. Change-Id: I8d9f6a54228ed5650ded6a285c1afbf9525511e6 --- .../server/permissions/DefaultRefFilter.java | 24 ++++++++++--- .../acceptance/git/RefAdvertisementIT.java | 36 +++++++++++++++++-- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java index 249c8722f7..ea4073da67 100644 --- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java +++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java @@ -133,9 +133,10 @@ class DefaultRefFilter { refs = addUsersSelfSymref(refs); } + boolean hasReadOnRefsStar = + checkProjectPermission(permissionBackendForProject, ProjectPermission.READ); if (skipFullRefEvaluationIfAllRefsAreVisible && !projectState.isAllUsers()) { - if (projectState.statePermitsRead() - && checkProjectPermission(permissionBackendForProject, ProjectPermission.READ)) { + if (projectState.statePermitsRead() && hasReadOnRefsStar) { skipFilterCount.increment(); return refs; } else if (projectControl.allRefsAreVisible(ImmutableSet.of(RefNames.REFS_CONFIG))) { @@ -197,9 +198,22 @@ class DefaultRefFilter { result.put(name, ref); } } else if (isTag(ref)) { - // If its a tag, consider it later. - if (ref.getObjectId() != null) { - deferredTags.add(ref); + if (hasReadOnRefsStar) { + // The user has READ on refs/*. This is the broadest permission one can assign. There is + // no way to grant access to (specific) tags in Gerrit, so we have to assume that these + // users can see all tags because there could be tags that aren't reachable by any visible + // ref while the user can see all non-Gerrit refs. This matches Gerrit's historic + // behavior. + // This makes it so that these users could see commits that they can't see otherwise + // (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on + // the regular Git tree that users interact with, not on any of the Gerrit trees, so this + // is a negligible risk. + result.put(name, ref); + } else { + // If its a tag, consider it later. + if (ref.getObjectId() != null) { + deferredTags.add(ref); + } } } else if (name.startsWith(RefNames.REFS_SEQUENCES)) { // Sequences are internal database implementation details. diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 7108a98a45..ac9958115a 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -46,13 +46,13 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.config.AllUsersName; -import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.git.receive.ReceiveCommitsAdvertiseRefsHook; import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions; import com.google.gerrit.server.project.testing.Util; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.testing.ConfigSuite; import com.google.inject.Inject; import java.io.IOException; import java.util.ArrayList; @@ -62,6 +62,7 @@ import java.util.Map; import java.util.function.Predicate; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; @@ -75,7 +76,6 @@ import org.junit.Test; public class RefAdvertisementIT extends AbstractDaemonTest { @Inject private PermissionBackend permissionBackend; @Inject private ChangeNoteUtil noteUtil; - @Inject @AnonymousCowardName private String anonymousCowardName; @Inject private AllUsersName allUsersName; private AccountGroup.UUID admins; @@ -90,6 +90,13 @@ public class RefAdvertisementIT extends AbstractDaemonTest { private String r3; private String r4; + @ConfigSuite.Config + public static Config enableFullRefEvaluation() { + Config cfg = new Config(); + cfg.setBoolean("auth", null, "skipFullRefEvaluationIfAllRefsAreVisible", false); + return cfg; + } + @Before public void setUp() throws Exception { admins = adminGroupUuid(); @@ -376,6 +383,28 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } } + @Test + public void uploadPackAllRefsAreVisibleOrphanedTag() throws Exception { + allow("refs/*", Permission.READ, REGISTERED_USERS); + // Delete the pending change on 'branch' and 'branch' itself so that the tag gets orphaned + gApi.changes().id(c4.getId().id).delete(); + gApi.projects().name(project.get()).branch("refs/heads/branch").delete(); + + setApiUser(user); + assertUploadPackRefs( + "HEAD", + "refs/meta/config", + r1 + "1", + r1 + "meta", + r2 + "1", + r2 + "meta", + r3 + "1", + r3 + "meta", + "refs/heads/master", + "refs/tags/branch-tag", + "refs/tags/master-tag"); + } + @Test public void receivePackListsOpenChangesAsAdditionalHaves() throws Exception { ReceiveCommitsAdvertiseRefsHook.Result r = getReceivePackRefs(); @@ -565,6 +594,9 @@ public class RefAdvertisementIT extends AbstractDaemonTest { @Test public void advertisedReferencesIncludePrivateChangesWhenAllRefsMayBeRead() throws Exception { + assume() + .that(baseConfig.getBoolean("auth", "skipFullRefEvaluationIfAllRefsAreVisible", true)) + .isTrue(); allow("refs/*", Permission.READ, REGISTERED_USERS); TestRepository userTestRepository = cloneProject(project, user);