Welcome to the Treehouse Community

The Treehouse Community is a meeting place for developers, designers, and programmers of all backgrounds and skill levels to get support. Collaborate here on code errors or bugs that you need feedback on, or asking for an extra set of eyes on your latest project. Join thousands of Treehouse students and alumni in the community today. (Note: Only Treehouse students can comment or ask questions, but non-students are welcome to browse our conversations.)

Looking to learn something new?

Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and a supportive community. Start your free trial today.

JavaScript User Authentication With Express and Mongo Sessions and Cookies Authenticating the Username and Password

Brendan Moran
Brendan Moran
14,052 Points

A solution for more specific error handling

I immediately got confused when Dave manually created another error in the index.js in the callback of the POST/login route. This renders the "user not found" error from user.js superflous. We'll never see that error, because the POST/login callback is just testing for any error and then saying, "Wrong email or password." I also don't like the user experience of saying "Wrong email or password." Which one was wrong? Mostly, it just doesn't feel clean to have one error handler in user.js, and another in the callback of the POST/login route.

Here is what I came up with; I personally just like the flow of this a lot more.

In user.js:

UserSchema.statics.authenticate = function (email, password, callback) {
    User.findOne( { email: email })
        .exec( (err, user) => { //when findOne has found one (or not), execute this function.
            if (err) return callback(err); //if there is an error with the query operation itself
            else if (!user) {
                let error = new Error('User not found with that email address.');
                error.status = 401;
                return callback(error);
            }
            bcrypt.compare(password, user.password, (err, result) => {
                if (result === true) return callback(null, user);
                if (result === false) {
                    let error = new Error("Incorrect password.");
                    error.status = 401;
                    return callback(error);
                }
            })
        });
}

I explicitly test true and false just for semantic/readability reasons. Also, I tried simply returning callback(err) after the the first if block (no test, no creating a new error) and this didn't work; not sure why. Anyway, I would prefer to create a new error and simply say "Incorrect password" than use whatever error bcrypt.compare would send along.

In index.js:

//POST /login
router.post('/login', (req, res, next) => {
  if (req.body.email && req.body.password) {
    User.authenticate(req.body.email, req.body.password, (err, user) => {
      if (err) return next(err);
      req.session.id = user._id;
      return res.redirect("/profile");
    });
  } else {
    var err = new Error('Email and password are required!');
    err.status = 401;
    return next(err);
  }
});

It works, and it gives me a nice specific error saying whether the email wasn't found, or the password was wrong.

1 Answer

Hello,

While I do see that you have come up with an alternative solution that, by a simple overview, appears that it will work well, I would like to point out something with the 'usability' statement you have made.

The reason behind simply saying "Wrong email or password", giving the user an unhelpful user experience is for security. You will generally find nowadays, that many websites will not specifically tell you whether you have incorrectly entered a password or username/email address. By telling the user that the password does not match, it let's them know that the username/email does. Therefore, they know they can attempt again to 'guess' by trying or programmatically badger the server until it either crashes or they are allowed in. (If you haven't guessed by now, I am talking about a malicious user). The next thought may be to only allow them to try a certain amount of times. This is helpful but is generally only temporary.

As for the username/email, this is generally an email. Saying the password is incorrect tells a malicious user the location at which an email lives, where they can go attack another site. This also gives way to a malicious user to have user enumeration, a list of valid users to which they can potentially try to enter the account of on either the site they found it out or the one associated with their email. Unfortunately, since most places have an email as a username, if a malicious user gains access to one account, they might be able to gain access to multiple (being that many users use the same email & password)

There are many articles across the internet about login/authentication security, so take a search on Google and learn more, maybe even apply it to what you have written here.

Just wanted to share some insight, and again, kudos on venturing off to write you own implementation, normally the developers that take their own initiative make the best developers!

Brendan Moran
Brendan Moran
14,052 Points

Thanks! This is really good to know. That is exactly the kind of information I was hoping to garner when I published this. (I figured, "If I'm wrong about this, someone will say something.") Dave is an experienced developer, so I thought there must be some reason for what he was doing that I don't know. (But I wish he said it!) I still don't like that we make an error handler in user.js that we end up never seeing. :) In that case, I guess we can just get rid of the "user not found" error in user.js and only give the generic "Email or Password not found" in the route.