From eabea43a2874aad5ed65bf6d189b6ffb744bd679 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Thu, 21 Jun 2012 10:48:18 -0700 Subject: [PATCH] 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 --- .../google/gerrit/server/project/ProjectState.java | 5 ++++- .../gerrit/server/project/SectionSortCache.java | 14 ++++++++++++-- .../gerrit/server/project/RefControlTest.java | 13 ++++++++++++- 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 all = new ArrayList(); Set seen = new HashSet(); + 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 srcMap = new IdentityHashMap(); 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(