diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee index e2531332f2..9024cd845a 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationManager.coffee +++ b/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 diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee index 8bb6d3907d..6cadf03853 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee +++ b/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) -> diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 70d61a91b2..85af0a638c 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/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) -> diff --git a/services/web/public/src/directives/asyncForm.js b/services/web/public/src/directives/asyncForm.js index cc2a735929..c8824459fd 100644 --- a/services/web/public/src/directives/asyncForm.js +++ b/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 -} diff --git a/services/web/public/src/directives/complexPassword.js b/services/web/public/src/directives/complexPassword.js new file mode 100644 index 0000000000..ff6fe6e4f2 --- /dev/null +++ b/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 + }) + } + })) +}) diff --git a/services/web/public/src/main.js b/services/web/public/src/main.js index ee1bc3b165..92570318be 100644 --- a/services/web/public/src/main.js +++ b/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', diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee index be632a64fb..67f92f3a83 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/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 -> diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee index 377cd0884d..0af316a349 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee +++ b/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)=> diff --git a/services/web/test/unit/coffee/User/UserControllerTests.coffee b/services/web/test/unit/coffee/User/UserControllerTests.coffee index becc26ef69..d17d079387 100644 --- a/services/web/test/unit/coffee/User/UserControllerTests.coffee +++ b/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