diff --git a/web/src/main/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandler.java b/web/src/main/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandler.java index c37ccf1f27..c399807799 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandler.java +++ b/web/src/main/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandler.java @@ -29,7 +29,7 @@ import org.springframework.util.Assert; * Performs a logout through all the {@link LogoutHandler} implementations. * If any exception is thrown by * {@link #logout(HttpServletRequest, HttpServletResponse, Authentication)}, - * next element in {@link #logoutHandlers} is not invoked. + * no additional LogoutHandler are invoked. * * @author Eddú Meléndez * @since 4.2.0 @@ -54,5 +54,4 @@ public final class CompositeLogoutHandler implements LogoutHandler { handler.logout(request, response, authentication); } } - -} +} \ No newline at end of file diff --git a/web/src/main/java/org/springframework/security/web/authentication/logout/LogoutFilter.java b/web/src/main/java/org/springframework/security/web/authentication/logout/LogoutFilter.java index 29f0e01a23..8bb36f1ec6 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/logout/LogoutFilter.java +++ b/web/src/main/java/org/springframework/security/web/authentication/logout/LogoutFilter.java @@ -56,7 +56,7 @@ public class LogoutFilter extends GenericFilterBean { private RequestMatcher logoutRequestMatcher; - private LogoutHandler handler; + private final LogoutHandler handler; private final LogoutSuccessHandler logoutSuccessHandler; // ~ Constructors diff --git a/web/src/main/java/org/springframework/security/web/servletapi/HttpServlet3RequestFactory.java b/web/src/main/java/org/springframework/security/web/servletapi/HttpServlet3RequestFactory.java index 4d726e50b4..1aace78c88 100644 --- a/web/src/main/java/org/springframework/security/web/servletapi/HttpServlet3RequestFactory.java +++ b/web/src/main/java/org/springframework/security/web/servletapi/HttpServlet3RequestFactory.java @@ -45,6 +45,7 @@ import org.springframework.security.web.AuthenticationEntryPoint; import org.springframework.security.web.authentication.logout.CompositeLogoutHandler; import org.springframework.security.web.authentication.logout.LogoutHandler; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * Provides integration with the Servlet 3 APIs in addition to the ones found in @@ -81,7 +82,6 @@ final class HttpServlet3RequestFactory implements HttpServletRequestFactory { private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); private AuthenticationEntryPoint authenticationEntryPoint; private AuthenticationManager authenticationManager; - private List logoutHandlers; private LogoutHandler logoutHandler; HttpServlet3RequestFactory(String rolePrefix) { @@ -146,7 +146,7 @@ final class HttpServlet3RequestFactory implements HttpServletRequestFactory { * {@link HttpServletRequest#logout()}. */ public void setLogoutHandlers(List logoutHandlers) { - this.logoutHandlers = logoutHandlers; + this.logoutHandler = CollectionUtils.isEmpty(logoutHandlers) ? null : new CompositeLogoutHandler(logoutHandlers); } /** @@ -246,19 +246,16 @@ final class HttpServlet3RequestFactory implements HttpServletRequestFactory { @Override public void logout() throws ServletException { - List handlers = HttpServlet3RequestFactory.this.logoutHandlers; - if (handlers == null) { + LogoutHandler handler = HttpServlet3RequestFactory.this.logoutHandler; + if (handler == null) { HttpServlet3RequestFactory.this.logger.debug( "logoutHandlers is null, so allowing original HttpServletRequest to handle logout"); super.logout(); return; - } else { - HttpServlet3RequestFactory.this.logoutHandler = new - CompositeLogoutHandler(handlers); } Authentication authentication = SecurityContextHolder.getContext() .getAuthentication(); - HttpServlet3RequestFactory.this.logoutHandler.logout(this, this.response, authentication); + handler.logout(this, this.response, authentication); } private boolean isAuthenticated() { diff --git a/web/src/test/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandlerTests.java b/web/src/test/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandlerTests.java index e77cf35eb9..6cea0b6cbb 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/logout/CompositeLogoutHandlerTests.java @@ -28,9 +28,8 @@ import org.junit.rules.ExpectedException; import org.mockito.InOrder; import org.springframework.security.core.Authentication; -import org.springframework.test.util.ReflectionTestUtils; -import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; @@ -41,6 +40,8 @@ import static org.mockito.Mockito.verify; /** * @author Eddú Meléndez + * @author Rob Winch + * @since 4.2.0 */ public class CompositeLogoutHandlerTests { @@ -55,24 +56,16 @@ public class CompositeLogoutHandlerTests { } @Test - public void buildCompositeLogoutHandlerWithArray() { - LogoutHandler[] logoutHandlers = {new SecurityContextLogoutHandler()}; - LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers); - assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull(); - assertThat(((List)ReflectionTestUtils.getField(handler, - "logoutHandlers")).size()) - .isEqualTo(1); - } + public void callLogoutHandlersSuccessfullyWithArray() { + LogoutHandler securityContextLogoutHandler = mock(SecurityContextLogoutHandler.class); + LogoutHandler csrfLogoutHandler = mock(SecurityContextLogoutHandler.class); - @Test - public void buildCompositeLogoutHandlerWithList() { - LogoutHandler securityContextLogoutHandler = new SecurityContextLogoutHandler(); - List logoutHandlers = Arrays.asList(securityContextLogoutHandler); - LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers); - assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull(); - assertThat(((List)ReflectionTestUtils.getField(handler, - "logoutHandlers")).size()) - .isEqualTo(1); + LogoutHandler handler = new CompositeLogoutHandler(securityContextLogoutHandler, csrfLogoutHandler); + + handler.logout(mock(HttpServletRequest.class), mock(HttpServletResponse.class), mock(Authentication.class)); + + verify(securityContextLogoutHandler, times(1)).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); + verify(csrfLogoutHandler, times(1)).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); } @Test @@ -82,8 +75,6 @@ public class CompositeLogoutHandlerTests { List logoutHandlers = Arrays.asList(securityContextLogoutHandler, csrfLogoutHandler); LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers); - assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull(); - assertThat(((List)ReflectionTestUtils.getField(handler, "logoutHandlers")).size()).isEqualTo(2); handler.logout(mock(HttpServletRequest.class), mock(HttpServletResponse.class), mock(Authentication.class)); @@ -93,42 +84,24 @@ public class CompositeLogoutHandlerTests { @Test public void callLogoutHandlersThrowException() { - LogoutHandler firstLogoutHandler = mock(FirstLogoutHandler.class); - LogoutHandler secondLogoutHandler = mock(SecondLogoutHandler.class); + LogoutHandler firstLogoutHandler = mock(LogoutHandler.class); + LogoutHandler secondLogoutHandler = mock(LogoutHandler.class); doThrow(new IllegalArgumentException()).when(firstLogoutHandler).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); List logoutHandlers = Arrays.asList(firstLogoutHandler, secondLogoutHandler); LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers); - assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull(); - assertThat(((List)ReflectionTestUtils.getField(handler, "logoutHandlers")).size()).isEqualTo(2); try { handler.logout(mock(HttpServletRequest.class), mock(HttpServletResponse.class), mock(Authentication.class)); - } catch (IllegalArgumentException ex) { - // Do nothing - } finally { - InOrder logoutHandlersInOrder = inOrder(firstLogoutHandler, secondLogoutHandler); - - logoutHandlersInOrder.verify(firstLogoutHandler, times(1)).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); - logoutHandlersInOrder.verify(secondLogoutHandler, never()).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); + fail("Expected Exception"); + } catch (IllegalArgumentException success) { } - } - static class FirstLogoutHandler implements LogoutHandler { + InOrder logoutHandlersInOrder = inOrder(firstLogoutHandler, secondLogoutHandler); - @Override - public void logout(HttpServletRequest request, HttpServletResponse response, Authentication authentication) { - - } - } - - static class SecondLogoutHandler implements LogoutHandler { - - @Override - public void logout(HttpServletRequest request, HttpServletResponse response, Authentication authentication) { - - } + logoutHandlersInOrder.verify(firstLogoutHandler, times(1)).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); + logoutHandlersInOrder.verify(secondLogoutHandler, never()).logout(any(HttpServletRequest.class), any(HttpServletResponse.class), any(Authentication.class)); } }