From 98a0c540040e3ad98f392f6d2cf568fc4550bf8b Mon Sep 17 00:00:00 2001
From: Brian Gough <bjg@network-theory.co.uk>
Date: Fri, 17 Jun 2016 12:11:39 +0100
Subject: [PATCH] use parameter for bcrypt rounds, rehash passwords on login if
 necessary

---
 .../AuthenticationManager.coffee              | 16 +++++++--
 services/web/config/settings.defaults.coffee  |  1 +
 .../AuthenticationManagerTests.coffee         | 33 ++++++++++++++++++-
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee
index 201643e88e..bfcd55855d 100644
--- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee
+++ b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee
@@ -1,8 +1,11 @@
+Settings = require "settings-sharelatex"
 User = require("../../models/User").User
 {db, ObjectId} = require("../../infrastructure/mongojs")
 crypto = require 'crypto'
 bcrypt = require 'bcrypt'
 
+BCRYPT_ROUNDS = Settings?.security?.bcryptRounds or 12
+
 module.exports = AuthenticationManager =
 	authenticate: (query, password, callback = (error, user) ->) ->
 		# Using Mongoose for legacy reasons here. The returned User instance
@@ -15,7 +18,9 @@ module.exports = AuthenticationManager =
 					bcrypt.compare password, user.hashedPassword, (error, match) ->
 						return callback(error) if error?
 						if match
-							callback null, user
+							AuthenticationManager.checkRounds user, user.hashedPassword, password, (err) ->
+								return callback(err) if err?
+								callback null, user
 						else
 							callback null, null
 				else
@@ -24,7 +29,7 @@ module.exports = AuthenticationManager =
 				callback null, null
 
 	setUserPassword: (user_id, password, callback = (error) ->) ->
-		bcrypt.genSalt 7, (error, salt) ->
+		bcrypt.genSalt BCRYPT_ROUNDS, (error, salt) ->
 			return callback(error) if error?
 			bcrypt.hash password, salt, (error, hash) ->
 				return callback(error) if error?
@@ -35,3 +40,10 @@ module.exports = AuthenticationManager =
 					$unset: password: true
 				}, callback)
 
+	checkRounds: (user, hashedPassword, password, callback = (error) ->) ->
+		# check current number of rounds and rehash if necessary
+		currentRounds = bcrypt.getRounds hashedPassword
+		if currentRounds < BCRYPT_ROUNDS
+			AuthenticationManager.setUserPassword user._id, password, callback
+		else
+			callback()
diff --git a/services/web/config/settings.defaults.coffee b/services/web/config/settings.defaults.coffee
index beb933e4c9..1950141518 100644
--- a/services/web/config/settings.defaults.coffee
+++ b/services/web/config/settings.defaults.coffee
@@ -137,6 +137,7 @@ module.exports =
 	# --------
 	security:
 		sessionSecret: sessionSecret
+		bcryptRounds: 14
 
 	httpAuthUsers: httpAuthUsers
 
diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee
index f4d5e72c26..2805527259 100644
--- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee
+++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee
@@ -16,6 +16,7 @@ describe "AuthenticationManager", ->
 					users: {}
 				ObjectId: ObjectId
 			"bcrypt": @bcrypt = {}
+			"settings-sharelatex": { security: { bcryptRounds: 12 } }
 		@callback = sinon.stub()
 
 	describe "authenticate", ->
@@ -31,6 +32,7 @@ describe "AuthenticationManager", ->
 				beforeEach (done) ->
 					@user.hashedPassword = @hashedPassword = "asdfjadflasdf"
 					@bcrypt.compare = sinon.stub().callsArgWith(2, null, true)
+					@bcrypt.getRounds = sinon.stub().returns 12
 					@AuthenticationManager.authenticate email: @email, @unencryptedPassword, (error, user) =>
 						@callback(error, user)
 						done()
@@ -54,6 +56,35 @@ describe "AuthenticationManager", ->
 				it "should not return the user", ->
 					@callback.calledWith(null, null).should.equal true
 
+			describe "when the hashed password matches but the number of rounds is too low", ->
+				beforeEach (done) ->
+					@user.hashedPassword = @hashedPassword = "asdfjadflasdf"
+					@bcrypt.compare = sinon.stub().callsArgWith(2, null, true)
+					@bcrypt.getRounds = sinon.stub().returns 7
+					@AuthenticationManager.setUserPassword = sinon.stub().callsArgWith(2, null)
+					@AuthenticationManager.authenticate email: @email, @unencryptedPassword, (error, user) =>
+						@callback(error, user)
+						done()
+
+				it "should look up the correct user in the database", ->
+					@User.findOne.calledWith(email: @email).should.equal true
+
+				it "should check that the passwords match", ->
+					@bcrypt.compare
+						.calledWith(@unencryptedPassword, @hashedPassword)
+						.should.equal true
+
+				it "should check the number of rounds", ->
+					@bcrypt.getRounds.called.should.equal true
+
+				it "should set the users password (with a higher number of rounds)", ->
+					@AuthenticationManager.setUserPassword
+						.calledWith("user-id", @unencryptedPassword)
+						.should.equal true
+
+				it "should return the user", ->
+					@callback.calledWith(null, @user).should.equal true
+
 		describe "when the user does not exist in the database", ->
 			beforeEach ->
 				@User.findOne = sinon.stub().callsArgWith(1, null, null)
@@ -87,7 +118,7 @@ describe "AuthenticationManager", ->
 
 		it "should hash the password", ->
 			@bcrypt.genSalt
-				.calledWith(7)
+				.calledWith(12)
 				.should.equal true
 			@bcrypt.hash
 				.calledWith(@password, @salt)