diff --git a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java index bc5d897d4b..a18a265822 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptor.java @@ -51,21 +51,16 @@ public class MethodSecurityInterceptor extends AbstractSecurityInterceptor imple * * @param mi The method being invoked which requires a security decision * - * @return The returned value from the method invocation + * @return The returned value from the method invocation (possibly modified by the {@code AfterInvocationManager}). * * @throws Throwable if any error occurs */ public Object invoke(MethodInvocation mi) throws Throwable { - Object result = null; InterceptorStatusToken token = super.beforeInvocation(mi); - try { - result = mi.proceed(); - } finally { - result = super.afterInvocation(token, result); - } + Object result = mi.proceed(); - return result; + return super.afterInvocation(token, result); } public MethodSecurityMetadataSource getSecurityMetadataSource() { 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 index 8682056b70..3ea0231802 100644 --- 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 @@ -37,15 +37,10 @@ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterc * @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); - } + Object result = advisorProceed.proceedWithObject(); - return result; + return super.afterInvocation(token, result); } } diff --git a/core/src/test/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptorTests.java b/core/src/test/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptorTests.java index 0d099145e0..3b5dd4c235 100644 --- a/core/src/test/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/access/intercept/aopalliance/MethodSecurityInterceptorTests.java @@ -259,6 +259,27 @@ public class MethodSecurityInterceptorTests { advisedTarget.makeUpperCase("hello"); } + @Test + public void afterInvocationManagerIsNotInvokedIfExceptionIsRaised() throws Throwable { + MethodInvocation mi = mock(MethodInvocation.class); + token.setAuthenticated(true); + SecurityContextHolder.getContext().setAuthentication(token); + mdsReturnsUserRole(); + + AfterInvocationManager aim = mock(AfterInvocationManager.class); + interceptor.setAfterInvocationManager(aim); + + when(mi.proceed()).thenThrow(new Throwable()); + + try { + interceptor.invoke(mi); + fail("Expected exception"); + } catch (Throwable expected) { + } + + verifyZeroInteractions(aim); + } + void mdsReturnsNull() { when(mds.getAttributes(any(MethodInvocation.class))).thenReturn(null); } diff --git a/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptorTests.java b/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptorTests.java index b79b3580be..c69fab8731 100644 --- a/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/access/intercept/aspectj/AspectJMethodSecurityInterceptorTests.java @@ -19,6 +19,7 @@ import static org.junit.Assert.*; import static org.mockito.Matchers.any; import static org.mockito.Mockito.*; +import org.aopalliance.intercept.MethodInvocation; import org.aspectj.lang.JoinPoint; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.Signature; @@ -32,6 +33,7 @@ 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.AfterInvocationManager; import org.springframework.security.access.method.MethodSecurityMetadataSource; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -129,4 +131,24 @@ public class AspectJMethodSecurityInterceptorTests { assertEquals(m, mia.getMethod()); assertSame(to, mia.getThis()); } + + @Test + public void afterInvocationManagerIsNotInvokedIfExceptionIsRaised() throws Throwable { + token.setAuthenticated(true); + SecurityContextHolder.getContext().setAuthentication(token); + + AfterInvocationManager aim = mock(AfterInvocationManager.class); + interceptor.setAfterInvocationManager(aim); + + when(aspectJCallback.proceedWithObject()).thenThrow(new RuntimeException()); + + try { + interceptor.invoke(joinPoint, aspectJCallback); + fail("Expected exception"); + } catch (RuntimeException expected) { + } + + verifyZeroInteractions(aim); + } + } diff --git a/docs/manual/src/docbook/technical-overview.xml b/docs/manual/src/docbook/technical-overview.xml index 5437be7e4f..cc8c933d67 100644 --- a/docs/manual/src/docbook/technical-overview.xml +++ b/docs/manual/src/docbook/technical-overview.xml @@ -551,7 +551,9 @@ Successfully authenticated. Security context contains: \ Call the AfterInvocationManager if - configured, once the invocation has returned. + configured, once the invocation has returned. If the invocation raised an + exception, the AfterInvocationManager + will not be invoked.
@@ -602,7 +604,7 @@ Successfully authenticated. Security context contains: \
AfterInvocationManager - Following the secure object proceeding and then returning - which may mean a + Following the secure object invocation proceeding and then returning - which may mean a method invocation completing or a filter chain proceeding - the AbstractSecurityInterceptor gets one final chance to handle the invocation. At this stage the @@ -613,7 +615,10 @@ Successfully authenticated. Security context contains: \ AbstractSecurityInterceptor will pass control to an AfterInvocationManager to actually modify the object if needed. This class can even entirely replace the object, or throw an exception, - or not change it in any way as it chooses. + or not change it in any way as it chooses. The after-invocation checks will only + be executed if the invocation is successful. If an exception occurs, the additional + checks will be skipped. + AbstractSecurityInterceptor and its related objects are shown in .
diff --git a/web/src/main/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptor.java b/web/src/main/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptor.java index b095e9ea69..55dd0ce198 100644 --- a/web/src/main/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptor.java +++ b/web/src/main/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptor.java @@ -113,11 +113,9 @@ public class FilterSecurityInterceptor extends AbstractSecurityInterceptor imple InterceptorStatusToken token = super.beforeInvocation(fi); - try { - fi.getChain().doFilter(fi.getRequest(), fi.getResponse()); - } finally { - super.afterInvocation(token, null); - } + fi.getChain().doFilter(fi.getRequest(), fi.getResponse()); + + super.afterInvocation(token, null); } } diff --git a/web/src/test/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptorTests.java b/web/src/test/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptorTests.java index 3b4dde5c80..cfc6eaffb6 100644 --- a/web/src/test/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptorTests.java +++ b/web/src/test/java/org/springframework/security/web/access/intercept/FilterSecurityInterceptorTests.java @@ -15,22 +15,18 @@ package org.springframework.security.web.access.intercept; +import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Mockito.*; -import java.util.*; -import javax.servlet.FilterChain; - -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import org.junit.*; import org.springframework.context.ApplicationEventPublisher; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.access.AccessDecisionManager; -import org.springframework.security.access.ConfigAttribute; import org.springframework.security.access.SecurityConfig; import org.springframework.security.access.event.AuthorizedEvent; +import org.springframework.security.access.intercept.AfterInvocationManager; import org.springframework.security.access.intercept.RunAsManager; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -38,6 +34,10 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.FilterInvocation; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + /** * Tests {@link FilterSecurityInterceptor}. @@ -95,23 +95,51 @@ public class FilterSecurityInterceptorTests { */ @Test public void testSuccessfulInvocation() throws Throwable { - final MockHttpServletResponse response = new MockHttpServletResponse(); - final MockHttpServletRequest request = new MockHttpServletRequest(); - request.setServletPath("/secure/page.html"); - // Setup a Context - final Authentication token = new TestingAuthenticationToken("Test", "Password", "NOT_USED"); + Authentication token = new TestingAuthenticationToken("Test", "Password", "NOT_USED"); SecurityContextHolder.getContext().setAuthentication(token); - // Create and test our secure object - final FilterChain chain = mock(FilterChain.class); - final FilterInvocation fi = new FilterInvocation(request, response, chain); - final List attributes = SecurityConfig.createList("MOCK_OK"); + FilterInvocation fi = createinvocation(); - when(ods.getAttributes(fi)).thenReturn(attributes); + when(ods.getAttributes(fi)).thenReturn(SecurityConfig.createList("MOCK_OK")); interceptor.invoke(fi); - verify(publisher).publishEvent(any(AuthorizedEvent.class)); + verify(publisher).publishEvent(any(AuthorizedEvent.class)); } + + @Test + public void afterInvocationIsNotInvokedIfExceptionThrown() throws Exception { + Authentication token = new TestingAuthenticationToken("Test", "Password", "NOT_USED"); + SecurityContextHolder.getContext().setAuthentication(token); + + FilterInvocation fi = createinvocation(); + FilterChain chain = fi.getChain(); + + doThrow(new RuntimeException()).when(chain).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class)); + when(ods.getAttributes(fi)).thenReturn(SecurityConfig.createList("MOCK_OK")); + + AfterInvocationManager aim = mock(AfterInvocationManager.class); + interceptor.setAfterInvocationManager(aim); + + try { + interceptor.invoke(fi); + fail("Expected exception"); + } catch (RuntimeException expected) { + } + + verifyZeroInteractions(aim); + } + + private FilterInvocation createinvocation() { + MockHttpServletResponse response = new MockHttpServletResponse(); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setServletPath("/secure/page.html"); + + FilterChain chain = mock(FilterChain.class); + FilterInvocation fi = new FilterInvocation(request, response, chain); + + return fi; + } + }