WIP: add support for attaching realm and client roles to users and groups

[mattock]

This PR adds support for attaching realm - and later client - roles to users and groups.

It modifies the kcadm method to accept a hash with arbitrary keys and values so that we can run things like "kcadm-wrapper.sh add-roles -r myrealm --uusername john --rolename foo --rolename bar".

If this approach looks generally ok to you I will add unit and acceptance tests. Any feedback on the Ruby style is also most welcome.

I'm thinking of making the client role attachments a separate type, e.g. keycloak_client_role_mapping. For example:

keycloak_client_role_attachment { 'service-account-myclient':
  realm => 'myrealm',
  client_id => 'myclient',
  user => 'service-account-myclient',
  roles => ['myclient-role-a', 'myclient-role-b'],
}

Alternatively I could handle all client roles for a user/group in one place, by passing in a hash like

roles => {
  'myclient' => ['myclient-role-a', 'myclient-role-b'],
  'otherclient' => ['otherclient-role-a', 'otherclient-role-b'], }            

Thoughts?

[treydock]

Why not use the API to make these changes? https://www.keycloak.org/docs-api/11.0/rest-api/index.html

Ideally just use REST API to make the changes if API endpoints exist.

[treydock]

If the ability to do this via API exists and this is assigning a role to a client I think would make sense to support this:

keycloak_client { '...':
  ...
  roles => [...],

There are a few provides in this module where they get data from numerous API calls, I think clients or client protocol mappers are ones like that. The benefit to that is it becomes easier to ensure stray roles don't get assigned by checking the list of desired roles against the list of actually configured roles.

[mattock]

I initially tried to use the REST API. I was able to figure out the correct REST API incantations and payloads with Keycloak Admin UI + Chrome Devtools.

When I tried to replicate the same calls with kcadm it refused to co-operate. Basically it claimed that the endpoint, for example /{realm}/groups/{id}/role-mappings/realm, does not support POST requests. The GET endpoints did work with kcadm, but that alone was not very useful. Anyways, this may be the reason why there are dedicated "add-roles", "get-roles", etc. commands in kcadm.

I can double-check this though, just in case, and provide you with kcadm command-lines you can try out yourself if you want.

[treydock]

I think to add client role to group and user:

POST /{realm}/groups/{id}/role-mappings/clients/{client}
POST /{realm}/users/{id}/role-mappings/clients/{client}

https://www.keycloak.org/docs-api/11.0/rest-api/index.html#_client_role_mappings_resource

I think the realm role: https://www.keycloak.org/docs-api/11.0/rest-api/index.html#_role_mapper_resource

Were those the API calls that were not working?

[mattock]

Were those the API calls that were not working?

I will double-check.

[mattock]

@treydock you were correct. I'm not sure why my initial tests failed. To add or delete realm role mapping for a user

/opt/keycloak/bin/kcadm-wrapper.sh create users/5bc5a439-9571-4199-890f-2fdb4c5f4a32/role-mappings/realm -r foobar -f testrole.json
/opt/keycloak/bin/kcadm-wrapper.sh delete users/5bc5a439-9571-4199-890f-2fdb4c5f4a32/role-mappings/realm -r foobar -f testrole.json

Where the playload is

[{"id":"3ee9f7e4-b80b-44b2-9331-d211d5726a35","name":"testrole","description":"Test role","composite":false,"clientRole":false,"containerId":"foobar"}]

I will restructure the provider to use the API.

[mattock]

Works with client roles as well:

/opt/keycloak/bin/kcadm-wrapper.sh create users/5bc5a439-9571-4199-890f-2fdb4c5f4a32/role-mappings/clients/e906db94-5344-4437-8091-881a6f8f886d -r foobar -f testclient-role-john.json

where the payload is

[{"id":"73e292f3-61c4-40ef-b53c-9ea046930312","name":"testclient-role","composite":false,"clientRole":true,"containerId":"e906db94-5344-4437-8091-881a6f8f886d"}]

[mattock]

I did a bit more testing and the current implementation seems to be the most user-friendly and peformant approach after all.

The challenge with the API approach is that it requires passing role's "id" and "name" to work. Moreover, the URL needs to have the "id" of the user being modified. The "add-roles" command is able to do its work without the "id" for either of these.

So, either the user of this type/provider is forced to define the "id" for both user and role, or the provider has to dynamically figure them out, which results in two extra API calls (sample code below):

[root@localhost vagrant]# time ./realm-roles.json.rb 
Logging into http://localhost:8080/auth as user admin of realm master

Logging into http://localhost:8080/auth as user admin of realm master

Logging into http://localhost:8080/auth as user admin of realm master

real    0m4.132s
user    0m3.376s
sys     0m0.400s

When the equivalent changes are made using kcadm add-roles it is much faster:

[root@localhost vagrant]# time /opt/keycloak/bin/kcadm-wrapper.sh add-roles -r foobar --uusername john --rolename testrole --rolename anotherrole
Logging into http://localhost:8080/auth as user admin of realm master

real    0m1.415s
user    0m1.081s
sys     0m0.135s

The crude but functional Ruby PoC here:

#!/opt/puppetlabs/puppet/bin/ruby
require 'json'
require 'tempfile'

realm = "foobar"
username = "john"
desired_roles = ["testrole", "anotherrole"]
action = "create"
#action = "delete"

# Get realm roles. These are used to map role names to role ids
output = `/opt/keycloak/bin/kcadm-wrapper.sh get roles -r #{realm}` 
begin
  role_data = JSON.parse(output)
rescue JSON::ParserError
  Puppet.debug('Unable to parse output from kcadm get roles')
end

# Create indexed version of returned role data to reduce looping
# when modifying multiple roles at the same time
roles = {}
role_data.each { |role| roles[role['name']] = role }

# Get realm users. These are used to map user names to user ids.
output = `/opt/keycloak/bin/kcadm-wrapper.sh get users -r #{realm}` 
begin
  user_data = JSON.parse(output)
rescue JSON::ParserError
  Puppet.debug('Unable to parse output from kcadm get roles')
end

users = {}
user_data.each { |user| users[user['username']] = user }

#p roles
#p users

# Ensure that the desired roles are present in the realm
desired_roles.each do |desired_role|
  unless roles.has_key?(desired_role)
    puts "ERROR: role #{desired_role} does not exist in the realm!"  
    exit 1
  end
end

data = []
desired_roles.each do |desired_role|
  data << { id: roles[desired_role]['id'],
            composite: false,
            clientRole: false,
            name: desired_role,
            containerId: realm }
end

#p data
t = Tempfile.new('keycloak_role_mapping')
t.write(JSON.pretty_generate(data))
t.close

user_id = users[username]['id']

`/opt/keycloak/bin/kcadm-wrapper.sh #{action} users/#{user_id}/role-mappings/realm -r #{realm} -f #{t.path}`

Any opinion on this @treydock?

[treydock]

The main benefit with using the pure API approach is you should in theory be able to query all existing role assignments (prefetch) and then add or modify what's needed. That's currently how all the types work for this module, they have expensive code at the beginning that essentially dumps all resources and then evaluates those in-memory data structures to see what should be added/removed/modified. This is always going to be more costly on one or two resources but it scales better. If you have dozens of resources defined and then only way to query them is individual "get" of each resource, that begins to slow things down much more than if you do one massive dump and compare data structures.

[mattock]

Yeah, indeed. If I skip extra validation steps like "is this client role really available to be attached to a user" and just let kcadm barf if it is not, then the number of API calls could be kept reasonable, even for client roles. Anyhow, I'll move forward with the API-based approach with prefetch and all. It would definitely be nice to be able to gather all the data in one go and reuse it across provider instances.

[treydock]

I've not yet found a good way to re-used prefetched data across different resource types, like a keycloak_client can't use prefetched data that might be identically queried by keycloak_client_scope. I am sure there might be ways with Ruby to put the code into lib/puppet/provider/keycloak_api.rb and then do something like this:

def get_something(args)
  return @something if defined?(@something)

  @something = do_query(args)
  return @something
end

I have used this pattern in Rails applications for caching data so expensive queries are only performed once but not sure if will work with prefetch since my usage of such patterns has usually been with instance methods and not class methods though I think the same pattern might work for a class method.

[mattock]

I created a separate branch for developing the API-based provider. I've implemented self.instances and self.prefetch so far. Current implementation's performance won't be great because many following API calls will be necessary:

• get realms
  • get all users
    • get role mappings for user a
    • get role mappings for user b
    • get role mappings for user c
    • --- snip ---
  • get all groups
    • get role mappings for group a
    • get role mappings for group b
    • get role mappings for group c
    • --- snip ---
  • do one or two API calls per changed/missing role mapping to fix the divergences

Maybe the best option would be that self.prefetch would check the Puppet::Type::Keycloak_role_mapping instances passed to it via the "resources" parameter and only check the state of those instances. If no state was found then assume the resource is not present. This way looping through users and groups and their respective role mappings could be avoided. Self.instances could be kept as-is (=get all data, even if it takes times).

My new work is here, please ignore the horrific commit history. Or better yet, don't even look at it 😄

[treydock]

I would recommend opening a new pull request if you want to do something in parallel to this pull request, or just replace this pull request with that branch using instances/prefetch. This will make it easier for me to provide comments inline.

Just a quick look at the code and I think it might be better to create 2 types/provides. keycloak_user_role_mapping and keycloak_group_role_mapping, as it looked like you could be setting role mappings for users or groups. This will improve performance because the prefetch for each is only run if a resource of that type is defined, so if someone only defines keycloak_group_role_mapping they won't have to deal with prefetch penalty from keycloak_user_role_mapping.

I would not worry too much about performance. The Keycloak API is not structured in a way to lend itself to high performance automation. At some point I'd like to circle back and find ways for people to opt-out of the slow operations they may not need.

[treydock]

I did some testing and may have changed my mind on how to best handle this. I have 4853 groups in my Keycloak instance, our LDAP has a lot of groups, and the first query of all groups took a while, like 20-30 seconds, but subsequent queries have been less than 1 second. Iterating over all those groups would take forever though since the prefetch would end up making one query per group which is extremely expensive. So given that I think this approach, not using the prefetch/instances might be best unless you want to use prefetch/instances and somehow query the catalog for resource names that are defined and only loop over those group/user names, which is pretty much what this pull request's approach would do.

[treydock]

Also sorry if you spent a lot of time on the prefetch/instances just to switch back to this pull request. I should have tested the prefetch approach on my environment as I have rather massive LDAP deployment.

[mattock]

@treydock no worries. This gave me the opportunity to really learn self.instances and self.prefetch. I had not needed them previously. I do like the simplicity of the original implementation. I'll look a bit more into separating self.prefetch from self.instances plus implementing self.flush, then make some decision.

[mattock]

I spent a fair amount of time to optimize the self.prefetch implementation and decided to give up. Technically it is possible to only query the role mappings that are present in the catalog, but code complexity gets pretty high for little gain if one wants to minimize the amount of API calls:

• Which realms are present in the catalog? We don't want to "get users" for those that are not.
• Which users-realm and group-realm pairs are in the catalog? We don't want to run multiple "get users/<id>/role-mappings" if that query has already been made earlier (e.g. same user has multiple mappings in catalog). Deduplicate or filter in advance...

So in a nutshell the return on investment was poor.

I'll meld self.instances from that abandoned approach here, clean up and write tests. Then this should be good to go.

[mattock]

Please ignore the latest changes. I pushed completely WIP work to this branch, forgetting I had created a PR about it 😄.

[jverrydt]

We were also planning to implement this functionality so is it possible to give me a status update on this pull request?

[mattock]

@jverrydt I did some code archaeology and here is the original implementation that worked well afaics:

• https://github.com/Puppet-Finland/puppet-module-keycloak/commits/service_account_user_roles

It needs rspec tests and all that, of course. The only thing in this PR that is possibly worth saving is the self.instances implementation.

@jverrydt if you want to finish the work by all means do. I've found it really difficult to find enough dedicated working time to wrap this thing up. If you decide to do that let me know - I can do code review and testing.

[treydock]

Closing in favor of #233