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

LDAP: Cleanup unused parameters #1329

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Conversation

sgissi
Copy link
Contributor

@sgissi sgissi commented Jan 23, 2023

Removes LDAP parameters from configuration that are unused and could mislead users.

Signed-off-by: Silvio Gissi silvio@gissilabs.com

Signed-off-by: Silvio Gissi <silvio@gissilabs.com>
@sgissi sgissi mentioned this pull request Jan 23, 2023
@marcelfolaron marcelfolaron merged commit 1841d0b into Leantime:master Jan 23, 2023
Copy link
Contributor

@b90g b90g left a comment

Choose a reason for hiding this comment

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

  • Documentation is missing how leantime is supposed to login into LDAP
  • How was this unused?!
  • how does lean time now login to ldap to check user credentials?..

@sgissi
Copy link
Contributor Author

sgissi commented Feb 3, 2023

Hi @b90g, good point about documentation, I noticed that in #1319. LDAP is listed as "Beta" in the documentation. Once Marcel and team considers it stable, I'll be happy to propose a PR for the docs.

The unused parameters ended up like that as LDAP implementation changed over time:

  • All searches are bound by LdapDN, so BaseDN is not used.
  • User domain was joined with the username to make the e-mail, however the actual email is an user attribute.
  • Bind user and password are not used anymore because the user supplies their own credentials at both Leantime login and import pages.

The idea behind the cleanup was exactly to avoid luring users into setting things that have no effect. Please let me know if you caught any issues as a result of the clean-up and I will propose a fix.

@b90g
Copy link
Contributor

b90g commented Feb 3, 2023

Thanks for the quick reply.

I see. My understanding of LDAP is very little and i was relying on the concept of a bind authentication user to much. (nextcloud, zammad, wekan..)

I will try to understand the config where in the user authenticates against ldap directly.

@sgissi
Copy link
Contributor Author

sgissi commented Feb 3, 2023

No worries, I spent quite some time in the code to figure them out. The only call to ldap_bind comes from the LDAP Service class, in the bind function.

The two places that I have found calls the LDAP Service bind function are:

  • Import at the users controller - Using the username and password from the form
  • Login at the auth service - Using the username and password from the login screen

I hope I didn't miss anything in the search!

@b90g
Copy link
Contributor

b90g commented Feb 3, 2023

UTC+1 and tired:

I notice the default ldap client config requires valid certificates.
I use univention coporate servers openldap implementation which has selfsigned certificates.

Maybe an option to override the invalid certificate would help? not sure if this applies

@sgissi
Copy link
Contributor Author

sgissi commented Feb 4, 2023

That is a great catch @b90g. Actually the way we use ldap_connect is passing host and port, that ends up with only unencrypted connections and is deprecated. The correct way is to pass the URL (ldap://host:port or ldaps://host:port). Besides that, if you are using a self-signed certificate, you will have to point to a copy of the public CA file (https://www.php.net/manual/en/ldap.constants.php#constant.ldap-opt-x-tls-cacertfile).

Do you mind opening a new issue to track that feature request? This PR is closed and will not have the right visibility.

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

Successfully merging this pull request may close these issues.

None yet

3 participants