marcospereirampj / python-keycloak

MIT License
704 stars 297 forks source link

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

Closed NikolaiES closed 4 months ago

NikolaiES commented 6 months ago

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.

robson90 commented 6 months ago

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

ryshoooo commented 5 months ago

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?

NikolaiES commented 4 months ago

@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!