vesse / node-ldapauth-fork

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

Promisify connection to handle timeout #52

Closed j-langlois closed 7 years ago

j-langlois commented 7 years ago

I am trying to wrap my ldap connection in a promise thereby catching all kinds of errors that could occur. Say my ldap server were to go down, and since the library only emits error events, how could I handle the emitted error such as a timeout? This is my code so far:

    // Convert LDAP async call to return a Promise.
    protected async authenticateLdap(ldapConnection: LdapAuth, username: string, password: string): Promise<ILdapUser> {
        return new Promise<ILdapUser>((resolve, reject) => {

            // Comes in deferred, crashing the thread (uncaughtException)
            ldapConnection.on('error', function (msg: string) {
                reject(err);
            });

            ldapConnection.authenticate(username, password, (err: any, result: ILdapUser) => {
                if (err) {
                    reject(err)
                } else {
                    resolve(result);
                }
            });
        });
    }

I considered using the connectTimeout ldapjs option but it would have to be applied on every auth attempt. How would you guys do it otherwise ? Would't we need a success event such as

 ldapConnection.on('success', function () {
     resolve();
 });

Thanks for the help

vesse commented 7 years ago

After merging PR #49 (released as v4.0.2) connectTimeout events are re-emitted as errors.

Other than that I'd suggest checking Stackoverflow on how to promisify things using event emitter. I'd assume it's not trivial because there could be some network related events emitted when there is no-one calling authentication (ie. you have already resolved the promise but the error handler then later would like to reject). Also it looks to me you assign a new error handler on every authentication call.