python-keycloak icon indicating copy to clipboard operation
python-keycloak copied to clipboard

get_groups only collects the sugroups of top level groups and only max 10 subgroups

Open NikolaiES opened this issue 1 year ago • 1 comments

In the newest version 3.8.4 the logic to collect subGroups only collects subGroups from the decedents of the groups at root level. So, if you have a hierarchy of several layers then the get_groups call will only collect the subgroups of the groups at root level.

In the following example you can see that sub0 has subGroupCount 2 but no subGroups.

[
    {
        "id": "3c3ade77-6e51-421d-a0e1-66f51703b58c",
        "name": "root0",
        "path": "/root0",
        "subGroupCount": 1,
        "subGroups": [
            {
                "id": "50735510-04e7-4aef-a63e-3d2a59d128a9",
                "name": "sub0",
                "path": "/root0/sub0",
                "parentId": "3c3ade77-6e51-421d-a0e1-66f51703b58c",
                "subGroupCount": 2,
                "subGroups": [],
                "attributes": {},
                "realmRoles": [],
                "clientRoles": {},
                "access": {
                    "view": true,
                    "viewMembers": true,
                    "manageMembers": true,
                    "manage": true,
                    "manageMembership": true
                }
            }
        ],
        "access": {
            "view": true,
            "viewMembers": true,
            "manageMembers": true,
            "manage": true,
            "manageMembership": true
        }
    }
]

Another related issue is that the call to the children endpoint in get_group_children user raw_get instead of __fetch_all. The children endpoint is paginated with a default page size of 10. The documentation for the children endpoint says null but the code says 10. So, in the example above if root0 had 15 subGroups only the first 10 would be included. This was tested against the newest version of keycloak 23.0.6 with a simple get_groups() call no with no arguments.

NikolaiES avatar Feb 15 '24 12:02 NikolaiES

duplicate Have a look here: https://github.com/marcospereirampj/python-keycloak/issues/509

robson90 avatar Feb 29 '24 09:02 robson90

The get_group_children ought to be fixed with https://github.com/marcospereirampj/python-keycloak/pull/534.

I think it's questionable whether we should go down the path of querying all the subgroups recursively. There's a reason Keycloak made these endpoints paginated and single-level, especially when you deal with a lot of groups. It is a nice feature to just get everything, but maybe it shouldn't be the default behavior when calling get_groups without any parameters. The same can be said about the get_users I suppose.

I'd be more prone to create new methods like get_all_groups and get_all_users and keep get_groups and get_users paginated by default. The get_all_groups could have an additional argument like full_hierarchy: bool, which would recursively traverse through the subgroups and fetch their subgroups etc. It could I guess be clumped into single method, maybe something like def get_groups(all: bool = False, traverse: bool = False, query=None).

Would a solution like this suffice? What are your thoughts?

ryshoooo avatar Apr 07 '24 12:04 ryshoooo

@ryshoooo Hello. Sorry for not replying to this earlier I completed missed this message. I fully agree that get_groups default behavior should not be to fetch all groups. The changes you conducted in #556 looks good and exactly what I needed. Thank you!

NikolaiES avatar Apr 27 '24 19:04 NikolaiES