Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have moved all my webserver code dealing with user management into a single Node.JS module that I paste 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;
}
share|improve this question

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.