Polish CompositeLogoutHandler

Issue gh-3895

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

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

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<LogoutHandler> logoutHandlers;
 	private LogoutHandler logoutHandler;
 
 	HttpServlet3RequestFactory(String rolePrefix) {
@@ -146,7 +146,7 @@ final class HttpServlet3RequestFactory implements HttpServletRequestFactory {
 	 * {@link HttpServletRequest#logout()}.
 	 */
 	public void setLogoutHandlers(List<LogoutHandler> 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<LogoutHandler> 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() {

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<LogoutHandler>)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<LogoutHandler> logoutHandlers = Arrays.asList(securityContextLogoutHandler);
-		LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers);
-		assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull();
-		assertThat(((List<LogoutHandler>)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<LogoutHandler> logoutHandlers = Arrays.asList(securityContextLogoutHandler, csrfLogoutHandler);
 		LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers);
-		assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull();
-		assertThat(((List<LogoutHandler>)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<LogoutHandler> logoutHandlers = Arrays.asList(firstLogoutHandler, secondLogoutHandler);
 		LogoutHandler handler = new CompositeLogoutHandler(logoutHandlers);
-		assertThat(ReflectionTestUtils.getField(handler, "logoutHandlers")).isNotNull();
-		assertThat(((List<LogoutHandler>)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));
-		}
-	}
-
-	static class FirstLogoutHandler implements LogoutHandler {
-
-		@Override
-		public void logout(HttpServletRequest request, HttpServletResponse response, Authentication authentication) {
-
+			fail("Expected Exception");
+		} catch (IllegalArgumentException success) {
 		}
-	}
 
-	static class SecondLogoutHandler implements LogoutHandler {
+		InOrder logoutHandlersInOrder = inOrder(firstLogoutHandler, secondLogoutHandler);
 
-		@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));
 	}
 
 }