Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SEC-1249: AbstractPreAuthenticatedProcessingFilter should better support continueFilterChainOnUnsuccessfulAuthentication #1499

Closed
spring-issuemaster opened this Issue Sep 25, 2009 · 5 comments

Comments

Projects
None yet
2 participants

Ray Suliteanu (Migrated from SEC-1249) said:

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().

Ray Suliteanu said:

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.

Luke Taylor said:

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.

Luke Taylor said:

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.

@spring-issuemaster spring-issuemaster added this to the 3.0.0 RC1 milestone Feb 5, 2016

This issue is related to #1500

I would prefer if authentication exceptions in the getPreAuthenticatedPrinciple call of AbstractPreAuthenticatedProcessingFilter.doAuthenticate were handled by the AbstractPreAuthenticatedProcessingFilter.unsuccessfulAuthentication method. That way errors in the RequestHeaderAuthenticationFilter.getPreAuthenticatedPrinciple or any other implementation would be handled in a way that is consistent with other errors. Currently a 500 error is returned rather than a 400 Bad Request which I would expect.

Currently the only way to override the current functionality is to override doFilter and wrap the super call in a try catch.

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