Check alias is unique for authenticator config when it is created

Closes #31727

Signed-off-by: rmartinc <rmartinc@redhat.com>

services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java

@@ -461,6 +461,10 @@ public class AuthenticationManagementResource {
 
                     if (config.getAlias() != null) {
                         config.setAlias(newName + " " + config.getAlias());
+                        if (configManager.getAuthenticatorConfigByAlias(realm, config.getAlias()) != null) {
+                            logger.warnf("Authentication execution configuration [%s] already exists", config.getAlias());
+                            throw new IllegalStateException("Authentication execution configuration " + config.getAlias() + " already exists.");
+                        }
                     }
 
                     AuthenticatorConfigModel newConfig = realm.addAuthenticatorConfig(config);
@@ -1025,24 +1029,41 @@ public class AuthenticationManagementResource {
     public Response newExecutionConfig(@Parameter(description = "Execution id") @PathParam("executionId") String execution, @Parameter(description = "JSON with new configuration") AuthenticatorConfigRepresentation json) {
         auth.realm().requireManageRealm();
 
+        if (json.getAlias() == null || json.getAlias().isEmpty()) {
+            throw ErrorResponse.exists("Failed to create authentication execution configuration with empty alias name");
+        }
+
         ReservedCharValidator.validate(json.getAlias());
 
         AuthenticationExecutionModel model = realm.getAuthenticationExecutionById(execution);
         if (model == null) {
             session.getTransactionManager().setRollbackOnly();
             throw new NotFoundException("Illegal execution");
+        }
 
+        // retrieve the previous configuration if assigned
+        AuthenticatorConfigModel prevConfig = null;
+        if (model.getAuthenticatorConfig() != null) {
+            prevConfig = realm.getAuthenticatorConfigById(model.getAuthenticatorConfig());
         }
+
+        AuthenticatorConfigModel otherConfig = realm.getAuthenticatorConfigByAlias(json.getAlias());
+        if (otherConfig != null && (prevConfig == null || !prevConfig.getId().equals(otherConfig.getId()))) {
+            throw ErrorResponse.exists("Authentication execution configuration " + json.getAlias() + " already exists");
+        }
+
+        // remove the previous config
+        if (prevConfig != null) {
+            realm.removeAuthenticatorConfig(prevConfig);
+        }
+
         AuthenticatorConfigModel config = RepresentationToModel.toModel(json);
-        if (config.getAlias() == null) {
-            throw ErrorResponse.error("Alias missing", Response.Status.BAD_REQUEST);
-        }
         config = realm.addAuthenticatorConfig(config);
         model.setAuthenticatorConfig(config.getId());
         realm.updateAuthenticatorExecution(model);
 
         json.setId(config.getId());
-        adminEvent.operation(OperationType.CREATE).resource(ResourceType.AUTH_EXECUTION).resourcePath(session.getContext().getUri()).representation(json).success();
+        adminEvent.operation(OperationType.CREATE).resource(ResourceType.AUTHENTICATOR_CONFIG).resourcePath(session.getContext().getUri()).representation(json).success();
         return Response.created(session.getContext().getUri().getAbsolutePathBuilder().path(config.getId()).build()).build();
     }
 
@@ -1529,6 +1550,14 @@ public class AuthenticationManagementResource {
     public Response createAuthenticatorConfig(@Parameter(description = "JSON describing new authenticator configuration") AuthenticatorConfigRepresentation rep) {
         auth.realm().requireManageRealm();
 
+        if (rep.getAlias() == null || rep.getAlias().isEmpty()) {
+            throw ErrorResponse.exists("Failed to create authentication execution configuration with empty alias name");
+        }
+
+        if (realm.getAuthenticatorConfigByAlias(rep.getAlias()) != null) {
+            throw ErrorResponse.exists("Authentication execution configuration " + rep.getAlias() + " already exists");
+        }
+
         ReservedCharValidator.validate(rep.getAlias());
 
         AuthenticatorConfigModel config = realm.addAuthenticatorConfig(RepresentationToModel.toModel(rep));

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/AuthenticatorConfigTest.java

@@ -20,6 +20,7 @@ package org.keycloak.testsuite.admin.authentication;
 import org.junit.Before;
 import org.junit.Test;
 import org.keycloak.authentication.authenticators.broker.IdpCreateUserIfUniqueAuthenticatorFactory;
+import org.keycloak.authentication.authenticators.broker.IdpDetectExistingBrokerUserAuthenticatorFactory;
 import org.keycloak.events.admin.OperationType;
 import org.keycloak.events.admin.ResourceType;
 import org.keycloak.representations.idm.AuthenticationExecutionInfoRepresentation;
@@ -30,34 +31,42 @@ import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.util.AdminEventPaths;
 
+import jakarta.ws.rs.BadRequestException;
 import jakarta.ws.rs.NotFoundException;
 import jakarta.ws.rs.core.Response;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import jakarta.ws.rs.BadRequestException;
 
 /**
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
  */
 public class AuthenticatorConfigTest extends AbstractAuthenticationTest {
 
+    private String flowId;
     private String executionId;
+    private String executionId2;
 
     @Before
     public void beforeConfigTest() {
         AuthenticationFlowRepresentation flowRep = newFlow("firstBrokerLogin2", "firstBrokerLogin2", "basic-flow", true, false);
-        createFlow(flowRep);
+        flowId = createFlow(flowRep);
 
         HashMap<String, Object> params = new HashMap<>();
         params.put("provider", IdpCreateUserIfUniqueAuthenticatorFactory.PROVIDER_ID);
         authMgmtResource.addExecution("firstBrokerLogin2", params);
         assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionPath("firstBrokerLogin2"), params, ResourceType.AUTH_EXECUTION);
+        params.put("provider", IdpDetectExistingBrokerUserAuthenticatorFactory.PROVIDER_ID);
+        authMgmtResource.addExecution("firstBrokerLogin2", params);
+        assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionPath("firstBrokerLogin2"), params, ResourceType.AUTH_EXECUTION);
 
         List<AuthenticationExecutionInfoRepresentation> executionReps = authMgmtResource.getExecutions("firstBrokerLogin2");
         AuthenticationExecutionInfoRepresentation exec = findExecutionByProvider(IdpCreateUserIfUniqueAuthenticatorFactory.PROVIDER_ID, executionReps);
         Assert.assertNotNull(exec);
         executionId = exec.getId();
+        exec = findExecutionByProvider(IdpDetectExistingBrokerUserAuthenticatorFactory.PROVIDER_ID, executionReps);
+        Assert.assertNotNull(exec);
+        executionId2 = exec.getId();
     }
 
     @Test
@@ -72,9 +81,9 @@ public class AuthenticatorConfigTest extends AbstractAuthenticationTest {
         AuthenticatorConfigRepresentation cfg = newConfig("foo", IdpCreateUserIfUniqueAuthenticatorFactory.REQUIRE_PASSWORD_UPDATE_AFTER_REGISTRATION, "true");
 
         // Attempt to create config for non-existent execution
-        Response response = authMgmtResource.newExecutionConfig("exec-id-doesnt-exists", cfg);
+        try (Response response = authMgmtResource.newExecutionConfig("exec-id-doesnt-exists", cfg)) {
-        Assert.assertEquals(404, response.getStatus());
+            Assert.assertEquals(404, response.getStatus());
-        response.close();
+        }
 
         // Create config success
         String cfgId = createConfig(executionId, cfg);
@@ -102,18 +111,14 @@ public class AuthenticatorConfigTest extends AbstractAuthenticationTest {
     public void testUpdateConfig() {
         AuthenticatorConfigRepresentation cfg = newConfig("foo", IdpCreateUserIfUniqueAuthenticatorFactory.REQUIRE_PASSWORD_UPDATE_AFTER_REGISTRATION, "true");
         String cfgId = createConfig(executionId, cfg);
-        AuthenticatorConfigRepresentation cfgRep = authMgmtResource.getAuthenticatorConfig(cfgId);
+        final AuthenticatorConfigRepresentation cfgRepNonExistent = authMgmtResource.getAuthenticatorConfig(cfgId);
 
         // Try to update not existent config
-        try {
+        NotFoundException nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.updateAuthenticatorConfig("not-existent", cfgRepNonExistent));
-            authMgmtResource.updateAuthenticatorConfig("not-existent", cfgRep);
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
-            Assert.fail("Config didn't found");
-        } catch (NotFoundException nfe) {
-            // Expected
-        }
 
         // Assert nothing changed
-        cfgRep = authMgmtResource.getAuthenticatorConfig(cfgId);
+        AuthenticatorConfigRepresentation cfgRep = authMgmtResource.getAuthenticatorConfig(cfgId);
         assertConfig(cfgRep, cfgId, "foo", IdpCreateUserIfUniqueAuthenticatorFactory.REQUIRE_PASSWORD_UPDATE_AFTER_REGISTRATION, "true");
 
         // Update success
@@ -141,26 +146,17 @@ public class AuthenticatorConfigTest extends AbstractAuthenticationTest {
                 IdpCreateUserIfUniqueAuthenticatorFactory.PROVIDER_ID, authMgmtResource.getExecutions("firstBrokerLogin2"));
         Assert.assertEquals(cfgRep.getId(), execution.getAuthenticationConfig());
 
-
         // Test remove not-existent
-        try {
+        NotFoundException nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.removeAuthenticatorConfig("not-existent"));
-            authMgmtResource.removeAuthenticatorConfig("not-existent");
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
-            Assert.fail("Config didn't found");
-        } catch (NotFoundException nfe) {
-            // Expected
-        }
 
         // Test remove our config
         authMgmtResource.removeAuthenticatorConfig(cfgId);
         assertAdminEvents.assertEvent(testRealmId, OperationType.DELETE, AdminEventPaths.authExecutionConfigPath(cfgId), ResourceType.AUTHENTICATOR_CONFIG);
 
         // Assert config not found
-        try {
+        nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getAuthenticatorConfig(cfgRep.getId()));
-            authMgmtResource.getAuthenticatorConfig(cfgRep.getId());
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
-            Assert.fail("Not expected to find config");
-        } catch (NotFoundException nfe) {
-            // Expected
-        }
 
         // Assert execution doesn't have our config
         execution = findExecutionByProvider(
@@ -178,13 +174,61 @@ public class AuthenticatorConfigTest extends AbstractAuthenticationTest {
         Assert.assertTrue(description.getProperties().isEmpty());
     }
 
+    @Test
+    public void testDuplicateAuthenticatorConfigAlias() {
+        // create a config for step1
+        AuthenticatorConfigRepresentation config1 = new AuthenticatorConfigRepresentation();
+        config1.setAlias("test-config-1");
+        config1.setConfig(Map.of("key", "value"));
+        String config1Id = createConfig(executionId, config1);
+
+        // create the same config name for step2, should fail
+        try (Response response = authMgmtResource.newExecutionConfig(executionId2, config1)) {
+            Assert.assertEquals(409, response.getStatus());
+        }
+
+        // create a config for step2
+        AuthenticatorConfigRepresentation config2 = new AuthenticatorConfigRepresentation();
+        config2.setAlias("test-config-2");
+        config2.setConfig(Map.of("key", "value"));
+        String config2Id = createConfig(executionId, config2);
+
+        // create a new config for step1, config1 should be removed
+        AuthenticatorConfigRepresentation config3 = new AuthenticatorConfigRepresentation();
+        config3.setAlias("test-config-1-modified");
+        config3.setConfig(Map.of("key", "value"));
+        String tmpConfig3Id = createConfig(executionId, config3);
+        NotFoundException nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getAuthenticatorConfig(config1Id));
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
+
+        // create a new config with thew same name but that overwrites the previous one
+        String config3Id = createConfig(executionId, config3);
+        nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getAuthenticatorConfig(tmpConfig3Id));
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
+
+        // delete execution step1, config3 should be removed
+        authMgmtResource.removeExecution(executionId);
+        assertAdminEvents.assertEvent(testRealmId, OperationType.DELETE, AdminEventPaths.authExecutionPath(executionId), ResourceType.AUTH_EXECUTION);
+        nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getAuthenticatorConfig(config3Id));
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
+
+        // remove flow, config and exec for step2 should be removed
+        authMgmtResource.deleteFlow(flowId);
+        assertAdminEvents.assertEvent(testRealmId, OperationType.DELETE, AdminEventPaths.authFlowPath(flowId), ResourceType.AUTH_FLOW);
+        nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getExecution(executionId));
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
+        nfe = Assert.assertThrows(NotFoundException.class, () -> authMgmtResource.getAuthenticatorConfig(config2Id));
+        Assert.assertEquals(404, nfe.getResponse().getStatus());
+    }
+
     private String createConfig(String executionId, AuthenticatorConfigRepresentation cfg) {
-        Response resp = authMgmtResource.newExecutionConfig(executionId, cfg);
+        try (Response resp = authMgmtResource.newExecutionConfig(executionId, cfg)) {
-        Assert.assertEquals(201, resp.getStatus());
+            Assert.assertEquals(201, resp.getStatus());
-        String cfgId = ApiUtil.getCreatedId(resp);
+            String cfgId = ApiUtil.getCreatedId(resp);
-        Assert.assertNotNull(cfgId);
+            Assert.assertNotNull(cfgId);
-        assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionConfigPath(executionId), cfg, ResourceType.AUTH_EXECUTION);
+            assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionConfigPath(executionId), cfg, ResourceType.AUTHENTICATOR_CONFIG);
-        return cfgId;
+            return cfgId;
+        }
     }
 
     private AuthenticatorConfigRepresentation newConfig(String alias, String cfgKey, String cfgValue) {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java

@@ -736,7 +736,7 @@ public class FlowTest extends AbstractAuthenticationTest {
 
         try (Response response = authMgmtResource.newExecutionConfig(executionWithConfig.getId(), executionConfig)) {
             getCleanup().addAuthenticationConfigId(ApiUtil.getCreatedId(response));
-            assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionConfigPath(executionWithConfig.getId()), executionConfig, ResourceType.AUTH_EXECUTION);
+            assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionConfigPath(executionWithConfig.getId()), executionConfig, ResourceType.AUTHENTICATOR_CONFIG);
         }
 
         String newFlowName = "Duplicated of " + DefaultAuthenticationFlows.BROWSER_FLOW;

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/AuthenticationMethodReferenceTest.java

@@ -395,7 +395,7 @@ public class AuthenticationMethodReferenceTest extends AbstractOIDCScopeTest{
         if (execution.getAuthenticationConfig() == null){
             // create config if it doesn't exist
             AuthenticatorConfigRepresentation config = new AuthenticatorConfigRepresentation();
-            config.setAlias("test");
+            config.setAlias(KeycloakModelUtils.generateId());
             config.setConfig(new HashMap<>(){{
                 put(AMR_VALUE_KEY, amrValue);
                 put(AMR_MAX_AGE_KEY, maxAge.toString());