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

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()
+      if (projectState.statePermitsRead() && hasReadOnRefsStar) {
-          && checkProjectPermission(permissionBackendForProject, ProjectPermission.READ)) {
         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 (hasReadOnRefsStar) {
-        if (ref.getObjectId() != null) {
+          // The user has READ on refs/*. This is the broadest permission one can assign. There is
-          deferredTags.add(ref);
+          // 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.

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