commit ad7e7a3c741fda9c8b32006243b9caad62011850
author Eryk Szymanski <eryksz@gmail.com> Thu May 15 01:05:18 2014 +0200
committer David Pursehouse <david.pursehouse@sonymobile.com> Thu May 15 14:56:46 2014 +0900
tree fd9222dfd01347861b6d55309360b96e29b9b3de
parent fa6756cf1dfbcb67240744cce9b600785523cf63

Fix a possibility to overcome BLOCK permission

This patch closes the possibility to overwrite BLOCK
permission from parent project by simply re-adding exactly
the same BLOCK entry followed by ALLOW entry in the child project.

The two new unit tests in RefControlTest show the problem.

Change-Id: I0bf0f0cd549185d72f8753d04f02d87d8709a3b2
Signed-off-by: Eryk Szymanski <eryksz@gmail.com>

gerrit-server/src/main/java/com/google/gerrit/server/project/PermissionCollection.java

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PermissionCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PermissionCollection.java
index 3b6ce91..dbfe34b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/PermissionCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PermissionCollection.java
@@ -110,7 +110,6 @@
       sorter.sort(ref, sections);
 
       Set<SeenRule> seen = new HashSet<SeenRule>();
-      Set<SeenRule> seenBlockingRules = new HashSet<SeenRule>();
       Set<String> exclusiveGroupPermissions = new HashSet<String>();
 
       HashMap<String, List<PermissionRule>> permissions =
@@ -126,7 +125,7 @@
             SeenRule s = new SeenRule(section, permission, rule);
             boolean addRule;
             if (rule.isBlock()) {
-              addRule = seenBlockingRules.add(s);
+              addRule = true;
             } else {
               addRule = seen.add(s) && !rule.isDeny() && !exclusivePermissionExists;
             }

gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java

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 75063c8..54765ac 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
@@ -299,6 +299,15 @@
   public void testBlockRule_ParentBlocksChild() {
     grant(local, PUSH, DEVS, "refs/tags/*");
     grant(util.getParentConfig(), PUSH, ANONYMOUS_USERS, "refs/tags/*").setBlock();
+    ProjectControl u = util.user(local, DEVS);
+    assertFalse("u can't update tag", u.controlForRef("refs/tags/V10").canUpdate());
+  }
+
+  @Test
+  public void testBlockRule_ParentBlocksChildEvenIfAlreadyBlockedInChild() {
+    grant(local, PUSH, DEVS, "refs/tags/*");
+    grant(local, PUSH, ANONYMOUS_USERS, "refs/tags/*").setBlock();
+    grant(util.getParentConfig(), PUSH, ANONYMOUS_USERS, "refs/tags/*").setBlock();
 
     ProjectControl u = util.user(local, DEVS);
     assertFalse("u can't update tag", u.controlForRef("refs/tags/V10").canUpdate());
@@ -319,6 +328,23 @@
   }
 
   @Test
+  public void testBlockLabelRange_ParentBlocksChildEvenIfAlreadyBlockedInChild() {
+    grant(local, LABEL + "Code-Review", -2, +2, DEVS, "refs/heads/*");
+    grant(local, LABEL + "Code-Review", -2, +2, DEVS, "refs/heads/*").setBlock();
+    grant(util.getParentConfig(), LABEL + "Code-Review", -2, +2, DEVS,
+        "refs/heads/*").setBlock();
+
+    ProjectControl u = util.user(local, DEVS);
+
+    PermissionRange range =
+        u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertTrue("u can vote -1", range.contains(-1));
+    assertTrue("u can vote +1", range.contains(1));
+    assertFalse("u can't vote -2", range.contains(-2));
+    assertFalse("u can't vote 2", range.contains(2));
+  }
+
+  @Test
   public void testUnblockNoForce() {
     grant(local, PUSH, ANONYMOUS_USERS, "refs/heads/*").setBlock();
     grant(local, PUSH, DEVS, "refs/heads/*");