diff --git a/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java b/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java index b263230c18..0deb198670 100644 --- a/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java +++ b/core/src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java @@ -66,7 +66,7 @@ import org.springframework.util.Assert; *
  • Pass control back to the concrete subclass, which will actually proceed with executing the object. * A {@link InterceptorStatusToken} is returned so that after the subclass has finished proceeding with * execution of the object, its finally clause can ensure the AbstractSecurityInterceptor - * is re-called and tidies up correctly.
    • + * is re-called and tidies up correctly using {@link #finallyInvocation(InterceptorStatusToken)}. *
  • The concrete subclass will re-call the AbstractSecurityInterceptor via the * {@link #afterInvocation(InterceptorStatusToken, Object)} method.
    • *
  • If the RunAsManager replaced the Authentication object, return the @@ -91,6 +91,7 @@ import org.springframework.util.Assert; * * * @author Ben Alex + * @author Rob Winch */ public abstract class AbstractSecurityInterceptor implements InitializingBean, ApplicationEventPublisherAware, MessageSourceAware { @@ -242,6 +243,23 @@ public abstract class AbstractSecurityInterceptor implements InitializingBean, A } } + /** + * Cleans up the work of the AbstractSecurityInterceptor after the secure object invocation has been + * completed. This method should be invoked after the secure object invocation and before afterInvocation regardless + * of the secure object invocation returning successfully (i.e. it should be done in a finally block). + * + * @param token as returned by the {@link #beforeInvocation(Object)} method + */ + protected void finallyInvocation(InterceptorStatusToken token) { + if (token != null && token.isContextHolderRefreshRequired()) { + if (logger.isDebugEnabled()) { + logger.debug("Reverting to original Authentication: " + token.getSecurityContext().getAuthentication()); + } + + SecurityContextHolder.setContext(token.getSecurityContext()); + } + } + /** * Completes the work of the AbstractSecurityInterceptor after the secure object invocation has been * completed. @@ -256,13 +274,7 @@ public abstract class AbstractSecurityInterceptor implements InitializingBean, A return returnedObject; } - if (token.isContextHolderRefreshRequired()) { - if (logger.isDebugEnabled()) { - logger.debug("Reverting to original Authentication: " + token.getSecurityContext().getAuthentication()); - } - - SecurityContextHolder.setContext(token.getSecurityContext()); - } + finallyInvocation(token); // continue to clean in this method for passivity if (afterInvocationManager != null) { // Attempt after invocation handling 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 a18a265822..f7eac2621d 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 @@ -34,6 +34,7 @@ import org.aopalliance.intercept.MethodInvocation; * Refer to {@link AbstractSecurityInterceptor} for details on the workflow. * * @author Ben Alex + * @author Rob Winch */ public class MethodSecurityInterceptor extends AbstractSecurityInterceptor implements MethodInterceptor { //~ Instance fields ================================================================================================ @@ -58,8 +59,12 @@ public class MethodSecurityInterceptor extends AbstractSecurityInterceptor imple public Object invoke(MethodInvocation mi) throws Throwable { InterceptorStatusToken token = super.beforeInvocation(mi); - Object result = mi.proceed(); - + Object result; + try { + result = mi.proceed(); + } finally { + super.finallyInvocation(token); + } return super.afterInvocation(token, result); } 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 3ea0231802..a6bb9509d2 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 @@ -12,6 +12,7 @@ import org.springframework.security.access.intercept.aopalliance.MethodSecurityI * Alternatively you can use one of the pre-defined aspects from the aspects module. * * @author Luke Taylor + * @author Rob Winch * @since 3.0.3 */ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterceptor { @@ -39,7 +40,12 @@ public final class AspectJMethodSecurityInterceptor extends MethodSecurityInterc public Object invoke(JoinPoint jp, AspectJCallback advisorProceed) { InterceptorStatusToken token = super.beforeInvocation(new MethodInvocationAdapter(jp)); - Object result = advisorProceed.proceedWithObject(); + Object result; + try { + result = advisorProceed.proceedWithObject(); + }finally { + super.finallyInvocation(token); + } 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 9f5f54b784..87006a9bb9 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 @@ -45,11 +45,11 @@ import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import java.util.*; - /** * Tests {@link MethodSecurityInterceptor}. * * @author Ben Alex + * @author Rob Winch */ @SuppressWarnings("unchecked") public class MethodSecurityInterceptorTests { @@ -267,6 +267,31 @@ public class MethodSecurityInterceptorTests { assertSame(token, SecurityContextHolder.getContext().getAuthentication()); } + // SEC-1967 + @Test + public void runAsReplacementCleansAfterException() throws Exception { + createTarget(true); + when(realTarget.makeUpperCase(anyString())).thenThrow(new RuntimeException()); + SecurityContext ctx = SecurityContextHolder.getContext(); + ctx.setAuthentication(token); + token.setAuthenticated(true); + final RunAsManager runAs = mock(RunAsManager.class); + final RunAsUserToken runAsToken = + new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class); + interceptor.setRunAsManager(runAs); + mdsReturnsUserRole(); + when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken); + + try { + advisedTarget.makeUpperCase("hello"); + fail("Expected Exception"); + }catch(RuntimeException success) {} + + // Check we've changed back + assertSame(ctx, SecurityContextHolder.getContext()); + assertSame(token, SecurityContextHolder.getContext().getAuthentication()); + } + @Test(expected=AuthenticationCredentialsNotFoundException.class) public void emptySecurityContextIsRejected() throws Exception { mdsReturnsUserRole(); 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 c69fab8731..92ba83abd5 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 @@ -34,15 +34,19 @@ 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.intercept.RunAsManager; +import org.springframework.security.access.intercept.RunAsUserToken; 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.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.util.ClassUtils; import java.lang.reflect.Method; import java.util.Collection; +import java.util.List; /** @@ -50,6 +54,7 @@ import java.util.Collection; * * @author Ben Alex * @author Luke Taylor + * @author Rob Winch */ @SuppressWarnings("deprecation") public class AspectJMethodSecurityInterceptorTests { @@ -151,4 +156,51 @@ public class AspectJMethodSecurityInterceptorTests { verifyZeroInteractions(aim); } + // SEC-1967 + @Test + @SuppressWarnings("unchecked") + public void invokeWithAspectJCallbackRunAsReplacementCleansAfterException() throws Exception { + SecurityContext ctx = SecurityContextHolder.getContext(); + ctx.setAuthentication(token); + token.setAuthenticated(true); + final RunAsManager runAs = mock(RunAsManager.class); + final RunAsUserToken runAsToken = + new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class); + interceptor.setRunAsManager(runAs); + when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken); + when(aspectJCallback.proceedWithObject()).thenThrow(new RuntimeException()); + + try { + interceptor.invoke(joinPoint, aspectJCallback); + fail("Expected Exception"); + }catch(RuntimeException success) {} + + // Check we've changed back + assertSame(ctx, SecurityContextHolder.getContext()); + assertSame(token, SecurityContextHolder.getContext().getAuthentication()); + } + + // SEC-1967 + @Test + @SuppressWarnings("unchecked") + public void invokeRunAsReplacementCleansAfterException() throws Throwable { + SecurityContext ctx = SecurityContextHolder.getContext(); + ctx.setAuthentication(token); + token.setAuthenticated(true); + final RunAsManager runAs = mock(RunAsManager.class); + final RunAsUserToken runAsToken = + new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), TestingAuthenticationToken.class); + interceptor.setRunAsManager(runAs); + when(runAs.buildRunAs(eq(token), any(MethodInvocation.class), any(List.class))).thenReturn(runAsToken); + when(joinPoint.proceed()).thenThrow(new RuntimeException()); + + try { + interceptor.invoke(joinPoint); + fail("Expected Exception"); + }catch(RuntimeException success) {} + + // Check we've changed back + assertSame(ctx, SecurityContextHolder.getContext()); + assertSame(token, SecurityContextHolder.getContext().getAuthentication()); + } } 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 55dd0ce198..445796749f 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 @@ -39,6 +39,7 @@ import org.springframework.security.web.FilterInvocation; * Refer to {@link AbstractSecurityInterceptor} for details on the workflow.

    * * @author Ben Alex + * @author Rob Winch */ public class FilterSecurityInterceptor extends AbstractSecurityInterceptor implements Filter { //~ Static fields/initializers ===================================================================================== @@ -113,7 +114,11 @@ public class FilterSecurityInterceptor extends AbstractSecurityInterceptor imple InterceptorStatusToken token = super.beforeInvocation(fi); - fi.getChain().doFilter(fi.getRequest(), fi.getResponse()); + try { + fi.getChain().doFilter(fi.getRequest(), fi.getResponse()); + } finally { + super.finallyInvocation(token); + } 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 9b7effb3ca..8d7a3aeeaa 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,7 +15,7 @@ package org.springframework.security.web.access.intercept; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; import static org.mockito.Matchers.any; import static org.mockito.Mockito.*; @@ -28,9 +28,11 @@ 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.access.intercept.RunAsUserToken; import org.springframework.security.authentication.AuthenticationManager; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.FilterInvocation; @@ -44,6 +46,7 @@ import javax.servlet.http.HttpServletResponse; * * @author Ben Alex * @author Luke Taylor + * @author Rob Winch */ public class FilterSecurityInterceptorTests { private AuthenticationManager am; @@ -132,6 +135,39 @@ public class FilterSecurityInterceptorTests { verifyZeroInteractions(aim); } + // SEC-1967 + @Test + @SuppressWarnings("unchecked") + public void finallyInvocationIsInvokedIfExceptionThrown() throws Exception { + SecurityContext ctx = SecurityContextHolder.getContext(); + Authentication token = new TestingAuthenticationToken("Test", "Password", "NOT_USED"); + token.setAuthenticated(true); + ctx.setAuthentication(token); + + RunAsManager runAsManager = mock(RunAsManager.class); + when(runAsManager.buildRunAs(eq(token), any(), anyCollection())).thenReturn(new RunAsUserToken("key", "someone", "creds", token.getAuthorities(), token.getClass())); + interceptor.setRunAsManager(runAsManager); + + 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) { + } + + // Check we've changed back + assertSame(ctx, SecurityContextHolder.getContext()); + assertSame(token, SecurityContextHolder.getContext().getAuthentication()); + } + private FilterInvocation createinvocation() { MockHttpServletResponse response = new MockHttpServletResponse(); MockHttpServletRequest request = new MockHttpServletRequest();