SEC-1635: Stop security interceptors from calling AfterInvocationManager if exception occurs during invocation

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() {

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);
     }
 }

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);
     }

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);
+    }
+
 }

docs/manual/src/docbook/technical-overview.xml

@@ -551,7 +551,9 @@ Successfully authenticated. Security context contains: \
                 </listitem>
                 <listitem>
                     <para>Call the <interfacename>AfterInvocationManager</interfacename> if
-                        configured, once the invocation has returned.</para>
+                        configured, once the invocation has returned. If the invocation raised an
+                        exception, the <interfacename>AfterInvocationManager</interfacename>
+                        will not be invoked.</para>
                 </listitem>
                 </orderedlist></para>
             <section xml:id="tech-intro-config-attributes">
@@ -602,7 +604,7 @@ Successfully authenticated. Security context contains: \
             </section>
             <section>
                 <title>AfterInvocationManager</title>
-                <para>Following the secure object proceeding and then returning - which may mean a
+                <para>Following the secure object invocation proceeding and then returning - which may mean a
                     method invocation completing or a filter chain proceeding - the
                     <classname>AbstractSecurityInterceptor</classname> gets one final chance to
                     handle the invocation. At this stage the
@@ -613,7 +615,10 @@ Successfully authenticated. Security context contains: \
                     <classname>AbstractSecurityInterceptor</classname> will pass control to an
                     <literal>AfterInvocationManager</literal> 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.</para>
+                    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.
+                </para>
                 <para><classname>AbstractSecurityInterceptor</classname> and its related objects are
                     shown in <xref linkend="abstract-security-interceptor"/>. <figure
                     xml:id="abstract-security-interceptor">

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);
         }
     }
 

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<ConfigAttribute> 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;
+    }
+
 }