Exception on connection error about passport-ldapauth HOT 8 CLOSED

Setting an error event handler for the clients had no effect on the outcome so I filed an issue ldapjs/node-ldapjs#144. This happens only if using anonymous binding and I believe things would work just fine if instead of not binding at all in ldapauth-fork I change it to bind with empty dn and password when adminDn is not provided.

I'll change and release it tomorrow when I'm back on my laptop.

from passport-ldapauth.

On a second thought it is probably not the same to bind with empty credentials. I changed ldapauth-fork to allow setting empty string to adminDn (ie. if undefined or null, do not bind the admin client, otherwise bind with given credentials) anyways.

I'll keep the issue open for now and see if the issue I reported on node-ldapjs receives some helpful advise. I'll also try to see if connection pooling would help with the issues, the current solution in passport-ldapauth is not very elegant as it will create a new LdapAuth instance on every request.

from passport-ldapauth.

@vesse Give ldapjs/node-ldapjs@ebcba12 a try.

from passport-ldapauth.

I tried the patch with ldapauth-fork and I still get the unhandled ConnectionError, am I supposed to add an error callback in the ldapauth-fork code as well?

from passport-ldapauth.

It seems that passport-ldapauth is just passing it through to express, even with the fix

If I put this in the authenticate method, it handles it

      if (err.name === 'ConnectionError')
      {
        return self.fail('Connection failed to LDAP server');
      }

Does that look like a good fix? Or do we want to handle this a different way? It seems like something passport-ldapauth should handle

from passport-ldapauth.

@pfmooney No unhandled exceptions occur with the fix at least in my simple tests. Looks good to me.

@pyromanfo The exception you see is not unhandled - if it were you would see it clearly stated in the output, and an express app would crash. At least when I tried my express app just logged the error and kept running when using ldapauth-fork using the fixed version of ldapjs.

Leaking the error to express is, in my understanding, the way to handle real errors in Passport. Pretty much every strategy I checked has something like if (err) return self.error(err); in callbacks used in authenticate method. You would then handle the error in express error handlers (should you have any).

But of course it is not clear where to draw the line between an error and a failure. I'm a bit on the edge with this one - connection error to LDAP server seems pretty severe to me, but on the other hand passport-ldapauth currently creates a new client for every authentication request so a temporary connection error will not break a service.

The reason for creating a new instance on every authenticate call was related to connection errors but cannot remember exactly what kind of. My plan was to further modify ldapauth-fork to use connection pooling and improve the error handling to see if creating one client would suffice but then I saw that the original ldapauth module is rising from hibernation and being rewritten, and decided to put this now on hold. How this is related is that I think a ConnectionError would be less fatal if the underlying library would reconnect in case of errors.

But, whatever the final resolution to this issue is, it will not happen before a new version of ldapjs is released as I don't really want to publish modules having dependencies to unpublished stuff.

from passport-ldapauth.

Yeah, this was my bad, I did not have my error handling in the app setup correctly

http://expressjs.com/guide.html#error-handling

from passport-ldapauth.

@vesse Thanks for running some tests. There's one more ldapjs bug I want to wrap up before inquiring with Mark about cutting a new release. I hope to have it pushed out soon so you can bump your version requirements.

from passport-ldapauth.