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

Add enforceMasterKeyAccess middleware. #378

Merged
merged 1 commit into from
Feb 12, 2016

Conversation

nlutsenko
Copy link
Contributor

If we want to enforce master key access on any endpoint - we will have middleware for it right now.
Also migrated FilesController to use it now, hence there is validation for it.

@facebook-github-bot
Copy link

@nlutsenko updated the pull request.

@drew-gross
Copy link
Contributor

Is is possible to use a more descriptive error message? Half of our bug reports are people seeing "unauthorized" when they think they shouldn't and returning "unauthorized: master key required" or something like that might help alleviate that problem.

Is it also possible to update the other places that check for master key? (schemas api?) that would be handy.

I don't really care about changing error messages despite it technically being backwards incompatible, but changing error codes is a little different. I think in this case it's fine, but we should document somewhere that if you are relying on specific error codes/status codes in your scripts, that they may have changed.

@nlutsenko
Copy link
Contributor Author

Re: Error codes
I double checked the behavior on parse.com - it actually just says unauthorized without any error code for master-key based enforcing on schemas/roles/files.

Re: Schemas api
I tried to attack PromiseRouter with the custom middleware, but we need a slightly better story there, since our current model doesn't quite work for it, as it's using a single promise chain.
Will look more into it tomorrow, but I think we might need to extend a promise router to accept non-promise based routes, something like Express itself does it with middleware.

Re: Changing the error message
Agreed, we should be a slightly better resident.
I am going to change the error message here: unauthorized: master key is required.

@nlutsenko nlutsenko force-pushed the nlutsenko.middleware.master branch from 8cac7f5 to f53cb60 Compare February 12, 2016 07:26
@facebook-github-bot
Copy link

@nlutsenko updated the pull request.

nlutsenko added a commit that referenced this pull request Feb 12, 2016
@nlutsenko nlutsenko merged commit 6c6021a into master Feb 12, 2016
@nlutsenko nlutsenko deleted the nlutsenko.middleware.master branch February 12, 2016 07:33
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.

3 participants