Fix crash when LDAP server is unreachable

[gramakri]

Fixes #667

[MaxLeiter]

Should only be attempted if config.ldap is true (or, in this case, if authFunction === ldapAuth)

[astorije]

But that's already in the ldapAuth function, shouldn't that be enough?

[MaxLeiter]

from what I can tell, createClient() will be called regardless if LDAP is enabled; that's why it was in the if statement before

[xPaw]

createClient resides in ldapAuth function which is not called if LDAP is not enabled.

[gramakri]

As @xPaw said, the createClient is only called when using ldapAuth which is in-turn only called when LDAP is enabled.

[xPaw]

Should the client be torn down after performing bind? Pretty sure calling createClient multiple times would leave multiple clients hanging.

[gramakri]

@xPaw Good catch, it should be torn down. Strangely there is no way to do this in a documented way :/ I think maybe I should call https://github.com/mcavage/node-ldapjs/blob/master/lib/client/client.js#L974 directly. The "unbind" method does not destroy the socket - https://github.com/mcavage/node-ldapjs/blob/master/lib/client/client.js#L852 (for reference: http://ldapjs.org/client.html has no information on how to close the connection)

It does seem that 'error' atleast destroys the socket https://github.com/mcavage/node-ldapjs/blob/master/lib/client/client.js#L929.

[gramakri]

I have submitted a PR to ldapjs (ldapjs/node-ldapjs#393) to fix their docs. On further reading of the code, looks like unbind() is the way to go and not destroy()

[darshan-talati]

Thanks @gramakri for fixing the issue. It works perfectly!

[maxpoulin64]

Looks good to me. Can't test because I don't have my LDAP VM anymore, but the code makes sense.

[astorije]

Hey @gramakri, we have sticker packs for our contributors now!
If you're interested, you can fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂