Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature/migrate-to-bcrypt #237

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,31 @@ The database comes pre-populated with these user accounts created as part of the
npm install
```

5) Set up MongoDB. You can either install MongoDB locally or create a remote instance:
5) Set up MongoDB. You can install MongoDB a variety of ways:

* Using local MongoDB:
1) Install [MongoDB Community Server](https://docs.mongodb.com/manual/administration/install-community/)
2) Start [mongod](http://docs.mongodb.org/manual/reference/program/mongod/#bin.mongod)

* Using local docker instance:
1) Install [Docker](https://docs.docker.com/installation/)
2) Start the docker container first replacing `username` and `password` with your own:
```bash
docker run -d -p 27017:27017 --name mongodb -e MONGO_INITDB_ROOT_USERNAME=username -e MONGO_INITDB_ROOT_PASSWORD=password -e MONGO_INITDB_DATABASE=nodegoat mongo
```

2b) Or leave the environment variables off for immediate setup (unsecure):
```bash
docker run -d -p 27017:27017 --name mongodb mongo
```

3) Set the `MONGODB_URI` environment variable to point to the Docker container:
```
mongodb://username:password@localhost:27017/nodegoat
```

3b) If you followed 2b, then make no changes to the environment.

* Using remote MongoDB instance:
1) [Deploy a MongoDB Atlas free tier cluster](https://docs.atlas.mongodb.com/tutorial/deploy-free-tier-cluster/) (M0 Sandbox)
2) [Enable network access](https://docs.atlas.mongodb.com/security/add-ip-address-to-list/) to the cluster from your current IP address
Expand Down
62 changes: 37 additions & 25 deletions app/data/user-dao.js
Original file line number Diff line number Diff line change
@@ -1,33 +1,33 @@
const bcrypt = require("bcrypt-nodejs");
const bcrypt = require("bcrypt");

/* The number of Salts to generate for the hash, the more rounds, the longer it takes to hash & compare */
const BCRYPT_SALT_ROUNDS = 10;

/* The UserDAO must be constructed with a connected database object */
function UserDAO(db) {

"use strict";

/* If this constructor is called without the "new" operator, "this" points
* to the global object. Log a warning and call it correctly. */
/**
* If this constructor is called without the "new" operator, "this" points
* to the global object. Log a warning and call it correctly.
*/
if (false === (this instanceof UserDAO)) {
console.log("Warning: UserDAO constructor called without 'new' operator");
return new UserDAO(db);
}

const usersCol = db.collection("users");

this.addUser = (userName, firstName, lastName, password, email, callback) => {
this.addUser = async (userName, firstName, lastName, password, email, callback) => {

// Create user document
const user = {
userName,
firstName,
lastName,
benefitStartDate: this.getRandomFutureDate(),
password //received from request param
/*
// Fix for A2-1 - Broken Auth
// Stores password in a safer way using one way encryption and salt hashing
password: bcrypt.hashSync(password, bcrypt.genSaltSync())
*/
password: await this.hashPassword(password, BCRYPT_SALT_ROUNDS)
};

// Add email if set
Expand All @@ -51,28 +51,18 @@ function UserDAO(db) {
const day = (Math.floor(Math.random() * 10) + today.getDay()) % 29;
const month = (Math.floor(Math.random() * 10) + today.getMonth()) % 12;
const year = Math.ceil(Math.random() * 30) + today.getFullYear();
return `${year}-${("0" + month).slice(-2)}-${("0" + day).slice(-2)}`
return `${year}-${("0" + month).slice(-2)}-${("0" + day).slice(-2)}`;
};

this.validateLogin = (userName, password, callback) => {

// Helper function to compare passwords
const comparePassword = (fromDB, fromUser) => {
return fromDB === fromUser;
/*
// Fix for A2-Broken Auth
// compares decrypted password stored in this.addUser()
return bcrypt.compareSync(fromDB, fromUser);
*/
}

this.validateLogin = async (userName, password, callback) => {
// Callback to pass to MongoDB that validates a user document
const validateUserDoc = (err, user) => {
const validateUserDoc = async (err, user) => {

if (err) return callback(err, null);

if (user) {
if (comparePassword(password, user.password)) {
const isValidPassword = await this.comparePassword(password, user.password);
if (isValidPassword) {
callback(null, user);
} else {
const invalidPasswordError = new Error("Invalid password");
Expand Down Expand Up @@ -118,6 +108,28 @@ function UserDAO(db) {
},
(err, data) => err ? callback(err, null) : callback(null, data.value.seq));
};

/**
* Hashes the password and returns the hash.
* Returns a promise as per best-practice for webserver to not block event-loop.
*/
this.hashPassword = (password, saltOrRounds) => {
return password;
// Fix for A2-1 - Broken Auth
// Stores password in a safer way using one way encryption and random salt hashing
return bcrypt.hash(password, saltOrRounds);
};

/**
* Compares the password with the hashed pssword and returns a Boolean.
* Returns a promise as per best-practice for webserver to not block event-loop.
*/
this.comparePassword = (password, hashedPassword) => {
return password === hashedPassword;
// Fix for A2-1 - Broken Auth
// Compares plain text password with the hash stored in the db from this.addUser()
// return bcrypt.compare(password, hashedPassword);
};
}

module.exports = { UserDAO };
8 changes: 4 additions & 4 deletions app/routes/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ function SessionHandler(db) {
});
};

this.handleLoginRequest = (req, res, next) => {
this.handleLoginRequest = async (req, res, next) => {
const {
userName,
password
} = req.body
userDAO.validateLogin(userName, password, (err, user) => {
await userDAO.validateLogin(userName, password, (err, user) => {
const errorMessage = "Invalid username and/or password";
const invalidUserNameErrorMessage = "Invalid username";
const invalidPasswordErrorMessage = "Invalid password";
Expand Down Expand Up @@ -199,7 +199,7 @@ function SessionHandler(db) {

if (validateSignup(userName, firstName, lastName, password, verify, email, errors)) {

userDAO.getUserByUserName(userName, (err, user) => {
userDAO.getUserByUserName(userName, async (err, user) => {

if (err) return next(err);

Expand All @@ -211,7 +211,7 @@ function SessionHandler(db) {
});
}

userDAO.addUser(userName, firstName, lastName, password, email, (err, user) => {
await userDAO.addUser(userName, firstName, lastName, password, email, (err, user) => {

if (err) return next(err);

Expand Down
84 changes: 44 additions & 40 deletions artifacts/db-reset.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,52 +36,50 @@ const USERS_TO_INSERT = [
//"password" : "$2a$10$Tlx2cNv15M0Aia7wyItjsepeA8Y6PyBYaNdQqvpxkIUlcONf1ZHyq", // User2_123
}];

const tryDropCollection = (db, name) => {
return new Promise((resolve, reject) => {
db.dropCollection(name, (err, data) => {
if (!err) {
console.log(`Dropped collection: ${name}`);
}
resolve(undefined);
});
});
}

const parseResponse = (err, res, comm) => {
if (err) {
console.log("ERROR:");
console.log(comm);
console.log(JSON.stringify(err));
console.error("ERROR:");
console.error(comm);
console.error(JSON.stringify(err));
process.exit(1);
}
console.log(comm);
console.log(JSON.stringify(res));
}

const dropDatabase = () => {
MongoClient.connect(db, (err, db) => {
if (err) {
console.error("ERROR: connect");
console.error(JSON.stringify(err));
process.exit(1);
}
console.log("Connected to the database");

// remove existing database (if any)
console.log("Dropping existing database");
db.dropDatabase((err, data) => {
if (!err) {
console.error('Unable to drop the database');
console.error(err);
} else {
console.log('Successfully dropped the database');
return data
}
});
});
};


const seedDatabase = () => {
MongoClient.connect(db, (err, db) => {
if (err) {
console.error("ERROR: connect");
console.error(JSON.stringify(err));
process.exit(1);
}
console.log("Connected to the database");

// Starting here
MongoClient.connect(db, (err, db) => {
if (err) {
console.log("ERROR: connect");
console.log(JSON.stringify(err));
process.exit(1);
}
console.log("Connected to the database");

const collectionNames = [
"users",
"allocations",
"contributions",
"memos",
"counters"
];

// remove existing data (if any), we don't want to look for errors here
console.log("Dropping existing collections");
const dropPromises = collectionNames.map((name) => tryDropCollection(db, name));

// Wait for all drops to finish (or fail) before continuing
Promise.all(dropPromises).then(() => {
const usersCol = db.collection("users");
const allocationsCol = db.collection("allocations");
const countersCol = db.collection("counters");
Expand Down Expand Up @@ -125,11 +123,17 @@ MongoClient.connect(db, (err, db) => {
finalAllocations.forEach(allocation => console.log(JSON.stringify(allocation)));

allocationsCol.insertMany(finalAllocations, (err, data) => {
if (err) {
console.error(err)
process.exit(1);
}
parseResponse(err, data, "allocations.insertMany");
console.log("Database reset performed successfully")
console.log("Database reset performed successfully");
process.exit(0);
});

});
});
});
}

dropDatabase()
seedDatabase()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might cause regression of the Cypress test flakiness that got fixed in #214. seedDatabase isn't waiting for the async work queued by dropDatabase to finish. If seedDatabase manages to connect to the database faster than dropDatabase, then the seed data would end up deleted or incomplete.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, good catch! The previous setup was ignoring the rejection of collections that didn't and still resolving them.
I'll call dropDatabase() within seedDatabse() or wrap them asynchronously.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rcowsill It didn't go as smoothly as I wanted so I decided to revert the commit. It's probably best to keep the db-reset changes out of this PR.

Copy link
Contributor

@rcowsill rcowsill Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous setup was ignoring the rejection of collections that didn't and still resolving them.

Yes that was to avoid errors when running db-reset.js twice. The script doesn't seed the collections "contributions" or "errors", so they'd fail to delete on a second run. That isn't an issue if it's changed to drop the database, but I'm wondering if the script will fail when run on an empty MongoDB instance.

@rcowsill It didn't go as smoothly as I wanted so I decided to revert the commit. It's probably best to keep the db-reset changes out of this PR.

Yeah, that's a good idea 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That isn't an issue if it's changed to drop the database, but I'm wondering if the script will fail when run on an empty MongoDB instance.

Right, that's what I noticed. It was always failing to run for the first time with the CI tests, though it ran fine manually in all cases (as the errors of dropping unknown collections weren't rejecting).

Anyways, we can always create another issue and work on it. 😄

Loading