Skip to content

FR: ACL for Parse.File #7001

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

Open
dblythy opened this issue Nov 10, 2020 · 6 comments
Open

FR: ACL for Parse.File #7001

dblythy opened this issue Nov 10, 2020 · 6 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Nov 10, 2020

Currently, to access a file, you only need the URL. Once someone has the file URL, it's there's forever. Although the file names are long random strings, some deeper security might be required for certain files.

I'm working on a PR that could hopefully help manage Parse.Files a bit better, which could work towards keeping track of file reference counts and "cleaning up" files with no references. I've also worked on some other features:

This PR, creates a "_File" object on the save of a file, which generates an OTP secret. The OTP secret is then used to create a token when a Parse.Object with a Parse.File is requested (providing the auth is valid for the "_File" acl).

The file url will now have a token attached to it, which expires in 5 min, meaning that the GET request can be performed, but the URL won't be able to pull the file later (unless ACL is set to public read).

I added 'references' to the '_File' object, and I was thinking when a Parse.Object is saved referencing a file, increment 'references', and when it changes or removes the file, decrement 'references'. Then, if 'references' == 0, delete the file.

I've currently passed the ACL through setTags (for proof of concept) as there's no setACL on Parse.File yet.

I'm also not sure how running .save on an existing file would affect this (E.g changing data or changing ACLs. Maybe I should query "_File" first?).

I've started working on a draft PR as to how I imagine this feature being implemented. Any thoughts or suggestions are valued.

This is in relation to #6572, #5080, #1023, #6780 and this discussion in the community forum.

@davimacedo
Copy link
Member

Some comments:

  • I'd try to address the references issue in one PR and the security in another;
  • The ACL will be set when uploading the file, right? It would be good if it could be also set using triggers. Some kind of CLP will also exists?
  • I'd make the 5 min a parameter
  • The temporary url should be addresses in the adapter side since, for example, S3 already provide off-the-shelf way to generate a temporary url.
  • We need to think how all of these will work when using S3 adapter with oprion directAccess set to true.

@mtrezza
Copy link
Member

mtrezza commented Nov 10, 2020

This could also be a stepping stone towards "Cleaning Up Files".

The most recent discussion has been in
#6780. I suggest we continue the discussion there regarding file reference / clean-up feature as a separate issue.

This PR, creates a "_File" object on the save of a file, which generates an OTP secret.

Shouldn't this be a feature of retrieving a file rather than saving a file?

  • A use case may require to retrieve both, the original, permanent URL in one scenario and a temporary, token-based URL in another scenario. Is this possible with the current suggestion?

  • What about files that are already uploaded, how would one enable that feature to increase security in an existing deployment?

Instead of the current approach, how about introducing a new Parse Server configuration option where one can specify which files, when retrieved, come with a temporary URL. The specification should be in RegEx for flexibility.

Example:

files: {
  temporaryUrlExpiration: 60, // expire temporary URL after 60 seconds
  temporaryUrlClasses: [
    '*', // files in any class in any field
    'Videos/*', // files in 'Videos' class in any field
    'Images/file', // files in 'Images' class in 'file' field
  ]
}

The master key should still retrieve the permanent URL, so that a developer can retrieve

  • the temporary URL with the user session and
  • the permanent URL with the master key.

@dblythy
Copy link
Member Author

dblythy commented Nov 10, 2020

Thank you for the suggestions.

Shouldn't this be a feature of retrieving a file rather than saving a file?

The OTP secret used to generate the token is saved on create of a file. The OTP secret is a long, private string used to create and verify TOTP codes. That secret is then used on find of a file in a object to create a token in the URL, which is validated when retrieving the file. The secret is never exposed to the client, and is always the same for one ParseFile, but different for each one.

What about files that are already uploaded, how would one enable that feature to increase security in an existing deployment?

Ideally, file.setACL, and then file.save. But is there an updateHandler for file.save, or only createHandler?

Instead of the current approach, how about introducing a new Parse Server configuration option where one can specify which files, when retrieved, come with a temporary URL. The specification should be in RegEx for flexibility.

I like the idea, however I don’t think there would be any way to tell during a GET FILE request the difference between the classes that the file is reference to. Unless we add ‘parent=class’ to the URL but that could be easily changed by the user.

The master key should still retrieve the permanent URL.

Yeah, good call. Can easily add that to the GET endpoint.

@dblythy
Copy link
Member Author

dblythy commented Nov 10, 2020

The ACL will be set when uploading the file, right?

Correct, with my approach the ACL can be changed in an existing fileTrigger as well.

The temporary url should be addresses in the adapter side

Ok, so move the functionality to the adapters, but ignore S3?

@davimacedo
Copy link
Member

Ok, so move the functionality to the adapters, but ignore S3?

We can also implement for s3 adapter. direct access set false, it would basically same thing of the others, for direct access set true we can use aws service for generating the tempo urls.

@mtrezza mtrezza added the type:feature New feature or improvement of existing feature label Nov 11, 2020
@dblythy
Copy link
Member Author

dblythy commented Dec 13, 2020

@mtrezza my mistake, it would be easy to interpret which class the file came from during the expandFilesInObject function. Building it this way would need:

  1. Skip all if directAccess and S3.
  2. on expandFilesInObject, if the object has a file and temporaryFiles contains the object class, create a _FileToken with the file reference, random Token, and Token Expiry (expiry duration based on temporaryFiles config). Append the token to the File URL.
  3. On getHandler of file, if query _FileToken for the file and token expiry greater than now, and if objects exist, require a token parameter. Loop through valid tokens and throw if the token doesn't match any existing tokens.
  4. Delete _FileToken object after expiry duration (is this within the scope of parse server, as would need a cron scheduler, or would it need to be a cleanup file token script / route?).

Limitations:

-If any temporary files are used, every file get request will have to check for a token, even those with a masterKey, as it's impossible to know which GET requests are performed with a masterKey and which ones aren't.
-Every query of a file will bloat the FileToken DB, considering a new token will be created every time a file is encoded.

Have I interpreted your proposal correctly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

3 participants