Introduced a new PermissionRule.Action: BLOCK.
Besides already existing ALLOW and DENY actions this change introduces
the BLOCK action in order to enable blocking some permission rules
globally.
A typical use case, and the main motivation for this change, is to
globally block update and deletion of tags. When tags are used to tag
releases it is important to ensure that tags are never updated or
deleted.
When looking to see if we can perform a given permission in RefControl
/ ProjectControl, we look to see if any of the parents has defined a
section matching this reference with BLOCK for the permission's
action. If so, we are forbidden from using the permission, even if
there is an ALLOW. This searching for BLOCK ignores the Exclusive flag
that is normally applied to sections.
A BLOCK-ing push rule blocks any type of push, force or not. A
BLOCK-ing force push rule blocks only force pushes, but allows
non-forced pushes if an ALLOW rule would have permitted it.
Site administrators that need to deny tag updates can thus put into
their All-Projects.git:
[access "refs/tags/*"]
push = block group Anonymous Users
It is also possible to block label ranges. To block a group X from
voting -2 and +2, but keep their existing voting permissions for the
-1..+1 range intact we would define:
[access "refs/heads/*"]
label-Code-Review = block -2..+2 group X
The interpretation of the min..max range in case of a blocking rule
is: block every vote from -INFINITE..min and max..INFINITE. This is the
only interpretation of the min..max range for a blocking rule that makes
sense for practical purposes IMO.
NOTE: Gerrit UI is still not able to produce a blocking label range rule.
The UI needs to be extended for that. This will be done in a follow up
change.
Change-Id: I8baeff2a49280fde05b2112df99322b87ce5d442
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
index 6deb14f..2ddd8a7 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
@@ -16,7 +16,7 @@
public class PermissionRule implements Comparable<PermissionRule> {
public static enum Action {
- ALLOW, DENY,
+ ALLOW, DENY, BLOCK,
INTERACTIVE, BATCH;
}
@@ -53,6 +53,14 @@
action = Action.DENY;
}
+ public boolean isBlock() {
+ return action == Action.BLOCK;
+ }
+
+ public void setBlock() {
+ action = Action.BLOCK;
+ }
+
public Boolean getForce() {
return force;
}
@@ -97,7 +105,10 @@
void mergeFrom(PermissionRule src) {
if (getAction() != src.getAction()) {
- if (getAction() == Action.DENY || src.getAction() == Action.DENY) {
+ if (getAction() == Action.BLOCK || src.getAction() == Action.BLOCK) {
+ setAction(Action.BLOCK);
+
+ } else if (getAction() == Action.DENY || src.getAction() == Action.DENY) {
setAction(Action.DENY);
} else if (getAction() == Action.BATCH || src.getAction() == Action.BATCH) {
@@ -151,6 +162,10 @@
r.append("deny ");
break;
+ case BLOCK:
+ r.append("block ");
+ break;
+
case INTERACTIVE:
r.append("interactive ");
break;
@@ -189,6 +204,10 @@
rule.setAction(Action.DENY);
src = src.substring("deny ".length()).trim();
+ } else if (src.startsWith("block ")) {
+ rule.setAction(Action.BLOCK);
+ src = src.substring("block ".length()).trim();
+
} else if (src.startsWith("interactive ")) {
rule.setAction(Action.INTERACTIVE);
src = src.substring("interactive ".length()).trim();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java
index 98e6283..7f5b573 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java
@@ -124,7 +124,8 @@
action.setValue(PermissionRule.Action.ALLOW);
action.setAcceptableValues(Arrays.asList(
PermissionRule.Action.ALLOW,
- PermissionRule.Action.DENY));
+ PermissionRule.Action.DENY,
+ PermissionRule.Action.BLOCK));
}
}
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 1196b02..02f56c5 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
@@ -99,19 +99,25 @@
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 =
new HashMap<String, List<PermissionRule>>();
for (AccessSection section : sections) {
for (Permission permission : section.getPermissions()) {
- if (exclusiveGroupPermissions.contains(permission.getName())) {
- continue;
- }
+ boolean exclusivePermissionExists =
+ exclusiveGroupPermissions.contains(permission.getName());
for (PermissionRule rule : permission.getRules()) {
SeenRule s = new SeenRule(section, permission, rule);
- if (seen.add(s) && !rule.isDeny()) {
+ boolean addRule = false;
+ if (rule.isBlock()) {
+ addRule = seenBlockingRules.add(s);
+ } else {
+ addRule = seen.add(s) && !rule.isDeny() && !exclusivePermissionExists;
+ }
+ if (addRule) {
List<PermissionRule> r = permissions.get(permission.getName());
if (r == null) {
r = new ArrayList<PermissionRule>(2);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index 1024f9a..ee42833 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -19,6 +19,7 @@
import com.google.gerrit.common.data.Capable;
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.AbstractAgreement;
import com.google.gerrit.reviewdb.AccountAgreement;
import com.google.gerrit.reviewdb.AccountGroup;
@@ -393,7 +394,7 @@
}
for (PermissionRule rule : permission.getRules()) {
- if (rule.isDeny() || !match(rule)) {
+ if (rule.isBlock() || rule.isDeny() || !match(rule)) {
continue;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index 984623d..9c60133 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -163,12 +163,16 @@
// granting of powers beyond pushing to the configuration.
return false;
}
+ boolean result = false;
for (PermissionRule rule : access(Permission.PUSH)) {
+ if (rule.isBlock()) {
+ return false;
+ }
if (rule.getForce()) {
- return true;
+ result = true;
}
}
- return false;
+ return result;
}
/**
@@ -313,16 +317,35 @@
List<PermissionRule> ruleList) {
int min = 0;
int max = 0;
+ int blockMin = Integer.MIN_VALUE;
+ int blockMax = Integer.MAX_VALUE;
for (PermissionRule rule : ruleList) {
- min = Math.min(min, rule.getMin());
- max = Math.max(max, rule.getMax());
+ if (rule.isBlock()) {
+ blockMin = Math.max(blockMin, rule.getMin());
+ blockMax = Math.min(blockMax, rule.getMax());
+ } else {
+ min = Math.min(min, rule.getMin());
+ max = Math.max(max, rule.getMax());
+ }
+ }
+ if (blockMin > Integer.MIN_VALUE) {
+ min = Math.max(min, blockMin + 1);
+ }
+ if (blockMax < Integer.MAX_VALUE) {
+ max = Math.min(max, blockMax - 1);
}
return new PermissionRange(permissionName, min, max);
}
/** True if the user has this permission. Works only for non labels. */
boolean canPerform(String permissionName) {
- return !access(permissionName).isEmpty();
+ List<PermissionRule> access = access(permissionName);
+ for (PermissionRule rule : access) {
+ if (rule.isBlock() && !rule.getForce()) {
+ return false;
+ }
+ }
+ return !access.isEmpty();
}
/** Rules for the given permission, or the empty list. */
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 f829ba0..29b2762 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
@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
+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;
import static com.google.gerrit.common.data.Permission.READ;
@@ -21,6 +22,7 @@
import com.google.gerrit.common.data.Capable;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.reviewdb.AccountGroup;
import com.google.gerrit.reviewdb.AccountProjectWatch;
@@ -221,6 +223,26 @@
assertTrue("d can read", d.controlForRef("refs/heads/foo-QA-bar").isVisible());
}
+ public void testBlockRule_ParentBlocksChild() {
+ grant(local, PUSH, devs, "refs/tags/*");
+ grant(parent, PUSH, anonymous, "refs/tags/*").setBlock();
+
+ ProjectControl u = user(devs);
+ assertFalse("u can't force update tag", u.controlForRef("refs/tags/V10").canForceUpdate());
+ }
+
+ public void testBlockLabelRange_ParentBlocksChild() {
+ grant(local, LABEL + "Code-Review", -2, +2, devs, "refs/heads/*");
+ grant(parent, LABEL + "Code-Review", -2, +2, devs, "refs/heads/*").setBlock();
+
+ ProjectControl u = user(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));
+ }
// -----------------------------------------------------------------------
private final Map<Project.NameKey, ProjectState> all;
@@ -317,7 +339,20 @@
private PermissionRule grant(ProjectConfig project, String permissionName,
AccountGroup.UUID group, String ref) {
+ return grant(project, permissionName, newRule(project, group), ref);
+ }
+
+ private PermissionRule grant(ProjectConfig project, String permissionName,
+ int min, int max, AccountGroup.UUID group, String ref) {
PermissionRule rule = newRule(project, group);
+ rule.setMin(min);
+ rule.setMax(max);
+ return grant(project, permissionName, rule, ref);
+ }
+
+
+ private PermissionRule grant(ProjectConfig project, String permissionName,
+ PermissionRule rule, String ref) {
project.getAccessSection(ref, true) //
.getPermission(permissionName, true) //
.add(rule);
