diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index ee70ec7274..e9811646b0 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -220,7 +220,8 @@ const AuthenticationManager = { AuthenticationManager.promises = { authenticate: util.promisify(AuthenticationManager.authenticate), - hashPassword: util.promisify(AuthenticationManager.hashPassword) + hashPassword: util.promisify(AuthenticationManager.hashPassword), + setUserPassword: util.promisify(AuthenticationManager.setUserPassword) } module.exports = AuthenticationManager diff --git a/services/web/app/src/Features/Email/EmailBuilder.js b/services/web/app/src/Features/Email/EmailBuilder.js index afb8a9ab08..62599d7576 100644 --- a/services/web/app/src/Features/Email/EmailBuilder.js +++ b/services/web/app/src/Features/Email/EmailBuilder.js @@ -225,21 +225,6 @@ If you didn't request a password reset, let us know.\ } }) -templates.passwordChanged = NoCTAEmailTemplate({ - subject(opts) { - return `Password Changed - ${settings.appName}` - }, - title(opts) { - return `Password Changed` - }, - message(opts) { - return [ - `We're contacting you to notify you that your password has been set or changed.`, - `If you have recently set your password for the first time, or if you just changed your password, you don't need to take any further action. If you didn't set or change your password, please contact us.` - ] - } -}) - templates.confirmEmail = CTAEmailTemplate({ subject() { return `Confirm Email - ${settings.appName}` diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index 1a1a3acb8f..fce5a94027 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -29,6 +29,71 @@ async function _ensureAffiliation(userId, emailData) { } } +async function changePassword(req, res, next) { + metrics.inc('user.password-change') + const userId = AuthenticationController.getLoggedInUserId(req) + + const user = await AuthenticationManager.promises.authenticate( + { _id: userId }, + req.body.currentPassword + ) + if (!user) { + return HttpErrorHandler.badRequest(req, res, 'Your old password is wrong') + } + + if (req.body.newPassword1 !== req.body.newPassword2) { + return HttpErrorHandler.badRequest( + req, + res, + req.i18n.translate('password_change_passwords_do_not_match') + ) + } + const validationError = AuthenticationManager.validatePassword( + req.body.newPassword1 + ) + if (validationError != null) { + return HttpErrorHandler.badRequest(req, res, validationError.message) + } + + await UserAuditLogHandler.promises.addEntry( + user._id, + 'update-password', + user._id, + req.ip + ) + + await AuthenticationManager.promises.setUserPassword( + user._id, + req.body.newPassword1 + ) + + const emailOptions = { + to: user.email, + actionDescribed: `your password has been changed on your account ${ + user.email + }`, + action: 'password changed' + } + EmailHandler.sendEmail('securityAlert', emailOptions, error => { + if (error) { + // log error when sending security alert email but do not pass back + logger.error({ err: error }) + } + }) + + await UserSessionsManager.promises.revokeAllUserSessions(user, [ + req.sessionID + ]) + + return res.json({ + message: { + type: 'success', + email: user.email, + text: req.i18n.translate('password_change_successful') + } + }) +} + async function clearSessions(req, res, next) { metrics.inc('user.clear-sessions') const user = AuthenticationController.getSessionUser(req) @@ -394,84 +459,7 @@ const UserController = { ) }, - changePassword(req, res, next) { - metrics.inc('user.password-change') - const internalError = { - message: { type: 'error', text: req.i18n.translate('internal_error') } - } - const userId = AuthenticationController.getLoggedInUserId(req) - AuthenticationManager.authenticate( - { _id: userId }, - req.body.currentPassword, - (err, user) => { - if (err) { - return res.status(500).json(internalError) - } - 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') - } - }) - } - ) - } - ) - } - ) - } + changePassword: expressify(changePassword) } UserController.promises = { diff --git a/services/web/test/acceptance/src/PasswordUpdateTests.js b/services/web/test/acceptance/src/PasswordUpdateTests.js new file mode 100644 index 0000000000..b0e0ad6bc4 --- /dev/null +++ b/services/web/test/acceptance/src/PasswordUpdateTests.js @@ -0,0 +1,133 @@ +const { expect } = require('chai') +const RateLimiter = require('../../../app/src/infrastructure/RateLimiter') +const UserHelper = require('./helpers/UserHelper') + +describe('PasswordUpdate', function() { + let email, password, response, user, userHelper + afterEach(async function() { + await RateLimiter.promises.clearRateLimit( + 'password_reset_rate_limit', + '127.0.0.1' + ) + }) + beforeEach(async function() { + userHelper = new UserHelper() + email = userHelper.getDefaultEmail() + password = 'old-password' + userHelper = await UserHelper.createUser({ email, password }) + userHelper = await UserHelper.loginUser({ + email, + password + }) + await userHelper.getCsrfToken() + }) + describe('success', function() { + beforeEach(async function() { + response = await userHelper.request.post('/user/password/update', { + form: { + currentPassword: password, + newPassword1: 'new-password', + newPassword2: 'new-password' + }, + simple: false + }) + user = (await UserHelper.getUser({ email })).user + }) + it('should return 200', async function() { + expect(response.statusCode).to.equal(200) + }) + it('should update the audit log', function() { + expect(user.auditLog[0]).to.exist + expect(typeof user.auditLog[0].initiatorId).to.equal('object') + expect(user.auditLog[0].initiatorId).to.deep.equal(user._id) + expect(user.auditLog[0].operation).to.equal('update-password') + expect(user.auditLog[0].ipAddress).to.equal('127.0.0.1') + expect(user.auditLog[0].timestamp).to.exist + }) + }) + describe('errors', function() { + describe('missing current password', function() { + beforeEach(async function() { + response = await userHelper.request.post('/user/password/update', { + form: { + newPassword1: 'new-password', + newPassword2: 'new-password' + }, + simple: false + }) + user = (await UserHelper.getUser({ email })).user + }) + it('should return 500', async function() { + expect(response.statusCode).to.equal(500) + }) + it('should not update audit log', async function() { + expect(user.auditLog[0]).to.not.exist + }) + }) + describe('wrong current password', function() { + beforeEach(async function() { + response = await userHelper.request.post('/user/password/update', { + form: { + currentPassword: 'wrong-password', + newPassword1: 'new-password', + newPassword2: 'new-password' + }, + simple: false + }) + user = (await UserHelper.getUser({ email })).user + }) + it('should return 400', async function() { + expect(response.statusCode).to.equal(400) + }) + it('should not update audit log', async function() { + expect(user.auditLog[0]).to.not.exist + }) + }) + describe('newPassword1 does not match newPassword2', function() { + beforeEach(async function() { + response = await userHelper.request.post('/user/password/update', { + form: { + currentPassword: password, + newPassword1: 'new-password', + newPassword2: 'oops-password' + }, + json: true, + simple: false + }) + user = (await UserHelper.getUser({ email })).user + }) + it('should return 400', async function() { + expect(response.statusCode).to.equal(400) + }) + it('should return error message', async function() { + expect(response.body.message).to.equal('Passwords do not match') + }) + it('should not update audit log', async function() { + expect(user.auditLog[0]).to.not.exist + }) + }) + describe('new password is not valid', function() { + beforeEach(async function() { + response = await userHelper.request.post('/user/password/update', { + form: { + currentPassword: password, + newPassword1: 'short', + newPassword2: 'short' + }, + json: true, + simple: false + }) + user = (await UserHelper.getUser({ email })).user + }) + it('should return 400', async function() { + expect(response.statusCode).to.equal(400) + }) + it('should return error message', async function() { + expect(response.body.message).to.equal('password is too short') + }) + it('should not update audit log', async function() { + expect(user.auditLog[0]).to.not.exist + }) + }) + }) +}) diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index 9498245a34..b91aae5f9d 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -13,6 +13,7 @@ describe('UserController', function() { this.user = { _id: this.user_id, + email: 'email@overleaf.com', save: sinon.stub().callsArgWith(0), ace: {} } @@ -31,6 +32,7 @@ describe('UserController', function() { i18n: { translate: text => text }, + ip: '0:0:0:0', query: {} } @@ -50,8 +52,11 @@ describe('UserController', function() { } this.AuthenticationManager = { authenticate: sinon.stub(), - setUserPassword: sinon.stub(), - validatePassword: sinon.stub() + validatePassword: sinon.stub(), + promises: { + authenticate: sinon.stub(), + setUserPassword: sinon.stub() + } } this.ReferalAllocator = { allocate: sinon.stub() } this.SubscriptionDomainHandler = { autoAllocate: sinon.stub() } @@ -75,6 +80,7 @@ describe('UserController', function() { } this.SudoModeHandler = { clearSudoMode: sinon.stub() } this.HttpErrorHandler = { + badRequest: sinon.stub(), conflict: sinon.stub(), unprocessableEntity: sinon.stub(), legacyInternal: sinon.stub() @@ -648,60 +654,164 @@ describe('UserController', function() { }) describe('changePassword', function() { - it('should check the old password is the current one at the moment', function() { - this.AuthenticationManager.authenticate.yields() - this.req.body = { currentPassword: 'oldpasshere' } - 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() { - this.AuthenticationManager.authenticate.yields(null, {}) - this.req.body = { - newPassword1: '1', - newPassword2: '2' - } - 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() { - this.AuthenticationManager.authenticate.yields(null, this.user) - this.AuthenticationManager.setUserPassword.yields() - this.req.body = { - newPassword1: 'newpass', - newPassword2: 'newpass' - } - 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() { - this.AuthenticationManager.validatePassword = sinon - .stub() - .returns({ message: 'validation-error' }) - this.AuthenticationManager.authenticate.yields(null, {}) - this.req.body = { - newPassword1: 'newpass', - newPassword2: 'newpass' - } - 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' + describe('success', function() { + beforeEach(function() { + this.AuthenticationManager.promises.authenticate.resolves(this.user) + this.AuthenticationManager.promises.setUserPassword.resolves() + this.req.body = { + newPassword1: 'newpass', + newPassword2: 'newpass' } }) + it('should set the new password if they do match', function(done) { + this.res.json.callsFake(() => { + this.AuthenticationManager.promises.setUserPassword.should.have.been.calledWith( + this.user._id, + 'newpass' + ) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + + it('should log the update', function(done) { + this.res.json.callsFake(() => { + this.UserAuditLogHandler.promises.addEntry.should.have.been.calledWith( + this.user._id, + 'update-password', + this.user._id, + this.req.ip + ) + this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( + 1 + ) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + + it('should send security alert email', function(done) { + this.res.json.callsFake(() => { + const expectedArg = { + to: this.user.email, + actionDescribed: `your password has been changed on your account ${ + this.user.email + }`, + action: 'password changed' + } + const emailCall = this.EmailHandler.sendEmail.lastCall + expect(emailCall.args[0]).to.equal('securityAlert') + expect(emailCall.args[1]).to.deep.equal(expectedArg) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + }) + + describe('errors', function() { + it('should check the old password is the current one at the moment', function(done) { + this.AuthenticationManager.promises.authenticate.resolves() + this.req.body = { currentPassword: 'oldpasshere' } + this.HttpErrorHandler.badRequest.callsFake(() => { + expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith( + this.req, + this.res, + 'Your old password is wrong' + ) + this.AuthenticationManager.promises.authenticate.should.have.been.calledWith( + { _id: this.user._id }, + 'oldpasshere' + ) + this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( + 0 + ) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + + it('it should not set the new password if they do not match', function(done) { + this.AuthenticationManager.promises.authenticate.resolves({}) + this.req.body = { + newPassword1: '1', + newPassword2: '2' + } + this.HttpErrorHandler.badRequest.callsFake(() => { + expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith( + this.req, + this.res, + 'password_change_passwords_do_not_match' + ) + this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( + 0 + ) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + + it('it should not set the new password if it is invalid', function(done) { + this.AuthenticationManager.validatePassword = sinon + .stub() + .returns({ message: 'validation-error' }) + this.AuthenticationManager.promises.authenticate.resolves({}) + this.req.body = { + newPassword1: 'newpass', + newPassword2: 'newpass' + } + this.HttpErrorHandler.badRequest.callsFake(() => { + expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith( + this.req, + this.res, + 'validation-error' + ) + this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( + 0 + ) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + + describe('UserAuditLogHandler error', function() { + it('should return error and not update password', function(done) { + this.UserAuditLogHandler.promises.addEntry.rejects(new Error('oops')) + this.AuthenticationManager.promises.authenticate.resolves(this.user) + this.AuthenticationManager.promises.setUserPassword.resolves() + this.req.body = { + newPassword1: 'newpass', + newPassword2: 'newpass' + } + + this.UserController.changePassword(this.req, this.res, error => { + expect(error).to.be.instanceof(Error) + this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( + 0 + ) + done() + }) + }) + }) + + describe('EmailHandler error', function() { + beforeEach(function() { + this.AuthenticationManager.promises.authenticate.resolves(this.user) + this.AuthenticationManager.promises.setUserPassword.resolves() + this.req.body = { + newPassword1: 'newpass', + newPassword2: 'newpass' + } + this.EmailHandler.sendEmail.yields(new Error('oops')) + }) + it('should not return error but should log it', function(done) { + this.res.json.callsFake(result => { + expect(result.message.type).to.equal('success') + this.logger.error.callCount.should.equal(1) + done() + }) + this.UserController.changePassword(this.req, this.res) + }) + }) }) })