Fix permissions bug caused by directly inheriting from All-Projects

If any project on a server inherited explicitly from All-Projects
using its project.config, for example:

  [access]
    inheritFrom = All-Projects

and this project's permissions were evaluated early in the lifetime
of a server's process, it could poison the permission_sort cache to
include access sections from All-Projects twice, while ignoring an
identically named section from a child project that inherits from
All-Projects indirectly by not having an explicitly declared parent.

A unit test has been added to RefControlTest that sets up this case,
and thereby poisons the permission_sort cache during the first test
of isVisible(). A second test with an unrelated project would fail
to consider the READ permission on refs/* for group devs. Without
fixes to ProjectState or SectionSortCache this test would fail.

ProjectState incorrectly added All-Projects twice, because it did
not consider after the loop exited that the project may have been
added inside of the loop through an explicit parent reference. Fix
this by tracking the All-Projects name in the seen collection just
like any other parent reference.

SectionSortCache can be poisoned by being fed an input List where
the same AccessSection reference exists in multiple positions. If
this happens, consider the cache entry to be poison and do not put
the sorted result into the cache. Instead always sort these lists
on the fly.

Change-Id: I2c9d9ccd6b4fb991d317f9709428e681e5bde104

3 files changed, 28 insertions, 4 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
index 9f6f6e71a8..a72fc1618d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
@@ -197,6 +197,7 @@ public class ProjectState {
 
     List<SectionMatcher> all = new ArrayList<SectionMatcher>();
     Set<Project.NameKey> seen = new HashSet<Project.NameKey>();
+    ProjectState allProjects = projectCache.getAllProjects();
     seen.add(getProject().getNameKey());
 
     ProjectState s = this;
@@ -209,7 +210,9 @@ public class ProjectState {
       }
       s = projectCache.get(parent);
     } while (s != null);
-    all.addAll(projectCache.getAllProjects().getLocalAccessSections());
+    if (seen.add(allProjects.getProject().getNameKey())) {
+      all.addAll(allProjects.getLocalAccessSections());
+    }
     return all;
   }
 
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SectionSortCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SectionSortCache.java
index c0d2034cf6..2bc957ebcf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SectionSortCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SectionSortCache.java
@@ -28,6 +28,8 @@ import com.google.inject.TypeLiteral;
 import com.google.inject.name.Named;
 
 import org.apache.commons.lang.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Arrays;
 import java.util.Collections;
@@ -38,6 +40,9 @@ import java.util.List;
 /** Caches the order AccessSections should be sorted for evaluation. */
 @Singleton
 public class SectionSortCache {
+  private static final Logger log =
+      LoggerFactory.getLogger(SectionSortCache.class);
+
   private static final String CACHE_NAME = "permission_sort";
 
   public static Module module() {
@@ -79,10 +84,11 @@ public class SectionSortCache {
       }
 
     } else {
+      boolean poison = false;
       IdentityHashMap<AccessSection, Integer> srcMap =
           new IdentityHashMap<AccessSection, Integer>();
       for (int i = 0; i < cnt; i++) {
-        srcMap.put(sections.get(i), i);
+        poison |= srcMap.put(sections.get(i), i) != null;
       }
 
       Collections.sort(sections, new MostSpecificComparator(ref));
@@ -97,7 +103,11 @@ public class SectionSortCache {
         }
       }
 
-      cache.put(key, new EntryVal(srcIdx));
+      if (poison) {
+        log.error("Received duplicate AccessSection instances, not caching sort");
+      } else {
+        cache.put(key, new EntryVal(srcIdx));
+      }
     }
   }
 
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 29b27628c9..77775d8d15 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
@@ -145,6 +145,18 @@ public class RefControlTest extends TestCase {
         u.controlForRef("refs/heads/foobar").canUpload());
   }
 
+  public void testInheritDuplicateSections() {
+    grant(parent, READ, admin, "refs/*");
+    grant(local, READ, devs, "refs/heads/*");
+    local.getProject().setParentName(parent.getProject().getName());
+    assertTrue("a can read", user("a", admin).isVisible());
+
+    local = new ProjectConfig(new Project.NameKey("local"));
+    local.createInMemory();
+    grant(local, READ, devs, "refs/*");
+    assertTrue("d can read", user("d", devs).isVisible());
+  }
+
   public void testInheritRead_OverrideWithDeny() {
     grant(parent, READ, registered, "refs/*");
     grant(local, READ, registered, "refs/*").setDeny();
@@ -321,7 +333,6 @@ public class RefControlTest extends TestCase {
 
     local = new ProjectConfig(new Project.NameKey("local"));
     local.createInMemory();
-    local.getProject().setParentName(parent.getProject().getName());
 
     sectionSorter =
         new PermissionCollection.Factory(