Merge changes If917e10d,Ic5fae13c

* changes:
  Rewrite permission evaluation
  RefControlTest: BLOCK aggregates all the forbidden votes across projects

Documentation/access-control.txt

@@ -1422,6 +1422,177 @@ allows the granted group to
 link:cmd-show-queue.html[look at the Gerrit task queue via ssh].
 
 
+[[reference]]
+== Permission evaluation reference
+
+Permission evaluation is expressed in the following concepts:
+
+* PermisssionRule: a single combination of {ALLOW, DENY, BLOCK} and
+group, and optionally a vote range and an 'exclusive' bit.
+
+* Permission: groups PermissionRule by permission name. All
+PermissionRules for same access type (eg. "read", "push") are grouped
+into a Permission implicitly. The exclusive bit lives here.
+
+* AccessSection: ties a list of Permissions to a single ref pattern.
+Each AccessSection comes from a single project.
+
+
+
+Here is how these play out in a link:config-project-config.html[project.config] file:
+
+----
+  # An AccessSection
+  [access "refs/heads/stable/*"]
+     exclusiveGroupPermissions = create
+
+     # Each of the following lines corresponds to a PermissionRule
+     # The next two PermissionRule together form the "read" Permission
+     read = group Administrators
+     read = group Registered Users
+
+     # A Permission with a block and block-override
+     create = block group Registered Users
+     create = group Project Owners
+
+     # A Permission and PermissionRule for a label
+     label-Code-Review = -2..+2 group Project Owners
+----
+
+=== Ref permissions
+
+Access to refs can be blocked, allowed or denied.
+
+==== BLOCK
+
+For blocking access, all rules marked BLOCK are tested, and if one
+such rule matches, the user is denied access.
+
+The rules are ordered by inheritance, starting from All-Projects down.
+Within a project, more specific ref patterns come first. The downward
+ordering lets administrators enforce access rules across all projects
+in a site.
+
+BLOCK rules can have exceptions defined on the same project (eg. BLOCK
+anonymous users, ie. everyone, but make an exception for Admin users),
+either by:
+
+1. adding ALLOW PermissionRules in the same Permission. This implies
+they apply to the same ref pattern.
+
+2. adding an ALLOW Permission in the same project with a more specific
+ref pattern, but marked "exclusive". This allows them to apply to
+different ref patterns.
+
+Such additions not only bypass BLOCK rules, but they will also grant
+permissions when they are processed in the ALLOW/DENY processing, as
+described in the next subsection.
+
+==== ALLOW
+
+For allowing access, all ALLOW/DENY rules that might apply to a ref
+are tested until one granting access is found, or until either an
+"exclusive" rule ends the search, or all rules have been tested.
+
+The rules are ordered from specific ref patterns to general patterns,
+and for equally specific patterns, from originating project up to
+All-Projects.
+
+This ordering lets project owners apply permissions specific to their
+project, overwriting the site defaults specified in All-Projects.
+
+==== DENY
+
+DENY is processed together with ALLOW.
+
+As said, during ALLOW/DENY processing, rules are tried out one by one.
+For each (permission, ref-pattern, group) only a single rule
+ALLOW/DENY rule is picked. If that first rule is a DENY rule, any
+following ALLOW rules for the same (permission, ref-pattern, group)
+will be ignored, canceling out their effect.
+
+DENY is confusing because it only works on a specific (ref-pattern,
+group) pair. The parent project can undo the effect of a DENY rule by
+introducing an extra rule which features a more general ref pattern or
+a different group.
+
+==== DENY/ALLOW example
+
+Consider the ref "refs/a" and the following configuration:
+----
+
+child-project: project.config
+    [access "refs/a"]
+      read = deny group A
+
+All-Projects: project.config
+    [access "refs/a"]
+      read = group A      # ALLOW
+    [access "refs/*"]
+      read = group B      # ALLOW
+----
+
+When determining access, first "read = DENY group A" on "refs/a" is
+encountered. The following rule to consider is "ALLOW read group A" on
+"refs/a". The latter rule applies to the same (permission,
+ref-pattern, group) tuple, so it it is ignored.
+
+The DENY rule does not affect the last rule for "refs/*", since that
+has a different ref pattern and a different group. If group B is a
+superset of group A, the last rule will still grant group A access to
+"refs/a".
+
+
+==== Double use of exclusive
+
+An 'exclusive' permission is evaluated both during BLOCK processing
+and during ALLOW/DENY: when looking BLOCK, 'exclusive' stops the
+search downward, while the same permission in the ALLOW/DENY
+processing will stop looking upward for further rule matches
+
+==== Force permission
+
+The 'force' setting may be set on ALLOW and BLOCK rules. In the case
+of ALLOW, the 'force' option makes the permission stronger (allowing
+both forced and unforced actions). For BLOCK, the 'force' option makes
+it weaker (the BLOCK with 'force' only blocks forced actions).
+
+
+=== Labels
+
+Labels use the same mechanism, with the following observations:
+
+* The 'force' setting has no effect on label ranges.
+
+* BLOCK specifies the values that a group cannot vote, eg.
+----
+  label-Code-Review = block -2..+2 group Anonymous Users
+----
+prevents all users from voting -2 or +2.
+
+* DENY works for votes too, with the same caveats
+
+* The blocked vote range is the union of the all the blocked vote
+ranges across projects, so in
+----
+All-Projects: project.config
+     label-Code-Review = block -2..+1 group A
+
+Child-Project: project-config
+     label-Code-Review = block -1..+2 group A
+----
+members of group A cannot vote at all in the Child-Project.
+
+
+* The allowed vote range is the union of vote ranges allowed by all of
+the ALLOW rules. For example, in
+----
+     label-Code-Review = -2..+1 group A
+     label-Code-Review = -1..+2 group B
+----
+a user that is both in A and B can vote -2..2.
+
+
 GERRIT
 ------
 Part of link:index.html[Gerrit Code Review]

java/com/google/gerrit/server/permissions/PermissionCollection.java

@@ -14,18 +14,18 @@
 
 package com.google.gerrit.server.permissions;
 
-import static com.google.common.base.MoreObjects.firstNonNull;
+import static com.google.gerrit.common.data.PermissionRule.Action.BLOCK;
 import static com.google.gerrit.server.project.RefPattern.isRE;
+import static java.util.stream.Collectors.mapping;
+import static java.util.stream.Collectors.toList;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
-import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRule;
+import com.google.gerrit.common.data.PermissionRule.Action;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.CurrentUser;
@@ -35,13 +35,13 @@ import com.google.gerrit.server.project.SectionMatcher;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 /**
  * Effective permissions applied to a reference in a project.
@@ -126,78 +126,134 @@ public class PermissionCollection {
       // LinkedHashMap to maintain input ordering.
       Map<AccessSection, Project.NameKey> sectionToProject = new LinkedHashMap<>();
       boolean perUser = filterRefMatchingSections(matcherList, ref, user, sectionToProject);
-
       List<AccessSection> sections = Lists.newArrayList(sectionToProject.keySet());
 
       // Sort by ref pattern specificity. For equally specific patterns, the sections from the
       // project closer to the current one come first.
       sorter.sort(ref, sections);
 
+      // For block permissions, we want a different order: first, we want to go from parent to child.
+      List<Map.Entry<AccessSection, Project.NameKey>> accessDescending =
+          Lists.reverse(Lists.newArrayList(sectionToProject.entrySet()));
+
+      Map<Project.NameKey, List<AccessSection>> accessByProject =
+          accessDescending
+              .stream()
+              .collect(
+                  Collectors.groupingBy(
+                      e -> e.getValue(), LinkedHashMap::new, mapping(e -> e.getKey(), toList())));
+      // Within each project, sort by ref specificity.
+      for (List<AccessSection> secs : accessByProject.values()) {
+        sorter.sort(ref, secs);
+      }
+
+      return new PermissionCollection(
+          Lists.newArrayList(accessByProject.values()), sections, perUser);
+    }
+  }
+
+  /** Returns permissions in the right order for evaluating BLOCK status. */
+  List<List<Permission>> getBlockRules(String perm) {
+    List<List<Permission>> ps = blockPerProjectByPermission.get(perm);
+    if (ps == null) {
+      ps = calculateBlockRules(perm);
+      blockPerProjectByPermission.put(perm, ps);
+    }
+    return ps;
+  }
+
+  /** Returns permissions in the right order for evaluating ALLOW/DENY status. */
+  List<PermissionRule> getAllowRules(String perm) {
+    List<PermissionRule> ps = rulesByPermission.get(perm);
+    if (ps == null) {
+      ps = calculateAllowRules(perm);
+      rulesByPermission.put(perm, ps);
+    }
+    return ps;
+  }
+
+  /** calculates permissions for ALLOW processing. */
+  private List<PermissionRule> calculateAllowRules(String permName) {
     Set<SeenRule> seen = new HashSet<>();
-      Set<String> exclusiveGroupPermissions = new HashSet<>();
 
-      HashMap<String, List<PermissionRule>> permissions = new HashMap<>();
+    List<PermissionRule> r = new ArrayList<>();
-      HashMap<String, List<PermissionRule>> overridden = new HashMap<>();
+    for (AccessSection s : accessSectionsUpward) {
-      Map<PermissionRule, ProjectRef> ruleProps = Maps.newIdentityHashMap();
+      Permission p = s.getPermission(permName);
-      ListMultimap<Project.NameKey, String> exclusivePermissionsByProject =
+      if (p == null) {
-          MultimapBuilder.hashKeys().arrayListValues().build();
+        continue;
-      for (AccessSection section : sections) {
+      }
-        Project.NameKey project = sectionToProject.get(section);
+      for (PermissionRule pr : p.getRules()) {
-        for (Permission permission : section.getPermissions()) {
+        SeenRule sr = SeenRule.create(s, pr);
-          boolean exclusivePermissionExists =
+        if (seen.contains(sr)) {
-              exclusiveGroupPermissions.contains(permission.getName());
+          // We allow only one rule per (ref-pattern, group) tuple. This is used to implement DENY:
+          // If we see a DENY before an ALLOW rule, that causes the ALLOW rule to be skipped here,
+          // negating access.
+          continue;
+        }
+        seen.add(sr);
 
-          for (PermissionRule rule : permission.getRules()) {
+        if (pr.getAction() == BLOCK) {
-            SeenRule s = SeenRule.create(section, permission, rule);
+          // Block rules are handled elsewhere.
-            boolean addRule;
+          continue;
-            if (rule.isBlock()) {
-              addRule = !exclusivePermissionsByProject.containsEntry(project, permission.getName());
-            } else {
-              addRule = seen.add(s) && !rule.isDeny() && !exclusivePermissionExists;
         }
 
-            HashMap<String, List<PermissionRule>> p = null;
+        if (pr.getAction() == PermissionRule.Action.DENY) {
-            if (addRule) {
+          // DENY rules work by not adding ALLOW rules. Nothing else to do.
-              p = permissions;
+          continue;
-            } else if (!rule.isDeny() && !exclusivePermissionExists) {
+        }
-              p = overridden;
+        r.add(pr);
+      }
+      if (p.getExclusiveGroup()) {
+        // We found an exclusive permission, so no need to further go up the hierarchy.
+        break;
+      }
+    }
+    return r;
   }
 
-            if (p != null) {
+  // Calculates the inputs for determining BLOCK status, grouped by project.
-              List<PermissionRule> r = p.get(permission.getName());
+  private List<List<Permission>> calculateBlockRules(String permName) {
-              if (r == null) {
+    List<List<Permission>> result = new ArrayList<>();
-                r = new ArrayList<>(2);
+    for (List<AccessSection> secs : this.accessSectionsPerProjectDownward) {
-                p.put(permission.getName(), r);
+      List<Permission> perms = new ArrayList<>();
+      boolean blockFound = false;
+      for (AccessSection sec : secs) {
+        Permission p = sec.getPermission(permName);
+        if (p == null) {
+          continue;
         }
-              r.add(rule);
+        for (PermissionRule pr : p.getRules()) {
-              ruleProps.put(rule, ProjectRef.create(project, section.getName()));
+          if (blockFound || pr.getAction() == Action.BLOCK) {
+            blockFound = true;
+            break;
           }
         }
 
-          if (permission.getExclusiveGroup()) {
+        perms.add(p);
-            exclusivePermissionsByProject.put(project, permission.getName());
-            exclusiveGroupPermissions.add(permission.getName());
-          }
-        }
       }
 
-      return new PermissionCollection(permissions, overridden, ruleProps, perUser);
+      if (blockFound) {
+        result.add(perms);
       }
     }
+    return result;
+  }
 
-  private final Map<String, List<PermissionRule>> rules;
+  private List<List<AccessSection>> accessSectionsPerProjectDownward;
-  private final Map<String, List<PermissionRule>> overridden;
+  private List<AccessSection> accessSectionsUpward;
-  private final Map<PermissionRule, ProjectRef> ruleProps;
+
+  private final Map<String, List<PermissionRule>> rulesByPermission;
+  private final Map<String, List<List<Permission>>> blockPerProjectByPermission;
   private final boolean perUser;
 
   private PermissionCollection(
-      Map<String, List<PermissionRule>> rules,
+      List<List<AccessSection>> accessSectionsDownward,
-      Map<String, List<PermissionRule>> overridden,
+      List<AccessSection> accessSectionsUpward,
-      Map<PermissionRule, ProjectRef> ruleProps,
       boolean perUser) {
-    this.rules = rules;
+    this.accessSectionsPerProjectDownward = accessSectionsDownward;
-    this.overridden = overridden;
+    this.accessSectionsUpward = accessSectionsUpward;
-    this.ruleProps = ruleProps;
+    this.rulesByPermission = new HashMap<>();
+    this.blockPerProjectByPermission = new HashMap<>();
     this.perUser = perUser;
   }
 
@@ -209,55 +265,18 @@ public class PermissionCollection {
     return perUser;
   }
 
-  /**
-   * Obtain all permission rules for a given type of permission.
-   *
-   * @param permissionName type of permission.
-   * @return all rules that apply to this reference, for any group. Never null; the empty list is
-   *     returned when there are no rules for the requested permission name.
-   */
-  public List<PermissionRule> getPermission(String permissionName) {
-    List<PermissionRule> r = rules.get(permissionName);
-    return r != null ? r : Collections.<PermissionRule>emptyList();
-  }
-
-  List<PermissionRule> getOverridden(String permissionName) {
-    return firstNonNull(overridden.get(permissionName), Collections.<PermissionRule>emptyList());
-  }
-
-  ProjectRef getRuleProps(PermissionRule rule) {
-    return ruleProps.get(rule);
-  }
-
-  /**
-   * Obtain all declared permission rules that match the reference.
-   *
-   * @return all rules. The collection will iterate a permission if it was declared in the project
-   *     configuration, either directly or inherited. If the project owner did not use a known
-   *     permission (for example {@link Permission#FORGE_SERVER}, then it will not be represented in
-   *     the result even if {@link #getPermission(String)} returns an empty list for the same
-   *     permission.
-   */
-  public Iterable<Map.Entry<String, List<PermissionRule>>> getDeclaredPermissions() {
-    return rules.entrySet();
-  }
-
   /** (ref, permission, group) tuple. */
   @AutoValue
   abstract static class SeenRule {
     public abstract String refPattern();
 
-    public abstract String permissionName();
-
     @Nullable
     public abstract AccountGroup.UUID group();
 
-    static SeenRule create(
+    static SeenRule create(AccessSection section, @Nullable PermissionRule rule) {
-        AccessSection section, Permission permission, @Nullable PermissionRule rule) {
       AccountGroup.UUID group =
           rule != null && rule.getGroup() != null ? rule.getGroup().getUUID() : null;
-      return new AutoValue_PermissionCollection_SeenRule(
+      return new AutoValue_PermissionCollection_SeenRule(section.getName(), group);
-          section.getName(), permission.getName(), group);
     }
   }
 }

java/com/google/gerrit/server/permissions/RefControl.java

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRange;
 import com.google.gerrit.common.data.PermissionRule;
+import com.google.gerrit.common.data.PermissionRule.Action;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.reviewdb.client.Change;
@@ -32,14 +33,9 @@ import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.util.MagicBranch;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.util.Providers;
-import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.EnumSet;
-import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
 /** Manages access control for Git references (aka branches, tags). */
@@ -50,8 +46,7 @@ class RefControl {
   /** All permissions that apply to this reference. */
   private final PermissionCollection relevant;
 
-  /** Cached set of permissions matching this user. */
+  // The next 4 members are cached canPerform() permissions.
-  private final Map<String, List<PermissionRule>> effective;
 
   private Boolean owner;
   private Boolean canForgeAuthor;
@@ -62,7 +57,6 @@ class RefControl {
     this.projectControl = projectControl;
     this.refName = ref;
     this.relevant = relevant;
-    this.effective = new HashMap<>();
   }
 
   ProjectControl getProjectControl() {
@@ -124,12 +118,12 @@ class RefControl {
       // granting of powers beyond submitting to the configuration.
       return projectControl.isOwner();
     }
-    return canPerform(Permission.SUBMIT, isChangeOwner);
+    return canPerform(Permission.SUBMIT, isChangeOwner, false);
   }
 
   /** @return true if this user can force edit topic names. */
   boolean canForceEditTopicName() {
-    return canForcePerform(Permission.EDIT_TOPIC_NAME);
+    return canPerform(Permission.EDIT_TOPIC_NAME, false, true);
   }
 
   /** The range of permitted values associated with a label permission. */
@@ -140,14 +134,14 @@ class RefControl {
   /** The range of permitted values associated with a label permission. */
   PermissionRange getRange(String permission, boolean isChangeOwner) {
     if (Permission.hasRange(permission)) {
-      return toRange(permission, access(permission, isChangeOwner));
+      return toRange(permission, isChangeOwner);
     }
     return null;
   }
 
   /** True if the user has this permission. Works only for non labels. */
   boolean canPerform(String permissionName) {
-    return canPerform(permissionName, false);
+    return canPerform(permissionName, false, false);
   }
 
   ForRef asForRef() {
@@ -198,7 +192,7 @@ class RefControl {
       case UNKNOWN:
       case WEB_BROWSER:
       default:
-        return (isOwner() && !isForceBlocked(Permission.PUSH)) || projectControl.isAdmin();
+        return (isOwner() && !isBlocked(Permission.PUSH, false, true)) || projectControl.isAdmin();
     }
   }
 
@@ -211,7 +205,7 @@ class RefControl {
       // granting of powers beyond pushing to the configuration.
       return false;
     }
-    return canForcePerform(Permission.PUSH);
+    return canPerform(Permission.PUSH, false, true);
   }
 
   /**
@@ -239,7 +233,10 @@ class RefControl {
       case UNKNOWN:
       case WEB_BROWSER:
       default:
-        return (isOwner() && !isForceBlocked(Permission.PUSH))
+        return
+        // We allow owner to delete refs even if they have no force-push rights. We forbid
+        // it if force push is blocked, though. See commit 40bd5741026863c99bea13eb5384bd27855c5e1b
+        (isOwner() && !isBlocked(Permission.PUSH, false, true))
             || canPushWithForce()
             || canPerform(Permission.DELETE)
             || projectControl.isAdmin();
@@ -267,158 +264,140 @@ class RefControl {
     return canPerform(Permission.FORGE_SERVER);
   }
 
-  private static class AllowedRange {
+  private static boolean isAllow(PermissionRule pr, boolean withForce) {
-    private int allowMin;
+    return pr.getAction() == Action.ALLOW && (pr.getForce() || !withForce);
-    private int allowMax;
+  }
-    private int blockMin = Integer.MIN_VALUE;
-    private int blockMax = Integer.MAX_VALUE;
 
-    void update(PermissionRule rule) {
+  private static boolean isBlock(PermissionRule pr, boolean withForce) {
-      if (rule.isBlock()) {
+    // BLOCK with force specified is a weaker rule than without.
-        blockMin = Math.max(blockMin, rule.getMin());
+    return pr.getAction() == Action.BLOCK && (!pr.getForce() || withForce);
-        blockMax = Math.min(blockMax, rule.getMax());
+  }
-      } else {
+
-        allowMin = Math.min(allowMin, rule.getMin());
+  private PermissionRange toRange(String permissionName, boolean isChangeOwner) {
-        allowMax = Math.max(allowMax, rule.getMax());
+    int blockAllowMin = Integer.MIN_VALUE, blockAllowMax = Integer.MAX_VALUE;
+
+    projectLoop:
+    for (List<Permission> ps : relevant.getBlockRules(permissionName)) {
+      boolean blockFound = false;
+      int projectBlockAllowMin = Integer.MIN_VALUE, projectBlockAllowMax = Integer.MAX_VALUE;
+
+      for (Permission p : ps) {
+        if (p.getExclusiveGroup()) {
+          for (PermissionRule pr : p.getRules()) {
+            if (pr.getAction() == Action.ALLOW && projectControl.match(pr, isChangeOwner)) {
+              // exclusive override, usually for a more specific ref.
+              continue projectLoop;
+            }
           }
         }
 
-    int getAllowMin() {
+        for (PermissionRule pr : p.getRules()) {
-      return allowMin;
+          if (pr.getAction() == Action.BLOCK && projectControl.match(pr, isChangeOwner)) {
-    }
+            projectBlockAllowMin = pr.getMin() + 1;
-
+            projectBlockAllowMax = pr.getMax() - 1;
-    int getAllowMax() {
+            blockFound = true;
-      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) {
+        if (blockFound) {
-    Map<ProjectRef, AllowedRange> ranges = new HashMap<>();
+          for (PermissionRule pr : p.getRules()) {
-    for (PermissionRule rule : ruleList) {
+            if (pr.getAction() == Action.ALLOW && projectControl.match(pr, isChangeOwner)) {
-      ProjectRef p = relevant.getRuleProps(rule);
+              projectBlockAllowMin = pr.getMin();
-      AllowedRange r = ranges.get(p);
+              projectBlockAllowMax = pr.getMax();
-      if (r == null) {
+              break;
-        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);
-  }
-
-  private boolean canPerform(String permissionName, boolean isChangeOwner) {
-    List<PermissionRule> access = access(permissionName, isChangeOwner);
-    List<PermissionRule> overridden = relevant.getOverridden(permissionName);
-    Set<ProjectRef> allows = new HashSet<>();
-    Set<ProjectRef> blocks = new HashSet<>();
-    for (PermissionRule rule : access) {
-      if (rule.isBlock() && !rule.getForce()) {
-        blocks.add(relevant.getRuleProps(rule));
-      } else {
-        allows.add(relevant.getRuleProps(rule));
             }
           }
-    for (PermissionRule rule : overridden) {
+          break;
-      blocks.remove(relevant.getRuleProps(rule));
-    }
-    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);
-    List<PermissionRule> overridden = relevant.getOverridden(permissionName);
-    Set<ProjectRef> allows = new HashSet<>();
-    Set<ProjectRef> blocks = new HashSet<>();
-    for (PermissionRule rule : access) {
-      if (rule.isBlock()) {
-        blocks.add(relevant.getRuleProps(rule));
-      } else if (rule.getForce()) {
-        allows.add(relevant.getRuleProps(rule));
-      }
-    }
-    for (PermissionRule rule : overridden) {
-      if (rule.getForce()) {
-        blocks.remove(relevant.getRuleProps(rule));
-      }
-    }
-    blocks.removeAll(allows);
-    return blocks.isEmpty() && !allows.isEmpty();
-  }
-
-  /** True if for this permission force is blocked for the user. Works only for non labels. */
-  private boolean isForceBlocked(String permissionName) {
-    List<PermissionRule> access = access(permissionName);
-    List<PermissionRule> overridden = relevant.getOverridden(permissionName);
-    Set<ProjectRef> allows = new HashSet<>();
-    Set<ProjectRef> blocks = new HashSet<>();
-    for (PermissionRule rule : access) {
-      if (rule.isBlock()) {
-        blocks.add(relevant.getRuleProps(rule));
-      } else if (rule.getForce()) {
-        allows.add(relevant.getRuleProps(rule));
-      }
-    }
-    for (PermissionRule rule : overridden) {
-      if (rule.getForce()) {
-        blocks.remove(relevant.getRuleProps(rule));
-      }
-    }
-    blocks.removeAll(allows);
-    return !blocks.isEmpty();
-  }
-
-  /** Rules for the given permission, or the empty list. */
-  private List<PermissionRule> access(String permissionName) {
-    return access(permissionName, false);
-  }
-
-  /** Rules for the given permission, or the empty list. */
-  private List<PermissionRule> access(String permissionName, boolean isChangeOwner) {
-    List<PermissionRule> rules = effective.get(permissionName);
-    if (rules != null) {
-      return rules;
-    }
-
-    rules = relevant.getPermission(permissionName);
-
-    List<PermissionRule> mine = new ArrayList<>(rules.size());
-    for (PermissionRule rule : rules) {
-      if (projectControl.match(rule, isChangeOwner)) {
-        mine.add(rule);
         }
       }
 
-    if (mine.isEmpty()) {
+      blockAllowMin = Math.max(projectBlockAllowMin, blockAllowMin);
-      mine = Collections.emptyList();
+      blockAllowMax = Math.min(projectBlockAllowMax, blockAllowMax);
     }
-    effective.put(permissionName, mine);
+
-    return mine;
+    int voteMin = 0, voteMax = 0;
+    for (PermissionRule pr : relevant.getAllowRules(permissionName)) {
+      if (pr.getAction() == PermissionRule.Action.ALLOW
+          && projectControl.match(pr, isChangeOwner)) {
+        // For votes, contrary to normal permissions, we aggregate all applicable rules.
+        voteMin = Math.min(voteMin, pr.getMin());
+        voteMax = Math.max(voteMax, pr.getMax());
+      }
+    }
+
+    return new PermissionRange(
+        permissionName, Math.max(voteMin, blockAllowMin), Math.min(voteMax, blockAllowMax));
+  }
+
+  private boolean isBlocked(String permissionName, boolean isChangeOwner, boolean withForce) {
+    // Permissions are ordered by (more general project, more specific ref). Because Permission
+    // does not have back pointers, we can't tell what ref-pattern or project each permission comes
+    // from.
+    List<List<Permission>> downwardPerProject = relevant.getBlockRules(permissionName);
+
+    projectLoop:
+    for (List<Permission> projectRules : downwardPerProject) {
+      boolean overrideFound = false;
+      for (Permission p : projectRules) {
+        // If this is an exclusive ALLOW, then block rules from the same project are ignored.
+        if (p.getExclusiveGroup()) {
+          for (PermissionRule pr : p.getRules()) {
+            if (isAllow(pr, withForce) && projectControl.match(pr, isChangeOwner)) {
+              overrideFound = true;
+              break;
+            }
+          }
+        }
+        if (overrideFound) {
+          // Found an exclusive override, nothing further to do in this project.
+          continue projectLoop;
+        }
+
+        boolean blocked = false;
+        for (PermissionRule pr : p.getRules()) {
+          if (!withForce && pr.getForce()) {
+            // force on block rule only applies to withForce permission.
+            continue;
+          }
+
+          if (isBlock(pr, withForce) && projectControl.match(pr, isChangeOwner)) {
+            blocked = true;
+            break;
+          }
+        }
+
+        if (blocked) {
+          // ALLOW in the same AccessSection (ie. in the same Permission) overrides the BLOCK.
+          for (PermissionRule pr : p.getRules()) {
+            if (isAllow(pr, withForce) && projectControl.match(pr, isChangeOwner)) {
+              blocked = false;
+              break;
+            }
+          }
+        }
+
+        if (blocked) {
+          return true;
+        }
+      }
+    }
+
+    return false;
+  }
+
+  /** True if the user has this permission. */
+  private boolean canPerform(String permissionName, boolean isChangeOwner, boolean withForce) {
+    if (isBlocked(permissionName, isChangeOwner, withForce)) {
+      return false;
+    }
+
+    for (PermissionRule pr : relevant.getAllowRules(permissionName)) {
+      if (isAllow(pr, withForce) && projectControl.match(pr, isChangeOwner)) {
+        return true;
+      }
+    }
+
+    return false;
   }
 
   private class ForRefImpl extends ForRef {

javatests/com/google/gerrit/server/permissions/RefControlTest.java

@@ -635,6 +635,16 @@ public class RefControlTest {
     assertCanForceUpdate("refs/heads/master", u);
   }
 
+  @Test
+  public void unblockRead_NotPossible() {
+    block(parent, READ, ANONYMOUS_USERS, "refs/*");
+    allow(parent, READ, ADMIN, "refs/*");
+    allow(local, READ, ANONYMOUS_USERS, "refs/*");
+    allow(local, READ, ADMIN, "refs/*");
+    ProjectControl u = user(local);
+    assertCannotRead("refs/heads/master", u);
+  }
+
   @Test
   public void unblockForceWithAllowNoForce_NotPossible() {
     PermissionRule r = block(local, PUSH, ANONYMOUS_USERS, "refs/heads/*");
@@ -888,6 +898,18 @@ public class RefControlTest {
     assertCanVote(2, range);
   }
 
+  @Test
+  public void unionOfBlockedVotes() {
+    allow(parent, LABEL + "Code-Review", -1, +1, DEVS, "refs/heads/*");
+    block(parent, LABEL + "Code-Review", -2, +2, REGISTERED_USERS, "refs/heads/*");
+    block(local, LABEL + "Code-Review", -2, +1, REGISTERED_USERS, "refs/heads/*");
+
+    ProjectControl u = user(local, DEVS);
+    PermissionRange range = u.controlForRef("refs/heads/master").getRange(LABEL + "Code-Review");
+    assertCanVote(-1, range);
+    assertCannotVote(1, range);
+  }
+
   @Test
   public void blockOwner() {
     block(parent, OWNER, ANONYMOUS_USERS, "refs/*");