4
\$\begingroup\$

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")
    })
}

})
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I'd like to ask you. How can I properly module into js files. I tried, did require('file.js') but wasn't sure how to invoke them \$\endgroup\$ – diegoaguilar May 20 '14 at 22:09
  • 1
    \$\begingroup\$ Read howtonode.org/creating-custom-modules, it is pretty straight forward \$\endgroup\$ – konijn May 21 '14 at 12:01
1
\$\begingroup\$

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)
    })
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.