[web] Promisify NotificationsBuilder.js (#28792)

* Replace request by fetch

* Promisify `dropboxDuplicateProjectNames`

* Promisify `dropboxUnlinkedDueToLapsedReconfirmation`

* Promisify `featuresUpgradedByAffiliation`

* Promisify `redundantPersonalSubscription`

* Promisify `projectInvite`

* Promisify `ipMatcherAffiliation`

* Promisify `tpdsFileLimit`

* Promisify `groupInvitation`

* Promisify `personalAndGroupSubscriptions`

* Sanitize URL

* Add default fetch timeout

* Update tests

* Update tests with fetch-utils

* Update external usage

* Import `ObjectId` from `mongodb-legacy`

Co-authored-by: Alf Eaton <alf.eaton@overleaf.com>

* Add `toString()` to userId in `ipMatcherAffiliation` calls

* [v1] Return "200 null" instead of no content in `v2/api/v2/users/:userId/ip_matcher` when there is no data, so it can be read as JSON in the frontend

Can't be 204 because of lint: "Do not specify body content for a response with a non-content status code"

* Temporarily remove `enrolment_ad_html` test to test notifications

* Revert "Temporarily remove `enrolment_ad_html` test to test notifications"

This reverts commit fb8b0c26a3adbbc64053cd3f5f2570ccc97328fb.

* Use `return await`

Co-authored-by: Andrew Rumble <andrew.rumble@overleaf.com>

---------

Co-authored-by: Alf Eaton <alf.eaton@overleaf.com>
Co-authored-by: Andrew Rumble <andrew.rumble@overleaf.com>
GitOrigin-RevId: 17ee861852aa4ac15d3b46b1b28c763fad5333d2

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -348,7 +348,7 @@ const AuthenticationController = {
 
   ipMatchCheck(req, user) {
     if (req.ip !== user.lastLoginIp) {
-      NotificationsBuilder.ipMatcherAffiliation(user._id).create(
+      NotificationsBuilder.ipMatcherAffiliation(user._id.toString()).create(
         req.ip,
         () => {}
       )

services/web/app/src/Features/Institutions/InstitutionsAPI.js

@@ -235,7 +235,7 @@ async function addAffiliation(userId, email, affiliationOptions) {
   // have notifications delete any ip matcher notifications for this university
   try {
     await NotificationsBuilder.promises
-      .ipMatcherAffiliation(userId)
+      .ipMatcherAffiliation(userId.toString())
       .read(university.id)
   } catch (err) {
     // log and ignore error

services/web/app/src/Features/Notifications/NotificationsBuilder.js

@@ -1,30 +1,28 @@
 const NotificationsHandler = require('./NotificationsHandler')
-const { promisifyAll } = require('@overleaf/promise-utils')
+const { callbackifyAll } = require('@overleaf/promise-utils')
-const request = require('request')
+const { fetchJson } = require('@overleaf/fetch-utils')
 const settings = require('@overleaf/settings')
+const path = require('path')
+const { ObjectId } = require('mongodb-legacy')
 
 function dropboxDuplicateProjectNames(userId) {
   return {
     key: `dropboxDuplicateProjectNames-${userId}`,
-    create(projectName, callback) {
+    async create(projectName) {
-      if (callback == null) {
+      return await NotificationsHandler.promises.createNotification(
-        callback = function () {}
-      }
-      NotificationsHandler.createNotification(
         userId,
         this.key,
         'notification_dropbox_duplicate_project_names',
         { projectName },
         null,
-        true,
+        true
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadWithKey(
-        callback = function () {}
+        userId,
-      }
+        this.key
-      NotificationsHandler.markAsReadWithKey(userId, this.key, callback)
+      )
     },
   }
 }
@@ -32,19 +30,21 @@ function dropboxDuplicateProjectNames(userId) {
 function dropboxUnlinkedDueToLapsedReconfirmation(userId) {
   return {
     key: 'drobox-unlinked-due-to-lapsed-reconfirmation',
-    create(callback) {
+    async create() {
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         userId,
         this.key,
         'notification_dropbox_unlinked_due_to_lapsed_reconfirmation',
         {},
         null,
-        true,
+        true
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      NotificationsHandler.markAsReadWithKey(userId, this.key, callback)
+      return await NotificationsHandler.promises.markAsReadWithKey(
+        userId,
+        this.key
+      )
     },
   }
 }
@@ -52,26 +52,22 @@ function dropboxUnlinkedDueToLapsedReconfirmation(userId) {
 function featuresUpgradedByAffiliation(affiliation, user) {
   return {
     key: `features-updated-by=${affiliation.institutionId}`,
-    create(callback) {
+    async create() {
-      if (callback == null) {
-        callback = function () {}
-      }
       const messageOpts = { institutionName: affiliation.institutionName }
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         user._id,
         this.key,
         'notification_features_upgraded_by_affiliation',
         messageOpts,
         null,
-        false,
+        false
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadWithKey(
-        callback = function () {}
+        user._id,
-      }
+        this.key
-      NotificationsHandler.markAsReadWithKey(user._id, this.key, callback)
+      )
     },
   }
 }
@@ -79,26 +75,22 @@ function featuresUpgradedByAffiliation(affiliation, user) {
 function redundantPersonalSubscription(affiliation, user) {
   return {
     key: `redundant-personal-subscription-${affiliation.institutionId}`,
-    create(callback) {
+    async create() {
-      if (callback == null) {
-        callback = function () {}
-      }
       const messageOpts = { institutionName: affiliation.institutionName }
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         user._id,
         this.key,
         'notification_personal_subscription_not_required_due_to_affiliation',
         messageOpts,
         null,
-        false,
+        false
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadWithKey(
-        callback = function () {}
+        user._id,
-      }
+        this.key
-      NotificationsHandler.markAsReadWithKey(user._id, this.key, callback)
+      )
     },
   }
 }
@@ -106,90 +98,74 @@ function redundantPersonalSubscription(affiliation, user) {
 function projectInvite(invite, project, sendingUser, user) {
   return {
     key: `project-invite-${invite._id}`,
-    create(callback) {
+    async create() {
-      if (callback == null) {
-        callback = function () {}
-      }
       const messageOpts = {
         userName: sendingUser.first_name,
         projectName: project.name,
         projectId: project._id.toString(),
         token: invite.token,
       }
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         user._id,
         this.key,
         'notification_project_invite',
         messageOpts,
-        invite.expires,
+        invite.expires
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key)
-        callback = function () {}
-      }
-      NotificationsHandler.markAsReadByKeyOnly(this.key, callback)
     },
   }
 }
 
 function ipMatcherAffiliation(userId) {
   return {
-    create(ip, callback) {
+    async create(ip) {
-      if (callback == null) {
-        callback = function () {}
-      }
       if (!settings.apis.v1.url) {
         // service is not configured
-        return callback()
+        return
       }
-      request(
+      if (!ObjectId.isValid(userId)) {
-        {
+        throw new Error('invalid user id')
-          method: 'GET',
+      }
-          url: `${settings.apis.v1.url}/api/v2/users/${userId}/ip_matcher`,
+      const url = new URL(settings.apis.v1.url)
-          auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass },
+      url.pathname = path.posix.join('/api/v2/users', userId, 'ip_matcher')
-          body: { ip },
+      url.searchParams.set('ip', ip)
-          json: true,
-          timeout: settings.apis.v1.timeout,
-        },
-        function (error, response, body) {
-          if (error != null) {
-            return callback(error)
-          }
-          if (response.statusCode !== 200 || !body) {
-            return callback()
-          }
 
-          const key = `ip-matched-affiliation-${body.id}`
+      const body = await fetchJson(url, {
-          const portalPath = body.portal_slug
+        method: 'GET',
-            ? `/${body.is_university ? 'edu' : 'org'}/${body.portal_slug}`
+        basicAuth: {
-            : undefined
+          user: settings.apis.v1.user,
-          const messageOpts = {
+          password: settings.apis.v1.pass,
-            university_name: body.name,
+        },
-            institutionId: body.id,
+        signal: AbortSignal.timeout(settings.apis.v1.timeout || 10_000),
-            portalPath,
+      })
-            ssoEnabled: body.sso_enabled,
+      if (!body) return
-          }
+
-          NotificationsHandler.createNotification(
+      const key = `ip-matched-affiliation-${body.id}`
-            userId,
+      const portalPath = body.portal_slug
-            key,
+        ? `/${body.is_university ? 'edu' : 'org'}/${body.portal_slug}`
-            'notification_ip_matched_affiliation',
+        : undefined
-            messageOpts,
+      const messageOpts = {
-            null,
+        university_name: body.name,
-            false,
+        institutionId: body.id,
-            callback
+        portalPath,
-          )
+        ssoEnabled: body.sso_enabled,
-        }
+      }
+      return await NotificationsHandler.promises.createNotification(
+        userId,
+        key,
+        'notification_ip_matched_affiliation',
+        messageOpts,
+        null,
+        false
       )
     },
 
-    read(universityId, callback) {
+    async read(universityId) {
-      if (callback == null) {
-        callback = function () {}
-      }
       const key = `ip-matched-affiliation-${universityId}`
-      NotificationsHandler.markAsReadWithKey(userId, key, callback)
+      return await NotificationsHandler.promises.markAsReadWithKey(userId, key)
     },
   }
 }
@@ -197,29 +173,22 @@ function ipMatcherAffiliation(userId) {
 function tpdsFileLimit(userId) {
   return {
     key: `tpdsFileLimit-${userId}`,
-    create(projectName, projectId, callback) {
+    async create(projectName, projectId) {
-      if (callback == null) {
-        callback = function () {}
-      }
       const messageOpts = {
         projectName,
         projectId,
       }
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         userId,
         this.key,
         'notification_tpds_file_limit',
         messageOpts,
         null,
-        true,
+        true
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key)
-        callback = function () {}
-      }
-      NotificationsHandler.markAsReadByKeyOnly(this.key, callback)
     },
   }
 }
@@ -227,30 +196,23 @@ function tpdsFileLimit(userId) {
 function groupInvitation(userId, subscriptionId, managedUsersEnabled) {
   return {
     key: `groupInvitation-${subscriptionId}-${userId}`,
-    create(invite, callback) {
+    async create(invite) {
-      if (callback == null) {
-        callback = function () {}
-      }
       const messageOpts = {
         token: invite.token,
         inviterName: invite.inviterName,
         managedUsersEnabled,
       }
-      NotificationsHandler.createNotification(
+      return await NotificationsHandler.promises.createNotification(
         userId,
         this.key,
         'notification_group_invitation',
         messageOpts,
         null,
-        true,
+        true
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key)
-        callback = function () {}
-      }
-      NotificationsHandler.markAsReadByKeyOnly(this.key, callback)
     },
   }
 }
@@ -258,67 +220,63 @@ function groupInvitation(userId, subscriptionId, managedUsersEnabled) {
 function personalAndGroupSubscriptions(userId) {
   return {
     key: 'personal-and-group-subscriptions',
-    create(callback) {
+    async create() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.createNotification(
-        callback = function () {}
-      }
-      NotificationsHandler.createNotification(
         userId,
         this.key,
         'notification_personal_and_group_subscriptions',
         {},
         null,
-        false,
+        false
-        callback
       )
     },
-    read(callback) {
+    async read() {
-      if (callback == null) {
+      return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key)
-        callback = function () {}
-      }
-      NotificationsHandler.markAsReadByKeyOnly(this.key, callback)
     },
   }
 }
 
 const NotificationsBuilder = {
   // Note: notification keys should be url-safe
-  dropboxUnlinkedDueToLapsedReconfirmation,
+  dropboxUnlinkedDueToLapsedReconfirmation(userId) {
-  dropboxDuplicateProjectNames,
+    return callbackifyAll(dropboxUnlinkedDueToLapsedReconfirmation(userId))
-  featuresUpgradedByAffiliation,
+  },
-  redundantPersonalSubscription,
+  dropboxDuplicateProjectNames(userId) {
-  projectInvite,
+    return callbackifyAll(dropboxDuplicateProjectNames(userId))
-  ipMatcherAffiliation,
+  },
-  tpdsFileLimit,
+  featuresUpgradedByAffiliation(affiliation, user) {
-  groupInvitation,
+    return callbackifyAll(featuresUpgradedByAffiliation(affiliation, user))
-  personalAndGroupSubscriptions,
+  },
+  redundantPersonalSubscription(affiliation, user) {
+    return callbackifyAll(redundantPersonalSubscription(affiliation, user))
+  },
+  projectInvite(invite, project, sendingUser, user) {
+    return callbackifyAll(projectInvite(invite, project, sendingUser, user))
+  },
+  ipMatcherAffiliation(userId) {
+    return callbackifyAll(ipMatcherAffiliation(userId))
+  },
+  tpdsFileLimit(userId) {
+    return callbackifyAll(tpdsFileLimit(userId))
+  },
+  groupInvitation(userId, groupId, managedUsersEnabled) {
+    return callbackifyAll(groupInvitation(userId, groupId, managedUsersEnabled))
+  },
+  personalAndGroupSubscriptions(userId) {
+    return callbackifyAll(personalAndGroupSubscriptions(userId))
+  },
 }
 
 NotificationsBuilder.promises = {
-  dropboxUnlinkedDueToLapsedReconfirmation: function (userId) {
+  dropboxUnlinkedDueToLapsedReconfirmation,
-    return promisifyAll(dropboxUnlinkedDueToLapsedReconfirmation(userId))
+  redundantPersonalSubscription,
-  },
+  dropboxDuplicateProjectNames,
-  redundantPersonalSubscription: function (affiliation, user) {
+  featuresUpgradedByAffiliation,
-    return promisifyAll(redundantPersonalSubscription(affiliation, user))
+  ipMatcherAffiliation,
-  },
+  groupInvitation,
-  dropboxDuplicateProjectNames(userId) {
+  projectInvite,
-    return promisifyAll(dropboxDuplicateProjectNames(userId))
+  personalAndGroupSubscriptions,
-  },
+  tpdsFileLimit,
-  featuresUpgradedByAffiliation: function (affiliation, user) {
-    return promisifyAll(featuresUpgradedByAffiliation(affiliation, user))
-  },
-  ipMatcherAffiliation: function (userId) {
-    return promisifyAll(ipMatcherAffiliation(userId))
-  },
-  groupInvitation: function (userId, groupId, managedUsersEnabled) {
-    return promisifyAll(groupInvitation(userId, groupId, managedUsersEnabled))
-  },
-  projectInvite(invite, project, sendingUser, user) {
-    return promisifyAll(projectInvite(invite, project, sendingUser, user))
-  },
-  personalAndGroupSubscriptions(userId) {
-    return promisifyAll(personalAndGroupSubscriptions(userId))
-  },
 }
 
 module.exports = NotificationsBuilder

services/web/app/src/Features/Project/ProjectListController.mjs

@@ -399,7 +399,7 @@ async function projectListPage(req, res, next) {
   if (Settings.overleaf != null && req.ip !== user.lastLoginIp) {
     try {
       await NotificationsBuilder.promises
-        .ipMatcherAffiliation(user._id)
+        .ipMatcherAffiliation(user._id.toString())
         .create(req.ip)
     } catch (err) {
       logger.error(

services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs

@@ -62,7 +62,9 @@ async function mergeUpdate(req, res) {
         { err, userId, filePath },
         'tpds trying to append to project over file limit'
       )
-      NotificationsBuilder.tpdsFileLimit(userId).create(projectName, projectId)
+      await NotificationsBuilder.promises
+        .tpdsFileLimit(userId)
+        .create(projectName, projectId)
       return res.sendStatus(400)
     } else {
       throw err

services/web/test/unit/src/Notifications/NotificationsBuilderTests.js

@@ -7,17 +7,21 @@ const modulePath = require('path').join(
 )
 
 describe('NotificationsBuilder', function () {
-  const userId = '123nd3ijdks'
+  const userId = '507f1f77bcf86cd799439011'
 
   beforeEach(function () {
-    this.handler = { createNotification: sinon.stub().callsArgWith(6) }
+    this.handler = { promises: { createNotification: sinon.stub().resolves() } }
-    this.settings = { apis: { v1: { url: 'v1.url', user: '', pass: '' } } }
+    this.settings = {
-    this.request = sinon.stub()
+      apis: { v1: { url: 'http://v1.url', user: '', pass: '' } },
+    }
+    this.FetchUtils = {
+      fetchJson: sinon.stub(),
+    }
     this.controller = SandboxedModule.require(modulePath, {
       requires: {
         './NotificationsHandler': this.handler,
         '@overleaf/settings': this.settings,
-        request: this.request,
+        '@overleaf/fetch-utils': this.FetchUtils,
       },
     })
   })
@@ -27,7 +31,7 @@ describe('NotificationsBuilder', function () {
       await this.controller.promises
         .dropboxUnlinkedDueToLapsedReconfirmation(userId)
         .create()
-      expect(this.handler.createNotification).to.have.been.calledWith(
+      expect(this.handler.promises.createNotification).to.have.been.calledWith(
         userId,
         'drobox-unlinked-due-to-lapsed-reconfirmation',
         'notification_dropbox_unlinked_due_to_lapsed_reconfirmation',
@@ -40,7 +44,7 @@ describe('NotificationsBuilder', function () {
       let anError
       beforeEach(function () {
         anError = new Error('oops')
-        this.handler.createNotification.yields(anError)
+        this.handler.promises.createNotification.rejects(anError)
       })
       it('should return errors from NotificationsHandler', async function () {
         let error
@@ -76,7 +80,7 @@ describe('NotificationsBuilder', function () {
           this.invite.managedUsersEnabled
         )
         .create(this.invite)
-      expect(this.handler.createNotification).to.have.been.calledWith(
+      expect(this.handler.promises.createNotification).to.have.been.calledWith(
         userId,
         `groupInvitation-${subscriptionId}-${userId}`,
         'notification_group_invitation',
@@ -101,20 +105,20 @@ describe('NotificationsBuilder', function () {
           portal_slug: null,
           sso_enabled: false,
         }
-        this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
+        this.FetchUtils.fetchJson.resolves(this.body)
       })
 
       it('should call v1 and create affiliation notifications', async function () {
         const ip = '192.168.0.1'
         await this.controller.promises.ipMatcherAffiliation(userId).create(ip)
-        this.request.calledOnce.should.equal(true)
+        this.FetchUtils.fetchJson.calledOnce.should.equal(true)
         const expectedOpts = {
           institutionId: this.body.id,
           university_name: this.body.name,
           ssoEnabled: false,
           portalPath: undefined,
         }
-        this.handler.createNotification
+        this.handler.promises.createNotification
           .calledWith(
             userId,
             `ip-matched-affiliation-${this.body.id}`,
@@ -133,20 +137,20 @@ describe('NotificationsBuilder', function () {
           portal_slug: 'stanford',
           sso_enabled: true,
         }
-        this.request.callsArgWith(1, null, { statusCode: 200 }, this.body)
+        this.FetchUtils.fetchJson.resolves(this.body)
       })
 
       it('should call v1 and create affiliation notifications', async function () {
         const ip = '192.168.0.1'
         await this.controller.promises.ipMatcherAffiliation(userId).create(ip)
-        this.request.calledOnce.should.equal(true)
+        this.FetchUtils.fetchJson.calledOnce.should.equal(true)
         const expectedOpts = {
           institutionId: this.body.id,
           university_name: this.body.name,
           ssoEnabled: true,
           portalPath: '/edu/stanford',
         }
-        this.handler.createNotification
+        this.handler.promises.createNotification
           .calledWith(
             userId,
             `ip-matched-affiliation-${this.body.id}`,

services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs

@@ -33,7 +33,9 @@ describe('TpdsController', function () {
       },
     }
     ctx.NotificationsBuilder = {
-      tpdsFileLimit: sinon.stub().returns({ create: sinon.stub() }),
+      promises: {
+        tpdsFileLimit: sinon.stub().returns({ create: sinon.stub() }),
+      },
     }
     ctx.SessionManager = {
       getLoggedInUserId: sinon.stub().returns('user-id'),
@@ -255,7 +257,7 @@ describe('TpdsController', function () {
         const res = {
           sendStatus: status => {
             expect(status).to.equal(400)
-            ctx.NotificationsBuilder.tpdsFileLimit.should.have.been.calledWith(
+            ctx.NotificationsBuilder.promises.tpdsFileLimit.should.have.been.calledWith(
               ctx.user_id
             )
             resolve()