Editing built-in client policy profiles are silently reverted

closes #27184

Signed-off-by: mposolda <mposolda@gmail.com>

core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java

@@ -18,6 +18,8 @@
 
 package org.keycloak.representations.idm;
 
+import java.util.Objects;
+
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.JsonNode;
 
@@ -47,4 +49,17 @@ public class ClientPolicyExecutorRepresentation {
     public void setConfiguration(JsonNode configuration) {
         this.configuration = configuration;
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        ClientPolicyExecutorRepresentation that = (ClientPolicyExecutorRepresentation) o;
+        return Objects.equals(executorProviderId, that.executorProviderId) && Objects.equals(configuration, that.configuration);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(executorProviderId, configuration);
+    }
 }

core/src/main/java/org/keycloak/representations/idm/ClientProfileRepresentation.java

@@ -18,6 +18,7 @@
 package org.keycloak.representations.idm;
 
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Client Profile's external representation class
@@ -53,4 +54,17 @@ public class ClientProfileRepresentation {
     public void setExecutors(List<ClientPolicyExecutorRepresentation> executors) {
         this.executors = executors;
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        ClientProfileRepresentation that = (ClientProfileRepresentation) o;
+        return Objects.equals(name, that.name) && Objects.equals(description, that.description) && Objects.equals(executors, that.executors);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(name, description, executors);
+    }
 }

js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties

@@ -562,7 +562,7 @@ oAuthMutual=OAuth 2.0 Mutual TLS Certificate Bound Access Tokens Enabled
 keystore=Keystore
 eventTypes.EXECUTE_ACTION_TOKEN.description=Execute action token
 eventTypes.CLIENT_INFO.description=Client info
-updateClientProfilesError=Provided JSON is incorrect\: Unexpected token { in JSON
+updateClientProfilesError=Could not update client profiles\: {{error}}
 canonicalizationHelp=Canonicalization Method for XML signatures.
 authorizationHelp=Enable/Disable fine-grained authorization support for a client
 sessions=Sessions
@@ -1138,6 +1138,7 @@ requestObjectEncryptionHelp=JWE algorithm, which client needs to use when sendin
 importSuccess=New certificate imported
 attributeConsumingServiceName=Attribute Consuming Service Name
 invalidJsonError=Unable to save user profile, the provided information is not valid JSON.
+invalidJsonClientProfilesError=Unable to save client profiles, the provided information is not valid JSON.
 promptHelp=Specifies whether the Authorization Server prompts the End-User for re-authentication and consent.
 deleteBtn=Delete
 defaultLocale=Default locale

js/apps/admin-ui/src/realm-settings/ProfilesTab.tsx

@@ -159,7 +159,7 @@ export default function ProfilesTab() {
         addError("updateClientProfilesError", error);
       }
     } catch (error) {
-      console.warn("Invalid json, ignoring value using {}");
+      addError("invalidJsonClientProfilesError", error);
     }
   };
 

services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java

@@ -294,6 +294,11 @@ public class ClientPoliciesUtil {
             throw new ClientPolicyException("proposed client profile name duplicated.");
         }
 
+        // validate global profiles not changed
+        if (isGlobalProfilesUpdated(proposedProfilesRep.getGlobalProfiles(), globalClientProfiles)) {
+            throw new ClientPolicyException("Global profiles cannot be updated");
+        }
+
         // Conflict with any global profile is not allowed
         Set<String> globalProfileNames = globalClientProfiles.stream().map(ClientProfileRepresentation::getName).collect(Collectors.toSet());
         for (ClientProfileRepresentation clientProfile : proposedProfileRepList) {
@@ -320,6 +325,12 @@ public class ClientPoliciesUtil {
         return proposedProfilesRep;
     }
 
+    private static boolean isGlobalProfilesUpdated(List<ClientProfileRepresentation> proposedGlobalProfiles, List<ClientProfileRepresentation> origGlobalProfiles) {
+        // if globalProfiles were not sent, we can skip this
+        if (proposedGlobalProfiles == null || proposedGlobalProfiles.isEmpty()) return false;
+        return !proposedGlobalProfiles.equals(origGlobalProfiles);
+    }
+
     /**
      * check whether the proposed executor's provider can be found in keycloak's ClientPolicyExecutorProvider list.
      * not return null.

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

@@ -221,6 +221,50 @@ public class ClientPoliciesLoadUpdateTest extends AbstractClientPoliciesTest {
         }
     }
 
+    @Test
+    public void testUpdatingGlobalProfilesNotAllowed() throws Exception {
+        ClientProfilesRepresentation clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        List<ClientProfileRepresentation> origGlobalProfiles = clientProfilesRep.getGlobalProfiles();
+
+        // Attempt to update description of some global profile. Should fail
+        clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        clientProfilesRep.getGlobalProfiles().stream()
+                .filter(clientProfile -> FAPI1_BASELINE_PROFILE_NAME.equals(clientProfile.getName()))
+                .forEach(clientProfile -> clientProfile.setDescription("some new description"));
+        try {
+            updateProfiles(clientProfilesRep);
+            fail();
+        } catch (ClientPolicyException cpe) {
+            assertEquals("update profiles failed", cpe.getError());
+        }
+
+        // Attempt to add new global profile. Should fail
+        clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        ClientProfileRepresentation newProfile = new ClientProfileRepresentation();
+        newProfile.setName("new-name");
+        newProfile.setDescription("desc");
+        clientProfilesRep.getGlobalProfiles().add(newProfile);
+        try {
+            updateProfiles(clientProfilesRep);
+            fail();
+        } catch (ClientPolicyException cpe) {
+            assertEquals("update profiles failed", cpe.getError());
+        }
+
+        // Attempt to update without global profiles. Should be OK
+        clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        clientProfilesRep.setGlobalProfiles(null);
+        updateProfiles(clientProfilesRep);
+
+        // Attempt to update with global profiles, but not change them. Should be OK
+        clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        updateProfiles(clientProfilesRep);
+
+        // Doublecheck global profiles were not changed
+        clientProfilesRep = adminClient.realm(REALM_NAME).clientPoliciesProfilesResource().getProfiles(true);
+        Assert.assertEquals(origGlobalProfiles, clientProfilesRep.getGlobalProfiles());
+    }
+
     @Test
     public void testNullProfiles() throws Exception {
         String beforeUpdateProfilesJson = ClientPoliciesUtil.convertClientProfilesRepresentationToJson(getProfilesWithGlobals());