Merge pull request #3234 from overleaf/sk-fix-password-validation-email

Overhaul password validation

GitOrigin-RevId: a591c4e192e30a0ac053eab6f80627543a8a92fe

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 (

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)

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 }
 }
 

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)
 

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
               ),

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 || {})}

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) {

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)
+        })
+      })
     })
   }
 

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
         )

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()
       }

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)
       })
 

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()
           })

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()
         })