HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

User management in Node.js

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managementusernode

Problem

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

Solution

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

Code Snippets

var API = {                            
  list:   [list  , 'private'],     
  login:  [login , 'public' ],    
  logout: [logout, 'private'],     
  add:    [add   , 'admin'  ],   
  remove: [remove, 'admin'  ],   
  edit:   [edit  , 'admin'  ]      
};
function handle( f ){
  return function( err ){
    if(err){
      console.error('ERROR: ', err.message);
      return;
    }
    f.apply( this , arguments );
  }
}
function remove(req, res) {
  find(req.body.username, handle( function (err, user) {
    user.remove( handle( function (err) {
      res.send('OK');
    }));
  }));
}
var callback   = API[label][CALLBACK];   //Where CALLBACK is earlier declared as 0
    var permission = API[label][PERMISSION]; //Where PERMISSION is earlier declared as 1

Context

StackExchange Code Review Q#20547, answer score: 3

Revisions (0)

No revisions yet.