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
(cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33)
(cherry picked from commit 1be1d6ff45f18c978fd21e5c7d437d0a1351d7d8)

2 files changed, 41 insertions, 2 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index fefc84d73d..1b035b95b8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -39,6 +39,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupMembership;
 import com.google.gerrit.server.change.IncludedInResolver;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.config.GitReceivePackGroups;
 import com.google.gerrit.server.config.GitUploadPackGroups;
@@ -171,6 +172,7 @@ public class ProjectControl {
   @Nullable private final SearchingChangeCacheImpl changeCache;
   private final Provider<InternalChangeQuery> queryProvider;
   private final Metrics metrics;
+  private final AllUsersName allUsersName;
 
   private List<SectionMatcher> allSections;
   private List<SectionMatcher> localSections;
@@ -190,6 +192,7 @@ public class ProjectControl {
       Provider<InternalChangeQuery> queryProvider,
       @Nullable SearchingChangeCacheImpl changeCache,
       @CanonicalWebUrl @Nullable String canonicalWebUrl,
+      AllUsersName allUsersName,
       @Assisted CurrentUser who,
       @Assisted ProjectState ps,
       Metrics metrics) {
@@ -204,6 +207,7 @@ public class ProjectControl {
     this.canonicalWebUrl = canonicalWebUrl;
     this.queryProvider = queryProvider;
     this.metrics = metrics;
+    this.allUsersName = allUsersName;
     user = who;
     state = ps;
   }
@@ -318,7 +322,9 @@ public class ProjectControl {
   }
 
   public boolean allRefsAreVisible(Set<String> ignore) {
-    return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore);
+    return user.isInternalUser()
+        || (!getProject().getNameKey().equals(allUsersName)
+            && canPerformOnAllRefs(Permission.READ, ignore));
   }
 
   /** Is this user a project owner? Ownership does not imply {@link #isVisible()} */
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
index 4f2284cb40..0c3d4c28e9 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
@@ -114,6 +114,14 @@ public class RefControlTest {
     assertThat(u.isVisible()).named("can read").isTrue();
   }
 
+  private void assertAllRefsAreVisible(ProjectControl u) {
+    assertThat(u.allRefsAreVisible()).named("all refs visible").isTrue();
+  }
+
+  private void assertAllRefsAreNotVisible(ProjectControl u) {
+    assertThat(u.allRefsAreVisible()).named("all refs NOT visible").isFalse();
+  }
+
   private void assertCannotRead(ProjectControl u) {
     assertThat(u.isVisible()).named("cannot read").isFalse();
   }
@@ -189,6 +197,7 @@ public class RefControlTest {
   private final Map<Project.NameKey, ProjectState> all = new HashMap<>();
   private Project.NameKey localKey = new Project.NameKey("local");
   private ProjectConfig local;
+  private ProjectConfig allUsers;
   private Project.NameKey parentKey = new Project.NameKey("parent");
   private ProjectConfig parent;
   private InMemoryRepositoryManager repoManager;
@@ -219,7 +228,7 @@ public class RefControlTest {
 
           @Override
           public ProjectState getAllUsers() {
-            return null;
+            return get(allUsersName);
           }
 
           @Override
@@ -273,6 +282,11 @@ public class RefControlTest {
       LabelType cr = Util.codeReview();
       allProjects.getLabelSections().put(cr.getName(), cr);
       add(allProjects);
+
+      Repository allUsersRepo = repoManager.createRepository(allUsersName);
+      allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get()));
+      allUsers.load(allUsersRepo);
+      add(allUsers);
     } catch (IOException | ConfigInvalidException e) {
       throw new RuntimeException(e);
     }
@@ -347,6 +361,24 @@ public class RefControlTest {
   }
 
   @Test
+  public void allRefsAreVisibleForRegularProject() throws Exception {
+    allow(local, READ, DEVS, "refs/*");
+    allow(local, READ, DEVS, "refs/groups/*");
+    allow(local, READ, DEVS, "refs/users/default");
+
+    assertAllRefsAreVisible(user(local, DEVS));
+  }
+
+  @Test
+  public void allRefsAreNotVisibleForAllUsers() throws Exception {
+    allow(allUsers, READ, DEVS, "refs/*");
+    allow(allUsers, READ, DEVS, "refs/groups/*");
+    allow(allUsers, READ, DEVS, "refs/users/default");
+
+    assertAllRefsAreNotVisible(user(allUsers, DEVS));
+  }
+
+  @Test
   public void branchDelegation1() {
     allow(local, OWNER, ADMIN, "refs/*");
     allow(local, OWNER, DEVS, "refs/heads/x/*");
@@ -908,6 +940,7 @@ public class RefControlTest {
         queryProvider,
         null,
         canonicalWebUrl,
+        allUsersName,
         new MockUser(name, memberOf),
         newProjectState(local),
         metrics);