Merge pull request #2190 from overleaf/as-invalid-password-errors

Throw Error instead of plain object if email/password is invalid

GitOrigin-RevId: 2a1b040b07834064d547cef7890676ca014ec0ae
This commit is contained in:
Jessica Lawshe 2019-10-02 09:07:28 -05:00 committed by sharelatex
parent b5f4e26840
commit 6737637b39
3 changed files with 200 additions and 151 deletions

View file

@ -0,0 +1,9 @@
const OError = require('@overleaf/o-error')
class InvalidEmailError extends OError {}
class InvalidPasswordError extends OError {}
module.exports = {
InvalidEmailError,
InvalidPasswordError
}

View file

@ -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) {

View file

@ -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)
})
})
})