Updates ldapjs to 1.0.0

[Mctalian]

Closes #29 by upgrading ldapjs to >0.7.2

[Mctalian]

There seems to be an issue with ECONNRESET after about 15 minutes when using ldapjs v1.0.0. I wouldn't consider this PR until that is better understood.

[sentientcucumber]

This is kind of a naive guess, but could it have to do with something outside of this module? I'm pretty new to LDAP, but given it's a persistent connection, could Node or possibly the LDAP server, close the connection after 15 minutes? I notice some timeouts happening after 5 minutes.

I don't see an unbind happening. I wonder if it would be worth exposing it so developers can determine when/if they want to unbind when they see fit? Not sure if this goes against the protocol standards though.

[Mctalian]

Yes, I agree. It is likely the LDAP server itself forcing the connection
closed after 15 minutes. However previously it did not cause the
application to crash. So yes, I think it's more of an issue with ldapjs not
this passport strategy, but I haven't done much investigation on it yet.
On Dec 8, 2015 10:11 AM, "Michael Hunsinger" notifications@github.com
wrote:

This is kind of a naive guess, but could it have to do with something
outside of this module? I'm pretty new to LDAP, but given it's a persistent
connection, could Node or possibly the LDAP server, close the connection
after 15 minutes? I notice some timeouts happening after 5 minutes.

I don't see an unbind happening. I wonder if it would be worth exposing it
so developers can determine when/if they want to unbind when they see fit?
Not sure if this goes against the protocol standards though.

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[dustinsmith1024]

This fixes my node 5 issues with this module. I have not run it in production yet though.

[dustinsmith1024]

@Mctalian assuming this the timeout issue your talking about? ldapjs/node-ldapjs#318

Looks like my client disconnects after about 15 minutes as well. The reconnect option is not working for me either. Has anyone had luck with this yet?

[Mctalian]

I have unfortunately not been able to look into this further yet. I've got one other task that I'm hoping to finish up this weekend then I can start messing with this again. There is a recent comment in that issue you linked though, it might provide some insight.

[dustinsmith1024]

Yea I noticed but hadn't time to play with it either. I should get work time on Monday or Tuesday to see. Might need to make a change here to implement the suggestion.

[dustinsmith1024]

I was able to get reconnect working here: Mctalian/passport-windowsauth@master...dustinsmith1024:master. I will clean the code up and PR tomorrow hopefully.

[Mctalian]

Nice work! That looks pretty solid, I'll have to try it out tomorrow to see if it also works for my scenario.

[Mctalian]

Yup! Confirmed, that works for me, good work!

[dustinsmith1024]

I have had less luck today. It seems to be not handling the error consistently. The second time the idle timeout happens the error is not handled properly. I tried reattaching an error handler after the client is reconnected, but it doesn't seem to help.

[Mctalian]

Ah... you're right. I didn't sit and wait for another timeout. I have the same issue over here.

[dustinsmith1024]

Think I have it worked out. I was making harder than it was. @Mctalian can you try this out? https://github.com/Mctalian/passport-windowsauth/compare/master...dustinsmith1024:error-handling?expand=1

If it works for you I can PR it. Basically credentials options needs to be bindCredentials and the reconnect option was not be passed down to LDAP. Then the last part was we needed to handle uncaught errors in the LdapValidator client instance.

[Mctalian]

Seems to be running fine after near 2 hours, I think this is a solid
solution!
On Jan 14, 2016 8:59 PM, "Dustin Smith" notifications@github.com wrote:

Think I have it worked out. I was making harder than it was. @Mctalian
https://github.com/Mctalian can you try this out?
https://github.com/Mctalian/passport-windowsauth/compare/master...dustinsmith1024:error-handling?expand=1

If it works for you I can PR it. Basically credentials options needs to
be bindCredentials and the reconnect option was not be passed down to
LDAP. Then the last part was we needed to handle uncaught errors in the
LdapValidator client instance.

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[dustinsmith1024]

👍 Awesome Ill take out the stack logging and submit a PR.

[Mctalian]

Ok, thanks to @dustinsmith1024 , this PR should now work with version ~1.0.0 of ldapjs

@jfromaniello can you check this over and merge it? Or tag someone who can/will?

Thanks!

[dustinsmith1024]

I wasn't 100% on this version number. If you want something else let me know.

[Mctalian]

IIRC, semver has cautions about anything below 1.0.0 that it's still in active development/experimental and the api/interface can change often. So unless the original author decides to release this as v1.0.0, I think it's totally fine. In all honesty, I think the author should be adjusting the version numbers, but considering the last commit was 10 months ago, it seems pretty unlikely that this will be pulled in anyway.

[jfromaniello]

Thank you all and sorry for the delay. I just merged these changes and released to npm as v1.0.0.

[Mctalian]

Thanks @jfromaniello !

[dcolens]

What about the econnreset errors, does someone know where these come from, I have them too, while I never saw any with ldapjs 0.7.x

[dustinsmith1024]

I am not sure. All I did was implement what the ldpapjs maintainer said to do in the other linked issue. Capture the error and pass reconnect=true.

My guess is the library used to do this behind the scenes. When I checked with our IT team they had Active Directory setup to timeout after 15 minutes. So it seems to me like the new version is acting correctly.

[dcolens]

I agree, I did the same

On Fri, Jan 29, 2016 at 6:37 PM, Dustin Smith notifications@github.com
wrote:

I am not sure. All I did was implement what the ldpapjs maintainer said to
do in the other linked issue. Capture the error and pass reconnect=true.

My guess is the library used to do this behind the scenes. When I checked
with our IT team they had Active Directory setup to timeout after 15
minutes. So it seems to me like the new version is acting correctly.

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[Mansuro]

Hi,
I am using latest versions of ldapjs and passport-windowsauth and I have the problem