Skip to content

JwtIssuerValidator handles issuer (iss) claim values as Strings and URLs #9137

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

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

cmouttet
Copy link
Contributor

  • NimbusJwtDecoder uses claim set converters: issuer claim is converted to an URL object
  • JwtIssuerValidator (created by JwtValidators.createDefaultWithIssuer(String)) wraps a JwtClaimValidator
  • because of different data types, equal() is always false

This change allows both Strings and URLs as values of the issuer

Issue gh-9136

@pivotal-issuemaster
Copy link

@cmouttet Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@cmouttet Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 17, 2020
@cmouttet cmouttet force-pushed the gh-9136 branch 4 times, most recently from f1c1679 to 2dc4e76 Compare October 17, 2020 14:39
Copy link
Contributor

@jzheaux jzheaux 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, @cmouttet, and for your patience while I was out of the office.

I've left one piece of feedback inline.

Also, will you please make sure that your comment uses "Closes gh-9136" instead of "Issue gh-9136"? If you do that, then GitHub will link this PR to that issue and also close the ticket when the PR is merged.


/**
* Constructs a {@link JwtIssuerValidator} using the provided parameters
* @param issuer - The issuer that each {@link Jwt} should have.
*/
public JwtIssuerValidator(String issuer) {
Assert.notNull(issuer, "issuer cannot be null");
this.validator = new JwtClaimValidator(JwtClaimNames.ISS, issuer::equals);

Predicate<Object> testClaimValueURL = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you already considered

Predicate<Object> testClaimValue = (claimValue) -> 
        claimValue != null && issuer.equals(claimValue.toString())

as it simplifies the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Josh, many thanks for your feedback. I simplified the logic as you suggested and pushed a fix-up commit. After the checks succeed I'll squash the commits and change the commit comment having "Closes ...".

- NimbusJwtDecoder uses claim set converters: issuer claim is converted to an URL object
- JwtIssuerValidator (created by JwtValidators.createDefaultWithIssuer(String)) wraps a JwtClaimValidator<String>
- because of different data types, equal() is always false

This change allows both Strings and URLs as values of the issuer

Closes spring-projectsgh-9136
@jzheaux jzheaux merged commit 6486857 into spring-projects:master Oct 28, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Oct 28, 2020

Thanks, @cmouttet! This is now merged.

@jzheaux jzheaux self-assigned this Nov 4, 2020
@jzheaux jzheaux added this to the 5.5.0-M1 milestone Nov 4, 2020
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 4, 2020
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: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants