patternjavascriptMinor
User management in Node.js
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
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:
-
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
Then you can
-
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:
-
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 1Code 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 1Context
StackExchange Code Review Q#20547, answer score: 3
Revisions (0)
No revisions yet.