From 3889eeda301057a1492052499289b9832ffe73a4 Mon Sep 17 00:00:00 2001 From: Takashi Norimatsu Date: Sun, 5 Jun 2022 08:41:28 +0900 Subject: [PATCH] Client Policies: pkce-enforcer executor with client-access-type condition is not applied on client change via Admin API Closes #12295 --- .../condition/ClientAccessTypeCondition.java | 29 ++++++++- .../ClientSecretRotationExecutor.java | 35 ++++++----- .../testsuite/client/ClientPoliciesTest.java | 60 +++++++++++++++++++ 3 files changed, 106 insertions(+), 18 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAccessTypeCondition.java b/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAccessTypeCondition.java index a600e2f140..c4e175b9f9 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAccessTypeCondition.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAccessTypeCondition.java @@ -25,9 +25,11 @@ import org.jboss.logging.Logger; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.representations.idm.ClientPolicyConditionConfigurationRepresentation; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.services.clientpolicy.ClientPolicyContext; import org.keycloak.services.clientpolicy.ClientPolicyException; import org.keycloak.services.clientpolicy.ClientPolicyVote; +import org.keycloak.services.clientpolicy.context.ClientCRUDContext; /** * @author Takashi Norimatsu @@ -74,10 +76,14 @@ public class ClientAccessTypeCondition extends AbstractClientPolicyConditionProv case TOKEN_INTROSPECT: case USERINFO_REQUEST: case LOGOUT_REQUEST: + case UPDATE: case UPDATED: case REGISTERED: if (isClientAccessTypeMatched()) return ClientPolicyVote.YES; return ClientPolicyVote.NO; + case REGISTER: + if (isProposedClientAccessTypeMatched((ClientCRUDContext)context)) return ClientPolicyVote.YES; + return ClientPolicyVote.NO; default: return ClientPolicyVote.ABSTAIN; } @@ -86,14 +92,31 @@ public class ClientAccessTypeCondition extends AbstractClientPolicyConditionProv private String getClientAccessType() { ClientModel client = session.getContext().getClient(); if (client == null) return null; + return getClientAccessType(client.isPublicClient(), client.isBearerOnly()); + } - if (client.isPublicClient()) return ClientAccessTypeConditionFactory.TYPE_PUBLIC; - if (client.isBearerOnly()) return ClientAccessTypeConditionFactory.TYPE_BEARERONLY; + private String getProposedClientAccessType(ClientCRUDContext context) { + ClientRepresentation clientRep = context.getProposedClientRepresentation(); + if (clientRep == null) return null; + return getClientAccessType(Optional.ofNullable(clientRep.isPublicClient()).orElse(Boolean.FALSE).booleanValue(), + Optional.ofNullable(clientRep.isBearerOnly()).orElse(Boolean.FALSE).booleanValue()); + } + + private String getClientAccessType(boolean isPublicClient, boolean isBearerOnly) { + if (isPublicClient) return ClientAccessTypeConditionFactory.TYPE_PUBLIC; + if (isBearerOnly) return ClientAccessTypeConditionFactory.TYPE_BEARERONLY; else return ClientAccessTypeConditionFactory.TYPE_CONFIDENTIAL; } private boolean isClientAccessTypeMatched() { - final String accessType = getClientAccessType(); + return isClientAccessTypeMatched(getClientAccessType()); + } + + private boolean isProposedClientAccessTypeMatched(ClientCRUDContext context) { + return isClientAccessTypeMatched(getProposedClientAccessType(context)); + } + + private boolean isClientAccessTypeMatched(String accessType) { if (accessType == null) return false; List expectedAccessTypes = Optional.ofNullable(configuration.getType()).orElse(Collections.emptyList()); diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java index 7f39bfdc6e..bfde92be0d 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/executor/ClientSecretRotationExecutor.java @@ -49,23 +49,23 @@ public class ClientSecretRotationExecutor implements @Override public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { - if (!session.getContext().getClient().isPublicClient() && !session.getContext().getClient() - .isBearerOnly()) { - session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED, Boolean.TRUE); - switch (context.getEvent()) { - case REGISTERED: - case UPDATED: + switch (context.getEvent()) { + case REGISTERED: + case UPDATED: + if(isClientWithSecret(session.getContext().getClient())) { + session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED, Boolean.TRUE); executeOnClientCreateOrUpdate((ClientCRUDContext) context); - break; - - case AUTHORIZATION_REQUEST: - case TOKEN_REQUEST: + } + break; + case AUTHORIZATION_REQUEST: + case TOKEN_REQUEST: + if(isClientWithSecret(session.getContext().getClient())) { + session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED, Boolean.TRUE); executeOnAuthRequest(); - return; - - default: - return; - } + } + break; + default: + return; } } @@ -80,6 +80,11 @@ public class ClientSecretRotationExecutor implements } + private boolean isClientWithSecret(ClientModel client) { + if (client == null) return false; + return (!client.isPublicClient() && !client.isBearerOnly()); + } + private void executeOnAuthRequest() { ClientModel client = session.getContext().getClient(); OIDCClientSecretConfigWrapper wrapper = OIDCClientSecretConfigWrapper.fromClientModel( 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 be51d8bd04..ec805624bd 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 @@ -1053,6 +1053,66 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { successfulLoginAndLogout(clientBetaId, null); failLoginWithoutNonce(clientAlphaId); + + // update profiles + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "El Primer Perfil") + .addExecutor(PKCEEnforcerExecutorFactory.PROVIDER_ID, + createPKCEEnforceExecutorConfig(Boolean.FALSE)) // check only + .toRepresentation() + ).toString(); + updateProfiles(json); + + // Attempt to create a confidential client without PKCE setting. Should fail + try { + createClientByAdmin(generateSuffixedName("Gamma-App"), (ClientRepresentation clientRep) -> { + clientRep.setSecret("secretGamma"); + clientRep.setBearerOnly(Boolean.FALSE); + clientRep.setPublicClient(Boolean.FALSE); + }); + fail(); + } catch (ClientPolicyException e) { + assertEquals(OAuthErrorException.INVALID_CLIENT_METADATA, e.getMessage()); + assertEquals("Invalid client metadata: code_challenge_method", e.getErrorDetail()); + } + + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "El Primer Perfil") + .addExecutor(PKCEEnforcerExecutorFactory.PROVIDER_ID, + createPKCEEnforceExecutorConfig(Boolean.TRUE)) // enforce + .toRepresentation() + ).toString(); + updateProfiles(json); + + authCreateClients(); + String clientGammaId = createClientDynamically(generateSuffixedName("Gamma-App"), (OIDCClientRepresentation clientRep) -> { + clientRep.setClientSecret("secretGamma"); + }); + + ClientRepresentation clientRep = getClientByAdmin(clientGammaId); + assertEquals(OAuth2Constants.PKCE_METHOD_S256, OIDCAdvancedConfigWrapper.fromClientRepresentation(clientRep).getPkceCodeChallengeMethod()); + + json = (new ClientProfilesBuilder()).addProfile( + (new ClientProfileBuilder()).createProfile(PROFILE_NAME, "El Primer Perfil") + .addExecutor(PKCEEnforcerExecutorFactory.PROVIDER_ID, + createPKCEEnforceExecutorConfig(Boolean.FALSE)) // check only + .toRepresentation() + ).toString(); + updateProfiles(json); + + // Attempt to update the confidential client with not allowed PKCE setting. Should fail + try { + updateClientByAdmin(clientGammaId, (ClientRepresentation updatingClientRep) -> { + updatingClientRep.setAttributes(new HashMap<>()); + updatingClientRep.getAttributes().put(OIDCConfigAttributes.PKCE_CODE_CHALLENGE_METHOD, OAuth2Constants.PKCE_METHOD_PLAIN); + }); + } catch (ClientPolicyException e) { + assertEquals(OAuthErrorException.INVALID_CLIENT_METADATA, e.getMessage()); + assertEquals("Invalid client metadata: code_challenge_method", e.getErrorDetail()); + } + ClientRepresentation cRep = getClientByAdmin(clientGammaId); + assertEquals(OAuth2Constants.PKCE_METHOD_S256, cRep.getAttributes().get(OIDCConfigAttributes.PKCE_CODE_CHALLENGE_METHOD)); + } @Test