-
Notifications
You must be signed in to change notification settings - Fork 6k
Oidclogin test support #7620
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
Oidclogin test support #7620
Conversation
test/src/main/java/org/springframework/security/test/web/support/WebMvcTestUtils.java
Outdated
Show resolved
Hide resolved
6d70bf2
to
aded60e
Compare
@rwinch, obviously feel free to comment on anything you like. I'm posting this PR so that you can take a look at a couple of multi-threading concerns I have. |
.../springframework/security/test/web/servlet/request/SecurityMockMvcRequestPostProcessors.java
Outdated
Show resolved
Hide resolved
aded60e
to
c1f69ce
Compare
OidcUser oidcUser = getOidcUser(); | ||
OAuth2AuthenticationToken token = new OAuth2AuthenticationToken(oidcUser, oidcUser.getAuthorities(), this.clientRegistration.getRegistrationId()); | ||
OAuth2AuthorizedClient client = new OAuth2AuthorizedClient(this.clientRegistration, token.getName(), this.accessToken); | ||
OAuth2AuthorizedClientRepository authorizedClientRepository = new HttpSessionOAuth2AuthorizedClientRepository(); |
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.
@jgrandja I looked into simply pulling this from the application context. The issue I discovered is that Spring Boot registers an AuthenticatedPrincipalOAuth2AuthorizedClientRepository
, which will store this client
in an InMemoryOAuth2AuthorizedClientService
. Since that is likely shared between tests, it could cause a problem in concurrent tests.
I've chosen to stick with HttpSessionOAuth2AuthorizedClientRepository
and documentation that indicates that tests should use that repository so that the argument resolver picks up the client created here.
With this approach, @RegisteredOAuth2AuthorizedClient
will work with still pretty minimal code in the test, e.g.:
@Test
public void test() {
this.mvc.perform(get("/")
.with(oidcLogin().clientRegistration(clientRegistration));
}
@Configuration
static class ClientConfig {
@Bean
ClientRegistrationRepository clients() {
return new InMemoryClientRegistrationRepository(clientRegistration);
}
@Bean
OAuth2AuthorizedClientRepository authorizedClients() {
return new HttpSessionOAuth2AuthorizedClientRepository();
}
}
This could be potentially reduced down the road if there's a simple, thread-safe way to reconfigure or override the OAuth2AuthorizedClientArgumentResolver
within the confines of the test.
/cc @rwinch for additional thoughts.
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.
@jzheaux I like the way the test reads. It's simple, concise and consistent with HttpSecurity.oauth2Login()
Servlet DSL. For the initial feature release, I think it's fine to require the use of HttpSessionOAuth2AuthorizedClientRepository
in tests. We can figure out ways to improve this later.
Merged via b35e18f |
No description provided.