Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISSUE: 501. CORS Support. #3032

Merged
merged 5 commits into from
Nov 15, 2017
Merged

ISSUE: 501. CORS Support. #3032

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ce0a0e
ISSUE: 501. CORS Support.
McMutton Oct 27, 2017
1ad7db3
Merge branch 'master' into CORS_PR2
McMutton Nov 3, 2017
88eeebb
Redundant test step removed.
McMutton Nov 6, 2017
b9a8b2f
Suggested changes added.
McMutton Nov 9, 2017
e3a6471
Variable name changes for readability
McMutton Nov 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGES_NEXT_RELEASE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
- Add: CORS Preflight Requests support for /v2 and /v2/entities, -corsMaxAge switch (#501)
- Add: CORS Preflight Requests support for all NGSIv2 resources, -corsMaxAge switch (#501)
- Fix: case-sensitive header duplication (e.g. "Content-Type" and "Content-type") in custom notifications (#2893)
- Fix: bug in GTE and LTE operations in query filters (q/mq), both for GET operations and subscriptions (#2995)
30 changes: 23 additions & 7 deletions src/app/contextBroker/contextBroker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@
#include "serviceRoutinesV2/deleteMetrics.h"
#include "serviceRoutinesV2/optionsGetOnly.h"
#include "serviceRoutinesV2/optionsGetPostOnly.h"
#include "serviceRoutinesV2/optionsGetDeleteOnly.h"
#include "serviceRoutinesV2/optionsAllNotDelete.h"
#include "serviceRoutinesV2/optionsGetPutOnly.h"
#include "serviceRoutinesV2/optionsGetPutDeleteOnly.h"
#include "serviceRoutinesV2/optionsGetDeletePatchOnly.h"
#include "serviceRoutinesV2/optionsPostOnly.h"

#include "contextBroker/version.h"
#include "common/string.h"
Expand Down Expand Up @@ -784,12 +790,12 @@ static const char* validLogLevels[] =


#define API_V2 \
{ "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 }, \
{ "*", EPS, EPS_COMPS_V2, ENT_COMPS_WORD, badVerbGetOnly }, \
\
{ "GET", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, getEntities }, \
{ "POST", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, postEntities }, \
{ "*", 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 }, \
{ "*", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, badVerbGetPostOnly }, \
\
{ "GET", IENT, IENT_COMPS_V2, IENT_COMPS_WORD, getEntity }, \
{ "DELETE", IENT, IENT_COMPS_V2, IENT_COMPS_WORD, deleteEntity }, \
Expand Down Expand Up @@ -833,8 +839,18 @@ static const char* validLogLevels[] =


#define API_V2_CORS \
{ "OPTIONS", EPS, EPS_COMPS_V2, ENT_COMPS_WORD, optionsGetOnly }, \
{ "OPTIONS", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, optionsGetPostOnly }
{ "OPTIONS", EPS, EPS_COMPS_V2, ENT_COMPS_WORD, optionsGetOnly }, \
{ "OPTIONS", ENT, ENT_COMPS_V2, ENT_COMPS_WORD, optionsGetPostOnly }, \
{ "OPTIONS", IENT, IENT_COMPS_V2, IENT_COMPS_WORD, optionsGetDeleteOnly }, \
{ "OPTIONS", IENTOA, IENTOA_COMPS_V2, IENTOA_COMPS_WORD, optionsAllNotDelete }, \
{ "OPTIONS", IENTATTRVAL, IENTATTRVAL_COMPS_V2, IENTATTRVAL_COMPS_WORD, optionsGetPutOnly }, \
{ "OPTIONS", IENTATTR, IENTATTR_COMPS_V2, IENTATTR_COMPS_WORD, optionsGetPutDeleteOnly }, \
{ "OPTIONS", ENTT, ENTT_COMPS_V2, ENTT_COMPS_WORD, optionsGetOnly }, \
{ "OPTIONS", ETT, ETT_COMPS_V2, ETT_COMPS_WORD, optionsGetOnly }, \
{ "OPTIONS", SSR, SSR_COMPS_V2, SSR_COMPS_WORD, optionsGetPostOnly }, \
{ "OPTIONS", ISR, ISR_COMPS_V2, ISR_COMPS_WORD, optionsGetDeletePatchOnly}, \
{ "OPTIONS", BQR, BQR_COMPS_V2, BQR_COMPS_WORD, optionsPostOnly }, \
{ "OPTIONS", BUR, BUR_COMPS_V2, BUR_COMPS_WORD, optionsPostOnly }


#define REGISTRY_STANDARD_REQUESTS_V0 \
Expand Down
29 changes: 28 additions & 1 deletion src/lib/rest/HttpHeaders.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@



/* ****************************************************************************
*
* HTTP Headers -
*/
#define CONTENT_TYPE "Content-Type"
#define FIWARE_SERVICE "Fiware-Service"
#define X_AUTH_TOKEN "X-Auth-Token"
#define X_REAL_IP "X-Real-IP"
#define X_FORWARDED_FOR "X-Forwarded-For"
#define FIWARE_CORRELATOR "Fiware-Correlator"
#define NGSIV2_ATTRSFORMAT "Ngsiv2-AttrsFormat"
#define FIWARE_SERVICEPATH "Fiware-Servicepath"
#define ACCESS_CONTROL_ALLOW_ORIGIN "Access-Control-Allow-Origin"
#define ACCESS_CONTROL_ALLOW_HEADERS "Access-Control-Allow-Headers"
#define ACCESS_CONTROL_ALLOW_METHODS "Access-Control-Allow-Methods"
#define ACCESS_CONTROL_MAX_AGE "Access-Control-Max-Age"



/* ****************************************************************************
*
* CORS Allowed Headers -
*/
#define CORS_ALLOWED_HEADERS CONTENT_TYPE", "FIWARE_SERVICE", "FIWARE_SERVICEPATH", "NGSIV2_ATTRSFORMAT", "FIWARE_CORRELATOR", "X_FORWARDED_FOR", " X_REAL_IP", " X_AUTH_TOKEN



// -----------------------------------------------------------------------------
//
// HttpAcceptHeader -
Expand Down Expand Up @@ -62,7 +89,7 @@ typedef struct HttpAcceptHeader

/* ****************************************************************************
*
* HttpHeaders -
* HttpHeaders -
*/
typedef struct HttpHeaders
{
Expand Down
2 changes: 2 additions & 0 deletions src/lib/rest/rest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ IpVersion ipVersionUsed = IPDUAL;
bool multitenant = false;
std::string rushHost = "";
unsigned short rushPort = NO_PORT;
bool corsEnabled = false;
char restAllowedOrigin[64];
int restCORSMaxAge;
static MHD_Daemon* mhdDaemon = NULL;
Expand Down Expand Up @@ -1781,6 +1782,7 @@ void restInit
mhdConnectionTimeout = _mhdTimeoutInSeconds;

strncpy(restAllowedOrigin, _allowedOrigin, sizeof(restAllowedOrigin));
corsEnabled = (restAllowedOrigin[0] != 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. We take the -corsOrigin switch value and assign it to allowedOrigin in contextBroker.cpp
  2. We call the restInit function, passing the value of allowedOrigin variable in contextBroker.cpp
  3. We define a global variable in rest.cpp called restAllowedOrigin
  4. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e3a6471


strncpy(bindIp, LOCAL_IP_V4, MAX_LEN_IP - 1);
strncpy(bindIPv6, LOCAL_IP_V6, MAX_LEN_IP - 1);
Expand Down
1 change: 1 addition & 0 deletions src/lib/rest/rest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ extern IpVersion ipVersionUsed;
extern std::string rushHost;
extern unsigned short rushPort;
extern bool multitenant;
extern bool corsEnabled;
extern char restAllowedOrigin[64];
extern int restCORSMaxAge;

Expand Down
15 changes: 8 additions & 7 deletions src/lib/rest/restReply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "rest/ConnectionInfo.h"
#include "rest/uriParamNames.h"
#include "rest/HttpStatusCode.h"
#include "rest/HttpHeaders.h"
#include "rest/mhd.h"
#include "rest/OrionError.h"
#include "rest/restReply.h"
Expand Down Expand Up @@ -162,32 +163,32 @@ 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 a bad verb response
if ((corsEnabled == true) && (ciP->httpStatusCode != SccBadVerb))
{
//Only GET method is supported for V1 API
if (ciP->apiVersion == V2 || (ciP->apiVersion == V1 && ciP->verb == GET))
if ((ciP->apiVersion == V2) || (ciP->apiVersion == V1 && ciP->verb == GET))
{
// If any origin is allowed, the header is sent always with "any" as value
if (strcmp(restAllowedOrigin, "__ALL") == 0)
{
MHD_add_response_header(response, "Access-Control-Allow-Origin", "*");
MHD_add_response_header(response, ACCESS_CONTROL_ALLOW_ORIGIN, "*");
}
// If a specific origin is allowed, the header is only sent if the origins match
else if (strcmp(ciP->httpHeaders.origin.c_str(), restAllowedOrigin) == 0)
{
MHD_add_response_header(response, "Access-Control-Allow-Origin", restAllowedOrigin);
MHD_add_response_header(response, ACCESS_CONTROL_ALLOW_ORIGIN, restAllowedOrigin);
}
}

if (ciP->verb == OPTIONS)
{
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");
MHD_add_response_header(response, ACCESS_CONTROL_ALLOW_HEADERS, CORS_ALLOWED_HEADERS);

char corsMaxAge[STRING_SIZE_FOR_INT];
snprintf(corsMaxAge, sizeof(corsMaxAge), "%d", restCORSMaxAge);

MHD_add_response_header(response, "Access-Control-Max-Age", corsMaxAge);
MHD_add_response_header(response, ACCESS_CONTROL_MAX_AGE, corsMaxAge);
}
}

Expand Down
11 changes: 9 additions & 2 deletions src/lib/serviceRoutines/badVerbGetDeleteOnly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/OrionError.h"
#include "serviceRoutines/badVerbGetDeleteOnly.h"



/* ****************************************************************************
*
* badVerbGetDeleteOnly -
* badVerbGetDeleteOnly -
*/
std::string badVerbGetDeleteOnly
(
Expand All @@ -54,7 +55,13 @@ std::string badVerbGetDeleteOnly
OrionError oe(SccBadVerb, ERROR_DESC_BAD_VERB);

ciP->httpHeader.push_back("Allow");
ciP->httpHeaderValue.push_back("GET, DELETE");
std::string headerValue = "GET, DELETE";
//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);
Expand Down
11 changes: 9 additions & 2 deletions src/lib/serviceRoutines/badVerbGetOnly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/OrionError.h"
#include "serviceRoutines/badVerbGetOnly.h"



/* ****************************************************************************
*
* badVerbGetOnly -
* badVerbGetOnly -
*/
std::string badVerbGetOnly
(
Expand All @@ -54,7 +55,13 @@ std::string badVerbGetOnly
OrionError oe(SccBadVerb, ERROR_DESC_BAD_VERB);

ciP->httpHeader.push_back("Allow");
ciP->httpHeaderValue.push_back("GET");
std::string headerValue = "GET";
//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);
Expand Down
11 changes: 9 additions & 2 deletions src/lib/serviceRoutines/badVerbGetPostOnly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/OrionError.h"
#include "serviceRoutines/badVerbGetPostOnly.h"



/* ****************************************************************************
*
* badVerbGetPostOnly -
* badVerbGetPostOnly -
*/
std::string badVerbGetPostOnly
(
Expand All @@ -54,7 +55,13 @@ std::string badVerbGetPostOnly
OrionError oe(SccBadVerb, ERROR_DESC_BAD_VERB);

ciP->httpHeader.push_back("Allow");
ciP->httpHeaderValue.push_back("POST, GET");
std::string headerValue = "POST, GET";
//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);
Expand Down
11 changes: 9 additions & 2 deletions src/lib/serviceRoutines/badVerbGetPutDeleteOnly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/OrionError.h"
#include "serviceRoutines/badVerbGetPutDeleteOnly.h"



/* ****************************************************************************
*
* badVerbGetPutDeleteOnly -
* badVerbGetPutDeleteOnly -
*/
std::string badVerbGetPutDeleteOnly
(
Expand All @@ -54,7 +55,13 @@ std::string badVerbGetPutDeleteOnly
OrionError oe(SccBadVerb, ERROR_DESC_BAD_VERB);

ciP->httpHeader.push_back("Allow");
ciP->httpHeaderValue.push_back("GET, PUT, DELETE");
std::string headerValue = "GET, PUT, DELETE";
//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);
Expand Down
11 changes: 9 additions & 2 deletions src/lib/serviceRoutines/badVerbPostOnly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/OrionError.h"
#include "serviceRoutines/badVerbPostOnly.h"



/* ****************************************************************************
*
* badVerbPostOnly -
* badVerbPostOnly -
*/
std::string badVerbPostOnly
(
Expand All @@ -54,7 +55,13 @@ std::string badVerbPostOnly
OrionError oe(SccBadVerb, ERROR_DESC_BAD_VERB);

ciP->httpHeader.push_back("Allow");
ciP->httpHeaderValue.push_back("POST");
std::string headerValue = "POST";
//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);
Expand Down
6 changes: 6 additions & 0 deletions src/lib/serviceRoutinesV2/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ getMetrics.cpp
deleteMetrics.cpp
optionsGetOnly.cpp
optionsGetPostOnly.cpp
optionsGetDeleteOnly
optionsAllNotDelete
optionsGetPutOnly
optionsGetPutDeleteOnly
optionsGetDeletePatchOnly
optionsPostOnly
)

SET (HEADERS
Expand Down
10 changes: 8 additions & 2 deletions src/lib/serviceRoutinesV2/badVerbAllNotDelete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

#include "ngsi/ParseData.h"
#include "rest/ConnectionInfo.h"
#include "rest/rest.h"
#include "rest/restReply.h"
#include "rest/OrionError.h"
#include "serviceRoutinesV2/badVerbGetDeletePatchOnly.h"
Expand All @@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//OPTIONS verb is only available for V2 API
if ((corsEnabled == true) && (ciP->apiVersion == V2))
{
headerValue = headerValue + ", OPTIONS";
}
ciP->httpHeaderValue.push_back(headerValue);
ciP->httpStatusCode = SccBadVerb;

alarmMgr.badInput(clientIp, details);

return (ciP->apiVersion == V1 || ciP->apiVersion == NO_VERSION)? "" : oe.smartRender(ciP->apiVersion);
}

Loading