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

Fix user login and creation with LDAP #2107

Merged
merged 11 commits into from
Feb 11, 2023

Conversation

cmintey
Copy link
Contributor

@cmintey cmintey commented Feb 8, 2023

What type of PR is this?

  • bug
  • documentation
  • feature

What this PR does / why we need it:

This change builds off of the great work done by @ERLKDev in #1777, which appears to be dead.

The LDAP flow was changed to the following:

  1. bind to the LDAP server using an external user (one that has search privileges)
  2. search for users given the LDAP_USER_FILTER. By doing this, we make no assumptions as to the query structure
  3. try to bind with the user_dn returned in step 2 and the provided password
  4. check if a user with the username or email already exists
  5. otherwise, create a new user

This PR will require new environment variables which have been documented as well.

Which issue(s) this PR fixes:

Fixes #2059
Fixes #1175
Fixes #1774
Fixes #1832

Testing

Unit tests were created in the original PR, they have been updated and pass as expected.

Release Notes

Fixes LDAP user login and creation. Please check the new environment variables required for LDAP (https://nightly.mealie.io/documentation/getting-started/installation/backend-config/#ldap)

@hay-kot
Copy link
Collaborator

hay-kot commented Feb 11, 2023

This looks excellent. Thanks so much for getting this over the finish line!

I've been looking at implement some E2E testing with https://glauth.github.io/ in a github action. Would love to get your thoughts on that if you have any.

@hay-kot hay-kot merged commit da60e56 into mealie-recipes:mealie-next Feb 11, 2023
@proffalken
Copy link
Contributor

This looks excellent. Thanks so much for getting this over the finish line!

I've been looking at implement some E2E testing with https://glauth.github.io/ in a github action. Would love to get your thoughts on that if you have any.

FWIW I'm currently running https://github.com/nitnelave/lldap on my home network, but given that I've now got Unifi ID installed on my cloudkey for my home network, I'm going to have a go at setting that up as well.

@cmintey
Copy link
Contributor Author

cmintey commented Feb 13, 2023

This looks excellent. Thanks so much for getting this over the finish line!

I've been looking at implement some E2E testing with https://glauth.github.io/ in a github action. Would love to get your thoughts on that if you have any.

I had a look at GLAuth and it seems like it would do well for e2e testing in github. You could even test out multiple configurations easily (see: https://github.com/glauth/glauth/blob/master/v2/sample-simple.cfg#L41-L45). I could assist in configuration if needed.

FWIW I'm currently running https://github.com/nitnelave/lldap on my home network, but given that I've now got Unifi ID installed on my cloudkey for my home network, I'm going to have a go at setting that up as well.

I'm also running lldap, which is great. Would be interested to see how well this integrates with the Unifi ID...

@Zackptg5
Copy link

Zackptg5 commented Feb 15, 2023

I've been testing this out and overall working great. One issue though, it appears that mealie still uses it's own database, only pulling from lldap when user doesn't exist in its database.
Is there a way to use lldap exclusively? For example, when new user is created/deleted in mealie, it's also created/deleted in lldap server. If logging in and user deleted from lldap, mealie will delete it on next login attempt or just not use its own user section at all.
In it's current iteration, mealie will default to it's own database over lldap which can get messy quick. If a user is created in mealie, it won't be added to lldap. If user is deleted in lldap, it won't be deleted in mealie.
It'd be better if ldap is enabled, mealie will use it exclusively for all users. This way I could just use lldap server for all user management for mealie and other services that pull from it

@cmintey
Copy link
Contributor Author

cmintey commented Feb 15, 2023

Yeah, I realized this problem as well and didn’t catch it in my testing. I have a proposed solution in this discussion #2126. Just need to get thoughts on if this is the right way to proceed.

I personally think having an admin set the login method (ldap or mealie) for each user would be better than just deleting the user entry if they log in with ldap for the first time.

I also don’t think that mealie should be managing user creation/deletion on your ldap server. If you need to add or remove a user, that should be done in the ldap server itself

@Zackptg5
Copy link

@cmintey exactly what I was thinking. If using ldap, all user/group management should be done there. It would be cool for mealie user/group management page to be able to manage that but I don't know how much extra complexity that would be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants