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

Import fails when creating composite realm roles when referenced roles don't exist on keycloak v 15.1.0+ #630

Closed Fynnyan closed 2 years ago

Fynnyan commented 2 years ago

Current Behavior

On Keycloak versions 15.1.1 and later the creation of new composite roles fails if the referenced roles do not exist yet.

On previous versions, we used 15.0.2, when creating a composite role, keycloak would fail silently and just create the base role.

That behavior of the management rest API for creating roles changed. with versions 15.1.1 and later the API returns an 404 if the referenced role doesn't exist.

Log statement from keycloak (v16.1.1) setup with the provided example data

2022-02-02 10:11:33.479 DEBUG 1 --- [           main] org.apache.http.wire                     : http-outgoing-0 >> "POST /auth/admin/realms/myrealm/roles HTTP/1.1[\r][\n]"
2022-02-02 10:11:33.480 DEBUG 1 --- [           main] org.apache.http.wire                     : http-outgoing-0 >> "{"id":null,"name":"vendor_user","description":"vendor","scopeParamRequired":null,"composite":true,"composites":{"realm":null,"client":{"fe":["vendor_user"],"be":["vendor_user"]},"application":nul
l},"clientRole":false,"containerId":null,"attributes":null}"
2022-02-02 10:11:33.496 DEBUG 1 --- [           main] org.apache.http.wire                     : http-outgoing-0 << "HTTP/1.1 404 Not Found[\r][\n]"
2022-02-02 10:11:33.497 DEBUG 1 --- [           main] org.apache.http.wire                     : http-outgoing-0 << "{"errorMessage":"Client Role with name vendor_user does not exist"}"

So when trying to import the realm into a keycloak instance of version 15.1.1+ fails and the cli-app terminates.

Expected Behavior

The import job for newer keycloak versions (15.1.0+) with the same JSON should work without failing

Steps To Reproduce

1. start a keycloak container of the affected version 
docker run -p 8080:8080 -e KEYCLOAK_USER=admin -e KEYCLOAK_PASSWORD=admin jboss/keycloak:16.1.1
2. start a cli container
docker run \
-e KEYCLOAK_URL=http://host.docker.internal:8080/auth \
-e KEYCLOAK_USER=admin \
-e KEYCLOAK_PASSWORD=admin \
-e KEYCLOAK_AVAILABILITYCHECK_ENABLED=true \
-e KEYCLOAK_AVAILABILITYCHECK_TIMEOUT=240s \
-e IMPORT_PATH=/config/realm.json \
-e IMPORT_FORCE=false \
-e LOGGING_LEVEL_KEYCLOAKCONFIGCLI=DEBUG \
-e LOGGING_LEVEL_HTTP=DEBUG \
-v $PWD/realm-issue.json:/config/realm.json \
adorsys/keycloak-config-cli:v4.6.1-16.1.0
3. wait until keycloak is up and the job runs

Zip with example JSON for reproduction: realm-issue.zip

Environment

Anything else?

The issue is currently affecting our development and I hacked together a fix / cli image that works for our use-case

The changes that I made can be found in my fork -> https://github.com/Fynnyan/keycloak-config-cli/commit/865711b8ab0046cb355055a1a8343e3547ceeb57 And the image can be pulled from docker hub -> https://hub.docker.com/repository/docker/fynnian/keycloak-config-cli

jkroepke commented 2 years ago

If you do already a fix on github, consider to create a PR including a test scenario?

Fynnyan commented 2 years ago

I wouldn't call it a proper fix as it only considers our realm setup and I would say it doesn't really solve the issue and may lead to problems for other people.

I need to see if I find time in the next days to work on the problem.

jkroepke commented 2 years ago

I wouldn't call it a proper fix as it only considers our realm setup and I would say it doesn't really solve the issue and may lead to problems for other people.

I may not 100% familiar with compsite groups. But we have also a lot of tests and a good test coverage. If there is a regression, I hope it will be detected by the test suite.

If you create a PR (even as WIP), then gh actions can validate the current approch.

ianwallen commented 2 years ago

I'm having a similar issue

The issue seems to be related to the order in which the roles are added.

Composite roles are roles that include other roles so if those other roles are not created first then there is a failure.

i.e. this will work because group 1 will be available when creating the composite role from group 2

"client": {
      "client1": [
        {
          "name": "group1",
          "composite": false,
          "clientRole": true,
        },
        {
          "name": "group2",
          "composite": true,
          "composites": {
            "client": {
              "client1": [
                "group1"
              ]
            }
          },

This will fail because when creating group2, it will not be able to locate group1 because it has not been created yet.

"client": {
      "client1": [
        {
          "name": "group2",
          "composite": true,
          "composites": {
            "client": {
              "client1": [
                "group1"
              ]
            }
          },
        {
          "name": "group1",
          "composite": false,
          "clientRole": true,
        },

Of course this is simple example and they can get more more complicated with groups nested several levels deep and also containing a mixture of client roles and realm roles.

I can see a couple or options for this fix but there are probably others. 1 - load the roles recursively ensuring that dependencies are created first in the correct order. This may not be that easy since there are realm roles and client roles in the mix.

2 - create roles with a 2 pass system. First create all roles without the composite group so that all the groups are created. Then on the second pass, all the composite groups are added (they would exists at that point)

jkroepke commented 2 years ago

@ianwallen Can you provide working complex sample? Such examples are a good base for test cases, since I'm start to develop feature by creating tests... :-)

The example that you provide may not even valid, since clients is an array and I do not have much experience in composite roles.

jkroepke commented 2 years ago

I added a fix on #631

@Fynnyan if you could test that PR, it would be awesome.

The fix now if to exclude composite attribute first and handle all composites attributes later. It based on your fix, but I re-use the same method as I used on other places.

ianwallen commented 2 years ago

@jkroepke

@ianwallen Can you provide working complex sample? Such examples are a good base for test cases, since I'm start to develop feature by creating tests... :-)

The example that you provide may not even valid, since clients is an array and I do not have much experience in composite roles.

I created a commit based off the nested-composites branch for #631

The commit contains another unit test where it add a composite client role before the related client roles are created.

It seems like the fix for #631 worked as the unit test worked as expected.

I'm not sure if you want to include that change in your PR or not?

jkroepke commented 2 years ago

Thanks, I pick you commit.

Fynnyan commented 2 years ago

I added a fix on #631

@Fynnyan if you could test that PR, it would be awesome.

The fix now if to exclude composite attribute first and handle all composites attributes later. It based on your fix, but I re-use the same method as I used on other places.

The fix you made works. I tested it with the given test data and also our projects realm configuration. The issue is resolved.

@jkroepke Thank you for your work.