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

Bug/2763 bug fix not refactor #2821

Merged
merged 13 commits into from
Jan 18, 2017
Merged
3 changes: 3 additions & 0 deletions CHANGES_NEXT_RELEASE
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
- Fix: bug when tenants are over 44 bytes long (#2811)
- Add: Support for null values in string filters (#2359)
- Fix: Partial sanity check for custon url, only if the url contains no replacements using ${xxx} (#2280, #2279)
- Fix: if entity::type is given but as an empty string, an error is returned (#1985)
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to explain in which operations this fix has been done. Maybe this?

"Fix: if subject.entities*.type is given but as an empty string in POST /v2/subscriptions or PATCH /v2/subscriptions/{sub} , an error is returned (#1985)"

Copy link
Member Author

Choose a reason for hiding this comment

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

The checks are done in low level functions, parseEntity() and parseEntityObject(). Should be valid for all V2 requests that have entites, including GET /v2/entities and probably more ops.

Do you still want a list of all the ops in CNR ?

Copy link
Member

Choose a reason for hiding this comment

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

So you mean that before this PR entity creation with "type": "" was failing? A bit surprissing... I think we already have .test in place that check min and max length for all id-kind fields (entity id/type, attr name/type, md name/type)...

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my understanding, I may be wrong.
In the json parse stage (parseEntity() and parseEntityObject()), there were no checks for empty 'type'. Perhaps there are checks that cover part of the request set somewhere else ... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, from what I've seen now, it seems all cases (except for /v2/subscriptions) of empty entity::type were caught already. just as @fgalan predicted :-), changing the CNR line

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in a93291f

- Fix: admin requests with errors now return '400 Bad Request', not '200 OK'
Copy link
Member

Choose a reason for hiding this comment

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

"adming request" could be a big obscure, and example would help. I mean:

"Fix: admin requests (e.g. GET /version) with errors now return '400 Bad Request', not '200 OK'"

Btw, issue number is missing at the end of the line. If the fix is not related with any issue, that's perfect (asking only to check if that is the case...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, an example may make it easier to understand.
No, no issue here, as far as I know, see PR description:

admin requests with errors now return '400 Bad Request', not '200 OK' (no issue, I think)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 8fa9c69

- Fix: attempt to create an entity with an error in service path (and probably more types of errors) now returns '400 Bad Request', not '200 OK' (#2763)
- Fix: not calling regfree when regcomp fails. Potential fix for a crash (#2769)
- Fix: wrongly overlogging metadata abscense in csub docs as Runtime Error (#2796)
- Fix: if HTTP header Content-Length contains a value above the maximum payload size, an error is returned and the message is not read (#2761)
Expand Down
2 changes: 1 addition & 1 deletion doc/manuals/user/ngsiv2_implementation_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ The particular validations that Orion implements on NGSIv2 subscription payloads
* **id** or **idPattern**: one of them is mandatory (but both at the same time is not allowed). id
must follow NGSIv2 restrictions for IDs. idPattern must be not empty and a valid regex.
* **type** or **typePattern**: optional (but both at the same time is not allowed). type must
follow NGSIv2 restrictions for IDs. typePattern must be not empty and a valid regex.
follow NGSIv2 restrictions for IDs. type must not be empty. typePattern must be a valid regex, and non-empty.
* **condition**: optional (but if present it must have a content, i.e. `{}` is not allowed)
* **attrs**: optional (but if present it must be a list; empty list is allowed)
* **expression**: optional (but if present it must have a content, i.e. `{}` is not allowed)
Expand Down
8 changes: 8 additions & 0 deletions src/lib/jsonParseV2/parseEntity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ std::string parseEntity(ConnectionInfo* ciP, Entity* eP, bool eidInURL)

eP->type = iter->value.GetString();
eP->typeGiven = true;

if (eP->type.empty())
{
alarmMgr.badInput(clientIp, "empty entity type");
ciP->httpStatusCode = SccBadRequest;
OrionError oe(SccBadRequest, "entity type length: 0, min length supported: 1", "BadRequest");
return oe.toJson();
}
}
else // attribute
{
Expand Down
5 changes: 5 additions & 0 deletions src/lib/jsonParseV2/parseEntityObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ std::string parseEntityObject(ConnectionInfo* ciP, Value::ConstValueIterator val
eP->type = iter->value.GetString();
eP->typeGiven = true;

if (eP->type.empty())
{
return "entity type length: 0, min length supported: 1";
Copy link
Member

Choose a reason for hiding this comment

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

Move to errorMessages.h

}

if (forbiddenChars(eP->type.c_str(), ""))
{
return "Invalid characters in entity type";
Expand Down
2 changes: 1 addition & 1 deletion src/lib/rest/OrionError.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ std::string OrionError::smartRender(ApiVersion apiVersion)
*/
std::string OrionError::setStatusCodeAndSmartRender(ApiVersion apiVersion, HttpStatusCode* scP)
{
if (apiVersion == V2)
if ((apiVersion == V2) || (apiVersion == ADMIN_API))
Copy link
Member

Choose a reason for hiding this comment

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

Great to see that the addition has not broken anything... setStatusCodeAndSmartRender() modifyication were hard in the past (at least to me...)

NTC

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I am not so sure. Seeing a few really strange errors in functests ...

Copy link
Member Author

@kzangeli kzangeli Jan 18, 2017

Choose a reason for hiding this comment

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

Errors under control again, fixed in f422157 (this line is intact, the problem was elsewhere)

{
*scP = code;
}
Expand Down
13 changes: 11 additions & 2 deletions src/lib/serviceRoutinesV2/postEntities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,22 @@ std::string postEntities
// 02. Call standard op postUpdateContext
postUpdateContext(ciP, components, compV, parseDataP, NGSIV2_FLAVOUR_ONCREATE);

// 03. Check error
//
// 03. Check error - 3 different ways to get an error from postUpdateContext ... :-(
// FIXME P4: make postUpdateContext have ONE way to return errors. See github issue #2763
//
std::string answer = "";
if (parseDataP->upcrs.res.oe.code != SccNone )
if (parseDataP->upcrs.res.oe.code != SccNone)
{
TIMED_RENDER(answer = parseDataP->upcrs.res.oe.toJson());
ciP->httpStatusCode = parseDataP->upcrs.res.oe.code;
}
else if (parseDataP->upcrs.res.errorCode.code != SccOk)
{
ciP->httpStatusCode = parseDataP->upcrs.res.errorCode.code;
TIMED_RENDER(answer = parseDataP->upcrs.res.errorCode.toJson(true));
ciP->answer = answer;
}
else
{
// Prepare HTTP headers
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# Copyright 2016 Telefonica Investigacion y Desarrollo, S.A.U
Copy link
Member

Choose a reason for hiding this comment

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

Looking again, I don't see the .test covering #1985.... Maybe you missed git add before commit? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find one. I have created the directory but there is no functest inside ...
Also can't find any modified functest for this ...

Must check this, understand what's happened here.

Copy link
Member Author

@kzangeli kzangeli Jan 18, 2017

Choose a reason for hiding this comment

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

There wasn't any, and the problem wasn't even fixed.
I just assumed (and forgot about the functest), that the low level parse functions would take care of this (as they should). However subscriptions use their own implementation of entity parsing ...

Fixed in f422157

#
# This file is part of Orion Context Broker.
#
# Orion Context Broker is free software: you can redistribute it and/or
# modify it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# Orion Context Broker is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero
# General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with Orion Context Broker. If not, see http://www.gnu.org/licenses/.
#
# For those usages not covered by this license please contact with
# iot_support at tid dot es

# VALGRIND_READY - to mark the test ready for valgrindTestSuite.sh

--NAME--
invalid_service_path_gives_400_bad_request

--SHELL-INIT--
dbInit CB
brokerStart CB

--SHELL--

echo "01. Send a /version request with an Invalid Service Path, see 400 Bad Request"
echo "============================================================================="
orionCurl --url /version --servicePath "notStartingWithSlash"
echo
echo


--REGEXPECT--
01. Send a /version request with an Invalid Service Path, see 400 Bad Request
=============================================================================
HTTP/1.1 400 Bad Request
Content-Length: 111
Content-Type: application/json
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)

{
"description": "Only /absolute/ Service Paths allowed [a service path must begin with /]",
"error": "BadRequest"
}


--TEARDOWN--
brokerStop CB
dbDrop CB
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# Copyright 2016 Telefonica Investigacion y Desarrollo, S.A.U
#
# This file is part of Orion Context Broker.
#
# Orion Context Broker is free software: you can redistribute it and/or
# modify it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# Orion Context Broker is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero
# General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with Orion Context Broker. If not, see http://www.gnu.org/licenses/.
#
# For those usages not covered by this license please contact with
# iot_support at tid dot es

# VALGRIND_READY - to mark the test ready for valgrindTestSuite.sh

--NAME--
postEntities with errors in service-path

--SHELL-INIT--
dbInit CB
brokerStart CB

--SHELL--

#
# 01. Try to create an entity with more than one service path - see error
# 02. Make sure the entity doesn't exist in the database
#

echo "01. Try to create an entity with more than one service path - see error"
echo "======================================================================="
payload='{ "id": "E1", "type": "T1" }'
orionCurl --url /v2/entities --payload "$payload" --servicePath /E1,/E2
echo
echo


echo "02. Make sure the entity doesn't exist in the database"
echo "======================================================"
echo "db.entities.findOne()" | mongo ftest
Copy link
Member

Choose a reason for hiding this comment

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

Why don't use the GET /v2/entities operation to check it? We should use keep direct mongo queries only as last resort measure when the API doesn't provide alternatives.

(Btw, I think we have a mongoCmd() function for test harness to avoid direct calls to mongo shell)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, GET /v2/entities works also. As we usually do. Not sure why I implemented like this (paranoia perhaps)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 159bd5f

echo
echo


--REGEXPECT--
01. Try to create an entity with more than one service path - see error
=======================================================================
HTTP/1.1 400 Bad Request
Content-Length: 79
Content-Type: application/json
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)

{
"code": "400",
"details": "more than one service path in context update request"
}


02. Make sure the entity doesn't exist in the database
======================================================
MongoDB shell version: REGEX(.*)
connecting to: ftest
null
bye


--TEARDOWN--
brokerStop CB
dbDrop CB
Original file line number Diff line number Diff line change
Expand Up @@ -117,27 +117,27 @@ Date: REGEX(.*)
02. POST /v2/op/query with forbidden chars '<>=;' in entity::id, see it fail due to forbidden chars
===================================================================================================
HTTP/1.1 400 Bad Request
Content-Length: 67
Content-Length: 70
Content-Type: application/json
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)

{
"description": "forbidden chars in entity id",
"description": "Invalid characters in entity id",
"error": "BadRequest"
}


03. POST /v2/op/query with forbidden chars '<>=;' in entity::type, see it fail due to forbidden chars
=====================================================================================================
HTTP/1.1 400 Bad Request
Content-Length: 69
Content-Length: 72
Content-Type: application/json
Fiware-Correlator: REGEX([0-9a-f\-]{36})
Date: REGEX(.*)

{
"description": "forbidden chars in entity type",
"description": "Invalid characters in entity type",
"error": "BadRequest"
}

Expand Down