Fix faulty behaviour in BLOCK permission BLOCK can be overruled with ALLOW on the same project, however there is a bug that happens when a child of the above project duplicates the ALLOW permission, in this case the BLOCK will always win for the child, even though the BLOCK was overruled in the parent. This behaviour occurs because the ALLOW permission of the parent is overridden by the ALLOW of the child, so when the BLOCK check occurs the code thinks the permission should be blocked because it doesn't see the ALLOW of the parent and BLOCK can only be overruled from the same project. Bug: issue 2995 Change-Id: Ib100deb181a0fdb07527a7242c4d4e8c4fe24b9b
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 63d2b35..321c2ea 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
@@ -14,6 +14,7 @@ package com.google.gerrit.server.project; +import static com.google.common.base.Objects.firstNonNull; import static com.google.gerrit.server.project.RefControl.isRE; import com.google.common.collect.Lists; @@ -113,6 +114,7 @@ Set<String> exclusiveGroupPermissions = new HashSet<>(); HashMap<String, List<PermissionRule>> permissions = new HashMap<>(); + HashMap<String, List<PermissionRule>> overridden = new HashMap<>(); Map<PermissionRule, ProjectRef> ruleProps = Maps.newIdentityHashMap(); for (AccessSection section : sections) { Project.NameKey project = sectionToProject.get(section); @@ -128,11 +130,19 @@ } else { addRule = seen.add(s) && !rule.isDeny() && !exclusivePermissionExists; } + + HashMap<String, List<PermissionRule>> p = null; if (addRule) { - List<PermissionRule> r = permissions.get(permission.getName()); + p = permissions; + } else if (!rule.isDeny() && !exclusivePermissionExists) { + p = overridden; + } + + if (p != null) { + List<PermissionRule> r = p.get(permission.getName()); if (r == null) { r = new ArrayList<>(2); - permissions.put(permission.getName(), r); + p.put(permission.getName(), r); } r.add(rule); ruleProps.put(rule, new ProjectRef(project, section.getName())); @@ -145,18 +155,22 @@ } } - return new PermissionCollection(permissions, ruleProps, perUser); + return new PermissionCollection(permissions, overridden, ruleProps, + perUser); } } private final Map<String, List<PermissionRule>> rules; + private final Map<String, List<PermissionRule>> overridden; private final Map<PermissionRule, ProjectRef> ruleProps; private final boolean perUser; private PermissionCollection(Map<String, List<PermissionRule>> rules, + Map<String, List<PermissionRule>> overridden, Map<PermissionRule, ProjectRef> ruleProps, boolean perUser) { this.rules = rules; + this.overridden = overridden; this.ruleProps = ruleProps; this.perUser = perUser; } @@ -182,6 +196,11 @@ 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); }
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 00ecee3..7dc7a2a 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
@@ -120,6 +120,7 @@ */ public boolean isVisibleByRegisteredUsers() { List<PermissionRule> access = relevant.getPermission(Permission.READ); + List<PermissionRule> overridden = relevant.getOverridden(Permission.READ); Set<ProjectRef> allows = Sets.newHashSet(); Set<ProjectRef> blocks = Sets.newHashSet(); for (PermissionRule rule : access) { @@ -129,6 +130,11 @@ allows.add(relevant.getRuleProps(rule)); } } + for (PermissionRule rule : overridden) { + if (SystemGroupBackend.isAnonymousOrRegistered(rule.getGroup())) { + blocks.remove(relevant.getRuleProps(rule)); + } + } blocks.removeAll(allows); return blocks.isEmpty() && !allows.isEmpty(); } @@ -492,6 +498,7 @@ private boolean doCanPerform(String permissionName, boolean blockOnly) { List<PermissionRule> access = access(permissionName); + List<PermissionRule> overridden = relevant.getOverridden(permissionName); Set<ProjectRef> allows = Sets.newHashSet(); Set<ProjectRef> blocks = Sets.newHashSet(); for (PermissionRule rule : access) { @@ -501,6 +508,9 @@ allows.add(relevant.getRuleProps(rule)); } } + for (PermissionRule rule : overridden) { + blocks.remove(relevant.getRuleProps(rule)); + } blocks.removeAll(allows); return blocks.isEmpty() && (!allows.isEmpty() || blockOnly); } @@ -508,6 +518,7 @@ /** 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 = Sets.newHashSet(); Set<ProjectRef> blocks = Sets.newHashSet(); for (PermissionRule rule : access) { @@ -517,6 +528,11 @@ 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(); } @@ -524,6 +540,7 @@ /** 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 = Sets.newHashSet(); Set<ProjectRef> blocks = Sets.newHashSet(); for (PermissionRule rule : access) { @@ -533,6 +550,11 @@ allows.add(relevant.getRuleProps(rule)); } } + for (PermissionRule rule : overridden) { + if (rule.getForce()) { + blocks.remove(relevant.getRuleProps(rule)); + } + } blocks.removeAll(allows); return !blocks.isEmpty(); }
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 5e64199..5478a6c 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
@@ -371,6 +371,17 @@ } @Test + public void testInheritSubmit_AllowInChildDoesntAffectUnblockInParent() { + block(parent, SUBMIT, ANONYMOUS_USERS, "refs/heads/*"); + allow(parent, SUBMIT, REGISTERED_USERS, "refs/heads/*"); + allow(local, SUBMIT, REGISTERED_USERS, "refs/heads/*"); + + ProjectControl u = util.user(local); + assertFalse("not blocked from submitting", u.controlForRef( + "refs/heads/master").isBlocked(SUBMIT)); + } + + @Test public void testUnblockNoForce() { block(local, PUSH, ANONYMOUS_USERS, "refs/heads/*"); allow(local, PUSH, DEVS, "refs/heads/*");