Express.js and Bluebird - Handling the promise chain

Question

In a backend API I have a login route which should perform the following sequence of actions:

• Given an username and password, try to authenticate the user against an Active Directory. If authentication has failed reply with status 401. If success, continue.

• Look for an user with the given username in the database. If not found reply with status 403, otherwise continue.

• Find if the user document has some details like email, display name, etc (in case this is not the first time logging in). If yes reply with the user object, otherwise continue.

• Get user details from the Active Directory and update the user object in the database. Reply with the updated object.

Code:

router.post('/login', (req, res, next) => {

  // capture credentials
  const username = req.body.username;
  const password = req.body.password;
  let user = null;

  // authenticate
  ad.authenticate(username, password)
    .then((success) => {
      if (!success) {
        res.status(401).send(); // authentication failed
        next();
      }
      return User.findOne({ username }).exec();
    })

    .then((found) => {
      if (!found) {
        res.status(403).send(); // unauthorized, no account in DB
        next();
      }
      user = found;
      if (user.displayName) {
        res.status(201).json(user); // all good, return user details
        next();
      }
      // fetch user details from the AD
      return ad.getUserDetails(username, password);
    })

    .then((details) => {
      // update user object with the response details and save
      // ...
      return user.save();
    })

    .then((update) => {
      res.status(201).json(update); // all good, return user object
      next();
    })

    .catch(err => next(err));

});

Now I had this running with callbacks but it was really nested. So I wanted to give Bluebird promises a try, but I have two problems:

• Looks chaotic, any better way to chain the calls and handle responses?

• Whenever I call next() to stop the request after replying, the execution continues to the other .then(). Although the client receives the correct response, in the server log I find that the execution have continued. For example, if there is no account in DB for a given user, the client receives the 403 response but in the server log I see an exception failed to read property displayName of null, because there was no user and it should have stopped in the next() after res.status(403).send();.

Answer 1

To your second question first: there is no way to break/stop a promise chain, unless your callback throw err like

doAsync()
.then(()=>{
    throw 'sth wrong'
})
.then(()=>{
    // code here never runs
})

You can simply try below demos to verify the second callback still runs.

doAsync()
.then(()=>{
    res.end('end')
})
.then(()=>{
    // code here always runs
})


doAsync()
.then(()=>{
    return;
})
.then(()=>{
    // code here always runs
})

To your first question: to use the second parameter in then(), which means reject. And each time split the logic to two parts.

var p = new Promise(function(resolve, reject) {
    return
    ad.auth(username, password).then(()={
        // check if 401 needed. If needed, return reject
        if (dont needed 401 in your logic) 
            resolve(username)
        else
            reject({ msg: 'authentication has failed', status: 401 })
    })
});

p
.then( (username)=>{
    // this only runs when the previous resolves
    return User.findOne({ username }).exec()
}, (data)=>{
    // in fact in your case you dont even have to have the reject callback
    return data
} )


.then( (found)=>{
    return
    new Promise(function(resolve, reject) {
        if (found && /*your logic to determine it's not 403*/)
            resolve(user)
        else
            reject({ msg: 'unauthorized, no account in DB', status: 403 })
    })
} )


.then( (found)=>{
    return
    new Promise(function(resolve, reject) {
        if (found && /*your logic to determine it's not 403*/)
            resolve(user)
        else
            reject({ msg: 'unauthorized, no account in DB', status: 403 })
    })
} )


.then( (user)=>{
    return
    new Promise(function(resolve, reject) {
        if (/*your logic to determine it has the full info*/)
            resolve(user)
        else
            return ad.getUserDetails(username, password)
    })
} )


.then( (user)=>{
    // all is good, do the good logic
}, (data)=>{
    // something wrong, so here you can handle all the reject in one place
    res.send(data) 
} )

Answer 2

To answer your second point, you have to reject your promise after calling next() (or at least return something, otherwise the line after will be executed). Something like

next();
return Promise.reject()

and change your catch so it works if you do not have an error

.catch(err => {
  if (err)     
    next(err)
});

Answer 3

Best use if/else to make clear what branches will execute and which won't:

ad.authenticate(username, password).then((success) => {
  if (!success) {
    res.status(401).send(); // authentication failed
  } else {
    return User.findOne({ username }).exec().then(user => {
      if (!user) {
        res.status(403).send(); // unauthorized, no account in DB
      } else if (user.displayName) {
        res.status(201).json(user); // all good, return user details
      } else {
        // fetch user details from the AD
        return ad.getUserDetails(username, password).then(details => {
          // update user object with the response details and save
          // ...
          return user.save();
        }).then(update => {
          res.status(201).json(update); // all good, return user object
        });
      }
    });
  }
}).then(() => next(), err => next(err));

The nesting of then calls is quite necessary for conditional evaluation, you cannot chain them linearly and "break out" in the middle (other than by throwing exceptions, which is really ugly).

If you don't like all those then callbacks, you can use async/await syntax (possibly with a transpiler - or use Bluebird's Promise.coroutine to emulate it with generator syntax). Your whole code then becomes

router.post('/login', async (req, res, next) => {
  try {
    // authenticate
    const success = await ad.authenticate(req.body.username, req.body.password);
    if (!success) {
      res.status(401).send(); // authentication failed
    } else {
      const user = await User.findOne({ username }).exec();
      if (!user) {
        res.status(403).send(); // unauthorized, no account in DB
      } else if (user.displayName) {
        res.status(201).json(user); // all good, return user details
      } else {
        // fetch user details from the AD
        const details = await ad.getUserDetails(username, password);
        // update user object with the response details and save
        // ...
        const update = await user.save();
        res.status(201).json(update); // all good, return user object
      }
    }
    next(); // let's hope this doesn't throw
  } catch(err) {
    next(err);
  }
});