Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial version of ldap authentication plugin. #1218

Merged
merged 17 commits into from
Sep 13, 2024

Conversation

petervarkoly
Copy link
Contributor

I've enhanced Radicale with ldap authentication plugin.
I've tested it and works fine with openldap2 and samba-ad.
In a next step I would like to enhance the rights module:
The ldap plugin reads the group of the authenticated user. From this reason I would enhance the from_file.py to be able handle group based rights.

@pbiering pbiering added this to the 3.2.x milestone Mar 1, 2024
@pbiering pbiering added the need:reporter feedback feedback from reporter required label Mar 3, 2024
@pbiering
Copy link
Collaborator

pbiering commented Mar 3, 2024

@petervarkoly : can you please create an empty commit to trigger "actions"? See here for more information: https://stackoverflow.com/questions/52408592/how-to-relaunch-github-check-without-pushing-new-commits

Also reduce the patch to minimal required changes and please do not apply changes to config and rights in an active way.

@pbiering pbiering mentioned this pull request Mar 23, 2024
@pbiering pbiering added need:rebase pull request cannot be applied before rebased reverse proxy reverse proxy related labels Mar 23, 2024
bishtawi added a commit to bishtawi/Radicale that referenced this pull request Jul 30, 2024
@pbiering pbiering removed the need:rebase pull request cannot be applied before rebased label Aug 25, 2024
@pbiering
Copy link
Collaborator

@petervarkoly : thank you for rebase, please honor also my comment in the code review

@pbiering
Copy link
Collaborator

@petervarkoly : thank you, please check my additional comments.

BTW: is ldaps supported out-of-the-box and which LDAP TLS truststore is in use? Potentially for TLS additional options for the TLS truststore can be helpful to be able to use a custom instead of a system one.

@petervarkoly
Copy link
Contributor Author

petervarkoly commented Aug 27, 2024 via email

@pbiering
Copy link
Collaborator

pbiering commented Sep 8, 2024

@petervarkoly : Test / lint is not happy about your code, please improve

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@pbiering
Copy link
Collaborator

@petervarkoly : lint found next issue, please try to run local tests before next commit, see https://github.com/Kozea/Radicale/wiki/Development-Testing-Release

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@pbiering
Copy link
Collaborator

@petervarkoly : flake8 is now happy, but mypy reports now issues

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@pbiering
Copy link
Collaborator

@petervarkoly : hmm, latest commit broke something

radicale/auth/__init__.py:55: in BaseAuth
    _ldap_groups: set[str] = set([])
E   TypeError: 'type' object is not subscriptable

please try to run the testsuite completly on your side before next commit

@petervarkoly
Copy link
Contributor Author

petervarkoly commented Sep 11, 2024 via email

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@pbiering
Copy link
Collaborator

@petervarkoly : no luck so far, lint plus test reporting now again issues

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@pbiering
Copy link
Collaborator

@petervarkoly : now lint needs to make happy again.

Copy link
Collaborator

@pbiering pbiering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@pbiering
Copy link
Collaborator

@petervarkoly : well done now, but coincidently via #1564 I found that there is already another LDAP plugin available https://github.com/malmeloo/radicale3-ldap-auth which was forked from older ones - were you aware about?

Would it make sense for any alignment before merge?

@petervarkoly
Copy link
Contributor Author

petervarkoly commented Sep 13, 2024 via email

@pbiering
Copy link
Collaborator

pbiering commented Sep 13, 2024

@petervarkoly : I will merge your code and will also notify the other project that it would be better to extend the merged code instead of having an external plugin in parallel.

@pbiering pbiering merged commit da844f4 into Kozea:master Sep 13, 2024
24 checks passed
@pbiering pbiering removed the need:reporter feedback feedback from reporter required label Sep 13, 2024
@pbiering
Copy link
Collaborator

pbiering commented Sep 13, 2024

@malmeloo : would it make sense that this merged PR can replace your plugin from https://github.com/malmeloo/radicale3-ldap-auth?

@malmeloo
Copy link

Probably! I'll be honest, I already switched away from Radicale due to performance issues on my NAS, but it's great to see native LDAP support. 'My' plugin is just a slightly modified version in a fork chain of 4 or 5 other repositories ;)

I'll add a note to the readme to indicate native LDAP support, to prevent confusion.

@pbiering pbiering added the auth:ldap Authentication LDAP label Sep 15, 2024
@pbiering pbiering modified the milestones: 3.2.4, 3.3.0 Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth:ldap Authentication LDAP feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants