From fd1a70edc230544b73df8e049626b6668d5dd201 Mon Sep 17 00:00:00 2001 From: Luke Taylor Date: Fri, 4 Mar 2011 17:45:37 +0000 Subject: [PATCH] SEC-1665: Add extra check of non-public declared methods in MethodInvocationAdapter, if public method cannot be found. --- .../aspect/AnnotationSecurityAspectTests.java | 59 +++++++++++++++++-- .../aspectj/MethodInvocationAdapter.java | 26 +++++++- 2 files changed, 77 insertions(+), 8 deletions(-) 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 index cfe1dc12bb..62ab7c268b 100644 --- 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 @@ -28,6 +28,7 @@ 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.access.vote.RoleVoter; import org.springframework.security.authentication.AuthenticationCredentialsNotFoundException; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -39,18 +40,23 @@ import org.springframework.security.core.context.SecurityContextHolder; * @since 3.0.3 */ public class AnnotationSecurityAspectTests { - private @Mock AccessDecisionManager adm; + private AffirmativeBased 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 SecuredImplSubclass securedSub = new SecuredImplSubclass(); private PrePostSecured prePostSecured = new PrePostSecured(); @Before public final void setUp() throws Exception { MockitoAnnotations.initMocks(this); interceptor = new AspectJMethodSecurityInterceptor(); + adm = new AffirmativeBased(); + AccessDecisionVoter[] voters = new AccessDecisionVoter[] + {new RoleVoter(), new PreInvocationAuthorizationAdviceVoter(new ExpressionBasedPreInvocationAdvice())}; + adm.setDecisionVoters(Arrays.asList(voters)); interceptor.setAccessDecisionManager(adm); interceptor.setAuthenticationManager(authman); interceptor.setSecurityMetadataSource(new SecuredAnnotationSecurityMetadataSource()); @@ -79,6 +85,31 @@ public class AnnotationSecurityAspectTests { secured.securedClassMethod(); } + @Test(expected=AccessDeniedException.class) + public void internalPrivateCallIsIntercepted() { + SecurityContextHolder.getContext().setAuthentication(anne); + + try { + secured.publicCallsPrivate(); + fail("Expected AccessDeniedException"); + } catch (AccessDeniedException expected) { + } + securedSub.publicCallsPrivate(); + } + + @Test(expected=AccessDeniedException.class) + public void protectedMethodIsIntercepted() throws Exception { + SecurityContextHolder.getContext().setAuthentication(anne); + + secured.protectedMethod(); + } + + @Test + public void overriddenProtectedMethodIsNotIntercepted() throws Exception { + // AspectJ doesn't inherit annotations + securedSub.protectedMethod(); + } + // SEC-1262 @Test(expected=AccessDeniedException.class) public void denyAllPreAuthorizeDeniesAccess() throws Exception { @@ -101,10 +132,6 @@ public class AnnotationSecurityAspectTests { DefaultMethodSecurityExpressionHandler eh = new DefaultMethodSecurityExpressionHandler(); interceptor.setSecurityMetadataSource(new PrePostAnnotationSecurityMetadataSource( new ExpressionBasedAnnotationAttributeFactory(eh))); - AffirmativeBased adm = new AffirmativeBased(); - AccessDecisionVoter[] voters = new AccessDecisionVoter[] - {new PreInvocationAuthorizationAdviceVoter(new ExpressionBasedPreInvocationAdvice())}; - adm.setDecisionVoters(Arrays.asList(voters)); interceptor.setAccessDecisionManager(adm); AfterInvocationProviderManager aim = new AfterInvocationProviderManager(); aim.setProviders(Arrays.asList(new PostInvocationAdviceProvider(new ExpressionBasedPostInvocationAdvice(eh)))); @@ -125,6 +152,28 @@ class SecuredImpl implements SecuredInterface { @Secured("ROLE_A") public void securedClassMethod() { } + + @Secured("ROLE_X") + private void privateMethod() { + } + + @Secured("ROLE_X") + protected void protectedMethod() { + } + + @Secured("ROLE_X") + public void publicCallsPrivate() { + privateMethod(); + } +} + +class SecuredImplSubclass extends SecuredImpl { + protected void protectedMethod() { + } + + public void publicCallsPrivate() { + super.publicCallsPrivate(); + } } class PrePostSecured { 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 index a664c7c8e0..271df0a7c9 100644 --- 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 @@ -32,11 +32,31 @@ public final class MethodInvocationAdapter implements MethodInvocation { } String targetMethodName = jp.getStaticPart().getSignature().getName(); Class[] types = ((CodeSignature) jp.getStaticPart().getSignature()).getParameterTypes(); - Class declaringType = ((CodeSignature) jp.getStaticPart().getSignature()).getDeclaringType(); + Class declaringType = jp.getStaticPart().getSignature().getDeclaringType(); - method = ClassUtils.getMethodIfAvailable(declaringType, targetMethodName, types); - Assert.notNull(method, "Could not obtain target method from JoinPoint: '"+ jp + "'"); + method = findMethod(targetMethodName, declaringType, types); + if(method == null) { + throw new IllegalArgumentException("Could not obtain target method from JoinPoint: '"+ jp + "'"); + } + } + + private Method findMethod(String name, Class declaringType, Class[] params) { + Method method = null; + + try { + method = declaringType.getMethod(name, params); + } catch (NoSuchMethodException ignored) { + } + + if (method == null) { + try { + method = declaringType.getDeclaredMethod(name, params); + } catch (NoSuchMethodException ignored) { + } + } + + return method; } public Method getMethod() {