User management in Node.js

Question

I have moved all my web-server code dealing with user management into a single Node.js module that I pasted below. I have spent time polishing it, and churned it through JSHint. I am generally happy with the code, except for the error handling. I find it clunky at best.

How can I improve error handling in the following code? Are there design or API guidelines I have not followed?

// ???
// Deal with the case where an admin no longer has admin rights, but still has a session open

var server = require('./server.js').server;
var User = require('./database.js').User;

// Define the user API
var API = {
    list: [list, 'private'],
    login: [login, 'public'],
    logout: [logout, 'private'],
    add: [add, 'admin'],
    remove: [remove, 'admin'],
    edit: [edit, 'admin']
};

// Attach path handlers
for(var label in API) {
    var denied = 'Permission denied';

    var wrapper = (function (label) {
        return function (req, res) {
            var callback = API[label][0];
            var permission = API[label][1];

            if(!req.session) {
                if(permission !== 'public') {
                    res.send(denied);
                    return;
                }
            } else if((permission === 'admin') && (req.session.rights !== 'Administrator')) {
                res.send(denied);
                return;
            }

            callback(req, res);
        };
    }(label));

    server.post('/user/' + label, wrapper);
}

function list(req, res) {
    User.find({}, null, {sort: {username: 1}}, function (err, users) {
        res.send(users);
    });
}

function login(req, res) {
    var errorMessage = 'Invalid user/password combination';

    find(req.body.username, function(err, user) {
        if(printError(err)) {
            return;
        }

        // Check we have a user
        if(!user) {
            res.send(errorMessage);
            return;
        }

        // Check for a username/password match
        user.comparePassword(req.body.password, function(err, isMatch) {
            if(printError(err)) {
                return;
            }

            if(isMatch) {
                // Populate the session cookie
                req.session.username = user.username;
                req.session.rights = user.rights;

                res.send('OK');
            } else {
                res.send(errorMessage);
            }
        });
    });
}

function logout(req, res) {
    // Clear session
    req.session.username = undefined;
    req.session.rights = undefined;
    res.send('OK');
}

function add(req, res) {
    find(req.body.username, function (err, user) {
        if(user) {
            res.send('The user already exists');
        } else {
            new User(req.body).save(function (err) {
                if(printError(err)) {
                    return;
                }

                res.send('OK');
            });
        }
    });
}

function remove(req, res) {
    find(req.body.username, function (err, user) {
        user.remove(function (err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function edit(req, res) {
    find(req.body.username, function (err, user) {
        if(printError(err)) {
            return;
        }

        // Populate username and rights
        user.username = req.body.user.username;
        user.rights = req.body.user.rights;

        // Only update nonempty passwords
        if(req.body.user.password !== '') {
            user.password = req.body.user.password;
        }

        user.save(function(err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function find(username, callback) {
    User.findOne({
        username: username
    }, callback);
}

function printError(err) {
    if(err) {
        console.error('ERROR: ', err.message);
        // console.trace();
    }

    return err;
}

Answer 1

From a once over

• Not everybody in CR agrees, but I find tabular data best presented in an aligned format:
  

  var API = {                            
    list:   [list  , 'private'],     
    login:  [login , 'public' ],    
    logout: [logout, 'private'],     
    add:    [add   , 'admin'  ],   
    remove: [remove, 'admin'  ],   
    edit:   [edit  , 'admin'  ]      
  };                           
  

• For your error handing, I would mention that using express is very nice for error handling. If you insist on doing it yourself you could do something like

  function handle( f ){
    return function( err ){
      if(err){
        console.error('ERROR: ', err.message);
        return;
      }
      f.apply( this , arguments );
    }
  }
  

  Then you can

  function remove(req, res) {
    find(req.body.username, handle( function (err, user) {
      user.remove( handle( function (err) {
        res.send('OK');
      }));
    }));
  }
  

• For your error message constants, I would declare them all together on top

• I am ambivalent about using an array for your API config instead of an object, if you keep using that, I would suggest you use named constants declared on top:

      var callback   = API[label][CALLBACK];   //Where CALLBACK is earlier declared as 0
      var permission = API[label][PERMISSION]; //Where PERMISSION is earlier declared as 1