[web] When switching primary email, delete the old primary if it's unconfirmed (#23688)

* Add note to ConfirmModal: unconfirmed primary will be deleted * Change confirm button copy * Promisify `UserEmailsController.setDefault` * Update tests after promisification * Delete unconfirmed primary when swapped * Fixup apostrophe in translation * `npm run extract-translations` * Add unit tests * Add acceptance tests * Fix frontend tests * Make email address bold * Add "We removed the previous primary..." to the email GitOrigin-RevId: c971e219e36e509f9963e1720acdd44f562a05b5
2026-06-02 05:41:33 +02:00 · 2025-02-21 08:42:26 +01:00
parent f0d94f06ed
commit 34cac93f9a
11 changed files with 195 additions and 70 deletions
--- a/services/web/app/src/Features/User/UserEmailsController.js
+++ b/services/web/app/src/Features/User/UserEmailsController.js
@@ -544,6 +544,73 @@ async function remove(req, res) {
  res.sendStatus(200)
 }

+async function setDefault(req, res, next) {
+  const userId = SessionManager.getLoggedInUserId(req.session)
+  const email = EmailHelper.parseEmail(req.body.email)
+
+  if (!email) {
+    return res.sendStatus(422)
+  }
+
+  const { emails, email: oldDefault } = await UserGetter.promises.getUser(
+    userId,
+    { email: 1, emails: 1 }
+  )
+  const primaryEmailData = emails?.find(email => email.email === oldDefault)
+  const deleteOldEmail =
+    req.query['delete-unconfirmed-primary'] !== undefined &&
+    primaryEmailData &&
+    !primaryEmailData.confirmedAt
+
+  const auditLog = {
+    initiatorId: userId,
+    ipAddress: req.ip,
+  }
+  try {
+    await UserUpdater.promises.setDefaultEmailAddress(
+      userId,
+      email,
+      false,
+      auditLog,
+      true,
+      deleteOldEmail
+    )
+  } catch (err) {
+    return UserEmailsController._handleEmailError(err, req, res, next)
+  }
+  SessionManager.setInSessionUser(req.session, { email })
+  const user = SessionManager.getSessionUser(req.session)
+  try {
+    await UserSessionsManager.promises.removeSessionsFromRedis(
+      user,
+      req.sessionID // remove all sessions except the current session
+    )
+  } catch (err) {
+    logger.warn(
+      { err },
+      'failed revoking secondary sessions after changing default email'
+    )
+  }
+  if (
+    req.query['delete-unconfirmed-primary'] !== undefined &&
+    primaryEmailData &&
+    !primaryEmailData.confirmedAt
+  ) {
+    await UserUpdater.promises.removeEmailAddress(
+      userId,
+      primaryEmailData.email,
+      {
+        initiatorId: userId,
+        ipAddress: req.ip,
+        extraInfo: {
+          info: 'removed unconfirmed email after setting new primary',
+        },
+      }
+    )
+  }
+  res.sendStatus(200)
+}
+
 const UserEmailsController = {
  list(req, res, next) {
    const userId = SessionManager.getLoggedInUserId(req.session)
@@ -566,43 +633,7 @@ const UserEmailsController = {

  remove: expressify(remove),

-  setDefault(req, res, next) {
-    const userId = SessionManager.getLoggedInUserId(req.session)
-    const email = EmailHelper.parseEmail(req.body.email)
-    if (!email) {
-      return res.sendStatus(422)
-    }
-    const auditLog = {
-      initiatorId: userId,
-      ipAddress: req.ip,
-    }
-    UserUpdater.setDefaultEmailAddress(
-      userId,
-      email,
-      false,
-      auditLog,
-      true,
-      err => {
-        if (err) {
-          return UserEmailsController._handleEmailError(err, req, res, next)
-        }
-        SessionManager.setInSessionUser(req.session, { email })
-        const user = SessionManager.getSessionUser(req.session)
-        UserSessionsManager.removeSessionsFromRedis(
-          user,
-          req.sessionID, // remove all sessions except the current session
-          err => {
-            if (err)
-              logger.warn(
-                { err },
-                'failed revoking secondary sessions after changing default email'
-              )
-          }
-        )
-        res.sendStatus(200)
-      }
-    )
-  },
+  setDefault: expressify(setDefault),

  endorse(req, res, next) {
    const userId = SessionManager.getLoggedInUserId(req.session)
--- a/services/web/app/src/Features/User/UserUpdater.js
+++ b/services/web/app/src/Features/User/UserUpdater.js
@@ -20,7 +20,13 @@ const _ = require('lodash')
 const Modules = require('../../infrastructure/Modules')
 const UserSessionsManager = require('./UserSessionsManager')

-async function _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email) {
+async function _sendSecurityAlertPrimaryEmailChanged(
+  userId,
+  oldEmail,
+  email,
+  deleteOldEmail
+) {
+  // here
  // Send email to the following:
  // - the old primary
  // - the new primary
@@ -31,6 +37,11 @@ async function _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email) {
  const emailOptions = {
    actionDescribed: `the primary email address on your account was changed to ${email}`,
    action: 'change of primary email address',
+    message: deleteOldEmail
+      ? [
+          `We also removed the previous primary email ${oldEmail} from the account.`,
+        ]
+      : [],
  }

  async function sendToRecipients(recipients) {
@@ -161,7 +172,8 @@ async function setDefaultEmailAddress(
  email,
  allowUnconfirmed,
  auditLog,
-  sendSecurityAlert
+  sendSecurityAlert,
+  deleteOldEmail = false
 ) {
  email = EmailHelper.parseEmail(email)
  if (email == null) {
@@ -212,11 +224,14 @@ async function setDefaultEmailAddress(

  if (sendSecurityAlert) {
    // no need to wait, errors are logged and not passed back
-    _sendSecurityAlertPrimaryEmailChanged(userId, oldEmail, email).catch(
-      err => {
-        logger.error({ err }, 'failed to send security alert email')
-      }
-    )
+    _sendSecurityAlertPrimaryEmailChanged(
+      userId,
+      oldEmail,
+      email,
+      deleteOldEmail
+    ).catch(err => {
+      logger.error({ err }, 'failed to send security alert email')
+    })
  }

  try {
--- a/services/web/frontend/extracted-translations.json
+++ b/services/web/frontend/extracted-translations.json
@@ -227,6 +227,7 @@
  "change_password": "",
  "change_password_in_account_settings": "",
  "change_plan": "",
+  "change_primary_email": "",
  "change_primary_email_address_instructions": "",
  "change_project_owner": "",
  "change_the_ownership_of_your_personal_projects": "",
@@ -1708,6 +1709,7 @@
  "this_total_reflects_the_amount_due_until": "",
  "this_was_helpful": "",
  "this_wasnt_helpful": "",
+  "this_will_remove_primary_email": "",
  "timedout": "",
  "tip": "",
  "title": "",
--- a/services/web/frontend/js/features/settings/components/emails-section.tsx
+++ b/services/web/frontend/js/features/settings/components/emails-section.tsx
@@ -21,6 +21,7 @@ function EmailsSectionContent() {
    isInitializingSuccess,
  } = useUserEmailsContext()
  const userEmails = Object.values(userEmailsData.byId)
+  const primary = userEmails.find(userEmail => userEmail.default)

  // Only show the "add email" button if the user has permission to add a secondary email
  const hideAddSecondaryEmail = getMeta('ol-cannot-add-secondary-email')
@@ -55,7 +56,7 @@ function EmailsSectionContent() {
          <>
            {userEmails?.map(userEmail => (
              <Fragment key={userEmail.email}>
-                <EmailsRow userEmailData={userEmail} />
+                <EmailsRow userEmailData={userEmail} primary={primary} />
                <div className="horizontal-divider" />
              </Fragment>
            ))}
--- a/services/web/frontend/js/features/settings/components/emails/actions.tsx
+++ b/services/web/frontend/js/features/settings/components/emails/actions.tsx
@@ -8,9 +8,10 @@ import { UserEmailData } from '../../../../../../types/user-email'

 type ActionsProps = {
  userEmailData: UserEmailData
+  primary?: UserEmailData
 }

-function Actions({ userEmailData }: ActionsProps) {
+function Actions({ userEmailData, primary }: ActionsProps) {
  const { t } = useTranslation()
  const { setLoading: setUserEmailsContextLoading } = useUserEmailsContext()
  const makePrimaryAsync = useAsync()
@@ -42,6 +43,7 @@ function Actions({ userEmailData }: ActionsProps) {
    <>
      <MakePrimary
        userEmailData={userEmailData}
+        primary={primary}
        makePrimaryAsync={makePrimaryAsync}
      />{' '}
      <Remove
--- a/services/web/frontend/js/features/settings/components/emails/actions/make-primary/confirmation-modal.tsx
+++ b/services/web/frontend/js/features/settings/components/emails/actions/make-primary/confirmation-modal.tsx
@@ -8,6 +8,7 @@ import OLModal, {
  OLModalHeader,
  OLModalTitle,
 } from '@/features/ui/components/ol/ol-modal'
+import { type UserEmailData } from '../../../../../../../../types/user-email'

 type ConfirmationModalProps = MergeAndOverride<
  React.ComponentProps<typeof AccessibleModal>,
@@ -16,6 +17,7 @@ type ConfirmationModalProps = MergeAndOverride<
    isConfirmDisabled: boolean
    onConfirm: () => void
    onHide: () => void
+    primary?: UserEmailData
  }
 >

@@ -25,6 +27,7 @@ function ConfirmationModal({
  show,
  onConfirm,
  onHide,
+  primary,
 }: ConfirmationModalProps) {
  const { t } = useTranslation()

@@ -33,7 +36,7 @@ function ConfirmationModal({
      <OLModalHeader closeButton>
        <OLModalTitle>{t('confirm_primary_email_change')}</OLModalTitle>
      </OLModalHeader>
-      <OLModalBody>
+      <OLModalBody className="pb-0">
        <p>
          <Trans
            i18nKey="do_you_want_to_change_your_primary_email_address_to"
@@ -43,7 +46,18 @@ function ConfirmationModal({
            tOptions={{ interpolation: { escapeValue: true } }}
          />
        </p>
-        <p className="mb-0">{t('log_in_with_primary_email_address')}</p>
+        <p>{t('log_in_with_primary_email_address')}</p>
+        {primary && !primary.confirmedAt && (
+          <p>
+            <Trans
+              i18nKey="this_will_remove_primary_email"
+              components={{ b: <b /> }}
+              values={{ email: primary.email }}
+              shouldUnescape
+              tOptions={{ interpolation: { escapeValue: true } }}
+            />
+          </p>
+        )}
      </OLModalBody>
      <OLModalFooter>
        <OLButton variant="secondary" onClick={onHide}>
@@ -54,7 +68,7 @@ function ConfirmationModal({
          disabled={isConfirmDisabled}
          onClick={onConfirm}
        >
-          {t('confirm')}
+          {t('change_primary_email')}
        </OLButton>
      </OLModalFooter>
    </OLModal>
--- a/services/web/frontend/js/features/settings/components/emails/actions/make-primary/make-primary.tsx
+++ b/services/web/frontend/js/features/settings/components/emails/actions/make-primary/make-primary.tsx
@@ -42,13 +42,19 @@ const getDescription = (

 type MakePrimaryProps = {
  userEmailData: UserEmailData
+  primary?: UserEmailData
  makePrimaryAsync: UseAsyncReturnType
 }

-function MakePrimary({ userEmailData, makePrimaryAsync }: MakePrimaryProps) {
+function MakePrimary({
+  userEmailData,
+  primary,
+  makePrimaryAsync,
+}: MakePrimaryProps) {
  const [show, setShow] = useState(false)
  const { t } = useTranslation()
-  const { state, makePrimary } = useUserEmailsContext()
+  const { state, makePrimary, deleteEmail, resetLeaversSurveyExpiration } =
+    useUserEmailsContext()

  const handleShowModal = () => setShow(true)
  const handleHideModal = () => setShow(false)
@@ -57,7 +63,10 @@ function MakePrimary({ userEmailData, makePrimaryAsync }: MakePrimaryProps) {

    makePrimaryAsync
      .runAsync(
-        postJSON('/user/emails/default', {
+        // 'delete-unconfirmed-primary' is a temporary parameter here to keep backward compatibility.
+        // So users with the old version of the frontend don't get their primary email deleted unexpectedly.
+        // https://github.com/overleaf/internal/issues/23536
+        postJSON('/user/emails/default?delete-unconfirmed-primary', {
          body: {
            email: userEmailData.email,
          },
@@ -65,6 +74,10 @@ function MakePrimary({ userEmailData, makePrimaryAsync }: MakePrimaryProps) {
      )
      .then(() => {
        makePrimary(userEmailData.email)
+        if (primary && !primary.confirmedAt) {
+          deleteEmail(primary.email)
+          resetLeaversSurveyExpiration(primary)
+        }
      })
      .catch(() => {})
  }
@@ -107,6 +120,7 @@ function MakePrimary({ userEmailData, makePrimaryAsync }: MakePrimaryProps) {
      <ConfirmationModal
        email={userEmailData.email}
        isConfirmDisabled={isConfirmDisabled}
+        primary={primary}
        show={show}
        onHide={handleHideModal}
        onConfirm={handleSetDefaultUserEmail}
--- a/services/web/frontend/js/features/settings/components/emails/row.tsx
+++ b/services/web/frontend/js/features/settings/components/emails/row.tsx
@@ -17,9 +17,10 @@ import OLButton from '@/features/ui/components/ol/ol-button'

 type EmailsRowProps = {
  userEmailData: UserEmailData
+  primary?: UserEmailData
 }

-function EmailsRow({ userEmailData }: EmailsRowProps) {
+function EmailsRow({ userEmailData, primary }: EmailsRowProps) {
  const hasSSOAffiliation = Boolean(
    userEmailData.affiliation &&
      ssoAvailableForInstitution(userEmailData.affiliation.institution)
@@ -42,7 +43,7 @@ function EmailsRow({ userEmailData }: EmailsRowProps) {
        </OLCol>
        <OLCol lg={3}>
          <EmailCell className="text-lg-end">
-            <Actions userEmailData={userEmailData} />
+            <Actions userEmailData={userEmailData} primary={primary} />
          </EmailCell>
        </OLCol>
      </OLRow>
--- a/services/web/locales/en.json
+++ b/services/web/locales/en.json
@@ -292,6 +292,7 @@
  "change_password": "Change Password",
  "change_password_in_account_settings": "Change password in Account Settings",
  "change_plan": "Change plan",
+  "change_primary_email": "Change primary email",
  "change_primary_email_address_instructions": "To change your primary email, please add your new primary email address first (by clicking <0>Add another email</0>) and confirm it. Then click the <0>Make Primary</0> button. <1>Learn more</1> about managing your __appName__ emails.",
  "change_project_owner": "Change Project Owner",
  "change_the_ownership_of_your_personal_projects": "Change the ownership of your personal projects to the new account. <0>Find out how to change project owner.</0>",
@@ -2227,6 +2228,7 @@
  "this_total_reflects_the_amount_due_until": "This total reflects the amount due from today until <strong>__date__</strong>, the end of the billing period of your existing plan.",
  "this_was_helpful": "This was helpful",
  "this_wasnt_helpful": "This wasn’t helpful",
+  "this_will_remove_primary_email": "Note that this will also remove the email address <b>__email__</b> from the account because it’s an unconfirmed email. If you want to keep it, please confirm it first.",
  "three_free_collab": "Three free collaborators",
  "timedout": "Timed out",
  "tip": "Tip",
--- a/services/web/test/frontend/features/settings/components/emails/emails-section-actions.test.tsx
+++ b/services/web/test/frontend/features/settings/components/emails/emails-section-actions.test.tsx
@@ -169,7 +169,9 @@ describe('email actions - make primary', function () {
      fireEvent.click(button)

      const withinModal = within(screen.getByRole('dialog'))
-      fireEvent.click(withinModal.getByRole('button', { name: /confirm/i }))
+      fireEvent.click(
+        withinModal.getByRole('button', { name: 'Change primary email' })
+      )
      expect(screen.queryByRole('dialog')).to.be.null

      await waitForElementToBeRemoved(() =>
@@ -196,7 +198,7 @@ describe('email actions - make primary', function () {
      withinModal.getByText(
        /important .* notifications will be sent to this email address/i
      )
-      withinModal.getByRole('button', { name: /confirm/i })
+      withinModal.getByRole('button', { name: 'Change primary email' })

      fireEvent.click(withinModal.getByRole('button', { name: /cancel/i }))
      expect(screen.queryByRole('dialog')).to.be.null
@@ -205,7 +207,7 @@ describe('email actions - make primary', function () {
    it('shows loader and removes button', async function () {
      fetchMock
        .get('/user/emails?ensureAffiliation=true', [userEmailData])
-        .post('/user/emails/default', 200)
+        .post('/user/emails/default?delete-primary-unconfirmed', 200)
      render(<EmailsSection />)

      await confirmPrimaryEmail()
@@ -221,7 +223,7 @@ describe('email actions - make primary', function () {
    it('shows error', async function () {
      fetchMock
        .get('/user/emails?ensureAffiliation=true', [userEmailData])
-        .post('/user/emails/default', 503)
+        .post('/user/emails/default?delete-primary-unconfirmed', 503)
      render(<EmailsSection />)

      await confirmPrimaryEmail()
--- a/services/web/test/unit/src/User/UserEmailsControllerTests.js
+++ b/services/web/test/unit/src/User/UserEmailsControllerTests.js
@@ -38,16 +38,16 @@ describe('UserEmailsController', function () {
      hasFeature: sinon.stub(),
    }
    this.UserSessionsManager = {
-      removeSessionsFromRedis: sinon.stub().yields(),
+      promises: { removeSessionsFromRedis: sinon.stub().resolves() },
    }
    this.UserUpdater = {
      addEmailAddress: sinon.stub(),
-      setDefaultEmailAddress: sinon.stub(),
      updateV1AndSetDefaultEmailAddress: sinon.stub(),
      promises: {
        addEmailAddress: sinon.stub().resolves(),
        confirmEmail: sinon.stub().resolves(),
        removeEmailAddress: sinon.stub(),
+        setDefaultEmailAddress: sinon.stub().resolves(),
      },
    }
    this.EmailHelper = { parseEmail: sinon.stub() }
@@ -555,8 +555,6 @@ describe('UserEmailsController', function () {
    })

    it('sets default email', function (done) {
-      this.UserUpdater.setDefaultEmailAddress.yields()
-
      this.UserEmailsController.setDefault(this.req, {
        sendStatus: code => {
          code.should.equal(200)
@@ -569,7 +567,7 @@ describe('UserEmailsController', function () {
            }
          )
          assertCalledWith(
-            this.UserUpdater.setDefaultEmailAddress,
+            this.UserUpdater.promises.setDefaultEmailAddress,
            this.user._id,
            this.email
          )
@@ -578,24 +576,68 @@ describe('UserEmailsController', function () {
      })
    })

+    it('deletes unconfirmed primary if delete-unconfirmed-primary is set', function (done) {
+      this.user.emails = [{ email: 'example@overleaf.com' }]
+      this.req.query['delete-unconfirmed-primary'] = ''
+
+      this.UserEmailsController.setDefault(this.req, {
+        sendStatus: () => {
+          assertCalledWith(
+            this.UserUpdater.promises.removeEmailAddress,
+            this.user._id,
+            'example@overleaf.com',
+            {
+              initiatorId: this.user._id,
+              ipAddress: this.req.ip,
+              extraInfo: {
+                info: 'removed unconfirmed email after setting new primary',
+              },
+            }
+          )
+          done()
+        },
+      })
+    })
+
+    it('doesnt delete a confirmed primary', function (done) {
+      this.user.emails = [
+        { email: 'example@overleaf.com', confirmedAt: '2000-01-01' },
+      ]
+      this.req.query['delete-unconfirmed-primary'] = ''
+
+      this.UserEmailsController.setDefault(this.req, {
+        sendStatus: () => {
+          assertNotCalled(this.UserUpdater.promises.removeEmailAddress)
+          done()
+        },
+      })
+    })
+
+    it('doesnt delete primary if delete-unconfirmed-primary is not set', function (done) {
+      this.UserEmailsController.setDefault(this.req, {
+        sendStatus: () => {
+          assertNotCalled(this.UserUpdater.promises.removeEmailAddress)
+          done()
+        },
+      })
+    })
+
    it('handles email parse error', function (done) {
      this.EmailHelper.parseEmail.returns(null)

      this.UserEmailsController.setDefault(this.req, {
        sendStatus: code => {
          code.should.equal(422)
-          assertNotCalled(this.UserUpdater.setDefaultEmailAddress)
+          assertNotCalled(this.UserUpdater.promises.setDefaultEmailAddress)
          done()
        },
      })
    })

    it('should reset the users other sessions', function (done) {
-      this.UserUpdater.setDefaultEmailAddress.yields()
-
      this.res.callback = () => {
        expect(
-          this.UserSessionsManager.removeSessionsFromRedis
+          this.UserSessionsManager.promises.removeSessionsFromRedis
        ).to.have.been.calledWith(this.user, this.req.sessionID)
        done()
      }
@@ -604,11 +646,10 @@ describe('UserEmailsController', function () {
    })

    it('handles error from revoking sessions and returns 200', function (done) {
-      this.UserUpdater.setDefaultEmailAddress.yields()
      const redisError = new Error('redis error')
-      this.UserSessionsManager.removeSessionsFromRedis = sinon
+      this.UserSessionsManager.promises.removeSessionsFromRedis = sinon
        .stub()
-        .yields(redisError)
+        .rejects(redisError)

      this.res.callback = () => {
        expect(this.res.statusCode).to.equal(200)