Summary

I don't understand why AbstractAuthenticationProcessingFilter is initializing SecurityContextHolder after invoking remaining filters in the security chain.

Actual Behavior

AbstractAuthenticationProcessingFilter https://github.com/spring-projects/spring-security/blob/4f3072b3d9c6bf07b00b8ec050fff81123594910/web/src/main/java/org/springframework/security/web/authentication/AbstractAuthenticationProcessingFilter.java#L216 initializes SecurtyContextHolder in method successfulAuthentication but only after invoking remaining filters in the chain via chain.doFilter(request, response); This makes it impossible for the next filters in the chain to make use of SecurtyContextHolder with authenticated user. For example AbstractSecurityInterceptor cannot make any decision because AnonymousAuthenticationToken is used instead...

Expected Behavior

I would expect that SecurityContextHolder is initialized before executing proceeding with the chain so that later filters in the chain like AbstractSecurityInterceptor could relay on user already authenticated. This is really important for REST API where there is not any redirection after successful authentication.

I would suggest that there is a flag to init SecurtyContextHolder before proceeding with the security chain like 


private void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
            throws IOException, ServletException {
        if (!requiresAuthentication(request, response)) {
            chain.doFilter(request, response);
            return;
        }
        try {
            Authentication authenticationResult = attemptAuthentication(request, response);
            if (authenticationResult == null) {
                // return immediately as subclass has indicated that it hasn't completed
                return;
            }
            this.sessionStrategy.onAuthentication(authenticationResult, request, response);
            // Authentication success
                        // --------------------------- Suggested change
            if (this.continueChainBeforeSuccessfulAuthentication) {
                chain.doFilter(request, response);
                                successfulAuthentication(request, response, chain, authenticationResult);
            } else if (this.continueChainAFTERSuccessfulAuthentication) {
                               successfulAuthentication(request, response, chain, authenticationResult);
                               chain.doFilter(request, response);
            } else {
                               successfulAuthentication(request, response, chain, authenticationResult);
                       }
                    //------------------------------------
        }
        catch (InternalAuthenticationServiceException failed) {
            this.logger.error("An internal error occurred while trying to authenticate the user.", failed);
            unsuccessfulAuthentication(request, response, failed);
        }
        catch (AuthenticationException ex) {
            // Authentication failed
            unsuccessfulAuthentication(request, response, ex);
        }
    }

Version

Spring Security 5.6.1

Comment From: sjohnr

Hi @pbartoszek, welcome to the project!

The AbstractAuthenticationProcessingFilter is an older component, and newer components typically focus on delegation instead of inheritance. Having said that, I believe this functionality would be easy to achieve by extending the AbstractAuthenticationProcessingFilter as in the following example:

public abstract class CustomAuthenticationProcessingFilter extends AbstractAuthenticationProcessingFilter {
    protected CustomAuthenticationProcessingFilter(String defaultFilterProcessesUrl) {
        super(defaultFilterProcessesUrl);
    }

    @Override
    protected void successfulAuthentication(HttpServletRequest request, HttpServletResponse response, FilterChain chain, Authentication authResult) throws IOException, ServletException {
        super.successfulAuthentication(request, response, chain, authResult);
        chain.doFilter(request, response);
    }
}

Note: I believe this could also work on concrete implementations, such as UsernamePasswordAuthenticationFilter as well.

Could you explain more about your use case, and whether this suggestion could work instead of modifying the base implementation directly?

Comment From: pbartoszek

Hi @sjohnr,

Thanks for suggestions and I think it should do the trick. My use case is stateless REST API.. so after successful authentication there should not be any extra http redirect etc... and I assume design philosophy behind AbstractAuthenticationProcessingFilter was that after successful authentication a redirect follows so that whole security chain can be re-evaluated on a new request which context holder already set up.....

Actually I am implementing authentication based on header api key... very common for rest api... I assume Spring must have a easier way to configure this...

UPDATE I realised that AbstractPreAuthenticatedProcessingFilter might be something I was looking for :)

Comment From: sjohnr

Hi @pbartoszek. Thanks for your response!

Yes, AbstractPreAuthenticatedProcessingFilter is a possible candidate for re-use in this case. Though in a personal project, I recently set up the BearerTokenAuthenticationFilter (from oauth2-resource-server) with a custom AuthenticationProvider to perform api-key based authentication, and it worked very well so I'd recommend taking a look at that as well. If you have any questions about it, you can ask here or link to a stackoverflow question and I'd be happy to take a look.

I think given the ease of the suggested workaround, I'm going to close this issue for now. But if others have a strong need for this change, feel free to comment and we can re-open this issue if necessary.