adorsys / keycloak-config-cli

Import YAML/JSON-formatted configuration files into Keycloak - Configuration as Code for Keycloak.
Apache License 2.0
714 stars 134 forks source link

Mismatch between doc and code for import.remove-default-role-from-user #658

Closed maschmi closed 2 years ago

maschmi commented 2 years ago

Hi,

looking in the documentation it is stated:

import.remove-default-role-from-user
Keycloak 13 attach a default role named default-role-$REALM that contains some defaults from any user. Previously keycloak-config-cli remove that default role, if the role not defined inside the import json. The flag prevents keycloak-config-cli from exclude default-roles-$REALM from removal logic. This results that it's not longer possible to explicit remove the role from a user, if import.remove-default-role-from-user set to true.

The variable is defaulted to false. According to the documentation I would assume the behaviour did not change and the default-role would be removed when it is not given in the json import.

However, in code it looks like this:

private void handleRolesToBeRemoved(List<String> usersRealmLevelRolesToUpdate, List<String> existingUsersRealmLevelRoles) {
            List<String> rolesToDelete = searchForMissing(existingUsersRealmLevelRoles, usersRealmLevelRolesToUpdate);
            if (!importConfigProperties.isRemoveDefaultRoleFromUser()) {
                rolesToDelete.remove("default-roles-" + realmName.toLowerCase());
            }

            if (rolesToDelete.isEmpty()) return;

So the default-roles-$realmName is removed from the list of roles to delete when the config variable is false.

In my understanding this somehow mismatches. Maybe I just understood the documentation incrorrectly. In both cases I would like to suggest to adjust the documentation to be clearer as this is not changing existing behaviour. I'm also happy to supply a pull-request for this documentation part if you like.

jkroepke commented 2 years ago

Hi @maschmi

by re-reading the parameter name, i would expect:

By reading the documentation,

This results that it's not longer possible to explicit remove the role from a user, if import.remove-default-role-from-user set to true.

This isn't true, it should be

Keycloak 13 attach a default role named default-role-$REALM that contains some defaults from any user. Previously keycloak-config-cli remove that default role, if the role not defined inside the import json. The flag prevents keycloak-config-cli from exclude default-roles-$REALM from removal logic. This results that it's not longer possible to explicit remove the role from a user, unless import.remove-default-role-from-user set to true.

Does it make sense on your side?

maschmi commented 2 years ago

Hi @jkroepke ,

yes. This looks good.

Reading the parameter I had the same feeling, so I checked the code as it was not working as expected.

If I may suggest an alternative wording:

Keycloak 13 attach a default role named default-role-$REALM that contains some defaults from any user. Previously keycloak-config-cli remove that default role, if the role not defined inside the import json. The default setting of this flag prevents keycloak-config-cli from removing default-roles-$REALM, even if its not defined in the import json. To make keycloak-config-cli able to remove the default-roles-$REALM, import.remove-default-role-from-user must be set to true.

But the word "unless" would also be enough.

Thank you for the quick response!

jkroepke commented 2 years ago

Hi,

I used you alternative wording and append a disclaimer. Take a look, if its fine #659