opendev/gerrit
Code Issues Proposed changes

Introduced a new PermissionRule.Action: BLOCK.

Browse Source 
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
This commit is contained in:
Sasa Zivkov
2011-06-30 13:54:09 +02:00
parent e5f0457942
commit ae3b0598bf
6 changed files with 98 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
View File

@@ -16,7 +16,7 @@ package com.google.gerrit.common.data;
public class PermissionRule implements Comparable<PermissionRule> {
public static enum Action {
ALLOW, DENY,
ALLOW, DENY, BLOCK,
INTERACTIVE, BATCH;
}
@@ -53,6 +53,14 @@ public class PermissionRule implements Comparable<PermissionRule> {
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 @@ public class PermissionRule implements Comparable<PermissionRule> {
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 @@ public class PermissionRule implements Comparable<PermissionRule> {
r.append("deny ");
break;
case BLOCK:
r.append("block ");
break;
case INTERACTIVE:
r.append("interactive ");
break;
@@ -189,6 +204,10 @@ public class PermissionRule implements Comparable<PermissionRule> {
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();

3
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionRuleEditor.java
View File

@@ -124,7 +124,8 @@ public class PermissionRuleEditor extends Composite implements
action.setValue(PermissionRule.Action.ALLOW);
action.setAcceptableValues(Arrays.asList(
PermissionRule.Action.ALLOW,
PermissionRule.Action.DENY));
PermissionRule.Action.DENY,
PermissionRule.Action.BLOCK));
}
}

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

@@ -99,19 +99,25 @@ public class PermissionCollection {
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);

3
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -19,6 +19,7 @@ import com.google.gerrit.common.data.AccessSection;
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 @@ public class ProjectControl {
}
for (PermissionRule rule : permission.getRules()) {
if (rule.isDeny() || !match(rule)) {
if (rule.isBlock() || rule.isDeny() || !match(rule)) {
continue;
}

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

@@ -163,13 +163,17 @@ public class RefControl {
// granting of powers beyond pushing to the configuration.
return false;
}
boolean result = false;
for (PermissionRule rule : access(Permission.PUSH)) {
if (rule.getForce()) {
return true;
}
}
if (rule.isBlock()) {
return false;
}
if (rule.getForce()) {
result = true;
}
}
return result;
}
/**
* Determines whether the user can create a new Git ref.
@@ -313,16 +317,35 @@ public class RefControl {
List<PermissionRule> ruleList) {
int min = 0;
int max = 0;
int blockMin = Integer.MIN_VALUE;
int blockMax = Integer.MAX_VALUE;
for (PermissionRule rule : ruleList) {
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. */

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

@@ -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 static com.google.gerrit.common.data.Permission.SUBMIT;
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 @@ public class RefControlTest extends TestCase {
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 @@ public class RefControlTest extends TestCase {
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);