vsivsi/meteor-file-collection

Separate permissions for OPTIONS (for Cordova)

edemaine opened this issue · 3 comments

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.

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?

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.

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