Merge pull request #17155 from overleaf/dp-mongoose-callback-user-registration-handler

Promisify UserRegistrationHandler and UserRegistrationHandlerTests

GitOrigin-RevId: b561f5574883b016824077e971aa4613b44a42dd

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

@@ -3,13 +3,17 @@ const UserCreator = require('./UserCreator')
 const UserGetter = require('./UserGetter')
 const AuthenticationManager = require('../Authentication/AuthenticationManager')
 const NewsletterManager = require('../Newsletter/NewsletterManager')
-const async = require('async')
 const logger = require('@overleaf/logger')
 const crypto = require('crypto')
 const EmailHandler = require('../Email/EmailHandler')
 const OneTimeTokenHandler = require('../Security/OneTimeTokenHandler')
 const settings = require('@overleaf/settings')
 const EmailHelper = require('../Helpers/EmailHelper')
+const {
+  callbackify,
+  callbackifyMultiResult,
+} = require('@overleaf/promise-utils')
+const OError = require('@overleaf/o-error')
 
 const UserRegistrationHandler = {
   _registrationRequestIsValid(body) {
@@ -21,10 +25,10 @@ const UserRegistrationHandler = {
     return !(invalidEmail || invalidPassword)
   },
 
-  _createNewUserIfRequired(user, userDetails, callback) {
+  async _createNewUserIfRequired(user, userDetails) {
     if (!user) {
       userDetails.holdingAccount = false
-      UserCreator.createNewUser(
+      return await UserCreator.promises.createNewUser(
         {
           holdingAccount: false,
           email: userDetails.email,
@@ -32,116 +36,101 @@ const UserRegistrationHandler = {
           last_name: userDetails.last_name,
           analyticsId: userDetails.analyticsId,
         },
-        {},
-        callback
+        {}
       )
-    } else {
-      callback(null, user)
     }
+    return user
   },
 
-  registerNewUser(userDetails, callback) {
-    const self = this
-    const requestIsValid = this._registrationRequestIsValid(userDetails)
+  async registerNewUser(userDetails) {
+    const requestIsValid =
+      UserRegistrationHandler._registrationRequestIsValid(userDetails)
+
     if (!requestIsValid) {
-      return callback(new Error('request is not valid'))
+      throw new Error('request is not valid')
     }
     userDetails.email = EmailHelper.parseEmail(userDetails.email)
-    UserGetter.getUserByAnyEmail(userDetails.email, (error, user) => {
-      if (error) {
-        return callback(error)
-      }
-      if (user && user.holdingAccount === false) {
-        return callback(new Error('EmailAlreadyRegistered'), user)
-      }
-      self._createNewUserIfRequired(user, userDetails, (error, user) => {
-        if (error) {
-          return callback(error)
-        }
-        async.series(
-          [
-            callback =>
-              User.updateOne(
-                { _id: user._id },
-                { $set: { holdingAccount: false } },
-                callback
-              ),
-            callback =>
-              AuthenticationManager.setUserPassword(
-                user,
-                userDetails.password,
-                callback
-              ),
-            callback => {
-              if (userDetails.subscribeToNewsletter === 'true') {
-                NewsletterManager.subscribe(user, error => {
-                  if (error) {
-                    logger.warn(
-                      { err: error, user },
-                      'Failed to subscribe user to newsletter'
-                    )
-                  }
-                })
-              }
-              callback()
-            }, // this can be slow, just fire it off
-          ],
-          error => {
-            callback(error, user)
-          }
+
+    let user = await UserGetter.promises.getUserByAnyEmail(userDetails.email)
+    if (user && user.holdingAccount === false) {
+      // We add userId to the error object so that the calling function can access
+      // the id of the already existing user account.
+      throw new OError('EmailAlreadyRegistered', { userId: user._id })
+    }
+
+    user = await UserRegistrationHandler._createNewUserIfRequired(
+      user,
+      userDetails
+    )
+
+    await User.updateOne(
+      { _id: user._id },
+      { $set: { holdingAccount: false } }
+    ).exec()
+
+    await AuthenticationManager.promises.setUserPassword(
+      user,
+      userDetails.password
+    )
+
+    if (userDetails.subscribeToNewsletter === 'true') {
+      try {
+        NewsletterManager.subscribe(user)
+      } catch (error) {
+        logger.warn(
+          { err: error, user },
+          'Failed to subscribe user to newsletter'
         )
-      })
-    })
+        throw error
+      }
+    }
+
+    return user
   },
 
-  registerNewUserAndSendActivationEmail(email, callback) {
-    UserRegistrationHandler.registerNewUser(
-      {
+  async registerNewUserAndSendActivationEmail(email) {
+    let user
+    try {
+      user = await UserRegistrationHandler.registerNewUser({
         email,
         password: crypto.randomBytes(32).toString('hex'),
-      },
-      (error, user) => {
-        if (error && error.message !== 'EmailAlreadyRegistered') {
-          return callback(error)
-        }
-
-        if (error && error.message === 'EmailAlreadyRegistered') {
-          logger.debug(
-            { email },
-            'user already exists, resending welcome email'
-          )
-        }
-
-        const ONE_WEEK = 7 * 24 * 60 * 60 // seconds
-        OneTimeTokenHandler.getNewToken(
-          'password',
-          { user_id: user._id.toString(), email: user.email },
-          { expiresIn: ONE_WEEK },
-          (error, token) => {
-            if (error) {
-              return callback(error)
-            }
-
-            const setNewPasswordUrl = `${settings.siteUrl}/user/activate?token=${token}&user_id=${user._id}`
-
-            EmailHandler.sendEmail(
-              'registered',
-              {
-                to: user.email,
-                setNewPasswordUrl,
-              },
-              error => {
-                if (error) {
-                  logger.warn({ err: error }, 'failed to send activation email')
-                }
-                callback(null, user, setNewPasswordUrl)
-              }
-            )
-          }
-        )
+      })
+    } catch (error) {
+      if (error.message === 'EmailAlreadyRegistered') {
+        logger.debug({ email }, 'user already exists, resending welcome email')
+        user = await UserGetter.promises.getUserByAnyEmail(email)
+      } else {
+        throw error
       }
+    }
+
+    const ONE_WEEK = 7 * 24 * 60 * 60 // seconds
+    const token = await OneTimeTokenHandler.promises.getNewToken(
+      'password',
+      { user_id: user._id.toString(), email: user.email },
+      { expiresIn: ONE_WEEK }
     )
+
+    const setNewPasswordUrl = `${settings.siteUrl}/user/activate?token=${token}&user_id=${user._id}`
+
+    try {
+      EmailHandler.promises.sendEmail('registered', {
+        to: user.email,
+        setNewPasswordUrl,
+      })
+    } catch (error) {
+      logger.warn({ err: error }, 'failed to send activation email')
+    }
+
+    return { user, setNewPasswordUrl }
   },
 }
 
-module.exports = UserRegistrationHandler
+module.exports = {
+  registerNewUser: callbackify(UserRegistrationHandler.registerNewUser),
+  registerNewUserAndSendActivationEmail: callbackifyMultiResult(
+    UserRegistrationHandler.registerNewUserAndSendActivationEmail,
+    ['user', 'setNewPasswordUrl']
+  ),
+  promises: UserRegistrationHandler,
+}

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

@@ -16,19 +16,33 @@ describe('UserRegistrationHandler', function () {
       _id: (this.user_id = '31j2lk21kjl'),
       analyticsId: this.analyticsId,
     }
-    this.User = { updateOne: sinon.stub().callsArgWith(2) }
-    this.UserGetter = { getUserByAnyEmail: sinon.stub() }
+    this.User = {
+      updateOne: sinon.stub().returns({ exec: sinon.stub().resolves() }),
+    }
+    this.UserGetter = {
+      promises: {
+        getUserByAnyEmail: sinon.stub(),
+      },
+    }
     this.UserCreator = {
-      createNewUser: sinon.stub().callsArgWith(2, null, this.user),
+      promises: {
+        createNewUser: sinon.stub().resolves(this.user),
+      },
     }
     this.AuthenticationManager = {
       validateEmail: sinon.stub().returns(null),
       validatePassword: sinon.stub().returns(null),
-      setUserPassword: sinon.stub().callsArgWith(2),
+      promises: {
+        setUserPassword: sinon.stub().resolves(this.user),
+      },
+    }
+    this.NewsLetterManager = {
+      subscribe: sinon.stub(),
     }
-    this.NewsLetterManager = { subscribe: sinon.stub().callsArgWith(1) }
-    this.EmailHandler = { sendEmail: sinon.stub().callsArgWith(2) }
-    this.OneTimeTokenHandler = { getNewToken: sinon.stub() }
+    this.EmailHandler = {
+      promises: { sendEmail: sinon.stub() },
+    }
+    this.OneTimeTokenHandler = { promises: { getNewToken: sinon.stub() } }
     this.handler = SandboxedModule.require(modulePath, {
       requires: {
         '../../models/User': { User: this.User },
@@ -60,7 +74,7 @@ describe('UserRegistrationHandler', function () {
 
   describe('validate Register Request', function () {
     it('allows passing validation through', function () {
-      const result = this.handler._registrationRequestIsValid(
+      const result = this.handler.promises._registrationRequestIsValid(
         this.passingRequest
       )
       result.should.equal(true)
@@ -74,7 +88,7 @@ describe('UserRegistrationHandler', function () {
       })
 
       it('does not allow through', function () {
-        const result = this.handler._registrationRequestIsValid(
+        const result = this.handler.promises._registrationRequestIsValid(
           this.passingRequest
         )
         return result.should.equal(false)
@@ -89,7 +103,7 @@ describe('UserRegistrationHandler', function () {
       })
 
       it('does not allow through', function () {
-        const result = this.handler._registrationRequestIsValid(
+        const result = this.handler.promises._registrationRequestIsValid(
           this.passingRequest
         )
         result.should.equal(false)
@@ -101,118 +115,100 @@ describe('UserRegistrationHandler', function () {
     describe('holdingAccount', function (done) {
       beforeEach(function () {
         this.user.holdingAccount = true
-        this.handler._registrationRequestIsValid = sinon.stub().returns(true)
-        this.UserGetter.getUserByAnyEmail.callsArgWith(1, null, this.user)
+        this.handler.promises._registrationRequestIsValid = sinon
+          .stub()
+          .returns(true)
+        this.UserGetter.promises.getUserByAnyEmail.resolves(this.user)
       })
 
-      it('should not create a new user if there is a holding account there', function (done) {
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.UserCreator.createNewUser.called.should.equal(false)
-          done(error)
-        })
+      it('should not create a new user if there is a holding account there', async function () {
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.UserCreator.promises.createNewUser.called.should.equal(false)
       })
 
-      it('should set holding account to false', function (done) {
-        this.handler.registerNewUser(this.passingRequest, error => {
-          const update = this.User.updateOne.args[0]
-          assert.deepEqual(update[0], { _id: this.user._id })
-          assert.deepEqual(update[1], { $set: { holdingAccount: false } })
-          done(error)
-        })
+      it('should set holding account to false', async function () {
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        const update = this.User.updateOne.args[0]
+        assert.deepEqual(update[0], { _id: this.user._id })
+        assert.deepEqual(update[1], { $set: { holdingAccount: false } })
       })
     })
 
     describe('invalidRequest', function () {
-      it('should not create a new user if the the request is not valid', function (done) {
-        this.handler._registrationRequestIsValid = sinon.stub().returns(false)
-        this.handler.registerNewUser(this.passingRequest, error => {
-          expect(error).to.be.instanceOf(Error)
-
-          this.UserCreator.createNewUser.called.should.equal(false)
-          done()
-        })
+      it('should not create a new user if the the request is not valid', async function () {
+        this.handler.promises._registrationRequestIsValid = sinon
+          .stub()
+          .returns(false)
+        expect(this.handler.promises.registerNewUser(this.passingRequest)).to.be
+          .rejected
+        this.UserCreator.promises.createNewUser.called.should.equal(false)
       })
 
-      it('should return email registered in the error if there is a non holdingAccount there', function (done) {
-        this.UserGetter.getUserByAnyEmail.callsArgWith(
-          1,
-          null,
+      it('should return email registered in the error if there is a non holdingAccount there', async function () {
+        this.UserGetter.promises.getUserByAnyEmail.resolves(
           (this.user = { holdingAccount: false })
         )
-        this.handler.registerNewUser(this.passingRequest, (err, user) => {
-          expect(err).to.be.instanceOf(Error)
-          expect(err).to.have.property('message', 'EmailAlreadyRegistered')
-          user.should.deep.equal(this.user)
-          done()
-        })
+        expect(
+          this.handler.promises.registerNewUser(this.passingRequest)
+        ).to.be.rejectedWith('EmailAlreadyRegistered')
       })
     })
 
     describe('validRequest', function () {
       beforeEach(function () {
-        this.handler._registrationRequestIsValid = sinon.stub().returns(true)
-        this.UserGetter.getUserByAnyEmail.callsArgWith(1)
+        this.handler.promises._registrationRequestIsValid = sinon
+          .stub()
+          .returns(true)
+        this.UserGetter.promises.getUserByAnyEmail.resolves()
       })
 
-      it('should create a new user', function (done) {
-        this.handler.registerNewUser(this.passingRequest, error => {
-          sinon.assert.calledWith(this.UserCreator.createNewUser, {
-            email: this.passingRequest.email,
-            holdingAccount: false,
-            first_name: this.passingRequest.first_name,
-            last_name: this.passingRequest.last_name,
-            analyticsId: this.user.analyticsId,
-          })
-          done(error)
+      it('should create a new user', async function () {
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        sinon.assert.calledWith(this.UserCreator.promises.createNewUser, {
+          email: this.passingRequest.email,
+          holdingAccount: false,
+          first_name: this.passingRequest.first_name,
+          last_name: this.passingRequest.last_name,
+          analyticsId: this.user.analyticsId,
         })
       })
 
-      it('lower case email', function (done) {
+      it('lower case email', async function () {
         this.passingRequest.email = 'soMe@eMail.cOm'
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.UserCreator.createNewUser.args[0][0].email.should.equal(
-            'some@email.com'
-          )
-          done(error)
-        })
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.UserCreator.promises.createNewUser.args[0][0].email.should.equal(
+          'some@email.com'
+        )
       })
 
-      it('trim white space from email', function (done) {
+      it('trim white space from email', async function () {
         this.passingRequest.email = ' some@email.com '
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.UserCreator.createNewUser.args[0][0].email.should.equal(
-            'some@email.com'
-          )
-          done(error)
-        })
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.UserCreator.promises.createNewUser.args[0][0].email.should.equal(
+          'some@email.com'
+        )
       })
 
-      it('should set the password', function (done) {
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.AuthenticationManager.setUserPassword
-            .calledWith(this.user, this.passingRequest.password)
-            .should.equal(true)
-          done(error)
-        })
+      it('should set the password', async function () {
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.AuthenticationManager.promises.setUserPassword
+          .calledWith(this.user, this.passingRequest.password)
+          .should.equal(true)
       })
 
-      it('should add the user to the newsletter if accepted terms', function (done) {
+      it('should add the user to the newsletter if accepted terms', async function () {
         this.passingRequest.subscribeToNewsletter = 'true'
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.NewsLetterManager.subscribe
-            .calledWith(this.user)
-            .should.equal(true)
-          done(error)
-        })
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.NewsLetterManager.subscribe
+          .calledWith(this.user)
+          .should.equal(true)
       })
 
-      it('should not add the user to the newsletter if not accepted terms', function (done) {
-        this.handler.registerNewUser(this.passingRequest, error => {
-          this.NewsLetterManager.subscribe
-            .calledWith(this.user)
-            .should.equal(false)
-          done(error)
-        })
+      it('should not add the user to the newsletter if not accepted terms', async function () {
+        await this.handler.promises.registerNewUser(this.passingRequest)
+        this.NewsLetterManager.subscribe
+          .calledWith(this.user)
+          .should.equal(false)
       })
     })
   })
@@ -225,26 +221,24 @@ describe('UserRegistrationHandler', function () {
           return (this.password = 'mock-password')
         },
       })
-      this.OneTimeTokenHandler.getNewToken.yields(
-        null,
+      this.OneTimeTokenHandler.promises.getNewToken.resolves(
         (this.token = 'mock-token')
       )
-      this.handler.registerNewUser = sinon.stub()
-      this.callback = sinon.stub()
+      this.handler.promises.registerNewUser = sinon.stub()
     })
 
     describe('with a new user', function () {
-      beforeEach(function () {
+      beforeEach(async function () {
         this.user.email = this.email.toLowerCase()
-        this.handler.registerNewUser.callsArgWith(1, null, this.user)
-        this.handler.registerNewUserAndSendActivationEmail(
-          this.email,
-          this.callback
-        )
+        this.handler.promises.registerNewUser.resolves(this.user)
+        this.result =
+          await this.handler.promises.registerNewUserAndSendActivationEmail(
+            this.email
+          )
       })
 
       it('should ask the UserRegistrationHandler to register user', function () {
-        sinon.assert.calledWith(this.handler.registerNewUser, {
+        sinon.assert.calledWith(this.handler.promises.registerNewUser, {
           email: this.email,
           password: this.password,
         })
@@ -255,13 +249,13 @@ describe('UserRegistrationHandler', function () {
           user_id: this.user._id.toString(),
           email: this.user.email,
         }
-        this.OneTimeTokenHandler.getNewToken
+        this.OneTimeTokenHandler.promises.getNewToken
           .calledWith('password', data, { expiresIn: 7 * 24 * 60 * 60 })
           .should.equal(true)
       })
 
       it('should send a registered email', function () {
-        this.EmailHandler.sendEmail
+        this.EmailHandler.promises.sendEmail
           .calledWith('registered', {
             to: this.user.email,
             setNewPasswordUrl: `${this.settings.siteUrl}/user/activate?token=${this.token}&user_id=${this.user_id}`,
@@ -269,33 +263,29 @@ describe('UserRegistrationHandler', function () {
           .should.equal(true)
       })
 
-      it('should return the user', function () {
-        this.callback
-          .calledWith(
-            null,
-            this.user,
-            `${this.settings.siteUrl}/user/activate?token=${this.token}&user_id=${this.user_id}`
-          )
-          .should.equal(true)
+      it('should return the user and new password url', function () {
+        const { user, setNewPasswordUrl } = this.result
+        expect(user).to.deep.equal(this.user)
+        expect(setNewPasswordUrl).to.equal(
+          `${this.settings.siteUrl}/user/activate?token=${this.token}&user_id=${this.user_id}`
+        )
       })
     })
 
     describe('with a user that already exists', function () {
-      beforeEach(function () {
-        this.handler.registerNewUser.callsArgWith(
-          1,
-          new Error('EmailAlreadyRegistered'),
-          this.user
+      beforeEach(async function () {
+        this.handler.promises.registerNewUser.rejects(
+          new Error('EmailAlreadyRegistered')
         )
-        this.handler.registerNewUserAndSendActivationEmail(
-          this.email,
-          this.callback
+        this.UserGetter.promises.getUserByAnyEmail.resolves(this.user)
+        await this.handler.promises.registerNewUserAndSendActivationEmail(
+          this.email
         )
       })
 
       it('should still generate a new password token and email', function () {
-        this.OneTimeTokenHandler.getNewToken.called.should.equal(true)
-        this.EmailHandler.sendEmail.called.should.equal(true)
+        this.OneTimeTokenHandler.promises.getNewToken.called.should.equal(true)
+        this.EmailHandler.promises.sendEmail.called.should.equal(true)
       })
     })
   })