diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutor.java index c3858d69fa..8ced7060b0 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutor.java @@ -17,6 +17,7 @@ package org.keycloak.services.clientpolicy.executor; +import com.fasterxml.jackson.annotation.JsonProperty; import org.keycloak.events.Errors; import org.keycloak.models.ClientModel; import org.keycloak.representations.idm.ClientPolicyExecutorConfigurationRepresentation; @@ -28,18 +29,22 @@ import org.keycloak.services.clientpolicy.context.ClientCRUDContext; /** * @author Takashi Norimatsu */ -public class ConsentRequiredExecutor implements ClientPolicyExecutorProvider { +public class ConsentRequiredExecutor implements ClientPolicyExecutorProvider { + + private ConsentRequiredExecutor.Configuration configuration; @Override public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { ClientCRUDContext clientUpdateContext = null; switch (context.getEvent()) { - case REGISTERED: + case REGISTER: clientUpdateContext = (ClientCRUDContext)context; - afterRegister(clientUpdateContext.getTargetClient()); + autoConfigure(clientUpdateContext.getProposedClientRepresentation()); + validate(clientUpdateContext.getProposedClientRepresentation()); break; case UPDATE: clientUpdateContext = (ClientCRUDContext)context; + autoConfigure(clientUpdateContext.getProposedClientRepresentation()); beforeUpdate(clientUpdateContext.getTargetClient(), clientUpdateContext.getProposedClientRepresentation()); break; default: @@ -47,29 +52,56 @@ public class ConsentRequiredExecutor implements ClientPolicyExecutorProvider getExecutorConfigurationClass() { + return ConsentRequiredExecutor.Configuration.class; + } + + public static class Configuration extends ClientPolicyExecutorConfigurationRepresentation { + @JsonProperty("auto-configure") + protected Boolean autoConfigure; + + public Boolean isAutoConfigure() { + return autoConfigure; + } + + public void setAutoConfigure(Boolean autoConfigure) { + this.autoConfigure = autoConfigure; + } + } + @Override public String getProviderId() { return ConsentRequiredExecutorFactory.PROVIDER_ID; } - private void afterRegister(ClientModel clientModel) { - clientModel.setConsentRequired(true); + private void autoConfigure(ClientRepresentation proposedClient) throws ClientPolicyException { + if (configuration.isAutoConfigure()) { + proposedClient.setConsentRequired(true); + } + } + + private void validate(ClientRepresentation proposedClient) throws ClientPolicyException { + if (proposedClient.isConsentRequired() == null || !proposedClient.isConsentRequired()) { + throw new ClientPolicyException(Errors.INVALID_REGISTRATION, "Client is required to enable consentRequired"); + } } public void beforeUpdate(ClientModel clientToBeUpdated, ClientRepresentation proposedClient) throws ClientPolicyException { - if (proposedClient.isConsentRequired() == null) { - return; - } if (clientToBeUpdated == null) { return; } - - boolean isConsentRequired = clientToBeUpdated.isConsentRequired(); - boolean newConsentRequired = proposedClient.isConsentRequired(); - - if (isConsentRequired && !newConsentRequired) { - throw new ClientPolicyException(Errors.INVALID_REGISTRATION, "Not permitted to update consentRequired to false"); + // We are not updating consentRequired in the representation, but it is already set to true on the client + if (proposedClient.isConsentRequired() == null && clientToBeUpdated.isConsentRequired()) { + return; } + + validate(proposedClient); } } diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutorFactory.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutorFactory.java index dd620cc6ee..06ea256025 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutorFactory.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ConsentRequiredExecutorFactory.java @@ -32,6 +32,12 @@ public class ConsentRequiredExecutorFactory implements ClientPolicyExecutorProvi public static final String PROVIDER_ID = "consent-required"; + public static final String AUTO_CONFIGURE = "auto-configure"; + + private static final ProviderConfigProperty AUTO_CONFIGURE_PROPERTY = new ProviderConfigProperty( + AUTO_CONFIGURE, "Auto-configure", "If On, the configuration of the client will be auto-configured to enable consentRequired during client creation or update." + + "If Off, the clients are validated to have consentRequired enabled during create/update client", ProviderConfigProperty.BOOLEAN_TYPE, true); + @Override public ClientPolicyExecutorProvider create(KeycloakSession session) { return new ConsentRequiredExecutor(); @@ -56,12 +62,12 @@ public class ConsentRequiredExecutorFactory implements ClientPolicyExecutorProvi @Override public String getHelpText() { - return "When present, then newly registered client will always have 'consentRequired' switch enabled"; + return "When present, then registered/updated clients will be verified to have 'consentRequired' switch enabled and eventually will be auto-configured for 'consentRequired' switch to be enabled"; } @Override public List getConfigProperties() { - return Collections.emptyList(); + return Collections.singletonList(AUTO_CONFIGURE_PROPERTY); } } diff --git a/services/src/main/resources/keycloak-default-client-profiles.json b/services/src/main/resources/keycloak-default-client-profiles.json index fc70a00f3c..058674fe56 100644 --- a/services/src/main/resources/keycloak-default-client-profiles.json +++ b/services/src/main/resources/keycloak-default-client-profiles.json @@ -31,7 +31,9 @@ }, { "executor": "consent-required", - "configuration": {} + "configuration": { + "auto-configure": true + } }, { "executor": "full-scope-disabled", @@ -95,7 +97,9 @@ }, { "executor": "consent-required", - "configuration": {} + "configuration": { + "auto-configure": true + } }, { "executor": "full-scope-disabled", diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java index 0c303ec538..bbaa37ca83 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java @@ -138,6 +138,7 @@ import static org.keycloak.testsuite.util.ClientPoliciesUtil.createClientUpdateC import static org.keycloak.testsuite.util.ClientPoliciesUtil.createClientUpdateSourceGroupsConditionConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createClientUpdateSourceHostsConditionConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createClientUpdateSourceRolesConditionConfig; +import static org.keycloak.testsuite.util.ClientPoliciesUtil.createConsentRequiredExecutorConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createHolderOfKeyEnforceExecutorConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createPKCEEnforceExecutorConfig; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createSecureClientAuthenticatorExecutorConfig; @@ -2379,7 +2380,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { // register profiles String json = (new ClientProfilesBuilder()).addProfile( (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Test Profile") - .addExecutor(ConsentRequiredExecutorFactory.PROVIDER_ID, null) + .addExecutor(ConsentRequiredExecutorFactory.PROVIDER_ID, createConsentRequiredExecutorConfig(true)) .toRepresentation() ).toString(); updateProfiles(json); @@ -2394,6 +2395,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { ).toString(); updatePolicies(json); + // Client will be auto-configured to enable consentRequired String clientId = generateSuffixedName("aaa-app"); String cid = createClientByAdmin(clientId, (ClientRepresentation clientRep) -> { clientRep.setImplicitFlowEnabled(Boolean.FALSE); @@ -2402,6 +2404,32 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { ClientRepresentation clientRep = getClientByAdmin(cid); assertEquals(Boolean.TRUE, clientRep.isConsentRequired()); + // Client cannot be updated to disable consentRequired + updateClientByAdmin(cid, (ClientRepresentation cRep) -> { + cRep.setConsentRequired(Boolean.FALSE); + }); + clientRep = getClientByAdmin(cid); + assertEquals(Boolean.TRUE, clientRep.isConsentRequired()); + + // Switch auto-configure to false. Auto-configuration won't happen, but validation will still be here, so should not be possible to disable consentRequired + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "Test Profile") + .addExecutor(ConsentRequiredExecutorFactory.PROVIDER_ID, createConsentRequiredExecutorConfig(false)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // Not possible to register client with consentRequired due the validation + try { + createClientByAdmin(clientId, (ClientRepresentation clientRep2) -> { + clientRep2.setConsentRequired(Boolean.FALSE); + }); + fail(); + } catch (ClientPolicyException cpe) { + assertEquals(Errors.INVALID_REGISTRATION, cpe.getError()); + } + + // Not possible to update existing client to consentRequired due the validation try { updateClientByAdmin(cid, (ClientRepresentation cRep) -> { cRep.setConsentRequired(Boolean.FALSE); @@ -2419,6 +2447,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { }); clientRep = getClientByAdmin(cid); assertEquals(Boolean.TRUE, clientRep.isImplicitFlowEnabled()); + assertEquals(Boolean.TRUE, clientRep.isConsentRequired()); } catch (ClientPolicyException cpe) { fail(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java index 95a1c94ea0..a56c2641ac 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/FAPI1Test.java @@ -60,7 +60,7 @@ import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.representations.oidc.OIDCClientRepresentation; import org.keycloak.services.clientpolicy.ClientPolicyException; import org.keycloak.services.clientpolicy.condition.AnyClientConditionFactory; -import org.keycloak.services.clientpolicy.executor.SecureRequestObjectExecutor; +import org.keycloak.services.clientpolicy.condition.ClientUpdaterContextConditionFactory; import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; @@ -93,6 +93,7 @@ import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; import static org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPoliciesBuilder; import static org.keycloak.testsuite.util.ClientPoliciesUtil.ClientPolicyBuilder; import static org.keycloak.testsuite.util.ClientPoliciesUtil.createAnyClientConditionConfig; +import static org.keycloak.testsuite.util.ClientPoliciesUtil.createClientUpdateContextConditionConfig; /** * Test for the FAPI 1 specifications: @@ -198,6 +199,24 @@ public class FAPI1Test extends AbstractClientPoliciesTest { } + // KEYCLOAK-19555 + @Test + public void testFAPIBaselineSecureSettingsWhenUseAdminPolicy() throws Exception { + // Apply policy for admin REST API and Dynamic Client Registration requests + setupPolicyFAPIBaselineForAdminRESTAndDynamicClientRegistrationRequests(); + + // Try to register client with default authenticator - should pass. Client authenticator should be "client-jwt" + String clientUUID = createClientByAdmin("client-jwt-3", (ClientRepresentation clientRep) -> { + }); + ClientRepresentation client = getClientByAdmin(clientUUID); + Assert.assertEquals(JWTClientAuthenticator.PROVIDER_ID, client.getClientAuthenticatorType()); + + // Check the Consent is enabled, PKCS set to S256 + Assert.assertTrue(client.isConsentRequired()); + Assert.assertEquals(OAuth2Constants.PKCE_METHOD_S256, OIDCAdvancedConfigWrapper.fromClientRepresentation(client).getPkceCodeChallengeMethod()); + } + + @Test public void testFAPIBaselineOIDCClientRegistration() throws Exception { setupPolicyFAPIBaselineForAllClient(); @@ -697,6 +716,20 @@ public class FAPI1Test extends AbstractClientPoliciesTest { updatePolicies(json); } + private void setupPolicyFAPIBaselineForAdminRESTAndDynamicClientRegistrationRequests() throws Exception { + String json = (new ClientPoliciesBuilder()).addPolicy( + (new ClientPolicyBuilder()).createPolicy(POLICY_NAME, "MyClientUpdaterContextPolicy", Boolean.TRUE) + .addCondition(ClientUpdaterContextConditionFactory.PROVIDER_ID, + createClientUpdateContextConditionConfig(Arrays.asList( + ClientUpdaterContextConditionFactory.BY_AUTHENTICATED_USER, + ClientUpdaterContextConditionFactory.BY_INITIAL_ACCESS_TOKEN, + ClientUpdaterContextConditionFactory.BY_REGISTRATION_ACCESS_TOKEN))) + .addProfile(FAPI1_BASELINE_PROFILE_NAME) + .toRepresentation() + ).toString(); + updatePolicies(json); + } + private void setupPolicyFAPIAdvancedForAllClient() throws Exception { String json = (new ClientPoliciesBuilder()).addPolicy( (new ClientPolicyBuilder()).createPolicy("MyPolicy", "Policy for enable FAPI Advanced for all clients", Boolean.TRUE) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java index d3939780d6..96629c2364 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java @@ -37,6 +37,7 @@ import org.keycloak.services.clientpolicy.condition.ClientUpdaterContextConditio import org.keycloak.services.clientpolicy.condition.ClientUpdaterSourceGroupsCondition; import org.keycloak.services.clientpolicy.condition.ClientUpdaterSourceHostsCondition; import org.keycloak.services.clientpolicy.condition.ClientUpdaterSourceRolesCondition; +import org.keycloak.services.clientpolicy.executor.ConsentRequiredExecutor; import org.keycloak.services.clientpolicy.executor.FullScopeDisabledExecutor; import org.keycloak.services.clientpolicy.executor.HolderOfKeyEnforcerExecutor; import org.keycloak.services.clientpolicy.executor.PKCEEnforcerExecutor; @@ -160,6 +161,12 @@ public final class ClientPoliciesUtil { return config; } + public static ConsentRequiredExecutor.Configuration createConsentRequiredExecutorConfig(Boolean autoConfigure) { + ConsentRequiredExecutor.Configuration config = new ConsentRequiredExecutor.Configuration(); + config.setAutoConfigure(autoConfigure); + return config; + } + public static SecureClientAuthenticatorExecutor.Configuration createSecureClientAuthenticatorExecutorConfig(List allowedClientAuthenticators, String defaultClientAuthenticator) { SecureClientAuthenticatorExecutor.Configuration config = new SecureClientAuthenticatorExecutor.Configuration(); config.setAllowedClientAuthenticators(allowedClientAuthenticators);