Skip to content

Conversation

@ymajoros
Copy link

Closes 13185

@pivotal-cla
Copy link

@ymajoros 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-cla
Copy link

@ymajoros 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 May 16, 2023
@jzheaux jzheaux self-assigned this May 22, 2023
@jzheaux jzheaux added status: duplicate A duplicate of another issue type: enhancement A general enhancement 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 May 22, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Jun 5, 2023

@ymajoros, thank you very much for the PR. I don't think that it should be in JwtDecoders, but NimbusJwtDecoder. Also, it is missing the needed validation. Please see my comment about what needs to be done to add this support.

@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Jun 12, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Feb 21, 2025

@ymajoros, I don't think we should add this feature without also introducing validation. Allow me to elaborate on what I meant here.

I think it would be valuable for NimbusJwtDecoder's builders to turn off the Nimbus typ validation so that applications can better choose which validation types, JWT, at+jwt, etc. they want to support.

The builders could change like so:

NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();

Then, we can change JwtValidators to add typ validation by default similar to Nimbus, allowing the following:

NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();
decoder.setJwtValidator(JwtValidators.createDefaultWithIssuer(issuer));

to remain an otherwise passive change.

Then, this PR would also introduce a new validator for at+jwt, something like:

JwtValidators.createDefaultForAtJwt(issuer);

So then to turn on at+jwt support, that would look like this:

NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();
decoder.setJwtValidator(JwtValidators.createDefaultForAtJwt(issuer));

Is this something you'd still be interested in putting together? If not, I can mark this as ideal-for-contribution so others know that it's available.

@jzheaux jzheaux removed the status: duplicate A duplicate of another issue label Feb 21, 2025
@ymajoros
Copy link
Author

@ymajoros, I don't think we should add this feature without also introducing validation. Allow me to elaborate on what I meant here.

I think it would be valuable for NimbusJwtDecoder's builders to turn off the Nimbus typ validation so that applications can better choose which validation types, JWT, at+jwt, etc. they want to support.

The builders could change like so:

NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();

Then, we can change JwtValidators to add typ validation by default similar to Nimbus, allowing the following:

NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();
decoder.setJwtValidator(JwtValidators.createDefaultWithIssuer(issuer));

to remain an otherwise passive change.

Then, this PR would also introduce a new validator for at+jwt, something like:

JwtValidators.createDefaultForAtJwt(issuer);

So then to turn on at+jwt support, that would look like this:

NimbusJwtDecoder decoder = NimbusJwtDecoder.withIssuerLocation(issuer)
    .useNimbusTypeVerifier(false).build();
decoder.setJwtValidator(JwtValidators.createDefaultForAtJwt(issuer));

Is this something you'd still be interested in putting together? If not, I can mark this as ideal-for-contribution so others know that it's available.

Hello, thanks for the analysis. TBH, I created this years ago because a colleague from security asked for it, but I don't even work there anymore and I won't follow this in any case. I just think being able to follow standards is always a good option, but I have no sponsor or personal interest in this anymore.

Thanks for your feedback, anyway.

@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 Feb 22, 2025
@jzheaux jzheaux added status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: feedback-provided Feedback has been provided labels Feb 24, 2025
@jzheaux
Copy link
Contributor

jzheaux commented Feb 24, 2025

No problem at all @ymajoros -- I like making sure before making a change like that to a PR, so thank you for responding as quickly as you did.

I've moved this to ideal-for-contribution to invite others to contribute and complete the PR.

@jzheaux jzheaux removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Feb 25, 2025
@jzheaux
Copy link
Contributor

jzheaux commented Feb 25, 2025

I've been talking with the team about this, and since it likely has a flag that will require migration between 6.5 and 7, I'll take this up myself instead.

@jzheaux jzheaux merged commit 7df85a2 into spring-projects:main Feb 27, 2025
6 checks passed
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