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
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 9f6f6e7..a72fc16 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 @@
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 @@
}
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 c0d2034..2bc957e 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.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 @@
/** 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 @@
}
} 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 @@
}
}
- 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 29b2762..77775d8 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 @@
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 @@
local = new ProjectConfig(new Project.NameKey("local"));
local.createInMemory();
- local.getProject().setParentName(parent.getProject().getName());
sectionSorter =
new PermissionCollection.Factory(
