Introduce Customizable AuthorizationFailureHandler in OAuth2AuthorizationRequestRedirectFilter

[leewin12]

This PR introduces a new AuthorizationFailureHandler to OAuth2AuthorizationRequestRedirectFilter, Customizable AuthorizationFailureHandler in OAuth2AuthorizationRequestRedirectFilter (Related to gh-13793).

Since most failure handlers in Spring Security focus on AuthenticationException, I'm proposing a new failure handler specifically for the OAuth2AuthorizationRequestRedirectFilter to address a wider range of errors.

I welcome any alternative solutions or improvements you might suggest. I will start working on the unit tests once this approach is confirmed.

[sjohnr]

Hi @leewin12, thanks for the PR!

My initial feedback is that I would prefer not to introduce a sub-interface into the filter for this case. Additionally, re-reading your issue description:

Allow users to receive a more descriptive custom error message or be redirected to a specific error URL when an invalid client registration is provided during the OIDC process.

I'm not sure I am clear on how introducing a failure handler specifically solves the first problem ("Allow users to receive a more descriptive custom error message"). Can you describe how you would do this with a failure handler that differs from what we could do by wrapping and delegating to the existing OAuth2AuthorizationRequestResolver? Or are you only trying to solve the second problem ("Allow users to [...] be redirected to a specific error URL when an invalid client registration is provided during the OIDC process")?

Depending on your answer to the above, I think we should consider simply re-using AuthenticationFailureHandler. We can wrap the existing exception for an invalid registration id in an AuthenticationException of some kind.

[leewin12]

Hi @sjohnr, Happy New Year! Wishing you and all the Spring Security contributors a great year ahead.

I'm not sure I am clear on how introducing a failure handler specifically solves the first problem ("Allow users to receive a more descriptive custom error message"). Can you describe how you would do this with a failure handler that differs from what we could do by wrapping and delegating to the existing OAuth2AuthorizationRequestResolver? Or are you only trying to solve the second problem ("Allow users to [...] be redirected to a specific error URL when an invalid client registration is provided during the OIDC process")?

I realize I may have conflated two different concepts: the users of Spring Security (developers) and the end-users of services built using Spring Security. To clarify, the first problem is intended to provide more descriptive custom error messages for end-users.

I initially thought that developers could use the new AuthorizationFailureHandler to customize actions for failures, such as providing more detailed error messages, redirecting to specific pages, or returning specific HTTP error codes.

However, wrapping and delegating to the existing OAuth2AuthorizationRequestResolver does not fully address this because the OAuth2AuthorizationRequestRedirectFilter consistently sends an INTERNAL_SERVER_ERROR for every exception from the OAuth2AuthorizationRequestResolver.

Depending on your answer to the above, I think we should consider simply re-using AuthenticationFailureHandler. We can wrap the existing exception for an invalid registration id in an AuthenticationException of some kind.

Regarding your suggestion to reuse AuthenticationFailureHandler, I agree it's a better idea. My hesitation was due to OAuth2AuthorizationRequestResolver.resolve() not throwing an AuthenticationException. I'm uncertain about using AuthenticationException without modifying the OAuth2AuthorizationRequestResolver.resolve() interface to include throws AuthenticationException. Although it is an unchecked exception, my understanding is that most Spring Security interfaces declare throws AuthenticationException quite precisely. Changing the interface to OAuth2AuthorizationRequestResolver.resolve() throws AuthenticationException seems feasible for the build, but I'm unsure about potential issues in reflection or other areas that I might not be aware of.

[sjohnr]

@leewin12 I apologize for the delay in response! 😬 It has been very busy the last several weeks.

Regarding your suggestion to reuse AuthenticationFailureHandler, I agree it's a better idea. My hesitation was due to OAuth2AuthorizationRequestResolver.resolve() not throwing an AuthenticationException.

I have pushed a branch with my idea for better illustration (see this commit). It wraps the thrown exception received from the process of resolving and redirecting (whatever that Exception may be) for the purposes of re-using the AuthenticationFailureHandler interface.

Whether this exception is authentication-related or not, I think we'll find as many reasons not to want/need a new interface just for this error as well. I also don't see a problem with needing to introduce something new later, as it would be an easy deprecation and replacement with a new type of error handler if needed (which I suspect won't ever be necessary). I also think this will handle both of your use cases. What do you think?

[sjohnr]

Hi @leewin12, did you have a chance to see the branch I pushed (see above)?

[leewin12]

Hi @leewin12, did you have a chance to see the branch I pushed (see above)?

Hi @sjohnr Thank you for remind and guidance, I missed the notification. I will re-work my pr based on your guidance.

[willemvd]

I was also creating a new branch based on your work @sjohnr but having trouble with the ./gradlew format check on a fresh fork from the main (raised #14575 for that). Are you able to finish this issue @leewin12 or do you mind that I'm picking this up?

[leewin12]

I was also creating a new branch based on your work @sjohnr but having trouble with the ./gradlew format check on a fresh fork from the main (raised #14575 for that). Are you able to finish this issue @leewin12 or do you mind that I'm picking this up?

@willemvd Yes, I am able to finish this issue. thanks you for your professional courtesy. I will notify you if I am unable to do so.

[leewin12]

@sjohnr pr is ready for review. I've just added some test code, thanks to your guidance. Please take a look and feel free to let me know if you need any updates.

[willemvd]

@leewin12 thank for picking it up!

I looked at your PR and I think it is missing configuration so a developer can override the AuthenticationFailureHandler from the OAuth2LoginConfigurer like we can also override the authorizationRequestResolver for example. This would also impact the OAuth2LoginConfigurer.AuthorizationEndpointConfig and AuthorizationEndpointDsl.

Another thing that I would like to change, is to make the OAuth2AuthorizationRequestException not a private inner class, but a fully new public class (in the package org.springframework.security.oauth2.client.web). This makes it possible for developers to handle the OAuth2AuthorizationRequestException in their own AuthenticationFailureHandler.

In that sense, also the InvalidClientRegistrationIdException should be change from package-private class to a public class. @sjohnr is that something you also agree upon?

[leewin12]

@willemvd I've agreed with your review and have applied all the points that you mentioned. Please take a look and feel free to share your thoughts.

[willemvd]

@leewin12 thanks for the changes! the only thing I'm missing is part 2 of my comment:

Another thing that I would like to change, is to make the OAuth2AuthorizationRequestException not a private inner class, but a fully new public class (in the package org.springframework.security.oauth2.client.web). This makes it possible for developers to handle the OAuth2AuthorizationRequestException in their own AuthenticationFailureHandler.

[leewin12]

@willemvd oops, my mistake. I've just fixed it. Thank you once again for your review!!

[willemvd]

👍 LGTM! 😀

[willemvd]

@sjohnr , gentle reminder, can you also have a look? 😄

[sjohnr]

@sjohnr , gentle reminder, can you also have a look? 😄

@willemvd @leewin12 apologies, I have been out on vacation for 2 weeks. I have not yet had a chance to closely review the PR but will add it to my list for the week. Let me first respond to the comments you added.

I looked at your PR and I think it is missing configuration so a developer can override the AuthenticationFailureHandler from the OAuth2LoginConfigurer like we can also override the authorizationRequestResolver for example. This would also impact the OAuth2LoginConfigurer.AuthorizationEndpointConfig and AuthorizationEndpointDsl.

Generally, I think we would wait on this type of change until it is deemed necessary/asked for. We prefer to keep the DSL minimal and not introduce unnecessary additions. In the meantime, you can set the new authentication failure handler by using the setter directly on the OAuth2AuthorizationRequestRedirectFilter and register your own filter manually, or you can register an ObjectPostProcessor to modify the filter added by the configurer. Make sense?

Another thing that I would like to change, is to make the OAuth2AuthorizationRequestException not a private inner class, but a fully new public class (in the package org.springframework.security.oauth2.client.web). This makes it possible for developers to handle the OAuth2AuthorizationRequestException in their own AuthenticationFailureHandler.

In that sense, also the InvalidClientRegistrationIdException should be change from package-private class to a public class. @sjohnr is that something you also agree upon?

I do not prefer to open up classes to the public API unless absolutely necessary, and I don't currently see a reason to do so. For context, the reason the private class InvalidClientRegistrationIdException was added was to specifically provide a WARN message in the logs (without throwing a new type of exception) when the default exception handling in the framework is used. Remember that both the authorizationRequestResolver and new authenticationFailureHandler can be easily customized, which should be the preferred way to set up custom exception handling interactions between these two components.

[willemvd]

Generally, I think we would wait on this type of change until it is deemed necessary/asked for. We prefer to keep the DSL minimal and not introduce unnecessary additions. In the meantime, you can set the new authentication failure handler by using the setter directly on the OAuth2AuthorizationRequestRedirectFilter and register your own filter manually, or you can register an ObjectPostProcessor to modify the filter added by the configurer. Make sense?

My thoughts were that we can set it but from the DSL to keep it in line with the other setters, since doing it with manually registering or via the ObjectPostProcessor is more "complex" way (or not in line with setting the other fields) of setting it from an application perspective.

I do not prefer to open up classes to the public API unless absolutely necessary, and I don't currently see a reason to do so. For context, the reason the private class InvalidClientRegistrationIdException was added was to specifically provide a WARN message in the logs (without throwing a new type of exception) when the default exception handling in the framework is used. Remember that both the authorizationRequestResolver and new authenticationFailureHandler can be easily customized, which should be the preferred way to set up custom exception handling interactions between these two components.

Ok, agree!

[sjohnr]

My thoughts were that we can set it but from the DSL to keep it in line with the other setters, since doing it with manually registering or via the ObjectPostProcessor is more "complex" way (or not in line with setting the other fields) of setting it from an application perspective.

You are correct of course that they are more complex. The reasoning for waiting to update the DSL is that adding more methods to the DSL makes it more complex as well. So if very few users need to set this new field, we impact fewer users by leaving it off the DSL. I think the best course would be to simply file a new issue for adding it to the DSL and see if anyone else has interest. If so, we can easily add it as a separate PR. If nothing else, it keeps this PR focused on just one change.

[leewin12]

@sjohnr Thank you for sharing your review. I have no objections to two of the subjects and will revisit them within this week. @willemvd I believe the added complexity is worth it. Let's discuss this matter in a separate PR in the future.

[leewin12]

@sjohnr I remove those commits for supporting DSL and opening the private class. Please let me know if you need any updates.

[sjohnr]

Thanks @leewin12! This is now merged as 07ac0b616b8ca68a033a74a559f05d42864d2bad.

[leewin12]

@sjohnr I am so glad to contribute. thanks to your guidance. I would like ask for this feature to be backport so I can use it in my daily task with an older version of spring security.

[sjohnr]

Thanks for contributing, @leewin12!

I would like ask for this feature to be backport so I can use it in my daily task with an older version of spring security.

We do not generally backport enhancements. We usually recommend that you copy the source of the class into your own project and use it in the interim, if you need to use it prior to upgrading. To register a customized version of the class as a filter, you will also need to register an ObjectPostProcessor to replace the framework supplied instance with your own.

[leewin12]

@sjohnr okay, thank you for the advise. I am glad to contribute to spring-security.