Merge "Overrule BLOCK with ALLOW on the same project"

Documentation/access-control.txt

@@ -1109,8 +1109,9 @@ non-blocked rules as they wish. This gives best of both worlds:
 ~~~~~~~~~~~~~~~~~~~
 
 The 'BLOCK' rule blocks a permission globally. An inherited 'BLOCK' rule cannot
-be overridden in the inheriting project. Any 'ALLOW' rule which conflicts with
+be overridden in the inheriting project. Any 'ALLOW' rule, from a different
-an inherited 'BLOCK' rule will not be honored.  Searching for 'BLOCK' rules, in
+access section or from an inheriting project, which conflicts with an
+inherited 'BLOCK' rule will not be honored.  Searching for 'BLOCK' rules, in
 the chain of parent projects, ignores the Exclusive flag that is normally
 applied to access sections.
 
@@ -1131,6 +1132,26 @@ The interpretation of the 'min..max' range in case of a blocking rule is: block
 every vote from '-INFINITE..min' and 'max..INFINITE'. For the example above it
 means that the range '-1..+1' is not affected by this block.
 
+'BLOCK' and 'ALLOW' rules in the same access section
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+When an access section of a project contains a 'BLOCK' and an 'ALLOW' rule for
+the same permission then this 'ALLOW' rule overrides the 'BLOCK' rule:
+
+====
+  [access "refs/heads/*"]
+    push = block group X
+    push = group Y
+====
+
+In this case a user which is a member of the group 'Y' will still be allowed to
+push to 'refs/heads/*' even if it is a member of the group 'X'.
+
+NOTE: An 'ALLOW' rule overrides a 'BLOCK' rule only when both of them are
+inside the same access section of the same project. An 'ALLOW' rule in a
+different access section of the same project or in any access section in an
+inheriting project cannot override a 'BLOCK' rule.
+
 Examples
 ~~~~~~~~
 
@@ -1160,6 +1181,23 @@ owners>> are allowed to create tags, we would extend the example above:
     pushTag = group Project Owners
 ====
 
+Let only a dedicated group vote in a special category
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Assume there is a more restrictive process for submitting changes in stable
+release branches which is manifested as a new voting category
+'Release-Process'. Assume we want to make sure that only a 'Release Engineers'
+group can vote in this category and that even project owners cannot approve
+this category. We have to block everyone except the 'Release Engineers' to vote
+in this category and, of course, allow 'Release Engineers' to vote in that
+category. In the "`All-Projects`" we define the following rules:
+
+====
+  [access "refs/heads/stable*"]
+    label-Release-Proces = block -1..+1 group Anonymous Users
+    label-Release-Proces = -1..+1 group Release Engineers
+====
+
 [[conversion_table]]
 Conversion table from 2.1.x series to 2.2.x series
 --------------------------------------------------

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

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.project;
 
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
@@ -42,6 +44,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 
 /** Manages access control for Git references (aka branches, tags). */
@@ -117,18 +120,18 @@ public class RefControl {
    */
   public boolean isVisibleByRegisteredUsers() {
     List<PermissionRule> access = relevant.getPermission(Permission.READ);
+    Set<ProjectRef> allows = Sets.newHashSet();
+    Set<ProjectRef> blocks = Sets.newHashSet();
     for (PermissionRule rule : access) {
       if (rule.isBlock()) {
-        return false;
+        blocks.add(relevant.getRuleProps(rule));
-      }
+      } else if (rule.getGroup().getUUID().equals(AccountGroup.ANONYMOUS_USERS)
-    }
-    for (PermissionRule rule : access) {
-      if (rule.getGroup().getUUID().equals(AccountGroup.ANONYMOUS_USERS)
           || rule.getGroup().getUUID().equals(AccountGroup.REGISTERED_USERS)) {
-        return true;
+        allows.add(relevant.getRuleProps(rule));
       }
     }
-    return false;
+    blocks.removeAll(allows);
+    return blocks.isEmpty() && !allows.isEmpty();
   }
 
   /**
@@ -218,16 +221,7 @@ public class RefControl {
       // granting of powers beyond pushing to the configuration.
       return false;
     }
-    boolean result = false;
+    return canForcePerform(Permission.PUSH);
-    for (PermissionRule rule : access(Permission.PUSH)) {
-      if (rule.isBlock()) {
-        return false;
-      }
-      if (rule.getForce()) {
-        result = true;
-      }
-    }
-    return result;
   }
 
   /**
@@ -375,16 +369,7 @@ public class RefControl {
 
   /** @return true if this user can force edit topic names. */
   public boolean canForceEditTopicName() {
-    boolean result = false;
+    return canForcePerform(Permission.EDIT_TOPIC_NAME);
-    for (PermissionRule rule : access(Permission.EDIT_TOPIC_NAME)) {
-      if (rule.isBlock()) {
-        return false;
-      }
-      if (rule.getForce()) {
-        result = true;
-      }
-    }
-    return result;
   }
 
   /** All value ranges of any allowed label permission. */
@@ -416,39 +401,97 @@ public class RefControl {
     return null;
   }
 
-  private static PermissionRange toRange(String permissionName,
+  private static class AllowedRange {
-      List<PermissionRule> ruleList) {
+    private int allowMin = 0;
-    int min = 0;
+    private int allowMax = 0;
-    int max = 0;
+    private int blockMin = Integer.MIN_VALUE;
-    int blockMin = Integer.MIN_VALUE;
+    private int blockMax = Integer.MAX_VALUE;
-    int blockMax = Integer.MAX_VALUE;
+
-    for (PermissionRule rule : ruleList) {
+    void update(PermissionRule rule) {
       if (rule.isBlock()) {
         blockMin = Math.max(blockMin, rule.getMin());
         blockMax = Math.min(blockMax, rule.getMax());
       } else {
-        min = Math.min(min, rule.getMin());
+        allowMin = Math.min(allowMin, rule.getMin());
-        max = Math.max(max, rule.getMax());
+        allowMax = Math.max(allowMax, rule.getMax());
       }
     }
-    if (blockMin > Integer.MIN_VALUE) {
+
-      min = Math.max(min, blockMin + 1);
+    int getAllowMin() {
+      return allowMin;
     }
-    if (blockMax < Integer.MAX_VALUE) {
+    int getAllowMax() {
-      max = Math.min(max, blockMax - 1);
+      return allowMax;
     }
+    int getBlockMin() {
+      // ALLOW wins over BLOCK on the same project
+      return Math.min(blockMin, allowMin - 1);
+    }
+    int getBlockMax() {
+      // ALLOW wins over BLOCK on the same project
+      return Math.max(blockMax, allowMax + 1);
+    }
+  }
+
+  private PermissionRange toRange(String permissionName,
+      List<PermissionRule> ruleList) {
+    Map<ProjectRef, AllowedRange> ranges = Maps.newHashMap();
+    for (PermissionRule rule : ruleList) {
+      ProjectRef p = relevant.getRuleProps(rule);
+      AllowedRange r = ranges.get(p);
+      if (r == null) {
+        r = new AllowedRange();
+        ranges.put(p, r);
+      }
+      r.update(rule);
+    }
+    int allowMin = 0;
+    int allowMax = 0;
+    int blockMin = Integer.MIN_VALUE;
+    int blockMax = Integer.MAX_VALUE;
+    for (AllowedRange r : ranges.values()) {
+      allowMin = Math.min(allowMin, r.getAllowMin());
+      allowMax = Math.max(allowMax, r.getAllowMax());
+      blockMin = Math.max(blockMin, r.getBlockMin());
+      blockMax = Math.min(blockMax, r.getBlockMax());
+    }
+
+    // BLOCK wins over ALLOW across projects
+    int min = Math.max(allowMin, blockMin + 1);
+    int max = Math.min(allowMax, blockMax - 1);
     return new PermissionRange(permissionName, min, max);
   }
 
   /** True if the user has this permission. Works only for non labels. */
   boolean canPerform(String permissionName) {
     List<PermissionRule> access = access(permissionName);
+    Set<ProjectRef> allows = Sets.newHashSet();
+    Set<ProjectRef> blocks = Sets.newHashSet();
     for (PermissionRule rule : access) {
       if (rule.isBlock() && !rule.getForce()) {
-        return false;
+        blocks.add(relevant.getRuleProps(rule));
+      } else {
+        allows.add(relevant.getRuleProps(rule));
       }
     }
-    return !access.isEmpty();
+    blocks.removeAll(allows);
+    return blocks.isEmpty() && !allows.isEmpty();
+  }
+
+  /** True if the user has force this permission. Works only for non labels. */
+  private boolean canForcePerform(String permissionName) {
+    List<PermissionRule> access = access(permissionName);
+    Set<ProjectRef> allows = Sets.newHashSet();
+    Set<ProjectRef> blocks = Sets.newHashSet();
+    for (PermissionRule rule : access) {
+      if (rule.isBlock()) {
+        blocks.add(relevant.getRuleProps(rule));
+      } else if (rule.getForce()) {
+        allows.add(relevant.getRuleProps(rule));
+      }
+    }
+    blocks.removeAll(allows);
+    return blocks.isEmpty() && !allows.isEmpty();
   }
 
   /** Rules for the given permission, or the empty list. */

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.project;
 
+import static com.google.gerrit.common.data.Permission.EDIT_TOPIC_NAME;
 import static com.google.gerrit.common.data.Permission.LABEL;
 import static com.google.gerrit.common.data.Permission.OWNER;
 import static com.google.gerrit.common.data.Permission.PUSH;
@@ -253,6 +254,139 @@ public class RefControlTest extends TestCase {
     assertFalse("u can't vote -2", range.contains(-2));
     assertFalse("u can't vote 2", range.contains(2));
   }
+
+  public void testUnblockNoForce() {
+    grant(local, PUSH, anonymous, "refs/heads/*").setBlock();
+    grant(local, PUSH, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    assertTrue("u can push", u.controlForRef("refs/heads/master").canUpdate());
+  }
+
+  public void testUnblockForce() {
+    PermissionRule r = grant(local, PUSH, anonymous, "refs/heads/*");
+    r.setBlock();
+    r.setForce(true);
+    grant(local, PUSH, devs, "refs/heads/*").setForce(true);
+
+    ProjectControl u = user(devs);
+    assertTrue("u can force push", u.controlForRef("refs/heads/master").canForceUpdate());
+  }
+
+  public void testUnblockForceWithAllowNoForce_NotPossible() {
+    PermissionRule r = grant(local, PUSH, anonymous, "refs/heads/*");
+    r.setBlock();
+    r.setForce(true);
+    grant(local, PUSH, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    assertFalse("u can't force push", u.controlForRef("refs/heads/master").canForceUpdate());
+  }
+
+  public void testUnblockMoreSpecificRef_Fails() {
+    grant(local, PUSH, anonymous, "refs/heads/*").setBlock();
+    grant(local, PUSH, devs, "refs/heads/master");
+
+    ProjectControl u = user(devs);
+    assertFalse("u can't push", u.controlForRef("refs/heads/master").canUpdate());
+  }
+
+  public void testUnblockLargerScope_Fails() {
+    grant(local, PUSH, anonymous, "refs/heads/master").setBlock();
+    grant(local, PUSH, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    assertFalse("u can't push", u.controlForRef("refs/heads/master").canUpdate());
+  }
+
+  public void testUnblockInLocal_Fails() {
+    grant(parent, PUSH, anonymous, "refs/heads/*").setBlock();
+    grant(local, PUSH, fixers, "refs/heads/*");
+
+    ProjectControl f = user(fixers);
+    assertFalse("u can't push", f.controlForRef("refs/heads/master").canUpdate());
+  }
+
+  public void testUnblockInParentBlockInLocal() {
+    grant(parent, PUSH, anonymous, "refs/heads/*").setBlock();
+    grant(parent, PUSH, devs, "refs/heads/*");
+    grant(local, PUSH, devs, "refs/heads/*").setBlock();
+
+    ProjectControl d = user(devs);
+    assertFalse("u can't push", d.controlForRef("refs/heads/master").canUpdate());
+  }
+
+  public void testUnblockVisibilityByRegisteredUsers() {
+    grant(local, READ, anonymous, "refs/heads/*").setBlock();
+    grant(local, READ, registered, "refs/heads/*");
+
+    ProjectControl u = user(registered);
+    assertTrue("u can read", u.controlForRef("refs/heads/master").isVisibleByRegisteredUsers());
+  }
+
+  public void testUnblockInLocalVisibilityByRegisteredUsers_Fails() {
+    grant(parent, READ, anonymous, "refs/heads/*").setBlock();
+    grant(local, READ, registered, "refs/heads/*");
+
+    ProjectControl u = user(registered);
+    assertFalse("u can't read", u.controlForRef("refs/heads/master").isVisibleByRegisteredUsers());
+  }
+
+  public void testUnblockForceEditTopicName() {
+    grant(local, EDIT_TOPIC_NAME, anonymous, "refs/heads/*").setBlock();
+    grant(local, EDIT_TOPIC_NAME, devs, "refs/heads/*").setForce(true);
+
+    ProjectControl u = user(devs);
+    assertTrue("u can edit topic name", u.controlForRef("refs/heads/master").canForceEditTopicName());
+  }
+
+  public void testUnblockInLocalForceEditTopicName_Fails() {
+    grant(parent, EDIT_TOPIC_NAME, anonymous, "refs/heads/*").setBlock();
+    grant(local, EDIT_TOPIC_NAME, devs, "refs/heads/*").setForce(true);
+
+    ProjectControl u = user(registered);
+    assertFalse("u can't edit topic name", u.controlForRef("refs/heads/master").canForceEditTopicName());
+  }
+
+  public void testUnblockRange() {
+    grant(local, LABEL + "Code-Review", -1, +1, anonymous, "refs/heads/*").setBlock();
+    grant(local, LABEL + "Code-Review", -2, +2, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    PermissionRange range = u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertTrue("u can vote -2", range.contains(-2));
+    assertTrue("u can vote +2", range.contains(2));
+  }
+
+  public void testUnblockRangeOnMoreSpecificRef_Fails() {
+    grant(local, LABEL + "Code-Review", -1, +1, anonymous, "refs/heads/*").setBlock();
+    grant(local, LABEL + "Code-Review", -2, +2, devs, "refs/heads/master");
+
+    ProjectControl u = user(devs);
+    PermissionRange range = u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertFalse("u can't vote -2", range.contains(-2));
+    assertFalse("u can't vote +2", range.contains(-2));
+  }
+
+  public void testUnblockRangeOnLargerScope_Fails() {
+    grant(local, LABEL + "Code-Review", -1, +1, anonymous, "refs/heads/master").setBlock();
+    grant(local, LABEL + "Code-Review", -2, +2, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    PermissionRange range = u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertFalse("u can't vote -2", range.contains(-2));
+    assertFalse("u can't vote +2", range.contains(-2));
+  }
+
+  public void testUnblockInLocalRange_Fails() {
+    grant(parent, LABEL + "Code-Review", -1, 1, anonymous, "refs/heads/*").setBlock();
+    grant(local, LABEL + "Code-Review", -2, +2, devs, "refs/heads/*");
+
+    ProjectControl u = user(devs);
+    PermissionRange range = u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertFalse("u can't vote -2", range.contains(-2));
+    assertFalse("u can't vote 2", range.contains(2));
+  }
   // -----------------------------------------------------------------------
 
   private final Map<Project.NameKey, ProjectState> all;