From 907469638245316d2462e2fa27a9e3179dfb8769 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 31 May 2024 19:28:17 +0200 Subject: [PATCH] Editing built-in client policy profiles are silently reverted closes #27184 Signed-off-by: mposolda --- .../ClientPolicyExecutorRepresentation.java | 15 +++++ .../idm/ClientProfileRepresentation.java | 14 +++++ .../admin/messages/messages_en.properties | 3 +- .../src/realm-settings/ProfilesTab.tsx | 2 +- .../clientpolicy/ClientPoliciesUtil.java | 11 ++++ .../ClientPoliciesLoadUpdateTest.java | 56 +++++++++++++++++-- 6 files changed, 93 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java index c0215bf498..a0c2a3fb4f 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java +++ b/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); + } } diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientProfileRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientProfileRepresentation.java index 12b05c3af4..ac95e01c48 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientProfileRepresentation.java +++ b/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 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); + } } 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 dba923fede..922d4d7e11 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 @@ -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 diff --git a/js/apps/admin-ui/src/realm-settings/ProfilesTab.tsx b/js/apps/admin-ui/src/realm-settings/ProfilesTab.tsx index f5f9df2085..bf83cf63eb 100644 --- a/js/apps/admin-ui/src/realm-settings/ProfilesTab.tsx +++ b/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); } }; 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 499a5eb8d6..fc844cd82b 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java +++ b/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 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 proposedGlobalProfiles, List 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. diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesLoadUpdateTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesLoadUpdateTest.java index 7aaabeed8b..c84a8a7f1c 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesLoadUpdateTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesLoadUpdateTest.java @@ -60,7 +60,7 @@ import static org.keycloak.testsuite.util.ClientPoliciesUtil.createSecureClientA /** * This test class is for testing loading and updating profiles and policies file of client policies. - * + * * @author Takashi Norimatsu */ public class ClientPoliciesLoadUpdateTest extends AbstractClientPoliciesTest { @@ -175,7 +175,7 @@ public class ClientPoliciesLoadUpdateTest extends AbstractClientPoliciesTest { null)) .addExecutor(PKCEEnforcerExecutorFactory.PROVIDER_ID, createPKCEEnforceExecutorConfig(Boolean.FALSE)) - .addExecutor("no-such-executor", + .addExecutor("no-such-executor", createPKCEEnforceExecutorConfig(Boolean.TRUE)) .toRepresentation(); @@ -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 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()); @@ -306,22 +350,22 @@ public class ClientPoliciesLoadUpdateTest extends AbstractClientPoliciesTest { String beforeUpdatePoliciesJson = ClientPoliciesUtil.convertClientPoliciesRepresentationToJson(getPolicies()); // load policies - ClientPolicyRepresentation duplicatedPoliciesRep = + ClientPolicyRepresentation duplicatedPoliciesRep = (new ClientPolicyBuilder()).createPolicy( "builtin-duplicated-new-policy", "builtin duplicated new policy is ignored.", Boolean.TRUE) - .addCondition(ClientRolesConditionFactory.PROVIDER_ID, + .addCondition(ClientRolesConditionFactory.PROVIDER_ID, createClientRolesConditionConfig(Arrays.asList(SAMPLE_CLIENT_ROLE))) .addProfile(FAPI1_BASELINE_PROFILE_NAME) .toRepresentation(); - ClientPolicyRepresentation loadedPolicyRep = + ClientPolicyRepresentation loadedPolicyRep = (new ClientPolicyBuilder()).createPolicy( "new-policy", "duplicated profiles are ignored.", Boolean.TRUE) - .addCondition(ClientAccessTypeConditionFactory.PROVIDER_ID, + .addCondition(ClientAccessTypeConditionFactory.PROVIDER_ID, createClientAccessTypeConditionConfig(Arrays.asList(ClientAccessTypeConditionFactory.TYPE_PUBLIC, ClientAccessTypeConditionFactory.TYPE_BEARERONLY))) .addProfile("lack-of-builtin-field-test-profile") .addProfile("ordinal-test-profile")