From 9601fd097a402e27ac8969381a1b79fbd5b31664 Mon Sep 17 00:00:00 2001 From: roo hutton Date: Mon, 22 Apr 2024 08:08:39 +0100 Subject: [PATCH] Merge pull request #17946 from overleaf/rh-promisify-third-party-identity- [web] Promisify ThirdPartyIdentityManager and ThirdPartyIdentityManagerTests GitOrigin-RevId: f7d24f73213fb0a43eb453aa21749b21ba60b83d --- .../User/ThirdPartyIdentityManager.js | 258 ++++++++---------- .../User/ThirdPartyIdentityManagerTests.js | 182 ++++++------ 2 files changed, 211 insertions(+), 229 deletions(-) diff --git a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js index ac6d52b559..0c98bd6bb9 100644 --- a/services/web/app/src/Features/User/ThirdPartyIdentityManager.js +++ b/services/web/app/src/Features/User/ThirdPartyIdentityManager.js @@ -4,148 +4,128 @@ const EmailOptionsHelper = require('../../../../app/src/Features/Email/EmailOpti const Errors = require('../Errors/Errors') const _ = require('lodash') const logger = require('@overleaf/logger') -const OError = require('@overleaf/o-error') const settings = require('@overleaf/settings') const { User } = require('../../../../app/src/models/User') -const { promisifyAll } = require('@overleaf/promise-utils') +const { callbackify } = require('@overleaf/promise-utils') +const OError = require('@overleaf/o-error') const oauthProviders = settings.oauthProviders || {} -function getUser(providerId, externalUserId, callback) { +async function getUser(providerId, externalUserId) { if (providerId == null || externalUserId == null) { - return callback( - new OError('invalid SSO arguments', { - externalUserId, - providerId, - }) - ) + throw new OError('invalid SSO arguments', { + externalUserId, + providerId, + }) + } + + const query = _getUserQuery(providerId, externalUserId) + const user = await User.findOne(query).exec() + if (!user) { + throw new Errors.ThirdPartyUserNotFoundError() + } + return user +} + +async function login(providerId, externalUserId, externalData) { + const user = await ThirdPartyIdentityManager.promises.getUser( + providerId, + externalUserId + ) + if (!externalData) { + return user } const query = _getUserQuery(providerId, externalUserId) - User.findOne(query, function (err, user) { - if (err != null) { - return callback(err) - } - if (!user) { - return callback(new Errors.ThirdPartyUserNotFoundError()) - } - callback(null, user) - }) -} - -function login(providerId, externalUserId, externalData, callback) { - ThirdPartyIdentityManager.getUser( + const update = _thirdPartyIdentifierUpdate( + user, providerId, externalUserId, - function (err, user) { - if (err != null) { - return callback(err) - } - if (!externalData) { - return callback(null, user) - } - const query = _getUserQuery(providerId, externalUserId) - const update = _thirdPartyIdentifierUpdate( - user, - providerId, - externalUserId, - externalData - ) - User.findOneAndUpdate(query, update, { new: true }, callback) - } + externalData ) + return await User.findOneAndUpdate(query, update, { new: true }).exec() } -function link( +async function link( userId, providerId, externalUserId, externalData, auditLog, - callback, retry ) { const accountLinked = true if (!oauthProviders[providerId]) { - return callback(new Error('Not a valid provider')) + throw new Error('Not a valid provider') } - UserAuditLogHandler.addEntry( + await UserAuditLogHandler.promises.addEntry( userId, 'link-sso', auditLog.initiatorId, auditLog.ipAddress, { providerId, + } + ) + + const query = { + _id: userId, + 'thirdPartyIdentifiers.providerId': { + $ne: providerId, }, - error => { - if (error) { - return callback(error) - } - const query = { - _id: userId, - 'thirdPartyIdentifiers.providerId': { - $ne: providerId, - }, - } - const update = { - $push: { - thirdPartyIdentifiers: { - externalUserId, - externalData, - providerId, - }, - }, - } - // add new tpi only if an entry for the provider does not exist - // projection includes thirdPartyIdentifiers for tests - User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { - if (err && err.code === 11000) { - callback( - new Errors.ThirdPartyIdentityExistsError({ - info: { externalUserId }, - }) - ) - } else if (err != null) { - callback(err) - } else if (res) { - _sendSecurityAlert(accountLinked, providerId, res, userId) - callback(null, res) - } else if (retry) { - // if already retried then throw error - callback(new Error('update failed')) - } else { - // attempt to clear existing entry then retry - ThirdPartyIdentityManager.unlink( - userId, - providerId, - auditLog, - function (err) { - if (err != null) { - return callback(err) - } - ThirdPartyIdentityManager.link( - userId, - providerId, - externalUserId, - externalData, - auditLog, - callback, - true - ) - } - ) - } + } + const update = { + $push: { + thirdPartyIdentifiers: { + externalUserId, + externalData, + providerId, + }, + }, + } + // add new tpi only if an entry for the provider does not exist + // projection includes thirdPartyIdentifiers for tests + let res + try { + res = await User.findOneAndUpdate(query, update, { new: 1 }).exec() + } catch (err) { + if (err.code === 11000) { + throw new Errors.ThirdPartyIdentityExistsError({ + info: { externalUserId }, }) } + throw err + } + + if (res) { + _sendSecurityAlert(accountLinked, providerId, res, userId) + return res + } + + if (retry) { + // if already retried then throw error + throw new Error('update failed') + } + + // attempt to clear existing entry then retry + await ThirdPartyIdentityManager.promises.unlink(userId, providerId, auditLog) + return await ThirdPartyIdentityManager.promises.link( + userId, + providerId, + externalUserId, + externalData, + auditLog, + true ) } -function unlink(userId, providerId, auditLog, callback) { +async function unlink(userId, providerId, auditLog) { const accountLinked = false if (!oauthProviders[providerId]) { - return callback(new Error('Not a valid provider')) + throw new Error('Not a valid provider') } - UserAuditLogHandler.addEntry( + + await UserAuditLogHandler.promises.addEntry( userId, 'unlink-sso', auditLog.initiatorId, @@ -153,35 +133,26 @@ function unlink(userId, providerId, auditLog, callback) { { ...(auditLog.extraInfo || {}), providerId, - }, - error => { - if (error) { - return callback(error) - } - const query = { - _id: userId, - } - const update = { - $pull: { - thirdPartyIdentifiers: { - providerId, - }, - }, - } - // projection includes thirdPartyIdentifiers for tests - User.findOneAndUpdate(query, update, { new: 1 }, (err, res) => { - if (err != null) { - callback(err) - } else if (!res) { - callback(new Error('update failed')) - } else { - // no need to wait, errors are logged and not passed back - _sendSecurityAlert(accountLinked, providerId, res, userId) - callback(null, res) - } - }) } ) + + const query = { + _id: userId, + } + const update = { + $pull: { + thirdPartyIdentifiers: { + providerId, + }, + }, + } + // projection includes thirdPartyIdentifiers for tests + const res = await User.findOneAndUpdate(query, update, { new: 1 }) + if (!res) { + throw new Error('update failed') + } + _sendSecurityAlert(accountLinked, providerId, res, userId) + return res } function _getUserQuery(providerId, externalUserId) { @@ -194,21 +165,21 @@ function _getUserQuery(providerId, externalUserId) { return query } -function _sendSecurityAlert(accountLinked, providerId, user, userId) { +async function _sendSecurityAlert(accountLinked, providerId, user, userId) { const providerName = oauthProviders[providerId].name const emailOptions = EmailOptionsHelper.linkOrUnlink( accountLinked, providerName, user.email ) - EmailHandler.sendEmail('securityAlert', emailOptions, error => { - if (error) { - logger.error( - { err: error, userId }, - `could not send security alert email when ${emailOptions.action.toLowerCase()}` - ) - } - }) + try { + await EmailHandler.promises.sendEmail('securityAlert', emailOptions) + } catch (error) { + logger.error( + { err: error, userId }, + `could not send security alert email when ${emailOptions.action.toLowerCase()}` + ) + } } function _thirdPartyIdentifierUpdate( @@ -230,12 +201,17 @@ function _thirdPartyIdentifierUpdate( } const ThirdPartyIdentityManager = { + getUser: callbackify(getUser), + login: callbackify(login), + link: callbackify(link), + unlink: callbackify(unlink), +} + +ThirdPartyIdentityManager.promises = { getUser, login, link, unlink, } -ThirdPartyIdentityManager.promises = promisifyAll(ThirdPartyIdentityManager) - module.exports = ThirdPartyIdentityManager diff --git a/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js index faac674b14..de67251c8b 100644 --- a/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js +++ b/services/web/test/unit/src/User/ThirdPartyIdentityManagerTests.js @@ -1,6 +1,7 @@ const sinon = require('sinon') const { expect } = require('chai') const SandboxedModule = require('sandboxed-module') +const OError = require('@overleaf/o-error') const modulePath = '../../../../app/src/Features/User/ThirdPartyIdentityManager.js' @@ -18,16 +19,24 @@ describe('ThirdPartyIdentityManager', function () { requires: { '../../../../app/src/Features/User/UserAuditLogHandler': (this.UserAuditLogHandler = { - addEntry: sinon.stub().yields(), + promises: { + addEntry: sinon.stub().resolves(), + }, }), '../../../../app/src/Features/Email/EmailHandler': (this.EmailHandler = { - sendEmail: sinon.stub().yields(), + promises: { + sendEmail: sinon.stub().resolves(), + }, }), '../../../../app/src/models/User': { User: (this.User = { - findOneAndUpdate: sinon.stub().yields(undefined, this.user), - findOne: sinon.stub().yields(undefined, undefined), + findOneAndUpdate: sinon + .stub() + .returns({ exec: sinon.stub().resolves(this.user) }), + findOne: sinon.stub().returns({ + exec: sinon.stub().resolves(undefined), + }), }), }, '@overleaf/settings': { @@ -44,28 +53,23 @@ describe('ThirdPartyIdentityManager', function () { }) }) describe('getUser', function () { - it('should an error when missing providerId or externalUserId', function (done) { - this.ThirdPartyIdentityManager.getUser( - undefined, - undefined, - (error, user) => { - expect(error).to.exist - expect(error.message).to.equal('invalid SSO arguments') - expect(error.info).to.deep.equal({ - externalUserId: undefined, - providerId: undefined, - }) - done() - } - ) + it('should throw an error when missing providerId or externalUserId', async function () { + await expect( + this.ThirdPartyIdentityManager.promises.getUser(undefined, undefined) + ).to.be.rejectedWith(OError, `invalid SSO arguments`) }) + describe('when user linked', function () { beforeEach(function () { - this.User.findOne.yields(undefined, this.user) + this.User.findOne.returns({ + exec: sinon.stub().resolves(this.user), + }) }) it('should return the user', async function () { - this.User.findOne.returns(undefined, this.user) + this.User.findOne.returns({ + exec: sinon.stub().resolves(this.user), + }) const user = await this.ThirdPartyIdentityManager.promises.getUser( 'google', 'an-id-linked' @@ -94,7 +98,7 @@ describe('ThirdPartyIdentityManager', function () { this.externalData, this.auditLog ) - const emailCall = this.EmailHandler.sendEmail.getCall(0) + const emailCall = this.EmailHandler.promises.sendEmail.getCall(0) expect(emailCall.args[0]).to.equal('securityAlert') expect(emailCall.args[1].actionDescribed).to.contain( 'a Google account was linked' @@ -109,7 +113,9 @@ describe('ThirdPartyIdentityManager', function () { this.externalData, this.auditLog ) - expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith( + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledOnceWith( this.userId, 'link-sso', this.auditLog.initiatorId, @@ -121,49 +127,47 @@ describe('ThirdPartyIdentityManager', function () { }) describe('errors', function () { const anError = new Error('oops') - it('should not unlink if the UserAuditLogHandler throws an error', function (done) { - this.UserAuditLogHandler.addEntry.yields(anError) - this.ThirdPartyIdentityManager.link( - this.userId, - 'google', - this.externalUserId, - this.externalData, - this.auditLog, - error => { - expect(error).to.exist - expect(error).to.equal(anError) - expect(this.User.findOneAndUpdate).to.not.have.been.called - done() - } - ) - }) - describe('EmailHandler', function () { - beforeEach(function () { - this.EmailHandler.sendEmail.yields(anError) - }) - it('should log but not return the error', function (done) { - this.ThirdPartyIdentityManager.link( + + it('should not unlink if the UserAuditLogHandler throws an error', async function () { + this.UserAuditLogHandler.promises.addEntry.throws(anError) + await expect( + this.ThirdPartyIdentityManager.promises.link( this.userId, 'google', this.externalUserId, this.externalData, - this.auditLog, - error => { - expect(error).to.not.exist - expect(this.logger.error.lastCall).to.be.calledWithExactly( - { - err: anError, - userId: this.userId, - }, - 'could not send security alert email when new account linked' - ) - done() - } + this.auditLog + ) + ).to.be.rejectedWith(anError) + expect(this.User.findOneAndUpdate).to.not.have.been.called + }) + + describe('EmailHandler', function () { + beforeEach(function () { + this.EmailHandler.promises.sendEmail.throws(anError) + }) + it('should log but not return the error', async function () { + await expect( + this.ThirdPartyIdentityManager.promises.link( + this.userId, + 'google', + this.externalUserId, + this.externalData, + this.auditLog + ) + ).to.be.fulfilled + expect(this.logger.error.lastCall).to.be.calledWithExactly( + { + err: anError, + userId: this.userId, + }, + 'could not send security alert email when new account linked' ) }) }) }) }) + describe('unlink', function () { it('should send email alert', async function () { await this.ThirdPartyIdentityManager.promises.unlink( @@ -171,7 +175,7 @@ describe('ThirdPartyIdentityManager', function () { 'orcid', this.auditLog ) - const emailCall = this.EmailHandler.sendEmail.getCall(0) + const emailCall = this.EmailHandler.promises.sendEmail.getCall(0) expect(emailCall.args[0]).to.equal('securityAlert') expect(emailCall.args[1].actionDescribed).to.contain( 'an ORCID account was unlinked from' @@ -183,7 +187,9 @@ describe('ThirdPartyIdentityManager', function () { 'orcid', this.auditLog ) - expect(this.UserAuditLogHandler.addEntry).to.have.been.calledOnceWith( + expect( + this.UserAuditLogHandler.promises.addEntry + ).to.have.been.calledOnceWith( this.userId, 'unlink-sso', this.auditLog.initiatorId, @@ -193,43 +199,43 @@ describe('ThirdPartyIdentityManager', function () { } ) }) + describe('errors', function () { const anError = new Error('oops') - it('should not unlink if the UserAuditLogHandler throws an error', function (done) { - this.UserAuditLogHandler.addEntry.yields(anError) - this.ThirdPartyIdentityManager.unlink( - this.userId, - 'orcid', - this.auditLog, - error => { - expect(error).to.exist - expect(error).to.equal(anError) - expect(this.User.findOneAndUpdate).to.not.have.been.called - done() - } - ) + + it('should not unlink if the UserAuditLogHandler throws an error', async function () { + this.UserAuditLogHandler.promises.addEntry.throws(anError) + + await expect( + this.ThirdPartyIdentityManager.promises.unlink( + this.userId, + 'orcid', + this.auditLog + ) + ).to.be.rejectedWith(anError) + expect(this.User.findOneAndUpdate).to.not.have.been.called }) + describe('EmailHandler', function () { beforeEach(function () { - this.EmailHandler.sendEmail.yields(anError) + this.EmailHandler.promises.sendEmail.throws(anError) }) - it('should log but not return the error', function (done) { - this.ThirdPartyIdentityManager.unlink( - this.userId, - 'google', - this.auditLog, - error => { - expect(error).to.not.exist - expect(this.logger.error.lastCall).to.be.calledWithExactly( - { - err: anError, - userId: this.userId, - }, - 'could not send security alert email when account no longer linked' - ) - done() - } + it('should log but not return the error', async function () { + await expect( + this.ThirdPartyIdentityManager.promises.unlink( + this.userId, + 'google', + this.auditLog + ) + ).to.be.fulfilled + + expect(this.logger.error.lastCall).to.be.calledWithExactly( + { + err: anError, + userId: this.userId, + }, + 'could not send security alert email when account no longer linked' ) }) })