Skip to content

Adds authentication to file retrieval #3897

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

Closed
wants to merge 3 commits into from

Conversation

davidrichard23
Copy link

This is my first time contributing so I'm not sure if this was the appropriate way to add this feature. Please let me know if there was a better way or any other pointers you have. Thanks!

#3887

@dplewis
Copy link
Member

dplewis commented Jun 9, 2017

I have a website that shows financial documents and the URL are accessible to anybody that has the URL.

What would a URL look like in a img tag vs /files route are they the same?

@hhanesand
Copy link

@dplewis I'm pretty sure they would be the same.

@dplewis
Copy link
Member

dplewis commented Jun 11, 2017

@hhanesand thanks that's what I thought

@davidrichard23
Copy link
Author

@dplewis yup, they'd be the same. For any file type, you just need to append /<REFERENCING_CLASS_NAME>/<REFERENCING_KEY_NAME>/<SESSION_TOKEN> to the file URL. If authenticatedFileRetrieval == true and you do not append the URL, the URL will return as unauthenticated.

@flovilmart
Copy link
Contributor

@davidrichard23 would you be against an implementation based upon a HTTP header instead of one that overloads the path? I would probably make for clearer error messages.

@flovilmart
Copy link
Contributor

Also there's a bug flow in the design of that feature, any logged in user has access to it's own profile, which would make an easy path to build, and therefore able to access any file protected with those accesses.

@davidrichard23
Copy link
Author

I don't really have too much experience dealing with HTTP headers, but after looking into it a little, I think you're right, it would be better. I'll try to update it.

@davidrichard23
Copy link
Author

which would make an easy path to build, and therefore able to access any file protected with those accesses.

Can you share more details on this? I'm not sure I understand fully

@flovilmart
Copy link
Contributor

I understand you want to authenticate the file retrieval based on the ability of a particular session token to a particular object.
Every logged in user have at least access to their own profile, so a malicious attacker could leverage that in order to retrieve the seemingly 'protected' files

@davidrichard23
Copy link
Author

Maybe I'm missing something. The auth function queries for the file at the specified key in the specified class using the filename. If the key or class is changed by a malicious user, the auth would fail because the query wouldn't find the file in whatever class/key the malicious user used because a file with that name doesn't exist in that location, right?

@flovilmart
Copy link
Contributor

Yeah, I missed that part sorry. Also I don't see any tests

@davidrichard23
Copy link
Author

I'm new to this whole process, sorry. Where would the test go? I don't see a spec file for for FilesRouter. Should I create one or should I place the test in a different file?

@flovilmart
Copy link
Contributor

Yes you can add the tests in a new FilesRouter.spec.js

@@ -10,7 +10,7 @@ export class FilesRouter {

expressRouter(options = {}) {
var router = express.Router();
router.get('/files/:appId/:filename', this.getHandler);
router.get('/files/:appId/:filename/:referencingClass?/:referencingKey?/:sessionToken?', this.getHandler);
Copy link
Contributor

Choose a reason for hiding this comment

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

could we split the 2 endpoints for the sake a readability and ease of maintenance?

This way the endpoint that validates the auth calls the getHandler if everything is authorized. This will remove the clutter in the getHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even better probably make it a middleware to not clutter the logic inside the function

@davidrichard23
Copy link
Author

Sorry for the delay, I've been super busy with work. I'll try and get to it soon!

@chderen
Copy link
Contributor

chderen commented Aug 22, 2017

Why not use the _id of the document for faster search?
limit the result to 1. and select (keys option) only the _id?

The sessionToken is not always required, in some cases the file may be public.

Thanks

@chderen
Copy link
Contributor

chderen commented Aug 29, 2017

The code is missing:

Parse.applicationId = config.applicationId;
Parse.javascriptKey = config.javascriptKey || '';
Parse.masterKey = config.masterKey;

before making the rest request.
when running more than one application, it's required.

Thanks

@jasonm1
Copy link

jasonm1 commented Jan 25, 2018

Any updates on this functionality? I would also find it useful to have authentication for accessing particular files.

@stale
Copy link

stale bot commented Sep 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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.

6 participants