diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java index a06178592c..9e771581cf 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java +++ b/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); + } } diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyRepresentation.java index 3674d8c5b7..c34e795920 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyRepresentation.java +++ b/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); + } } diff --git a/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties b/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties index eed0b61a01..7cdf677a82 100644 --- a/js/apps/admin-ui/maven-resources/theme/keycloak.v2/admin/messages/messages_en.properties +++ b/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 diff --git a/js/apps/admin-ui/src/realm-settings/PoliciesTab.tsx b/js/apps/admin-ui/src/realm-settings/PoliciesTab.tsx index a8ea343240..df02d009fc 100644 --- a/js/apps/admin-ui/src/realm-settings/PoliciesTab.tsx +++ b/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); } }; diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java b/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java index fc844cd82b..5dd947aedf 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java +++ b/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 proposedGlobalPolicies, List 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 allProfiles = new ArrayList<>(getClientProfilesRepresentation(session, realm).getProfiles()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/securityprofile/StrictSecurityProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/securityprofile/StrictSecurityProfileTest.java index 8432a43131..3b8e0d5e25 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/securityprofile/StrictSecurityProfileTest.java +++ b/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 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()); + } + } }