Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add connect callback for adminClient to rebind on reconnect #68

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

theasp
Copy link

@theasp theasp commented Oct 8, 2018

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
Copy link
Owner

vesse commented Oct 9, 2018

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
Copy link
Author

theasp commented Oct 11, 2018

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
Copy link
Author

theasp commented Dec 2, 2018

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

@vesse
Copy link
Owner

vesse commented Dec 3, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants