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.

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

UserDetailsServiceAutoConfiguration should not be disabled when ClientRegistrationRepository is present #28813

Closed
edouardhue opened this issue Nov 25, 2021 · 3 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@edouardhue
Copy link

org.springframework.boot.autoconfigure.security.servlet.UserDetailsServiceAutoConfiguration has a conditional on missing beans org.springframework.security.oauth2.client.registration.ClientRegistrationRepository (some OAuth2 clients are registered) and org.springframework.security.oauth2.server.resource.introspection.OpaqueTokenIntrospector (a resource server is configured). It is annoying that the default UserDetailsManager is disabled as soon as some OAuth2 clients are registered. I suppose it was assumed that an application registering clients would always use itself an OAuth2 authentication, but this is not always the case. For example, one might setup a simple HTTP Basic authentication with only one user (as with default auto-configuration), but this application may need to access a remote resource server with a OAuth2 client.

I suggest to simply remove the conditional on org.springframework.security.oauth2.client.registration.ClientRegistrationRepository.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 25, 2021
@wilkinsona
Copy link
Member

Thanks for the suggestion. The current behaviour is intentional and we don't plan to change it. Please see this comment from @rwinch which explains the reasons for the behaviour and #24454 and #21503 for a few additional details.

@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 25, 2021
@edouardhue
Copy link
Author

Hi @wilkinsona, @rwinch,

If I get it right, the idea was to skip the UserDetailsService when HttpSecurity#oauth2Client() is used, which seems perfectly correct. The problem is with the assumption that the UserDetailsService should be skipped whenever a ClientRegistrationRepository bean is present. It is true that the OAuth2ClientConfigurer will create a ClientRegistrationRepository in support to the authorization code grant filters, but my point is that this is not the only situation in which a ClientRegistrationRepository may be used. For example, using an AuthorizedClientServiceOAuth2AuthorizedClientManager and a WebClient with the client credentials grant has absolutely nothing to do with authenticating incoming requests, but still uses a ClientRegistrationRepository. It should not have the side effect of disabling the default UserDetailsService. I believe a more specific bean should be used in the condition, such as OAuth2AuthorizationRequestRedirectFilter or OAuth2AuthorizationCodeGrantFilter, which are created by OAuth2ClientConfigurer for the sole purpose of supporting the authorization code grant.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 29, 2021
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jan 12, 2022
@wilkinsona
Copy link
Member

Unfortunately, this is a situation where we cannot please all of the people all of the time. Our preference is to err on the side not creating the user details service when it may be unnecessary. For cases where it was still wanted, you should define your own configured to meet your needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants