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.

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

Hardening/3109 url parse refactor #20

Merged
merged 10 commits into from
Mar 10, 2018
824 changes: 361 additions & 463 deletions src/app/contextBroker/contextBroker.cpp

Large diffs are not rendered by default.

106 changes: 89 additions & 17 deletions src/lib/rest/RestService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,61 @@
#include "common/string.h"
#include "common/limits.h"
#include "common/errorMessages.h"

#include "alarmMgr/alarmMgr.h"
#include "metricsMgr/metricsMgr.h"

#include "ngsi/ParseData.h"
#include "mongoBackend/mongoSubCache.h"
#include "jsonParseV2/jsonRequestTreat.h"
#include "parse/textParse.h"
#include "serviceRoutines/badRequest.h"

#include "rest/ConnectionInfo.h"
#include "rest/OrionError.h"
#include "rest/RestService.h"
#include "rest/restReply.h"
#include "rest/rest.h"
#include "rest/uriParamNames.h"
#include "mongoBackend/mongoSubCache.h"
#include "rest/RestService.h"



/* ****************************************************************************
*
* service vectors -
*/
static RestService* getServiceV = NULL;
static RestService* putServiceV = NULL;
static RestService* postServiceV = NULL;
static RestService* patchServiceV = NULL;
static RestService* deleteServiceV = NULL;
static RestService* optionsServiceV = NULL;
RestService* restBadVerbV = NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this one is not static? Why the name uses a different pattern (versus badVerbServiceV for instance)?

Copy link
Author

Choose a reason for hiding this comment

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

This vector is needed in another file (use grep to see which), and as it is not static, it needs a prefix.
As it is in the rest-library, I used the prefix "rest".
Perhaps restBadVerbServiceV would have been a better name. A bit long though

Copy link
Author

Choose a reason for hiding this comment

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

In the next-coming refactoring, all RestService vectors will be moved to RestService.cpp or rest.cpp, and then perhaps all of them can be made static ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

FIXME PR for this or NTC... As you prefer :)

Copy link
Author

Choose a reason for hiding this comment

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

NTC (I am preparing a list of nextcoming PRs about this)




/* ****************************************************************************
*
* serviceVectorsSet - only for unit tests
*/
void serviceVectorsSet
Copy link
Collaborator

Choose a reason for hiding this comment

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

If only used in unit tests, then use #ifdef UNIT_TEST.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

Copy link
Author

Choose a reason for hiding this comment

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

Actually, the function is used, by restInit() to set the vectors.
It wasn't like that in the beginning, that's why I added the comment.
But now the function is used both for the normal broker compilation as for the unit test compilation so ...

NTC

(
RestService* _getServiceV,
RestService* _putServiceV,
RestService* _postServiceV,
RestService* _patchServiceV,
RestService* _deleteServiceV,
RestService* _optionsServiceV,
RestService* _restBadVerbV
)
{
getServiceV = _getServiceV;
putServiceV = _putServiceV;
postServiceV = _postServiceV;
patchServiceV = _patchServiceV;
deleteServiceV = _deleteServiceV;
optionsServiceV = _optionsServiceV;
restBadVerbV = _restBadVerbV;
}



Expand Down Expand Up @@ -445,7 +487,7 @@ static bool compErrorDetect
*
* restService -
*/
std::string restService(ConnectionInfo* ciP, RestService* serviceV)
static std::string restService(ConnectionInfo* ciP, RestService* serviceV, const char* serviceVectorName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the new parameter serviceVectorName being used in this function?

Maybe github diff is fooling me, but I cannot find any ocurrence in the diff page appart from this one...

Copy link
Author

Choose a reason for hiding this comment

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

Not easy to find this kind of things in the github jungle ...

% find . -name "*.cpp" -exec grep "restService(" {} /dev/null
./src/lib/rest/RestService.cpp:static std::string restService(ConnectionInfo* ciP, RestService* serviceV, const char* serviceVectorName)
./src/lib/rest/RestService.cpp:    return restService(ciP, restBadVerbV, "BAD VERB");
./src/lib/rest/RestService.cpp:  if      ((ciP->verb == GET)     && (getServiceV     != NULL))    return restService(ciP, getServiceV,     "GET");
./src/lib/rest/RestService.cpp:  else if ((ciP->verb == POST)    && (postServiceV    != NULL))    return restService(ciP, postServiceV,    "POST");
./src/lib/rest/RestService.cpp:  else if ((ciP->verb == PUT)     && (putServiceV     != NULL))    return restService(ciP, putServiceV,     "PUT");
./src/lib/rest/RestService.cpp:  else if ((ciP->verb == PATCH)   && (patchServiceV   != NULL))    return restService(ciP, patchServiceV,   "PATCH");
./src/lib/rest/RestService.cpp:  else if ((ciP->verb == DELETE)  && (deleteServiceV  != NULL))    return restService(ciP, deleteServiceV,  "DELETE");
./src/lib/rest/RestService.cpp:  else if ((ciP->verb == OPTIONS) && (optionsServiceV != NULL))    return restService(ciP, optionsServiceV, "OPTIONS");
./src/lib/rest/RestService.cpp:  else                                                             return restService(ciP, restBadVerbV,    "BADVERB");

If I'm not mistaken, I used the "serviceVectorName" fior debugging purposes.
Need to make sure, but if so, it will be removed.

Copy link
Author

@kzangeli kzangeli Mar 6, 2018

Choose a reason for hiding this comment

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

The parameter is no longer used.
Removed in 64983d1

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 64983d1

{
std::vector<std::string> compV;
int components;
Expand Down Expand Up @@ -487,38 +529,36 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)
//
// Lookup the requested service
//

for (unsigned int ix = 0; serviceV[ix].treat != NULL; ++ix)
{
if ((serviceV[ix].components != 0) && (serviceV[ix].components != components))
{
continue;
}

if ((ciP->method != serviceV[ix].verb) && (serviceV[ix].verb != "*"))
{
continue;
}

strncpy(ciP->payloadWord, serviceV[ix].payloadWord.c_str(), sizeof(ciP->payloadWord));
bool match = true;
for (int compNo = 0; compNo < components; ++compNo)
{
if (serviceV[ix].compV[compNo] == "*")
const char* component = serviceV[ix].compV[compNo].c_str();

if ((component[0] == '*') && (component[1] == 0))
{
continue;
}

if (ciP->apiVersion == V1)
{
if (strcasecmp(serviceV[ix].compV[compNo].c_str(), compV[compNo].c_str()) != 0)
if (strcasecmp(component, compV[compNo].c_str()) != 0)
{
match = false;
break;
}
}
else
{
if (strcmp(serviceV[ix].compV[compNo].c_str(), compV[compNo].c_str()) != 0)
if (strcmp(component, compV[compNo].c_str()) != 0)
{
match = false;
break;
Expand All @@ -531,7 +571,11 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)
continue;
}

if ((ciP->payload != NULL) && (ciP->payloadSize != 0) && (ciP->payload[0] != 0) && (serviceV[ix].verb != "*"))

//
// If in restBadVerbV vector, no need to check the payload
//
if ((serviceV != restBadVerbV) && (ciP->payload != NULL) && (ciP->payloadSize != 0) && (ciP->payload[0] != 0))
{
std::string response;
std::string spath = (ciP->servicePathV.size() > 0)? ciP->servicePathV[0] : "";
Expand Down Expand Up @@ -563,7 +607,7 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)
}
}

LM_T(LmtService, ("Treating service %s %s", serviceV[ix].verb.c_str(), ciP->url.c_str())); // Sacred - used in 'heavyTest'
LM_T(LmtService, ("Treating service %s %s", ciP->method.c_str(), ciP->url.c_str())); // Sacred - used in 'heavyTest'
if (ciP->payloadSize == 0)
{
ciP->inMimeType = NOMIMETYPE;
Expand Down Expand Up @@ -610,12 +654,11 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)

//
// If we have gotten this far the Input is OK.
// Except for all the badVerb/badRequest, etc.
// A common factor for all these 'services' is that the verb is '*'
// Except for all the badVerb/badRequest, in the restBadVerbV vector.
//
// So, the 'Bad Input' alarm is cleared for this client.
//
if (serviceV[ix].verb != "*")
if (serviceV != restBadVerbV)
{
alarmMgr.badInputReset(clientIp);
}
Expand Down Expand Up @@ -645,6 +688,18 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)
return response;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaboratate on the purpose of the two if clauses, please?

Copy link
Author

@kzangeli kzangeli Mar 6, 2018

Choose a reason for hiding this comment

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

Sure.
restService is a two pass function. First it tries to find the service in a put/post/get etc - vector (depending on the verb that was used).
If nothing found, the it calls itself with the badVerb service vector (that's why it cannot be local to rest.cpp - it is used in RestService.cpp). Now, if no service routine is found in the badVerb vector either, then we have a case of

std::string details = std::string("service '") + ciP->url + "' not recognized";

And, if the badVerb service vector is NULL, then:

return badRequest(ciP, 0, cV, NULL);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a comment explaining a bit this recursive way of working would be useful (or maybe the comment is already there but I haven't seen).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 74c7a9c

if (restBadVerbV == NULL)
{
std::vector<std::string> cV;

return badRequest(ciP, 0, cV, NULL);
}

if (serviceV != restBadVerbV)
{
return restService(ciP, restBadVerbV, "BAD VERB");
}

std::string details = std::string("service '") + ciP->url + "' not recognized";
alarmMgr.badInput(clientIp, details);

Expand All @@ -655,3 +710,20 @@ std::string restService(ConnectionInfo* ciP, RestService* serviceV)
compV.clear();
return answer;
}



/* ****************************************************************************
*
* orionServe -
*/
std::string orionServe(ConnectionInfo* ciP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

orionServe doesn't provide clues about the function purpose... what about restServiceSelector, routesSelector ?

Copy link
Author

Choose a reason for hiding this comment

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

This function was static and called serve(). Now that it needs to be external, its name must change. I picked orionServe() for a name without worrying too much.

What the function does is to call restService() with the service vector that corresponds to the verb/method used in the request (found in ciP).

The best name might be to override restService ...

"Selector" doesn't do the job as the function both selects and executes ...

"orionRestServe"? "orionRestExecute".

I think "restService" is the best name, even though I can't say I like it too much ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using a namespace, e.g. orion::server().

In fact, I think orion:: namespace is currently in use, so it would make sense...

{
if ((ciP->verb == GET) && (getServiceV != NULL)) return restService(ciP, getServiceV, "GET");
else if ((ciP->verb == POST) && (postServiceV != NULL)) return restService(ciP, postServiceV, "POST");
else if ((ciP->verb == PUT) && (putServiceV != NULL)) return restService(ciP, putServiceV, "PUT");
else if ((ciP->verb == PATCH) && (patchServiceV != NULL)) return restService(ciP, patchServiceV, "PATCH");
else if ((ciP->verb == DELETE) && (deleteServiceV != NULL)) return restService(ciP, deleteServiceV, "DELETE");
else if ((ciP->verb == OPTIONS) && (optionsServiceV != NULL)) return restService(ciP, optionsServiceV, "OPTIONS");
else return restService(ciP, restBadVerbV, "BADVERB");
}
34 changes: 25 additions & 9 deletions src/lib/rest/RestService.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ typedef std::string (*RestServiceHandler)(ConnectionInfo* ciP, int compononts, s
typedef std::string (*RestTreat)(ConnectionInfo* ciP, int components, std::vector<std::string>& compV, ParseData* reqDataP);
typedef struct RestService
{
std::string verb; // The method of the service, as a plain string. ("*" matches ALL methods)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have take advantage of this PR to remove an long-time unused variable? Or the removal is related with the purpose of the PR?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is the whole idea with the PR.
The verb is no longer used for comparison, as the service vector is selected depending on the verb. I thought that much was clear before I even started ... :-)

Copy link
Author

Choose a reason for hiding this comment

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

NTC

RequestType request; // The type of the request
int components; // Number of components in the URL path
std::string compV[10]; // Vector of URL path components. E.g. { "v2", "entities" }
Expand All @@ -61,14 +60,6 @@ typedef struct RestService



/* ****************************************************************************
*
* restService -
*/
extern std::string restService(ConnectionInfo* ciP, RestService* serviceV);



/* ****************************************************************************
*
* payloadParse -
Expand All @@ -91,4 +82,29 @@ extern std::string payloadParse
*/
extern std::string tenantCheck(const std::string& tenant);



/* ****************************************************************************
*
* serviceVectorsSet - only for unit tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that at the end this function is not only used only for unit tests, as restInit() also uses it. Thus the "- only for unit tests" fragment should be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Same in the .ccp file)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in a265c79

*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

If only used in unit tests, then use #ifdef UNIT_TEST.

Copy link
Author

Choose a reason for hiding this comment

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

NTC

extern void serviceVectorsSet
(
RestService* _getServiceV,
RestService* _putServiceV,
RestService* _postServiceV,
RestService* _patchServiceV,
RestService* _deleteServiceV,
RestService* _optionsServiceV,
RestService* _restBadVerbV
);



/* ****************************************************************************
*
* orionServe -
*/
extern std::string orionServe(ConnectionInfo* ciP);

#endif
25 changes: 10 additions & 15 deletions src/lib/rest/rest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
*
* Globals
*/
static RestService* restServiceV = NULL;
static unsigned short port = 0;
static RestServeFunction serveFunction = NULL;
static char bindIp[MAX_LEN_IP] = "0.0.0.0";
Expand Down Expand Up @@ -555,17 +554,6 @@ static int httpHeaderGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, co



/* ****************************************************************************
*
* serve -
*/
static void serve(ConnectionInfo* ciP)
{
restService(ciP, restServiceV);
}



/* ****************************************************************************
*
* requestCompleted -
Expand Down Expand Up @@ -1758,7 +1746,13 @@ static int restStart(IpVersion ipVersion, const char* httpsKey = NULL, const cha
*/
void restInit
(
RestService* _restServiceV,
RestService* _getServiceV,
RestService* _putServiceV,
RestService* _postServiceV,
RestService* _patchServiceV,
RestService* _deleteServiceV,
RestService* _optionsServiceV,
RestService* _restBadVerbV,
IpVersion _ipVersion,
const char* _bindAddress,
unsigned short _port,
Expand All @@ -1779,10 +1773,11 @@ void restInit
const char* key = _httpsKey;
const char* cert = _httpsCertificate;

serviceVectorsSet(_getServiceV, _putServiceV, _postServiceV, _patchServiceV, _deleteServiceV, _optionsServiceV, _restBadVerbV);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If only used in unit tests, then use #ifdef UNIT_TEST.

However, as alternative, have you considered the possiblity of moving the initialization vectors to testInit.cpp and invoke restInit() from that code?

Copy link
Author

Choose a reason for hiding this comment

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

NTC


port = _port;
restServiceV = _restServiceV;
ipVersionUsed = _ipVersion;
serveFunction = (_serveFunction != NULL)? _serveFunction : serve;
serveFunction = (_serveFunction != NULL)? _serveFunction : orionServe;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have looked for usages of restInit() and it seems that we neever use _serveFunction parameter. Thus, maybe we should avoid the complexity of this function selection function and hardwire orionServer() function as routing logic.

Copy link
Author

Choose a reason for hiding this comment

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

This is old stuff from when orion was both ContextBroker and IotAgent and ConfiguratrionManager at the same time.
So, yes, it definitely could be removed, but that is another refactoring. restInit() will change completely etc, etc.
Not for this PR

Copy link
Collaborator

@fgalan fgalan Mar 6, 2018

Choose a reason for hiding this comment

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

Ok. I'd suggest to mark it with a // FIXME PR: to be removed within #3109 refactor task or a similar wording.

Copy link
Author

Choose a reason for hiding this comment

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

This has nothing to do with the current refactoring.
I could mark it with a FIXME saying something like "this might be refactored, or not, in the future" ... Really don't see the need of it though.
What would help here if we really want to make this change, is a new issue about it

Copy link
Author

Choose a reason for hiding this comment

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

Name changed from orionServe to orion::requestServe in 02d46a8

Copy link
Author

@kzangeli kzangeli Mar 7, 2018

Choose a reason for hiding this comment

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

Next refactoring on my list is exactly this - move RestService vectors into rest lib. No FIXME needed ... Right?

Copy link
Collaborator

@fgalan fgalan Mar 7, 2018

Choose a reason for hiding this comment

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

Current situation is that there is an parameter, the one named _serveFunction in a function that is never used. I think that is a "weird" situation that worth the one-line of effort ;) that a FIXME PR require.

But you have more neurons on this than me right now, so as you prefer...

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I can fix that. Almost easier than putting it in the issue ... :)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 124c24a (the variable serveFunction is removed as well - which makes us have to modify images ...)

Copy link
Author

Choose a reason for hiding this comment

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

Images modified in eaa194d

multitenant = _multitenant;
connMemory = _connectionMemory;
maxConns = _maxConnections;
Expand Down
36 changes: 34 additions & 2 deletions src/lib/rest/rest.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,23 @@ extern bool multitenant;
extern bool corsEnabled;
extern char corsOrigin[64];
extern int corsMaxAge;
extern RestService* restBadVerbV;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this addition to global variables?

Copy link
Author

Choose a reason for hiding this comment

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

I answered before. The vector for bad verbs is needed in RestService.cpp

Copy link
Author

Choose a reason for hiding this comment

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

NTC




/* ****************************************************************************
*
* RestServeFunction -
*/
typedef void (*RestServeFunction)(ConnectionInfo* ciP);
typedef std::string (*RestServeFunction)(ConnectionInfo* ciP);



/* ****************************************************************************
*
* serve -
*/
extern std::string serve(ConnectionInfo* ciP);
Copy link
Collaborator

Choose a reason for hiding this comment

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

After the change from

serveFunction    = (_serveFunction != NULL)? _serveFunction : serve;

to

serveFunction    = (_serveFunction != NULL)? _serveFunction : orionServe;

Is the serve() function actually used in some place? Should it be removed?

Copy link
Author

Choose a reason for hiding this comment

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

The serve()function changed name to orionServe(), that's all.
And yes, it is used :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Github diff is not always as smart as it should be with renamings... ;)

Copy link
Author

Choose a reason for hiding this comment

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

NTC




Expand All @@ -90,7 +99,13 @@ typedef void (*RestServeFunction)(ConnectionInfo* ciP);
*/
extern void restInit
(
RestService* _restServiceV,
RestService* _getServiceV,
RestService* _putServiceV,
RestService* _postServiceV,
RestService* _patchServiceV,
RestService* _deleteServiceV,
RestService* _optionsServiceV,
RestService* _noServiceV,
IpVersion _ipVersion,
const char* _bindAddress,
unsigned short _port,
Expand Down Expand Up @@ -133,4 +148,21 @@ extern void firstServicePath(const char* servicePath, char* servicePath0, int se
*/
extern bool isOriginAllowedForCORS(const std::string& requestOrigin);



/* ****************************************************************************
*
* serviceVectorsSet - only for unit tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Repeated declaration? (in seems is also at RestService.h)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in a265c79

*/
extern void serviceVectorsSet
Copy link
Collaborator

Choose a reason for hiding this comment

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

If only used in unit tests, then use #ifdef UNIT_TEST.

(Maybe I'm repeating myself too much... sorry :)

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned, the comment is no longer valid.

NTC

(
RestService* _getServiceV,
RestService* _putServiceV,
RestService* _postServiceV,
RestService* _patchServiceV,
RestService* _deleteServiceV,
RestService* _optionsServiceV,
RestService* _badVerbV
);

#endif
Loading