From f6071f680a52eccb966e2980b642f790f952b38a Mon Sep 17 00:00:00 2001 From: Giuseppe Graziano Date: Sat, 23 Mar 2024 15:14:16 +0100 Subject: [PATCH] Avoid the same userSessionId after re-authentication Closes keycloak/keycloak-private#69 Signed-off-by: Giuseppe Graziano --- .../AuthenticationProcessor.java | 19 ++++++ .../AuthenticationSessionManager.java | 23 ++++--- .../resources/LoginActionsService.java | 3 +- .../testsuite/forms/ReAuthenticationTest.java | 64 +++++++++++++++++++ 4 files changed, 99 insertions(+), 10 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java index a4fc46874a..fe2fbad65b 100755 --- a/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java +++ b/services/src/main/java/org/keycloak/authentication/AuthenticationProcessor.java @@ -43,6 +43,7 @@ import org.keycloak.models.utils.FormMessage; import org.keycloak.protocol.ClientData; import org.keycloak.protocol.LoginProtocol; import org.keycloak.protocol.LoginProtocol.Error; +import org.keycloak.protocol.RestartLoginCookie; import org.keycloak.protocol.oidc.TokenManager; import org.keycloak.services.ErrorPage; import org.keycloak.services.ErrorPageException; @@ -51,12 +52,14 @@ import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.services.managers.BruteForceProtector; import org.keycloak.services.managers.ClientSessionCode; import org.keycloak.services.managers.UserSessionManager; +import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.services.messages.Messages; import org.keycloak.services.resources.LoginActionsService; import org.keycloak.services.util.CacheControlUtil; import org.keycloak.services.util.AuthenticationFlowURLHelper; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.sessions.CommonClientSessionModel; +import org.keycloak.sessions.RootAuthenticationSessionModel; import org.keycloak.util.JsonSerialization; import jakarta.ws.rs.core.MultivaluedHashMap; @@ -957,6 +960,22 @@ public class AuthenticationProcessor { authSession.setAuthNote(CURRENT_FLOW_PATH, flowPath); } + // Recreate new root auth session and new auth session from the given auth session. + public static AuthenticationSessionModel recreate(KeycloakSession session, AuthenticationSessionModel authSession) { + AuthenticationSessionManager authenticationSessionManager = new AuthenticationSessionManager(session); + RootAuthenticationSessionModel rootAuthenticationSession = authenticationSessionManager.createAuthenticationSession(authSession.getRealm(), true); + AuthenticationSessionModel newAuthSession = rootAuthenticationSession.createAuthenticationSession(authSession.getClient()); + newAuthSession.setRedirectUri(authSession.getRedirectUri()); + newAuthSession.setProtocol(authSession.getProtocol()); + + for (Map.Entry clientNote : authSession.getClientNotes().entrySet()) { + newAuthSession.setClientNote(clientNote.getKey(), clientNote.getValue()); + } + + authenticationSessionManager.removeAuthenticationSession(authSession.getRealm(), authSession, true); + RestartLoginCookie.setRestartCookie(session, authSession); + return newAuthSession; + } // Clone new authentication session from the given authSession. New authenticationSession will have same parent (rootSession) and will use same client public static AuthenticationSessionModel clone(KeycloakSession session, AuthenticationSessionModel authSession) { diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java index 348d421b5f..c7ca1b7ddc 100644 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java @@ -211,18 +211,23 @@ public class AuthenticationSessionManager { public void updateAuthenticationSessionAfterSuccessfulAuthentication(RealmModel realm, AuthenticationSessionModel authSession) { boolean removedRootAuthSession = removeTabIdInAuthenticationSession(realm, authSession); if (!removedRootAuthSession) { - RootAuthenticationSessionModel rootAuthSession = authSession.getParentSession(); + if(realm.getSsoSessionIdleTimeout() < SessionExpiration.getAuthSessionLifespan(realm) && realm.getSsoSessionMaxLifespan() < SessionExpiration.getAuthSessionLifespan(realm)) { + removeAuthenticationSession(realm, authSession, true); + } + else { + RootAuthenticationSessionModel rootAuthSession = authSession.getParentSession(); - // 1 minute by default. Same timeout, which is used for client to complete "authorization code" flow - // Very short timeout should be OK as when this cookie is set, other existing browser tabs are supposed to be refreshed immediately by JS script authChecker.js - // and login user automatically. No need to have authenticationSession and cookie living any longer - int authSessionExpiresIn = realm.getAccessCodeLifespan(); + // 1 minute by default. Same timeout, which is used for client to complete "authorization code" flow + // Very short timeout should be OK as when this cookie is set, other existing browser tabs are supposed to be refreshed immediately by JS script authChecker.js + // and login user automatically. No need to have authenticationSession and cookie living any longer + int authSessionExpiresIn = realm.getAccessCodeLifespan(); - // Set timestamp to the past to make sure that authSession is scheduled for expiration in "authSessionExpiresIn" seconds - int authSessionExpirationTime = Time.currentTime() - SessionExpiration.getAuthSessionLifespan(realm) + authSessionExpiresIn; - rootAuthSession.setTimestamp(authSessionExpirationTime); + // Set timestamp to the past to make sure that authSession is scheduled for expiration in "authSessionExpiresIn" seconds + int authSessionExpirationTime = Time.currentTime() - SessionExpiration.getAuthSessionLifespan(realm) + authSessionExpiresIn; + rootAuthSession.setTimestamp(authSessionExpirationTime); - log.tracef("Removed authentication session of root session '%s' with tabId '%s'. But there are remaining tabs in the root session. Root authentication session will expire in %d seconds", rootAuthSession.getId(), authSession.getTabId(), authSessionExpiresIn); + log.tracef("Removed authentication session of root session '%s' with tabId '%s'. But there are remaining tabs in the root session. Root authentication session will expire in %d seconds", rootAuthSession.getId(), authSession.getTabId(), authSessionExpiresIn); + } } } 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 69e92fdfce..56f55caee5 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -245,12 +245,13 @@ public class LoginActionsService { if (userSession != null) { logger.debugf("Logout of user session %s when restarting flow during re-authentication", userSession.getId()); AuthenticationManager.backchannelLogout(session, userSession, false); + authSession = AuthenticationProcessor.recreate(session, authSession); } } AuthenticationProcessor.resetFlow(authSession, flowPath); - URI redirectUri = getLastExecutionUrl(flowPath, null, authSession.getClient().getClientId(), tabId, AuthenticationProcessor.getClientData(session, authSession)); + URI redirectUri = getLastExecutionUrl(flowPath, null, authSession.getClient().getClientId(), authSession.getTabId(), AuthenticationProcessor.getClientData(session, authSession)); logger.debugf("Flow restart requested. Redirecting to %s", redirectUri); return Response.status(Response.Status.FOUND).location(redirectUri).build(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ReAuthenticationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ReAuthenticationTest.java index 24207a2d71..9b1a36c853 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ReAuthenticationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ReAuthenticationTest.java @@ -28,10 +28,13 @@ import org.jboss.arquillian.drone.api.annotation.Drone; import org.jboss.arquillian.graphene.page.Page; import org.jboss.arquillian.test.api.ArquillianResource; import org.junit.Test; +import org.keycloak.OAuth2Constants; import org.keycloak.admin.client.resource.UserResource; import org.keycloak.authentication.authenticators.browser.PasswordFormFactory; import org.keycloak.authentication.authenticators.browser.UsernameFormFactory; import org.keycloak.models.AuthenticationExecutionModel; +import org.keycloak.protocol.oidc.OIDCLoginProtocol; +import org.keycloak.representations.AccessToken; import org.keycloak.representations.idm.FederatedIdentityRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.RealmRepresentation; @@ -302,6 +305,67 @@ public class ReAuthenticationTest extends AbstractTestRealmKeycloakTest { BrowserFlowTest.revertFlows(testRealm(), "browser - identity first"); } + @Test + public void restartLoginWithNewRootAuthSession() { + loginPage.open(); + loginPage.login("test-user@localhost", "password"); + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + OAuthClient.AccessTokenResponse response1 = oauth.doAccessTokenRequest(code, "password"); + + oauth.prompt(OIDCLoginProtocol.PROMPT_VALUE_LOGIN); + loginPage.open(); + loginPage.clickResetLogin(); + loginPage.login("john-doh@localhost", "password"); + + code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + OAuthClient.AccessTokenResponse response2 = oauth.doAccessTokenRequest(code, "password"); + + + AccessToken accessToken1 = oauth.verifyToken(response1.getAccessToken()); + AccessToken accessToken2 = oauth.verifyToken(response2.getAccessToken()); + + Assert.assertNotEquals(accessToken1.getSubject(), accessToken2.getSubject()); + Assert.assertNotEquals(accessToken1.getSessionId(), accessToken2.getSessionId()); + } + + @Test + public void loginAfterExpiredUserSession() { + RealmRepresentation rep = testRealm().toRepresentation(); + Integer originalSsoSessionIdleTimeout = rep.getSsoSessionIdleTimeout(); + Integer originalSsoSessionMaxLifespan = rep.getSsoSessionMaxLifespan(); + + rep.setSsoSessionIdleTimeout(10); + rep.setSsoSessionMaxLifespan(10); + realmsResouce().realm(rep.getRealm()).update(rep); + + loginPage.open(); + driver.navigate().refresh(); + loginPage.login("test-user@localhost", "password"); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + OAuthClient.AccessTokenResponse response1 = oauth.doAccessTokenRequest(code, "password"); + + //set time offset after user session expiration (10s) but before accessCodeLifespanLogin (1800s) and accessCodeLifespan (60s) + setTimeOffset(20); + + loginPage.open(); + loginPage.login("john-doh@localhost", "password"); + + code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + OAuthClient.AccessTokenResponse response2 = oauth.doAccessTokenRequest(code, "password"); + + AccessToken accessToken1 = oauth.verifyToken(response1.getAccessToken()); + AccessToken accessToken2 = oauth.verifyToken(response2.getAccessToken()); + + Assert.assertNotEquals(accessToken1.getSubject(), accessToken2.getSubject()); + Assert.assertNotEquals(accessToken1.getSessionId(), accessToken2.getSessionId()); + + setTimeOffset(0); + rep.setSsoSessionIdleTimeout(originalSsoSessionIdleTimeout); + rep.setSsoSessionMaxLifespan(originalSsoSessionMaxLifespan); + realmsResouce().realm(rep.getRealm()).update(rep); + } + private void setupIdentityFirstFlow() { String newFlowAlias = "browser - identity first"; testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias));