Code Review Asked by imnewhere on January 6, 2022
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.");
});
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.
Answered by Sᴀᴍ Onᴇᴌᴀ on January 6, 2022
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP