Add possibility to override permissions from parent project.

In case if local permissions have the same (category,group,ref) as a
permission from parent, then parent's permission is overriden by the
local one.

Here is an example

ParentProject (READ, group1, refs/*) -> [-1,1] # Allow groupA read project
ChildProject (READ, group1, refs/*) -> [-1,-1] # Override parent's permission.
                                               # Now group1 cannot read the project

Pay attention that is refspec is different from the parent's permission,
then the local permission *adds* rights. An example:

ParentProject (READ, group1, refs/*) -> [-1,1] # Allow groupA read project
ChildProject (READ, group1, refs/tags/*) -> [-1,-1]
 # This permission does nothing as it adds [-1,-1] for refs/tags/*

In this case if you want to disallow to group1 read refs/tags/* then you
need to override parent's permission first.

ParentProject (READ, group1, refs/*) -> [-1,1] # Allow groupA read project
ChildProject (READ, group1, refs/*) -> [-1,-1] # Override parent's permission
ChildProject (READ, group1, -refs/tags/*) -> [-1,1]
 # Explicitly says that group1 has access to everything except refs/tags/*

Change-Id: If183e6255e4a1625b557e3bbc024093b18740d04

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

@@ -191,29 +191,19 @@ public class ProjectControl {
         || canPerformOnAnyRef(ApprovalCategory.PUSH_TAG, (short) 1);
   }
 
+  // TODO (anatol.pomazau): Try to merge this method with similar RefRightsForPattern#canPerform
   private boolean canPerformOnAnyRef(ApprovalCategory.Id actionId,
       short requireValue) {
     final Set<AccountGroup.Id> groups = user.getEffectiveGroups();
-    int val = Integer.MIN_VALUE;
 
-    for (final RefRight pr : state.getLocalRights(actionId)) {
+    for (final RefRight pr : state.getAllRights(actionId, true)) {
-      if (groups.contains(pr.getAccountGroupId())) {
+      if (groups.contains(pr.getAccountGroupId())
-        val = Math.max(pr.getMaxValue(), val);
+          && pr.getMaxValue() >= requireValue) {
-      }
+        return true;
-    }
-    if (val >= requireValue) {
-      return true;
-    }
-
-    if (actionId.canInheritFromWildProject()) {
-      for (final RefRight pr : state.getInheritedRights(actionId)) {
-        if (groups.contains(pr.getAccountGroupId())) {
-          val = Math.max(pr.getMaxValue(), val);
-        }
       }
     }
 
-    return val >= requireValue;
+    return false;
   }
 
   private boolean canPerformOnAllRefs(ApprovalCategory.Id actionId,
@@ -238,14 +228,9 @@ public class ProjectControl {
 
   private Set<String> allRefPatterns(ApprovalCategory.Id actionId) {
     final Set<String> all = new HashSet<String>();
-    for (final RefRight pr : state.getLocalRights(actionId)) {
+    for (final RefRight pr : state.getAllRights(actionId, true)) {
       all.add(pr.getRefPattern());
     }
-    if (actionId.canInheritFromWildProject()) {
-      for (final RefRight pr : state.getInheritedRights(actionId)) {
-        all.add(pr.getRefPattern());
-      }
-    }
     return all;
   }
 

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

@@ -28,6 +28,8 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
 
@@ -137,16 +139,59 @@ public class ProjectState {
   }
 
   /**
-   * Get the rights this project inherits from the wild project.
+   * Utility class that is needed to filter overridden refrights
+   */
+  private static class Grant {
+    final AccountGroup.Id group;
+    final String pattern;
+
+    private Grant(AccountGroup.Id group, String pattern) {
+      this.group = group;
+      this.pattern = pattern;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      Grant grant = (Grant) o;
+      return group.equals(grant.group) && pattern.equals(grant.pattern);
+    }
+
+    @Override
+    public int hashCode() {
+      int result = group.hashCode();
+      result = 31 * result + pattern.hashCode();
+      return result;
+    }
+  }
+
+  /**
+   * Get the rights this project has and inherits from the wild project.
    *
    * @param action the category requested.
+   * @param dropOverridden whether to remove inherited permissions in case if we have a
+   *     local one that matches (action,group,ref)
    * @return immutable collection of rights for the requested category.
    */
-  public Collection<RefRight> getInheritedRights(ApprovalCategory.Id action) {
+  public Collection<RefRight> getAllRights(ApprovalCategory.Id action, boolean dropOverridden) {
+    Collection<RefRight> rights = new LinkedList<RefRight>(getLocalRights(action));
     if (action.canInheritFromWildProject()) {
-      return filter(getInheritedRights(), action);
+      rights.addAll(filter(getInheritedRights(), action));
     }
-    return Collections.emptyList();
+    if (dropOverridden) {
+      Set<Grant> grants = new HashSet<Grant>();
+      Iterator<RefRight> iter = rights.iterator();
+      while (iter.hasNext()) {
+        RefRight right = iter.next();
+
+        Grant grant = new Grant(right.getAccountGroupId(), right.getRefPattern());
+        if (grants.contains(grant)) {
+          iter.remove();
+        } else {
+          grants.add(grant);
+        }
+      }
+    }
+    return Collections.unmodifiableCollection(rights);
   }
 
   /** Is this the special wild project which manages inherited rights? */

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

@@ -302,41 +302,35 @@ public class RefControl {
      * @param groups The groups of the user
      * @return The allowed value for this ref for all the specified groups
      */
-    public int allowedValueForRef(Set<AccountGroup.Id> groups) {
+    private boolean allowedValueForRef(Set<AccountGroup.Id> groups, short level) {
-      int val = Integer.MIN_VALUE;
       for (RefRight right : rights) {
-        if (groups.contains(right.getAccountGroupId())) {
+        if (groups.contains(right.getAccountGroupId())
-          val = Math.max(right.getMaxValue(), val);
+            && right.getMaxValue() >= level) {
+          return true;
         }
       }
-      return val;
+      return false;
     }
   }
 
   boolean canPerform(ApprovalCategory.Id actionId, short level) {
     final Set<AccountGroup.Id> groups = getCurrentUser().getEffectiveGroups();
-    int val = Integer.MIN_VALUE;
 
     List<RefRight> allRights = new ArrayList<RefRight>();
-    allRights.addAll(getLocalRights(actionId));
+    allRights.addAll(getAllRights(actionId));
-
-    if (actionId.canInheritFromWildProject()) {
-      allRights.addAll(getInheritedRights(actionId));
-    }
 
     SortedMap<String, RefRightsForPattern> perPatternRights =
       sortedRightsByPattern(allRights);
 
     for (RefRightsForPattern right : perPatternRights.values()) {
-      val = Math.max(val, right.allowedValueForRef(groups));
+      if (right.allowedValueForRef(groups, level)) {
-      if (val >= level) {
+        return true;
-        break;
       }
       if (right.containsExclusive() && !actionId.equals(OWN)) {
         break;
       }
     }
-    return val >= level;
+    return false;
   }
 
   /**
@@ -467,12 +461,8 @@ public class RefControl {
     return rights;
   }
 
-  private List<RefRight> getLocalRights(ApprovalCategory.Id actionId) {
+  private List<RefRight> getAllRights(ApprovalCategory.Id actionId) {
-    return filter(getProjectState().getLocalRights(actionId));
+    return filter(getProjectState().getAllRights(actionId, true));
-  }
-
-  private List<RefRight> getInheritedRights(ApprovalCategory.Id actionId) {
-    return filter(getProjectState().getInheritedRights(actionId));
   }
 
   /**
@@ -487,8 +477,7 @@ public class RefControl {
    */
   public List<RefRight> getApplicableRights(final ApprovalCategory.Id id) {
     List<RefRight> l = new ArrayList<RefRight>();
-    l.addAll(getLocalRights(id));
+    l.addAll(getAllRights(id));
-    l.addAll(getInheritedRights(id));
     SortedMap<String, RefRightsForPattern> perPatternRights =
       sortedRightsByPattern(l);
     List<RefRight> applicable = new ArrayList<RefRight>();

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 import static com.google.gerrit.reviewdb.ApprovalCategory.OWN;
 import static com.google.gerrit.reviewdb.ApprovalCategory.READ;
+import static com.google.gerrit.reviewdb.ApprovalCategory.SUBMIT;
 
 import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.reviewdb.AccountProjectWatch;
@@ -50,7 +51,7 @@ import java.util.Set;
 
 public class RefControlTest extends TestCase {
   public void testOwnerProject() {
-    local.add(grant(OWN, admin, "refs/*", 1));
+    grant(local, OWN, admin, "refs/*", 1);
 
     ProjectControl uBlah = user(devs);
     ProjectControl uAdmin = user(devs, admin);
@@ -60,8 +61,8 @@ public class RefControlTest extends TestCase {
   }
 
   public void testBranchDelegation1() {
-    local.add(grant(OWN, admin, "refs/*", 1));
+    grant(local, OWN, admin, "refs/*", 1);
-    local.add(grant(OWN, devs, "refs/heads/x/*", 1));
+    grant(local, OWN, devs, "refs/heads/x/*", 1);
 
     ProjectControl uDev = user(devs);
     assertFalse("not owner", uDev.isOwner());
@@ -76,9 +77,9 @@ public class RefControlTest extends TestCase {
   }
 
   public void testBranchDelegation2() {
-    local.add(grant(OWN, admin, "refs/*", 1));
+    grant(local, OWN, admin, "refs/*", 1);
-    local.add(grant(OWN, devs, "refs/heads/x/*", 1));
+    grant(local, OWN, devs, "refs/heads/x/*", 1);
-    local.add(grant(OWN, fixers, "-refs/heads/x/y/*", 1));
+    grant(local, OWN, fixers, "-refs/heads/x/y/*", 1);
 
     ProjectControl uDev = user(devs);
     assertFalse("not owner", uDev.isOwner());
@@ -103,8 +104,8 @@ public class RefControlTest extends TestCase {
   }
 
   public void testInheritRead_SingleBranchDeniesUpload() {
-    inherited.add(grant(READ, registered, "refs/*", 1, 2));
+    grant(parent, READ, registered, "refs/*", 1, 2);
-    local.add(grant(READ, registered, "-refs/heads/foobar", 1, 1));
+    grant(local, READ, registered, "-refs/heads/foobar", 1);
 
     ProjectControl u = user();
     assertTrue("can upload", u.canPushToAtLeastOneRef());
@@ -117,8 +118,8 @@ public class RefControlTest extends TestCase {
   }
 
   public void testInheritRead_SingleBranchDoesNotOverrideInherited() {
-    inherited.add(grant(READ, registered, "refs/*", 1, 2));
+    grant(parent, READ, registered, "refs/*", 1, 2);
-    local.add(grant(READ, registered, "refs/heads/foobar", 1, 1));
+    grant(local, READ, registered, "refs/heads/foobar", 1);
 
     ProjectControl u = user();
     assertTrue("can upload", u.canPushToAtLeastOneRef());
@@ -130,9 +131,51 @@ public class RefControlTest extends TestCase {
         u.controlForRef("refs/heads/foobar").canUpload());
   }
 
+  public void testInheritRead_OverrideWithDeny() {
+    grant(parent, READ, registered, "refs/*", 1);
+    grant(local, READ, registered, "refs/*", 0);
+
+    ProjectControl u = user();
+    assertFalse("can't read", u.isVisible());
+  }
+
+  public void testInheritRead_AppendWithDenyOfRef() {
+    grant(parent, READ, registered, "refs/*", 1);
+    grant(local, READ, registered, "refs/heads/*", 0);
+
+    ProjectControl u = user();
+    assertTrue("can read", u.isVisible());
+    assertTrue("can read", u.controlForRef("refs/master").isVisible());
+    assertTrue("can read", u.controlForRef("refs/tags/foobar").isVisible());
+    assertTrue("no master", u.controlForRef("refs/heads/master").isVisible());
+  }
+
+  public void testInheritRead_OverridesAndDeniesOfRef() {
+    grant(parent, READ, registered, "refs/*", 1);
+    grant(local, READ, registered, "refs/*", 0);
+    grant(local, READ, registered, "refs/heads/*", -1, 1);
+
+    ProjectControl u = user();
+    assertTrue("can read", u.isVisible());
+    assertFalse("can't read", u.controlForRef("refs/foobar").isVisible());
+    assertFalse("can't read", u.controlForRef("refs/tags/foobar").isVisible());
+    assertTrue("can read", u.controlForRef("refs/heads/foobar").isVisible());
+  }
+
+  public void testInheritSubmit_OverridesAndDeniesOfRef() {
+    grant(parent, SUBMIT, registered, "refs/*", 1);
+    grant(local, SUBMIT, registered, "refs/*", 0);
+    grant(local, SUBMIT, registered, "refs/heads/*", -1, 1);
+
+    ProjectControl u = user();
+    assertFalse("can't submit", u.controlForRef("refs/foobar").canSubmit());
+    assertFalse("can't submit", u.controlForRef("refs/tags/foobar").canSubmit());
+    assertTrue("can submit", u.controlForRef("refs/heads/foobar").canSubmit());
+  }
+
   public void testCannotUploadToAnyRef() {
-    inherited.add(grant(READ, registered, "refs/*", 1, 1));
+    grant(parent, READ, registered, "refs/*", 1);
-    local.add(grant(READ, devs, "refs/heads/*",1,2));
+    grant(local, READ, devs, "refs/heads/*", 1, 2);
 
     ProjectControl u = user();
     assertFalse("cannot upload", u.canPushToAtLeastOneRef());
@@ -143,7 +186,8 @@ public class RefControlTest extends TestCase {
 
   // -----------------------------------------------------------------------
 
-  private final Project.NameKey projectNameKey = new Project.NameKey("test");
+  private final Project.NameKey local = new Project.NameKey("test");
+  private final Project.NameKey parent = new Project.NameKey("parent");
   private final AccountGroup.Id admin = new AccountGroup.Id(1);
   private final AccountGroup.Id anonymous = new AccountGroup.Id(2);
   private final AccountGroup.Id registered = new AccountGroup.Id(3);
@@ -183,14 +227,14 @@ public class RefControlTest extends TestCase {
     anonymousUser = injector.getInstance(AnonymousUser.class);
   }
 
-  private List<RefRight> local;
+  private List<RefRight> localRights;
-  private List<RefRight> inherited;
+  private List<RefRight> inheritedRights;
 
   @Override
   protected void setUp() throws Exception {
     super.setUp();
-    local = new ArrayList<RefRight>();
+    localRights = new ArrayList<RefRight>();
-    inherited = new ArrayList<RefRight>();
+    inheritedRights = new ArrayList<RefRight>();
   }
 
   private static void assertOwner(String ref, ProjectControl u) {
@@ -201,19 +245,26 @@ public class RefControlTest extends TestCase {
     assertFalse("NOT OWN " + ref, u.controlForRef(ref).isOwner());
   }
 
-  private RefRight grant(ApprovalCategory.Id categoryId, AccountGroup.Id group,
+  private void grant(Project.NameKey project, ApprovalCategory.Id categoryId, 
-      String ref, int maxValue) {
+      AccountGroup.Id group, String ref, int maxValue) {
-    return grant(categoryId, group, ref, maxValue, maxValue);
+    grant(project, categoryId, group, ref, maxValue, maxValue);
   }
 
-  private RefRight grant(ApprovalCategory.Id categoryId, AccountGroup.Id group,
+  private void grant(Project.NameKey project, ApprovalCategory.Id categoryId, AccountGroup.Id group,
       String ref, int minValue, int maxValue) {
     RefRight right =
-        new RefRight(new RefRight.Key(projectNameKey, new RefPattern(ref),
+        new RefRight(new RefRight.Key(project, new RefPattern(ref),
             categoryId, group));
     right.setMinValue((short) minValue);
     right.setMaxValue((short) maxValue);
-    return right;
+
+    if (project == parent) {
+      inheritedRights.add(right);
+    } else if (project == local) {
+      localRights.add(right);
+    } else {
+      fail("Unknown project key: " + project);
+    }
   }
 
   private ProjectControl user(AccountGroup.Id... memberOf) {
@@ -228,8 +279,8 @@ public class RefControlTest extends TestCase {
     ProjectControl.AssistedFactory projectControlFactory = null;
     ProjectState ps =
         new ProjectState(anonymousUser, projectCache, wildProject,
-            projectControlFactory, new Project(projectNameKey), local);
+            projectControlFactory, new Project(parent), localRights);
-    ps.setInheritedRights(inherited);
+    ps.setInheritedRights(inheritedRights);
     return ps;
   }