Skip to content
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

feat(object): Allow for presigning upload url #244

Closed

Conversation

MagnusHJensen
Copy link

@MagnusHJensen MagnusHJensen commented Dec 9, 2022

What kind of change does this PR introduce?

Introduces a new feature to sign an upload url, which allows for uploading straight to the bucket without credentials.

What is the current behavior?

It's currently not possible to upload a file from the client side, without routing it through your own server, which is not the preferred way.

What is the new behavior?

You can now presign a upload url with your service token, and give the url to any client which can then upload the file directly.

Additional context

Closes #81

Add any other context or screenshots.
First PR for the Storage API

@fenos
Copy link
Contributor

fenos commented Dec 11, 2022

@MagnusHJensen great work with this PR!
This feature is indeed very useful.

@fenos
Copy link
Contributor

fenos commented Dec 12, 2022

This week i'll spend sometime to check it out 👍

@MagnusHJensen
Copy link
Author

Sounds great @fenos
Let me know of any findings or questions.

@fenos
Copy link
Contributor

fenos commented Jan 4, 2023

Hello @MagnusHJensen sorry for the delay, I was away during the Christmas holiday.

I had time to look over the PR and generally looks good.
However, one important aspect that we need to get right is to understand who has permission to create the upload URL.

Practically, we think that whoever has the right to upload to a specific path it should be able to generate the upload URL for that location.

In order for us to be able to 'test' if a user has the right permission to create a row is to create a record and somehow roll it back within a transaction

try {
found = await this.findObject(objectName)
} catch (e) {
// Don't do anything, since it will throw an error if the object doesn't exist (but that is what we want.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work reliably,
If we have certain RLS policies defined this might return 404 because we don't have access to the file not because it doesn't exist

We might want to run this query asSuperUser to make sure the file is not present at this location.

However, when doing this as superUser we don't have any protection on who can create these upload URL.

As a second step, after the file existence, we need to check if the user has the CREATE RLS permission in order to generate the upload url

@MagnusHJensen
Copy link
Author

Hi @fenos
Feel free to take over this PR if you have the time and find the feature needed enough.
I don't think I will have the time in the near future, for finishing this. If it still stands once I do have time, I will finish it up

@MagnusHJensen MagnusHJensen marked this pull request as draft February 6, 2023 07:00
@subvertallchris
Copy link

Sorry to be annoying with a +1 that doesn't contribute anything but... I hope this makes it in, it would be a fantastic feature.

@fenos
Copy link
Contributor

fenos commented Mar 8, 2023

Closing in favour of #81

@fenos fenos closed this Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signed urls for upload
3 participants