From 6696c0f0b22044b0fc33b46c0059d5fdb5d9be4b Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 13 Dec 2017 13:08:04 +0100 Subject: [PATCH] KEYCLOAK-5245 Restart failures when deleting a client with existing sessions/offline_tokens --- .../JpaUserSessionPersisterProvider.java | 21 ++++- .../model/UserSessionInitializerTest.java | 82 ++++++++++++++----- 2 files changed, 81 insertions(+), 22 deletions(-) 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 8d07942d11..a4c02de415 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 @@ -36,8 +36,10 @@ import javax.persistence.Query; import javax.persistence.TypedQuery; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * @author Marek Posolda @@ -162,7 +164,11 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv @Override public void onClientRemoved(RealmModel realm, ClientModel client) { - int num = em.createNamedQuery("deleteClientSessionsByClient").setParameter("clientId", client.getId()).executeUpdate(); + onClientRemoved(client.getId()); + } + + private void onClientRemoved(String clientUUID) { + int num = em.createNamedQuery("deleteClientSessionsByClient").setParameter("clientId", clientUUID).executeUpdate(); num = em.createNamedQuery("deleteDetachedUserSessions").executeUpdate(); } @@ -223,6 +229,8 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv userSessionIds.add(entity.getUserSessionId()); } + Set removedClientUUIDs = new HashSet<>(); + if (!userSessionIds.isEmpty()) { TypedQuery query2 = em.createNamedQuery("findClientSessionsByUserSessions", PersistentClientSessionEntity.class); query2.setParameter("userSessionIds", userSessionIds); @@ -240,7 +248,13 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv PersistentClientSessionEntity clientSession = clientSessions.get(j); if (clientSession.getUserSessionId().equals(userSession.getId())) { PersistentAuthenticatedClientSessionAdapter clientSessAdapter = toAdapter(userSession.getRealm(), userSession, clientSession); - currentClientSessions.put(clientSession.getClientId(), clientSessAdapter); + + // Case when client was removed in the meantime + if (clientSessAdapter.getClient() == null) { + removedClientUUIDs.add(clientSession.getClientId()); + } else { + currentClientSessions.put(clientSession.getClientId(), clientSessAdapter); + } j++; } else { next = false; @@ -249,6 +263,9 @@ public class JpaUserSessionPersisterProvider implements UserSessionPersisterProv } } + for (String clientUUID : removedClientUUIDs) { + onClientRemoved(clientUUID); + } return result; } diff --git a/testsuite/integration-deprecated/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java b/testsuite/integration-deprecated/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java index c8ba7472b0..9574052900 100644 --- a/testsuite/integration-deprecated/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java +++ b/testsuite/integration-deprecated/src/test/java/org/keycloak/testsuite/model/UserSessionInitializerTest.java @@ -33,7 +33,6 @@ import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionModel; import org.keycloak.models.UserSessionProvider; import org.keycloak.models.UserSessionProviderFactory; -import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.models.UserManager; import org.keycloak.services.managers.UserSessionManager; @@ -80,14 +79,72 @@ public class UserSessionInitializerTest { @Test public void testUserSessionInitializer() { - UserSessionModel[] origSessions = createSessions(); + int started = Time.currentTime(); + int serverStartTime = session.getProvider(ClusterProvider.class).getClusterStartupTime(); + + UserSessionModel[] origSessions = createSessionsInPersisterOnly(); + + // Load sessions from persister into infinispan/memory + UserSessionProviderFactory userSessionFactory = (UserSessionProviderFactory) session.getKeycloakSessionFactory().getProviderFactory(UserSessionProvider.class); + userSessionFactory.loadPersistentSessions(session.getKeycloakSessionFactory(), 1, 2); resetSession(); - // Create and persist offline sessions + // Assert sessions are in + ClientModel testApp = realm.getClientByClientId("test-app"); + ClientModel thirdparty = realm.getClientByClientId("third-party"); + Assert.assertEquals(3, session.sessions().getOfflineSessionsCount(realm, testApp)); + Assert.assertEquals(1, session.sessions().getOfflineSessionsCount(realm, thirdparty)); + + List loadedSessions = session.sessions().getOfflineUserSessions(realm, testApp, 0, 10); + UserSessionProviderTest.assertSessions(loadedSessions, origSessions); + + UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[0].getId(), session.users().getUserByUsername("user1", realm), "127.0.0.1", started, serverStartTime, "test-app", "third-party"); + UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[1].getId(), session.users().getUserByUsername("user1", realm), "127.0.0.2", started, serverStartTime, "test-app"); + UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[2].getId(), session.users().getUserByUsername("user2", realm), "127.0.0.3", started, serverStartTime, "test-app"); + } + + + // KEYCLOAK-5245 + @Test + public void testUserSessionInitializerWithDeletingClient() { int started = Time.currentTime(); int serverStartTime = session.getProvider(ClusterProvider.class).getClusterStartupTime(); + UserSessionModel[] origSessions = createSessionsInPersisterOnly(); + + // Delete one of the clients now. Delete it directly in DB just for the purpose of simulating the issue (normally clients should be removed through ClientManager) + ClientModel testApp = realm.getClientByClientId("test-app"); + realm.removeClient(testApp.getId()); + + resetSession(); + + // Load sessions from persister into infinispan/memory + UserSessionProviderFactory userSessionFactory = (UserSessionProviderFactory) session.getKeycloakSessionFactory().getProviderFactory(UserSessionProvider.class); + userSessionFactory.loadPersistentSessions(session.getKeycloakSessionFactory(), 1, 2); + + resetSession(); + + // Assert sessions are in + ClientModel thirdparty = realm.getClientByClientId("third-party"); + Assert.assertEquals(1, session.sessions().getOfflineSessionsCount(realm, thirdparty)); + + List loadedSessions = session.sessions().getOfflineUserSessions(realm, thirdparty, 0, 10); + Assert.assertEquals(1, loadedSessions.size()); + + UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[0].getId(), session.users().getUserByUsername("user1", realm), "127.0.0.1", started, serverStartTime, "third-party"); + + // Revert client + realm.addClient("test-app"); + } + + + // Create sessions in persister+infinispan, but then delete them from infinispan cache. This is to allow later testing of initializer. Return the list of "origSessions" + private UserSessionModel[] createSessionsInPersisterOnly() { + UserSessionModel[] origSessions = createSessions(); + + resetSession(); + for (UserSessionModel origSession : origSessions) { UserSessionModel userSession = session.sessions().getUserSession(realm, origSession.getId()); for (AuthenticatedClientSessionModel clientSession : userSession.getAuthenticatedClientSessions().values()) { @@ -111,25 +168,10 @@ public class UserSessionInitializerTest { Assert.assertEquals(0, session.sessions().getOfflineSessionsCount(realm, testApp)); Assert.assertEquals(0, session.sessions().getOfflineSessionsCount(realm, thirdparty)); - // Load sessions from persister into infinispan/memory - UserSessionProviderFactory userSessionFactory = (UserSessionProviderFactory) session.getKeycloakSessionFactory().getProviderFactory(UserSessionProvider.class); - userSessionFactory.loadPersistentSessions(session.getKeycloakSessionFactory(), 1, 2); - - resetSession(); - - // Assert sessions are in - testApp = realm.getClientByClientId("test-app"); - Assert.assertEquals(3, session.sessions().getOfflineSessionsCount(realm, testApp)); - Assert.assertEquals(1, session.sessions().getOfflineSessionsCount(realm, thirdparty)); - - List loadedSessions = session.sessions().getOfflineUserSessions(realm, testApp, 0, 10); - UserSessionProviderTest.assertSessions(loadedSessions, origSessions); - - UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[0].getId(), session.users().getUserByUsername("user1", realm), "127.0.0.1", started, serverStartTime, "test-app", "third-party"); - UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[1].getId(), session.users().getUserByUsername("user1", realm), "127.0.0.2", started, serverStartTime, "test-app"); - UserSessionPersisterProviderTest.assertSessionLoaded(loadedSessions, origSessions[2].getId(), session.users().getUserByUsername("user2", realm), "127.0.0.3", started, serverStartTime, "test-app"); + return origSessions; } + private AuthenticatedClientSessionModel createClientSession(ClientModel client, UserSessionModel userSession, String redirect, String state, Set roles, Set protocolMappers) { AuthenticatedClientSessionModel clientSession = session.sessions().createClientSession(realm, client, userSession); clientSession.setRedirectUri(redirect);