Add support for keycloak and option to disable group-of-groups

[BlackVoid]

Hi

Want to start of with thanking you for creating this project, stumbled upon this after digging into this kind of service for the thousand time and finally finding a project that makes it easy to integrate OIDC with LDAP for the services which do not support OIDC.

I've made a few changes in order to fit my use case and if you want to I can split it up into multiple PRs or if you don't feel like merging certain changes I can remove them.

The following changes have been made:

• I've added support for a Keycloak backend which also handled the UID & GID, so it does not use the uid cache backends. Was considering using it but in the end skipping the use of the cache simplified the code.
• In order to support different parameters to the backends I inspect the arguments and only include the once discovered.
• I also noticed the groups-of-groups functionality which I personally don't have a use-case for or understand the purpose for. So I made a flag to disable it.
• Unset values (None) were being included as the value "None" instead of the field being excluded from the output, so I made a check to exclude None values.
• Adds the optional field "mail" which is an part of "inetOrgPerson"

[seang96]

Cool PR! I had to do some changes to make it work and noted them in my review. I had the intent to do this today and luckily found that you made this PR 3 hours prior!

Thanks for the contribution!

[BlackVoid]

Hi @seang96

I did not see any comments on the PR, maybe you forgot to submit the comments?
FYI I did some changes after making the PR like changing the naming of environment variable and arguments to better fit their use and be aligned with the other backend.

[seang96]

I did a review, perhaps the maintainer needs to approve them or something?

[seang96]

Ah woops I did forget to submit the review. Don't do many of those sorry about that haha

[seang96]

I just noticed my review was still pending after submitting it.. let's see if it works now.

[BlackVoid]

Did some quick changes to fix the mentioned issues, will test it more later this week.

[jemrobinson]

Hi @BlackVoid. Thanks a lot for this - it looks really nice

I've added support for a Keycloak backend which also handled the UID & GID, so it does not use the uid cache backends. Was considering using it but in the end skipping the use of the cache simplified the code.

Yes - there's no need to use the UID cache if Keycloak is already providing the UIDs

In order to support different parameters to the backends I inspect the arguments and only include the once discovered.

This is a neat trick. Don't think I've seen that before.

I also noticed the groups-of-groups functionality which I personally don't have a use-case for or understand the purpose for. So I made a flag to disable it.

It's a bit opaque, but the purpose of this is to allow you to make an LDAP request that looks like "all groups that members of group X belong to". For example if you have

users:
- X
- Y
- Z
groups:
- A: [X, Y]
- B: [Y, Z]
- C: [Z]

then you'll get

users: X, Y, Z
groups: A, B, C
primary_user_groups: X, Y, Z
groups_of_groups: A, B, C

where groups_of_groups A contains primary_user_groups X and Y. If you make an LDAP request that looks like: "(&(objectClass=posixGroup)(|(CN=A)(memberOf=Primary user groups for A)))" then you'll get the set of groups: A, X, Y which are the set of groups you need for users X and Y to be able to log in to e.g. a Linux machine.

This is to work around a problem with Microsoft Entra which doesn't have primary user groups natively. I assume Keycloak does? If so, I'll take a look at moving the group-of-groups logic into MicrosoftEntraClient or somewhere else less global.

Unset values (None) were being included as the value "None" instead of the field being excluded from the output, so I made a check to exclude None values.

I could imagine this might cause the attribute validation to fail. If it doesn't then I don't see a problem with this.

Adds the optional field "mail" which is an part of "inetOrgPerson"

Sounds fine - I haven't included all the optional fields for the various classes, but there's no reason not to have this.

I don't have time for a full review now, but I'll take a look maybe next week or the week after?

[jemrobinson]

@BlackVoid are you happy to allow edits from maintainers on your fork?

[BlackVoid]

Sure, I've enabled it now :)

[BlackVoid]

Keycloak does not have primary user groups either, so I think this is just that our use cases are different, I understand your explanation right. So I think it's fine to have it be available for all backends in case someone has the same usecase but uses Keycloak or perhaps in the future some other provider.

My usecase is for the most part to integrate with systems that use ldap like Proxmox and various self hosting solutions that do not support OIDC like calibre web and homeassistant. So I only use regular groups for access management and don't really have a need for the primary user groups.

[BlackVoid]

Just a note, I'm not really in a hurry to get this or the other PR merged. It's changes I've made to use it for my own purpose and just thought it would be nice to eventually get it in to the project so others can have a use for it. However I might not have the coming weeks to promptly respond or do changes to the PR.

How do you want me to solve the python linting. I personally don't agree with the linting rule since I think it's quite clear what the true and false stands for, but I can change it if you want to :)

[jemrobinson]

@BlackVoid: I've fixed the linting errors and made one change to your logic: I replaced the custom UID calculation method with a call to UidCache but kept the logic of reading from the attribute dict if it exists.

Could you check whether these changes still work for you?

[BlackVoid]

I've tested the changes and other than some minor issues that I fixed with the latest commit it appears to work well.

[jemrobinson]

LGTM. Thanks @BlackVoid !