From 387695c2b13200c5248c1ad8a3d18b3d82f66f3c Mon Sep 17 00:00:00 2001 From: Ersun Warncke Date: Tue, 2 Jun 2020 10:02:06 -0400 Subject: [PATCH] send tpds updates for all dropbox users GitOrigin-RevId: b690693b98b0b6288a4c3a734d4ff2d28901a496 --- .../ThirdPartyDataStore/TpdsUpdateSender.js | 110 +++++----- .../web/app/src/Features/User/UserGetter.js | 52 +++-- .../TpdsUpdateSenderTests.js | 198 +++++++++++++----- .../web/test/unit/src/User/UserGetterTests.js | 27 ++- 4 files changed, 261 insertions(+), 126 deletions(-) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js index e22984cc11..643f9c07c9 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateSender.js @@ -8,6 +8,7 @@ const settings = require('settings-sharelatex') const CollaboratorsGetter = require('../Collaborators/CollaboratorsGetter') .promises +const UserGetter = require('../User/UserGetter.js').promises const tpdsUrl = _.get(settings, ['apis', 'thirdPartyDataStore', 'url']) @@ -27,23 +28,22 @@ async function addDoc(options) { async function addEntity(options) { const projectUserIds = await getProjectUsersIds(options.project_id) - if (!projectUserIds.length) { - return - } - const job = { - method: 'post', - headers: { - sl_entity_rev: options.rev, - sl_project_id: options.project_id, - sl_all_user_ids: JSON.stringify(projectUserIds) - }, - uri: buildTpdsUrl(projectUserIds[0], options.project_name, options.path), - title: 'addFile', - streamOrigin: options.streamOrigin - } + for (const userId of projectUserIds) { + const job = { + method: 'post', + headers: { + sl_entity_rev: options.rev, + sl_project_id: options.project_id, + sl_all_user_ids: JSON.stringify([userId]) + }, + uri: buildTpdsUrl(userId, options.project_name, options.path), + title: 'addFile', + streamOrigin: options.streamOrigin + } - return enqueue(options.project_id, 'pipeStreamFrom', job) + await enqueue(userId, 'pipeStreamFrom', job) + } } async function addFile(options) { @@ -79,22 +79,21 @@ async function deleteEntity(options) { metrics.inc('tpds.delete-entity') const projectUserIds = await getProjectUsersIds(options.project_id) - if (!projectUserIds.length) { - return - } - const job = { - method: 'delete', - headers: { - sl_project_id: options.project_id, - sl_all_user_ids: JSON.stringify(projectUserIds) - }, - uri: buildTpdsUrl(projectUserIds[0], options.project_name, options.path), - title: 'deleteEntity', - sl_all_user_ids: JSON.stringify(projectUserIds) - } + for (const userId of projectUserIds) { + const job = { + method: 'delete', + headers: { + sl_project_id: options.project_id, + sl_all_user_ids: JSON.stringify([userId]) + }, + uri: buildTpdsUrl(userId, options.project_name, options.path), + title: 'deleteEntity', + sl_all_user_ids: JSON.stringify([userId]) + } - return enqueue(options.project_id, 'standardHttpRequest', job) + await enqueue(userId, 'standardHttpRequest', job) + } } async function enqueue(group, method, job) { @@ -104,12 +103,13 @@ async function enqueue(group, method, job) { return } try { - await request({ + const response = request({ uri: `${tpdsWorkerUrl}/enqueue/web_to_tpds_http_requests`, json: { group, job, method }, method: 'post', timeout: 5 * 1000 }) + return response } catch (err) { // log error and continue logger.error({ err, group, job, method }, 'error enqueueing tpdsworker job') @@ -119,36 +119,46 @@ async function enqueue(group, method, job) { async function getProjectUsersIds(projectId) { // get list of all user ids with access to project. project owner // will always be the first entry in the list. - // TODO: filter this list to only return users with dropbox linked - return CollaboratorsGetter.getInvitedMemberIds(projectId) + const projectUserIds = await CollaboratorsGetter.getInvitedMemberIds( + projectId + ) + // filter list to only return users with dropbox linked + const users = await UserGetter.getUsers( + { + _id: { $in: projectUserIds } + }, + { + _id: 1 + } + ) + return users.map(user => user._id) } async function moveEntity(options) { metrics.inc('tpds.move-entity') const projectUserIds = await getProjectUsersIds(options.project_id) - if (!projectUserIds.length) { - return - } const { endPath, startPath } = buildMovePaths(options) - const job = { - method: 'put', - title: 'moveEntity', - uri: `${tpdsUrl}/user/${projectUserIds[0]}/entity`, - headers: { - sl_project_id: options.project_id, - sl_entity_rev: options.rev, - sl_all_user_ids: JSON.stringify(projectUserIds) - }, - json: { - user_id: projectUserIds[0], - endPath, - startPath + for (const userId of projectUserIds) { + const job = { + method: 'put', + title: 'moveEntity', + uri: `${tpdsUrl}/user/${userId}/entity`, + headers: { + sl_project_id: options.project_id, + sl_entity_rev: options.rev, + sl_all_user_ids: JSON.stringify([userId]) + }, + json: { + user_id: userId, + endPath, + startPath + } } - } - return enqueue(options.project_id, 'standardHttpRequest', job) + await enqueue(userId, 'standardHttpRequest', job) + } } async function pollDropboxForUser(userId) { diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index afed144c53..518b5a198e 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/services/web/app/src/Features/User/UserGetter.js @@ -11,24 +11,14 @@ const Features = require('../../infrastructure/Features') const UserGetter = { getUser(query, projection, callback) { - if (!query) { - return callback(new Error('no query provided')) - } if (arguments.length === 2) { callback = projection projection = {} } - if (typeof query === 'string') { - try { - query = { _id: ObjectId(query) } - } catch (e) { - return callback(null, null) - } - } else if (query instanceof ObjectId) { - query = { _id: query } - } - - db.users.findOne(query, projection, callback) + normalizeQuery(query, (err, query) => { + if (err) return callback(err) + db.users.findOne(query, projection, callback) + }) }, getUserEmail(userId, callback) { @@ -138,14 +128,14 @@ const UserGetter = { db.users.find(query, projection, callback) }, - getUsers(userIds, projection, callback) { - try { - userIds = userIds.map(u => ObjectId(u.toString())) - } catch (error) { - return callback(error) + getUsers(query, projection, callback) { + if (!query) { + return callback(new Error('no query provided')) } - - db.users.find({ _id: { $in: userIds } }, projection, callback) + normalizeQuery(query, (err, query) => { + if (err) return callback(err) + db.users.find(query, projection, callback) + }) }, // check for duplicate email address. This is also enforced at the DB level @@ -159,6 +149,26 @@ const UserGetter = { } } +function normalizeQuery(query, callback) { + if (!query) { + return callback(new Error('no query provided')) + } + try { + if (typeof query === 'string') { + callback(null, { _id: ObjectId(query) }) + } else if (query instanceof ObjectId) { + callback(null, { _id: query }) + } else if (Array.isArray(query)) { + const userIds = query.map(u => ObjectId(u.toString())) + callback(null, { _id: { $in: userIds } }) + } else { + callback(null, query) + } + } catch (err) { + callback(err, null) + } +} + var decorateFullEmails = ( defaultEmail, emailsData, diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js index d3422a1f1c..f827feaf3d 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js @@ -25,6 +25,9 @@ const filestoreUrl = 'filestore.sharelatex.com' describe('TpdsUpdateSender', function() { beforeEach(function() { + this.fakeUser = { + _id: '12390i' + } this.requestQueuer = function(queue, meth, opts, callback) {} const memberIds = [userId, collaberatorRef, readOnlyRef] this.CollaboratorsGetter = { @@ -47,6 +50,14 @@ describe('TpdsUpdateSender', function() { } } } + const getUsers = sinon.stub().resolves( + memberIds.map(userId => { + return { _id: userId } + }) + ) + this.UserGetter = { + promises: { getUsers } + } this.updateSender = SandboxedModule.require(modulePath, { globals: { console: console @@ -56,6 +67,7 @@ describe('TpdsUpdateSender', function() { 'logger-sharelatex': { log() {} }, 'request-promise-native': this.request, '../Collaborators/CollaboratorsGetter': this.CollaboratorsGetter, + '../User/UserGetter.js': this.UserGetter, 'metrics-sharelatex': { inc() {} } @@ -71,14 +83,14 @@ describe('TpdsUpdateSender', function() { it('should post the message to the tpdsworker', async function() { this.settings.apis.tpdsworker = { url: 'www.tpdsworker.env' } - const group = 'myproject' - const method = 'somemethod' - const job = 'do something' - await this.updateSender.promises.enqueue(group, method, job) + const group0 = 'myproject' + const method0 = 'somemethod0' + const job0 = 'do something' + await this.updateSender.promises.enqueue(group0, method0, job0) const args = this.request.firstCall.args[0] - args.json.group.should.equal(group) - args.json.job.should.equal(job) - args.json.method.should.equal(method) + args.json.group.should.equal(group0) + args.json.job.should.equal(job0) + args.json.method.should.equal(method0) args.uri.should.equal( 'www.tpdsworker.env/enqueue/web_to_tpds_http_requests' ) @@ -101,19 +113,33 @@ describe('TpdsUpdateSender', function() { project_name: projectName }) - const { group, job, method } = this.request.firstCall.args[0].json - group.should.equal(projectId) - method.should.equal('pipeStreamFrom') - job.method.should.equal('post') - job.streamOrigin.should.equal( + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json + group0.should.equal(userId) + method0.should.equal('pipeStreamFrom') + job0.method.should.equal('post') + job0.streamOrigin.should.equal( `${filestoreUrl}/project/${projectId}/file/${fileId}` ) const expectedUrl = `${thirdPartyDataStoreApiUrl}/user/${userId}/entity/${encodeURIComponent( projectName )}${encodeURIComponent(path)}` - job.uri.should.equal(expectedUrl) - job.headers.sl_all_user_ids.should.equal( - JSON.stringify([userId, collaberatorRef, readOnlyRef]) + job0.uri.should.equal(expectedUrl) + job0.headers.sl_all_user_ids.should.equal(JSON.stringify([userId])) + + const { group: group1, job: job1 } = this.request.secondCall.args[0].json + group1.should.equal('collaberator_ref_1_here') + job1.headers.sl_all_user_ids.should.equal( + JSON.stringify(['collaberator_ref_1_here']) + ) + + const { group: group2, job: job2 } = this.request.thirdCall.args[0].json + group2.should.equal('read_only_ref_1_id_here') + job2.headers.sl_all_user_ids.should.equal( + JSON.stringify(['read_only_ref_1_id_here']) ) }) @@ -130,20 +156,34 @@ describe('TpdsUpdateSender', function() { project_name: projectName }) - const { group, job, method } = this.request.firstCall.args[0].json + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json - group.should.equal(projectId) - method.should.equal('pipeStreamFrom') - job.method.should.equal('post') + group0.should.equal(userId) + method0.should.equal('pipeStreamFrom') + job0.method.should.equal('post') const expectedUrl = `${thirdPartyDataStoreApiUrl}/user/${userId}/entity/${encodeURIComponent( projectName )}${encodeURIComponent(path)}` - job.uri.should.equal(expectedUrl) - job.streamOrigin.should.equal( + job0.uri.should.equal(expectedUrl) + job0.streamOrigin.should.equal( `${this.docstoreUrl}/project/${projectId}/doc/${docId}/raw` ) - job.headers.sl_all_user_ids.should.eql( - JSON.stringify([userId, collaberatorRef, readOnlyRef]) + job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId])) + + const { group: group1, job: job1 } = this.request.secondCall.args[0].json + group1.should.equal('collaberator_ref_1_here') + job1.headers.sl_all_user_ids.should.equal( + JSON.stringify(['collaberator_ref_1_here']) + ) + + const { group: group2, job: job2 } = this.request.thirdCall.args[0].json + group2.should.equal('read_only_ref_1_id_here') + job2.headers.sl_all_user_ids.should.equal( + JSON.stringify(['read_only_ref_1_id_here']) ) }) @@ -156,18 +196,32 @@ describe('TpdsUpdateSender', function() { project_name: projectName }) - const { group, job, method } = this.request.firstCall.args[0].json + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json - group.should.equal(projectId) - method.should.equal('standardHttpRequest') - job.method.should.equal('delete') + group0.should.equal(userId) + method0.should.equal('standardHttpRequest') + job0.method.should.equal('delete') const expectedUrl = `${thirdPartyDataStoreApiUrl}/user/${userId}/entity/${encodeURIComponent( projectName )}${encodeURIComponent(path)}` - job.headers.sl_all_user_ids.should.eql( - JSON.stringify([userId, collaberatorRef, readOnlyRef]) + job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId])) + job0.uri.should.equal(expectedUrl) + + const { group: group1, job: job1 } = this.request.secondCall.args[0].json + group1.should.equal('collaberator_ref_1_here') + job1.headers.sl_all_user_ids.should.equal( + JSON.stringify(['collaberator_ref_1_here']) + ) + + const { group: group2, job: job2 } = this.request.thirdCall.args[0].json + group2.should.equal('read_only_ref_1_id_here') + job2.headers.sl_all_user_ids.should.equal( + JSON.stringify(['read_only_ref_1_id_here']) ) - job.uri.should.equal(expectedUrl) }) it('moving entity', async function() { @@ -181,16 +235,32 @@ describe('TpdsUpdateSender', function() { project_name: projectName }) - const { group, job, method } = this.request.firstCall.args[0].json + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json - group.should.equal(projectId) - method.should.equal('standardHttpRequest') - job.method.should.equal('put') - job.uri.should.equal(`${thirdPartyDataStoreApiUrl}/user/${userId}/entity`) - job.json.startPath.should.equal(`/${projectName}/${startPath}`) - job.json.endPath.should.equal(`/${projectName}/${endPath}`) - job.headers.sl_all_user_ids.should.eql( - JSON.stringify([userId, collaberatorRef, readOnlyRef]) + group0.should.equal(userId) + method0.should.equal('standardHttpRequest') + job0.method.should.equal('put') + job0.uri.should.equal( + `${thirdPartyDataStoreApiUrl}/user/${userId}/entity` + ) + job0.json.startPath.should.equal(`/${projectName}/${startPath}`) + job0.json.endPath.should.equal(`/${projectName}/${endPath}`) + job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId])) + + const { group: group1, job: job1 } = this.request.secondCall.args[0].json + group1.should.equal('collaberator_ref_1_here') + job1.headers.sl_all_user_ids.should.equal( + JSON.stringify(['collaberator_ref_1_here']) + ) + + const { group: group2, job: job2 } = this.request.thirdCall.args[0].json + group2.should.equal('read_only_ref_1_id_here') + job2.headers.sl_all_user_ids.should.equal( + JSON.stringify(['read_only_ref_1_id_here']) ) }) @@ -204,30 +274,50 @@ describe('TpdsUpdateSender', function() { newProjectName }) - const { group, job, method } = this.request.firstCall.args[0].json + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json - group.should.equal(projectId) - method.should.equal('standardHttpRequest') - job.method.should.equal('put') - job.uri.should.equal(`${thirdPartyDataStoreApiUrl}/user/${userId}/entity`) - job.json.startPath.should.equal(oldProjectName) - job.json.endPath.should.equal(newProjectName) - job.headers.sl_all_user_ids.should.eql( - JSON.stringify([userId, collaberatorRef, readOnlyRef]) + group0.should.equal(userId) + method0.should.equal('standardHttpRequest') + job0.method.should.equal('put') + job0.uri.should.equal( + `${thirdPartyDataStoreApiUrl}/user/${userId}/entity` + ) + job0.json.startPath.should.equal(oldProjectName) + job0.json.endPath.should.equal(newProjectName) + job0.headers.sl_all_user_ids.should.eql(JSON.stringify([userId])) + + const { group: group1, job: job1 } = this.request.secondCall.args[0].json + group1.should.equal('collaberator_ref_1_here') + job1.headers.sl_all_user_ids.should.equal( + JSON.stringify(['collaberator_ref_1_here']) + ) + + const { group: group2, job: job2 } = this.request.thirdCall.args[0].json + group2.should.equal('read_only_ref_1_id_here') + job2.headers.sl_all_user_ids.should.equal( + JSON.stringify(['read_only_ref_1_id_here']) ) }) it('pollDropboxForUser', async function() { await this.updateSender.promises.pollDropboxForUser(userId) - const { group, job, method } = this.request.firstCall.args[0].json + const { + group: group0, + job: job0, + method: method0 + } = this.request.firstCall.args[0].json - group.should.equal(`poll-dropbox:${userId}`) - method.should.equal('standardHttpRequest') + group0.should.equal(`poll-dropbox:${userId}`) + method0.should.equal('standardHttpRequest') - job.method.should.equal('post') - job.uri.should.equal(`${thirdPartyDataStoreApiUrl}/user/poll`) - job.json.user_ids[0].should.equal(userId) + job0.method.should.equal('post') + job0.uri.should.equal(`${thirdPartyDataStoreApiUrl}/user/poll`) + job0.json.user_ids[0].should.equal(userId) }) }) }) diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index 37b114fc6b..6d11f34f58 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/services/web/test/unit/src/User/UserGetterTests.js @@ -77,8 +77,33 @@ describe('UserGetter', function() { }) it('should not allow null query', function(done) { - this.UserGetter.getUser(null, {}, (error, user) => { + this.UserGetter.getUser(null, {}, error => { error.should.exist + error.message.should.equal('no query provided') + done() + }) + }) + }) + + describe('getUsers', function() { + it('should get users with array of userIds', function(done) { + const query = [new ObjectId()] + const projection = { email: 1 } + this.UserGetter.getUsers(query, projection, (error, users) => { + expect(error).to.not.exist + this.find.should.have.been.calledWithMatch( + { _id: { $in: query } }, + projection + ) + users.should.deep.equal([this.fakeUser]) + done() + }) + }) + + it('should not allow null query', function(done) { + this.UserGetter.getUser(null, {}, error => { + error.should.exist + error.message.should.equal('no query provided') done() }) })