6
\$\begingroup\$

I'm currently building an application using Passport and bcrypt for user authentication. I've created the registration part. Is there anything I could improve on? I'm still getting the hang of writing in JavaScript and I'm not sure if I'm using arrow functions correctly.

require('dotenv').config();
const express = require('express');
const bodyParser = require('body-parser');
const ejs = require("ejs");
const mongoose = require('mongoose'),
  Schema = mongoose.Schema,
  bcrypt = require('bcrypt'),
  saltRounds = 10;
const session = require('express-session');
const passport = require('passport');
const passportLocalMongoose = require("passport-local-mongoose");
const GoogleStrategy = require('passport-google-oauth20').Strategy;
const findOrCreate = require('mongoose-findorcreate')

const port = process.env.PORT;

const app = express();

app.use(express.static(__dirname + '/public'));
app.set('view engine', 'ejs');
app.use(bodyParser.urlencoded({
  extended: true
}));

app.use(session({
  secret: process.env.SECRET,
  resave: false,
  saveUninitialized: false
}));

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

mongoose.connect('mongodb://localhost:27017/userDB', {
  useNewUrlParser: true,
  useUnifiedTopology: true
});
mongoose.set('useCreateIndex', true);

const userSchema = new Schema({
  username: {
    type: String,
    unique: true,
    lowercase: true,
    required: true
  },
  password: String,
  googleId: String,
});

userSchema.pre('save', function(next) {
  const user = this;

  // only hash the password if it has been modified (or is new)
  if (!user.isModified('password')) return next();

  // generate a salt
  bcrypt.genSalt(saltRounds, (err, salt) => {
    if (err) return next(err);

    // hash the password using our new salt
    bcrypt.hash(req.body.password, saltRounds, (err, hash) => {
      if (err) return next(err);

      // override the cleartext password with the hashed one
      req.body.password = hash;
      next();
    });
  });
});

userSchema.methods.comparePassword = function(candidatePassword, cb) {
  bcrypt.compare(candidatePassword, this.password, function(err, isMatch) {
    if (err) return cb(err);
    cb(null, isMatch);
  });
};

userSchema.plugin(passportLocalMongoose);
userSchema.plugin(findOrCreate);

const User = new mongoose.model('User', userSchema);

passport.serializeUser((user, done) => {
  done(null, user.id);
});

passport.deserializeUser((id, done) => {
  User.findById(id, (err, user) => {
    done(err, user);
  });
});

passport.use(new GoogleStrategy({
    clientID: process.env.CLIENT_ID,
    clientSecret: process.env.CLIENT_SECRET,
    callbackURL: 'http://localhost:3000/auth/google/welcome',
    userProfileURL: 'https://www.googleapis.com/oauth2/v3/userinfo'
  },
  function(accessToken, refreshToken, profile, cb) {
    User.findOrCreate({
      googleId: profile.id
    }, (err, user) => {
      return cb(err, user);
    });
  }
));

app.get('/', (req, res) => {
  res.render('pages/home');
});

app.get('/auth/google', (req, res) => {
  passport.authenticate('google', {
    scope: ['profile']
  })
});

app.get('/auth/google/welcome',
  passport.authenticate('google', {
    failureRedirect: '/login'
  }), (req, res) => {
    // Successful authentication, redirect welcome page.
    res.redirect('/welcome');
  });

app.get('/login', (req, res) => {
  res.render('pages/login');
});

app.get('/register', (req, res) => {
  res.render('pages/register');
});

app.get('/welcome', (req, res) => {
  res.render('pages/welcome');
});

app.get('/welcome', (req, res) => {
  if (req.isAuthenticated()) {
    res.render('/register');
  } else {
    res.redirect('/welcome')
  }
});

app.post('/register', (req, res) => {

  User.register({
    username: req.body.username
  }, req.body.password, (err, user) => {
    if (err) {
      console.log(err);
      res.redirect('/register');
    } else {
      passport.authenticate('local')(req, res, () => {
        res.redirect('/welcome');
      });
    }
  });
});

app.post('/login', (req, res) => {

  const user = new User({
    username: req.body.username,
    password: req.body.password
  });

  req.login(user, (err) => {
    if (err) {
      console.log(err);
    } else {
      passport.authenticate('local')(req, res, () => {
        res.redirect('pages/welcome');
      });
    }
  });
});

app.listen(port, () => {
  console.log("Server has started.");
});
\$\endgroup\$
3
\$\begingroup\$

The use of arrow functions looks okay to me. One advantage to using them is that they can be simplified to a single line but one might hold the opinion that is less readable.

For example:

app.get('/welcome', (req, res) => {
  res.render('pages/welcome');
});

Could be simplified to:

app.get('/welcome', (req, res) => res.render('pages/welcome'));

Note that while the return value isn’t used, the single line arrow function returns the value of the single expression.


There is a common convention in JavaScript and many other languages to put constant names in All_CAPS- so the name saltRounds would be changed to SALT_ROUNDS instead.


There is a single use variable user here:

userSchema.pre('save', function(next) {
  const user = this;

I often see code like this when there is a need to reference the context of this in a different function context, but that doesn’t appear to be the case. If it was the case, arrow functions or Function.bind() could eliminate that need.

Why not just use this in the one spot user is used?


One possibility to reduce the nesting levels is to use async functions- refer to this article for more information.

| improve this answer | |
\$\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.