Validate client scopes registration policy configuration #40254
Conversation
|
@graziang Thanks! Question: Do we have something similar for "client policies" as well? When someone uploads JSON with the client-policy-executor referencing not existing client scope, do we also prevent this? Original bug report was for "client registration policies", but thinking it can be good to doublecheck "client policies" as well (unless we are oing already?) |
|
@mposolda thanks! Added in a separate commit a validation system for the configuration of client policy conditions. So far, I’ve implemented validation only for the client-scopes condition. This might cause issues when updating existing client policies that are misconfigured with invalid scopes. |
There was a problem hiding this comment.
@graziang The idea looks good to me, Thanks! But there are test failures, which look as a regression of the change.
IMO when there is fake-scope configured in the list of client-scopes condition, the create/update should fail. But if the configured list is empty or null, it should probably work? Could it be the reason of the test failure or is it something different?
| public void validateConfiguration(KeycloakSession session, RealmModel realm, ClientPolicyConditionRepresentation conditionRepresentation) throws ClientPolicyException { | ||
| ClientScopesCondition.Configuration configuration = JsonSerialization.mapper.convertValue(conditionRepresentation.getConfiguration(), ClientScopesCondition.Configuration.class); | ||
|
|
||
| if (!realm.getClientScopesStream().map(ClientScopeModel::getName).toList().containsAll(configuration.getScopes())) { |
There was a problem hiding this comment.
Will this work if configuration.getScopes() is null or empty list? I guess it doesn't work for null. Not 100% sure if this could be a reason of the test failures in the current PR (but maybe it is some completely cause...)
|
@mposolda Thanks for the review. The tests were failing because a client role was being used as a client 8000 scope, so the new validation was actually doing the right thing. I created a sample client scope to fix it. I also added the null check and included a test. Everything is in a separate commit to simplify the review. |
Closes keycloak#40187 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#40187 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes keycloak#40187 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Closes #40187
This PR just implements
ClientScopesClientRegistrationPolicyFactory.validateConfiguration()to enable configuration validation for client registration policies. For client policies, looking at the code, it seems we don’t have any kind of configuration validation, It might be worth defining something similar to what we have for client registration policies as a follow-up.