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