Merge pull request #16444 from overleaf/jel-v1-api

[web] Begin promisifying InstitutionsAPI

GitOrigin-RevId: aa615e22d3fc8aa77255a26dcdab0e2ce28d93c0

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

@@ -1,3 +1,4 @@
+const { callbackify } = require('util')
 const OError = require('@overleaf/o-error')
 const logger = require('@overleaf/logger')
 const metrics = require('@overleaf/metrics')
@@ -9,210 +10,306 @@ const {
   V1ConnectionError,
   InvalidInstitutionalEmailError,
 } = require('../Errors/Errors')
+const { fetchJson, fetchNothing } = require('@overleaf/fetch-utils')
 
-const InstitutionsAPI = {
-  getInstitutionAffiliations(institutionId, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: `/api/v2/institutions/${institutionId.toString()}/affiliations`,
-        defaultErrorMessage: "Couldn't get institution affiliations",
-      },
-      (error, body) => callback(error, body || [])
-    )
-  },
-
-  getConfirmedInstitutionAffiliations(institutionId, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: `/api/v2/institutions/${institutionId.toString()}/confirmed_affiliations`,
-        defaultErrorMessage: "Couldn't get institution affiliations",
-      },
-      (error, body) => callback(error, body || [])
-    )
-  },
-
-  getInstitutionAffiliationsCounts(institutionId, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: `/api/v2/institutions/${institutionId.toString()}/affiliations_counts`,
-        defaultErrorMessage: "Couldn't get institution counts",
-      },
-      (error, body) => callback(error, body || [])
-    )
-  },
-
-  getLicencesForAnalytics(lag, queryDate, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: `/api/v2/institutions/institutions_licences`,
-        body: { query_date: queryDate, lag },
-        defaultErrorMessage: 'Could not get institutions licences',
-      },
-      callback
-    )
-  },
-
-  getUserAffiliations(userId, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: `/api/v2/users/${userId.toString()}/affiliations`,
-        defaultErrorMessage: "Couldn't get user affiliations",
-      },
-      (error, body) => callback(error, body || [])
-    )
-  },
-
-  getUsersNeedingReconfirmationsLapsedProcessed(callback) {
-    makeAffiliationRequest(
-      {
-        method: 'GET',
-        path: '/api/v2/institutions/need_reconfirmation_lapsed_processed',
-        defaultErrorMessage:
-          'Could not get users that need reconfirmations lapsed processed',
+function _makeRequestOptions(options) {
+  const requestOptions = {
+    method: options.method,
+    basicAuth: { user: settings.apis.v1.user, password: settings.apis.v1.pass },
+    signal: AbortSignal.timeout(settings.apis.v1.timeout),
+  }
+
+  if (options.body) {
+    requestOptions.json = options.body
+  }
+
+  return requestOptions
+}
+
+function _responseErrorHandling(options, error) {
+  const status = error.response.status
+
+  if (status >= 500) {
+    throw new V1ConnectionError({
+      message: 'error getting affiliations from v1',
+      info: {
+        status,
+        body: error.body,
       },
-      (error, body) => callback(error, body || [])
-    )
-  },
-
-  addAffiliation(userId, email, affiliationOptions, callback) {
-    if (!callback) {
-      // affiliationOptions is optional
-      callback = affiliationOptions
-      affiliationOptions = {}
+    })
+  }
+
+  let errorBody
+
+  try {
+    if (error.body) {
+      errorBody = JSON.parse(error.body)
     }
+  } catch (e) {}
 
-    const {
-      university,
-      department,
-      role,
-      confirmedAt,
-      entitlement,
-      rejectIfBlocklisted,
-    } = affiliationOptions
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: `/api/v2/users/${userId.toString()}/affiliations`,
-        body: {
-          email,
-          university,
-          department,
-          role,
-          confirmedAt,
-          entitlement,
-          rejectIfBlocklisted,
-        },
-        defaultErrorMessage: "Couldn't create affiliation",
-      },
-      function (error, body) {
-        if (error) {
-          if (error.info && error.info.statusCode === 422) {
-            return callback(
-              new InvalidInstitutionalEmailError(error.message).withCause(error)
-            )
-          }
-          return callback(error)
-        }
-        if (!university) {
-          return callback(null, body)
-        }
-
-        // have notifications delete any ip matcher notifications for this university
-        NotificationsBuilder.ipMatcherAffiliation(userId).read(
-          university.id,
-          function (err) {
-            if (err) {
-              // log and ignore error
-              logger.err(
-                { err },
-                'Something went wrong marking ip notifications read'
-              )
-            }
-            callback(null, body)
-          }
-        )
-      }
-    )
-  },
-
-  removeAffiliation(userId, email, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: `/api/v2/users/${userId.toString()}/affiliations/remove`,
-        body: { email },
-        extraSuccessStatusCodes: [404], // `Not Found` responses are considered successful
-        defaultErrorMessage: "Couldn't remove affiliation",
-      },
-      callback
-    )
-  },
-
-  endorseAffiliation(userId, email, role, department, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: `/api/v2/users/${userId.toString()}/affiliations/endorse`,
-        body: { email, role, department },
-        defaultErrorMessage: "Couldn't endorse affiliation",
-      },
-      callback
-    )
-  },
-
-  deleteAffiliations(userId, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'DELETE',
-        path: `/api/v2/users/${userId.toString()}/affiliations`,
-        defaultErrorMessage: "Couldn't delete affiliations",
-      },
-      callback
-    )
-  },
-
-  addEntitlement(userId, email, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: `/api/v2/users/${userId}/affiliations/add_entitlement`,
-        body: { email },
-        defaultErrorMessage: "Couldn't add entitlement",
-      },
-      callback
-    )
-  },
-
-  removeEntitlement(userId, email, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: `/api/v2/users/${userId}/affiliations/remove_entitlement`,
-        body: { email },
-        defaultErrorMessage: "Couldn't remove entitlement",
-        extraSuccessStatusCodes: [404],
-      },
-      callback
-    )
-  },
-
-  sendUsersWithReconfirmationsLapsedProcessed(users, callback) {
-    makeAffiliationRequest(
-      {
-        method: 'POST',
-        path: '/api/v2/institutions/reconfirmation_lapsed_processed',
-        body: { users },
-        defaultErrorMessage:
-          'Could not update reconfirmation_lapsed_processed_at',
+  let errorMessage
+  if (errorBody?.errors) {
+    errorMessage = `${status}: ${errorBody.errors}`
+  } else {
+    errorMessage = `${options.defaultErrorMessage}: ${status}`
+  }
+
+  throw new OError(errorMessage, { status })
+}
+
+async function _affiliationRequestFetchJson(options) {
+  if (!settings.apis.v1.url) {
+    return
+  } // service is not configured
+
+  const url = `${settings.apis.v1.url}${options.path}`
+
+  const requestOptions = _makeRequestOptions(options)
+
+  try {
+    return await fetchJson(url, requestOptions)
+  } catch (error) {
+    _responseErrorHandling(options, error)
+  }
+}
+
+async function _affiliationRequestFetchNothing(options) {
+  if (!settings.apis.v1.url) {
+    return
+  } // service is not configured
+
+  const url = `${settings.apis.v1.url}${options.path}`
+
+  const requestOptions = _makeRequestOptions(options)
+
+  try {
+    await fetchNothing(url, requestOptions)
+  } catch (error) {
+    _responseErrorHandling(options, error)
+  }
+}
+
+async function _affiliationRequestFetchNothing404Ok(options) {
+  try {
+    await _affiliationRequestFetchNothing(options)
+  } catch (error) {
+    const status = error.info?.status
+    if (status !== 404) {
+      throw error
+    }
+  }
+}
+
+function getInstitutionAffiliations(institutionId, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'GET',
+      path: `/api/v2/institutions/${institutionId.toString()}/affiliations`,
+      defaultErrorMessage: "Couldn't get institution affiliations",
+    },
+    (error, body) => callback(error, body || [])
+  )
+}
+
+function getConfirmedInstitutionAffiliations(institutionId, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'GET',
+      path: `/api/v2/institutions/${institutionId.toString()}/confirmed_affiliations`,
+      defaultErrorMessage: "Couldn't get institution affiliations",
+    },
+    (error, body) => callback(error, body || [])
+  )
+}
+
+function getInstitutionAffiliationsCounts(institutionId, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'GET',
+      path: `/api/v2/institutions/${institutionId.toString()}/affiliations_counts`,
+      defaultErrorMessage: "Couldn't get institution counts",
+    },
+    (error, body) => callback(error, body || [])
+  )
+}
+
+function getLicencesForAnalytics(lag, queryDate, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'GET',
+      path: `/api/v2/institutions/institutions_licences`,
+      body: { query_date: queryDate, lag },
+      defaultErrorMessage: 'Could not get institutions licences',
+    },
+    callback
+  )
+}
+
+function getUserAffiliations(userId, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'GET',
+      path: `/api/v2/users/${userId.toString()}/affiliations`,
+      defaultErrorMessage: "Couldn't get user affiliations",
+    },
+    (error, body) => callback(error, body || [])
+  )
+}
+
+async function getUsersNeedingReconfirmationsLapsedProcessed() {
+  return await _affiliationRequestFetchJson({
+    method: 'GET',
+    path: '/api/v2/institutions/need_reconfirmation_lapsed_processed',
+    defaultErrorMessage:
+      'Could not get users that need reconfirmations lapsed processed',
+  })
+}
+
+async function addAffiliation(userId, email, affiliationOptions) {
+  const {
+    university,
+    department,
+    role,
+    confirmedAt,
+    entitlement,
+    rejectIfBlocklisted,
+  } = affiliationOptions
+
+  try {
+    await _affiliationRequestFetchNothing({
+      method: 'POST',
+      path: `/api/v2/users/${userId.toString()}/affiliations`,
+      body: {
+        email,
+        university,
+        department,
+        role,
+        confirmedAt,
+        entitlement,
+        rejectIfBlocklisted,
       },
-      (error, body) => callback(error, body || [])
-    )
-  },
+      defaultErrorMessage: "Couldn't create affiliation",
+    })
+  } catch (error) {
+    if (error.info?.status === 422) {
+      throw new InvalidInstitutionalEmailError(error.message).withCause(error)
+    }
+    throw error
+  }
+
+  if (!university) {
+    return
+  }
+
+  // have notifications delete any ip matcher notifications for this university
+  try {
+    await NotificationsBuilder.promises
+      .ipMatcherAffiliation(userId)
+      .read(university.id)
+  } catch (err) {
+    // log and ignore error
+    logger.err({ err }, 'Something went wrong marking ip notifications read')
+  }
+}
+
+async function removeAffiliation(userId, email) {
+  await _affiliationRequestFetchNothing404Ok({
+    method: 'POST',
+    path: `/api/v2/users/${userId.toString()}/affiliations/remove`,
+    body: { email },
+    defaultErrorMessage: "Couldn't remove affiliation",
+  })
+}
+
+function endorseAffiliation(userId, email, role, department, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'POST',
+      path: `/api/v2/users/${userId.toString()}/affiliations/endorse`,
+      body: { email, role, department },
+      defaultErrorMessage: "Couldn't endorse affiliation",
+    },
+    callback
+  )
+}
+
+function deleteAffiliations(userId, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'DELETE',
+      path: `/api/v2/users/${userId.toString()}/affiliations`,
+      defaultErrorMessage: "Couldn't delete affiliations",
+    },
+    callback
+  )
+}
+
+function addEntitlement(userId, email, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'POST',
+      path: `/api/v2/users/${userId}/affiliations/add_entitlement`,
+      body: { email },
+      defaultErrorMessage: "Couldn't add entitlement",
+    },
+    callback
+  )
+}
+
+function removeEntitlement(userId, email, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'POST',
+      path: `/api/v2/users/${userId}/affiliations/remove_entitlement`,
+      body: { email },
+      defaultErrorMessage: "Couldn't remove entitlement",
+      extraSuccessStatusCodes: [404],
+    },
+    callback
+  )
+}
+
+function sendUsersWithReconfirmationsLapsedProcessed(users, callback) {
+  makeAffiliationRequest(
+    {
+      method: 'POST',
+      path: '/api/v2/institutions/reconfirmation_lapsed_processed',
+      body: { users },
+      defaultErrorMessage:
+        'Could not update reconfirmation_lapsed_processed_at',
+    },
+    (error, body) => callback(error, body || [])
+  )
+}
+
+const InstitutionsAPI = {
+  getInstitutionAffiliations,
+
+  getConfirmedInstitutionAffiliations,
+
+  getInstitutionAffiliationsCounts,
+
+  getLicencesForAnalytics,
+
+  getUserAffiliations,
+
+  getUsersNeedingReconfirmationsLapsedProcessed: callbackify(
+    getUsersNeedingReconfirmationsLapsedProcessed
+  ),
+
+  addAffiliation: callbackify(addAffiliation),
+
+  removeAffiliation: callbackify(removeAffiliation),
+
+  endorseAffiliation,
+
+  deleteAffiliations,
+
+  addEntitlement,
+
+  removeEntitlement,
+
+  sendUsersWithReconfirmationsLapsedProcessed,
 }
 
 function makeAffiliationRequest(options, callback) {
@@ -280,8 +377,6 @@ function makeAffiliationRequest(options, callback) {
   'getInstitutionAffiliations',
   'getConfirmedInstitutionAffiliations',
   'getUserAffiliations',
-  'addAffiliation',
-  'removeAffiliation',
 ].map(method =>
   metrics.timeAsyncMethod(
     InstitutionsAPI,
@@ -291,5 +386,17 @@ function makeAffiliationRequest(options, callback) {
   )
 )
 
-InstitutionsAPI.promises = promisifyAll(InstitutionsAPI)
+InstitutionsAPI.promises = promisifyAll(InstitutionsAPI, {
+  without: [
+    'addAffiliation',
+    'removeAffiliation',
+    'getUsersNeedingReconfirmationsLapsedProcessed',
+  ],
+})
+
+InstitutionsAPI.promises.addAffiliation = addAffiliation
+InstitutionsAPI.promises.removeAffiliation = removeAffiliation
+InstitutionsAPI.promises.getUsersNeedingReconfirmationsLapsedProcessed =
+  getUsersNeedingReconfirmationsLapsedProcessed
+
 module.exports = InstitutionsAPI

services/web/test/unit/src/Institutions/InstitutionsAPITests.js

@@ -10,10 +10,13 @@ const Errors = require('../../../../app/src/Features/Errors/Errors')
 
 describe('InstitutionsAPI', function () {
   beforeEach(function () {
-    this.settings = { apis: { v1: { url: 'v1.url', user: '', pass: '' } } }
+    this.settings = {
+      apis: { v1: { url: 'v1.url', user: '', pass: '', timeout: 5000 } },
+    }
     this.request = sinon.stub()
+    this.fetchNothing = sinon.stub()
     this.ipMatcherNotification = {
-      read: (this.markAsReadIpMatcher = sinon.stub().callsArgWith(1, null)),
+      read: (this.markAsReadIpMatcher = sinon.stub().resolves()),
     }
     this.InstitutionsAPI = SandboxedModule.require(modulePath, {
       requires: {
@@ -22,10 +25,16 @@ describe('InstitutionsAPI', function () {
         },
         '@overleaf/settings': this.settings,
         requestretry: this.request,
+        '@overleaf/fetch-utils': {
+          fetchNothing: this.fetchNothing,
+          fetchJson: (this.fetchJson = sinon.stub()),
+        },
         '../Notifications/NotificationsBuilder': {
-          ipMatcherAffiliation: sinon
-            .stub()
-            .returns(this.ipMatcherNotification),
+          promises: {
+            ipMatcherAffiliation: sinon
+              .stub()
+              .returns(this.ipMatcherNotification),
+          },
         },
       },
     })
@@ -162,14 +171,14 @@ describe('InstitutionsAPI', function () {
 
   describe('getUsersNeedingReconfirmationsLapsedProcessed', function () {
     it('get the list of users', function (done) {
-      this.request.callsArgWith(1, null, { statusCode: 200 })
+      this.fetchJson.resolves({ statusCode: 200 })
       this.InstitutionsAPI.getUsersNeedingReconfirmationsLapsedProcessed(
         error => {
           expect(error).not.to.exist
-          this.request.calledOnce.should.equal(true)
-          const requestOptions = this.request.lastCall.args[0]
+          this.fetchJson.calledOnce.should.equal(true)
+          const requestOptions = this.fetchJson.lastCall.args[1]
           const expectedUrl = `v1.url/api/v2/institutions/need_reconfirmation_lapsed_processed`
-          requestOptions.url.should.equal(expectedUrl)
+          this.fetchJson.lastCall.args[0].should.equal(expectedUrl)
           requestOptions.method.should.equal('GET')
           done()
         }
@@ -177,7 +186,7 @@ describe('InstitutionsAPI', function () {
     })
 
     it('handle error', function (done) {
-      this.request.callsArgWith(1, null, { statusCode: 500 })
+      this.fetchJson.throws({ info: { statusCode: 500 } })
       this.InstitutionsAPI.getUsersNeedingReconfirmationsLapsedProcessed(
         error => {
           expect(error).to.exist
@@ -189,14 +198,14 @@ describe('InstitutionsAPI', function () {
 
   describe('addAffiliation', function () {
     beforeEach(function () {
-      this.request.callsArgWith(1, null, { statusCode: 201 })
+      this.fetchNothing.resolves({ status: 201 })
     })
 
     it('add affiliation', function (done) {
       const affiliationOptions = {
         university: { id: 1 },
-        role: 'Prof',
         department: 'Math',
+        role: 'Prof',
         confirmedAt: new Date(),
         entitlement: true,
       }
@@ -206,38 +215,86 @@ describe('InstitutionsAPI', function () {
         affiliationOptions,
         err => {
           expect(err).not.to.exist
-          this.request.calledOnce.should.equal(true)
-          const requestOptions = this.request.lastCall.args[0]
+          this.fetchNothing.calledOnce.should.equal(true)
+          const requestOptions = this.fetchNothing.lastCall.args[1]
           const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations`
-          requestOptions.url.should.equal(expectedUrl)
+          expect(this.fetchNothing.lastCall.args[0]).to.equal(expectedUrl)
           requestOptions.method.should.equal('POST')
-          requestOptions.maxAttempts.should.equal(0)
 
-          const { body } = requestOptions
-          Object.keys(body).length.should.equal(7)
-          body.email.should.equal(this.newEmail)
-          body.university.should.equal(affiliationOptions.university)
-          body.department.should.equal(affiliationOptions.department)
-          body.role.should.equal(affiliationOptions.role)
-          body.confirmedAt.should.equal(affiliationOptions.confirmedAt)
-          body.entitlement.should.equal(affiliationOptions.entitlement)
+          const { json } = requestOptions
+          Object.keys(json).length.should.equal(7)
+          expect(json).to.deep.equal(
+            Object.assign(
+              { email: this.newEmail, rejectIfBlocklisted: undefined },
+              affiliationOptions
+            )
+          )
           this.markAsReadIpMatcher.calledOnce.should.equal(true)
           done()
         }
       )
     })
 
-    it('handle error', function (done) {
-      const body = { errors: 'affiliation error message' }
-      this.request.callsArgWith(1, null, { statusCode: 422 }, body)
+    it('handles 422 error', function (done) {
+      const messageFromApi = 'affiliation error message'
+      const body = JSON.stringify({ errors: messageFromApi })
+      this.fetchNothing.throws({ response: { status: 422 }, body })
       this.InstitutionsAPI.addAffiliation(
         this.stubbedUser._id,
         this.newEmail,
         {},
         err => {
           expect(err).to.exist
+          expect(err).to.be.instanceOf(Errors.InvalidInstitutionalEmailError)
           err.message.should.have.string(422)
-          err.message.should.have.string(body.errors)
+          err.message.should.have.string(messageFromApi)
+          done()
+        }
+      )
+    })
+
+    it('handles 500 error', function (done) {
+      const body = { errors: 'affiliation error message' }
+      this.fetchNothing.throws({ response: { status: 500 }, body })
+      this.InstitutionsAPI.addAffiliation(
+        this.stubbedUser._id,
+        this.newEmail,
+        {},
+        err => {
+          expect(err).to.be.instanceOf(Errors.V1ConnectionError)
+          expect(err.message).to.equal('error getting affiliations from v1')
+          expect(err.info).to.deep.equal({ status: 500, body })
+          done()
+        }
+      )
+    })
+
+    it('uses default error message when no error body in response', function (done) {
+      this.fetchNothing.throws({ response: { status: 429 } })
+      this.InstitutionsAPI.addAffiliation(
+        this.stubbedUser._id,
+        this.newEmail,
+        {},
+        err => {
+          expect(err).to.exist
+          expect(err.message).to.equal("Couldn't create affiliation: 429")
+          done()
+        }
+      )
+    })
+
+    it('does not try to mark IP matcher notifications as read if no university passed', function (done) {
+      const affiliationOptions = {
+        confirmedAt: new Date(),
+      }
+
+      this.InstitutionsAPI.addAffiliation(
+        this.stubbedUser._id,
+        this.newEmail,
+        affiliationOptions,
+        err => {
+          expect(err).not.to.exist
+          expect(this.markAsReadIpMatcher.callCount).to.equal(0)
           done()
         }
       )
@@ -246,7 +303,7 @@ describe('InstitutionsAPI', function () {
 
   describe('removeAffiliation', function () {
     beforeEach(function () {
-      this.request.callsArgWith(1, null, { statusCode: 404 })
+      this.fetchNothing.throws({ response: { status: 404 } })
     })
 
     it('remove affiliation', function (done) {
@@ -255,19 +312,19 @@ describe('InstitutionsAPI', function () {
         this.newEmail,
         err => {
           expect(err).not.to.exist
-          this.request.calledOnce.should.equal(true)
-          const requestOptions = this.request.lastCall.args[0]
+          this.fetchNothing.calledOnce.should.equal(true)
+          const requestOptions = this.fetchNothing.lastCall.args[1]
           const expectedUrl = `v1.url/api/v2/users/${this.stubbedUser._id}/affiliations/remove`
-          requestOptions.url.should.equal(expectedUrl)
+          this.fetchNothing.lastCall.args[0].should.equal(expectedUrl)
           requestOptions.method.should.equal('POST')
-          expect(requestOptions.body).to.deep.equal({ email: this.newEmail })
+          expect(requestOptions.json).to.deep.equal({ email: this.newEmail })
           done()
         }
       )
     })
 
     it('handle error', function (done) {
-      this.request.callsArgWith(1, null, { statusCode: 500 })
+      this.fetchNothing.throws({ response: { status: 500 } })
       this.InstitutionsAPI.removeAffiliation(
         this.stubbedUser._id,
         this.newEmail,