Consider rule action while constructing local owners list

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>

3 files changed, 48 insertions, 5 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
index 50f232a7bf..a7a96f0fd6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java
@@ -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());
             }
           }
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 74156dd226..059974f50d 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
@@ -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());
+  }
 }
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
index e4de9ed7e8..ba2aeca02d 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
@@ -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;