Merge pull request #17946 from overleaf/rh-promisify-third-party-identity-

[web] Promisify ThirdPartyIdentityManager and ThirdPartyIdentityManagerTests

GitOrigin-RevId: f7d24f73213fb0a43eb453aa21749b21ba60b83d

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

@@ -4,148 +4,128 @@ const EmailOptionsHelper = require('../../../../app/src/Features/Email/EmailOpti
 const Errors = require('../Errors/Errors')
 const _ = require('lodash')
 const logger = require('@overleaf/logger')
-const OError = require('@overleaf/o-error')
 const settings = require('@overleaf/settings')
 const { User } = require('../../../../app/src/models/User')
-const { promisifyAll } = require('@overleaf/promise-utils')
+const { callbackify } = require('@overleaf/promise-utils')
+const OError = require('@overleaf/o-error')
 
 const oauthProviders = settings.oauthProviders || {}
 
-function getUser(providerId, externalUserId, callback) {
+async function getUser(providerId, externalUserId) {
   if (providerId == null || externalUserId == null) {
-    return callback(
-      new OError('invalid SSO arguments', {
-        externalUserId,
-        providerId,
-      })
-    )
+    throw new OError('invalid SSO arguments', {
+      externalUserId,
+      providerId,
+    })
   }
+
   const query = _getUserQuery(providerId, externalUserId)
-  User.findOne(query, function (err, user) {
-    if (err != null) {
-      return callback(err)
-    }
-    if (!user) {
-      return callback(new Errors.ThirdPartyUserNotFoundError())
-    }
-    callback(null, user)
-  })
+  const user = await User.findOne(query).exec()
+  if (!user) {
+    throw new Errors.ThirdPartyUserNotFoundError()
+  }
+  return user
 }
 
-function login(providerId, externalUserId, externalData, callback) {
-  ThirdPartyIdentityManager.getUser(
+async function login(providerId, externalUserId, externalData) {
+  const user = await ThirdPartyIdentityManager.promises.getUser(
+    providerId,
+    externalUserId
+  )
+  if (!externalData) {
+    return user
+  }
+  const query = _getUserQuery(providerId, externalUserId)
+  const update = _thirdPartyIdentifierUpdate(
+    user,
     providerId,
     externalUserId,
-    function (err, user) {
-      if (err != null) {
-        return callback(err)
-      }
-      if (!externalData) {
-        return callback(null, user)
-      }
-      const query = _getUserQuery(providerId, externalUserId)
-      const update = _thirdPartyIdentifierUpdate(
-        user,
-        providerId,
-        externalUserId,
-        externalData
-      )
-      User.findOneAndUpdate(query, update, { new: true }, callback)
-    }
+    externalData
   )
+  return await User.findOneAndUpdate(query, update, { new: true }).exec()
 }
 
-function link(
+async function link(
   userId,
   providerId,
   externalUserId,
   externalData,
   auditLog,
-  callback,
   retry
 ) {
   const accountLinked = true
   if (!oauthProviders[providerId]) {
-    return callback(new Error('Not a valid provider'))
+    throw new Error('Not a valid provider')
   }
 
-  UserAuditLogHandler.addEntry(
+  await UserAuditLogHandler.promises.addEntry(
     userId,
     'link-sso',
     auditLog.initiatorId,
     auditLog.ipAddress,
     {
       providerId,
+    }
+  )
+
+  const query = {
+    _id: userId,
+    'thirdPartyIdentifiers.providerId': {
+      $ne: providerId,
+    },
+  }
+  const update = {
+    $push: {
+      thirdPartyIdentifiers: {
+        externalUserId,
+        externalData,
+        providerId,
+      },
     },
-    error => {
-      if (error) {
-        return callback(error)
-      }
-      const query = {
-        _id: userId,
-        'thirdPartyIdentifiers.providerId': {
-          $ne: providerId,
-        },
-      }
-      const update = {
-        $push: {
-          thirdPartyIdentifiers: {
-            externalUserId,
-            externalData,
-            providerId,
-          },
-        },
-      }
-      // add new tpi only if an entry for the provider does not exist
-      // projection includes thirdPartyIdentifiers for tests
-      User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
-        if (err && err.code === 11000) {
-          callback(
-            new Errors.ThirdPartyIdentityExistsError({
-              info: { externalUserId },
-            })
-          )
-        } else if (err != null) {
-          callback(err)
-        } else if (res) {
-          _sendSecurityAlert(accountLinked, providerId, res, userId)
-          callback(null, res)
-        } else if (retry) {
-          // if already retried then throw error
-          callback(new Error('update failed'))
-        } else {
-          // attempt to clear existing entry then retry
-          ThirdPartyIdentityManager.unlink(
-            userId,
-            providerId,
-            auditLog,
-            function (err) {
-              if (err != null) {
-                return callback(err)
-              }
-              ThirdPartyIdentityManager.link(
-                userId,
-                providerId,
-                externalUserId,
-                externalData,
-                auditLog,
-                callback,
-                true
-              )
-            }
-          )
-        }
+  }
+  // add new tpi only if an entry for the provider does not exist
+  // projection includes thirdPartyIdentifiers for tests
+  let res
+  try {
+    res = await User.findOneAndUpdate(query, update, { new: 1 }).exec()
+  } catch (err) {
+    if (err.code === 11000) {
+      throw new Errors.ThirdPartyIdentityExistsError({
+        info: { externalUserId },
       })
     }
+    throw err
+  }
+
+  if (res) {
+    _sendSecurityAlert(accountLinked, providerId, res, userId)
+    return res
+  }
+
+  if (retry) {
+    // if already retried then throw error
+    throw new Error('update failed')
+  }
+
+  // attempt to clear existing entry then retry
+  await ThirdPartyIdentityManager.promises.unlink(userId, providerId, auditLog)
+  return await ThirdPartyIdentityManager.promises.link(
+    userId,
+    providerId,
+    externalUserId,
+    externalData,
+    auditLog,
+    true
   )
 }
 
-function unlink(userId, providerId, auditLog, callback) {
+async function unlink(userId, providerId, auditLog) {
   const accountLinked = false
   if (!oauthProviders[providerId]) {
-    return callback(new Error('Not a valid provider'))
+    throw new Error('Not a valid provider')
   }
-  UserAuditLogHandler.addEntry(
+
+  await UserAuditLogHandler.promises.addEntry(
     userId,
     'unlink-sso',
     auditLog.initiatorId,
@@ -153,35 +133,26 @@ function unlink(userId, providerId, auditLog, callback) {
     {
       ...(auditLog.extraInfo || {}),
       providerId,
-    },
-    error => {
-      if (error) {
-        return callback(error)
-      }
-      const query = {
-        _id: userId,
-      }
-      const update = {
-        $pull: {
-          thirdPartyIdentifiers: {
-            providerId,
-          },
-        },
-      }
-      // projection includes thirdPartyIdentifiers for tests
-      User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => {
-        if (err != null) {
-          callback(err)
-        } else if (!res) {
-          callback(new Error('update failed'))
-        } else {
-          // no need to wait, errors are logged and not passed back
-          _sendSecurityAlert(accountLinked, providerId, res, userId)
-          callback(null, res)
-        }
-      })
     }
   )
+
+  const query = {
+    _id: userId,
+  }
+  const update = {
+    $pull: {
+      thirdPartyIdentifiers: {
+        providerId,
+      },
+    },
+  }
+  // projection includes thirdPartyIdentifiers for tests
+  const res = await User.findOneAndUpdate(query, update, { new: 1 })
+  if (!res) {
+    throw new Error('update failed')
+  }
+  _sendSecurityAlert(accountLinked, providerId, res, userId)
+  return res
 }
 
 function _getUserQuery(providerId, externalUserId) {
@@ -194,21 +165,21 @@ function _getUserQuery(providerId, externalUserId) {
   return query
 }
 
-function _sendSecurityAlert(accountLinked, providerId, user, userId) {
+async function _sendSecurityAlert(accountLinked, providerId, user, userId) {
   const providerName = oauthProviders[providerId].name
   const emailOptions = EmailOptionsHelper.linkOrUnlink(
     accountLinked,
     providerName,
     user.email
   )
-  EmailHandler.sendEmail('securityAlert', emailOptions, error => {
-    if (error) {
-      logger.error(
-        { err: error, userId },
-        `could not send security alert email when ${emailOptions.action.toLowerCase()}`
-      )
-    }
-  })
+  try {
+    await EmailHandler.promises.sendEmail('securityAlert', emailOptions)
+  } catch (error) {
+    logger.error(
+      { err: error, userId },
+      `could not send security alert email when ${emailOptions.action.toLowerCase()}`
+    )
+  }
 }
 
 function _thirdPartyIdentifierUpdate(
@@ -230,12 +201,17 @@ function _thirdPartyIdentifierUpdate(
 }
 
 const ThirdPartyIdentityManager = {
+  getUser: callbackify(getUser),
+  login: callbackify(login),
+  link: callbackify(link),
+  unlink: callbackify(unlink),
+}
+
+ThirdPartyIdentityManager.promises = {
   getUser,
   login,
   link,
   unlink,
 }
 
-ThirdPartyIdentityManager.promises = promisifyAll(ThirdPartyIdentityManager)
-
 module.exports = ThirdPartyIdentityManager

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

@@ -1,6 +1,7 @@
 const sinon = require('sinon')
 const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
+const OError = require('@overleaf/o-error')
 const modulePath =
   '../../../../app/src/Features/User/ThirdPartyIdentityManager.js'
 
@@ -18,16 +19,24 @@ describe('ThirdPartyIdentityManager', function () {
       requires: {
         '../../../../app/src/Features/User/UserAuditLogHandler':
           (this.UserAuditLogHandler = {
-            addEntry: sinon.stub().yields(),
+            promises: {
+              addEntry: sinon.stub().resolves(),
+            },
           }),
         '../../../../app/src/Features/Email/EmailHandler': (this.EmailHandler =
           {
-            sendEmail: sinon.stub().yields(),
+            promises: {
+              sendEmail: sinon.stub().resolves(),
+            },
           }),
         '../../../../app/src/models/User': {
           User: (this.User = {
-            findOneAndUpdate: sinon.stub().yields(undefined, this.user),
-            findOne: sinon.stub().yields(undefined, undefined),
+            findOneAndUpdate: sinon
+              .stub()
+              .returns({ exec: sinon.stub().resolves(this.user) }),
+            findOne: sinon.stub().returns({
+              exec: sinon.stub().resolves(undefined),
+            }),
           }),
         },
         '@overleaf/settings': {
@@ -44,28 +53,23 @@ describe('ThirdPartyIdentityManager', function () {
     })
   })
   describe('getUser', function () {
-    it('should an error when missing providerId or externalUserId', function (done) {
-      this.ThirdPartyIdentityManager.getUser(
-        undefined,
-        undefined,
-        (error, user) => {
-          expect(error).to.exist
-          expect(error.message).to.equal('invalid SSO arguments')
-          expect(error.info).to.deep.equal({
-            externalUserId: undefined,
-            providerId: undefined,
-          })
-          done()
-        }
-      )
+    it('should throw an error when missing providerId or externalUserId', async function () {
+      await expect(
+        this.ThirdPartyIdentityManager.promises.getUser(undefined, undefined)
+      ).to.be.rejectedWith(OError, `invalid SSO arguments`)
     })
+
     describe('when user linked', function () {
       beforeEach(function () {
-        this.User.findOne.yields(undefined, this.user)
+        this.User.findOne.returns({
+          exec: sinon.stub().resolves(this.user),
+        })
       })
 
       it('should return the user', async function () {
-        this.User.findOne.returns(undefined, this.user)
+        this.User.findOne.returns({
+          exec: sinon.stub().resolves(this.user),
+        })
         const user = await this.ThirdPartyIdentityManager.promises.getUser(
           'google',
           'an-id-linked'
@@ -94,7 +98,7 @@ describe('ThirdPartyIdentityManager', function () {
         this.externalData,
         this.auditLog
       )
-      const emailCall = this.EmailHandler.sendEmail.getCall(0)
+      const emailCall = this.EmailHandler.promises.sendEmail.getCall(0)
       expect(emailCall.args[0]).to.equal('securityAlert')
       expect(emailCall.args[1].actionDescribed).to.contain(
         'a Google account was linked'
@@ -109,7 +113,9 @@ describe('ThirdPartyIdentityManager', function () {
         this.externalData,
         this.auditLog
       )
-      expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith(
+      expect(
+        this.UserAuditLogHandler.promises.addEntry
+      ).to.have.been.calledOnceWith(
         this.userId,
         'link-sso',
         this.auditLog.initiatorId,
@@ -121,49 +127,47 @@ describe('ThirdPartyIdentityManager', function () {
     })
     describe('errors', function () {
       const anError = new Error('oops')
-      it('should not unlink if the UserAuditLogHandler throws an error', function (done) {
-        this.UserAuditLogHandler.addEntry.yields(anError)
-        this.ThirdPartyIdentityManager.link(
-          this.userId,
-          'google',
-          this.externalUserId,
-          this.externalData,
-          this.auditLog,
-          error => {
-            expect(error).to.exist
-            expect(error).to.equal(anError)
-            expect(this.User.findOneAndUpdate).to.not.have.been.called
-            done()
-          }
-        )
-      })
-      describe('EmailHandler', function () {
-        beforeEach(function () {
-          this.EmailHandler.sendEmail.yields(anError)
-        })
-        it('should log but not return the error', function (done) {
-          this.ThirdPartyIdentityManager.link(
+
+      it('should not unlink if the UserAuditLogHandler throws an error', async function () {
+        this.UserAuditLogHandler.promises.addEntry.throws(anError)
+        await expect(
+          this.ThirdPartyIdentityManager.promises.link(
             this.userId,
             'google',
             this.externalUserId,
             this.externalData,
-            this.auditLog,
-            error => {
-              expect(error).to.not.exist
-              expect(this.logger.error.lastCall).to.be.calledWithExactly(
-                {
-                  err: anError,
-                  userId: this.userId,
-                },
-                'could not send security alert email when new account linked'
-              )
-              done()
-            }
+            this.auditLog
+          )
+        ).to.be.rejectedWith(anError)
+        expect(this.User.findOneAndUpdate).to.not.have.been.called
+      })
+
+      describe('EmailHandler', function () {
+        beforeEach(function () {
+          this.EmailHandler.promises.sendEmail.throws(anError)
+        })
+        it('should log but not return the error', async function () {
+          await expect(
+            this.ThirdPartyIdentityManager.promises.link(
+              this.userId,
+              'google',
+              this.externalUserId,
+              this.externalData,
+              this.auditLog
+            )
+          ).to.be.fulfilled
+          expect(this.logger.error.lastCall).to.be.calledWithExactly(
+            {
+              err: anError,
+              userId: this.userId,
+            },
+            'could not send security alert email when new account linked'
           )
         })
       })
     })
   })
+
   describe('unlink', function () {
     it('should send email alert', async function () {
       await this.ThirdPartyIdentityManager.promises.unlink(
@@ -171,7 +175,7 @@ describe('ThirdPartyIdentityManager', function () {
         'orcid',
         this.auditLog
       )
-      const emailCall = this.EmailHandler.sendEmail.getCall(0)
+      const emailCall = this.EmailHandler.promises.sendEmail.getCall(0)
       expect(emailCall.args[0]).to.equal('securityAlert')
       expect(emailCall.args[1].actionDescribed).to.contain(
         'an ORCID account was unlinked from'
@@ -183,7 +187,9 @@ describe('ThirdPartyIdentityManager', function () {
         'orcid',
         this.auditLog
       )
-      expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith(
+      expect(
+        this.UserAuditLogHandler.promises.addEntry
+      ).to.have.been.calledOnceWith(
         this.userId,
         'unlink-sso',
         this.auditLog.initiatorId,
@@ -193,43 +199,43 @@ describe('ThirdPartyIdentityManager', function () {
         }
       )
     })
+
     describe('errors', function () {
       const anError = new Error('oops')
-      it('should not unlink if the UserAuditLogHandler throws an error', function (done) {
-        this.UserAuditLogHandler.addEntry.yields(anError)
-        this.ThirdPartyIdentityManager.unlink(
-          this.userId,
-          'orcid',
-          this.auditLog,
-          error => {
-            expect(error).to.exist
-            expect(error).to.equal(anError)
-            expect(this.User.findOneAndUpdate).to.not.have.been.called
-            done()
-          }
-        )
+
+      it('should not unlink if the UserAuditLogHandler throws an error', async function () {
+        this.UserAuditLogHandler.promises.addEntry.throws(anError)
+
+        await expect(
+          this.ThirdPartyIdentityManager.promises.unlink(
+            this.userId,
+            'orcid',
+            this.auditLog
+          )
+        ).to.be.rejectedWith(anError)
+
         expect(this.User.findOneAndUpdate).to.not.have.been.called
       })
+
       describe('EmailHandler', function () {
         beforeEach(function () {
-          this.EmailHandler.sendEmail.yields(anError)
+          this.EmailHandler.promises.sendEmail.throws(anError)
         })
-        it('should log but not return the error', function (done) {
-          this.ThirdPartyIdentityManager.unlink(
-            this.userId,
-            'google',
-            this.auditLog,
-            error => {
-              expect(error).to.not.exist
-              expect(this.logger.error.lastCall).to.be.calledWithExactly(
-                {
-                  err: anError,
-                  userId: this.userId,
-                },
-                'could not send security alert email when account no longer linked'
-              )
-              done()
-            }
+        it('should log but not return the error', async function () {
+          await expect(
+            this.ThirdPartyIdentityManager.promises.unlink(
+              this.userId,
+              'google',
+              this.auditLog
+            )
+          ).to.be.fulfilled
+
+          expect(this.logger.error.lastCall).to.be.calledWithExactly(
+            {
+              err: anError,
+              userId: this.userId,
+            },
+            'could not send security alert email when account no longer linked'
           )
         })
       })