Merge pull request #11436 from overleaf/jk-increase-password-min-length-to-8

[web] Increase the minimum password length to 8 characters

GitOrigin-RevId: 94eb3c5605183b5e189babd3342dc308f403ebbd

services/web/app/src/Features/Authentication/AuthenticationManager.js

@@ -14,6 +14,7 @@ const util = require('util')
 const HaveIBeenPwned = require('./HaveIBeenPwned')
 const UserAuditLogHandler = require('../User/UserAuditLogHandler')
 const logger = require('@overleaf/logger')
+const Metrics = require('@overleaf/metrics')
 
 const BCRYPT_ROUNDS = Settings.security.bcryptRounds || 12
 const BCRYPT_MINOR_VERSION = Settings.security.bcryptMinorVersion || 'a'
@@ -38,6 +39,14 @@ function _validatePasswordNotTooLong(password) {
   return null
 }
 
+function _metricsForSuccessfulPasswordMatch(password) {
+  const validationResult = AuthenticationManager.validatePassword(password)
+  const status =
+    validationResult === null ? 'success' : validationResult?.info?.code
+  Metrics.inc('check-password', { status })
+  return null
+}
+
 const AuthenticationManager = {
   _checkUserPassword(query, password, callback) {
     // Using Mongoose for legacy reasons here. The returned User instance
@@ -54,6 +63,9 @@ const AuthenticationManager = {
         if (error) {
           return callback(error)
         }
+        if (match) {
+          _metricsForSuccessfulPasswordMatch(password)
+        }
         return callback(null, user, match)
       })
     })
@@ -157,7 +169,7 @@ const AuthenticationManager = {
       }
     }
     allowAnyChars = !!allowAnyChars
-    min = min || 6
+    min = min || 8
     max = max || 72
 
     // we don't support passwords > 72 characters in length, because bcrypt truncates them

services/web/app/views/user/setPassword.pug

@@ -63,7 +63,7 @@ block content
 						div(data-ol-hide-on-error-message="token-expired")
 							div #{translate('in_order_to_have_a_secure_account_make_sure_your_password')}
 							ul.mb-4.ps-4
-								li #{translate('is_longer_than_n_characters', {n: 6})}
+								li #{translate('is_longer_than_n_characters', {n: settings.passwordStrengthOptions.length.min})}
 								li #{translate('does_not_contain_or_significantly_match_your_email')}
 								li !{translate('is_not_a_common_or_obvious_password_as_defined_by_the_have_i_been_pwned_database', {}, [{name: 'a', attrs: {href: 'https://haveibeenpwned.com', rel: 'noopener noreferrer', target: '_blank'}}])}
 						.actions

services/web/config/settings.defaults.js

@@ -444,7 +444,7 @@ module.exports = {
   // opts are from http://antelle.github.io/passfield
   passwordStrengthOptions: {
     length: {
-      min: 6,
+      min: 8,
       // Bcrypt does not support longer passwords than that.
       max: 72,
     },

services/web/frontend/js/features/settings/components/password-section.tsx

@@ -128,7 +128,7 @@ function PasswordForm() {
         label={t('new_password')}
         value={newPassword1}
         handleChange={handleNewPassword1Change}
-        minLength={passwordStrengthOptions?.length?.min || 6}
+        minLength={passwordStrengthOptions?.length?.min || 8}
         autoComplete="new-password"
       />
       <PasswordFormGroup

services/web/test/unit/src/Authentication/AuthenticationManagerTests.js

@@ -12,6 +12,7 @@ describe('AuthenticationManager', function () {
   beforeEach(function () {
     tk.freeze(Date.now())
     this.settings = { security: { bcryptRounds: 4 } }
+    this.metrics = { inc: sinon.stub().returns() }
     this.AuthenticationManager = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/User': {
@@ -34,6 +35,7 @@ describe('AuthenticationManager', function () {
         '../User/UserAuditLogHandler': (this.UserAuditLogHandler = {
           addEntry: sinon.stub().callsArgWith(5, null),
         }),
+        '@overleaf/metrics': this.metrics,
       },
     })
     this.callback = sinon.stub()
@@ -63,6 +65,7 @@ describe('AuthenticationManager', function () {
         }
         this.user.hashedPassword = this.testPassword
         this.User.findOne = sinon.stub().callsArgWith(1, null, this.user)
+        this.metrics.inc.reset()
       })
 
       describe('when the hashed password matches', function () {
@@ -98,6 +101,12 @@ describe('AuthenticationManager', function () {
         it('should return the user', function () {
           this.callback.calledWith(null, this.user).should.equal(true)
         })
+
+        it('should send metrics', function () {
+          expect(
+            this.metrics.inc.calledWith('check-password', { status: 'success' })
+          ).to.equal(true)
+        })
       })
 
       describe('when the encrypted passwords do not match', function () {
@@ -128,6 +137,10 @@ describe('AuthenticationManager', function () {
         it('should not return the user', function () {
           this.callback.calledWith(null, null).should.equal(true)
         })
+
+        it('should not send metrics', function () {
+          expect(this.metrics.inc.called).to.equal(false)
+        })
       })
 
       describe('when another request runs in parallel', function () {
@@ -240,6 +253,7 @@ describe('AuthenticationManager', function () {
         }
         this.unencryptedPassword = 'banana'
         this.User.findOne = sinon.stub().callsArgWith(1, null, this.user)
+        this.metrics.inc.reset()
       })
 
       describe('when the hashed password matches', function () {
@@ -267,6 +281,14 @@ describe('AuthenticationManager', function () {
             .should.equal(true)
         })
 
+        it('should send metrics', function () {
+          expect(
+            this.metrics.inc.calledWith('check-password', {
+              status: 'too_short',
+            })
+          ).to.equal(true)
+        })
+
         it('should return the user', function () {
           this.callback.calledWith(null, this.user).should.equal(true)
         })
@@ -283,6 +305,10 @@ describe('AuthenticationManager', function () {
           )
         })
 
+        it('should not send metrics', function () {
+          expect(this.metrics.inc.called).to.equal(false)
+        })
+
         it('should not return the user', function () {
           this.callback.calledWith(null, null).should.equal(true)
           this.UserAuditLogHandler.addEntry.callCount.should.equal(0)
@@ -649,7 +675,7 @@ describe('AuthenticationManager', function () {
   describe('setUserPassword', function () {
     beforeEach(function () {
       this.user_id = ObjectId()
-      this.password = 'banana'
+      this.password = 'bananagram'
       this.hashedPassword = 'asdkjfa;osiuvandf'
       this.salt = 'saltaasdfasdfasdf'
       this.user = {