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

Do not validate parameters in ServerBearerTokenAuthenticationConverter and DefaultBearerTokenResolver if not enabled #16039

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jonah1und1
Copy link

@jonah1und1 jonah1und1 commented Nov 5, 2024

Closes issue: #16038

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 5, 2024
# Conflicts:
#	oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolver.java
#	oauth2/oauth2-resource-server/src/main/java/org/springframework/security/oauth2/server/resource/web/server/authentication/ServerBearerTokenAuthenticationConverter.java
#	oauth2/oauth2-resource-server/src/test/java/org/springframework/security/oauth2/server/resource/web/DefaultBearerTokenResolverTests.java
@sjohnr sjohnr self-assigned this Nov 5, 2024
@sjohnr sjohnr added type: bug A general bug 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 Nov 5, 2024
Copy link
Member

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @jonah1und1! I have provided some feedback on tests below.

@jonah1und1
Copy link
Author

Thanks for the feedback!
I incorporated your suggestions and added some code to fix failing tests in OAuth2ResourceServerConfigurerTests.java and OAuth2ResourceServerBeanDefinitionParserTests.java, please feel free to comment.

@jonah1und1 jonah1und1 requested a review from sjohnr November 8, 2024 08:00
@sjohnr
Copy link
Member

sjohnr commented Nov 12, 2024

Thanks for providing updates @jonah1und1! I wanted to let you know an update from my side as well: I spoke with a team member on our approach here to validate parameters up front. I realize I should have done this prior to responding to the original PR, as the feedback I received pointed out an alternative to this approach.

The alternative approach is to actually have multiple implementations of BearerTokenResolver instead of one implementation with multiple boolean flags. This would allow a delegating implementation that composes whichever options the user wants to enable. However, there would still be two strategies for composing multiple resolvers together that result in different outcomes:

  1. Try multiple delegates and find the first non-null returning one.
  2. Try all delegates and throw an error if more than one returns something. (similar to this PR)

Having the option to do one or the other of these is clearly more flexible, at the expense of requiring user-defined delegation logic. So we would need to provide one of the delegating patterns above as well, and would need to choose which one to provide. In any case, if we took this approach, we would have to go back to the drawing board on this PR.

Long story short, I think I'd like to pause on this for a bit and think about it more before deciding how to move forward. We don't want to make a change only to realize it was the wrong change, so I think it best to hold off and not merge anything into the upcoming release. I apologize for any inconvenience, and thank you for contributing what you have so far. Let me know if anything I said doesn't make sense.

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) type: bug A general bug
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants