From 75c7a58100e6132e838ef8fb1b49995ea3ded416 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Thu, 2 Nov 2023 08:47:16 -0400 Subject: [PATCH] Merge pull request #15515 from overleaf/em-promisify-user-controller Promisify UserController GitOrigin-RevId: d56ddb32abff2c33f45efa58285c7bf02b578cd2 --- .../app/src/Features/User/UserController.js | 572 ++++++++---------- .../web/app/src/Features/User/UserHandler.js | 2 + .../src/Features/User/UserSessionsManager.js | 1 + .../test/unit/src/User/UserControllerTests.js | 209 ++++--- 4 files changed, 376 insertions(+), 408 deletions(-) diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index e086fdfbca..bb47ac7b00 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -16,7 +16,7 @@ const HttpErrorHandler = require('../Errors/HttpErrorHandler') const OError = require('@overleaf/o-error') const EmailHandler = require('../Email/EmailHandler') const UrlHelper = require('../Helpers/UrlHelper') -const { promisify } = require('util') +const { promisify, callbackify } = require('util') const { expressify } = require('@overleaf/promise-utils') const { acceptsJson, @@ -46,15 +46,15 @@ function _sendSecurityAlertPasswordChanged(user) { actionDescribed: `your password has been changed on your account ${user.email}`, action: 'password changed', } - EmailHandler.sendEmail('securityAlert', emailOptions, error => { - if (error) { + EmailHandler.promises + .sendEmail('securityAlert', emailOptions) + .catch(error => { // log error when sending security alert email but do not pass back logger.error( { error, userId: user._id }, 'could not send security alert email when password changed' ) - } - }) + }) } async function _ensureAffiliation(userId, emailData) { @@ -210,326 +210,276 @@ async function ensureAffiliationMiddleware(req, res, next) { return next() } -const UserController = { - clearSessions: expressify(clearSessions), +async function tryDeleteUser(req, res, next) { + const userId = SessionManager.getLoggedInUserId(req.session) + const { password } = req.body + req.logger.addFields({ userId }) - tryDeleteUser(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - const { password } = req.body + if (password == null || password === '') { + logger.err({ userId }, 'no password supplied for attempt to delete account') + return res.sendStatus(403) + } - if (password == null || password === '') { - logger.err( - { userId }, - 'no password supplied for attempt to delete account' - ) - return res.sendStatus(403) + const user = await AuthenticationManager.promises.authenticate( + { _id: userId }, + password + ) + if (!user) { + logger.err({ userId }, 'auth failed during attempt to delete account') + return res.sendStatus(403) + } + + try { + await UserDeleter.promises.deleteUser(userId, { + deleterUser: user, + ipAddress: req.ip, + }) + } catch (err) { + const errorData = { + message: 'error while deleting user account', + info: { userId }, } - AuthenticationManager.authenticate( - { _id: userId }, - password, - (err, user) => { - if (err != null) { - OError.tag( - err, - 'error authenticating during attempt to delete account', - { - userId, - } - ) - return next(err) - } - if (!user) { - logger.err({ userId }, 'auth failed during attempt to delete account') - return res.sendStatus(403) - } - UserDeleter.deleteUser( - userId, - { deleterUser: user, ipAddress: req.ip }, - err => { - if (err) { - const errorData = { - message: 'error while deleting user account', - info: { userId }, - } - if (err instanceof Errors.SubscriptionAdminDeletionError) { - // set info.public.error for JSON response so frontend can display - // a specific message - errorData.info.public = { - error: 'SubscriptionAdminDeletionError', - } - logger.warn(OError.tag(err, errorData.message, errorData.info)) - return HttpErrorHandler.unprocessableEntity( - req, - res, - errorData.message, - errorData.info.public - ) - } else { - return next(OError.tag(err, errorData.message, errorData.info)) - } - } - const sessionId = req.sessionID - if (typeof req.logout === 'function') { - req.logout() - } - req.session.destroy(err => { - if (err != null) { - OError.tag(err, 'error destroying session') - return next(err) - } - UserSessionsManager.untrackSession(user, sessionId, () => {}) - res.sendStatus(200) - }) - } + if (err instanceof Errors.SubscriptionAdminDeletionError) { + // set info.public.error for JSON response so frontend can display + // a specific message + errorData.info.public = { + error: 'SubscriptionAdminDeletionError', + } + logger.warn(OError.tag(err, errorData.message, errorData.info)) + return HttpErrorHandler.unprocessableEntity( + req, + res, + errorData.message, + errorData.info.public + ) + } else { + throw err + } + } + + const sessionId = req.sessionID + + if (typeof req.logout === 'function') { + req.logout() + } + + const destroySession = promisify(req.session.destroy.bind(req.session)) + await destroySession() + + UserSessionsManager.promises.untrackSession(user, sessionId).catch(err => { + logger.warn({ err, userId: user._id }, 'failed to untrack session') + }) + res.sendStatus(200) +} + +async function subscribe(req, res, next) { + const userId = SessionManager.getLoggedInUserId(req.session) + req.logger.addFields({ userId }) + + const user = await UserGetter.promises.getUser(userId, { + _id: 1, + email: 1, + first_name: 1, + last_name: 1, + }) + await NewsletterManager.promises.subscribe(user) + res.json({ + message: req.i18n.translate('thanks_settings_updated'), + }) +} + +async function unsubscribe(req, res, next) { + const userId = SessionManager.getLoggedInUserId(req.session) + req.logger.addFields({ userId }) + + const user = await UserGetter.promises.getUser(userId, { + _id: 1, + email: 1, + first_name: 1, + last_name: 1, + }) + await NewsletterManager.promises.unsubscribe(user) + await Modules.promises.hooks.fire('newsletterUnsubscribed', user) + res.json({ + message: req.i18n.translate('thanks_settings_updated'), + }) +} + +async function updateUserSettings(req, res, next) { + const userId = SessionManager.getLoggedInUserId(req.session) + req.logger.addFields({ userId }) + + const user = await User.findById(userId).exec() + if (user == null) { + throw new OError('problem updating user settings', { userId }) + } + + if (req.body.first_name != null) { + user.first_name = req.body.first_name.trim() + } + if (req.body.last_name != null) { + user.last_name = req.body.last_name.trim() + } + if (req.body.role != null) { + user.role = req.body.role.trim() + } + if (req.body.institution != null) { + user.institution = req.body.institution.trim() + } + if (req.body.mode != null) { + user.ace.mode = req.body.mode + } + if (req.body.editorTheme != null) { + user.ace.theme = req.body.editorTheme + } + if (req.body.overallTheme != null) { + user.ace.overallTheme = req.body.overallTheme + } + if (req.body.fontSize != null) { + user.ace.fontSize = req.body.fontSize + } + if (req.body.autoComplete != null) { + user.ace.autoComplete = req.body.autoComplete + } + if (req.body.autoPairDelimiters != null) { + user.ace.autoPairDelimiters = req.body.autoPairDelimiters + } + if (req.body.spellCheckLanguage != null) { + user.ace.spellCheckLanguage = req.body.spellCheckLanguage + } + if (req.body.pdfViewer != null) { + user.ace.pdfViewer = req.body.pdfViewer + } + if (req.body.syntaxValidation != null) { + user.ace.syntaxValidation = req.body.syntaxValidation + } + if (req.body.fontFamily != null) { + user.ace.fontFamily = req.body.fontFamily + } + if (req.body.lineHeight != null) { + user.ace.lineHeight = req.body.lineHeight + } + await user.save() + + const newEmail = req.body.email?.trim().toLowerCase() + if ( + newEmail == null || + newEmail === user.email || + req.externalAuthenticationSystemUsed() + ) { + // end here, don't update email + SessionManager.setInSessionUser(req.session, { + first_name: user.first_name, + last_name: user.last_name, + }) + res.sendStatus(200) + } else if (newEmail.indexOf('@') === -1) { + // email invalid + res.sendStatus(400) + } else { + // update the user email + const auditLog = { + initiatorId: userId, + ipAddress: req.ip, + } + + try { + await UserUpdater.promises.changeEmailAddress(userId, newEmail, auditLog) + } catch (err) { + if (err instanceof Errors.EmailExistsError) { + const translation = req.i18n.translate('email_already_registered') + return HttpErrorHandler.conflict(req, res, translation) + } else { + return HttpErrorHandler.legacyInternal( + req, + res, + req.i18n.translate('problem_changing_email_address'), + OError.tag(err, 'problem_changing_email_address', { + userId, + newEmail, + }) ) } - ) - }, + } - subscribe(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - UserGetter.getUser( - userId, - { _id: 1, email: 1, first_name: 1, last_name: 1 }, - (err, user) => { - if (err != null) { - return next(err) - } - NewsletterManager.subscribe(user, err => { - if (err != null) { - OError.tag(err, 'error subscribing to newsletter') - return next(err) - } - return res.json({ - message: req.i18n.translate('thanks_settings_updated'), - }) - }) - } - ) - }, - - unsubscribe(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - UserGetter.getUser( - userId, - { _id: 1, email: 1, first_name: 1, last_name: 1 }, - (err, user) => { - if (err != null) { - return next(err) - } - NewsletterManager.unsubscribe(user, err => { - if (err != null) { - OError.tag(err, 'error unsubscribing to newsletter') - return next(err) - } - Modules.hooks.fire('newsletterUnsubscribed', user, err => { - if (err) { - OError.tag(err, 'error firing "newsletterUnsubscribed" hook') - return next(err) - } - return res.json({ - message: req.i18n.translate('thanks_settings_updated'), - }) - }) - }) - } - ) - }, - - updateUserSettings(req, res, next) { - const userId = SessionManager.getLoggedInUserId(req.session) - User.findById(userId, (err, user) => { - if (err != null || user == null) { - logger.err({ err, userId }, 'problem updaing user settings') - return res.sendStatus(500) - } - - if (req.body.first_name != null) { - user.first_name = req.body.first_name.trim() - } - if (req.body.last_name != null) { - user.last_name = req.body.last_name.trim() - } - if (req.body.role != null) { - user.role = req.body.role.trim() - } - if (req.body.institution != null) { - user.institution = req.body.institution.trim() - } - if (req.body.mode != null) { - user.ace.mode = req.body.mode - } - if (req.body.editorTheme != null) { - user.ace.theme = req.body.editorTheme - } - if (req.body.overallTheme != null) { - user.ace.overallTheme = req.body.overallTheme - } - if (req.body.fontSize != null) { - user.ace.fontSize = req.body.fontSize - } - if (req.body.autoComplete != null) { - user.ace.autoComplete = req.body.autoComplete - } - if (req.body.autoPairDelimiters != null) { - user.ace.autoPairDelimiters = req.body.autoPairDelimiters - } - if (req.body.spellCheckLanguage != null) { - user.ace.spellCheckLanguage = req.body.spellCheckLanguage - } - if (req.body.pdfViewer != null) { - user.ace.pdfViewer = req.body.pdfViewer - } - if (req.body.syntaxValidation != null) { - user.ace.syntaxValidation = req.body.syntaxValidation - } - if (req.body.fontFamily != null) { - user.ace.fontFamily = req.body.fontFamily - } - if (req.body.lineHeight != null) { - user.ace.lineHeight = req.body.lineHeight - } - - user.save(err => { - if (err != null) { - return next(err) - } - const newEmail = - req.body.email != null - ? req.body.email.trim().toLowerCase() - : undefined - if ( - newEmail == null || - newEmail === user.email || - req.externalAuthenticationSystemUsed() - ) { - // end here, don't update email - SessionManager.setInSessionUser(req.session, { - first_name: user.first_name, - last_name: user.last_name, - }) - res.sendStatus(200) - } else if (newEmail.indexOf('@') === -1) { - // email invalid - res.sendStatus(400) - } else { - // update the user email - const auditLog = { - initiatorId: userId, - ipAddress: req.ip, - } - UserUpdater.changeEmailAddress(userId, newEmail, auditLog, err => { - if (err) { - if (err instanceof Errors.EmailExistsError) { - const translation = req.i18n.translate( - 'email_already_registered' - ) - return HttpErrorHandler.conflict(req, res, translation) - } else { - return HttpErrorHandler.legacyInternal( - req, - res, - req.i18n.translate('problem_changing_email_address'), - OError.tag(err, 'problem_changing_email_address', { - userId, - newEmail, - }) - ) - } - } - User.findById(userId, (err, user) => { - if (err != null) { - logger.err( - { err, userId }, - 'error getting user for email update' - ) - return res.sendStatus(500) - } - SessionManager.setInSessionUser(req.session, { - email: user.email, - first_name: user.first_name, - last_name: user.last_name, - }) - UserHandler.populateTeamInvites(user, err => { - // need to refresh this in the background - if (err != null) { - logger.err({ err }, 'error populateTeamInvites') - } - res.sendStatus(200) - }) - }) - }) - } - }) + const user = await User.findById(userId).exec() + SessionManager.setInSessionUser(req.session, { + email: user.email, + first_name: user.first_name, + last_name: user.last_name, }) - }, - doLogout(req, cb) { - metrics.inc('user.logout') - const user = SessionManager.getSessionUser(req.session) - logger.debug({ user }, 'logging out') - const sessionId = req.sessionID - if (typeof req.logout === 'function') { - req.logout() - } // passport logout - req.session.destroy(err => { - if (err) { - OError.tag(err, 'error destroying session') - return cb(err) - } - if (user != null) { - UserSessionsManager.untrackSession(user, sessionId, () => {}) - } - cb() + try { + await UserHandler.promises.populateTeamInvites(user) + } catch (err) { + logger.error({ err }, 'error populateTeamInvites') + } + + res.sendStatus(200) + } +} + +async function doLogout(req) { + metrics.inc('user.logout') + const user = SessionManager.getSessionUser(req.session) + logger.debug({ user }, 'logging out') + const sessionId = req.sessionID + + // passport logout + if (typeof req.logout === 'function') { + req.logout() + } + + const destroySession = promisify(req.session.destroy.bind(req.session)) + await destroySession() + + if (user != null) { + UserSessionsManager.promises.untrackSession(user, sessionId).catch(err => { + logger.warn({ err, userId: user._id }, 'failed to untrack session') }) - }, + } +} - logout(req, res, next) { - const requestedRedirect = req.body.redirect - ? UrlHelper.getSafeRedirectPath(req.body.redirect) - : undefined - const redirectUrl = requestedRedirect || '/login' +async function logout(req, res, next) { + const requestedRedirect = req.body.redirect + ? UrlHelper.getSafeRedirectPath(req.body.redirect) + : undefined + const redirectUrl = requestedRedirect || '/login' - UserController.doLogout(req, err => { - if (err != null) { - return next(err) - } - if (acceptsJson(req)) { - res.status(200).json({ redir: redirectUrl }) - } else { - res.redirect(redirectUrl) - } - }) - }, + await doLogout(req) - expireDeletedUser(req, res, next) { - const userId = req.params.userId - UserDeleter.expireDeletedUser(userId, error => { - if (error) { - return next(error) - } + if (acceptsJson(req)) { + res.status(200).json({ redir: redirectUrl }) + } else { + res.redirect(redirectUrl) + } +} - res.sendStatus(204) - }) - }, +async function expireDeletedUser(req, res, next) { + const userId = req.params.userId + await UserDeleter.promises.expireDeletedUser(userId) + res.sendStatus(204) +} - expireDeletedUsersAfterDuration(req, res, next) { - UserDeleter.expireDeletedUsersAfterDuration(error => { - if (error) { - return next(error) - } - - res.sendStatus(204) - }) - }, +async function expireDeletedUsersAfterDuration(req, res, next) { + await UserDeleter.promises.expireDeletedUsersAfterDuration() + res.sendStatus(204) +} +module.exports = { + clearSessions: expressify(clearSessions), changePassword: expressify(changePassword), + tryDeleteUser: expressify(tryDeleteUser), + subscribe: expressify(subscribe), + unsubscribe: expressify(unsubscribe), + updateUserSettings: expressify(updateUserSettings), + doLogout: callbackify(doLogout), + logout: expressify(logout), + expireDeletedUser: expressify(expireDeletedUser), + expireDeletedUsersAfterDuration: expressify(expireDeletedUsersAfterDuration), + promises: { + doLogout, + ensureAffiliation, + ensureAffiliationMiddleware, + }, } - -UserController.promises = { - doLogout: promisify(UserController.doLogout), - ensureAffiliation, - ensureAffiliationMiddleware, -} - -module.exports = UserController diff --git a/services/web/app/src/Features/User/UserHandler.js b/services/web/app/src/Features/User/UserHandler.js index 0966664c33..7404e358bc 100644 --- a/services/web/app/src/Features/User/UserHandler.js +++ b/services/web/app/src/Features/User/UserHandler.js @@ -1,3 +1,4 @@ +const { promisifyAll } = require('@overleaf/promise-utils') const TeamInvitesHandler = require('../Subscription/TeamInvitesHandler') const UserHandler = { @@ -13,3 +14,4 @@ const UserHandler = { }, } module.exports = UserHandler +module.exports.promises = promisifyAll(UserHandler) diff --git a/services/web/app/src/Features/User/UserSessionsManager.js b/services/web/app/src/Features/User/UserSessionsManager.js index 8be80e1ae0..9e7685ce6d 100644 --- a/services/web/app/src/Features/User/UserSessionsManager.js +++ b/services/web/app/src/Features/User/UserSessionsManager.js @@ -243,6 +243,7 @@ const UserSessionsManager = { UserSessionsManager.promises = { getAllUserSessions: promisify(UserSessionsManager.getAllUserSessions), revokeAllUserSessions: promisify(UserSessionsManager.revokeAllUserSessions), + untrackSession: promisify(UserSessionsManager.untrackSession), } module.exports = UserSessionsManager diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 0e73c49716..cae14467fe 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -12,7 +12,7 @@ describe('UserController', function () { this.user = { _id: this.user_id, email: 'email@overleaf.com', - save: sinon.stub().callsArgWith(0), + save: sinon.stub().resolves(), ace: {}, } @@ -34,29 +34,37 @@ describe('UserController', function () { ip: '0:0:0:0', query: {}, headers: {}, + logger: { + addFields: sinon.stub(), + }, } - this.UserDeleter = { deleteUser: sinon.stub().yields() } + this.UserDeleter = { promises: { deleteUser: sinon.stub().resolves() } } + this.UserGetter = { - getUser: sinon.stub().yields(null, this.user), promises: { getUser: sinon.stub().resolves(this.user) }, } - this.User = { findById: sinon.stub().yields(null, this.user) } + + this.User = { + findById: sinon + .stub() + .returns({ exec: sinon.stub().resolves(this.user) }), + } + this.NewsLetterManager = { - subscribe: sinon.stub().yields(), - unsubscribe: sinon.stub().yields(), - } - this.AuthenticationController = { - establishUserSession: sinon.stub().callsArg(2), + promises: { + subscribe: sinon.stub().resolves(), + unsubscribe: sinon.stub().resolves(), + }, } + this.SessionManager = { getLoggedInUserId: sinon.stub().returns(this.user._id), getSessionUser: sinon.stub().returns(this.req.session.user), setInSessionUser: sinon.stub(), } + this.AuthenticationManager = { - authenticate: sinon.stub(), - validatePassword: sinon.stub(), promises: { authenticate: sinon.stub(), setUserPassword: sinon.stub(), @@ -65,24 +73,29 @@ describe('UserController', function () { .stub() .returns({ type: 'error', key: 'some-key' }), } + this.UserUpdater = { - changeEmailAddress: sinon.stub(), promises: { + changeEmailAddress: sinon.stub().resolves(), confirmEmail: sinon.stub().resolves(), addAffiliationForNewUser: sinon.stub().resolves(), }, } + this.settings = { siteUrl: 'sharelatex.example.com' } - this.UserHandler = { populateTeamInvites: sinon.stub().callsArgWith(1) } + + this.UserHandler = { + promises: { populateTeamInvites: sinon.stub().resolves() }, + } + this.UserSessionsManager = { - trackSession: sinon.stub(), - untrackSession: sinon.stub(), - revokeAllUserSessions: sinon.stub().callsArgWith(2, null), promises: { getAllUserSessions: sinon.stub().resolves(), revokeAllUserSessions: sinon.stub().resolves(), + untrackSession: sinon.stub().resolves(), }, } + this.HttpErrorHandler = { badRequest: sinon.stub(), conflict: sinon.stub(), @@ -97,7 +110,24 @@ describe('UserController', function () { .withArgs('https://evil.com') .returns(undefined) this.UrlHelper.getSafeRedirectPath.returnsArg(0) - this.acceptsJson = sinon.stub().returns(false) + + this.Features = { + hasFeature: sinon.stub(), + } + + this.UserAuditLogHandler = { + promises: { + addEntry: sinon.stub().resolves(), + }, + } + + this.RequestContentTypeDetection = { + acceptsJson: sinon.stub().returns(false), + } + + this.EmailHandler = { + promises: { sendEmail: sinon.stub().resolves() }, + } this.UserController = SandboxedModule.require(modulePath, { requires: { @@ -105,37 +135,22 @@ describe('UserController', function () { './UserGetter': this.UserGetter, './UserDeleter': this.UserDeleter, './UserUpdater': this.UserUpdater, - '../../models/User': { - User: this.User, - }, + '../../models/User': { User: this.User }, '../Newsletter/NewsletterManager': this.NewsLetterManager, '../Authentication/AuthenticationController': this.AuthenticationController, '../Authentication/SessionManager': this.SessionManager, '../Authentication/AuthenticationManager': this.AuthenticationManager, - '../../infrastructure/Features': (this.Features = { - hasFeature: sinon.stub(), - }), - './UserAuditLogHandler': (this.UserAuditLogHandler = { - promises: { - addEntry: sinon.stub().resolves(), - }, - }), + '../../infrastructure/Features': this.Features, + './UserAuditLogHandler': this.UserAuditLogHandler, './UserHandler': this.UserHandler, './UserSessionsManager': this.UserSessionsManager, '../Errors/HttpErrorHandler': this.HttpErrorHandler, '@overleaf/settings': this.settings, - '@overleaf/metrics': { - inc() {}, - }, '@overleaf/o-error': OError, - '../Email/EmailHandler': (this.EmailHandler = { - sendEmail: sinon.stub(), - promises: { sendEmail: sinon.stub().resolves() }, - }), - '../../infrastructure/RequestContentTypeDetection': { - acceptsJson: this.acceptsJson, - }, + '../Email/EmailHandler': this.EmailHandler, + '../../infrastructure/RequestContentTypeDetection': + this.RequestContentTypeDetection, }, }) @@ -154,13 +169,11 @@ describe('UserController', function () { beforeEach(function () { this.req.body.password = 'wat' this.req.logout = sinon.stub() - this.req.session.destroy = sinon.stub().callsArgWith(0, null) + this.req.session.destroy = sinon.stub().yields() this.SessionManager.getLoggedInUserId = sinon .stub() .returns(this.user._id) - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, null, this.user) + this.AuthenticationManager.promises.authenticate.resolves(this.user) }) it('should send 200', function (done) { @@ -173,10 +186,12 @@ describe('UserController', function () { it('should try to authenticate user', function (done) { this.res.sendStatus = code => { - this.AuthenticationManager.authenticate.callCount.should.equal(1) - this.AuthenticationManager.authenticate - .calledWith({ _id: this.user._id }, this.req.body.password) - .should.equal(true) + this.AuthenticationManager.promises.authenticate.should.have.been + .calledOnce + this.AuthenticationManager.promises.authenticate.should.have.been.calledWith( + { _id: this.user._id }, + this.req.body.password + ) done() } this.UserController.tryDeleteUser(this.req, this.res, this.next) @@ -184,8 +199,10 @@ describe('UserController', function () { it('should delete the user', function (done) { this.res.sendStatus = code => { - this.UserDeleter.deleteUser.callCount.should.equal(1) - this.UserDeleter.deleteUser.calledWith(this.user._id).should.equal(true) + this.UserDeleter.promises.deleteUser.should.have.been.calledOnce + this.UserDeleter.promises.deleteUser.should.have.been.calledWith( + this.user._id + ) done() } this.UserController.tryDeleteUser(this.req, this.res, this.next) @@ -207,9 +224,9 @@ describe('UserController', function () { describe('when authenticate produces an error', function () { beforeEach(function () { - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, new Error('woops')) + this.AuthenticationManager.promises.authenticate.rejects( + new Error('woops') + ) }) it('should call next with an error', function (done) { @@ -224,9 +241,7 @@ describe('UserController', function () { describe('when authenticate does not produce a user', function () { beforeEach(function () { - this.AuthenticationManager.authenticate = sinon - .stub() - .callsArgWith(2, null, null) + this.AuthenticationManager.promises.authenticate.resolves(null) }) it('should return 403', function (done) { @@ -240,7 +255,7 @@ describe('UserController', function () { describe('when deleteUser produces an error', function () { beforeEach(function () { - this.UserDeleter.deleteUser = sinon.stub().yields(new Error('woops')) + this.UserDeleter.promises.deleteUser.rejects(new Error('woops')) }) it('should call next with an error', function (done) { @@ -255,9 +270,9 @@ describe('UserController', function () { describe('when deleteUser produces a known error', function () { beforeEach(function () { - this.UserDeleter.deleteUser = sinon - .stub() - .yields(new Errors.SubscriptionAdminDeletionError()) + this.UserDeleter.promises.deleteUser.rejects( + new Errors.SubscriptionAdminDeletionError() + ) }) it('should return a HTTP Unprocessable Entity error', function (done) { @@ -298,9 +313,9 @@ describe('UserController', function () { it('should send the user to subscribe', function (done) { this.res.json = data => { expect(data.message).to.equal('thanks_settings_updated') - this.NewsLetterManager.subscribe - .calledWith(this.user) - .should.equal(true) + this.NewsLetterManager.promises.subscribe.should.have.been.calledWith( + this.user + ) done() } this.UserController.subscribe(this.req, this.res) @@ -311,12 +326,12 @@ describe('UserController', function () { it('should send the user to unsubscribe', function (done) { this.res.json = data => { expect(data.message).to.equal('thanks_settings_updated') - this.NewsLetterManager.unsubscribe - .calledWith(this.user) - .should.equal(true) + this.NewsLetterManager.promises.unsubscribe.should.have.been.calledWith( + this.user + ) done() } - this.UserController.unsubscribe(this.req, this.res) + this.UserController.unsubscribe(this.req, this.res, this.next) }) }) @@ -333,7 +348,7 @@ describe('UserController', function () { this.user.save.called.should.equal(true) done() } - this.UserController.updateUserSettings(this.req, this.res) + this.UserController.updateUserSettings(this.req, this.res, this.next) }) it('should set the first name', function (done) { @@ -401,12 +416,13 @@ describe('UserController', function () { it('should call the user updater with the new email and user _id', function (done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(3) this.res.sendStatus = code => { code.should.equal(200) - this.UserUpdater.changeEmailAddress - .calledWith(this.user_id, this.newEmail, this.auditLog) - .should.equal(true) + this.UserUpdater.promises.changeEmailAddress.should.have.been.calledWith( + this.user_id, + this.newEmail, + this.auditLog + ) done() } this.UserController.updateUserSettings(this.req, this.res) @@ -414,14 +430,15 @@ describe('UserController', function () { it('should update the email on the session', function (done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(3) let callcount = 0 - this.User.findById = (id, cb) => { - if (++callcount === 2) { - this.user.email = this.newEmail - } - cb(null, this.user) - } + this.User.findById = id => ({ + exec: async () => { + if (++callcount === 2) { + this.user.email = this.newEmail + } + return this.user + }, + }) this.res.sendStatus = code => { code.should.equal(200) this.SessionManager.setInSessionUser @@ -438,12 +455,11 @@ describe('UserController', function () { it('should call populateTeamInvites', function (done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(3) this.res.sendStatus = code => { code.should.equal(200) - this.UserHandler.populateTeamInvites - .calledWith(this.user) - .should.equal(true) + this.UserHandler.promises.populateTeamInvites.should.have.been.calledWith( + this.user + ) done() } this.UserController.updateUserSettings(this.req, this.res) @@ -452,7 +468,7 @@ describe('UserController', function () { describe('when changeEmailAddress yields an error', function () { it('should pass on an error and not send a success status', function (done) { this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith(3, new OError()) + this.UserUpdater.promises.changeEmailAddress.rejects(new OError()) this.HttpErrorHandler.legacyInternal = sinon.spy( (req, res, message, error) => { expect(req).to.exist @@ -473,8 +489,7 @@ describe('UserController', function () { done() }) this.req.body.email = this.newEmail.toUpperCase() - this.UserUpdater.changeEmailAddress.callsArgWith( - 3, + this.UserUpdater.promises.changeEmailAddress.rejects( new Errors.EmailExistsError() ) this.UserController.updateUserSettings(this.req, this.res) @@ -483,7 +498,6 @@ describe('UserController', function () { describe('when using an external auth source', function () { beforeEach(function () { - this.UserUpdater.changeEmailAddress.callsArgWith(2) this.newEmail = 'someone23@example.com' this.req.externalAuthenticationSystemUsed = sinon.stub().returns(true) }) @@ -492,7 +506,7 @@ describe('UserController', function () { this.req.body.email = this.newEmail this.res.sendStatus = code => { code.should.equal(200) - this.UserUpdater.changeEmailAddress + this.UserUpdater.promises.changeEmailAddress .calledWith(this.user_id, this.newEmail) .should.equal(false) done() @@ -504,7 +518,7 @@ describe('UserController', function () { describe('logout', function () { beforeEach(function () { - this.acceptsJson.returns(false) + this.RequestContentTypeDetection.acceptsJson.returns(false) }) it('should destroy the session', function (done) { @@ -522,10 +536,12 @@ describe('UserController', function () { this.req.session.destroy = sinon.stub().callsArgWith(0) this.res.redirect = url => { url.should.equal('/login') - this.UserSessionsManager.untrackSession.callCount.should.equal(1) - this.UserSessionsManager.untrackSession - .calledWith(sinon.match(this.req.user), this.req.sessionID) - .should.equal(true) + this.UserSessionsManager.promises.untrackSession.should.have.been + .calledOnce + this.UserSessionsManager.promises.untrackSession.should.have.been.calledWith( + sinon.match(this.req.user), + this.req.sessionID + ) done() } @@ -562,7 +578,7 @@ describe('UserController', function () { }) it('should send json with redir property for json request', function (done) { - this.acceptsJson.returns(true) + this.RequestContentTypeDetection.acceptsJson.returns(true) this.req.session.destroy = sinon.stub().callsArgWith(0) this.res.status = code => { code.should.equal(200) @@ -580,9 +596,8 @@ describe('UserController', function () { describe('success', function () { it('should call revokeAllUserSessions', function (done) { this.res.sendStatus.callsFake(() => { - this.UserSessionsManager.promises.revokeAllUserSessions.callCount.should.equal( - 1 - ) + this.UserSessionsManager.promises.revokeAllUserSessions.should.have + .been.calledOnce done() }) this.UserController.clearSessions(this.req, this.res) @@ -717,7 +732,7 @@ describe('UserController', function () { actionDescribed: `your password has been changed on your account ${this.user.email}`, action: 'password changed', } - const emailCall = this.EmailHandler.sendEmail.lastCall + const emailCall = this.EmailHandler.promises.sendEmail.lastCall expect(emailCall.args[0]).to.equal('securityAlert') expect(emailCall.args[1]).to.deep.equal(expectedArg) done() @@ -826,7 +841,7 @@ describe('UserController', function () { newPassword1: 'newpass', newPassword2: 'newpass', } - this.EmailHandler.sendEmail.yields(anError) + this.EmailHandler.promises.sendEmail.rejects(anError) }) it('should not return error but should log it', function (done) { this.res.json.callsFake(result => {