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

error events from ldapjs must be handled #61

Open
tswaters opened this issue Apr 20, 2018 · 1 comment
Open

error events from ldapjs must be handled #61

tswaters opened this issue Apr 20, 2018 · 1 comment

Comments

@tswaters
Copy link

If the ldap client periodically closes idle connections, an error event will bubble up from ldapjs to the top of the stack & kill the process. We see this with ECONNRESET errors that terminate the process.

Thing is, this module will check if it has a connection open and if not open it whenever calling into authenticate.... the best way to deal with it seems to be to attach a dummy error handler to the client:

const ldapClient = LdapClient({...options})

ldapClient.on('error', () => { console.log('worry not!' }) // <-- clients need this or risk process termination

module.exports = (req, res, next) => {

  ldapClient.authenticate(uname, password, (err, user) => {
    if (err) { return next(err) } // <-- errors from reconnect after failed connection are handled here
    req.user = user
    next()
  })

})

Or, if the library wants to handle it - it just needs to not re-emit the error event here:
https:/vesse/node-ldapauth-fork/blob/v4.0.2/lib/ldapauth.js#L158

I'm not sure if there are things I'm missing here, so it might be best to leave this to the client to handle... hence my first comment on add this to the basicAuth example in the readme.

@Philzen
Copy link

Philzen commented Jun 24, 2022

@vesse thx so much for creating this excellent wrapper interface around ldapjs.

However, this issue was a serious problem on my first tests. After a couple of successful queries i always got:

api | Error: connect ECONNREFUSED 52.87.186.93:389
api |     at __node_internal_captureLargerStackTrace (node:internal/errors:465:5)
api |     at __node_internal_exceptionWithHostPort (node:internal/errors:643:12)
api |     at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1187:16)
api |     at TCPConnectWrap.callbackTrampoline (node:internal/async_hooks:130:17)
api | Emitted 'error' event on LdapAuth instance at:
api |     at LdapAuth._handleError (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/ldapauth-fork/lib/ldapauth.js:202:8)
api |     at Client.emit (node:events:527:28)
api |     at Client.emitError (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/ldapjs/lib/client/client.js:1294:10)
api |     at Backoff.<anonymous> (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/ldapjs/lib/client/client.js:1038:12)
api |     at Backoff.emit (node:events:527:28)
api |     at Backoff.backoff (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/backoff/lib/backoff.js:41:14)
api |     at /home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/ldapjs/lib/client/client.js:1024:15
api |     at f (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/once/once.js:25:25)
api |     at Socket.onResult (/home/phil/prog/testing/rw-ldap-integration-sandbox/node_modules/ldapjs/lib/client/client.js:814:7)
api |     at Object.onceWrapper (node:events:642:26) {
api |   errno: -111,
api |   code: 'ECONNREFUSED',
api |   syscall: 'connect',
api |   address: '52.87.186.93',
api |   port: 389
api | }

From that moment on, the complete process was dead / unresponsive – which i didn't know was even possible in a RedwoodJS setup.

On my tests today so far, i could reproduce it, but the api-server stayed alive, still accepting subsequent requests. I could even handle and process the error return, so i'm unsure if this is fixed now or not.

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

No branches or pull requests

2 participants