LdapAuth._onConnectAdmin will be executed twice with each authenticate request

[acappella2017]

LdapAuth._onConnectAdmin will be executed twice with each authenticate request since https://github.com/vesse/node-ldapauth-fork/pull/68.

@vesse @theasp Is this the expected behavior?

[vesse]

Oh yeah, didn't think of that. If you create a an instance and immediately call authenticate the connect event has not yet been emitted/handled. Naturally shouldn't do that, but likely it does not matter too much. Open for PRs of course.

[theasp]

@acappella2017 are you seeing this for every authenticate request, or just requests made before the connect event is handled?

I think this is what I was referring to in the PR:

This will also bind as soon as the connection to the LDAP server is made, not when _adminBind is called later. I left the previous behaviour of having _adminBind attempt a bind and call the callback when it's not previously bound, but I think it would be cleaner to return an "admin client is not bound" error in this case.

Given the previous definition of _adminBind(), it appears that the same situation would have occurred if you made two requests back to back, as adminBound will not be set until the callback is handled.

LdapAuth.prototype._adminBind = function(callback) {
  // Anonymous binding
  if (typeof this.bindDN === 'undefined' || this.bindDN === null) {
    return callback();
  }
  if (this._adminBound) {
    return callback();
  }
  var self = this;
  this._adminClient.bind(
    this.bindDN,
    this.bindCredentials,
    function(err) {
      if (err) {
        self.log && self.log.trace('ldap authenticate: bind error: %s', err);
        return callback(err);
      }
      self._adminBound = true;
      return callback();
    }
  );
};

[acappella2017]

@vesse @theasp Thanks for your response.

Actually, we are using passport-ldapauth to do user authentication.

var handleAuthentication = function(req, options) {
  ...
  ldap = new LdapAuth(this.options.server);
  ldap.on('error', errorHandler);
  ldap.authenticate(username, password, function(err, user) {
     ldap.close(function(){
      // We don't care about the closing
    });
   ...
  }
}

As you can see, the handleAuthentication method creates an ldapAuth instance and immediately call authenticate method..So we get duplicate admin bind requests for every user authentication request.

[theasp]

Ok, that's unfortunate. I suppose it could only register the connect event handler when reconnect is enabled, @vesse would you like a PR for that?

Another option would be for passport-ldapauth to keep the last LdapAuth object, and create a new one if the options change, with the added bonus of quicker responses.

@acappella2017 I'm curious, is this causing you any problems?

[acappella2017]

@theasp the second option looks good to me especially for the performance improvement.

@vesse I'm not sure whether caching the LdapAuth object will cause any issue in passport-ldapauth or not?

[vesse]

Yeah passport-ldapauth recreates an LdapAuth instance on every request because initially when it was implemented, ldapauth did not reconnect and that of course was not good when used with passport, and as I was in a work related need for a passport LDAP authentication I just made it that way.

Naturally would be great if only one client was created and it would reconnect when needed - originally the problem in passport-ldapauth was that if no one logged in in about 15 minutes the LDAP server closed the connection and then no one could login before restarting.

Unfortunately I have not been using neither of these libraries since 2015 myself so I don't have an actual environment where to see how things then work.

[acappella2017]

@theasp @vesse I rethought this issue, caching ldapAuth instance may introduce much complexity. Maybe we can just defer the creation of reconnect listener to fix this issue. I created https://github.com/vesse/node-ldapauth-fork/pull/71 for it. Could you help to review the code?

[theasp]

I don't have time to try it for a few days, but your PR looks like it should work. However, I think this would restore the previous behaviour for "one shot" use, but would use the new behaviour when reconnect is used, or a new option for adminBindOnConnect is true:

  if (opts.reconnect || opts.adminBindOnConnect) {
    this._adminClient.on('connect', function() {
      self._onConnectAdmin();
    });
  }

What do you think?

[acappella2017]

@vesse you fix looks simple and clean. But we may still encounter the duplicate admin binding requests when reconnect option is enabled.

How about only install the reconnect listener when reconnect option is enabled?

Like this:

  if (opts.reconnect) {
    var self = this;
    this.once('_installReconnectListener', function() {
      self.log && self.log.trace('install reconnect listener');
      self._adminClient.on('connect', function() {
        self._onConnectAdmin();
      });
    });
  }

      if (opts.reconnect) {
        self.emit('_installReconnectListener');
      }

[theasp]

It appears to me that your fix would only prevent the connect handler from being added twice, which isn't happening, but not prevent a double bind when reconnect is used. Outside of "one shot" use, the calls to adminBind (from authenticate) are what could cause a double bind, as they also will attempt a bind if one has not been completed, ie adminBound is false.

To prevent a double bind in a reconnect use case, adminBind could throw an error if reconnect is enabled and adminBound is false. This isn't the problem you are reporting with passport though.

I've not tried to recreate the problem you are having, but could you tell us how it's affecting your project? I.e. is the double bind preventing authentication?

[acappella2017]

@theasp Thanks for your response. It's easy to reproduce. Just download the passport-ldapauth project and run npm test, you can see that _onConnectAdmin is called twice in most of the test cases.

 LDAP authentication strategy
    by itself
      ✓ should export Strategy constructor directly
      ✓ should export Strategy constructor separately as well
      ✓ should be named ldapauth
      ✓ should throw an error if no arguments are provided
      ✓ should throw an error if options are not accepted by ldapauth
      ✓ should initialize without a verify callback
    with basic settings
      ✓ should return unauthorized if credentials are not given
_onConnectAdmin
_onConnectAdmin
      ✓ should allow access with valid credentials
_onConnectAdmin
_onConnectAdmin
      ✓ should allow access with valid credentials in query string
_onConnectAdmin
_onConnectAdmin

The double binding doesn't prevent authentication. But it causes performance regression in our project. (every admin bind means network request to LDAP server)

I just noticed that the check to _adminBind flag is removed since #68.

LdapAuth.prototype._adminBind = function(callback) {

   - if (this._adminBound) {        
   -   return callback();       
   - }

So looks like admin bind will be called duplicately when we call authentication in the same ldapAuth instance for several times.

I believe we'd better add the check back. But reset the _adminBound flag in reconnect listener (which means the ldap client is disconnected and reconnected, so we have to bind the admin client again).

[theasp]

@acappella2017, those are already in master: https://github.com/vesse/node-ldapauth-fork/blob/e58896361087e5317fee4d34ebaea851e0b26811/lib/ldapauth.js#L210 https://github.com/vesse/node-ldapauth-fork/blob/e58896361087e5317fee4d34ebaea851e0b26811/lib/ldapauth.js#L165

The error handler doesn't get called when reconnect is uses though, it's handled by the ldap library on it's own.

[theasp]

@vesse I've added PR #72 which restores the previous behaviour of relying on _adminBind when reconnect isn't true, so passport-ldapauth should behave as it did before. It also prevents some the occurrences of double binds @acappella2017 is referring to when reconnect is true.

I've lightly tested it, and it seems to pass the test @acappella2017 provided above:

    with basic settings
      ✓ should return unauthorized if credentials are not given
In _onConnectAdmin
      ✓ should allow access with valid credentials (70ms)
In _onConnectAdmin
      ✓ should allow access with valid credentials in query string (53ms)
In _onConnectAdmin
      ✓ should return unauthorized with invalid credentials (54ms)
In _onConnectAdmin
      ✓ should return unauthorized with non-existing user
In _onConnectAdmin
[...]

[acappella2017]

@theasp Sorry I missed this part. Now I rollbacked the check for _adminBind in #71.

I don't really like the new option in #72 but either way (#71 or #72) works for me.

[uerg]

we also call ldapauth immediately after creating an ldap instance. with v4.1.0 ldapauth never returns. #71 solves the problem, #72 not.

[vesse]

Sorry I was on vacation during the holidays. I'll look into this more tomorrow, but after quickly checking the alternatives I kinda prefer the _installReconnectListener version.