JwtTimestampsValidator can require exp and nbf claims #17030
Conversation
Signed-off-by: Ferenc Kemeny <ferenc.kemeny79+oss@gmail.com>
spring-projects-issues
added
the
status: waiting-for-triage
Signed-off-by: Ferenc Kemeny <ferenc.kemeny79+oss@gmail.com>
| @@ -32,19 +32,30 @@ public final class JwtAudienceValidator implements OAuth2TokenValidator<Jwt> { | |||
|
|
|||
| private final JwtClaimValidator<Collection<String>> validator; | |||
|
|
|||
| private boolean allowEmpty; | |||
There was a problem hiding this comment.
Having thought about this further, I think we should wait on adding this toggle to validators that require the claim by default. We can relax it when a clear need arises.
| @@ -32,20 +30,30 @@ public final class JwtIssuerValidator implements OAuth2TokenValidator<Jwt> { | |||
|
|
|||
| private final JwtClaimValidator<Object> validator; | |||
|
|
|||
| private boolean allowEmpty; | |||
There was a problem hiding this comment.
Having thought about this further, I think we should wait on adding this toggle to validators that require the claim by default. We can relax it when a clear need arises.
Other than the error message cleanup you added in validate, let's please not make further functional changes to this class in this PR.
| * Whether to allow the {@code exp} or {@code nbf} header to be empty. The default value is | ||
| * {@code true} | ||
| */ | ||
| public void setAllowEmpty(boolean allowEmpty) { |
There was a problem hiding this comment.
I think this needs to be more flexible, given that there are two different claims. It seems reasonable to say that exp is required while nbf is not.
Will you please change this to be setAllowEmptyExpiryClaim and setAllowEmptyNotBeforeClaim? These would each have their own toggle and be true by default.
| this.clockSkew = clockSkew; | ||
| } | ||
|
|
||
| /** | ||
| * Whether to allow the {@code exp} or {@code nbf} header to be empty. The default value is | ||
| * {@code true} |
There was a problem hiding this comment.
On new public methods, please add @since 7.0
| @@ -52,6 +52,8 @@ public final class JwtTimestampValidator implements OAuth2TokenValidator<Jwt> { | |||
|
|
|||
| private static final Duration DEFAULT_MAX_CLOCK_SKEW = Duration.of(60, ChronoUnit.SECONDS); | |||
|
|
|||
| private boolean allowEmpty; | |||
There was a problem hiding this comment.
Please set the default value here instead of in the constructor. Also, please declare it after the final fields.
| @@ -128,30 +128,77 @@ public void validateWhenConfiguredWithFixedClockThenValidatesUsingFixedTime() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void validateWhenNeitherExpiryNorNotBeforeIsSpecifiedThenReturnsSuccessfulResult() { | |||
There was a problem hiding this comment.
Will you please ensure that this file's copyright changes to 2025?
| } | ||
|
|
||
| @Test | ||
| public void validateWhenAllowEmptyDefaultAndJwtIsNullThenThrowsIllegalArgumentException() { |
There was a problem hiding this comment.
Thank you for these additional tests. Will you please place any tests or other changes that are unrelated to the feature in separate commits so that each commit is atomic?
| } | ||
|
|
||
| @Override | ||
| public OAuth2TokenValidatorResult validate(Jwt token) { | ||
| Assert.notNull(token, "token cannot be null"); | ||
| Assert.notNull(token, "jwt cannot be null"); |
There was a problem hiding this comment.
Thank you for this additional cleanup. Will you please place this is in a previous commit?
| } | ||
|
|
||
| @Override | ||
| public OAuth2TokenValidatorResult validate(Jwt token) { | ||
| Assert.notNull(token, "token cannot be null"); |
There was a problem hiding this comment.
I think the intent is to say the parameter name, not the datatype. Will you please leave this as-is?
| } | ||
|
|
||
| @Override | ||
| public OAuth2TokenValidatorResult validate(Jwt token) { | ||
| Assert.notNull(token, "token cannot be null"); |
There was a problem hiding this comment.
I think the intent is to say the parameter name, not the datatype. Will you please leave this as-is?
jzheaux
added
type: enhancement
jzheaux
changed the title
I implemented
requiredparameter inJwtTimestampValidator,JwtIssuerValidatorandJwtAudienceValidator. 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