Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support custom token validators for OAuth2 Resource Server auto-configuration #35874

Closed
wants to merge 2 commits into from
Closed

Support custom token validators for OAuth2 Resource Server auto-configuration #35874

wants to merge 2 commits into from

Conversation

romangr
Copy link
Contributor

@romangr romangr commented Jun 13, 2023

Added injections of OAuth2TokenValidator<Jwt> beans to ReactiveOAuth2ResourceServerJwkConfigurationand OAuth2ResourceServerJwtConfiguration to provide an easy way to add custom validations and still use auto-configuration (#35783)

Closes gh-35783

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 13, 2023
@romangr
Copy link
Contributor Author

romangr commented Jun 21, 2023

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

@wilkinsona wilkinsona mentioned this pull request Jun 30, 2023
Closed
@wilkinsona wilkinsona self-assigned this Jun 30, 2023
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 30, 2023
@wilkinsona wilkinsona added this to the 3.2.x milestone Jun 30, 2023
wilkinsona
wilkinsona requested changes Jun 30, 2023
View reviewed changes
...oot/autoconfigure/security/oauth2/resource/servlet/OAuth2ResourceServerJwtConfiguration.java
if (!CollectionUtils.isEmpty(audiences)) {
validators.add(new JwtClaimValidator<List<String>>(JwtClaimNames.AUD,
(aud) -> aud != null && !Collections.disjoint(aud, audiences)));
}
return new DelegatingOAuth2TokenValidator<>(validators);
Copy link
Member

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.

Copy link
Contributor Author

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.

...re/security/oauth2/resource/reactive/ReactiveOAuth2ResourceServerAutoConfigurationTests.java Outdated
Comment on lines 460 to 466
assertThat(reactiveJwtDecoder).extracting("jwtValidator.tokenValidators")
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class))
.hasSize(1)
.first()
.extracting("tokenValidators")
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class))
.hasAtLeastOneElementOfType(JwtIssuerValidator.class);
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

...re/security/oauth2/resource/reactive/ReactiveOAuth2ResourceServerAutoConfigurationTests.java Outdated
Comment on lines 484 to 492
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);
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

...re/security/oauth2/resource/reactive/ReactiveOAuth2ResourceServerAutoConfigurationTests.java Outdated
@@ -740,4 +784,15 @@ SecurityWebFilterChain testSpringSecurityFilterChain(ServerHttpSecurity http) {

}

@Configuration(proxyBeanMethods = false)
@EnableWebFluxSecurity
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

...onfigure/security/oauth2/resource/reactive/ReactiveOAuth2ResourceServerJwkConfiguration.java
if (!CollectionUtils.isEmpty(audiences)) {
validators.add(new JwtClaimValidator<List<String>>(JwtClaimNames.AUD,
(aud) -> aud != null && !Collections.disjoint(aud, audiences)));
}
return new DelegatingOAuth2TokenValidator<>(validators);
Copy link
Member

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.

Copy link
Contributor Author

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.

...toconfigure/security/oauth2/resource/servlet/OAuth2ResourceServerAutoConfigurationTests.java Outdated
Comment on lines 478 to 481
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class))
.hasSize(1)
.first()
.extracting("tokenValidators")
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

...toconfigure/security/oauth2/resource/servlet/OAuth2ResourceServerAutoConfigurationTests.java Outdated
Comment on lines 501 to 504
.asInstanceOf(InstanceOfAssertFactories.collection(OAuth2TokenValidator.class))
.hasSize(1)
.first()
.extracting("tokenValidators")
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

...toconfigure/security/oauth2/resource/servlet/OAuth2ResourceServerAutoConfigurationTests.java Outdated
@@ -745,4 +793,15 @@ SecurityFilterChain testSecurityFilterChain(HttpSecurity http) throws Exception

}

@Configuration(proxyBeanMethods = false)
@EnableWebSecurity
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@wilkinsona
Copy link
Member

Thanks for the PR, @romangr. I've left a few comments for your consideration when you have a minute.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jun 30, 2023
@romangr romangr requested a review from wilkinsona June 30, 2023 16:20
@wilkinsona wilkinsona removed the status: waiting-for-feedback We need additional information before we can continue label Jul 5, 2023
@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.2.0-M1 Jul 5, 2023
wilkinsona pushed a commit that referenced this pull request Jul 5, 2023
@romangr @wilkinsona
Support custom token validators for OAuth2
7500dab 
See gh-35874
wilkinsona added a commit that referenced this pull request Jul 5, 2023
@wilkinsona
Polish "Support custom token validators for OAuth2"
4feaa28 
See gh-35874
@wilkinsona wilkinsona closed this in 83d5d89 Jul 5, 2023
@wilkinsona
Copy link
Member

@romangr, thanks very much for making your first contribution to Spring Boot.

@romangr romangr mentioned this pull request Jul 13, 2023
Open
@jzheaux jzheaux mentioned this pull request Jun 17, 2024
Open
@mrodal mrodal mentioned this pull request Jun 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkinsona wilkinsona Awaiting requested review from wilkinsona

Assignees

@wilkinsona wilkinsona

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.2.0-M1
Development

Successfully merging this pull request may close these issues.

Make it easier to add validators to the auto-configured DelegatingOAuth2TokenValidator
3 participants
@romangr @wilkinsona @spring-projects-issues