Updating client policies in JSON editor is buggy. Attempt to update global client policies should throw the error

closes #30102

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

core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.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 ClientPolicyConditionRepresentation {
     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;
+        ClientPolicyConditionRepresentation that = (ClientPolicyConditionRepresentation) o;
+        return Objects.equals(conditionProviderId, that.conditionProviderId) && Objects.equals(configuration, that.configuration);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(conditionProviderId, configuration);
+    }
 }

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

@@ -18,6 +18,7 @@
 package org.keycloak.representations.idm;
 
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Client Policy's external representation class
@@ -72,4 +73,16 @@ public class ClientPolicyRepresentation {
         this.profiles = profiles;
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        ClientPolicyRepresentation that = (ClientPolicyRepresentation) o;
+        return Objects.equals(name, that.name) && Objects.equals(description, that.description) && Objects.equals(enabled, that.enabled) && Objects.equals(conditions, that.conditions) && Objects.equals(profiles, that.profiles);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(name, description, enabled, conditions, profiles);
+    }
 }

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

@@ -1139,6 +1139,7 @@ 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.
+invalidJsonClientPoliciesError=Unable to save client policies, 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
@@ -1987,7 +1988,7 @@ allowRegexComparisonHelp=If OFF, then the Subject DN from given client certifica
 eventTypes.UPDATE_TOTP_ERROR.description=Update totp error
 titleEvents=Events
 signServiceProviderMetadata=Sign service provider metadata
-updateClientPoliciesError=Provided JSON is incorrect\: Unexpected token { in JSON
+updateClientPoliciesError=Could not update client policies\: {{error}}
 acceptsPromptNoneHelp=This is just used together with Identity Provider Authenticator or when kc_idp_hint points to this identity provider. In case that client sends a request with prompt\=none and user is not yet authenticated, the error will not be directly returned to client, but the request with prompt\=none will be forwarded to this identity provider.
 roleDetails=Role details
 eventTypes.USER_INFO_REQUEST.name=User info request

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

@@ -13,6 +13,7 @@ import {
   Title,
   ToolbarItem,
 } from "@patternfly/react-core";
+import { omit } from "lodash-es";
 import { useState } from "react";
 import { Controller, useForm, type UseFormReturn } from "react-hook-form";
 import { useTranslation } from "react-i18next";
@@ -113,6 +114,9 @@ export const PoliciesTab = () => {
     }
   };
 
+  const normalizePolicy = (policy: ClientPolicy): ClientPolicyRepresentation =>
+    omit(policy, "global");
+
   const save = async () => {
     if (!code) {
       return;
@@ -121,9 +125,18 @@ export const PoliciesTab = () => {
     try {
       const obj: ClientPolicy[] = JSON.parse(code);
 
+      const changedPolicies = obj
+        .filter((policy) => !policy.global)
+        .map((policy) => normalizePolicy(policy));
+
+      const changedGlobalPolicies = obj
+        .filter((policy) => policy.global)
+        .map((policy) => normalizePolicy(policy));
+
       try {
         await adminClient.clientPolicies.updatePolicy({
-          policies: obj,
+          policies: changedPolicies,
+          globalPolicies: changedGlobalPolicies,
         });
         addAlert(t("updateClientPoliciesSuccess"), AlertVariant.success);
         refresh();
@@ -132,7 +145,7 @@ export const PoliciesTab = () => {
       }
     } catch (error) {
       console.warn("Invalid json, ignoring value using {}");
-      addError("updateClientPoliciesError", error);
+      addError("invalidJsonClientPoliciesError", error);
     }
   };
 

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

@@ -331,6 +331,12 @@ public class ClientPoliciesUtil {
         return !proposedGlobalProfiles.equals(origGlobalProfiles);
     }
 
+    private static boolean isGlobalPoliciesUpdated(List<ClientPolicyRepresentation> proposedGlobalPolicies, List<ClientPolicyRepresentation> origGlobalPolicies) {
+        // if globalPolicies were not sent, we can skip this
+        if (proposedGlobalPolicies == null || proposedGlobalPolicies.isEmpty()) return false;
+        return !proposedGlobalPolicies.equals(origGlobalPolicies);
+    }
+
     /**
      * check whether the proposed executor's provider can be found in keycloak's ClientPolicyExecutorProvider list.
      * not return null.
@@ -564,6 +570,11 @@ public class ClientPoliciesUtil {
             throw new ClientPolicyException("realm not specified.");
         }
 
+        // validate global profiles not changed
+        if (isGlobalPoliciesUpdated(proposedPoliciesRep.getGlobalPolicies(), existingGlobalPolicies)) {
+            throw new ClientPolicyException("Global policies cannot be updated");
+        }
+
         ClientPoliciesRepresentation updatingPoliciesRep = new ClientPoliciesRepresentation();
 
         List<ClientProfileRepresentation> allProfiles = new ArrayList<>(getClientProfilesRepresentation(session, realm).getProfiles());

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/securityprofile/StrictSecurityProfileTest.java

@@ -19,6 +19,7 @@ package org.keycloak.testsuite.securityprofile;
 import jakarta.ws.rs.BadRequestException;
 import jakarta.ws.rs.core.Response;
 import java.util.Collections;
+import java.util.List;
 import java.util.stream.Collectors;
 import org.hamcrest.MatcherAssert;
 import org.hamcrest.Matchers;
@@ -35,12 +36,16 @@ import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.ErrorRepresentation;
 import org.keycloak.representations.idm.OAuth2ErrorRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
+import org.keycloak.services.clientpolicy.ClientPolicyException;
 import org.keycloak.testsuite.AbstractTestRealmKeycloakTest;
 import org.keycloak.testsuite.admin.ApiUtil;
 import org.keycloak.testsuite.arquillian.annotation.SetDefaultProvider;
 import org.keycloak.testsuite.auth.page.login.OIDCLogin;
 import org.keycloak.testsuite.util.ClientBuilder;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
 @SetDefaultProvider(spi = "security-profile", providerId = "default", config = {"name", "strict-security-profile"})
 public class StrictSecurityProfileTest extends AbstractTestRealmKeycloakTest {
 
@@ -68,6 +73,50 @@ public class StrictSecurityProfileTest extends AbstractTestRealmKeycloakTest {
         MatcherAssert.assertThat(error.getErrorMessage(), Matchers.containsString("duplicated as a global policy"));
     }
 
+    @Test
+    public void testUpdatingGlobalPoliciesNotAllowed() throws Exception {
+        ClientPoliciesRepresentation clientPoliciesRep = getClientPolicies();
+        List<ClientPolicyRepresentation> origGlobalPolicies = clientPoliciesRep.getGlobalPolicies();
+
+        // Attempt to update description of some global policy. Should fail
+        clientPoliciesRep = getClientPolicies();
+        clientPoliciesRep.getGlobalPolicies().stream()
+                .filter(clientPolicy -> "Saml secure client (signatures, post, https)".equals(clientPolicy.getName()))
+                .forEach(clientPolicy -> clientPolicy.setDescription("some new description"));
+        try {
+            updatePolicies(clientPoliciesRep);
+            fail();
+        } catch (ClientPolicyException cpe) {
+            assertEquals("update policies failed", cpe.getError());
+        }
+
+        // Attempt to add new global policy. Should fail
+        clientPoliciesRep = getClientPolicies();
+        ClientPolicyRepresentation newPolicy = new ClientPolicyRepresentation();
+        newPolicy.setName("new-name");
+        newPolicy.setDescription("desc");
+        clientPoliciesRep.getGlobalPolicies().add(newPolicy);
+        try {
+            updatePolicies(clientPoliciesRep);
+            fail();
+        } catch (ClientPolicyException cpe) {
+            assertEquals("update policies failed", cpe.getError());
+        }
+
+        // Attempt to update without global policies. Should be OK
+        clientPoliciesRep = getClientPolicies();
+        clientPoliciesRep.setGlobalPolicies(null);
+        updatePolicies(clientPoliciesRep);
+
+        // Attempt to update with global policies, but not change them. Should be OK
+        clientPoliciesRep = getClientPolicies();
+        updatePolicies(clientPoliciesRep);
+
+        // Doublecheck global policies were not changed
+        clientPoliciesRep = getClientPolicies();
+        org.keycloak.testsuite.Assert.assertEquals(origGlobalPolicies, clientPoliciesRep.getGlobalPolicies());
+    }
+
     @Test
     public void testCreatePublicOpenIdConnectClientSecure() {
         RealmResource realm = testRealm();
@@ -197,4 +246,15 @@ public class StrictSecurityProfileTest extends AbstractTestRealmKeycloakTest {
         Assert.assertEquals(Response.Status.CREATED.getStatusCode(), resp.getStatus());
         getCleanup().addClientUuid(ApiUtil.getCreatedId(resp));
     }
+
+    private ClientPoliciesRepresentation getClientPolicies() {
+        return adminClient.realm(TEST_REALM_NAME).clientPoliciesPoliciesResource().getPolicies(true);
+    }
+    protected void updatePolicies(ClientPoliciesRepresentation rep) throws ClientPolicyException {
+        try {
+            adminClient.realm(TEST_REALM_NAME).clientPoliciesPoliciesResource().updatePolicies(rep);
+        } catch (BadRequestException e) {
+            throw new ClientPolicyException("update policies failed", e.getResponse().getStatusInfo().toString());
+        }
+    }
 }