From 318e1df13b222bd062b0ea51a45e5e0336fc8795 Mon Sep 17 00:00:00 2001 From: Mohamed ELIDRISSI <67818913+elidrissidev@users.noreply.github.com> Date: Fri, 30 Dec 2022 18:49:01 +0100 Subject: [PATCH] refactor: add server side validation schema for attachments (#4606) * refactor: schema for attachments.create * refactor: schema for attachments.delete * refactor: remove deprecated "public" request param --- .../api/{ => attachments}/attachments.test.ts | 42 --------- .../api/{ => attachments}/attachments.ts | 86 ++++++++----------- server/routes/api/attachments/index.ts | 1 + server/routes/api/attachments/schema.ts | 28 ++++++ 4 files changed, 67 insertions(+), 90 deletions(-) rename server/routes/api/{ => attachments}/attachments.test.ts (90%) rename server/routes/api/{ => attachments}/attachments.ts (71%) create mode 100644 server/routes/api/attachments/index.ts create mode 100644 server/routes/api/attachments/schema.ts diff --git a/server/routes/api/attachments.test.ts b/server/routes/api/attachments/attachments.test.ts similarity index 90% rename from server/routes/api/attachments.test.ts rename to server/routes/api/attachments/attachments.test.ts index 75891db5a..a71c37479 100644 --- a/server/routes/api/attachments.test.ts +++ b/server/routes/api/attachments/attachments.test.ts @@ -22,20 +22,6 @@ describe("#attachments.create", () => { }); describe("member", () => { - it("should allow simple image upload for public attachments", async () => { - const user = await buildUser(); - const res = await server.post("/api/attachments.create", { - body: { - name: "test.png", - contentType: "image/png", - size: 1000, - public: true, - token: user.getJwtToken(), - }, - }); - expect(res.status).toEqual(200); - }); - it("should allow upload using avatar preset", async () => { const user = await buildUser(); const res = await server.post("/api/attachments.create", { @@ -89,20 +75,6 @@ describe("#attachments.create", () => { expect(attachment!.expiresAt).toBeTruthy(); }); - it("should not allow file upload for public attachments", async () => { - const user = await buildUser(); - const res = await server.post("/api/attachments.create", { - body: { - name: "test.pdf", - contentType: "application/pdf", - size: 1000, - public: true, - token: user.getJwtToken(), - }, - }); - expect(res.status).toEqual(400); - }); - it("should not allow attachment creation for other documents", async () => { const user = await buildUser(); const document = await buildDocument(); @@ -136,20 +108,6 @@ describe("#attachments.create", () => { }); describe("viewer", () => { - it("should allow simple image upload for public attachments", async () => { - const user = await buildViewer(); - const res = await server.post("/api/attachments.create", { - body: { - name: "test.png", - contentType: "image/png", - size: 1000, - public: true, - token: user.getJwtToken(), - }, - }); - expect(res.status).toEqual(200); - }); - it("should allow attachment creation for documents in collections with edit access", async () => { const user = await buildViewer(); const collection = await buildCollection({ diff --git a/server/routes/api/attachments.ts b/server/routes/api/attachments/attachments.ts similarity index 71% rename from server/routes/api/attachments.ts rename to server/routes/api/attachments/attachments.ts index 37eac2d25..d74e083d3 100644 --- a/server/routes/api/attachments.ts +++ b/server/routes/api/attachments/attachments.ts @@ -5,46 +5,32 @@ import { bytesToHumanReadable } from "@shared/utils/files"; import { AttachmentValidation } from "@shared/validations"; import { AuthorizationError, ValidationError } from "@server/errors"; import auth from "@server/middlewares/authentication"; -import { - transaction, - TransactionContext, -} from "@server/middlewares/transaction"; +import { transaction } from "@server/middlewares/transaction"; +import validate from "@server/middlewares/validate"; import { Attachment, Document, Event } from "@server/models"; import AttachmentHelper from "@server/models/helpers/AttachmentHelper"; import { authorize } from "@server/policies"; import { presentAttachment } from "@server/presenters"; -import { ContextWithState } from "@server/types"; +import { APIContext, ContextWithState } from "@server/types"; import { getPresignedPost, publicS3Endpoint } from "@server/utils/s3"; -import { assertIn, assertPresent, assertUuid } from "@server/validation"; +import { assertIn, assertUuid } from "@server/validation"; +import * as T from "./schema"; const router = new Router(); router.post( "attachments.create", auth(), + validate(T.AttachmentsCreateSchema), transaction(), - async (ctx: TransactionContext) => { - const { - name, - documentId, - contentType = "application/octet-stream", - size, - // 'public' is now deprecated and can be removed on December 1 2022. - public: isPublicDeprecated, - preset = isPublicDeprecated - ? AttachmentPreset.Avatar - : AttachmentPreset.DocumentAttachment, - } = ctx.request.body; + async (ctx: APIContext) => { + const { name, documentId, contentType, size, preset } = ctx.input; const { user, transaction } = ctx.state; - assertPresent(name, "name is required"); - assertPresent(size, "size is required"); - // All user types can upload an avatar so no additional authorization is needed. if (preset === AttachmentPreset.Avatar) { assertIn(contentType, AttachmentValidation.avatarContentTypes); } else if (preset === AttachmentPreset.DocumentAttachment && documentId) { - assertUuid(documentId, "documentId must be a uuid"); const document = await Document.findByPk(documentId, { userId: user.id, }); @@ -121,34 +107,38 @@ router.post( } ); -router.post("attachments.delete", auth(), async (ctx) => { - const { id } = ctx.request.body; - assertUuid(id, "id is required"); - const { user } = ctx.state; - const attachment = await Attachment.findByPk(id, { - rejectOnEmpty: true, - }); - - if (attachment.documentId) { - const document = await Document.findByPk(attachment.documentId, { - userId: user.id, +router.post( + "attachments.delete", + auth(), + validate(T.AttachmentDeleteSchema), + async (ctx: APIContext) => { + const { id } = ctx.input; + const { user } = ctx.state; + const attachment = await Attachment.findByPk(id, { + rejectOnEmpty: true, }); - authorize(user, "update", document); + + if (attachment.documentId) { + const document = await Document.findByPk(attachment.documentId, { + userId: user.id, + }); + authorize(user, "update", document); + } + + authorize(user, "delete", attachment); + await attachment.destroy(); + await Event.create({ + name: "attachments.delete", + teamId: user.teamId, + actorId: user.id, + ip: ctx.request.ip, + }); + + ctx.body = { + success: true, + }; } - - authorize(user, "delete", attachment); - await attachment.destroy(); - await Event.create({ - name: "attachments.delete", - teamId: user.teamId, - actorId: user.id, - ip: ctx.request.ip, - }); - - ctx.body = { - success: true, - }; -}); +); const handleAttachmentsRedirect = async (ctx: ContextWithState) => { const id = ctx.request.body?.id ?? ctx.request.query?.id; diff --git a/server/routes/api/attachments/index.ts b/server/routes/api/attachments/index.ts new file mode 100644 index 000000000..5e6ca6cb7 --- /dev/null +++ b/server/routes/api/attachments/index.ts @@ -0,0 +1 @@ +export { default } from "./attachments"; diff --git a/server/routes/api/attachments/schema.ts b/server/routes/api/attachments/schema.ts new file mode 100644 index 000000000..763960877 --- /dev/null +++ b/server/routes/api/attachments/schema.ts @@ -0,0 +1,28 @@ +import { z } from "zod"; +import { AttachmentPreset } from "@shared/types"; + +export const AttachmentsCreateSchema = z.object({ + /** Attachment name */ + name: z.string(), + + /** Id of the document to which the Attachment belongs */ + documentId: z.string().uuid().optional(), + + /** File size of the Attachment */ + size: z.number(), + + /** Content-Type of the Attachment */ + contentType: z.string().optional().default("application/octet-stream"), + + /** Attachment type */ + preset: z.nativeEnum(AttachmentPreset), +}); + +export type AttachmentCreateReq = z.infer; + +export const AttachmentDeleteSchema = z.object({ + /** Id of the attachment to be deleted */ + id: z.string().uuid(), +}); + +export type AttachmentDeleteReq = z.infer;