User-logging authentication

Question

I'm little frustrated because I don't know how well I can refactor this script. I wrote the authentication function but it's too complicated, maybe too large in one function, and seems like spaghetti code. How will I be able to make this a little better?

I'm using Restify framework for NodeJS. There are a function that checks if a user is logged. There is a session hash (pnp_access_token) that I need check with another webservice, but I don't need check all times, only the first, so I'm using a cache. I'm using Redis to set a cache and only check the first time.

I don't want refactor business rules. What can be done about this?

"use strict";
var errorResponse = require('../errorResponse'),
    paynopain = require('../../core/paynopain'),
    User = require('../../core/User'),
    cache = require('../../core/cache/cache'),
    validateValues = require('../validateValues'),
    API_PATH = 'v1/';

function authenticate(req, res, next) {

    if (req.url.indexOf(API_PATH) == 0){

        var pnpUserId;
        var validated = validateValues(req, ['pnp_access_token']);
        if(validated.error){
            return next(validated.error);
        }
        var pnpAccessToken = req.params.pnp_access_token;

        cache.read('pnpToken:' + pnpAccessToken)
            .then(function(userId){
                if(userId){
                    storeUserIdInRequest(req, next, userId);
                }else{
                    var user = new User(req.logger);
                    return paynopain.getIdFromAccessToken(pnpAccessToken)
                        .then(function(pnpUserIdResult){
                            pnpUserId = pnpUserIdResult;
                            return user.findByPnpID(pnpUserIdResult);
                        })
                        .then(function(userData){

                            if(userData._id){
                                return storeUserIdInRequest(req, next, userData._id.toString());
                            }else{
                                var newUser = {
                                    pnpId: pnpUserId,
                                    radius: 1000
                                };
                                return user.create(newUser)
                                    .then(function(userData){
                                        return storeUserIdInRequest(req, next, userData._id);
                                    });
                            }

                        });
                }
            })
            .fail(function(e){

                if(e.message === 'invalid_grant'){
                    next(errorResponse.unauthorized());
                }else{
                    req.logger.log('error', 'Error authenticating', {
                        error: {
                            message: e.message,
                            stack: e.stack
                        }
                    });
                    next(errorResponse.internalError());
                }

            });

    }else{
        next();
    }

}

function storeUserIdInRequest(req, next, userId){

    var oneHourInSeconds =  60 * 60;
    cache.write('pnpToken:' + req.params.pnp_access_token, userId, {
        expire: oneHourInSeconds
    })
        .then(function(){
            req.userId = userId;
            req.logger.setExtra({
                userId: userId
            });
            next();
        });

}

function init(server){
    server.use(authenticate);
    require('./configuration/routes')(API_PATH, server);
    require('./products/routes')(API_PATH, server);
    require('./tracking/routes')(API_PATH, server);
    require('./lists/routes')(API_PATH, server);
}
module.exports = init;

Answer 1

Interesting question,

I think the first thing to do is read this.

You could replace this:

if (req.url.indexOf(API_PATH) == 0){
  //Lots of code
}else{
  next();
}

To

if (req.url.indexOf(API_PATH) != 0){
  next();
  return;
}
//Lots of code,with one less level of indentation

This early exit makes the rest of the code look less busy.

Furthermore, in cases like this

req.logger.log('error', 'Error authenticating', {
  error: {
    message: e.message,
    stack: e.stack
  }
});

You should notice that the object you build has the exact same property names as in the e object, you might as well do the the far cleaner

req.logger.log('error', 'Error authenticating', { error: e });

On a side note, I would also log invalid log-ins, this way you can detect when someone might be trying to hack in to your system.

You can remove some of the arrowness here as well:

                    .then(function(pnpUserIdResult){
                        pnpUserId = pnpUserIdResult;
                        return user.findByPnpID(pnpUserIdResult);
                    })
                    .then(function(userData){

                        if(userData._id){
                            return storeUserIdInRequest(req, next, userData._id.toString());
                        }else{
                            var newUser = {
                                pnpId: pnpUserId,
                                radius: 1000
                            };
                            return user.create(newUser)
                                .then(function(userData){
                                    return storeUserIdInRequest(req, next, userData._id);
                                });
                        }

I am not sure why you need 2 then's, so I will just merge them.

                    .then(function(pnpUserIdResult){
                        pnpUserId = pnpUserIdResult;
                        var userData = user.findByPnpID(pnpUserIdResult);
                        if(userData._id){
                            return storeUserIdInRequest(req, next, userData._id.toString());
                        }
                        return user.create({
                            pnpId: pnpUserId,
                            radius: 1000
                        }).then(function(userData){
                            return storeUserIdInRequest(req, next, userData._id);
                        });

From a naming perspective;

• I get really annoyed by the pnp acronym all over the place, it should be established on top that this for pnp and then you can just go with user and userId or user.id
• paynopain <- this is a meaningless name, why would it have getIdFromAccessToken?
• var user = new User(req.logger);, whatever user is here, it does not represent a user since you only use it for findByPnpID..
• radius: 1000, what does radius mean for a user ? What does 1000 mean ?

Finally, for storeUserIdInRequest (which lies about what it does), you dont have to wait for the cache to be set, you can simply (and this should be faster) do

function storeUserIdInRequest(req, next, userId){
  cache.write('pnpToken:' + req.params.pnp_access_token, userId, {
      expire: 60 * 60 //1 hour in seconds
  });
  req.userId = userId;
  req.logger.setExtra({
    userId: userId
  });
  next();
}