From d7b818cba21c5ccf907f265e29653a6a5cccd7b0 Mon Sep 17 00:00:00 2001 From: Bill Burke Date: Fri, 19 Jun 2015 14:43:40 -0400 Subject: [PATCH] cleanup client sessions --- .../keycloak/models/UserSessionProvider.java | 1 + .../InfinispanUserSessionProvider.java | 16 +++++++++++++ .../entities/ClientSessionEntity.java | 2 ++ .../infinispan/entities/SessionEntity.java | 17 ++++++++++++++ .../sessions/jpa/ClientSessionAdapter.java | 4 ++++ .../sessions/jpa/JpaUserSessionProvider.java | 8 +++++++ .../jpa/entities/ClientSessionEntity.java | 17 ++++++++++++++ .../sessions/mem/ClientSessionAdapter.java | 4 +++- .../sessions/mem/MemUserSessionProvider.java | 11 +++++++++ .../mem/entities/ClientSessionEntity.java | 17 ++++++++++++++ .../mongo/MongoUserSessionProvider.java | 13 +++++++++++ .../keycloak/protocol/saml/SamlProtocol.java | 9 +++----- .../AuthenticationProcessor.java | 7 +++--- .../org/keycloak/protocol/LoginProtocol.java | 1 - .../protocol/oidc/OIDCLoginProtocol.java | 1 + .../resources/LoginActionsService.java | 23 +++++++++++++++---- .../testsuite/forms/ResetPasswordTest.java | 1 + 17 files changed, 136 insertions(+), 16 deletions(-) mode change 100644 => 100755 model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/SessionEntity.java diff --git a/model/api/src/main/java/org/keycloak/models/UserSessionProvider.java b/model/api/src/main/java/org/keycloak/models/UserSessionProvider.java index 3a642068cc..89f8cee939 100755 --- a/model/api/src/main/java/org/keycloak/models/UserSessionProvider.java +++ b/model/api/src/main/java/org/keycloak/models/UserSessionProvider.java @@ -29,6 +29,7 @@ public interface UserSessionProvider extends Provider { void removeUserSessions(RealmModel realm, UserModel user); void removeExpiredUserSessions(RealmModel realm); void removeUserSessions(RealmModel realm); + void removeClientSession(RealmModel realm, ClientSessionModel clientSession); UsernameLoginFailureModel getUserLoginFailure(RealmModel realm, String username); UsernameLoginFailureModel addUserLoginFailure(RealmModel realm, String username); diff --git a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java index e7ff079f62..2275a9864e 100755 --- a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java +++ b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java @@ -334,6 +334,21 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { } } + @Override + public void removeClientSession(RealmModel realm, ClientSessionModel clientSession) { + UserSessionModel userSession = clientSession.getUserSession(); + if (userSession != null) { + UserSessionEntity entity = ((UserSessionAdapter) userSession).getEntity(); + if (entity.getClientSessions() != null) { + entity.getClientSessions().remove(clientSession.getId()); + + } + tx.replace(sessionCache, entity.getId(), entity); + } + tx.remove(sessionCache, clientSession.getId()); + } + + void dettachSession(UserSessionModel userSession, ClientSessionModel clientSession) { UserSessionEntity entity = ((UserSessionAdapter) userSession).getEntity(); String clientSessionId = clientSession.getId(); @@ -359,6 +374,7 @@ public class InfinispanUserSessionProvider implements UserSessionProvider { } } + InfinispanKeycloakTransaction getTx() { return tx; } diff --git a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/ClientSessionEntity.java b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/ClientSessionEntity.java index 3cb6614248..a00e805029 100755 --- a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/ClientSessionEntity.java +++ b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/ClientSessionEntity.java @@ -135,4 +135,6 @@ public class ClientSessionEntity extends SessionEntity { public void setUserSessionNotes(Map userSessionNotes) { this.userSessionNotes = userSessionNotes; } + + } diff --git a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/SessionEntity.java b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/SessionEntity.java old mode 100644 new mode 100755 index 5d6fc3cd64..62e1861384 --- a/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/SessionEntity.java +++ b/model/sessions-infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/SessionEntity.java @@ -26,4 +26,21 @@ public class SessionEntity implements Serializable { public void setRealm(String realm) { this.realm = realm; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof SessionEntity)) return false; + + SessionEntity that = (SessionEntity) o; + + if (id != null ? !id.equals(that.id) : that.id != null) return false; + + return true; + } + + @Override + public int hashCode() { + return id != null ? id.hashCode() : 0; + } } diff --git a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java index e8498e9dde..a6b3016a69 100755 --- a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java +++ b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/ClientSessionAdapter.java @@ -128,6 +128,10 @@ public class ClientSessionAdapter implements ClientSessionModel { return realm.getClientById(entity.getClientId()); } + public ClientSessionEntity getEntity() { + return entity; + } + @Override public void setUserSession(UserSessionModel userSession) { if (userSession == null) { diff --git a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/JpaUserSessionProvider.java b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/JpaUserSessionProvider.java index 23086d9470..8614ec06fa 100755 --- a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/JpaUserSessionProvider.java +++ b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/JpaUserSessionProvider.java @@ -18,6 +18,7 @@ import org.keycloak.util.Time; import javax.persistence.EntityManager; import javax.persistence.TypedQuery; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; @@ -47,6 +48,13 @@ public class JpaUserSessionProvider implements UserSessionProvider { return new ClientSessionAdapter(session, em, realm, entity); } + @Override + public void removeClientSession(RealmModel realm, ClientSessionModel clientSession) { + ClientSessionEntity clientSessionEntity = ((ClientSessionAdapter)clientSession).getEntity(); + em.remove(clientSessionEntity); + em.flush(); + } + @Override public ClientSessionModel getClientSession(RealmModel realm, String id) { ClientSessionEntity clientSession = em.find(ClientSessionEntity.class, id); diff --git a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/entities/ClientSessionEntity.java b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/entities/ClientSessionEntity.java index 300c45a67e..5789ea82ef 100755 --- a/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/entities/ClientSessionEntity.java +++ b/model/sessions-jpa/src/main/java/org/keycloak/models/sessions/jpa/entities/ClientSessionEntity.java @@ -186,4 +186,21 @@ public class ClientSessionEntity { public void setUserSessionNotes(Collection userSessionNotes) { this.userSessionNotes = userSessionNotes; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ClientSessionEntity)) return false; + + ClientSessionEntity that = (ClientSessionEntity) o; + + if (id != null ? !id.equals(that.id) : that.id != null) return false; + + return true; + } + + @Override + public int hashCode() { + return id != null ? id.hashCode() : 0; + } } diff --git a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java index f9e52d095b..3053a9d8a2 100755 --- a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java +++ b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/ClientSessionAdapter.java @@ -39,7 +39,9 @@ public class ClientSessionAdapter implements ClientSessionModel { return session.realms().getRealm(entity.getRealmId()); } - + public ClientSessionEntity getEntity() { + return entity; + } @Override public ClientModel getClient() { diff --git a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/MemUserSessionProvider.java b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/MemUserSessionProvider.java index 67a218014f..ac6655f5c7 100755 --- a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/MemUserSessionProvider.java +++ b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/MemUserSessionProvider.java @@ -58,6 +58,17 @@ public class MemUserSessionProvider implements UserSessionProvider { return new ClientSessionAdapter(session, this, realm, entity); } + @Override + public void removeClientSession(RealmModel realm, ClientSessionModel clientSession) { + ClientSessionEntity entity = ((ClientSessionAdapter)clientSession).getEntity(); + UserSessionModel userSession = clientSession.getUserSession(); + if (userSession != null) { + UserSessionEntity userSessionEntity = ((UserSessionAdapter)userSession).getEntity(); + userSessionEntity.getClientSessions().remove(entity); + } + clientSessions.remove(clientSession.getId()); + } + @Override public ClientSessionModel getClientSession(RealmModel realm, String id) { ClientSessionEntity entity = clientSessions.get(id); diff --git a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/entities/ClientSessionEntity.java b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/entities/ClientSessionEntity.java index da5e050772..7c6fb22fca 100755 --- a/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/entities/ClientSessionEntity.java +++ b/model/sessions-mem/src/main/java/org/keycloak/models/sessions/mem/entities/ClientSessionEntity.java @@ -132,4 +132,21 @@ public class ClientSessionEntity { public Map getUserSessionNotes() { return userSessionNotes; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ClientSessionEntity)) return false; + + ClientSessionEntity that = (ClientSessionEntity) o; + + if (id != null ? !id.equals(that.id) : that.id != null) return false; + + return true; + } + + @Override + public int hashCode() { + return id != null ? id.hashCode() : 0; + } } diff --git a/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/MongoUserSessionProvider.java b/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/MongoUserSessionProvider.java index 848fdeb267..82045fd1f7 100755 --- a/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/MongoUserSessionProvider.java +++ b/model/sessions-mongo/src/main/java/org/keycloak/models/sessions/mongo/MongoUserSessionProvider.java @@ -55,6 +55,19 @@ public class MongoUserSessionProvider implements UserSessionProvider { return new ClientSessionAdapter(session, this, realm, entity, invocationContext); } + @Override + public void removeClientSession(RealmModel realm, ClientSessionModel clientSession) { + MongoClientSessionEntity entity = ((ClientSessionAdapter)clientSession).getMongoEntity(); + if (entity.getSessionId() != null) { + MongoUserSessionEntity userSessionEntity = getUserSessionEntity(realm, entity.getSessionId()); + getMongoStore().pullItemFromList(userSessionEntity, "clientSessions", entity.getSessionId(), invocationContext); + + } + + mongoStore.removeEntity(entity, invocationContext); + + } + @Override public ClientSessionModel getClientSession(RealmModel realm, String id) { MongoClientSessionEntity entity = getClientSessionEntity(id); diff --git a/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java b/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java index 9db48d10e6..d5630bd54a 100755 --- a/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java +++ b/saml/saml-protocol/src/main/java/org/keycloak/protocol/saml/SamlProtocol.java @@ -134,12 +134,9 @@ public class SamlProtocol implements LoginProtocol { @Override public Response cancelLogin(ClientSessionModel clientSession) { - return getErrorResponse(clientSession, JBossSAMLURIConstants.STATUS_REQUEST_DENIED.get()); - } - - @Override - public Response invalidSessionError(ClientSessionModel clientSession) { - return getErrorResponse(clientSession, JBossSAMLURIConstants.STATUS_AUTHNFAILED.get()); + Response error = getErrorResponse(clientSession, JBossSAMLURIConstants.STATUS_REQUEST_DENIED.get()); + session.sessions().removeClientSession(realm, clientSession); + return error; } protected String getResponseIssuer(RealmModel realm) { diff --git a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java index fce95a26a7..aa0a0cfec5 100755 --- a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java +++ b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java @@ -459,7 +459,8 @@ public class AuthenticationProcessor { return authenticationComplete(); } - protected void resetFlow() { + public static void resetFlow(ClientSessionModel clientSession) { + clientSession.setAuthenticatedUser(null); clientSession.clearExecutionStatus(); clientSession.clearUserSessionNotes(); clientSession.removeNote(CURRENT_AUTHENTICATION_EXECUTION); @@ -471,14 +472,14 @@ public class AuthenticationProcessor { if (!execution.equals(current)) { logger.debug("Current execution does not equal executed execution. Might be a page refresh"); logFailure(); - resetFlow(); + resetFlow(clientSession); return authenticate(); } AuthenticationExecutionModel model = realm.getAuthenticationExecutionById(execution); if (model == null) { logger.debug("Cannot find execution, reseting flow"); logFailure(); - resetFlow(); + resetFlow(clientSession); return authenticate(); } event.event(EventType.LOGIN); diff --git a/services/src/main/java/org/keycloak/protocol/LoginProtocol.java b/services/src/main/java/org/keycloak/protocol/LoginProtocol.java index be1711b544..51676f399c 100755 --- a/services/src/main/java/org/keycloak/protocol/LoginProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/LoginProtocol.java @@ -28,7 +28,6 @@ public interface LoginProtocol extends Provider { LoginProtocol setEventBuilder(EventBuilder event); Response cancelLogin(ClientSessionModel clientSession); - Response invalidSessionError(ClientSessionModel clientSession); Response authenticated(UserSessionModel userSession, ClientSessionCode accessCode); Response consentDenied(ClientSessionModel clientSession); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java index 4ef8544aab..c002335311 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java @@ -122,6 +122,7 @@ public class OIDCLoginProtocol implements LoginProtocol { if (state != null) { redirectUri.queryParam(OAuth2Constants.STATE, state); } + session.sessions().removeClientSession(realm, clientSession); return Response.status(302).location(redirectUri.build()).build(); } diff --git a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java index cca6bb0534..d921cf1fc4 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -182,7 +182,15 @@ public class LoginActionsService { } else if (!(clientCode.isActionActive(requiredAction) || clientCode.isActionActive(alternativeRequiredAction))) { event.client(clientCode.getClientSession().getClient()); event.error(Errors.EXPIRED_CODE); - response = ErrorPage.error(session, Messages.EXPIRED_CODE); + if (clientCode.getClientSession().getAction().equals(ClientSessionModel.Action.AUTHENTICATE.name())) { + AuthenticationProcessor.resetFlow(clientCode.getClientSession()); + response = processAuthentication(null, clientCode.getClientSession()); + } else { + if (clientCode.getClientSession().getUserSession() == null) { + session.sessions().removeClientSession(realm, clientCode.getClientSession()); + } + response = ErrorPage.error(session, Messages.EXPIRED_CODE); + } return false; } else { return true; @@ -207,21 +215,26 @@ public class LoginActionsService { return false; } ClientSessionModel clientSession = clientCode.getClientSession(); + if (clientSession == null) { + event.error(Errors.INVALID_CODE); + response = ErrorPage.error(session, Messages.INVALID_CODE); + return false; + } event.detail(Details.CODE_ID, clientSession.getId()); ClientModel client = clientSession.getClient(); if (client == null) { event.error(Errors.CLIENT_NOT_FOUND); response = ErrorPage.error(session, Messages.UNKNOWN_LOGIN_REQUESTER); + session.sessions().removeClientSession(realm, clientSession); return false; } - session.getContext().setClient(client); - if (!client.isEnabled()) { event.error(Errors.CLIENT_NOT_FOUND); response = ErrorPage.error(session, Messages.LOGIN_REQUESTER_NOT_ENABLED); + session.sessions().removeClientSession(realm, clientSession); return false; } - session.getContext().setClient(clientCode.getClientSession().getClient()); + session.getContext().setClient(client); return true; } } @@ -239,7 +252,7 @@ public class LoginActionsService { @QueryParam("execution") String execution) { event.event(EventType.LOGIN); Checks checks = new Checks(); - if (!checks.check(code)) { + if (!checks.check(code, ClientSessionModel.Action.AUTHENTICATE.name(), ClientSessionModel.Action.RECOVER_PASSWORD.name())) { return checks.response; } event.detail(Details.CODE_ID, code); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java index 17ca887161..e3926ef1db 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java @@ -137,6 +137,7 @@ public class ResetPasswordTest { events.expectRequiredAction(EventType.SEND_RESET_PASSWORD).user(userId).detail(Details.USERNAME, "login-test").detail(Details.EMAIL, "login@test.com").assertEvent().getSessionId(); + String src = driver.getPageSource(); resetPasswordPage.backToLogin(); assertTrue(loginPage.isCurrent());