search

vsivsi / meteor-file-collection

Extends Meteor Collections to handle file data using MongoDB gridFS.
http://atmospherejs.com/vsivsi/file-collection
Other
159 stars 37 forks source link

Separate permissions for OPTIONS (for Cordova) #167

Open edemaine opened 6 years ago

edemaine commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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