Merge pull request #1356 from sharelatex/spd-password-complexity

Make password validation more consistent between backend and frontend

GitOrigin-RevId: 6ba729da842bf474cf7e9b5e0b2435db0544737c

services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee

@@ -45,15 +45,22 @@ module.exports = AuthenticationManager =
 			return { message: 'email not valid' }
 		return null
 
+	# validates a password based on a similar set of rules to `complexPassword.js` on the frontend
+	# note that `passfield.js` enforces more rules than this, but these are the most commonly set.
+	# returns null on success, or an error string.
 	validatePassword: (password) ->
-		if !password?
-			return { message: 'password not set' }
-		if (Settings.passwordStrengthOptions?.length?.max? and
-				password.length > Settings.passwordStrengthOptions?.length?.max)
-			return { message: "password is too long" }
-		if (Settings.passwordStrengthOptions?.length?.min? and
-				password.length < Settings.passwordStrengthOptions?.length?.min)
-			return { message: 'password is too short' }
+		return { message: 'password not set' } unless password?
+
+		allowAnyChars = Settings.passwordStrengthOptions?.allowAnyChars == true
+		min = Settings.passwordStrengthOptions?.length?.min || 6
+		max = Settings.passwordStrengthOptions?.length?.max || 72
+
+		# we don't support passwords > 72 characters in length, because bcrypt truncates them
+		max = 72 if max > 72
+
+		return { message: 'password is too short' } unless password.length >= min
+		return { message: 'password is too long' } unless password.length <= max
+		return { message: 'password contains an invalid character' } unless allowAnyChars || AuthenticationManager._passwordCharactersAreValid(password)
 		return null
 
 	setUserPassword: (user_id, password, callback = (error, changed) ->) ->
@@ -111,3 +118,16 @@ module.exports = AuthenticationManager =
 		V1Handler.doPasswordReset v1_user_id, password, (error, reset)->
 			return callback(error) if error?
 			return callback(error, reset)
+
+	_passwordCharactersAreValid: (password) ->
+		digits = Settings.passwordStrengthOptions?.chars?.digits || '1234567890'
+		letters = Settings.passwordStrengthOptions?.chars?.letters || 'abcdefghijklmnopqrstuvwxyz'
+		letters_up = Settings.passwordStrengthOptions?.chars?.letters_up || 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
+		symbols = Settings.passwordStrengthOptions?.chars?.symbols || '@#$%^&*()-_=+[]{};:<>/?!£€.,'
+
+		for charIndex in [0..password.length - 1]
+			return false unless digits.indexOf(password[charIndex]) > -1 or
+				letters.indexOf(password[charIndex]) > -1 or
+				letters_up.indexOf(password[charIndex]) > -1 or
+				symbols.indexOf(password[charIndex]) > -1
+		return true

services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee

@@ -1,6 +1,7 @@
 PasswordResetHandler = require("./PasswordResetHandler")
 RateLimiter = require("../../infrastructure/RateLimiter")
 AuthenticationController = require("../Authentication/AuthenticationController")
+AuthenticationManager = require("../Authentication/AuthenticationManager")
 UserGetter = require("../User/UserGetter")
 UserSessionsManager = require("../User/UserSessionsManager")
 logger = require "logger-sharelatex"
@@ -42,7 +43,7 @@ module.exports =
 
 	setNewUserPassword: (req, res, next)->
 		{passwordResetToken, password} = req.body
-		if !password? or password.length == 0 or !passwordResetToken? or passwordResetToken.length == 0
+		if !password? or password.length == 0 or !passwordResetToken? or passwordResetToken.length == 0 or AuthenticationManager.validatePassword(password?.trim())?
 			return res.sendStatus 400
 		delete req.session.resetToken
 		PasswordResetHandler.setNewUserPassword passwordResetToken?.trim(), password?.trim(), (err, found, user_id) ->

services/web/app/coffee/Features/User/UserController.coffee

@@ -168,12 +168,19 @@ module.exports = UserController =
 				logger.log user: user._id, "changing password"
 				newPassword1 = req.body.newPassword1
 				newPassword2 = req.body.newPassword2
+				validationError = AuthenticationManager.validatePassword(newPassword1)
 				if newPassword1 != newPassword2
 					logger.log user: user, "passwords do not match"
 					res.send
 						message:
 							type:'error'
 							text:'Your passwords do not match'
+				else if validationError?
+					logger.log user: user, validationError.message
+					res.send
+						message:
+							type: 'error'
+							text: validationError.message
 				else
 					logger.log user: user, "password changed"
 					AuthenticationManager.setUserPassword user._id, newPassword1, (error) ->

services/web/public/src/directives/asyncForm.js

@@ -10,7 +10,6 @@
  * decaffeinate suggestions:
  * DS101: Remove unnecessary use of Array.from
  * DS102: Remove unnecessary code created because of implicit returns
- * DS103: Rewrite code to no longer use __guard__
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
@@ -175,85 +174,4 @@ define(['base', 'libs/passfield'], function(App) {
       form: '=for'
     }
   }))
-
-  return App.directive('complexPassword', () => ({
-    require: ['^asyncForm', 'ngModel'],
-
-    link(scope, element, attrs, ctrl) {
-      PassField.Config.blackList = []
-      const defaultPasswordOpts = {
-        pattern: '',
-        length: {
-          min: 6,
-          max: 128
-        },
-        allowEmpty: false,
-        allowAnyChars: true,
-        isMasked: true,
-        showToggle: false,
-        showGenerate: false,
-        showTip: false,
-        showWarn: false,
-        checkMode: PassField.CheckModes.STRICT,
-        chars: {
-          digits: '1234567890',
-          letters: 'abcdefghijklmnopqrstuvwxyz',
-          letters_up: 'ABCDEFGHIJKLMNOPQRSTUVWXYZ',
-          symbols: '@#$%^&*()-_=+[]{};:<>/?!£€.,'
-        }
-      }
-
-      const opts = _.defaults(
-        window.passwordStrengthOptions || {},
-        defaultPasswordOpts
-      )
-      if (opts.length.min === 1) {
-        opts.acceptRate = 0 // this allows basically anything to be a valid password
-      }
-      const passField = new PassField.Field('passwordField', opts)
-
-      const [asyncFormCtrl, ngModelCtrl] = Array.from(ctrl)
-
-      return ngModelCtrl.$parsers.unshift(function(modelValue) {
-        let isValid = passField.validatePass()
-        const email = asyncFormCtrl.getEmail() || window.usersEmail
-        if (!isValid) {
-          scope.complexPasswordErrorMessage = passField.getPassValidationMessage()
-        } else if (email != null && email !== '') {
-          const startOfEmail = __guard__(
-            email != null ? email.split('@') : undefined,
-            x => x[0]
-          )
-          if (
-            modelValue.indexOf(email) !== -1 ||
-            modelValue.indexOf(startOfEmail) !== -1
-          ) {
-            isValid = false
-            scope.complexPasswordErrorMessage =
-              'Password can not contain email address'
-          }
-        }
-        if (opts.length.max != null && modelValue.length === opts.length.max) {
-          isValid = false
-          scope.complexPasswordErrorMessage = `Maximum password length ${
-            opts.length.max
-          } reached`
-        }
-        if (opts.length.min != null && modelValue.length < opts.length.min) {
-          isValid = false
-          scope.complexPasswordErrorMessage = `Password too short, minimum ${
-            opts.length.min
-          }`
-        }
-        ngModelCtrl.$setValidity('complexPassword', isValid)
-        return modelValue
-      })
-    }
-  }))
 })
-
-function __guard__(value, transform) {
-  return typeof value !== 'undefined' && value !== null
-    ? transform(value)
-    : undefined
-}

services/web/public/src/directives/complexPassword.js

@@ -0,0 +1,89 @@
+/* eslint-disable
+    no-undef,
+    max-len
+*/
+define(['base', 'libs/passfield'], function(App) {
+  App.directive('complexPassword', () => ({
+    require: ['^asyncForm', 'ngModel'],
+
+    link(scope, element, attrs, ctrl) {
+      PassField.Config.blackList = []
+      const defaultPasswordOpts = {
+        pattern: '',
+        length: {
+          min: 6,
+          max: 72
+        },
+        allowEmpty: false,
+        allowAnyChars: false,
+        isMasked: true,
+        showToggle: false,
+        showGenerate: false,
+        showTip: false,
+        showWarn: false,
+        checkMode: PassField.CheckModes.STRICT,
+        chars: {
+          digits: '1234567890',
+          letters: 'abcdefghijklmnopqrstuvwxyz',
+          letters_up: 'ABCDEFGHIJKLMNOPQRSTUVWXYZ',
+          symbols: '@#$%^&*()-_=+[]{};:<>/?!£€.,'
+        }
+      }
+
+      const opts = _.defaults(
+        window.passwordStrengthOptions || {},
+        defaultPasswordOpts
+      )
+
+      if (opts.length.min === 1) {
+        // this allows basically anything to be a valid password
+        opts.acceptRate = 0
+      }
+
+      if (opts.length.max > 72) {
+        // there is a hard limit of 71 characters in the password at the backend
+        opts.length.max = 72
+      }
+
+      if (opts.length.max > 0) {
+        // PassField's notion of 'max' is non-inclusive
+        opts.length.max += 1
+      }
+
+      const passField = new PassField.Field('passwordField', opts)
+      const [asyncFormCtrl, ngModelCtrl] = Array.from(ctrl)
+
+      ngModelCtrl.$parsers.unshift(function(modelValue) {
+        let isValid = passField.validatePass()
+        const email = asyncFormCtrl.getEmail() || window.usersEmail
+
+        if (!isValid) {
+          scope.complexPasswordErrorMessage = passField.getPassValidationMessage()
+        } else if (typeof email === 'string' && email !== '') {
+          const startOfEmail = email.split('@')[0]
+          if (
+            modelValue.indexOf(email) !== -1 ||
+            modelValue.indexOf(startOfEmail) !== -1
+          ) {
+            isValid = false
+            scope.complexPasswordErrorMessage =
+              'Password can not contain email address'
+          }
+        }
+        if (opts.length.max != null && modelValue.length >= opts.length.max) {
+          isValid = false
+          scope.complexPasswordErrorMessage = `Maximum password length ${opts
+            .length.max - 1} exceeded`
+        }
+        if (opts.length.min != null && modelValue.length < opts.length.min) {
+          isValid = false
+          scope.complexPasswordErrorMessage = `Password too short, minimum ${
+            opts.length.min
+          }`
+        }
+        ngModelCtrl.$setValidity('complexPassword', isValid)
+        return modelValue
+      })
+    }
+  }))
+})

services/web/public/src/main.js

@@ -40,6 +40,7 @@ define([
   'main/importing',
   'analytics/AbTestingManager',
   'directives/asyncForm',
+  'directives/complexPassword',
   'directives/stopPropagation',
   'directives/focus',
   'directives/equals',

services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee

@@ -115,31 +115,83 @@ describe "AuthenticationManager", ->
 				expect(result.message).to.equal 'email not valid'
 
 	describe "validatePassword", ->
-		it "should return null if valid", ->
-			result = @AuthenticationManager.validatePassword 'banana'
-			expect(result).to.equal null
+		beforeEach ->
+			# 73 characters:
+			@longPassword = '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345678'
 
-		describe "invalid", ->
-			beforeEach ->
-				@settings.passwordStrengthOptions =
-					length:
-						max:10
-						min:6
+		describe "with a null password", ->
+			it "should return an error", ->
+				expect(@AuthenticationManager.validatePassword()).to.eql { message: 'password not set' }
 
-			it "should return validation error object if not set", ->
-				result = @AuthenticationManager.validatePassword()
-				expect(result).to.not.equal null
-				expect(result.message).to.equal 'password not set'
+		describe "password length", ->
+			describe "with the default password length options", ->
+				it "should reject passwords that are too short", ->
+					expect(@AuthenticationManager.validatePassword('')).to.eql { message: 'password is too short' }
+					expect(@AuthenticationManager.validatePassword('foo')).to.eql { message: 'password is too short' }
 
-			it "should return validation error object if too short", ->
-				result = @AuthenticationManager.validatePassword 'dsd'
-				expect(result).to.not.equal null
-				expect(result.message).to.equal 'password is too short'
+				it "should reject passwords that are too long", ->
+					expect(@AuthenticationManager.validatePassword(@longPassword)).to.eql { message: 'password is too long' }
 
-			it "should return validation error object if too long", ->
-				result = @AuthenticationManager.validatePassword 'dsdsadsadsadsadsadkjsadjsadjsadljs'
-				expect(result).to.not.equal null
-				expect(result.message).to.equal 'password is too long'
+				it "should accept passwords that are a good length", ->
+					expect(@AuthenticationManager.validatePassword('l337h4x0r')).to.equal null
+
+			describe "when the password length is specified in settings", ->
+				beforeEach ->
+					@settings.passwordStrengthOptions =
+						length:
+							min: 10
+							max: 12
+
+				it "should reject passwords that are too short", ->
+					expect(@AuthenticationManager.validatePassword('012345678')).to.eql { message: 'password is too short' }
+
+				it "should accept passwords of exactly minimum length", ->
+					expect(@AuthenticationManager.validatePassword('0123456789')).to.equal null
+
+				it "should reject passwords that are too long", ->
+					expect(@AuthenticationManager.validatePassword('0123456789abc')).to.eql { message: 'password is too long' }
+
+				it "should accept passwords of exactly maximum length", ->
+					expect(@AuthenticationManager.validatePassword('0123456789ab')).to.equal null
+
+			describe "when the maximum password length is set to >72 characters in settings", ->
+				beforeEach ->
+					@settings.passwordStrengthOptions =
+						length:
+							max: 128
+
+				it "should still reject passwords > 72 characters in length", ->
+					expect(@AuthenticationManager.validatePassword(@longPassword)).to.eql { message: 'password is too long' }
+
+		describe "allowed characters", ->
+			describe "with the default settings for allowed characters", ->
+				it "should allow passwords with valid characters", ->
+					expect(@AuthenticationManager.validatePassword("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")).to.equal null
+					expect(@AuthenticationManager.validatePassword("1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,")).to.equal null
+
+				it "should not allow passwords with invalid characters", ->
+					expect(@AuthenticationManager.validatePassword("correct horse battery staple")).to.eql { message: 'password contains an invalid character' }
+
+			describe "when valid characters are overridden in settings", ->
+				beforeEach ->
+					@settings.passwordStrengthOptions =
+						chars:
+							symbols: " "
+
+				it "should allow passwords with valid characters", ->
+					expect(@AuthenticationManager.validatePassword("correct horse battery staple")).to.equal null
+
+				it "should disallow passwords with invalid characters", ->
+					expect(@AuthenticationManager.validatePassword("1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,")).to.eql { message: 'password contains an invalid character' }
+
+			describe "when allowAnyChars is set", ->
+				beforeEach ->
+					@settings.passwordStrengthOptions =
+						allowAnyChars: true
+
+				it "should allow any characters", ->
+					expect(@AuthenticationManager.validatePassword("correct horse battery staple")).to.equal null
+					expect(@AuthenticationManager.validatePassword("1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,")).to.equal null
 
 	describe "setUserPassword", ->
 		beforeEach ->

services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee

@@ -19,12 +19,15 @@ describe "PasswordResetController", ->
 			addCount: sinon.stub()
 		@UserSessionsManager =
 			revokeAllUserSessions: sinon.stub().callsArgWith(2, null)
+		@AuthenticationManager =
+			validatePassword: sinon.stub()
 		@PasswordResetController = SandboxedModule.require modulePath, requires:
 			"settings-sharelatex":@settings
 			"./PasswordResetHandler":@PasswordResetHandler
 			"logger-sharelatex": log:->
 			"../../infrastructure/RateLimiter":@RateLimiter
 			"../Authentication/AuthenticationController": @AuthenticationController = {}
+			"../Authentication/AuthenticationManager": @AuthenticationManager
 			"../User/UserGetter": @UserGetter = {}
 			"../User/UserSessionsManager": @UserSessionsManager
 
@@ -131,6 +134,16 @@ describe "PasswordResetController", ->
 				done()
 			@PasswordResetController.setNewUserPassword @req, @res
 
+		it "should return 400 (Bad Request) if the password is invalid", (done)->
+			@req.body.password = "correct horse battery staple"
+			@AuthenticationManager.validatePassword = sinon.stub().returns { message: 'password contains invalid characters' }
+			@PasswordResetHandler.setNewUserPassword.callsArgWith(2)
+			@res.sendStatus = (code)=>
+				code.should.equal 400
+				@PasswordResetHandler.setNewUserPassword.called.should.equal false
+				done()
+			@PasswordResetController.setNewUserPassword @req, @res
+
 		it "should clear the session.resetToken", (done) ->
 			@PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true, @user_id)
 			@res.sendStatus = (code)=>

services/web/test/unit/coffee/User/UserControllerTests.coffee

@@ -47,6 +47,7 @@ describe "UserController", ->
 		@AuthenticationManager =
 			authenticate: sinon.stub()
 			setUserPassword: sinon.stub()
+			validatePassword: sinon.stub()
 		@ReferalAllocator =
 			allocate:sinon.stub()
 		@SubscriptionDomainHandler =
@@ -379,7 +380,6 @@ describe "UserController", ->
 				done()
 			@UserController.changePassword @req, @res
 
-
 		it "it should not set the new password if they do not match", (done)->
 			@AuthenticationManager.authenticate.callsArgWith(2, null, {})
 			@req.body =
@@ -400,3 +400,14 @@ describe "UserController", ->
 				@AuthenticationManager.setUserPassword.calledWith(@user._id, "newpass").should.equal true
 				done()
 			@UserController.changePassword @req, @res
+
+		it "it should not set the new password if it is invalid", (done)->
+			@AuthenticationManager.validatePassword = sinon.stub().returns { message: 'password contains invalid characters' }
+			@AuthenticationManager.authenticate.callsArgWith(2, null, {})
+			@req.body =
+				newPassword1: "correct horse battery staple"
+				newPassword2: "correct horse battery staple"
+			@res.send = =>
+				@AuthenticationManager.setUserPassword.called.should.equal false
+				done()
+			@UserController.changePassword @req, @res