-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix OAuth2TokenIntrospection issuer validation and add unit tests #2206
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
Fix OAuth2TokenIntrospection issuer validation and add unit tests #2206
Conversation
Signed-off-by: Ramesh <ramesh200212@gmail.com>
9058cf9
to
2e4f2ed
Compare
try { | ||
new URI(url.toString()).toURL(); | ||
// Try parsing as URI | ||
new URI(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there really a benefit from first trying to parse a URI, and if this fails, still continue successfully? IMHO it would be enough to just check if the string is blank or not. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback You’re right — if we allow non-URI strings (like client IDs), then attempting to parse as a URI may not add much value.
My initial thought behind keeping the URI parsing was to still preserve compatibility with tokens where iss is a proper URI, and not reject them unintentionally. But since the validation already ensures the string isn’t blank, simplifying to just a non-blank check would be sufficient and cleaner.
I can update the implementation to only validate that iss is not blank. Would you prefer that direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would prefer simplicity. Compatibility is still preserved with such a simpler approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make the changes
5b777dd
to
dbd464d
Compare
Hey matt can you please review changes |
I like that. It would be even better to also slightly adjust the error message to something like "iss must not be empty" |
… must not be empty\ Signed-off-by: Ramesh <ramesh200212@gmail.com>
dbd464d
to
587d526
Compare
I have made the changes please review it |
Thanks @MatthiasDrewsCS for the review and approval |
Closing as per comment |
Summary
This PR updates the OAuth2TokenIntrospection class to correctly handle the 'issuer' (iss) claim. The changes ensure that the issuer can be either a valid URI or a plain string without throwing exceptions during token introspection validation.
Changes
OAuth2TokenIntrospection.Builder
to accept non-URI issuer strings.validateIssuer
method to allow plain string issuers while still supporting valid URIs.Benefits