refactor: delegate logout handling from login.php to LoginService#109
refactor: delegate logout handling from login.php to LoginService#109jcdelepine wants to merge 1 commit into
Conversation
login.php handled logout logic directly (CSRF check, clearAuth, session setup, redirect). LoginService.performLogout() already encapsulates all of this. Delegate to it so logout behaviour is consistent whether the request comes from login.php or from the modern routing stack (ResponsiveLogoutController). Pre-logout handlers registered in LoginService now run on all logout paths, not just those going through the responsive stack.
ralflang
left a comment
ralflang
left a comment
There was a problem hiding this comment.
@jcdelepine Thank you for submitting this. Please don't be worried by the number of comments here. The crucial change is twofold: Where does the preLogoutHandler come from in your setup (probably a hook?)
The other parts are minutiae which don't affect the happy path and which I can work on myself.
| $this->notification->detach('status'); | ||
| $this->notification->attach('status'); | ||
|
|
||
| // Check redirect_on_logout config |
There was a problem hiding this comment.
I think this redirect will make your own redirect code unaccessible?
There was a problem hiding this comment.
You are right. This is fixed in the OIDC PR where redirect_on_logout
is evaluated after sessionLifecycle->setup() and after the
pre-logout handlers have run, so handler redirects take priority.
Feel free to fix this directly.
| forwardedFor: $_SERVER['HTTP_X_FORWARDED_FOR'] ?? null, | ||
| )); | ||
| $is_auth = null; | ||
| } else { |
There was a problem hiding this comment.
I think we need to reset the notification handler in this path, too?
| try { | ||
| $data = $handler->onBeforeLogout($currentUser, $request->reason); | ||
| if (!empty($data['redirect']) && $postLogoutRedirect === null) { | ||
| $postLogoutRedirect = $data['redirect']; |
There was a problem hiding this comment.
What if we have multiple preLogoutHandlers with different redirect attributes? Only the first one will be honored (which is probably what we want OR notice we have contradicting setups and just crash with a useful error message.
There was a problem hiding this comment.
Only the first non-null redirect is honoured — subsequent handlers
can still perform their cleanup (token revocation etc.) but their
redirect is ignored. This is intentional: in practice a deployment
will have at most one handler that produces a redirect (the OIDC SLO
handler). If contradicting redirects were a real concern, a more
explicit conflict-detection mechanism would be warranted, but I felt
that was premature for now.
A The concrete implementation in our setup is |
|
@jcdelepine This is roughly what I thought it could be. However, excuse my ignorance, I think I am missing the PR against horde/core. I merged the ones against horde/jwt and horde/Oauth#4 (and I am reviewing your additional horde/Oauth#5 in the mean time). |
|
@ralflang The missing context is now available in the horde/Core PR which I have
The full OIDC integration also covers horde/imp and horde/ingo (XOAUTH2 PRs in dependency order:
|
|
I see. The first two points are not controversial and expected. |
2 participants
This PR refactors logout handling in login.php to delegate to LoginService::performLogout(), ensuring consistent behavior across all logout paths (legacy and modern routing).
It removes duplicated logout logic and ensures pre-logout handlers and session lifecycle management are executed uniformly.
@ralflang My OIDC login implementation is ready. This is PR 3 of 3 required to
continue the implementation.