SEC-936: NPE in AbstractFallbackMethodDefinitionSource

http://jira.springframework.org/browse/SEC-936. Changed to check if the value of MethodInvocation.getThis() is null to prevent NPE. MapBasedMethodDefinitionSource now ignores calls to findAttributes() with a null target class (all its entries require a class) and the fallback option in AbstractFallbackMethodDefinitionSource is used if the targetClass is null (i.e. Method.getDeclaringClass() will be used as the Class)

core/src/main/java/org/springframework/security/intercept/method/AbstractFallbackMethodDefinitionSource.java

@@ -18,11 +18,11 @@ import org.springframework.util.ObjectUtils;
  * Abstract implementation of {@link MethodDefinitionSource} that supports both Spring AOP and AspectJ and
  * caches configuration attribute resolution from: 1. specific target method; 2. target class;  3. declaring method;
  * 4. declaring class/interface.
- * 
+ *
  * <p>
  * This class mimics the behaviour of Spring's AbstractFallbackTransactionAttributeSource class.
  * </p>
- * 
+ *
  * <p>
  * Note that this class cannot extract security metadata where that metadata is expressed by way of
  * a target method/class (ie #1 and #2 above) AND the target method/class is encapsulated in another
@@ -31,7 +31,7 @@ import org.springframework.util.ObjectUtils;
  * another proxy), move the metadata to declared methods or interfaces the proxy implements, or provide
  * your own replacement <tt>MethodDefinitionSource</tt>.
  * </p>
- * 
+ *
  * @author Ben Alex
  * @version $Id$
  * @since 2.0
@@ -39,15 +39,16 @@ import org.springframework.util.ObjectUtils;
 public abstract class AbstractFallbackMethodDefinitionSource implements MethodDefinitionSource {
 
     private static final Log logger = LogFactory.getLog(AbstractFallbackMethodDefinitionSource.class);
-	private final static Object NULL_CONFIG_ATTRIBUTE = new Object();
+    private final static Object NULL_CONFIG_ATTRIBUTE = new Object();
     private final Map attributeCache = new HashMap();
 
     public ConfigAttributeDefinition getAttributes(Object object) throws IllegalArgumentException {
         Assert.notNull(object, "Object cannot be null");
 
         if (object instanceof MethodInvocation) {
-        	MethodInvocation mi = (MethodInvocation) object;
-            return getAttributes(mi.getMethod(), mi.getThis().getClass());
+            MethodInvocation mi = (MethodInvocation) object;
+            Object target = mi.getThis();
+            return getAttributes(mi.getMethod(), target == null ? null : target.getClass());
         }
 
         if (object instanceof JoinPoint) {
@@ -59,143 +60,143 @@ public abstract class AbstractFallbackMethodDefinitionSource implements MethodDe
 
             Method method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types);
             Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'");
-            
+
             return getAttributes(method, targetClass);
         }
 
         throw new IllegalArgumentException("Object must be a MethodInvocation or JoinPoint");
     }
-    
+
     public final boolean supports(Class clazz) {
         return (MethodInvocation.class.isAssignableFrom(clazz) || JoinPoint.class.isAssignableFrom(clazz));
     }
-    
+
     public ConfigAttributeDefinition getAttributes(Method method, Class targetClass) {
-		// First, see if we have a cached value.
-		Object cacheKey = new DefaultCacheKey(method, targetClass);
-		synchronized (this.attributeCache) {
-			Object cached = this.attributeCache.get(cacheKey);
-			if (cached != null) {
-				// Value will either be canonical value indicating there is no config attribute,
-				// or an actual config attribute.
-				if (cached == NULL_CONFIG_ATTRIBUTE) {
-					return null;
-				}
-				else {
-					return (ConfigAttributeDefinition) cached;
-				}
-			}
-			else {
-				// We need to work it out.
-				ConfigAttributeDefinition cfgAtt = computeAttributes(method, targetClass);
-				// Put it in the cache.
-				if (cfgAtt == null) {
-					this.attributeCache.put(cacheKey, NULL_CONFIG_ATTRIBUTE);
-				}
-				else {
-					if (logger.isDebugEnabled()) {
-						logger.debug("Adding security method [" + cacheKey + "] with attribute [" + cfgAtt + "]");
-					}
-					this.attributeCache.put(cacheKey, cfgAtt);
-				}
-				return cfgAtt;
-			}
-		}
+        // First, see if we have a cached value.
+        Object cacheKey = new DefaultCacheKey(method, targetClass);
+        synchronized (this.attributeCache) {
+            Object cached = this.attributeCache.get(cacheKey);
+            if (cached != null) {
+                // Value will either be canonical value indicating there is no config attribute,
+                // or an actual config attribute.
+                if (cached == NULL_CONFIG_ATTRIBUTE) {
+                    return null;
+                }
+                else {
+                    return (ConfigAttributeDefinition) cached;
+                }
+            }
+            else {
+                // We need to work it out.
+                ConfigAttributeDefinition cfgAtt = computeAttributes(method, targetClass);
+                // Put it in the cache.
+                if (cfgAtt == null) {
+                    this.attributeCache.put(cacheKey, NULL_CONFIG_ATTRIBUTE);
+                }
+                else {
+                    if (logger.isDebugEnabled()) {
+                        logger.debug("Adding security method [" + cacheKey + "] with attribute [" + cfgAtt + "]");
+                    }
+                    this.attributeCache.put(cacheKey, cfgAtt);
+                }
+                return cfgAtt;
+            }
+        }
     }
-    
+
     /**
-     * 
-	 * @param method the method for the current invocation (never <code>null</code>)
-	 * @param targetClass the target class for this invocation (may be <code>null</code>)
+     *
+     * @param method the method for the current invocation (never <code>null</code>)
+     * @param targetClass the target class for this invocation (may be <code>null</code>)
      * @return
      */
     private ConfigAttributeDefinition computeAttributes(Method method, Class targetClass) {
-    	// The method may be on an interface, but we need attributes from the target class.
-		// If the target class is null, the method will be unchanged.
-		Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass);
-		// First try is the method in the target class.
-		ConfigAttributeDefinition attr = findAttributes(specificMethod, targetClass);
-		if (attr != null) {
-			return attr;
-		}
-
-		// Second try is the config attribute on the target class.
-		attr = findAttributes(specificMethod.getDeclaringClass());
-		if (attr != null) {
-			return attr;
-		}
-
-		if (specificMethod != method) {
-			// Fallback is to look at the original method.
-			attr = findAttributes(method, method.getDeclaringClass());
-			if (attr != null) {
-				return attr;
-			}
-			// Last fallback is the class of the original method.
-			return findAttributes(method.getDeclaringClass());
-		}
-		return null;
+        // The method may be on an interface, but we need attributes from the target class.
+        // If the target class is null, the method will be unchanged.
+        Method specificMethod = ClassUtils.getMostSpecificMethod(method, targetClass);
+        // First try is the method in the target class.
+        ConfigAttributeDefinition attr = findAttributes(specificMethod, targetClass);
+        if (attr != null) {
+            return attr;
+        }
+
+        // Second try is the config attribute on the target class.
+        attr = findAttributes(specificMethod.getDeclaringClass());
+        if (attr != null) {
+            return attr;
+        }
+
+        if (specificMethod != method || targetClass == null) {
+            // Fallback is to look at the original method.
+            attr = findAttributes(method, method.getDeclaringClass());
+            if (attr != null) {
+                return attr;
+            }
+            // Last fallback is the class of the original method.
+            return findAttributes(method.getDeclaringClass());
+        }
+        return null;
 
     }
-    
+
     /**
      * Obtains the security metadata applicable to the specified method invocation.
-     * 
+     *
      * <p>
      * Note that the {@link Method#getDeclaringClass()} may not equal the <code>targetClass</code>.
      * Both parameters are provided to assist subclasses which may wish to provide advanced
      * capabilities related to method metadata being "registered" against a method even if the
      * target class does not declare the method (i.e. the subclass may only inherit the method).
-     * 
+     *
      * @param method the method for the current invocation (never <code>null</code>)
      * @param targetClass the target class for the invocation (may be <code>null</code>)
      * @return the security metadata (or null if no metadata applies)
      */
     protected abstract ConfigAttributeDefinition findAttributes(Method method, Class targetClass);
-    
+
     /**
      * Obtains the security metadata registered against the specified class.
-     * 
+     *
      * <p>
      * Subclasses should only return metadata expressed at a class level. Subclasses should NOT
      * aggregate metadata for each method registered against a class, as the abstract superclass
      * will separate invoke {@link #findAttributes(Method, Class)} for individual methods as
-     * appropriate. 
-     * 
+     * appropriate.
+     *
      * @param clazz the target class for the invocation (never <code>null</code>)
      * @return the security metadata (or null if no metadata applies)
      */
     protected abstract ConfigAttributeDefinition findAttributes(Class clazz);
-    
-	private static class DefaultCacheKey {
-
-		private final Method method;
-		private final Class targetClass;
-
-		public DefaultCacheKey(Method method, Class targetClass) {
-			this.method = method;
-			this.targetClass = targetClass;
-		}
-
-		public boolean equals(Object other) {
-			if (this == other) {
-				return true;
-			}
-			if (!(other instanceof DefaultCacheKey)) {
-				return false;
-			}
-			DefaultCacheKey otherKey = (DefaultCacheKey) other;
-			return (this.method.equals(otherKey.method) &&
-					ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
-		}
-
-		public int hashCode() {
-			return this.method.hashCode() * 21 + (this.targetClass != null ? this.targetClass.hashCode() : 0);
-		}
-
-		public String toString() {
-			return "CacheKey[" + (targetClass == null ? "-" : targetClass.getName()) + "; " + method + "]";
-		}
-	}
-    
+
+    private static class DefaultCacheKey {
+
+        private final Method method;
+        private final Class targetClass;
+
+        public DefaultCacheKey(Method method, Class targetClass) {
+            this.method = method;
+            this.targetClass = targetClass;
+        }
+
+        public boolean equals(Object other) {
+            if (this == other) {
+                return true;
+            }
+            if (!(other instanceof DefaultCacheKey)) {
+                return false;
+            }
+            DefaultCacheKey otherKey = (DefaultCacheKey) other;
+            return (this.method.equals(otherKey.method) &&
+                    ObjectUtils.nullSafeEquals(this.targetClass, otherKey.targetClass));
+        }
+
+        public int hashCode() {
+            return this.method.hashCode() * 21 + (this.targetClass != null ? this.targetClass.hashCode() : 0);
+        }
+
+        public String toString() {
+            return "CacheKey[" + (targetClass == null ? "-" : targetClass.getName()) + "; " + method + "]";
+        }
+    }
+
 }

core/src/main/java/org/springframework/security/intercept/method/MapBasedMethodDefinitionSource.java

@@ -88,6 +88,10 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
      * Will walk the method inheritance tree to find the most specific declaration applicable.
      */
     protected ConfigAttributeDefinition findAttributes(Method method, Class targetClass) {
+        if (targetClass == null) {
+            return null;
+        }
+
         return findAttributesSpecifiedAgainst(method, targetClass);
     }
 
@@ -179,9 +183,9 @@ public class MapBasedMethodDefinitionSource extends AbstractFallbackMethodDefini
 
     /**
      * Adds configuration attributes for a specific method, for example where the method has been
-     * matched using a pointcut expression. If a match already exists in the map for the method, then 
-     * the existing match will be retained, so that if this method is called for a more general pointcut 
-     * it will not override a more specific one which has already been added. This  
+     * matched using a pointcut expression. If a match already exists in the map for the method, then
+     * the existing match will be retained, so that if this method is called for a more general pointcut
+     * it will not override a more specific one which has already been added. This
      */
     public void addSecureMethod(Class javaType, Method method, ConfigAttributeDefinition attr) {
         RegisteredMethod key = new RegisteredMethod(method, javaType);