Add CompositeLogoutHandler #3904

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@eddumelendez
Contributor

eddumelendez commented May 30, 2016

  • I have signed the CLA

Fixes gh-3895

+/**
+ * @author Eddú Meléndez
+ */
+public class CompositeLogoutHandler implements LogoutHandler {

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

Mark as final

@rwinch

rwinch May 31, 2016

Member

Mark as final

+ */
+public class CompositeLogoutHandler implements LogoutHandler {
+
+ private List<LogoutHandler> logoutHandlers;

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

mark as final

@rwinch

rwinch May 31, 2016

Member

mark as final

+ private List<LogoutHandler> logoutHandlers;
+
+ public CompositeLogoutHandler(LogoutHandler... logoutHandlers) {
+ this.logoutHandlers = Arrays.asList(logoutHandlers);

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

Can we validate that there are no null values

@rwinch

rwinch May 31, 2016

Member

Can we validate that there are no null values

+ public CompositeLogoutHandler(LogoutHandler... logoutHandlers) {
+ this.logoutHandlers = Arrays.asList(logoutHandlers);
+ }
+

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

Please add another constructor that accepts a List. This will allow HttpServlet3RequestFactory to be cleaned up.

@rwinch

rwinch May 31, 2016

Member

Please add another constructor that accepts a List. This will allow HttpServlet3RequestFactory to be cleaned up.

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch May 31, 2016

Member

Thanks for the PR @eddumelendez! In general this looks pretty good. I provided some comments inline. Additionally, can we get unit tests for CompsiteLogoutHandler added?

Member

rwinch commented May 31, 2016

Thanks for the PR @eddumelendez! In general this looks pretty good. I provided some comments inline. Additionally, can we get unit tests for CompsiteLogoutHandler added?

@@ -58,6 +59,7 @@
private RequestMatcher logoutRequestMatcher;
private final List<LogoutHandler> handlers;
+ private CompositeLogoutHandler compositeLogoutHandler;

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

Please change the type to LogoutHandler

@rwinch

rwinch May 31, 2016

Member

Please change the type to LogoutHandler

@@ -68,6 +70,7 @@
private String expiredUrl;
private LogoutHandler[] handlers = new LogoutHandler[] { new SecurityContextLogoutHandler() };
private RedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
+ private CompositeLogoutHandler compositeLogoutHandler = new CompositeLogoutHandler(this.handlers);

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

Please change the type to LogoutHandler

@rwinch

rwinch May 31, 2016

Member

Please change the type to LogoutHandler

@@ -68,6 +70,7 @@
private String expiredUrl;
private LogoutHandler[] handlers = new LogoutHandler[] { new SecurityContextLogoutHandler() };

This comment has been minimized.

@rwinch

rwinch May 31, 2016

Member

You should be able to remove handlers.

@rwinch

rwinch May 31, 2016

Member

You should be able to remove handlers.

@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Jun 1, 2016

Contributor

@rwinch PR is updated. Comments have been fixed and new test was added to cover CompositeLogoutHandler.

Contributor

eddumelendez commented Jun 1, 2016

@rwinch PR is updated. Comments have been fixed and new test was added to cover CompositeLogoutHandler.

@pivotal-issuemaster

This comment has been minimized.

Show comment
Hide comment
@pivotal-issuemaster

This comment has been minimized.

Show comment
Hide comment
@pivotal-issuemaster

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Jun 21, 2016

Contributor

@rwinch any feedback about this?

Contributor

eddumelendez commented Jun 21, 2016

@rwinch any feedback about this?

+ "logoutHandlers")).size())
+ .isEqualTo(1);
+ }
+

This comment has been minimized.

@rwinch

rwinch Jun 21, 2016

Member

Can we add a test that ensures that the logoutHandlers that are injected are invoked when CompositeLogoutHandler.logout is invoked? This would likely work well with a mock.

We should also document what happens if an Exception happens in one of the LogoutHandlers (the remaining LogoutHandlers won't be invoked) and have a test to verify the documentation is correct.

@rwinch

rwinch Jun 21, 2016

Member

Can we add a test that ensures that the logoutHandlers that are injected are invoked when CompositeLogoutHandler.logout is invoked? This would likely work well with a mock.

We should also document what happens if an Exception happens in one of the LogoutHandlers (the remaining LogoutHandlers won't be invoked) and have a test to verify the documentation is correct.

@@ -81,6 +82,7 @@
private AuthenticationEntryPoint authenticationEntryPoint;
private AuthenticationManager authenticationManager;
private List<LogoutHandler> logoutHandlers;
+ private CompositeLogoutHandler compositeLogoutHandler;

This comment has been minimized.

@rwinch

rwinch Jun 21, 2016

Member

It looks like this type could be LogoutHandler

@rwinch

rwinch Jun 21, 2016

Member

It looks like this type could be LogoutHandler

+import org.springframework.security.core.Authentication;
+import org.springframework.util.Assert;
+
+/**

This comment has been minimized.

@rwinch

rwinch Jun 21, 2016

Member

We should have some Javadoc. You can also add the @since 4.2 tag as this will be targeted for Spring Security 4.2

@rwinch

rwinch Jun 21, 2016

Member

We should have some Javadoc. You can also add the @since 4.2 tag as this will be targeted for Spring Security 4.2

@rwinch

This comment has been minimized.

Show comment
Hide comment
Member

rwinch commented Jul 7, 2016

@eddumelendez

This comment has been minimized.

Show comment
Hide comment
@eddumelendez

eddumelendez Jul 8, 2016

Contributor

@rwinch sorry for the delay. PR updated!

Contributor

eddumelendez commented Jul 8, 2016

@rwinch sorry for the delay. PR updated!

@rwinch

This comment has been minimized.

Show comment
Hide comment
@rwinch

rwinch Jul 8, 2016

Member

@rwinch sorry for the delay. PR updated!

@eddumelendez No problem...just was not like you since you are always on top of things :)

This is now merged into master via 1effc18 I also applied a bit of polish via 70787fc

NOTE: I modified the tests so that they weren't dependent on the name of the private variable. I'd prefer to keep tests verifying behavior rather than if a member variable is set or not. This is something that is no consistent w/ the codebase, but I'd like to keep it this way going forward.

Member

rwinch commented Jul 8, 2016

@rwinch sorry for the delay. PR updated!

@eddumelendez No problem...just was not like you since you are always on top of things :)

This is now merged into master via 1effc18 I also applied a bit of polish via 70787fc

NOTE: I modified the tests so that they weren't dependent on the name of the private variable. I'd prefer to keep tests verifying behavior rather than if a member variable is set or not. This is something that is no consistent w/ the codebase, but I'd like to keep it this way going forward.

@rwinch rwinch added this to the 4.2.0 M1 milestone Jul 8, 2016

@rwinch rwinch self-assigned this Jul 8, 2016

@rwinch rwinch closed this Jul 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment