Invalidate in progress authentication sessions on credential reset by gaoyikeshuer · Pull Request #50702 · keycloak/keycloak

gaoyikeshuer · 2026-07-08T15:45:15Z

The problem right now is when user reset their credentials and picks "Sign out of other devices", keycloak logged out their finished sessions but not their half-finished ones. An attacker who authenticated with the old password on such stop could complete it after the reset and gain the access.

This PR's idea is to find a user's in-progress authentication sessions, and on the 'reset with sign out others' step, delete all of them except the one is doing the reset.

Closes #50621

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2026-07-13T11:35:36Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest#crudWithoutFailover

Keycloak CI - Clustering IT

org.opentest4j.AssertionFailedError: expected: <false> but was: <true>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertFalse.failNotFalse(AssertFalse.java:63)
	at org.junit.jupiter.api.AssertFalse.assertFalse(AssertFalse.java:36)
...

Report flaky test

org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest#crudWithFailover

Keycloak CI - Clustering IT

jakarta.ws.rs.WebApplicationException: Create method returned status Forbidden (Code: 403); expected status: Created (201). Response body: {"error":"HTTP 403 Forbidden"}
	at org.keycloak.testsuite.admin.ApiUtil.getCreatedId(ApiUtil.java:38)
	at org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest.createEntity(ClientScopeInvalidationClusterTest.java:56)
	at org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest.createEntity(ClientScopeInvalidationClusterTest.java:32)
	at org.keycloak.testsuite.cluster.AbstractInvalidationClusterTest.createEntityOnCurrentFailNode(AbstractInvalidationClusterTest.java:111)
...

Report flaky test

Copilot

Pull request overview

Adds invalidation of in-progress authentication sessions when credential reset requests sign-out from other devices.

Changes:

Adds authenticated-user session lookup to the provider SPI.
Implements lookup for JPA and Infinispan stores.
Integrates invalidation and adds model tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`AuthenticationSessionTest.java`	Tests lookup and removal behavior.
`AuthenticatorUtil.java`	Invalidates other authentication sessions.
`AuthenticationSessionProvider.java`	Adds the lookup SPI method.
JPA `RootAuthenticationSessionEntity.java`	Adds the user lookup query.
`JpaAuthenticationSessionProvider.java`	Implements JPA lookup.
`RemoteInfinispanAuthenticationSessionProvider.java`	Implements remote-cache lookup.
`InfinispanAuthenticationSessionProvider.java`	Implements embedded-cache lookup.
Infinispan `RootAuthenticationSessionEntity.java`	Adds user-session matching helper.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

+    default Stream<RootAuthenticationSessionModel> getRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user) {
+        return Stream.empty();
+    }


+    public Stream<RootAuthenticationSessionModel> getRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user) {
+        return getEntityManager()
+                .createNamedQuery("findRootAuthSessionIdsByUser", String.class)
+                .setParameter("realmId", realm.getId())
+                .setParameter("userId", user.getId())
+                .getResultList()
+                .stream()
+                .map(id -> getRootAuthenticationSession(realm, id))
+                .filter(Objects::nonNull);
+    }


+        return StreamSupport.stream(sessionTx.getCache().entrySet().stream()
+                        .filter(SessionWrapperPredicate.create(realm.getId()))
+                        .spliterator(), false)
+                .map(entry -> entry.getValue().getEntity())
+                .filter(entity -> entity.hasAuthenticationSessionForUser(user.getId()))
+                .map(entity -> (RootAuthenticationSessionModel) wrap(realm, entity));


mabartos

@gaoyikeshuer Thanks for the PR! Just put a comment there, and we should wait for someone from the @keycloak/sre team to look at it (@pruivo @ryanemerson).

Moreover, it'd be good to check all the Copilot's suggestions, and would be best to comment on it why it's invalid.

mabartos · 2026-07-23T08:54:34Z

+
+        return QueryHelper.streamAll(query, 100, Function.identity())
+                .filter(entity -> entity.hasAuthenticationSessionForUser(user.getId()))
+                .map(RootAuthenticationSessionEntity::getId)


So we first fetch the IDs and drop the entities, and then getting it again? It'd be nice to avoid it, but probably @pruivo will navigate you for the optimal solution.

My Claude assistant suggested sth like this:

return QueryHelper.streamAll(query, 100, Function.identity()) .filter(entity -> entity.hasAuthenticationSessionForUser(user.getId())) .map(entity -> { var updater = transaction.wrap(entity.getId(), entity, Updater.NO_VERSION); updater.initialize(session, realm, authSessionsLimit); return (RootAuthenticationSessionModel) updater; });

But the transaction wrapping needs to be investigated more.

Thanks @mabartos, yes you are right it fetches the entities, reduces them to IDs, then refetch each via getRootAuthenticationSession. It's redundant now. according to @pruivo , if we delete the find method and do a direct server side delete, then we don't need this method at all. My only concern is the remote part. it would mean adding an index to the cache object to me, idk how big the risk is on clustered side. would like to wait for the final decision from team before implementing

pedroigor

Thanks, @gaoyikeshuer.

The changes LGTM. We probably want a review from @keycloak/sre mainly because of iterating over all sessions to then filter them out by user id, in-memory. Perhaps there is an alternative we do not know about that is more optimal.

pruivo · 2026-08-04T08:17:53Z

I'm sorry for the late reply; I was on PTO.

I would like to suggest another option: in the AuthenticationSessionProvider, add a method to remove all sessions for a user.
Instead of fetching all data to a single node for filtering and sending individual deletes, we could make a single delete request.

WDYT @mabartos @pedroigor @gaoyikeshuer ?

mabartos · 2026-08-05T11:00:26Z

I would like to suggest another option: in the AuthenticationSessionProvider, add a method to remove all sessions for a user.
Instead of fetching all data to a single node for filtering and sending individual deletes, we could make a single delete request.

@pruivo Yes, IMO, it might work nicely :) Delete all except the current one. @rmartinc We should be fine, right?

cc: @gaoyikeshuer

gaoyikeshuer · 2026-08-06T09:53:16Z

I'm sorry for the late reply; I was on PTO.

I would like to suggest another option: in the AuthenticationSessionProvider, add a method to remove all sessions for a user. Instead of fetching all data to a single node for filtering and sending individual deletes, we could make a single delete request.

WDYT @mabartos @pedroigor @gaoyikeshuer ?

Hi @pruivo , sorry for the late reply. I was checking if this way is feasible. I assume this way would be similar to the one removeUserSessions(realm, user) which could delete them all in one request. but I check auth session record which doesn't have user id at the top. it only has id, realm, and a map of child sessions. and the user id lives in the child which doesn't have index. I'm thinking if we go this way should we add the index on the root? but this will change the stored data format (the cache object field)? Please correct me if I'm wrong.

WDYT @pedroigor @rmartinc @mabartos

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/changes/remote/remover/query/AuthenticationSessionQueryConditionalRemover.java:83

This remote delete predicate references id, but RootAuthenticationSessionEntity.getId() has no indexing annotation; only realmId and the new authenticatedUserIds field are indexed. With remote indexed querying, the non-null keep-id path used by credential reset will therefore fail query validation instead of deleting sessions; index id or use the supported cache-key predicate.

            return "(realmId = :%s && authenticatedUserIds = :%s && id != :%s)".formatted(realmParameter, userParameter, keepParameter);

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

server-spi/src/main/java/org/keycloak/sessions/AuthenticationSessionProvider.java:95

This security operation silently does nothing for any custom AuthenticationSessionProvider that inherits the default, leaving those deployments vulnerable while credential reset appears successful. Make the operation mandatory or otherwise fail closed rather than providing a no-op default.

    default void removeRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user, String rootAuthenticationSessionIdToKeep) {
    }

services/src/main/java/org/keycloak/authentication/AuthenticatorUtil.java:220

This is a snapshot deletion with no synchronization against concurrent authentication. Because UpdatePassword.java:143-148 invokes logout before changing the credential, a request that validates the old password concurrently can persist its authenticated session after this removal and still complete; use a credential/invalidation generation checked at flow completion or serialize these operations.

        session.authenticationSessions().removeRootAuthenticationSessionsByAuthenticatedUser(realm, user,
                authSession.getParentSession().getId());

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanAuthenticationSessionProvider.java:107

Every user-triggered “sign out other devices” operation now traverses and deserializes the entire distributed authentication-session cache. This is O(all active authentication sessions) and can become a cluster-wide bottleneck at scale; use a user-to-root lookup/index or another targeted removal mechanism.

        sessionTx.getCache().entrySet().stream()
                .filter(SessionWrapperPredicate.create(realm.getId()))
                .filter(entry -> entry.getValue().getEntity().hasAuthenticationSessionForUser(user.getId()))
                .map(entry -> entry.getKey())
                .filter(rootSessionId -> !Objects.equals(rootSessionId, rootAuthenticationSessionIdToKeep))
                .toList()
                .forEach(rootSessionId -> sessionTx.addTask(rootSessionId, Tasks.removeSync()));

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/RootAuthenticationSessionEntity.java:87

Remote entries created before upgrade, or by old nodes during a rolling upgrade, do not contain this new protobuf field. External authentication-session caches use indexing.startupMode=NONE, so the indexed removeByUser query will miss those existing sessions; add an upgrade-safe fallback or reindex/migration strategy.

    @ProtoField(value = 5, collectionImplementation = HashSet.class)
    @Basic
    public Set<String> getAuthenticatedUserIds() {

+     * Removes all root authentication sessions of the given realm that hold an in-progress authentication session
+     * for the given authenticated user, except for the provided root authentication session id.


Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/changes/remote/remover/query/AuthenticationSessionQueryConditionalRemover.java:83

The production caller always supplies a keep ID, but RootAuthenticationSessionEntity.id is only a ProtoStream field and is not marked @Basic, unlike the other fields used by these indexed Hot Rod queries. This predicate therefore cannot be executed against the remote-cache index; index the ID field or exclude the cache key using a supported key predicate.

            return "(realmId = :%s && authenticatedUserIds = :%s && id != :%s)".formatted(realmParameter, userParameter, keepParameter);

+    default void removeRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user, String rootAuthenticationSessionIdToKeep) {
+    }


Closes keycloak#50621 Signed-off-by: Yike Gao <yikegao8@gmail.com>

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

server-spi/src/main/java/org/keycloak/sessions/AuthenticationSessionProvider.java:95

This default no-op silently leaves the vulnerability open for any third-party AuthenticationSessionProvider that has not implemented the new method, while the credential reset still succeeds. The security-sensitive operation should fail closed or require an explicit provider capability/implementation rather than silently skipping invalidation.

    default void removeRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user, String rootAuthenticationSessionIdToKeep) {
    }

services/src/main/java/org/keycloak/authentication/AuthenticatorUtil.java:219

Keeping the parent root preserves every sibling tab in that browser, since RootAuthenticationSessionModel is a multi-tab container (server-spi/src/main/java/org/keycloak/sessions/RootAuthenticationSessionModel.java:26-27). A parked sibling flow for this user can therefore still submit after the reset; preserve only authSession.getTabId() and invalidate the other matching child sessions, removing roots only when empty.

        session.authenticationSessions().removeRootAuthenticationSessionsByAuthenticatedUser(realm, user,
                authSession.getParentSession().getId());

pruivo · 2026-08-11T12:49:43Z

I'm sorry for the late reply; I was on PTO.
I would like to suggest another option: in the AuthenticationSessionProvider, add a method to remove all sessions for a user. Instead of fetching all data to a single node for filtering and sending individual deletes, we could make a single delete request.
WDYT @mabartos @pedroigor @gaoyikeshuer ?

Hi @pruivo , sorry for the late reply. I was checking if this way is feasible. I assume this way would be similar to the one removeUserSessions(realm, user) which could delete them all in one request. but I check auth session record which doesn't have user id at the top. it only has id, realm, and a map of child sessions. and the user id lives in the child which doesn't have index. I'm thinking if we go this way should we add the index on the root? but this will change the stored data format (the cache object field)? Please correct me if I'm wrong.

WDYT @pedroigor @rmartinc @mabartos

I missed your comment. If you are talking about RemoteInfinispanAuthenticationSessionProvider, you can retrieve the data into Keycloak for filtering. This provider is going to be deprecated for removal in the next release, so I'm not too worried about the performance impact.
The InfinispanAuthenticationSessionProvider and JpaAuthenticationSessionProvider are the ones I care.

About InfinispanAuthenticationSessionProvider, it can be simplified by using removeIf: sessionTx.getCache().values().removeIf(...). The Predicate must be marshalled with ProtoStream, so each node will loop through it own data and never do remote calls. You have examples in this package:
https://github.com/keycloak/keycloak/tree/a2c1055f8df21b53b969f922fb9a044a76d22604/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/stream
Let me know if you need any help.

pruivo · 2026-08-11T12:52:31Z

@pedroigor @rmartinc @mabartos, just for my curiosity, why is the user ID only present in the child session? How is it possible RootAuthenticationSessionModel to be shared between different users?

rmartinc · 2026-08-12T08:38:32Z

@pedroigor @rmartinc @mabartos, just for my curiosity, why is the user ID only present in the child session? How is it possible RootAuthenticationSessionModel to be shared between different users?

AFAIUI the root authentication session is linked to the auth session id cookie, so all the tabs used for login in the same browser share the same root auth session. In each tab the final user can theoretically start the login with a different user. For example in tab1 I can start the login with user1 and stop at OTP for example, and in tab2 I can perform and complete the login with user2. In the moment the user is assigned (for example after password, but pending 2FA or required actions), it is assigned in the AuthenticationSessionModel. So you can have two auth sessions in the same root with different users (it's not common but possible).

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 3c44f93 to bdb2bdb Compare July 13, 2026 10:30

keycloak-github-bot Bot added the flaky-test label Jul 13, 2026

keycloak-github-bot Bot reviewed Jul 13, 2026

View reviewed changes

stianst added the team/core-authn label Jul 14, 2026

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from bdb2bdb to 2cbbd79 Compare July 14, 2026 13:12

gaoyikeshuer marked this pull request as ready for review July 14, 2026 13:15

gaoyikeshuer requested a review from a team as a code owner July 14, 2026 13:15

Copilot AI balanced review requested due to automatic review settings July 14, 2026 13:15

gaoyikeshuer requested review from a team as code owners July 14, 2026 13:15

Copilot started reviewing on behalf of gaoyikeshuer July 14, 2026 13:15 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 14, 2026 15:44

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 2cbbd79 to e5039c8 Compare July 14, 2026 15:44

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Copilot started reviewing on behalf of gaoyikeshuer July 14, 2026 16:28 View session

Copilot AI review requested due to automatic review settings July 15, 2026 10:05

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from e5039c8 to 5079e0f Compare July 15, 2026 10:05

Copilot started reviewing on behalf of gaoyikeshuer July 15, 2026 10:21 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

mabartos reviewed Jul 23, 2026

View reviewed changes

pedroigor reviewed Jul 27, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 11, 2026 09:10

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 5079e0f to 910124f Compare August 11, 2026 09:10

Copilot started reviewing on behalf of gaoyikeshuer August 11, 2026 09:11 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Comment thread services/src/main/java/org/keycloak/authentication/AuthenticatorUtil.java

Copilot AI review requested due to automatic review settings August 11, 2026 10:27

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 910124f to 8ed29d1 Compare August 11, 2026 10:27

Copilot started reviewing on behalf of gaoyikeshuer August 11, 2026 10:27 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 11, 2026 11:48

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 8ed29d1 to 12cfc09 Compare August 11, 2026 11:48

Copilot started reviewing on behalf of gaoyikeshuer August 11, 2026 11:48 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Invalidate in progress authentication sessions on credential reset

7392225

Closes keycloak#50621 Signed-off-by: Yike Gao <yikegao8@gmail.com>

Copilot AI review requested due to automatic review settings August 11, 2026 12:28

gaoyikeshuer force-pushed the invalidate-in-porgress-authentication-50632 branch from 12cfc09 to 7392225 Compare August 11, 2026 12:28

Copilot started reviewing on behalf of gaoyikeshuer August 11, 2026 12:29 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

		* Removes all root authentication sessions of the given realm that hold an in-progress authentication session
		* for the given authenticated user, except for the provided root authentication session id.

		default void removeRootAuthenticationSessionsByAuthenticatedUser(RealmModel realm, UserModel user, String rootAuthenticationSessionIdToKeep) {
		}

Uh oh!

Conversation

gaoyikeshuer commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot Bot commented Jul 13, 2026

Unreported flaky test detected

org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest#crudWithoutFailover

org.keycloak.testsuite.cluster.ClientScopeInvalidationClusterTest#crudWithFailover

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

mabartos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabartos Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

gaoyikeshuer Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

pedroigor left a comment

Choose a reason for hiding this comment

Uh oh!

pruivo commented Aug 4, 2026

Uh oh!

mabartos commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaoyikeshuer commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

pruivo commented Aug 11, 2026

gaoyikeshuer commented Jul 8, 2026 •

edited

Loading

mabartos left a comment •

edited

Loading

mabartos commented Aug 5, 2026 •

edited

Loading

gaoyikeshuer commented Aug 6, 2026 •

edited

Loading