SEC-2595: @EnableGlobalMethodSecurity AspectJ fixes

build.gradle

@@ -62,7 +62,7 @@ ext.javaProjects = subprojects.findAll { project -> project.name != 'docs' && pr
 ext.sampleProjects = subprojects.findAll { project -> project.name.startsWith('spring-security-samples') }
 ext.itestProjects = subprojects.findAll { project -> project.name.startsWith('itest') }
 ext.coreModuleProjects = javaProjects - sampleProjects - itestProjects
-ext.aspectjProjects = [project(':spring-security-aspects'), project(':spring-security-samples-aspectj-xml')]
+ext.aspectjProjects = [project(':spring-security-aspects'), project(':spring-security-samples-aspectj-xml'), project(':spring-security-samples-aspectj-jc')]
 
 configure(allprojects - javaProjects) {
     task afterEclipseImport {

config/config.gradle

@@ -28,6 +28,7 @@ dependencies {
 
     testCompile project(':spring-security-cas'),
                 project(':spring-security-core').sourceSets.test.output,
+                project(':spring-security-aspects'),
                 'javax.annotation:jsr250-api:1.0',
                 "org.springframework.ldap:spring-ldap-core:$springLdapVersion",
                 "org.springframework:spring-expression:$springVersion",

config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityAspectJAutoProxyRegistrar.java

@@ -18,6 +18,8 @@ package org.springframework.security.config.annotation.method.configuration;
 import java.util.Map;
 
 import org.springframework.aop.config.AopConfigUtils;
+import org.springframework.beans.factory.config.BeanDefinition;
+import org.springframework.beans.factory.support.BeanDefinitionBuilder;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.context.annotation.ImportBeanDefinitionRegistrar;
 import org.springframework.core.annotation.AnnotationAttributes;
@@ -50,18 +52,15 @@ class GlobalMethodSecurityAspectJAutoProxyRegistrar implements
             AnnotationMetadata importingClassMetadata,
             BeanDefinitionRegistry registry) {
 
-        AopConfigUtils
-                .registerAspectJAnnotationAutoProxyCreatorIfNecessary(registry);
+        BeanDefinition interceptor = registry.getBeanDefinition("methodSecurityInterceptor");
 
-        Map<String, Object> annotationAttributes = importingClassMetadata
-                .getAnnotationAttributes(EnableGlobalMethodSecurity.class
-                        .getName());
-        AnnotationAttributes enableAJAutoProxy = AnnotationAttributes
-                .fromMap(annotationAttributes);
+        BeanDefinitionBuilder aspect =
+                BeanDefinitionBuilder.rootBeanDefinition("org.springframework.security.access.intercept.aspectj.aspect.AnnotationSecurityAspect");
+        aspect.setFactoryMethod("aspectOf");
+        aspect.setRole(BeanDefinition.ROLE_INFRASTRUCTURE);
+        aspect.addPropertyValue("securityInterceptor", interceptor);
 
-        if (enableAJAutoProxy.getBoolean("proxyTargetClass")) {
-            AopConfigUtils.forceAutoProxyCreatorToUseClassProxying(registry);
-        }
+        registry.registerBeanDefinition("annotationSecurityAspect$0", aspect.getBeanDefinition());
     }
 
 }

config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java

@@ -24,10 +24,7 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.config.BeanDefinition;
-import org.springframework.context.annotation.Bean;
-import org.springframework.context.annotation.Configuration;
-import org.springframework.context.annotation.ImportAware;
-import org.springframework.context.annotation.Role;
+import org.springframework.context.annotation.*;
 import org.springframework.core.annotation.AnnotationAttributes;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.core.type.AnnotationMetadata;
@@ -48,6 +45,7 @@ import org.springframework.security.access.intercept.AfterInvocationProviderMana
 import org.springframework.security.access.intercept.RunAsManager;
 import org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor;
 import org.springframework.security.access.intercept.aopalliance.MethodSecurityMetadataSourceAdvisor;
+import org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor;
 import org.springframework.security.access.method.DelegatingMethodSecurityMetadataSource;
 import org.springframework.security.access.method.MethodSecurityMetadataSource;
 import org.springframework.security.access.prepost.PostInvocationAdviceProvider;
@@ -111,7 +109,7 @@ public class GlobalMethodSecurityConfiguration implements ImportAware {
      */
     @Bean
     public MethodInterceptor methodSecurityInterceptor() throws Exception {
-        MethodSecurityInterceptor methodSecurityInterceptor = new MethodSecurityInterceptor();
+        MethodSecurityInterceptor methodSecurityInterceptor = isAspectJ() ? new AspectJMethodSecurityInterceptor() : new MethodSecurityInterceptor();
         methodSecurityInterceptor
                 .setAccessDecisionManager(accessDecisionManager());
         methodSecurityInterceptor
@@ -379,6 +377,10 @@ public class GlobalMethodSecurityConfiguration implements ImportAware {
         return (Integer) enableMethodSecurity().get("order");
     }
 
+    private boolean isAspectJ() {
+        return enableMethodSecurity().getEnum("mode") == AdviceMode.ASPECTJ;
+    }
+
     private AnnotationAttributes enableMethodSecurity() {
         if (enableMethodSecurity == null) {
             // if it is null look at this instance (i.e. a subclass was used)

config/src/test/groovy/org/springframework/security/config/annotation/method/configuration/NamespaceGlobalMethodSecurityTests.groovy

@@ -15,11 +15,14 @@
  */
 package org.springframework.security.config.annotation.method.configuration
 
+import org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor
+
 import static org.fest.assertions.Assertions.assertThat
 import static org.junit.Assert.fail
 
 import java.lang.reflect.Method
 
+import org.springframework.security.access.intercept.aspectj.aspect.AnnotationSecurityAspect
 import org.springframework.aop.aspectj.annotation.AnnotationAwareAspectJAutoProxyCreator
 import org.springframework.beans.factory.BeanCreationException
 import org.springframework.context.ConfigurableApplicationContext
@@ -188,8 +191,8 @@ public class NamespaceGlobalMethodSecurityTests extends BaseSpringSpec {
         when:
             context = new AnnotationConfigApplicationContext(AspectJModeConfig)
         then:
-            AnnotationAwareAspectJAutoProxyCreator autoProxyCreator = context.getBean(AnnotationAwareAspectJAutoProxyCreator)
-            autoProxyCreator.proxyTargetClass == true
+            context.getBean(AnnotationSecurityAspect)
+            context.getBean(AspectJMethodSecurityInterceptor)
     }
 
     @Configuration
@@ -201,8 +204,8 @@ public class NamespaceGlobalMethodSecurityTests extends BaseSpringSpec {
         when:
             context = new AnnotationConfigApplicationContext(BaseMethodConfig,AspectJModeExtendsGMSCConfig)
         then:
-            AnnotationAwareAspectJAutoProxyCreator autoProxyCreator = context.getBean(AnnotationAwareAspectJAutoProxyCreator)
-            autoProxyCreator.proxyTargetClass == false
+            context.getBean(AnnotationSecurityAspect)
+            context.getBean(AspectJMethodSecurityInterceptor)
     }
 
     @Configuration

samples/aspectj-jc/build.gradle

@@ -0,0 +1,9 @@
+
+dependencies {
+    compile project(':spring-security-core'),
+            project(':spring-security-config')
+
+    aspectpath project(':spring-security-aspects')
+
+    runtime project(':spring-security-aspects')
+}

samples/aspectj-jc/pom.xml

@@ -0,0 +1,133 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>org.springframework.security</groupId>
+  <artifactId>spring-security-samples-aspectj-jc</artifactId>
+  <version>4.0.0.CI-SNAPSHOT</version>
+  <name>spring-security-samples-aspectj-jc</name>
+  <description>spring-security-samples-aspectj-jc</description>
+  <url>http://spring.io/spring-security</url>
+  <organization>
+    <name>spring.io</name>
+    <url>http://spring.io/</url>
+  </organization>
+  <licenses>
+    <license>
+      <name>The Apache Software License, Version 2.0</name>
+      <url>http://www.apache.org/licenses/LICENSE-2.0.txt</url>
+      <distribution>repo</distribution>
+    </license>
+  </licenses>
+  <developers>
+    <developer>
+      <id>rwinch</id>
+      <name>Rob Winch</name>
+      <email>rwinch@gopivotal.com</email>
+    </developer>
+  </developers>
+  <scm>
+    <connection>scm:git:git://github.com/spring-projects/spring-security</connection>
+    <developerConnection>scm:git:git://github.com/spring-projects/spring-security</developerConnection>
+    <url>https://github.com/spring-projects/spring-security</url>
+  </scm>
+  <build>
+    <plugins>
+      <plugin>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <configuration>
+          <source>1.7</source>
+          <target>1.7</target>
+        </configuration>
+      </plugin>
+    </plugins>
+  </build>
+  <repositories>
+    <repository>
+      <id>spring-snasphot</id>
+      <url>https://repo.spring.io/snapshot</url>
+    </repository>
+  </repositories>
+  <dependencies>
+    <dependency>
+      <groupId>org.springframework.security</groupId>
+      <artifactId>spring-security-config</artifactId>
+      <version>4.0.0.CI-SNAPSHOT</version>
+      <scope>compile</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework.security</groupId>
+      <artifactId>spring-security-core</artifactId>
+      <version>4.0.0.CI-SNAPSHOT</version>
+      <scope>compile</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-core</artifactId>
+      <version>4.1.0.BUILD-SNAPSHOT</version>
+      <scope>compile</scope>
+      <exclusions>
+        <exclusion>
+          <artifactId>commons-logging</artifactId>
+          <groupId>commons-logging</groupId>
+        </exclusion>
+      </exclusions>
+    </dependency>
+    <dependency>
+      <groupId>commons-logging</groupId>
+      <artifactId>commons-logging</artifactId>
+      <version>1.1.1</version>
+      <scope>compile</scope>
+      <optional>true</optional>
+    </dependency>
+    <dependency>
+      <groupId>org.aspectj</groupId>
+      <artifactId>aspectjrt</artifactId>
+      <version>1.6.10</version>
+      <scope>compile</scope>
+      <optional>true</optional>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework.security</groupId>
+      <artifactId>spring-security-aspects</artifactId>
+      <version>4.0.0.CI-SNAPSHOT</version>
+      <scope>runtime</scope>
+    </dependency>
+    <dependency>
+      <groupId>ch.qos.logback</groupId>
+      <artifactId>logback-classic</artifactId>
+      <version>0.9.29</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>
+      <version>4.11</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.easytesting</groupId>
+      <artifactId>fest-assert</artifactId>
+      <version>1.4</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <version>1.9.5</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.slf4j</groupId>
+      <artifactId>jcl-over-slf4j</artifactId>
+      <version>1.7.5</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.springframework</groupId>
+      <artifactId>spring-test</artifactId>
+      <version>4.1.0.BUILD-SNAPSHOT</version>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+</project>

samples/aspectj-jc/src/main/java/sample/aspectj/AspectjSecurityConfig.java

@@ -0,0 +1,45 @@
+/*
+ * 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 sample.aspectj;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.AdviceMode;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder;
+import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
+
+/**
+ * @author Rob Winch
+ */
+@Configuration
+@EnableGlobalMethodSecurity(mode = AdviceMode.ASPECTJ,securedEnabled = true)
+public class AspectjSecurityConfig {
+	@Bean
+	public Service service() {
+		return new Service();
+	}
+
+	@Bean
+	public SecuredService securedService() {
+		return new SecuredService();
+	}
+
+	@Autowired
+	public void configureGlobal(AuthenticationManagerBuilder auth) throws Exception {
+		auth.inMemoryAuthentication();
+	}
+}

samples/aspectj-jc/src/main/java/sample/aspectj/SecuredService.java

@@ -0,0 +1,18 @@
+package sample.aspectj;
+
+import org.springframework.security.access.annotation.Secured;
+
+/**
+ * Service which is secured on the class level
+ *
+ * @author Mike Wiesner
+ * @since 3.0
+ */
+@Secured("ROLE_USER")
+public class SecuredService {
+
+    public void secureMethod() {
+        // nothing
+    }
+
+}

samples/aspectj-jc/src/main/java/sample/aspectj/Service.java

@@ -0,0 +1,22 @@
+package sample.aspectj;
+
+import org.springframework.security.access.annotation.Secured;
+
+/**
+ * Service which is secured on method level
+ *
+ * @author Mike Wiesner
+ * @since 1.0
+ */
+public class Service {
+
+    @Secured("ROLE_USER")
+    public void secureMethod() {
+        // nothing
+    }
+
+    public void publicMethod() {
+        // nothing
+    }
+
+}

samples/aspectj-jc/src/test/java/sample/aspectj/AspectJInterceptorTests.java

@@ -0,0 +1,93 @@
+package sample.aspectj;
+
+import org.junit.After;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.AdviceMode;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
+import org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder;
+import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
+import org.springframework.security.core.Authentication;
+import org.springframework.security.core.authority.AuthorityUtils;
+import org.springframework.security.core.context.SecurityContextHolder;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
+
+import java.lang.reflect.Proxy;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(classes=AspectjSecurityConfig.class)
+public class AspectJInterceptorTests {
+    private Authentication admin = new UsernamePasswordAuthenticationToken("test", "xxx", AuthorityUtils.createAuthorityList("ROLE_ADMIN"));
+    private Authentication user = new UsernamePasswordAuthenticationToken("test", "xxx", AuthorityUtils.createAuthorityList("ROLE_USER"));
+
+    @Autowired
+    private Service service;
+
+    @Autowired
+    private SecuredService securedService;
+
+    @Test
+    public void publicMethod() throws Exception {
+        service.publicMethod();
+    }
+
+    @Test(expected = AuthenticationCredentialsNotFoundException.class)
+    public void securedMethodNotAuthenticated() throws Exception {
+        service.secureMethod();
+    }
+
+    @Test(expected = AccessDeniedException.class)
+    public void securedMethodWrongRole() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(admin);
+        service.secureMethod();
+    }
+
+    @Test
+    public void securedMethodEverythingOk() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(user);
+        service.secureMethod();
+    }
+
+    @Test(expected = AuthenticationCredentialsNotFoundException.class)
+    public void securedClassNotAuthenticated() throws Exception {
+        securedService.secureMethod();
+    }
+
+    @Test(expected = AccessDeniedException.class)
+    public void securedClassWrongRole() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(admin);
+        securedService.secureMethod();
+    }
+
+    @Test(expected = AccessDeniedException.class)
+    public void securedClassWrongRoleOnNewedInstance() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(admin);
+        new SecuredService().secureMethod();
+    }
+
+    @Test
+    public void securedClassEverythingOk() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(user);
+        securedService.secureMethod();
+        new SecuredService().secureMethod();
+    }
+
+    // SEC-2595
+    @Test
+    public void notProxy() {
+        assertThat(Proxy.isProxyClass(securedService.getClass())).isFalse();
+    }
+
+    @After
+    public void tearDown() {
+        SecurityContextHolder.clearContext();
+    }
+}

samples/aspectj-jc/src/test/resources/logback-test.xml

@@ -0,0 +1,14 @@
+<configuration>
+  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
+    <encoder>
+      <pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern>
+    </encoder>
+  </appender>
+
+  <logger name="org.springframework.security" level="${sec.log.level}:-WARN"/>
+
+  <root level="${root.level}:-WARN">
+    <appender-ref ref="STDOUT" />
+  </root>
+
+</configuration>

settings.gradle

@@ -17,6 +17,7 @@ def String[] samples = [
     'contacts-xml',
     'openid-xml',
     'aspectj-xml',
+    'aspectj-jc',
     'gae-xml',
     'dms-xml',
     'preauth-xml',