bhoriuchi / passport-activedirectory

Active Directory strategy for passport.js
29 stars 16 forks source link

Optionally reuse an ActiveDirectory instance #1

Closed sd65 closed 8 years ago

sd65 commented 8 years ago

If you already have an ActiveDirectory instance, use it instead of creating a new one.

Exemple :

[...]
var ad = new ActiveDirectory(options)
[...]
passport.use(new ActiveDirectoryStrategy({
  integrated: false,
  ldap: ad
[...]

If you provide an config object, a new instance will be created. Else, the existing one is used.

bhoriuchi commented 8 years ago

I had to revert this merge because it appears the logic is off

old line

this._ad = new ActiveDirectory(options.ldap)

your new code

if (typeof this._ad !== 'function') { this._ad = options.ldap }
else { this._ad = new ActiveDirectory(options.ldap) }

case 1 would always be true since we haven't set this._ad yet

the line should look like

this._ad = (typeof options.ldap === 'function') ? options.ldap : new ActiveDirectory(options.ldap)

i will add this.

sd65 commented 8 years ago

My bad, I had to commit with the web interface and made a mistake.

Anyway, thanks for the quick merge ! :+1:

bhoriuchi commented 8 years ago

np, just going to update the docs and will publish a build soon.

bhoriuchi commented 8 years ago

v1.0.2 published to npm

sd65 commented 8 years ago

Hi, I just came back from holidays and this is not usable, there is an error. (This time, I tested my code)

The problem is that both the config an the ActiveDirectory instance are objects. So our condition is not correct.

The good (and more elegant) way is :

this._ad = (options.ldap instanceof ActiveDirectory) ? options.ldap : new ActiveDirectory(options.ldap);

What do you think ?

bhoriuchi commented 8 years ago

Ah yes you are correct. I'll publish a fix some time today with your solution. Thanks!