KEYCLOAK-2431 Ensure users removed through UserManager to properly invoke callbacks. Make UserSessionPersister implementations more resistent when user was not properly removed

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 <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
@@ -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());
                 }
             }
         }

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());
                 }
             }
         }

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();
+        onUserRemoved(realm, user.getId());
-        num = em.createNamedQuery("deleteUserSessionsByUser").setParameter("userId", user.getId()).executeUpdate();
+    }
+
+    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<UserSessionModel> result = new ArrayList<>();
         List<String> 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) {
+    private PersistentUserSessionAdapter toAdapter(RealmModel realm, UserModel user, PersistentUserSessionEntity entity) {
-        RealmModel realm = session.realms().getRealm(entity.getRealmId());
-        UserModel user = session.users().getUserById(entity.getUserId(), realm);
-
         PersistentUserSessionModel model = new PersistentUserSessionModel();
         model.setUserSessionId(entity.getUserSessionId());
         model.setLastSessionRefresh(entity.getLastSessionRefresh());

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<UserSessionModel> 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) {
+    private PersistentUserSessionAdapter toAdapter(RealmModel realm, UserModel user, PersistentUserSessionEntity entity) {
-        RealmModel realm = session.realms().getRealm(entity.getRealmId());
-        UserModel user = session.users().getUserById(entity.getUserId(), realm);
-
         PersistentUserSessionModel model = new PersistentUserSessionModel();
         model.setUserSessionId(entity.getId());
         model.setLastSessionRefresh(entity.getLastSessionRefresh());

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());
             }
 

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;

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;

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);
+        if (user1 != null) {
-        um.removeUser(realm, user2);
+            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<UserSessionModel> 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(result.size(), expectedSessionsCount);
-        Assert.assertEquals(count, result.size());
         return result;
     }
 }