From a8cdd63b445a7d8837c8c0069348ee0e0569e462 Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Fri, 27 Oct 2023 16:07:18 +0000 Subject: [PATCH 1/4] Add an endpoint to download a file --- backend/package-lock.json | 4 +- backend/package.json | 1 + backend/src/clients/s3.ts | 20 ++++- .../src/connectors/v2/authorisation/Base.ts | 21 ++++- .../src/connectors/v2/authorisation/silly.ts | 83 ++++++++++++++++++- backend/src/routes.ts | 2 + backend/src/routes/v1/registryAuth.ts | 32 +------ .../routes/v2/model/file/getDownloadFile.ts | 56 +++++++++++++ .../minimal_upload_schema_beta.json | 3 +- backend/src/services/v2/file.ts | 16 +++- backend/src/utils/v2/error.ts | 4 + 11 files changed, 206 insertions(+), 36 deletions(-) create mode 100644 backend/src/routes/v2/model/file/getDownloadFile.ts diff --git a/backend/package-lock.json b/backend/package-lock.json index 5342ea911..051b53e43 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -19,6 +19,7 @@ "chalk": "^5.2.0", "config": "^3.3.9", "connect-mongo": "^5.0.0", + "content-disposition": "^0.5.4", "cross-fetch": "^3.1.8", "dedent-js": "^1.0.1", "dev-null": "^0.1.1", @@ -6046,7 +6047,8 @@ }, "node_modules/content-disposition": { "version": "0.5.4", - "license": "MIT", + "resolved": "https://registry.npmjs.org/content-disposition/-/content-disposition-0.5.4.tgz", + "integrity": "sha512-FveZTNuGw04cxlAiWbzi6zTAL/lhehaWbTtgluJh4/E95DqMwTmha3KZN1aAWA8cFIhHzMZUvLevkw5Rqk+tSQ==", "dependencies": { "safe-buffer": "5.2.1" }, diff --git a/backend/package.json b/backend/package.json index 267768601..415e47460 100644 --- a/backend/package.json +++ b/backend/package.json @@ -29,6 +29,7 @@ "chalk": "^5.2.0", "config": "^3.3.9", "connect-mongo": "^5.0.0", + "content-disposition": "^0.5.4", "cross-fetch": "^3.1.8", "dedent-js": "^1.0.1", "dev-null": "^0.1.1", diff --git a/backend/src/clients/s3.ts b/backend/src/clients/s3.ts index e0a8119c6..4785a6695 100644 --- a/backend/src/clients/s3.ts +++ b/backend/src/clients/s3.ts @@ -1,4 +1,4 @@ -import { S3Client } from '@aws-sdk/client-s3' +import { GetObjectCommand, GetObjectRequest, S3Client } from '@aws-sdk/client-s3' import { Upload } from '@aws-sdk/lib-storage' import config from '../utils/v2/config.js' @@ -31,3 +31,21 @@ export async function putObjectStream(bucket: string, key: string, body: Readabl fileSize, } } + +export async function getObjectStream(bucket: string, key: string, range?: { start: number; end: number }) { + const client = await getS3Client() + + const input: GetObjectRequest = { + Bucket: bucket, + Key: key, + } + + if (range) { + input.Range = `bytes=${range.start}-${range.end}` + } + + const command = new GetObjectCommand(input) + const response = await client.send(command) + + return response +} diff --git a/backend/src/connectors/v2/authorisation/Base.ts b/backend/src/connectors/v2/authorisation/Base.ts index c7ed771a9..075e3a9c5 100644 --- a/backend/src/connectors/v2/authorisation/Base.ts +++ b/backend/src/connectors/v2/authorisation/Base.ts @@ -1,7 +1,9 @@ import { AccessRequestDoc } from '../../../models/v2/AccessRequest.js' +import { FileInterfaceDoc } from '../../../models/v2/File.js' import { ModelDoc, ModelVisibility } from '../../../models/v2/Model.js' import { ReleaseDoc } from '../../../models/v2/Release.js' import { UserDoc } from '../../../models/v2/User.js' +import { Access } from '../../../routes/v1/registryAuth.js' import authentication from '../authentication/index.js' export const ModelAction = { @@ -29,6 +31,17 @@ export const AccessRequestAction = { } export type AccessRequestActionKeys = (typeof ReleaseAction)[keyof typeof ReleaseAction] +export const FileAction = { + Download: 'download', +} +export type FileActionKeys = (typeof FileAction)[keyof typeof FileAction] + +export const ImageAction = { + Pull: 'pull', + Push: 'push', +} +export type ImageActionKeys = (typeof ImageAction)[keyof typeof ImageAction] + export abstract class BaseAuthorisationConnector { abstract userModelAction(user: UserDoc, model: ModelDoc, action: ModelActionKeys): Promise abstract userReleaseAction( @@ -43,7 +56,13 @@ export abstract class BaseAuthorisationConnector { accessRequest: AccessRequestDoc, action: AccessRequestActionKeys, ): Promise - + abstract userFileAction( + user: UserDoc, + model: ModelDoc, + file: FileInterfaceDoc, + action: FileActionKeys, + ): Promise + abstract userImageAction(user: UserDoc, model: ModelDoc, access: Access, action: ImageActionKeys): Promise async hasModelVisibilityAccess(user: UserDoc, model: ModelDoc) { if (model.visibility === ModelVisibility.Public) { return true diff --git a/backend/src/connectors/v2/authorisation/silly.ts b/backend/src/connectors/v2/authorisation/silly.ts index 63acf044b..4d4c242f1 100644 --- a/backend/src/connectors/v2/authorisation/silly.ts +++ b/backend/src/connectors/v2/authorisation/silly.ts @@ -1,8 +1,22 @@ import { AccessRequestDoc } from '../../../models/v2/AccessRequest.js' +import { FileInterfaceDoc } from '../../../models/v2/File.js' import { ModelDoc } from '../../../models/v2/Model.js' import { ReleaseDoc } from '../../../models/v2/Release.js' import { UserDoc } from '../../../models/v2/User.js' -import { AccessRequestActionKeys, BaseAuthorisationConnector, ModelActionKeys, ReleaseActionKeys } from './Base.js' +import { Access } from '../../../routes/v1/registryAuth.js' +import { getAccessRequestsByModel } from '../../../services/v2/accessRequest.js' +import log from '../../../services/v2/log.js' +import authentication from '../authentication/index.js' +import { + AccessRequestActionKeys, + BaseAuthorisationConnector, + FileAction, + FileActionKeys, + ImageAction, + ImageActionKeys, + ModelActionKeys, + ReleaseActionKeys, +} from './Base.js' export class SillyAuthorisationConnector extends BaseAuthorisationConnector { constructor() { @@ -48,4 +62,71 @@ export class SillyAuthorisationConnector extends BaseAuthorisationConnector { // Allow any other action to be completed return true } + + async userFileAction( + user: UserDoc, + model: ModelDoc, + file: FileInterfaceDoc, + action: FileActionKeys, + ): Promise { + // Prohibit non-collaborators from seeing private models + if (!(await this.hasModelVisibilityAccess(user, model))) { + return false + } + + const entities = await authentication.getEntities(user) + if (model.collaborators.some((collaborator) => entities.includes(collaborator.entity))) { + // Collaborators can upload or download files + return true + } + + if (action !== FileAction.Download) { + log.warn({ userDn: user.dn, file: file._id }, 'Non-collaborator can only download artefacts') + return false + } + + const accessRequests = await getAccessRequestsByModel(user, model.id) + const accessRequest = accessRequests.find((accessRequest) => + accessRequest.metadata.overview.entities.some((entity) => entities.includes(entity)), + ) + + if (!accessRequest) { + // User does not have a valid access request + log.warn({ userDn: user.dn, file: file._id }, 'No valid access request found') + return false + } + + return true + } + + async userImageAction(user: UserDoc, model: ModelDoc, access: Access, action: ImageActionKeys): Promise { + // Prohibit non-collaborators from seeing private models + if (!(await this.hasModelVisibilityAccess(user, model))) { + return false + } + + const entities = await authentication.getEntities(user) + if (model.collaborators.some((collaborator) => entities.includes(collaborator.entity))) { + // Collaborators can upload or download files + return true + } + + if (action !== ImageAction.Pull) { + log.warn({ userDn: user.dn, access }, 'Non-collaborator can only pull models') + return false + } + + const accessRequests = await getAccessRequestsByModel(user, model.id) + const accessRequest = accessRequests.find((accessRequest) => + accessRequest.metadata.overview.entities.some((entity) => entities.includes(entity)), + ) + + if (!accessRequest) { + // User does not have a valid access request + log.warn({ userDn: user.dn, access }, 'No valid access request found') + return false + } + + return true + } } diff --git a/backend/src/routes.ts b/backend/src/routes.ts index 615c6a01c..f60e36006 100644 --- a/backend/src/routes.ts +++ b/backend/src/routes.ts @@ -54,6 +54,7 @@ import { getModelAccessRequests } from './routes/v2/model/accessRequest/getModel import { patchAccessRequest } from './routes/v2/model/accessRequest/patchAccessRequest.js' import { postAccessRequest } from './routes/v2/model/accessRequest/postAccessRequest.js' import { deleteFile } from './routes/v2/model/file/deleteFile.js' +import { getDownloadFile } from './routes/v2/model/file/getDownloadFile.js' import { getFiles } from './routes/v2/model/file/getFiles.js' import { postFinishMultipartUpload } from './routes/v2/model/file/postFinishMultipartUpload.js' import { postSimpleUpload } from './routes/v2/model/file/postSimpleUpload.js' @@ -224,6 +225,7 @@ if (config.experimental.v2) { server.post('/api/v2/model/:modelId/access-request/:accessRequestId/review', ...postAccessRequestReviewResponse) server.get('/api/v2/model/:modelId/files', ...getFiles) + server.get('/api/v2/model/:modelId/file/:fileId/download', ...getDownloadFile) server.post('/api/v2/model/:modelId/files/upload/simple', ...postSimpleUpload) server.post('/api/v2/model/:modelId/files/upload/multipart/start', ...postStartMultipartUpload) server.post('/api/v2/model/:modelId/files/upload/multipart/finish', ...postFinishMultipartUpload) diff --git a/backend/src/routes/v1/registryAuth.ts b/backend/src/routes/v1/registryAuth.ts index 549dd789b..800a63ef5 100644 --- a/backend/src/routes/v1/registryAuth.ts +++ b/backend/src/routes/v1/registryAuth.ts @@ -6,11 +6,11 @@ import jwt from 'jsonwebtoken' import { isEqual } from 'lodash-es' import { stringify as uuidStringify, v4 as uuidv4 } from 'uuid' -import authentication from '../../connectors/v2/authentication/index.js' +import { ImageAction } from '../../connectors/v2/authorisation/Base.js' +import authorisation from '../../connectors/v2/authorisation/index.js' import { ModelDoc } from '../../models/v2/Model.js' import { UserDoc as UserDocV2 } from '../../models/v2/User.js' import { findDeploymentByUuid } from '../../services/deployment.js' -import { getAccessRequestsByModel } from '../../services/v2/accessRequest.js' import log from '../../services/v2/log.js' import { getModelById } from '../../services/v2/model.js' import { ModelId, UserDoc } from '../../types/types.js' @@ -175,32 +175,8 @@ async function checkAccessV2(access: Access, user: UserDocV2) { return false } - const entities = await authentication.getEntities(user) - if (model.collaborators.some((collaborator) => entities.includes(collaborator.entity))) { - // They are a collaborator to the model, let them push or pull. - return true - } - - if (!isEqual(access.actions, ['pull'])) { - // If users are not collaborators, they should only be able to pull - log.warn({ userDn: user.dn, access }, 'Non-collaborator can only pull models') - return false - } - - // TODO: If the model is 'public access' automatically approve pulls. - - const accessRequests = await getAccessRequestsByModel(user, modelId) - const accessRequest = accessRequests.find((accessRequest) => - accessRequest.metadata.overview.entities.some((entity) => entities.includes(entity)), - ) - - if (!accessRequest) { - // User does not have a valid access request - log.warn({ userDn: user.dn, access }, 'No valid access request found') - return false - } - - return true + const action = isEqual(access.actions, ['pull']) ? ImageAction.Pull : ImageAction.Push + return authorisation.userImageAction(user, model, access, action) } async function checkAccess(access: Access, user: UserDoc) { diff --git a/backend/src/routes/v2/model/file/getDownloadFile.ts b/backend/src/routes/v2/model/file/getDownloadFile.ts new file mode 100644 index 000000000..7c431099a --- /dev/null +++ b/backend/src/routes/v2/model/file/getDownloadFile.ts @@ -0,0 +1,56 @@ +import bodyParser from 'body-parser' +import contentDisposition from 'content-disposition' +import { Request, Response } from 'express' +import { z } from 'zod' + +import { FileInterface } from '../../../../models/v2/File.js' +import { downloadFile, getFileById } from '../../../../services/v2/file.js' +import { BadReq, InternalError } from '../../../../utils/v2/error.js' +import { parse } from '../../../../utils/validate.js' + +export const getDownloadFileSchema = z.object({ + params: z.object({ + modelId: z.string({ + required_error: 'Must specify model id as param', + }), + fileId: z.string(), + }), +}) + +interface GetDownloadFileResponse { + files: Array +} + +export const getDownloadFile = [ + bodyParser.json(), + async (req: Request, res: Response) => { + const { + params: { fileId }, + } = parse(req, getDownloadFileSchema) + + const file = await getFileById(req.user, fileId) + + // required to support utf-8 file names + res.set('Content-Disposition', contentDisposition(file.name, { type: 'inline' })) + res.set('Content-Type', file.mime) + res.set('Cache-Control', 'public, max-age=604800, immutable') + + if (req.headers.range) { + // TODO: support ranges + throw BadReq('Ranges are not supported', { fileId }) + } + + res.set('Content-Length', String(file.size)) + // TODO: support ranges + // res.set('Accept-Ranges', 'bytes') + res.writeHead(200) + + const stream = await downloadFile(req.user, fileId) + if (!stream.Body) { + throw InternalError('We were not able to retrieve the body of this file', { fileId }) + } + + // The AWS library doesn't seem to properly type 'Body' as being pipeable? + ;(stream.Body as any).pipe(res) + }, +] diff --git a/backend/src/scripts/example_schemas/minimal_upload_schema_beta.json b/backend/src/scripts/example_schemas/minimal_upload_schema_beta.json index b5173263c..9e36a9575 100644 --- a/backend/src/scripts/example_schemas/minimal_upload_schema_beta.json +++ b/backend/src/scripts/example_schemas/minimal_upload_schema_beta.json @@ -12,8 +12,7 @@ "description": "A description of what the model does.", "type": "string", "minLength": 1, - "maxLength": 5000, - "widget": "customTextInput" + "maxLength": 5000 }, "tags": { "title": "Descriptive tags for the model.", diff --git a/backend/src/services/v2/file.ts b/backend/src/services/v2/file.ts index ba7dd4f9b..e2c6daebe 100644 --- a/backend/src/services/v2/file.ts +++ b/backend/src/services/v2/file.ts @@ -1,5 +1,5 @@ -import { putObjectStream } from '../../clients/s3.js' -import { ModelAction } from '../../connectors/v2/authorisation/Base.js' +import { getObjectStream, putObjectStream } from '../../clients/s3.js' +import { FileAction, ModelAction } 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' @@ -37,6 +37,18 @@ export async function uploadFile(user: UserDoc, modelId: string, name: string, m return file } +export async function downloadFile(user: UserDoc, fileId: string, range?: { start: number; end: number }) { + const file = await getFileById(user, fileId) + const model = await getModelById(user, file.modelId) + + const access = await authorisation.userFileAction(user, model, file, FileAction.Download) + if (!access) { + throw Forbidden(`You do not have permission to download this model.`, { user: user.dn, fileId }) + } + + return getObjectStream(file.bucket, file.path, range) +} + export async function getFileById(user: UserDoc, fileId: string) { const file = await FileModel.findOne({ _id: fileId, diff --git a/backend/src/utils/v2/error.ts b/backend/src/utils/v2/error.ts index 1f93a4e1e..460972a3f 100644 --- a/backend/src/utils/v2/error.ts +++ b/backend/src/utils/v2/error.ts @@ -39,3 +39,7 @@ export function Forbidden(message: string, context?: BailoError['context'], logg export function NotFound(message: string, context?: BailoError['context'], logger?: Logger) { return GenericError(404, message, context, logger) } + +export function InternalError(message: string, context?: BailoError['context'], logger?: Logger) { + return GenericError(500, message, context, logger) +} From 097b6bf95a03fe955b306ac2529166d636470e56 Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Sun, 29 Oct 2023 19:45:47 +0000 Subject: [PATCH 2/4] Fix config mocking --- .../authorisation/authorisation.spec.ts | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/backend/test/connectors/authorisation/authorisation.spec.ts b/backend/test/connectors/authorisation/authorisation.spec.ts index 38ab58552..200f95919 100644 --- a/backend/test/connectors/authorisation/authorisation.spec.ts +++ b/backend/test/connectors/authorisation/authorisation.spec.ts @@ -1,18 +1,9 @@ import { describe, expect, test, vi } from 'vitest' import { getAuthorisationConnector } from '../../../src/connectors/v2/authorisation/index.js' +import config from '../../../src/utils/v2/__mocks__/config.js' -const configMock = vi.hoisted(() => ({ - connectors: { - authorisation: { - kind: 'silly', - }, - }, -})) -vi.mock('../../../src/utils/v2/config.js', () => ({ - __esModule: true, - default: configMock, -})) +vi.mock('../../../src/utils/v2/config.js') vi.mock('../../../src/connectors/v2/authentication/index.js', () => ({ default: { @@ -27,7 +18,7 @@ describe('connectors > authorisation', () => { }) test('invalid', () => { - configMock.connectors.authorisation.kind = 'invalid' + config.connectors.authorisation.kind = 'invalid' expect(() => getAuthorisationConnector(false)).toThrowError('No valid authorisation connector provided.') }) From 2bc73e596e746a5eced405c4834e7b79f2a19436 Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Mon, 30 Oct 2023 17:24:09 +0000 Subject: [PATCH 3/4] Narrow type, remove zod checks --- backend/src/routes/v2/model/file/getDownloadFile.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/backend/src/routes/v2/model/file/getDownloadFile.ts b/backend/src/routes/v2/model/file/getDownloadFile.ts index 7c431099a..c3e2d4926 100644 --- a/backend/src/routes/v2/model/file/getDownloadFile.ts +++ b/backend/src/routes/v2/model/file/getDownloadFile.ts @@ -1,6 +1,7 @@ import bodyParser from 'body-parser' import contentDisposition from 'content-disposition' import { Request, Response } from 'express' +import stream from 'stream' import { z } from 'zod' import { FileInterface } from '../../../../models/v2/File.js' @@ -10,9 +11,7 @@ import { parse } from '../../../../utils/validate.js' export const getDownloadFileSchema = z.object({ params: z.object({ - modelId: z.string({ - required_error: 'Must specify model id as param', - }), + modelId: z.string(), fileId: z.string(), }), }) @@ -51,6 +50,6 @@ export const getDownloadFile = [ } // The AWS library doesn't seem to properly type 'Body' as being pipeable? - ;(stream.Body as any).pipe(res) + ;(stream.Body as stream.Readable).pipe(res) }, ] From c53e1fa63978e44c0b8693ae5f106c4a88993173 Mon Sep 17 00:00:00 2001 From: a3957273 <89583054+a3957273@users.noreply.github.com> Date: Thu, 2 Nov 2023 14:47:07 +0000 Subject: [PATCH 4/4] Add tests for s3 and file services --- backend/test/clients/s3.spec.ts | 32 ++++++++++++++++++++++++++++++ backend/test/services/file.spec.ts | 26 +++++++++++++++++++++++- 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 backend/test/clients/s3.spec.ts diff --git a/backend/test/clients/s3.spec.ts b/backend/test/clients/s3.spec.ts new file mode 100644 index 000000000..be176fb2a --- /dev/null +++ b/backend/test/clients/s3.spec.ts @@ -0,0 +1,32 @@ +import { describe, expect, test, vi } from 'vitest' + +import { getObjectStream } from '../../src/clients/s3.js' + +const s3Mocks = vi.hoisted(() => { + const send = vi.fn(() => 'response') + + return { + send, + GetObjectCommand: vi.fn(() => ({})), + S3Client: vi.fn(() => ({ send })), + } +}) +vi.mock('@aws-sdk/client-s3', () => s3Mocks) + +describe('clients > s3', () => { + test('getObjectStream > success', async () => { + const bucket = 'test-bucket' + const key = 'test-key' + const range = { start: 0, end: 100 } + + const response = await getObjectStream(bucket, key, range) + + expect(s3Mocks.GetObjectCommand).toHaveBeenCalledWith({ + Bucket: bucket, + Key: key, + Range: `bytes=${range.start}-${range.end}`, + }) + expect(s3Mocks.send).toHaveBeenCalled() + expect(response).toBe('response') + }) +}) diff --git a/backend/test/services/file.spec.ts b/backend/test/services/file.spec.ts index 8b5da22dd..7a3b0699d 100644 --- a/backend/test/services/file.spec.ts +++ b/backend/test/services/file.spec.ts @@ -2,17 +2,19 @@ import { Readable } from 'stream' import { describe, expect, test, vi } from 'vitest' import { UserDoc } from '../../src/models/v2/User.js' -import { getFilesByModel, removeFile, uploadFile } from '../../src/services/v2/file.js' +import { downloadFile, getFilesByModel, removeFile, uploadFile } from '../../src/services/v2/file.js' vi.mock('../../src/utils/config.js') const s3Mocks = vi.hoisted(() => ({ putObjectStream: vi.fn(() => ({ fileSize: 100 })), + getObjectStream: vi.fn(() => 'fileStream'), })) vi.mock('../../src/clients/s3.js', () => s3Mocks) const authorisationMocks = vi.hoisted(() => ({ userModelAction: vi.fn(() => true), + userFileAction: vi.fn(() => true), })) vi.mock('../../src/connectors/v2/authorisation/index.js', async () => ({ default: authorisationMocks, @@ -105,4 +107,26 @@ describe('services > file', () => { expect(fileModelMocks.delete).not.toBeCalled() }) + + test('downloadFile > success', async () => { + const user = { dn: 'testUser' } as UserDoc + const fileId = 'testFileId' + const range = { start: 0, end: 50 } + + const result = await downloadFile(user, fileId, range) + + expect(result).toBe('fileStream') + }) + + test('downloadFile > no permission', async () => { + const user = { dn: 'testUser' } as UserDoc + const fileId = 'testFileId' + const range = { start: 0, end: 50 } + + authorisationMocks.userFileAction.mockResolvedValueOnce(false) + + await expect(downloadFile(user, fileId, range)).rejects.toThrowError( + /^You do not have permission to download this model./, + ) + }) })