From 8432513daa1c19437b48b2a87dcc97e093212008 Mon Sep 17 00:00:00 2001 From: Martin Kanis Date: Mon, 25 Jan 2021 17:18:08 +0100 Subject: [PATCH] KEYCLOAK-16908 Refactor UserSessionPersisterProvider --- .../AuthenticatedClientSessionAdapter.java | 4 ++++ .../InfinispanUserSessionProvider.java | 24 ++++++++++++++++++- .../JpaUserSessionPersisterProvider.java | 4 ++-- ...paUserSessionPersisterProviderFactory.java | 6 +++++ ...tentAuthenticatedClientSessionAdapter.java | 11 +++++++-- .../UserSessionPersisterProviderFactory.java | 22 ----------------- .../keycloak/models/UserSessionProvider.java | 9 ++++++- .../services/managers/ClientManager.java | 6 ----- .../services/managers/RealmManager.java | 6 ----- .../services/managers/UserSessionManager.java | 5 ---- .../scheduled/ClearExpiredUserSessions.java | 2 -- .../model/UserSessionInitializerTest.java | 7 +++--- 12 files changed, 56 insertions(+), 50 deletions(-) diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/AuthenticatedClientSessionAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/AuthenticatedClientSessionAdapter.java index a76639d3fb..c32cea2256 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/AuthenticatedClientSessionAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/AuthenticatedClientSessionAdapter.java @@ -26,6 +26,7 @@ import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserSessionModel; +import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.models.sessions.infinispan.changes.InfinispanChangelogBasedTransaction; import org.keycloak.models.sessions.infinispan.changes.SessionEntityWrapper; import org.keycloak.models.sessions.infinispan.changes.ClientSessionUpdateTask; @@ -85,6 +86,9 @@ public class AuthenticatedClientSessionAdapter implements AuthenticatedClientSes */ @Override public void detachFromUserSession() { + if (this.userSession.isOffline()) { + kcSession.getProvider(UserSessionPersisterProvider.class).removeClientSession(userSession.getId(), client.getId(), true); + } // Intentionally do not remove the clientUUID from the user session, invalid session is handled // as nonexistent in org.keycloak.models.sessions.infinispan.UserSessionAdapter.getAuthenticatedClientSessions() this.userSession = null; diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java index 5728e6773e..20db30bf0a 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java @@ -36,6 +36,7 @@ import org.keycloak.models.UserLoginFailureModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.UserSessionProvider; +import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.models.sessions.infinispan.changes.Tasks; import org.keycloak.models.sessions.infinispan.changes.sessions.CrossDCLastSessionRefreshStore; import org.keycloak.models.sessions.infinispan.changes.sessions.PersisterLastSessionRefreshStore; @@ -467,6 +468,7 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { log.debugf("Removing expired sessions"); removeExpiredUserSessions(realm); removeExpiredOfflineUserSessions(realm); + session.getProvider(UserSessionPersisterProvider.class).removeExpired(realm); } private void removeExpiredUserSessions(RealmModel realm) { @@ -613,7 +615,8 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { removeLocalUserSessions(realmId, false); } - private void removeLocalUserSessions(String realmId, boolean offline) { + // public for usage in the testsuite + public void removeLocalUserSessions(String realmId, boolean offline) { FuturesHelper futures = new FuturesHelper(); Cache> cache = getCache(offline); @@ -725,6 +728,11 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { clusterEventsSenderTx.addEvent( RealmRemovedSessionEvent.createEvent(RealmRemovedSessionEvent.class, InfinispanUserSessionProviderFactory.REALM_REMOVED_SESSION_EVENT, session, realm.getId(), true), ClusterProvider.DCNotify.LOCAL_DC_ONLY); + + UserSessionPersisterProvider sessionsPersister = session.getProvider(UserSessionPersisterProvider.class); + if (sessionsPersister != null) { + sessionsPersister.onRealmRemoved(realm); + } } protected void onRealmRemovedEvent(String realmId) { @@ -738,6 +746,10 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { // clusterEventsSenderTx.addEvent( // ClientRemovedSessionEvent.createEvent(ClientRemovedSessionEvent.class, InfinispanUserSessionProviderFactory.CLIENT_REMOVED_SESSION_EVENT, session, realm.getId(), true), // ClusterProvider.DCNotify.LOCAL_DC_ONLY); + UserSessionPersisterProvider sessionsPersister = session.getProvider(UserSessionPersisterProvider.class); + if (sessionsPersister != null) { + sessionsPersister.onClientRemoved(realm, client); + } } protected void onClientRemovedEvent(String realmId, String clientUuid) { @@ -750,6 +762,11 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { removeUserSessions(realm, user, false); removeUserLoginFailure(realm, user.getId()); + + UserSessionPersisterProvider persisterProvider = session.getProvider(UserSessionPersisterProvider.class); + if (persisterProvider != null) { + persisterProvider.onUserRemoved(realm, user); + } } @Override @@ -803,6 +820,8 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { offlineUserSession.getEntity().setStarted(currentTime); offlineUserSession.setLastSessionRefresh(currentTime); + session.getProvider(UserSessionPersisterProvider.class).createUserSession(userSession, true); + return offlineUserSession; } @@ -828,6 +847,7 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { if (userSessionEntity != null) { removeUserSession(userSessionEntity, true); } + session.getProvider(UserSessionPersisterProvider.class).removeUserSession(userSession.getId(), true); } @Override @@ -842,6 +862,8 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { // update timestamp to current time offlineClientSession.setTimestamp(Time.currentTime()); + session.getProvider(UserSessionPersisterProvider.class).createClientSession(clientSession, true); + return offlineClientSession; } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java index 8ea2e5492e..76df84f339 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java @@ -85,7 +85,7 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv @Override public void createClientSession(AuthenticatedClientSessionModel clientSession, boolean offline) { - PersistentAuthenticatedClientSessionAdapter adapter = new PersistentAuthenticatedClientSessionAdapter(clientSession); + PersistentAuthenticatedClientSessionAdapter adapter = new PersistentAuthenticatedClientSessionAdapter(session, clientSession); PersistentClientSessionModel model = adapter.getUpdatedModel(); PersistentClientSessionEntity entity = new PersistentClientSessionEntity(); @@ -318,7 +318,7 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv model.setUserId(userSession.getUserId()); model.setTimestamp(entity.getTimestamp()); model.setData(entity.getData()); - return new PersistentAuthenticatedClientSessionAdapter(model, realm, client, userSession); + return new PersistentAuthenticatedClientSessionAdapter(session, model, realm, client, userSession); } @Override diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProviderFactory.java b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProviderFactory.java index 5613b2da7d..d2d13bf903 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProviderFactory.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProviderFactory.java @@ -20,6 +20,7 @@ package org.keycloak.models.jpa.session; import org.keycloak.Config; import org.keycloak.connections.jpa.JpaConnectionProvider; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.models.session.UserSessionPersisterProviderFactory; @@ -43,6 +44,11 @@ public class JpaUserSessionPersisterProviderFactory implements UserSessionPersis } + @Override + public void postInit(KeycloakSessionFactory factory) { + + } + @Override public void close() { diff --git a/server-spi-private/src/main/java/org/keycloak/models/session/PersistentAuthenticatedClientSessionAdapter.java b/server-spi-private/src/main/java/org/keycloak/models/session/PersistentAuthenticatedClientSessionAdapter.java index 2bb13b73ae..362038fd3b 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/session/PersistentAuthenticatedClientSessionAdapter.java +++ b/server-spi-private/src/main/java/org/keycloak/models/session/PersistentAuthenticatedClientSessionAdapter.java @@ -20,6 +20,7 @@ package org.keycloak.models.session; import com.fasterxml.jackson.annotation.JsonProperty; import org.keycloak.models.AuthenticatedClientSessionModel; import org.keycloak.models.ClientModel; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.UserSessionModel; @@ -36,6 +37,7 @@ import java.util.Set; */ public class PersistentAuthenticatedClientSessionAdapter implements AuthenticatedClientSessionModel { + private final KeycloakSession session; private final PersistentClientSessionModel model; private final RealmModel realm; private final ClientModel client; @@ -43,7 +45,7 @@ public class PersistentAuthenticatedClientSessionAdapter implements Authenticate private PersistentClientSessionData data; - public PersistentAuthenticatedClientSessionAdapter(AuthenticatedClientSessionModel clientSession) { + public PersistentAuthenticatedClientSessionAdapter(KeycloakSession session, AuthenticatedClientSessionModel clientSession) { data = new PersistentClientSessionData(); data.setAction(clientSession.getAction()); data.setAuthMethod(clientSession.getProtocol()); @@ -56,12 +58,14 @@ public class PersistentAuthenticatedClientSessionAdapter implements Authenticate model.setUserSessionId(clientSession.getUserSession().getId()); model.setTimestamp(clientSession.getTimestamp()); + this.session = session; realm = clientSession.getRealm(); client = clientSession.getClient(); userSession = clientSession.getUserSession(); } - public PersistentAuthenticatedClientSessionAdapter(PersistentClientSessionModel model, RealmModel realm, ClientModel client, UserSessionModel userSession) { + public PersistentAuthenticatedClientSessionAdapter(KeycloakSession session, PersistentClientSessionModel model, RealmModel realm, ClientModel client, UserSessionModel userSession) { + this.session = session; this.model = model; this.realm = realm; this.client = client; @@ -115,6 +119,9 @@ public class PersistentAuthenticatedClientSessionAdapter implements Authenticate @Override public void detachFromUserSession() { + if (this.userSession.isOffline()) { + session.getProvider(UserSessionPersisterProvider.class).removeClientSession(userSession.getId(), client.getId(), true); + } setUserSession(null); } diff --git a/server-spi-private/src/main/java/org/keycloak/models/session/UserSessionPersisterProviderFactory.java b/server-spi-private/src/main/java/org/keycloak/models/session/UserSessionPersisterProviderFactory.java index 350f4f977e..2c0b98c514 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/session/UserSessionPersisterProviderFactory.java +++ b/server-spi-private/src/main/java/org/keycloak/models/session/UserSessionPersisterProviderFactory.java @@ -17,32 +17,10 @@ package org.keycloak.models.session; -import org.keycloak.models.KeycloakSessionFactory; -import org.keycloak.models.UserModel; -import org.keycloak.provider.ProviderEvent; -import org.keycloak.provider.ProviderEventListener; import org.keycloak.provider.ProviderFactory; /** * @author Marek Posolda */ public interface UserSessionPersisterProviderFactory extends ProviderFactory { - - @Override - default void postInit(KeycloakSessionFactory factory) { - factory.register(new ProviderEventListener() { - - @Override - public void onEvent(ProviderEvent event) { - if (event instanceof UserModel.UserRemovedEvent) { - UserModel.UserRemovedEvent userRemovedEvent = (UserModel.UserRemovedEvent) event; - - UserSessionPersisterProvider provider = userRemovedEvent.getKeycloakSession().getProvider(UserSessionPersisterProvider.class, getId()); - provider.onUserRemoved(userRemovedEvent.getRealm(), userRemovedEvent.getUser()); - } - } - - }); - } - } diff --git a/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java b/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java index f60ce4cea7..777714de8f 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java +++ b/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java @@ -138,8 +138,15 @@ public interface UserSessionProvider extends Provider { void removeUserSession(RealmModel realm, UserSessionModel session); void removeUserSessions(RealmModel realm, UserModel user); - /** Implementation doesn't need to propagate removal of expired userSessions to userSessionPersister. Cleanup on persister will be called separately **/ + /** + * Removes expired user sessions owned by this realm from this provider. + * If this `UserSessionProvider` uses `UserSessionPersister`, the removal of the expired + * {@link UserSessionModel user sessions} is also propagated to relevant `UserSessionPersister`. + * + * @param realm {@link RealmModel} Realm where all the expired user sessions to be removed from. + */ void removeExpired(RealmModel realm); + void removeUserSessions(RealmModel realm); UserLoginFailureModel getUserLoginFailure(RealmModel realm, String userId); diff --git a/services/src/main/java/org/keycloak/services/managers/ClientManager.java b/services/src/main/java/org/keycloak/services/managers/ClientManager.java index 0eb5aa1835..dea2176837 100644 --- a/services/src/main/java/org/keycloak/services/managers/ClientManager.java +++ b/services/src/main/java/org/keycloak/services/managers/ClientManager.java @@ -30,7 +30,6 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserManager; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionProvider; -import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.models.utils.RepresentationToModel; import org.keycloak.protocol.LoginProtocol; import org.keycloak.protocol.LoginProtocolFactory; @@ -100,11 +99,6 @@ public class ClientManager { sessions.onClientRemoved(realm, client); } - UserSessionPersisterProvider sessionsPersister = realmManager.getSession().getProvider(UserSessionPersisterProvider.class); - if (sessionsPersister != null) { - sessionsPersister.onClientRemoved(realm, client); - } - AuthenticationSessionProvider authSessions = realmManager.getSession().authenticationSessions(); if (authSessions != null) { authSessions.onClientRemoved(realm, client); diff --git a/services/src/main/java/org/keycloak/services/managers/RealmManager.java b/services/src/main/java/org/keycloak/services/managers/RealmManager.java index 6fec811891..9f3feb4c65 100755 --- a/services/src/main/java/org/keycloak/services/managers/RealmManager.java +++ b/services/src/main/java/org/keycloak/services/managers/RealmManager.java @@ -33,7 +33,6 @@ import org.keycloak.models.RealmProvider; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionProvider; -import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.models.utils.DefaultAuthenticationFlows; import org.keycloak.models.utils.DefaultClientScopes; import org.keycloak.models.utils.DefaultRequiredActions; @@ -265,11 +264,6 @@ public class RealmManager { sessions.onRealmRemoved(realm); } - UserSessionPersisterProvider sessionsPersister = session.getProvider(UserSessionPersisterProvider.class); - if (sessionsPersister != null) { - sessionsPersister.onRealmRemoved(realm); - } - AuthenticationSessionProvider authSessions = session.authenticationSessions(); if (authSessions != null) { authSessions.onRealmRemoved(realm); diff --git a/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java b/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java index c8a90be521..31cba06562 100644 --- a/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java @@ -107,7 +107,6 @@ public class UserSessionManager { } clientSession.detachFromUserSession(); - persister.removeClientSession(userSession.getId(), client.getId(), true); checkOfflineUserSessionHasClientSessions(realm, user, userSession); anyRemoved.set(true); } @@ -121,7 +120,6 @@ public class UserSessionManager { logger.tracef("Removing offline user session '%s' for user '%s' ", userSession.getId(), userSession.getLoginUsername()); } kcSession.sessions().removeOfflineUserSession(userSession.getRealm(), userSession); - persister.removeUserSession(userSession.getId(), true); } public boolean isOfflineTokenAllowed(ClientSessionContext clientSessionCtx) { @@ -141,7 +139,6 @@ public class UserSessionManager { } UserSessionModel offlineUserSession = kcSession.sessions().createOfflineUserSession(userSession); - persister.createUserSession(offlineUserSession, true); return offlineUserSession; } @@ -152,7 +149,6 @@ public class UserSessionManager { } kcSession.sessions().createOfflineClientSession(clientSession, offlineUserSession); - persister.createClientSession(clientSession, true); } // Check if userSession has any offline clientSessions attached to it. Remove userSession if not @@ -166,6 +162,5 @@ public class UserSessionManager { logger.tracef("Removing offline userSession for user %s as it doesn't have any client sessions attached. UserSessionID: %s", user.getUsername(), userSession.getId()); } kcSession.sessions().removeOfflineUserSession(realm, userSession); - persister.removeUserSession(userSession.getId(), true); } } diff --git a/services/src/main/java/org/keycloak/services/scheduled/ClearExpiredUserSessions.java b/services/src/main/java/org/keycloak/services/scheduled/ClearExpiredUserSessions.java index 3ddb19e5b3..e6741d9232 100755 --- a/services/src/main/java/org/keycloak/services/scheduled/ClearExpiredUserSessions.java +++ b/services/src/main/java/org/keycloak/services/scheduled/ClearExpiredUserSessions.java @@ -21,7 +21,6 @@ import org.jboss.logging.Logger; import org.keycloak.common.util.Time; import org.keycloak.models.KeycloakSession; import org.keycloak.models.UserSessionProvider; -import org.keycloak.models.session.UserSessionPersisterProvider; import org.keycloak.timer.ScheduledTask; /** @@ -41,7 +40,6 @@ public class ClearExpiredUserSessions implements ScheduledTask { session.realms().getRealmsStream().forEach(realm -> { sessions.removeExpired(realm); session.authenticationSessions().removeExpired(realm); - session.getProvider(UserSessionPersisterProvider.class).removeExpired(realm); }); long took = Time.currentTimeMillis() - currentTimeMillis; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java index f37ede6c5e..3f1abd9166 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java @@ -32,6 +32,7 @@ import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.UserSessionProvider; import org.keycloak.models.UserSessionProviderFactory; +import org.keycloak.models.sessions.infinispan.InfinispanUserSessionProvider; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.representations.idm.RealmRepresentation; @@ -210,13 +211,13 @@ public class UserSessionInitializerTest extends AbstractTestRealmKeycloakTest { KeycloakSession currentSession = inheritClientConnection(session, createSessionPersister3); RealmModel realm = currentSession.realms().getRealmByName(realmName); - // Delete cache (persisted sessions are still kept) - currentSession.sessions().onRealmRemoved(realm); + // Delete local user cache (persisted sessions are still kept) + InfinispanUserSessionProvider userSessionProvider = (InfinispanUserSessionProvider) currentSession.getProvider(UserSessionProvider.class); + userSessionProvider.removeLocalUserSessions(realm.getId(), true); // Clear ispn cache to ensure initializerState is removed as well InfinispanConnectionProvider infinispan = currentSession.getProvider(InfinispanConnectionProvider.class); infinispan.getCache(InfinispanConnectionProvider.WORK_CACHE_NAME).clear(); - }); KeycloakModelUtils.runJobInTransaction(session.getKeycloakSessionFactory(), (KeycloakSession createSessionPersister4) -> {