vesse / node-ldapauth-fork

Simple node.js module to authenticate against an LDAP server
Other
127 stars 79 forks source link

Add connect callback for adminClient to rebind on reconnect #68

Closed theasp closed 5 years ago

theasp commented 6 years ago

This solves the anonymous after reconnect issue from #67, and adds a bit of logging around binding adminClient. Let me know if you'd like anything further changed.

vesse commented 6 years ago

Didn't lost connection emit anything? This looks like it works, but since error and connectTimeout handlers unset the _adminBound flag this could technically lead into having the rebind .on('connect') handler assigned multiple times.

theasp commented 6 years ago

The error signal isn't emitted when reconnect is turned on because the library is handling the error on it's own. I've updated the PR to always use the connect handler to bind to the admin user, ensuring that it's only added to adminClient once, and allowing for reconnect to not be handled as a special case.

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.

As an aside, it looks like _handleError will set _adminBound to false on an error from userClient:

  this._adminClient.on('error', this._handleError.bind(this));
  this._userClient.on('error', this._handleError.bind(this));

// ...

LdapAuth.prototype._handleError = function(err) {                                                                                                                                             
  this.log && this.log.trace('ldap emitted error: %s', err);
  this._adminBound = false;
  this.emit('error', err);
};
theasp commented 5 years ago

@vesse Is there anything I can do to help move this along?

vesse commented 5 years ago

@theasp reminding is enough - sorry! Seems to work at least in my simple test (should really implement tests some day but I haven't been working with LDAP implementations lately).

There's one semicolon missing, linter complains, but I'll fix that myself.