Closed chenrujun closed 3 years ago
The javadoc of AuthenticatedPrincipalOAuth2AuthorizedClientRepository
suggests that it delegates to HttpSessionOAuth2AuthorizedClientRepository
for unauthenticated requests. Could you provide a bit more background about your suggested change?
/cc @jgrandja
Hi, @philwebb , Thank you for your response.
InMemoryOAuth2AuthorizedClientService
will save all authorizedClient
s in memory, it may cause OOM in some case.
I created a draft PR: https://github.com/spring-projects/spring-boot/pull/24303/files
Thanks @chenrujun.
One problem with the suggested change is that users will no longer be able to just provide a OAuth2AuthorizedClientService
bean. They'd need to provide an AuthenticatedPrincipalOAuth2AuthorizedClientRepository
with the OAuth2AuthorizedClientService
already set.
I wonder if InMemoryOAuth2AuthorizedClientService
should be constrained so that it accepts a maximum number of entries. @jgrandja Do you have any thoughts on this?
@philwebb
Current logic is like this:
If one user login, saved some authorizedClient
s in InMemoryOAuth2AuthorizedClientService
, then the user left.
The the authorizedClient
will never be deleted automically.
maximum number of entries
is not a good idea. Because the web application may have millions of users,
IMU, with HttpSessionOAuth2AuthorizedClientRepository
, when session expired, the authorizedClient
s will be cleaned automically.
@philwebb
How about create a new class HttpSessionOAuth2AuthorizedClientService
, use it to replace InMemoryOAuth2AuthorizedClientService
?
If you agree, I'm willing to create another PR.
I'd like to see what @jgrandja has to say first.
@chenrujun I'd like to give you some background between OAuth2AuthorizedClientService
vs. OAuth2AuthorizedClientRepository
.
OAuth2AuthorizedClientService
is designed to act as a core "service" that supplies client registrations. You will notice it's not dependent on the Servlet API so can be used outside of a web application context. However, the OAuth2AuthorizedClientRepository
is tied to the Servlet API and is the main entry point to OAuth2AuthorizedClient
's for
web-tier collaborators. The OAuth2AuthorizedClientRepository
is meant to delegate to an OAuth2AuthorizedClientService
that holds/maintains the actual client registrations.
The AuthenticatedPrincipalOAuth2AuthorizedClientRepository
is the default OAuth2AuthorizedClientRepository
for web applications. It handles "unauthenticated" requests via a HttpSessionOAuth2AuthorizedClientRepository
delegate and handles "authenticated" requests by delegating to the OAuth2AuthorizedClientService
.
The 2x implementations of OAuth2AuthorizedClientService
is InMemoryOAuth2AuthorizedClientService
and JdbcOAuth2AuthorizedClientService
.
InMemoryOAuth2AuthorizedClientService
really should only be used for development / testing. I would not recommend it in a production environment.
JdbcOAuth2AuthorizedClientService
is new in 5.3
and requires custom configuration to take effect. See the reference doc and javadoc.
The auto-configuration in OAuth2WebSecurityConfiguration
makes sense. Boot cannot auto-configure the JdbcOAuth2AuthorizedClientService
since the schema needs to be defined along with connection properties. Therefore, that leaves us with InMemoryOAuth2AuthorizedClientService
, hence the default.
NOTE: For production applications, the expectation is that the application would configure JdbcOAuth2AuthorizedClientService
or provide a custom OAuth2AuthorizedClientService
@Bean
.
@philwebb Feel free to close this, as OAuth2WebSecurityConfiguration
is aligned and there are no issues.
@jgrandja .
Thank you very much for your detailed explanation.
IMU, the root reason is that spring-security-oauth2-client
is not only used in web applications. But HttpSession
only exist in web application. So it doesn't make sense to use HttpSessionOAuth2AuthorizedClientRepository
as default repository.
I'll close the issue.
@philwebb @jgrandja
InMemoryOAuth2AuthorizedClientService really should only be used for development / testing. I would not recommend it in a production environment.
To make less misunderstanding, I suggest to write this in reference doc or java doc.
I've opened #24313 to look at the documentation
@jgrandja Could you please elaborate a bit on this point from above:
"InMemoryOAuth2AuthorizedClientService really should only be used for development / testing. I would not recommend it in a production environment."
Yes, it is in memory and can be bad for applications with million of users, but looking at the source code of InMemoryOAuth2AuthorizedClientService
I don't see a memory leak there.
The call to AuthenticatedPrincipalOAuth2AuthorizedClientRepository::removeAuthorizedClient
delegates, in case of an authenticated user, to InMemoryOAuth2AuthorizedClientService::removeAuthorizedClient
and removes the registration there.
If you don't need millions of users, this sounds like a reasonable default to me.
@knoobie
InMemoryOAuth2AuthorizedClientService::removeAuthorizedClient
will not be executed when authenticated user logout.
@chenrujun thanks for the heads up! You are right, that is really unexpected. That sounds like something to be documented.
@chenrujun thanks for the heads up! You are right, that is really unexpected. That sounds like something to be documented.
I was quite confused by this. I guess it makes sense since one may want to make calls on behalf of users when they are not logged in. It is however quite surprising that there is no mechanism at all to clean those tokens. Even the JdbcOAuth2AuthorizedClientService
will never clear expired tokens (see also this SO question).
Some recommendations/guidelines in the documentation would be welcome, especially since it recommends (since #24313) to either use JdbcOAuth2AuthorizedClientService
or provide your own OAuth2AuthorizedClientService
implementation.
Just got bit by this one again in a different context, as of course the default implementation does not support clustering (last time it was a memory leak).
I’m wondering: couldn’t Spring Boot detect that Spring Session is present, and choose the implementation based on that? (including WebSessionServerOAuth2AuthorizedClientRepository
if applicable). I’m not sure if Spring Session would be absolutely required though, as I don’t think it is required for session-based handling.
However…
IMU, the root reason is that
spring-security-oauth2-client
is not only used in web applications. ButHttpSession
only exist in web application. So it doesn't make sense to useHttpSessionOAuth2AuthorizedClientRepository
as default repository.
Is that really possible @chenrujun and @philwebb ? AuthenticatedPrincipalOAuth2AuthorizedClientRepository
(resp. AuthenticatedPrincipalServerOAuth2AuthorizedClientRepository
) delegates to HttpSessionOAuth2AuthorizedClientRepository
(resp. WebSessionServerOAuth2AuthorizedClientRepository
) for anonymous requests by default, so would it work outside of a web application?
@DidierLoiseau
so would it work outside of a web application?
IMU, session only exists for web application.
@chenrujun
IMU, session only exists for web application.
In that case, the default OAuth implementation only works with those as well.
@DidierLoiseau
In that case, the default OAuth implementation only works with those as well.
What does "those" mean here? Does "those" mean non-web application?
Yes, the default implementation works well for non-web application.
What does "those" mean here? Does "those" mean non-web application?
No I meant web applications.
Yes, the default implementation works well for non-web application.
How? As said in a previous comment, it delegates the management of anonymous cases to HttpSessionOAuth2AuthorizedClientRepository
. For non-web applications, this implies that it works only with authenticated principals. But maybe that’s the intent?
Anyway, the main point is that maybe HttpSessionOAuth2AuthorizedClientRepository
could be used by default for web applications, couldn’t it?
@DidierLoiseau
Yes, the default implementation works well for non-web application.
The default implementation is InMemoryOAuth2AuthorizedClientService
. It has no relationship with HttpSessionOAuth2AuthorizedClientRepository
. See OAuth2WebSecurityConfiguration.
Anyway, the main point is that maybe HttpSessionOAuth2AuthorizedClientRepository could be used by default for web applications, couldn’t it?
No,the default one is still InMemoryOAuth2AuthorizedClientService
. If you are developing a web application, you can use HttpSessionOAuth2AuthorizedClientRepository
by providing a self-defined bean. Override the bean defined in OAuth2WebSecurityConfiguration
@DidierLoiseau
Yes, the default implementation works well for non-web application.
The default implementation is
InMemoryOAuth2AuthorizedClientService
. It has no relationship withHttpSessionOAuth2AuthorizedClientRepository
. See OAuth2WebSecurityConfiguration.
A few lines below that you also have the declaration of the default OAuth2AuthorizedClientRepository
, which is AuthenticatedPrincipalOAuth2AuthorizedClientRepository
. This one uses the provided OAuth2AuthorizedClientService
you mentioned, but internally instantiates an HttpSessionOAuth2AuthorizedClientRepository
, so I would assume that it does not work in all cases for non-web applications.
(ok, re-reading the first comments here, I notice this was already mentioned)
Anyway, the main point is that maybe HttpSessionOAuth2AuthorizedClientRepository could be used by default for web applications, couldn’t it?
No,the default one is still
InMemoryOAuth2AuthorizedClientService
. If you are developing a web application, you can useHttpSessionOAuth2AuthorizedClientRepository
by providing a self-defined bean. Override the bean defined in OAuth2WebSecurityConfiguration
Well, in fact, if you are developing any application, you have to provide a self-defined bean, since as stated above, as well as in the documentation
The
InMemoryOAuth2AuthorizedClientService
has limited capabilities and we recommend using it only for development environments.
Indeed this in-memory implementation will cause memory leaks since it never clears its expired entries. It is a bit surprising that Spring Boot configures by default an implementation that is not recommended for production, without a runtime warning (like there is e.g. in UserDetailsServiceAutoConfiguration
).
Of course, there isn’t really a suitable implementation available for a default OAuth2AuthorizedClientService
, so I wonder if it wouldn’t be better if Boot didn’t configure one, and force users to make an informed choice if they need one (which isn’t the case when using an HttpSessionOAuth2AuthorizedClientRepository
).
I don’t really know what’s best, maybe I could just create a new ticket to add a warning like in UserDetailsServiceAutoConfiguration
.
I don’t really know what’s best, maybe I could just create a new ticket to add a warning like in UserDetailsServiceAutoConfiguration.
@DidierLoiseau
Already documented here: https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#web.security.oauth2.client
If you think it's not enough, and necessary to add a warn log. I agree with you.
Hi, @philwebb , what's your opinion?
https://github.com/spring-projects/spring-boot/blob/v2.4.0/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/oauth2/client/servlet/OAuth2WebSecurityConfiguration.java
IMO, Use
HttpSessionOAuth2AuthorizedClientRepository
is more reasonable: theauthorizedClient
will cleared automatically after session expired.