ISSUE: 501. CORS Support. #3018

McMutton · 2017-10-14T12:39:03Z

The first PR for #501.

Adds: -enableCORS switch, OPTIONS request for two resources and required tests.

Note: -corsOrigin switch has been left as it is in order not to break the current functionality. Once all the PRs towards #501 are merged to master, it can be removed.

kzangeli

Very good!
Just a few details ...

kzangeli · 2017-10-14T19:28:54Z

src/app/contextBroker/contextBroker.cpp

@@ -380,6 +384,7 @@ PaArgument paArgs[] =
  { "-writeConcern",  &writeConcern, "WRITE_CONCERN",  PaInt,    PaOpt, 1,          0,      1,     WRITE_CONCERN_DESC },

  { "-corsOrigin",       allowedOrigin,     "ALLOWED_ORIGIN",    PaString, PaOpt, _i "",          PaNL,  PaNL,     ALLOWED_ORIGIN_DESC    },
+  { "-enableCORS",       corsEnabled,       "CORS_ENABLED",      PaString, PaOpt, _i "",          PaNL,  PaNL,     CORS_ENABLED_DESC      },


Why not simply -cors ... ?
'enable' is kind of there implicitly when a CLI option is used.

Fixed in de7d90b

kzangeli · 2017-10-14T19:33:37Z

src/app/contextBroker/contextBroker.cpp

-  { "GET",    EPS,          EPS_COMPS_V2,         ENT_COMPS_WORD,          entryPointsTreat         }, \
-  { "*",      EPS,          EPS_COMPS_V2,         ENT_COMPS_WORD,          badVerbGetOnly           }, \
+  { "GET",     EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          entryPointsTreat         }, \
+  { "OPTIONS", EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          optionsGetOnly           }, \


Service routines are named as VERB+'what'.
The 'what' in this case (/v2) is not very clear, but perhaps we can use 'entrypoint' as the previous service uses it.
[ Or v2root ... ? ]
The name of the service routine would then be optionsEntrypoint (also, entryPointsTreat should be renamed to getEntrypoint, but not in this PR, of course).

Thanks for your feedback @kzangeli. We had a short discussion with @fgalan about this in #501.

What we do for options requests is independent of the service logic, we are just pushing the necessary headers to the response. Therefore, to prevent duplication and limit the number of files created, a similar approach as in badVerb... handlers was used.

I agree that using the form optionsEntryPoint is semantically better but this means we have to duplicate the same handler more than 10 times. For example, for every resource that allows GET and POST, we need to create the same handler with a different name all of which would do the exact same operation: pushing Access-Control-Allow-Methods: GET, POST header to the response.

What do you think?

So, if it is one single service routine, then we should find a generic name for it. 'options' is perhaps too generic, what about 'optionsTreat'. We've used Treat as function suffix in other places (for service routines)

Let's see if I finally have understood this correctly (I have no prior knowledge of CORS, I'm afraid).

Each and every URL that the broker supports will be equipped with the OPTIONS method and only return the list of methods that the URL supports?
Which would mean that we need to add OPTIONS for the "Allow" header in all bad verb functions,
Funny, the info that you need with the OPTIONS verb is already there, thanks to the badVerb routines ...
Basically we have to copy all badVerb routines and make a minimal change (Allow => Access-Control-Allow-Methods).

[ We could even use the badVerb routines and depending on ciP->method do one thing or another ... ]

[ We could even use the badVerb routines and depending on ciP->method do one thing or another ... ]

I prefer to have a new method for that, given that bad verb processing and options verb processing are semantically different. In this line, the suggested name optionsTreat sounds good, although, if I'm understanding correctly, what we have at the end it not a single method covering all URLs, but several ones, covering each one a set of URLs. Thus, only a optionsTreat method probably is not enougth for all URL, but a small set of them (maybe using all them the optionsTreat prefix in the name) could be fine. This is a similar idea to current bad verb method, as far as I remember.

I think the initial idea of "copying" badVerb service routines is the way to go.
We don't need various functions dpimg the exact same thing.

Actually, there is another option here ...
Let's see what you think.
We could implement only one service routine, but a smarter one, that looks up the URL path in the ReatService vector and saves the verbs that it finds.I thought about changing the badVerb like this ... a few years ago ... Perhaps now it's time. It would be valid for both options and badVerb

@McMutton please hold on at this point. I seems @kzangeli and me are proposing opposite alternatives :) So, we'll discuss and provide a conclusive alternative.

After @kzangeli and @fgalan discusssion, we thenk the option of having 8-12 handlers depending on the allowed verbs in a given URL is the best one.

Handler name should follow the same criteria than bad verb, i.e. including in the name of the methods to be covered, e.g: optionsGetOnly, optionsGetPostOnly, etc.... as you are already doing as far as I see in this PR :)

NTC?

(Not sure, but it seems the PR is already aligned with the conclusions to this comment threads, so no additional change needs to be done)

kzangeli · 2017-10-14T19:37:06Z

src/app/contextBroker/contextBroker.cpp

-  { "*",      EPS,          EPS_COMPS_V2,         ENT_COMPS_WORD,          badVerbGetOnly           }, \
+  { "GET",     EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          entryPointsTreat         }, \
+  { "OPTIONS", EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          optionsGetOnly           }, \
+  { "*",       EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          badVerbGetOnly           }, \


This is no longer 'getOnly'.
'get only' means that the path, in this case /v2, only accepts the GET verb, which is no longer true, as OPTIONS is now supported as well.
We will need a new badVerb service routine, called badVerbGetOptionsOnly, and that service routine must be used here.

However, there is no real need to rename ALL badVerb routines.
Let's just keep the old names. OPTIONS is implicit, it will be present in all of the badVerb service routines.
[ Unless some standard says it shouldn't :-) ]

NTC

kzangeli · 2017-10-14T19:37:49Z

src/app/contextBroker/contextBroker.cpp

-  { "*",      ENT,          ENT_COMPS_V2,         ENT_COMPS_WORD,          badVerbGetPostOnly       }, \
+  { "GET",     ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          getEntities              }, \
+  { "POST",    ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          postEntities             }, \
+  { "OPTIONS", ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          optionsGetPostOnly       }, \


Same here, the service routine should be called optionsEntities.

Nope, better to use optionsGetPostOnly as initially proposed.

NTC

kzangeli · 2017-10-14T19:38:32Z

src/app/contextBroker/contextBroker.cpp

+  { "GET",     ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          getEntities              }, \
+  { "POST",    ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          postEntities             }, \
+  { "OPTIONS", ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          optionsGetPostOnly       }, \
+  { "*",       ENT,          ENT_COMPS_V2,        ENT_COMPS_WORD,          badVerbGetPostOnly       }, \


And here, another badVerb service routine, badVerbGetPostOptionsOnly is necessary.

Same same,

NTC

kzangeli · 2017-10-14T19:42:40Z

src/lib/serviceRoutinesV2/optionsGetOnly.cpp

@@ -0,0 +1,54 @@
+/*
+*
+* Copyright 2013 Telefonica Investigacion y Desarrollo, S.A.U


Fixed in 8c48f60

kzangeli · 2017-10-14T19:43:19Z

src/lib/serviceRoutinesV2/optionsGetPostOnly.h

+
+/*
+*
+* Copyright 2013 Telefonica Investigacion y Desarrollo, S.A.U


Fix all 2013 => 2017, please

Fixed in 8c48f60

kzangeli · 2017-10-14T19:44:43Z

test/functionalTest/cases/0501_cors/allowed_headers.test

+
+--SHELL--
+echo "01. Allowed headers: GET OPTIONS"
+echo "=============================================================================="


In functests, we try to make the "====-line" as long as its preceding text

Fixed in 8c48f60

kzangeli · 2017-10-14T19:46:19Z

test/functionalTest/cases/0501_cors/allowed_headers.test

+
+--TEARDOWN--
+brokerStop CB
+dbDrop CB


For completeness, please add a 'PUT /v2/entities' test case to see the correct 'bad verb' message

Since preflight requests are only sent with the OPTIONS method, I thought keeping them isolated in their own context would be better. Also, bad verb messages have their own tests. What do you think?

I agree with @McMutton , bad verb has (or should have) their own .test. I don't see them related with this PR.

Sure, it doesn't have to be in this file.
However, if it isn't covered by any other functest (I imagine it is) then we need to add a test case in one of the existing badVerb tests

So... NTC? (with regards to this PR)

kzangeli · 2017-10-14T19:46:54Z

test/functionalTest/cases/0501_cors/any_origin.test

+
+--TEARDOWN--
+brokerStop CB
+dbDrop CB


For completeness, please add a 'POST /v2' test case to see the correct 'bad verb' message

Same as above.

kzangeli · 2017-10-14T19:52:14Z

Ah, also, this PR will require some update of the documentation.
@fgalan will tell us exactly where.

fgalan · 2017-10-16T15:31:57Z

src/lib/rest/restReply.cpp

+
+    if (ciP->verb == OPTIONS)
+    {
+      MHD_add_response_header(response, "Access-Control-Allow-Headers", "User-Agent, Fiware-Service, Fiware-Servicepath, Ngsiv2-AttrsFormat, Fiware-Correlator, X-Forwarded-For, X-Real-IP, X-Auth-Token");


Maybe this is a naïve question as I'm not an expert in CORS but :)... why this particular election of headers?

: ) I took a look at the httpHeaderGet method of rest.cpp and among the headers that are defined there, those are the headers that would trigger a preflight request according to the CORS spec (details here).

Thanks for the article link! (I think that one could be used in cli.md, as I mention in other comment).

However, it is pretty long and I'd take some time to read it. In the meanwhile, could you point me to the place (section, etc.) in which the criteria to include or not include a given header in Access-Control-Allow-Headers is described, please?

Sure here, you can find the details on headers for the preflighted requests.

Since this part itself includes additional links, it becomes a long read eventually, sorry :)

After looking to the document, the only criteria that I have found (tell me if I'm wrong and it is other, please) is:

The Access-Control-Allow-Headers header is used in response to a preflight request to indicate which HTTP headers can be used when making the actual request.

So, my original question can be reformulated in the following way: are User-Agent, Fiware-Service, Fiware-Servicepath, Ngsiv2-AttrsFormat, Fiware-Correlator, X-Forwarded-For, X-Real-IP and X-Auth-Token the only headers that should be allowed?

Probably yes, but I'd like to have your confirmation on this :)

The rest of the headers we process are:

Host, Expect, Connection, Content-Length, Origin. All of which are Forbidden header names that should not be in the Access-Control-Request-Headers in the first place.

Accept, Content-Type. These two are CORS-safelisted request headers and they are not required to be checked.

Thanks for your questsion @fgalan, it made me realize that, Content-Type is only listed as CORS-safe for a subset of its values. So I will add it to the list.

Having said that, for the next PR, I will check what we have in the Access-Control-Request-Headers and set our Access-Control-Allow-Headers accordingly in order not to create an unnecessarily long response.

More info here.

Fixed in 543c64b

fgalan · 2017-10-16T15:32:21Z

src/lib/rest/restReply.cpp

+    if (ciP->verb == OPTIONS)
+    {
+      MHD_add_response_header(response, "Access-Control-Allow-Headers", "User-Agent, Fiware-Service, Fiware-Servicepath, Ngsiv2-AttrsFormat, Fiware-Correlator, X-Forwarded-For, X-Real-IP, X-Auth-Token");
+      MHD_add_response_header(response, "Access-Control-Max-Age", "86400");


How long is 86400? Why is this hardwired?

It is in seconds, corresponding to 24 hours. This value is the cap for Firefox but is different for other browsers (10 minutes for Chrome for example).

This is hardwired at the moment since we planned this issue to be handled in several PRs, I thought we could discuss and decide on how to handle it in the latter PRs. I think we should pass it as a parameter to the switch. I am not sure about the allowed headers though.

I think we should pass it [the max age] as a parameter to the switch

I agree.

In the meanwhile, if you don't want to create that switch for this PR, add a "FIXME" comment about it just above this line.

Fixed in 8c48f60

fgalan · 2017-10-16T15:44:31Z

CHANGES_NEXT_RELEASE

+- Fix: broken JSON due to unscaped quotes (") in NGSIv2 error description field (#2955)
+- Add: Partial CORS Preflight Requests support, -cors switch (#501)


What means "partial" in this context? That this PR implements only a subset of resources (in particular on the /v2 and /v2/entities resources)? Of that the implemented stuff (i.e. adding some headers, etc.) is not enough to implement CORS preflight and something else will be needed in the future?

(Again probably a naïve-CORS question :) )

No no, it is a perfectly valid question : )

I wrote partial since it only implements the handling of preflight request for only two resources at the moment, /v2 and /v2/entities.

In terms of logic, I don't think we miss anything but for the next PRs, we need to cover the rest of the resources, have a switch parameter for the Max-Age and also remove the -corsOrigin switch.

I'd suggest to use:

- Add: CORS Preflight Requests support for /v2 and /v2/entities, -cors switch (#501)

and, for future PRs, new lines with:

- Add: CORS Preflight Requests support for <whichever URLs covered by the PR>, -cors switch (#501)

so, at the end, we can use all these lines to "checksum" if we have covered all the URLs in the NGSIv2 API, and, in that case, replace them all in a final PR with:

- Add: CORS Preflight support for all NGSIv2 resources, -cors switch (#501)

Fixed in 543c64b

fgalan · 2017-10-17T07:56:23Z

src/app/contextBroker/contextBroker.cpp

@@ -252,6 +254,7 @@ bool            https;
 bool            mtenant;
 char            rush[256];
 char            allowedOrigin[64];
+char            corsEnabled[64];


Just to check if I'm understading correctly how this would work...

If -corsOrigin foo is used, then Orion adds Access-Control-Allow-Origin: foo to the response of all GET operations.

If -enableCORS bar is used, then Orion adds Access-Control-Allow-Origin: bar to the responses of all OPTIONS operations (*) along with others headers: Access-Control-Allow-Headers (with hardwired value), Access-Control-Max-Age (hardwired value) and Access-Control-Allow-Methods (value dedpending on the methods supported by the URL).

Is my understanding correct?

(*) Well, not in this PR, that covers only two URLs, but at the end it will be that way :)

Well yes and no :)

Considering what ´-corsOrigin foo´ does, ´-enableCORS foo´ (going to be -cors upon @kzangeli's suggestion) does the exact same thing. But it also adds the rest of the headers that should be there in a proper response to a preflight request.

I decided not to change ´-corsOrigin´ to ´-cors´ directly in case there are people using it and we might want to go with a soft roll-out, removing the -corsOrigin switch in a latter PR.

Since we are going to remove the -corsOrigin switch once we have all the required changes for #501 anyway, I can do it in a commit directly if this is more appropriate.

I am open to suggestions :)

My point is that, with the current implementation, you can have different Access-Control-Allow-Origin for GET responses and for OPTIONS responses. Does that make sense in CORS? If not, then we should unify to just one switch.

...and I'd suggest to that the unification in this PR (as I understand it is easy to do).

Yes, you are right, I haven't implemented any defense mechanism against setting two different allowed origins at the same time. I will remove the old switch in this PR.

Instead of removing the old switch... what about of reusing it? That way current Orion users with contextBroker ... -corsOrigin foo will not break when upgrading Orion version.

I thought about reusing it but the naming might be a problem since we are not only setting the CORS allowed origin but also the max-age.

Having said that, is it currently possible to have more than one parameter for a switch? If yes, I would prefer -cors __ALL 86400 for example. If not, should we go for something like -corsOrigin __ALL -corsMaxAge 86400?

No, I think it is not possible. The "trick" we have used in similar cases is using some character as separator, e.g. ":"

-notificationMode threadpool:600:20

However, in this case I think is clearer having two switches, as you cite:

-corsOrigin __ALL -corsMaxAge 86400

With defaults would be:

-corsOrigin: CORS not enabled at all (i.e. no domain header in GET responses, no routes for OPTIONS verb)

-corsMaxAge 86400 (24 hours)

Fixed in 8c48f60

fgalan · 2017-10-17T07:59:15Z

Nice work! I have added some comments to guide some needed changes, please don't hesiate to ask if you have doubts.

With regards to:

Ah, also, this PR will require some update of the documentation. @fgalan will tell us exactly where.

The only point I have located in the documentation regarding CORS is the one at https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/admin/cli.md. You have to update that document in order to describe the new CLI being added. Adding link to how CORS works (specially the prefligth mechanism) would be also a good idea.

fgalan · 2017-10-18T08:01:51Z

test/functionalTest/cases/0501_cors/allowed_headers.test

+Access-Control-Max-Age: REGEX([0-9]{5})
+Access-Control-Allow-Headers: REGEX(.*)
+Access-Control-Allow-Origin: *
+Access-Control-Allow-Methods: GET, OPTIONS


According to CORS specification the OPTIONS verb that is used to get the list of methods in this header should be also included in the list, isn't it?

I am sorry I didn't get this. OPTIONS is already listed in the Access-Control-Allow-Methods list. Can you please elaborate?

It is only a doubt... not sure if OPTIONS should be there or no. My doubt is based in my lack of expersie on CORS.

However, I understand that if you haven't get this is probably because the answer to my doubt is "yes, OPTIONS has to be there" :)

NTC

Ah ok, I see your point. Well this was a question mark for me as well during the implementation. The examples in the spec add OPTIONS to the responses and also I checked the REST APIs of Google, Github etc. and it is always there :)

fgalan · 2017-10-18T08:07:56Z

src/app/contextBroker/contextBroker.cpp

-  { "*",      EPS,          EPS_COMPS_V2,         ENT_COMPS_WORD,          badVerbGetOnly           }, \
+  { "GET",     EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          entryPointsTreat         }, \
+  { "OPTIONS", EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          optionsGetOnly           }, \
+  { "*",       EPS,          EPS_COMPS_V2,        ENT_COMPS_WORD,          badVerbGetOnly           }, \


Up to know badVerbGetOnly generates something like this:

HTTP/1.1 405 Method Not Allowed Content-Length: 0 Allow: GET

however, after this PR, I understand this should be:

HTTP/1.1 405 Method Not Allowed Content-Length: 0 Allow: GET, OPTIONS

Is that correct?

(Similar comment for badVerbGetPostOnly function below)

I agree but this should only be the case when CORS is enabled. Maybe we can add add a new logic in badVerb... handlers to check if CORS is enabled and if so add OPTIONS to the outputs.

Which brings us to your question earlier (which we skipped and got buried under other comments). What if CORS is not enabled and we receive an OPTIONS request anyway? These kinds of requests will not be treated with the badVerb... handlers since we already have the OPTIONS in the service vectors.

Also related with this, I understand that OPTIONS processing make only sense in the case of CORS functionality (at least by the time being). Thus, the OPTIONS routes should be enabled only when CORS is activated. In other words, these restServiceV[] vector should include these entries only if -enableCORS (or whicheer name we use at the end for the CLI parameter) is being used.
We did in the past a similar "trick" with the -ngsi9 switch (which disables all the NGSI9 routes in the V1 API). @kzangeli do you remember how we did that time? Otherwise, maybe we should look to old versions of the contextBroker.app (go to tag 0.24.0 in github), as -ngsi9 was deprecated and removed time ago.

With regards to:

Maybe we can add add a new logic in badVerb... handlers to check if CORS is enabled and if so add OPTIONS to the outputs.

Correct. It should be just a matter of checking -enableCORS (or whichever name we use at the end for the CLI parameter) has been provided or not, adjusting the list to include in the Allow header accordingly.

What if CORS is not enabled and we receive an OPTIONS request anyway?

That should be considered a bad verb case and, if restServiceV[] is corrrectly set (as I outline in my previous comment, see 0.24.0 code), the request will be reouted to the right bad verb handler.

Actually, we will need some logic to make this work. A conditional that looks at the -cors switch when looking up the services.
This is my backyard, I can take a look at it soon. If you want, just ignore this and I'll fix it later. Soon

I am sorry for the spam @fgalan but I need a confirmation. I checked the tag you mentioned and I have some questions about the details.

So basically in the main function, we chose which rest service vector to use depending on the switch.

RestService* rsP = (ngsi9Only == true)? restServiceNgsi9 : restServiceV;

So I will create another vector called restServiceCORS and choose this or the original depending on the switch. And instead of having API_V2 inside this vector, I can use a duplicate of the whole API_V2 as API_V2_CORS which also includes the OPTIONS handlers.

This would work but my question is do we really need to duplicate the whole API_V2 definition? Is it possible to define API_V2_CORS with only OPTIONS entries like below:

{ "OPTIONS", EPS, EPS_COMPS_V2, ENT_COMPS_WORD, optionsGetOnly }, { "OPTIONS", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, optionsGetPostOnly }, ...

and create the restServiceCORS vector as below:

RestService restServiceCORS[] = { API_V2, API_V2_CORS, REGISTRY_STANDARD_REQUESTS_V0, ...

Would this work or the sequence of the entries are important?

With regards to bad verd including the right set of verbs for Allows I agree in deferring to a next PR.

With regards disable OPTIONS routes when -enableCORS (or whichever name we use at the end for the CLI parameter) is not used, I'd like to see it in this PR. Orion users needs to control if they want cors-stuff included (and the potential security risks it brings ;) or not when they start the CB process.

Would this work or the sequence of the entries are important?

I'd say that the sequence of entries doesn't mind (*)

However, let's wait for @kzangeli feedback on this, which knows how this works better than me :)

(*) At least at this point... in the future, maybe some kind of optimization based on the frecuency of each operation could be implemented, but this is another history and outside the scope this PR).

We will have a look at the order some day.
Lots of time can be saved putting more frequent requests in the beginning.

But, forget about that for now, I will change a lot of things in main.cpp for this.
Longer lines, as it was before.
Right now it is very hard to understand these vectors

And, about replicating the entire vector of services for -cors, I like it !
Didn't think about that (obvious) option !!!

Fixed in 543c64b

kzangeli · 2017-10-18T13:25:07Z

If you put the new OPTIONS services first in the vector it will work.

The important thing is to have each OPTIONS before its corresponding '*', which otherwise will send it to badVerb.

When I do the refactor of all this I will put everything in order again, no worries.

Important part of that refactor is to decide the order that we want for the services, which one is the most frequent, etc

McMutton · 2017-10-18T15:50:21Z

I have pushed a commit which attends to the requests in some of the older comments on the PR. With the new corsMaxAge switch, I am aware that I need to add some more tests. I will add them and reflect the changes, that come with latest decisions, on the next commit to this PR.

fgalan · 2017-10-18T15:51:27Z

test/functionalTest/cases/0501_cors/get_specific_origin.test

@@ -1,211 +0,0 @@
-# Copyright 2014 Telefonica Investigacion y Desarrollo, S.A.U


It will be great not losing old .test in case/0501 in order not losing "covegare" for the basic CORS feature (i.e. only for GET withoug pre-fligth) that works for NGSIv1.

Is there something in this test that is precluding to work with the new implementation?

No but, the new logic covers that functionality, same with the new tests.

If the request is not a preflight and CORS is enabled, what we do now basically is the same thing. Adding the Allowed-Origin header to the response. And this is covered in the new tests.

The main reason I removed the tests was, it was only focusing on the GET method but in reality, simple CORS requests include POST and HEAD methods as well.

And this is covered in the new tests.

I understand new tests (in this PR and in the following ones you could have "queued" after it) cover the NGSIv2 URLs, but not NGSIv1 URL. Thus, until we can "cover" NGSIv1 with full CORS support (if at the end we decide that is important, which is something yet unders discussion on #501 (comment)) I think is valuable to keep these tests.

However, maybe a FIXME like this could be added to the NGSIv1 based .test preamble:

FIXME: this .test would be reviewed (or maybe removed) once full CORS support gets completed for NGSIv1 URLs

As a side note, in general, existing .test are not removed. Each single .test is a piece of the regression that ensures that, PR after PR, the code doesn't break backward compatiblity. Removing .test "decapitalize" our regression, i.e. makes regression weaker.

This is not a rule written in stone :), but make us very conservative in order to take .test removal decissions. Only in the case a new feature clearly overcomes an existing one, removal has been done. In case of doubt, we don't remove.

I see, sure I will add them back.

Fixed in 543c64b

…ix for CORS, re-introduce removed tests

kzangeli · 2017-10-19T17:50:15Z

OK, looks like everything has been fixed.
Great job, @McMutton !

LGTM

fgalan · 2017-10-20T08:27:56Z

doc/manuals/admin/cli.md

@@ -115,13 +115,15 @@ The list of available options is the following:
    for REST connections. Note that the default value is zero, i.e., no timeout (wait forever).
 -   **-cprForwardLimit**. Maximum number of forwarded requests to Context Providers for a single client request
    (default is no limit). Use 0 to disable Context Providers forwarding completely.
-   **-corsOrigin <domain>**. Configures CORS for GET requests,
+-   **-corsOrigin <domain>**. Enables Cross-Origin Resource Sharing,


Maybe you could add a sentence with a link to the nice document about CORS you mentioned, e.g.

Please find more information about CORS in this link.

Fixed in f97a6f1

fgalan · 2017-10-20T08:34:37Z

test/functionalTest/cases/0501_cors/specific_origin.test

+
+echo "05. OPTIONS Entry Points with arrakis origin (Access-Control-Allow-Origin header included)"
+echo "=========================================================================================="
+orionCurl --url /v2 -X OPTIONS --origin arrakis


It would be great to include a .test checking that when brokerStart doesn't use -corsOrigin then OPTIONS /v2 (or OPTSION /v2/entities) results in 405 Method not allowed.

This could be include in this PR (in this case, I'll wait for your "Fixme in") or in a next one (in this case, I'll wait for your NTC), as you prefer.

Fixed in f97a6f1

fgalan · 2017-10-20T08:47:53Z

"Amost" LGTM :) Good work!

After a full re-review, only a couple of new comments (one very easy regarding documentation, the other regardins adding a new .test is optional).

In the meanwhile, it would be great if @jmcanterafonseca could have a look, at least to the functional part (the .test files).

jmcanterafonseca · 2017-10-20T09:31:00Z

I think we need to take into account other methods such as PATCH and DELETE, particularly on

/v2/entities/<entityId>

and sub-resources

jmcanterafonseca · 2017-10-20T09:11:00Z

test/functionalTest/cases/0000_cli/command_line_options.test

@@ -57,7 +57,8 @@ Usage: contextBroker  [option '-U' (extended usage)]
                      [option '-reqTimeout' <connection timeout for REST requests (in seconds)>]
                      [option '-reqMutexPolicy' <mutex policy (none/read/write/all)>]
                      [option '-writeConcern' <db write concern (0:unacknowledged, 1:acknowledged)>]
-                      [option '-corsOrigin' <CORS allowed origin. use '__ALL' for any>]
+                      [option '-corsOrigin' <enable Cross-Origin Resouce Sharing with allowed origin. Use '__ALL' for any>]


Fixed in f97a6f1

jmcanterafonseca · 2017-10-20T09:11:24Z

test/functionalTest/cases/0000_cli/tracelevel_without_logLevel_as_DEBUG.test

@@ -58,7 +58,8 @@ Usage: contextBroker  [option '-U' (extended usage)]
                      [option '-reqTimeout' <connection timeout for REST requests (in seconds)>]
                      [option '-reqMutexPolicy' <mutex policy (none/read/write/all)>]
                      [option '-writeConcern' <db write concern (0:unacknowledged, 1:acknowledged)>]
-                      [option '-corsOrigin' <CORS allowed origin. use '__ALL' for any>]
+                      [option '-corsOrigin' <enable Cross-Origin Resouce Sharing with allowed origin. Use '__ALL' for any>]


Fixed in f97a6f1

jmcanterafonseca · 2017-10-20T09:19:59Z

src/lib/rest/restReply.cpp

+
+    if (ciP->verb == OPTIONS)
+    {
+      MHD_add_response_header(response, "Access-Control-Allow-Headers", "Content-Type, User-Agent, Fiware-Service, Fiware-Servicepath, Ngsiv2-AttrsFormat, Fiware-Correlator, X-Forwarded-For, X-Real-IP, X-Auth-Token");


Content-Type and User-Agent are not needed to be declared here. I think

In order to have better extensibility or maintainability of the code it would be great to define the headers as an array of constants, so that when new headers are needed it would be enough to only touch the constant array.

Content-Type and User-Agent are not needed to be declared here

Content-Type can be out of the scope of simple headers depending on its value. Please see Step 10 of 6.2 Preflight Request

Agreed with User-Agent. I will remove it.

In order to have better extensibility or maintainability of the code it would be great to define the headers as an array of constants, so that when new headers are needed it would be enough to only touch the constant array.

Yes, this is planned for the next PR.

Fixed in f97a6f1

fgalan · 2017-10-20T09:33:44Z

@jmcanterafonseca you are right, but take into account that this PR focuses on just /v2 and /v2/entities. In understand /v2/entities/<entityId> will be included in future PRs on the #501 issue. Could you confirm @McMutton , pls?

McMutton · 2017-10-20T14:08:47Z

I think we need to take into account other methods such as PATCH and DELETE, particularly on /v2/entities/and sub-resources

@jmcanterafonseca you are right, but take into account that this PR focuses on just /v2 and /v2/entities. In understand /v2/entities/ will be included in future PRs on the #501 issue. Could you confirm @McMutton , pls?

Yes the idea was to create the main logic and have a proof of concept. Once this PR is merged, all V2 resources will be covered in the next one.

…-Agent from list of headers.

fgalan · 2017-10-20T14:36:33Z

LGTM

Passing the ball to @jmcanterafonseca . Once he provides LGTM, this PR will be merged.

jmcanterafonseca · 2017-10-23T08:56:13Z

LGTM, thanks

fgalan · 2017-10-23T10:43:04Z

It seems this PR has introduced some style fails. We have realized in our CI platform after merge:

./scripts/style_check_in_makefile.sh
style_check.sh: found 8 errors (2 mayor, 2 minor errors) in 4569 lines of source code in 60 files (.17% style-guide-incompatibilities)
Lint Errors:

0002 errors of category 'Use int16/int64/etc'
0002 errors of category 'Blank line at the start of a code block'
=================================================================
src/lib/serviceRoutinesV2/getAllSubscriptions.cpp:62:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/lib/serviceRoutinesV2/optionsGetOnly.cpp:48:  Blank line at the start of a code block.  Is this needed?  [whitespace/blank_line] [2]
src/lib/serviceRoutinesV2/optionsGetOnly.cpp:0:  One or more unexpected \r (^M) found;better to use only a \n  [whitespace/newline] [1]
src/lib/serviceRoutinesV2/optionsGetPostOnly.cpp:48:  Blank line at the start of a code block.  Is this needed?  [whitespace/blank_line] [2]
src/lib/serviceRoutinesV2/optionsGetPostOnly.cpp:0:  One or more unexpected \r (^M) found;better to use only a \n  [whitespace/newline] [1]
src/lib/serviceRoutinesV2/postSubscriptions.cpp:61:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
src/lib/serviceRoutinesV2/optionsGetOnly.h:0:  One or more unexpected \r (^M) found;better to use only a \n  [whitespace/newline] [1]
src/lib/serviceRoutinesV2/optionsGetPostOnly.h:0:  One or more unexpected \r (^M) found;better to use only a \n  [whitespace/newline] [1]

Could you fix @McMutton , pls?

McMutton · 2017-10-23T12:22:05Z

Sure, but now that the PR is merged and closed, should I do another pull request or is it possible to push to the same branch to associate the changes with this PR?

And by the way two of these fails are not related with this PR, I have not modified those files:

src/lib/serviceRoutinesV2/getAllSubscriptions.cpp:62:  Use int16/int64/etc, rather than the C type long [runtime/int] [4]
src/lib/serviceRoutinesV2/postSubscriptions.cpp:61:  Use int16/int64/etc, rather than the C type long [runtime/int] [4]

fgalan · 2017-10-23T12:38:21Z

Sure, but now that the PR is merged and closed, should I do another pull request or is it possible to push to the same branch to associate the changes with this PR?

I think both are possible... As personal choice, I tend to use a fresh new branch is this situation, as it is cleaner.

And by the way two of these fails are not related with this PR, I have not modified those files

Fix only in the files touched by your PR. Not sure what happens with those two... we would fix if at the end they mean mayor errors.

Thanks!

ISSUE: 501. CORS Support.

bfa624c

kzangeli suggested changes Oct 14, 2017

View reviewed changes

Switch changed from -enableCORS to -cors

de7d90b

fgalan reviewed Oct 16, 2017

View reviewed changes

fgalan reviewed Oct 17, 2017

View reviewed changes

fgalan reviewed Oct 18, 2017

View reviewed changes

McMutton added 2 commits October 18, 2017 17:38

Added -corsMaxAge switch, changed -cors to -corsOrigin, fixed tests

8c48f60

merge with master

f947afe

fgalan reviewed Oct 18, 2017

View reviewed changes

McMutton added 2 commits October 19, 2017 16:52

Adds functional test for corsMaxAge, documentation updates, routing f…

543c64b

…ix for CORS, re-introduce removed tests

merge with master

4d1b293

fgalan modified the milestone: 1.10.0 Oct 19, 2017

fgalan reviewed Oct 20, 2017

View reviewed changes

jmcanterafonseca suggested changes Oct 20, 2017

View reviewed changes

fgalan mentioned this pull request Oct 20, 2017

CORS support #501

Closed

Updated cli doc, fixed typos, added new functional test. Removed User…

f97a6f1

…-Agent from list of headers.

fixed test name

0c1a5e6

jmcanterafonseca approved these changes Oct 23, 2017

View reviewed changes

fgalan merged commit 2c4f4c4 into telefonicaid:master Oct 23, 2017

McMutton mentioned this pull request Oct 23, 2017

ISSUE: 501. CORS Support. Style fix #3023

Merged

McMutton deleted the CORS_PR1 branch October 23, 2017 13:42

		- Fix: broken JSON due to unscaped quotes (") in NGSIv2 error description field (#2955)
		- Add: Partial CORS Preflight Requests support, -cors switch (#501)

		@@ -1,211 +0,0 @@
		# Copyright 2014 Telefonica Investigacion y Desarrollo, S.A.U

ISSUE: 501. CORS Support. #3018

ISSUE: 501. CORS Support. #3018

Conversation

McMutton commented Oct 14, 2017

kzangeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

McMutton Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli commented Oct 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

McMutton Oct 16, 2017 •

edited

Loading

kzangeli Oct 14, 2017 •

edited

Loading

McMutton Oct 18, 2017 •

edited

Loading

kzangeli Oct 14, 2017 •

edited

Loading

McMutton Oct 17, 2017 •

edited

Loading

McMutton Oct 19, 2017 •

edited

Loading

McMutton Oct 17, 2017 •

edited

Loading

fgalan Oct 16, 2017 •

edited

Loading

fgalan Oct 17, 2017 •

edited

Loading

McMutton Oct 17, 2017 •

edited

Loading

McMutton Oct 17, 2017 •

edited

Loading

fgalan Oct 18, 2017 •

edited

Loading

McMutton Oct 18, 2017 •

edited

Loading

McMutton Oct 18, 2017 •

edited

Loading