@@ -16,8 +17,8 @@ import org.springframework.security.access.intercept.aspectj.AspectJSecurityInte * interfaces are not 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"); + } } } diff --git a/aspects/src/test/java/org/springframework/security/access/intercept/aspectj/aspect/AnnotationSecurityAspectTests.java b/aspects/src/test/java/org/springframework/security/access/intercept/aspectj/aspect/AnnotationSecurityAspectTests.java new file mode 100644 index 0000000000..8e4a8828c5 --- /dev/null +++ b/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() { + } +} diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationCallback.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationCallback.java index 6b46bd6de9..0fd833b7e5 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationCallback.java +++ b/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 ======================================================================================================== diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationSecurityInterceptor.java index 5c193f1d8a..20018f6dce 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJAnnotationSecurityInterceptor.java +++ b/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 ================================================================================================ diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptor.java new file mode 100644 index 0000000000..8682056b70 --- /dev/null +++ b/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. + *
+ * 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;
+ }
+}
diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptor.java
index 9348d4d7f1..3e23102f2f 100644
--- a/core/src/main/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptor.java
+++ b/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 ================================================================================================
diff --git a/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java b/core/src/main/java/org/springframework/security/access/intercept/aspectj/MethodInvocationAdapter.java
new file mode 100644
index 0000000000..a664c7c8e0
--- /dev/null
+++ b/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();
+ }
+}
diff --git a/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java b/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java
index 1d377850e4..63ec450342 100644
--- a/core/src/main/java/org/springframework/security/access/method/AbstractMethodSecurityMetadataSource.java
+++ b/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) {
diff --git a/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptorTests.java b/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptorTests.java
index 2f84f5863a..fb160cbace 100644
--- a/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJSecurityInterceptorTests.java
+++ b/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();
}
}
diff --git a/gradle/aspectj.gradle b/gradle/aspectj.gradle
index 4d239ba55b..e366e6dc0c 100644
--- a/gradle/aspectj.gradle
+++ b/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)
+ }
}
}
}
-}
+}
\ No newline at end of file
diff --git a/gradle/javaprojects.gradle b/gradle/javaprojects.gradle
index ca5f4d9d3f..50079d8af2 100644
--- a/gradle/javaprojects.gradle
+++ b/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',
diff --git a/samples/aspectj/src/main/resources/aspectj-context.xml b/samples/aspectj/src/main/resources/aspectj-context.xml
index b73e73a0b9..ef48497453 100644
--- a/samples/aspectj/src/main/resources/aspectj-context.xml
+++ b/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">