From 95a3119642b3b69573bbde48b1e4b24ccaa7f6c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Pedro=20Pereira?= Date: Fri, 26 Jan 2024 16:08:07 +0000 Subject: [PATCH 1/3] Implement proxying of s3 and access control --- packages/core/src/lib/assets/local.ts | 6 + packages/core/src/lib/assets/s3.ts | 52 ++++-- packages/core/src/lib/assets/types.ts | 4 +- .../src/lib/server/createExpressServer.ts | 117 ++++++++++--- packages/core/src/types/config/index.ts | 154 ++++++++++-------- 5 files changed, 232 insertions(+), 101 deletions(-) diff --git a/packages/core/src/lib/assets/local.ts b/packages/core/src/lib/assets/local.ts index bc176a917b6..5eb4f4c5db0 100644 --- a/packages/core/src/lib/assets/local.ts +++ b/packages/core/src/lib/assets/local.ts @@ -26,6 +26,9 @@ export function localImageAssetsAPI ( } } }, + async download(filename, stream, headers) { + throw new Error("Not implemented yet") + } } } @@ -66,5 +69,8 @@ export function localFileAssetsAPI (storageConfig: StorageConfig & { kind: 'loca } } }, + async download(filename, stream, headers) { + throw new Error("Not implemented yet") + } } } diff --git a/packages/core/src/lib/assets/s3.ts b/packages/core/src/lib/assets/s3.ts index b3b7d7bd2fd..9c386c8d0a0 100644 --- a/packages/core/src/lib/assets/s3.ts +++ b/packages/core/src/lib/assets/s3.ts @@ -4,17 +4,20 @@ import { Upload } from '@aws-sdk/lib-storage' import type { StorageConfig } from '../../types' import type { FileAdapter, ImageAdapter } from './types' +import { Writable } from 'stream' -export function s3ImageAssetsAPI (storageConfig: StorageConfig & { kind: 's3' }): ImageAdapter { + + +export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): ImageAdapter { const { generateUrl, s3, presign, s3Endpoint } = s3AssetsCommon(storageConfig) return { - async url (id, extension) { + async url(id, extension) { if (!storageConfig.signed) { return generateUrl(`${s3Endpoint}${storageConfig.pathPrefix || ''}${id}.${extension}`) } return generateUrl(await presign(`${id}.${extension}`)) }, - async upload (buffer, id, extension) { + async upload(buffer, id, extension) { const upload = new Upload({ client: s3, params: { @@ -26,32 +29,34 @@ export function s3ImageAssetsAPI (storageConfig: StorageConfig & { kind: 's3' }) webp: 'image/webp', gif: 'image/gif', jpg: 'image/jpeg', + svg: 'image/svg+xml', }[extension], ACL: storageConfig.acl, }, }) await upload.done() }, - async delete (id, extension) { + async delete(id, extension) { await s3.deleteObject({ Bucket: storageConfig.bucketName, Key: `${storageConfig.pathPrefix || ''}${id}.${extension}`, }) }, + download: downloadFn(storageConfig) } } -export function s3FileAssetsAPI (storageConfig: StorageConfig & { kind: 's3' }): FileAdapter { +export function s3FileAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): FileAdapter { const { generateUrl, s3, presign, s3Endpoint } = s3AssetsCommon(storageConfig) return { - async url (filename) { + async url(filename) { if (!storageConfig.signed) { return generateUrl(`${s3Endpoint}${storageConfig.pathPrefix || ''}${filename}`) } return generateUrl(await presign(filename)) }, - async upload (stream, filename) { + async upload(stream, filename) { let filesize = 0 stream.on('data', data => { filesize += data.length @@ -72,16 +77,17 @@ export function s3FileAssetsAPI (storageConfig: StorageConfig & { kind: 's3' }): return { filename, filesize } }, - async delete (filename) { + async delete(filename) { await s3.deleteObject({ Bucket: storageConfig.bucketName, Key: (storageConfig.pathPrefix || '') + filename, }) }, + download: downloadFn(storageConfig) } } -export function getS3AssetsEndpoint (storageConfig: StorageConfig & { kind: 's3' }) { +export function getS3AssetsEndpoint(storageConfig: StorageConfig & { kind: 's3' }) { let endpoint = storageConfig.endpoint ? new URL(storageConfig.endpoint) : new URL(`https://s3.${storageConfig.region}.amazonaws.com`) @@ -96,14 +102,14 @@ export function getS3AssetsEndpoint (storageConfig: StorageConfig & { kind: 's3' return `${endpointString}/` } -function s3AssetsCommon (storageConfig: StorageConfig & { kind: 's3' }) { +export function s3AssetsCommon(storageConfig: StorageConfig & { kind: 's3' }) { const s3 = new S3({ credentials: storageConfig.accessKeyId && storageConfig.secretAccessKey ? { - accessKeyId: storageConfig.accessKeyId, - secretAccessKey: storageConfig.secretAccessKey, - } + accessKeyId: storageConfig.accessKeyId, + secretAccessKey: storageConfig.secretAccessKey, + } : undefined, region: storageConfig.region, endpoint: storageConfig.endpoint, @@ -128,3 +134,23 @@ function s3AssetsCommon (storageConfig: StorageConfig & { kind: 's3' }) { }, } } + +const downloadFn = (storageConfig: StorageConfig & { kind: 's3' }) => { + return async (filename: string, stream: Writable, headers: (key: string, val: string) => void) => { + const { s3 } = s3AssetsCommon(storageConfig) + const command = new GetObjectCommand({ + Bucket: storageConfig.bucketName, + Key: (storageConfig.pathPrefix || '') + filename, + }) + + const s3Response = await s3.send(command) + if (!s3Response.Body) { + throw new Error('No response body') + } + + headers('Content-Type', s3Response.ContentType ?? '') + headers('Content-Length', s3Response.ContentLength?.toString() ?? '') + + s3Response.Body.transformToWebStream().pipeTo(Writable.toWeb(stream)) + }; +} \ No newline at end of file diff --git a/packages/core/src/lib/assets/types.ts b/packages/core/src/lib/assets/types.ts index 5d5d01f5f06..945e5f3836d 100644 --- a/packages/core/src/lib/assets/types.ts +++ b/packages/core/src/lib/assets/types.ts @@ -1,13 +1,15 @@ -import type { Readable } from 'stream' +import type { Readable, Writable } from 'stream' import type { ImageExtension, FileMetadata } from '../../types' export type ImageAdapter = { + download(filename: string, stream: Writable, headers: (key: string, value: string) => void): void upload(buffer: Buffer, id: string, extension: string): Promise delete(id: string, extension: ImageExtension): Promise url(id: string, extension: ImageExtension): Promise } export type FileAdapter = { + download(filename: string, stream: Writable, headers: (key: string, value: string) => void): void upload(stream: Readable, filename: string): Promise delete(id: string): Promise url(filename: string): Promise diff --git a/packages/core/src/lib/server/createExpressServer.ts b/packages/core/src/lib/server/createExpressServer.ts index 8eb17fc3e0b..d489119da14 100644 --- a/packages/core/src/lib/server/createExpressServer.ts +++ b/packages/core/src/lib/server/createExpressServer.ts @@ -11,6 +11,7 @@ import { ApolloServerPluginLandingPageLocalDefault } from '@apollo/server/plugin import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js' import type { KeystoneConfig, KeystoneContext, GraphQLConfig } from '../../types' import { addHealthCheck } from './addHealthCheck' +import { s3AssetsCommon, s3FileAssetsAPI, s3ImageAssetsAPI } from '../assets/s3' /* NOTE: This creates the main Keystone express server, including the @@ -76,21 +77,99 @@ export const createExpressServer = async ( } if (config.storage) { - for (const val of Object.values(config.storage)) { - if (val.kind !== 'local' || !val.serverRoute) continue - expressServer.use( - val.serverRoute.path, - express.static(val.storagePath, { - setHeaders (res) { - if (val.type === 'file') { - res.setHeader('Content-Type', 'application/octet-stream') + for (const key of Object.keys(config.storage)) { + const storageConfig = config.storage[key]; + + /**we can only verify isAccessAllowed in the following cases: + * 1. local storage: if serverRoute is defined + * 2. s3 storage: if serverRoute is defined. + * + * Otherwise the generateUrl would generate a direct url to s3 supporting solution + * like aws or minio or what not and as such, we wouldn't be able to intercept the + * request properly. + * + * Actually we could by "redirection", sending the browser back to us + * and checking before redirecting, but that would be a bit of a hack. + */ + + //verifying if isAccessAllowed would be supported in non-proxied mode and warning about it at development time + if (storageConfig.isAccessAllowed && (storageConfig.kind === 's3' && !storageConfig.serverRoute)) { + process.env.NODE_ENV === 'development' && console.warn("storage api isAccessAllowed is not supported in non-proxied mode for kind: s3. The isAccessAllowed function will not get executed"); + } + + /** + * Also, checking if generateUrl respects the proxied flag. If it doesn't, we warn about it at development time. + */ + if (storageConfig.generateUrl && storageConfig.serverRoute) { + process.env.NODE_ENV === 'development' && console.warn("generateUrl storage api config should respect the serverRoute flag. Some assumptions about the generateUrl function must be in place for serverRoute mode to work.") + } + + if (storageConfig.serverRoute) { + //now we need 2 implementations + //one for local storage, using express static middleware BUT with isAccessAllowed checked properly beforehand + //another for s3, which will issue a GET Object request to s3, and pipe the stream to the response + let expressMiddleware: express.RequestHandler; + if (storageConfig.kind === 'local') { + + expressMiddleware = (request, response) => { + + const { extraPath } = request.params + + if (typeof storageConfig.isAccessAllowed === 'function' && + !storageConfig.isAccessAllowed(context, extraPath, response.getHeader)) { + response.status(403).send('Forbidden') + return } - }, - index: false, - redirect: false, - lastModified: false, - }) - ) + + express.static(storageConfig.storagePath, { + setHeaders(res) { + if (storageConfig.type === 'file') { + res.setHeader('Content-Type', 'application/octet-stream') + } + }, + index: false, + redirect: false, + lastModified: false, + }) + } + } + else if (storageConfig.kind === 's3') { + + const assetApi = storageConfig.type === 'image' ? s3ImageAssetsAPI(storageConfig) : s3FileAssetsAPI(storageConfig); + + expressMiddleware = (request, response) => { + const { extraPath } = request.params + + if (!extraPath) { + response.status(404).send('Not found') + return + } + + const { s3, s3Endpoint } = s3AssetsCommon(storageConfig); + + if (typeof storageConfig.isAccessAllowed === 'function' && + !storageConfig.isAccessAllowed(context, s3, s3Endpoint, extraPath, response.getHeader)) { + response.status(403).send('Forbidden') + return + } + + assetApi.download(extraPath, response, response.setHeader); + } + + } + else { + expressMiddleware = (request, response) => { + process.env.NODE_ENV === 'development' && console.warn("storage api kind not supported"); + response.status(404).send('Not found') + } + } + + + expressServer.use( + `${storageConfig.serverRoute.path}/:extraPath`, + expressMiddleware); + } + } } @@ -105,11 +184,11 @@ export const createExpressServer = async ( playgroundOption === 'apollo' ? apolloConfig?.plugins : [ - playgroundOption - ? ApolloServerPluginLandingPageLocalDefault() - : ApolloServerPluginLandingPageDisabled(), - ...(apolloConfig?.plugins || []), - ], + playgroundOption + ? ApolloServerPluginLandingPageLocalDefault() + : ApolloServerPluginLandingPageDisabled(), + ...(apolloConfig?.plugins || []), + ], } as ApolloServerOptions const apolloServer = new ApolloServer({ ...serverConfig }) diff --git a/packages/core/src/types/config/index.ts b/packages/core/src/types/config/index.ts index cb5684a7d40..890381eada2 100644 --- a/packages/core/src/types/config/index.ts +++ b/packages/core/src/types/config/index.ts @@ -20,76 +20,94 @@ import type { import type { BaseFields } from './fields' import type { ListAccessControl, FieldAccessControl } from './access-control' import type { ListHooks, FieldHooks } from './hooks' +import { S3Client } from '@aws-sdk/client-s3' type FileOrImage = // is given full file name, returns file name that will be used at | { type: 'file', transformName?: (filename: string) => MaybePromise } // return does not include extension, extension is handed over in case they want to use it | { - type: 'image' - // is given full file name, returns file name that will be used at - transformName?: (filename: string, extension: string) => MaybePromise - } + type: 'image' + // is given full file name, returns file name that will be used at + transformName?: (filename: string, extension: string) => MaybePromise + } export type StorageConfig = ( | { - /** The kind of storage being configured */ - kind: 'local' - /** The path to where the asset will be stored on disc, eg 'public/images' */ - storagePath: string - /** A function that receives a partial url, whose return will be used as the URL in graphql - * - * For example, a local dev usage of this might be: - * ```ts - * path => `http://localhost:3000/images${path}` - * ``` - */ - generateUrl: (path: string) => string - /** The configuration for keystone's hosting of the assets - if set to null, keystone will not host the assets */ - serverRoute: { - /** The partial path that the assets will be hosted at by keystone, eg `/images` or `/our-cool-files` */ - path: string - } | null - /** Sets whether the assets should be preserved locally on removal from keystone's database */ - preserve?: boolean - transformName?: (filename: string) => string - } + /** The kind of storage being configured */ + kind: 'local' + /** The path to where the asset will be stored on disc, eg 'public/images' */ + storagePath: string + /** A function that receives a partial url, whose return will be used as the URL in graphql + * + * For example, a local dev usage of this might be: + * ```ts + * path => `http://localhost:3000/images${path}` + * ``` + */ + generateUrl: (path: string) => string + /** The configuration for keystone's hosting of the assets - if set to null, keystone will not host the assets */ + serverRoute: { + /** The partial path that the assets will be hosted at by keystone, eg `/images` or `/our-cool-files` */ + path: string + } | null, + /** A function that is checked before serving the file or image to check for permissions. + * This function will only be respected if the serverRoute is set + */ + isAccessAllowed?: (context: KeystoneContext, fileKey: string, headers: (headerKey: string) => string | number | string[] | undefined) => boolean + /** Sets whether the assets should be preserved locally on removal from keystone's database */ + preserve?: boolean + transformName?: (filename: string) => string + } | { - /** The kind of storage being configured */ - kind: 's3' - /** Sets signing of the asset - for use when you want private assets */ - signed?: { expiry: number } - generateUrl?: (path: string) => string - /** Sets whether the assets should be preserved locally on removal from keystone's database */ - preserve?: boolean - pathPrefix?: string - /** Your s3 instance's bucket name */ - bucketName: string - /** Your s3 instance's region */ - region: string - /** An access Key ID with write access to your S3 instance */ - accessKeyId?: string - /** The secret access key that gives permissions to your access Key Id */ - secretAccessKey?: string - /** An endpoint to use - to be provided if you are not using AWS as your endpoint */ - endpoint?: string - /** If true, will force the 'old' S3 path style of putting bucket name at the start of the pathname of the URL */ - forcePathStyle?: boolean - /** A string that sets permissions for the uploaded assets. Default is 'private'. - * - * Amazon S3 supports a set of predefined grants, known as canned ACLs. - * See https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl - * for more details. - */ - acl?: - | 'private' - | 'public-read' - | 'public-read-write' - | 'aws-exec-read' - | 'authenticated-read' - | 'bucket-owner-read' - | 'bucket-owner-full-control' - } + /** The kind of storage being configured */ + kind: 's3' + /** Sets signing of the asset - for use when you want private assets */ + signed?: { expiry: number } + generateUrl?: (path: string) => string + /** Sets whether the assets should be preserved locally on removal from keystone's database */ + preserve?: boolean + pathPrefix?: string + /** Your s3 instance's bucket name */ + bucketName: string + /** Your s3 instance's region */ + region: string + /** An access Key ID with write access to your S3 instance */ + accessKeyId?: string + /** The secret access key that gives permissions to your access Key Id */ + secretAccessKey?: string + /** An endpoint to use - to be provided if you are not using AWS as your endpoint */ + endpoint?: string + /** If true, will force the 'old' S3 path style of putting bucket name at the start of the pathname of the URL */ + forcePathStyle?: boolean, + /** The configuration for keystone's hosting of the assets - if set to null, keystone will not host the assets */ + serverRoute: { + /** The partial path that the assets will be hosted at by keystone, eg `/images` or `/our-cool-files` */ + path: string, + } | null, + /** A function that is checked before serving the file or image to check for permissions. + * This function will only be respected if the serverRoute is set + */ + isAccessAllowed?: (context: KeystoneContext, s3: S3Client, s3Endpoint: string, fileKey: string, headers: (headerKey: string) => string | number | string[] | undefined) => boolean + /** A string that sets permissions for the uploaded assets. Default is 'private'. + * + * Amazon S3 supports a set of predefined grants, known as canned ACLs. + * See https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl + * for more details. + * + * In terms of access control, when using serverRoute with proxied: true, it only matters to protect + * the assets from being accessed directly from the S3 bucket. To protect the assets with serverRoute: { proxied: true } you should instead + * implement the isAccessAllowed function + */ + acl?: + | 'private' + | 'public-read' + | 'public-read-write' + | 'aws-exec-read' + | 'authenticated-read' + | 'bucket-owner-read' + | 'bucket-owner-full-control' + } ) & FileOrImage @@ -200,11 +218,11 @@ export type ServerConfig = { /** @deprecated */ healthCheck?: - | true - | { - path?: string - data?: Record | (() => Record) - } + | true + | { + path?: string + data?: Record | (() => Record) + } /** extend the Express application used by Keystone */ extendExpressApp?: ( @@ -219,15 +237,15 @@ export type ServerConfig = { graphqlSchema: GraphQLSchema ) => void } & ( - | { + | { /** Port number to start the server on. Defaults to process.env.PORT || 3000 */ port?: number } - | { + | { /** node http.Server options */ options?: ListenOptions } -) + ) // config.graphql From 8629eb3ecc21b97768a13e796165af609967a919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Pedro=20Pereira?= Date: Mon, 29 Jan 2024 19:58:13 +0000 Subject: [PATCH 2/3] Fix some minor issues and clear documentation --- docs/pages/docs/config/config.md | 2 +- docs/pages/docs/guides/images-and-files.md | 2 +- .../src/lib/assets/createImagesContext.ts | 4 +- packages/core/src/lib/assets/local.ts | 14 +- packages/core/src/lib/assets/s3.ts | 22 +- packages/core/src/lib/assets/types.ts | 2 +- .../src/lib/server/createExpressServer.ts | 221 ++++++++++-------- packages/core/src/types/config/index.ts | 23 +- 8 files changed, 167 insertions(+), 123 deletions(-) diff --git a/docs/pages/docs/config/config.md b/docs/pages/docs/config/config.md index 4f123ab1d73..e36e0cdcb99 100644 --- a/docs/pages/docs/config/config.md +++ b/docs/pages/docs/config/config.md @@ -482,7 +482,7 @@ export default config({ region, accessKeyId, secretAccessKey, - proxied: { baseUrl: '/images-proxy' }, + serverRoute: { path: '/images-proxy' }, signed: { expiry: 5000 } endpoint: 'http://127.0.0.1:9000/', forcePathStyle: true, diff --git a/docs/pages/docs/guides/images-and-files.md b/docs/pages/docs/guides/images-and-files.md index 3d3f56d9171..fc619bf80c3 100644 --- a/docs/pages/docs/guides/images-and-files.md +++ b/docs/pages/docs/guides/images-and-files.md @@ -15,7 +15,7 @@ The `storage` object defines how and where the assets are stored and accessed by - The `type` of `field` the storage is being used for – `file` or `image` - A function to generate the URL (`generateUrl`) Keystone returns in the GraphQL API – pointing to the location or the storage where the assets can be accessed - The actual location where Keystone stores the assets – either a local `path` or the details of an `s3` bucket -- The location Keystone will serve the assets from – either a `serverRoute` for `local` or a `proxied` connection for `s3`. Both of these options add a route to the Keystone backend which the files can be accessed from +- The location Keystone will serve the assets from – the `serverRoute` for `local` or `s3`. It will add a route to the Keystone backend which the files can be accessed from. Please note that `generateUrl` should be carefully defined when using proxied mode. ## Defining `storage` in Keystone config diff --git a/packages/core/src/lib/assets/createImagesContext.ts b/packages/core/src/lib/assets/createImagesContext.ts index b8da9e8e7b0..5b7a7c93917 100644 --- a/packages/core/src/lib/assets/createImagesContext.ts +++ b/packages/core/src/lib/assets/createImagesContext.ts @@ -32,8 +32,8 @@ export function createImagesContext (config: KeystoneConfig): ImagesContext { imageAssetsAPIs.set( storageKey, storageConfig.kind === 'local' - ? localImageAssetsAPI(storageConfig) - : s3ImageAssetsAPI(storageConfig) + ? localImageAssetsAPI(storageConfig,storageKey) + : s3ImageAssetsAPI(storageConfig,storageKey) ) } } diff --git a/packages/core/src/lib/assets/local.ts b/packages/core/src/lib/assets/local.ts index 5eb4f4c5db0..c58c461b6b0 100644 --- a/packages/core/src/lib/assets/local.ts +++ b/packages/core/src/lib/assets/local.ts @@ -6,10 +6,15 @@ import { type StorageConfig } from '../../types' import { type FileAdapter, type ImageAdapter } from './types' export function localImageAssetsAPI ( - storageConfig: StorageConfig & { kind: 'local' } + storageConfig: StorageConfig & { kind: 'local' }, storageKey: string='' ): ImageAdapter { return { async url (id, extension) { + + if(storageConfig.serverRoute) { + return storageConfig.generateUrl(`${storageConfig.serverRoute}/${storageKey}/${id}.${extension}`); + } + return storageConfig.generateUrl(`/${id}.${extension}`) }, async upload (buffer, id, extension) { @@ -32,9 +37,14 @@ export function localImageAssetsAPI ( } } -export function localFileAssetsAPI (storageConfig: StorageConfig & { kind: 'local' }): FileAdapter { +export function localFileAssetsAPI (storageConfig: StorageConfig & { kind: 'local' }, storageKey: string=''): FileAdapter { return { async url (filename) { + + if(storageConfig.serverRoute) { + return storageConfig.generateUrl(`${storageConfig.serverRoute}/${storageKey}/${filename}`); + } + return storageConfig.generateUrl(`/${filename}`) }, async upload (stream, filename) { diff --git a/packages/core/src/lib/assets/s3.ts b/packages/core/src/lib/assets/s3.ts index 9c386c8d0a0..f3243cbbd8d 100644 --- a/packages/core/src/lib/assets/s3.ts +++ b/packages/core/src/lib/assets/s3.ts @@ -6,15 +6,18 @@ import type { StorageConfig } from '../../types' import type { FileAdapter, ImageAdapter } from './types' import { Writable } from 'stream' - - -export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): ImageAdapter { +export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }, storageKey: string=''): ImageAdapter { const { generateUrl, s3, presign, s3Endpoint } = s3AssetsCommon(storageConfig) return { async url(id, extension) { + if(storageConfig.serverRoute) { + return generateUrl(`${storageConfig.serverRoute}/${storageKey}/${storageConfig.pathPrefix || ''}${id}.${extension}`); + } + if (!storageConfig.signed) { return generateUrl(`${s3Endpoint}${storageConfig.pathPrefix || ''}${id}.${extension}`) } + return generateUrl(await presign(`${id}.${extension}`)) }, async upload(buffer, id, extension) { @@ -28,8 +31,7 @@ export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): png: 'image/png', webp: 'image/webp', gif: 'image/gif', - jpg: 'image/jpeg', - svg: 'image/svg+xml', + jpg: 'image/jpeg' }[extension], ACL: storageConfig.acl, }, @@ -46,11 +48,14 @@ export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): } } -export function s3FileAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }): FileAdapter { +export function s3FileAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }, storageKey: string=''): FileAdapter { const { generateUrl, s3, presign, s3Endpoint } = s3AssetsCommon(storageConfig) return { async url(filename) { + if(storageConfig.serverRoute) { + return generateUrl(`${storageConfig.serverRoute}/${storageKey}/${storageConfig.pathPrefix || ''}${filename}`); + } if (!storageConfig.signed) { return generateUrl(`${s3Endpoint}${storageConfig.pathPrefix || ''}${filename}`) } @@ -138,9 +143,10 @@ export function s3AssetsCommon(storageConfig: StorageConfig & { kind: 's3' }) { const downloadFn = (storageConfig: StorageConfig & { kind: 's3' }) => { return async (filename: string, stream: Writable, headers: (key: string, val: string) => void) => { const { s3 } = s3AssetsCommon(storageConfig) + console.log({headers,stream}); const command = new GetObjectCommand({ Bucket: storageConfig.bucketName, - Key: (storageConfig.pathPrefix || '') + filename, + Key: filename, }) const s3Response = await s3.send(command) @@ -151,6 +157,6 @@ const downloadFn = (storageConfig: StorageConfig & { kind: 's3' }) => { headers('Content-Type', s3Response.ContentType ?? '') headers('Content-Length', s3Response.ContentLength?.toString() ?? '') - s3Response.Body.transformToWebStream().pipeTo(Writable.toWeb(stream)) + await s3Response.Body.transformToWebStream().pipeTo(Writable.toWeb(stream)) }; } \ No newline at end of file diff --git a/packages/core/src/lib/assets/types.ts b/packages/core/src/lib/assets/types.ts index 945e5f3836d..24bd14ba68e 100644 --- a/packages/core/src/lib/assets/types.ts +++ b/packages/core/src/lib/assets/types.ts @@ -10,7 +10,7 @@ export type ImageAdapter = { export type FileAdapter = { download(filename: string, stream: Writable, headers: (key: string, value: string) => void): void - upload(stream: Readable, filename: string): Promise + upload(stream: Readable, filename: string): Promise delete(id: string): Promise url(filename: string): Promise } diff --git a/packages/core/src/lib/server/createExpressServer.ts b/packages/core/src/lib/server/createExpressServer.ts index d489119da14..238c027a71f 100644 --- a/packages/core/src/lib/server/createExpressServer.ts +++ b/packages/core/src/lib/server/createExpressServer.ts @@ -1,17 +1,17 @@ -import { createServer, type Server } from 'http' -import cors, { type CorsOptions } from 'cors' -import { json } from 'body-parser' -import { expressMiddleware } from '@apollo/server/express4' -import express from 'express' -import type { GraphQLFormattedError, GraphQLSchema } from 'graphql' import { ApolloServer, type ApolloServerOptions } from '@apollo/server' +import { expressMiddleware } from '@apollo/server/express4' import { ApolloServerPluginLandingPageDisabled } from '@apollo/server/plugin/disabled' import { ApolloServerPluginLandingPageLocalDefault } from '@apollo/server/plugin/landingPage/default' +import { json } from 'body-parser' +import cors, { type CorsOptions } from 'cors' +import express from 'express' +import type { GraphQLFormattedError, GraphQLSchema } from 'graphql' +import { createServer, type Server } from 'http' // @ts-expect-error import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js' -import type { KeystoneConfig, KeystoneContext, GraphQLConfig } from '../../types' -import { addHealthCheck } from './addHealthCheck' +import type { GraphQLConfig, KeystoneConfig, KeystoneContext, StorageConfig } from '../../types' import { s3AssetsCommon, s3FileAssetsAPI, s3ImageAssetsAPI } from '../assets/s3' +import { addHealthCheck } from './addHealthCheck' /* NOTE: This creates the main Keystone express server, including the @@ -76,101 +76,11 @@ export const createExpressServer = async ( config.server?.extendHttpServer(httpServer, context, graphQLSchema) } - if (config.storage) { - for (const key of Object.keys(config.storage)) { - const storageConfig = config.storage[key]; - - /**we can only verify isAccessAllowed in the following cases: - * 1. local storage: if serverRoute is defined - * 2. s3 storage: if serverRoute is defined. - * - * Otherwise the generateUrl would generate a direct url to s3 supporting solution - * like aws or minio or what not and as such, we wouldn't be able to intercept the - * request properly. - * - * Actually we could by "redirection", sending the browser back to us - * and checking before redirecting, but that would be a bit of a hack. - */ - - //verifying if isAccessAllowed would be supported in non-proxied mode and warning about it at development time - if (storageConfig.isAccessAllowed && (storageConfig.kind === 's3' && !storageConfig.serverRoute)) { - process.env.NODE_ENV === 'development' && console.warn("storage api isAccessAllowed is not supported in non-proxied mode for kind: s3. The isAccessAllowed function will not get executed"); - } - - /** - * Also, checking if generateUrl respects the proxied flag. If it doesn't, we warn about it at development time. - */ - if (storageConfig.generateUrl && storageConfig.serverRoute) { - process.env.NODE_ENV === 'development' && console.warn("generateUrl storage api config should respect the serverRoute flag. Some assumptions about the generateUrl function must be in place for serverRoute mode to work.") - } - - if (storageConfig.serverRoute) { - //now we need 2 implementations - //one for local storage, using express static middleware BUT with isAccessAllowed checked properly beforehand - //another for s3, which will issue a GET Object request to s3, and pipe the stream to the response - let expressMiddleware: express.RequestHandler; - if (storageConfig.kind === 'local') { - - expressMiddleware = (request, response) => { - - const { extraPath } = request.params - - if (typeof storageConfig.isAccessAllowed === 'function' && - !storageConfig.isAccessAllowed(context, extraPath, response.getHeader)) { - response.status(403).send('Forbidden') - return - } - - express.static(storageConfig.storagePath, { - setHeaders(res) { - if (storageConfig.type === 'file') { - res.setHeader('Content-Type', 'application/octet-stream') - } - }, - index: false, - redirect: false, - lastModified: false, - }) - } - } - else if (storageConfig.kind === 's3') { - - const assetApi = storageConfig.type === 'image' ? s3ImageAssetsAPI(storageConfig) : s3FileAssetsAPI(storageConfig); - - expressMiddleware = (request, response) => { - const { extraPath } = request.params - - if (!extraPath) { - response.status(404).send('Not found') - return - } - - const { s3, s3Endpoint } = s3AssetsCommon(storageConfig); - - if (typeof storageConfig.isAccessAllowed === 'function' && - !storageConfig.isAccessAllowed(context, s3, s3Endpoint, extraPath, response.getHeader)) { - response.status(403).send('Forbidden') - return - } - - assetApi.download(extraPath, response, response.setHeader); - } - - } - else { - expressMiddleware = (request, response) => { - process.env.NODE_ENV === 'development' && console.warn("storage api kind not supported"); - response.status(404).send('Not found') - } - } - - - expressServer.use( - `${storageConfig.serverRoute.path}/:extraPath`, - expressMiddleware); - } - } + if (config.storage) { + Object.entries(config.storage).forEach(([key, storageConfig]) => { + proxyStorageIfNeeded(key, storageConfig, expressServer, context); + }); } const apolloConfig = config.graphql?.apolloConfig @@ -208,3 +118,110 @@ export const createExpressServer = async ( return { expressServer, apolloServer, httpServer } } + + +const proxyStorageIfNeeded = (storageConfigKey: string, storageConfig: StorageConfig, expressServer: express.Express, context: KeystoneContext) => { + + /**we can only verify isAccessAllowed in the following cases: + * 1. local storage: if serverRoute is defined + * 2. s3 storage: if serverRoute is defined. + * + * Otherwise the generateUrl would generate a direct url to s3 supporting solution + * like aws or minio or what not and as such, we wouldn't be able to intercept the + * request properly. + * + * Actually we could by "redirection", sending the browser back to us + * and checking before redirecting, but that would be a bit of a hack. + */ + + //verifying if isAccessAllowed would be supported in direct mode and warning about it at development time + if (storageConfig.isAccessAllowed && (storageConfig.kind === 's3' && !storageConfig.serverRoute)) { + process.env.NODE_ENV !== 'production' && console.warn("storage api isAccessAllowed is not supported in non-proxied mode for kind: s3. The isAccessAllowed function will not get executed"); + } + + /** + * Also, checking if generateUrl respects the serverRoute path. If it doesn't, we warn about it at development time. + */ + if (storageConfig.generateUrl && storageConfig.serverRoute) { + process.env.NODE_ENV !== 'production' && console.warn("generateUrl storage api config should respect the serverRoute flag. Some assumptions about the generateUrl function must be in place for serverRoute mode to work.") + } + + if (storageConfig.serverRoute) { + + const { isAccessAllowed } = storageConfig; + + if (isAccessAllowed) { + + const storageAccessControl: express.RequestHandler = async (request, response, next) => { + const fileKey = request.params[0]; + + if (!fileKey) { + response.status(404).send('Not found') + return + } + + const opts = { + context, + fileKey, + headers: (key: string) => { + return request.header(key); + }, + ...(storageConfig.kind === 's3' ? s3AssetsCommon(storageConfig) : {}) + }; + + if (!isAccessAllowed( + opts)) { + response.status(403).send('Forbidden'); + return; + } + + + }; + + const storageProxy: express.RequestHandler = async (request, response, next) => { + const fileKey = request.params[0]; + + //Leave the local storage to express.static as it was in the original code of Keystone + if (storageConfig.kind === 'local') { + next(); + return; + } + + //S3 downloads are handled by the s3AssetsAPI + try { + const assetApi = storageConfig.type === 'image' ? s3ImageAssetsAPI(storageConfig) : s3FileAssetsAPI(storageConfig); + await assetApi.download(fileKey, response, (key: string, value: string) => { + response.header(key, value); + }); + } catch (e) { + console.error(e); + response.status(500).send('Failed'); + + } + + response.end(); + return; + } + + expressServer + .route(`${storageConfig.serverRoute.path}/${storageConfigKey}/*`) + .get(storageAccessControl) + .get(storageProxy); + } + + if (storageConfig.kind === 'local') { + expressServer.use(`${storageConfig.serverRoute.path}/${storageConfigKey}`, + express.static(storageConfig.storagePath, { + setHeaders(res) { + if (storageConfig.type === 'file') { + res.setHeader('Content-Type', 'application/octet-stream') + } + }, + index: false, + redirect: false, + lastModified: false, + }) + ); + } + } +} \ No newline at end of file diff --git a/packages/core/src/types/config/index.ts b/packages/core/src/types/config/index.ts index 890381eada2..33291cfb7e1 100644 --- a/packages/core/src/types/config/index.ts +++ b/packages/core/src/types/config/index.ts @@ -32,6 +32,17 @@ type FileOrImage = transformName?: (filename: string, extension: string) => MaybePromise } +export type LocalStorageAccessAllowedOptions = { + context: KeystoneContext, + fileKey: string, + headers: (headerKey: string) => string | number | string[] | undefined +} + +export type StorageAccessAllowedOptions = LocalStorageAccessAllowedOptions & { + s3?: S3Client, + s3Endpoint?: string, +} + export type StorageConfig = ( | { /** The kind of storage being configured */ @@ -52,9 +63,9 @@ export type StorageConfig = ( path: string } | null, /** A function that is checked before serving the file or image to check for permissions. - * This function will only be respected if the serverRoute is set + * This function will only be respected if the serverRoute is set\ */ - isAccessAllowed?: (context: KeystoneContext, fileKey: string, headers: (headerKey: string) => string | number | string[] | undefined) => boolean + isAccessAllowed?: (options: StorageAccessAllowedOptions) => boolean, /** Sets whether the assets should be preserved locally on removal from keystone's database */ preserve?: boolean transformName?: (filename: string) => string @@ -88,16 +99,16 @@ export type StorageConfig = ( /** A function that is checked before serving the file or image to check for permissions. * This function will only be respected if the serverRoute is set */ - isAccessAllowed?: (context: KeystoneContext, s3: S3Client, s3Endpoint: string, fileKey: string, headers: (headerKey: string) => string | number | string[] | undefined) => boolean + isAccessAllowed?: (options: StorageAccessAllowedOptions) => boolean, /** A string that sets permissions for the uploaded assets. Default is 'private'. * * Amazon S3 supports a set of predefined grants, known as canned ACLs. * See https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html#canned-acl * for more details. * - * In terms of access control, when using serverRoute with proxied: true, it only matters to protect - * the assets from being accessed directly from the S3 bucket. To protect the assets with serverRoute: { proxied: true } you should instead - * implement the isAccessAllowed function + * In terms of access control, when using serverRoute (proxied through keystone) it only matters to protect + * the assets from being accessed directly from the S3 bucket. To protect the assets with serverRoute: { path: '/some-path' } you should instead + * implement the isAccessAllowed function */ acl?: | 'private' From 6542aa34322f9e7696d3dc344b2c0dfebb66dfd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Pedro=20Pereira?= Date: Thu, 1 Feb 2024 17:30:52 +0000 Subject: [PATCH 3/3] Some more minor improvements/fixes --- .../core/src/lib/assets/createFilesContext.ts | 4 +- packages/core/src/lib/assets/local.ts | 13 +++-- packages/core/src/lib/assets/s3.ts | 4 +- .../src/lib/server/createExpressServer.ts | 54 ++++++++++--------- 4 files changed, 41 insertions(+), 34 deletions(-) diff --git a/packages/core/src/lib/assets/createFilesContext.ts b/packages/core/src/lib/assets/createFilesContext.ts index eff8d887799..06c7f70ba89 100644 --- a/packages/core/src/lib/assets/createFilesContext.ts +++ b/packages/core/src/lib/assets/createFilesContext.ts @@ -28,8 +28,8 @@ export function createFilesContext (config: KeystoneConfig): FilesContext { adaptersMap.set( storageKey, storageConfig.kind === 'local' - ? localFileAssetsAPI(storageConfig) - : s3FileAssetsAPI(storageConfig) + ? localFileAssetsAPI(storageConfig,storageKey) + : s3FileAssetsAPI(storageConfig,storageKey) ) } } diff --git a/packages/core/src/lib/assets/local.ts b/packages/core/src/lib/assets/local.ts index c58c461b6b0..659cf3fc68b 100644 --- a/packages/core/src/lib/assets/local.ts +++ b/packages/core/src/lib/assets/local.ts @@ -10,12 +10,13 @@ export function localImageAssetsAPI ( ): ImageAdapter { return { async url (id, extension) { + const generateUrl = storageConfig.generateUrl ?? (url=>url); if(storageConfig.serverRoute) { - return storageConfig.generateUrl(`${storageConfig.serverRoute}/${storageKey}/${id}.${extension}`); + return generateUrl(`${storageConfig.serverRoute.path}/${storageKey}/${id}.${extension}`); } - return storageConfig.generateUrl(`/${id}.${extension}`) + return generateUrl(`/${id}.${extension}`) }, async upload (buffer, id, extension) { await fs.writeFile(path.join(storageConfig.storagePath, `${id}.${extension}`), buffer) @@ -40,12 +41,14 @@ export function localImageAssetsAPI ( export function localFileAssetsAPI (storageConfig: StorageConfig & { kind: 'local' }, storageKey: string=''): FileAdapter { return { async url (filename) { - + const generateUrl = storageConfig.generateUrl ?? (url=>url); + + if(storageConfig.serverRoute) { - return storageConfig.generateUrl(`${storageConfig.serverRoute}/${storageKey}/${filename}`); + return generateUrl(`${storageConfig.serverRoute.path}/${storageKey}/${filename}`); } - return storageConfig.generateUrl(`/${filename}`) + return generateUrl(`/${filename}`) }, async upload (stream, filename) { const writeStream = fs.createWriteStream(path.join(storageConfig.storagePath, filename)) diff --git a/packages/core/src/lib/assets/s3.ts b/packages/core/src/lib/assets/s3.ts index f3243cbbd8d..a495bea209f 100644 --- a/packages/core/src/lib/assets/s3.ts +++ b/packages/core/src/lib/assets/s3.ts @@ -11,7 +11,7 @@ export function s3ImageAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }, return { async url(id, extension) { if(storageConfig.serverRoute) { - return generateUrl(`${storageConfig.serverRoute}/${storageKey}/${storageConfig.pathPrefix || ''}${id}.${extension}`); + return generateUrl(`${storageConfig.serverRoute.path}/${storageKey}/${storageConfig.pathPrefix || ''}${id}.${extension}`); } if (!storageConfig.signed) { @@ -54,7 +54,7 @@ export function s3FileAssetsAPI(storageConfig: StorageConfig & { kind: 's3' }, s return { async url(filename) { if(storageConfig.serverRoute) { - return generateUrl(`${storageConfig.serverRoute}/${storageKey}/${storageConfig.pathPrefix || ''}${filename}`); + return generateUrl(`${storageConfig.serverRoute.path}/${storageKey}/${storageConfig.pathPrefix || ''}${filename}`); } if (!storageConfig.signed) { return generateUrl(`${s3Endpoint}${storageConfig.pathPrefix || ''}${filename}`) diff --git a/packages/core/src/lib/server/createExpressServer.ts b/packages/core/src/lib/server/createExpressServer.ts index 238c027a71f..53654c676e2 100644 --- a/packages/core/src/lib/server/createExpressServer.ts +++ b/packages/core/src/lib/server/createExpressServer.ts @@ -149,10 +149,13 @@ const proxyStorageIfNeeded = (storageConfigKey: string, storageConfig: StorageCo if (storageConfig.serverRoute) { const { isAccessAllowed } = storageConfig; + let storageAccessControl: express.RequestHandler = async (request, response, next) => { + next(); + } if (isAccessAllowed) { - const storageAccessControl: express.RequestHandler = async (request, response, next) => { + storageAccessControl = async (request, response, next) => { const fileKey = request.params[0]; if (!fileKey) { @@ -174,41 +177,42 @@ const proxyStorageIfNeeded = (storageConfigKey: string, storageConfig: StorageCo response.status(403).send('Forbidden'); return; } - + next(); }; - const storageProxy: express.RequestHandler = async (request, response, next) => { - const fileKey = request.params[0]; + } - //Leave the local storage to express.static as it was in the original code of Keystone - if (storageConfig.kind === 'local') { - next(); - return; - } + const storageProxy: express.RequestHandler = async (request, response, next) => { + const fileKey = request.params[0]; - //S3 downloads are handled by the s3AssetsAPI - try { - const assetApi = storageConfig.type === 'image' ? s3ImageAssetsAPI(storageConfig) : s3FileAssetsAPI(storageConfig); - await assetApi.download(fileKey, response, (key: string, value: string) => { - response.header(key, value); - }); - } catch (e) { - console.error(e); - response.status(500).send('Failed'); + //Leave the local storage to express.static as it was in the original code of Keystone + if (storageConfig.kind === 'local') { + next(); + return; + } - } + //S3 downloads are handled by the s3AssetsAPI + try { + const assetApi = storageConfig.type === 'image' ? s3ImageAssetsAPI(storageConfig) : s3FileAssetsAPI(storageConfig); + await assetApi.download(fileKey, response, (key: string, value: string) => { + response.header(key, value); + }); + } catch (e) { + console.error(e); + response.status(500).send('Failed'); - response.end(); - return; } - expressServer - .route(`${storageConfig.serverRoute.path}/${storageConfigKey}/*`) - .get(storageAccessControl) - .get(storageProxy); + response.end(); + return; } + expressServer + .route(`${storageConfig.serverRoute.path}/${storageConfigKey}/*`) + .get(storageAccessControl) + .get(storageProxy); + if (storageConfig.kind === 'local') { expressServer.use(`${storageConfig.serverRoute.path}/${storageConfigKey}`, express.static(storageConfig.storagePath, {