Enhance LogoutResource to dynamically resolve OAuth2 client registration #28420
Conversation
| String originUrl = request.getHeader(HttpHeaders.ORIGIN); | ||
|
|
||
| logoutUrl.append("?id_token_hint=").append(oidcUser.getIdToken().getTokenValue()).append("&post_logout_redirect_uri=").append(originUrl); | ||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); |
There was a problem hiding this comment.
I think that you can retreive the authentication from the OidcUser object
There was a problem hiding this comment.
Hi @Tcharl,
Thank you for the suggestion! My understanding is that retrieving the Authentication directly from the OidcUser isn’t feasible, as OidcUser interface doesn’t provide access to the Authentication instance.
While I’m aware that OidcUser can also be obtained by casting the Principal from the Authentication, this approach adds unnecessary casting. For simplicity, I decided to keep the current implementation where OidcUser is directly injected using @AuthenticationPrincipal.
If my understanding is incorrect or there’s a better approach, please feel free to let me know. I really appreciate it! 😊
There was a problem hiding this comment.
I don't remember how I achieved that in the past, but it is either via a cast of the oidcuser (easy testable using debugger) or either through the injection of another object in the controller (like an UserPrincipal object).
using SecurityContextHolder to retrieve principal is usually a bad idea because it uses ThreadLocal (which is local to the jvm), which may lead to unexpected behavior in a multiinstance topology. Can you please continue a bit the investigation and provide something following this statement please?
There was a problem hiding this comment.
I believe there's no inherent conflict between using ThreadLocal and multi-instance deployments. Parameter injection still uses SecurityContextHolder and ThreadLocal behind the scenes. In Servlet-based applications, while a servlet container (like Tomcat) uses a thread pool to handle requests, each request is processed by a single thread throughout its lifecycle, and that thread can be reused for subsequent requests. Spring Security ensures thread safety by cleaning up the ThreadLocal after each request completes, preventing data leakage between different requests handled by the same thread.
Regarding multi-instance concerns:
First, JHipster's LogoutResource is only generated for oauth2Login applications (API Gateway and monolithic applications), both of which use session-based authentication. After successful login, Spring Security stores the authenticated SecurityContext in the HttpSession. For each request, the SecurityContext is retrieved from the HttpSession and temporarily stored in ThreadLocal, allowing all methods within the current request to share the same SecurityContext through ThreadLocal. The ThreadLocal is cleared after the request completes.
Unfortunately, HttpSession is stored in JVM by default, making it impossible to horizontally scale API Gateway and monolithic apps (as instances cannot share HttpSession state). Therefore, the key challenge in multi-instance deployments is "how to share session state between instances". The solution is to persist HttpSession externally (e.g., using JDBC or Redis), but I believe this is beyond the scope of this PR. I'm happy to provide more information if needed.
Of course, this is just my understanding. Please feel free to let me know if I've made any mistakes. I really appreciate your feedback!
There was a problem hiding this comment.
Just as a matter of test: injecting the Authentication as a method argument of the controller does not work?
Seems to be cleaner if it passes. Singleton pattern is so oldie^^
There was a problem hiding this comment.
Hi @Tcharl,
Thank you for your reply! Injecting the Authentication as a method parameter works perfectly, and it’s indeed the best practice. I’ve refactored the code accordingly. I really appreciate your valuable feedback!
| ) { | ||
| return session.invalidate().then(this.registration.map(oidc -> prepareLogoutUri(request, oidc, oidcUser.getIdToken()))); | ||
| if (authToken instanceof OAuth2AuthenticationToken) { |
There was a problem hiding this comment.
I think that you can retreive the authentication from the OidcUser object
There was a problem hiding this comment.
Hi @Tcharl,
Thank you for the suggestion! My understanding is that retrieving the Authentication directly from the OidcUser isn’t feasible, as OidcUser interface doesn’t provide access to the Authentication instance.
While I’m aware that OidcUser can also be obtained by casting the Principal from the Authentication, this approach adds unnecessary casting. For simplicity, I decided to keep the current implementation where OidcUser is directly injected using @AuthenticationPrincipal.
If my understanding is incorrect or there’s a better approach, please feel free to let me know. I really appreciate it! 😊
yhao3
force-pushed
the
enhance-logout-resource
branch
from
4539180 to
49f2875
Compare
|
I can see how this might be better because the "oidc" provider is not hard-coded. However, I don't think we've had any complaints about this either. Have you encountered a bug with the current implementation or do you just want to make it so "oidc" isn't hard-coded? |
Description
This update enhances the
LogoutResourceto dynamically resolve the OAuth2 client registration based on the currently authenticated user'sOAuth2AuthenticationToken.Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (below reviewers) and adding
skip-cilabel, you can still see CI build result at your branch.