diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js index 79573e3a3a..2be8abd9d1 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationManager.js +++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js @@ -3,7 +3,6 @@ const { User } = require('../../models/User') const { db, ObjectId } = require('../../infrastructure/mongodb') const bcrypt = require('bcrypt') const EmailHelper = require('../Helpers/EmailHelper') -const V1Handler = require('../V1/V1Handler') const { InvalidEmailError, InvalidPasswordError @@ -66,8 +65,8 @@ const AuthenticationManager = { // validates a password based on a similar set of rules to `complexPassword.js` on the frontend // note that `passfield.js` enforces more rules than this, but these are the most commonly set. - // returns null on success, or an error string. - validatePassword(password) { + // returns null on success, or an error object. + validatePassword(password, email) { if (password == null) { return new InvalidPasswordError({ message: 'password not set', @@ -113,11 +112,23 @@ const AuthenticationManager = { info: { code: 'invalid_character' } }) } + if (typeof email === 'string' && email !== '') { + const startOfEmail = email.split('@')[0] + if ( + password.indexOf(email) !== -1 || + password.indexOf(startOfEmail) !== -1 + ) { + return new InvalidPasswordError({ + message: 'password contains part of email address', + info: { code: 'contains_email' } + }) + } + } return null }, - setUserPassword(userId, password, callback) { - AuthenticationManager.setUserPasswordInV2(userId, password, callback) + setUserPassword(user, password, callback) { + AuthenticationManager.setUserPasswordInV2(user, password, callback) }, checkRounds(user, hashedPassword, password, callback) { @@ -128,7 +139,7 @@ const AuthenticationManager = { // check current number of rounds and rehash if necessary const currentRounds = bcrypt.getRounds(hashedPassword) if (currentRounds < BCRYPT_ROUNDS) { - AuthenticationManager.setUserPassword(user._id, password, callback) + AuthenticationManager.setUserPassword(user, password, callback) } else { callback() } @@ -143,8 +154,11 @@ const AuthenticationManager = { }) }, - setUserPasswordInV2(userId, password, callback) { - const validationError = this.validatePassword(password) + setUserPasswordInV2(user, password, callback) { + if (!user || !user.email || !user._id) { + return callback(new Error('invalid user object')) + } + const validationError = this.validatePassword(password, user.email) if (validationError) { return callback(validationError) } @@ -154,7 +168,7 @@ const AuthenticationManager = { } db.users.updateOne( { - _id: ObjectId(userId.toString()) + _id: ObjectId(user._id.toString()) }, { $set: { @@ -174,20 +188,6 @@ const AuthenticationManager = { }) }, - setUserPasswordInV1(v1UserId, password, callback) { - const validationError = this.validatePassword(password) - if (validationError) { - return callback(validationError.message) - } - - V1Handler.doPasswordReset(v1UserId, password, function(error, reset) { - if (error) { - return callback(error) - } - callback(error, reset) - }) - }, - _passwordCharactersAreValid(password) { let digits, letters, lettersUp, symbols if ( diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetController.js b/services/web/app/src/Features/PasswordReset/PasswordResetController.js index 3af826546e..fd7a76d46e 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetController.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetController.js @@ -1,7 +1,6 @@ const PasswordResetHandler = require('./PasswordResetHandler') const RateLimiter = require('../../infrastructure/RateLimiter') const AuthenticationController = require('../Authentication/AuthenticationController') -const AuthenticationManager = require('../Authentication/AuthenticationManager') const UserGetter = require('../User/UserGetter') const UserUpdater = require('../User/UserUpdater') const UserSessionsManager = require('../User/UserSessionsManager') @@ -15,9 +14,6 @@ async function setNewUserPassword(req, res, next) { return res.sendStatus(400) } passwordResetToken = passwordResetToken.trim() - if (AuthenticationManager.validatePassword(password) != null) { - return res.sendStatus(400) - } delete req.session.resetToken const initiatorId = AuthenticationController.getLoggedInUserId(req) diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index abe9b96de1..e58630e4eb 100644 --- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js +++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js @@ -54,7 +54,7 @@ function getUserForPasswordResetToken(token, callback) { } UserGetter.getUserByMainEmail( data.email, - { _id: 1, 'overleaf.id': 1 }, + { _id: 1, 'overleaf.id': 1, email: 1 }, (err, user) => { if (err != null) { callback(err) @@ -93,6 +93,11 @@ async function setNewUserPassword(token, password, auditLog) { } } + const reset = await AuthenticationManager.promises.setUserPassword( + user, + password + ) + await UserAuditLogHandler.promises.addEntry( user._id, 'reset-password', @@ -100,11 +105,6 @@ async function setNewUserPassword(token, password, auditLog) { auditLog.ip ) - const reset = await AuthenticationManager.promises.setUserPassword( - user._id, - password - ) - return { found: true, reset, userId: user._id } } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index e85c071147..b7c689d4f3 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -85,13 +85,19 @@ async function changePassword(req, res, next) { 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) - } + try { + await AuthenticationManager.promises.setUserPassword( + user, + req.body.newPassword1 + ) + } catch (error) { + if (error.name === 'InvalidPasswordError') { + return HttpErrorHandler.badRequest(req, res, error.message) + } else { + throw error + } + } await UserAuditLogHandler.promises.addEntry( user._id, 'update-password', @@ -99,11 +105,6 @@ async function changePassword(req, res, next) { req.ip ) - await AuthenticationManager.promises.setUserPassword( - user._id, - req.body.newPassword1 - ) - // no need to wait, errors are logged and not passed back _sendSecurityAlertPasswordChanged(user) diff --git a/services/web/app/src/Features/User/UserRegistrationHandler.js b/services/web/app/src/Features/User/UserRegistrationHandler.js index 84578e1faa..ab18d59242 100644 --- a/services/web/app/src/Features/User/UserRegistrationHandler.js +++ b/services/web/app/src/Features/User/UserRegistrationHandler.js @@ -16,7 +16,8 @@ const UserRegistrationHandler = { _registrationRequestIsValid(body, callback) { const invalidEmail = AuthenticationManager.validateEmail(body.email || '') const invalidPassword = AuthenticationManager.validatePassword( - body.password || '' + body.password || '', + body.email ) if (invalidEmail != null || invalidPassword != null) { return false @@ -71,7 +72,7 @@ const UserRegistrationHandler = { ), cb => AuthenticationManager.setUserPassword( - user._id, + user, userDetails.password, cb ), diff --git a/services/web/app/views/user/settings.pug b/services/web/app/views/user/settings.pug index 1adf44e2e5..05ea39e0ef 100644 --- a/services/web/app/views/user/settings.pug +++ b/services/web/app/views/user/settings.pug @@ -283,4 +283,5 @@ block content span(ng-show="state.inflight") #{translate("deleting")}… script(type='text/javascript'). + window.usersEmail = !{StringHelper.stringifyJsonForScript(user.email)}; window.passwordStrengthOptions = !{StringHelper.stringifyJsonForScript(settings.passwordStrengthOptions || {})} diff --git a/services/web/frontend/js/main/affiliations/controllers/UserAffiliationsController.js b/services/web/frontend/js/main/affiliations/controllers/UserAffiliationsController.js index 359ee0ce3e..137c570e15 100644 --- a/services/web/frontend/js/main/affiliations/controllers/UserAffiliationsController.js +++ b/services/web/frontend/js/main/affiliations/controllers/UserAffiliationsController.js @@ -194,6 +194,7 @@ export default App.controller('UserAffiliationsController', function( email.default = false } userEmail.default = true + window.usersEmail = userEmail.email }) $scope.removeUserEmail = function(userEmail) { diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index 8e5d42df48..2aba52b711 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -20,7 +20,7 @@ class User { } ] this.email = this.emails[0].email - this.password = `acceptance-test-${count}-password` + this.password = `a-terrible-secret-${count}` count++ this.jar = request.jar() this.request = request.defaults({ @@ -111,21 +111,17 @@ class User { return callback(error) } this.setExtraAttributes(user) - AuthenticationManager.setUserPasswordInV2( - user._id, - this.password, - error => { + AuthenticationManager.setUserPasswordInV2(user, this.password, error => { + if (error != null) { + return callback(error) + } + this.mongoUpdate({ $set: { emails: this.emails } }, error => { if (error != null) { return callback(error) } - this.mongoUpdate({ $set: { emails: this.emails } }, error => { - if (error != null) { - return callback(error) - } - callback(null, this.password) - }) - } - ) + callback(null, this.password) + }) + }) }) } diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js index 032a556df2..c3c8edf615 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js @@ -26,7 +26,6 @@ describe('AuthenticationManager', function() { }, bcrypt: (this.bcrypt = {}), 'settings-sharelatex': this.settings, - '../V1/V1Handler': (this.V1Handler = {}), '../User/UserGetter': (this.UserGetter = {}), './AuthenticationErrors': AuthenticationErrors } @@ -99,6 +98,8 @@ describe('AuthenticationManager', function() { _id: '5c8791477192a80b5e76ca7e', email: (this.email = 'USER@sharelatex.com') } + this.db.users.updateOne = sinon + this.User.findOne = sinon.stub().callsArgWith(2, null, this.user) this.db.users.updateOne = sinon .stub() .callsArgWith(2, null, { modifiedCount: 1 }) @@ -106,7 +107,7 @@ describe('AuthenticationManager', function() { it('should not produce an error', function(done) { this.AuthenticationManager.setUserPasswordInV2( - this.user._id, + this.user, 'testpassword', (err, updated) => { expect(err).to.not.exist @@ -118,7 +119,7 @@ describe('AuthenticationManager', function() { it('should set the hashed password', function(done) { this.AuthenticationManager.setUserPasswordInV2( - this.user._id, + this.user, 'testpassword', err => { expect(err).to.not.exist @@ -224,7 +225,7 @@ describe('AuthenticationManager', function() { it('should set the users password (with a higher number of rounds)', function() { this.AuthenticationManager.setUserPassword - .calledWith('user-id', this.unencryptedPassword) + .calledWith(this.user, this.unencryptedPassword) .should.equal(true) }) @@ -258,7 +259,7 @@ describe('AuthenticationManager', function() { it('should not set the users password (with a higher number of rounds)', function() { this.AuthenticationManager.setUserPassword - .calledWith('user-id', this.unencryptedPassword) + .calledWith(this.user, this.unencryptedPassword) .should.equal(false) }) @@ -525,11 +526,16 @@ describe('AuthenticationManager', function() { describe('setUserPassword', function() { beforeEach(function() { this.user_id = ObjectId() + this.user = { + _id: this.user_id, + email: 'user@example.com' + } this.password = 'banana' this.hashedPassword = 'asdkjfa;osiuvandf' this.salt = 'saltaasdfasdfasdf' 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(2, null, this.user) this.db.users.updateOne = sinon.stub().callsArg(2) }) @@ -545,7 +551,7 @@ describe('AuthenticationManager', function() { it('should return and error', function(done) { this.AuthenticationManager.setUserPassword( - this.user_id, + this.user, this.password, err => { expect(err).to.exist @@ -556,7 +562,7 @@ describe('AuthenticationManager', function() { it('should not start the bcrypt process', function(done) { this.AuthenticationManager.setUserPassword( - this.user_id, + this.user, this.password, () => { this.bcrypt.genSalt.called.should.equal(false) @@ -567,6 +573,42 @@ describe('AuthenticationManager', function() { }) }) + describe('contains full email', function() { + beforeEach(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() + } + ) + }) + }) + + describe('contains first part of email', function() { + beforeEach(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() + } + ) + }) + }) + describe('too short', function() { beforeEach(function() { this.settings.passwordStrengthOptions = { @@ -580,7 +622,7 @@ describe('AuthenticationManager', function() { it('should return and error', function(done) { this.AuthenticationManager.setUserPassword( - this.user_id, + this.user, this.password, err => { expect(err).to.exist @@ -591,7 +633,7 @@ describe('AuthenticationManager', function() { it('should not start the bcrypt process', function(done) { this.AuthenticationManager.setUserPassword( - this.user_id, + this.user, this.password, () => { this.bcrypt.genSalt.called.should.equal(false) @@ -606,7 +648,7 @@ describe('AuthenticationManager', function() { beforeEach(function() { this.UserGetter.getUser = sinon.stub().yields(null, { overleaf: null }) this.AuthenticationManager.setUserPassword( - this.user_id, + this.user, this.password, this.callback ) diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js index a4d7b49689..d249bd0462 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetControllerTests.js @@ -44,9 +44,6 @@ describe('PasswordResetController', function() { revokeAllUserSessions: sinon.stub().resolves() } } - this.AuthenticationManager = { - validatePassword: sinon.stub() - } this.UserUpdater = { promises: { removeReconfirmFlag: sinon.stub().resolves() @@ -70,7 +67,6 @@ describe('PasswordResetController', function() { getLoggedInUserId: sinon.stub(), finishLogin: sinon.stub() }), - '../Authentication/AuthenticationManager': this.AuthenticationManager, '../User/UserGetter': (this.UserGetter = { promises: { getUser: sinon.stub() @@ -248,13 +244,13 @@ describe('PasswordResetController', function() { it('should return 400 (Bad Request) if the password is invalid', function(done) { this.req.body.password = 'correct horse battery staple' - this.AuthenticationManager.validatePassword = sinon - .stub() - .returns({ message: 'password contains invalid characters' }) + const err = new Error('bad') + err.name = 'InvalidPasswordError' + this.PasswordResetHandler.promises.setNewUserPassword.rejects(err) this.res.sendStatus = code => { code.should.equal(400) this.PasswordResetHandler.promises.setNewUserPassword.called.should.equal( - false + true ) done() } diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index a04d5c6899..7d16d75ee5 100644 --- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js +++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js @@ -39,7 +39,6 @@ describe('PasswordResetHandler', function() { } this.EmailHandler = { sendEmail: sinon.stub() } this.AuthenticationManager = { - setUserPasswordInV1: sinon.stub(), setUserPasswordInV2: sinon.stub(), promises: { setUserPassword: sinon.stub().resolves() @@ -227,7 +226,7 @@ describe('PasswordResetHandler', function() { email: this.email }) this.AuthenticationManager.promises.setUserPassword - .withArgs(this.user._id, this.password) + .withArgs(this.user, this.password) .resolves(true) }) @@ -355,18 +354,18 @@ describe('PasswordResetHandler', function() { new Error('oops') ) }) - it('should return the error and not update the password', function(done) { + it('should return the error', function(done) { this.PasswordResetHandler.setNewUserPassword( this.token, this.password, this.auditLog, - (error, result) => { + (error, _result) => { expect(error).to.exist expect( this.UserAuditLogHandler.promises.addEntry.callCount ).to.equal(1) expect(this.AuthenticationManager.promises.setUserPassword).to - .not.have.been.called + .have.been.called done() } ) @@ -386,7 +385,7 @@ describe('PasswordResetHandler', function() { email: this.email }) this.AuthenticationManager.promises.setUserPassword - .withArgs(this.user._id, this.password) + .withArgs(this.user, this.password) .resolves(true) }) diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index f073ab2bc8..63a0bb9887 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -664,7 +664,7 @@ describe('UserController', function() { 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, + this.user, 'newpass' ) done() @@ -749,9 +749,12 @@ describe('UserController', function() { }) 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.validatePassword = sinon + // .stub() + // .returns({ message: 'validation-error' }) + const err = new Error('bad') + err.name = 'InvalidPasswordError' + this.AuthenticationManager.promises.setUserPassword.rejects(err) this.AuthenticationManager.promises.authenticate.resolves({}) this.req.body = { newPassword1: 'newpass', @@ -761,10 +764,10 @@ describe('UserController', function() { expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith( this.req, this.res, - 'validation-error' + err.message ) this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( - 0 + 1 ) done() }) @@ -784,7 +787,7 @@ describe('UserController', function() { this.UserController.changePassword(this.req, this.res, error => { expect(error).to.be.instanceof(Error) this.AuthenticationManager.promises.setUserPassword.callCount.should.equal( - 0 + 1 ) done() }) diff --git a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js index 8765beb5ea..c3de866c36 100644 --- a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js +++ b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js @@ -208,7 +208,7 @@ describe('UserRegistrationHandler', function() { it('should set the password', function(done) { return this.handler.registerNewUser(this.passingRequest, err => { this.AuthenticationManager.setUserPassword - .calledWith(this.user._id, this.passingRequest.password) + .calledWith(this.user, this.passingRequest.password) .should.equal(true) return done() })