Workaround Gitiles bug on All-Users visibility

Gitiles has special FilteredRepository wrapper that
allows to carefully hide refs based on the project's ACLs.
There is however an optimisation that skips the filtering
in case a user has READ permissions on every ACLs patterns.

When the target repository is All-Users, the optimisation
turns into a security issue because it allows seeing everything
that belongs to everyone:
- draft comments
- PII of all users
- external ids
- draft edits

Block Gitiles or any other part of Gerrit to abuse of this
power when the target repository is All-Users, where nobody
can be authorised to skip the ACLs evaluation.

Cover the additional special case of the All-Users project
access with two explicit positive and negative tests,
so that the security check is covered.

Bug: Issue 13621
Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7

2 files changed, 48 insertions, 1 deletions

diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index 52478a4d5e..c0fbf364de 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.conditions.BooleanCondition;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.GroupMembership;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GitReceivePackGroups;
 import com.google.gerrit.server.config.GitUploadPackGroups;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -76,6 +77,7 @@ class ProjectControl {
   private final ChangeControl.Factory changeControlFactory;
   private final PermissionCollection.Factory permissionFilter;
   private final DefaultRefFilter.Factory refFilterFactory;
+  private final AllUsersName allUsersName;
 
   private List<SectionMatcher> allSections;
   private Map<String, RefControl> refControls;
@@ -91,6 +93,7 @@ class ProjectControl {
       RefVisibilityControl refVisibilityControl,
       GitRepositoryManager repositoryManager,
       DefaultRefFilter.Factory refFilterFactory,
+      AllUsersName allUsersName,
       @Assisted CurrentUser who,
       @Assisted ProjectState ps) {
     this.changeControlFactory = changeControlFactory;
@@ -101,6 +104,7 @@ class ProjectControl {
     this.refVisibilityControl = refVisibilityControl;
     this.repositoryManager = repositoryManager;
     this.refFilterFactory = refFilterFactory;
+    this.allUsersName = allUsersName;
     user = who;
     state = ps;
   }
@@ -176,7 +180,9 @@ class ProjectControl {
   }
 
   boolean allRefsAreVisible(Set<String> ignore) {
-    return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore);
+    return user.isInternalUser()
+        || (!getProject().getNameKey().equals(allUsersName)
+            && canPerformOnAllRefs(Permission.READ, ignore));
   }
 
   /** Can the user run upload pack? */
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index 7f8f2592a4..6a1c037acf 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -47,6 +47,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.GroupMembership;
 import com.google.gerrit.server.account.ListGroupMembership;
 import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.meta.MetaDataUpdate;
 import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener;
 import com.google.gerrit.server.project.ProjectCache;
@@ -62,6 +63,7 @@ import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Optional;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Repository;
@@ -89,6 +91,18 @@ public class RefControlTest {
     assertWithMessage("not owner").that(u.isOwner()).isFalse();
   }
 
+  private void assertAllRefsAreVisible(ProjectControl u) {
+    assertWithMessage("all refs visible")
+        .that(u.allRefsAreVisible(Collections.emptySet()))
+        .isTrue();
+  }
+
+  private void assertAllRefsAreNotVisible(ProjectControl u) {
+    assertWithMessage("all refs NOT visible")
+        .that(u.allRefsAreVisible(Collections.emptySet()))
+        .isFalse();
+  }
+
   private void assertNotOwner(String ref, ProjectControl u) {
     assertWithMessage("NOT OWN " + ref).that(u.controlForRef(ref).isOwner()).isFalse();
   }
@@ -180,6 +194,7 @@ public class RefControlTest {
   private final Project.NameKey parentKey = Project.nameKey("parent");
 
   @Inject private AllProjectsName allProjectsName;
+  @Inject private AllUsersName allUsersName;
   @Inject private InMemoryRepositoryManager repoManager;
   @Inject private MetaDataUpdate.Server metaDataUpdateFactory;
   @Inject private ProjectCache projectCache;
@@ -271,6 +286,32 @@ public class RefControlTest {
   }
 
   @Test
+  public void allRefsAreVisibleForRegularProject() throws Exception {
+    projectOperations
+        .project(localKey)
+        .forUpdate()
+        .add(allow(READ).ref("refs/*").group(DEVS))
+        .add(allow(READ).ref("refs/groups/*").group(DEVS))
+        .add(allow(READ).ref("refs/users/default").group(DEVS))
+        .update();
+
+    assertAllRefsAreVisible(user(localKey, DEVS));
+  }
+
+  @Test
+  public void allRefsAreNotVisibleForAllUsers() throws Exception {
+    projectOperations
+        .project(allUsersName)
+        .forUpdate()
+        .add(allow(READ).ref("refs/*").group(DEVS))
+        .add(allow(READ).ref("refs/groups/*").group(DEVS))
+        .add(allow(READ).ref("refs/users/default").group(DEVS))
+        .update();
+
+    assertAllRefsAreNotVisible(user(allUsersName, DEVS));
+  }
+
+  @Test
   public void branchDelegation1() throws Exception {
     projectOperations
         .project(localKey)