From b96f44566da9d797f17c98874a6536739f49b584 Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Sat, 4 Nov 2023 21:59:51 +0000 Subject: [PATCH 1/2] Separate file actions to their own authorisation function --- .../src/connectors/v2/authorisation/Base.ts | 6 +++-- backend/src/services/v2/file.ts | 23 +++++++++---------- .../services/__snapshots__/file.spec.ts.snap | 2 ++ backend/test/services/file.spec.ts | 19 +++++++++++---- 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/backend/src/connectors/v2/authorisation/Base.ts b/backend/src/connectors/v2/authorisation/Base.ts index 5f155fb3f..be830a530 100644 --- a/backend/src/connectors/v2/authorisation/Base.ts +++ b/backend/src/connectors/v2/authorisation/Base.ts @@ -11,8 +11,6 @@ export const ModelAction = { Create: 'create', View: 'view', Update: 'update', - DeleteFile: 'delete_file', - UploadFile: 'upload_file', Write: 'write', } as const export type ModelActionKeys = (typeof ModelAction)[keyof typeof ModelAction] @@ -38,6 +36,10 @@ export const SchemaAction = { export type SchemaActionKeys = (typeof SchemaAction)[keyof typeof SchemaAction] export const FileAction = { + Delete: 'delete', + Upload: 'upload', + // 'view' refers to the ability to see metadata about the file. 'download' lets the user view the file contents. + View: 'view', Download: 'download', } export type FileActionKeys = (typeof FileAction)[keyof typeof FileAction] diff --git a/backend/src/services/v2/file.ts b/backend/src/services/v2/file.ts index 548236986..84001713d 100644 --- a/backend/src/services/v2/file.ts +++ b/backend/src/services/v2/file.ts @@ -1,5 +1,5 @@ import { getObjectStream, putObjectStream } from '../../clients/s3.js' -import { FileAction, ModelAction } from '../../connectors/v2/authorisation/Base.js' +import { FileAction } from '../../connectors/v2/authorisation/Base.js' import authorisation from '../../connectors/v2/authorisation/index.js' import FileModel from '../../models/v2/File.js' import { UserDoc } from '../../models/v2/User.js' @@ -11,27 +11,27 @@ import { getModelById } from './model.js' export async function uploadFile(user: UserDoc, modelId: string, name: string, mime: string, stream: ReadableStream) { const model = await getModelById(user, modelId) - if (!(await authorisation.userModelAction(user, model, ModelAction.UploadFile))) { - throw Forbidden(`You do not have permission to upload a file to this model.`, { userDn: user.dn }) - } - const fileId = longId() const bucket = config.s3.buckets.uploads const path = `beta/model/${modelId}/files/${fileId}` - const { fileSize } = await putObjectStream(bucket, path, stream) - const file = new FileModel({ modelId, name, mime, bucket, path, - size: fileSize, complete: true, }) + if (!(await authorisation.userFileAction(user, model, file, FileAction.Upload))) { + throw Forbidden(`You do not have permission to upload a file to this model.`, { userDn: user.dn }) + } + + const { fileSize } = await putObjectStream(bucket, path, stream) + file.size = fileSize + await file.save() return file @@ -60,7 +60,7 @@ export async function getFileById(user: UserDoc, fileId: string) { const model = await getModelById(user, file.modelId) - if (!(await authorisation.userModelAction(user, model, ModelAction.View))) { + if (!(await authorisation.userFileAction(user, model, file, FileAction.View))) { throw Forbidden(`You do not have permission to get this file.`, { userDn: user.dn, fileId }) } @@ -76,13 +76,12 @@ export async function getFilesByModel(user: UserDoc, modelId: string) { export async function removeFile(user: UserDoc, modelId: string, fileId: string) { const model = await getModelById(user, modelId) + const file = await getFileById(user, fileId) - if (!(await authorisation.userModelAction(user, model, ModelAction.DeleteFile))) { + if (!(await authorisation.userFileAction(user, model, file, FileAction.Delete))) { throw Forbidden(`You do not have permission to delete a file from this model.`, { userDn: user.dn }) } - const file = await getFileById(user, fileId) - // We don't actually remove the file from storage, we only hide all // references to it. This makes the file not visible to the user. await file.delete() diff --git a/backend/test/services/__snapshots__/file.spec.ts.snap b/backend/test/services/__snapshots__/file.spec.ts.snap index 403c9ffb3..7c1b19912 100644 --- a/backend/test/services/__snapshots__/file.spec.ts.snap +++ b/backend/test/services/__snapshots__/file.spec.ts.snap @@ -20,6 +20,7 @@ exports[`services > file > getFilesByModel > success 1`] = ` }, "findOne": [MockFunction spy], "save": [MockFunction spy], + "size": 100, } `; @@ -53,5 +54,6 @@ exports[`services > file > removeFile > success 1`] = ` ], }, "save": [MockFunction spy], + "size": 100, } `; diff --git a/backend/test/services/file.spec.ts b/backend/test/services/file.spec.ts index ec0b78c16..2697ecbe4 100644 --- a/backend/test/services/file.spec.ts +++ b/backend/test/services/file.spec.ts @@ -1,6 +1,7 @@ import { Readable } from 'stream' import { describe, expect, test, vi } from 'vitest' +import { FileAction } from '../../src/connectors/v2/authorisation/Base.js' import { UserDoc } from '../../src/models/v2/User.js' import { downloadFile, getFilesByModel, removeFile, uploadFile } from '../../src/services/v2/file.js' @@ -14,7 +15,7 @@ vi.mock('../../src/clients/s3.js', () => s3Mocks) const authorisationMocks = vi.hoisted(() => ({ userModelAction: vi.fn(() => true), - userFileAction: vi.fn(() => true), + userFileAction: vi.fn((_user, _model, _file, _action) => true), })) vi.mock('../../src/connectors/v2/authorisation/index.js', async () => ({ default: authorisationMocks, @@ -57,7 +58,7 @@ describe('services > file', () => { }) test('uploadFile > no permission', async () => { - authorisationMocks.userModelAction.mockResolvedValueOnce(false) + authorisationMocks.userFileAction.mockResolvedValueOnce(false) expect(() => uploadFile({} as any, 'modelId', 'name', 'mime', new Readable() as any)).rejects.toThrowError( /^You do not have permission to upload a file to this model./, @@ -75,7 +76,13 @@ describe('services > file', () => { }) test('removeFile > no permission', async () => { - authorisationMocks.userModelAction.mockResolvedValueOnce(false) + authorisationMocks.userModelAction.mockResolvedValueOnce(true) + authorisationMocks.userFileAction.mockImplementation((_user, _model, _file, action) => { + if (action === FileAction.View) return true + if (action === FileAction.Delete) return false + + return false + }) const user = { dn: 'testUser' } as UserDoc const modelId = 'testModelId' @@ -121,8 +128,12 @@ describe('services > file', () => { const fileId = 'testFileId' const range = { start: 0, end: 50 } - authorisationMocks.userFileAction.mockResolvedValueOnce(false) + authorisationMocks.userFileAction.mockImplementation((_user, _model, _file, action) => { + if (action === FileAction.View) return true + if (action === FileAction.Download) return false + return false + }) await expect(downloadFile(user, fileId, range)).rejects.toThrowError( /^You do not have permission to download this model./, ) From 5113b43bf62a57e31d91b09bddccdc767286969c Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Sat, 4 Nov 2023 22:13:00 +0000 Subject: [PATCH 2/2] Add in file authorisation checks for 'getFilesByModel' --- backend/src/services/v2/file.ts | 7 +++--- .../services/__snapshots__/file.spec.ts.snap | 24 ++++--------------- backend/test/services/file.spec.ts | 16 ++++++++----- 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/backend/src/services/v2/file.ts b/backend/src/services/v2/file.ts index 84001713d..e79bae981 100644 --- a/backend/src/services/v2/file.ts +++ b/backend/src/services/v2/file.ts @@ -3,6 +3,7 @@ import { FileAction } from '../../connectors/v2/authorisation/Base.js' import authorisation from '../../connectors/v2/authorisation/index.js' import FileModel from '../../models/v2/File.js' import { UserDoc } from '../../models/v2/User.js' +import { asyncFilter } from '../../utils/general.js' import config from '../../utils/v2/config.js' import { Forbidden, NotFound } from '../../utils/v2/error.js' import { longId } from '../../utils/v2/id.js' @@ -68,10 +69,10 @@ export async function getFileById(user: UserDoc, fileId: string) { } export async function getFilesByModel(user: UserDoc, modelId: string) { - await getModelById(user, modelId) - + const model = await getModelById(user, modelId) const files = await FileModel.find({ modelId }) - return files + + return asyncFilter(files, (file) => authorisation.userFileAction(user, model, file, FileAction.View)) } export async function removeFile(user: UserDoc, modelId: string, fileId: string) { diff --git a/backend/test/services/__snapshots__/file.spec.ts.snap b/backend/test/services/__snapshots__/file.spec.ts.snap index 7c1b19912..39f66259f 100644 --- a/backend/test/services/__snapshots__/file.spec.ts.snap +++ b/backend/test/services/__snapshots__/file.spec.ts.snap @@ -1,27 +1,11 @@ // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html exports[`services > file > getFilesByModel > success 1`] = ` -{ - "delete": [MockFunction spy], - "find": [MockFunction spy] { - "calls": [ - [ - { - "modelId": "testModelId", - }, - ], - ], - "results": [ - { - "type": "return", - "value": [Circular], - }, - ], +[ + { + "example": "file", }, - "findOne": [MockFunction spy], - "save": [MockFunction spy], - "size": 100, -} +] `; exports[`services > file > removeFile > success 1`] = ` diff --git a/backend/test/services/file.spec.ts b/backend/test/services/file.spec.ts index 2697ecbe4..594c685aa 100644 --- a/backend/test/services/file.spec.ts +++ b/backend/test/services/file.spec.ts @@ -96,6 +96,8 @@ describe('services > file', () => { }) test('getFilesByModel > success', async () => { + fileModelMocks.find.mockResolvedValueOnce([{ example: 'file' }]) + const user = { dn: 'testUser' } as UserDoc const modelId = 'testModelId' @@ -104,14 +106,16 @@ describe('services > file', () => { expect(result).toMatchSnapshot() }) - // test('getFilesByModel > no permission', async () => { - // authorisationMocks.userModelAction.mockResolvedValueOnce(false) + test('getFilesByModel > no permission', async () => { + authorisationMocks.userFileAction.mockResolvedValue(false) + fileModelMocks.find.mockResolvedValueOnce([{ example: 'file' }]) - // const user = { dn: 'testUser' } as UserDoc - // const modelId = 'testModelId' + const user = { dn: 'testUser' } as UserDoc + const modelId = 'testModelId' - // expect(() => getFilesByModel(user, modelId)).rejects.toThrowError(/^You do not have permission to get these files./) - // }) + const files = await getFilesByModel(user, modelId) + expect(files).toStrictEqual([]) + }) test('downloadFile > success', async () => { const user = { dn: 'testUser' } as UserDoc