"Allowed Protocol Mapper Types" prevents clients from self-updating via client registration api (#27578)

closes #27558 

Signed-off-by: Réda Housni Alaoui <reda-alaoui@hey.com>

services/src/main/java/org/keycloak/services/clientregistration/policy/impl/ProtocolMappersClientRegistrationPolicy.java

@@ -18,12 +18,15 @@
 package org.keycloak.services.clientregistration.policy.impl;
 
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.stream.Collectors;
 
 import org.jboss.logging.Logger;
 import org.keycloak.component.ComponentModel;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.ProtocolMapperModel;
 import org.keycloak.representations.idm.ProtocolMapperRepresentation;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.clientregistration.ClientRegistrationContext;
@@ -48,10 +51,10 @@ public class ProtocolMappersClientRegistrationPolicy implements ClientRegistrati
 
     @Override
     public void beforeRegister(ClientRegistrationContext context) throws ClientRegistrationPolicyException {
-        testMappers(context);
+        testMappers(context, null);
     }
 
-    protected void testMappers(ClientRegistrationContext context) throws ClientRegistrationPolicyException {
+    protected void testMappers(ClientRegistrationContext context, ClientModel clientModel) throws ClientRegistrationPolicyException {
         List<ProtocolMapperRepresentation> protocolMappers = context.getClient().getProtocolMappers();
         if (protocolMappers == null) {
             return;
@@ -59,16 +62,42 @@ public class ProtocolMappersClientRegistrationPolicy implements ClientRegistrati
 
         List<String> allowedMapperProviders = getAllowedMapperProviders();
 
-        for (ProtocolMapperRepresentation mapper : protocolMappers) {
+        for (ProtocolMapperRepresentation mapperRepresentation : protocolMappers) {
-            String mapperType = mapper.getProtocolMapper();
+            String mapperType = mapperRepresentation.getProtocolMapper();
 
-            if (!allowedMapperProviders.contains(mapperType)) {
+			if (allowedMapperProviders.contains(mapperType)) {
-                ServicesLogger.LOGGER.clientRegistrationMapperNotAllowed(mapper.getName(), mapperType);
+				continue;
-                throw new ClientRegistrationPolicyException("ProtocolMapper type not allowed");
+			}
-            }
+			if (clientModel == null) {
-        }
+				failWithProtocolMapperTypeNotAllowedError(mapperRepresentation);
+				return;
+			}
+			String mapperRepresentationId = mapperRepresentation.getId();
+			if (mapperRepresentationId == null) {
+				String message = "Missing id for mapper named '%s'".formatted(mapperRepresentation.getName());
+				ServicesLogger.LOGGER.warn(message);
+				throw new ClientRegistrationPolicyException(message);
+			}
+			ProtocolMapperModel mapperModel = clientModel.getProtocolMapperById(mapperRepresentationId);
+			if (mapperModel == null) {
+				String message = "No existing mapper model found for id '%s'".formatted(mapperRepresentationId);
+				ServicesLogger.LOGGER.warn(message);
+				throw new ClientRegistrationPolicyException(message);
+			}
+			Map<String, String> modelConfig = mapperModel.getConfig();
+			Map<String, String> representationConfig = mapperRepresentation.getConfig();
+			if (!Objects.equals(representationConfig, modelConfig)) {
+				failWithProtocolMapperTypeNotAllowedError(mapperRepresentation);
+				return;
+			}
+		}
     }
 
+	protected void failWithProtocolMapperTypeNotAllowedError(ProtocolMapperRepresentation mapper) {
+		ServicesLogger.LOGGER.clientRegistrationMapperNotAllowed(mapper.getName(), mapper.getProtocolMapper());
+		throw new ClientRegistrationPolicyException("ProtocolMapper type not allowed");
+	}
+
     // Remove builtin mappers of unsupported types too
     @Override
     public void afterRegister(ClientRegistrationContext context, ClientModel clientModel) {
@@ -82,10 +111,9 @@ public class ProtocolMappersClientRegistrationPolicy implements ClientRegistrati
                 .forEach(clientModel::removeProtocolMapper);
     }
 
-    // We don't take already existing protocolMappers into consideration for now
     @Override
     public void beforeUpdate(ClientRegistrationContext context, ClientModel clientModel) throws ClientRegistrationPolicyException {
-        testMappers(context);
+        testMappers(context, clientModel);
     }
 
     @Override

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationPoliciesTest.java

@@ -21,6 +21,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.UUID;
 import java.util.stream.Collectors;
 
 import org.junit.After;
@@ -537,7 +538,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe
 
         // Revert policy change
         ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
-        protocolMapperPolicyRep.getConfig().remove(ProtocolMappersClientRegistrationPolicyFactory.ALLOWED_PROTOCOL_MAPPER_TYPES, HardcodedRole.PROVIDER_ID);
+        protocolMapperPolicyRep.getConfig().get(ProtocolMappersClientRegistrationPolicyFactory.ALLOWED_PROTOCOL_MAPPER_TYPES).remove(HardcodedRole.PROVIDER_ID);
         realmResource().components().component(protocolMapperPolicyRep.getId()).update(protocolMapperPolicyRep);
     }
 
@@ -572,7 +573,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe
         registeredClient.getProtocolMappers().add(createHardcodedMapperRep());
 
         // Check I can't update client because of protocolMapper
-        assertFail(ClientRegOp.UPDATE, registeredClient, 403, "ProtocolMapper type not allowed");
+        assertFail(ClientRegOp.UPDATE, registeredClient, 403, "Missing id for mapper named 'Hardcoded foo role'");
 
         // Remove "bad" protocolMapper
         registeredClient.getProtocolMappers().removeIf((ProtocolMapperRepresentation mapper) -> {
@@ -586,6 +587,94 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe
         ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
     }
 
+    @Test
+    public void passingAnAlreadyAssignedProtocolMapperWithDisallowedTypeDoesNotFail() throws Exception {
+        setTrustedHost("localhost");
+
+        ClientRepresentation clientRep = createRep("test-app");
+        // Create the client with a client registration disallowed protocolMapper
+        clientRep.setProtocolMappers(List.of(createHardcodedMapperRep()));
+        Response adminClientCreationResponse = realmResource().clients().create(clientRep);
+        String clientTechnicalId = ApiUtil.getCreatedId(adminClientCreationResponse);
+        adminClientCreationResponse.close();
+
+        ClientResource clientResource = realmResource().clients().get(clientTechnicalId);
+
+        reg.auth(Auth.token(clientResource.regenerateRegistrationAccessToken()));
+        // Check the client can be updated with a representation keeping the disallowed protocolMapper
+        reg.update(clientResource.toRepresentation());
+
+        // Revert client
+        ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
+    }
+
+    @Test
+    public void passingNewProtocolMapperOfDisallowedTypeInAdditionToAnAlreadyAssignedOneOfTheSameTypeFails() {
+        setTrustedHost("localhost");
+
+        ClientRepresentation clientRep = createRep("test-app");
+        // Create the client with a client registration disallowed protocolMapper
+        clientRep.setProtocolMappers(List.of(createHardcodedMapperRep()));
+        Response adminClientCreationResponse = realmResource().clients().create(clientRep);
+        String clientTechnicalId = ApiUtil.getCreatedId(adminClientCreationResponse);
+        adminClientCreationResponse.close();
+
+        ClientResource clientResource = realmResource().clients().get(clientTechnicalId);
+
+        reg.auth(Auth.token(clientResource.regenerateRegistrationAccessToken()));
+
+        ClientRepresentation representation = clientResource.toRepresentation();
+        representation.getProtocolMappers().add(createHardcodedMapperRep());
+        assertFail(ClientRegOp.UPDATE, representation, 403, "Missing id for mapper named 'Hardcoded foo role'");
+
+        // Revert client
+        ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
+    }
+
+    @Test
+    public void alteringAnAlreadyAssignedProtocolMappersConfigWithDisallowedTypeFails() {
+        setTrustedHost("localhost");
+
+        ClientRepresentation clientRep = createRep("test-app");
+        // Create the client with a client registration disallowed protocolMapper
+        clientRep.setProtocolMappers(List.of(createHardcodedMapperRep()));
+        Response adminClientCreationResponse = realmResource().clients().create(clientRep);
+        String clientTechnicalId = ApiUtil.getCreatedId(adminClientCreationResponse);
+        adminClientCreationResponse.close();
+
+        ClientResource clientResource = realmResource().clients().get(clientTechnicalId);
+
+        reg.auth(Auth.token(clientResource.regenerateRegistrationAccessToken()));
+        // Check the client can be updated with a representation keeping the disallowed protocolMapper
+        ClientRepresentation representation = clientResource.toRepresentation();
+        representation.getProtocolMappers().forEach(mapper -> mapper.getConfig().put("foo", "bar"));
+        assertFail(ClientRegOp.UPDATE, representation, 403, "ProtocolMapper type not allowed");
+
+        // Revert client
+        ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
+    }
+
+    @Test
+    public void alteringAnAlreadyAssignedProtocolMappersIdWithDisallowedTypeFails() {
+        setTrustedHost("localhost");
+
+        ClientRepresentation clientRep = createRep("test-app");
+        // Create the client with a client registration disallowed protocolMapper
+        clientRep.setProtocolMappers(List.of(createHardcodedMapperRep()));
+        Response adminClientCreationResponse = realmResource().clients().create(clientRep);
+        String clientTechnicalId = ApiUtil.getCreatedId(adminClientCreationResponse);
+        adminClientCreationResponse.close();
+
+        ClientResource clientResource = realmResource().clients().get(clientTechnicalId);
+
+        reg.auth(Auth.token(clientResource.regenerateRegistrationAccessToken()));
+        ClientRepresentation representation = clientResource.toRepresentation();
+        representation.getProtocolMappers().forEach(mapper -> mapper.setId(UUID.randomUUID().toString()));
+        assertFail(ClientRegOp.UPDATE, representation, 403, "No existing mapper model found for id");
+
+        // Revert client
+        ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
+    }
 
     @Test
     public void testProtocolMappersConsentRequired() throws Exception {
@@ -622,7 +711,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe
 
         // Revert
         ApiUtil.findClientResourceByClientId(realmResource(), "test-app").remove();
-        protocolMapperPolicyRep.getConfig().remove(ProtocolMappersClientRegistrationPolicyFactory.ALLOWED_PROTOCOL_MAPPER_TYPES, HardcodedRole.PROVIDER_ID);
+        protocolMapperPolicyRep.getConfig().get(ProtocolMappersClientRegistrationPolicyFactory.ALLOWED_PROTOCOL_MAPPER_TYPES).remove(HardcodedRole.PROVIDER_ID);
         realmResource().components().component(protocolMapperPolicyRep.getId()).update(protocolMapperPolicyRep);
     }