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

Question about CORS / Cordova - Configuration for _resumable - Endpoint? #90

Open DanielDornhardt opened 8 years ago

DanielDornhardt commented 8 years ago

Hello @vsivsi,

I tried to use the new CORS - support for our file-collections according to the documentation, but I couldn't get it to work.

Do we need to add CORS-Responses to the _resumable - Endpoint manually? Or is it covered otherwise with proper CORS-Response-Headers? (I'm trying to find out if there's a good general solution for CORS Handling here on the Forums over here btw: What should be the default CORS Access-Control-Allow-Origin - Policy for custom HTTP Endpoints?)

Collections.Photos = new FileCollection('photos', {
    resumable: true,
    resumableIndexName: 'photos',
    http: [{
        method: 'get',
        path: '/md5/:md5',                  // this will be at route "/gridfs/photos/md5/:md5"
        lookup: function (params, query) {  // uses express style url params
            return { md5: params.md5 };     // a query mapping url to photos
        }
    }, {
        method: 'get',
        path: '/_id/:_id',                  // this will be at route "/gridfs/photos/_id/:_id"
        lookup: function (params, query) {  // uses express style url params
            return { _id: params._id };     // a query mapping url to photos
        }
    }, {
        method: 'options',  // Enable an OPTIONS endpoint (for CORS)
        path: '/_resumable',  // this will be at route "/gridfs/photos/_resumable"
        handler: function (req, res, next) {  // Custom express.js handler for OPTIONS
            res.writeHead(200, {
                'Content-Type': 'text/plain',
                'Access-Control-Allow-Origin': req.headers.origin,
                'Access-Control-Allow-Methods': 'GET, PUT'
            });
            res.end();
            return;
        }
    }
  }]
})

Is this how it's supposed to work? Because with or without this the uploads don't seem to work. I feel that the resumable server should probably be configured automatically (and maybe the other routes too).

Thanks you for listening and best wishes!

vsivsi commented 8 years ago

Hi, the one problem I can spot immediately is that resumable.js uses POST, GET and HEAD requests, but not PUT for uploading data. That may fix it. I don't know anything about req.headers.origin as an origin, is that a valid value in your case?

Also, there was this reported requirement (from another issue):

Note!: Reportedly due to a bug in Cordova, you need to add the following line into your mobile-config.js App.accessRule("blob:*");

Not sure if you're doing this for Cordova, or some other application, but thought I'd mention it.

As for the question of whether this should be "automatic" for the resumable.js endpoint... I considered that, but ultimately settled on this more explicit options definition method, at least for now. The same origin policy exists for good reasons, and so opening holes in that via CORS is a choice that should be explicit for the developer, IMO.

With that said, the documentation for the specifics of enabling CORS for the /_resumable endpoint is inadequate, as you've discovered. Once you get it working, I'd be happy to accept your working code as an example of how to do it, and add that to the CORS section in the README.

Thanks!

DanielDornhardt commented 8 years ago

Hi, boys are back in town!

It took a while, but I finally got around trying to get this working again instead of our in-house "hacked-into-pieces-and-with-manual-setting-of-headers-added-everywhere" - fork of meteor-file-collection :)

But I still can't get it to work it seems.

The issue I see is that as far as I can see, it's not enough to just set the access-control-headers once in the pre-flight request. We also need to be able to set it to the responses to the eg. POST - Requests which contain the individual chunks which should get uploaded.

And I can't see how to get a "handle" (a handler) on that?

I tried with this:

a) for the 'options' handler - I got it to work by using a dummy lookup function - without it wouldn't run - what'd be a correct lookup for the _resumable options-request? Do I need any?

It looks like this now:

{ method: 'options',  // Enable an OPTIONS endpoint (for CORS)
    path: '/_resumable',  // this will be at route "/gridfs/myFiles/_resumable"
    lookup: function (params, query) {  // uses express style url params
        return {};     // a query mapping url to photos
    },
    handler: function (req, res, next) {  // Custom express.js handler for OPTIONS
        log('************** HANDLING OPTIONS REQUEST ***********')

        if (req.headers && req.headers.origin && (cordovaClientOriginRegex.test(req.headers.origin) || req.headers.origin === 'http://meteor.local')) {
            res.writeHead(200, {
                'Content-Type': 'text/plain',
                'Access-Control-Allow-Origin': req.headers.origin,
                'Access-Control-Allow-Headers': 'x-auth-token',
                'Access-Control-Allow-Methods': 'GET, PUT, POST'
            })
        }

        res.end()
        return
    }
}

This works for the first OPTIONS request, but then the POST - Requests still fail because they're missing the required headers. I tried to stuff them in like this:

{
    method: 'post',  // Enable an OPTIONS endpoint (for CORS)
    path: '/_resumable',  // this will be at route "/gridfs/myFiles/_resumable"
    lookup: function (params, query) {  // uses express style url params
        log('#### IN POST LOOKUP #####')
        return {};     // a query mapping url to photos
    },
    handler: function (req, res, next) {  // Custom express.js handler for OPTIONS
        log('************** HANDLING POST REQUEST ***********')

        if (req.headers && req.headers.origin && (cordovaClientOriginRegex.test(req.headers.origin) || req.headers.origin === 'http://meteor.local')) {
            res.writeHead(200, {
                'Content-Type': 'text/plain',
                'Access-Control-Allow-Origin': req.headers.origin,
                'Access-Control-Allow-Headers': 'x-auth-token',
                'Access-Control-Allow-Methods': 'GET, PUT, POST, HEAD'
            })
        }

        res.end()
        return
    }
},

Which of course probably wouldn't be the best handler if it'd .end() the response then and there, but anyways, it never gets called.

So, is there a way to add these headers to the POST and other requests using the current meteor-file-collection mechanism?

Or am I on the wrong track?

Thanks & best wishes and once more thank you for this great package of course.

vsivsi commented 8 years ago

Hi, so I'm studying this example linked to from this Mozilla blog post.

It doesn't show any CORS headers added to the POST request that follows the "preflight" OPTIONs request. I haven't seen any such use of the CORS headers in any of the documentation I've seen regarding CORS anywhere.

Probably the best next step to sorting this out is for you to make and share a dead-simple vanilla (browser based, not Cordova) Meteor app that clearly demonstrates the issue. Perhaps it can exploit the fact that 127.0.0.1 and localhost are considered different origins under CORS. That way we can then easily explore this locally using Meteor running in developer mode.

vsivsi commented 8 years ago

Okay, I've been able to replicate this in the file-collection sample app using the localhost vs 127.0.0.1 trick, and I now see that the server response does need to include a CORS header:

Access-Control-Allow-Origin: http://127.0.0.1:3000

Let me think a bit about the best way to implement that...

vsivsi commented 8 years ago

Okay, I needed to make two small modifications to file-collection to get the following code to work... The changes are on the master branch now. With those in place, the following code works for me:

      [
            {
               method: 'head',
               path: '/_resumable',
               lookup: function (params, query) { return {}; }, 
               handler: function (req, res, next) {
                  if (req.headers && req.headers.origin) {
                     res.setHeader('Access-Control-Allow-Origin', req.headers.origin);
                     res.setHeader('Access-Control-Allow-Credentials', true);
                  }
                  next();
               }
            },
            {
               method: 'post',
               path: '/_resumable',
               lookup: function (params, query) { return {}; }, 
               handler: function (req, res, next) {
                  if (req.headers && req.headers.origin) {
                     res.setHeader('Access-Control-Allow-Origin', req.headers.origin);
                     res.setHeader('Access-Control-Allow-Credentials', true);
                  }
                  next();
               }
            },
            {
               method: 'options',
               path: '/_resumable',
               lookup: function (params, query) { return {}; }, 
               handler: function (req, res, next) {
                   if (req.headers && req.headers.origin) {
                     res.writeHead(200,
                       {
                        'Content-Type': 'text/plain',
                        'Access-Control-Allow-Origin': req.headers.origin,
                        'Access-Control-Allow-Credentials': true,
                        'Access-Control-Allow-Headers': 'x-auth-token, user-agent',
                        'Access-Control-Allow-Methods': 'POST, HEAD'
                       });
                     res.end();
                   }
                }
            }
       ]

It's not very DRY but it works. The HEAD and POST handlers are identical. This is obviously a big kludge, but it will work for now while I sort out a more elegant way to optionally do this automatically.

Please try this with the latest master commit and let me know how it goes.

DanielDornhardt commented 8 years ago

Hello @vsivsi, thank you for trying this out on your own.

I'll try this out in the next days.

I think it's very reasonable of you to think about some kind of integration for this for the resumable endpoints because you're bundling resumable and users aren't involved in setting up the endpoints, just enabling it by setting resumable=true.

vsivsi commented 8 years ago

Great, let me know how it goes and I'll package these changes up and release on Atmosphere. Making CORS support more "automatic" will need to wait for a bit, but that'll give me time to think about it. If you have any suggestions,please document them here.

DanielDornhardt commented 8 years ago

Hello vsivsi, i'd say it looks like it's working to me. It'd be great if you could do a new release soon-ish. I tried to create a test-release for ourselves, but I can't seem to get it to build properly at the moment, looks like I'm having some issues with the meteor build servers.

Normally i'm trying to make sure everything works even with our production environment, but in this case i'll give up and declare this as fixed. I'm sure it'll work.

Thank you so much for your great work!

vsivsi commented 8 years ago

Okay, I'll release sometime today.

vsivsi commented 8 years ago

These latest changes are in 1.3.4, on atmosphere now.