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

Separate permissions for OPTIONS (for Cordova) #167

Open
edemaine opened this issue Oct 12, 2017 · 3 comments
Open

Separate permissions for OPTIONS (for Cordova) #167

edemaine opened this issue Oct 12, 2017 · 3 comments

Comments

@edemaine
Copy link
Contributor

I just got meteor-file-collection working on Android via Cordova, but I ran into an issue that could use some tweaks to these lines:

                     when 'OPTIONS'  # Should there be a permission for options?
                        unless (share.check_allow_deny.bind(@)('read', req.meteorUserId, req.gridFS) or
                                share.check_allow_deny.bind(@)('write', req.meteorUserId, req.gridFS) or
                                share.check_allow_deny.bind(@)('remove', req.meteorUserId, req.gridFS))

My conclusion is that the answer to # Should there be a permission for options? is essentially "no". The issue is that the browser triggers a preflighted request without sending the X-Auth-Token cookie (as claimed here and verified by extensive experiments). Thus I need to allow OPTIONS requests from everyone, without authorization, just to tell the browser "yes send your credentials". But I don't want to change my read, write, remove allow rules which do actual authentication once I have the X-Auth-Token.

I see two options:

  1. Remove permission checking from OPTIONS altogether. There are no default OPTIONS actions anyway, so any needed permission checking could be done manually when defining a handler (and I'm not sure it'd make sense anyway).
  2. Add an allow/deny rule specific to options just like we have one for read, insert, write, remove. Presumably then we'd just call check_allow_deny for options instead of or-ing them all together, but I don't really care either way.

Happy to put together a PR once you decide which option you'd like.

@edemaine
Copy link
Contributor Author

I haven't tested this, but I worry about another issue: OPTIONS is currently running a query (in the README demo, the "everything" query {}), and bailing if that query doesn't return anything. I believe this means it will be impossible to initiate an upload into an empty collection, when dealing with CORS. (In my case, the collection is already nonempty, from non-Cordova connections.)

I believe it would make sense to simply skip the lookup when lookup is null, instead of bailing. Thoughts?

@vsivsi
Copy link
Owner

vsivsi commented Oct 12, 2017

Hi, thanks for the very clear distillation of what you are seeing here.

Honestly, the OPTIONS request handling in FC was kind of a kludge just take the shortest path to get CORS working.

A better long-term solution is probably to just adopt the express.js CORS middleware and provide a nice, well documented way to add it in.

You can probably already accomplish that using the handler functionality in FC, by just patching in the CORS middleware, but that's a kludge on a kludge given that FC is already using express under the hood.

Anyway, if you were to put together a PR, I would prefer it to be along those lines (integrating support for express.js CORS middleware) rather than continuing down the path of patching-up the OPTIONS request support. My 2c.

@vsivsi
Copy link
Owner

vsivsi commented Oct 12, 2017

BTW https://github.com/edemaine/coauthor looks like a cool project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants