diff --git a/services/web/app/src/Features/User/UserEmailsController.js b/services/web/app/src/Features/User/UserEmailsController.js index 44c907e0d0..1af4476b93 100644 --- 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) diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 09940fbc99..627e73875d 100644 --- 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 { diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index f5e777bb0d..dac76a9250 100644 --- 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": "", diff --git a/services/web/frontend/js/features/settings/components/emails-section.tsx b/services/web/frontend/js/features/settings/components/emails-section.tsx index 25fead1270..8c7e3ec6e9 100644 --- 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 => ( - +
))} diff --git a/services/web/frontend/js/features/settings/components/emails/actions.tsx b/services/web/frontend/js/features/settings/components/emails/actions.tsx index 9ac715524d..842e2db762 100644 --- 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) { <> {' '} , @@ -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({ {t('confirm_primary_email_change')} - +

-

{t('log_in_with_primary_email_address')}

+

{t('log_in_with_primary_email_address')}

+ {primary && !primary.confirmedAt && ( +

+ }} + values={{ email: primary.email }} + shouldUnescape + tOptions={{ interpolation: { escapeValue: true } }} + /> +

+ )} @@ -54,7 +68,7 @@ function ConfirmationModal({ disabled={isConfirmDisabled} onClick={onConfirm} > - {t('confirm')} + {t('change_primary_email')} diff --git 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 index 40583aef98..3e3faf2203 100644 --- 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) { - + diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 40ecc9804a..bb69866dd0 100644 --- 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) and confirm it. Then click the <0>Make Primary button. <1>Learn more 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.", @@ -2227,6 +2228,7 @@ "this_total_reflects_the_amount_due_until": "This total reflects the amount due from today until __date__, 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 __email__ 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", diff --git 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 index 573a2c69b2..f770155ba9 100644 --- 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() 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() await confirmPrimaryEmail() diff --git a/services/web/test/unit/src/User/UserEmailsControllerTests.js b/services/web/test/unit/src/User/UserEmailsControllerTests.js index d83f641f44..926cd6c816 100644 --- 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)