From d624c29b6fb60f120762193e0f79997bb4e75209 Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Fri, 14 Jun 2019 12:31:46 -0400 Subject: [PATCH] remove v1 deps for password change/reset GitOrigin-RevId: be25f19ae589c50bfde0b170860127fa8d6f63b7 --- .../Authentication/AuthenticationManager.js | 43 +-- .../PasswordReset/PasswordResetController.js | 127 +++---- .../PasswordReset/PasswordResetHandler.js | 144 +++----- .../app/src/Features/User/UserController.js | 127 +++---- services/web/app/views/user/setPassword.pug | 8 +- services/web/app/views/user/settings.pug | 5 +- .../AuthenticationManagerTests.js | 128 ++----- .../PasswordResetControllerTests.js | 6 +- .../PasswordResetHandlerTests.js | 335 +++++++----------- .../test/unit/src/User/UserControllerTests.js | 82 +++-- 10 files changed, 363 insertions(+), 642 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 5f75a8e67b..2094015203 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -134,48 +134,7 @@ module.exports = AuthenticationManager = { }, setUserPassword(user_id, password, callback) { - if (callback == null) { - callback = function(error, changed) {} - } - const validation = this.validatePassword(password) - if (validation != null) { - return callback(validation.message) - } - - return UserGetter.getUser(user_id, { email: 1, overleaf: 1 }, function( - error, - user - ) { - if (error != null) { - return callback(error) - } - const v1IdExists = - (user.overleaf != null ? user.overleaf.id : undefined) != null - if (v1IdExists && Settings.overleaf != null) { - // v2 user in v2 - // v2 user in v2, change password in v1 - return AuthenticationManager.setUserPasswordInV1( - user.overleaf.id, - password, - callback - ) - } else if (v1IdExists && Settings.overleaf == null) { - // v2 user in SL - return callback(new Errors.NotInV2Error('Password Reset Attempt')) - } else if (!v1IdExists && Settings.overleaf == null) { - // SL user in SL, change password in SL - return AuthenticationManager.setUserPasswordInV2( - user_id, - password, - callback - ) - } else if (!v1IdExists && Settings.overleaf != null) { - // SL user in v2, should not happen - return callback(new Errors.SLInV2Error('Password Reset Attempt')) - } else { - return callback(new Error('Password Reset Attempt Failed')) - } - }) + AuthenticationManager.setUserPasswordInV2(user_id, password, callback) }, checkRounds(user, hashedPassword, password, callback) { diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index bbbbc04dfa..4c87806d55 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -6,7 +6,6 @@ const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const UserSessionsManager = require('../User/UserSessionsManager') const logger = require('logger-sharelatex') -const Settings = require('settings-sharelatex') module.exports = { renderRequestResetForm(req, res) { @@ -22,23 +21,18 @@ module.exports = { subjectName: req.ip, throttle: 6 } - RateLimiter.addCount(opts, function(err, canContinue) { + RateLimiter.addCount(opts, (err, canContinue) => { if (err != null) { - return next(err) + res.send(500, { message: err.message }) } if (!canContinue) { return res.send(429, { message: req.i18n.translate('rate_limit_hit_wait') }) } - PasswordResetHandler.generateAndEmailResetToken(email, function( - err, - status - ) { + PasswordResetHandler.generateAndEmailResetToken(email, (err, status) => { if (err != null) { - res.send(500, { - message: err.message - }) + res.send(500, { message: err.message }) } else if (status === 'primary') { res.send(200, { message: { text: req.i18n.translate('password_reset_email_sent') } @@ -47,12 +41,6 @@ module.exports = { res.send(404, { message: req.i18n.translate('secondary_email_password_reset') }) - } else if (status === 'sharelatex') { - res.send(404, { - message: `${req.i18n.translate('reset_from_sl')}` - }) } else { res.send(404, { message: req.i18n.translate('cant_find_email') @@ -77,78 +65,61 @@ module.exports = { }, setNewUserPassword(req, res, next) { - const { passwordResetToken, password } = req.body - if ( - !password || - !passwordResetToken || - AuthenticationManager.validatePassword(password.trim()) != null - ) { + let { passwordResetToken, password } = req.body + if (!passwordResetToken || !password) { + return res.sendStatus(400) + } + password = password.trim() + passwordResetToken = passwordResetToken.trim() + if (AuthenticationManager.validatePassword(password) != null) { return res.sendStatus(400) } delete req.session.resetToken PasswordResetHandler.setNewUserPassword( - passwordResetToken.trim(), - password.trim(), - function(err, found, userId) { - if (err && err.name && err.name === 'NotFoundError') { - res.status(404).send('NotFoundError') - } else if (err && err.name && err.name === 'NotInV2Error') { - res.status(403).send('NotInV2Error') - } else if (err && err.name && err.name === 'SLInV2Error') { - res.status(403).send('SLInV2Error') - } else if (err && err.statusCode && err.statusCode === 500) { - res.status(500) - } else if (err && !err.statusCode) { - res.status(500) - } else if (found) { - if (userId == null) { - return res.sendStatus(200) - } // will not exist for v1-only users - UserSessionsManager.revokeAllUserSessions( - { _id: userId }, - [], - function(err) { + passwordResetToken, + password, + (err, found, userId) => { + if ((err && err.name === 'NotFoundError') || !found) { + return res.status(404).send('NotFoundError') + } else if (err) { + return res.status(500) + } + UserSessionsManager.revokeAllUserSessions({ _id: userId }, [], err => { + if (err != null) { + return next(err) + } + UserUpdater.removeReconfirmFlag(userId, err => { + if (err != null) { + return next(err) + } + if (!req.body.login_after) { + return res.sendStatus(200) + } + UserGetter.getUser(userId, { email: 1 }, (err, user) => { if (err != null) { return next(err) } - UserUpdater.removeReconfirmFlag(userId, function(err) { - if (err != null) { - return next(err) - } - if (req.body.login_after) { - UserGetter.getUser(userId, { email: 1 }, function(err, user) { - if (err != null) { - return next(err) - } - AuthenticationController.afterLoginSessionSetup( - req, - user, - function(err) { - if (err != null) { - logger.warn( - { err, email: user.email }, - 'Error setting up session after setting password' - ) - return next(err) - } - res.json({ - redir: - AuthenticationController._getRedirectFromSession( - req - ) || '/project' - }) - } + AuthenticationController.afterLoginSessionSetup( + req, + user, + err => { + if (err != null) { + logger.err( + { err, email: user.email }, + 'Error setting up session after setting password' ) + return next(err) + } + res.json({ + redir: + AuthenticationController._getRedirectFromSession(req) || + '/project' }) - } else { - res.sendStatus(200) } - }) - } - ) - } else { - res.sendStatus(404) - } + ) + }) + }) + }) } ) } diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index 570687c873..92815bbc58 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -3,81 +3,58 @@ const UserGetter = require('../User/UserGetter') const OneTimeTokenHandler = require('../Security/OneTimeTokenHandler') const EmailHandler = require('../Email/EmailHandler') const AuthenticationManager = require('../Authentication/AuthenticationManager') -const logger = require('logger-sharelatex') -const V1Api = require('../V1/V1Api') const PasswordResetHandler = { generateAndEmailResetToken(email, callback) { - PasswordResetHandler._getPasswordResetData(email, function( - error, - exists, - data - ) { - if (error != null) { - callback(error) - } else if (exists) { - OneTimeTokenHandler.getNewToken('password', data, function(err, token) { + UserGetter.getUserByAnyEmail(email, (err, user) => { + if (err || !user) { + return callback(err, null) + } + if (user.email !== email) { + return callback(null, 'secondary') + } + const data = { user_id: user._id.toString(), email: email } + OneTimeTokenHandler.getNewToken('password', data, (err, token) => { + if (err) { + return callback(err) + } + const emailOptions = { + to: email, + setNewPasswordUrl: `${ + settings.siteUrl + }/user/password/set?passwordResetToken=${token}&email=${encodeURIComponent( + email + )}` + } + EmailHandler.sendEmail('passwordResetRequested', emailOptions, err => { if (err) { return callback(err) } - const emailOptions = { - to: email, - setNewPasswordUrl: `${ - settings.siteUrl - }/user/password/set?passwordResetToken=${token}&email=${encodeURIComponent( - email - )}` - } - EmailHandler.sendEmail( - 'passwordResetRequested', - emailOptions, - function(error) { - if (error != null) { - return callback(error) - } - callback(null, 'primary') - } - ) + callback(null, 'primary') }) - } else { - UserGetter.getUserByAnyEmail(email, function(err, user) { - if (err != null) { - return callback(err) - } - if (!user) { - callback(null, null) - } else if (user.overleaf == null || user.overleaf.id == null) { - callback(null, 'sharelatex') - } else { - callback(null, 'secondary') - } - }) - } + }) }) }, setNewUserPassword(token, password, callback) { - PasswordResetHandler.getUserForPasswordResetToken( - token, - (err, user, version) => { - if (err != null) { - return callback(err) - } - if (user == null) { - return callback(null, false, null) - } - AuthenticationManager.setUserPassword( - user._id, - password, - (err, reset) => { - if (err) { - return callback(err) - } - callback(null, reset, user._id) - } - ) + PasswordResetHandler.getUserForPasswordResetToken(token, (err, user) => { + if (err) { + return callback(err) } - ) + if (!user) { + return callback(null, false, null) + } + AuthenticationManager.setUserPassword( + user._id, + password, + (err, reset) => { + if (err) { + return callback(err) + } + callback(null, reset, user._id) + } + ) + }) }, getUserForPasswordResetToken(token, callback) { @@ -107,13 +84,13 @@ const PasswordResetHandler = { data.user_id != null && data.user_id === user._id.toString() ) { - callback(null, user, 'v2') + callback(null, user) } else if ( data.v1_user_id != null && user.overleaf != null && data.v1_user_id === user.overleaf.id ) { - callback(null, user, 'v1') + callback(null, user) } else { callback(null, null) } @@ -121,43 +98,6 @@ const PasswordResetHandler = { ) } ) - }, - - _getPasswordResetData(email, callback) { - if (settings.overleaf != null) { - // Overleaf v2 - V1Api.request( - { - url: '/api/v1/sharelatex/user_emails', - qs: { - email - }, - expectedStatusCodes: [404] - }, - function(error, response, body) { - if (error != null) { - return callback(error) - } - if (response.statusCode === 404) { - callback(null, false) - } else { - callback(null, true, { v1_user_id: body.user_id, email: email }) - } - } - ) - } else { - // ShareLaTeX - UserGetter.getUserByMainEmail(email, function(err, user) { - if (err) { - return callback(err) - } - if (user == null || user.holdingAccount || user.overleaf != null) { - logger.err({ email }, 'user could not be found for password reset') - return callback(null, false) - } - callback(null, true, { user_id: user._id, email: email }) - }) - } } } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index ccdd70d8e7..043e995c24 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -29,6 +29,7 @@ const UserUpdater = require('./UserUpdater') const SudoModeHandler = require('../SudoMode/SudoModeHandler') const settings = require('settings-sharelatex') const Errors = require('../Errors/Errors') +const EmailHandler = require('../Email/EmailHandler') module.exports = UserController = { tryDeleteUser(req, res, next) { @@ -302,78 +303,80 @@ module.exports = UserController = { }, changePassword(req, res, next) { - if (next == null) { - next = function(error) {} - } metrics.inc('user.password-change') - const oldPass = req.body.currentPassword + const internalError = { + message: { type: 'error', text: req.i18n.translate('internal_error') } + } const user_id = AuthenticationController.getLoggedInUserId(req) - return AuthenticationManager.authenticate( + AuthenticationManager.authenticate( { _id: user_id }, - oldPass, - function(err, user) { - if (err != null) { - return next(err) + req.body.currentPassword, + (err, user) => { + if (err) { + return res.status(500).json(internalError) } - if (user) { - logger.log({ user: user._id }, 'changing password') - const { newPassword1 } = req.body - const { newPassword2 } = req.body - const validationError = AuthenticationManager.validatePassword( - newPassword1 - ) - if (newPassword1 !== newPassword2) { - logger.log({ user }, 'passwords do not match') - return res.send({ - message: { - type: 'error', - text: 'Your passwords do not match' - } - }) - } else if (validationError != null) { - logger.log({ user }, validationError.message) - return res.send({ - message: { - type: 'error', - text: validationError.message - } - }) - } else { - logger.log({ user }, 'password changed') - return AuthenticationManager.setUserPassword( - user._id, - newPassword1, - function(error) { - if (error != null) { - return next(error) - } - return UserSessionsManager.revokeAllUserSessions( - user, - [req.sessionID], - function(err) { - if (err != null) { - return next(err) - } - return res.send({ - message: { - type: 'success', - text: 'Your password has been changed' - } - }) - } - ) - } - ) - } - } else { - logger.log({ user_id }, 'current password wrong') - return res.send({ + if (!user) { + return res.status(400).json({ message: { type: 'error', text: 'Your old password is wrong' } }) } + if (req.body.newPassword1 !== req.body.newPassword2) { + return res.status(400).json({ + message: { + type: 'error', + text: req.i18n.translate('password_change_passwords_do_not_match') + } + }) + } + const validationError = AuthenticationManager.validatePassword( + req.body.newPassword1 + ) + if (validationError != null) { + return res.status(400).json({ + message: { + type: 'error', + text: validationError.message + } + }) + } + AuthenticationManager.setUserPassword( + user._id, + req.body.newPassword1, + err => { + if (err) { + return res.status(500).json(internalError) + } + // log errors but do not wait for response + EmailHandler.sendEmail( + 'passwordChanged', + { to: user.email }, + err => { + if (err) { + logger.warn(err) + } + } + ) + UserSessionsManager.revokeAllUserSessions( + user, + [req.sessionID], + err => { + if (err != null) { + return res.status(500).json(internalError) + } + res.json({ + message: { + type: 'success', + email: user.email, + text: req.i18n.translate('password_change_successful') + } + }) + } + ) + } + ) } ) } diff --git a/services/web/app/views/user/setPassword.pug b/services/web/app/views/user/setPassword.pug index 26a01f8152..4bec7dbe23 100644 --- a/services/web/app/views/user/setPassword.pug +++ b/services/web/app/views/user/setPassword.pug @@ -25,12 +25,8 @@ block content br a(href="/user/password/reset" style="text-decoration: underline;") | Request a new password reset email - .alert.alert-danger(ng-show="passwordResetForm.response.data == 'SLInV2Error'") - a(href=settings.accountMerge.sharelatexHost + "/user/password/reset") - | Please go to ShareLaTeX to reset your password - .alert.alert-danger(ng-show="passwordResetForm.response.data == 'NotInV2Error'") - a(href=settings.accountMerge.betaHost + "/user/password/reset" style="text-decoration: underline;") - | Please go to Overleaf to reset your password + .alert.alert-danger(ng-show="passwordResetForm.response.status == 400") + | #{translate('invalid_password')} .alert.alert-danger(ng-show="passwordResetForm.response.status == 500") | #{translate('error_performing_request')} diff --git a/services/web/app/views/user/settings.pug b/services/web/app/views/user/settings.pug index f9a2a9f09e..da0a413e57 100644 --- a/services/web/app/views/user/settings.pug +++ b/services/web/app/views/user/settings.pug @@ -98,10 +98,7 @@ block content | #[a(href="/user/password/reset", target='_blank') #{translate("no_existing_password")}] else - var submitAction - if settings.overleaf - - submitAction = '/user/change_password/v1' - else - - submitAction = '/user/password/update' + - submitAction = '/user/password/update' form( async-form="changepassword" name="changePasswordForm" diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 238dd1d5f6..5d210a3536 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -574,114 +574,40 @@ describe('AuthenticationManager', function() { }) }) - describe('password set attempt', function() { - describe('with SL user in SL', function() { - beforeEach(function() { - this.UserGetter.getUser = sinon - .stub() - .yields(null, { overleaf: null }) - return this.AuthenticationManager.setUserPassword( - this.user_id, - this.password, - this.callback - ) - }) + describe('successful password set attempt', function() { + beforeEach(function() { + this.UserGetter.getUser = sinon.stub().yields(null, { overleaf: null }) + this.AuthenticationManager.setUserPassword( + this.user_id, + this.password, + this.callback + ) + }) - it('should look up the user', function() { - return this.UserGetter.getUser - .calledWith(this.user_id) - .should.equal(true) + it("should update the user's password in the database", function() { + const { args } = this.db.users.update.lastCall + expect(args[0]).to.deep.equal({ + _id: ObjectId(this.user_id.toString()) }) - - it("should update the user's password in the database", function() { - const { args } = this.db.users.update.lastCall - expect(args[0]).to.deep.equal({ - _id: ObjectId(this.user_id.toString()) - }) - return expect(args[1]).to.deep.equal({ - $set: { - hashedPassword: this.hashedPassword - }, - $unset: { - password: true - } - }) - }) - - 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) - }) - - it('should call the callback', function() { - return this.callback.called.should.equal(true) + return expect(args[1]).to.deep.equal({ + $set: { + hashedPassword: this.hashedPassword + }, + $unset: { + password: true + } }) }) - describe('with SL user in v2', function() { - beforeEach(function(done) { - this.settings.overleaf = true - this.UserGetter.getUser = sinon - .stub() - .yields(null, { overleaf: null }) - return this.AuthenticationManager.setUserPassword( - this.user_id, - this.password, - (err, changed) => { - this.callback(err, changed) - return done() - } - ) - }) - it('should error', function() { - return this.callback - .calledWith(new Errors.SLInV2Error('Password Reset Attempt')) - .should.equal(true) - }) + 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) }) - describe('with v2 user in SL', function() { - beforeEach(function(done) { - this.UserGetter.getUser = sinon - .stub() - .yields(null, { overleaf: { id: 1 } }) - return this.AuthenticationManager.setUserPassword( - this.user_id, - this.password, - (err, changed) => { - this.callback(err, changed) - return done() - } - ) - }) - it('should error', function() { - return this.callback - .calledWith(new Errors.NotInV2Error('Password Reset Attempt')) - .should.equal(true) - }) - }) - - describe('with v2 user in v2', function() { - beforeEach(function(done) { - this.settings.overleaf = true - this.UserGetter.getUser = sinon - .stub() - .yields(null, { overleaf: { id: 1 } }) - this.V1Handler.doPasswordReset = sinon.stub().yields(null, true) - return this.AuthenticationManager.setUserPassword( - this.user_id, - this.password, - (err, changed) => { - this.callback(err, changed) - return done() - } - ) - }) - it('should set the password in v2', function() { - return this.callback.calledWith(null, true).should.equal(true) - }) + it('should call the callback', function() { + return this.callback.called.should.equal(true) }) }) }) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index 08c8fb3361..99706428aa 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js @@ -74,7 +74,7 @@ describe('PasswordResetController', function() { query: {} } - return (this.res = {}) + this.res = {} }) describe('requestReset', function() { @@ -202,9 +202,9 @@ describe('PasswordResetController', function() { false, this.user_id ) - this.res.sendStatus = code => { + this.res.status = code => { code.should.equal(404) - return done() + done() } return this.PasswordResetController.setNewUserPassword(this.req, this.res) }) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index 4542f86907..ddf3d1386b 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -1,11 +1,29 @@ -const { expect } = require('chai') +/* eslint-disable + handle-callback-err, + max-len, + no-return-assign, + no-unused-vars, +*/ +// 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 SandboxedModule = require('sandboxed-module') +const assert = require('assert') const path = require('path') const sinon = require('sinon') +const sinonChai = require('sinon-chai') +const chai = require('chai') const modulePath = path.join( __dirname, '../../../../app/src/Features/PasswordReset/PasswordResetHandler' ) +const should = require('chai').should() +chai.use(sinonChai) +const { expect } = chai describe('PasswordResetHandler', function() { beforeEach(function() { @@ -21,19 +39,17 @@ describe('PasswordResetHandler', function() { } this.EmailHandler = { sendEmail: sinon.stub() } this.AuthenticationManager = { - setUserPassword: sinon.stub() + setUserPassword: sinon.stub(), + setUserPasswordInV1: sinon.stub(), + setUserPasswordInV2: sinon.stub() } - this.V1Api = { request: sinon.stub() } this.PasswordResetHandler = SandboxedModule.require(modulePath, { - globals: { - console: console - }, + globals: { console: console }, requires: { '../User/UserGetter': this.UserGetter, '../Security/OneTimeTokenHandler': this.OneTimeTokenHandler, '../Email/EmailHandler': this.EmailHandler, '../Authentication/AuthenticationManager': this.AuthenticationManager, - '../V1/V1Api': this.V1Api, 'settings-sharelatex': this.settings, 'logger-sharelatex': { log() {}, @@ -42,128 +58,39 @@ describe('PasswordResetHandler', function() { } }) this.token = '12312321i' - this.email = 'bob@bob.com' - this.user = { - _id: 'user_id_here', - email: this.email - } + this.user_id = 'user_id_here' + this.user = { email: (this.email = 'bob@bob.com'), _id: 'user-id' } this.password = 'my great secret password' this.callback = sinon.stub() + // this should not have any effect now + this.settings.overleaf = true + }) + + afterEach(function() { + this.settings.overleaf = false }) describe('generateAndEmailResetToken', function() { - describe('when in ShareLaTeX', function() { - it('should check the user exists', function(done) { - this.UserGetter.getUserByMainEmail.callsArgWith(1) - this.UserGetter.getUserByAnyEmail.callsArgWith(1) - this.OneTimeTokenHandler.getNewToken.yields() - this.PasswordResetHandler.generateAndEmailResetToken( - this.user.email, - (err, status) => { - if (err) { - return done(err) - } - expect(status).to.be.null - done() - } - ) - }) - - it('should send the email with the token', function(done) { - this.UserGetter.getUserByMainEmail.callsArgWith(1, null, this.user) - this.OneTimeTokenHandler.getNewToken.yields(null, this.token) - this.EmailHandler.sendEmail.callsArgWith(2) - this.PasswordResetHandler.generateAndEmailResetToken( - this.user.email, - (err, status) => { - if (err) { - return done(err) - } - this.EmailHandler.sendEmail.called.should.equal(true) - this.OneTimeTokenHandler.getNewToken.should.have.been.calledWith( - 'password', - { - user_id: this.user._id, - email: this.email - } - ) - status.should.equal('primary') - const args = this.EmailHandler.sendEmail.args[0] - args[0].should.equal('passwordResetRequested') - args[1].setNewPasswordUrl.should.equal( - `${this.settings.siteUrl}/user/password/set?passwordResetToken=${ - this.token - }&email=${encodeURIComponent(this.user.email)}` - ) - done() - } - ) - }) - - it('should return exists == null for a holdingAccount', function(done) { - this.user.holdingAccount = true - this.UserGetter.getUserByMainEmail.callsArgWith(1, null, this.user) - this.UserGetter.getUserByAnyEmail.callsArgWith(1) - this.OneTimeTokenHandler.getNewToken.yields() - this.PasswordResetHandler.generateAndEmailResetToken( - this.user.email, - (err, status) => { - if (err) { - return done(err) - } - expect(status).to.be.null - done() - } - ) - }) - - it('should set the password token data to the user id and email', function() { - this.UserGetter.getUserByMainEmail.callsArgWith(1, null, this.user) - this.OneTimeTokenHandler.getNewToken.yields(null, this.token) - this.EmailHandler.sendEmail.callsArgWith(2) - }) + it('should check the user exists', function() { + this.UserGetter.getUserByAnyEmail.yields() + this.PasswordResetHandler.generateAndEmailResetToken( + this.user.email, + this.callback + ) + this.UserGetter.getUserByAnyEmail.should.have.been.calledWith( + this.user.email + ) }) - describe('when in overleaf', function() { - beforeEach(function() { - this.settings.overleaf = true - }) - - describe('when the email exists', function() { - beforeEach(function() { - this.V1Api.request.yields(null, {}, { user_id: 42 }) - this.OneTimeTokenHandler.getNewToken.yields(null, this.token) - this.EmailHandler.sendEmail.yields() - this.PasswordResetHandler.generateAndEmailResetToken( - this.email, - this.callback - ) - }) - - it('should call the v1 api for the user', function() { - this.V1Api.request - .calledWith({ - url: '/api/v1/sharelatex/user_emails', - qs: { - email: this.email - }, - expectedStatusCodes: [404] - }) - .should.equal(true) - }) - - it('should set the password token data to the user id and email', function() { - this.OneTimeTokenHandler.getNewToken.should.have.been.calledWith( - 'password', - { - v1_user_id: 42, - email: this.email - } - ) - }) - - it('should send an email with the token', function() { + it('should send the email with the token', function(done) { + this.UserGetter.getUserByAnyEmail.yields(null, this.user) + this.OneTimeTokenHandler.getNewToken.yields(null, this.token) + this.EmailHandler.sendEmail.yields() + this.PasswordResetHandler.generateAndEmailResetToken( + this.user.email, + (err, status) => { this.EmailHandler.sendEmail.called.should.equal(true) + status.should.equal('primary') const args = this.EmailHandler.sendEmail.args[0] args[0].should.equal('passwordResetRequested') args[1].setNewPasswordUrl.should.equal( @@ -171,110 +98,107 @@ describe('PasswordResetHandler', function() { this.token }&email=${encodeURIComponent(this.user.email)}` ) - }) + done() + } + ) + }) - it('should return status == true', function() { - this.callback.calledWith(null, 'primary').should.equal(true) - }) + describe('when the email exists', function() { + beforeEach(function() { + this.UserGetter.getUserByAnyEmail.yields(null, this.user) + this.OneTimeTokenHandler.getNewToken.yields(null, this.token) + this.EmailHandler.sendEmail.yields() + this.PasswordResetHandler.generateAndEmailResetToken( + this.email, + this.callback + ) }) - describe("when the email doesn't exist", function() { - beforeEach(function() { - this.V1Api.request = sinon - .stub() - .yields(null, { statusCode: 404 }, {}) - this.UserGetter.getUserByAnyEmail.callsArgWith(1) - this.PasswordResetHandler.generateAndEmailResetToken( - this.email, - this.callback - ) - }) - - it('should not set the password token data', function() { - this.OneTimeTokenHandler.getNewToken.called.should.equal(false) - }) - - it('should send an email with the token', function() { - this.EmailHandler.sendEmail.called.should.equal(false) - }) - - it('should return status == null', function() { - this.callback.calledWith(null, null).should.equal(true) - }) + it('should set the password token data to the user id and email', function() { + this.OneTimeTokenHandler.getNewToken.should.have.been.calledWith( + 'password', + { + email: this.email, + user_id: this.user._id + } + ) }) - describe("when the user isn't on v2", function() { - beforeEach(function() { - this.V1Api.request = sinon - .stub() - .yields(null, { statusCode: 404 }, {}) - this.UserGetter.getUserByAnyEmail.callsArgWith(1, null, this.user) - this.PasswordResetHandler.generateAndEmailResetToken( - this.email, - this.callback - ) - }) - - it('should not set the password token data', function() { - this.OneTimeTokenHandler.getNewToken.called.should.equal(false) - }) - - it('should not send an email with the token', function() { - this.EmailHandler.sendEmail.called.should.equal(false) - }) - - it('should return status == sharelatex', function() { - this.callback.calledWith(null, 'sharelatex').should.equal(true) - }) + it('should send an email with the token', function() { + this.EmailHandler.sendEmail.called.should.equal(true) + const args = this.EmailHandler.sendEmail.args[0] + args[0].should.equal('passwordResetRequested') + args[1].setNewPasswordUrl.should.equal( + `${this.settings.siteUrl}/user/password/set?passwordResetToken=${ + this.token + }&email=${encodeURIComponent(this.user.email)}` + ) }) - describe('when the email is a secondary email', function() { - beforeEach(function() { - this.V1Api.request = sinon - .stub() - .yields(null, { statusCode: 404 }, {}) - this.user.overleaf = { id: 101 } - this.UserGetter.getUserByAnyEmail.callsArgWith(1, null, this.user) - this.PasswordResetHandler.generateAndEmailResetToken( - this.email, - this.callback - ) - }) + it('should return status == true', function() { + this.callback.calledWith(null, 'primary').should.equal(true) + }) + }) - it('should not set the password token data', function() { - this.OneTimeTokenHandler.getNewToken.called.should.equal(false) - }) + describe("when the email doesn't exist", function() { + beforeEach(function() { + this.UserGetter.getUserByAnyEmail.yields(null, null) + this.PasswordResetHandler.generateAndEmailResetToken( + this.email, + this.callback + ) + }) - it('should not send an email with the token', function() { - this.EmailHandler.sendEmail.called.should.equal(false) - }) + it('should not set the password token data', function() { + this.OneTimeTokenHandler.getNewToken.called.should.equal(false) + }) - it('should return status == secondary', function() { - this.callback.calledWith(null, 'secondary').should.equal(true) - }) + it('should send an email with the token', function() { + this.EmailHandler.sendEmail.called.should.equal(false) + }) + + it('should return status == null', function() { + this.callback.calledWith(null, null).should.equal(true) + }) + }) + + describe('when the email is a secondary email', function() { + beforeEach(function() { + this.UserGetter.getUserByAnyEmail.callsArgWith(1, null, this.user) + this.PasswordResetHandler.generateAndEmailResetToken( + 'secondary@email.com', + this.callback + ) + }) + + it('should not set the password token data', function() { + this.OneTimeTokenHandler.getNewToken.called.should.equal(false) + }) + + it('should not send an email with the token', function() { + this.EmailHandler.sendEmail.called.should.equal(false) + }) + + it('should return status == secondary', function() { + this.callback.calledWith(null, 'secondary').should.equal(true) }) }) }) describe('setNewUserPassword', function() { - describe('when no token is found', function() { + describe('when no data is found', function() { beforeEach(function() { this.OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, null) - }) - - it('should return found == false when', function(done) { this.PasswordResetHandler.setNewUserPassword( this.token, this.password, - (err, found) => { - if (err != null) { - return done(err) - } - expect(found).to.be.false - done() - } + this.callback ) }) + + it('should return exists == false', function() { + this.callback.calledWith(null, false).should.equal(true) + }) }) describe('when the token has a user_id and email', function() { @@ -336,8 +260,9 @@ describe('PasswordResetHandler', function() { describe('when the email and user match', function() { beforeEach(function() { - this.UserGetter.getUserByMainEmail - .withArgs(this.email) + this.PasswordResetHandler.getUserForPasswordResetToken = sinon + .stub() + .withArgs(this.token) .yields(null, this.user) }) diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index ca0f49425b..bab0132f52 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -42,7 +42,10 @@ describe('UserController', function() { email: 'old@something.com' } }, - body: {} + body: {}, + i18n: { + translate: text => text + } } this.UserDeleter = { deleteUser: sinon.stub().callsArgWith(1) } @@ -109,10 +112,13 @@ describe('UserController', function() { this.res = { send: sinon.stub(), + status: sinon.stub(), sendStatus: sinon.stub(), json: sinon.stub() } - return (this.next = sinon.stub()) + this.res.status.returns(this.res) + this.next = sinon.stub() + this.callback = sinon.stub() }) describe('tryDeleteUser', function() { @@ -511,62 +517,60 @@ describe('UserController', function() { }) describe('changePassword', function() { - it('should check the old password is the current one at the moment', function(done) { - this.AuthenticationManager.authenticate.callsArgWith(2) + it('should check the old password is the current one at the moment', function() { + this.AuthenticationManager.authenticate.yields() this.req.body = { currentPassword: 'oldpasshere' } - this.res.send = () => { - this.AuthenticationManager.authenticate - .calledWith({ _id: this.user._id }, 'oldpasshere') - .should.equal(true) - this.AuthenticationManager.setUserPassword.called.should.equal(false) - return done() - } - return this.UserController.changePassword(this.req, this.res) + this.UserController.changePassword(this.req, this.res, this.callback) + this.AuthenticationManager.authenticate.should.have.been.calledWith( + { _id: this.user._id }, + 'oldpasshere' + ) + this.AuthenticationManager.setUserPassword.callCount.should.equal(0) }) - it('it should not set the new password if they do not match', function(done) { - this.AuthenticationManager.authenticate.callsArgWith(2, null, {}) + it('it should not set the new password if they do not match', function() { + this.AuthenticationManager.authenticate.yields(null, {}) this.req.body = { newPassword1: '1', newPassword2: '2' } - this.res.send = () => { - this.AuthenticationManager.setUserPassword.called.should.equal(false) - return done() - } - return this.UserController.changePassword(this.req, this.res) + this.UserController.changePassword(this.req, this.res, this.callback) + this.res.status.should.have.been.calledWith(400) + this.AuthenticationManager.setUserPassword.callCount.should.equal(0) }) - it('should set the new password if they do match', function(done) { - this.AuthenticationManager.authenticate.callsArgWith(2, null, this.user) - this.AuthenticationManager.setUserPassword.callsArgWith(2) + it('should set the new password if they do match', function() { + this.AuthenticationManager.authenticate.yields(null, this.user) + this.AuthenticationManager.setUserPassword.yields() this.req.body = { newPassword1: 'newpass', newPassword2: 'newpass' } - this.res.send = () => { - this.AuthenticationManager.setUserPassword - .calledWith(this.user._id, 'newpass') - .should.equal(true) - return done() - } - return this.UserController.changePassword(this.req, this.res) + this.UserController.changePassword(this.req, this.res, this.callback) + this.AuthenticationManager.setUserPassword.should.have.been.calledWith( + this.user._id, + 'newpass' + ) }) - it('it should not set the new password if it is invalid', function(done) { + it('it should not set the new password if it is invalid', function() { this.AuthenticationManager.validatePassword = sinon .stub() - .returns({ message: 'password contains invalid characters' }) - this.AuthenticationManager.authenticate.callsArgWith(2, null, {}) + .returns({ message: 'validation-error' }) + this.AuthenticationManager.authenticate.yields(null, {}) this.req.body = { - newPassword1: 'correct horse battery staple', - newPassword2: 'correct horse battery staple' + newPassword1: 'newpass', + newPassword2: 'newpass' } - this.res.send = () => { - this.AuthenticationManager.setUserPassword.called.should.equal(false) - return done() - } - return this.UserController.changePassword(this.req, this.res) + this.UserController.changePassword(this.req, this.res, this.callback) + this.AuthenticationManager.setUserPassword.callCount.should.equal(0) + this.res.status.should.have.been.calledWith(400) + this.res.json.should.have.been.calledWith({ + message: { + type: 'error', + text: 'validation-error' + } + }) }) }) })