Spring Security
  1. Spring Security
  2. SEC-1249

AbstractPreAuthenticatedProcessingFilter should better support continueFilterChainOnUnsuccessfulAuthentication

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.4
    • Fix Version/s: 3.0.0 RC1
    • Component/s: Web
    • Labels:
      None

      Description

      AbstractPreAuthenticatedProcessingFilter helpfully has continueFilterChainOnUnsuccessfulAuthentication. However, the try/catch that enables the continue is not broad enough. The methods getPreAuthenticatedPrincipal() and getPreAuthenticatedCredentials() are not covered, so if, for example, when using the request header preauth and the preauth header (SM_USER or otherwise) is not found, the filter chain does not continue; you get an auth failure instead because it throws PreAuthenticatedCredentialsNotFoundException.

      The behavior I'm looking for is if AbstractPreAuthenticatedProcessingFilter.doAuthenticate() fails, the filter chain continues, whether it's getPreAuthenticatedPrincipal() or getPreAuthenticatedCredentials() or authenticationManager.authenticate() that fails.

      I don't know if you want to just extend the try/catch to cover the two get methods or have some separate configuration property, but since you already support a "continue" configuration it'd be nice to support continuing on any failure in doAuthenticate().

        Issue Links

          Activity

          Hide
          Ray Suliteanu added a comment -

          Or have all subclasses that implement getPreAuthenticatedPrincipal() return null as AbstractPreAuthenticatedProcessingFilter seems to expect. It seems like the "contract" is not being kept by all implementations, so if you just enhance them to return null vs. throw exception that would work too. Thanks.

          Show
          Ray Suliteanu added a comment - Or have all subclasses that implement getPreAuthenticatedPrincipal() return null as AbstractPreAuthenticatedProcessingFilter seems to expect. It seems like the "contract" is not being kept by all implementations, so if you just enhance them to return null vs. throw exception that would work too. Thanks.
          Hide
          Luke Taylor added a comment -

          I believe the only implementation that doesn't return null if it fails to obtain a principal is the RequestHeaderAuthenticationFilter. This is deliberate as a missing header is potentially a dangerous configuration error. It is intended to fail by default.

          Show
          Luke Taylor added a comment - I believe the only implementation that doesn't return null if it fails to obtain a principal is the RequestHeaderAuthenticationFilter. This is deliberate as a missing header is potentially a dangerous configuration error. It is intended to fail by default.
          Hide
          Luke Taylor added a comment -

          You should now be able to override the behaviour of RequestHeaderAuthenticationFilter, as a result of SEC-1250, so I don't think any additional changes are necessary.

          Show
          Luke Taylor added a comment - You should now be able to override the behaviour of RequestHeaderAuthenticationFilter, as a result of SEC-1250 , so I don't think any additional changes are necessary.

            People

            • Assignee:
              Luke Taylor
              Reporter:
              Ray Suliteanu
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: