diff --git a/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js b/services/web/app/src/Features/PasswordReset/PasswordResetHandler.js index a532704752..669e955247 100644 --- 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) { diff --git a/services/web/app/src/Features/Security/OneTimeTokenHandler.js b/services/web/app/src/Features/Security/OneTimeTokenHandler.js index e9112ae529..29e3f279ff 100644 --- 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() diff --git a/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js b/services/web/test/unit/src/PasswordReset/PasswordResetHandlerTests.js index b13483569a..8e1b84838e 100644 --- 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) diff --git a/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js b/services/web/test/unit/src/Security/OneTimeTokenHandlerTests.js index 2f6df14f45..f9cc1c7aeb 100644 --- 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) })