Allow customization of error and HTTP response within OAuth2AuthenticationFailureHandler

[ddubson]

This is an initial proof-of-concept for the last segment of gh-1369, as stated in the original:

For the second iteration, let's see if we can come up with a hook in the new AuthenticationFailureHandler that will allow for reuse and customization for these additional Filter's:

OAuth2ClientAuthenticationFilter
OidcUserInfoEndpointFilter
OidcClientRegistrationEndpointFilter

There are currently failing existing test cases, and no additional new test cases, as this draft PR is meant for early review of the approach.

Once the rough sketch is approved for this implementation, the TODO list is as follows:

• Fix failing existing test cases
• Add test coverage for the newly implemented functionality
• Polish and document newly added functions or blocks

Fixes gh-1369

[ddubson]

@jgrandja -- I'd like to get your input as to the approach for this implementation, let me know if you need further clarification on any part of this draft.

[jgrandja]

Thanks for the PR @ddubson. Please see review comments.

[jgrandja]

I'm not sure how I feel about using a BiConsumer. Currently, the framework does not make use of BiConsumer. I'll have to think about this for a bit.

[ddubson]

Will a Context class work in this situation? Now that I've learned about that pattern, I can see it fitting in here, however I'm not sure if the project uses it in handling error conditions as this warrants.

[jgrandja]

I don't think it makes sense to use a context class here.

I've been giving this some more thought and I'm starting to wonder if any changes are needed after all.

We have the ability to customize the error response parameters as mentioned in this comment, so the only other customization required at this point is to change the status code. However, this can be done using this:

private AuthenticationFailureHandler authenticationFailureHandler = createAuthenticationFailureHandler();

private static AuthenticationFailureHandler createAuthenticationFailureHandler() {
	OAuth2ErrorHttpMessageConverter errorResponseConverter = new OAuth2ErrorHttpMessageConverter() {
		@Override
		protected void writeInternal(OAuth2Error error, HttpOutputMessage outputMessage)
				throws HttpMessageNotWritableException {

			HttpServletResponse servletResponse = ((ServletServerHttpResponse) outputMessage).getServletResponse();
			if (OAuth2ErrorCodes.INVALID_CLIENT.equals(error.getErrorCode())) {
				servletResponse.setStatus(HttpStatus.UNAUTHORIZED.value());
			} else {
				servletResponse.setStatus(HttpStatus.BAD_REQUEST.value());
			}
			super.writeInternal(error, outputMessage);
		}

	};
	errorResponseConverter.setErrorParametersConverter(error -> {
		Map<String, String> parameters = new HashMap<>();
		parameters.put(OAuth2ParameterNames.ERROR, error.getErrorCode());
		return parameters;

	});

	OAuth2ErrorAuthenticationFailureHandler authenticationFailureHandler = new OAuth2ErrorAuthenticationFailureHandler();
	authenticationFailureHandler.setErrorResponseConverter(errorResponseConverter);

	return authenticationFailureHandler;
}

This AuthenticationFailureHandler would work for OAuth2ClientAuthenticationFilter. I know it's not pretty though.

I'm just having 2nd thoughts on introducing a new API just so we can customize the status code. Furthermore, no one from the community has asked for this specific customization. I realize this is my ask in the hopes of reusing OAuth2ErrorAuthenticationFailureHandler for the other 3x Filter's.

What are your thoughts?

[ddubson]

If there's little to no demand from the developer/consumer side, I think a good course of action is to document the approach you mentioned above as a way to do the error customization somewhere within the official docs -- that may very well be useful to those that need it in the future, without spending time and effort for an API change.

Additionally, a test case demonstrating this customization approach could also be useful in tandem with the docs change.

[jgrandja]

I think a good course of action is to document the approach you mentioned above as a way to do the error customization somewhere within the official docs -- that may very well be useful to those that need it in the future, without spending time and effort for an API change.

Fully agree and good idea on adding the sample customization to the reference. For now, I added the sample code to gh-541 as it should be added as part of that How-to guide.

I'm going to close this PR as we'll hold off on any API changes at this point. Thank you for spending your time on coming up with the right approach 👍