From f051a0cdb3cf09c767692182766f8a083489b429 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 17 Apr 2023 08:38:24 +0200 Subject: [PATCH] Improve SessionCodeChecks to detect better the ALREADY_LOGGED_IN situation Closes https://github.com/keycloak/keycloak/issues/19677 --- .../managers/AuthenticationManager.java | 7 + .../services/resources/SessionCodeChecks.java | 45 +++---- .../resources/admin/UserResource.java | 1 + .../servlet/SAMLServletAdapterTest.java | 122 ++++++++++++++++++ .../adapter-test/keycloak-saml/testsaml.json | 13 ++ 5 files changed, 160 insertions(+), 28 deletions(-) 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 17ea01354f..72320856ba 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -867,6 +867,13 @@ public class AuthenticationManager { expireCookie(realm, AuthenticationSessionManager.AUTH_SESSION_ID, oldPath, true, connection, SameSiteAttributeValue.NONE, session); } + public static void expireAuthSessionCookie(RealmModel realm, UriInfo uriInfo, KeycloakSession session) { + logger.debugv("Expire {1} cookie .", AuthenticationSessionManager.AUTH_SESSION_ID); + ClientConnection connection = session.getContext().getConnection(); + String oldPath = getRealmCookiePath(realm, uriInfo); + expireCookie(realm, AuthenticationSessionManager.AUTH_SESSION_ID, oldPath, true, connection, SameSiteAttributeValue.NONE, session); + } + protected static String getIdentityCookiePath(RealmModel realm, UriInfo uriInfo) { return getRealmCookiePath(realm, uriInfo); } diff --git a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java index 9fc8e3612a..99af4935db 100644 --- a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java +++ b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java @@ -18,7 +18,6 @@ package org.keycloak.services.resources; import static org.keycloak.services.managers.AuthenticationManager.authenticateIdentityCookie; -import static org.keycloak.services.managers.AuthenticationSessionManager.AUTH_SESSION_ID; import static org.keycloak.utils.LockObjectsForModification.lockUserSessionsForModification; import java.net.URI; @@ -52,7 +51,6 @@ import org.keycloak.services.managers.ClientSessionCode; import org.keycloak.services.messages.Messages; import org.keycloak.services.util.BrowserHistoryHelper; import org.keycloak.services.util.AuthenticationFlowURLHelper; -import org.keycloak.services.util.CookieHelper; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.sessions.RootAuthenticationSessionModel; @@ -181,35 +179,26 @@ public class SessionCodeChecks { } - // See if we are already authenticated and userSession with same ID exists. - UserSessionModel userSession = authSessionManager.getUserSessionFromAuthCookie(realm); - - boolean authenticating = !CookieHelper.getCookieValue(session, AUTH_SESSION_ID).isEmpty(); - - if (authenticating) { - // if there is an auth session, make sure the user is not yet authenticated - AuthenticationManager.AuthResult authResult = lockUserSessionsForModification(session, () -> authenticateIdentityCookie(session, realm, false)); - - if (authResult != null) { - userSession = authResult.getSession(); - } - } - - if (userSession != null) { - LoginFormsProvider loginForm = session.getProvider(LoginFormsProvider.class).setAuthenticationSession(authSession) - .setSuccess(Messages.ALREADY_LOGGED_IN); - - if (client == null) { - loginForm.setAttribute(Constants.SKIP_LINK, true); - } - - response = loginForm.createInfoPage(); - return null; - } - // Otherwise just try to restart from the cookie RootAuthenticationSessionModel existingRootAuthSession = authSessionManager.getCurrentRootAuthenticationSession(realm); response = restartAuthenticationSessionFromCookie(existingRootAuthSession); + + // if restart from cookie was not found check if the user is already authenticated + if (response.getStatus() != Response.Status.FOUND.getStatusCode()) { + AuthenticationManager.AuthResult authResult = lockUserSessionsForModification(session, () -> authenticateIdentityCookie(session, realm, false)); + + if (authResult != null && authResult.getSession() != null) { + LoginFormsProvider loginForm = session.getProvider(LoginFormsProvider.class).setAuthenticationSession(authSession) + .setSuccess(Messages.ALREADY_LOGGED_IN); + + if (client == null) { + loginForm.setAttribute(Constants.SKIP_LINK, true); + } + + response = loginForm.createInfoPage(); + } + } + return null; } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index e30aa80fb3..5a602b682a 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -340,6 +340,7 @@ public class UserResource { UserSessionModel userSession = lockUserSessionsForModification(session, () -> session.sessions().getUserSession(authenticatedRealm, sessionState)); AuthenticationManager.expireIdentityCookie(realm, session.getContext().getUri(), session); AuthenticationManager.expireRememberMeCookie(realm, session.getContext().getUri(), session); + AuthenticationManager.expireAuthSessionCookie(realm, session.getContext().getUri(), session); AuthenticationManager.backchannelLogout(session, authenticatedRealm, userSession, session.getContext().getUri(), clientConnection, headers, true); } EventBuilder event = new EventBuilder(realm, session, clientConnection); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLServletAdapterTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLServletAdapterTest.java index 448a924423..4663faf543 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLServletAdapterTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLServletAdapterTest.java @@ -47,6 +47,7 @@ import java.security.cert.Certificate; import java.security.cert.CertificateFactory; import java.util.Arrays; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; @@ -87,24 +88,31 @@ import javax.xml.xpath.XPathConstants; import javax.xml.xpath.XPathExpression; import javax.xml.xpath.XPathFactory; +import org.apache.http.HttpResponse; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; +import org.apache.http.client.methods.RequestBuilder; import org.apache.http.client.protocol.HttpClientContext; +import org.apache.http.impl.client.BasicCookieStore; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.util.EntityUtils; import org.jboss.arquillian.container.test.api.Deployment; import org.jboss.arquillian.graphene.page.Page; +import org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder; import org.jboss.shrinkwrap.api.asset.StringAsset; import org.jboss.shrinkwrap.api.spec.WebArchive; import org.junit.Assert; import org.junit.Test; +import org.keycloak.admin.client.Keycloak; +import org.keycloak.admin.client.KeycloakBuilder; import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.admin.client.resource.ProtocolMappersResource; +import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.RoleScopeResource; import org.keycloak.admin.client.resource.UserResource; import org.keycloak.common.util.Base64; @@ -119,6 +127,7 @@ import org.keycloak.dom.saml.v2.protocol.StatusResponseType; import org.keycloak.keys.Attributes; import org.keycloak.keys.ImportedRsaKeyProviderFactory; import org.keycloak.keys.KeyProvider; +import org.keycloak.models.Constants; import org.keycloak.protocol.saml.SamlConfigAttributes; import org.keycloak.protocol.saml.SamlProtocol; import org.keycloak.representations.idm.ClientRepresentation; @@ -135,17 +144,23 @@ import org.keycloak.saml.common.util.XmlKeyInfoKeyNameTransformer; import org.keycloak.saml.processing.core.parsers.saml.SAMLParser; import org.keycloak.saml.processing.core.saml.v2.common.SAMLDocumentHolder; import org.keycloak.saml.processing.core.saml.v2.util.AssertionUtil; +import org.keycloak.services.managers.AuthenticationManager; +import org.keycloak.services.managers.AuthenticationSessionManager; import org.keycloak.services.resources.RealmsResource; +import org.keycloak.services.util.CookieHelper; import org.keycloak.testsuite.adapter.page.*; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.AppServerContainer; import org.keycloak.testsuite.util.ServerURLs; import org.keycloak.testsuite.utils.arquillian.ContainerConstants; import org.keycloak.testsuite.auth.page.login.Login; +import org.keycloak.testsuite.auth.page.login.OneTimeCode; import org.keycloak.testsuite.auth.page.login.SAMLIDPInitiatedLogin; import org.keycloak.testsuite.auth.page.login.SAMLPostLoginTenant1; import org.keycloak.testsuite.auth.page.login.SAMLPostLoginTenant2; import org.keycloak.testsuite.page.AbstractPage; +import org.keycloak.testsuite.pages.ErrorPage; +import org.keycloak.testsuite.pages.InfoPage; import org.keycloak.testsuite.saml.AbstractSamlTest; import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.updaters.Creator; @@ -159,6 +174,7 @@ import org.keycloak.testsuite.util.WaitUtils; import org.keycloak.testsuite.utils.io.IOUtil; import org.openqa.selenium.By; +import org.openqa.selenium.Cookie; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -289,6 +305,15 @@ public class SAMLServletAdapterTest extends AbstractSAMLServletAdapterTest { @Page protected SAMLPostLoginTenant2 tenant2RealmSAMLPostLoginPage; + @Page + protected OneTimeCode authenticate; + + @Page + private InfoPage infoPage; + + @Page + private ErrorPage errorPage; + public static final String FORBIDDEN_TEXT = "HTTP status code: 403"; public static final String WEBSPHERE_FORBIDDEN_TEXT = "Error reported: 403"; @@ -1773,6 +1798,103 @@ public class SAMLServletAdapterTest extends AbstractSAMLServletAdapterTest { } + @Test + public void testReloginWithInvalidAuthSessionCookie() { + assertSuccessfulLogin(salesPostServletPage, bburkeUser, testRealmSAMLPostLoginPage, "principal=bburke"); + assertSuccessfullyLoggedIn(salesPostSigServletPage, "principal=bburke"); + // remove session cookies for the application to force re-login against the keycloak server + driver.manage().deleteAllCookies(); + + // go to the login-actions/authenticate page and change AUTH_SESSION_ID cookies + authenticate.setAuthRealm(SAMLSERVLETDEMO); + authenticate.navigateTo(); + waitForPageToLoad(); + infoPage.assertCurrent(); + Assert.assertEquals("You are already logged in.", infoPage.getInfo()); + Cookie identityCookie = driver.manage().getCookieNamed(AuthenticationManager.KEYCLOAK_IDENTITY_COOKIE); + Assert.assertNotNull(identityCookie); + driver.manage().deleteCookieNamed(AuthenticationSessionManager.AUTH_SESSION_ID); + driver.manage().deleteCookieNamed(AuthenticationSessionManager.AUTH_SESSION_ID + CookieHelper.LEGACY_COOKIE); + driver.manage().addCookie(new Cookie(AuthenticationSessionManager.AUTH_SESSION_ID, "invalid-value", identityCookie.getPath())); + driver.manage().addCookie(new Cookie(AuthenticationSessionManager.AUTH_SESSION_ID + CookieHelper.LEGACY_COOKIE, "invalid-value", identityCookie.getPath())); + + // go back to the app page, re-login should work with the invalid cookie + testRealmSAMLPostLoginPage.navigateTo(); + waitForPageToLoad(); + assertSuccessfullyLoggedIn(salesPostSigServletPage, "principal=bburke"); + driver.manage().deleteAllCookies(); + + salesPostSigServletPage.logout(); + checkLoggedOut(salesPostSigEmailServletPage, testRealmSAMLPostLoginPage); + } + + private List impersonate(String admin, String adminPassword, String userId) throws IOException { + ResteasyClientBuilder resteasyClientBuilder = (ResteasyClientBuilder) ResteasyClientBuilder.newBuilder(); + resteasyClientBuilder.connectionPoolSize(10); + resteasyClientBuilder.httpEngine(AdminClientUtil.getCustomClientHttpEngine(resteasyClientBuilder, 10, null)); + + BasicCookieStore cookieStore = new BasicCookieStore(); + try (Keycloak client = KeycloakBuilder.builder().serverUrl(loginPage.getAuthRoot()).realm(SAMLSERVLETDEMO) + .username(admin).password(adminPassword).clientId(Constants.ADMIN_CLI_CLIENT_ID) + .resteasyClient(ResteasyClientBuilder.newBuilder().build()).build(); + CloseableHttpClient httpClient = HttpClientBuilder.create().setDefaultCookieStore(cookieStore).build()) { + HttpUriRequest req = RequestBuilder.post() + .setUri(loginPage.getAuthRoot() + "/admin/realms/" + SAMLSERVLETDEMO + "/users/" + userId + "/impersonation") + .addHeader(HttpHeaders.AUTHORIZATION, "Bearer " + client.tokenManager().getAccessTokenString()) + .build(); + HttpResponse res = httpClient.execute(req); + Assert.assertEquals(Response.Status.OK.getStatusCode(), res.getStatusLine().getStatusCode()); + String resBody = EntityUtils.toString(res.getEntity()); + + Assert.assertNotNull(resBody); + Assert.assertTrue(resBody.contains("redirect")); + + // return cookies not expired in the store as selenium cookies + final Date now = new Date(); + return cookieStore.getCookies().stream() + .filter(c -> !c.isExpired(now)) + .map(c -> new Cookie(c.getName(), c.getValue(), c.getDomain(), c.getPath(), c.getExpiryDate(), c.isSecure(), true)) + .collect(Collectors.toList()); + } + } + + @Test + public void testImpersonationForSaml() throws IOException { + RealmResource realm = adminClient.realm(SAMLSERVLETDEMO); + List users = realm.users().search("bburke", true); + Assert.assertNotNull(users); + Assert.assertEquals(1, users.size()); + + // impersonate and get all returned cookies + List cookies = impersonate("admindemo", "password", users.get(0).getId()); + Assert.assertNotNull(cookies); + + // go to the authenticate page and add all the returned cookies by the impersonation + authenticate.setAuthRealm(SAMLSERVLETDEMO); + authenticate.navigateTo(); + waitForPageToLoad(); + errorPage.assertCurrent(); + cookies.stream().forEach(c -> driver.manage().addCookie(c)); + driver.navigate().refresh(); + waitForPageToLoad(); + infoPage.assertCurrent(); + Assert.assertEquals("You are already logged in.", infoPage.getInfo()); + + // now go to the saml app with all the impersonation cookies + testRealmSAMLPostLoginPage.navigateTo(); + waitForPageToLoad(); + assertSuccessfullyLoggedIn(salesPostSigServletPage, "principal=bburke"); + driver.manage().deleteAllCookies(); + + // go back to the app page a second time + testRealmSAMLPostLoginPage.navigateTo(); + waitForPageToLoad(); + assertSuccessfullyLoggedIn(salesPostSigServletPage, "principal=bburke"); + + salesPostSigServletPage.logout(); + checkLoggedOut(salesPostSigEmailServletPage, testRealmSAMLPostLoginPage); + } + public static void printDocument(Source doc, OutputStream out) throws IOException, TransformerException { TransformerFactory tf = TransformerFactory.newInstance(); Transformer transformer = tf.newTransformer(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/keycloak-saml/testsaml.json b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/keycloak-saml/testsaml.json index 63e437da0c..53a8a99abc 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/keycloak-saml/testsaml.json +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/adapter-test/keycloak-saml/testsaml.json @@ -99,6 +99,19 @@ "phone": "617" }, "realmRoles": ["manager", "user"] + }, + { + "username" : "admindemo", + "enabled": true, + "email" : "admindemo@redhat.com", + "credentials" : [ + { "type" : "password", + "value" : "password" } + ], + "realmRoles": ["manager", "user"], + "clientRoles" : { + "realm-management" : [ "impersonation", "view-users" ] + } } ], "clients": [