diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 70e9cfded6..9b97b94052 100644 --- a/Documentation/access-control.txt +++ b/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 -an inherited 'BLOCK' rule will not be honored. Searching for 'BLOCK' rules, in +be overridden in the inheriting project. Any 'ALLOW' rule, from a different +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 -------------------------------------------------- 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 229b062103..59b7670f65 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 @@ -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 access = relevant.getPermission(Permission.READ); + Set allows = Sets.newHashSet(); + Set blocks = Sets.newHashSet(); for (PermissionRule rule : access) { if (rule.isBlock()) { - return false; - } - } - for (PermissionRule rule : access) { - if (rule.getGroup().getUUID().equals(AccountGroup.ANONYMOUS_USERS) + blocks.add(relevant.getRuleProps(rule)); + } else 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; - for (PermissionRule rule : access(Permission.PUSH)) { - if (rule.isBlock()) { - return false; - } - if (rule.getForce()) { - result = true; - } - } - return result; + return canForcePerform(Permission.PUSH); } /** @@ -375,16 +369,7 @@ public class RefControl { /** @return true if this user can force edit topic names. */ public boolean canForceEditTopicName() { - boolean result = false; - for (PermissionRule rule : access(Permission.EDIT_TOPIC_NAME)) { - if (rule.isBlock()) { - return false; - } - if (rule.getForce()) { - result = true; - } - } - return result; + return canForcePerform(Permission.EDIT_TOPIC_NAME); } /** All value ranges of any allowed label permission. */ @@ -416,39 +401,97 @@ public class RefControl { return null; } - private static PermissionRange toRange(String permissionName, - List ruleList) { - int min = 0; - int max = 0; - int blockMin = Integer.MIN_VALUE; - int blockMax = Integer.MAX_VALUE; - for (PermissionRule rule : ruleList) { + private static class AllowedRange { + private int allowMin = 0; + private int allowMax = 0; + private int blockMin = Integer.MIN_VALUE; + private int blockMax = Integer.MAX_VALUE; + + 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()); - max = Math.max(max, rule.getMax()); + allowMin = Math.min(allowMin, rule.getMin()); + 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) { - max = Math.min(max, blockMax - 1); + int getAllowMax() { + 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 ruleList) { + Map 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 access = access(permissionName); + Set allows = Sets.newHashSet(); + Set 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 access = access(permissionName); + Set allows = Sets.newHashSet(); + Set 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. */ 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 ab5ad59a2c..51ea5a2725 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.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 all;