From b7e78c91d2ae013ee71a7559b9ef9fe166860c89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20Walstr=C3=B6m?= Date: Tue, 29 Oct 2024 09:10:39 +0100 Subject: [PATCH] restconf: add upstream patches for rousette --- ...the-list-key-values-when-they-differ.patch | 199 ++++++++ ...ame-url-encoding-to-percent-encoding.patch | 50 ++ ...0003-uri-correct-x3-rule-class-names.patch | 34 ++ ...should-work-on-raw-percent-encoded-p.patch | 152 +++++++ ...y-values-checking-must-respect-libya.patch | 428 ++++++++++++++++++ ...-test-querying-lists-with-union-keys.patch | 300 ++++++++++++ 6 files changed, 1163 insertions(+) create mode 100644 patches/rousette/v1/0001-restconf-report-the-list-key-values-when-they-differ.patch create mode 100644 patches/rousette/v1/0002-uri-rename-url-encoding-to-percent-encoding.patch create mode 100644 patches/rousette/v1/0003-uri-correct-x3-rule-class-names.patch create mode 100644 patches/rousette/v1/0004-restconf-parser-should-work-on-raw-percent-encoded-p.patch create mode 100644 patches/rousette/v1/0005-restconf-list-key-values-checking-must-respect-libya.patch create mode 100644 patches/rousette/v1/0006-tests-test-querying-lists-with-union-keys.patch diff --git a/patches/rousette/v1/0001-restconf-report-the-list-key-values-when-they-differ.patch b/patches/rousette/v1/0001-restconf-report-the-list-key-values-when-they-differ.patch new file mode 100644 index 000000000..862551303 --- /dev/null +++ b/patches/rousette/v1/0001-restconf-report-the-list-key-values-when-they-differ.patch @@ -0,0 +1,199 @@ +From 9622a68eba4aeaa60619b4c33d050ce91b27653d Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Tue, 8 Oct 2024 12:22:49 +0200 +Subject: [PATCH 1/6] restconf: report the list key values when they differ + between URI and data +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +For creating (leaf-)list instances with PUT and PATCH methods one has +to specify the list key values in the URI and in the data as well. +Those values must be the same. In case they are not, it is an error. +We reported that the values mismatch in case this happens, but the error +message did not report what the values were. +Knowing that might be beneficial when one is about to insert key values +that can be namespace qualified (like identityrefs) and that are +sometimes manipulated by libyang (e.g., when the identity belongs to the +same namespace as the list node, it is not necessary for it to be +namespace qualified by the client, but libyang adds the namespace +internally). + +Change-Id: Ie0d42511bde01ab4c39d61370b6601f8808e40c5 +Signed-off-by: Mattias Walström +--- + src/restconf/Server.cpp | 29 +++++++++++++++++++++-------- + tests/restconf-plain-patch.cpp | 2 +- + tests/restconf-writing.cpp | 14 +++++++------- + tests/restconf-yang-patch.cpp | 2 +- + 4 files changed, 30 insertions(+), 17 deletions(-) + +diff --git a/src/restconf/Server.cpp b/src/restconf/Server.cpp +index 53d6625..5f560ed 100644 +--- a/src/restconf/Server.cpp ++++ b/src/restconf/Server.cpp +@@ -154,9 +154,22 @@ auto rejectYangPatch(const std::string& patchId, const std::string& editId) + }; + } + ++struct KeyMismatch { ++ libyang::DataNode offendingNode; ++ std::optional uriKeyValue; ++ ++ std::string message() const { ++ if (uriKeyValue) { ++ return "List key mismatch between URI path ('"s + *uriKeyValue + "') and data ('" + offendingNode.asTerm().valueStr() + "')."; ++ } else { ++ return "List key mismatch (key missing in the data)."; ++ } ++ } ++}; ++ + /** @brief In case node is a (leaf-)list check if the key values are the same as the keys specified in the lastPathSegment. + * @return The node where the mismatch occurs */ +-std::optional checkKeysMismatch(const libyang::DataNode& node, const PathSegment& lastPathSegment) ++std::optional checkKeysMismatch(const libyang::DataNode& node, const PathSegment& lastPathSegment) + { + if (node.schema().nodeType() == libyang::NodeType::List) { + const auto& listKeys = node.schema().asList().keys(); +@@ -164,18 +177,18 @@ std::optional checkKeysMismatch(const libyang::DataNode& node + const auto& keyValueURI = lastPathSegment.keys[i]; + auto keyNodeData = node.findPath(listKeys[i].module().name() + ':' + listKeys[i].name()); + if (!keyNodeData) { +- return node; ++ return KeyMismatch{node, std::nullopt}; + } + + const auto& keyValueData = keyNodeData->asTerm().valueStr(); + + if (keyValueURI != keyValueData) { +- return keyNodeData; ++ return KeyMismatch{*keyNodeData, keyValueURI}; + } + } + } else if (node.schema().nodeType() == libyang::NodeType::Leaflist) { + if (lastPathSegment.keys[0] != node.asTerm().valueStr()) { +- return node; ++ return KeyMismatch{node, lastPathSegment.keys[0]}; + } + } + return std::nullopt; +@@ -350,8 +363,8 @@ libyang::CreatedNodes createEditForPutAndPatch(libyang::Context& ctx, const std: + if (isSameNode(child, lastPathSegment)) { + // 1) a single child that is created by parseSubtree(), its name is the same as `lastPathSegment`. + // It could be a list; then we need to check if the keys in provided data match the keys in URI. +- if (auto offendingNode = checkKeysMismatch(child, lastPathSegment)) { +- throw ErrorResponse(400, "protocol", "invalid-value", "List key mismatch between URI path and data.", offendingNode->path()); ++ if (auto keyMismatch = checkKeysMismatch(child, lastPathSegment)) { ++ throw ErrorResponse(400, "protocol", "invalid-value", keyMismatch->message(), keyMismatch->offendingNode.path()); + } + replacementNode = child; + } else if (isKeyNode(*node, child)) { +@@ -373,8 +386,8 @@ libyang::CreatedNodes createEditForPutAndPatch(libyang::Context& ctx, const std: + if (!isSameNode(*replacementNode, lastPathSegment)) { + throw ErrorResponse(400, "protocol", "invalid-value", "Data contains invalid node.", replacementNode->path()); + } +- if (auto offendingNode = checkKeysMismatch(*parent, lastPathSegment)) { +- throw ErrorResponse(400, "protocol", "invalid-value", "List key mismatch between URI path and data.", offendingNode->path()); ++ if (auto keyMismatch = checkKeysMismatch(*parent, lastPathSegment)) { ++ throw ErrorResponse(400, "protocol", "invalid-value", keyMismatch->message(), keyMismatch->offendingNode.path()); + } + } + } +diff --git a/tests/restconf-plain-patch.cpp b/tests/restconf-plain-patch.cpp +index 10d653a..d4f3952 100644 +--- a/tests/restconf-plain-patch.cpp ++++ b/tests/restconf-plain-patch.cpp +@@ -72,7 +72,7 @@ TEST_CASE("Plain patch") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='blabla']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('libyang') and data ('blabla')." + } + ] + } +diff --git a/tests/restconf-writing.cpp b/tests/restconf-writing.cpp +index d46952f..96dbb25 100644 +--- a/tests/restconf-writing.cpp ++++ b/tests/restconf-writing.cpp +@@ -432,7 +432,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='ahoj']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('netconf') and data ('ahoj')." + } + ] + } +@@ -447,7 +447,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:top-level-list[name='ahoj']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('netconf') and data ('ahoj')." + } + ] + } +@@ -505,7 +505,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='netconf']/collection[.='666']", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('667') and data ('666')." + } + ] + } +@@ -520,7 +520,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:top-level-leaf-list[.='666']", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('667') and data ('666')." + } + ] + } +@@ -535,7 +535,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='sysrepo']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('netconf') and data ('sysrepo')." + } + ] + } +@@ -550,7 +550,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='sysrepo']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('netconf') and data ('sysrepo')." + } + ] + } +@@ -565,7 +565,7 @@ TEST_CASE("writing data") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='libyang']/collection[.='42']", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('5') and data ('42')." + } + ] + } +diff --git a/tests/restconf-yang-patch.cpp b/tests/restconf-yang-patch.cpp +index 9d70912..7cc8946 100644 +--- a/tests/restconf-yang-patch.cpp ++++ b/tests/restconf-yang-patch.cpp +@@ -436,7 +436,7 @@ TEST_CASE("YANG patch") + "error-type": "protocol", + "error-tag": "invalid-value", + "error-path": "/example:tlc/list[name='asdasdauisbdhaijbsdad']/name", +- "error-message": "List key mismatch between URI path and data." ++ "error-message": "List key mismatch between URI path ('libyang') and data ('asdasdauisbdhaijbsdad')." + } + ] + } +-- +2.43.0 + diff --git a/patches/rousette/v1/0002-uri-rename-url-encoding-to-percent-encoding.patch b/patches/rousette/v1/0002-uri-rename-url-encoding-to-percent-encoding.patch new file mode 100644 index 000000000..64233a6fb --- /dev/null +++ b/patches/rousette/v1/0002-uri-rename-url-encoding-to-percent-encoding.patch @@ -0,0 +1,50 @@ +From 070cffb48fda789910581930265d4624a7213e1b Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Wed, 16 Oct 2024 11:02:45 +0200 +Subject: [PATCH 2/6] uri: rename url-encoding to percent-encoding +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +RFC 3986 [1] calls it the percent-encoding, let's be consistent. + +[1] https://datatracker.ietf.org/doc/html/rfc3986 + +Change-Id: Iee8b76c980b2694b6643e627b462f8bfc2c21c45 +Signed-off-by: Mattias Walström +--- + src/restconf/uri.cpp | 6 +++--- + 1 file changed, 3 insertions(+), 3 deletions(-) + +diff --git a/src/restconf/uri.cpp b/src/restconf/uri.cpp +index 6e27168..b144d92 100644 +--- a/src/restconf/uri.cpp ++++ b/src/restconf/uri.cpp +@@ -29,12 +29,12 @@ auto add = [](auto& ctx) { + char c = std::tolower(_attr(ctx)); + _val(ctx) = _val(ctx) * 16 + (c >= 'a' ? c - 'a' + 10 : c - '0'); + }; +-const auto urlEncodedChar = x3::rule{"urlEncodedChar"} = x3::lit('%')[set_zero] >> x3::xdigit[add] >> x3::xdigit[add]; ++const auto percentEncodedChar = x3::rule{"percentEncodedChar"} = x3::lit('%')[set_zero] >> x3::xdigit[add] >> x3::xdigit[add]; + + /* reserved characters according to RFC 3986, sec. 2.2 with '%' added. The '%' character is not specified as reserved but it effectively is because + * "Percent sign serves as the indicator for percent-encoded octets, it must be percent-encoded (...)" [RFC 3986, sec. 2.4]. */ + const auto reservedChars = x3::lit(':') | '/' | '?' | '#' | '[' | ']' | '@' | '!' | '$' | '&' | '\'' | '(' | ')' | '*' | '+' | ',' | ',' | ';' | '=' | '%'; +-const auto keyValue = x3::rule{"keyValue"} = *(urlEncodedChar | (x3::char_ - reservedChars)); ++const auto keyValue = x3::rule{"keyValue"} = *(percentEncodedChar | (x3::char_ - reservedChars)); + + const auto keyList = x3::rule>{"keyList"} = keyValue % ','; + const auto identifier = x3::rule{"identifier"} = (x3::alpha | x3::char_('_')) >> *(x3::alnum | x3::char_('_') | x3::char_('-') | x3::char_('.')); +@@ -117,7 +117,7 @@ const auto dateAndTime = x3::rule{"dateAndTime"} + x3::repeat(4)[x3::digit] >> x3::char_('-') >> x3::repeat(2)[x3::digit] >> x3::char_('-') >> x3::repeat(2)[x3::digit] >> x3::char_('T') >> + x3::repeat(2)[x3::digit] >> x3::char_(':') >> x3::repeat(2)[x3::digit] >> x3::char_(':') >> x3::repeat(2)[x3::digit] >> -(x3::char_('.') >> +x3::digit) >> + (x3::char_('Z') | (-(x3::char_('+')|x3::char_('-')) >> x3::repeat(2)[x3::digit] >> x3::char_(':') >> x3::repeat(2)[x3::digit])); +-const auto filter = x3::rule{"filter"} = +(urlEncodedChar | (x3::char_ - '&')); ++const auto filter = x3::rule{"filter"} = +(percentEncodedChar | (x3::char_ - '&')); + const auto depthParam = x3::rule{"depthParam"} = x3::uint_[validDepthValues] | (x3::string("unbounded") >> x3::attr(queryParams::UnboundedDepth{})); + const auto queryParamPair = x3::rule>{"queryParamPair"} = + (x3::string("depth") >> "=" >> depthParam) | +-- +2.43.0 + diff --git a/patches/rousette/v1/0003-uri-correct-x3-rule-class-names.patch b/patches/rousette/v1/0003-uri-correct-x3-rule-class-names.patch new file mode 100644 index 000000000..17db73ebd --- /dev/null +++ b/patches/rousette/v1/0003-uri-correct-x3-rule-class-names.patch @@ -0,0 +1,34 @@ +From 8a233202647f24538f2bbb8fff1e38d52e3599a4 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Wed, 16 Oct 2024 11:05:09 +0200 +Subject: [PATCH 3/6] uri: correct x3 rule class names +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +Fixes: e06d5bf ("server: parser for data resources in URI paths") +Change-Id: Ic953e568d841032113ede1c0e896574361c0ebe2 +Signed-off-by: Mattias Walström +--- + src/restconf/uri.cpp | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/restconf/uri.cpp b/src/restconf/uri.cpp +index b144d92..8e8dc23 100644 +--- a/src/restconf/uri.cpp ++++ b/src/restconf/uri.cpp +@@ -37,8 +37,8 @@ const auto reservedChars = x3::lit(':') | '/' | '?' | '#' | '[' | ']' | '@' | '! + const auto keyValue = x3::rule{"keyValue"} = *(percentEncodedChar | (x3::char_ - reservedChars)); + + const auto keyList = x3::rule>{"keyList"} = keyValue % ','; +-const auto identifier = x3::rule{"identifier"} = (x3::alpha | x3::char_('_')) >> *(x3::alnum | x3::char_('_') | x3::char_('-') | x3::char_('.')); +-const auto apiIdentifier = x3::rule{"apiIdentifier"} = -(identifier >> ':') >> identifier; ++const auto identifier = x3::rule{"identifier"} = (x3::alpha | x3::char_('_')) >> *(x3::alnum | x3::char_('_') | x3::char_('-') | x3::char_('.')); ++const auto apiIdentifier = x3::rule{"apiIdentifier"} = -(identifier >> ':') >> identifier; + const auto listInstance = x3::rule{"listInstance"} = apiIdentifier >> -('=' >> keyList); + const auto fullyQualifiedApiIdentifier = x3::rule{"apiIdentifier"} = identifier >> ':' >> identifier; + const auto fullyQualifiedListInstance = x3::rule{"listInstance"} = fullyQualifiedApiIdentifier >> -('=' >> keyList); +-- +2.43.0 + diff --git a/patches/rousette/v1/0004-restconf-parser-should-work-on-raw-percent-encoded-p.patch b/patches/rousette/v1/0004-restconf-parser-should-work-on-raw-percent-encoded-p.patch new file mode 100644 index 000000000..5fa975e14 --- /dev/null +++ b/patches/rousette/v1/0004-restconf-parser-should-work-on-raw-percent-encoded-p.patch @@ -0,0 +1,152 @@ +From 96cbf730010ee9539d05d0d72697dc960b3a938c Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Mon, 7 Oct 2024 20:46:24 +0200 +Subject: [PATCH 4/6] restconf: parser should work on raw percent-encoded paths +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +The difference between nghttp2's uri_ref::path and raw_path is that the +raw_path keeps the percent encoding, while the path converts the percent +encoded chars to their "normal" form. + +Our parser expects some parts of the URI to be percent encoded so we +have to use raw_paths everywhere. + +I thought about rewriting the parser not to expect percent encoded +characters but that would bring some complications. For instance when +querying lists, the RESTCONF RFC specifies that every key value is +percent encoded and individual key values are delimited by commas [1]. +So, when somebody sends a request like /restconf/data/list=a%2Cb,c the +"normal" form is /restconf/data/list=a,b,c and in that case we obtain +three keys, but the client sent only two, where the first one contained +comma. + +I am adding few tests to check for percent encoded values. + +We realized this while working on one of the reported bugs [1]. The +query sent by the client there is wrong, the ':' char should be +percent-encoded. + +[1] https://github.com/CESNET/rousette/issues/12 + +Bug: https://github.com/CESNET/rousette/issues/12 +Change-Id: I473501cef3c8eae9af0c5d0751393cdad647e23c +Signed-off-by: Mattias Walström +--- + src/restconf/Server.cpp | 8 ++++---- + src/restconf/uri.cpp | 4 ++-- + tests/restconf-reading.cpp | 15 +++++++++++++++ + tests/uri-parser.cpp | 5 +++-- + 4 files changed, 24 insertions(+), 8 deletions(-) + +diff --git a/src/restconf/Server.cpp b/src/restconf/Server.cpp +index 5f560ed..79d8ff6 100644 +--- a/src/restconf/Server.cpp ++++ b/src/restconf/Server.cpp +@@ -416,7 +416,7 @@ void processActionOrRPC(std::shared_ptr requestCtx, const std::c + * - The data node exists but might get deleted right after this check: Sysrepo throws an error when this happens. + * - The data node does not exist but might get created right after this check: The node was not there when the request was issues so it should not be a problem + */ +- auto [pathToParent, pathSegment] = asLibyangPathSplit(ctx, requestCtx->req.uri().path); ++ auto [pathToParent, pathSegment] = asLibyangPathSplit(ctx, requestCtx->req.uri().raw_path); + if (!requestCtx->sess.getData(pathToParent, 0, sysrepo::GetOptions::Default, timeout)) { + throw ErrorResponse(400, "application", "operation-failed", "Action data node '" + requestCtx->restconfRequest.path + "' does not exist."); + } +@@ -539,7 +539,7 @@ void processYangPatchEdit(const std::shared_ptr& requestCtx, con + auto target = childLeafValue(editContainer, "target"); + auto operation = childLeafValue(editContainer, "operation"); + +- auto [singleEdit, replacementNode] = createEditForPutAndPatch(ctx, requestCtx->req.uri().path + target, yangPatchValueAsJSON(editContainer), libyang::DataFormat::JSON); ++ auto [singleEdit, replacementNode] = createEditForPutAndPatch(ctx, requestCtx->req.uri().raw_path + target, yangPatchValueAsJSON(editContainer), libyang::DataFormat::JSON); + validateInputMetaAttributes(ctx, *singleEdit); + + // insert and move are not defined in RFC6241. sec 7.3 and sysrepo does not support them directly +@@ -658,7 +658,7 @@ void processPutOrPlainPatch(std::shared_ptr requestCtx, const st + throw ErrorResponse(400, "protocol", "invalid-value", "Target resource does not exist"); + } + +- auto [edit, replacementNode] = createEditForPutAndPatch(ctx, requestCtx->req.uri().path, requestCtx->payload, *requestCtx->dataFormat.request /* caller checks if the dataFormat.request is present */); ++ auto [edit, replacementNode] = createEditForPutAndPatch(ctx, requestCtx->req.uri().raw_path, requestCtx->payload, *requestCtx->dataFormat.request /* caller checks if the dataFormat.request is present */); + validateInputMetaAttributes(ctx, *edit); + + if (requestCtx->req.method() == "PUT") { +@@ -954,7 +954,7 @@ Server::Server(sysrepo::Connection conn, const std::string& address, const std:: + dataFormat = chooseDataEncoding(req.header()); + authorizeRequest(nacm, sess, req); + +- auto restconfRequest = asRestconfRequest(sess.getContext(), req.method(), req.uri().path, req.uri().raw_query); ++ auto restconfRequest = asRestconfRequest(sess.getContext(), req.method(), req.uri().raw_path, req.uri().raw_query); + + switch (restconfRequest.type) { + case RestconfRequest::Type::RestconfRoot: +diff --git a/src/restconf/uri.cpp b/src/restconf/uri.cpp +index 8e8dc23..ac399b7 100644 +--- a/src/restconf/uri.cpp ++++ b/src/restconf/uri.cpp +@@ -34,9 +34,9 @@ const auto percentEncodedChar = x3::rule{"pe + /* reserved characters according to RFC 3986, sec. 2.2 with '%' added. The '%' character is not specified as reserved but it effectively is because + * "Percent sign serves as the indicator for percent-encoded octets, it must be percent-encoded (...)" [RFC 3986, sec. 2.4]. */ + const auto reservedChars = x3::lit(':') | '/' | '?' | '#' | '[' | ']' | '@' | '!' | '$' | '&' | '\'' | '(' | ')' | '*' | '+' | ',' | ',' | ';' | '=' | '%'; +-const auto keyValue = x3::rule{"keyValue"} = *(percentEncodedChar | (x3::char_ - reservedChars)); ++const auto percentEncodedString = x3::rule{"percentEncodedString"} = *(percentEncodedChar | (x3::char_ - reservedChars)); + +-const auto keyList = x3::rule>{"keyList"} = keyValue % ','; ++const auto keyList = x3::rule>{"keyList"} = percentEncodedString % ','; + const auto identifier = x3::rule{"identifier"} = (x3::alpha | x3::char_('_')) >> *(x3::alnum | x3::char_('_') | x3::char_('-') | x3::char_('.')); + const auto apiIdentifier = x3::rule{"apiIdentifier"} = -(identifier >> ':') >> identifier; + const auto listInstance = x3::rule{"listInstance"} = apiIdentifier >> -('=' >> keyList); +diff --git a/tests/restconf-reading.cpp b/tests/restconf-reading.cpp +index fa1cbcc..2898839 100644 +--- a/tests/restconf-reading.cpp ++++ b/tests/restconf-reading.cpp +@@ -261,6 +261,21 @@ TEST_CASE("reading data") + } + )"}); + ++ // percent-encoded comma is a part of the key value, it is not a delimiter ++ REQUIRE(get(RESTCONF_DATA_ROOT "/ietf-system:system/radius/server=a%2Cb", {AUTH_DWDM}) == Response{404, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "application", ++ "error-tag": "invalid-value", ++ "error-message": "No data from sysrepo." ++ } ++ ] ++ } ++} ++)"}); ++ ++ // comma is a delimiter of list key values + REQUIRE(get(RESTCONF_DATA_ROOT "/ietf-system:system/radius/server=a,b", {AUTH_DWDM}) == Response{400, jsonHeaders, R"({ + "ietf-restconf:errors": { + "error": [ +diff --git a/tests/uri-parser.cpp b/tests/uri-parser.cpp +index a748e09..5977afc 100644 +--- a/tests/uri-parser.cpp ++++ b/tests/uri-parser.cpp +@@ -158,9 +158,9 @@ TEST_CASE("URI path parser") + {{"prefix", "lst"}, {"key1"}}, + {{"prefix", "leaf"}}, + }}}, +- {"/restconf/data/foo:bar/lst=key1,,key3", {{ ++ {"/restconf/data/foo:bar/lst=module%3Akey1,,key3", {{ + {{"foo", "bar"}}, +- {{"lst"}, {"key1", "", "key3"}}, ++ {{"lst"}, {"module:key1", "", "key3"}}, + }}}, + {"/restconf/data/foo:bar/lst=key%2CWithCommas,,key2C", {{ + {{"foo", "bar"}}, +@@ -240,6 +240,7 @@ TEST_CASE("URI path parser") + "/restconf/data/foo:list=A%2", + "/restconf/data/foo:list=A%2,", + "/restconf/data/foo:bar/list1=%%", ++ "/restconf/data/foo:bar/list1=module:smth", + "/restconf/data/foo:bar/", + "/restconf/data/ foo : bar", + "/rest conf/data / foo:bar", +-- +2.43.0 + diff --git a/patches/rousette/v1/0005-restconf-list-key-values-checking-must-respect-libya.patch b/patches/rousette/v1/0005-restconf-list-key-values-checking-must-respect-libya.patch new file mode 100644 index 000000000..0b5c82669 --- /dev/null +++ b/patches/rousette/v1/0005-restconf-list-key-values-checking-must-respect-libya.patch @@ -0,0 +1,428 @@ +From 8b13c1e4ccaa61a241674c27063439e257fa88de Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Wed, 2 Oct 2024 20:21:28 +0200 +Subject: [PATCH 5/6] restconf: list key values checking must respect libyang's + canonicalization +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +While dealing with the issue that was fixed in the previous commit we +realized that the issue is deeper. Not only that our parser rejected +the input when someone used identityref with module prefix in the URI, +but also, our internal code for creating/modifying list entries was +wrong. + +In PUT requests we have to check if user entered the same key value in +URI and yang-data payload. However, some values are canonicalized by +libyang (e.g. decimal64 type with fraction-digits=2 or even the +identityrefs) and so if the client entered two different but +canonically equivalent values, we would reject such request. + +Bug: https://github.com/CESNET/rousette/issues/12 +Change-Id: I44245d831e8de6d0e6f991fcd18319c095b49b1d +Signed-off-by: Mattias Walström +--- + CMakeLists.txt | 3 +- + src/restconf/Server.cpp | 49 +++++++++++++++++---- + src/restconf/utils/yang.cpp | 5 +++ + src/restconf/utils/yang.h | 1 + + tests/restconf-reading.cpp | 59 +++++++++++++++++++++++++ + tests/restconf-writing.cpp | 82 +++++++++++++++++++++++++++++++++++ + tests/uri-parser.cpp | 2 + + tests/yang/example-types.yang | 13 ++++++ + tests/yang/example.yang | 25 +++++++++++ + 9 files changed, 229 insertions(+), 10 deletions(-) + create mode 100644 tests/yang/example-types.yang + +diff --git a/CMakeLists.txt b/CMakeLists.txt +index c563dcf..465bef9 100644 +--- a/CMakeLists.txt ++++ b/CMakeLists.txt +@@ -204,7 +204,8 @@ if(BUILD_TESTING) + --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example.yang --enable-feature f1 + --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example-delete.yang + --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example-augment.yang +- --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example-notif.yang) ++ --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example-notif.yang ++ --install ${CMAKE_CURRENT_SOURCE_DIR}/tests/yang/example-types.yang) + rousette_test(NAME restconf-reading LIBRARIES rousette-restconf FIXTURE common-models WRAP_PAM) + rousette_test(NAME restconf-writing LIBRARIES rousette-restconf FIXTURE common-models WRAP_PAM) + rousette_test(NAME restconf-delete LIBRARIES rousette-restconf FIXTURE common-models WRAP_PAM) +diff --git a/src/restconf/Server.cpp b/src/restconf/Server.cpp +index 79d8ff6..b515d66 100644 +--- a/src/restconf/Server.cpp ++++ b/src/restconf/Server.cpp +@@ -154,6 +154,15 @@ auto rejectYangPatch(const std::string& patchId, const std::string& editId) + }; + } + ++/** @brief Check if these two paths compare the same after path canonicalization */ ++bool compareKeyValue(const libyang::Context& ctx, const std::string& pathA, const std::string& pathB) ++{ ++ auto [parentA, nodeA] = ctx.newPath2(pathA, std::nullopt); ++ auto [parentB, nodeB] = ctx.newPath2(pathB, std::nullopt); ++ ++ return nodeA->asTerm().valueStr() == nodeB->asTerm().valueStr(); ++} ++ + struct KeyMismatch { + libyang::DataNode offendingNode; + std::optional uriKeyValue; +@@ -169,25 +178,46 @@ struct KeyMismatch { + + /** @brief In case node is a (leaf-)list check if the key values are the same as the keys specified in the lastPathSegment. + * @return The node where the mismatch occurs */ +-std::optional checkKeysMismatch(const libyang::DataNode& node, const PathSegment& lastPathSegment) ++std::optional checkKeysMismatch(libyang::Context& ctx, const libyang::DataNode& node, const std::string& lyParentPath, const PathSegment& lastPathSegment) + { ++ const auto pathPrefix = (lyParentPath.empty() ? "" : lyParentPath) + "/" + lastPathSegment.apiIdent.name(); ++ + if (node.schema().nodeType() == libyang::NodeType::List) { + const auto& listKeys = node.schema().asList().keys(); + for (size_t i = 0; i < listKeys.size(); ++i) { +- const auto& keyValueURI = lastPathSegment.keys[i]; + auto keyNodeData = node.findPath(listKeys[i].module().name() + ':' + listKeys[i].name()); + if (!keyNodeData) { + return KeyMismatch{node, std::nullopt}; + } + +- const auto& keyValueData = keyNodeData->asTerm().valueStr(); +- +- if (keyValueURI != keyValueData) { +- return KeyMismatch{*keyNodeData, keyValueURI}; ++ /* ++ * If the key's value has a canonical form then libyang makes the value canonical ++ * but there is no guarantee that the client provided the value in the canonical form. ++ * ++ * Let libyang do the work. Create two data nodes, one with the key value from the data and the other ++ * with the key value from the URI. Then compare the values from the two nodes. If they are different, ++ * they certainly mismatch. ++ * ++ * This can happen in cases like ++ * * The key's type is identityref and the client provided the key value as a string without the module name. Libyang will canonicalize the value by adding the module name. ++ * * The key's type is decimal64 with fractional-digits 2; then the client can provide the value as 1.0 or 1.00 and they should be the same. Libyang will canonicalize the value. ++ */ ++ ++ auto keysWithValueFromData = lastPathSegment.keys; ++ keysWithValueFromData[i] = keyNodeData->asTerm().valueStr(); ++ ++ const auto suffix = "/" + listKeys[i].name(); ++ const auto pathFromData = pathPrefix + listKeyPredicate(listKeys, keysWithValueFromData) + suffix; ++ const auto pathFromURI = pathPrefix + listKeyPredicate(listKeys, lastPathSegment.keys) + suffix; ++ ++ if (!compareKeyValue(ctx, pathFromData, pathFromURI)) { ++ return KeyMismatch{*keyNodeData, lastPathSegment.keys[i]}; + } + } + } else if (node.schema().nodeType() == libyang::NodeType::Leaflist) { +- if (lastPathSegment.keys[0] != node.asTerm().valueStr()) { ++ const auto pathFromData = pathPrefix + leaflistKeyPredicate(node.asTerm().valueStr()); ++ const auto pathFromURI = pathPrefix + leaflistKeyPredicate(lastPathSegment.keys[0]); ++ if (!compareKeyValue(ctx, pathFromData, pathFromURI)) { + return KeyMismatch{node, lastPathSegment.keys[0]}; + } + } +@@ -363,7 +393,7 @@ libyang::CreatedNodes createEditForPutAndPatch(libyang::Context& ctx, const std: + if (isSameNode(child, lastPathSegment)) { + // 1) a single child that is created by parseSubtree(), its name is the same as `lastPathSegment`. + // It could be a list; then we need to check if the keys in provided data match the keys in URI. +- if (auto keyMismatch = checkKeysMismatch(child, lastPathSegment)) { ++ if (auto keyMismatch = checkKeysMismatch(ctx, child, lyParentPath, lastPathSegment)) { + throw ErrorResponse(400, "protocol", "invalid-value", keyMismatch->message(), keyMismatch->offendingNode.path()); + } + replacementNode = child; +@@ -386,7 +416,8 @@ libyang::CreatedNodes createEditForPutAndPatch(libyang::Context& ctx, const std: + if (!isSameNode(*replacementNode, lastPathSegment)) { + throw ErrorResponse(400, "protocol", "invalid-value", "Data contains invalid node.", replacementNode->path()); + } +- if (auto keyMismatch = checkKeysMismatch(*parent, lastPathSegment)) { ++ ++ if (auto keyMismatch = checkKeysMismatch(ctx, *parent, lyParentPath, lastPathSegment)) { + throw ErrorResponse(400, "protocol", "invalid-value", keyMismatch->message(), keyMismatch->offendingNode.path()); + } + } +diff --git a/src/restconf/utils/yang.cpp b/src/restconf/utils/yang.cpp +index 15cceb6..4c4d619 100644 +--- a/src/restconf/utils/yang.cpp ++++ b/src/restconf/utils/yang.cpp +@@ -50,6 +50,11 @@ std::string listKeyPredicate(const std::vector& listKeyLeafs, con + return res; + } + ++std::string leaflistKeyPredicate(const std::string& keyValue) ++{ ++ return "[.=" + escapeListKey(keyValue) + ']'; ++} ++ + bool isUserOrderedList(const libyang::DataNode& node) + { + if (node.schema().nodeType() == libyang::NodeType::List) { +diff --git a/src/restconf/utils/yang.h b/src/restconf/utils/yang.h +index 677d049..e91ba8a 100644 +--- a/src/restconf/utils/yang.h ++++ b/src/restconf/utils/yang.h +@@ -16,6 +16,7 @@ namespace rousette::restconf { + + std::string escapeListKey(const std::string& str); + std::string listKeyPredicate(const std::vector& listKeyLeafs, const std::vector& keyValues); ++std::string leaflistKeyPredicate(const std::string& keyValue); + bool isUserOrderedList(const libyang::DataNode& node); + bool isKeyNode(const libyang::DataNode& maybeList, const libyang::DataNode& node); + } +diff --git a/tests/restconf-reading.cpp b/tests/restconf-reading.cpp +index 2898839..96c38ab 100644 +--- a/tests/restconf-reading.cpp ++++ b/tests/restconf-reading.cpp +@@ -287,6 +287,65 @@ TEST_CASE("reading data") + ] + } + } ++)"}); ++ ++ srSess.setItem("/example:list-with-identity-key[type='example:derived-identity'][name='name']", std::nullopt); ++ srSess.setItem("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']", std::nullopt); ++ srSess.setItem("/example:tlc/decimal-list[.='1.00']", std::nullopt); ++ srSess.applyChanges(); ++ ++ // dealing with keys which can have prefixes (YANG identities) ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-identity-key": [ ++ { ++ "type": "derived-identity", ++ "name": "name" ++ } ++ ] ++} ++)"}); ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example%3Aderived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-identity-key": [ ++ { ++ "type": "derived-identity", ++ "name": "name" ++ } ++ ] ++} ++)"}); ++ ++ // an identity from another module must be namespace-qualified ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=another-derived-identity,name", {AUTH_ROOT}) == Response{404, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "application", ++ "error-tag": "invalid-value", ++ "error-message": "No data from sysrepo." ++ } ++ ] ++ } ++} ++)"}); ++ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-identity-key": [ ++ { ++ "type": "example-types:another-derived-identity", ++ "name": "name" ++ } ++ ] ++} ++)"}); ++ ++ // test canonicalization of list key values; the key value was inserted as "1.00" ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:tlc/decimal-list=1", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:tlc": { ++ "decimal-list": [ ++ "1.0" ++ ] ++ } ++} + )"}); + } + +diff --git a/tests/restconf-writing.cpp b/tests/restconf-writing.cpp +index 96dbb25..8418554 100644 +--- a/tests/restconf-writing.cpp ++++ b/tests/restconf-writing.cpp +@@ -389,6 +389,88 @@ TEST_CASE("writing data") + REQUIRE(put(RESTCONF_DATA_ROOT "/example:tlc/list=libyang/nested=11,12,13", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:nested": [{"first": "11", "second": 12, "third": "13"}]}]})") == Response{201, noContentTypeHeaders, ""}); + } + ++ SECTION("Test canonicalization of keys") ++ { ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']", std::nullopt), ++ CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/type", "example:derived-identity"), ++ CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/name", "name"), ++ CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/text", "blabla")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ // prefixed in the URI, not prefixed in the data ++ EXPECT_CHANGE( ++ MODIFIED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/text", "hehe")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example%3Aderived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "derived-identity", "text": "hehe"}]}]})") == Response{204, noContentTypeHeaders, ""}); ++ ++ ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=another-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{400, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "protocol", ++ "error-tag": "invalid-value", ++ "error-message": "Validation failure: Can't parse data: LY_EVALID" ++ } ++ ] ++ } ++} ++)"}); ++ ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']", std::nullopt), ++ CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/type", "example-types:another-derived-identity"), ++ CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/name", "name"), ++ CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/text", "blabla")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "example-types:another-derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ // missing namespace in the data ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{400, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "protocol", ++ "error-tag": "invalid-value", ++ "error-message": "Validation failure: Can't parse data: LY_EVALID" ++ } ++ ] ++ } ++} ++)"}); ++ ++ EXPECT_CHANGE(CREATED("/example:leaf-list-with-identity-key[.='example-types:another-derived-identity']", "example-types:another-derived-identity")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:leaf-list-with-identity-key=example-types%3Aanother-derived-identity", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:leaf-list-with-identity-key": ["example-types:another-derived-identity"]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ // missing namespace in the URI ++ EXPECT_CHANGE(CREATED("/example:leaf-list-with-identity-key[.='example:derived-identity']", "example:derived-identity")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:leaf-list-with-identity-key=derived-identity", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:leaf-list-with-identity-key": ["example:derived-identity"]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:leaf-list-with-identity-key=example-types%3Aanother-derived-identity", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:leaf-list-with-identity-key": ["example:derived-identity"]})") == Response{400, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "protocol", ++ "error-tag": "invalid-value", ++ "error-path": "/example:leaf-list-with-identity-key[.='example:derived-identity']", ++ "error-message": "List key mismatch between URI path ('example-types:another-derived-identity') and data ('example:derived-identity')." ++ } ++ ] ++ } ++} ++)"}); ++ ++ // value in the URI and in the data have the same canonical form ++ EXPECT_CHANGE(CREATED("/example:tlc/decimal-list[.='1.0']", "1.0")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:tlc/decimal-list=1.00", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:decimal-list": ["1.0"]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ // nothing is changed, still the same value ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:tlc/decimal-list=1.000", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:decimal-list": ["1"]})") == Response{204, noContentTypeHeaders, ""}); ++ ++ // different value ++ EXPECT_CHANGE(CREATED("/example:tlc/decimal-list[.='1.01']", "1.01")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:tlc/decimal-list=1.010", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:decimal-list": ["1.01"]})") == Response{201, noContentTypeHeaders, ""}); ++ } ++ + SECTION("Modify a leaf in a list entry") + { + EXPECT_CHANGE(MODIFIED("/example:tlc/list[name='libyang']/choice1", "restconf")); +diff --git a/tests/uri-parser.cpp b/tests/uri-parser.cpp +index 5977afc..7320c3c 100644 +--- a/tests/uri-parser.cpp ++++ b/tests/uri-parser.cpp +@@ -293,6 +293,7 @@ TEST_CASE("URI path parser") + {"/restconf/data/example:tlc/list=eth0/choice1", "/example:tlc/list[name='eth0']/choice1", std::nullopt}, + {"/restconf/data/example:tlc/list=eth0/choice2", "/example:tlc/list[name='eth0']/choice2", std::nullopt}, + {"/restconf/data/example:tlc/list=eth0/collection=val", "/example:tlc/list[name='eth0']/collection[.='val']", std::nullopt}, ++ {"/restconf/data/example:list-with-identity-key=example-types%3Aanother-derived-identity,aaa", "/example:list-with-identity-key[type='example-types:another-derived-identity'][name='aaa']", std::nullopt}, + {"/restconf/data/example:tlc/status", "/example:tlc/status", std::nullopt}, + // container example:a has a container b inserted locally and also via an augment. Check that we return the correct one + {"/restconf/data/example:a/b", "/example:a/b", std::nullopt}, +@@ -327,6 +328,7 @@ TEST_CASE("URI path parser") + {"/restconf/data/example:tlc/status", "/example:tlc", {{"example", "status"}}}, + {"/restconf/data/example:a/example-augment:b/c", "/example:a/example-augment:b", {{"example-augment", "c"}}}, + {"/restconf/ds/ietf-datastores:startup/example:a/example-augment:b/c", "/example:a/example-augment:b", {{"example-augment", "c"}}}, ++ {"/restconf/data/example:list-with-identity-key=example-types%3Aanother-derived-identity,aaa", "", {{"example", "list-with-identity-key"}, {"example-types:another-derived-identity", "aaa"}}}, + }) { + CAPTURE(httpMethod); + CAPTURE(expectedRequestType); +diff --git a/tests/yang/example-types.yang b/tests/yang/example-types.yang +new file mode 100644 +index 0000000..5bc2fb0 +--- /dev/null ++++ b/tests/yang/example-types.yang +@@ -0,0 +1,13 @@ ++module example-types { ++ yang-version 1.1; ++ namespace "http://example.tld/example-types"; ++ prefix ex-types; ++ ++ import example { ++ prefix ex; ++ } ++ ++ identity another-derived-identity { ++ base ex:base-identity; ++ } ++} +diff --git a/tests/yang/example.yang b/tests/yang/example.yang +index df1301f..c46273c 100644 +--- a/tests/yang/example.yang ++++ b/tests/yang/example.yang +@@ -6,6 +6,13 @@ module example { + feature f1 { } + feature f2 { } + ++ identity base-identity { ++ } ++ ++ identity derived-identity { ++ base base-identity; ++ } ++ + leaf top-level-leaf { type string; } + leaf top-level-leaf2 { type string; default "x"; } + +@@ -50,6 +57,11 @@ module example { + config false; + leaf name { type string; } + } ++ leaf-list decimal-list { ++ type decimal64 { ++ fraction-digits 2; ++ } ++ } + leaf status { + type enumeration { + enum on { } +@@ -109,6 +121,19 @@ module example { + } + } + ++ list list-with-identity-key { ++ key "type name"; ++ leaf type { ++ type identityref { base base-identity; } ++ } ++ leaf name { type string; } ++ leaf text { type string; } ++ } ++ ++ leaf-list leaf-list-with-identity-key { ++ type identityref { base base-identity; } ++ } ++ + rpc test-rpc { + input { + leaf i { +-- +2.43.0 + diff --git a/patches/rousette/v1/0006-tests-test-querying-lists-with-union-keys.patch b/patches/rousette/v1/0006-tests-test-querying-lists-with-union-keys.patch new file mode 100644 index 000000000..7f858349a --- /dev/null +++ b/patches/rousette/v1/0006-tests-test-querying-lists-with-union-keys.patch @@ -0,0 +1,300 @@ +From fda47b6a6cfdaecc24e96c4d6138c6de3ef116e0 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Pecka?= +Date: Mon, 7 Oct 2024 21:21:22 +0200 +Subject: [PATCH 6/6] tests: test querying lists with union keys +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit +Organization: Addiva Elektronik + +Test that we correctly work with keys that are unions of something +that can have a module namespace and that must not have it. + +By the way, enum values are not supposed to have a namespace prefix, +good to know [1]. + +[1] https://www.rfc-editor.org/rfc/rfc7951#section-6.4 + +Change-Id: I5a70f18117bb453330b4bb2ce0d2fb47d35b6ea6 +Signed-off-by: Mattias Walström +--- + tests/restconf-reading.cpp | 53 ++++++++++++++++++++++++----- + tests/restconf-writing.cpp | 68 ++++++++++++++++++++++++++++---------- + tests/uri-parser.cpp | 2 +- + tests/yang/example.yang | 26 +++++++++++++-- + 4 files changed, 120 insertions(+), 29 deletions(-) + +diff --git a/tests/restconf-reading.cpp b/tests/restconf-reading.cpp +index 96c38ab..2ded3f0 100644 +--- a/tests/restconf-reading.cpp ++++ b/tests/restconf-reading.cpp +@@ -289,14 +289,16 @@ TEST_CASE("reading data") + } + )"}); + +- srSess.setItem("/example:list-with-identity-key[type='example:derived-identity'][name='name']", std::nullopt); +- srSess.setItem("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']", std::nullopt); ++ srSess.setItem("/example:list-with-union-keys[type='example:derived-identity'][name='name']", std::nullopt); ++ srSess.setItem("/example:list-with-union-keys[type='example-types:another-derived-identity'][name='name']", std::nullopt); ++ srSess.setItem("/example:list-with-union-keys[type='fiii'][name='name']", std::nullopt); ++ srSess.setItem("/example:list-with-union-keys[type='zero'][name='name']", std::nullopt); // enum value + srSess.setItem("/example:tlc/decimal-list[.='1.00']", std::nullopt); + srSess.applyChanges(); + + // dealing with keys which can have prefixes (YANG identities) +- REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ +- "example:list-with-identity-key": [ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-union-keys": [ + { + "type": "derived-identity", + "name": "name" +@@ -304,8 +306,8 @@ TEST_CASE("reading data") + ] + } + )"}); +- REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example%3Aderived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ +- "example:list-with-identity-key": [ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example%3Aderived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-union-keys": [ + { + "type": "derived-identity", + "name": "name" +@@ -315,7 +317,7 @@ TEST_CASE("reading data") + )"}); + + // an identity from another module must be namespace-qualified +- REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=another-derived-identity,name", {AUTH_ROOT}) == Response{404, jsonHeaders, R"({ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=another-derived-identity,name", {AUTH_ROOT}) == Response{404, jsonHeaders, R"({ + "ietf-restconf:errors": { + "error": [ + { +@@ -328,8 +330,8 @@ TEST_CASE("reading data") + } + )"}); + +- REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ +- "example:list-with-identity-key": [ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example-types%3Aanother-derived-identity,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-union-keys": [ + { + "type": "example-types:another-derived-identity", + "name": "name" +@@ -346,6 +348,39 @@ TEST_CASE("reading data") + ] + } + } ++)"}); ++ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=zero,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-union-keys": [ ++ { ++ "type": "zero", ++ "name": "name" ++ } ++ ] ++} ++)"}); ++ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example%3Azero,name", {AUTH_ROOT}) == Response{404, jsonHeaders, R"({ ++ "ietf-restconf:errors": { ++ "error": [ ++ { ++ "error-type": "application", ++ "error-tag": "invalid-value", ++ "error-message": "No data from sysrepo." ++ } ++ ] ++ } ++} ++)"}); ++ ++ REQUIRE(get(RESTCONF_DATA_ROOT "/example:list-with-union-keys=fiii,name", {AUTH_ROOT}) == Response{200, jsonHeaders, R"({ ++ "example:list-with-union-keys": [ ++ { ++ "type": "fiii", ++ "name": "name" ++ } ++ ] ++} + )"}); + } + +diff --git a/tests/restconf-writing.cpp b/tests/restconf-writing.cpp +index 8418554..c1a9515 100644 +--- a/tests/restconf-writing.cpp ++++ b/tests/restconf-writing.cpp +@@ -392,46 +392,69 @@ TEST_CASE("writing data") + SECTION("Test canonicalization of keys") + { + EXPECT_CHANGE( +- CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']", std::nullopt), +- CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/type", "example:derived-identity"), +- CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/name", "name"), +- CREATED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/text", "blabla")); +- REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ CREATED("/example:list-with-union-keys[type='example:derived-identity'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='example:derived-identity'][name='name']/type", "example:derived-identity"), ++ CREATED("/example:list-with-union-keys[type='example:derived-identity'][name='name']/name", "name"), ++ CREATED("/example:list-with-union-keys[type='example:derived-identity'][name='name']/text", "blabla")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); + + // prefixed in the URI, not prefixed in the data + EXPECT_CHANGE( +- MODIFIED("/example:list-with-identity-key[type='example:derived-identity'][name='name']/text", "hehe")); +- REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example%3Aderived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "derived-identity", "text": "hehe"}]}]})") == Response{204, noContentTypeHeaders, ""}); ++ MODIFIED("/example:list-with-union-keys[type='example:derived-identity'][name='name']/text", "hehe")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example%3Aderived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "derived-identity", "text": "hehe"}]}]})") == Response{204, noContentTypeHeaders, ""}); + ++ // 'another-derived-identity' comes from a different module than the list itself, so this parses as string ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-union-keys[type='another-derived-identity'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='another-derived-identity'][name='name']/type", "another-derived-identity"), ++ CREATED("/example:list-with-union-keys[type='another-derived-identity'][name='name']/name", "name"), ++ CREATED("/example:list-with-union-keys[type='another-derived-identity'][name='name']/text", "blabla")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=another-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-union-keys[type='example-types:another-derived-identity'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='example-types:another-derived-identity'][name='name']/type", "example-types:another-derived-identity"), ++ CREATED("/example:list-with-union-keys[type='example-types:another-derived-identity'][name='name']/name", "name"), ++ CREATED("/example:list-with-union-keys[type='example-types:another-derived-identity'][name='name']/text", "blabla")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "example-types:another-derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); + +- REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=another-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{400, jsonHeaders, R"({ ++ // missing namespace in the data ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{400, jsonHeaders, R"({ + "ietf-restconf:errors": { + "error": [ + { + "error-type": "protocol", + "error-tag": "invalid-value", +- "error-message": "Validation failure: Can't parse data: LY_EVALID" ++ "error-path": "/example:list-with-union-keys[type='another-derived-identity'][name='name']/type", ++ "error-message": "List key mismatch between URI path ('example-types:another-derived-identity') and data ('another-derived-identity')." + } + ] + } + } + )"}); + ++ // zero is enum value + EXPECT_CHANGE( +- CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']", std::nullopt), +- CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/type", "example-types:another-derived-identity"), +- CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/name", "name"), +- CREATED("/example:list-with-identity-key[type='example-types:another-derived-identity'][name='name']/text", "blabla")); +- REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "example-types:another-derived-identity", "text": "blabla"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ CREATED("/example:list-with-union-keys[type='zero'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='zero'][name='name']/type", "zero"), ++ CREATED("/example:list-with-union-keys[type='zero'][name='name']/name", "name")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=zero,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "zero"}]}]})") == Response{201, noContentTypeHeaders, ""}); + +- // missing namespace in the data +- REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-identity-key=example-types%3Aanother-derived-identity,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-identity-key": [{"name": "name", "type": "another-derived-identity", "text": "blabla"}]}]})") == Response{400, jsonHeaders, R"({ ++ // example:zero is string, enum values are not namespace-prefixed ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-union-keys[type='example:zero'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='example:zero'][name='name']/type", "example:zero"), ++ CREATED("/example:list-with-union-keys[type='example:zero'][name='name']/name", "name")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example%3Azero,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "example:zero"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=zero,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:list-with-union-keys": [{"name": "name", "type": "example:zero"}]}]})") == Response{400, jsonHeaders, R"({ + "ietf-restconf:errors": { + "error": [ + { + "error-type": "protocol", + "error-tag": "invalid-value", +- "error-message": "Validation failure: Can't parse data: LY_EVALID" ++ "error-path": "/example:list-with-union-keys[type='example:zero'][name='name']/type", ++ "error-message": "List key mismatch between URI path ('zero') and data ('example:zero')." + } + ] + } +@@ -459,6 +482,17 @@ TEST_CASE("writing data") + } + )"}); + ++ EXPECT_CHANGE(CREATED("/example:fruit-list[.='example:apple']", "example:apple")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:fruit-list=example%3Aapple", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:fruit-list": ["apple"]})") == Response{201, noContentTypeHeaders, ""}); ++ ++ // leafref ++ EXPECT_CHANGE( ++ CREATED("/example:list-with-union-keys[type='example:apple'][name='name']", std::nullopt), ++ CREATED("/example:list-with-union-keys[type='example:apple'][name='name']/type", "example:apple"), ++ CREATED("/example:list-with-union-keys[type='example:apple'][name='name']/name", "name")); ++ REQUIRE(put(RESTCONF_DATA_ROOT "/example:list-with-union-keys=example%3Aapple,name", {AUTH_ROOT, CONTENT_TYPE_JSON}, ++ R"({"example:list-with-union-keys": [{"name": "name", "type": "apple"}]}]})") == Response{201, noContentTypeHeaders, ""}); ++ + // value in the URI and in the data have the same canonical form + EXPECT_CHANGE(CREATED("/example:tlc/decimal-list[.='1.0']", "1.0")); + REQUIRE(put(RESTCONF_DATA_ROOT "/example:tlc/decimal-list=1.00", {AUTH_ROOT, CONTENT_TYPE_JSON}, R"({"example:decimal-list": ["1.0"]})") == Response{201, noContentTypeHeaders, ""}); +diff --git a/tests/uri-parser.cpp b/tests/uri-parser.cpp +index 7320c3c..000fe5d 100644 +--- a/tests/uri-parser.cpp ++++ b/tests/uri-parser.cpp +@@ -293,7 +293,7 @@ TEST_CASE("URI path parser") + {"/restconf/data/example:tlc/list=eth0/choice1", "/example:tlc/list[name='eth0']/choice1", std::nullopt}, + {"/restconf/data/example:tlc/list=eth0/choice2", "/example:tlc/list[name='eth0']/choice2", std::nullopt}, + {"/restconf/data/example:tlc/list=eth0/collection=val", "/example:tlc/list[name='eth0']/collection[.='val']", std::nullopt}, +- {"/restconf/data/example:list-with-identity-key=example-types%3Aanother-derived-identity,aaa", "/example:list-with-identity-key[type='example-types:another-derived-identity'][name='aaa']", std::nullopt}, ++ {"/restconf/data/example:list-with-union-keys=example-types%3Aanother-derived-identity,aaa", "/example:list-with-union-keys[type='example-types:another-derived-identity'][name='aaa']", std::nullopt}, + {"/restconf/data/example:tlc/status", "/example:tlc/status", std::nullopt}, + // container example:a has a container b inserted locally and also via an augment. Check that we return the correct one + {"/restconf/data/example:a/b", "/example:a/b", std::nullopt}, +diff --git a/tests/yang/example.yang b/tests/yang/example.yang +index c46273c..75cd7a6 100644 +--- a/tests/yang/example.yang ++++ b/tests/yang/example.yang +@@ -13,6 +13,11 @@ module example { + base base-identity; + } + ++ identity fruit { } ++ identity apple { ++ base fruit; ++ } ++ + leaf top-level-leaf { type string; } + leaf top-level-leaf2 { type string; default "x"; } + +@@ -121,10 +126,23 @@ module example { + } + } + +- list list-with-identity-key { ++ list list-with-union-keys { + key "type name"; + leaf type { +- type identityref { base base-identity; } ++ type union { ++ type identityref { ++ base base-identity; ++ } ++ type enumeration { ++ enum zero; ++ enum one; ++ } ++ type leafref { ++ require-instance true; ++ path "/fruit-list"; ++ } ++ type string; ++ } + } + leaf name { type string; } + leaf text { type string; } +@@ -134,6 +152,10 @@ module example { + type identityref { base base-identity; } + } + ++ leaf-list fruit-list { ++ type identityref { base fruit; } ++ } ++ + rpc test-rpc { + input { + leaf i { +-- +2.43.0 +