diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java index e45debdc6f..a3f30baa28 100755 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java @@ -22,6 +22,7 @@ import org.keycloak.models.UserFederationProvider; import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserModel; import org.keycloak.common.constants.KerberosConstants; +import org.keycloak.services.managers.UserManager; /** * @author Marek Posolda @@ -242,7 +243,7 @@ public class KerberosFederationProvider implements UserFederationProvider { logger.warn("User with username " + username + " already exists and is linked to provider [" + model.getDisplayName() + "] but kerberos principal is not correct. Kerberos principal on user is: " + user.getFirstAttribute(KERBEROS_PRINCIPAL)); logger.warn("Will re-create user"); - session.userStorage().removeUser(realm, user); + new UserManager(session).removeUser(realm, user, session.userStorage()); } } } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java index 61786ccc69..07eb752e55 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java @@ -26,6 +26,7 @@ import org.keycloak.models.UserFederationProvider; import org.keycloak.models.UserFederationProviderModel; import org.keycloak.models.UserModel; import org.keycloak.common.constants.KerberosConstants; +import org.keycloak.services.managers.UserManager; import java.util.ArrayList; import java.util.Arrays; @@ -465,7 +466,7 @@ public class LDAPFederationProvider implements UserFederationProvider { logger.warnf("User with username [%s] aready exists and is linked to provider [%s] but is not valid. Stale LDAP_ID on local user is: %s", username, model.getDisplayName(), user.getFirstAttribute(LDAPConstants.LDAP_ID)); logger.warn("Will re-create user"); - session.userStorage().removeUser(realm, user); + new UserManager(session).removeUser(realm, user, session.userStorage()); } } } 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 5fa648ed36..56c39643dc 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 @@ -150,8 +150,12 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv @Override public void onUserRemoved(RealmModel realm, UserModel user) { - int num = em.createNamedQuery("deleteClientSessionsByUser").setParameter("userId", user.getId()).executeUpdate(); - num = em.createNamedQuery("deleteUserSessionsByUser").setParameter("userId", user.getId()).executeUpdate(); + onUserRemoved(realm, user.getId()); + } + + private void onUserRemoved(RealmModel realm, String userId) { + int num = em.createNamedQuery("deleteClientSessionsByUser").setParameter("userId", userId).executeUpdate(); + num = em.createNamedQuery("deleteUserSessionsByUser").setParameter("userId", userId).executeUpdate(); } @Override @@ -184,7 +188,16 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv List result = new ArrayList<>(); List userSessionIds = new ArrayList<>(); for (PersistentUserSessionEntity entity : results) { - result.add(toAdapter(entity)); + RealmModel realm = session.realms().getRealm(entity.getRealmId()); + UserModel user = session.users().getUserById(entity.getUserId(), realm); + + // Case when user was deleted in the meantime + if (user == null) { + onUserRemoved(realm, entity.getUserId()); + return loadUserSessions(firstResult, maxResults, offline); + } + + result.add(toAdapter(realm, user, entity)); userSessionIds.add(entity.getUserSessionId()); } @@ -217,10 +230,7 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv return result; } - private PersistentUserSessionAdapter toAdapter(PersistentUserSessionEntity entity) { - RealmModel realm = session.realms().getRealm(entity.getRealmId()); - UserModel user = session.users().getUserById(entity.getUserId(), realm); - + private PersistentUserSessionAdapter toAdapter(RealmModel realm, UserModel user, PersistentUserSessionEntity entity) { PersistentUserSessionModel model = new PersistentUserSessionModel(); model.setUserSessionId(entity.getUserSessionId()); model.setLastSessionRefresh(entity.getLastSessionRefresh()); diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/MongoUserSessionPersisterProvider.java b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/MongoUserSessionPersisterProvider.java index f23e7fbe99..47b719f30e 100644 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/MongoUserSessionPersisterProvider.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/MongoUserSessionPersisterProvider.java @@ -191,8 +191,12 @@ public class MongoUserSessionPersisterProvider implements UserSessionPersisterPr @Override public void onUserRemoved(RealmModel realm, UserModel user) { + onUserRemoved(realm, user.getId()); + } + + private void onUserRemoved(RealmModel realm, String userId) { DBObject query = new QueryBuilder() - .and("userId").is(user.getId()) + .and("userId").is(userId) .get(); getMongoStore().removeEntities(MongoOnlineUserSessionEntity.class, query, false, invocationContext); getMongoStore().removeEntities(MongoOfflineUserSessionEntity.class, query, false, invocationContext); @@ -263,16 +267,22 @@ public class MongoUserSessionPersisterProvider implements UserSessionPersisterPr List results = new LinkedList<>(); for (MongoUserSessionEntity entity : entities) { - PersistentUserSessionAdapter userSession = toAdapter(entity); + RealmModel realm = session.realms().getRealm(entity.getRealmId()); + UserModel user = session.users().getUserById(entity.getUserId(), realm); + + // Case when user was deleted in the meantime + if (user == null) { + onUserRemoved(realm, entity.getUserId()); + return loadUserSessions(firstResult, maxResults, offline); + } + + PersistentUserSessionAdapter userSession = toAdapter(realm, user, entity); results.add(userSession); } return results; } - private PersistentUserSessionAdapter toAdapter(PersistentUserSessionEntity entity) { - RealmModel realm = session.realms().getRealm(entity.getRealmId()); - UserModel user = session.users().getUserById(entity.getUserId(), realm); - + private PersistentUserSessionAdapter toAdapter(RealmModel realm, UserModel user, PersistentUserSessionEntity entity) { PersistentUserSessionModel model = new PersistentUserSessionModel(); model.setUserSessionId(entity.getId()); model.setLastSessionRefresh(entity.getLastSessionRefresh()); diff --git a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java index c58278b672..581fc3b1bc 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java +++ b/server-spi/src/main/java/org/keycloak/models/UserFederationManager.java @@ -2,6 +2,7 @@ package org.keycloak.models; import org.jboss.logging.Logger; import org.keycloak.models.utils.KeycloakModelUtils; +import org.keycloak.services.managers.UserManager; import java.util.ArrayList; import java.util.Arrays; @@ -113,7 +114,7 @@ public class UserFederationManager implements UserProvider { RealmModel realmModel = session.realms().getRealm(realm.getId()); if (realmModel == null) return; UserModel deletedUser = session.userStorage().getUserById(user.getId(), realmModel); - session.userStorage().removeUser(realmModel, deletedUser); + new UserManager(session).removeUser(realmModel, deletedUser, session.userStorage()); logger.debugf("Removed invalid user '%s'", user.getUsername()); } diff --git a/services/src/main/java/org/keycloak/services/managers/UserManager.java b/server-spi/src/main/java/org/keycloak/services/managers/UserManager.java similarity index 80% rename from services/src/main/java/org/keycloak/services/managers/UserManager.java rename to server-spi/src/main/java/org/keycloak/services/managers/UserManager.java index 686aedb576..54edd83f0c 100755 --- a/services/src/main/java/org/keycloak/services/managers/UserManager.java +++ b/server-spi/src/main/java/org/keycloak/services/managers/UserManager.java @@ -3,6 +3,7 @@ package org.keycloak.services.managers; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.models.UserProvider; import org.keycloak.models.UserSessionProvider; import org.keycloak.models.session.UserSessionPersisterProvider; @@ -18,6 +19,10 @@ public class UserManager { } public boolean removeUser(RealmModel realm, UserModel user) { + return removeUser(realm, user, session.users()); + } + + public boolean removeUser(RealmModel realm, UserModel user, UserProvider userProvider) { UserSessionProvider sessions = session.sessions(); if (sessions != null) { sessions.onUserRemoved(realm, user); @@ -28,7 +33,7 @@ public class UserManager { sessionsPersister.onUserRemoved(realm, user); } - if (session.users().removeUser(realm, user)) { + if (userProvider.removeUser(realm, user)) { return true; } return false; 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 db68a3c6ec..585e0630cf 100644 --- a/services/src/main/java/org/keycloak/services/managers/ClientManager.java +++ b/services/src/main/java/org/keycloak/services/managers/ClientManager.java @@ -104,7 +104,7 @@ public class ClientManager { UserModel serviceAccountUser = realmManager.getSession().users().getUserByServiceAccountClient(client); if (serviceAccountUser != null) { - realmManager.getSession().users().removeUser(realm, serviceAccountUser); + new UserManager(realmManager.getSession()).removeUser(realm, serviceAccountUser); } return true; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionPersisterProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionPersisterProviderTest.java index cbc8284c22..88e13e8e6b 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionPersisterProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionPersisterProviderTest.java @@ -53,8 +53,12 @@ public class UserSessionPersisterProviderTest { UserModel user2 = session.users().getUserByUsername("user2", realm); UserManager um = new UserManager(session); - um.removeUser(realm, user1); - um.removeUser(realm, user2); + if (user1 != null) { + um.removeUser(realm, user1); + } + if (user2 != null) { + um.removeUser(realm, user2); + } kc.stopSession(session, true); } @@ -290,6 +294,43 @@ public class UserSessionPersisterProviderTest { realmMgr.removeRealm(realmMgr.getRealm("foo")); } + @Test + public void testOnUserRemoved() { + // Create some sessions in infinispan + int started = Time.currentTime(); + UserSessionModel[] origSessions = createSessions(); + + resetSession(); + + // Persist 2 offline sessions of 2 users + UserSessionModel userSession1 = session.sessions().getUserSession(realm, origSessions[1].getId()); + UserSessionModel userSession2 = session.sessions().getUserSession(realm, origSessions[2].getId()); + persistUserSession(userSession1, true); + persistUserSession(userSession2, true); + + resetSession(); + + // Load offline sessions + List loadedSessions = loadPersistedSessionsPaginated(true, 10, 1, 2); + + // Properly delete user and assert his offlineSession removed + UserModel user1 = session.users().getUserByUsername("user1", realm); + new UserManager(session).removeUser(realm, user1); + + resetSession(); + + loadedSessions = loadPersistedSessionsPaginated(true, 10, 1, 1); + UserSessionModel persistedSession = loadedSessions.get(0); + UserSessionProviderTest.assertSession(persistedSession, session.users().getUserByUsername("user2", realm), "127.0.0.3", started, started, "test-app"); + + // KEYCLOAK-2431 Assert that userSessionPersister is resistent even to situation, when users are deleted "directly" + UserModel user2 = session.users().getUserByUsername("user2", realm); + session.users().removeUser(realm, user2); + + loadedSessions = loadPersistedSessionsPaginated(true, 10, 0, 0); + + } + // KEYCLOAK-1999 @Test public void testNoSessions() { @@ -376,8 +417,7 @@ public class UserSessionPersisterProviderTest { } Assert.assertEquals(pageCount, expectedPageCount); - Assert.assertEquals(count, expectedSessionsCount); - Assert.assertEquals(count, result.size()); + Assert.assertEquals(result.size(), expectedSessionsCount); return result; } }