vesse / passport-ldapauth

LDAP authentication strategy for Passport
MIT License
313 stars 100 forks source link

Client doesn't unbind #88

Closed KaiserWolfTaco closed 4 years ago

KaiserWolfTaco commented 5 years ago

I see a note about the unbind issue affecting node.js v10. Is there a fix for this? The only fix i see in node-ldapjs issue 483 is to use ldapts but I don't think that's possible with this module. Is there another ldap passport strategy i can use?

scottbell commented 5 years ago

Looks like this got fixed: https://github.com/ldapjs/node-ldapjs/pull/497

KaiserWolfTaco commented 5 years ago

Thanks for the heads up Scott. Do you know when this will find its way into passport-ldapauth?

scottbell commented 5 years ago

It looks like this part of a larger effort to get node-ldapjs to be compatible with Node 10 for node-ldapjs 2.0: https://github.com/ldapjs/node-ldapjs/issues/524 This passport plugin is pegged: "ldapjs": "^1.0.1" so this plugin will need to be updated when node-ldapjs releases 2.0

scottbell commented 5 years ago

If you wanted to get ahead of the release, you could fork this plugin and have it point to: https://github.com/ldapjs/node-ldapjs/tree/next for its node-ldapjs dependency.

vesse commented 5 years ago

Hopefully ldapjs 2.0 will be released soon. Looks like it should not even be a breaking release for the client even if major version number is bumped. Quickly tested with the next branch and simple test at least passed without any changes to ldapauth-fork.

tnrich commented 5 years ago

Just checking in, any idea of an eta for node v10 support? We're eagerly awaiting this as well :)

vesse commented 5 years ago

Yeah seems that release is not progressing much. I'll try to find time to do a release candidate that depends on the release candidate of ldapjs soon

vesse commented 5 years ago

I've published a prerelease versions of ldapauth-fork and passport-ldapauth that depend on the next version of ldapjs and should thus unbind on Node v10 too. Install with npm install passport-ldapauth@next

seshness commented 4 years ago

Thanks for publishing, @vesse. I tried npm install passport-ldapauth@next locally, but still ended up with an old version of ldapjs that does not unbind.

The next version published on npm maps to 3.0.0-rc.1, which has this transitive dependency on ldapjs: passport-ldapauth@3.0.0-rc.1 -> ldapauth-fork@^4.3.2 -> ldapjs@^1.0.2

However with 3.0.0-rc.0, this dependency structure looks like passport-ldapauth@3.0.0-rc.0 -> ldapauth-fork@next -> ldapjs@next

So, at this time, npm install passport-ldapauth@3.0.0-rc.0 transitively installs ldapjs@2.0.0-pre.5.

Should passport-ldapauth@next refer to a newer version of ldapauth-fork, say 5.0.0-rc.5?

vesse commented 4 years ago

Oh shoot. I'll try to check these soon. I noticed that it's really easy to mess up the deps when publishing both from master and from the next branch.

vesse commented 4 years ago

@seshness should be fixed now:

└─┬ passport-ldapauth@3.0.0-rc.2
...
  ├─┬ ldapauth-fork@5.0.0-rc.5
  │ ├─┬ @types/ldapjs@1.0.6
  │ │ └── @types/node@13.9.5 deduped
  │ ├── @types/node@13.9.5 deduped
  │ ├── bcryptjs@2.4.3
  │ ├─┬ ldapjs@2.0.0-pre.5
seshness commented 4 years ago

Thank you @vesse! I appreciate it.

vesse commented 4 years ago

passport-ldapauth@3.0.0 has now been published after noticing that ldapjs had published their v2 (already a while ago), no need to use the next version anymore. I'll close this now.