From 914dcf5de520c44b663a7f756febb953efa3fe24 Mon Sep 17 00:00:00 2001 From: David <33458145+davidmcpowell@users.noreply.github.com> Date: Mon, 11 Mar 2024 10:03:35 +0000 Subject: [PATCH] Merge pull request #17408 from overleaf/dp-mongoose-callback-autherntication-manager Promisify AuthenticationManager and AuthenticationManagerTests GitOrigin-RevId: 8120bf55d19380a6ecf5241ffab8722eff2d4fe3 --- .../Authentication/AuthenticationManager.js | 391 +++++++------- .../AuthenticationManagerTests.js | 500 ++++++++---------- 2 files changed, 412 insertions(+), 479 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index d58697a67f..937e1d67b1 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -10,7 +10,7 @@ const { PasswordMustBeDifferentError, PasswordReusedError, } = require('./AuthenticationErrors') -const util = require('util') +const { callbackify } = require('util') const HaveIBeenPwned = require('./HaveIBeenPwned') const UserAuditLogHandler = require('../User/UserAuditLogHandler') const logger = require('@overleaf/logger') @@ -30,13 +30,9 @@ function _exceedsMaximumLengthRatio(password, maxSimilarity, value) { ) } -const _checkWriteResult = function (result, callback) { +const _checkWriteResult = function (result) { // for MongoDB - if (result && result.modifiedCount === 1) { - callback(null, true) - } else { - callback(null, false) - } + return !!(result && result.modifiedCount === 1) } function _validatePasswordNotTooLong(password) { @@ -59,144 +55,129 @@ function _metricsForSuccessfulPasswordMatch(password) { } const AuthenticationManager = { - _checkUserPassword(query, password, callback) { + async _checkUserPassword(query, password) { // Using Mongoose for legacy reasons here. The returned User instance // gets serialized into the session and there may be subtle differences // between the user returned by Mongoose vs mongodb (such as default values) - User.findOne(query, (error, user) => { - if (error) { - return callback(error) - } - if (!user || !user.hashedPassword) { - return callback(null, null, null) - } - let rounds = 0 - try { - rounds = bcrypt.getRounds(user.hashedPassword) - } catch (err) { - let prefix, suffix, length - if (typeof user.hashedPassword === 'string') { - length = user.hashedPassword.length - if (user.hashedPassword.length > 50) { - // A full bcrypt hash is 60 characters long. - prefix = user.hashedPassword.slice(0, '$2a$12$x'.length) - suffix = user.hashedPassword.slice(-4) - } else if (user.hashedPassword.length > 20) { - prefix = user.hashedPassword.slice(0, 4) - suffix = user.hashedPassword.slice(-4) - } else { - prefix = user.hashedPassword.slice(0, 4) - } + const user = await User.findOne(query).exec() + + if (!user || !user.hashedPassword) { + return { user: null, match: null } + } + + let rounds = 0 + try { + rounds = bcrypt.getRounds(user.hashedPassword) + } catch (err) { + let prefix, suffix, length + if (typeof user.hashedPassword === 'string') { + length = user.hashedPassword.length + if (user.hashedPassword.length > 50) { + // A full bcrypt hash is 60 characters long. + prefix = user.hashedPassword.slice(0, '$2a$12$x'.length) + suffix = user.hashedPassword.slice(-4) + } else if (user.hashedPassword.length > 20) { + prefix = user.hashedPassword.slice(0, 4) + suffix = user.hashedPassword.slice(-4) + } else { + prefix = user.hashedPassword.slice(0, 4) } - logger.warn( - { - err, - userId: user._id, - hashedPassword: { - type: typeof user.hashedPassword, - length, - prefix, - suffix, - }, + } + logger.warn( + { + err, + userId: user._id, + hashedPassword: { + type: typeof user.hashedPassword, + length, + prefix, + suffix, }, - 'unexpected user.hashedPassword value' - ) - } - Metrics.inc('bcrypt', 1, { - method: 'compare', - path: rounds, - }) - bcrypt.compare(password, user.hashedPassword, function (error, match) { - if (error) { - return callback(error) - } - if (match) { - _metricsForSuccessfulPasswordMatch(password) - } - callback(null, user, match) - }) + }, + 'unexpected user.hashedPassword value' + ) + } + Metrics.inc('bcrypt', 1, { + method: 'compare', + path: rounds, }) + + const match = await bcrypt.compare(password, user.hashedPassword) + + if (match) { + _metricsForSuccessfulPasswordMatch(password) + } + + return { user, match } }, - authenticate(query, password, auditLog, { skipHIBPCheck = false }, callback) { - AuthenticationManager._checkUserPassword( + async authenticate(query, password, auditLog, { skipHIBPCheck = false }) { + const { user, match } = await AuthenticationManager._checkUserPassword( query, - password, - (error, user, match) => { - if (error) { - return callback(error) - } - if (!user) { - return callback(null, null) - } - const update = { $inc: { loginEpoch: 1 } } - if (!match) { - update.$set = { lastFailedLogin: new Date() } - } - User.updateOne( - { _id: user._id, loginEpoch: user.loginEpoch }, - update, - {}, - (err, result) => { - if (err) { - return callback(err) - } - if (result.modifiedCount !== 1) { - return callback(new ParallelLoginError()) - } - if (!match) { - if (!auditLog) { - return callback(null, null) - } else { - return UserAuditLogHandler.addEntry( - user._id, - 'failed-password-match', - user._id, - auditLog.ipAddress, - auditLog.info, - err => { - if (err) { - logger.error( - { userId: user._id, err, info: auditLog.info }, - 'Error while adding AuditLog entry for failed-password-match' - ) - } - callback(null, null) - } - ) - } - } - AuthenticationManager.checkRounds( - user, - user.hashedPassword, - password, - function (err) { - if (err) { - return callback(err) - } - if (skipHIBPCheck) { - callback(null, user) - HaveIBeenPwned.checkPasswordForReuseInBackground(password) - return - } - HaveIBeenPwned.checkPasswordForReuse( - password, - (err, isPasswordReused) => { - if (err) { - logger.err({ err }, 'cannot check password for re-use') - } - if (isPasswordReused) { - return callback(new PasswordReusedError()) - } - callback(null, user) - } - ) - } - ) - } - ) - } + password ) + + if (!user) { + return null + } + + const update = { $inc: { loginEpoch: 1 } } + if (!match) { + update.$set = { lastFailedLogin: new Date() } + } + + const result = await User.updateOne( + { _id: user._id, loginEpoch: user.loginEpoch }, + update, + {} + ).exec() + + if (result.modifiedCount !== 1) { + throw new ParallelLoginError() + } + + if (!match) { + if (!auditLog) { + return null + } else { + try { + await UserAuditLogHandler.promises.addEntry( + user._id, + 'failed-password-match', + user._id, + auditLog.ipAddress, + auditLog.info + ) + } catch (err) { + logger.error( + { userId: user._id, err, info: auditLog.info }, + 'Error while adding AuditLog entry for failed-password-match' + ) + } + return null + } + } + await AuthenticationManager.checkRounds(user, user.hashedPassword, password) + + if (skipHIBPCheck) { + HaveIBeenPwned.checkPasswordForReuseInBackground(password) + return user + } + + let isPasswordReused + try { + isPasswordReused = await HaveIBeenPwned.promises.checkPasswordForReuse( + password + ) + } catch (err) { + logger.err({ err }, 'cannot check password for re-use') + } + + if (isPasswordReused) { + throw new PasswordReusedError() + } + + return user }, validateEmail(email) { @@ -295,108 +276,93 @@ const AuthenticationManager = { return null }, - setUserPassword(user, password, callback) { - AuthenticationManager.setUserPasswordInV2(user, password, callback) + async setUserPassword(user, password) { + return await AuthenticationManager.setUserPasswordInV2(user, password) }, - checkRounds(user, hashedPassword, password, callback) { + async checkRounds(user, hashedPassword, password) { // Temporarily disable this function, TODO: re-enable this if (Settings.security.disableBcryptRoundsUpgrades) { Metrics.inc('bcrypt_check_rounds', 1, { status: 'disabled' }) - return callback() + return } // check current number of rounds and rehash if necessary const currentRounds = bcrypt.getRounds(hashedPassword) if (currentRounds < BCRYPT_ROUNDS) { Metrics.inc('bcrypt_check_rounds', 1, { status: 'upgrade' }) - AuthenticationManager._setUserPasswordInMongo(user, password, callback) + return await AuthenticationManager._setUserPasswordInMongo(user, password) } else { Metrics.inc('bcrypt_check_rounds', 1, { status: 'success' }) - callback() } }, - hashPassword(password, callback) { + async hashPassword(password) { // Double-check the size to avoid truncating in bcrypt. const error = _validatePasswordNotTooLong(password) if (error) { - return callback(error) + throw error } - bcrypt.genSalt(BCRYPT_ROUNDS, BCRYPT_MINOR_VERSION, function (error, salt) { - if (error) { - return callback(error) - } - Metrics.inc('bcrypt', 1, { - method: 'hash', - path: BCRYPT_ROUNDS, - }) - bcrypt.hash(password, salt, callback) + + const salt = await bcrypt.genSalt(BCRYPT_ROUNDS, BCRYPT_MINOR_VERSION) + + Metrics.inc('bcrypt', 1, { + method: 'hash', + path: BCRYPT_ROUNDS, }) + return await bcrypt.hash(password, salt) }, - setUserPasswordInV2(user, password, callback) { + async setUserPasswordInV2(user, password) { if (!user || !user.email || !user._id) { - return callback(new Error('invalid user object')) + throw new Error('invalid user object') } const validationError = this.validatePassword(password, user.email) if (validationError) { - return callback(validationError) + throw validationError } // check if we can log in with this password. In which case we should reject it, // because it is the same as the existing password. - AuthenticationManager._checkUserPassword( + const { match } = await AuthenticationManager._checkUserPassword( { _id: user._id }, - password, - (err, _user, match) => { - if (err) { - return callback(err) - } - if (match) { - return callback(new PasswordMustBeDifferentError()) - } - - HaveIBeenPwned.checkPasswordForReuse( - password, - (error, isPasswordReused) => { - if (error) { - logger.err({ error }, 'cannot check password for re-use') - } - - if (!error && isPasswordReused) { - return callback(new PasswordReusedError()) - } - - // password is strong enough or the validation with the service did not happen - this._setUserPasswordInMongo(user, password, callback) - } - ) - } + password ) + + if (match) { + throw new PasswordMustBeDifferentError() + } + + let isPasswordReused + try { + isPasswordReused = await HaveIBeenPwned.promises.checkPasswordForReuse( + password + ) + } catch (error) { + logger.err({ error }, 'cannot check password for re-use') + } + + if (isPasswordReused) { + throw new PasswordReusedError() + } + + // password is strong enough or the validation with the service did not happen + return await this._setUserPasswordInMongo(user, password) }, - _setUserPasswordInMongo(user, password, callback) { - this.hashPassword(password, function (error, hash) { - if (error) { - return callback(error) - } - db.users.updateOne( - { _id: new ObjectId(user._id.toString()) }, - { - $set: { - hashedPassword: hash, - }, - $unset: { - password: true, - }, + async _setUserPasswordInMongo(user, password) { + const hash = await this.hashPassword(password) + const result = await db.users.updateOne( + { _id: new ObjectId(user._id.toString()) }, + { + $set: { + hashedPassword: hash, }, - function (updateError, result) { - if (updateError) { - return callback(updateError) - } - _checkWriteResult(result, callback) - } - ) - }) + $unset: { + password: true, + }, + } + ) + + return _checkWriteResult(result) }, _passwordCharactersAreValid(password) { @@ -500,10 +466,17 @@ const AuthenticationManager = { }, } -AuthenticationManager.promises = { - authenticate: util.promisify(AuthenticationManager.authenticate), - hashPassword: util.promisify(AuthenticationManager.hashPassword), - setUserPassword: util.promisify(AuthenticationManager.setUserPassword), +module.exports = { + _validatePasswordNotTooSimilar: + AuthenticationManager._validatePasswordNotTooSimilar, // Private function exported for tests + validateEmail: AuthenticationManager.validateEmail, + validatePassword: AuthenticationManager.validatePassword, + getMessageForInvalidPasswordError: + AuthenticationManager.getMessageForInvalidPasswordError, + authenticate: callbackify(AuthenticationManager.authenticate), + setUserPassword: callbackify(AuthenticationManager.setUserPassword), + checkRounds: callbackify(AuthenticationManager.checkRounds), + hashPassword: callbackify(AuthenticationManager.hashPassword), + setUserPasswordInV2: callbackify(AuthenticationManager.setUserPasswordInV2), + promises: AuthenticationManager, } - -module.exports = AuthenticationManager diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 0eb62707a8..866882d3e5 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -14,14 +14,18 @@ describe('AuthenticationManager', function () { this.settings = { security: { bcryptRounds: 4 } } this.metrics = { inc: sinon.stub().returns() } this.HaveIBeenPwned = { - checkPasswordForReuse: sinon.stub().yields(null, false), + promises: { + checkPasswordForReuse: sinon.stub().resolves(false), + }, checkPasswordForReuseInBackground: sinon.stub(), } this.AuthenticationManager = SandboxedModule.require(modulePath, { requires: { '../../models/User': { User: (this.User = { - updateOne: sinon.stub().callsArgWith(3, null, { modifiedCount: 1 }), + updateOne: sinon + .stub() + .returns({ exec: sinon.stub().resolves({ modifiedCount: 1 }) }), }), }, '../../infrastructure/mongodb': { @@ -32,16 +36,17 @@ describe('AuthenticationManager', function () { getRounds: sinon.stub().returns(4), }), '@overleaf/settings': this.settings, - '../User/UserGetter': (this.UserGetter = {}), + '../User/UserGetter': (this.UserGetter = { promises: {} }), './AuthenticationErrors': AuthenticationErrors, './HaveIBeenPwned': this.HaveIBeenPwned, '../User/UserAuditLogHandler': (this.UserAuditLogHandler = { - addEntry: sinon.stub().callsArgWith(5, null), + promises: { + addEntry: sinon.stub().resolves(null), + }, }), '@overleaf/metrics': this.metrics, }, }) - this.callback = sinon.stub() }) afterEach(function () { @@ -67,22 +72,20 @@ describe('AuthenticationManager', function () { email: (this.email = 'USER@overleaf.com'), } this.user.hashedPassword = this.testPassword - this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) + this.User.findOne = sinon + .stub() + .returns({ exec: sinon.stub().resolves(this.user) }) this.metrics.inc.reset() }) describe('when the hashed password matches', function () { - beforeEach(function (done) { + beforeEach(async function () { this.unencryptedPassword = 'testpassword' - this.AuthenticationManager.authenticate( + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: true }, - (error, user) => { - this.callback(error, user) - done() - } + { skipHIBPCheck: true } ) }) @@ -104,7 +107,7 @@ describe('AuthenticationManager', function () { }) it('should return the user', function () { - this.callback.should.have.been.calledWith(null, this.user) + this.result.should.equal(this.user) }) it('should send metrics', function () { @@ -115,16 +118,12 @@ describe('AuthenticationManager', function () { }) describe('when the encrypted passwords do not match', function () { - beforeEach(function (done) { - this.AuthenticationManager.authenticate( + beforeEach(async function () { + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, 'notthecorrectpassword', null, - { skipHIBPCheck: true }, - (...args) => { - this.callback(...args) - done() - } + { skipHIBPCheck: true } ) }) @@ -142,7 +141,7 @@ describe('AuthenticationManager', function () { }) it('should not return the user', function () { - this.callback.calledWith(null, null).should.equal(true) + expect(this.result).to.equal(null) }) }) @@ -150,50 +149,37 @@ describe('AuthenticationManager', function () { beforeEach(function () { this.User.updateOne = sinon .stub() - .callsArgWith(3, null, { modifiedCount: 0 }) + .returns({ exec: sinon.stub().resolves({ modifiedCount: 0 }) }) }) describe('correct password', function () { - beforeEach(function (done) { - this.AuthenticationManager.authenticate( - { email: this.email }, - 'testpassword', - null, - { skipHIBPCheck: true }, - (...args) => { - this.callback(...args) - done() - } - ) - }) - - it('should return an error', function () { - this.callback.should.have.been.calledWith( - sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError) - ) + it('should return an error', async function () { + await expect( + this.AuthenticationManager.promises.authenticate( + { email: this.email }, + 'testpassword', + null, + { skipHIBPCheck: true } + ) + ).to.be.rejectedWith(AuthenticationErrors.ParallelLoginError) }) }) describe('bad password', function () { - beforeEach(function (done) { + beforeEach(function () { this.User.updateOne = sinon .stub() - .yields(null, { modifiedCount: 0 }) - this.AuthenticationManager.authenticate( - { email: this.email }, - 'notthecorrectpassword', - null, - { skipHIBPCheck: true }, - (...args) => { - this.callback(...args) - done() - } - ) + .returns({ exec: sinon.stub().resolves({ modifiedCount: 0 }) }) }) - it('should return an error', function () { - this.callback.should.have.been.calledWith( - sinon.match.instanceOf(AuthenticationErrors.ParallelLoginError) - ) + it('should return an error', async function () { + await expect( + this.AuthenticationManager.promises.authenticate( + { email: this.email }, + 'notthecorrectpassword', + null, + { skipHIBPCheck: true } + ) + ).to.be.rejectedWith(AuthenticationErrors.ParallelLoginError) }) }) }) @@ -206,50 +192,41 @@ describe('AuthenticationManager', function () { email: (this.email = 'USER@overleaf.com'), } this.db.users.updateOne = sinon - this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) - this.db.users.updateOne = sinon + this.User.findOne = sinon .stub() - .callsArgWith(2, null, { modifiedCount: 1 }) + .returns({ exec: sinon.stub().resolves(this.user) }) + this.bcrypt.compare = sinon.stub().resolves(false) + this.db.users.updateOne = sinon.stub().resolves({ modifiedCount: 1 }) }) - it('should not produce an error', function (done) { - this.AuthenticationManager.setUserPasswordInV2( - this.user, - 'testpassword', - (err, updated) => { - expect(err).to.not.exist - expect(updated).to.equal(true) - done() - } - ) + it('should not produce an error', async function () { + const updated = + await this.AuthenticationManager.promises.setUserPasswordInV2( + this.user, + 'testpassword' + ) + expect(updated).to.equal(true) }) - it('should set the hashed password', function (done) { - this.AuthenticationManager.setUserPasswordInV2( + it('should set the hashed password', async function () { + await this.AuthenticationManager.promises.setUserPasswordInV2( this.user, - 'testpassword', - err => { - expect(err).to.not.exist - const { hashedPassword } = - this.db.users.updateOne.lastCall.args[1].$set - expect(hashedPassword).to.exist - expect(hashedPassword.length).to.equal(60) - expect(hashedPassword).to.match(/^\$2a\$04\$[a-zA-Z0-9/.]{53}$/) - done() - } + 'testpassword' ) + + const { hashedPassword } = this.db.users.updateOne.lastCall.args[1].$set + expect(hashedPassword).to.exist + expect(hashedPassword.length).to.equal(60) + expect(hashedPassword).to.match(/^\$2a\$04\$[a-zA-Z0-9/.]{53}$/) }) }) }) describe('hashPassword', function () { - it('should block too long passwords', function (done) { - this.AuthenticationManager.hashPassword('x'.repeat(100), err => { - expect(err).to.exist - expect(err.message).to.equal('password is too long') - done() - }) + it('should block too long passwords', async function () { + await expect( + this.AuthenticationManager.promises.hashPassword('x'.repeat(100)) + ).to.be.rejectedWith('password is too long') }) }) @@ -261,24 +238,22 @@ describe('AuthenticationManager', function () { email: (this.email = 'USER@overleaf.com'), } this.unencryptedPassword = 'banana' - this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) + this.User.findOne = sinon + .stub() + .returns({ exec: sinon.stub().resolves(this.user) }) this.metrics.inc.reset() }) describe('when the hashed password matches', function () { - beforeEach(function (done) { + beforeEach(async function () { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true) + this.bcrypt.compare = sinon.stub().resolves(true) this.bcrypt.getRounds = sinon.stub().returns(4) - this.AuthenticationManager.authenticate( + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: true }, - (error, user) => { - this.callback(error, user) - done() - } + { skipHIBPCheck: true } ) }) @@ -301,70 +276,65 @@ describe('AuthenticationManager', function () { }) it('should return the user', function () { - this.callback.calledWith(null, this.user).should.equal(true) + this.result.should.equal(this.user) }) describe('HIBP', function () { it('should not check HIBP if not requested', function () { - this.HaveIBeenPwned.checkPasswordForReuse.should.not.have.been - .called + this.HaveIBeenPwned.promises.checkPasswordForReuse.should.not.have + .been.called }) - it('should check HIBP if requested', function (done) { - this.AuthenticationManager.authenticate( + it('should check HIBP if requested', async function () { + await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: false }, - error => { - if (error) return done(error) - this.HaveIBeenPwned.checkPasswordForReuse.should.have.been.calledWith( - this.unencryptedPassword - ) - done() - } + { skipHIBPCheck: false } + ) + + this.HaveIBeenPwned.promises.checkPasswordForReuse.should.have.been.calledWith( + this.unencryptedPassword ) }) }) }) describe('when the encrypted passwords do not match', function () { - beforeEach(function () { + beforeEach(async function () { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) - this.AuthenticationManager.authenticate( + this.bcrypt.compare = sinon.stub().resolves(false) + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: true }, - this.callback + { skipHIBPCheck: true } ) }) it('should not return the user', function () { - this.callback.calledWith(null, null).should.equal(true) - this.UserAuditLogHandler.addEntry.callCount.should.equal(0) + expect(this.result).to.equal(null) + this.UserAuditLogHandler.promises.addEntry.callCount.should.equal(0) }) }) describe('when the encrypted passwords do not match, with auditLog', function () { - beforeEach(function () { + beforeEach(async function () { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) + this.bcrypt.compare = sinon.stub().resolves(false) this.auditLog = { ipAddress: 'ip', info: { method: 'foo' } } - this.AuthenticationManager.authenticate( + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, this.auditLog, - { skipHIBPCheck: true }, - this.callback + { skipHIBPCheck: true } ) }) it('should not return the user, but add entry to audit log', function () { - this.callback.calledWith(null, null).should.equal(true) - this.UserAuditLogHandler.addEntry.callCount.should.equal(1) - this.UserAuditLogHandler.addEntry + expect(this.result).to.equal(null) + this.UserAuditLogHandler.promises.addEntry.callCount.should.equal(1) + this.UserAuditLogHandler.promises.addEntry .calledWith( this.user._id, 'failed-password-match', @@ -377,22 +347,18 @@ describe('AuthenticationManager', function () { }) describe('when the hashed password matches but the number of rounds is too low', function () { - beforeEach(function (done) { + beforeEach(async function () { this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true) + this.bcrypt.compare = sinon.stub().resolves(true) this.bcrypt.getRounds = sinon.stub().returns(1) - this.AuthenticationManager._setUserPasswordInMongo = sinon + this.AuthenticationManager.promises._setUserPasswordInMongo = sinon .stub() - .callsArgWith(2, null) - this.AuthenticationManager.authenticate( + .resolves() + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: true }, - (error, user) => { - this.callback(error, user) - done() - } + { skipHIBPCheck: true } ) }) @@ -415,34 +381,30 @@ describe('AuthenticationManager', function () { }) it('should set the users password (with a higher number of rounds)', function () { - this.AuthenticationManager._setUserPasswordInMongo + this.AuthenticationManager.promises._setUserPasswordInMongo .calledWith(this.user, this.unencryptedPassword) .should.equal(true) }) it('should return the user', function () { - this.callback.calledWith(null, this.user).should.equal(true) + this.result.should.equal(this.user) }) }) describe('when the hashed password matches but the number of rounds is too low, but upgrades disabled', function () { - beforeEach(function (done) { + beforeEach(async function () { this.settings.security.disableBcryptRoundsUpgrades = true this.user.hashedPassword = this.hashedPassword = 'asdfjadflasdf' - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, true) + this.bcrypt.compare = sinon.stub().resolves(true) this.bcrypt.getRounds = sinon.stub().returns(1) - this.AuthenticationManager.setUserPassword = sinon + this.AuthenticationManager.promises.setUserPassword = sinon .stub() - .callsArgWith(2, null) - this.AuthenticationManager.authenticate( + .resolves() + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencryptedPassword, null, - { skipHIBPCheck: true }, - (error, user) => { - this.callback(error, user) - done() - } + { skipHIBPCheck: true } ) }) @@ -455,31 +417,32 @@ describe('AuthenticationManager', function () { }) it('should not set the users password (with a higher number of rounds)', function () { - this.AuthenticationManager.setUserPassword + this.AuthenticationManager.promises.setUserPassword .calledWith(this.user, this.unencryptedPassword) .should.equal(false) }) it('should return the user', function () { - this.callback.calledWith(null, this.user).should.equal(true) + this.result.should.equal(this.user) }) }) }) describe('when the user does not exist in the database', function () { - beforeEach(function () { - this.User.findOne = sinon.stub().callsArgWith(1, null, null) - this.AuthenticationManager.authenticate( + beforeEach(async function () { + this.User.findOne = sinon + .stub() + .returns({ exec: sinon.stub().resolves(null) }) + this.result = await this.AuthenticationManager.promises.authenticate( { email: this.email }, this.unencrpytedPassword, null, - { skipHIBPCheck: true }, - this.callback + { skipHIBPCheck: true } ) }) it('should not return a user', function () { - this.callback.calledWith(null, null).should.equal(true) + expect(this.result).to.equal(null) }) }) }) @@ -799,28 +762,27 @@ describe('AuthenticationManager', function () { email: 'user@example.com', hashedPassword: this.hashedPassword, } - this.bcrypt.compare = sinon.stub().callsArgWith(2, null, false) - this.bcrypt.genSalt = sinon.stub().callsArgWith(2, null, this.salt) - this.bcrypt.hash = sinon.stub().callsArgWith(2, null, this.hashedPassword) - this.User.findOne = sinon.stub().callsArgWith(1, null, this.user) - this.db.users.updateOne = sinon.stub().callsArg(2) + this.bcrypt.compare = sinon.stub().resolves(false) + this.bcrypt.genSalt = sinon.stub().resolves(this.salt) + this.bcrypt.hash = sinon.stub().resolves(this.hashedPassword) + this.User.findOne = sinon + .stub() + .returns({ exec: sinon.stub().resolves(this.user) }) + this.db.users.updateOne = sinon.stub().resolves() }) describe('same as previous password', function () { beforeEach(function () { - this.bcrypt.compare.callsArgWith(2, null, true) + this.bcrypt.compare.resolves(true) }) - it('should return an error', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - expect(err.name).to.equal('PasswordMustBeDifferentError') - done() - } - ) + it('should be rejected', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejectedWith(AuthenticationErrors.PasswordMustBeDifferentError) }) }) @@ -834,27 +796,25 @@ describe('AuthenticationManager', function () { this.password = 'dsdsadsadsadsadsadkjsadjsadjsadljs' }) - it('should return and error', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - done() - } - ) + it('should return and error', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejectedWith('password is too long') }) - it('should not start the bcrypt process', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - () => { - this.bcrypt.genSalt.called.should.equal(false) - this.bcrypt.hash.called.should.equal(false) - done() - } - ) + it('should not start the bcrypt process', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejected + + this.bcrypt.genSalt.called.should.equal(false) + this.bcrypt.hash.called.should.equal(false) }) }) @@ -863,16 +823,13 @@ describe('AuthenticationManager', function () { this.password = `some${this.user.email}password` }) - it('should reject the password', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - expect(err.name).to.equal('InvalidPasswordError') - done() - } - ) + it('should reject the password', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejectedWith(AuthenticationErrors.InvalidPasswordError) }) }) @@ -881,16 +838,13 @@ describe('AuthenticationManager', function () { this.password = `some${this.user.email.split('@')[0]}password` }) - it('should reject the password', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - expect(err.name).to.equal('InvalidPasswordError') - done() - } - ) + it('should reject the password', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejectedWith(AuthenticationErrors.InvalidPasswordError) }) }) @@ -901,13 +855,17 @@ describe('AuthenticationManager', function () { user = { _id: 'some-user-id', email: 'someuser@somedomain.com' } }) - it('should reject the password', function (done) { - this.AuthenticationManager.setUserPassword(user, password, err => { - expect(err).to.exist + it('should reject the password', async function () { + try { + await this.AuthenticationManager.promises.setUserPassword( + user, + password + ) + expect.fail('should have thrown') + } catch (err) { expect(err.name).to.equal('InvalidPasswordError') expect(err?.info?.code).to.equal('contains_email') - done() - }) + } }) }) @@ -922,27 +880,25 @@ describe('AuthenticationManager', function () { this.password = 'dsd' }) - it('should return and error', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - done() - } - ) + it('should return and error', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejectedWith('password is too short') }) - it('should not start the bcrypt process', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - () => { - this.bcrypt.genSalt.called.should.equal(false) - this.bcrypt.hash.called.should.equal(false) - done() - } - ) + it('should not start the bcrypt process', async function () { + await expect( + this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + ).to.be.rejected + + this.bcrypt.genSalt.called.should.equal(false) + this.bcrypt.hash.called.should.equal(false) }) }) @@ -953,45 +909,54 @@ describe('AuthenticationManager', function () { this.metrics.inc.reset() }) - it('should produce an error when the password is too similar to the email', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password, - err => { - expect(err).to.exist - expect(err?.info?.code).to.equal('too_similar') - expect( - this.metrics.inc.calledWith('password-too-similar-to-email') - ).to.equal(true) - done() - } - ) + it('should produce an error when the password is too similar to the email', async function () { + try { + await this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password + ) + expect.fail('should have thrown') + } catch (err) { + expect(err.message).to.equal( + 'password is too similar to email address' + ) + expect(err?.info?.code).to.equal('too_similar') + } + + expect( + this.metrics.inc.calledWith('password-too-similar-to-email') + ).to.equal(true) }) - it('should produce an error when the password is too similar to the email, regardless of case', function (done) { - this.AuthenticationManager.setUserPassword( - this.user, - this.password.toUpperCase(), - err => { - expect(err).to.exist - expect(err?.info?.code).to.equal('too_similar') - expect( - this.metrics.inc.calledWith('password-too-similar-to-email') - ).to.equal(true) - done() - } - ) + it('should produce an error when the password is too similar to the email, regardless of case', async function () { + try { + await this.AuthenticationManager.promises.setUserPassword( + this.user, + this.password.toUpperCase() + ) + expect.fail('should have thrown') + } catch (err) { + expect(err.message).to.equal( + 'password is too similar to email address' + ) + expect(err?.info?.code).to.equal('too_similar') + } + + expect( + this.metrics.inc.calledWith('password-too-similar-to-email') + ).to.equal(true) }) }) describe('successful password set attempt', function () { - beforeEach(function () { + beforeEach(async function () { this.metrics.inc.reset() - this.UserGetter.getUser = sinon.stub().yields(null, { overleaf: null }) - this.AuthenticationManager.setUserPassword( + this.UserGetter.promises.getUser = sinon + .stub() + .resolves({ overleaf: null }) + await this.AuthenticationManager.promises.setUserPassword( this.user, - this.password, - this.callback + this.password ) }) @@ -1020,11 +985,6 @@ describe('AuthenticationManager', function () { this.metrics.inc.calledWith('password-too-similar-to-email') ).to.equal(false) }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - expect(this.callback.lastCall.args[0]).to.not.be.instanceOf(Error) - }) }) }) })