Prevent removing flow that used by client flow overrides

Closes #30707

Signed-off-by: Lex Cao <lexcao@foxmail.com>

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<ClientModel> browserFlowOverridingClients = realm.searchClientByAuthenticationFlowBindingOverrides(Collections.singletonMap("browser", model.getId()), 0, 1);
+        Stream<ClientModel> 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()));

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;
     }
 }

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<AuthenticationFlowRepresentation> 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<String, Object> 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<Supplier<String>, Consumer<String>> 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<Supplier<String>, Consumer<String>> 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() {

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
+            // 2. remove the flow through database, since we could not delete the flow which is used by client
-            revertFlows(testRealm(), newFlowAlias);
+            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());