0

I'm adding a very basic login method to a Bookshelf User model in an ExpressJS app, but I can't catch the errors from the rejected promise that the login function in the User model returns. I'm looking at Bookshelf's example login in the docs at http://bookshelfjs.org/#Model-static-extend but that example uses Bluebird, whereas I'm trying to do the same with the built-in ES6 promises.

My login method in the User model:

userModel.js

function login(email, password) {
  return new Promise((resolve, reject) => {
    User.where('email', email)
      .fetch({ require: true })
      .then(user => {
        bcrypt.compare(password, user.get('password'), (err, matched) => {
          if (!matched) return reject(new Error('Password didn\'t match!'));
          resolve(user);
        });
      });
  });

The controller action that implements login and calls User.login from the Bookshelf User model:

usersAuthController.js

function logUserIn(req, res) {
  new User().login(req.body.email, req.body.password)
    .then(user => res.json({ message: 'Login succeeded!' }))
    .catch(User.NotFoundError, () => res.status(404).json({ error: 'User not found!' }) // catch #1
    .catch(err => res.status(401).json({ err: err.message })); // catch #2
}

My intention is that login() can return a rejected promise when Bookshelf's User.fetch method can't find the user with the given email. In that case, the .catch(User.NotFoundError ...) (catch #1) line should catch it and return a 404. I also intend login() to return a rejected Promise when bcrypt determines that the password passed to login() doesn't match the user's password, in which case the "catch-all" (catch #2) below the User.NotFoundError catch statement should return a 401.

If I put in the incorrect password, the logUserIn() controller action in the above code goes to catch #2 with the error message { error: "Cannot set property 'message' of undefined" } instead of the message 'Password didn't match!' message that I rejected in login(). If I put in a nonexistent email, the response is never sent and the error Unhandled rejection CustomError: EmptyResponse is thrown in the console. Only valid input works.

An attempt at fix: catching User.NotFoundError directly in the model instead.

I moved catch #1 to the User model so that the login method now looks like:

userModel.js

function login(email, password) {
  return new Promise((resolve, reject) => {
    User.where('email', email)
      .fetch({ require: true })
      .then(user => {
        bcrypt.compare(password, user.get('password'), (err, matched) => {
          if (!matched) return reject(new Error('Password didn\'t match!'));
          resolve(user);
        });
      })
      .catch(User.NotFoundError, () => reject({ error: 'User not found!' }));
  });

This way, I can catch both errors (incorrect password and nonexistent email) correctly, but this way I can't specify the status code in the controller. If the user with the given email couldn't be found, then it should return a 404, but if the password was incorrect, then it should return a 401, but both errors go to the catch-all (catch #2) in the controller action (which always returns a 401).

To solve this problem, in the User model I could do .catch(User.NotFoundError, () => reject({ name: 'NotFoundError', message: 'User not found!' })) and in the controller action I can check for what kind of error I'm getting with const statusCode = err.name === 'NotFoundError' ? 404 : 401 but that seems really messy and misses the point of having these .catch statements.

Is there a way to catch the User.NotFoundError from the model's login method and any other errors all in logInUser? Why doesn't the setup I had at first work, the one with both catch statements in usersAuthController.js, and what did the Cannot set property 'message' of undefined' and CustomError: EmptyResponse errors mean (does it have something to do with mixing up Bookshelf's Bluebird promises versus the built-in ES6 promises)? What's the best way to handle this?

satray
  • 43
  • 1
  • 7
  • Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572)! You should only promisify `bcrypt.compare` in that `login` function. – Bergi Jul 16 '16 at 15:59

1 Answers1

2

In your initial implementation, you're not propagating any rejections that may be caused by User.fetch(). Also, because User.fetch() already returns a promise, wrapping it with a new promise is a bit of an anti-pattern (although you still need to wrap bcrypt.compare() with a promise, since that only works with callbacks).

Try this:

function login(email, password) {
  return User .where('email', email)
              .fetch({ require: true })
              .then(user => {
                return new Promise((resolve, reject) => {
                  bcrypt.compare(password, user.get('password'), (err, matched) => {
                    if (err)      return reject(err);
                    if (!matched) return reject(new Error('Password didn\'t match!'));
                    resolve(user);
                  });
                })
              });
}
robertklep
  • 198,204
  • 35
  • 394
  • 381
  • Thanks! Just to clarify, usually when should I wrap stuff in a Promise (if I get this, is the point of returning the Promise in `.then` is so that I can chain the `.then`s)? – satray Jul 16 '16 at 08:53
  • 1
    @satray if something already returns a promise, you don't have to wrap it. In this case, because `bcrypt.compare` doesn't return a promise, it gets wrapped (but "as late as possible"). It's always a good thing to return promises (and promise chains), even if you handle `.then()` and `.catch()` already. – robertklep Jul 16 '16 at 10:37