-
Notifications
You must be signed in to change notification settings - Fork 40.8k
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
Support custom token validators for OAuth2 Resource Server auto-configuration #35874
Support custom token validators for OAuth2 Resource Server auto-configuration #35874
Conversation
Rebased to the main branch since some tests unrelated to the PR failed in the previous run https://github.com/spring-projects/spring-boot/actions/runs/5258769234/jobs/9667349868?pr=35874 |
if (!CollectionUtils.isEmpty(audiences)) { | ||
validators.add(new JwtClaimValidator<List<String>>(JwtClaimNames.AUD, | ||
(aud) -> aud != null && !Collections.disjoint(aud, audiences))); | ||
} | ||
return new DelegatingOAuth2TokenValidator<>(validators); |
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.
If there are no audiences and no custom validators, we should return the defaultValidator
rather than creating a DelegatingOAuth2TokenValidator
with a single delegate.
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 added a check for number of validators in the list to avoid duplication of the CollectionUtils.isEmpty(audiences)
check. I think this way it will be easier to add new predefined validators like the one for audiences if needed.
assertThat(reactiveJwtDecoder).extracting("jwtValidator.tokenValidators") | ||
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasSize(1) | ||
.first() | ||
.extracting("tokenValidators") | ||
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasAtLeastOneElementOfType(JwtIssuerValidator.class); |
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.
These changes should be reverted. The suggestion above should allow the original test to pass.
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.
Reverted
assertThat(reactiveJwtDecoder).extracting("jwtValidator.tokenValidators") | ||
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasSize(1) | ||
.first() | ||
.extracting("tokenValidators") | ||
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasExactlyElementsOfTypes(JwtTimestampValidator.class) | ||
.doesNotHaveAnyElementsOfTypes(JwtClaimValidator.class) | ||
.doesNotHaveAnyElementsOfTypes(JwtIssuerValidator.class); |
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.
These changes should be reverted. The suggestion above should allow the original test to pass.
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.
Reverted
@@ -740,4 +784,15 @@ SecurityWebFilterChain testSpringSecurityFilterChain(ServerHttpSecurity http) { | |||
|
|||
} | |||
|
|||
@Configuration(proxyBeanMethods = false) | |||
@EnableWebFluxSecurity |
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 don't think @EnableWebFluxSecurity
is needed here.
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.
Removed
if (!CollectionUtils.isEmpty(audiences)) { | ||
validators.add(new JwtClaimValidator<List<String>>(JwtClaimNames.AUD, | ||
(aud) -> aud != null && !Collections.disjoint(aud, audiences))); | ||
} | ||
return new DelegatingOAuth2TokenValidator<>(validators); |
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.
If there are no audiences and no custom validators, we should return the defaultValidator
rather than creating a DelegatingOAuth2TokenValidator
with a single delegate.
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 added a check for number of validators in the list to avoid duplication of the CollectionUtils.isEmpty(audiences)
check. I think this way it will be easier to add new predefined validators like the one for audiences if needed.
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasSize(1) | ||
.first() | ||
.extracting("tokenValidators") |
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.
These changes should be reverted. The suggestion above should allow the original test to pass.
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.
Reverted
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class)) | ||
.hasSize(1) | ||
.first() | ||
.extracting("tokenValidators") |
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.
These changes should be reverted. The suggestion above should allow the original test to pass.
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.
Reverted
@@ -745,4 +793,15 @@ SecurityFilterChain testSecurityFilterChain(HttpSecurity http) throws Exception | |||
|
|||
} | |||
|
|||
@Configuration(proxyBeanMethods = false) | |||
@EnableWebSecurity |
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 don't think @EnableWebSecurity
is needed here.
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.
Removed
Thanks for the PR, @romangr. I've left a few comments for your consideration when you have a minute. |
@romangr, thanks very much for making your first contribution to Spring Boot. |
Added injections of
OAuth2TokenValidator<Jwt>
beans toReactiveOAuth2ResourceServerJwkConfiguration
andOAuth2ResourceServerJwtConfiguration
to provide an easy way to add custom validations and still use auto-configuration (#35783)Closes gh-35783