From 9cd26168e1168acadeaaa84acbbeb09c0f40480e Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 3 Jul 2022 18:19:56 +0200 Subject: [PATCH] Separates policy for file operations --- app/hooks/useAuthorizedSettingsConfig.ts | 8 +-- server/policies/fileOperation.ts | 21 +++++++ server/policies/index.ts | 12 +++- server/policies/team.ts | 2 +- server/routes/api/collections.ts | 4 +- server/routes/api/fileOperations.test.ts | 75 ++++++++++++++---------- server/routes/api/fileOperations.ts | 53 ++++++----------- server/utils/__mocks__/s3.ts | 4 ++ server/validation.ts | 2 +- 9 files changed, 108 insertions(+), 73 deletions(-) create mode 100644 server/policies/fileOperation.ts diff --git a/app/hooks/useAuthorizedSettingsConfig.ts b/app/hooks/useAuthorizedSettingsConfig.ts index b542ee6966c5..47aafc5722cf 100644 --- a/app/hooks/useAuthorizedSettingsConfig.ts +++ b/app/hooks/useAuthorizedSettingsConfig.ts @@ -149,7 +149,7 @@ const useAuthorizedSettingsConfig = () => { name: t("Import"), path: "/settings/import", component: Import, - enabled: can.manage, + enabled: can.createImport, group: t("Team"), icon: NewDocumentIcon, }, @@ -157,7 +157,7 @@ const useAuthorizedSettingsConfig = () => { name: t("Export"), path: "/settings/export", component: Export, - enabled: can.export, + enabled: can.createExport, group: t("Team"), icon: DownloadIcon, }, @@ -190,8 +190,8 @@ const useAuthorizedSettingsConfig = () => { [ can.createApiKey, can.createWebhookSubscription, - can.export, - can.manage, + can.createExport, + can.createImport, can.update, t, ] diff --git a/server/policies/fileOperation.ts b/server/policies/fileOperation.ts new file mode 100644 index 000000000000..d22cf73e0d71 --- /dev/null +++ b/server/policies/fileOperation.ts @@ -0,0 +1,21 @@ +import { User, Team, FileOperation } from "@server/models"; +import { allow } from "./cancan"; + +allow( + User, + ["createFileOperation", "createImport", "createExport"], + Team, + (user, team) => { + if (!team || user.isViewer || user.teamId !== team.id) { + return false; + } + return user.isAdmin; + } +); + +allow(User, ["read", "delete"], FileOperation, (user, fileOperation) => { + if (!fileOperation || user.isViewer || user.teamId !== fileOperation.teamId) { + return false; + } + return user.isAdmin; +}); diff --git a/server/policies/index.ts b/server/policies/index.ts index 22147b4b7f8b..9f65f5dabd85 100644 --- a/server/policies/index.ts +++ b/server/policies/index.ts @@ -1,5 +1,6 @@ import { Attachment, + FileOperation, Team, User, Collection, @@ -12,6 +13,7 @@ import "./attachment"; import "./authenticationProvider"; import "./collection"; import "./document"; +import "./fileOperation"; import "./integration"; import "./notificationSetting"; import "./pins"; @@ -42,7 +44,15 @@ export const abilities = _abilities; */ export function serialize( model: User, - target: Attachment | Team | Collection | Document | User | Group | null + target: + | Attachment + | FileOperation + | Team + | Collection + | Document + | User + | Group + | null ): Policy { const output = {}; abilities.forEach((ability) => { diff --git a/server/policies/team.ts b/server/policies/team.ts index 63f5b72b6ecb..f517b66f8ef0 100644 --- a/server/policies/team.ts +++ b/server/policies/team.ts @@ -10,7 +10,7 @@ allow(User, "share", Team, (user, team) => { return team.sharing; }); -allow(User, ["update", "export", "manage"], Team, (user, team) => { +allow(User, ["update", "manage"], Team, (user, team) => { if (!team || user.isViewer || user.teamId !== team.id) { return false; } diff --git a/server/routes/api/collections.ts b/server/routes/api/collections.ts index 60e59d41cab7..bc03264d24c2 100644 --- a/server/routes/api/collections.ts +++ b/server/routes/api/collections.ts @@ -488,7 +488,7 @@ router.post("collections.export", auth(), async (ctx) => { assertUuid(id, "id is required"); const { user } = ctx.state; const team = await Team.findByPk(user.teamId); - authorize(user, "export", team); + authorize(user, "createExport", team); const collection = await Collection.scope({ method: ["withMembership", user.id], @@ -516,7 +516,7 @@ router.post("collections.export", auth(), async (ctx) => { router.post("collections.export_all", auth(), async (ctx) => { const { user } = ctx.state; const team = await Team.findByPk(user.teamId); - authorize(user, "export", team); + authorize(user, "createExport", team); const fileOperation = await sequelize.transaction(async (transaction) => { return collectionExporter({ diff --git a/server/routes/api/fileOperations.test.ts b/server/routes/api/fileOperations.test.ts index 75b7e06f7220..2b4b1d5c863c 100644 --- a/server/routes/api/fileOperations.test.ts +++ b/server/routes/api/fileOperations.test.ts @@ -17,6 +17,8 @@ import { flushdb } from "@server/test/support"; const app = webService(); const server = new TestServer(app.callback()); +jest.mock("@server/utils/s3"); + beforeEach(() => flushdb()); afterAll(() => server.close()); @@ -35,7 +37,6 @@ describe("#fileOperations.info", () => { body: { id: exportData.id, token: admin.getJwtToken(), - type: FileOperationType.Export, }, }); const body = await res.json(); @@ -61,7 +62,26 @@ describe("#fileOperations.info", () => { body: { id: exportData.id, token: user.getJwtToken(), - type: FileOperationType.Export, + }, + }); + expect(res.status).toEqual(403); + }); + + it("should require authorization", async () => { + const team = await buildTeam(); + const admin = await buildAdmin(); + await buildUser({ + teamId: team.id, + }); + const exportData = await buildFileOperation({ + type: FileOperationType.Export, + teamId: team.id, + userId: admin.id, + }); + const res = await server.post("/api/fileOperations.info", { + body: { + id: exportData.id, + token: admin.getJwtToken(), }, }); expect(res.status).toEqual(403); @@ -196,7 +216,7 @@ describe("#fileOperations.list", () => { expect(data.user.id).toBe(admin.id); }); - it("should require authorization", async () => { + it("should require admin", async () => { const user = await buildUser(); const res = await server.post("/api/fileOperations.list", { body: { @@ -229,74 +249,69 @@ describe("#fileOperations.redirect", () => { expect(res.status).toEqual(400); expect(body.message).toEqual("export is not complete yet"); }); -}); -describe("#fileOperations.info", () => { - it("should return file operation", async () => { + it("should require authorization", async () => { const team = await buildTeam(); - const admin = await buildAdmin({ + const user = await buildUser({ teamId: team.id, }); + const admin = await buildAdmin(); const exportData = await buildFileOperation({ + state: FileOperationState.Complete, type: FileOperationType.Export, teamId: team.id, - userId: admin.id, + userId: user.id, }); - const res = await server.post("/api/fileOperations.info", { + const res = await server.post("/api/fileOperations.redirect", { body: { token: admin.getJwtToken(), id: exportData.id, }, }); - const body = await res.json(); - expect(res.status).toBe(200); - expect(body.data.id).toBe(exportData.id); - expect(body.data.user.id).toBe(admin.id); + expect(res.status).toEqual(403); }); +}); - it("should require authorization", async () => { +describe("#fileOperations.delete", () => { + it("should delete file operation", async () => { const team = await buildTeam(); const admin = await buildAdmin({ teamId: team.id, }); - const user = await buildUser({ - teamId: team.id, - }); const exportData = await buildFileOperation({ type: FileOperationType.Export, teamId: team.id, userId: admin.id, + state: FileOperationState.Complete, }); - const res = await server.post("/api/fileOperations.info", { + const deleteResponse = await server.post("/api/fileOperations.delete", { body: { - token: user.getJwtToken(), + token: admin.getJwtToken(), id: exportData.id, }, }); - expect(res.status).toBe(403); + expect(deleteResponse.status).toBe(200); + expect(await Event.count()).toBe(1); + expect(await FileOperation.count()).toBe(0); }); -}); -describe("#fileOperations.delete", () => { - it("should delete file operation", async () => { + it("should require authorization", async () => { const team = await buildTeam(); - const admin = await buildAdmin({ + const user = await buildUser({ teamId: team.id, }); + const admin = await buildAdmin(); const exportData = await buildFileOperation({ type: FileOperationType.Export, teamId: team.id, - userId: admin.id, - state: FileOperationState.Complete, + userId: user.id, }); - const deleteResponse = await server.post("/api/fileOperations.delete", { + const res = await server.post("/api/fileOperations.delete", { body: { token: admin.getJwtToken(), id: exportData.id, }, }); - expect(deleteResponse.status).toBe(200); - expect(await Event.count()).toBe(1); - expect(await FileOperation.count()).toBe(0); + expect(res.status).toEqual(403); }); }); diff --git a/server/routes/api/fileOperations.ts b/server/routes/api/fileOperations.ts index 09e26546dfb7..df10d8ab06f9 100644 --- a/server/routes/api/fileOperations.ts +++ b/server/routes/api/fileOperations.ts @@ -1,13 +1,14 @@ import Router from "koa-router"; import { WhereOptions } from "sequelize/types"; import fileOperationDeleter from "@server/commands/fileOperationDeleter"; -import { NotFoundError, ValidationError } from "@server/errors"; +import { ValidationError } from "@server/errors"; import auth from "@server/middlewares/authentication"; import { FileOperation, Team } from "@server/models"; +import { FileOperationType } from "@server/models/FileOperation"; import { authorize } from "@server/policies"; import { presentFileOperation } from "@server/presenters"; import { getSignedUrl } from "@server/utils/s3"; -import { assertPresent, assertIn, assertUuid } from "@server/validation"; +import { assertIn, assertSort, assertUuid } from "@server/validation"; import pagination from "./middlewares/pagination"; const router = new Router(); @@ -16,16 +17,11 @@ router.post("fileOperations.info", auth(), async (ctx) => { const { id } = ctx.body; assertUuid(id, "id is required"); const { user } = ctx.state; - const team = await Team.findByPk(user.teamId); const fileOperation = await FileOperation.findByPk(id, { rejectOnEmpty: true, }); - authorize(user, fileOperation.type, team); - - if (!fileOperation) { - throw NotFoundError(); - } + authorize(user, "read", fileOperation); ctx.body = { data: presentFileOperation(fileOperation), @@ -35,12 +31,8 @@ router.post("fileOperations.info", auth(), async (ctx) => { router.post("fileOperations.list", auth(), pagination(), async (ctx) => { let { direction } = ctx.body; const { sort = "createdAt", type } = ctx.body; - assertPresent(type, "type is required"); - assertIn( - type, - ["import", "export"], - "type must be one of 'import' or 'export'" - ); + assertIn(type, Object.values(FileOperationType)); + assertSort(sort, FileOperation); if (direction !== "ASC") { direction = "DESC"; @@ -51,7 +43,7 @@ router.post("fileOperations.list", auth(), pagination(), async (ctx) => { type, }; const team = await Team.findByPk(user.teamId); - authorize(user, type, team); + authorize(user, "manage", team); const [exports, total] = await Promise.all([ await FileOperation.findAll({ @@ -76,20 +68,16 @@ router.post("fileOperations.redirect", auth(), async (ctx) => { assertUuid(id, "id is required"); const { user } = ctx.state; - const team = await Team.findByPk(user.teamId); - const fileOp = await FileOperation.unscoped().findByPk(id); - - if (!fileOp) { - throw NotFoundError(); - } - - authorize(user, fileOp.type, team); + const fileOperation = await FileOperation.unscoped().findByPk(id, { + rejectOnEmpty: true, + }); + authorize(user, "read", fileOperation); - if (fileOp.state !== "complete") { - throw ValidationError(`${fileOp.type} is not complete yet`); + if (fileOperation.state !== "complete") { + throw ValidationError(`${fileOperation.type} is not complete yet`); } - const accessUrl = await getSignedUrl(fileOp.key); + const accessUrl = await getSignedUrl(fileOperation.key); ctx.redirect(accessUrl); }); @@ -98,15 +86,12 @@ router.post("fileOperations.delete", auth(), async (ctx) => { assertUuid(id, "id is required"); const { user } = ctx.state; - const team = await Team.findByPk(user.teamId); - const fileOp = await FileOperation.findByPk(id); - - if (!fileOp) { - throw NotFoundError(); - } + const fileOperation = await FileOperation.unscoped().findByPk(id, { + rejectOnEmpty: true, + }); + authorize(user, "delete", fileOperation); - authorize(user, fileOp.type, team); - await fileOperationDeleter(fileOp, user, ctx.request.ip); + await fileOperationDeleter(fileOperation, user, ctx.request.ip); ctx.body = { success: true, diff --git a/server/utils/__mocks__/s3.ts b/server/utils/__mocks__/s3.ts index c4f0fea03665..7caa2cf698c6 100644 --- a/server/utils/__mocks__/s3.ts +++ b/server/utils/__mocks__/s3.ts @@ -1,3 +1,7 @@ export const uploadToS3FromBuffer = jest.fn().mockReturnValue("/endpoint/key"); export const publicS3Endpoint = jest.fn().mockReturnValue("http://mock"); + +export const getSignedUrl = jest.fn().mockReturnValue("http://s3mock"); + +export const getSignedUrlPromise = jest.fn().mockResolvedValue("http://s3mock"); diff --git a/server/validation.ts b/server/validation.ts index 211cb960bcb0..55973c5dd94c 100644 --- a/server/validation.ts +++ b/server/validation.ts @@ -22,7 +22,7 @@ export const assertIn = ( message?: string ) => { if (!options.includes(value)) { - throw ValidationError(message); + throw ValidationError(message ?? `Must be one of ${options.join(", ")}`); } };