From a218ad0b20467fa4fa913130153c0235b15fb545 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 25 Mar 2024 13:03:50 -0300 Subject: [PATCH 01/12] Include unique id in exported messages zip files to avoid replacements in external storage services --- apps/meteor/server/lib/dataExport/uploadZipFile.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/meteor/server/lib/dataExport/uploadZipFile.ts b/apps/meteor/server/lib/dataExport/uploadZipFile.ts index 5fe9ea2d57dd..77a16004bf64 100644 --- a/apps/meteor/server/lib/dataExport/uploadZipFile.ts +++ b/apps/meteor/server/lib/dataExport/uploadZipFile.ts @@ -3,6 +3,7 @@ import { stat } from 'fs/promises'; import type { IUser } from '@rocket.chat/core-typings'; import { Users } from '@rocket.chat/models'; +import { Random } from '@rocket.chat/random'; import { FileUpload } from '../../../app/file-upload/server'; @@ -18,10 +19,12 @@ export const uploadZipFile = async (filePath: string, userId: IUser['_id'], expo const utcDate = new Date().toISOString().split('T')[0]; const fileSuffix = exportType === 'json' ? '-data' : ''; + const fileId = Random.id(); - const newFileName = encodeURIComponent(`${utcDate}-${userDisplayName}${fileSuffix}.zip`); + const newFileName = encodeURIComponent(`${utcDate}-${userDisplayName}${fileSuffix}-${fileId}.zip`); const details = { + _id: fileId, userId, type: contentType, size, From ea60e7978a7f9a0adad4f8471840085d75574979 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Mon, 25 Mar 2024 20:26:34 -0300 Subject: [PATCH 02/12] Add unit tests --- .../lib/dataExport/uploadZipFile.spec.ts | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts diff --git a/apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts new file mode 100644 index 000000000000..c02b82aa0d6e --- /dev/null +++ b/apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts @@ -0,0 +1,101 @@ +import { expect } from 'chai'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; + +// Create stubs for dependencies +const stubs = { + findOneUserById: sinon.stub(), + randomId: sinon.stub(), + stat: sinon.stub(), + getStore: sinon.stub(), + insertFileStub: sinon.stub(), + createReadStream: sinon.stub(), +}; + +const { uploadZipFile } = proxyquire.noCallThru().load('./uploadZipFile.ts', { + '@rocket.chat/models': { + Users: { + findOneById: stubs.findOneUserById, + }, + }, + '@rocket.chat/random': { + Random: { + id: stubs.randomId, + }, + }, + 'fs/promises': { + stat: stubs.stat, + }, + 'fs': { + createReadStream: stubs.createReadStream, + }, + '../../../app/file-upload/server': { + FileUpload: { + getStore: stubs.getStore, + }, + }, +}); + +describe('Export - uploadZipFile', () => { + const randomId = 'random-id'; + const fileStat = 100; + const userName = 'John Doe'; + const userId = 'user-id'; + const filePath = 'random-path'; + + beforeAll(() => { + stubs.findOneUserById.returns({ name: userName }); + stubs.stat.returns(fileStat); + stubs.randomId.returns(randomId); + stubs.getStore.returns({ insert: stubs.insertFileStub }); + stubs.insertFileStub.callsFake((details) => ({ _id: details._id, name: details.name })); + }); + + it('should correctly build file name for json exports', async () => { + const result = await uploadZipFile(filePath, userId, 'json'); + + expect(stubs.findOneUserById.calledWith(userId)).to.be.true; + expect(stubs.stat.calledWith(filePath)).to.be.true; + expect(stubs.createReadStream.calledWith(filePath)).to.be.true; + expect(stubs.getStore.calledWith('UserDataFiles')).to.be.true; + expect( + stubs.insertFileStub.calledWith( + sinon.match({ + _id: randomId, + userId, + type: 'application/zip', + size: fileStat, + }), + ), + ).to.be.true; + + expect(result).to.have.property('_id', randomId); + expect(result).to.have.property('name').that.is.a.string; + const fileName: string = result.name; + expect(fileName.endsWith(`${userId}-data-${randomId}.zip`)); + }); + + it('should correctly build file name for html exports', async () => { + const result = await uploadZipFile(filePath, userId, 'html'); + + expect(stubs.findOneUserById.calledWith(userId)).to.be.true; + expect(stubs.stat.calledWith(filePath)).to.be.true; + expect(stubs.createReadStream.calledWith(filePath)).to.be.true; + expect(stubs.getStore.calledWith('UserDataFiles')).to.be.true; + expect( + stubs.insertFileStub.calledWith( + sinon.match({ + _id: randomId, + userId, + type: 'application/zip', + size: fileStat, + }), + ), + ).to.be.true; + + expect(result).to.have.property('_id', randomId); + expect(result).to.have.property('name').that.is.a.string; + const fileName: string = result.name; + expect(fileName.endsWith(`${userId}-${randomId}.zip`)); + }); +}); From 661f28aceabae450ebc7bbc3b9836f1bf4671c66 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Mon, 25 Mar 2024 20:30:13 -0300 Subject: [PATCH 03/12] Create changeset --- .changeset/twelve-seas-battle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/twelve-seas-battle.md diff --git a/.changeset/twelve-seas-battle.md b/.changeset/twelve-seas-battle.md new file mode 100644 index 000000000000..8b224803946b --- /dev/null +++ b/.changeset/twelve-seas-battle.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Added file id to message exports zip file names in order to avoid overwriting previous exports when using external storage services (such as Amazon S3) From 777142cf357f87b193ac1c32c153c5e00da2c605 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 26 Mar 2024 09:31:51 -0300 Subject: [PATCH 04/12] Improve changeset --- .changeset/twelve-seas-battle.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/twelve-seas-battle.md b/.changeset/twelve-seas-battle.md index 8b224803946b..3e170abe98b1 100644 --- a/.changeset/twelve-seas-battle.md +++ b/.changeset/twelve-seas-battle.md @@ -2,4 +2,4 @@ "@rocket.chat/meteor": patch --- -Added file id to message exports zip file names in order to avoid overwriting previous exports when using external storage services (such as Amazon S3) +Added file id to message exports zip file names in order to avoid overwriting previous exports generated in the same day when using external storage services (such as Amazon S3) From bf7169eabe748bd03c6bbed6e70d077b4976d3e3 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 26 Mar 2024 10:02:34 -0300 Subject: [PATCH 05/12] Update unit test file folder --- .../unit}/server/lib/dataExport/uploadZipFile.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename apps/meteor/{ => tests/unit}/server/lib/dataExport/uploadZipFile.spec.ts (96%) diff --git a/apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts similarity index 96% rename from apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts rename to apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index c02b82aa0d6e..10b065404efb 100644 --- a/apps/meteor/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -12,7 +12,7 @@ const stubs = { createReadStream: sinon.stub(), }; -const { uploadZipFile } = proxyquire.noCallThru().load('./uploadZipFile.ts', { +const { uploadZipFile } = proxyquire.noCallThru().load('../../../../server/lib/dataExport/uploadZipFile.ts', { '@rocket.chat/models': { Users: { findOneById: stubs.findOneUserById, From 2be52470c2d27f05df97356a1cf3da13b97e2fc0 Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 26 Mar 2024 10:52:23 -0300 Subject: [PATCH 06/12] Fix file path --- .../tests/unit/server/lib/dataExport/uploadZipFile.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index 10b065404efb..24b988d90008 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -12,7 +12,7 @@ const stubs = { createReadStream: sinon.stub(), }; -const { uploadZipFile } = proxyquire.noCallThru().load('../../../../server/lib/dataExport/uploadZipFile.ts', { +const { uploadZipFile } = proxyquire.noCallThru().load('../../../../../server/lib/dataExport/uploadZipFile.ts', { '@rocket.chat/models': { Users: { findOneById: stubs.findOneUserById, From 800779e0d1533aa8f0dbba5d529d3e0ae6eb22be Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 26 Mar 2024 11:10:34 -0300 Subject: [PATCH 07/12] Add tests for displayNames fallbacks --- .../lib/dataExport/uploadZipFile.spec.ts | 53 ++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index 24b988d90008..b7524397d357 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -40,6 +40,7 @@ describe('Export - uploadZipFile', () => { const randomId = 'random-id'; const fileStat = 100; const userName = 'John Doe'; + const userUsername = 'john.doe'; const userId = 'user-id'; const filePath = 'random-path'; @@ -72,7 +73,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userId}-data-${randomId}.zip`)); + expect(fileName.endsWith(`${userName}-data-${randomId}.zip`)); }); it('should correctly build file name for html exports', async () => { @@ -93,6 +94,56 @@ describe('Export - uploadZipFile', () => { ), ).to.be.true; + expect(result).to.have.property('_id', randomId); + expect(result).to.have.property('name').that.is.a.string; + const fileName: string = result.name; + expect(fileName.endsWith(`${userName}-${randomId}.zip`)); + }); + + it("should use username as a fallback in the zip file name when user's name is not defined", async () => { + stubs.findOneUserById.returns({ username: userUsername }); + const result = await uploadZipFile(filePath, userId, 'html'); + + expect(stubs.findOneUserById.calledWith(userId)).to.be.true; + expect(stubs.stat.calledWith(filePath)).to.be.true; + expect(stubs.createReadStream.calledWith(filePath)).to.be.true; + expect(stubs.getStore.calledWith('UserDataFiles')).to.be.true; + expect( + stubs.insertFileStub.calledWith( + sinon.match({ + _id: randomId, + userId, + type: 'application/zip', + size: fileStat, + }), + ), + ).to.be.true; + + expect(result).to.have.property('_id', randomId); + expect(result).to.have.property('name').that.is.a.string; + const fileName: string = result.name; + expect(fileName.endsWith(`${userUsername}-${randomId}.zip`)); + }); + + it("should use userId as a fallback in the zip file name when user's name and username are not defined", async () => { + stubs.findOneUserById.returns(undefined); + const result = await uploadZipFile(filePath, userId, 'html'); + + expect(stubs.findOneUserById.calledWith(userId)).to.be.true; + expect(stubs.stat.calledWith(filePath)).to.be.true; + expect(stubs.createReadStream.calledWith(filePath)).to.be.true; + expect(stubs.getStore.calledWith('UserDataFiles')).to.be.true; + expect( + stubs.insertFileStub.calledWith( + sinon.match({ + _id: randomId, + userId, + type: 'application/zip', + size: fileStat, + }), + ), + ).to.be.true; + expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; From e01b6f5bd9fec69815c4ee4305e416bf92ded81c Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 26 Mar 2024 11:37:55 -0300 Subject: [PATCH 08/12] Use mocha describe, it and before --- .../tests/unit/server/lib/dataExport/uploadZipFile.spec.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index b7524397d357..4d682c9699ef 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -1,4 +1,5 @@ import { expect } from 'chai'; +import { before, describe, it } from 'mocha'; import proxyquire from 'proxyquire'; import sinon from 'sinon'; @@ -44,7 +45,7 @@ describe('Export - uploadZipFile', () => { const userId = 'user-id'; const filePath = 'random-path'; - beforeAll(() => { + before(() => { stubs.findOneUserById.returns({ name: userName }); stubs.stat.returns(fileStat); stubs.randomId.returns(randomId); From 6f5db0d7c84f436f9955464e1315b51b7f4de8da Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Tue, 26 Mar 2024 12:07:15 -0300 Subject: [PATCH 09/12] Fix stub return --- .../tests/unit/server/lib/dataExport/uploadZipFile.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index 4d682c9699ef..ed85e96a3a5f 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -47,7 +47,7 @@ describe('Export - uploadZipFile', () => { before(() => { stubs.findOneUserById.returns({ name: userName }); - stubs.stat.returns(fileStat); + stubs.stat.returns({ size: fileStat }); stubs.randomId.returns(randomId); stubs.getStore.returns({ insert: stubs.insertFileStub }); stubs.insertFileStub.callsFake((details) => ({ _id: details._id, name: details.name })); From 297196b57d2d5fde3404294ab8414fee435e1228 Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Tue, 26 Mar 2024 12:08:00 -0300 Subject: [PATCH 10/12] Update changeset Co-authored-by: Debdut Chakraborty --- .changeset/twelve-seas-battle.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/twelve-seas-battle.md b/.changeset/twelve-seas-battle.md index 3e170abe98b1..a527a93f6212 100644 --- a/.changeset/twelve-seas-battle.md +++ b/.changeset/twelve-seas-battle.md @@ -2,4 +2,4 @@ "@rocket.chat/meteor": patch --- -Added file id to message exports zip file names in order to avoid overwriting previous exports generated in the same day when using external storage services (such as Amazon S3) +Fixed an issue where old exports would get overwritten by new ones if generated on the same day, when using external storage services (such as Amazon S3) From dea324eda1d8135df7a31bb87468e9b4092cde7a Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Wed, 27 Mar 2024 09:40:23 -0300 Subject: [PATCH 11/12] Apply suggestions from code review Co-authored-by: Marcos Spessatto Defendi --- .../unit/server/lib/dataExport/uploadZipFile.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index ed85e96a3a5f..37ead74c9621 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -74,7 +74,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userName}-data-${randomId}.zip`)); + expect(fileName.endsWith(`${userName}-data-${randomId}.zip`)).to.be.true; }); it('should correctly build file name for html exports', async () => { @@ -98,7 +98,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userName}-${randomId}.zip`)); + expect(fileName.endsWith(`${userName}-${randomId}.zip`)).to.be.true; }); it("should use username as a fallback in the zip file name when user's name is not defined", async () => { @@ -123,7 +123,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userUsername}-${randomId}.zip`)); + expect(fileName.endsWith(`${userUsername}-${randomId}.zip`)).to.be.true; }); it("should use userId as a fallback in the zip file name when user's name and username are not defined", async () => { @@ -148,6 +148,6 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userId}-${randomId}.zip`)); + expect(fileName.endsWith(`${userId}-${randomId}.zip`)).to.be.true; }); }); From 4dbb73525ab9a8d4b1ae3c810af52c0397e9dddd Mon Sep 17 00:00:00 2001 From: matheusbsilva137 Date: Wed, 27 Mar 2024 10:22:00 -0300 Subject: [PATCH 12/12] Fix unit tests --- .../tests/unit/server/lib/dataExport/uploadZipFile.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts index 37ead74c9621..61a477b40df5 100644 --- a/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts +++ b/apps/meteor/tests/unit/server/lib/dataExport/uploadZipFile.spec.ts @@ -74,7 +74,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userName}-data-${randomId}.zip`)).to.be.true; + expect(fileName.endsWith(encodeURIComponent(`${userName}-data-${randomId}.zip`))).to.be.true; }); it('should correctly build file name for html exports', async () => { @@ -98,7 +98,7 @@ describe('Export - uploadZipFile', () => { expect(result).to.have.property('_id', randomId); expect(result).to.have.property('name').that.is.a.string; const fileName: string = result.name; - expect(fileName.endsWith(`${userName}-${randomId}.zip`)).to.be.true; + expect(fileName.endsWith(encodeURIComponent(`${userName}-${randomId}.zip`))).to.be.true; }); it("should use username as a fallback in the zip file name when user's name is not defined", async () => {