KEYCLOAK-19555 Improvements in ConsentRequiredExecutor of client policies

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 <a href="mailto:takashi.norimatsu.ws@hitachi.com">Takashi Norimatsu</a>
  */
-public class ConsentRequiredExecutor implements ClientPolicyExecutorProvider<ClientPolicyExecutorConfigurationRepresentation> {
+public class ConsentRequiredExecutor implements ClientPolicyExecutorProvider<ConsentRequiredExecutor.Configuration> {
+
+    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<Cli
         }
     }
 
+    @Override
+    public void setupConfiguration(ConsentRequiredExecutor.Configuration config) {
+        this.configuration = config;
+    }
+
+    @Override
+    public Class<ConsentRequiredExecutor.Configuration> 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) {
+    private void autoConfigure(ClientRepresentation proposedClient) throws ClientPolicyException {
-        clientModel.setConsentRequired(true);
+        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;
         }
-
+        // We are not updating consentRequired in the representation, but it is already set to true on the client
-        boolean isConsentRequired = clientToBeUpdated.isConsentRequired();
+        if (proposedClient.isConsentRequired() == null && clientToBeUpdated.isConsentRequired()) {
-        boolean newConsentRequired = proposedClient.isConsentRequired();
+            return;
-
-        if (isConsentRequired && !newConsentRequired) {
-            throw new ClientPolicyException(Errors.INVALID_REGISTRATION, "Not permitted to update consentRequired to false");
         }
+
+        validate(proposedClient);
     }
 
 }

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<ProviderConfigProperty> getConfigProperties() {
-        return Collections.emptyList();
+        return Collections.singletonList(AUTO_CONFIGURE_PROPERTY);
     }
 
 }

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",

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();
         }

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)

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<String> allowedClientAuthenticators, String defaultClientAuthenticator) {
         SecureClientAuthenticatorExecutor.Configuration config = new SecureClientAuthenticatorExecutor.Configuration();
         config.setAllowedClientAuthenticators(allowedClientAuthenticators);