client.bind() should not pass an error to the callback function before having tried to connect to all server urls

[gizmodus]

First, thanks for the great library! Also very cool that multiple server urls are supported now. Regarding this I ran into a problem.

I use the following code:

this.client.bind(
  this.activeDirectoryConfig.username,
  this.activeDirectoryConfig.password,
  (error) => {
    if (error) {
      sharedLogger.error(
        `Error in ActiveDirectoryClient.bindServiceUser() - ${error}`
      );
      reject(error);
    } else {
      resolve();
    }
  }
);

In case the client can not connect to the first server url in the array, the callback is called with an error. Later, when attempting to connect to the second server url, the callback is called again, this time without an error.

In my opinion the callback should not be called with an error before all server urls have been tried. Only if all attempts have failed, an error should be thrown.

[mceyzar]

I have the same issue, I Didn't want to make pull request as I'm not sure how bad I'll break it. The fix for my issue is shown below, may help someone until an official fix is available

Get version 2.2.3 from NPM, then:

patch node_modules/ldapjs/lib/client/client.js

find line 114 this._nextServer = 0 add this on the next line this._allServersLooped = false

then find function connectSocket, mine was on line 797 update the first few lines to:

  // Establish basic socket connection
  function connectSocket (cb) {
    const server = self.urls[self._nextServer]
    self._nextServer = (self._nextServer + 1) 
    if(self._nextServer >= self.urls.length ){
      self._allServersLooped = true
      self._nextServer = self._nextServer % self.urls.length
    }

    cb = once(cb)

    function onResult (err, res) {
      if (err) {
        if (self.connectTimer) {
          clearTimeout(self.connectTimer)
          self.connectTimer = null
        }
        if(self._allServersLooped)self.emit('connectError', err)
      }
      cb(err, res)
    }
...

That should fix this problem. However, it is on your own risk, I have not tested all the features. But at least the project continues.

[UziTech]

Should it emit all of the errors if all servers fail? or just the last error?

[mceyzar]

Since I encapsulate the connect in a try-catch block (within my project code), The hack I put there will throw the first error after it goes the list of server at least once. I am not sure if this is the desired result for every cases. But it works out for my project. What do you think should be the case?

[UziTech]

I kind of think it should emit every error as it does now. Then the user can decide how they want to handle the error.

[gizmodus]

I would only emit one error. This error could be a summary of all failed connection attempts:

"Error: Connection to ldap servers failed: [error of connection attempt to first server], [error of connection attempt to second server] ..."

[jsumners]

I think this is something we just plain forgot to address in https://github.com/ldapjs/node-ldapjs/pull/658

I am okay with a PR that:

1. Recognizes an array has been passed for the server list
2. In the case of the array list, tries each server until one succeeds or all fail
3. If all fail returns an error like:

{
  message: 'could not connect to any specified servers',
  details: [
    { message: 'cause for server connection failure', serverAddress: 'tried.one.example.com' },
    { message: 'cause for server connection failure', serverAddress: 'tried.two.example.com' }
  ]
}

[jsumners]

👋

On February 22, 2023, we released version 3 of this library. As a result, we are closing this issue/pull request.

Please see issue #839 for more information, including how to proceed if you feel this closure is in error.