From a441be57ed6cbf44df8ae12435407914141ae41f Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 19 Feb 2016 11:49:30 +0100 Subject: [PATCH] KEYCLOAK-2508 Possible NullPointerException during bigger load when removing UserSession --- .../InfinispanUserSessionProvider.java | 27 +++++++++++++------ .../sessions/infinispan/stream/Mappers.java | 11 ++++++++ .../keycloak/models/UserSessionProvider.java | 2 +- .../managers/AuthenticationManager.java | 4 +-- .../services/managers/UserSessionManager.java | 9 ++++--- .../model/UserSessionProviderTest.java | 19 +++++++++++++ 6 files changed, 56 insertions(+), 16 deletions(-) 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 86028341d0..7630225384 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 @@ -269,7 +269,8 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { @Override public void removeUserSession(RealmModel realm, UserSessionModel session) { - removeUserSession(realm, session.getId(), false); + UserSessionEntity entity = getUserSessionEntity(session, false); + removeUserSession(realm, entity, false); } @Override @@ -280,9 +281,10 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { protected void removeUserSessions(RealmModel realm, UserModel user, boolean offline) { Cache cache = getCache(offline); - Iterator itr = cache.entrySet().stream().filter(UserSessionPredicate.create(realm.getId()).user(user.getId())).map(Mappers.sessionId()).iterator(); + Iterator itr = cache.entrySet().stream().filter(UserSessionPredicate.create(realm.getId()).user(user.getId())).map(Mappers.sessionEntity()).iterator(); while (itr.hasNext()) { - removeUserSession(realm, itr.next(), offline); + UserSessionEntity userSessionEntity = (UserSessionEntity) itr.next(); + removeUserSession(realm, userSessionEntity, offline); } } @@ -486,12 +488,11 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { } } - protected void removeUserSession(RealmModel realm, String userSessionId, boolean offline) { + protected void removeUserSession(RealmModel realm, UserSessionEntity sessionEntity, boolean offline) { Cache cache = getCache(offline); - tx.remove(cache, userSessionId); + tx.remove(cache, sessionEntity.getId()); - UserSessionEntity sessionEntity = (UserSessionEntity) cache.get(userSessionId); if (sessionEntity.getClientSessions() != null) { for (String clientSessionId : sessionEntity.getClientSessions()) { tx.remove(cache, clientSessionId); @@ -547,6 +548,15 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { return models; } + UserSessionEntity getUserSessionEntity(UserSessionModel userSession, boolean offline) { + if (userSession instanceof UserSessionAdapter) { + return ((UserSessionAdapter) userSession).getEntity(); + } else { + Cache cache = getCache(offline); + return (UserSessionEntity) cache.get(userSession.getId()); + } + } + @Override public UserSessionModel createOfflineUserSession(UserSessionModel userSession) { @@ -566,8 +576,9 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { } @Override - public void removeOfflineUserSession(RealmModel realm, String userSessionId) { - removeUserSession(realm, userSessionId, true); + public void removeOfflineUserSession(RealmModel realm, UserSessionModel userSession) { + UserSessionEntity userSessionEntity = getUserSessionEntity(userSession, true); + removeUserSession(realm, userSessionEntity, true); } @Override diff --git a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/stream/Mappers.java b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/stream/Mappers.java index a06f575461..6bf13580ed 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/stream/Mappers.java +++ b/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/stream/Mappers.java @@ -45,6 +45,10 @@ public class Mappers { return new SessionIdMapper(); } + public static Function, SessionEntity> sessionEntity() { + return new SessionEntityMapper(); + } + public static Function, LoginFailureKey> loginFailureId() { return new LoginFailureIdMapper(); } @@ -76,6 +80,13 @@ public class Mappers { } } + private static class SessionEntityMapper implements Function, SessionEntity>, Serializable { + @Override + public SessionEntity apply(Map.Entry entry) { + return entry.getValue(); + } + } + private static class LoginFailureIdMapper implements Function, LoginFailureKey>, Serializable { @Override public LoginFailureKey apply(Map.Entry entry) { 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 57cd49c3d2..d824584fbe 100755 --- a/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java +++ b/server-spi/src/main/java/org/keycloak/models/UserSessionProvider.java @@ -61,7 +61,7 @@ public interface UserSessionProvider extends Provider { UserSessionModel getOfflineUserSession(RealmModel realm, String userSessionId); // Removes the attached clientSessions as well - void removeOfflineUserSession(RealmModel realm, String userSessionId); + void removeOfflineUserSession(RealmModel realm, UserSessionModel userSession); ClientSessionModel createOfflineClientSession(ClientSessionModel clientSession); ClientSessionModel getOfflineClientSession(RealmModel realm, String clientSessionId); diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java index 36c4caad7d..b169f7cd45 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -181,9 +181,7 @@ public class AuthenticationManager { String authMethod = clientSession.getAuthMethod(); if (authMethod == null) continue; // must be a keycloak service like account redirectClients.add(clientSession); - continue; - } - if (client instanceof ClientModel && !client.isFrontchannelLogout()) { + } else { String authMethod = clientSession.getAuthMethod(); if (authMethod == null) continue; // must be a keycloak service like account LoginProtocol protocol = session.getProvider(LoginProtocol.class, authMethod); 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 3989bc4724..e764adf69f 100644 --- a/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/UserSessionManager.java @@ -117,7 +117,7 @@ public class UserSessionManager { kcSession.sessions().removeOfflineClientSession(realm, clientSession.getId()); persister.removeClientSession(clientSession.getId(), true); - checkOfflineUserSessionHasClientSessions(realm, user, clientSession.getUserSession().getId(), clientSessions); + checkOfflineUserSessionHasClientSessions(realm, user, clientSession.getUserSession(), clientSessions); anyRemoved = true; } } @@ -129,7 +129,7 @@ public class UserSessionManager { if (logger.isTraceEnabled()) { logger.tracef("Removing offline user session '%s' for user '%s' ", userSession.getId(), userSession.getLoginUsername()); } - kcSession.sessions().removeOfflineUserSession(userSession.getRealm(), userSession.getId()); + kcSession.sessions().removeOfflineUserSession(userSession.getRealm(), userSession); persister.removeUserSession(userSession.getId(), true); } @@ -165,7 +165,8 @@ public class UserSessionManager { } // Check if userSession has any offline clientSessions attached to it. Remove userSession if not - private void checkOfflineUserSessionHasClientSessions(RealmModel realm, UserModel user, String userSessionId, List clientSessions) { + private void checkOfflineUserSessionHasClientSessions(RealmModel realm, UserModel user, UserSessionModel userSession, List clientSessions) { + String userSessionId = userSession.getId(); for (ClientSessionModel clientSession : clientSessions) { if (clientSession.getUserSession().getId().equals(userSessionId)) { return; @@ -175,7 +176,7 @@ public class UserSessionManager { if (logger.isTraceEnabled()) { logger.tracef("Removing offline userSession for user %s as it doesn't have any client sessions attached. UserSessionID: %s", user.getUsername(), userSessionId); } - kcSession.sessions().removeOfflineUserSession(realm, userSessionId); + kcSession.sessions().removeOfflineUserSession(realm, userSession); persister.removeUserSession(userSessionId, true); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionProviderTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionProviderTest.java index ab9f45307d..3da7d232d5 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionProviderTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserSessionProviderTest.java @@ -380,6 +380,25 @@ public class UserSessionProviderTest { } } + // KEYCLOAK-2508 + @Test + public void testRemovingExpiredSession() { + UserSessionModel[] sessions = createSessions(); + try { + Time.setOffset(3600000); + UserSessionModel userSession = sessions[0]; + RealmModel realm = userSession.getRealm(); + session.sessions().removeExpired(realm); + + resetSession(); + + // Assert no exception is thrown here + session.sessions().removeUserSession(realm, userSession); + } finally { + Time.setOffset(0); + } + } + @Test public void testGetByClient() { UserSessionModel[] sessions = createSessions();