Set x_mtok Cookie on Cordova

[s-ol]

This PR supersedes #659. It is more lightweight and cleans up all the comments from that PR.

For fixing the cookie transport problem, it uses the ostrio:cookies send() method.
This method, or rather all of ostrio:cookies, was broken for Cordova previously.
A fix for all of this has been provided in veliovgroup/meteor-cookies#11 and is a precondition for this PR.

Changes in this PR:

• use better hooks for refreshing the mtok-cookie (tested on Cordova and browser, works with server restarts)
• trigger cookie.send() on Cordova
• set Access-Control-Allow-Credentials: true and Access-Control-Allow-Origin for http://localhost:12000-13000 on /cdn/... routes
• respond to OPTIONS HTTP requests correctly to enable CORS uploads

[vbelolapotkov]

@s-ol I can say you for sure that hardcoded port number won't work. Meteor is using a random port number in the range 12000-13000 for the app based on appId as written in docs.

So far I experienced additional CORS issues because of missing header Access-Control-Allow-Headers in a response to the OPTIONS request (see details in #670 ).

I'd try to implement a hook based solution similar to responseHeaders option to setup custom headers or workaround that problem only when it makes sense for the app.

Unfortunately I don't have that much time right now to submit a proper PR or improvements.

[s-ol]

@vbelolapotkov: good point, i read about that after submitting this too - the port is fixed per app-id though, so it can be fixed e.g. in settings.json or ENV by developers to activate this feature.

I have not included all headers in the PR since we set some by nginx, but you are right, there may be some more required. Especially regarding byte ranges. The x-mtok header for sure is superfluous with Allow-Credentials: true.

[menelike]

I can say you for sure that hardcoded port number won't work. Meteor is using a random port number in the range 12000-13000 for the app based on appId as written in docs

AFAIK the port is randomized and will be different on each application (or at least has a minimized collision chance).

[s-ol]

@vbelolapotkov the port issue is resolved now, I am thinking about moving the remaining headers into responseHeaders as you said.

Had a chance to look at this, @dr-dimitru?

[s-ol]

Added options.disableCORS for disabling all the headers. Moving them into options.responseHeaders didn't work out because they are conditional on info not available there and I didn't want to open that interface up. Besides IMO a Meteor package per default should work on all meteor platforms, so this solution seems reasonable to me.

[s-ol]

@vbelolapotkov your remarks from above are all solved as far as I can tell. Have you tested this current state again?

@dr-dimitru we have been testing this solution and have now been using this branch in production over at @risetechnologies for some time. As far as we are concerned, it finally solves the Cookie Problem with no added user effort or tech complexity in many cases, and low effort in the few necessary cases (fetching files via XHR).

[vbelolapotkov]

@s-ol thank you for the update. This week is super busy for me. Scheduled testing for the next week. Will update you once done.

[dr-dimitru]

Added some notes with minor change request, ready to merge asap

[dr-dimitru]

I have a question for all developers in this thread (@s-ol, @vbelolapotkov, and @menelike, @jankapunkt) — Shall we apply changes only to index.js, which looks more like fixes to the current implementation, but reject the rest of changes?

Let me explain: We can achieve the same results using config.responseHeaders using changes suggested in this PR we can build nice tutorial, so every project can adapt it to their needs, or simply copy-paste it to solve an 🔥 issue.

[dr-dimitru]

That will help to keep package extendable/hackable and lightweight, with good documentation

[menelike]

@s-ol is actually helping me/us (@risetechnologies) to implement this, so I will agree upon his recommendations. All I can say about this at this point is that this finally solves the Cordova authentication issues we had to fight with (including a lot of drawbacks) for the last two years.

Especially scaling meteor is cumbersome without the changes of this PR.

[vbelolapotkov]

@dr-dimitru I've tried to use config.responseHeaders to solve the issue, but it was never called on uploads (OPTIONS, PUT requests) and I constantly had CORS issues. That is why changes to the index were necessary. However, I agree using the config.responseHeaders function + good documentation examples would make the package more flexible and quite easy to use, but the function has to be called properly on each request.

[dr-dimitru]

@vbelolapotkov thank you for valuable feedback

it was never called on uploads (OPTIONS, PUT requests) and I constantly had CORS issues

You're right!

but the function has to be called properly on each request.

Sounds like something we can apply quickly

[s-ol]

@dr-dimitru my main concern has always been first to make it possible in general.

My main reasoning for pushing this into the Meteor-Files source in general is the following:
The problem this fixes is a general problem affecting everyone who uses Meteor and Meteor-Files. Yes, many small projects will never build or need a cordova app, but many projects may grow to the point where it becomes necessary. If you want to remove complexity of the default configuration, I would even consider disabling this by default, although still I think that default behavior of this package should be "fully functional" - i.e. "fix upstream bugs to make myself work always".

In my opinion, if I install Meteor-Files and it doesn't just work on Cordova, then Meteor-Files is buggy.

[s-ol]

In many cases I strongly agree with minimising bloat / splitting out features into extension packages, user code etc.

In this case I think that might be wrong, because this is not really a "user feature" although it can be added as one. I see this more as a core part of what Meteor-Files is trying to do - make uploading files work in all of Meteor.

Lastly (and sorry for the long lamentation) as we know from the long history of this PR and the previous issues, this is a very difficult topic to understand! It took me two attempts of at least two weeks each time, spread apart by a year to figure out what exactly the parameters of the problem are. Most users will be unwilling or unable to do this amount of research by themselves. Do you want to trust users to take your solution into their code, where they have to maintain it themselves, without fully knowing the problem?

[menelike]

Folks let us please set up a common ground as a base for further discussions:

Precondition

The Meteor Cordova implementation is completely incompatible with cookies, either you change the implementation of https://github.com/meteor/cordova-plugin-meteor-webapp or you work around this.

Status quo

Having no cookies means:

1. Http uploads are not supported which is currently solved with using ddp instead, the downside is already documented here https://github.com/VeliovGroup/Meteor-Files/blob/master/docs/insert.md
   Note: upload via http is at least twice faster. HTTP will properly work only with "sticky sessions" Default: ddp

2. In order to download a file from cordova, the xmtok needs to be attached to the src as a get parameter. If for example a download button with a simple href is already mounted in react and you lose the Meteor connection, you need to update that href, otherwise, the authentication will fail on click. Keeping the UI sync is a pain.

Both cases have workarounds, none of them make fun.

The result of this PR

We are using https://github.com/risetechnologies/Meteor-Files/tree/cordova-send1.1.0 in conjunction with https://github.com/risetechnologies/Meteor-Cookies/tree/cordova-send2.4.0

We can use http uploads and don't need to think about xmtok in general. The only downside is that we need to set withCredentials whenever we use to make calls like:

fetch API

      window.fetch(url, {
        method: 'GET',
        credentials: 'same-origin',
      })

HLS.js

hls = new Hls({
        capLevelToPlayerSize: true,
        autoStartLoad: true,
        startLevel: 1,
        // allow LB Cookies to go through on Cordova
        xhrSetup: (xhr) => {
          xhr.withCredentials = true; // eslint-disable-line no-param-reassign
        },
        fetchSetup: (context, initParams) => {
          initParams.credentials = 'include'; // eslint-disable-line no-param-reassign
          return new Request(context.url, initParams);
        },
      });

Please note that we had to do this anyway to support the sticky sessions coming from our load balancer, so those examples are not directly related to Meteor-Files

More importantly, we don't need to do any of this if you just want to, for example, load an <img src="foo" />

Background story

Initially, when we set up our load balancers we observed that cookies set by the server (sticky sessions) were actually used by further requests from the Cordova application, our plan was to make use of that.

The way it works is that the client (Meteor-Files) sends a request to the backend (Meteor-Cookies) with the to be set cookies as get parameters. Those are then responded by the server per res.setHeader('Set-Cookie', cookies);. This is more or less a switch/translation from get parameters to a cookie.

Last words

The implementation details can, of course, be discussed, but if the design behind this isn't acceptable it is nonsense to discuss this PR any further. We just wanted to share this with you folks, if you aren't interested that's absolutely fine, but we should stop wasting time on both ends.

*On a sidenote: veliovgroup/meteor-cookies@d2717c4 Partly implemented (our) suggested changes which doesn't help at all since it doesn't solve the issue.

[menelike]

@dr-dimitru Glad that this got merged! Fantastic.

Please don't forget that this only works in conjunction with https://github.com/risetechnologies/Meteor-Cookies/tree/cordova-send2.4.0

Do you want me to create a pull request for the second part?

Update

Opened a new PR at veliovgroup/meteor-cookies#14