RealmAdminResource.getGroupByPathGroup does not work with space in path parameter

[thomasdarimont]

Before reporting an issue

• I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

admin/api

Describe the bug

I just found a regression in org.keycloak.services.resources.admin.RealmAdminResource#getGroupByPathGroup.
Path parameter values that contain a space are not decoded properly. Instead of "My Group" a group name like "My%20Group" is used for a search by name which obviously yields no results.

Version

23.0.0

Expected behavior

Search for a group by group path with spaces should work.

Actual behavior

Search for a group by group path with spaces always yields not found.

How to Reproduce?

1. Create a realm
2. Create top level group with spaces in group name, e.g. "My Group"
3. try to search for group by path

See: bin/kcadm.sh get realms/realmWithGroups/group-by-path/My%20Group

Anything else?

This is a regression and worked in Keycloak 22.0.5.

[transducer]

Same issue for us with RealmAdminResource.getGroupByPath. Applies to use of the Java client and calling the REST API directly.

Reproduction scenario

docker run -p 8081:8080 \
  -e KEYCLOAK_ADMIN=admin \
  -e KEYCLOAK_ADMIN_PASSWORD=admin \
  quay.io/keycloak/keycloak:23.0.3 start-dev

Add a group with name "NAME" and a group with name "NAME WITH SPACE" in UI and a confidential client:

Add a confidential client to call the API with later:

Then call the API for both the group with and without spaces:

LOGIN_HOST=http://localhost:8081
REALM=master
CLIENT_ID=master
CLIENT_SECRET=NMrh4ibfUL2shHvevicHeKnCrCI3w66j
USERNAME=admin
PASSWORD=admin

accessToken=$(
  curl -s -X POST "${LOGIN_HOST}/realms/${REALM}/protocol/openid-connect/token" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    -d "client_id=${CLIENT_ID}" \
    -d "client_secret=${CLIENT_SECRET}" \
    -d "username=${USERNAME}" \
    -d "password=${PASSWORD}" \
    -d "grant_type=password" | jq --raw-output '.access_token'
)

curl -s "${LOGIN_HOST}/admin/realms/${REALM}/group-by-path/NAME" \
  -H "Authorization: Bearer ${accessToken}"

curl -s "${LOGIN_HOST}/admin/realms/${REALM}/group-by-path/NAME%20WITH%20SPACE" \
  -H "Authorization: Bearer ${accessToken}"

Returns respectively

{"id":"eed307b6-5eb7-4dd8-bd17-b3fe375e2e67","name":"NAME","path":"/NAME"}
{"error":"Group path does not exist"}`

When using quay.io/keycloak/keycloak:22.0.5 instead of 23.0.1, the last call does return the group.

[transducer]

Probably related to https://www.keycloak.org/docs/latest/release_notes/index.html#group-scalability in PR #22700.

There's no unit test at the moment that tests for paths with spaces.

[transducer]

@alice-wondered in PR Group scalability upgrades (#22700) code related to groups was changed. Could this issue be related to that PR?

[rmartinc]

@alice-wondered @transducer I think this issue is triggered by quarkus/resteasy-eactive change. My feeling is that the reason is this PR quarkusio/quarkus#34809. That change is not Ok, because it's not decoding the path params when using a custom regexp (like in our case). Probably we need to open an issue in quarkus although maybe we can also do some workaround. I'm investigating it.

Yep! I found the issue: quarkusio/quarkus#35960

[rmartinc]

I have just sent a PR to quarkus: quarkusio/quarkus#37513. Don't know how it will go and we need a backport to 3.2.x.
There is also a workaround, we can use a list of PathSegment which is decoded like in this branch: https://github.com/rmartinc/keycloak/tree/issue-25111-workaround (it can be used after the fix in quarkus).

@thomasdarimont @transducer WDYT? Do we wait a bit or do I send a PR with the workaround?

[transducer]

@rmartinc great work, thank you. I see your PR quarkusio/quarkus#37513 is already merged and someone mentions backporting. If the issue is backported in Quarkus is it then also solved in the Keycloak release? Then maybe best to wait a bit since it's the most clean solution, without the need for any temporary workaround code.

From the perspective of my company it would be great if we get a fix for this. We are still running on 21.1.2 and there are fixes since that version we are eagerly awaiting, but we have not been able to update due to regressions that affected us, of which this issue seems to be the last (we are depending on calls to getGroupByPath in a user and group management application). But we can surely wait a bit longer (and are happy we can use this open source software :)).

[rmartinc]

@transducer I have sent the PR just in case. And yes, if the quarkus fix is backported to 3.2.x, upgrading the quarkus dependency is enough. If that happens before 23.0.2 I'll change the PR to just include the test.

[transducer]

@rmartinc this is not fixed in 23.0.3. The reproduction scenario specified above using 23.0.3 still returns "Group path does not exist".

Since the fix is available in the code, can this issue be reopened or shall I open a new one?

[rmartinc]

@transducer The backport for 23.0 is still not merged, so it was not included in 23.0.3. You will see it in 23.0.x when you see this PR merged: #25471

[transducer]

@rmartinc aha, thanks for the clarification!