diff --git a/.changeset/young-ants-applaud.md b/.changeset/young-ants-applaud.md new file mode 100644 index 0000000000000..4c500b0fb2744 --- /dev/null +++ b/.changeset/young-ants-applaud.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Fixes file deletion being called twice and ignoring errors with FileSystem storage type. diff --git a/apps/meteor/.mocharc.js b/apps/meteor/.mocharc.js index 4a47aa3a52d65..78a2a22611a5b 100644 --- a/apps/meteor/.mocharc.js +++ b/apps/meteor/.mocharc.js @@ -27,6 +27,7 @@ module.exports = { 'lib/callbacks.spec.ts', 'server/lib/ldap/*.spec.ts', 'server/lib/ldap/**/*.spec.ts', + 'server/ufs/*.spec.ts', 'ee/server/lib/ldap/*.spec.ts', 'ee/tests/**/*.tests.ts', 'ee/tests/**/*.spec.ts', diff --git a/apps/meteor/server/ufs/ufs-local.spec.ts b/apps/meteor/server/ufs/ufs-local.spec.ts new file mode 100644 index 0000000000000..b2eba7bf473e5 --- /dev/null +++ b/apps/meteor/server/ufs/ufs-local.spec.ts @@ -0,0 +1,115 @@ +import fs from 'fs'; + +import { expect } from 'chai'; +import { before, beforeEach, afterEach, describe, it } from 'mocha'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; +import type { SinonStub } from 'sinon'; + +// import { Config } from './ufs-config'; +// import { UploadFS } from './ufs'; +import type { LocalStore as LocalStoreClass } from './ufs-local'; + +const fakeCollection = { + removeById: sinon.stub(), + findOne: sinon.stub(), +}; + +// Proxyquire chain to mock all Meteor dependencies +const ufsFilterMock = proxyquire.noCallThru().load('./ufs-filter', { + 'meteor/meteor': {}, + 'meteor/check': {}, + 'meteor/mongo': {}, + 'meteor/npm-mongo': {}, +}); + +const { UploadFS } = proxyquire.noCallThru().load('./ufs', { + 'meteor/meteor': {}, + 'meteor/check': {}, + 'meteor/mongo': {}, + 'meteor/npm-mongo': {}, + './ufs-filter': ufsFilterMock, + './ufs-store': { + Store: Object, + }, +}); + +// Create the UploadFS mock object as a callable function with properties +// function UploadFS() { +// return UploadFS; +// } +// Object.assign(UploadFS, { +// store: {}, +// config: new Config(), +// addStore: sinon.stub(), +// getStore: sinon.stub().returns(undefined), +// getTempFilePath: sinon.stub().returns('/tmp/mockfile'), +// }); +// The module must export { UploadFS: ... } +const ufsMock = { UploadFS }; +const ufsMockModule = { UploadFS }; +const ufsIndexMock = { UploadFS }; + +const ufsStoreMockRaw = proxyquire.noCallThru().load('./ufs-store', { + 'meteor/meteor': {}, + 'meteor/check': {}, + 'meteor/mongo': {}, + 'meteor/npm-mongo': {}, + './ufs': ufsMockModule, + './ufs-filter': ufsFilterMock, + './index': ufsIndexMock, +}); +// Patch ufsStoreMock to export all possible ways (default, named, etc) +const ufsStoreMock = Object.assign({}, ufsStoreMockRaw, { default: ufsStoreMockRaw, ufsStoreMock: ufsStoreMockRaw }); +const localStoreProxy = proxyquire.noCallThru().load('./ufs-local', { + // 'mkdirp': sinon.stub(), + 'meteor/meteor': {}, + 'meteor/check': {}, + 'meteor/mongo': {}, + 'meteor/npm-mongo': {}, + './ufs': ufsMock, + './ufs-store': ufsStoreMock, + './index': ufsIndexMock, +}); + +const { LocalStore } = localStoreProxy as { + LocalStore: typeof LocalStoreClass; +}; + +describe('LocalStore', () => { + let store: LocalStoreClass; + let unlinkStub: SinonStub; + + before(() => { + fakeCollection.removeById.resolves(); + fakeCollection.findOne.resolves({ _id: 'test', name: 'file.txt' }); + store = new LocalStore({ name: 'test', collection: fakeCollection as any, path: '/tmp/ufs-local' }); + }); + + beforeEach(() => { + unlinkStub = sinon.stub(fs.promises, 'unlink').resolves(); + fakeCollection.removeById.resetHistory(); + fakeCollection.findOne.resetHistory(); + }); + + afterEach(() => { + unlinkStub.restore(); + }); + + it('should not throw if file does not exist (ENOENT)', async () => { + unlinkStub.rejects(Object.assign(new Error('not found'), { code: 'ENOENT' })); + await expect(store.delete('test')).to.be.fulfilled; + // Should only call unlink once + expect(unlinkStub.calledOnce).to.be.true; + }); + + it('should throw if unlink fails with non-ENOENT error', async () => { + unlinkStub.rejects(Object.assign(new Error('fail'), { code: 'EACCES' })); + await expect(store.delete('test')).to.be.rejectedWith('fail'); + }); + + it('should call findOne to get file info', async () => { + await store.delete('test'); + expect(fakeCollection.findOne.calledWith('test')).to.be.true; + }); +}); diff --git a/apps/meteor/server/ufs/ufs-local.ts b/apps/meteor/server/ufs/ufs-local.ts index e054cc54d8728..b8609f197fe16 100644 --- a/apps/meteor/server/ufs/ufs-local.ts +++ b/apps/meteor/server/ufs/ufs-local.ts @@ -1,5 +1,6 @@ import fs from 'fs'; -import { stat, unlink } from 'fs/promises'; +import { unlink } from 'fs/promises'; +import { isNativeError } from 'util/types'; import type { IUpload } from '@rocket.chat/core-typings'; import mkdirp from 'mkdirp'; @@ -68,18 +69,14 @@ export class LocalStore extends Store { this.delete = async (fileId, options) => { const path = await this.getFilePath(fileId); - try { - if (!(await stat(path)).isFile()) { - return; + await unlink(path); + } catch (error) { + if (!isNativeError(error) || !('code' in error) || !(error.code === 'ENOENT')) { + throw error; } - } catch (_e) { - // FIXME(user) don't ignore, rather this block shouldn't run twice like it does now - return; } - - await unlink(path); - await this.removeById(fileId, { session: options?.session }); + await this.removeById(fileId, { session: options?.session }, true); }; this.getReadStream = async (fileId: string, file: IUpload, options?: { start?: number; end?: number }) => { diff --git a/apps/meteor/server/ufs/ufs-store.ts b/apps/meteor/server/ufs/ufs-store.ts index 32caadd4a9db8..00979b8d934cf 100644 --- a/apps/meteor/server/ufs/ufs-store.ts +++ b/apps/meteor/server/ufs/ufs-store.ts @@ -229,9 +229,11 @@ export class Store { }; } - async removeById(fileId: string, options?: { session?: ClientSession }) { + async removeById(fileId: string, options?: { session?: ClientSession }, isDeleted = false): Promise { // Delete the physical file in the store - await this.delete(fileId); + if (!isDeleted) { + await this.delete(fileId); + } const tmpFile = UploadFS.getTempFilePath(fileId);