Add support for proxying images and files always via keystone express server, no matter local or s3, and add security access control extension hook

[sunnymoon]

Implemented proxied mode (it was referred to in the documentation and examples as creating a server route on express, but it didn't for s3 case) in s3 as well as leaving the same "static express mapping" as before for local storage mode.
Implemented an access Control hook extension point to validate access to either local or s3 assets with proxied mode
Fix the documentation to conform to "serverRoute" always (instead of distinguishing between s3 or local kinds).
Still feel the code could be further improved by:

1. Not allowing "generateUrl" to be implemented on the case of "proxied" mode (serverRoute.path defined)
2. Check for "overlaps" between the "serverRoute.path" and other express middlewares at startup and warning the devs about it
3. Implement the pathPrefix option (correctly on s3) on local storage kind (and mkdir accordingly at startup)
4. Check for "signed" vs "serverRoute.path" vs "acl" options and warn devs appropriately

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 79620ca:

Sandbox	Source
@keystone-6/sandbox	Configuration

[sunnymoon]

Is anyone looking at PRs? How could I help further ?

[pmatrola-linkare]

I need this pull request to use in my project

[dcousens]

Hi @sunnymoon, I am reviewing pull requests, but I haven't had a chance to test this particular pull request yet.

If anyone else is up to help review and test, that will be appreciated.

[sunnymoon]

Hello Daniel: Thanks for getting back. In terms of the review what or whom would you suggest could do the PR analysis... Someone from our Team/Company? Best regards José A sexta, 1/03/2024, 12:18, Daniel Cousens ***@***.***> escreveu:

…

Hi @sunnymoon <https://github.com/sunnymoon>, I am reviewing pull requests, but I haven't had a chance to test this particular pull request yet. If anyone else is up to help review and test, that will be appreciated. — Reply to this email directly, view it on GitHub <#9001 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBHUKAI3RUR2SYGG7TPWM3YWBWXXAVCNFSM6AAAAABCVKKCB6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZTGA4DQOJRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dcousens]

@sunnymoon sorry I missed your question in March, any review is helpful, and this pull request is needing a rebase and preferably an eslint --fix call 💛

[dcousens]

Let's drop unnecessary changes from this pull request

[dcousens]

If this only works if serverRoute is set, we should probably put this in the serverRoute object

[dcousens]

I'm wondering if this is how you should approach this generally, something seems like an anti-pattern somewhere so I might need to look at this more conceptually first.

[sunnymoon]

Hi! Which unnecessary changes are you referring to? A quinta, 23/05/2024, 02:55, Daniel Cousens ***@***.***> escreveu:

…

***@***.**** commented on this pull request. ------------------------------ In packages/core/src/lib/server/createExpressServer.ts <#9001 (comment)>: > import { ApolloServerPluginLandingPageDisabled } from ***@***.***/server/plugin/disabled' import { ApolloServerPluginLandingPageLocalDefault } from ***@***.***/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' Let's drop unnecessary changes from this pull request — Reply to this email directly, view it on GitHub <#9001 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBHUKE2QYXQ2PGYGSYN223ZDVEBRAVCNFSM6AAAAABCVKKCB6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZSGY4DENZVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sunnymoon]

We should and I wonder why it wasn't done like that from the start. You can actually only allow Access if you control the serving of the bytes... Otherwise anyone can share the link after it has been generated (even with S3 presigned URLs). However, talking about "leaving unnecessary changes out of this PR", changing this would break user space (which I didn't want to go as far, hoping for the PR to be integrated sooner and easier). Do you still think we should go all the way and break previous installations of keystone6 ? A quinta, 23/05/2024, 02:57, Daniel Cousens ***@***.***> escreveu:

…

***@***.**** commented on this pull request. ------------------------------ In packages/core/src/types/config/index.ts <#9001 (comment)>: > + 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?: (options: StorageAccessAllowedOptions) => boolean, If this only works if serverRoute is set, we should probably put this in the serverRoute object — Reply to this email directly, view it on GitHub <#9001 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBHUKDQFTK66YQ7USENUL3ZDVEG5AVCNFSM6AAAAABCVKKCB6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANZSGY4DGNZZGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sunnymoon]

Hi. I don't see an anti pattern but rather I would call it a smell. My thoughts on the subject is that createExpressRoute and 'download api' seems to be way too tightly coupled. I don't know if this is also your thoughts on the subject, and would like to hear from you. As far as I see it, support for S3 (or filesystem or any other kind of storage) shouldn't be this baked in, but rather some sort of "pluggable libraries" or "extensions" that could be contributed to keystone6 as additional libraries... However, once again, I didn't want to break any previous existing ideas, APIs, usage of keystone6 in the wild and followed as much as possible whichever was developed previously. I do believe we could add value by integrating as it is (maybe with the rebasing and eslinting, yes, obviously) and make future plan for a complete rewrite of "storage extensions"... What do you think ? A quinta, 23/05/2024, 03:03, Daniel Cousens ***@***.***> escreveu:

…

I'm wondering if this is how you should approach this generally, something seems like an anti-pattern somewhere so I might need to look at this more conceptually first. — Reply to this email directly, view it on GitHub <#9001 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBHUKGCQVCGCRZK4UYJEZTZDVE67AVCNFSM6AAAAABCVKKCB6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRWGA2TONJVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>