Spring Security Remember me login fired multiple times

Summary

We're handling the InteractiveAuthenticationSuccessEvent to determine when a user logged in. However, this event is getting fired multiple times, causing unexpected behavior in our application. It seems when accessing a page requiring full authentication, the security context is cleared in the ExceptionTranslationFilter, causing a partially authenticated user to be in effect logged out. The user will then be redirected to the login page, where remember me login kicks in again (the cookie is still present). Seems to be a side effect of the fix for this issue.

Actual Behavior

User accesses any page. Remember-me kicks in and user is logged in automatically. InteractiveAuthenticationSuccessEvent is published.
User accesses page requiring full authentication and is redirected to login page. Remember-me kicks in again and InteractiveAuthenticationSuccessEvent is published. Login page itself is not the issue, since remember-me will kick in regardless of which page we redirect to.

Expected Behavior

InteractiveAuthenticationSuccessEvent should be fired only once, in step 1.
Logout/login should not be repeated in circles behind the scenes.

Configuration

A mix of authenticated and fullyAuthenticated urls. Persistent token based remember-me enabled:

    .and().rememberMe()
        .key(REMEMBER_ME_CRYPTO_KEY)
        .alwaysRemember(true)
        .tokenValiditySeconds(REMEMBER_ME_TOKEN_VALIDITY)
        .userDetailsService(userDetailsService)
        .rememberMeServices(rememberMeServices)
        .tokenRepository(rememberMeTokenRepository)
        .useSecureCookie(true)

The overall configuration is too customized to describe here. If others haven't experienced/can't reproduce this issue, I will try to provide a sample. Just didn't have time at the moment.

Version

5.0.7.RELEASE

Comment From: rougou

As a temporary solution I have added a custom filter after the ExceptionTranslationFilter (in effect replacing it). I'd prefer not to have to do this, though. At the very least, the current authentication info shouldn't be cleared on InsufficientAuthenticationException. I'm not sure about the other exceptions.

public class MyExceptionTranslationFilter extends ExceptionTranslationFilter {
 
private AuthenticationEntryPoint authenticationEntryPoint;
private RequestCache requestCache;
 
public MyExceptionTranslationFilter(AuthenticationEntryPoint authenticationEntryPoint, RequestCache requestCache, AccessDeniedHandler accessDeniedHandler) {
     super(authenticationEntryPoint, requestCache);
     this.authenticationEntryPoint = authenticationEntryPoint;
     this.requestCache = requestCache;
     setAccessDeniedHandler(accessDeniedHandler);
}
 
@Override
protected void sendStartAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, AuthenticationException reason) throws ServletException, IOException {
     if ( ! (reason instanceof InsufficientAuthenticationException)) {
          // SEC-112: Clear the SecurityContextHolder's Authentication, as the
          // existing Authentication is no longer considered valid
          SecurityContextHolder.getContext().setAuthentication(null);
     }
 
     requestCache.saveRequest(request, response);
     logger.debug("Calling Authentication entry point.");
     authenticationEntryPoint.commence(request, response, reason);
}
}

Security config (relevant parts):

protected void configure(HttpSecurity http) throws Exception {
     RequestCache requestCache = new NullRequestCache(); // Spring default is HttpSessionRequestCache, but I handle redirects in a custom manner so have no need for it.
     MyExceptionTranslationFilter exceptionTranslationFilter = new MyExceptionTranslationFilter(authenticationEntryPoint(), requestCache, accessDeniedHandler());
...
     .and().requestCache().requestCache(requestCache)
...
     .addFilterAfter(exceptionTranslationFilter, ExceptionTranslationFilter.class)
...
}

Comment From: rwinch

Thanks for the report @rougou! A sample would be helpful to better understand the issue and find an appropriate fix.

Comment From: rougou

@rwinch I have forked and modified Spring's Securing a Web Application sample to demonstrate this issue: https://github.com/rougou/gs-securing-web/tree/sample5761. Be sure to checkout the sample5761 branch. I simply enabled remember me, added a page named hellofull that requires full authentication, and logged InteractiveAuthenticationSuccessEvent events in a class named LoginEventListener. Here are the only changes to the original sample.

To reproduce: - Launch application and visit http://localhost:8080 - Login via either link (user/password) - Delete JSESSIONID cookie (keep remember-me cookie) - Access http://localhost:8080/hellofull and notice "Remember me login success!" is logged twice (first on hellofull page and then again on login page. - Without signing in, access http://localhost:8080/hellofull repeatedly and notice "Remember me login success!" is logged each time.

Comment From: rwinch

The real problem seems to be that we try to clear out the SecurityContextHolder, but this is not sufficient to log the user out. Instead, we should trigger a logout. I sent a PR that works around the issue with minimal changes. Can you confirm a strategy like this works for you?

If so, we will likely invoke logout in ExceptionTranslationFilter to fix this issue.

Comment From: rougou

@rwinch No, sorry if I wasn't very clear. I need the user to stay logged in when they are "remember-me" logged in but need to be fully authenticated. Essentially I have a shopping application where the user needs to enter their password only to complete a purchase or change account information. If they got logged out every time full authentication is requested I'd lose track of which user it was in the first place.

Also, I think your PR would break the SavedRequestAwareAuthenticationSuccessHandler, which is the default strategy for redirecting after login - the saved request would be lost after the logout.

*I'm not sure if the current implementation affects the redirect URL since I have a custom AuthenticationSuccessHandler that doesn't rely on the saved request.

Comment From: rwinch

Thanks for the response and the clarification.

Ultimately your suggestion reintroduces the problem that is present in #373, so we cannot do that.

We will likely need to re-think how this is handled. I'm open to suggestions, but we do not want to cause the old issue to occur. We should also think about how multi factor authentication will work. For now I think the best bet is the custom ExceptionTranslationFilter.

Also, I think your PR would break the SavedRequestAwareAuthenticationSuccessHandler, which is the default strategy for redirecting after login - the saved request would be lost after the logout.

You are right this workaround would cause an issue here. However, this would not be an issue with the long term solution which invalidates the session before the RequestCache.

Comment From: rougou

@rwinch I think it makes sense to clear the authentication when an AuthenticationException occurs, as is the case with #373. However, it can be argued that this shouldn't happen for an AccessDeniedException.

I also noticed that the isRememberMe() case in ExceptionTranslationFilter was added in a pretty recent commit. So it was probably not an issue when #373 was resolved back in 2005.

So my proposal would be to not clear the authentication on an AccessDeniedException (or at the very least, not when remember-me authenticated). If for some reason it needs to be cleared, this could be done in a custom AuthenticationEntryPoint. This would also allow the custom AuthenticationEntryPoint to have access to the current authentication and possibly redirect to a more user friendly page if the user is already remember-me authenticated.

Either way, I'll stick with my custom filter for now, so no rush.

Comment From: rwinch

Thanks for the response.

I think your suggestion of removing the remember me check and requiring a custom AccessDeniedHandler to trigger authentication under some circumstances makes sense. This would likely be a good option for multifactor authentication too.

I'm going to think about this one for a bit. In the meantime, as you mention, there is a reasonable workaround.