SEC-2136: Lazy load MethodSecurityExpressionHandler & MethodSecurityExpressionHandler.expressionParser

Previously wiring dependencies created with a FactoryBean into
MethodSecurityExpressionHandler &
MethodSecurityExpressionHandler.expressionParser and  would cause
NoSuchBeanDefinitionException's to occur. These changes make it easier
(but not impossible) to avoid such errors.

The following changes were made:

    - ExpressionBasedAnnotationAttributeFactory delays the invocation of
      MethodSecurityExpressionHandler.getExpressionParser()
    - MethodSecurityExpressionHandler is automatically wrapped in a
      LazyInitTargetSource and marked as lazyInit=true

config/config.gradle

@@ -33,9 +33,12 @@ dependencies {
                 "org.springframework.ldap:spring-ldap-core:$springLdapVersion",
                 "org.springframework:spring-expression:$springVersion",
                 "org.springframework:spring-jdbc:$springVersion",
+                "org.springframework:spring-orm:$springVersion",
                 "org.springframework:spring-tx:$springVersion",
                 'org.spockframework:spock-core:0.6-groovy-1.8',
                 "org.slf4j:jcl-over-slf4j:$slf4jVersion",
+                "org.hibernate.javax.persistence:hibernate-jpa-2.0-api:1.0.1.Final",
+                "org.hibernate:hibernate-entitymanager:4.1.0.Final",
                 powerMockDependencies
     testCompile('org.openid4java:openid4java-nodeps:0.9.6') {
        exclude group: 'com.google.code.guice', module: 'guice'

config/src/main/java/org/springframework/security/config/method/GlobalMethodSecurityBeanDefinitionParser.java

@@ -10,6 +10,8 @@ import java.util.Map;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.aop.config.AopNamespaceUtils;
+import org.springframework.aop.framework.ProxyFactoryBean;
+import org.springframework.aop.target.LazyInitTargetSource;
 import org.springframework.beans.BeanMetadataElement;
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.BeanFactory;
@@ -17,15 +19,19 @@ import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanReference;
+import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.beans.factory.config.RuntimeBeanReference;
 import org.springframework.beans.factory.parsing.BeanComponentDefinition;
 import org.springframework.beans.factory.parsing.CompositeComponentDefinition;
 import org.springframework.beans.factory.support.BeanDefinitionBuilder;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor;
 import org.springframework.beans.factory.support.ManagedList;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.beans.factory.xml.BeanDefinitionParser;
 import org.springframework.beans.factory.xml.ParserContext;
 import org.springframework.security.access.ConfigAttribute;
+import org.springframework.security.access.PermissionEvaluator;
 import org.springframework.security.access.SecurityConfig;
 import org.springframework.security.access.annotation.Jsr250MethodSecurityMetadataSource;
 import org.springframework.security.access.annotation.Jsr250Voter;
@@ -34,6 +40,7 @@ import org.springframework.security.access.expression.method.DefaultMethodSecuri
 import org.springframework.security.access.expression.method.ExpressionBasedAnnotationAttributeFactory;
 import org.springframework.security.access.expression.method.ExpressionBasedPostInvocationAdvice;
 import org.springframework.security.access.expression.method.ExpressionBasedPreInvocationAdvice;
+import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler;
 import org.springframework.security.access.intercept.AfterInvocationProviderManager;
 import org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor;
 import org.springframework.security.access.intercept.aopalliance.MethodSecurityMetadataSourceAdvisor;
@@ -139,6 +146,20 @@ public class GlobalMethodSecurityBeanDefinitionParser implements BeanDefinitionP
 
                 if (StringUtils.hasText(expressionHandlerRef)) {
                     logger.info("Using bean '" + expressionHandlerRef + "' as method ExpressionHandler implementation");
+                    RootBeanDefinition lazyInitPP = new RootBeanDefinition(LazyInitBeanDefinitionRegistryPostProcessor.class);
+                    lazyInitPP.getConstructorArgumentValues().addGenericArgumentValue(expressionHandlerRef);
+                    pc.getReaderContext().registerWithGeneratedName(lazyInitPP);
+
+                    BeanDefinitionBuilder lazyMethodSecurityExpressionHandlerBldr = BeanDefinitionBuilder.rootBeanDefinition(LazyInitTargetSource.class);
+                    lazyMethodSecurityExpressionHandlerBldr.addPropertyValue("targetBeanName", expressionHandlerRef);
+
+                    BeanDefinitionBuilder expressionHandlerProxyBldr = BeanDefinitionBuilder.rootBeanDefinition(ProxyFactoryBean.class);
+                    expressionHandlerProxyBldr.addPropertyValue("targetSource", lazyMethodSecurityExpressionHandlerBldr.getBeanDefinition());
+                    expressionHandlerProxyBldr.addPropertyValue("proxyInterfaces", MethodSecurityExpressionHandler.class);
+
+                    expressionHandlerRef = pc.getReaderContext().generateBeanName(expressionHandlerProxyBldr.getBeanDefinition());
+
+                    pc.registerBeanComponent(new BeanComponentDefinition(expressionHandlerProxyBldr.getBeanDefinition(), expressionHandlerRef));
                 } else {
                     BeanDefinition expressionHandler = new RootBeanDefinition(DefaultMethodSecurityExpressionHandler.class);
                     expressionHandlerRef = pc.getReaderContext().generateBeanName(expressionHandler);
@@ -401,4 +422,26 @@ public class GlobalMethodSecurityBeanDefinitionParser implements BeanDefinitionP
             this.beanFactory = beanFactory;
         }
     }
+
+    /**
+     * Delays setting a bean of a given name to be lazyily initialized until after all the beans are registered.
+     *
+     * @author Rob Winch
+     * @since 3.2
+     */
+    private static final class LazyInitBeanDefinitionRegistryPostProcessor implements BeanDefinitionRegistryPostProcessor {
+        private final String beanName;
+
+        private LazyInitBeanDefinitionRegistryPostProcessor(String beanName) {
+            this.beanName = beanName;
+        }
+
+        public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException {
+            BeanDefinition beanDefinition = registry.getBeanDefinition(beanName);
+            beanDefinition.setLazyInit(true);
+        }
+
+        public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException {
+        }
+    }
 }

config/src/test/java/org/springframework/security/config/method/sec2136/JpaPermissionEvaluator.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.security.config.method.sec2136;
+
+import java.io.Serializable;
+
+import javax.persistence.EntityManager;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.security.access.PermissionEvaluator;
+import org.springframework.security.core.Authentication;
+
+/**
+ *
+ * @author Rob Winch
+ *
+ */
+public class JpaPermissionEvaluator implements PermissionEvaluator {
+    @Autowired
+    private EntityManager entityManager;
+
+    public JpaPermissionEvaluator() {
+        System.out.println("initializing "+this);
+    }
+
+    public boolean hasPermission(Authentication authentication,
+            Object targetDomainObject, Object permission) {
+        return true;
+    }
+
+    public boolean hasPermission(Authentication authentication,
+            Serializable targetId, String targetType, Object permission) {
+        return true;
+    }
+}

config/src/test/java/org/springframework/security/config/method/sec2136/Sec2136Tests.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.springframework.security.config.method.sec2136;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+/**
+ *
+ * @author Rob Winch
+ * @since 3.2
+ */
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration("sec2136.xml")
+public class Sec2136Tests {
+
+    @Test
+    public void configurationLoads() {
+
+    }
+}

config/src/test/resources/org/springframework/security/config/method/sec2136/sec2136.xml

@@ -0,0 +1,55 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns:jpa="http://www.springframework.org/schema/data/jpa"
+    xmlns:context="http://www.springframework.org/schema/context"
+    xmlns:jdbc="http://www.springframework.org/schema/jdbc"
+    xmlns:security="http://www.springframework.org/schema/security"
+    xsi:schemaLocation="http://www.springframework.org/schema/jdbc http://www.springframework.org/schema/jdbc/spring-jdbc-3.1.xsd
+        http://www.springframework.org/schema/security http://www.springframework.org/schema/security/spring-security-3.1.xsd
+        http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-3.1.xsd
+        http://www.springframework.org/schema/data/jpa http://www.springframework.org/schema/data/jpa/spring-jpa.xsd
+        http://www.springframework.org/schema/context http://www.springframework.org/schema/context/spring-context-3.1.xsd">
+
+    <context:annotation-config />
+
+    <bean id="entityManagerFactory"
+          class="org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean">
+        <property name="dataSource" ref="dataSource" />
+        <property name="packagesToScan" value="entity" />
+        <property name="jpaVendorAdapter">
+            <bean class="org.springframework.orm.jpa.vendor.HibernateJpaVendorAdapter">
+                <property name="showSql" value="true" />
+                <property name="generateDdl" value="false" />
+                <property name="databasePlatform" value="org.hibernate.dialect.HSQLDialect" />
+            </bean>
+        </property>
+        <property name="jpaProperties">
+            <props>
+                <prop key="hibernate.hbm2ddl.auto">create-drop</prop>
+                <prop key="hibernate.id.new_generator_mappings">true</prop>
+                <prop key="hibernate.cache.use_second_level_cache">false</prop>
+            </props>
+        </property>
+    </bean>
+
+    <!-- DataSource Configuration -->
+    <jdbc:embedded-database id="dataSource"/>
+
+    <!-- Transaction Configuration -->
+    <bean id="transactionManager" class="org.springframework.orm.jpa.JpaTransactionManager">
+        <property name="entityManagerFactory" ref="entityManagerFactory" />
+    </bean>
+
+    <security:global-method-security pre-post-annotations="enabled">
+        <security:expression-handler ref="methodExpressionHandler"/>
+    </security:global-method-security>
+
+    <bean id="methodExpressionHandler"
+        class="org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler">
+        <property name="permissionEvaluator" ref="jpaPermEvaluator"/>
+    </bean>
+
+    <!-- Must be lazy init or inner bean to prevent eager loading of singletons -->
+    <bean id="jpaPermEvaluator" class="org.springframework.security.config.method.sec2136.JpaPermissionEvaluator" autowire="byType" lazy-init="true"/>
+</beans>

core/src/main/java/org/springframework/security/access/expression/method/ExpressionBasedAnnotationAttributeFactory.java

@@ -1,5 +1,17 @@
-/**
+/*
+ * Copyright 2002-2013 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
  *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
  */
 package org.springframework.security.access.expression.method;
 
@@ -15,18 +27,22 @@ import org.springframework.security.access.prepost.PrePostInvocationAttributeFac
  * an expression to be evaluated at runtime.
  *
  * @author Luke Taylor
+ * @author Rob Winch
  * @since 3.0
  */
 public class ExpressionBasedAnnotationAttributeFactory implements PrePostInvocationAttributeFactory {
-    private final ExpressionParser parser;
+    private final Object parserLock = new Object();
+    private ExpressionParser parser;
+    private MethodSecurityExpressionHandler handler;
 
     public ExpressionBasedAnnotationAttributeFactory(MethodSecurityExpressionHandler handler) {
-        parser = handler.getExpressionParser();
+        this.handler = handler;
     }
 
     public PreInvocationAttribute createPreInvocationAttribute(String preFilterAttribute, String filterObject, String preAuthorizeAttribute) {
         try {
-         // TODO: Optimization of permitAll
+            // TODO: Optimization of permitAll
+            ExpressionParser parser = getParser();
             Expression preAuthorizeExpression = preAuthorizeAttribute == null ? parser.parseExpression("permitAll") : parser.parseExpression(preAuthorizeAttribute);
             Expression preFilterExpression = preFilterAttribute == null ? null : parser.parseExpression(preFilterAttribute);
             return new PreInvocationExpressionAttribute(preFilterExpression, filterObject, preAuthorizeExpression);
@@ -37,6 +53,7 @@ public class ExpressionBasedAnnotationAttributeFactory implements PrePostInvocat
 
     public PostInvocationAttribute createPostInvocationAttribute(String postFilterAttribute, String postAuthorizeAttribute) {
         try {
+            ExpressionParser parser = getParser();
             Expression postAuthorizeExpression = postAuthorizeAttribute == null ? null : parser.parseExpression(postAuthorizeAttribute);
             Expression postFilterExpression = postFilterAttribute == null ? null : parser.parseExpression(postFilterAttribute);
 
@@ -49,4 +66,20 @@ public class ExpressionBasedAnnotationAttributeFactory implements PrePostInvocat
 
         return null;
     }
+
+    /**
+     * Delay the lookup of the {@link ExpressionParser} to prevent SEC-2136
+     *
+     * @return
+     */
+    private ExpressionParser getParser() {
+        if(this.parser != null) {
+            return this.parser;
+        }
+        synchronized(parserLock) {
+            this.parser = handler.getExpressionParser();
+            this.handler = null;
+        }
+        return this.parser;
+    }
 }