Merge pull request #12342 from overleaf/jk-password-ux-please-use-another-password

[web] Password UX: 'Please use another password'

GitOrigin-RevId: ca9b26cbcf2dabb27c716da314764ee40ffc83dd

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

@@ -402,6 +402,48 @@ const AuthenticationManager = {
       }
     }
   },
+
+  getMessageForInvalidPasswordError(error, req) {
+    const errorCode = error?.info?.code
+    const message = {
+      type: 'error',
+    }
+    switch (errorCode) {
+      case 'not_set':
+        message.key = 'password-not-set'
+        message.text = req.i18n.translate('invalid_password_not_set')
+        break
+      case 'invalid_character':
+        message.key = 'password-invalid-character'
+        message.text = req.i18n.translate('invalid_password_invalid_character')
+        break
+      case 'contains_email':
+        message.key = 'password-contains-email'
+        message.text = req.i18n.translate('invalid_password_contains_email')
+        break
+      case 'too_similar':
+        message.key = 'password-too-similar'
+        message.text = req.i18n.translate('invalid_password_too_similar')
+        break
+      case 'too_short':
+        message.key = 'password-too-short'
+        message.text = req.i18n.translate('invalid_password_too_short', {
+          minLength: Settings.passwordStrengthOptions?.length?.min || 8,
+        })
+        break
+      case 'too_long':
+        message.key = 'password-too-long'
+        message.text = req.i18n.translate('invalid_password_too_long', {
+          maxLength: Settings.passwordStrengthOptions?.length?.max || 72,
+        })
+        break
+      default:
+        logger.error({ err: error }, 'Unknown password validation error code')
+        message.text = req.i18n.translate('invalid_password')
+        break
+    }
+    return message
+  },
 }
 
 AuthenticationManager.promises = {

services/web/app/src/Features/PasswordReset/PasswordResetController.js

@@ -22,23 +22,11 @@ async function setNewUserPassword(req, res, next) {
 
   const err = AuthenticationManager.validatePassword(password, email)
   if (err) {
-    if (err?.info?.code === 'contains_email') {
+    const message = AuthenticationManager.getMessageForInvalidPasswordError(
-      return res.status(400).json({
+      err,
-        message: {
+      req
-          text: req.i18n.translate('invalid_password_contains_email'),
+    )
-        },
+    return res.status(400).json({ message })
-      })
-    } else if (err?.info?.code === 'too_similar') {
-      return res.status(400).json({
-        message: {
-          text: req.i18n.translate('invalid_password_too_similar'),
-        },
-      })
-    } else {
-      return res.status(400).json({
-        message: { text: err.message },
-      })
-    }
   }
 
   passwordResetToken = passwordResetToken.trim()

services/web/app/src/Features/User/UserController.js

@@ -96,21 +96,11 @@ async function changePassword(req, res, next) {
     )
   } catch (error) {
     if (error.name === 'InvalidPasswordError') {
-      if (error?.info?.code === 'contains_email') {
+      const message = AuthenticationManager.getMessageForInvalidPasswordError(
-        return HttpErrorHandler.badRequest(
+        error,
-          req,
+        req
-          res,
+      )
-          req.i18n.translate('invalid_password_contains_email')
+      return res.status(400).json({ message })
-        )
-      } else if (error?.info?.code === 'too_similar') {
-        return HttpErrorHandler.badRequest(
-          req,
-          res,
-          req.i18n.translate('invalid_password_too_similar')
-        )
-      } else {
-        return HttpErrorHandler.badRequest(req, res, error.message)
-      }
     } else if (error.name === 'PasswordMustBeDifferentError') {
       return HttpErrorHandler.badRequest(
         req,

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

@@ -24,6 +24,14 @@ block content
 						p(data-ol-hide-on-error-message="token-expired") #{translate("create_a_new_password_for_your_account")}.
 						+formMessages()
 
+						+customFormMessage('password-contains-email', 'danger')
+							| #{translate('invalid_password_contains_email')}.
+							| #{translate('use_a_different_password')}
+
+						+customFormMessage('password-too-similar', 'danger')
+							| #{translate('invalid_password_too_similar')}.
+							| #{translate('use_a_different_password')}
+
 						+customFormMessage('token-expired', 'danger')
 							| #{translate('password_reset_token_expired')}
 							br

services/web/frontend/extracted-translations.json

@@ -410,6 +410,8 @@
   "invalid_email": "",
   "invalid_file_name": "",
   "invalid_filename": "",
+  "invalid_password_contains_email": "",
+  "invalid_password_too_similar": "",
   "invalid_request": "",
   "invite_more_collabs": "",
   "invite_not_accepted": "",

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

@@ -164,6 +164,16 @@ function PasswordForm() {
                 />
                 . {t('use_a_different_password')}
               </>
+            ) : getErrorMessageKey(error) === 'password-contains-email' ? (
+              <>
+                {t('invalid_password_contains_email')}.{' '}
+                {t('use_a_different_password')}
+              </>
+            ) : getErrorMessageKey(error) === 'password-too-similar' ? (
+              <>
+                {t('invalid_password_too_similar')}.{' '}
+                {t('use_a_different_password')}
+              </>
             ) : (
               getUserFacingMessage(error)
             )}

services/web/locales/en.json

@@ -751,7 +751,7 @@
   "invalid_password_not_set": "Password is required",
   "invalid_password_too_long": "Maximum password length __maxLength__ exceeded",
   "invalid_password_too_short": "Password too short, minimum __minLength__",
-  "invalid_password_too_similar": "Password is too similar to email address",
+  "invalid_password_too_similar": "Password is too similar to parts of email address",
   "invalid_request": "Invalid Request. Please correct the data and try again.",
   "invalid_zip_file": "Invalid zip file",
   "invite_more_collabs": "Invite more collaborators",

services/web/test/acceptance/src/PasswordResetTests.js

@@ -200,7 +200,11 @@ describe('PasswordReset', function () {
         expect(response.status).to.equal(400)
         const body = await response.json()
         expect(body).to.deep.equal({
-          message: { text: 'Password cannot contain parts of email address' },
+          message: {
+            type: 'error',
+            key: 'password-contains-email',
+            text: 'Password cannot contain parts of email address',
+          },
         })
       })
 
@@ -223,7 +227,11 @@ describe('PasswordReset', function () {
         expect(response.status).to.equal(400)
         const body = await response.json()
         expect(body).to.deep.equal({
-          message: { text: 'Password is too similar to email address' },
+          message: {
+            type: 'error',
+            key: 'password-too-similar',
+            text: 'Password is too similar to parts of email address',
+          },
         })
       })
 

services/web/test/acceptance/src/PasswordUpdateTests.js

@@ -133,7 +133,43 @@ describe('PasswordUpdate', function () {
       })
       it('should return error message', async function () {
         const body = await response.json()
-        expect(body.message).to.equal('password is too short')
+        expect(body.message).to.deep.equal({
+          type: 'error',
+          key: 'password-too-short',
+          text: 'Password too short, minimum 8',
+        })
+      })
+      it('should not update audit log', async function () {
+        const auditLog = userHelper.getAuditLogWithoutNoise()
+        expect(auditLog).to.deep.equal([])
+      })
+    })
+    describe('new password contains part of email', function () {
+      beforeEach(async function () {
+        response = await userHelper.fetch('/user/password/update', {
+          method: 'POST',
+          headers: {
+            'Content-Type': 'application/json',
+            Accept: 'application/json',
+          },
+          body: JSON.stringify({
+            currentPassword: password,
+            newPassword1: 'somecooluser123',
+            newPassword2: 'somecooluser123',
+          }),
+        })
+        userHelper = await UserHelper.getUser({ email })
+      })
+      it('should return 400', async function () {
+        expect(response.status).to.equal(400)
+      })
+      it('should return error message', async function () {
+        const body = await response.json()
+        expect(body.message).to.deep.equal({
+          key: 'password-contains-email',
+          type: 'error',
+          text: 'Password cannot contain parts of email address',
+        })
       })
       it('should not update audit log', async function () {
         const auditLog = userHelper.getAuditLogWithoutNoise()
@@ -161,9 +197,11 @@ describe('PasswordUpdate', function () {
       })
       it('should return error message', async function () {
         const body = await response.json()
-        expect(body.message).to.equal(
+        expect(body.message).to.deep.equal({
-          'Password is too similar to email address'
+          key: 'password-too-similar',
-        )
+          type: 'error',
+          text: 'Password is too similar to parts of email address',
+        })
       })
       it('should not update audit log', async function () {
         const auditLog = userHelper.getAuditLogWithoutNoise()

services/web/test/unit/src/User/UserControllerTests.js

@@ -61,6 +61,9 @@ describe('UserController', function () {
         authenticate: sinon.stub(),
         setUserPassword: sinon.stub(),
       },
+      getMessageForInvalidPasswordError: sinon
+        .stub()
+        .returns({ type: 'error', key: 'some-key' }),
     }
     this.UserUpdater = {
       changeEmailAddress: sinon.stub(),
@@ -771,18 +774,21 @@ describe('UserController', function () {
         //   .returns({ message: 'validation-error' })
         const err = new Error('bad')
         err.name = 'InvalidPasswordError'
+        const message = {
+          type: 'error',
+          key: 'some-message-key',
+        }
+        this.AuthenticationManager.getMessageForInvalidPasswordError.returns(
+          message
+        )
         this.AuthenticationManager.promises.setUserPassword.rejects(err)
         this.AuthenticationManager.promises.authenticate.resolves({})
         this.req.body = {
           newPassword1: 'newpass',
           newPassword2: 'newpass',
         }
-        this.HttpErrorHandler.badRequest.callsFake(() => {
+        this.res.json.callsFake(result => {
-          expect(this.HttpErrorHandler.badRequest).to.have.been.calledWith(
+          expect(result.message).to.deep.equal(message)
-            this.req,
-            this.res,
-            err.message
-          )
           this.AuthenticationManager.promises.setUserPassword.callCount.should.equal(
             1
           )