GroupResource POST /children cannot update existing subgroups

[borshopCZ]

Before reporting an issue

• I have searched existing issues
• I have reproduced the issue with the latest release

Area

admin/api

Describe the bug

There was an undocumented change in Admin client API for GroupResource endpoint
POST /{realm}/groups/{id}/children
https://www.keycloak.org/docs-api/21.1.1/rest-api/index.html#_groups_resource

in version 21.1.x caused most likely by this commit (line 159):
cf61a65#diff-d233e82db465ac2b1ad22b124ab0bae3714f02d58c108c29afbbb935653d3ceaR159

Prior to this version, the behavior was "create or update a subgroup", now it throws HTTP status error 409 Conflict when the subgroup passed in the attribute already exists instead of updating it. This breaks existing implementations and I am not aware of any implementation replacing the previous function. I have not found anything neither in the migration guide or the API doc itself.

Is this a bug or intentional behavior? In case of the latter, is there any recommended way how to update the subgroup?

Version

21.1.1

Expected behavior

Calling the API will either ignore or better - update attributes of the existing subgroup when the subgroup with the given name and/or ID already exists

Actual behavior

API throws 409 Conflict

How to Reproduce?

GroupResource resource = client.realm("...").groups().group("...");
GroupRepresentation g = resource.toRepresentation();
GroupRepresentation sg = g.getSubGroups().get(0);
Response response = resource .subGroup(sg); // this throws 409

Anything else?

No response

[rmartinc]

Hi @borshopCZ!

I don't know if that commit produced this problem. That issue is trying to give a better error instead of returning a general error produced by the database or similar. Did your code worked for a previous keycloak version? What version exactly?

You can always update the child directly using the update over the child ID, something like:

        GroupRepresentation sg = g.getSubGroups().get(0);
        GroupResource resourceChild = realm.groups().group(sg.getId());
        resourceChild.update(sg);

Regards!

[borshopCZ]

Hi @borshopCZ!

I don't know if that commit produced this problem. That issue is trying to give a better error instead of returning a general error produced by the database or similar. Did your code worked for a previous keycloak version? What version exactly?

You can always update the child directly using the update over the child ID, something like:

        GroupRepresentation sg = g.getSubGroups().get(0);
        GroupResource resourceChild = realm.groups().group(sg.getId());
        resourceChild.update(sg);

Regards!

Hi, thanks for your input. It worked fine in version 20.0.5.
I think in the previous version, there was no check for existence of the subgroup, thus, it somehow worked fine. I do not say it was correct behaviour, but did not produce any errors. This is the contract change I mentioned. At least I would expect such change to be documented in the migration guide to let implementers know an intervention is required. But nothing is here https://www.keycloak.org/docs/latest/upgrading/index.html#migration-changes

We use this code generally for "installing the whole Keycloak data hierarchy", so it does not work exactly like my exmple code, this was just to demonstrate the behavior. In real we iterate over all groups and its subgroups recursively and update them. But you are correct it can be changed to the behavior you proposed and it should do the same job, so thank you for that.

[rmartinc]

Thanks for the version! I could reproduce it!

I think this is a minor regression, although what you are doing makes no sense. Now if a group is found with the same name the error is thrown but, in your case, you are moving the subgroup to the same parent group (so you are doing nothing and the error is not OK). But please, note that you are not modifying the subgroup here. You are just moving the group to the same parent, so you are not moving anything.

Nevertheless I will try to send a little PR to avoid the exception if you are sending exactly the same ID to be moved to the current parent.

[borshopCZ]

Hi @rmartinc

thanks for your reply! You are correct, we use it in a general code which iterates over the complete tree of groups and its subgroups through all realms, so it is used generally for "all possible scenarios":

1. subgroup does not exist - create + link it as subgroup
2. exists, but not in the tree where it should be - move it under the correct parent
3. exists in the correct location - do nothing (but now throws an exception)

Our code does a lot of other stuff as it is primarily used for unification of data among environments (kind of an installation pacakge). Thus, it needs to remain as generic as possible.

For now, we will adjust the code of the installation package so that we will not call the /children endpoint (for scenario 3 described above) and all should be set. But generally, as you mentioned, it would be beneficial if changes of this kind were notified explicitly to avoid troubles in production environments, like we experienced these days.