From e41c3bcf0a3dbf5ae65f17bbd2a351c70130ece9 Mon Sep 17 00:00:00 2001 From: jonathan langlois Date: Thu, 29 Aug 2024 10:44:39 -0700 Subject: [PATCH 1/2] feat: db session removal make dc auth flow keep session if only linked to current client --- .tool-versions | 2 +- .../configuration/24/quarkus.properties | 8 +- .../keycloak/extensions-24/services/pom.xml | 22 ++- .../authenticators/UserSessionRemover.java | 25 ++- .../UserSessionRemoverTest.java | 145 ++++++++++++++++++ 5 files changed, 182 insertions(+), 20 deletions(-) create mode 100644 docker/keycloak/extensions-24/services/src/test/java/com/github/bcgov/keycloak/authenticators/UserSessionRemoverTest.java diff --git a/.tool-versions b/.tool-versions index d720b89d..e59b9e79 100644 --- a/.tool-versions +++ b/.tool-versions @@ -8,5 +8,5 @@ k6 0.34.1 terraform 1.2.0 terraform-docs 0.12.1 tflint 0.28.1 -java openjdk-14.0.1 +java openjdk-17.0.1 gradle 7.3.1 diff --git a/docker/keycloak/configuration/24/quarkus.properties b/docker/keycloak/configuration/24/quarkus.properties index bd0a5b9a..61b29461 100644 --- a/docker/keycloak/configuration/24/quarkus.properties +++ b/docker/keycloak/configuration/24/quarkus.properties @@ -5,8 +5,8 @@ quarkus.log.file.json.exception-output-type=formatted quarkus.log.file.json.key-overrides=timestamp=@timestamp quarkus.log.file.json.additional-field."@version".value=1 # Quarkus will auto-compress if ending with .zip: https://quarkus.io/guides/logging. -quarkus.log.file.rotation.file-suffix=.zip +quarkus.log.file.rotation.file-suffix=${QUARKUS_LOG_FILE_ROTATION_FILE_SUFFIX:.zip} # Optional: Disable rotation by size (adjust value as needed) -quarkus.log.file.rotation.max-file-size=200M -# The number of rotated files. From above configuration, this will keep 200M * 42 files ~= 8Gigabytes of data before replacing. -quarkus.log.file.rotation.max-backup-index=42 +quarkus.log.file.rotation.max-file-size=${QUARKUS_LOG_FILE_ROTATION_MAX_FILE_SIZE:200M} +# The number of rotated files per pod. From above configuration, this will keep 200M * 14 files * 3pods ~= 8Gigabytes of data before replacing. +quarkus.log.file.rotation.max-backup-index=${QUARKUS_LOG_FILE_ROTATION_MAX_BACKUP_INDEX:14} diff --git a/docker/keycloak/extensions-24/services/pom.xml b/docker/keycloak/extensions-24/services/pom.xml index e544a4e4..26a6b71a 100644 --- a/docker/keycloak/extensions-24/services/pom.xml +++ b/docker/keycloak/extensions-24/services/pom.xml @@ -46,6 +46,11 @@ 17 + + org.apache.maven.plugins + maven-surefire-plugin + 2.22.0 + @@ -136,12 +141,13 @@ junit junit + 4.13.2 test org.mockito - mockito-all - 1.9.5 + mockito-core + 5.3.1 test @@ -149,5 +155,17 @@ hamcrest-all test + + org.junit.jupiter + junit-jupiter-engine + 5.9.1 + test + + + org.junit.jupiter + junit-jupiter-api + 5.9.1 + test + diff --git a/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java b/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java index fc4c27dc..bdb68860 100644 --- a/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java +++ b/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java @@ -6,9 +6,11 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserSessionProvider; +import org.keycloak.models.AuthenticatedClientSessionModel; import org.keycloak.services.managers.AuthenticationManager; import org.keycloak.authentication.AuthenticationFlowContext; import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.models.UserSessionModel; import java.util.Map; @@ -23,12 +25,8 @@ public boolean requiresUser() { @Override public void authenticate(AuthenticationFlowContext context) { - AuthenticationSessionModel session = context.getAuthenticationSession(); - AuthenticationManager.AuthResult authResult = AuthenticationManager.authenticateIdentityCookie( - context.getSession(), - context.getRealm(), - true - ); + UserSessionModel userSessionModel; + AuthenticationManager.AuthResult authResult = AuthenticationManager.authenticateIdentityCookie(context.getSession(), context.getRealm(), true); // 1. If no Cookie session, proceed to next step if (authResult == null) { @@ -36,21 +34,22 @@ public void authenticate(AuthenticationFlowContext context) { return; } - // Need to use the KeycloakSession context to get the authenticating client ID. Not available on the AuthenticationFlowContext. - KeycloakSession keycloakSession = context.getSession(); - String authenticatingClientUUID = keycloakSession.getContext().getClient().getId(); + userSessionModel = authResult.getSession(); - // Get all existing sessions. If any session is associated with a different client, clear all user sessions. - UserSessionProvider userSessionProvider = keycloakSession.sessions(); - Map activeClientSessionStats = userSessionProvider.getActiveClientSessionStats(context.getRealm(), false); + String authenticatingClientUUID = context.getSession().getContext().getClient().getId(); + UserSessionProvider userSessionProvider = context.getSession().sessions(); - for (String activeSessionClientUUID : activeClientSessionStats.keySet()) { + // Must fetch sessions from the user session model, user session provider has all session in the realm + Map authenticatedClientSessions = userSessionModel.getAuthenticatedClientSessions(); + + for (String activeSessionClientUUID : authenticatedClientSessions.keySet()) { if (!activeSessionClientUUID.equals(authenticatingClientUUID)) { userSessionProvider.removeUserSession(context.getRealm(), authResult.getSession()); } } context.attempted(); + return; } @Override diff --git a/docker/keycloak/extensions-24/services/src/test/java/com/github/bcgov/keycloak/authenticators/UserSessionRemoverTest.java b/docker/keycloak/extensions-24/services/src/test/java/com/github/bcgov/keycloak/authenticators/UserSessionRemoverTest.java new file mode 100644 index 00000000..8c9a0564 --- /dev/null +++ b/docker/keycloak/extensions-24/services/src/test/java/com/github/bcgov/keycloak/authenticators/UserSessionRemoverTest.java @@ -0,0 +1,145 @@ +package com.github.bcgov.keycloak.testsuite.authenticators; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import org.mockito.Mockito; +import org.mockito.MockedStatic; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.BeforeEach; + +import com.github.bcgov.keycloak.authenticators.UserSessionRemover; +import org.keycloak.authentication.AuthenticationFlowContext; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.AuthenticatedClientSessionModel; +import org.keycloak.models.RealmModel; +import org.keycloak.services.managers.AuthenticationManager; +import org.keycloak.models.ClientModel; +import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.models.UserSessionProvider; +import org.keycloak.models.AuthenticatedClientSessionModel; +import org.keycloak.models.UserSessionModel; +import org.keycloak.models.KeycloakContext; +import java.util.HashMap; +import java.util.Map; + +public class UserSessionRemoverTest { + private static final UserSessionRemover userSessionRemover = new UserSessionRemover(); + + private AuthenticationFlowContext context; + private KeycloakSession session; + private RealmModel realm; + private AuthenticationSessionModel authSession; + private UserSessionProvider userSessionProvider; + private KeycloakSession keycloakSession; + private ClientModel client; + private KeycloakContext keycloakContext; + private AuthenticationManager.AuthResult authResult; + private UserSessionModel userSessionModel; + private AuthenticatedClientSessionModel authenticatedClientSessionModel; + + @BeforeEach + public void setup() { + // Initialize mocks for necessary objects + context = mock(AuthenticationFlowContext.class); + realm = mock(RealmModel.class); + authSession = mock(AuthenticationSessionModel.class); + userSessionProvider = mock(UserSessionProvider.class); + keycloakSession = mock(KeycloakSession.class); + keycloakContext = mock(KeycloakContext.class); + client = mock(ClientModel.class); + authResult = mock(AuthenticationManager.AuthResult.class); + userSessionModel = mock(UserSessionModel.class); + authenticatedClientSessionModel = mock(AuthenticatedClientSessionModel.class); + + + // Set up common behavior of the mocks + when(context.getSession()).thenReturn(keycloakSession); + when(context.getRealm()).thenReturn(realm); + when(context.getAuthenticationSession()).thenReturn(authSession); + when(keycloakSession.sessions()).thenReturn(userSessionProvider); + when(context.getSession()).thenReturn(keycloakSession); + when(keycloakSession.getContext()).thenReturn(keycloakContext); + when(keycloakContext.getClient()).thenReturn(client); + when(authResult.getSession()).thenReturn(userSessionModel); + } + + @Test + public void testSkipClientSessionCheckWhenNullAuthResult() throws Exception { + try (MockedStatic authenticationManager = Mockito.mockStatic(AuthenticationManager.class)) { + authenticationManager.when(() -> AuthenticationManager.authenticateIdentityCookie( + any(KeycloakSession.class), any(RealmModel.class), any(Boolean.class) + )).thenReturn(null); + userSessionRemover.authenticate(context); + + // Keycloak Session Context check skipped if no Auth Session + verify(keycloakSession, times(0)).getContext(); + verify(userSessionProvider, times(0)).removeUserSession(any(RealmModel.class), any(UserSessionModel.class)); + } + } + + @Test + public void testRemovesUserSessionsWhenMultipleClientSessionsExist() throws Exception { + when(client.getId()).thenReturn("client1"); + Map authenticatedClientSessions = new HashMap<>(); + authenticatedClientSessions.put("client1", authenticatedClientSessionModel); + authenticatedClientSessions.put("client2", authenticatedClientSessionModel); + + when(userSessionModel.getAuthenticatedClientSessions()).thenReturn(authenticatedClientSessions); + + try (MockedStatic authenticationManager = Mockito.mockStatic(AuthenticationManager.class)) { + authenticationManager.when(() -> AuthenticationManager.authenticateIdentityCookie( + any(KeycloakSession.class), any(RealmModel.class), any(Boolean.class) + )).thenReturn(authResult); + + userSessionRemover.authenticate(context); + + verify(keycloakSession, times(1)).getContext(); + verify(userSessionProvider, times(1)).removeUserSession(any(RealmModel.class), any(UserSessionModel.class)); + } + } + + @Test + public void testRemovesUserSessionsWhenSingleDifferentClientSessionFound() throws Exception { + when(client.getId()).thenReturn("client1"); + Map authenticatedClientSessions = new HashMap<>(); + authenticatedClientSessions.put("client2", authenticatedClientSessionModel); + + when(userSessionModel.getAuthenticatedClientSessions()).thenReturn(authenticatedClientSessions); + + try (MockedStatic authenticationManager = Mockito.mockStatic(AuthenticationManager.class)) { + authenticationManager.when(() -> AuthenticationManager.authenticateIdentityCookie( + any(KeycloakSession.class), any(RealmModel.class), any(Boolean.class) + )).thenReturn(authResult); + userSessionRemover.authenticate(context); + + verify(keycloakSession, times(1)).getContext(); + verify(userSessionProvider, times(1)).removeUserSession(any(RealmModel.class), any(UserSessionModel.class)); + } + } + + @Test + public void testLeavesExistingSessionWhenOnlyAssociatedToAuthenticatingClient() throws Exception { + when(client.getId()).thenReturn("client1"); + Map authenticatedClientSessions = new HashMap<>(); + authenticatedClientSessions.put("client1", authenticatedClientSessionModel); + + when(userSessionModel.getAuthenticatedClientSessions()).thenReturn(authenticatedClientSessions); + + try (MockedStatic authenticationManager = Mockito.mockStatic(AuthenticationManager.class)) { + authenticationManager.when(() -> AuthenticationManager.authenticateIdentityCookie( + any(KeycloakSession.class), any(RealmModel.class), any(Boolean.class) + )).thenReturn(authResult); + userSessionRemover.authenticate(context); + + // Verify the keycloak session context is invoked to check client sessions + verify(keycloakSession, times(1)).getContext(); + + // Remove user session should be skipped + verify(userSessionProvider, times(0)).removeUserSession(any(RealmModel.class), any(UserSessionModel.class)); + } + } +} From dcb5932de0c5ee23fafb8be0def43ce6727eaf36 Mon Sep 17 00:00:00 2001 From: jonathan langlois Date: Thu, 29 Aug 2024 10:51:28 -0700 Subject: [PATCH 2/2] feat: reuse var remove duplicate method call --- .../bcgov/keycloak/authenticators/UserSessionRemover.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java b/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java index bdb68860..ced75e6f 100644 --- a/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java +++ b/docker/keycloak/extensions-24/services/src/main/java/com/github/bcgov/keycloak/authenticators/UserSessionRemover.java @@ -44,7 +44,7 @@ public void authenticate(AuthenticationFlowContext context) { for (String activeSessionClientUUID : authenticatedClientSessions.keySet()) { if (!activeSessionClientUUID.equals(authenticatingClientUUID)) { - userSessionProvider.removeUserSession(context.getRealm(), authResult.getSession()); + userSessionProvider.removeUserSession(context.getRealm(), userSessionModel); } }