Expressjs authentication

Question

I have some questions regarding login and sessions. I have this code:

The db query:

login: function(req,callback) { var query = 'SELECT id FROM users WHERE email = "' + req.body.email_login + '" AND password = "' + hashlib.sha1(req.body.password_login) + '" LIMIT 1'; client.query(query, callback); }

The route:

app.post('/login', function(req, res, next) { users.login(req,function(err, results) { if (err) { res.render('index'); } else if (results[0]) { req.session.userdata = results[0]; req.session.is_logged_in = true; res.render('site/news'); } } }

Auth middleware:

var auth = function (req, res, next) { if (req.session.userdata && req.session.is_logged_in === true) { next(); } else { res.redirect('/'); } }

I use db store for the session.

Now my questions are:

1) Is this a safe way to do it? Or should I consider doing it some other way?

2) Say I have this URL /domain/users/1, where the last segment is the user id which is used to fetch user data. And on that view I have a form for changing user data. Is it safe to check if the user id matches the session user id and then show the form?

In the view:

// e.g. get the session.id from dynamichelper if (data.userid === session.userdata.id) { // The form where user can change his data contained within here }

The server is going to use SSL.

Thanks in advance

George

alessioalex · Accepted Answer · 2011-12-09 13:19:25Z

In the db query code, check for req.body.email_login and req.body.password_login to make sure they're not null and that they're strings. Someone could sent an empty response and that will generate an Internal Error on your side.

Also in the route, you might want to log the error and redirect the user to the /500.html page (internal error):

if (err) { console.log(error); res.redirect('500'); } else ...

You shouldn't do this in the view:

if(data.userid === session.userdata.id) { //The form where user can change his data contained within here }

Try instead to achieve this in the model (preferably), make a function for it and pass only one parameter to the view like so:

res.render('view', { loggedIn: true });

The function from the model:

function checkUser(id, session) { return (userid === session.userdata.id); } ... module.exports.checkUser = checkUser;

You can call it from the route like so (for ex):

res.render('view', { loggedIn: model.checkUser(req.body.id, req.session); }

Hi again :) Ok, good advice, but im am not sure I grasp one thing though, why shouldn't I check for id in the view? And how would that look? Small example maybe?
You can do that, it's just more of an advice, to have these kind of stuff structured somewhere else: helper, model. I'll add the example now.
Oh i understand, thats a good way of doing it. More maintainable. Thanks for the advice :)

Tony O'Hagan · Accepted Answer · 2012-12-12 15:55:23Z

2

You might also want to look at http://passportjs.org/

answered Dec 12, 2012 at 15:55

Tony O'Hagan

23k3 gold badges73 silver badges83 bronze badges

Collectives™ on Stack Overflow

Expressjs authentication

2 Answers 2

3 Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Related