From 9b4a67f7096e1718be9831a2e85f38a9cd83d390 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 13 Nov 2023 20:14:27 +0000 Subject: [PATCH 1/6] chore: Move from S3 -> File module --- .../{s3 => modules/file}/controller.ts | 7 ++-- api.planx.uk/modules/file/docs.yaml | 0 .../s3.test.ts => modules/file/file.test.ts} | 4 +-- api.planx.uk/modules/file/routes.ts | 34 +++++++++++++++++++ .../file/service}/deleteFile.ts | 0 .../{s3 => modules/file/service}/getFile.ts | 0 .../file/service}/uploadFile.ts | 2 +- .../file/service}/utils.test.ts | 0 .../{s3 => modules/file/service}/utils.ts | 2 +- .../sanitiseApplicationData/operations.ts | 2 +- api.planx.uk/s3/index.ts | 13 ------- api.planx.uk/send/exportZip.ts | 2 +- api.planx.uk/server.ts | 30 ++-------------- 13 files changed, 46 insertions(+), 50 deletions(-) rename api.planx.uk/{s3 => modules/file}/controller.ts (91%) create mode 100644 api.planx.uk/modules/file/docs.yaml rename api.planx.uk/{s3/s3.test.ts => modules/file/file.test.ts} (99%) create mode 100644 api.planx.uk/modules/file/routes.ts rename api.planx.uk/{s3 => modules/file/service}/deleteFile.ts (100%) rename api.planx.uk/{s3 => modules/file/service}/getFile.ts (100%) rename api.planx.uk/{s3 => modules/file/service}/uploadFile.ts (97%) rename api.planx.uk/{s3 => modules/file/service}/utils.test.ts (100%) rename api.planx.uk/{s3 => modules/file/service}/utils.ts (95%) delete mode 100644 api.planx.uk/s3/index.ts diff --git a/api.planx.uk/s3/controller.ts b/api.planx.uk/modules/file/controller.ts similarity index 91% rename from api.planx.uk/s3/controller.ts rename to api.planx.uk/modules/file/controller.ts index 86f6f406ac..193530fd44 100644 --- a/api.planx.uk/s3/controller.ts +++ b/api.planx.uk/modules/file/controller.ts @@ -1,8 +1,9 @@ import assert from "assert"; -import { uploadPrivateFile, uploadPublicFile } from "./uploadFile"; -import { getFileFromS3 } from "./getFile"; + import { NextFunction, Request, Response } from "express"; -import { buildFilePath } from "./utils"; +import { uploadPrivateFile, uploadPublicFile } from "./service/uploadFile"; +import { buildFilePath } from "./service/utils"; +import { getFileFromS3 } from "./service/getFile"; assert(process.env.AWS_S3_BUCKET); assert(process.env.AWS_S3_REGION); diff --git a/api.planx.uk/modules/file/docs.yaml b/api.planx.uk/modules/file/docs.yaml new file mode 100644 index 0000000000..e69de29bb2 diff --git a/api.planx.uk/s3/s3.test.ts b/api.planx.uk/modules/file/file.test.ts similarity index 99% rename from api.planx.uk/s3/s3.test.ts rename to api.planx.uk/modules/file/file.test.ts index bec2a3e429..b6432f91b5 100644 --- a/api.planx.uk/s3/s3.test.ts +++ b/api.planx.uk/modules/file/file.test.ts @@ -1,7 +1,7 @@ import supertest from "supertest"; -import app from "../server"; -import { deleteFilesByURL } from "./deleteFile"; +import app from "../../server"; +import { deleteFilesByURL } from "./service/deleteFile"; let mockPutObject: jest.Mocked<() => void>; let mockGetObject: jest.Mocked<() => void>; diff --git a/api.planx.uk/modules/file/routes.ts b/api.planx.uk/modules/file/routes.ts new file mode 100644 index 0000000000..0a1dc57722 --- /dev/null +++ b/api.planx.uk/modules/file/routes.ts @@ -0,0 +1,34 @@ +import { Router } from "express"; + +import multer from "multer"; +import { useFilePermission } from "../auth/middleware"; +import { + privateDownloadController, + privateUploadController, + publicDownloadController, + publicUploadController, +} from "./controller"; + +const router = Router(); + +router.post( + "/private-file-upload", + multer().single("file"), + privateUploadController, +); + +router.post( + "/public-file-upload", + multer().single("file"), + publicUploadController, +); + +router.get("/file/public/:fileKey/:fileName", publicDownloadController); + +router.get( + "/file/private/:fileKey/:fileName", + useFilePermission, + privateDownloadController, +); + +export default router; diff --git a/api.planx.uk/s3/deleteFile.ts b/api.planx.uk/modules/file/service/deleteFile.ts similarity index 100% rename from api.planx.uk/s3/deleteFile.ts rename to api.planx.uk/modules/file/service/deleteFile.ts diff --git a/api.planx.uk/s3/getFile.ts b/api.planx.uk/modules/file/service/getFile.ts similarity index 100% rename from api.planx.uk/s3/getFile.ts rename to api.planx.uk/modules/file/service/getFile.ts diff --git a/api.planx.uk/s3/uploadFile.ts b/api.planx.uk/modules/file/service/uploadFile.ts similarity index 97% rename from api.planx.uk/s3/uploadFile.ts rename to api.planx.uk/modules/file/service/uploadFile.ts index a3e76c7a8e..856eae425c 100644 --- a/api.planx.uk/s3/uploadFile.ts +++ b/api.planx.uk/modules/file/service/uploadFile.ts @@ -2,7 +2,7 @@ import S3 from "aws-sdk/clients/s3"; import { customAlphabet } from "nanoid"; import { getType } from "mime"; import { s3Factory } from "./utils"; -import { isLiveEnv } from "../helpers"; +import { isLiveEnv } from "../../../helpers"; const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); export const uploadPublicFile = async ( diff --git a/api.planx.uk/s3/utils.test.ts b/api.planx.uk/modules/file/service/utils.test.ts similarity index 100% rename from api.planx.uk/s3/utils.test.ts rename to api.planx.uk/modules/file/service/utils.test.ts diff --git a/api.planx.uk/s3/utils.ts b/api.planx.uk/modules/file/service/utils.ts similarity index 95% rename from api.planx.uk/s3/utils.ts rename to api.planx.uk/modules/file/service/utils.ts index 03c51a002d..d7e97f1487 100644 --- a/api.planx.uk/s3/utils.ts +++ b/api.planx.uk/modules/file/service/utils.ts @@ -1,5 +1,5 @@ import S3 from "aws-sdk/clients/s3"; -import { isLiveEnv } from "../helpers"; +import { isLiveEnv } from "../../../helpers"; export function s3Factory() { return new S3({ diff --git a/api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts b/api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts index c5d3a0697c..8e1af46be2 100644 --- a/api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts +++ b/api.planx.uk/modules/webhooks/service/sanitiseApplicationData/operations.ts @@ -3,7 +3,7 @@ import { subMonths } from "date-fns"; import { Operation, OperationResult, QueryResult } from "./types"; import { runSQL } from "../../../../lib/hasura/schema"; -import { deleteFilesByURL } from "../../../../s3/deleteFile"; +import { deleteFilesByURL } from "../../../file/service/deleteFile"; import { $api } from "../../../../client"; import { Passport } from "@opensystemslab/planx-core"; diff --git a/api.planx.uk/s3/index.ts b/api.planx.uk/s3/index.ts deleted file mode 100644 index b3b4e67951..0000000000 --- a/api.planx.uk/s3/index.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { - privateUploadController, - publicUploadController, - publicDownloadController, - privateDownloadController, -} from "./controller"; - -export { - privateUploadController, - publicUploadController, - publicDownloadController, - privateDownloadController, -}; diff --git a/api.planx.uk/send/exportZip.ts b/api.planx.uk/send/exportZip.ts index c342e43ff8..80800d84e8 100644 --- a/api.planx.uk/send/exportZip.ts +++ b/api.planx.uk/send/exportZip.ts @@ -5,7 +5,7 @@ import { stringify } from "csv-stringify"; import fs from "fs"; import str from "string-to-stream"; import AdmZip from "adm-zip"; -import { getFileFromS3 } from "../s3/getFile"; +import { getFileFromS3 } from "../modules/file/service/getFile"; import { hasRequiredDataForTemplate, generateMapHTML, diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 86e2ff1418..487f64ee7b 100644 --- a/api.planx.uk/server.ts +++ b/api.planx.uk/server.ts @@ -11,7 +11,6 @@ import pinoLogger from "express-pino-logger"; import { Server } from "http"; import passport from "passport"; import helmet from "helmet"; -import multer from "multer"; import { ServerError } from "./errors"; import { locationSearch } from "./gis/index"; @@ -32,7 +31,6 @@ import { fetchPaymentRequestViaProxy, } from "./inviteToPay"; import { - useFilePermission, useHasuraAuth, useSendEmailAuth, usePlatformAdminAuth, @@ -41,12 +39,6 @@ import { import airbrake from "./airbrake"; import { sendEmailLimiter, apiLimiter } from "./rateLimit"; -import { - privateDownloadController, - privateUploadController, - publicDownloadController, - publicUploadController, -} from "./s3"; import { sendToBOPS } from "./send/bops"; import { createSendEvents } from "./send/createSendEvents"; import { downloadApplicationFiles, sendToEmail } from "./send/email"; @@ -64,6 +56,7 @@ import webhookRoutes from "./modules/webhooks/routes"; import analyticsRoutes from "./modules/analytics/routes"; import adminRoutes from "./modules/admin/routes"; import ordnanceSurveyRoutes from "./modules/ordnanceSurvey/routes"; +import fileRoutes from "./modules/file/routes"; import { useSwaggerDocs } from "./docs"; import { Role } from "@opensystemslab/planx-core/types"; import { $public } from "./client"; @@ -185,6 +178,7 @@ app.use("/webhooks", webhookRoutes); app.use("/analytics", analyticsRoutes); app.use("/admin", adminRoutes); app.use(ordnanceSurveyRoutes); +app.use(fileRoutes); app.use("/gis", router); @@ -307,26 +301,6 @@ app.get("/flows/:flowId/download-schema", async (req, res, next) => { } }); -app.post( - "/private-file-upload", - multer().single("file"), - privateUploadController, -); - -app.post( - "/public-file-upload", - multer().single("file"), - publicUploadController, -); - -app.get("/file/public/:fileKey/:fileName", publicDownloadController); - -app.get( - "/file/private/:fileKey/:fileName", - useFilePermission, - privateDownloadController, -); - assert(process.env.GOVUK_NOTIFY_API_KEY); app.post( "/send-email/:template", From eade261f634dce23caee0b15c5b2d98da285d0e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 13 Nov 2023 20:17:35 +0000 Subject: [PATCH 2/6] chore: Rename routes --- api.planx.uk/modules/file/file.test.ts | 6 +++--- api.planx.uk/modules/file/routes.ts | 12 ++++-------- api.planx.uk/server.ts | 1 + editor.planx.uk/src/api/upload.ts | 4 ++-- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/api.planx.uk/modules/file/file.test.ts b/api.planx.uk/modules/file/file.test.ts index b6432f91b5..6bd57d904b 100644 --- a/api.planx.uk/modules/file/file.test.ts +++ b/api.planx.uk/modules/file/file.test.ts @@ -41,7 +41,7 @@ describe("File upload", () => { }); describe("Private", () => { - const ENDPOINT = "/private-file-upload"; + const ENDPOINT = "/file/private/upload"; it("should not upload without filename", async () => { await supertest(app) @@ -89,7 +89,7 @@ describe("File upload", () => { })); await supertest(app) - .post("/private-file-upload") + .post("/file/private/upload") .field("filename", "some_file.txt") .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) @@ -101,7 +101,7 @@ describe("File upload", () => { }); describe("Public", () => { - const ENDPOINT = "/public-file-upload"; + const ENDPOINT = "/file/public/upload"; it("should not upload without filename", async () => { await supertest(app) diff --git a/api.planx.uk/modules/file/routes.ts b/api.planx.uk/modules/file/routes.ts index 0a1dc57722..0e8a6e9dab 100644 --- a/api.planx.uk/modules/file/routes.ts +++ b/api.planx.uk/modules/file/routes.ts @@ -12,21 +12,17 @@ import { const router = Router(); router.post( - "/private-file-upload", + "/private/upload", multer().single("file"), privateUploadController, ); -router.post( - "/public-file-upload", - multer().single("file"), - publicUploadController, -); +router.post("/public/upload", multer().single("file"), publicUploadController); -router.get("/file/public/:fileKey/:fileName", publicDownloadController); +router.get("/public/:fileKey/:fileName", publicDownloadController); router.get( - "/file/private/:fileKey/:fileName", + "/private/:fileKey/:fileName", useFilePermission, privateDownloadController, ); diff --git a/api.planx.uk/server.ts b/api.planx.uk/server.ts index 487f64ee7b..b21b55d8dc 100644 --- a/api.planx.uk/server.ts +++ b/api.planx.uk/server.ts @@ -179,6 +179,7 @@ app.use("/analytics", analyticsRoutes); app.use("/admin", adminRoutes); app.use(ordnanceSurveyRoutes); app.use(fileRoutes); +app.use("/file", fileRoutes); app.use("/gis", router); diff --git a/editor.planx.uk/src/api/upload.ts b/editor.planx.uk/src/api/upload.ts index 1f3834b1da..40279b4e07 100644 --- a/editor.planx.uk/src/api/upload.ts +++ b/editor.planx.uk/src/api/upload.ts @@ -36,8 +36,8 @@ function handleUpload( // Private uploads for test applications should be handled by the staging environment const paths = { - public: `${process.env.REACT_APP_API_URL}/public-file-upload`, - private: `${process.env.REACT_APP_API_URL}/private-file-upload`, + public: `${process.env.REACT_APP_API_URL}/file/public/upload`, + private: `${process.env.REACT_APP_API_URL}/file/private/upload`, }; const endpoint = paths[path]; From c47aef3a71727b82690c30adc2c11fbc6cc4d331 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 13 Nov 2023 21:00:07 +0000 Subject: [PATCH 3/6] chore: Add validation and type handling --- api.planx.uk/modules/file/controller.ts | 119 ++++++++++++++++-------- api.planx.uk/modules/file/file.test.ts | 34 +++---- api.planx.uk/modules/file/routes.ts | 18 +++- 3 files changed, 112 insertions(+), 59 deletions(-) diff --git a/api.planx.uk/modules/file/controller.ts b/api.planx.uk/modules/file/controller.ts index 193530fd44..a63b03b621 100644 --- a/api.planx.uk/modules/file/controller.ts +++ b/api.planx.uk/modules/file/controller.ts @@ -1,83 +1,120 @@ import assert from "assert"; - -import { NextFunction, Request, Response } from "express"; import { uploadPrivateFile, uploadPublicFile } from "./service/uploadFile"; import { buildFilePath } from "./service/utils"; import { getFileFromS3 } from "./service/getFile"; +import { z } from "zod"; +import { ValidatedRequestHandler } from "../../shared/middleware/validate"; +import { ServerError } from "../../errors"; +import { S3 } from "aws-sdk"; assert(process.env.AWS_S3_BUCKET); assert(process.env.AWS_S3_REGION); assert(process.env.AWS_ACCESS_KEY); assert(process.env.AWS_SECRET_KEY); -export const privateUploadController = async ( - req: Request, - res: Response, - next: NextFunction, -) => { - if (!req.body.filename) - return next({ status: 422, message: "missing filename" }); - if (!req.file) return next({ status: 422, message: "missing file" }); +interface UploadFileResponse { + // TODO: change to snake case + file_type: string | null; + fileUrl: string; +} - try { - const fileResponse = await uploadPrivateFile(req.file, req.body.filename); +export const uploadFileSchema = z.object({ + body: z.object({ + filename: z.string().trim().min(1), + }), +}); +export type UploadController = ValidatedRequestHandler< + typeof uploadFileSchema, + UploadFileResponse +>; + +export const privateUploadController: UploadController = async ( + req, + res, + next, +) => { + try { + if (!req.file) throw Error("Missing file"); + const { filename } = res.locals.parsedReq.body; + const fileResponse = await uploadPrivateFile(req.file, filename); res.json(fileResponse); - } catch (err) { - next(err); + } catch (error) { + return next( + new ServerError({ message: `Failed to upload private file: ${error}` }), + ); } }; -export const publicUploadController = async ( - req: Request, - res: Response, - next: NextFunction, +export const publicUploadController: UploadController = async ( + req, + res, + next, ) => { - if (!req.body.filename) - return next({ status: 422, message: "missing filename" }); - if (!req.file) return next({ status: 422, message: "missing file" }); - try { - const fileResponse = await uploadPublicFile(req.file, req.body.filename); - + if (!req.file) throw Error("Missing file"); + const { filename } = res.locals.parsedReq.body; + const fileResponse = await uploadPublicFile(req.file, filename); res.json(fileResponse); - } catch (err) { - next(err); + } catch (error) { + return next( + new ServerError({ + message: `Failed to upload public file: ${(error as Error).message}`, + }), + ); } }; -export const publicDownloadController = async ( - req: Request, - res: Response, - next: NextFunction, +export const downloadFileSchema = z.object({ + params: z.object({ + fileKey: z.string(), + fileName: z.string(), + }), +}); + +export type DownloadController = ValidatedRequestHandler< + typeof downloadFileSchema, + S3.Body | undefined +>; + +export const publicDownloadController: DownloadController = async ( + req, + res, + next, ) => { - const filePath = buildFilePath(req.params.fileKey, req.params.fileName); + const { fileKey, fileName } = res.locals.parsedReq.params; + const filePath = buildFilePath(fileKey, fileName); try { const { body, headers, isPrivate } = await getFileFromS3(filePath); - if (isPrivate) return next({ status: 400, message: "bad request" }); + if (isPrivate) throw Error("Bad request"); res.set(headers); res.send(body); - } catch (err) { - next(err); + } catch (error) { + return next( + new ServerError({ message: `Failed to download public file: ${error}` }), + ); } }; -export const privateDownloadController = async ( - req: Request, - res: Response, - next: NextFunction, +export const privateDownloadController: DownloadController = async ( + req, + res, + next, ) => { - const filePath = buildFilePath(req.params.fileKey, req.params.fileName); + const { fileKey, fileName } = res.locals.parsedReq.params; + const filePath = buildFilePath(fileKey, fileName); try { const { body, headers } = await getFileFromS3(filePath); res.set(headers); res.send(body); - } catch (err) { - next(err); + } catch (error) { + return next( + new ServerError({ message: `Failed to download private file: ${error}` }), + ); } }; diff --git a/api.planx.uk/modules/file/file.test.ts b/api.planx.uk/modules/file/file.test.ts index 6bd57d904b..6824871102 100644 --- a/api.planx.uk/modules/file/file.test.ts +++ b/api.planx.uk/modules/file/file.test.ts @@ -48,10 +48,11 @@ describe("File upload", () => { .post(ENDPOINT) .field("filename", "") .attach("file", Buffer.from("some data"), "some_file.txt") - .expect(422) + .expect(400) .then((res) => { expect(mockPutObject).not.toHaveBeenCalled(); - expect(res.body.error).toBe("missing filename"); + expect(res.body).toHaveProperty("issues"); + expect(res.body).toHaveProperty("name", "ZodError"); }); }); @@ -59,10 +60,10 @@ describe("File upload", () => { await supertest(app) .post(ENDPOINT) .field("filename", "some filename") - .expect(422) + .expect(500) .then((res) => { expect(mockPutObject).not.toHaveBeenCalled(); - expect(res.body.error).toBe("missing file"); + expect(res.body.error).toMatch(/Missing file/); }); }); @@ -94,7 +95,7 @@ describe("File upload", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) .then((res) => { - expect(res.body).toEqual({ error: "S3 error!" }); + expect(res.body.error).toMatch(/S3 error!/); }); expect(mockPutObject).toHaveBeenCalledTimes(1); }); @@ -108,10 +109,11 @@ describe("File upload", () => { .post(ENDPOINT) .field("filename", "") .attach("file", Buffer.from("some data"), "some_file.txt") - .expect(422) + .expect(400) .then((res) => { expect(mockPutObject).not.toHaveBeenCalled(); - expect(res.body.error).toBe("missing filename"); + expect(res.body).toHaveProperty("issues"); + expect(res.body).toHaveProperty("name", "ZodError"); }); }); @@ -119,10 +121,10 @@ describe("File upload", () => { await supertest(app) .post(ENDPOINT) .field("filename", "some filename") - .expect(422) + .expect(500) .then((res) => { expect(mockPutObject).not.toHaveBeenCalled(); - expect(res.body.error).toBe("missing file"); + expect(res.body.error).toMatch(/Missing file/); }); }); @@ -154,7 +156,7 @@ describe("File upload", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) .then((res) => { - expect(res.body).toEqual({ error: "S3 error!" }); + expect(res.body.error).toMatch(/S3 error!/); }); expect(mockPutObject).toHaveBeenCalledTimes(1); }); @@ -206,10 +208,10 @@ describe("File download", () => { await supertest(app) .get(`/file/public/${filePath}`) - .expect(400) + .expect(500) .then((res) => { expect(mockGetObject).toHaveBeenCalledTimes(1); - expect(res.body.error).toBe("bad request"); + expect(res.body.error).toMatch(/Bad request/); }); }); @@ -224,7 +226,7 @@ describe("File download", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) .then((res) => { - expect(res.body).toEqual({ error: "S3 error!" }); + expect(res.body.error).toMatch(/S3 error!/); }); expect(mockGetObject).toHaveBeenCalledTimes(1); }); @@ -249,10 +251,10 @@ describe("File download", () => { await supertest(app) .get(`/file/public/${filePath}`) - .expect(400) + .expect(500) .then((res) => { expect(mockGetObject).toHaveBeenCalledTimes(1); - expect(res.body.error).toBe("bad request"); + expect(res.body.error).toMatch(/Bad request/); }); }); @@ -303,7 +305,7 @@ describe("File download", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) .then((res) => { - expect(res.body).toEqual({ error: "S3 error!" }); + expect(res.body.error).toMatch(/S3 error!/); }); expect(mockGetObject).toHaveBeenCalledTimes(1); }); diff --git a/api.planx.uk/modules/file/routes.ts b/api.planx.uk/modules/file/routes.ts index 0e8a6e9dab..1a0e26ce1d 100644 --- a/api.planx.uk/modules/file/routes.ts +++ b/api.planx.uk/modules/file/routes.ts @@ -3,27 +3,41 @@ import { Router } from "express"; import multer from "multer"; import { useFilePermission } from "../auth/middleware"; import { + downloadFileSchema, privateDownloadController, privateUploadController, publicDownloadController, publicUploadController, + uploadFileSchema, } from "./controller"; +import { validate } from "../../shared/middleware/validate"; const router = Router(); router.post( "/private/upload", multer().single("file"), + validate(uploadFileSchema), privateUploadController, ); -router.post("/public/upload", multer().single("file"), publicUploadController); +router.post( + "/public/upload", + multer().single("file"), + validate(uploadFileSchema), + publicUploadController, +); -router.get("/public/:fileKey/:fileName", publicDownloadController); +router.get( + "/public/:fileKey/:fileName", + validate(downloadFileSchema), + publicDownloadController, +); router.get( "/private/:fileKey/:fileName", useFilePermission, + validate(downloadFileSchema), privateDownloadController, ); From 00d7f0c4fb751ff191037d69a09faa9c24bb846f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 13 Nov 2023 21:02:19 +0000 Subject: [PATCH 4/6] chore: file_type -> fileType --- api.planx.uk/modules/file/controller.ts | 3 +-- api.planx.uk/modules/file/file.test.ts | 4 ++-- .../modules/file/service/uploadFile.ts | 4 ++-- .../FileUploadAndLabel/Public.test.tsx | 20 +++++++++---------- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/api.planx.uk/modules/file/controller.ts b/api.planx.uk/modules/file/controller.ts index a63b03b621..ba77a1a26e 100644 --- a/api.planx.uk/modules/file/controller.ts +++ b/api.planx.uk/modules/file/controller.ts @@ -13,8 +13,7 @@ assert(process.env.AWS_ACCESS_KEY); assert(process.env.AWS_SECRET_KEY); interface UploadFileResponse { - // TODO: change to snake case - file_type: string | null; + fileType: string | null; fileUrl: string; } diff --git a/api.planx.uk/modules/file/file.test.ts b/api.planx.uk/modules/file/file.test.ts index 6824871102..084ba3ad33 100644 --- a/api.planx.uk/modules/file/file.test.ts +++ b/api.planx.uk/modules/file/file.test.ts @@ -74,7 +74,7 @@ describe("File upload", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .then((res) => { expect(res.body).toEqual({ - file_type: "text/plain", + fileType: "text/plain", fileUrl: expect.stringContaining( "/file/private/nanoid/modified%20key", ), @@ -135,7 +135,7 @@ describe("File upload", () => { .attach("file", Buffer.from("some data"), "some_file.txt") .then((res) => { expect(res.body).toEqual({ - file_type: "text/plain", + fileType: "text/plain", fileUrl: expect.stringContaining( "file/public/nanoid/modified%20key", ), diff --git a/api.planx.uk/modules/file/service/uploadFile.ts b/api.planx.uk/modules/file/service/uploadFile.ts index 856eae425c..f1ec699c82 100644 --- a/api.planx.uk/modules/file/service/uploadFile.ts +++ b/api.planx.uk/modules/file/service/uploadFile.ts @@ -17,7 +17,7 @@ export const uploadPublicFile = async ( const fileUrl = buildFileUrl(key, "public"); return { - file_type: fileType, + fileType, fileUrl, }; }; @@ -38,7 +38,7 @@ export const uploadPrivateFile = async ( const fileUrl = buildFileUrl(key, "private"); return { - file_type: fileType, + fileType, fileUrl, }; }; diff --git a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx index 85334f57eb..75ac777f5f 100644 --- a/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx +++ b/editor.planx.uk/src/@planx/components/FileUploadAndLabel/Public.test.tsx @@ -203,7 +203,7 @@ describe("Modal trigger", () => { ); const mockedPost = mockedAxios.post.mockResolvedValue({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test.jpg", }, }); @@ -236,14 +236,14 @@ describe("Modal trigger", () => { const mockedPost = mockedAxios.post .mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test1.jpg", }, }) .mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test2.jpg", }, @@ -282,14 +282,14 @@ describe("Modal trigger", () => { mockedAxios.post .mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test1.jpg", }, }) .mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test2.jpg", }, @@ -343,7 +343,7 @@ describe("Adding tags and syncing state", () => { // Upload one file mockedAxios.post.mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test1.jpg", }, }); @@ -406,7 +406,7 @@ describe("Adding tags and syncing state", () => { // Upload one file mockedAxios.post.mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test1.jpg", }, }); @@ -472,7 +472,7 @@ describe("Error handling", () => { mockedAxios.post.mockResolvedValueOnce({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test.png", }, }); @@ -524,7 +524,7 @@ describe("Error handling", () => { mockedAxios.post.mockResolvedValue({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test.jpg", }, }); @@ -565,7 +565,7 @@ describe("Error handling", () => { mockedAxios.post.mockResolvedValue({ data: { - file_type: "image/png", + fileType: "image/png", fileUrl: "https://api.editor.planx.dev/file/private/gws7l5d1/test.jpg", }, }); From 7ff5fcc01e2d891cfeec7b9f3228ae205f562958 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Mon, 13 Nov 2023 22:26:46 +0000 Subject: [PATCH 5/6] docs: API docs for file endpoints --- api.planx.uk/docs/index.ts | 7 ++ api.planx.uk/modules/file/controller.ts | 4 +- api.planx.uk/modules/file/docs.yaml | 99 +++++++++++++++++++++++++ api.planx.uk/modules/file/file.test.ts | 17 +++++ api.planx.uk/modules/file/routes.ts | 11 +-- 5 files changed, 131 insertions(+), 7 deletions(-) diff --git a/api.planx.uk/docs/index.ts b/api.planx.uk/docs/index.ts index 11d4f9d5e1..f12939a0b6 100644 --- a/api.planx.uk/docs/index.ts +++ b/api.planx.uk/docs/index.ts @@ -8,6 +8,13 @@ const securitySchemes = { scheme: "bearer", bearerFormat: "JWT", }, + fileAPIKeyAuth: { + type: "apiKey", + in: "header", + name: "api-key", + description: + "API key granted to third-party integration partners to access files uploaded by users as part of their application", + }, hasuraAuth: { type: "apiKey", in: "header", diff --git a/api.planx.uk/modules/file/controller.ts b/api.planx.uk/modules/file/controller.ts index ba77a1a26e..0e718e133e 100644 --- a/api.planx.uk/modules/file/controller.ts +++ b/api.planx.uk/modules/file/controller.ts @@ -77,7 +77,7 @@ export type DownloadController = ValidatedRequestHandler< >; export const publicDownloadController: DownloadController = async ( - req, + _req, res, next, ) => { @@ -99,7 +99,7 @@ export const publicDownloadController: DownloadController = async ( }; export const privateDownloadController: DownloadController = async ( - req, + _req, res, next, ) => { diff --git a/api.planx.uk/modules/file/docs.yaml b/api.planx.uk/modules/file/docs.yaml index e69de29bb2..0315cf0424 100644 --- a/api.planx.uk/modules/file/docs.yaml +++ b/api.planx.uk/modules/file/docs.yaml @@ -0,0 +1,99 @@ +openapi: 3.1.0 +info: + title: Planâś• API + version: 0.1.0 +tags: + - name: file + description: Endpoints for uploading and downloading files +components: + parameters: + fileKey: + in: path + name: fileKey + type: string + required: true + fileName: + in: path + name: fileName + type: string + required: true + schemas: + UploadFile: + type: object + properties: + filename: + type: string + required: true + file: + type: string + format: binary + responses: + UploadFile: + type: object + properties: + fileType: + oneOf: + - type: string + - type: "null" + fileUrl: + type: string + DownloadFile: + description: Successful response + content: + application/octet-stream: + schema: + type: string + format: binary +paths: + /file/private/upload: + post: + tags: ["file"] + security: + - bearerAuth: [] + requestBody: + content: + multipart/form-data: + schema: + $ref: "#/components/schemas/UploadFile" + responses: + "200": + $ref: "#/components/responses/UploadFile" + "500": + $ref: "#/components/responses/ErrorMessage" + /file/public/upload: + post: + tags: ["file"] + requestBody: + content: + multipart/form-data: + schema: + $ref: "#/components/schemas/UploadFile" + responses: + "200": + $ref: "#/components/responses/UploadFile" + "500": + $ref: "#/components/responses/ErrorMessage" + /file/public/{fileKey}/{fileName}: + get: + tags: ["file"] + parameters: + - $ref: "#/components/parameters/fileKey" + - $ref: "#/components/parameters/fileName" + responses: + "200": + $ref: "#/components/responses/DownloadFile" + "500": + $ref: "#/components/responses/ErrorMessage" + /file/private/{fileKey}/{fileName}: + get: + tags: ["file"] + parameters: + - $ref: "#/components/parameters/fileKey" + - $ref: "#/components/parameters/fileName" + security: + - fileAPIKeyAuth: [] + responses: + "200": + $ref: "#/components/responses/DownloadFile" + "500": + $ref: "#/components/responses/ErrorMessage" diff --git a/api.planx.uk/modules/file/file.test.ts b/api.planx.uk/modules/file/file.test.ts index 084ba3ad33..5364319cb8 100644 --- a/api.planx.uk/modules/file/file.test.ts +++ b/api.planx.uk/modules/file/file.test.ts @@ -2,6 +2,7 @@ import supertest from "supertest"; import app from "../../server"; import { deleteFilesByURL } from "./service/deleteFile"; +import { authHeader } from "../../tests/mockJWT"; let mockPutObject: jest.Mocked<() => void>; let mockGetObject: jest.Mocked<() => void>; @@ -42,10 +43,23 @@ describe("File upload", () => { describe("Private", () => { const ENDPOINT = "/file/private/upload"; + const auth = authHeader({ role: "teamEditor" }); + + it("returns an error if authorization headers are not set", async () => { + await supertest(app) + .post("/flows/1/move/new-team") + .expect(401) + .then((res) => { + expect(res.body).toEqual({ + error: "No authorization token was found", + }); + }); + }); it("should not upload without filename", async () => { await supertest(app) .post(ENDPOINT) + .set(auth) .field("filename", "") .attach("file", Buffer.from("some data"), "some_file.txt") .expect(400) @@ -59,6 +73,7 @@ describe("File upload", () => { it("should not upload without file", async () => { await supertest(app) .post(ENDPOINT) + .set(auth) .field("filename", "some filename") .expect(500) .then((res) => { @@ -70,6 +85,7 @@ describe("File upload", () => { it("should upload file", async () => { await supertest(app) .post(ENDPOINT) + .set(auth) .field("filename", "some_file.txt") .attach("file", Buffer.from("some data"), "some_file.txt") .then((res) => { @@ -91,6 +107,7 @@ describe("File upload", () => { await supertest(app) .post("/file/private/upload") + .set(auth) .field("filename", "some_file.txt") .attach("file", Buffer.from("some data"), "some_file.txt") .expect(500) diff --git a/api.planx.uk/modules/file/routes.ts b/api.planx.uk/modules/file/routes.ts index 1a0e26ce1d..239687f474 100644 --- a/api.planx.uk/modules/file/routes.ts +++ b/api.planx.uk/modules/file/routes.ts @@ -1,7 +1,7 @@ import { Router } from "express"; import multer from "multer"; -import { useFilePermission } from "../auth/middleware"; +import { useFilePermission, useTeamEditorAuth } from "../auth/middleware"; import { downloadFileSchema, privateDownloadController, @@ -15,17 +15,18 @@ import { validate } from "../../shared/middleware/validate"; const router = Router(); router.post( - "/private/upload", + "/public/upload", multer().single("file"), validate(uploadFileSchema), - privateUploadController, + publicUploadController, ); router.post( - "/public/upload", + "/private/upload", multer().single("file"), + useTeamEditorAuth, validate(uploadFileSchema), - publicUploadController, + privateUploadController, ); router.get( From d5a9bd70e2fbfd35742c0526f6ec48e73ba7e4aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dafydd=20Ll=C5=B7r=20Pearson?= Date: Tue, 14 Nov 2023 10:48:40 +0000 Subject: [PATCH 6/6] fix: tsc compilation issues --- api.planx.uk/modules/file/controller.ts | 3 +-- api.planx.uk/modules/file/service/getFile.ts | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/api.planx.uk/modules/file/controller.ts b/api.planx.uk/modules/file/controller.ts index 0e718e133e..206750bb19 100644 --- a/api.planx.uk/modules/file/controller.ts +++ b/api.planx.uk/modules/file/controller.ts @@ -5,7 +5,6 @@ import { getFileFromS3 } from "./service/getFile"; import { z } from "zod"; import { ValidatedRequestHandler } from "../../shared/middleware/validate"; import { ServerError } from "../../errors"; -import { S3 } from "aws-sdk"; assert(process.env.AWS_S3_BUCKET); assert(process.env.AWS_S3_REGION); @@ -73,7 +72,7 @@ export const downloadFileSchema = z.object({ export type DownloadController = ValidatedRequestHandler< typeof downloadFileSchema, - S3.Body | undefined + Buffer | undefined >; export const publicDownloadController: DownloadController = async ( diff --git a/api.planx.uk/modules/file/service/getFile.ts b/api.planx.uk/modules/file/service/getFile.ts index b9761cd8d1..29b3c539cd 100644 --- a/api.planx.uk/modules/file/service/getFile.ts +++ b/api.planx.uk/modules/file/service/getFile.ts @@ -11,7 +11,7 @@ export const getFileFromS3 = async (fileId: string) => { const file = await s3.getObject(params).promise(); return { - body: file.Body, + body: file.Body as Buffer, isPrivate: file.Metadata?.is_private === "true", headers: { "Content-Type": file.ContentType,