Allow override AbstractPreAuthenticatedProcessingFilter#requiresAuthentication

[ttddyy]

Hi,

I'd like to add more precheck logic on AbstractPreAuthenticatedProcessingFilter#requiresAuthentication. (Currently, it is a private method)
Similar class AbstractAuthenticationProcessingFilter has requiresAuthentication as a protected method.

Making the method protected allows sub-classes to extend the precondition check.

Thanks,

[rwinch]

Thanks for the suggestion. Please change this to use a RequestMatcher with the default implementation being private and using the same existing logic. This is preferable since Spring prefers composition to inheritance.

[ttddyy]

@rwinch

Updated the PR to use RequestMatcher in AbstractPreAuthenticatedProcessingFilter
Thanks,

[rwinch]

Thanks for the fast turnaround. See my comments for areas of concern.

[rwinch]

I'm interested in discussing this approach where it augments the existing logic for when authentication is required vs being the entire logic (i.e. the default implementation of RequestMatcher copies the logic from this method). What are your thoughts on each approach?

My concern about augmenting the existing logic is that there is no way to change the existing logic.

[ttddyy]

You mean performing request matcher check in requiresAuthentication() method vs performing the check in doFilter(), so that entire logic of this filter may be skipped if request doesn't match?

public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
    throws IOException, ServletException {
    
    if(!requiresAuthenticationRequestMatcher.matches(request)){
      chain.doFilter(request, response);
      return;
    }
    
    // existing logic from here
    
    if (!requiresAuthentication(request, response)) {
      chain.doFilter(request, response);
      return;
    }
    ...
}

Currently, requiresAuthentication() method on this class AbstractPreAuthenticatedProcessingFilter (APAPF) is private, so where to put the request-matcher check doesn't really affect any other existing sub classes. So, behavior-wise it is same.

The similar class AbstractAuthenticationProcessingFilter(AAPF), the request-matcher check is in requiresAuthentication() which is a protected method.

So, to align the implementation with AAPF, then request-matcher check is in requiresAuthentication().
If we consider request-matcher check is the first-check even before requiresAuthentication(), then the check can go into doFilter()(shown in above snippet), and maybe update the AAPF as well (updating AAPF will be a behavioral change).

My preference is put the request-matcher check in requiresAuthentication() as in current PR which aligns with AAPF, then also make requiresAuthentication() protected, so that sub-class can modify the behavior, even it is inheritance. (Yes, I know spring is for composition, but since this is an abstract class which is meant to be inherited and may be overridden.)

My concern about augmenting the existing logic is that there is no way to change the existing logic.

Currently the method is private, adding all-match request-matcher should have no impact to existing logic.

[rwinch]

I understand that it won't have an impact on users. My concern is that as it is written now, a user cannot ignore the existing logic. For example, what if they don't want it to require authentication if the current user is null when requesting the URL /foo/bar? This would not be possible as implemented in this PR. So my suggestion was to move all of this to the default RequestMatcher implementation so that users can customize anything they want.

[ttddyy]

So, logic can be moved to RequestMatcher implementation.

private class PreAuthenticatedProcessingRequestMatcher implements RequestMatcher {

  @Override
  public boolean matches(HttpServletRequest request) {

    Authentication currentUser = SecurityContextHolder.getContext().getAuthentication();

    if (currentUser == null) {
      return true;
    }

    if (!checkForPrincipalChanges) {
      return false;
    }

    if (!principalChanged(request, currentUser)) {
      return false;
    }

    logger.debug("Pre-authenticated principal has changed and will be reauthenticated");

    if (invalidateSessionOnPrincipalChange) {
      SecurityContextHolder.clearContext();

      HttpSession session = request.getSession(false);

      if (session != null) {
        logger.debug("Invalidating existing session");
        session.invalidate();
        request.getSession();
      }
    }

    return true;
  }

}

and caller side is:

private boolean requiresAuthentication(HttpServletRequest request) {
  return authenticationRequestMatcher.matches(request);
}

Though, in the matcher logic, there are still references:
checkForPrincipalChanges, invalidateSessionOnPrincipalChange variables, and principalChanged method.

two variables can be passed to matcher, and principalChanged method can be also moved to matcher (though it is currently a protected method).

Is this what you mean?

[rwinch]

Yes. Since the default implementation is internal it would still be able to refer to the member variables.

[ttddyy]

OK, updated the PR.
Moved the existing auth check logic into the new request matcher.

[rwinch]

Thanks for the updates. I have provided additional feedback.

[ttddyy]

Updated the PR and reflected the feedbacks

[rwinch]

Thanks for the fast turnaround! Looks good to me. I'm going to wait for the build to complete before merging into master

[eleftherias]

Thanks for the PR @ttddyy! This is now merged into master.