From 6737637b39731d0c3e0664ba46d4e8f8d93158bc Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Wed, 2 Oct 2019 09:07:28 -0500 Subject: [PATCH] Merge pull request #2190 from overleaf/as-invalid-password-errors Throw Error instead of plain object if email/password is invalid GitOrigin-RevId: 2a1b040b07834064d547cef7890676ca014ec0ae --- .../Authentication/AuthenticationErrors.js | 9 + .../Authentication/AuthenticationManager.js | 38 ++- .../AuthenticationManagerTests.js | 304 ++++++++++-------- 3 files changed, 200 insertions(+), 151 deletions(-) create mode 100644 services/web/app/src/Features/Authentication/AuthenticationErrors.js diff --git a/services/web/app/src/Features/Authentication/AuthenticationErrors.js b/services/web/app/src/Features/Authentication/AuthenticationErrors.js new file mode 100644 index 0000000000..355dc87fdb --- /dev/null +++ b/services/web/app/src/Features/Authentication/AuthenticationErrors.js @@ -0,0 +1,9 @@ +const OError = require('@overleaf/o-error') + +class InvalidEmailError extends OError {} +class InvalidPasswordError extends OError {} + +module.exports = { + InvalidEmailError, + InvalidPasswordError +} diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index a74c2a24b4..48aa36afec 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -5,6 +5,10 @@ const { db, ObjectId } = require('../../infrastructure/mongojs') const bcrypt = require('bcrypt') const EmailHelper = require('../Helpers/EmailHelper') const V1Handler = require('../V1/V1Handler') +const { + InvalidEmailError, + InvalidPasswordError +} = require('./AuthenticationErrors') const BCRYPT_ROUNDS = Settings.security.bcryptRounds || 12 const BCRYPT_MINOR_VERSION = Settings.security.bcryptMinorVersion || 'a' @@ -55,7 +59,7 @@ module.exports = AuthenticationManager = { validateEmail(email) { const parsed = EmailHelper.parseEmail(email) if (!parsed) { - return { message: 'email not valid' } + return new InvalidEmailError({ message: 'email not valid' }) } return null }, @@ -65,7 +69,10 @@ module.exports = AuthenticationManager = { // returns null on success, or an error string. validatePassword(password) { if (password == null) { - return { message: 'password not set' } + return new InvalidPasswordError({ + message: 'password not set', + info: { code: 'not_set' } + }) } let allowAnyChars, min, max @@ -86,16 +93,25 @@ module.exports = AuthenticationManager = { } if (password.length < min) { - return { message: 'password is too short' } + return new InvalidPasswordError({ + message: 'password is too short', + info: { code: 'too_short' } + }) } if (password.length > max) { - return { message: 'password is too long' } + return new InvalidPasswordError({ + message: 'password is too long', + info: { code: 'too_long' } + }) } if ( !allowAnyChars && !AuthenticationManager._passwordCharactersAreValid(password) ) { - return { message: 'password contains an invalid character' } + return new InvalidPasswordError({ + message: 'password contains an invalid character', + info: { code: 'invalid_character' } + }) } return null }, @@ -128,9 +144,9 @@ module.exports = AuthenticationManager = { }, setUserPasswordInV2(userId, password, callback) { - const validation = this.validatePassword(password) - if (validation) { - return callback(validation.message) + const validationError = this.validatePassword(password) + if (validationError) { + return callback(validationError) } this.hashPassword(password, function(error, hash) { if (error) { @@ -159,9 +175,9 @@ module.exports = AuthenticationManager = { }, setUserPasswordInV1(v1UserId, password, callback) { - const validation = this.validatePassword(password) - if (validation) { - return callback(validation.message) + const validationError = this.validatePassword(password) + if (validationError) { + return callback(validationError.message) } V1Handler.doPasswordReset(v1UserId, password, function(error, reset) { diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 6a56db3402..e45c6f54d0 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -1,27 +1,13 @@ -/* eslint-disable - handle-callback-err, - max-len, - no-return-assign, - no-unused-vars, - no-useless-escape, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const chai = require('chai') -const should = chai.should() +const SandboxedModule = require('sandboxed-module') +const { ObjectId } = require('mongojs') +const AuthenticationErrors = require('../../../../app/src/Features/Authentication/AuthenticationErrors') + +chai.should() const { expect } = chai const modulePath = '../../../../app/src/Features/Authentication/AuthenticationManager.js' -const SandboxedModule = require('sandboxed-module') -const events = require('events') -const { ObjectId } = require('mongojs') -const Errors = require('../../../../app/src/Features/Errors/Errors') describe('AuthenticationManager', function() { beforeEach(function() { @@ -41,10 +27,11 @@ describe('AuthenticationManager', function() { bcrypt: (this.bcrypt = {}), 'settings-sharelatex': this.settings, '../V1/V1Handler': (this.V1Handler = {}), - '../User/UserGetter': (this.UserGetter = {}) + '../User/UserGetter': (this.UserGetter = {}), + './AuthenticationErrors': AuthenticationErrors } }) - return (this.callback = sinon.stub()) + this.callback = sinon.stub() }) describe('with real bcrypt', function() { @@ -55,8 +42,8 @@ describe('AuthenticationManager', function() { this.bcrypt.genSalt = bcrypt.genSalt this.bcrypt.hash = bcrypt.hash // Hash of 'testpassword' - return (this.testPassword = - '$2a$12$zhtThy3R5tLtw5sCwr5XD.zhPENGn4ecjeMcP87oYSYrIICFqBpei') + this.testPassword = + '$2a$12$zhtThy3R5tLtw5sCwr5XD.zhPENGn4ecjeMcP87oYSYrIICFqBpei' }) describe('authenticate', function() { @@ -65,39 +52,35 @@ describe('AuthenticationManager', function() { _id: 'user-id', email: (this.email = 'USER@sharelatex.com') } - return (this.User.findOne = sinon - .stub() - .callsArgWith(1, null, this.user)) + this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) }) describe('when the hashed password matches', function() { beforeEach(function(done) { this.unencryptedPassword = 'testpassword' this.user.hashedPassword = this.testPassword - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, (error, user) => { this.callback(error, user) - return done() + done() } ) }) it('should look up the correct user in the database', function() { - return this.User.findOne - .calledWith({ email: this.email }) - .should.equal(true) + this.User.findOne.calledWith({ email: this.email }).should.equal(true) }) it('should return the user', function() { - return this.callback.calledWith(null, this.user).should.equal(true) + this.callback.calledWith(null, this.user).should.equal(true) }) }) describe('when the encrypted passwords do not match', function() { beforeEach(function() { - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, 'notthecorrectpassword', this.callback @@ -105,7 +88,7 @@ describe('AuthenticationManager', function() { }) it('should not return the user', function() { - return this.callback.calledWith(null, null).should.equal(true) + this.callback.calledWith(null, null).should.equal(true) }) }) }) @@ -116,36 +99,36 @@ describe('AuthenticationManager', function() { _id: '5c8791477192a80b5e76ca7e', email: (this.email = 'USER@sharelatex.com') } - return (this.db.users.update = sinon + this.db.users.update = sinon .stub() - .callsArgWith(2, null, { nModified: 1 })) + .callsArgWith(2, null, { nModified: 1 }) }) it('should not produce an error', function(done) { - return this.AuthenticationManager.setUserPasswordInV2( + this.AuthenticationManager.setUserPasswordInV2( this.user._id, 'testpassword', (err, updated) => { expect(err).to.not.exist expect(updated).to.equal(true) - return done() + done() } ) }) it('should set the hashed password', function(done) { - return this.AuthenticationManager.setUserPasswordInV2( + this.AuthenticationManager.setUserPasswordInV2( this.user._id, 'testpassword', - (err, updated) => { + err => { expect(err).to.not.exist const { hashedPassword } = this.db.users.update.lastCall.args[1].$set expect(hashedPassword).to.exist expect(hashedPassword.length).to.equal(60) - expect(hashedPassword).to.match(/^\$2a\$12\$[a-zA-Z0-9\/.]{53}$/) - return done() + expect(hashedPassword).to.match(/^\$2a\$12\$[a-zA-Z0-9/.]{53}$/) + done() } ) }) @@ -160,9 +143,7 @@ describe('AuthenticationManager', function() { email: (this.email = 'USER@sharelatex.com') } this.unencryptedPassword = 'banana' - return (this.User.findOne = sinon - .stub() - .callsArgWith(1, null, this.user)) + this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) }) describe('when the hashed password matches', function() { @@ -170,36 +151,34 @@ describe('AuthenticationManager', function() { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true) this.bcrypt.getRounds = sinon.stub().returns(12) - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, (error, user) => { this.callback(error, user) - return done() + done() } ) }) it('should look up the correct user in the database', function() { - return this.User.findOne - .calledWith({ email: this.email }) - .should.equal(true) + this.User.findOne.calledWith({ email: this.email }).should.equal(true) }) it('should check that the passwords match', function() { - return this.bcrypt.compare + this.bcrypt.compare .calledWith(this.unencryptedPassword, this.hashedPassword) .should.equal(true) }) it('should return the user', function() { - return this.callback.calledWith(null, this.user).should.equal(true) + this.callback.calledWith(null, this.user).should.equal(true) }) }) describe('when the encrypted passwords do not match', function() { beforeEach(function() { - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, this.callback @@ -207,7 +186,7 @@ describe('AuthenticationManager', function() { }) it('should not return the user', function() { - return this.callback.calledWith(null, null).should.equal(true) + this.callback.calledWith(null, null).should.equal(true) }) }) @@ -219,40 +198,38 @@ describe('AuthenticationManager', function() { this.AuthenticationManager.setUserPassword = sinon .stub() .callsArgWith(2, null) - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, (error, user) => { this.callback(error, user) - return done() + done() } ) }) it('should look up the correct user in the database', function() { - return this.User.findOne - .calledWith({ email: this.email }) - .should.equal(true) + this.User.findOne.calledWith({ email: this.email }).should.equal(true) }) it('should check that the passwords match', function() { - return this.bcrypt.compare + this.bcrypt.compare .calledWith(this.unencryptedPassword, this.hashedPassword) .should.equal(true) }) it('should check the number of rounds', function() { - return this.bcrypt.getRounds.called.should.equal(true) + this.bcrypt.getRounds.called.should.equal(true) }) it('should set the users password (with a higher number of rounds)', function() { - return this.AuthenticationManager.setUserPassword + this.AuthenticationManager.setUserPassword .calledWith('user-id', this.unencryptedPassword) .should.equal(true) }) it('should return the user', function() { - return this.callback.calledWith(null, this.user).should.equal(true) + this.callback.calledWith(null, this.user).should.equal(true) }) }) @@ -265,28 +242,28 @@ describe('AuthenticationManager', function() { this.AuthenticationManager.setUserPassword = sinon .stub() .callsArgWith(2, null) - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencryptedPassword, (error, user) => { this.callback(error, user) - return done() + done() } ) }) it('should not check the number of rounds', function() { - return this.bcrypt.getRounds.called.should.equal(false) + this.bcrypt.getRounds.called.should.equal(false) }) it('should not set the users password (with a higher number of rounds)', function() { - return this.AuthenticationManager.setUserPassword + this.AuthenticationManager.setUserPassword .calledWith('user-id', this.unencryptedPassword) .should.equal(false) }) it('should return the user', function() { - return this.callback.calledWith(null, this.user).should.equal(true) + this.callback.calledWith(null, this.user).should.equal(true) }) }) }) @@ -294,7 +271,7 @@ describe('AuthenticationManager', function() { describe('when the user does not exist in the database', function() { beforeEach(function() { this.User.findOne = sinon.stub().callsArgWith(1, null, null) - return this.AuthenticationManager.authenticate( + this.AuthenticationManager.authenticate( { email: this.email }, this.unencrpytedPassword, this.callback @@ -302,7 +279,7 @@ describe('AuthenticationManager', function() { }) it('should not return a user', function() { - return this.callback.calledWith(null, null).should.equal(true) + this.callback.calledWith(null, null).should.equal(true) }) }) }) @@ -313,21 +290,23 @@ describe('AuthenticationManager', function() { const result = this.AuthenticationManager.validateEmail( 'foo@example.com' ) - return expect(result).to.equal(null) + expect(result).to.equal(null) }) }) describe('invalid', function() { it('should return validation error object for no email', function() { const result = this.AuthenticationManager.validateEmail('') - expect(result).to.not.equal(null) - return expect(result.message).to.equal('email not valid') + expect(result).to.an.instanceOf(AuthenticationErrors.InvalidEmailError) + expect(result.message).to.equal('email not valid') }) it('should return validation error object for invalid', function() { const result = this.AuthenticationManager.validateEmail('notanemail') - expect(result).to.not.equal(null) - return expect(result.message).to.equal('email not valid') + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidEmailError + ) + expect(result.message).to.equal('email not valid') }) }) }) @@ -335,37 +314,54 @@ describe('AuthenticationManager', function() { describe('validatePassword', function() { beforeEach(function() { // 73 characters: - return (this.longPassword = - '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345678') + this.longPassword = + '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef012345678' }) describe('with a null password', function() { it('should return an error', function() { - return expect(this.AuthenticationManager.validatePassword()).to.eql({ - message: 'password not set' - }) + const result = this.AuthenticationManager.validatePassword() + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal('password not set') + expect(result.info.code).to.equal('not_set') }) }) describe('password length', function() { describe('with the default password length options', function() { it('should reject passwords that are too short', function() { - expect(this.AuthenticationManager.validatePassword('')).to.eql({ - message: 'password is too short' - }) - return expect( - this.AuthenticationManager.validatePassword('foo') - ).to.eql({ message: 'password is too short' }) + const result1 = this.AuthenticationManager.validatePassword('') + expect(result1).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result1.message).to.equal('password is too short') + expect(result1.info.code).to.equal('too_short') + + const result2 = this.AuthenticationManager.validatePassword('foo') + expect(result2).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result2.message).to.equal('password is too short') + expect(result2.info.code).to.equal('too_short') }) it('should reject passwords that are too long', function() { - return expect( - this.AuthenticationManager.validatePassword(this.longPassword) - ).to.eql({ message: 'password is too long' }) + const result = this.AuthenticationManager.validatePassword( + this.longPassword + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal('password is too long') + expect(result.info.code).to.equal('too_long') }) it('should accept passwords that are a good length', function() { - return expect( + expect( this.AuthenticationManager.validatePassword('l337h4x0r') ).to.equal(null) }) @@ -373,34 +369,46 @@ describe('AuthenticationManager', function() { describe('when the password length is specified in settings', function() { beforeEach(function() { - return (this.settings.passwordStrengthOptions = { + this.settings.passwordStrengthOptions = { length: { min: 10, max: 12 } - }) + } }) it('should reject passwords that are too short', function() { - return expect( - this.AuthenticationManager.validatePassword('012345678') - ).to.eql({ message: 'password is too short' }) + const result = this.AuthenticationManager.validatePassword( + '012345678' + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal('password is too short') + expect(result.info.code).to.equal('too_short') }) it('should accept passwords of exactly minimum length', function() { - return expect( + expect( this.AuthenticationManager.validatePassword('0123456789') ).to.equal(null) }) it('should reject passwords that are too long', function() { - return expect( - this.AuthenticationManager.validatePassword('0123456789abc') - ).to.eql({ message: 'password is too long' }) + const result = this.AuthenticationManager.validatePassword( + '0123456789abc' + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal('password is too long') + expect(result.info.code).to.equal('too_long') }) it('should accept passwords of exactly maximum length', function() { - return expect( + expect( this.AuthenticationManager.validatePassword('0123456789ab') ).to.equal(null) }) @@ -408,17 +416,23 @@ describe('AuthenticationManager', function() { describe('when the maximum password length is set to >72 characters in settings', function() { beforeEach(function() { - return (this.settings.passwordStrengthOptions = { + this.settings.passwordStrengthOptions = { length: { max: 128 } - }) + } }) it('should still reject passwords > 72 characters in length', function() { - return expect( - this.AuthenticationManager.validatePassword(this.longPassword) - ).to.eql({ message: 'password is too long' }) + const result = this.AuthenticationManager.validatePassword( + this.longPassword + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal('password is too long') + expect(result.info.code).to.equal('too_long') }) }) }) @@ -431,7 +445,7 @@ describe('AuthenticationManager', function() { 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' ) ).to.equal(null) - return expect( + expect( this.AuthenticationManager.validatePassword( '1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,' ) @@ -439,25 +453,31 @@ describe('AuthenticationManager', function() { }) it('should not allow passwords with invalid characters', function() { - return expect( - this.AuthenticationManager.validatePassword( - 'correct horse battery staple' - ) - ).to.eql({ message: 'password contains an invalid character' }) + const result = this.AuthenticationManager.validatePassword( + 'correct horse battery staple' + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal( + 'password contains an invalid character' + ) + expect(result.info.code).to.equal('invalid_character') }) }) describe('when valid characters are overridden in settings', function() { beforeEach(function() { - return (this.settings.passwordStrengthOptions = { + this.settings.passwordStrengthOptions = { chars: { symbols: ' ' } - }) + } }) it('should allow passwords with valid characters', function() { - return expect( + expect( this.AuthenticationManager.validatePassword( 'correct horse battery staple' ) @@ -465,19 +485,25 @@ describe('AuthenticationManager', function() { }) it('should disallow passwords with invalid characters', function() { - return expect( - this.AuthenticationManager.validatePassword( - '1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,' - ) - ).to.eql({ message: 'password contains an invalid character' }) + const result = this.AuthenticationManager.validatePassword( + '1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,' + ) + + expect(result).to.be.an.instanceOf( + AuthenticationErrors.InvalidPasswordError + ) + expect(result.message).to.equal( + 'password contains an invalid character' + ) + expect(result.info.code).to.equal('invalid_character') }) }) describe('when allowAnyChars is set', function() { beforeEach(function() { - return (this.settings.passwordStrengthOptions = { + this.settings.passwordStrengthOptions = { allowAnyChars: true - }) + } }) it('should allow any characters', function() { @@ -486,7 +512,7 @@ describe('AuthenticationManager', function() { 'correct horse battery staple' ) ).to.equal(null) - return expect( + expect( this.AuthenticationManager.validatePassword( '1234567890@#$%^&*()-_=+[]{};:<>/?!£€.,' ) @@ -504,7 +530,7 @@ describe('AuthenticationManager', function() { this.salt = 'saltaasdfasdfasdf' this.bcrypt.genSalt = sinon.stub().callsArgWith(2, null, this.salt) this.bcrypt.hash = sinon.stub().callsArgWith(2, null, this.hashedPassword) - return (this.db.users.update = sinon.stub().callsArg(2)) + this.db.users.update = sinon.stub().callsArg(2) }) describe('too long', function() { @@ -514,28 +540,28 @@ describe('AuthenticationManager', function() { max: 10 } } - return (this.password = 'dsdsadsadsadsadsadkjsadjsadjsadljs') + this.password = 'dsdsadsadsadsadsadkjsadjsadjsadljs' }) it('should return and error', function(done) { - return this.AuthenticationManager.setUserPassword( + this.AuthenticationManager.setUserPassword( this.user_id, this.password, err => { expect(err).to.exist - return done() + done() } ) }) it('should not start the bcrypt process', function(done) { - return this.AuthenticationManager.setUserPassword( + this.AuthenticationManager.setUserPassword( this.user_id, this.password, - err => { + () => { this.bcrypt.genSalt.called.should.equal(false) this.bcrypt.hash.called.should.equal(false) - return done() + done() } ) }) @@ -549,28 +575,28 @@ describe('AuthenticationManager', function() { min: 6 } } - return (this.password = 'dsd') + this.password = 'dsd' }) it('should return and error', function(done) { - return this.AuthenticationManager.setUserPassword( + this.AuthenticationManager.setUserPassword( this.user_id, this.password, err => { expect(err).to.exist - return done() + done() } ) }) it('should not start the bcrypt process', function(done) { - return this.AuthenticationManager.setUserPassword( + this.AuthenticationManager.setUserPassword( this.user_id, this.password, - err => { + () => { this.bcrypt.genSalt.called.should.equal(false) this.bcrypt.hash.called.should.equal(false) - return done() + done() } ) }) @@ -591,7 +617,7 @@ describe('AuthenticationManager', function() { expect(args[0]).to.deep.equal({ _id: ObjectId(this.user_id.toString()) }) - return expect(args[1]).to.deep.equal({ + expect(args[1]).to.deep.equal({ $set: { hashedPassword: this.hashedPassword }, @@ -603,13 +629,11 @@ describe('AuthenticationManager', function() { it('should hash the password', function() { this.bcrypt.genSalt.calledWith(12).should.equal(true) - return this.bcrypt.hash - .calledWith(this.password, this.salt) - .should.equal(true) + this.bcrypt.hash.calledWith(this.password, this.salt).should.equal(true) }) it('should call the callback', function() { - return this.callback.called.should.equal(true) + this.callback.called.should.equal(true) }) }) })