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

Modularize this node.js express server code for routes and session handlers?

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

Problem

I have express node.js server code using Passport. It receives a user to authenticate it and send him to some data registration form.

Some concerns:

  • My whole routes definition depends upon a MongoDB connection using mongo-db ,



-
but model used by Passport is done through another connection by
mongoose.

-
The main problem is that even though Passport it's doing it's work, I still can go to localhost/registro directly no matter I didn't logged in first.

  • Code already looks a little messy and disorganized.



I care about security. I'd also like to have some information about the user during the session time, but am unsure how to best achieve that.

This is my server.js:

```
var express = require('express')
var mongodb = require('mongodb')
var mongoose = require('mongoose')
var bodyParser = require('body-parser')
var passport = require('passport')
var LocalStrategy = require('passport-local').Strategy;

var app = express()

var BSON = mongodb.BSONPure

app.use(passport.initialize());
app.use(passport.session());

app.use(express.static(__dirname+"/public"))
app.use(bodyParser())

var MongoDBClient = mongodb.MongoClient

mongoose.connect('mongodb://localhost/psicologosTuxtepecDB')

var Schema = mongoose.Schema
var userCredential = new Schema({

username: String,
password: String

}, {
collection: 'members'
})

var userCredentials = mongoose.model('members', userCredential)

passport.serializeUser(function(user, done) {
done(null, user);
})

passport.deserializeUser(function(user, done) {
done(null, user);
})

passport.use(new LocalStrategy(function(username, password, done) {
process.nextTick(function() {
userCredentials.findOne({
'username': username,
}, function(err, user) {
if (err) {
return done(err);
}

if (!user) {
return done(null, false);
}

if (user.password != password) {
return done(null, false);
}

return

Solution

First, read about how .get() can chain multiple callbacks, then read how the sample code of Passport uses ensureAuthenticated as a callback. That should give you enough inspiration.

Furthermore:

  • English only in source code, especially for variables, even for comments



-
Comments, your code could use more of them, and less, this comment is pretty useles:

//Seleccionamos una colección
var psicologosCollection = psicologosTuxtepecDB.collection("psicologos")


  • Semicolons, use them



  • Indenting, use it consistently, consider using jsbeautifier.com, this comment stems mostly from looking at var userCredential



  • user.password != password



-
I would use throw here instead of console.log:

console.log("We've got a connection error, etc.")


  • I would not define the routes inside the callback of .connect()



-
Again, indenting, the following is terrible to read:

psicologosCollection.findOne( 
                                    {'_id':id},
                                    function(error,responseFromDB) { if (error) {response.send(responseFromDB)} response.send(responseFromDB)}
                                )


All in all, I would read up on building modules yourself, I would put all my authentication code in auth.js, all my db code in db.js and all my routes in routes.js.

Code Snippets

//Seleccionamos una colección
var psicologosCollection = psicologosTuxtepecDB.collection("psicologos")
console.log("We've got a connection error, etc.")
psicologosCollection.findOne( 
                                    {'_id':id},
                                    function(error,responseFromDB) { if (error) {response.send(responseFromDB)} response.send(responseFromDB)}
                                )

Context

StackExchange Code Review Q#51213, answer score: 4

Revisions (0)

No revisions yet.