SEC-1262: Added new (replacement) AspectJ interceptor which wraps the JoinPoint in a MethodInvocation adapter to provide compatibility with classes which only support MethodInvocation instances.

Also deprecated the existing AspectJ interceptors. This will also allow future simplification of the AbstractMethodSecurityMetadataSource, as it no longer needs to support JoinPoints.

aspects/aspects.gradle

@@ -1,5 +1,6 @@
 
 dependencies {
     compile project(':spring-security-core'),
-            "org.springframework:spring-beans:$springVersion"
+            "org.springframework:spring-beans:$springVersion",
+            "org.springframework:spring-context:$springVersion"
 }

aspects/src/main/java/org/springframework/security/access/intercept/aspectj/aspect/AnnotationSecurityAspect.aj

@@ -2,11 +2,12 @@ package org.springframework.security.access.intercept.aspectj.aspect;
 
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.security.access.annotation.Secured;
+import org.springframework.security.access.prepost.*;
 import org.springframework.security.access.intercept.aspectj.AspectJCallback;
-import org.springframework.security.access.intercept.aspectj.AspectJSecurityInterceptor;
+import org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor;
 
 /**
- * Concrete AspectJ transaction aspect using Spring Security @Secured annotation
+ * Concrete AspectJ aspect using Spring Security @Secured annotation
  * for JDK 1.5+.
  *
  * <p>
@@ -16,8 +17,8 @@ import org.springframework.security.access.intercept.aspectj.AspectJSecurityInte
  * interfaces are <i>not</i> inherited. This will vary from Spring AOP.
  *
  * @author Mike Wiesner
- * @since 1.0
- * @version $Id$
+ * @author Luke Taylor
+ * @since 3.1
  */
 public aspect AnnotationSecurityAspect implements InitializingBean {
 
@@ -34,11 +35,19 @@ public aspect AnnotationSecurityAspect implements InitializingBean {
     private pointcut executionOfSecuredMethod() :
         execution(* *(..)) && @annotation(Secured);
 
+    /**
+     * Matches the execution of any method with Pre/Post annotations.
+     */
+    private pointcut executionOfPrePostAnnotatedMethod() :
+        execution(* *(..)) && (@annotation(PreAuthorize) || @annotation(PreFilter)
+                || @annotation(PostAuthorize) || @annotation(PostFilter));
+
     private pointcut securedMethodExecution() :
         executionOfAnyPublicMethodInAtSecuredType() ||
-        executionOfSecuredMethod();
+        executionOfSecuredMethod() ||
+        executionOfPrePostAnnotatedMethod();
 
-    private AspectJSecurityInterceptor securityInterceptor;
+    private AspectJMethodSecurityInterceptor securityInterceptor;
 
     Object around(): securedMethodExecution() {
         if (this.securityInterceptor == null) {
@@ -54,13 +63,14 @@ public aspect AnnotationSecurityAspect implements InitializingBean {
         return this.securityInterceptor.invoke(thisJoinPoint, callback);
     }
 
-    public void setSecurityInterceptor(AspectJSecurityInterceptor securityInterceptor) {
+    public void setSecurityInterceptor(AspectJMethodSecurityInterceptor securityInterceptor) {
         this.securityInterceptor = securityInterceptor;
     }
 
     public void afterPropertiesSet() throws Exception {
-        if (this.securityInterceptor == null)
+        if (this.securityInterceptor == null) {
             throw new IllegalArgumentException("securityInterceptor required");
+        }
     }
 
 }

aspects/src/test/java/org/springframework/security/access/intercept/aspectj/aspect/AnnotationSecurityAspectTests.java

@@ -0,0 +1,110 @@
+package org.springframework.security.access.intercept.aspectj.aspect;
+
+import java.util.Arrays;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.springframework.security.access.AccessDecisionManager;
+import org.springframework.security.access.AccessDecisionVoter;
+import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.access.annotation.Secured;
+import org.springframework.security.access.annotation.SecuredAnnotationSecurityMetadataSource;
+import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
+import org.springframework.security.access.expression.method.ExpressionBasedAnnotationAttributeFactory;
+import org.springframework.security.access.expression.method.ExpressionBasedPreInvocationAdvice;
+import org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor;
+import org.springframework.security.access.prepost.PreAuthorize;
+import org.springframework.security.access.prepost.PreInvocationAuthorizationAdviceVoter;
+import org.springframework.security.access.prepost.PrePostAnnotationSecurityMetadataSource;
+import org.springframework.security.access.vote.AffirmativeBased;
+import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException;
+import org.springframework.security.authentication.AuthenticationManager;
+import org.springframework.security.authentication.TestingAuthenticationToken;
+import org.springframework.security.core.context.SecurityContextHolder;
+
+/**
+ *
+ * @author Luke Taylor
+ * @since 3.0.3
+ */
+public class AnnotationSecurityAspectTests {
+    private @Mock AccessDecisionManager adm;
+    private @Mock AuthenticationManager authman;
+    private TestingAuthenticationToken anne = new TestingAuthenticationToken("anne", "", "ROLE_A");
+//    private TestingAuthenticationToken bob = new TestingAuthenticationToken("bob", "", "ROLE_B");
+    private AspectJMethodSecurityInterceptor interceptor;
+    private SecuredImpl secured = new SecuredImpl();
+    private PrePostSecured prePostSecured = new PrePostSecured();
+
+    @Before
+    public final void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+        interceptor = new AspectJMethodSecurityInterceptor();
+        interceptor.setAccessDecisionManager(adm);
+        interceptor.setAuthenticationManager(authman);
+        interceptor.setSecurityMetadataSource(new SecuredAnnotationSecurityMetadataSource());
+        AnnotationSecurityAspect secAspect = AnnotationSecurityAspect.aspectOf();
+        secAspect.setSecurityInterceptor(interceptor);
+    }
+
+    @After
+    public void clearContext() {
+        SecurityContextHolder.clearContext();
+    }
+
+    @Test
+    public void securedInterfaceMethodAllowsAllAccess() throws Exception {
+        secured.securedMethod();
+    }
+
+    @Test(expected=AuthenticationCredentialsNotFoundException.class)
+    public void securedClassMethodDeniesUnauthenticatedAccess() throws Exception {
+        secured.securedClassMethod();
+    }
+
+    @Test
+    public void securedClassMethodAllowsAccessToRoleA() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(anne);
+        secured.securedClassMethod();
+    }
+
+    // SEC-1262
+    @Test(expected=AccessDeniedException.class)
+    public void denyAllPreAuthorizeDeniesAccess() throws Exception {
+        SecurityContextHolder.getContext().setAuthentication(anne);
+        interceptor.setSecurityMetadataSource(new PrePostAnnotationSecurityMetadataSource(
+                new ExpressionBasedAnnotationAttributeFactory(new DefaultMethodSecurityExpressionHandler())));
+        AffirmativeBased adm = new AffirmativeBased();
+        AccessDecisionVoter[] voters = new AccessDecisionVoter[]
+                       {new PreInvocationAuthorizationAdviceVoter(new ExpressionBasedPreInvocationAdvice())};
+        adm.setDecisionVoters(Arrays.asList(voters));
+        interceptor.setAccessDecisionManager(adm);
+        prePostSecured.denyAllMethod();
+    }
+}
+
+interface SecuredInterface {
+    @Secured("ROLE_X")
+    void securedMethod();
+}
+
+class SecuredImpl implements SecuredInterface {
+
+    // Not really secured because AspectJ doesn't inherit annotations from interfaces
+    public void securedMethod() {
+    }
+
+    @Secured("ROLE_A")
+    public void securedClassMethod() {
+    }
+}
+
+class PrePostSecured {
+
+    @PreAuthorize("denyAll")
+    public void denyAllMethod() {
+    }
+}

core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationCallback.java

@@ -6,8 +6,9 @@ package org.springframework.security.access.intercept.aspectj;
  * AspectJ processing to continue.
  *
  * @author Mike Wiesner
+ * @deprecated
  */
-
+@Deprecated
 public interface AspectJAnnotationCallback {
     //~ Methods ========================================================================================================
 

core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationSecurityInterceptor.java

@@ -11,7 +11,9 @@ import org.aspectj.lang.JoinPoint;
  * AspectJ interceptor that supports @Aspect notation.
  *
  * @author Mike Wiesner
+ * @deprecated Use AspectJMethodSecurityInterceptor instead
  */
+@Deprecated
 public class AspectJAnnotationSecurityInterceptor extends AbstractSecurityInterceptor {
     //~ Instance fields ================================================================================================
 

core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java

@@ -0,0 +1,51 @@
+package org.springframework.security.access.intercept.aspectj;
+
+import org.aspectj.lang.JoinPoint;
+import org.springframework.security.access.intercept.InterceptorStatusToken;
+import org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor;
+
+/**
+ * AspectJ {@code JoinPoint} security interceptor which wraps the {@code JoinPoint} in a {@code MethodInvocation}
+ * adapter to make it compatible with security infrastructure classes which only support {@code MethodInvocation}s.
+ * <p>
+ * One of the {@code invoke} methods should be called from the {@code around()} advice in your aspect.
+ * Alternatively you can use one of the pre-defined aspects from the aspects module.
+ *
+ * @author Luke Taylor
+ * @since 3.0.3
+ */
+public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterceptor {
+
+    /**
+     * Method that is suitable for user with @Aspect notation.
+     *
+     * @param jp The AspectJ joint point being invoked which requires a security decision
+     * @return The returned value from the method invocation
+     * @throws Throwable if the invocation throws one
+     */
+    public Object invoke(JoinPoint jp) throws Throwable {
+        return super.invoke(new MethodInvocationAdapter(jp));
+    }
+
+    /**
+     * Method that is suitable for user with traditional AspectJ-code aspects.
+     *
+     * @param jp The AspectJ joint point being invoked which requires a security decision
+     * @param advisorProceed the advice-defined anonymous class that implements {@code AspectJCallback} containing
+     *        a simple {@code return proceed();} statement
+     *
+     * @return The returned value from the method invocation
+     */
+    public Object invoke(JoinPoint jp, AspectJCallback advisorProceed) {
+        Object result = null;
+        InterceptorStatusToken token = super.beforeInvocation(new MethodInvocationAdapter(jp));
+
+        try {
+            result = advisorProceed.proceedWithObject();
+        } finally {
+            result = super.afterInvocation(token, result);
+        }
+
+        return result;
+    }
+}

core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptor.java

@@ -37,7 +37,9 @@ import org.aspectj.lang.JoinPoint;
  * Refer to {@link AbstractSecurityInterceptor} for details on the workflow.
  *
  * @author Ben Alex
+ * @deprecated Use AspectJMethodSecurityInterceptor instead
  */
+@Deprecated
 public class AspectJSecurityInterceptor extends AbstractSecurityInterceptor {
     //~ Instance fields ================================================================================================
 

core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java

@@ -0,0 +1,61 @@
+package org.springframework.security.access.intercept.aspectj;
+
+import java.lang.reflect.AccessibleObject;
+import java.lang.reflect.Method;
+
+import org.aopalliance.intercept.MethodInvocation;
+import org.aspectj.lang.JoinPoint;
+import org.aspectj.lang.ProceedingJoinPoint;
+import org.aspectj.lang.reflect.CodeSignature;
+import org.springframework.util.Assert;
+import org.springframework.util.ClassUtils;
+
+/**
+ * Decorates a JoinPoint to allow it to be used with method-security infrastructure
+ * classes which support {@code MethodInvocation} instances.
+ *
+ * @author Luke Taylor
+ * @since 3.0.3
+ */
+public final class MethodInvocationAdapter implements MethodInvocation {
+    private final ProceedingJoinPoint jp;
+    private final Method method;
+    private final Object target;
+
+    MethodInvocationAdapter(JoinPoint jp) {
+        this.jp = (ProceedingJoinPoint)jp;
+        if (jp.getTarget() != null) {
+            target = jp.getTarget();
+        } else {
+            // SEC-1295: target may be null if an ITD is in use
+            target = jp.getSignature().getDeclaringType();
+        }
+        String targetMethodName = jp.getStaticPart().getSignature().getName();
+        Class<?>[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes();
+        Class<?> declaringType = ((CodeSignature) jp.getStaticPart().getSignature()).getDeclaringType();
+
+        method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types);
+        Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'");
+
+    }
+
+    public Method getMethod() {
+        return method;
+    }
+
+    public Object[] getArguments() {
+        return jp.getArgs();
+    }
+
+    public AccessibleObject getStaticPart() {
+        return method;
+    }
+
+    public Object getThis() {
+        return target;
+    }
+
+    public Object proceed() throws Throwable {
+        return jp.proceed();
+    }
+}

core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java

@@ -49,7 +49,13 @@ public abstract class AbstractMethodSecurityMetadataSource implements MethodSecu
         if (object instanceof MethodInvocation) {
             MethodInvocation mi = (MethodInvocation) object;
             Object target = mi.getThis();
-            return getAttributes(mi.getMethod(), target == null ? null : target.getClass());
+            Class<?> targetClass = null;
+
+            if (target != null) {
+                targetClass = target instanceof Class<?> ? (Class<?>)target : target.getClass();
+            }
+
+            return getAttributes(mi.getMethod(), targetClass);
         }
 
         if (object instanceof JoinPoint) {

core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptorTests.java

@@ -15,26 +15,28 @@
 
 package org.springframework.security.access.intercept.aspectj;
 
+import static org.junit.Assert.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;
+
 import java.lang.reflect.Method;
-import java.util.List;
+import java.util.Collection;
 
 import org.aspectj.lang.JoinPoint;
-import org.jmock.Expectations;
-import org.jmock.Mockery;
-import org.jmock.integration.junit4.JUnit4Mockery;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
 import org.springframework.security.MockJoinPoint;
 import org.springframework.security.TargetObject;
 import org.springframework.security.access.AccessDecisionManager;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.SecurityConfig;
-import org.springframework.security.access.intercept.aspectj.AspectJCallback;
-import org.springframework.security.access.intercept.aspectj.AspectJSecurityInterceptor;
 import org.springframework.security.access.method.MethodSecurityMetadataSource;
 import org.springframework.security.authentication.AuthenticationManager;
 import org.springframework.security.authentication.TestingAuthenticationToken;
+import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolder;
 
 
@@ -42,33 +44,33 @@ import org.springframework.security.core.context.SecurityContextHolder;
  * Tests {@link AspectJSecurityInterceptor}.
  *
  * @author Ben Alex
+ * @author Luke Taylor
  */
+@SuppressWarnings("deprecation")
 public class AspectJSecurityInterceptorTests {
-    private Mockery jmock = new JUnit4Mockery();
     private TestingAuthenticationToken token;
     private AspectJSecurityInterceptor interceptor;
-    private AccessDecisionManager adm;
-    private MethodSecurityMetadataSource mds;
-    private AuthenticationManager authman;
-    private AspectJCallback aspectJCallback;
+    private @Mock AccessDecisionManager adm;
+    private @Mock MethodSecurityMetadataSource mds;
+    private @Mock AuthenticationManager authman;
+    private @Mock AspectJCallback aspectJCallback;
     private JoinPoint joinPoint;
 
     //~ Methods ========================================================================================================
 
     @Before
     public final void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
         SecurityContextHolder.clearContext();
         token = new TestingAuthenticationToken("Test", "Password");
         interceptor = new AspectJSecurityInterceptor();
-        adm = jmock.mock(AccessDecisionManager.class);
-        authman = jmock.mock(AuthenticationManager.class);
-        mds = jmock.mock(MethodSecurityMetadataSource.class);
         interceptor.setAccessDecisionManager(adm);
         interceptor.setAuthenticationManager(authman);
         interceptor.setSecurityMetadataSource(mds);
         Method method = TargetObject.class.getMethod("countLength", new Class[] {String.class});
         joinPoint = new MockJoinPoint(new TargetObject(), method);
-        aspectJCallback = jmock.mock(AspectJCallback.class);
+        when(mds.getAttributes(any(JoinPoint.class))).thenReturn(SecurityConfig.createList("ROLE_USER"));
+        when(authman.authenticate(token)).thenReturn(token);
     }
 
     @After
@@ -77,33 +79,23 @@ public class AspectJSecurityInterceptorTests {
     }
 
     @Test
-    @SuppressWarnings("unchecked")
     public void callbackIsInvokedWhenPermissionGranted() throws Exception {
-        jmock.checking(new Expectations() {{
-            oneOf(mds).getAttributes(with(any(JoinPoint.class))); will (returnValue(SecurityConfig.createList("ROLE_USER")));
-            oneOf(authman).authenticate(token); will(returnValue(token));
-            oneOf(adm).decide(with(token), with(aNonNull(JoinPoint.class)), with(aNonNull(List.class)));
-            oneOf(aspectJCallback).proceedWithObject();
-        }});
-
         SecurityContextHolder.getContext().setAuthentication(token);
         interceptor.invoke(joinPoint, aspectJCallback);
-        jmock.assertIsSatisfied();
+        verify(aspectJCallback).proceedWithObject();
     }
 
     @SuppressWarnings("unchecked")
-    @Test(expected=AccessDeniedException.class)
+    @Test
     public void callbackIsNotInvokedWhenPermissionDenied() throws Exception {
-        jmock.checking(new Expectations() {{
-            oneOf(mds).getAttributes(with(any(JoinPoint.class))); will (returnValue(SecurityConfig.createList("ROLE_USER")));
-            oneOf(authman).authenticate(token); will(returnValue(token));
-            oneOf(adm).decide(with(token), with(aNonNull(JoinPoint.class)), with(aNonNull(List.class)));
-                will(throwException(new AccessDeniedException("denied")));
-            never(aspectJCallback).proceedWithObject();
-        }});
+        doThrow(new AccessDeniedException("denied")).when(adm).decide(any(Authentication.class), any(), any(Collection.class));
 
         SecurityContextHolder.getContext().setAuthentication(token);
-        interceptor.invoke(joinPoint, aspectJCallback);
-        jmock.assertIsSatisfied();
+        try {
+            interceptor.invoke(joinPoint, aspectJCallback);
+            fail("Expected AccessDeniedException");
+        } catch (AccessDeniedException expected) {
+        }
+        verify(aspectJCallback, never()).proceedWithObject();
     }
 }

gradle/aspectj.gradle

@@ -10,15 +10,38 @@ dependencies {
     compile "org.aspectj:aspectjrt:$aspectjVersion"
 }
 
-task compileJava(dependsOn: JavaPlugin.PROCESS_RESOURCES_TASK_NAME, overwrite: true, description: 'Compiles AspectJ Source') << {
-    println "Running ajc ..."
-    ant.taskdef(resource: "org/aspectj/tools/ant/taskdefs/aspectjTaskdefs.properties", classpath: configurations.ajtools.asPath)
-    ant.iajc(classpath: configurations.compile.asPath, fork: 'true', destDir: sourceSets.main.classesDir.absolutePath, source: sourceCompatibility, target: targetCompatibility,
-            aspectPath: configurations.aspectpath.asPath, sourceRootCopyFilter: '**/*.java') {
-        sourceroots {
-            sourceSets.main.java.srcDirs.each {
-                pathelement(location: it.absolutePath)
+task compileJava(overwrite: true, description: 'Compiles AspectJ Source', type: Ajc) {
+    dependsOn processResources
+    sourceSet = sourceSets.main
+    aspectPath = configurations.aspectpath
+}
+
+task compileTestJava(overwrite: true, description: 'Compiles AspectJ Test Source', type: Ajc) {
+    dependsOn processTestResources, compileJava, jar
+    sourceSet = sourceSets.test
+    aspectPath = files(configurations.aspectpath, jar.archivePath)
+}
+
+class Ajc extends DefaultTask {
+    @Input
+    SourceSet sourceSet
+
+    @Input
+    FileCollection aspectPath
+
+    @TaskAction
+    def compile() {
+        println "Running ajc ..."
+        ant.taskdef(resource: "org/aspectj/tools/ant/taskdefs/aspectjTaskdefs.properties", classpath: project.configurations.ajtools.asPath)
+        ant.iajc(classpath: sourceSet.compileClasspath.asPath, fork: 'true', destDir: sourceSet.classesDir.absolutePath,
+                source: project.convention.plugins.java.sourceCompatibility,
+                target: project.convention.plugins.java.targetCompatibility,
+                aspectPath: aspectPath.asPath, sourceRootCopyFilter: '**/*.java', showWeaveInfo: 'true') {
+            sourceroots {
+                sourceSet.java.srcDirs.each {
+                    pathelement(location: it.absolutePath)
+                }
             }
         }
     }
-}
+}

gradle/javaprojects.gradle

@@ -22,7 +22,7 @@ dependencies {
     }
 
     testCompile 'junit:junit:4.7',
-            'org.mockito:mockito-core:1.7',
+            'org.mockito:mockito-core:1.8.3',
             'org.jmock:jmock:2.5.1',
             'org.jmock:jmock-junit4:2.5.1',
             'org.hamcrest:hamcrest-core:1.1',

samples/aspectj/src/main/resources/aspectj-context.xml

@@ -4,7 +4,7 @@
     xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-3.0.xsd">
 
     <bean id="aspectJSecurityInterceptor"
-        class="org.springframework.security.access.intercept.aspectj.AspectJSecurityInterceptor">
+        class="org.springframework.security.access.intercept.aspectj.AspectJMethodSecurityInterceptor">
         <property name="authenticationManager" ref="authenticationManager" />
         <property name="accessDecisionManager" ref="accessDecisionManager" />
         <property name="securityMetadataSource">