Merge pull request #17572 from overleaf/tm-email-confirmation-require-login

Prevent email confirmation unless logged in to the requesting account

GitOrigin-RevId: 28af875b2887b8bbef8327097635aa01345c682c

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

@@ -7,6 +7,7 @@ const UserUpdater = require('./UserUpdater')
 const UserGetter = require('./UserGetter')
 const { callbackify, promisify } = require('util')
 const crypto = require('crypto')
+const SessionManager = require('../Authentication/SessionManager')
 
 // Reject email confirmation tokens after 90 days
 const TOKEN_EXPIRY_IN_S = 90 * 24 * 60 * 60
@@ -85,58 +86,51 @@ async function sendReconfirmationEmail(userId, email) {
   await EmailHandler.promises.sendEmail('reconfirmEmail', emailOptions)
 }
 
+async function confirmEmailFromToken(req, token) {
+  const { data } = await OneTimeTokenHandler.promises.peekValueFromToken(
+    TOKEN_USE,
+    token
+  )
+  if (!data) {
+    throw new Errors.NotFoundError('no token found')
+  }
+
+  const loggedInUserId = SessionManager.getLoggedInUserId(req.session)
+  // user_id may be stored as an ObjectId or string
+  const userId = data.user_id?.toString()
+  const email = data.email
+  if (!userId || email !== EmailHelper.parseEmail(email)) {
+    throw new Errors.NotFoundError('invalid data')
+  }
+  if (loggedInUserId !== userId) {
+    throw new Errors.ForbiddenError('logged in user does not match token user')
+  }
+  const user = await UserGetter.promises.getUser(userId, { emails: 1 })
+  if (!user) {
+    throw new Errors.NotFoundError('user not found')
+  }
+  const emailExists = user.emails.some(emailData => emailData.email === email)
+  if (!emailExists) {
+    throw new Errors.NotFoundError('email missing for user')
+  }
+
+  await OneTimeTokenHandler.promises.expireToken(TOKEN_USE, token)
+  await UserUpdater.promises.confirmEmail(userId, email)
+
+  return { userId, email }
+}
+
 const UserEmailsConfirmationHandler = {
   sendConfirmationEmail,
 
   sendReconfirmationEmail: callbackify(sendReconfirmationEmail),
 
-  confirmEmailFromToken(token, callback) {
-    OneTimeTokenHandler.getValueFromTokenAndExpire(
-      TOKEN_USE,
-      token,
-      function (error, data) {
-        if (error) {
-          return callback(error)
-        }
-        if (!data) {
-          return callback(new Errors.NotFoundError('no token found'))
-        }
-        const userId = data.user_id
-        const email = data.email
-
-        if (!userId || email !== EmailHelper.parseEmail(email)) {
-          return callback(new Errors.NotFoundError('invalid data'))
-        }
-        UserGetter.getUser(userId, { emails: 1 }, function (error, user) {
-          if (error) {
-            return callback(error)
-          }
-          if (!user) {
-            return callback(new Errors.NotFoundError('user not found'))
-          }
-          const emailExists = user.emails.some(
-            emailData => emailData.email === email
-          )
-          if (!emailExists) {
-            return callback(new Errors.NotFoundError('email missing for user'))
-          }
-          UserUpdater.confirmEmail(userId, email, function (error) {
-            if (error) {
-              return callback(error)
-            }
-            callback(null, { userId, email })
-          })
-        })
-      }
-    )
-  },
+  confirmEmailFromToken: callbackify(confirmEmailFromToken),
 }
 
 UserEmailsConfirmationHandler.promises = {
   sendConfirmationEmail: promisify(sendConfirmationEmail),
-  confirmEmailFromToken: promisify(
-    UserEmailsConfirmationHandler.confirmEmailFromToken
-  ),
+  confirmEmailFromToken,
   sendConfirmationCode,
 }
 

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

@@ -156,6 +156,13 @@ async function primaryEmailCheck(req, res) {
   AsyncFormHelper.redirect(req, res, '/project')
 }
 
+async function showConfirm(req, res, next) {
+  res.render('user/confirm_email', {
+    token: req.query.token,
+    title: 'confirm_email',
+  })
+}
+
 const UserEmailsController = {
   list(req, res, next) {
     const userId = SessionManager.getLoggedInUserId(req.session)
@@ -254,12 +261,7 @@ const UserEmailsController = {
 
   primaryEmailCheck: expressify(primaryEmailCheck),
 
-  showConfirm(req, res, next) {
-    res.render('user/confirm_email', {
-      token: req.query.token,
-      title: 'confirm_email',
-    })
-  },
+  showConfirm: expressify(showConfirm),
 
   confirm(req, res, next) {
     const { token } = req.body
@@ -269,10 +271,18 @@ const UserEmailsController = {
       })
     }
     UserEmailsConfirmationHandler.confirmEmailFromToken(
+      req,
       token,
       function (error, userData) {
         if (error) {
-          if (error instanceof Errors.NotFoundError) {
+          if (error instanceof Errors.ForbiddenError) {
+            res.status(403).json({
+              message: {
+                key: 'confirm-email-wrong-user',
+                text: `We can’t confirm this email. You must be logged in with the Overleaf account that requested the new secondary email.`,
+              },
+            })
+          } else if (error instanceof Errors.NotFoundError) {
             res.status(404).json({
               message: req.i18n.translate('confirmation_token_invalid'),
             })

services/web/app/src/router.js

@@ -303,9 +303,14 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) {
     UserController.promises.ensureAffiliationMiddleware,
     UserEmailsController.list
   )
-  webRouter.get('/user/emails/confirm', UserEmailsController.showConfirm)
+  webRouter.get(
+    '/user/emails/confirm',
+    AuthenticationController.requireLogin(),
+    UserEmailsController.showConfirm
+  )
   webRouter.post(
     '/user/emails/confirm',
+    AuthenticationController.requireLogin(),
     RateLimiterMiddleware.rateLimit(rateLimiters.confirmEmail),
     UserEmailsController.confirm
   )

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

@@ -4,10 +4,17 @@ block content
 	main.content.content-alt#main-content
 		.container
 			.row
-				.col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4
+				.col-md-8.col-md-offset-2.col-lg-6.col-lg-offset-3
 					.card
-						.page-header
+						.page-header(data-ol-hide-on-error-message="confirm-email-wrong-user")
 							h1 #{translate("confirm_email")}
+						form(
+							method="POST"
+							action="/logout"
+							id="logoutForm"
+						)
+							input(type="hidden", name="_csrf", value=csrfToken)
+							input(type="hidden", name="redirect", value=currentUrlWithQueryParams)
 						form(
 							data-ol-async-form,
 							data-ol-auto-submit,
@@ -21,11 +28,20 @@ block content
 
 							div(data-ol-not-sent)
 								+formMessages()
+								div(data-ol-custom-form-message="confirm-email-wrong-user" hidden)
+									h1.h3 #{translate("we_cant_confirm_this_email")}
+									p !{translate("to_confirm_email_address_you_must_be_logged_in_with_the_requesting_account")}
+									p !{translate("you_are_currently_logged_in_as", {email: getUserEmail()})}
+									.actions
+										button.btn-primary.btn.btn-block(
+											form="logoutForm"
+										) #{translate('log_in_with_a_different_account')}
 
 								.actions
 									button.btn-primary.btn.btn-block(
 										type='submit',
 										data-ol-disabled-inflight
+										data-ol-hide-on-error-message="confirm-email-wrong-user"
 									)
 										span(data-ol-inflight="idle")
 											| #{translate('confirm')}

services/web/locales/en.json

@@ -1029,6 +1029,7 @@
   "log_in_first_to_proceed": "You will need to <b>log in</b> first to proceed.",
   "log_in_now": "Log in now",
   "log_in_with": "Log in with __provider__",
+  "log_in_with_a_different_account": "Log in with a different account",
   "log_in_with_email": "Log in with __email__",
   "log_in_with_existing_institution_email": "Please log in with your existing <b>__appName__</b> account in order to get your <b>__appName__</b>  and <b>__institutionName__</b> institutional accounts linked.",
   "log_in_with_primary_email_address": "This will be the email address to use if you log in with an email address and password. Important __appName__ notifications will be sent to this email address.",
@@ -1913,6 +1914,7 @@
   "to_add_email_accounts_need_to_be_linked_2": "To add this email, your <0>__appName__</0> and <0>__institutionName__</0> accounts will need to be linked.",
   "to_add_more_collaborators": "To add more collaborators or turn on link sharing, please ask the project owner",
   "to_change_access_permissions": "To change access permissions, please ask the project owner",
+  "to_confirm_email_address_you_must_be_logged_in_with_the_requesting_account": "To confirm an email address, you <b>must be logged in</b> with the <b>Overleaf account that requested the new secondary email</b>.",
   "to_confirm_transfer_enter_email_address": "To accept the invitation, enter the email address linked to your account.",
   "to_confirm_unlink_all_users_enter_email": "To confirm you want to unlink all users, enter your email address:",
   "to_fix_this_you_can": "To fix this, you can:",
@@ -2121,6 +2123,7 @@
   "visual_editor": "Visual Editor",
   "visual_editor_is_only_available_for_tex_files": "Visual Editor is only available for TeX files",
   "want_change_to_apply_before_plan_end": "If you wish this change to apply before the end of your current billing period, please contact us.",
+  "we_cant_confirm_this_email": "We can’t confirm this email",
   "we_cant_find_any_sections_or_subsections_in_this_file": "We can’t find any sections or subsections in this file",
   "we_do_not_share_personal_information": "See our <0>Privacy Notice</0> for details of how we treat your personal data",
   "we_logged_you_in": "We have logged you in.",
@@ -2179,6 +2182,7 @@
   "you_are_a_manager_of_commons_at_institution_x": "You are a <0>manager</0> of the Overleaf Commons subscription at <0>__institutionName__</0>",
   "you_are_a_manager_of_publisher_x": "You are a <0>manager</0> of <0>__publisherName__</0>",
   "you_are_a_manager_of_x_plan_as_member_of_group_subscription_y_administered_by_z": "You are a <1>manager</1> of the <0>__planName__</0> group subscription <1>__groupName__</1> administered by <1>__adminEmail__</1>",
+  "you_are_currently_logged_in_as": "You are currently logged in as <b>__email__</b>.",
   "you_are_on_a_paid_plan_contact_support_to_find_out_more": "You’re on an __appName__ Paid plan. <0>Contact support</0> to find out more.",
   "you_are_on_x_plan_as_a_confirmed_member_of_institution_y": "You are on our <0>__planName__</0> plan as a <1>confirmed member</1> of <1>__institutionName__</1>",
   "you_are_on_x_plan_as_member_of_group_subscription_y_administered_by_z": "You are on our <0>__planName__</0> plan as a <1>member</1> of the group subscription <1>__groupName__</1> administered by <1>__adminEmail__</1>",

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

@@ -24,27 +24,38 @@ const EmailHelper = require('../../../../app/src/Features/Helpers/EmailHelper')
 
 describe('UserEmailsConfirmationHandler', function () {
   beforeEach(function () {
+    this.mockUser = {
+      _id: 'mock-user-id',
+      email: 'mock@example.com',
+      emails: [{ email: 'mock@example.com' }],
+    }
+    this.user_id = this.mockUser._id
+    this.email = this.mockUser.email
+    this.req = {}
     this.UserEmailsConfirmationHandler = SandboxedModule.require(modulePath, {
       requires: {
         '@overleaf/settings': (this.settings = {
           siteUrl: 'https://emails.example.com',
         }),
-        '../Security/OneTimeTokenHandler': (this.OneTimeTokenHandler = {}),
-        './UserUpdater': (this.UserUpdater = {}),
+        '../Security/OneTimeTokenHandler': (this.OneTimeTokenHandler = {
+          promises: {},
+        }),
+        './UserUpdater': (this.UserUpdater = {
+          promises: {},
+        }),
         './UserGetter': (this.UserGetter = {
           getUser: sinon.stub().yields(null, this.mockUser),
+          promises: {
+            getUser: sinon.stub().resolves(this.mockUser),
+          },
         }),
         '../Email/EmailHandler': (this.EmailHandler = {}),
         '../Helpers/EmailHelper': EmailHelper,
+        '../Authentication/SessionManager': (this.SessionManager = {
+          getLoggedInUserId: sinon.stub().returns(this.mockUser._id),
+        }),
       },
     })
-    this.mockUser = {
-      _id: 'mock-user-id',
-      email: 'mock@example.com',
-      emails: [{ email: 'mock@example.com' }],
-    }
-    this.user_id = this.mockUser._id
-    this.email = this.mockUser.email
     return (this.callback = sinon.stub())
   })
 
@@ -127,122 +138,139 @@ describe('UserEmailsConfirmationHandler', function () {
 
   describe('confirmEmailFromToken', function () {
     beforeEach(function () {
-      this.OneTimeTokenHandler.getValueFromTokenAndExpire = sinon
+      this.OneTimeTokenHandler.promises.peekValueFromToken = sinon
         .stub()
-        .yields(null, { user_id: this.user_id, email: this.email })
-      return (this.UserUpdater.confirmEmail = sinon.stub().yields())
+        .resolves({ data: { user_id: this.user_id, email: this.email } })
+      this.OneTimeTokenHandler.promises.expireToken = sinon.stub().resolves()
+      this.UserUpdater.promises.confirmEmail = sinon.stub().resolves()
     })
 
     describe('successfully', function () {
-      beforeEach(function () {
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
+      beforeEach(async function () {
+        await this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+          this.req,
+          (this.token = 'mock-token')
         )
       })
 
-      it('should call getValueFromTokenAndExpire', function () {
-        return this.OneTimeTokenHandler.getValueFromTokenAndExpire
+      it('should call peekValueFromToken', function () {
+        return this.OneTimeTokenHandler.promises.peekValueFromToken
           .calledWith('email_confirmation', this.token)
           .should.equal(true)
       })
 
-      it('should confirm the email of the user_id', function () {
-        return this.UserUpdater.confirmEmail
-          .calledWith(this.user_id, this.email)
+      it('should call expireToken', function () {
+        return this.OneTimeTokenHandler.promises.expireToken
+          .calledWith('email_confirmation', this.token)
           .should.equal(true)
       })
 
-      it('should call the callback', function () {
-        return this.callback.called.should.equal(true)
+      it('should confirm the email of the user_id', function () {
+        return this.UserUpdater.promises.confirmEmail
+          .calledWith(this.user_id, this.email)
+          .should.equal(true)
       })
     })
 
     describe('with an expired token', function () {
       beforeEach(function () {
-        this.OneTimeTokenHandler.getValueFromTokenAndExpire = sinon
+        this.OneTimeTokenHandler.promises.peekValueFromToken = sinon
           .stub()
-          .yields(null, null)
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
-        )
+          .rejects(new Errors.NotFoundError('no token found'))
       })
 
-      it('should call the callback with a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should reject with a NotFoundError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
       })
     })
 
     describe('with no user_id in the token', function () {
       beforeEach(function () {
-        this.OneTimeTokenHandler.getValueFromTokenAndExpire = sinon
+        this.OneTimeTokenHandler.promises.peekValueFromToken = sinon
           .stub()
-          .yields(null, { email: this.email })
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
-        )
+          .resolves({ data: { email: this.email } })
       })
 
-      it('should call the callback with a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should reject with a NotFoundError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
       })
     })
 
     describe('with no email in the token', function () {
       beforeEach(function () {
-        this.OneTimeTokenHandler.getValueFromTokenAndExpire = sinon
+        this.OneTimeTokenHandler.promises.peekValueFromToken = sinon
           .stub()
-          .yields(null, { user_id: this.user_id })
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
-        )
+          .resolves({ data: { user_id: this.user_id } })
       })
 
-      it('should call the callback with a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should reject with a NotFoundError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
       })
     })
 
     describe('with no user found', function () {
       beforeEach(function () {
-        this.UserGetter.getUser.yields(null, null)
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
-        )
+        this.UserGetter.promises.getUser.resolves(null)
       })
 
-      it('should call the callback with a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should reject with a NotFoundError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
       })
     })
 
     describe('with secondary email missing on user', function () {
       beforeEach(function () {
-        this.OneTimeTokenHandler.getValueFromTokenAndExpire = sinon
+        this.OneTimeTokenHandler.promises.peekValueFromToken = sinon
           .stub()
-          .yields(null, { user_id: this.user_id, email: 'deleted@email.com' })
-        return this.UserEmailsConfirmationHandler.confirmEmailFromToken(
-          (this.token = 'mock-token'),
-          this.callback
-        )
+          .resolves({
+            data: { user_id: this.user_id, email: 'deleted@email.com' },
+          })
       })
 
-      it('should call the callback with a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should reject with a NotFoundError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
+      })
+    })
+
+    describe('when the logged in user is not the token user', function () {
+      beforeEach(function () {
+        this.SessionManager.getLoggedInUserId = sinon
+          .stub()
+          .returns('other-user-id')
+      })
+
+      it('should reject with a ForbiddenError', async function () {
+        await expect(
+          this.UserEmailsConfirmationHandler.promises.confirmEmailFromToken(
+            this.req,
+            (this.token = 'mock-token')
+          )
+        ).to.be.rejectedWith(Errors.ForbiddenError)
       })
     })
   })

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

@@ -440,7 +440,7 @@ describe('UserEmailsController', function () {
 
       it('should confirm the email from the token', function () {
         this.UserEmailsConfirmationHandler.confirmEmailFromToken
-          .calledWith(this.token)
+          .calledWith(this.req, this.token)
           .should.equal(true)
       })