Merge pull request #985 from rockhouse/master

Switch to bcryptjs and make password comparison async

package.json

@@ -40,7 +40,7 @@
     "node": ">=4.2.0"
   },
   "dependencies": {
-    "bcrypt-nodejs": "0.0.3",
+    "bcryptjs": "2.4.3",
     "cheerio": "0.22.0",
     "colors": "1.1.2",
     "commander": "2.9.0",

src/helper.js

@@ -6,7 +6,7 @@ var path = require("path");
 var os = require("os");
 var fs = require("fs");
 var net = require("net");
-var bcrypt = require("bcrypt-nodejs");
+var bcrypt = require("bcryptjs");
 
 var Helper = {
 	config: null,
@@ -125,5 +125,5 @@ function passwordHash(password) {
 }
 
 function passwordCompare(password, expected) {
-	return bcrypt.compareSync(password, expected);
+	return bcrypt.compare(password, expected);
 }

src/server.js

@@ -192,17 +192,20 @@ function init(socket, client) {
 						});
 						return;
 					}
-					if (!Helper.password.compare(old || "", client.config.password)) {
+
+					Helper.password
+						.compare(old || "", client.config.password)
+						.then(matching => {
+							if (!matching) {
 								socket.emit("change-password", {
 									error: "The current password field does not match your account password"
 								});
 								return;
 							}
+							const hash = Helper.password.hash(p1);
 
-					var hash = Helper.password.hash(p1);
+							client.setPassword(hash, success => {
-
+								const obj = {};
-					client.setPassword(hash, function(success) {
-						var obj = {};
 
 								if (success) {
 									obj.success = "Successfully updated your password, all your other sessions were logged out";
@@ -213,6 +216,9 @@ function init(socket, client) {
 
 								socket.emit("change-password", obj);
 							});
+						}).catch(error => {
+							log.error(`Error while checking users password. Error: ${error}`);
+						});
 				}
 			);
 		}
@@ -267,19 +273,22 @@ function localAuth(client, user, password, callback) {
 		return callback(false);
 	}
 
-	var result = Helper.password.compare(password, client.config.password);
+	Helper.password
+		.compare(password, client.config.password)
+		.then(matching => {
+			if (Helper.password.requiresUpdate(client.config.password)) {
+				const hash = Helper.password.hash(password);
 
-	if (result && Helper.password.requiresUpdate(client.config.password)) {
+				client.setPassword(hash, success => {
-		var hash = Helper.password.hash(password);
-
-		client.setPassword(hash, function(success) {
 					if (success) {
 						log.info(`User ${colors.bold(client.name)} logged in and their hashed password has been updated to match new security requirements`);
 					}
 				});
 			}
-
+			callback(matching);
-	return callback(result);
+		}).catch(error => {
+			log.error(`Error while checking users password. Error: ${error}`);
+		});
 }
 
 function ldapAuth(client, user, password, callback) {

test/tests/passwords.js

@@ -10,14 +10,27 @@ describe("Client passwords", function() {
 		// Generated with third party tool to test implementation
 		let comparedPassword = Helper.password.compare(inputPassword, "$2a$11$zrPPcfZ091WNfs6QrRHtQeUitlgrJcecfZhxOFiQs0FWw7TN3Q1oS");
 
-		expect(comparedPassword).to.be.true;
+		return comparedPassword.then(result => {
+			expect(result).to.be.true;
+		});
+	});
+
+	it("wrong hashed password should not match", function() {
+		// Compare against a fake hash
+		let comparedPassword = Helper.password.compare(inputPassword, "$2a$11$zrPPcfZ091WRONGPASSWORDitlgrJcecfZhxOFiQs0FWw7TN3Q1oS");
+
+		return comparedPassword.then(result => {
+			expect(result).to.be.false;
+		});
 	});
 
 	it("freshly hashed password should match", function() {
 		let hashedPassword = Helper.password.hash(inputPassword);
 		let comparedPassword = Helper.password.compare(inputPassword, hashedPassword);
 
-		expect(comparedPassword).to.be.true;
+		return comparedPassword.then((result) => {
+			expect(result).to.be.true;
+		});
 	});
 
 	it("shout passwords should be marked as old", function() {