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

Question

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 done(null, user);
    });
  });
}));

MongoDBClient.connect("mongodb://localhost/psicologosTuxtepecDB", function (error, psicologosTuxtepecDB) {

    if (error) {

        console.log("We've got a connection error, so far we should take this function better for a correct debug")
    }

    else {

        console.log("Connection to psicologosTuxtepecDB has been successful")

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

        app.get('/registro', function(request,response) {

            response.sendfile("public/html/registro.html")
        })

        // Cuando nos hagan una petición HTTP de tipo POST en la ruta psicologos...
        app.post("/psychos", function(request, response) {

            var psychologist = {

                personalData:           request.body._personalData,
                professionalData:       request.body._professionalData,
                professionalInterests:  request.body._professionalInterests
            }


            psicologosCollection.insert(psychologist, function(error, responseFromDB) {

                if (error) {response.send(responseFromDB)}

                console.log("Se ha insertado: "+ JSON.strinfigy(responseFromDB))
                response.send(responseFromDB)
            })
        })

        app.get("/psychos/:id", function(request, response) {

            var id = new BSON.ObjectID(peticion.params.id)

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

        app.get("/psychos", function(request,response) {

            psicologosCollection.find().toArray(function(error,responseFromDB) {
                if (error) {response.send(responseFromDB)}
                response.send(responseFromDB)
            })
        })

        app.post('/login',
          passport.authenticate('local', {
            successRedirect: '/loginSuccess',
            failureRedirect: '/loginFailure'
          })
        )

        app.get('/loginFailure', function(req, res, next) {
            res.redirect('/')
        })

        app.get('registro', function(request, response) {
            response.sendfile('public/html/registro.html')
        })

        app.get('/loginSuccess', function(req, res, next) {
          res.redirect('/registro')

    })      

    app.listen(80, function () {
        console.log("app escuchando en el puerto Maricela fecha de nacimiento DDMM")
    })
}

})

Answer 1

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 <- WOT?? Do not store password to compare them later, read the API, this is terrible

• 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.

Answer 2

I'd rewrite the code into the following modules:

server.js

var http = require('http');
var app = require('./app');

http.createServer(app).listen(3000, function(){
  console.log('Express server listening on port ' + 3000);
  console.log("Maricela app listening on port birthdate MMDD")
});

app.js

module.exports = function() {
    var express = require('express');
    var bodyParser = require('body-parser');
    var LocalStrategy = require('passport-local').Strategy;

    var app = express();

    var passport = require('./auth');
    app.use(passport.initialize());
    app.use(passport.session());

    app.use(express.static(__dirname + "/public"));
    // app.use(bodyParser()); //Now deprecated
    app.use(bodyParser.urlencoded({
        extended: true
    }));
    app.use(bodyParser.json());

    app.get('/registro', function(request, response) {
        response.sendfile("public/html/registro.html");
    });

    var psychos = require('./psychos');
    // When we make a HTTP POST request to the psychologists route ...
    app.post("/psychos", psychos.save);

    app.get("/psychos/:id", psychos.findOne);

    app.get("/psychos", psychos.list);

    app.post('/login',
        passport.authenticate('local', {
            successRedirect: '/loginSuccess',
            failureRedirect: '/loginFailure'
        })
    );

    app.get('/loginFailure', function(req, res, next) {
        res.redirect('/')
    });

    app.get('registro', function(request, response) {
        response.sendfile('public/html/registro.html')
    });

    app.get('/loginSuccess', function(req, res, next) {
        res.redirect('/registro')
    });

};

auth.js

//START:------------MONGOOSE------------------
    var mongoose = require('mongoose');
    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);
//END:------------MONGOOSE--------------------

exports.passport = function() {
    var passport = require('passport');
    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 done(null, user);
            });
        });
    }));
};

psychos.js

var Server = require('mongodb').Server;
var BSON = require('mongodb').BSONPure;
var Db = require('mongodb').Db;
var db = new Db('psicologosTuxtepecDB', new Server('localhost', 27017), {safe:false});

// Select a collection
var psicologosCollection = null;

exports.save = function(request, response) {
    var psychologist = {
        personalData: request.body._personalData,
        professionalData: request.body._professionalData,
        professionalInterests: request.body._professionalInterests
    };

    db.open(function(err, db) {
        psicologosCollection = db.collection("psicologos");
    });

    psicologosCollection.insert(psychologist, function(error, responseFromDB) {
        if (error) {
            response.send(responseFromDB)
        }
        console.log("It has been inserted: " + JSON.strinfigy(responseFromDB))
        response.send(responseFromDB)
    });
};

exports.findOne = function(request, response) {
    var id = new BSON.ObjectID(peticion.params.id);
    db.open(function(err, db) {
        psicologosCollection = db.collection("psicologos");
    });

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

exports.list = function(request, response) {
    db.open(function(err, db) {
        psicologosCollection = db.collection("psicologos");
    });

    psicologosCollection.find().toArray(function(error, responseFromDB) {
        if (error) {
            response.send(responseFromDB)
        }
        response.send(responseFromDB)
    })
};