opendev/gerrit
Code Issues Proposed changes

Consider rule action while constructing local owners list

Browse Source 
Previously rule action was not considered during computation of local
owners list in ProjectState. This means that members of group that was
added to OWNER permission with BLOCK or DENY action were considered as
project owners.

This patch fixes this security breach. Now groups assigned to OWNER
permission with BLOCK or DENY action will not be recognized as owners

Change-Id: I048f52d7b23b55c9e8843c5b2c9907b3326c8d40
Signed-off-by: Dariusz Luksza <dariusz@luksza.org>
This commit is contained in:
Dariusz Luksza
2014-11-17 10:15:43 +01:00
committed by David Pursehouse
parent a1a5d62da9
commit b37ea2c10e
3 changed files with 48 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
import static com.google.gerrit.common.data.PermissionRule.Action.ALLOW;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Function;
@@ -141,7 +142,7 @@ public class ProjectState {
if (owner != null) {
for (PermissionRule rule : owner.getRules()) {
GroupReference ref = rule.getGroup();
if (ref.getUUID() != null) {
if (rule.getAction() == ALLOW && ref.getUUID() != null) {
groups.add(ref.getUUID());
}
}

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

@@ -25,6 +25,9 @@ import static com.google.gerrit.server.group.SystemGroupBackend.CHANGE_OWNER;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.Util.ADMIN;
import static com.google.gerrit.server.project.Util.DEVS;
import static com.google.gerrit.server.project.Util.allow;
import static com.google.gerrit.server.project.Util.block;
import static com.google.gerrit.server.project.Util.deny;
import static com.google.gerrit.server.project.Util.doNotInherit;
import static com.google.gerrit.server.project.Util.grant;
import static com.google.gerrit.testutil.InMemoryRepositoryManager.newRepository;
@@ -70,11 +73,23 @@ public class RefControlTest {
public void testOwnerProject() {
grant(local, OWNER, ADMIN, "refs/*");
ProjectControl uBlah = util.user(local, DEVS);
ProjectControl uAdmin = util.user(local, DEVS, ADMIN);
assertAdminsAreOwnersAndDevsAreNot();
}
assertFalse("not owner", uBlah.isOwner());
assertTrue("is owner", uAdmin.isOwner());
@Test
public void testDenyOwnerProject() {
allow(local, OWNER, ADMIN, "refs/*");
deny(local, OWNER, DEVS, "refs/*");
assertAdminsAreOwnersAndDevsAreNot();
}
@Test
public void testBlockOwnerProject() {
allow(local, OWNER, ADMIN, "refs/*");
block(local, OWNER, DEVS, "refs/*");
assertAdminsAreOwnersAndDevsAreNot();
}
@Test
@@ -523,4 +538,12 @@ public class RefControlTest {
assertFalse("u can vote -2", range.contains(-2));
assertFalse("u can vote +2", range.contains(2));
}
private void assertAdminsAreOwnersAndDevsAreNot() {
ProjectControl uBlah = util.user(local, DEVS);
ProjectControl uAdmin = util.user(local, DEVS, ADMIN);
assertFalse("not owner", uBlah.isOwner());
assertTrue("is owner", uAdmin.isOwner());
}
}

19
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@@ -127,6 +127,25 @@ public class Util {
return rule;
}
public static PermissionRule allow(ProjectConfig project,
String permissionName, AccountGroup.UUID group, String ref) {
return grant(project, permissionName, newRule(project, group), ref);
}
public static PermissionRule block(ProjectConfig project,
String permissionName, AccountGroup.UUID group, String ref) {
PermissionRule r = grant(project, permissionName, newRule(project, group), ref);
r.setBlock();
return r;
}
public static PermissionRule deny(ProjectConfig project,
String permissionName, AccountGroup.UUID group, String ref) {
PermissionRule r = grant(project, permissionName, newRule(project, group), ref);
r.setDeny();
return r;
}
private final Map<Project.NameKey, ProjectState> all;
private final ProjectCache projectCache;
private final CapabilityControl.Factory capabilityControlFactory;