Keystone-classic: User signin code appears twice, differently

Created on 23 Jun 2016 · 6Comments · Source: keystonejs/keystone-classic

The code to sign in a user is here:

    User.model.findOne({ email: emailRegExp }).exec(function (err, user) {
        if (user) {
            keystone.callHook(user, 'pre:signin', function (err) {
                if (err) return res.json({ error: 'pre:signin error', detail: err });
                user._.password.compare(req.body.password, function (err, isMatch) {
                    if (isMatch) {
                        session.signinWithUser(user, req, res, function () {
                            keystone.callHook(user, 'post:signin', function (err) {
                                if (err) return res.json({ error: 'post:signin error', detail: err });
                                res.json({ success: true, user: user });
                            });
                        });
    ...

And also here:

    var User = keystone.list(keystone.get('user model'));
    if (typeof lookup.email === 'string' && typeof lookup.password === 'string') {
        // ensure that it is an email, we don't want people being able to sign in by just using "\." and a haphazardly correct password.
        if (!utils.isEmail(lookup.email)) {
            return onFail(new Error('Incorrect email or password'));
        }
        // create regex for email lookup with special characters escaped
        var emailRegExp = new RegExp('^' + utils.escapeRegExp(lookup.email) + '$', 'i');
        // match email address and password
        User.model.findOne({ email: emailRegExp }).exec(function (err, user) {
            if (user) {
                user._.password.compare(lookup.password, function (err, isMatch) {
                    if (!err && isMatch) {
                        postHookedSigninWithUser(user, req, res, onSuccess, onFail);
                    } else {
                        onFail(err || new Error('Incorrect email or password'));
                    }
                });
            } else {
                onFail(err);
            }
        });
    } else {

The code is subtly different. I'm not clear which implementation is correct, but I suspect they aren't both correct. This code needs to be in exactly once place, so when security issues are fixed in one location they are also fixed in the other.

bug

Source

josephg

😕1 👍1

Most helpful comment

Hi @JedWatson @mxstbr
It would be really helpful to have passport integrated as internal authentication middleware https://productpains.com/post/keystonejs/integrate-passport-as-authentication-middleware

mahnunchik on 14 Jul 2016

❤1 👍1

All 6 comments

I wonder if this is a good opportunity to integrate passportjs, or if we should just make sure there is only one implementation right now.

wmertens on 23 Jun 2016

👍2

I'd rather not pull in any more dependencies by default.

+1 for having a keystone-passportjs package that you can drop in to replace the simpler built-in auth

IMO this looks like a bug, it should be centralised logic. Sign in APIs need to be cleaned up anyway for 0.4, I'll pick this up.

JedWatson on 26 Jun 2016

Just realised I never left a comment here. To clarify, the code in the Admin route is newer. I prototyped it there, and it does what we want. The code in the generic session module is older but they are equivalent implementations, with one exception: the generic one doesn't pass the user to the pre:signing hook. It'll require refactoring to implement that. I'd like to review the whole thing with you @josephg to figure out a better way of implementing generic functionality that the route can hook into, but right now this isn't a major issue.

JedWatson on 11 Jul 2016