From b1f754429e33dbcbab443b94d2b13d9320aa8ff9 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 5 Oct 2022 10:26:52 +0100 Subject: [PATCH] Merge pull request #9316 from overleaf/jpa-dropbox-sync-detached-from-project-name [misc] detach dropbox sync from project names GitOrigin-RevId: 57b3a131aec81bc97ff4da57497950d6658eaeff --- .../ThirdPartyDataStore/TpdsController.js | 24 ++++-- .../ThirdPartyDataStore/TpdsUpdateHandler.js | 58 +++++++++++--- services/web/app/src/router.js | 10 +++ .../TpdsControllerTests.js | 75 ++++++++++++++++-- .../TpdsUpdateHandlerTests.js | 76 +++++++++++++++++++ 5 files changed, 219 insertions(+), 24 deletions(-) diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js index cd9b2728e8..d68ce91553 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsController.js @@ -17,13 +17,14 @@ const TpdsQueueManager = require('./TpdsQueueManager') async function mergeUpdate(req, res) { metrics.inc('tpds.merge-update') - const { filePath, userId, projectName } = parseParams(req) + const { filePath, userId, projectId, projectName } = parseParams(req) const source = req.headers['x-sl-update-source'] || 'unknown' let metadata try { metadata = await TpdsUpdateHandler.promises.newUpdate( userId, + projectId, projectName, filePath, req, @@ -70,11 +71,12 @@ async function mergeUpdate(req, res) { async function deleteUpdate(req, res) { metrics.inc('tpds.delete-update') - const { filePath, userId, projectName } = parseParams(req) + const { filePath, userId, projectId, projectName } = parseParams(req) const source = req.headers['x-sl-update-source'] || 'unknown' await TpdsUpdateHandler.promises.deleteUpdate( userId, + projectId, projectName, filePath, source @@ -87,9 +89,11 @@ async function deleteUpdate(req, res) { */ async function updateFolder(req, res) { const userId = req.body.userId - const { projectName, filePath } = splitPath(req.body.path) + const projectId = req.body.projectId + const { projectName, filePath } = splitPath(projectId, req.body.path) const metadata = await TpdsUpdateHandler.promises.createFolder( userId, + projectId, projectName, filePath ) @@ -147,14 +151,18 @@ async function getQueues(req, res, next) { function parseParams(req) { const userId = req.params.user_id - const { projectName, filePath } = splitPath(req.params[0]) - return { filePath, userId, projectName } + const projectId = req.params.project_id + const { projectName, filePath } = splitPath(projectId, req.params[0]) + return { filePath, userId, projectName, projectId } } -function splitPath(path) { +function splitPath(projectId, path) { let filePath, projectName path = Path.join('/', path) - if (path.substring(1).indexOf('/') === -1) { + if (projectId) { + filePath = path + projectName = '' + } else if (path.substring(1).indexOf('/') === -1) { filePath = '/' projectName = path.substring(1) } else { @@ -163,7 +171,7 @@ function splitPath(path) { projectName = projectName.replace('/', '') } - return { projectName, filePath } + return { filePath, projectName } } module.exports = { diff --git a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js index 7da0e79325..91936bd6b4 100644 --- a/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js +++ b/services/web/app/src/Features/ThirdPartyDataStore/TpdsUpdateHandler.js @@ -19,8 +19,15 @@ const ROOT_DOC_TIMEOUT_LENGTH = 30 * 1000 const rootDocResets = new BackgroundTaskTracker('root doc resets') -async function newUpdate(userId, projectName, path, updateRequest, source) { - const project = await getOrCreateProject(userId, projectName) +async function newUpdate( + userId, + projectId, + projectName, + path, + updateRequest, + source +) { + const project = await getOrCreateProject(userId, projectId, projectName) if (project == null) { return null } @@ -46,13 +53,20 @@ async function newUpdate(userId, projectName, path, updateRequest, source) { return metadata } -async function deleteUpdate(userId, projectName, path, source) { +async function deleteUpdate(userId, projectId, projectName, path, source) { logger.debug({ userId, filePath: path }, 'handling delete update from tpds') - - const projects = await ProjectGetter.promises.findUsersProjectsByName( - userId, - projectName - ) + let projects = [] + if (projectId) { + const project = await findProjectByIdWithRWAccess(userId, projectId) + if (project) { + projects = [project] + } + } else { + projects = await ProjectGetter.promises.findUsersProjectsByName( + userId, + projectName + ) + } const activeProjects = projects.filter( project => !ProjectHelper.isArchivedOrTrashed(project, userId) ) @@ -84,7 +98,29 @@ async function deleteUpdate(userId, projectName, path, source) { } } -async function getOrCreateProject(userId, projectName) { +async function getOrCreateProject(userId, projectId, projectName) { + if (projectId) { + return findProjectByIdWithRWAccess(userId, projectId) + } else { + return getOrCreateProjectByName(userId, projectName) + } +} + +async function findProjectByIdWithRWAccess(userId, projectId) { + const allProjects = await ProjectGetter.promises.findAllUsersProjects( + userId, + 'name archived trashed' + ) + for (const projects of [allProjects.owned, allProjects.readAndWrite]) { + for (const project of projects) { + if (project._id.toString() === projectId) { + return project + } + } + } +} + +async function getOrCreateProjectByName(userId, projectName) { const projects = await ProjectGetter.promises.findUsersProjectsByName( userId, projectName @@ -144,8 +180,8 @@ async function handleDuplicateProjects(userId, projectName) { .create(projectName) } -async function createFolder(userId, projectName, path) { - const project = await getOrCreateProject(userId, projectName) +async function createFolder(userId, projectId, projectName, path) { + const project = await getOrCreateProject(userId, projectId, projectName) if (project == null) { return null } diff --git a/services/web/app/src/router.js b/services/web/app/src/router.js index 9b447428eb..50e1623722 100644 --- a/services/web/app/src/router.js +++ b/services/web/app/src/router.js @@ -923,6 +923,16 @@ function initialize(webRouter, privateApiRouter, publicApiRouter) { AuthenticationController.requirePrivateApiAuth(), TpdsController.deleteUpdate ) + privateApiRouter.post( + '/project/:project_id/user/:user_id/update/*', + AuthenticationController.requirePrivateApiAuth(), + TpdsController.mergeUpdate + ) + privateApiRouter.delete( + '/project/:project_id/user/:user_id/update/*', + AuthenticationController.requirePrivateApiAuth(), + TpdsController.deleteUpdate + ) privateApiRouter.post( '/project/:project_id/contents/*', diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js index 4f6670317c..b3fa2ddf57 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsControllerTests.js @@ -63,14 +63,18 @@ describe('TpdsController', function () { this.projectName = 'projectName' this.path = '/here.txt' this.req = { - params: { 0: `${this.projectName}${this.path}`, user_id: this.user_id }, + params: { + 0: `${this.projectName}${this.path}`, + user_id: this.user_id, + project_id: '', + }, headers: { 'x-sl-update-source': (this.source = 'dropbox'), }, } }) - it('should process the update with the update receiver', function (done) { + it('should process the update with the update receiver by name', function (done) { const res = { json: payload => { expect(payload).to.deep.equal({ @@ -84,6 +88,7 @@ describe('TpdsController', function () { this.TpdsUpdateHandler.promises.newUpdate .calledWith( this.user_id, + '', // projectId this.projectName, this.path, this.req, @@ -107,6 +112,34 @@ describe('TpdsController', function () { this.TpdsController.mergeUpdate(this.req, res) }) + it('should process the update with the update receiver by id', function (done) { + const path = '/here.txt' + const req = { + pause() {}, + params: { 0: path, user_id: this.user_id, project_id: '123' }, + session: { + destroy() {}, + }, + headers: { + 'x-sl-update-source': (this.source = 'dropbox'), + }, + } + const res = { + json: () => { + this.TpdsUpdateHandler.promises.newUpdate.should.have.been.calledWith( + this.user_id, + '123', + '', // projectName + '/here.txt', + req, + this.source + ) + done() + }, + } + this.TpdsController.mergeUpdate(req, res) + }) + it('should return a 500 error when the update receiver fails', function (done) { this.TpdsUpdateHandler.promises.newUpdate.rejects(new Error()) const res = { @@ -150,10 +183,10 @@ describe('TpdsController', function () { }) describe('getting a delete update', function () { - it('should process the delete with the update receiver', function (done) { + it('should process the delete with the update receiver by name', function (done) { const path = '/projectName/here.txt' const req = { - params: { 0: path, user_id: this.user_id }, + params: { 0: path, user_id: this.user_id, project_id: '' }, session: { destroy() {}, }, @@ -164,13 +197,45 @@ describe('TpdsController', function () { const res = { sendStatus: () => { this.TpdsUpdateHandler.promises.deleteUpdate - .calledWith(this.user_id, 'projectName', '/here.txt', this.source) + .calledWith( + this.user_id, + '', + 'projectName', + '/here.txt', + this.source + ) .should.equal(true) done() }, } this.TpdsController.deleteUpdate(req, res) }) + + it('should process the delete with the update receiver by id', function (done) { + const path = '/here.txt' + const req = { + params: { 0: path, user_id: this.user_id, project_id: '123' }, + session: { + destroy() {}, + }, + headers: { + 'x-sl-update-source': (this.source = 'dropbox'), + }, + } + const res = { + sendStatus: () => { + this.TpdsUpdateHandler.promises.deleteUpdate.should.have.been.calledWith( + this.user_id, + '123', + '', // projectName + '/here.txt', + this.source + ) + done() + }, + } + this.TpdsController.deleteUpdate(req, res) + }) }) describe('updateFolder', function () { diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js index c3e440b9a5..d769ffecf7 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateHandlerTests.js @@ -78,6 +78,9 @@ describe('TpdsUpdateHandler', function () { this.ProjectGetter = { promises: { findUsersProjectsByName: sinon.stub(), + findAllUsersProjects: sinon + .stub() + .resolves({ owned: [this.projects.active1], readAndWrite: [] }), }, } this.ProjectHelper = { @@ -119,6 +122,26 @@ describe('TpdsUpdateHandler', function () { }) describe('getting an update', function () { + describe('byId', function () { + describe('with no matching project', function () { + beforeEach(function () { + this.projectId = ObjectId().toString() + }) + receiveUpdateById() + expectProjectNotCreated() + expectUpdateNotProcessed() + }) + + describe('with one matching active project', function () { + beforeEach(function () { + this.projectId = this.projects.active1._id.toString() + }) + receiveUpdateById() + expectProjectNotCreated() + expectUpdateProcessed() + }) + }) + describe('with no matching project', function () { setupMatchingProjects([]) receiveUpdate() @@ -183,6 +206,7 @@ describe('TpdsUpdateHandler', function () { await expect( this.TpdsUpdateHandler.promises.newUpdate( this.userId, + '', // projectId this.projectName, this.path, this.update, @@ -195,6 +219,26 @@ describe('TpdsUpdateHandler', function () { }) describe('getting a file delete', function () { + describe('byId', function () { + describe('with no matching project', function () { + beforeEach(function () { + this.projectId = ObjectId().toString() + }) + receiveFileDeleteById() + expectDeleteNotProcessed() + expectProjectNotDeleted() + }) + + describe('with one matching active project', function () { + beforeEach(function () { + this.projectId = this.projects.active1._id.toString() + }) + receiveFileDeleteById() + expectDeleteProcessed() + expectProjectNotDeleted() + }) + }) + describe('with no matching project', function () { setupMatchingProjects([]) receiveFileDelete() @@ -342,6 +386,7 @@ describe('TpdsUpdateHandler', function () { await expect( this.TpdsUpdateHandler.promises.createFolder( this.userId, + this.projectId, this.projectName, this.path ) @@ -377,6 +422,7 @@ function receiveUpdate() { beforeEach(async function () { await this.TpdsUpdateHandler.promises.newUpdate( this.userId, + '', // projectId this.projectName, this.path, this.update, @@ -385,10 +431,25 @@ function receiveUpdate() { }) } +function receiveUpdateById() { + beforeEach(function (done) { + this.TpdsUpdateHandler.newUpdate( + this.userId, + this.projectId, + '', // projectName + this.path, + this.update, + this.source, + done + ) + }) +} + function receiveFileDelete() { beforeEach(async function () { await this.TpdsUpdateHandler.promises.deleteUpdate( this.userId, + '', // projectId this.projectName, this.path, this.source @@ -396,10 +457,24 @@ function receiveFileDelete() { }) } +function receiveFileDeleteById() { + beforeEach(function (done) { + this.TpdsUpdateHandler.deleteUpdate( + this.userId, + this.projectId, + '', // projectName + this.path, + this.source, + done + ) + }) +} + function receiveProjectDelete() { beforeEach(async function () { await this.TpdsUpdateHandler.promises.deleteUpdate( this.userId, + '', // projectId this.projectName, '/', this.source @@ -411,6 +486,7 @@ function receiveFolderUpdate() { beforeEach(async function () { await this.TpdsUpdateHandler.promises.createFolder( this.userId, + this.projectId, this.projectName, this.folderPath )