-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(medusa, medusa-file-*): Protected uploads for file services #2433
Conversation
🦋 Changeset detectedLatest commit: ba88998 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c130bb0
to
a2283fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very nice - great work @pKorsholm! I have a single thought, that I'd like your feedback on.
@@ -19,9 +19,19 @@ class S3Service extends AbstractFileService { | |||
upload(file) { | |||
this.updateAwsConfig() | |||
|
|||
return this.uploadFile(file, "public-read") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: It seems the interface is used slighly differently from plugin to plugin. Here we use it to directly pass the access level while in Minio it's a flag. Would it make sense to consistently use uploadFile
with a flag to indicate the privacy level?
This would mean, that we'd have to have the following in S3:
uploadFile(file, makePrivate) {
const s3 = new aws.S3()
const params = {
ACL: makePrivate ? "private" : "public-read",
...
And the following in Minio:
uploadFile(file, makePrivate) {
const parsedFilename = parse(file.originalname)
const fileKey = `${parsedFilename.name}-${Date.now()}${parsedFilename.ext}`
const s3 = new aws.S3()
const params = {
ACL: makePrivate ? "private" : "public-read",
Bucket: makePrivate ? this.private_bucket_ : this.bucket_,
....
And so forth for all plugins.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty agree on that in order to make the api consistent and easier to use. Would just say that the second argument could be like:
uploadFile(file, options: { isProtected?: boolean } = { isProtected: false })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that. This would also allow us to add an ACL-specific prop in case users need more granular access control:
uploadFile(file, options: { isProtected?: boolean, acl?: string } = { isProtected: false })
6903a2a
to
85388da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great work!
What
Why
How
Testing