vesse / node-ldapauth-fork

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

error events from ldapjs must be handled #61

Open tswaters opened 6 years ago

tswaters commented 6 years ago

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://github.com/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 commented 2 years ago

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