From 3818f8f575bbdb72953cd95c224f2801ba5bbf83 Mon Sep 17 00:00:00 2001 From: Lex Cao Date: Wed, 17 Jul 2024 23:29:08 +0800 Subject: [PATCH] Prevent removing flow that used by client flow overrides Closes #30707 Signed-off-by: Lex Cao --- .../models/utils/KeycloakModelUtils.java | 37 +++--- .../AbstractAuthenticationTest.java | 3 +- .../admin/authentication/FlowTest.java | 112 +++++++++++++++++- .../testsuite/forms/FlowOverrideTest.java | 16 ++- 4 files changed, 143 insertions(+), 25 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index a556e4ef93..a574495d94 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -30,15 +30,17 @@ import org.keycloak.common.util.Time; import org.keycloak.component.ComponentModel; import org.keycloak.crypto.Algorithm; import org.keycloak.deployment.DeployedConfigurationsManager; +import org.keycloak.models.AccountRoles; import org.keycloak.models.AuthenticationExecutionModel; -import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.AuthenticationFlowModel; +import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientScopeModel; import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.Constants; import org.keycloak.models.GroupModel; import org.keycloak.models.GroupProvider; +import org.keycloak.models.GroupProviderFactory; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakContext; import org.keycloak.models.KeycloakSession; @@ -50,15 +52,18 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.ScopeContainerModel; import org.keycloak.models.UserModel; import org.keycloak.protocol.oidc.OIDCConfigAttributes; +import org.keycloak.provider.Provider; +import org.keycloak.provider.ProviderFactory; import org.keycloak.representations.idm.CertificateRepresentation; +import org.keycloak.sessions.AuthenticationSessionModel; +import org.keycloak.sessions.RootAuthenticationSessionModel; import org.keycloak.transaction.JtaTransactionManagerLookup; import org.keycloak.utils.KeycloakSessionUtil; -import javax.crypto.spec.SecretKeySpec; import jakarta.transaction.InvalidTransactionException; import jakarta.transaction.SystemException; import jakarta.transaction.Transaction; - +import javax.crypto.spec.SecretKeySpec; import java.security.Key; import java.security.KeyPair; import java.security.PrivateKey; @@ -66,25 +71,18 @@ import java.security.PublicKey; import java.security.cert.X509Certificate; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.UUID; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import java.util.regex.Pattern; - -import org.keycloak.models.AccountRoles; -import org.keycloak.models.GroupProviderFactory; -import org.keycloak.provider.Provider; -import org.keycloak.provider.ProviderFactory; -import org.keycloak.sessions.AuthenticationSessionModel; -import org.keycloak.sessions.RootAuthenticationSessionModel; - import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Set of helper methods, which are useful in various model implementations. @@ -922,6 +920,17 @@ public final class KeycloakModelUtils { if ((realmFlow = realm.getDockerAuthenticationFlow()) != null && realmFlow.getId().equals(model.getId())) return true; if ((realmFlow = realm.getFirstBrokerLoginFlow()) != null && realmFlow.getId().equals(model.getId())) return true; + Stream browserFlowOverridingClients = realm.searchClientByAuthenticationFlowBindingOverrides(Collections.singletonMap("browser", model.getId()), 0, 1); + Stream directGrantFlowOverridingClients = realm.searchClientByAuthenticationFlowBindingOverrides(Collections.singletonMap("direct_grant", model.getId()), 0, 1); + boolean usedByClient = Stream.concat(browserFlowOverridingClients, directGrantFlowOverridingClients) + .limit(1) + .findAny() + .isPresent(); + + if (usedByClient) { + return true; + } + return realm.getIdentityProvidersStream().anyMatch(idp -> Objects.equals(idp.getFirstBrokerLoginFlowId(), model.getId()) || Objects.equals(idp.getPostBrokerLoginFlowId(), model.getId())); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/AbstractAuthenticationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/AbstractAuthenticationTest.java index 7167b7182a..05d9411bfe 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/AbstractAuthenticationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/AbstractAuthenticationTest.java @@ -217,12 +217,13 @@ public abstract class AbstractAuthenticationTest extends AbstractKeycloakTest { return config; } - void createFlow(AuthenticationFlowRepresentation flowRep) { + String createFlow(AuthenticationFlowRepresentation flowRep) { Response response = authMgmtResource.createFlow(flowRep); org.keycloak.testsuite.Assert.assertEquals(201, response.getStatus()); response.close(); String flowId = ApiUtil.getCreatedId(response); getCleanup().addAuthenticationFlowId(flowId); assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AssertAdminEvents.isExpectedPrefixFollowedByUuid(AdminEventPaths.authFlowsPath()), flowRep, ResourceType.AUTH_FLOW); + return flowId; } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java index 073c04da3a..913605af3b 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java @@ -19,6 +19,8 @@ package org.keycloak.testsuite.admin.authentication; import org.junit.Assert; import org.junit.Test; +import org.keycloak.admin.client.resource.ClientResource; +import org.keycloak.admin.client.resource.IdentityProviderResource; import org.keycloak.authentication.authenticators.browser.IdentityProviderAuthenticatorFactory; import org.keycloak.common.util.StreamUtil; import org.keycloak.events.admin.OperationType; @@ -28,7 +30,10 @@ import org.keycloak.representations.idm.AuthenticationExecutionExportRepresentat import org.keycloak.representations.idm.AuthenticationExecutionInfoRepresentation; import org.keycloak.representations.idm.AuthenticationFlowRepresentation; import org.keycloak.representations.idm.AuthenticatorConfigRepresentation; +import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.idm.OAuth2ErrorRepresentation; +import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.ContainerAssume; @@ -37,6 +42,7 @@ import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.ClientErrorException; import jakarta.ws.rs.InternalServerErrorException; import jakarta.ws.rs.NotFoundException; +import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; import java.io.IOException; @@ -47,14 +53,17 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.fail; import static org.keycloak.testsuite.util.Matchers.body; import static org.keycloak.testsuite.util.Matchers.statusCodeIs; @@ -70,7 +79,7 @@ public class FlowTest extends AbstractAuthenticationTest { createFlow(newFlow("Foo", "Foo flow", "generic", true, false)); addFlowToParent("Foo", "child"); addFlowToParent("child", "grandchild"); - + List flows = authMgmtResource.getFlows(); AuthenticationFlowRepresentation found = findFlowByAlias("Foo", flows); authMgmtResource.deleteFlow(found.getId()); @@ -78,7 +87,7 @@ public class FlowTest extends AbstractAuthenticationTest { createFlow(newFlow("Foo", "Foo flow", "generic", true, false)); addFlowToParent("Foo", "child"); - + // Under the old code, this would throw an error because "grandchild" // was left in the database addFlowToParent("child", "grandchild"); @@ -106,7 +115,7 @@ public class FlowTest extends AbstractAuthenticationTest { authMgmtResource.deleteFlow(findFlowByAlias("Foo", authMgmtResource.getFlows()).getId()); } - + private void addFlowToParent(String parentAlias, String childAlias) { Map data = new HashMap<>(); data.put("alias", childAlias); @@ -114,13 +123,13 @@ public class FlowTest extends AbstractAuthenticationTest { data.put("description", childAlias + " flow"); authMgmtResource.addExecutionFlow(parentAlias, data); } - + @Test public void testAddFlowWithRestrictedCharInAlias() { Response resp = authMgmtResource.createFlow(newFlow("fo]o", "Browser flow", "basic-flow", true, false)); Assert.assertEquals(400, resp.getStatus()); } - + @Test public void testAddRemoveFlow() { @@ -261,6 +270,97 @@ public class FlowTest extends AbstractAuthenticationTest { } } + @Test + public void testRemoveUsedFlow() { + String flowAlias = "test"; + String flowId = createFlow(newFlow(flowAlias, "Test flow", "generic", true, false)); + + Runnable assertRemoveFail = () -> { + try { + authMgmtResource.deleteFlow(flowId); + Assert.fail("Not expected to delete flow that in used."); + } catch (WebApplicationException e) { + OAuth2ErrorRepresentation error = e.getResponse().readEntity(OAuth2ErrorRepresentation.class); + Assert.assertEquals("For more on this error consult the server log at the debug level.", error.getErrorDescription()); + } + }; + + { + // used in realm flow + RealmRepresentation realm = realmResource.toRepresentation(); + BiConsumer, Consumer> assertRemoveFailInRealm = + (rollbackFlow, updateFlow) -> { + String rollbackValue = rollbackFlow.get(); + try { + updateFlow.accept(flowAlias); + realmResource.update(realm); + + assertRemoveFail.run(); + } finally { + updateFlow.accept(rollbackValue); + realmResource.update(realm); + } + }; + + assertRemoveFailInRealm.accept(realm::getBrowserFlow, realm::setBrowserFlow); + assertRemoveFailInRealm.accept(realm::getRegistrationFlow, realm::setRegistrationFlow); + assertRemoveFailInRealm.accept(realm::getClientAuthenticationFlow, realm::setClientAuthenticationFlow); + assertRemoveFailInRealm.accept(realm::getDirectGrantFlow, realm::setDirectGrantFlow); + assertRemoveFailInRealm.accept(realm::getResetCredentialsFlow, realm::setResetCredentialsFlow); + assertRemoveFailInRealm.accept(realm::getDockerAuthenticationFlow, realm::setDockerAuthenticationFlow); + assertRemoveFailInRealm.accept(realm::getFirstBrokerLoginFlow, realm::setFirstBrokerLoginFlow); + } + + { + // used by client override + ClientRepresentation client = realmResource.clients().findByClientId("account").get(0); + ClientResource clientResource = realmResource.clients().get(client.getId()); + + try { + client.setAuthenticationFlowBindingOverrides( + Map.of("browser", flowId) + ); + clientResource.update(client); + + assertRemoveFail.run(); + } finally { + client.setAuthenticationFlowBindingOverrides( + Map.of("browser", "") + ); + clientResource.update(client); + } + } + + { + // used by idp override + IdentityProviderRepresentation idp = new IdentityProviderRepresentation(); + idp.setAlias("idp"); + idp.setProviderId("oidc"); + + Response response = realmResource.identityProviders().create(idp); + Assert.assertNotNull(ApiUtil.getCreatedId(response)); + response.close(); + getCleanup().addIdentityProviderAlias(idp.getAlias()); + + IdentityProviderResource idpResource = realmResource.identityProviders().get("idp"); + BiConsumer, Consumer> assertRemoveFailByIdp = + (rollbackIdp, updateIdp) -> { + String rollbackValue = rollbackIdp.get(); + try { + updateIdp.accept(flowAlias); + idpResource.update(idp); + + assertRemoveFail.run(); + } finally { + updateIdp.accept(rollbackValue); + idpResource.update(idp); + } + }; + + assertRemoveFailByIdp.accept(idp::getFirstBrokerLoginFlowAlias, idp::setFirstBrokerLoginFlowAlias); + assertRemoveFailByIdp.accept(idp::getPostBrokerLoginFlowAlias, idp::setPostBrokerLoginFlowAlias); + } + } @Test public void testCopyFlow() { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/FlowOverrideTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/FlowOverrideTest.java index 8d94d5ad1a..a255a27db3 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/FlowOverrideTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/FlowOverrideTest.java @@ -27,12 +27,14 @@ import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.admin.client.resource.ClientsResource; import org.keycloak.authentication.authenticators.browser.UsernamePasswordFormFactory; import org.keycloak.common.Profile; +import org.keycloak.connections.jpa.JpaConnectionProvider; import org.keycloak.events.Details; import org.keycloak.models.AuthenticationExecutionModel; import org.keycloak.models.AuthenticationFlowBindings; import org.keycloak.models.AuthenticationFlowModel; import org.keycloak.models.ClientModel; import org.keycloak.models.RealmModel; +import org.keycloak.models.jpa.entities.AuthenticationFlowEntity; import org.keycloak.models.utils.TimeBasedOTP; import org.keycloak.representations.idm.AuthenticationFlowRepresentation; import org.keycloak.representations.idm.ClientRepresentation; @@ -50,6 +52,8 @@ import org.keycloak.testsuite.util.FlowUtil; import org.keycloak.util.BasicAuthHelper; import org.openqa.selenium.By; +import jakarta.persistence.EntityManager; +import jakarta.persistence.LockModeType; import jakarta.ws.rs.client.Client; import jakarta.ws.rs.client.Entity; import jakarta.ws.rs.client.WebTarget; @@ -61,7 +65,6 @@ import java.util.Map; import java.util.function.Consumer; import static org.junit.Assert.assertEquals; -import static org.keycloak.testsuite.forms.BrowserFlowTest.revertFlows; /** * Test that clients can override auth flows @@ -395,15 +398,20 @@ public class FlowOverrideTest extends AbstractFlowTest { String newFlowAlias = "Copy of Browser Flow"; testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); + AuthenticationFlowRepresentation newFlow = findFlowByAlias(newFlowAlias); try { // 1. set a flow override for client - AuthenticationFlowRepresentation newFlow = findFlowByAlias(newFlowAlias); clientRep.setAuthenticationFlowBindingOverrides(Map.of(binding, newFlow.getId())); clientResource.update(clientRep); - // 2. remove the flow - revertFlows(testRealm(), newFlowAlias); + // 2. remove the flow through database, since we could not delete the flow which is used by client + testingClient.server("test").run(session -> { + EntityManager em = session.getProvider(JpaConnectionProvider.class).getEntityManager(); + AuthenticationFlowEntity entity = em.find(AuthenticationFlowEntity.class, newFlow.getId(), LockModeType.PESSIMISTIC_WRITE); + em.remove(entity); + em.flush(); + }); // 3. login with client testNoOverride.accept(clientRep.getClientId());