[SUITE 2560] Prevent StackOverflowError for SecureCatalog

src/main/src/main/java/org/geoserver/security/SecureCatalogImpl.java

@@ -35,9 +35,7 @@
 import org.geoserver.catalog.StoreInfo;
 import org.geoserver.catalog.StyleInfo;
 import org.geoserver.catalog.ValidationResult;
-import org.geoserver.catalog.WMSLayerInfo;
 import org.geoserver.catalog.WMSStoreInfo;
-import org.geoserver.catalog.WMTSLayerInfo;
 import org.geoserver.catalog.WMTSStoreInfo;
 import org.geoserver.catalog.WorkspaceInfo;
 import org.geoserver.catalog.event.CatalogListener;
@@ -48,11 +46,11 @@
 import org.geoserver.platform.GeoServerResourceLoader;
 import org.geoserver.security.decorators.DecoratingCatalogFactory;
 import org.geoserver.security.decorators.SecuredCoverageInfo;
-import org.geoserver.security.decorators.SecuredCoverageStoreInfo;
 import org.geoserver.security.decorators.SecuredDataStoreInfo;
 import org.geoserver.security.decorators.SecuredFeatureTypeInfo;
 import org.geoserver.security.decorators.SecuredLayerGroupInfo;
 import org.geoserver.security.decorators.SecuredLayerInfo;
+import org.geoserver.security.decorators.SecuredObjects;
 import org.geoserver.security.decorators.SecuredWMSLayerInfo;
 import org.geoserver.security.decorators.SecuredWMTSLayerInfo;
 import org.geoserver.security.impl.DataAccessRuleDAO;
@@ -69,6 +67,10 @@
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 
+import org.geoserver.security.decorators.SecuredCoverageStoreInfo;
+import org.geoserver.security.decorators.SecuredWMSStoreInfo;
+import org.geoserver.security.decorators.SecuredWMTSStoreInfo;
+
 /**
  * Wraps the catalog and applies the security directives provided by a {@link ResourceAccessManager}
  * or a {@link DataAccessManager} registered in the Spring application context
@@ -532,17 +534,7 @@ else if(policy.level == AccessLevel.READ_WRITE && policy.getLimits() == null)
 
         // otherwise we are in a mixed case where the user can read but not write, or
         // cannot read but is allowed by the operation mode to access the metadata
-        if(info instanceof FeatureTypeInfo) { 
-            return (T) new SecuredFeatureTypeInfo((FeatureTypeInfo) info, policy);
-        } else if(info instanceof CoverageInfo) {
-            return (T) new SecuredCoverageInfo((CoverageInfo) info, policy);
-        } else if(info instanceof WMSLayerInfo) {
-            return (T) new SecuredWMSLayerInfo((WMSLayerInfo) info, policy);
-        } else if(info instanceof WMTSLayerInfo) {
-            return (T) new SecuredWMTSLayerInfo((WMTSLayerInfo) info, policy);
-        } else {
-            throw new RuntimeException("Unknown resource type " + info.getClass());
-        }
+        return (T) SecuredObjects.secure(info, policy);
    }
 
     /**
@@ -586,16 +578,9 @@ else if(policy.level == AccessLevel.READ_WRITE ||
         // write, or
         // cannot read but is allowed by the operation mode to access the
         // metadata
-        if (store instanceof DataStoreInfo) {
-            return (T) new SecuredDataStoreInfo((DataStoreInfo) store, policy);
-        } else if(store instanceof CoverageStoreInfo) {
-            return (T) new SecuredCoverageStoreInfo((CoverageStoreInfo) store, policy);
-        } else if (store instanceof WMSStoreInfo) {
-            // TODO: implement WMSStoreInfo wrappring if necessary
-            return store;
-        } else if (store instanceof WMTSStoreInfo) {
-            // TODO: implement WMTSStoreInfo wrappring if necessary
-            return store;
+        if (store instanceof DataStoreInfo || store instanceof CoverageStoreInfo || store instanceof WMSStoreInfo ||
+            store instanceof WMTSStoreInfo) {
+            return (T) SecuredObjects.secure(store, policy);
         } else {
             throw new RuntimeException("Unknown store type " + store.getClass());
         }
@@ -621,7 +606,7 @@ else if(policy.level == AccessLevel.READ_WRITE && policy.getLimits() == null)
 
         // otherwise we are in a mixed case where the user can read but not write, or
         // cannot read but is allowed by the operation mode to access the metadata
-        return new SecuredLayerInfo(layer, policy);
+        return (LayerInfo) SecuredObjects.secure(layer, policy);
     }
 
     /**
@@ -1122,6 +1107,12 @@ static ResourceInfo unwrap(ResourceInfo info) {
     static StoreInfo unwrap(StoreInfo info) {
         if(info instanceof SecuredDataStoreInfo)
             return ((SecuredDataStoreInfo) info).unwrap(StoreInfo.class);
+        if (info instanceof SecuredCoverageStoreInfo)
+            return ((SecuredCoverageStoreInfo) info).unwrap(StoreInfo.class);
+        if (info instanceof SecuredWMSStoreInfo)
+            return ((SecuredWMSStoreInfo) info).unwrap(StoreInfo.class);
+        if (info instanceof SecuredWMTSStoreInfo)
+            return ((SecuredWMTSStoreInfo) info).unwrap(StoreInfo.class);
         return info;
     }
 

src/main/src/main/java/org/geoserver/security/decorators/DefaultSecureCatalogFactory.java

@@ -5,13 +5,19 @@
  */
 package org.geoserver.security.decorators;
 
+import java.util.logging.Logger;
+
 import org.geoserver.catalog.CoverageInfo;
 import org.geoserver.catalog.CoverageStoreInfo;
 import org.geoserver.catalog.DataStoreInfo;
 import org.geoserver.catalog.FeatureTypeInfo;
 import org.geoserver.catalog.LayerInfo;
+import org.geoserver.catalog.WMSLayerInfo;
+import org.geoserver.catalog.WMTSLayerInfo;
+import org.geoserver.catalog.impl.ModificationProxy;
 import org.geoserver.platform.ExtensionPriority;
 import org.geoserver.security.WrapperPolicy;
+import org.geotools.util.logging.Logging;
 
 /**
  * Creates security wrappers for the most common catalog objects
@@ -21,6 +27,9 @@
  */
 public class DefaultSecureCatalogFactory implements SecuredObjectFactory {
 
+    private static final Logger LOGGER = Logging.getLogger(DefaultSecureCatalogFactory.class);
+
+    @Override
     public boolean canSecure(Class clazz) {
         return CoverageInfo.class.isAssignableFrom(clazz)
                 || CoverageStoreInfo.class.isAssignableFrom(clazz)
@@ -29,32 +38,157 @@ public boolean canSecure(Class clazz) {
                 || LayerInfo.class.isAssignableFrom(clazz);
     }
 
+    @Override
     public Object secure(Object object, WrapperPolicy policy) {
         // null safe
         if (object == null)
             return null;
 
         Class clazz = object.getClass();
+        // for each supported Info type, log a warning if the object to be secured is already secured. If this happens,
+        // it could lead to a StackOverflowError if the object is re-wrapped, over time, over and over agian.
         if (CoverageInfo.class.isAssignableFrom(clazz))
-            return new SecuredCoverageInfo((CoverageInfo) object, policy);
+            return new SecuredCoverageInfo(logIfSecured((CoverageInfo) object), policy);
         else if (CoverageStoreInfo.class.isAssignableFrom(clazz))
-            return new SecuredCoverageStoreInfo((CoverageStoreInfo) object, policy);
+            return new SecuredCoverageStoreInfo(logIfSecured((CoverageStoreInfo) object), policy);
         else if (DataStoreInfo.class.isAssignableFrom(clazz))
-            return new SecuredDataStoreInfo((DataStoreInfo) object, policy);
+            return new SecuredDataStoreInfo(logIfSecured((DataStoreInfo) object), policy);
         else if (FeatureTypeInfo.class.isAssignableFrom(clazz))
-            return new SecuredFeatureTypeInfo((FeatureTypeInfo) object, policy);
+            return new SecuredFeatureTypeInfo(logIfSecured((FeatureTypeInfo) object), policy);
         else if (LayerInfo.class.isAssignableFrom(clazz))
-            return new SecuredLayerInfo((LayerInfo) object, policy);
+            return new SecuredLayerInfo(logIfSecured((LayerInfo) object), policy);
+        else if (WMSLayerInfo.class.isAssignableFrom(clazz))
+            return new SecuredWMSLayerInfo(logIfSecured((WMSLayerInfo) object), policy);
+        else if (WMTSLayerInfo.class.isAssignableFrom(clazz))
+            return new SecuredWMTSLayerInfo(logIfSecured((WMTSLayerInfo) object), policy);
         else
-            throw new IllegalArgumentException("Don't know how to wrap");
+            throw new IllegalArgumentException("Don't know how to wrap " + object);
     }
-
     /**
      * Returns {@link ExtensionPriority#LOWEST} since the wrappers generated by
      * this factory
+     * @return {@link ExtensionPriority#LOWEST}
      */
+    @Override
     public int getPriority() {
         return ExtensionPriority.LOWEST;
     }
 
+    private void logDoubleWrap(Object unwrapped, Object orig) {
+        String msg = String.format("Tried to double secure: %s already securing %s", orig, unwrapped);
+        LOGGER.warning(msg);
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link WMTSLayerInfo} to check.
+     * @return The original object to be checked.
+     */
+    private WMTSLayerInfo logIfSecured(WMTSLayerInfo object) {
+        WMTSLayerInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredWMTSLayerInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link WMSLayerInfo} to check.
+     * @return The original object to be checked.
+     */
+    private WMSLayerInfo logIfSecured(WMSLayerInfo object) {
+        WMSLayerInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredWMSLayerInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link LayerInfo} to check.
+     * @return The original object to be checked.
+     */
+    private LayerInfo logIfSecured(LayerInfo object) {
+        LayerInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredLayerInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link FeatureTypeInfo} to check.
+     * @return The original object to be checked.
+     */
+    private FeatureTypeInfo logIfSecured(FeatureTypeInfo object) {
+        FeatureTypeInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredFeatureTypeInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link CoverageStoreInfo} to check.
+     * @return The original object to be checked.
+     */
+    private CoverageStoreInfo logIfSecured(CoverageStoreInfo object) {
+        CoverageStoreInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredCoverageStoreInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link CoverageInfo} to check.
+     * @return The original object to be checked.
+     */
+    private CoverageInfo logIfSecured(CoverageInfo object) {
+        CoverageInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredDataStoreInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
+    /**
+     * Generates a warning log if the Info object is already wrapped with a Secured decorator. This method is only
+     * intended to log a situation where a Catalog Info object is being secured, but is already secured. Repeated calls
+     * to this will keep adding additional wrapper layers and may eventually cause a StackOverflowError. The log
+     * generated is merely to aid in finding the real issue, as opposed to masking it here.
+     * @param object {@link DataStoreInfo} to check.
+     * @return The original object to be checked.
+     */
+    private DataStoreInfo logIfSecured(DataStoreInfo object) {
+        DataStoreInfo unwrapped = ModificationProxy.unwrap(object);
+        if(unwrapped instanceof SecuredDataStoreInfo) {
+            logDoubleWrap(unwrapped, object);
+        }
+        return object;
+    }
+
 }

src/main/src/main/java/org/geoserver/security/decorators/SecuredCoverageInfo.java

@@ -9,6 +9,7 @@
 
 import org.geoserver.catalog.CoverageInfo;
 import org.geoserver.catalog.CoverageStoreInfo;
+import org.geoserver.catalog.StoreInfo;
 import org.geoserver.ows.Dispatcher;
 import org.geoserver.ows.Request;
 import org.geoserver.security.AccessLevel;
@@ -65,7 +66,12 @@ public GridCoverageReader getGridCoverageReader(ProgressListener listener,
 
     @Override
     public CoverageStoreInfo getStore() {
-        return new SecuredCoverageStoreInfo(super.getStore(), policy);
+        return (CoverageStoreInfo) SecuredObjects.secure(super.getStore(), policy);
     }
 
+    @Override
+    public void setStore(StoreInfo store) {
+        // need to make sure the store isn't secured
+        super.setStore((StoreInfo)SecureCatalogImpl.unwrap(store));
+    }
 }

src/main/src/main/java/org/geoserver/security/decorators/SecuredFeatureTypeInfo.java

@@ -17,6 +17,7 @@
 
 import org.geoserver.catalog.DataStoreInfo;
 import org.geoserver.catalog.FeatureTypeInfo;
+import org.geoserver.catalog.StoreInfo;
 import org.geoserver.security.AccessLevel;
 import org.geoserver.security.SecureCatalogImpl;
 import org.geoserver.security.VectorAccessLimits;
@@ -97,6 +98,7 @@ public FeatureType getFeatureType() throws IOException {
     // WRAPPED METHODS TO ENFORCE SECURITY POLICY
     //--------------------------------------------------------------------------
 
+    @Override
     public FeatureSource getFeatureSource(ProgressListener listener, Hints hints)
             throws IOException {
         final FeatureSource fs = delegate.getFeatureSource(listener, hints);
@@ -108,8 +110,15 @@ public FeatureSource getFeatureSource(ProgressListener listener, Hints hints)
         }
     }
 
+    @Override
     public DataStoreInfo getStore() {
         return (DataStoreInfo) SecuredObjects.secure(delegate.getStore(), policy);
     }
-
+
+    @Override
+    public void setStore(StoreInfo store) {
+        // need to make sure the store isn't secured
+        super.setStore((StoreInfo)SecureCatalogImpl.unwrap(store));
+    }
+
 }

src/main/src/main/java/org/geoserver/security/decorators/SecuredLayerInfo.java

@@ -33,13 +33,13 @@ public ResourceInfo getResource() {
         if (r == null)
             return null;
         else if (r instanceof FeatureTypeInfo)
-            return new SecuredFeatureTypeInfo((FeatureTypeInfo) r, policy);
+            return (FeatureTypeInfo) SecuredObjects.secure(r, policy);
         else if (r instanceof CoverageInfo)
-            return new SecuredCoverageInfo((CoverageInfo) r, policy);
+            return (CoverageInfo) SecuredObjects.secure(r, policy);
         else if (r instanceof WMSLayerInfo)
-            return new SecuredWMSLayerInfo((WMSLayerInfo) r, policy);
+            return (WMSLayerInfo) SecuredObjects.secure(r, policy);
         else if (r instanceof WMTSLayerInfo)
-            return new SecuredWMTSLayerInfo((WMTSLayerInfo) r, policy);
+            return (WMTSLayerInfo) SecuredObjects.secure(r, policy);
         else
             throw new RuntimeException("Don't know how to make resource of type " + r.getClass());
     }

src/main/src/main/java/org/geoserver/security/decorators/SecuredObjects.java

@@ -47,7 +47,7 @@ public static Object secure(Object object, WrapperPolicy policy) {
 
         // if we already know what can handle the wrapping, just do it, don't
         // scan the extension points once more
-        Class clazz = object.getClass();
+        Class<?> clazz = object.getClass();
         SecuredObjectFactory candidate = FACTORY_CACHE.get(clazz);
 
         // otherwise scan and store (or complain)

src/main/src/main/java/org/geoserver/security/decorators/SecuredWMSLayerInfo.java

@@ -5,10 +5,13 @@
  */
 package org.geoserver.security.decorators;
 
+import org.geoserver.catalog.StoreInfo;
+
 import java.io.IOException;
 
 import org.geoserver.catalog.WMSLayerInfo;
 import org.geoserver.catalog.WMSStoreInfo;
+import org.geoserver.security.SecureCatalogImpl;
 import org.geoserver.security.WrapperPolicy;
 import org.geotools.data.ows.Layer;
 import org.opengis.util.ProgressListener;
@@ -41,4 +44,10 @@ public Layer getWMSLayer(ProgressListener listener) throws IOException {
     public WMSStoreInfo getStore() {
         return new SecuredWMSStoreInfo(delegate.getStore(), policy);
     }
+
+    @Override
+    public void setStore(StoreInfo store) {
+        // need to make sure the store isn't secured
+        super.setStore((StoreInfo)SecureCatalogImpl.unwrap(store));
+    }
 }