From c58a35f0c02d121c9b43869afb9a2d023c7e60bb Mon Sep 17 00:00:00 2001 From: Stevche Radevski Date: Thu, 18 Jul 2024 09:46:10 +0200 Subject: [PATCH] feat: Add support for private files to file module (#8169) --- .../core-flows/src/file/steps/upload-files.ts | 1 + .../src/file/workflows/upload-files.ts | 1 + packages/core/types/src/file/mutations.ts | 5 ++ packages/core/types/src/file/provider.ts | 10 ++- .../core/types/src/file/providers/local.ts | 1 + .../medusa/src/api/admin/uploads/route.ts | 1 + .../file-local/src/services/local-file.ts | 52 ++++++++---- .../__tests__/services.spec.ts | 79 ++++++++++--------- .../providers/file-s3/src/services/s3-file.ts | 4 +- .../integration-tests/__tests__/index.spec.ts | 2 +- .../integration-tests/__tests__/index.spec.ts | 5 +- 11 files changed, 98 insertions(+), 63 deletions(-) diff --git a/packages/core/core-flows/src/file/steps/upload-files.ts b/packages/core/core-flows/src/file/steps/upload-files.ts index d14a740b58..11d104f92e 100644 --- a/packages/core/core-flows/src/file/steps/upload-files.ts +++ b/packages/core/core-flows/src/file/steps/upload-files.ts @@ -7,6 +7,7 @@ type UploadFilesStepInput = { filename: string mimeType: string content: string + access: "public" | "private" }[] } diff --git a/packages/core/core-flows/src/file/workflows/upload-files.ts b/packages/core/core-flows/src/file/workflows/upload-files.ts index f8751e879e..a5c4a71b0b 100644 --- a/packages/core/core-flows/src/file/workflows/upload-files.ts +++ b/packages/core/core-flows/src/file/workflows/upload-files.ts @@ -7,6 +7,7 @@ type WorkflowInput = { filename: string mimeType: string content: string + access: "public" | "private" }[] } diff --git a/packages/core/types/src/file/mutations.ts b/packages/core/types/src/file/mutations.ts index 14a772799d..e6fb3a871b 100644 --- a/packages/core/types/src/file/mutations.ts +++ b/packages/core/types/src/file/mutations.ts @@ -16,4 +16,9 @@ export interface CreateFileDTO { * The file content as a binary-encoded string */ content: string + + /** + * The access level of the file. Defaults to private if not passed + */ + access?: "public" | "private" } diff --git a/packages/core/types/src/file/provider.ts b/packages/core/types/src/file/provider.ts index d7c6a6b140..bd9322322b 100644 --- a/packages/core/types/src/file/provider.ts +++ b/packages/core/types/src/file/provider.ts @@ -24,10 +24,6 @@ export type ProviderGetFileDTO = { * The file's key. */ fileKey: string - /** - * Whether the file is private. - */ - isPrivate?: boolean [x: string]: unknown } @@ -65,6 +61,11 @@ export type ProviderUploadFileDTO = { * The file content as a binary-encoded string */ content: string + + /** + * The access level of the file. Defaults to private if not passed + */ + access?: "public" | "private" } export interface IFileProvider { @@ -85,6 +86,7 @@ export interface IFileProvider { * */ delete(fileData: ProviderDeleteFileDTO): Promise + /** * This method is used to retrieve a download URL of the file. For some file services, such as S3, a presigned URL indicates a temporary URL to get access to a file. * diff --git a/packages/core/types/src/file/providers/local.ts b/packages/core/types/src/file/providers/local.ts index 71b39706b9..3eb7213cf5 100644 --- a/packages/core/types/src/file/providers/local.ts +++ b/packages/core/types/src/file/providers/local.ts @@ -1,4 +1,5 @@ export interface LocalFileServiceOptions { upload_dir?: string + private_upload_dir?: string backend_url?: string } diff --git a/packages/medusa/src/api/admin/uploads/route.ts b/packages/medusa/src/api/admin/uploads/route.ts index 9db8b1c96b..f573372c81 100644 --- a/packages/medusa/src/api/admin/uploads/route.ts +++ b/packages/medusa/src/api/admin/uploads/route.ts @@ -25,6 +25,7 @@ export const POST = async ( filename: f.originalname, mimeType: f.mimetype, content: f.buffer.toString("binary"), + access: "public", })), }, }) diff --git a/packages/modules/providers/file-local/src/services/local-file.ts b/packages/modules/providers/file-local/src/services/local-file.ts index af45e186a7..ac6c40486d 100644 --- a/packages/modules/providers/file-local/src/services/local-file.ts +++ b/packages/modules/providers/file-local/src/services/local-file.ts @@ -6,11 +6,14 @@ import path from "path" export class LocalFileService extends AbstractFileProviderService { static identifier = "localfs" protected uploadDir_: string + protected privateUploadDir_: string protected backendUrl_: string constructor(_, options: LocalFileServiceOptions) { super() this.uploadDir_ = options?.upload_dir || path.join(process.cwd(), "static") + this.privateUploadDir_ = + options?.private_upload_dir || path.join(process.cwd(), "private") this.backendUrl_ = options?.backend_url || "http://localhost:9000/static" } @@ -29,14 +32,19 @@ export class LocalFileService extends AbstractFileProviderService { } const parsedFilename = path.parse(file.filename) - await this.ensureDirExists(parsedFilename.dir) + const baseDir = + file.access === "public" ? this.uploadDir_ : this.privateUploadDir_ + const dir = await this.ensureDirExists(baseDir, parsedFilename.dir) const fileKey = path.join( parsedFilename.dir, - `${Date.now()}-${parsedFilename.base}` + // We append "private" to the file key so deletions and presigned URLs can know which folder to look into + `${Date.now()}-${parsedFilename.base}${ + file.access === "public" ? "" : "-private" + }` ) - const filePath = this.getUploadFilePath(fileKey) + const filePath = this.getUploadFilePath(baseDir, fileKey) const fileUrl = this.getUploadFileUrl(fileKey) const content = Buffer.from(file.content as string, "binary") @@ -49,7 +57,11 @@ export class LocalFileService extends AbstractFileProviderService { } async delete(file: FileTypes.ProviderDeleteFileDTO): Promise { - const filePath = this.getUploadFilePath(file.fileKey) + const baseDir = file.fileKey.endsWith("-private") + ? this.privateUploadDir_ + : this.uploadDir_ + + const filePath = this.getUploadFilePath(baseDir, file.fileKey) try { await fs.access(filePath, fs.constants.F_OK) await fs.unlink(filePath) @@ -60,26 +72,34 @@ export class LocalFileService extends AbstractFileProviderService { return } + // For private files, we simply return the file path, which can then be loaded manually by the backend. + // The local file provider doesn't support presigned URLs for private files. async getPresignedDownloadUrl( - fileData: FileTypes.ProviderGetFileDTO + file: FileTypes.ProviderGetFileDTO ): Promise { + const isPrivate = file.fileKey.endsWith("-private") + const baseDir = isPrivate ? this.privateUploadDir_ : this.uploadDir_ + + const filePath = this.getUploadFilePath(baseDir, file.fileKey) + try { - await fs.access( - this.getUploadFilePath(fileData.fileKey), - fs.constants.F_OK - ) + await fs.access(filePath, fs.constants.F_OK) } catch { throw new MedusaError( MedusaError.Types.NOT_FOUND, - `File with key ${fileData.fileKey} not found` + `File with key ${file.fileKey} not found` ) } - return this.getUploadFileUrl(fileData.fileKey) + if (isPrivate) { + return filePath + } + + return this.getUploadFileUrl(file.fileKey) } - private getUploadFilePath = (fileKey: string) => { - return path.join(this.uploadDir_, fileKey) + private getUploadFilePath = (baseDir: string, fileKey: string) => { + return path.join(baseDir, fileKey) } private getUploadFileUrl = (fileKey: string) => { @@ -88,12 +108,14 @@ export class LocalFileService extends AbstractFileProviderService { return baseUrl.href } - private async ensureDirExists(dirPath: string) { - const relativePath = path.join(this.uploadDir_, dirPath) + private async ensureDirExists(baseDir: string, dirPath: string) { + const relativePath = path.join(baseDir, dirPath) try { await fs.access(relativePath, fs.constants.F_OK) } catch (e) { await fs.mkdir(relativePath, { recursive: true }) } + + return relativePath } } diff --git a/packages/modules/providers/file-s3/integration-tests/__tests__/services.spec.ts b/packages/modules/providers/file-s3/integration-tests/__tests__/services.spec.ts index 02460d483a..e624879b99 100644 --- a/packages/modules/providers/file-s3/integration-tests/__tests__/services.spec.ts +++ b/packages/modules/providers/file-s3/integration-tests/__tests__/services.spec.ts @@ -35,45 +35,50 @@ describe.skip("S3 File Plugin", () => { } ) }) + ;(["public", "private"] as const).forEach((access) => { + it("uploads, reads, and then deletes a file successfully", async () => { + const fileContent = await fs.readFile(fixtureImagePath) + const fixtureAsBinary = fileContent.toString("binary") - it("uploads, reads, and then deletes a file successfully", async () => { - const fileContent = await fs.readFile(fixtureImagePath) - const fixtureAsBinary = fileContent.toString("binary") + const resp = await s3Service.upload({ + filename: "catphoto.jpg", + mimeType: "image/jpeg", + content: fixtureAsBinary, + access, + }) - const resp = await s3Service.upload({ - filename: "catphoto.jpg", - mimeType: "image/jpeg", - content: fixtureAsBinary, + expect(resp).toEqual({ + key: expect.stringMatching(/tests\/catphoto.*\.jpg/), + url: expect.stringMatching(/https:\/\/.*\.jpg/), + }) + + const urlResp = await axios.get(resp.url).catch((e) => e.response) + expect(urlResp.status).toEqual(access === "public" ? 200 : 403) + + const signedUrl = await s3Service.getPresignedDownloadUrl({ + fileKey: resp.key, + }) + + const signedUrlFile = Buffer.from( + await axios + .get(signedUrl, { responseType: "arraybuffer" }) + .then((r) => r.data) + ) + + expect(signedUrlFile.toString("binary")).toEqual(fixtureAsBinary) + + await s3Service.delete({ fileKey: resp.key }) + + // TODO: Currently the presignedURL will be returned even if the file doesn't exist. Should we check for existence first? + const deletedFileUrl = await s3Service.getPresignedDownloadUrl({ + fileKey: resp.key, + }) + + const { response } = await axios + .get(deletedFileUrl, { responseType: "arraybuffer" }) + .catch((e) => e) + + expect(response.status).toEqual(404) }) - - expect(resp).toEqual({ - key: expect.stringMatching(/tests\/catphoto.*\.jpg/), - url: expect.stringMatching(/https:\/\/.*\.jpg/), - }) - - const signedUrl = await s3Service.getPresignedDownloadUrl({ - fileKey: resp.key, - }) - - const signedUrlFile = Buffer.from( - await axios - .get(signedUrl, { responseType: "arraybuffer" }) - .then((r) => r.data) - ) - - expect(signedUrlFile.toString("binary")).toEqual(fixtureAsBinary) - - await s3Service.delete({ fileKey: resp.key }) - - // TODO: Currently the presignedURL will be returned even if the file doesn't exist. Should we check for existence first? - const deletedFileUrl = await s3Service.getPresignedDownloadUrl({ - fileKey: resp.key, - }) - - const { response } = await axios - .get(deletedFileUrl, { responseType: "arraybuffer" }) - .catch((e) => e) - - expect(response.status).toEqual(404) }) }) diff --git a/packages/modules/providers/file-s3/src/services/s3-file.ts b/packages/modules/providers/file-s3/src/services/s3-file.ts index a1a6e005ac..1377a1d2a2 100644 --- a/packages/modules/providers/file-s3/src/services/s3-file.ts +++ b/packages/modules/providers/file-s3/src/services/s3-file.ts @@ -92,14 +92,12 @@ export class S3FileService extends AbstractFileProviderService { const content = Buffer.from(file.content, "binary") const command = new PutObjectCommand({ - // TODO: Add support for private files // We probably also want to support a separate bucket altogether for private files // protected private_bucket_: string // protected private_access_key_id_: string // protected private_secret_access_key_: string - // ACL: options.acl ?? (options.isProtected ? "private" : "public-read"), - ACL: "public-read", + ACL: file.access === "public" ? "public-read" : "private", Bucket: this.config_.bucket, Body: content, Key: fileKey, diff --git a/packages/modules/workflow-engine-inmemory/integration-tests/__tests__/index.spec.ts b/packages/modules/workflow-engine-inmemory/integration-tests/__tests__/index.spec.ts index bffbd61b48..faeb0a70d4 100644 --- a/packages/modules/workflow-engine-inmemory/integration-tests/__tests__/index.spec.ts +++ b/packages/modules/workflow-engine-inmemory/integration-tests/__tests__/index.spec.ts @@ -377,7 +377,7 @@ moduleIntegrationTestRunner({ asFunction(() => "test") ) - const spy = await createScheduled("remove-scheduled", { + const spy = await createScheduled("shared-container-job", { cron: "* * * * * *", }) await jest.runOnlyPendingTimersAsync() diff --git a/packages/modules/workflow-engine-redis/integration-tests/__tests__/index.spec.ts b/packages/modules/workflow-engine-redis/integration-tests/__tests__/index.spec.ts index 677b58e606..951395c39e 100644 --- a/packages/modules/workflow-engine-redis/integration-tests/__tests__/index.spec.ts +++ b/packages/modules/workflow-engine-redis/integration-tests/__tests__/index.spec.ts @@ -74,12 +74,11 @@ describe("Workflow Orchestrator module", function () { }, }) - await onApplicationStart() - query = remoteQuery sharedContainer_ = sharedContainer! await runMigrations() + await onApplicationStart() workflowOrcModule = modules.workflows as unknown as IWorkflowEngineService }) @@ -391,7 +390,7 @@ describe("Workflow Orchestrator module", function () { asFunction(() => "test") ) - const spy = await createScheduled("remove-scheduled", { + const spy = await createScheduled("shared-container-job", { cron: "* * * * * *", }) await setTimeout(1100)