-
Notifications
You must be signed in to change notification settings - Fork 6k
JwtTimestampsValidator can require exp and nbf claims #17030
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
base: main
Are you sure you want to change the base?
JwtTimestampsValidator can require exp and nbf claims #17030
Conversation
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, @FerencKemeny! I've leave my feedback inline.
.../oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtAudienceValidator.java
Outdated
Show resolved
Hide resolved
...h2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtIssuerValidator.java
Outdated
Show resolved
Hide resolved
...oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtTimestampValidator.java
Outdated
Show resolved
Hide resolved
...oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtTimestampValidator.java
Show resolved
Hide resolved
...oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtTimestampValidator.java
Outdated
Show resolved
Hide resolved
...2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtTimestampValidatorTests.java
Show resolved
Hide resolved
...uth2-jose/src/test/java/org/springframework/security/oauth2/jwt/JwtIssuerValidatorTests.java
Outdated
Show resolved
Hide resolved
...h2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtIssuerValidator.java
Outdated
Show resolved
Hide resolved
...h2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/JwtIssuerValidator.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public OAuth2TokenValidatorResult validate(Jwt token) { | ||
Assert.notNull(token, "token cannot be null"); |
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 think the intent is to say the parameter name, not the datatype. Will you please leave this as-is?
9a92ff2
to
291c6e7
Compare
This commit corrects the test that checks for both nbf and exp missing. It also adds one for just exp and on for just nbf. Issue spring-projectsgh-17004 Signed-off-by: Ferenc Kemeny <ferenc.kemeny79+oss@gmail.com>
Closes spring-projectsgh-17004 Signed-off-by: Ferenc Kemeny <ferenc.kemeny79+oss@gmail.com>
291c6e7
to
36513ff
Compare
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, @FerencKemeny, for the PR! We'll merge this shortly after we've wrapped up the 6.5 release.
Thank you, @jzheaux, for all of your help on the PR. Appreciated! |
I implemented
required
parameter inJwtTimestampValidator
,JwtIssuerValidator
andJwtAudienceValidator
. I left the function of the original constructors untouched. In the original implementation successful validation was returned even if timestamps, issues or audience claims were missing. So this way previous API, implementations are not breaking. With my changes it is possible now to specify more strict specification, to tell if the given claim is mandatory to present and indiate it with failed validation if the claim is missing.This is the feature indicated in #17004