From 2413a93c5e0a324acc8c94bbbdc92bbe51207ce2 Mon Sep 17 00:00:00 2001 From: Antoine Clausse Date: Wed, 22 Oct 2025 11:10:56 +0200 Subject: [PATCH] [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 * 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 --------- Co-authored-by: Alf Eaton Co-authored-by: Andrew Rumble GitOrigin-RevId: 17ee861852aa4ac15d3b46b1b28c763fad5333d2 --- .../AuthenticationController.js | 2 +- .../Features/Institutions/InstitutionsAPI.js | 2 +- .../Notifications/NotificationsBuilder.js | 300 ++++++++---------- .../Project/ProjectListController.mjs | 2 +- .../ThirdPartyDataStore/TpdsController.mjs | 4 +- .../NotificationsBuilderTests.js | 32 +- .../TpdsController.test.mjs | 6 +- 7 files changed, 157 insertions(+), 191 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index e33b2ce7d8..a78b7e5ba0 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/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, () => {} ) diff --git a/services/web/app/src/Features/Institutions/InstitutionsAPI.js b/services/web/app/src/Features/Institutions/InstitutionsAPI.js index 3db0a470da..34cd2f9908 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsAPI.js +++ b/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 diff --git a/services/web/app/src/Features/Notifications/NotificationsBuilder.js b/services/web/app/src/Features/Notifications/NotificationsBuilder.js index 9236b628ab..42cc446f0a 100644 --- a/services/web/app/src/Features/Notifications/NotificationsBuilder.js +++ b/services/web/app/src/Features/Notifications/NotificationsBuilder.js @@ -1,30 +1,28 @@ const NotificationsHandler = require('./NotificationsHandler') -const { promisifyAll } = require('@overleaf/promise-utils') -const request = require('request') +const { callbackifyAll } = require('@overleaf/promise-utils') +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) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.createNotification( + async create(projectName) { + return await NotificationsHandler.promises.createNotification( userId, this.key, 'notification_dropbox_duplicate_project_names', { projectName }, null, - true, - callback + true ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadWithKey(userId, this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadWithKey( + userId, + this.key + ) }, } } @@ -32,19 +30,21 @@ function dropboxDuplicateProjectNames(userId) { function dropboxUnlinkedDueToLapsedReconfirmation(userId) { return { key: 'drobox-unlinked-due-to-lapsed-reconfirmation', - create(callback) { - NotificationsHandler.createNotification( + async create() { + return await NotificationsHandler.promises.createNotification( userId, this.key, 'notification_dropbox_unlinked_due_to_lapsed_reconfirmation', {}, null, - true, - callback + true ) }, - read(callback) { - NotificationsHandler.markAsReadWithKey(userId, this.key, callback) + async read() { + 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) { - if (callback == null) { - callback = function () {} - } + async create() { const messageOpts = { institutionName: affiliation.institutionName } - NotificationsHandler.createNotification( + return await NotificationsHandler.promises.createNotification( user._id, this.key, 'notification_features_upgraded_by_affiliation', messageOpts, null, - false, - callback + false ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadWithKey(user._id, this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadWithKey( + user._id, + this.key + ) }, } } @@ -79,26 +75,22 @@ function featuresUpgradedByAffiliation(affiliation, user) { function redundantPersonalSubscription(affiliation, user) { return { key: `redundant-personal-subscription-${affiliation.institutionId}`, - create(callback) { - if (callback == null) { - callback = function () {} - } + async create() { 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, - callback + false ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadWithKey(user._id, this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadWithKey( + user._id, + this.key + ) }, } } @@ -106,90 +98,74 @@ function redundantPersonalSubscription(affiliation, user) { function projectInvite(invite, project, sendingUser, user) { return { key: `project-invite-${invite._id}`, - create(callback) { - if (callback == null) { - callback = function () {} - } + async create() { 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, - callback + invite.expires ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadByKeyOnly(this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key) }, } } function ipMatcherAffiliation(userId) { return { - create(ip, callback) { - if (callback == null) { - callback = function () {} - } + async create(ip) { if (!settings.apis.v1.url) { // service is not configured - return callback() + return } - request( - { - method: 'GET', - url: `${settings.apis.v1.url}/api/v2/users/${userId}/ip_matcher`, - auth: { user: settings.apis.v1.user, pass: settings.apis.v1.pass }, - body: { 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() - } + if (!ObjectId.isValid(userId)) { + throw new Error('invalid user id') + } + const url = new URL(settings.apis.v1.url) + url.pathname = path.posix.join('/api/v2/users', userId, 'ip_matcher') + url.searchParams.set('ip', ip) - const key = `ip-matched-affiliation-${body.id}` - const portalPath = body.portal_slug - ? `/${body.is_university ? 'edu' : 'org'}/${body.portal_slug}` - : undefined - const messageOpts = { - university_name: body.name, - institutionId: body.id, - portalPath, - ssoEnabled: body.sso_enabled, - } - NotificationsHandler.createNotification( - userId, - key, - 'notification_ip_matched_affiliation', - messageOpts, - null, - false, - callback - ) - } + const body = await fetchJson(url, { + method: 'GET', + basicAuth: { + user: settings.apis.v1.user, + password: settings.apis.v1.pass, + }, + signal: AbortSignal.timeout(settings.apis.v1.timeout || 10_000), + }) + if (!body) return + + const key = `ip-matched-affiliation-${body.id}` + const portalPath = body.portal_slug + ? `/${body.is_university ? 'edu' : 'org'}/${body.portal_slug}` + : undefined + const messageOpts = { + university_name: body.name, + institutionId: body.id, + portalPath, + ssoEnabled: body.sso_enabled, + } + return await NotificationsHandler.promises.createNotification( + userId, + key, + 'notification_ip_matched_affiliation', + messageOpts, + null, + false ) }, - read(universityId, callback) { - if (callback == null) { - callback = function () {} - } + async read(universityId) { 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) { - if (callback == null) { - callback = function () {} - } + async create(projectName, projectId) { const messageOpts = { projectName, projectId, } - NotificationsHandler.createNotification( + return await NotificationsHandler.promises.createNotification( userId, this.key, 'notification_tpds_file_limit', messageOpts, null, - true, - callback + true ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadByKeyOnly(this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key) }, } } @@ -227,30 +196,23 @@ function tpdsFileLimit(userId) { function groupInvitation(userId, subscriptionId, managedUsersEnabled) { return { key: `groupInvitation-${subscriptionId}-${userId}`, - create(invite, callback) { - if (callback == null) { - callback = function () {} - } + async create(invite) { 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, - callback + true ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadByKeyOnly(this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key) }, } } @@ -258,67 +220,63 @@ function groupInvitation(userId, subscriptionId, managedUsersEnabled) { function personalAndGroupSubscriptions(userId) { return { key: 'personal-and-group-subscriptions', - create(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.createNotification( + async create() { + return await NotificationsHandler.promises.createNotification( userId, this.key, 'notification_personal_and_group_subscriptions', {}, null, - false, - callback + false ) }, - read(callback) { - if (callback == null) { - callback = function () {} - } - NotificationsHandler.markAsReadByKeyOnly(this.key, callback) + async read() { + return await NotificationsHandler.promises.markAsReadByKeyOnly(this.key) }, } } const NotificationsBuilder = { // Note: notification keys should be url-safe - dropboxUnlinkedDueToLapsedReconfirmation, - dropboxDuplicateProjectNames, - featuresUpgradedByAffiliation, - redundantPersonalSubscription, - projectInvite, - ipMatcherAffiliation, - tpdsFileLimit, - groupInvitation, - personalAndGroupSubscriptions, + dropboxUnlinkedDueToLapsedReconfirmation(userId) { + return callbackifyAll(dropboxUnlinkedDueToLapsedReconfirmation(userId)) + }, + dropboxDuplicateProjectNames(userId) { + return callbackifyAll(dropboxDuplicateProjectNames(userId)) + }, + featuresUpgradedByAffiliation(affiliation, user) { + return callbackifyAll(featuresUpgradedByAffiliation(affiliation, user)) + }, + 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) { - return promisifyAll(dropboxUnlinkedDueToLapsedReconfirmation(userId)) - }, - redundantPersonalSubscription: function (affiliation, user) { - return promisifyAll(redundantPersonalSubscription(affiliation, user)) - }, - dropboxDuplicateProjectNames(userId) { - return promisifyAll(dropboxDuplicateProjectNames(userId)) - }, - 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)) - }, + dropboxUnlinkedDueToLapsedReconfirmation, + redundantPersonalSubscription, + dropboxDuplicateProjectNames, + featuresUpgradedByAffiliation, + ipMatcherAffiliation, + groupInvitation, + projectInvite, + personalAndGroupSubscriptions, + tpdsFileLimit, } module.exports = NotificationsBuilder diff --git a/services/web/app/src/Features/Project/ProjectListController.mjs b/services/web/app/src/Features/Project/ProjectListController.mjs index 26220d4816..087f2b2340 100644 --- a/services/web/app/src/Features/Project/ProjectListController.mjs +++ b/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( diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs index 451a966351..96faac3842 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.mjs +++ b/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 diff --git a/services/web/test/unit/src/Notifications/NotificationsBuilderTests.js b/services/web/test/unit/src/Notifications/NotificationsBuilderTests.js index 8a8eda5a34..5ad0476427 100644 --- a/services/web/test/unit/src/Notifications/NotificationsBuilderTests.js +++ b/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.settings = { apis: { v1: { url: 'v1.url', user: '', pass: '' } } } - this.request = sinon.stub() + this.handler = { promises: { createNotification: sinon.stub().resolves() } } + this.settings = { + 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}`, diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs b/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs index b38e3c2bab..f59273eed1 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsController.test.mjs +++ b/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()