Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable Exception for Invalid Client Registration ID in OAuth2AuthorizationRequestRedirectFilter #13793

Closed
leewin12 opened this issue Sep 12, 2023 · 3 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement

Comments

@leewin12
Copy link
Contributor

leewin12 commented Sep 12, 2023

Thank you for your continuous efforts in maintaining and improving spring security.
I would like to discuss a potential enhancement that could benefit many users.

Expected Behavior

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.

Current Behavior

  1. A user enters the Client Registration ID and initiates the OIDC login process on our SPA page.
  2. The SPA frontend constructs an OIDC initiation URL as https://{domain}/oauth2/authorization/{ClientRegistrationId}.
  3. The user's browser redirects to the URL mentioned and reaches DefaultOAuth2AuthorizationRequestResolver via OAuth2AuthorizationRequestRedirectFilter.
  4. If the ClientRegistrationId is invalid, DefaultOAuth2AuthorizationRequestResolver throws either an IllegalArgumentException (5.7.x) or an InvalidClientRegistrationIdException (5.8.x).
  5. OAuth2AuthorizationRequestRedirectFilter interrupts the filter chain and returns HttpStatus.INTERNAL_SERVER_ERROR. (code)
  6. The user is redirected to https://{domain}/error, triggering a white-label page.

Context

  1. Pre-validating the ClientRegistrationId cannot prevent this situation.
  2. In step (5), because OAuth2AuthorizationRequestRedirectFilter interrupts the filter chain, any additional error-handling filters become ineffective.
  3. Overriding OAuth2AuthorizationRequestRedirectFilter offers no benefit due to the private accessor of unsuccessfulRedirectForAuthorization.
  4. To the best of my knowledge, checking the ClientRegistrationId is not part of the OIDC process, so the framework might allow individual applications to set their own policies on this matter.
@leewin12 leewin12 added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Sep 12, 2023
@sjohnr
Copy link
Member

sjohnr commented Sep 28, 2023

@leewin12 thanks for the report.

I think the current intention of the framework is that such an invalid registrationId case would be a configuration error, and is not intended to be handled (it should instead be fixed).

Having said that, there is room for improving error handling in this area and making it more flexible. I believe it would be fairly challenging though, so it could require some iterating to come up with a solution. Do you have any ideas for something we could introduce to make it more customizable? Would you be interested in opening a PR with such changes when we get to that point? I'm happy to help work with you on it.

@sjohnr sjohnr self-assigned this Sep 28, 2023
@sjohnr sjohnr added status: waiting-for-feedback We need additional information before we can continue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 28, 2023
@leewin12
Copy link
Contributor Author

leewin12 commented Oct 1, 2023

Greetings @sjohnr ,

Thank you for your insights on this issue.

I'm indeed willing to open a PR; Please excuse that it might take several weeks due to my current schedule.

My initial thought is to introduce an error handling interface along with a default implementation. If you have any other ideas, or if you believe my approach might not align with the framework's direction, please let me know.

(I'd like to take this opportunity to express my gratitude to all the Spring Security framework contributors. Your efforts are deeply appreciated.)

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 1, 2023
@sjohnr
Copy link
Member

sjohnr commented Oct 2, 2023

(I'd like to take this opportunity to express my gratitude to all the Spring Security framework contributors. Your efforts are deeply appreciated.)

Thank you for the kind words! I'll pass on your appreciation to the team.

My initial thought is to introduce an error handling interface along with a default implementation.

My only thought at this point would be to suggest trying to use an existing interface before creating a new one. When you get closer to that point we can discuss possible options. We probably want to aim for more general solutions than ones specific to a single type of error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
Archived in project
Development

No branches or pull requests

3 participants