Skip to content

support file DELETEs #354

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

Merged
merged 1 commit into from
Feb 12, 2016
Merged

Conversation

westhom
Copy link
Contributor

@westhom westhom commented Feb 11, 2016

e.g.

curl -X DELETE \
        -H "X-Parse-Application-Id: <app_id>" \
        -H "X-Parse-Master-Key: <master_key>" \
        http://localhost:1337/files/2e85ede1e439506909e1a388710fc2b8_hey.jpg

Returns empty 200 on success, but probably wants to return useful JSON instead? Fixes #340

@@ -20,6 +20,17 @@ export class GridStoreAdapter extends FilesAdapter {
});
}

deleteFile(config, filename) {
return config.database.connect().then(() => {
let gridStore = new GridStore(config.database.db, filename, 'r');
Copy link
Contributor

Choose a reason for hiding this comment

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

r stands for reading mode here, I honestly expect it not to work for file deletions.

@nlutsenko
Copy link
Contributor

This is pretty exciting! Good job on implementing this!
Except of that nit, few thoughts and requests:

  • Can we have at least a single e2e test? Look at some of the basic tests in ParseFile.spec.js, we probably would want something REST-api based and e2e full (create/read/delete/read).
  • We need to enforce master-key security on this, since anyone with an appid/any-key will be able to delete files, which is pretty bad.

@westhom
Copy link
Contributor Author

westhom commented Feb 11, 2016

@nlutsenko Thanks for reviewing. I'll add in an e2e test to the PR. Regarding master-key security, I have a check in the deletionHandler for req.auth.isMaster, which seems to be working-- I can't delete images if I don't have X-Parse-Master-Key set correctly. Better way of doing this?

In any event, I'll also add a test for unauthenticated deletion attempt.

@nlutsenko
Copy link
Contributor

Sorry, totally missed it, you are absolutely correct.

@facebook-github-bot
Copy link

@westhom updated the pull request.

@facebook-github-bot
Copy link

@westhom updated the pull request.

@facebook-github-bot
Copy link

@westhom updated the pull request.

@westhom
Copy link
Contributor Author

westhom commented Feb 12, 2016

Added to ParseFile spec:

  • REST file end-to-end coverage for create, read, delete, read
  • verify you cannot delete file with incorrect or missing X-Parse-Master-Key

@facebook-github-bot
Copy link

@westhom updated the pull request.

@nlutsenko
Copy link
Contributor

Looks great! So glad we have this now!

nlutsenko added a commit that referenced this pull request Feb 12, 2016
@nlutsenko nlutsenko merged commit e6ef0ae into parse-community:master Feb 12, 2016
@ishaan1995
Copy link

Closed issue--> #340
The file delete is still not working, i have update the parse-server code today, that has the commit for file delete, but still the same error occurs as mentioned above.
It seems as the master key is doing nothing because even if i do not enter the master key, i get that same error--> cannot delete /parse/....

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.

4 participants