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

BUGFIX: Fix LDAP bind failed if login from different location #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manmath
Copy link

@manmath manmath commented Sep 1, 2017

Hi @radmiraal,

Here the pull request on what I have discussed with you on slack. Please review.

Thanks,
Man

@laurisaarni
Copy link

@radmiraal I faced the same bug. Please review and merge asap.

@radmiraal
Copy link
Contributor

hi, thanks for the pr. I'm not fully convinced to merge though. Just by reading it feels like the bind & authenticate are both using the same credentials, so if the actual credentials would be used, then why bind twice?

I still feel like the whole binding / authentication process could use some more rework. Unfortunately I am not able to test this change as I don't have ldap installed and also no time to set it up. But just by reading I'm not 100% sure yet.

@radmiraal
Copy link
Contributor

thinling about it I think the directory service itself should do binding if needed (Active Directory or non-anonymous bind to connect at all), but this should be a step before the authenticate stuff.

Thinking about it the initial bind might need to go to the ldapConnect() method. In case of ActiveDirectory this method should always bind. For LDAP this should only be done if bind credentials are given.

If that would be the case the authenticate() method could just do what it's named after... authenticating an account.

Does that make sense to you?

@manmath
Copy link
Author

manmath commented Sep 8, 2017

It binds twice because of the user come from different location in LDAP.

  • The first binding (initialize) is binded with static given dn and password from Settings configuration. If the initialize binding return false, it means there might be a problem with LDAP connection or the credential is not correct.
  • The second binding (bind with real credential from login form). When the first binding is success, there is a function (I could not remember now) will filter an LDAP information according to the given username (get from login form). If username is found then we can get the dn (login name for LDAP). So we have to bind again (verifyCredential()) with the found dn.

@radmiraal
Copy link
Contributor

@manmath yes, that's exactly what I mean. The first bind has nothing to do with authentication, but with the actual connection made with LDAP before actually authenticating. This would better be done outside the authenticate() method. It's like the connection to a database fails before checking a username / password record in a user table.

@kdambekalns kdambekalns changed the title [BUGFIX] Fix Ldap bind failed if login from different location BUGFIX: Fix LDAP bind failed if login from different location Sep 11, 2017
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.

3 participants