[FIX] Enable CORS for Restivus

[mrsimpson]

@RocketChat/core

Closes #7915

Previously, the setting to enable CORS on the API (Admin > General > REST Api) added the CORS-header, but the information was not propagated to Restivus.

[mrsimpson]

@graywolf336 I'm not really knowledgable about CORS and what the option in Restivus is.
Thus, I was confused by the setting not being propagated like I did now earlier - should the enabling in Restivus be a second setting (semantically)?

[graywolf336]

@mrsimpson only issue with this is that if you change it after the server has started, it won't be reflected until you restart the server. 🤔 so we need to figure out how to dynamically have this setting applied, I will take a look.

[mrsimpson]

@graywolf336 ah, Nö server side reactivity... how about getting the setting to a variable and set this variable additionally in a setting.load()-callback?

[mrsimpson]

@graywolf336 that's what I meant writing y'days comment.
Is that the proper way to do it?

Note to myself: I really should get an IDE on my mobile...

[graywolf336]

You're extremely close! Honestly, I would leave the function the way it is and just change the RocketChat.settings.onload handler to be RocketChat.settings.get('API_Enabled_CORS', () => {}) as that will be called everytime the setting is changed. I have no clue what onload will do 🤔

[graywolf336]

So, after doing some review it seems that get with a callback makes a call to onload and also immediately runs the callback..someone feel free to correct me if I'm misunderstanding things

[mrsimpson]

Is get() really being triggered after update of the setting? onLoad() surely is

[graywolf336]

@rodrigok which one is the preferred way to do what is being discussed here?

[rodrigok]

@graywolf336 Yes, get with callback will be called everytime the setting changes

[rodrigok]

Did you guys tested what happens if you create the same API twice?

[mrsimpson]

@rodrigok you mean whther we tested that changing the CORS-Setting creates a working api?

[rodrigok]

@mrsimpson Yes, now the same API could be create multiple times, is that working ok? Restivus replace the old v1 namespace by the new one correctly?

[mrsimpson]

@rodrigok I logged in via API, changed the setting and logged out via API again.
This was working fine, so to answer your question: it works.

However, when looking at the headers themselves, I noticed that the access-control-allow-origin with * as value was always present.

Disabled:

Enabled:

Thus, I did a quick where-used for RocketChat.settings.get('API_CORS_Origin') but couldn't find it come from any Rocket.Chat-code.
Also, it's not a matter of changing the setting.

So imho, this PR has been tested and does only make things better.
If the rest actually are issues, I'd recommend creating separate issues for them.

@graywolf336, @rodrigok wdyt?

[rodrigok]

The CI is showing a problem that could help by the delayed API creation:

Exception in callback of async function: TypeError: Cannot read property 'helperMethods' of undefined
    at requestParams.js (packages/rocketchat:api/server/v1/helpers/requestParams.js:1:19)
    at fileEvaluate (packages/modules-runtime.js:333:9)
    at require (packages/modules-runtime.js:228:16)
    at /tmp/build-test/bundle/programs/server/packages/rocketchat_api.js:3962:1

Any ideas?

[mrsimpson]

@rodrigok I can't really understand this, to be honest.
The very same script (.scripts/start.js) is executed twice in CircleCI. Once it failed (without oplog), once it was successful (with oplog). I cannot imagine this being a true positive.
Executed locally, the tests also succeed.

However, I noted that the delayed instantiation was done onLoad(), you recommended get(callback) some time back, so I changed that. let's see what the CI says now.

[rodrigok]

@mrsimpson That's why I said could happen. If you are delaying the registration of one API like RocketChat.API.v1, other part of the code could try to use if before the registration.

The solution for that, IMO, is register the normal API as before, without CORS enabled and then use the settings callback to register it again if different from previous registration, in this first case if the setting is true cuz the default registration was false. You could use a control variable to do that or check if you can access the config like RocketChat.API.v1.cors === true.

[mrsimpson]

@rodrigok you seem to have been right with the failure being caused by the delayed API creation:
I added it outside the callback as well and the CI is happy now.
This has the effect of the API being created twice on startup, but since this has only minimal impact on performance and no functional limitations, I believe it should be fine.

[rodrigok]

Oh, I just read your changes, that is almost what I said, can you change to only create the api if the setting value is different of the current value? And check if the first time you get the value from the setting in a sync way if the value is not undefined (set to false if undefined)?

[mrsimpson]

Amazing, how much bugs can be killed one can invest into five lines of code...
Updated, tested, pushed.

[mrsimpson]

I don't like accessing a "private" property for deciding about the buffering, particularly in an untyped world, but I preferred this to buffering the property in some other place.

[rodrigok]

Thanks @mrsimpson It looks good 😄