Merge pull request #17329 from overleaf/jel-async-peekValueFromToken

[web] Promisify peekValueFromToken GitOrigin-RevId: 4a7f6ae793ff0a1bd22c89c963881ef0957e29e8
2026-05-25 02:00:10 +02:00 · 2024-03-11 07:28:23 -05:00
parent b2b100d485
commit 7a9c2fd644
4 changed files with 105 additions and 123 deletions
--- a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js
+++ b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js
@@ -47,46 +47,43 @@ function expirePasswordResetToken(token, callback) {
 }

 function getUserForPasswordResetToken(token, callback) {
-  OneTimeTokenHandler.peekValueFromToken(
-    'password',
-    token,
-    (err, data, remainingUses) => {
-      if (err != null) {
-        if (err.name === 'NotFoundError') {
-          return callback(null, null)
-        } else {
-          return callback(err)
-        }
+  OneTimeTokenHandler.peekValueFromToken('password', token, (err, result) => {
+    const { data, remainingPeeks } = result || {}
+    if (err != null) {
+      if (err.name === 'NotFoundError') {
+        return callback(null, null)
+      } else {
+        return callback(err)
      }
-      if (data == null || data.email == null) {
-        return callback(null, null, remainingUses)
-      }
-      UserGetter.getUserByMainEmail(
-        data.email,
-        { _id: 1, 'overleaf.id': 1, email: 1 },
-        (err, user) => {
-          if (err != null) {
-            callback(err)
-          } else if (user == null) {
-            callback(null, null, 0)
-          } else if (
-            data.user_id != null &&
-            data.user_id === user._id.toString()
-          ) {
-            callback(null, user, remainingUses)
-          } else if (
-            data.v1_user_id != null &&
-            user.overleaf != null &&
-            data.v1_user_id === user.overleaf.id
-          ) {
-            callback(null, user, remainingUses)
-          } else {
-            callback(null, null, 0)
-          }
-        }
-      )
    }
-  )
+    if (data == null || data.email == null) {
+      return callback(null, null, remainingPeeks)
+    }
+    UserGetter.getUserByMainEmail(
+      data.email,
+      { _id: 1, 'overleaf.id': 1, email: 1 },
+      (err, user) => {
+        if (err != null) {
+          callback(err)
+        } else if (user == null) {
+          callback(null, null, 0)
+        } else if (
+          data.user_id != null &&
+          data.user_id === user._id.toString()
+        ) {
+          callback(null, user, remainingPeeks)
+        } else if (
+          data.v1_user_id != null &&
+          user.overleaf != null &&
+          data.v1_user_id === user.overleaf.id
+        ) {
+          callback(null, user, remainingPeeks)
+        } else {
+          callback(null, null, 0)
+        }
+      }
+    )
+  })
 }

 async function setNewUserPassword(token, password, auditLog) {
--- a/services/web/app/src/Features/Security/OneTimeTokenHandler.js
+++ b/services/web/app/src/Features/Security/OneTimeTokenHandler.js
@@ -2,9 +2,38 @@ const crypto = require('crypto')
 const { db } = require('../../infrastructure/mongodb')
 const Errors = require('../Errors/Errors')
 const { promisifyAll } = require('@overleaf/promise-utils')
+const { callbackify } = require('util')

 const ONE_HOUR_IN_S = 60 * 60

+async function peekValueFromToken(use, token) {
+  const result = await db.tokens.findOneAndUpdate(
+    {
+      use,
+      token,
+      expiresAt: { $gt: new Date() },
+      usedAt: { $exists: false },
+      peekCount: { $not: { $gte: OneTimeTokenHandler.MAX_PEEKS } },
+    },
+    {
+      $inc: { peekCount: 1 },
+    },
+    {
+      returnDocument: 'after',
+    }
+  )
+
+  const tokenDoc = result.value
+  if (!tokenDoc) {
+    throw new Errors.NotFoundError('no token found')
+  }
+  // The allowed number of peaks will be 1 less than OneTimeTokenHandler.MAX_PEEKS
+  // since the updated doc is returned after findOneAndUpdate above
+  const remainingPeeks = OneTimeTokenHandler.MAX_PEEKS - tokenDoc.peekCount
+
+  return { data: tokenDoc.data, remainingPeeks }
+}
+
 const OneTimeTokenHandler = {
  MAX_PEEKS: 4,

@@ -66,34 +95,7 @@ const OneTimeTokenHandler = {
    )
  },

-  peekValueFromToken(use, token, callback) {
-    db.tokens.findOneAndUpdate(
-      {
-        use,
-        token,
-        expiresAt: { $gt: new Date() },
-        usedAt: { $exists: false },
-        peekCount: { $not: { $gte: OneTimeTokenHandler.MAX_PEEKS } },
-      },
-      {
-        $inc: { peekCount: 1 },
-      },
-      {
-        returnDocument: 'after',
-      },
-      function (error, result) {
-        if (error) {
-          return callback(error)
-        }
-        const token = result.value
-        if (!token) {
-          return callback(new Errors.NotFoundError('no token found'))
-        }
-        const remainingPeeks = OneTimeTokenHandler.MAX_PEEKS - token.peekCount
-        callback(null, token.data, remainingPeeks)
-      }
-    )
-  },
+  peekValueFromToken: callbackify(peekValueFromToken),

  expireToken(use, token, callback) {
    const now = new Date()
--- a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js
+++ b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js
@@ -220,8 +220,10 @@ describe('PasswordResetHandler', function () {
        this.OneTimeTokenHandler.peekValueFromToken
          .withArgs('password', this.token)
          .yields(null, {
-            user_id: this.user._id,
-            email: this.email,
+            data: {
+              user_id: this.user._id,
+              email: this.email,
+            },
          })
        this.AuthenticationManager.promises.setUserPassword
          .withArgs(this.user, this.password)
@@ -424,8 +426,10 @@ describe('PasswordResetHandler', function () {
        this.OneTimeTokenHandler.peekValueFromToken
          .withArgs('password', this.token)
          .yields(null, {
-            v1_user_id: this.user.overleaf.id,
-            email: this.email,
+            data: {
+              v1_user_id: this.user.overleaf.id,
+              email: this.email,
+            },
          })
        this.AuthenticationManager.promises.setUserPassword
          .withArgs(this.user, this.password)
--- a/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js
+++ b/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js
@@ -1,33 +1,20 @@
-/* eslint-disable
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const SandboxedModule = require('sandboxed-module')
-const assert = require('assert')
 const path = require('path')
 const sinon = require('sinon')
 const modulePath = path.join(
  __dirname,
  '../../../../app/src/Features/Security/OneTimeTokenHandler'
 )
-const { expect } = require('chai')
 const Errors = require('../../../../app/src/Features/Errors/Errors')
 const tk = require('timekeeper')
+const { expect } = require('chai')

 describe('OneTimeTokenHandler', function () {
  beforeEach(function () {
    tk.freeze(Date.now()) // freeze the time for these tests
    this.stubbedToken = 'mock-token'
    this.callback = sinon.stub()
-    return (this.OneTimeTokenHandler = SandboxedModule.require(modulePath, {
+    this.OneTimeTokenHandler = SandboxedModule.require(modulePath, {
      requires: {
        '@overleaf/settings': this.settings,
        crypto: {
@@ -37,21 +24,21 @@ describe('OneTimeTokenHandler', function () {
          db: (this.db = { tokens: {} }),
        },
      },
-    }))
+    })
  })

  afterEach(function () {
-    return tk.reset()
+    tk.reset()
  })

  describe('getNewToken', function () {
    beforeEach(function () {
-      return (this.db.tokens.insertOne = sinon.stub().yields())
+      this.db.tokens.insertOne = sinon.stub().yields()
    })

    describe('normally', function () {
      beforeEach(function () {
-        return this.OneTimeTokenHandler.getNewToken(
+        this.OneTimeTokenHandler.getNewToken(
          'password',
          'mock-data-to-store',
          this.callback
@@ -59,7 +46,7 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should insert a generated token with a 1 hour expiry', function () {
-        return this.db.tokens.insertOne
+        this.db.tokens.insertOne
          .calledWith({
            use: 'password',
            token: this.stubbedToken,
@@ -71,15 +58,13 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should call the callback with the token', function () {
-        return this.callback
-          .calledWith(null, this.stubbedToken)
-          .should.equal(true)
+        this.callback.calledWith(null, this.stubbedToken).should.equal(true)
      })
    })

    describe('with an optional expiresIn parameter', function () {
      beforeEach(function () {
-        return this.OneTimeTokenHandler.getNewToken(
+        this.OneTimeTokenHandler.getNewToken(
          'password',
          'mock-data-to-store',
          { expiresIn: 42 },
@@ -88,7 +73,7 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should insert a generated token with a custom expiry', function () {
-        return this.db.tokens.insertOne
+        this.db.tokens.insertOne
          .calledWith({
            use: 'password',
            token: this.stubbedToken,
@@ -100,29 +85,27 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should call the callback with the token', function () {
-        return this.callback
-          .calledWith(null, this.stubbedToken)
-          .should.equal(true)
+        this.callback.calledWith(null, this.stubbedToken).should.equal(true)
      })
    })
  })

  describe('peekValueFromToken', function () {
    describe('successfully', function () {
-      const data = 'some-mock-data'
-      beforeEach(function () {
+      const data = { email: 'some-mock-data' }
+      let result
+      beforeEach(async function () {
        this.db.tokens.findOneAndUpdate = sinon
          .stub()
-          .yields(null, { value: { data } })
-        return this.OneTimeTokenHandler.peekValueFromToken(
+          .resolves({ value: { data, peekCount: 1 } })
+        result = await this.OneTimeTokenHandler.promises.peekValueFromToken(
          'password',
-          'mock-token',
-          this.callback
+          'mock-token'
        )
      })

      it('should increment the peekCount', function () {
-        return this.db.tokens.findOneAndUpdate
+        this.db.tokens.findOneAndUpdate
          .calledWith(
            {
              use: 'password',
@@ -139,26 +122,22 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should return the data', function () {
-        return this.callback.calledWith(null, data).should.equal(true)
+        expect(result).to.deep.equal({ data, remainingPeeks: 3 })
      })
    })

    describe('when a valid token is not found', function () {
      beforeEach(function () {
-        this.db.tokens.findOneAndUpdate = sinon
-          .stub()
-          .yields(null, { value: null })
-        return this.OneTimeTokenHandler.peekValueFromToken(
-          'password',
-          'mock-token',
-          this.callback
-        )
+        this.db.tokens.findOneAndUpdate = sinon.stub().resolves({ value: null })
      })

-      it('should return a NotFoundError', function () {
-        return this.callback
-          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
-          .should.equal(true)
+      it('should return a NotFoundError', async function () {
+        await expect(
+          this.OneTimeTokenHandler.promises.peekValueFromToken(
+            'password',
+            'mock-token'
+          )
+        ).to.be.rejectedWith(Errors.NotFoundError)
      })
    })
  })
@@ -197,7 +176,7 @@ describe('OneTimeTokenHandler', function () {
        this.db.tokens.findOneAndUpdate = sinon
          .stub()
          .yields(null, { value: { data: 'mock-data' } })
-        return this.OneTimeTokenHandler.getValueFromTokenAndExpire(
+        this.OneTimeTokenHandler.getValueFromTokenAndExpire(
          'password',
          'mock-token',
          this.callback
@@ -205,7 +184,7 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should expire the token', function () {
-        return this.db.tokens.findOneAndUpdate
+        this.db.tokens.findOneAndUpdate
          .calledWith(
            {
              use: 'password',
@@ -222,7 +201,7 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should return the data', function () {
-        return this.callback.calledWith(null, 'mock-data').should.equal(true)
+        this.callback.calledWith(null, 'mock-data').should.equal(true)
      })
    })

@@ -231,7 +210,7 @@ describe('OneTimeTokenHandler', function () {
        this.db.tokens.findOneAndUpdate = sinon
          .stub()
          .yields(null, { value: null })
-        return this.OneTimeTokenHandler.getValueFromTokenAndExpire(
+        this.OneTimeTokenHandler.getValueFromTokenAndExpire(
          'password',
          'mock-token',
          this.callback
@@ -239,7 +218,7 @@ describe('OneTimeTokenHandler', function () {
      })

      it('should return a NotFoundError', function () {
-        return this.callback
+        this.callback
          .calledWith(sinon.match.instanceOf(Errors.NotFoundError))
          .should.equal(true)
      })