ISSUE: 501. CORS Support. #3032

McMutton · 2017-11-03T13:06:00Z

Implements #501

Rest of the /v2 resources now support CORS. New unit and functional tests are added.

Tests added ISSUE: 501. CORS Support. cnr fix

McMutton · 2017-11-03T13:14:22Z

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.

about the line below:

MHD_add_response_header(response, "Access-Control-Allow-Headers", "Content-Type, Fiware-Service, Fiware-Servicepath, Ngsiv2-AttrsFormat, Fiware-Correlator, X-Forwarded-For, X-Real-IP, X-Auth-Token");

I did what was suggested but then, storing these values in an array then looping over it to form this string again felt a bit of an overkill.

Should we instead have a string constant for each HTTP header and them create a constant string concatenating them in restReply.cpp as cors_allowed_headers?

What is the standard way of handling this kind of issues in the context of Orion? I will do another commit for this after your feedback.

fgalan · 2017-11-06T09:55:56Z

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

@@ -40,6 +40,42 @@ orionCurl --url /v2/entities -X OPTIONS
 echo
 echo

+echo "03. Allowed headers: GET DELETE OPTIONS"


As far as I undertand, https://github.com/telefonicaid/fiware-orion/pull/3032/files#diff-c7a50c3b97af231671cfad8674374d21R844 is adding 10 new routes (i.e. 10 new URLs supporting OPTIONS).

However, only 6 new URL/steps have been included...

Maybe I'm missing something? :)

We had discussed about this more than a few times, it is normal to get confused :)

Right now we have 12 different routes for /v2 which have 8 different verb sets.

As an example, by having optionsGetPostOnly handler, we cover both /v2/entities and /v2/subscriptions. So that's why we have less handlers than the number of routes, similar to bad verb handlers.

Ok, I think I see now your point...

Note functional tests should be designed from usage/API perspective, not implementation perspective. I mean, I agree with you that in this particular case and with the current implementation, with just the 8 operations you include in the test you can cover the 8 methods options handlers.

However, imagine that at some point and for some unknown reason at the present moment the implementation changes (but not the API). You would need to re-think the test in order to align with the new implementation. Thus, if the test is designed with API perspective from the very beginning, you would change the implementation without changing test... which at the end is the actual purpose of functional testing ;)

Moreover, in this case the modification in the .test is pretty simple. Just adding the 4 missing cases.

It seems a similar situation occurs with cors_bad_verbs.test. It would be great (and easy :) to pass from 8 to 12 URLs also in that .test.

And now... I see your point :)

You are absolutely right, thanks for pointing this out. I'll fix this in the next commit.

Oh.. I just realized the name of this test file should have been allowed_methods. I will fixed that as well.

Fixed in b9a8b2f

fgalan · 2017-11-06T09:57:24Z

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

@@ -40,6 +40,12 @@ orionCurl --url /v2 -X OPTIONS
 echo
 echo

+echo "03. Bad Verb Entry Points"


This is already being tested https://github.com/telefonicaid/fiware-orion/pull/3032/files#diff-a375d95dcfcfe65f4a0395db510119dbR33, isn't it?

In that case, maybe this step is redundand in this .test and could be removed...

Fixed in 88eeebb

fgalan · 2017-11-06T10:00:15Z

I did what was suggested but then, storing these values in an array then looping over it to form this string again felt a bit of an overkill.

Should we instead have a string constant for each HTTP header and them create a constant string concatenating them in restReply.cpp as cors_allowed_headers?

What is the standard way of handling this kind of issues in the context of Orion? I will do another commit for this after your feedback.

I don't remember if we have an standard way for that... however, the approach of have a string constant for each HTTP header and them create a constant string concatenating them in restReply.cpp as cors_allowed_headers sound good. However, instead of using string constants for each HTTP header maybe a #define could be used.

I'd suggest you try something in that line a let's see how it looks like.

fgalan · 2017-11-06T10:01:27Z

Would it take advantage of this PR to consider #3030 ? What do you think @McMutton ?

McMutton · 2017-11-06T14:51:20Z

Would it take advantage of this PR to consider #3030 ? What do you think @McMutton ?

@fgalan I would prefer having a separate PR for this since a new response header (Access-Control-Expose-Headers) is going to be needed so having its discussion in a separate thread would be easier for the community and for us :)

I am not sure if the new PR should belong to #3030 or #501 though, what do you think?

fgalan · 2017-11-06T15:05:34Z

I am not sure if the new PR should belong to #3030 or #501 though, what do you think?

I think it would be better to make that PR belongs to #3030

McMutton · 2017-11-06T15:26:34Z

@fgalan, one more question about the HTTP header definitions.

I thought one of these might be suitable to hold the definitions but I am not sure which: httpHeaders.h, globals.h and rest.h

fgalan · 2017-11-06T16:14:56Z

@fgalan, one more question about the HTTP header definitions.

I thought one of these might be suitable to hold the definitions but I am not sure which: httpHeaders.h, globals.h and rest.h

I think the best one would be httpHeaders.h. @kzangeli what do you think?

kzangeli

Nice job!
Just a few minor things

kzangeli · 2017-11-06T17:42:00Z

src/lib/rest/restReply.cpp

@@ -162,8 +162,8 @@ void restReply(ConnectionInfo* ciP, const std::string& _answer)
    }
  }

-  //Check if CORS is enabled
-  if (strlen(restAllowedOrigin) > 0)
+  //Check if CORS is enabled and the response is not bad verb response


Style guide: space after //

Fixed in b9a8b2f

kzangeli · 2017-11-06T17:47:47Z

src/lib/rest/restReply.cpp

-  //Check if CORS is enabled
-  if (strlen(restAllowedOrigin) > 0)
+  //Check if CORS is enabled and the response is not bad verb response
+  if (strlen(restAllowedOrigin) > 0 && ciP->httpStatusCode != SccBadVerb)


restAllowedOrigin ... I have a feeling this variable will never change (set at startup and then never touched again).
I don't like very much to to a strlen() over and over on a string that doesn't change ...
Perhaps invent a boolean variable?

bool originAllowed = restAllowedOrigin[0] != 0;

Or something similar (and then use originAllowed of couirse :-)).
Easier to read and faster to execute.

Suggested names:

allowedOrigin for the string

hasAllowedOrigin for the bool

Agreed, this would be much better, thanks for the suggestion.

One comment about the name of the boolean variable. In the code we basically do the check (the length check as of now) to see if CORS is enabled or not. I think it would make more sense to have the variable name something like corsEnabled. This would also make the code easier to understand.

What do you think @kzangeli @fgalan ?

corsEnabled sounds great

corsEnabled is fine for the bool

Fixed in b9a8b2f

kzangeli · 2017-11-06T17:50:31Z

src/lib/serviceRoutines/badVerbGetDeleteOnly.cpp

-  ciP->httpHeaderValue.push_back("GET, DELETE");
+  std::string headerValue = "GET, DELETE";
+  //OPTIONS verb is only available for V2 API
+  if (strlen(restAllowedOrigin) > 0 && ciP->apiVersion == V2)


Same same, both comment and strlen(restAllowedOrigin) > 0.
Also, the style guide proposes to use parenthesis for 'complex' conditions, so:

if ((originAllowed == true) && (ciP->apiVersion == V2)) ...

Like this, there is no way it can go wrong and it is a little bit easier to read, right?

[ The same corrections for 7 more occurrences ]

Fixed in b9a8b2f

kzangeli · 2017-11-06T18:15:29Z

src/lib/serviceRoutinesV2/badVerbAllNotDelete.cpp

@@ -55,11 +56,16 @@ std::string badVerbAllNotDelete
  OrionError   oe(SccBadVerb, ERROR_DESC_BAD_VERB);

  ciP->httpHeader.push_back("Allow");
-  ciP->httpHeaderValue.push_back("GET, PATCH, POST, PUT");
+  std::string headerValue = "GET, PATCH, POST, PUT";


Just for curiosity, is there any 'established' order for the verbs/methods ?

Interesting question... what does the CORS specification say about that?

CORS specification does not provide any rules for the order of the methods and the example scenarios provided do not seem to be following any pattern.

Please see the example scneario at the end of the subsection: 7.1.5 Cross-Origin Request with Preflight.

So, NTC (alphabetic order seems ok to me)

I think I used an (by myself invented) "estimated use-frequency order", as: GOT. POST, PUT. PATCH ...
We should change the badVerb service routines to do alphabetic order instead . Can't go wrong ... (except perhaps for 100% backward compatibility :-( )
To be discussed, and nothing to be done in this PR. of course.

We should change the badVerb service routines to do alphabetic order instead . Can't go wrong ... (except perhaps for 100% backward compatibility :-( )
To be discussed, and nothing to be done in this PR. of course.

Taking into account order doesn't matter from specification point of view, I don't see any value on changing that.

Taking into account order doesn't matter from specification point of view, I don't see any value on changing that.

Uniformity seems like reason enough to me (surprised to hear that you don't care :-))
If it wasn't for the possible backward compatibility I would change it, it's easy enough.
But, let's not

kzangeli · 2017-11-06T18:39:56Z

I think the best one would be httpHeaders.h. @kzangeli what do you think?

Yeah, that sounds good

McMutton · 2017-11-09T12:06:42Z

In HttpHeaders.h, we now have a list of definitions for HTTP headers that we allow in CORS mode.

@kzangeli , @fgalan I know it is not related to this PR but this might be a good opportunity to discuss if it is worth to create an issue for adding more header definitions to the list.

There are many occurrences of hard-coded string values of HTTP headers in our source files and all these can be replaced by macros which can be added to HttpHeaders.h, extending the list of HTTP header definitions we have.

fgalan · 2017-11-10T09:55:44Z

src/lib/rest/rest.cpp

@@ -1781,6 +1782,7 @@ void restInit
  mhdConnectionTimeout = _mhdTimeoutInSeconds;

  strncpy(restAllowedOrigin, _allowedOrigin, sizeof(restAllowedOrigin));
+  corsEnabled = (restAllowedOrigin[0] != 0);


A cryptic way of checking if the char string is empty but it's ok... :)

Just to be sure... how restAllowedOrigin array is initialized? Can we asure that first element is zero? Or it is random and could be initilizalied with random stuff?

Actually it was suggested by @kzangeli :) I think it is better than checking the length.

restAllowedOrigin gets the value of -corsOrigin switch or if the switch is not used, initialized with empty string, "". For empty string, the first char will always be zero.

By the way, since we are talking about the variable, we have re-used some of the code Jose Manuel wrote for the first CORS implementation and some of the variable names that made sense back then, now got a bit confusing. Especially restAllowedOrigin. So this is the flow we have now:

We take the -corsOrigin switch value and assign it to allowedOrigin in contextBroker.cpp

We call the restInit function, passing the value of allowedOrigin variable in contextBroker.cpp

We define a global variable in rest.cpp called restAllowedOrigin

In restInit function, the passed value is mapped to _allowedOrigin in the signature and the value is re-assigned to restAllowedOrigin.

As you see, there are a lot of name changes which makes the code hard to read and harder to maintain. I propose using the same name corsOrigin except the method signature of restInit where we can have it as _corsOrigin. Same goes for restCORSMaxAge.

if the switch is not used, initialized with empty string, ""

That's the piece of the puzzle I missed. I didn't know if the argument parsing library puts "" in the case of missing swich or just left the holding variable with undefined content. I understand is the former.

I propose using the same name corsOrigin except the method signature of restInit where we can have it as _corsOrigin. Same goes for restCORSMaxAge.

Sounds good.

I guess this is related with

{ "-corsOrigin", allowedOrigin, "ALLOWED_ORIGIN", PaString, PaOpt, _i "", PaNL, PaNL, ALLOWED_ORIGIN_DESC },

That _i "" ensures that in the case of not using -corsOrigin at the CLI, the value for allowedOrigin is univocally set to empty string, isn't it @kzangeli ?

Ok. So this comment (and with it the whole PR, as far as I remember) is only pending of a new commit with the refactor in the variable names, as @McMutton suggested above.

Fixed in e3a6471

fgalan · 2017-11-10T10:06:30Z

@kzangeli , @fgalan I know it is not related to this PR but this might be a good opportunity to discuss if it is worth to create an issue for adding more header definitions to the list.

There are many occurrences of hard-coded string values of HTTP headers in our source files and all these can be replaced by macros which can be added to HttpHeaders.h, extending the list of HTTP header definitions we have.

Makes fully sense. Issue has been created: #3035

fgalan · 2017-11-14T10:11:07Z

Ok. So this comment (and with it the whole PR, as far as I remember) is only pending of a new commit with the refactor in the variable names, as @McMutton suggested above.

Anything else pending, @McMutton please?

McMutton · 2017-11-14T14:20:06Z

Anything else pending, @McMutton please?

No, there are no pending tasks, I plan to do the final commit today.

fgalan · 2017-11-14T19:25:59Z

I don't like maxAge too much as variable n ame, as it very generic and could be confused with some other "max ages" not related with CORS introduced in the future. However, we can keep it as it is now and change in the future if some conflict occur.

LGTM. Thasnk for the work! Passing the ball to @kzangeli for final LGTM.

kzangeli · 2017-11-15T18:53:06Z

LGTM

McMutton added 2 commits November 3, 2017 14:03

ISSUE: 501. CORS Support.

4ce0a0e

Tests added ISSUE: 501. CORS Support. cnr fix

Merge branch 'master' into CORS_PR2

1ad7db3

fgalan reviewed Nov 6, 2017

View reviewed changes

fgalan mentioned this pull request Nov 6, 2017

CORS support #501

Closed

Redundant test step removed.

88eeebb

kzangeli suggested changes Nov 6, 2017

View reviewed changes

Suggested changes added.

b9a8b2f

fgalan reviewed Nov 10, 2017

View reviewed changes

fgalan mentioned this pull request Nov 10, 2017

Unhardwire HTTP headers strings #3035

Closed

Variable name changes for readability

e3a6471

fgalan approved these changes Nov 14, 2017

View reviewed changes

fgalan merged commit 337083d into telefonicaid:master Nov 15, 2017

fgalan mentioned this pull request Nov 15, 2017

Add support for accessing response headers when making CORS requests #3030

Closed

McMutton deleted the CORS_PR2 branch November 16, 2017 10:01

ISSUE: 501. CORS Support. #3032

ISSUE: 501. CORS Support. #3032

Conversation

McMutton commented Nov 3, 2017 • edited by fgalan Loading

McMutton commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 6, 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 commented Nov 6, 2017 • edited Loading

fgalan commented Nov 6, 2017

McMutton commented Nov 6, 2017 • edited Loading

fgalan commented Nov 6, 2017

McMutton commented Nov 6, 2017

fgalan commented Nov 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Nov 7, 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

kzangeli commented Nov 6, 2017

McMutton commented Nov 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Nov 10, 2017

fgalan commented Nov 14, 2017

McMutton commented Nov 14, 2017

fgalan commented Nov 14, 2017

kzangeli commented Nov 15, 2017

McMutton commented Nov 3, 2017 •

edited by fgalan

Loading

fgalan Nov 6, 2017 •

edited

Loading

fgalan commented Nov 6, 2017 •

edited

Loading

McMutton commented Nov 6, 2017 •

edited

Loading

fgalan Nov 7, 2017 •

edited

Loading