Fixing bug reported by Nuno about layer group overrides

doc/en/user/source/security/layer.rst

@@ -56,6 +56,8 @@ Limited to WMS, layers will be also secured by considering their containing laye
 * Rules with other types of groups (*Named tree*, *Container tree*, *Earth Observation tree*) also affect contained layers and nested layer groups. 
   If the group is not accessible, the layers and groups contained in it won't either.
   The only exception is when another layer group contains the same layer or nested group, in that case the layers they will show up under the allowed groups.
+* Workspace rules gets precedence over global layer group ones when it comes to allow access to layers
+* Layer rules get precedence over all layer group rules when it comes to allow access to layers
 
 The following tables summarizes the layer group behavior depending on whether they are used in a public or secured environment:
 
@@ -282,57 +284,94 @@ To clarify, lets assume the following starting situation, in which all layers an
 
     root
     +- namedTreeGroupA
-    |   |   layerA
-    |   └   layerB
+    |   |   ws1:layerA
+    |   └   ws2:layerB
     +- namedTreeGroupB
-    |   |   layerB
-    |   └   layerC
+    |   |   ws2:layerB
+    |   └   ws1:layerC
     +- layerD
-    +- singleGroupC (contains layerA and layerD when requested)
+    +- singleGroupC (contains ws1:layerA and layerD when requested)
 
 
 Here are a few examples of how the structure changes based on different security rules.
 
 * Denying access to ``namedTreeGroupA`` by::
 
-    ``namedTreeGroupA.r=ROLE_PRIVATE`` 
+    namedTreeGroupA.r=ROLE_PRIVATE 
 
   Will give the following capabilities tree to anonymous users::
 
     root
     +- namedTreeGroupB
-    |   |   layerB
-    |   └   layerC
+    |   |   ws2:layerB
+    |   └   ws1:layerC
     +- layerD
     +- singleGroupC (contains only layerD when requested)
 
 
 * Denying access to ``namedTreeGroupB``by ::
 
-    ``namedTreeGroupB.r=ROLE_PRIVATE`` 
+    namedTreeGroupB.r=ROLE_PRIVATE 
 
   Will give the following capabilities tree to anonymous users::
 
     root
     +- namedTreeGroupA
-    |   |   layerA
-    |   └   layerB
+    |   |   ws1:layerA
+    |   └   ws2:layerB
     +- layerD
-    +- singleGroupC (contains layerA and layerD when requested)
+    +- singleGroupC (contains ws1:layerA and layerD when requested)
 
 * Denying access to ``singleGroupC`` by::
 
-    ``singleGroupC.r=ROLE_PRIVATE`` 
+    singleGroupC.r=ROLE_PRIVATE 
 
   Will give the following capabilities tree to anonymous users::
 
     root
     +- namedTreeGroupA
-    |   |   layerA
-    |   └   layerB
+    |   |   ws1:layerA
+    |   └   ws2:layerB
     +- namedTreeGroupB
-    |   |   layerB
-    |   └   layerC
+    |   |   ws2:layerB
+    |   └   ws1:layerC
     +- layerD
+    
+* Denying access to everything, but allowing explicit access to namedTreeGroupA by::
 
+    nameTreeGroupA.r=*
+    *.*.r=PRIVATE
+    *.*.w=PRIVATE 
 
+  Will give the following capabilities tree to anonymous users::
+
+    root
+    +- namedTreeGroupA
+        |   ws1:layerA
+        └   ws2:layerB
+
+* Denying access to ``nameTreeA`` and ``namedTreeGroupB`` but explicitly allowing access to ``ws1:layerA``::
+
+    namedTreeGroupA.r=ROLE_PRIVATE
+    namedTreeGroupB.r=ROLE_PRIVATE
+    ws1.layerA.r=* 
+
+  Will give the following capabilities tree to anonymous users (notice how ws1:layerA popped up to the root)::
+
+    root
+    +- ws1:layerA
+    +- layerD
+
+* Denying access to ``nameTreeA`` and ``namedTreeGroupB`` but explicitly allowing all layers in ws2
+  (a workspace rules overrides global groups ones)::
+
+    namedTreeGroupA.r=ROLE_PRIVATE
+    namedTreeGroupB.r=ROLE_PRIVATE
+    ws2.*.r=* 
+
+  Will give the following capabilities tree to anonymous users (notice how ws1:layerB popped up to the root)::
+
+    root
+    +- ws2:layerB
+    +- layerD
+    +- singleGroupC

src/main/src/main/java/org/geoserver/security/impl/DefaultResourceAccessManager.java

@@ -8,7 +8,6 @@
 import static org.geoserver.security.impl.DataAccessRule.ANY;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -30,7 +29,6 @@
 import org.geoserver.catalog.Predicates;
 import org.geoserver.catalog.PublishedInfo;
 import org.geoserver.catalog.ResourceInfo;
-import org.geoserver.catalog.StoreInfo;
 import org.geoserver.catalog.StyleInfo;
 import org.geoserver.catalog.WMSLayerInfo;
 import org.geoserver.catalog.WorkspaceInfo;
@@ -183,34 +181,35 @@ public boolean canAccess(Authentication user, LayerInfo layer, AccessMode mode)
     public boolean canAccess(Authentication user, ResourceInfo resource, AccessMode mode) {
         checkPropertyFile();
         String workspace;
+        final String resourceName = resource.getName();
         try {
             workspace = resource.getStore().getWorkspace().getName();
         } catch (Exception e) {
             LOGGER.log(Level.FINE, "Errors occurred trying to gather workspace of resource "
-                    + resource.getName());
+                    + resourceName);
             // it's a layer whose resource we don't know about
             return true;
         }
 
-        SecureTreeNode node = root.getDeepestNode(new String[] { workspace, resource.getName() });
-        if(!node.canAccess(user, mode)) {
-            return false;
-        }
-
-        // check if all containing layer groups stop access to the layer 
-        if(!layerGroupContainmentCheckRequired()) {
-            return true;
+        // if we have a catalog rule that is at resource level, it's the most specific type,
+        // it wins. As an alternative, it could be that we do not need to check layer groups at all
+        SecureTreeNode catalogNode = root.getDeepestNode(new String[] { workspace, resourceName });
+        int catalogNodeDepth = catalogNode.getDepth();
+        final boolean catalogNodeAllowsAccess = catalogNode.canAccess(user, mode);
+        if(catalogNodeDepth == SecureTreeNode.RESOURCE_DEPTH || !layerGroupContainmentCheckRequired()) {
+            return catalogNodeAllowsAccess;
         }
 
-        // check if there is any containing group that has security rules
+        // check if there is any containing group that has security rules, if not, the deepest
+        // catalog rule wins
         List<SecureTreeNode> nodes = getNodesContainingResource(resource);
         if(nodes.isEmpty()) {
-            return true;
+            return catalogNodeAllowsAccess;
         }
 
         // if we have at least one containing tree that is secured and authorizes access, then 
         // we have a winner
-        if(nodes.stream().anyMatch(n -> n.canAccess(user, mode))) {
+        if(nodes.stream().anyMatch(n -> n.canAccess(user, mode) && n.getDepth() > catalogNodeDepth)) {
             return true;
         }
 
@@ -221,13 +220,19 @@ public boolean canAccess(Authentication user, ResourceInfo resource, AccessMode
         Filter notSingle = Predicates.notEqual("mode", LayerGroupInfo.Mode.SINGLE);
         Filter global = Predicates.isNull("workspace");
         Filter inSameWorkspace = Predicates.equal("workspace.name", workspace);
+        // add a check on the depth here
         Filter wsMatch = Predicates.or(global, inSameWorkspace);
         Filter groupFilter = Predicates.and(wsMatch, notSingle, containsResource);
-        if(hasContainerGroupNotDenied(nodes, groupFilter)) {
+        if(hasContainerGroupNotDenied(user, nodes, groupFilter, catalogNodeDepth)) {
             return true;
         }
-        // could not find an authorized layer group that will allow authorization, so not authorized
-        return false;
+        // ok, no overrides, see if there was a more specific lg node denying access, otherwise go
+        // for the catalog one
+        if(nodes.stream().anyMatch(n -> !n.canAccess(user, mode) && n.getDepth() > catalogNodeDepth)) {
+            return false;
+        } else { 
+            return catalogNodeAllowsAccess;
+        }
     }
 
     private List<SecureTreeNode> getNodesContainingResource(ResourceInfo resource) {
@@ -474,28 +479,39 @@ public LayerGroupAccessLimits getAccessLimits(Authentication user, LayerGroupInf
             wsMatch = global;
         }
         Filter groupFilter = Predicates.and(wsMatch, notSingle, containsResource);
-        if(hasContainerGroupNotDenied(nodes, groupFilter)) {
+        if(hasContainerGroupNotDenied(user, nodes, groupFilter, node.getDepth())) {
             return null; // allow access
         }
         // could not find an authorized layer group that will allow authorization, so not authorized
         return new LayerGroupAccessLimits(getMode());
     }
 
-    private boolean hasContainerGroupNotDenied(List<SecureTreeNode> nodes, Filter groupFilter) {
+    private boolean hasContainerGroupNotDenied(Authentication user, List<SecureTreeNode> nodes, Filter groupFilter, int minimumDepth) {
+        if(minimumDepth >= SecureTreeNode.RESOURCE_DEPTH) {
+            return false;
+        }
         try(CloseableIterator<LayerGroupInfo> it = rawCatalog.list(LayerGroupInfo.class, groupFilter)) {
             while(it.hasNext()) {
                 LayerGroupInfo lg = it.next();
                 // if we have a node for this group it has already been considered
                 if(getNodeForGroup(lg) != null) {
                     continue;
                 }
+                // the layer group must be specific enough to override the catalog rule
+                if(minimumDepth >= 1 && lg.getWorkspace() == null) {
+                    continue;
+                }
                 // need to check if it's nested in another group in the relevant lot first
                 if(nodes.stream().anyMatch(n -> {
                     final String id = lg.getId();
                     return n.getContainedCatalogIds().contains(id);
                 })) {
                     continue;
                 }
+                // check if the group itself is allowed (global or workspace rules might deny access to it)
+                if(getAccessLimits(user, lg) != null) {
+                    continue;
+                }
                 // ok, then we have a container group that is not denied, allow
                 return true;
             }

src/main/src/main/java/org/geoserver/security/impl/SecureTreeNode.java

@@ -5,8 +5,11 @@
  */
 package org.geoserver.security.impl;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -33,6 +36,21 @@ public class SecureTreeNode {
      * The role given to the administrators
      */
     static final String ROOT_ROLE = GeoServerRole.ADMIN_ROLE.getAuthority();
+
+    /**
+     * Depth or the security tree root
+     */
+    static int ROOT_DEPTH = 0;
+
+    /**
+     * Depth of a workspace/global group rule
+     */
+    static int WS_LG_DEPTH = 1;
+
+    /**
+     * Depth of a resource specific rule
+     */
+    static int RESOURCE_DEPTH = 2;
 
     Map<String, SecureTreeNode> children = new HashMap<String, SecureTreeNode>();
 
@@ -185,16 +203,46 @@ public void setAuthorizedRoles(AccessMode mode, Set<String> roles) {
      */
     public SecureTreeNode getDeepestNode(String... pathElements) {
         SecureTreeNode curr = this;
+        SecureTreeNode result = this;
         for (int i = 0; i < pathElements.length; i++) {
             final SecureTreeNode next = curr.getChild(pathElements[i]);
-            if (next == null)
-                return curr;
-            else
+            if (next == null) {
+                return result;  
+            } else {
                 curr = next;
+                // don't return info about a node that has no explicit 
+                // rule associated, the parent will do
+                if(curr.authorizedRoles != null && !curr.authorizedRoles.isEmpty()) {
+                    result = curr;
+                }
+            }
         }
         return curr;
     }
 
+    /**
+     * Returns all the layer group nodes containing the specified catalog id
+     * @param id
+     * @return
+     */
+    public List<SecureTreeNode> getLayerGroupNodesForCatalogId(String catalogId) {
+        List<SecureTreeNode> result = new ArrayList<>();
+        collectLayerGroupNodesForCatalogId(catalogId, result);
+
+        return result;
+    }
+
+    private void collectLayerGroupNodesForCatalogId(String catalogId, List<SecureTreeNode> result) {
+        // add this node if it contains the catalog id
+        if(containerLayerGroup && containedCatalogIds != null && containedCatalogIds.contains(catalogId)) {
+            result.add(this);
+        } 
+        // recurse
+        for (SecureTreeNode child : children.values()) {
+            collectLayerGroupNodesForCatalogId(catalogId, result);
+        }
+    }
+
     /**
      * Utility method that drills down from the current node using the specified
      * list of child names, and returns an element only if it fully matches the provided path
@@ -256,6 +304,22 @@ public String toString() {
                 + containerLayerGroup + ", authorizedRoles=" + authorizedRoles + ", containedCatalogIds="
                 + containedCatalogIds + "]";
     }
-
+
+    /**
+     * Returns the node depth, 0 is the root, 1 is a workspace/global layer one, 2 is layer specific
+     * @return
+     */
+    int getDepth() {
+        int depth = 0;
+        Set<SecureTreeNode> visited = new HashSet<>();
+        SecureTreeNode n = this;
+        while(n.parent != null && !visited.contains(n.parent)) {
+            depth++;
+            visited.add(n);
+            n = n.parent;
+        }
+
+        return depth;
+    }
 
 }