Repository navigation
Conversation
backchannelLogout removes only its own tab when the logout session joined a root that still holds other tabs, such as an unfinished login in the same browser. createOrJoinLogoutSession no longer reuses a logout tab that belongs to another user session, and binding a logout tab to another user session removes the client logout states recorded for the previous one, so they cannot suppress the logout of the new user session's clients. A new logout root bound to the browser also updates KC_AUTH_SESSION_HASH, so a login that joins it does not keep reloading in authChecker.js. Closes keycloak#55118 Signed-off-by: Krish Garg <181051779+krishhgg@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Session rebinding can leave stale markers that skip client logout, while replacement logout tabs can unnecessarily evict unfinished login tabs when the shared root reaches its session limit.
2 open issues
What changed in this PR
Addresses #55118 by refining logout authentication-session handling while preserving the session-takeover cleanup introduced in #54942.
Changes:
- Preserves unrelated tabs when cleaning up shared logout roots.
- Replaces cross-session logout tabs and clears known client logout states when rebinding on the confirmation path.
- Synchronizes the authentication-session hash cookie and adds browser and server-side regression tests.
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/forms/LogoutSessionReuseTest.java |
Adds coverage for shared roots, cross-session logout, rebinding, and hash cookies. |
services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java |
Updates logout cleanup, tab reuse, client-state clearing, and cookie synchronization. |
services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.java |
Uses the binding helper before browser logout. |
Review effort: Balanced
Comment on lines
+471
to
+472
| boolean otherUserSession = userSession != null && prevUserSessionId != null && !prevUserSessionId.equals(userSession.getId()); | ||
| if (!otherUserSession && (!initiateLogout || client.getId().equals(prevAuthSession.getClient().getId()))) { |
| public static void bindLogoutSessionToUserSession(KeycloakSession session, AuthenticationSessionModel logoutSession, UserSessionModel userSession) { | ||
| String boundUserSessionId = logoutSession.getAuthNote(LOGOUT_USER_SESSION_ID); | ||
| if (boundUserSessionId != null && !boundUserSessionId.equals(userSession.getId())) { | ||
| Set<String> clientUuids = new HashSet<>(userSession.getAuthenticatedClientSessions().keySet()); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #55118
For the shared root case,
backchannelLogoutnow removes only its logout tab when the root still holds other tabs, and removes the root through the provider when the logout tab is the last one. I kept provider-level removal for the dedicated root because the JPA provider tracks new roots intransientSessions, and removing the last tab through the JPA adapter would leave a root created in the same transaction in that map and persist it on commit. The root that shares the user session id is still removed byremoveUserSessionAndRootAuthSession, so the cleanup added in #54942 is unchanged.For cross-session reuse,
createOrJoinLogoutSessionreplaces aLOGGING_OUTtab whoseLOGOUT_USER_SESSION_IDnames another user session with a fresh tab, as the issue suggests. A tab without that note is still reused, because it was never used to log out a user session and holds no client logout states.The same problem exists on the confirmation path, which the issue does not list. When the browser is logged in to session A and
id_token_hintnames session B, the logout endpoint joins A's logout tab andinitiateBrowserLogoutlater rebinds it to B. That tab holds this request'spost_logout_redirect_uri,stateand validated hint, so replacing it would lose them. Instead the newAuthenticationManager.bindLogoutSessionToUserSessionremoves thelogout.state.<client>notes for the clients of both user sessions when the binding changes. Auth notes cannot be listed, so a note for a client that neither user session has a client session for at that moment (for example because the previous session or its client session expired) stays in the tab. It only matters if the new session gets a session for that client while its logout is in progress, for example during an upstream identity provider logout. Scoping the notes to the bound user session id, or tracking them in an index note, would close that gap, but it changes what is stored in the logout session, so I would like a maintainer's view before doing that.For the hash cookie,
bindLogoutRootToBrowsernow writesKC_AUTH_SESSION_HASHwhenever it writesAUTH_SESSION_ID, so both cookies name the same root. It still writes neither for a recovered root.The tests are in
LogoutSessionReuseTest: one browser test per item, a confirmation-path variant of the cross-session test, and a server-side test of the rebinding. The unfinished logout of the other user session is simulated by recording a client as logged out in its logout tab, which is whatbackchannelLogoutClientSessiondoes. Against a server built from main, the four browser tests fail (root removed, logout token not delivered in both cross-session variants, stale hash). With this change,./mvnw -f tests/base/pom.xml test -Dtest=LogoutSessionReuseTest,StaleAuthSessionLogoutTest,RPInitiatedFrontChannelLogoutTestpasses.I used an AI coding assistant (Claude) to help write this change; I reviewed and tested it.