From 39ff85b7afc0fa672fdab17be11102d897b7376a Mon Sep 17 00:00:00 2001 From: natenichols Date: Mon, 13 Dec 2021 16:39:45 -0600 Subject: [PATCH 1/5] Enforce account RPC limits by account objects traversed --- src/ripple/rpc/handlers/AccountChannels.cpp | 85 ++++++++++++------- src/ripple/rpc/handlers/AccountLines.cpp | 91 ++++++++++++--------- src/ripple/rpc/handlers/AccountOffers.cpp | 87 +++++++++++++------- src/ripple/rpc/impl/RPCHelpers.cpp | 70 +++++++++++++--- src/ripple/rpc/impl/RPCHelpers.h | 19 +++++ src/ripple/rpc/impl/Tuning.h | 2 +- src/test/app/PayChan_test.cpp | 3 +- src/test/rpc/AccountLinesRPC_test.cpp | 18 ++-- src/test/rpc/AccountOffers_test.cpp | 18 +++- 9 files changed, 270 insertions(+), 123 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountChannels.cpp b/src/ripple/rpc/handlers/AccountChannels.cpp index d222e4c72cd..a2b3f7b0990 100644 --- a/src/ripple/rpc/handlers/AccountChannels.cpp +++ b/src/ripple/rpc/handlers/AccountChannels.cpp @@ -100,6 +100,9 @@ doAccountChannels(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountChannels, context)) return *err; + + if (limit == 0u) + return rpcError(rpcINVALID_PARAMS); Json::Value jsonChannels{Json::arrayValue}; struct VisitData @@ -110,71 +113,93 @@ doAccountChannels(RPC::JsonContext& context) AccountID const& raDstAccount; }; VisitData visitData = {{}, accountID, hasDst, raDstAccount}; - visitData.items.reserve(limit + 1); - uint256 startAfter; - std::uint64_t startHint; + visitData.items.reserve(limit); + uint256 startAfter = beast::zero; + std::uint64_t startHint = 0; if (params.isMember(jss::marker)) { - Json::Value const& marker(params[jss::marker]); - - if (!marker.isString()) + if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); - - if (!startAfter.parseHex(marker.asString())) - { + + // Marker is composed of a comma separated index and start hint. The + // former will be read as hex, and the latter using boost lexical cast. + std::stringstream ss(params[jss::marker].asString()); + std::string s; + if (!std::getline(ss, s, ',')) return rpcError(rpcINVALID_PARAMS); - } - auto const sleChannel = ledger->read({ltPAYCHAN, startAfter}); + if (!startAfter.parseHex(s)) + return rpcError(rpcINVALID_PARAMS); - if (!sleChannel) + if (!std::getline(ss, s, ',')) return rpcError(rpcINVALID_PARAMS); - if (!visitData.hasDst || - visitData.raDstAccount == (*sleChannel)[sfDestination]) + try { - visitData.items.emplace_back(sleChannel); - startHint = sleChannel->getFieldU64(sfOwnerNode); + startHint = boost::lexical_cast(s); } - else + catch (boost::bad_lexical_cast&) { return rpcError(rpcINVALID_PARAMS); } - } - else - { - startHint = 0; + + // We then must check if the object pointed to by the marker is actually + // owned by the account in the request. + auto const sle = ledger->read({ltANY, startAfter}); + + if (!sle) + return rpcError(rpcINVALID_PARAMS); + + auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); + + if (!isOwned) + return rpcError(rpcINVALID_PARAMS); } + auto ct = 0; + std::optional marker = {}; + std::uint64_t nextHint = 0; if (!forEachItemAfter( *ledger, accountID, startAfter, startHint, - limit - visitData.items.size() + 1, - [&visitData, &accountID](std::shared_ptr const& sleCur) { - if (sleCur && sleCur->getType() == ltPAYCHAN && + limit + 1, + [&visitData, &accountID, &ct, &limit, &marker, &nextHint] + (std::shared_ptr const& sleCur) { + if (!sleCur) + { + assert(false); + return false; + } + + if (++ct == limit) + { + marker = sleCur->key(); + nextHint = RPC::getStartHint(sleCur, visitData.accountID); + } + + if (ct <= limit && + sleCur->getType() == ltPAYCHAN && (*sleCur)[sfAccount] == accountID && (!visitData.hasDst || visitData.raDstAccount == (*sleCur)[sfDestination])) { visitData.items.emplace_back(sleCur); - return true; } - return false; + return true; })) { return rpcError(rpcINVALID_PARAMS); } - if (visitData.items.size() == limit + 1) + if (ct == limit + 1 && marker) { result[jss::limit] = limit; - - result[jss::marker] = to_string(visitData.items.back()->key()); - visitData.items.pop_back(); + result[jss::marker] = + to_string(*marker) + "," + std::to_string(nextHint); } result[jss::account] = context.app.accountIDCache().toBase58(accountID); diff --git a/src/ripple/rpc/handlers/AccountLines.cpp b/src/ripple/rpc/handlers/AccountLines.cpp index 2e1dac1d71d..d273ed9e142 100644 --- a/src/ripple/rpc/handlers/AccountLines.cpp +++ b/src/ripple/rpc/handlers/AccountLines.cpp @@ -123,84 +123,99 @@ doAccountLines(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountLines, context)) return *err; + + if (limit == 0) + return rpcError(rpcINVALID_PARAMS); Json::Value& jsonLines(result[jss::lines] = Json::arrayValue); VisitData visitData = {{}, accountID, hasPeer, raPeerAccount}; - unsigned int reserve(limit); - uint256 startAfter; - std::uint64_t startHint; + uint256 startAfter = beast::zero; + std::uint64_t startHint = 0; if (params.isMember(jss::marker)) { - // We have a start point. Use limit - 1 from the result and use the - // very last one for the resume. - Json::Value const& marker(params[jss::marker]); - - if (!marker.isString()) + if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); - - if (!startAfter.parseHex(marker.asString())) + + // Marker is composed of a comma separated index and start hint. The + // former will be read as hex, and the latter using boost lexical cast. + std::stringstream ss(params[jss::marker].asString()); + std::string s; + if (!std::getline(ss, s, ',')) return rpcError(rpcINVALID_PARAMS); - auto const sleLine = ledger->read({ltRIPPLE_STATE, startAfter}); + if (!startAfter.parseHex(s)) + return rpcError(rpcINVALID_PARAMS); - if (!sleLine) + if (!std::getline(ss, s, ',')) return rpcError(rpcINVALID_PARAMS); - if (sleLine->getFieldAmount(sfLowLimit).getIssuer() == accountID) - startHint = sleLine->getFieldU64(sfLowNode); - else if (sleLine->getFieldAmount(sfHighLimit).getIssuer() == accountID) - startHint = sleLine->getFieldU64(sfHighNode); - else + try + { + startHint = boost::lexical_cast(s); + } + catch (boost::bad_lexical_cast&) + { return rpcError(rpcINVALID_PARAMS); + } - // Caller provided the first line (startAfter), add it as first result - auto const line = RippleState::makeItem(accountID, sleLine); - if (line == nullptr) + // We then must check if the object pointed to by the marker is actually + // owned by the account in the request. + auto const sle = ledger->read({ltANY, startAfter}); + + if (!sle) return rpcError(rpcINVALID_PARAMS); - addLine(jsonLines, *line); - visitData.items.reserve(reserve); - } - else - { - startHint = 0; - // We have no start point, limit should be one higher than requested. - visitData.items.reserve(++reserve); + auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); + + if (!isOwned) + return rpcError(rpcINVALID_PARAMS); } + auto ct = 0; + std::optional marker = {}; + std::uint64_t nextHint = 0; { if (!forEachItemAfter( *ledger, accountID, startAfter, startHint, - reserve, - [&visitData](std::shared_ptr const& sleCur) { + limit + 1, + [&visitData, &ct, &marker, &limit, &nextHint](std::shared_ptr const& sleCur) { + if (!sleCur) + { + assert(false); + return false; + } + + if (++ct == limit) + { + marker = sleCur->key(); + nextHint = RPC::getStartHint(sleCur, visitData.accountID); + } + auto const line = RippleState::makeItem(visitData.accountID, sleCur); - if (line != nullptr && + if (ct <= limit && line != nullptr && (!visitData.hasPeer || visitData.raPeerAccount == line->getAccountIDPeer())) { visitData.items.emplace_back(line); - return true; } - return false; + return true; })) { return rpcError(rpcINVALID_PARAMS); } } - if (visitData.items.size() == reserve) + if (ct == limit + 1 && marker) { result[jss::limit] = limit; - - RippleState::pointer line(visitData.items.back()); - result[jss::marker] = to_string(line->key()); - visitData.items.pop_back(); + result[jss::marker] = + to_string(*marker) + "," + std::to_string(nextHint); } result[jss::account] = context.app.accountIDCache().toBase58(accountID); diff --git a/src/ripple/rpc/handlers/AccountOffers.cpp b/src/ripple/rpc/handlers/AccountOffers.cpp index 063d0874378..8dde179f147 100644 --- a/src/ripple/rpc/handlers/AccountOffers.cpp +++ b/src/ripple/rpc/handlers/AccountOffers.cpp @@ -85,69 +85,94 @@ doAccountOffers(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountOffers, context)) return *err; + + if (limit == 0) + return rpcError(rpcINVALID_PARAMS); Json::Value& jsonOffers(result[jss::offers] = Json::arrayValue); std::vector> offers; - unsigned int reserve(limit); - uint256 startAfter; - std::uint64_t startHint; + uint256 startAfter = beast::zero; + std::uint64_t startHint = 0; if (params.isMember(jss::marker)) { - // We have a start point. Use limit - 1 from the result and use the - // very last one for the resume. - Json::Value const& marker(params[jss::marker]); - - if (!marker.isString()) + if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); + + // Marker is composed of a comma separated index and start hint. The + // former will be read as hex, and the latter using boost lexical cast. + std::stringstream ss(params[jss::marker].asString()); + std::string s; + if (!std::getline(ss, s, ',')) + return rpcError(rpcINVALID_PARAMS); - if (!startAfter.parseHex(marker.asString())) + if (!startAfter.parseHex(s)) return rpcError(rpcINVALID_PARAMS); - auto const sleOffer = ledger->read({ltOFFER, startAfter}); + if (!std::getline(ss, s, ',')) + return rpcError(rpcINVALID_PARAMS); - if (!sleOffer || accountID != sleOffer->getAccountID(sfAccount)) + try + { + startHint = boost::lexical_cast(s); + } + catch (boost::bad_lexical_cast&) { return rpcError(rpcINVALID_PARAMS); } - startHint = sleOffer->getFieldU64(sfOwnerNode); - // Caller provided the first offer (startAfter), add it as first result - appendOfferJson(sleOffer, jsonOffers); - offers.reserve(reserve); - } - else - { - startHint = 0; - // We have no start point, limit should be one higher than requested. - offers.reserve(++reserve); + // We then must check if the object pointed to by the marker is actually + // owned by the account in the request. + auto const sle = ledger->read({ltANY, startAfter}); + + if (!sle) + return rpcError(rpcINVALID_PARAMS); + + auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); + + if (!isOwned) + return rpcError(rpcINVALID_PARAMS); } + auto ct = 0; + std::optional marker = {}; + std::uint64_t nextHint = 0; if (!forEachItemAfter( *ledger, accountID, startAfter, startHint, - reserve, - [&offers](std::shared_ptr const& offer) { - if (offer->getType() == ltOFFER) + limit + 1, + [&offers, &ct, &marker, &limit, &nextHint, &accountID] + (std::shared_ptr const& sle) { + if (!sle) + { + assert(false); + return false; + } + + if (++ct == limit) { - offers.emplace_back(offer); - return true; + marker = sle->key(); + nextHint = RPC::getStartHint(sle, accountID); } - return false; + if (ct <= limit && sle->getType() == ltOFFER) + { + offers.emplace_back(sle); + } + + return true; })) { return rpcError(rpcINVALID_PARAMS); } - if (offers.size() == reserve) + if (ct == limit + 1 && marker) { result[jss::limit] = limit; - - result[jss::marker] = to_string(offers.back()->key()); - offers.pop_back(); + result[jss::marker] = + to_string(*marker) + "," + std::to_string(nextHint); } for (auto const& offer : offers) diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp index be1d005a38a..e75a55d4d88 100644 --- a/src/ripple/rpc/impl/RPCHelpers.cpp +++ b/src/ripple/rpc/impl/RPCHelpers.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -89,6 +90,55 @@ accountFromString(AccountID& result, std::string const& strIdent, bool bStrict) return Json::objectValue; } +std::uint64_t +getStartHint(std::shared_ptr const& sle, AccountID const& accountID) +{ + if (sle->getType() == ltRIPPLE_STATE) + { + if (sle->getFieldAmount(sfLowLimit).getIssuer() == accountID) + return sle->getFieldU64(sfLowNode); + else if (sle->getFieldAmount(sfHighLimit).getIssuer() == accountID) + return sle->getFieldU64(sfHighNode); + } + + return sle->getFieldU64(sfOwnerNode); +} + +bool +isOwnedByAccount( + ReadView const& ledger, + std::shared_ptr const& sle, + AccountID const& accountID) +{ + if (sle->getType() == ltRIPPLE_STATE) + { + return (sle->getFieldAmount(sfLowLimit).getIssuer() == accountID) + || (sle->getFieldAmount(sfHighLimit).getIssuer() == accountID); + } + else if (sle->isFieldPresent(sfAccount)) + { + return sle->getAccountID(sfAccount) == accountID; + } + else if (sle->getType() == ltSIGNER_LIST) + { + auto hint = sle->getFieldU64(sfOwnerNode); + auto const rootIndex = keylet::ownerDir(accountID); + auto const pageIndex = keylet::page(rootIndex, hint); + + auto directory = ledger.read({ltDIR_NODE, pageIndex.key}); + + if (!directory) + return false; + + auto const& hashes = directory->getFieldV256(sfIndexes); + auto iter = std::find(hashes.begin(), hashes.end(), sle->key()); + + return iter != hashes.end(); + } + + return false; +} + bool getAccountObjects( ReadView const& ledger, @@ -144,19 +194,19 @@ getAccountObjects( typeMatchesFilter(typeFilter.value(), sleNode->getType())) { jvObjects.append(sleNode->getJson(JsonOptions::none)); + } - if (++i == limit) + if (++i == limit) + { + if (++iter != entries.end()) { - if (++iter != entries.end()) - { - jvResult[jss::limit] = limit; - jvResult[jss::marker] = - to_string(dirIndex) + ',' + to_string(*iter); - return true; - } - - break; + jvResult[jss::limit] = limit; + jvResult[jss::marker] = + to_string(dirIndex) + ',' + to_string(*iter); + return true; } + + break; } } diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index 0ecdebe24a8..5d744bad9ed 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -71,6 +71,25 @@ accountFromStringWithCode( std::string const& strIdent, bool bStrict = false); +/** Gets the start hint for traversing account objects + * @param sle - Ledger entry defined by the marker passed into the RPC. + * @param accountID - The ID of the account whose objects you are traversing. + */ +std::uint64_t +getStartHint(std::shared_ptr const& sle, AccountID const& accountID); + +/** + * Tests if a SLE is owned by accountID. + * @param ledger - The ledger used to search for the sle. + * @param sle - The SLE to test for ownership. + * @param account - The account that supposedly owns the SLE. + */ +bool +isOwnedByAccount( + ReadView const& ledger, + std::shared_ptr const& sle, + AccountID const& accountID); + /** Gathers all objects for an account in a ledger. @param ledger Ledger to search account objects. @param account AccountID to find objects for. diff --git a/src/ripple/rpc/impl/Tuning.h b/src/ripple/rpc/impl/Tuning.h index f52c60f096d..233e7379489 100644 --- a/src/ripple/rpc/impl/Tuning.h +++ b/src/ripple/rpc/impl/Tuning.h @@ -46,7 +46,7 @@ static LimitRange constexpr accountObjects = {10, 200, 400}; static LimitRange constexpr accountOffers = {10, 200, 400}; /** Limits for the book_offers command. */ -static LimitRange constexpr bookOffers = {0, 300, 400}; +static LimitRange constexpr bookOffers = {0, 60, 100}; /** Limits for the no_ripple_check command. */ static LimitRange constexpr noRippleCheck = {10, 300, 400}; diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 0e38de517de..cf600a9fc87 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1033,8 +1033,7 @@ struct PayChan_test : public beast::unit_test::suite { // degenerate case auto const r = testLimit(env, alice, 0); - BEAST_EXPECT(r.isMember(jss::marker)); - BEAST_EXPECT(r[jss::channels].size() == 0); + BEAST_EXPECT(r.isMember(jss::error_message)); } } diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp index e7dbe55933d..6834e9456c7 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLinesRPC_test.cpp @@ -320,7 +320,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite "account_lines", R"({"account": ")" + alice.human() + R"(", )" - R"("limit": 1, )" + R"("limit": 10, )" R"("peer": ")" + gw2.human() + R"("})"); auto const& line = lines[jss::result][jss::lines][0u]; @@ -390,8 +390,10 @@ class AccountLinesRPC_test : public beast::unit_test::suite env.close(); auto const USD = gw1["USD"]; + auto const AUD = gw1["AUD"]; auto const EUR = gw2["EUR"]; env(trust(alice, USD(200))); + env(trust(alice, AUD(200))); env(trust(becky, EUR(200))); env(trust(cheri, EUR(200))); env.close(); @@ -414,9 +416,9 @@ class AccountLinesRPC_test : public beast::unit_test::suite "account_lines", R"({"account": ")" + alice.human() + R"(", )" - R"("limit": 1})"); + R"("limit": 2})"); BEAST_EXPECT( - linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD"); + linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD"); BEAST_EXPECT(linesBeg[jss::result].isMember(jss::marker)); // alice pays 100 EUR to cheri. @@ -433,7 +435,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite R"("marker": ")" + linesBeg[jss::result][jss::marker].asString() + R"("})"); BEAST_EXPECT( - linesEnd[jss::result][jss::error_message] == + linesEnd[jss::result][jss::error_message] == RPC::make_error(rpcINVALID_PARAMS)[jss::error_message]); } @@ -966,7 +968,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite R"({"account": ")" + alice.human() + R"(", )" - R"("limit": 1, )" + R"("limit": 10, )" R"("peer": ")" + gw2.human() + R"("}})"); auto const& line = lines[jss::result][jss::lines][0u]; @@ -1068,8 +1070,10 @@ class AccountLinesRPC_test : public beast::unit_test::suite env.close(); auto const USD = gw1["USD"]; - auto const EUR = gw2["EUR"]; + auto const AUD = gw1["AUD"]; + auto const EUR = gw2["EUR"]; env(trust(alice, USD(200))); + env(trust(alice, AUD(200))); env(trust(becky, EUR(200))); env(trust(cheri, EUR(200))); env.close(); @@ -1098,7 +1102,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite R"({"account": ")" + alice.human() + R"(", )" - R"("limit": 1}})"); + R"("limit": 2}})"); BEAST_EXPECT( linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD"); BEAST_EXPECT(linesBeg[jss::result].isMember(jss::marker)); diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index 8871315aa79..1b80ee0000c 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -81,7 +81,8 @@ class AccountOffers_test : public beast::unit_test::suite "json", "account_offers", jvParams.toStyledString())[jss::result]; auto const& jro_l = jrr_l[jss::offers]; BEAST_EXPECT(checkMarker(jrr_l)); - BEAST_EXPECT(checkArraySize(jro_l, 10u)); + // 9u is the expected size, since one account object is a trustline + BEAST_EXPECT(checkArraySize(jro_l, 9u)); } void @@ -173,6 +174,7 @@ class AccountOffers_test : public beast::unit_test::suite // last item...with previous marker passed jvParams[jss::marker] = jrr_l_2[jss::marker]; + jvParams[jss::limit] = 10u; auto const jrr_l_3 = env.rpc( "json", "account_offers", @@ -203,9 +205,17 @@ class AccountOffers_test : public beast::unit_test::suite "account_offers", jvParams.toStyledString())[jss::result]; auto const& jro = jrr[jss::offers]; - BEAST_EXPECT(checkArraySize(jro, asAdmin ? 0u : 3u)); - BEAST_EXPECT( - asAdmin ? checkMarker(jrr) : (!jrr.isMember(jss::marker))); + if (asAdmin) + { + // limit == 0 is invalid + BEAST_EXPECT(jrr.isMember(jss::error_message)); + } + else + { + // Call should enforce min limit of 10 + BEAST_EXPECT(checkArraySize(jro, 3u)); + BEAST_EXPECT(!jrr.isMember(jss::marker)); + } } } From 566d6e76e133456d054bd85f453e300678e82fc1 Mon Sep 17 00:00:00 2001 From: natenichols Date: Tue, 14 Dec 2021 17:01:36 -0600 Subject: [PATCH 2/5] [FOLD] Formatting --- src/ripple/rpc/handlers/AccountChannels.cpp | 15 +++++++-------- src/ripple/rpc/handlers/AccountLines.cpp | 16 +++++++++------- src/ripple/rpc/handlers/AccountOffers.cpp | 14 +++++++------- src/ripple/rpc/impl/RPCHelpers.cpp | 8 ++++---- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountChannels.cpp b/src/ripple/rpc/handlers/AccountChannels.cpp index a2b3f7b0990..d02e12bee4c 100644 --- a/src/ripple/rpc/handlers/AccountChannels.cpp +++ b/src/ripple/rpc/handlers/AccountChannels.cpp @@ -100,9 +100,9 @@ doAccountChannels(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountChannels, context)) return *err; - + if (limit == 0u) - return rpcError(rpcINVALID_PARAMS); + return rpcError(rpcINVALID_PARAMS); Json::Value jsonChannels{Json::arrayValue}; struct VisitData @@ -121,8 +121,8 @@ doAccountChannels(RPC::JsonContext& context) { if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); - - // Marker is composed of a comma separated index and start hint. The + + // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. std::stringstream ss(params[jss::marker].asString()); std::string s; @@ -166,8 +166,8 @@ doAccountChannels(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&visitData, &accountID, &ct, &limit, &marker, &nextHint] - (std::shared_ptr const& sleCur) { + [&visitData, &accountID, &ct, &limit, &marker, &nextHint]( + std::shared_ptr const& sleCur) { if (!sleCur) { assert(false); @@ -180,8 +180,7 @@ doAccountChannels(RPC::JsonContext& context) nextHint = RPC::getStartHint(sleCur, visitData.accountID); } - if (ct <= limit && - sleCur->getType() == ltPAYCHAN && + if (ct <= limit && sleCur->getType() == ltPAYCHAN && (*sleCur)[sfAccount] == accountID && (!visitData.hasDst || visitData.raDstAccount == (*sleCur)[sfDestination])) diff --git a/src/ripple/rpc/handlers/AccountLines.cpp b/src/ripple/rpc/handlers/AccountLines.cpp index d273ed9e142..74884b9e540 100644 --- a/src/ripple/rpc/handlers/AccountLines.cpp +++ b/src/ripple/rpc/handlers/AccountLines.cpp @@ -123,7 +123,7 @@ doAccountLines(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountLines, context)) return *err; - + if (limit == 0) return rpcError(rpcINVALID_PARAMS); @@ -136,8 +136,8 @@ doAccountLines(RPC::JsonContext& context) { if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); - - // Marker is composed of a comma separated index and start hint. The + + // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. std::stringstream ss(params[jss::marker].asString()); std::string s; @@ -182,7 +182,8 @@ doAccountLines(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&visitData, &ct, &marker, &limit, &nextHint](std::shared_ptr const& sleCur) { + [&visitData, &ct, &marker, &limit, &nextHint]( + std::shared_ptr const& sleCur) { if (!sleCur) { assert(false); @@ -192,9 +193,10 @@ doAccountLines(RPC::JsonContext& context) if (++ct == limit) { marker = sleCur->key(); - nextHint = RPC::getStartHint(sleCur, visitData.accountID); + nextHint = + RPC::getStartHint(sleCur, visitData.accountID); } - + auto const line = RippleState::makeItem(visitData.accountID, sleCur); if (ct <= limit && line != nullptr && @@ -214,7 +216,7 @@ doAccountLines(RPC::JsonContext& context) if (ct == limit + 1 && marker) { result[jss::limit] = limit; - result[jss::marker] = + result[jss::marker] = to_string(*marker) + "," + std::to_string(nextHint); } diff --git a/src/ripple/rpc/handlers/AccountOffers.cpp b/src/ripple/rpc/handlers/AccountOffers.cpp index 8dde179f147..d0e8e324e07 100644 --- a/src/ripple/rpc/handlers/AccountOffers.cpp +++ b/src/ripple/rpc/handlers/AccountOffers.cpp @@ -85,9 +85,9 @@ doAccountOffers(RPC::JsonContext& context) unsigned int limit; if (auto err = readLimitField(limit, RPC::Tuning::accountOffers, context)) return *err; - + if (limit == 0) - return rpcError(rpcINVALID_PARAMS); + return rpcError(rpcINVALID_PARAMS); Json::Value& jsonOffers(result[jss::offers] = Json::arrayValue); std::vector> offers; @@ -98,8 +98,8 @@ doAccountOffers(RPC::JsonContext& context) { if (!params[jss::marker].isString()) return RPC::expected_field_error(jss::marker, "string"); - - // Marker is composed of a comma separated index and start hint. The + + // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. std::stringstream ss(params[jss::marker].asString()); std::string s; @@ -143,8 +143,8 @@ doAccountOffers(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&offers, &ct, &marker, &limit, &nextHint, &accountID] - (std::shared_ptr const& sle) { + [&offers, &ct, &marker, &limit, &nextHint, &accountID]( + std::shared_ptr const& sle) { if (!sle) { assert(false); @@ -171,7 +171,7 @@ doAccountOffers(RPC::JsonContext& context) if (ct == limit + 1 && marker) { result[jss::limit] = limit; - result[jss::marker] = + result[jss::marker] = to_string(*marker) + "," + std::to_string(nextHint); } diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp index e75a55d4d88..ef98cd06c35 100644 --- a/src/ripple/rpc/impl/RPCHelpers.cpp +++ b/src/ripple/rpc/impl/RPCHelpers.cpp @@ -20,8 +20,8 @@ #include #include #include -#include #include +#include #include #include #include @@ -100,7 +100,7 @@ getStartHint(std::shared_ptr const& sle, AccountID const& accountID) else if (sle->getFieldAmount(sfHighLimit).getIssuer() == accountID) return sle->getFieldU64(sfHighNode); } - + return sle->getFieldU64(sfOwnerNode); } @@ -112,8 +112,8 @@ isOwnedByAccount( { if (sle->getType() == ltRIPPLE_STATE) { - return (sle->getFieldAmount(sfLowLimit).getIssuer() == accountID) - || (sle->getFieldAmount(sfHighLimit).getIssuer() == accountID); + return (sle->getFieldAmount(sfLowLimit).getIssuer() == accountID) || + (sle->getFieldAmount(sfHighLimit).getIssuer() == accountID); } else if (sle->isFieldPresent(sfAccount)) { From a13f72481ed34b03e89334d4b26809bf61bed572 Mon Sep 17 00:00:00 2001 From: natenichols Date: Tue, 14 Dec 2021 17:03:51 -0600 Subject: [PATCH 3/5] [FOLD] format test --- src/test/rpc/AccountLinesRPC_test.cpp | 12 ++++++------ src/test/rpc/AccountOffers_test.cpp | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp index 6834e9456c7..2e25b90a5a6 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLinesRPC_test.cpp @@ -390,10 +390,10 @@ class AccountLinesRPC_test : public beast::unit_test::suite env.close(); auto const USD = gw1["USD"]; - auto const AUD = gw1["AUD"]; + auto const AUD = gw1["AUD"]; auto const EUR = gw2["EUR"]; env(trust(alice, USD(200))); - env(trust(alice, AUD(200))); + env(trust(alice, AUD(200))); env(trust(becky, EUR(200))); env(trust(cheri, EUR(200))); env.close(); @@ -418,7 +418,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite R"(", )" R"("limit": 2})"); BEAST_EXPECT( - linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD"); + linesBeg[jss::result][jss::lines][0u][jss::currency] == "USD"); BEAST_EXPECT(linesBeg[jss::result].isMember(jss::marker)); // alice pays 100 EUR to cheri. @@ -435,7 +435,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite R"("marker": ")" + linesBeg[jss::result][jss::marker].asString() + R"("})"); BEAST_EXPECT( - linesEnd[jss::result][jss::error_message] == + linesEnd[jss::result][jss::error_message] == RPC::make_error(rpcINVALID_PARAMS)[jss::error_message]); } @@ -1071,9 +1071,9 @@ class AccountLinesRPC_test : public beast::unit_test::suite auto const USD = gw1["USD"]; auto const AUD = gw1["AUD"]; - auto const EUR = gw2["EUR"]; + auto const EUR = gw2["EUR"]; env(trust(alice, USD(200))); - env(trust(alice, AUD(200))); + env(trust(alice, AUD(200))); env(trust(becky, EUR(200))); env(trust(cheri, EUR(200))); env.close(); diff --git a/src/test/rpc/AccountOffers_test.cpp b/src/test/rpc/AccountOffers_test.cpp index 1b80ee0000c..f4ad0a72595 100644 --- a/src/test/rpc/AccountOffers_test.cpp +++ b/src/test/rpc/AccountOffers_test.cpp @@ -81,7 +81,7 @@ class AccountOffers_test : public beast::unit_test::suite "json", "account_offers", jvParams.toStyledString())[jss::result]; auto const& jro_l = jrr_l[jss::offers]; BEAST_EXPECT(checkMarker(jrr_l)); - // 9u is the expected size, since one account object is a trustline + // 9u is the expected size, since one account object is a trustline BEAST_EXPECT(checkArraySize(jro_l, 9u)); } @@ -174,7 +174,7 @@ class AccountOffers_test : public beast::unit_test::suite // last item...with previous marker passed jvParams[jss::marker] = jrr_l_2[jss::marker]; - jvParams[jss::limit] = 10u; + jvParams[jss::limit] = 10u; auto const jrr_l_3 = env.rpc( "json", "account_offers", @@ -214,7 +214,7 @@ class AccountOffers_test : public beast::unit_test::suite { // Call should enforce min limit of 10 BEAST_EXPECT(checkArraySize(jro, 3u)); - BEAST_EXPECT(!jrr.isMember(jss::marker)); + BEAST_EXPECT(!jrr.isMember(jss::marker)); } } } From c51aea3f601da568aaf697f6884c140c9784f6ed Mon Sep 17 00:00:00 2001 From: natenichols Date: Wed, 15 Dec 2021 09:39:15 -0600 Subject: [PATCH 4/5] address comments --- src/ripple/rpc/handlers/AccountChannels.cpp | 29 ++++----- src/ripple/rpc/handlers/AccountLines.cpp | 44 +++++++------ src/ripple/rpc/handlers/AccountOffers.cpp | 29 ++++----- src/ripple/rpc/impl/RPCHelpers.cpp | 18 ++---- src/ripple/rpc/impl/RPCHelpers.h | 2 +- src/test/rpc/AccountLinesRPC_test.cpp | 69 +++++++++++++++++++++ 6 files changed, 130 insertions(+), 61 deletions(-) diff --git a/src/ripple/rpc/handlers/AccountChannels.cpp b/src/ripple/rpc/handlers/AccountChannels.cpp index d02e12bee4c..b2c0c9376b6 100644 --- a/src/ripple/rpc/handlers/AccountChannels.cpp +++ b/src/ripple/rpc/handlers/AccountChannels.cpp @@ -124,20 +124,20 @@ doAccountChannels(RPC::JsonContext& context) // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. - std::stringstream ss(params[jss::marker].asString()); - std::string s; - if (!std::getline(ss, s, ',')) + std::stringstream marker(params[jss::marker].asString()); + std::string value; + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); - if (!startAfter.parseHex(s)) + if (!startAfter.parseHex(value)) return rpcError(rpcINVALID_PARAMS); - if (!std::getline(ss, s, ',')) + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); try { - startHint = boost::lexical_cast(s); + startHint = boost::lexical_cast(value); } catch (boost::bad_lexical_cast&) { @@ -151,13 +151,11 @@ doAccountChannels(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); - - if (!isOwned) + if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } - auto ct = 0; + auto count = 0; std::optional marker = {}; std::uint64_t nextHint = 0; if (!forEachItemAfter( @@ -166,7 +164,7 @@ doAccountChannels(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&visitData, &accountID, &ct, &limit, &marker, &nextHint]( + [&visitData, &accountID, &count, &limit, &marker, &nextHint]( std::shared_ptr const& sleCur) { if (!sleCur) { @@ -174,13 +172,13 @@ doAccountChannels(RPC::JsonContext& context) return false; } - if (++ct == limit) + if (++count == limit) { marker = sleCur->key(); nextHint = RPC::getStartHint(sleCur, visitData.accountID); } - if (ct <= limit && sleCur->getType() == ltPAYCHAN && + if (count <= limit && sleCur->getType() == ltPAYCHAN && (*sleCur)[sfAccount] == accountID && (!visitData.hasDst || visitData.raDstAccount == (*sleCur)[sfDestination])) @@ -194,7 +192,10 @@ doAccountChannels(RPC::JsonContext& context) return rpcError(rpcINVALID_PARAMS); } - if (ct == limit + 1 && marker) + // Both conditions need to be checked because marker is set on the limit-th + // item, but if there is item on the limit + 1 iteration, then there is no + // need to return a marker. + if (count == limit + 1 && marker) { result[jss::limit] = limit; result[jss::marker] = diff --git a/src/ripple/rpc/handlers/AccountLines.cpp b/src/ripple/rpc/handlers/AccountLines.cpp index 74884b9e540..4f284ad4f39 100644 --- a/src/ripple/rpc/handlers/AccountLines.cpp +++ b/src/ripple/rpc/handlers/AccountLines.cpp @@ -139,20 +139,20 @@ doAccountLines(RPC::JsonContext& context) // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. - std::stringstream ss(params[jss::marker].asString()); - std::string s; - if (!std::getline(ss, s, ',')) + std::stringstream marker(params[jss::marker].asString()); + std::string value; + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); - if (!startAfter.parseHex(s)) + if (!startAfter.parseHex(value)) return rpcError(rpcINVALID_PARAMS); - if (!std::getline(ss, s, ',')) + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); try { - startHint = boost::lexical_cast(s); + startHint = boost::lexical_cast(value); } catch (boost::bad_lexical_cast&) { @@ -166,13 +166,11 @@ doAccountLines(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); - - if (!isOwned) + if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } - auto ct = 0; + auto count = 0; std::optional marker = {}; std::uint64_t nextHint = 0; { @@ -182,7 +180,7 @@ doAccountLines(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&visitData, &ct, &marker, &limit, &nextHint]( + [&visitData, &count, &marker, &limit, &nextHint]( std::shared_ptr const& sleCur) { if (!sleCur) { @@ -190,20 +188,25 @@ doAccountLines(RPC::JsonContext& context) return false; } - if (++ct == limit) + if (++count == limit) { marker = sleCur->key(); nextHint = RPC::getStartHint(sleCur, visitData.accountID); } - auto const line = - RippleState::makeItem(visitData.accountID, sleCur); - if (ct <= limit && line != nullptr && - (!visitData.hasPeer || - visitData.raPeerAccount == line->getAccountIDPeer())) + if (count <= limit) { - visitData.items.emplace_back(line); + auto const line = + RippleState::makeItem(visitData.accountID, sleCur); + + if (line != nullptr && + (!visitData.hasPeer || + visitData.raPeerAccount == + line->getAccountIDPeer())) + { + visitData.items.emplace_back(line); + } } return true; @@ -213,7 +216,10 @@ doAccountLines(RPC::JsonContext& context) } } - if (ct == limit + 1 && marker) + // Both conditions need to be checked because marker is set on the limit-th + // item, but if there is item on the limit + 1 iteration, then there is no + // need to return a marker. + if (count == limit + 1 && marker) { result[jss::limit] = limit; result[jss::marker] = diff --git a/src/ripple/rpc/handlers/AccountOffers.cpp b/src/ripple/rpc/handlers/AccountOffers.cpp index d0e8e324e07..e8b506f2c30 100644 --- a/src/ripple/rpc/handlers/AccountOffers.cpp +++ b/src/ripple/rpc/handlers/AccountOffers.cpp @@ -101,20 +101,20 @@ doAccountOffers(RPC::JsonContext& context) // Marker is composed of a comma separated index and start hint. The // former will be read as hex, and the latter using boost lexical cast. - std::stringstream ss(params[jss::marker].asString()); - std::string s; - if (!std::getline(ss, s, ',')) + std::stringstream marker(params[jss::marker].asString()); + std::string value; + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); - if (!startAfter.parseHex(s)) + if (!startAfter.parseHex(value)) return rpcError(rpcINVALID_PARAMS); - if (!std::getline(ss, s, ',')) + if (!std::getline(marker, value, ',')) return rpcError(rpcINVALID_PARAMS); try { - startHint = boost::lexical_cast(s); + startHint = boost::lexical_cast(value); } catch (boost::bad_lexical_cast&) { @@ -128,13 +128,11 @@ doAccountOffers(RPC::JsonContext& context) if (!sle) return rpcError(rpcINVALID_PARAMS); - auto isOwned = RPC::isOwnedByAccount(*ledger, sle, accountID); - - if (!isOwned) + if (!RPC::isOwnedByAccount(*ledger, sle, accountID)) return rpcError(rpcINVALID_PARAMS); } - auto ct = 0; + auto count = 0; std::optional marker = {}; std::uint64_t nextHint = 0; if (!forEachItemAfter( @@ -143,7 +141,7 @@ doAccountOffers(RPC::JsonContext& context) startAfter, startHint, limit + 1, - [&offers, &ct, &marker, &limit, &nextHint, &accountID]( + [&offers, &count, &marker, &limit, &nextHint, &accountID]( std::shared_ptr const& sle) { if (!sle) { @@ -151,13 +149,13 @@ doAccountOffers(RPC::JsonContext& context) return false; } - if (++ct == limit) + if (++count == limit) { marker = sle->key(); nextHint = RPC::getStartHint(sle, accountID); } - if (ct <= limit && sle->getType() == ltOFFER) + if (count <= limit && sle->getType() == ltOFFER) { offers.emplace_back(sle); } @@ -168,7 +166,10 @@ doAccountOffers(RPC::JsonContext& context) return rpcError(rpcINVALID_PARAMS); } - if (ct == limit + 1 && marker) + // Both conditions need to be checked because marker is set on the limit-th + // item, but if there is item on the limit + 1 iteration, then there is no + // need to return a marker. + if (count == limit + 1 && marker) { result[jss::limit] = limit; result[jss::marker] = diff --git a/src/ripple/rpc/impl/RPCHelpers.cpp b/src/ripple/rpc/impl/RPCHelpers.cpp index ef98cd06c35..5c42aae969b 100644 --- a/src/ripple/rpc/impl/RPCHelpers.cpp +++ b/src/ripple/rpc/impl/RPCHelpers.cpp @@ -101,6 +101,9 @@ getStartHint(std::shared_ptr const& sle, AccountID const& accountID) return sle->getFieldU64(sfHighNode); } + if (!sle->isFieldPresent(sfOwnerNode)) + return 0; + return sle->getFieldU64(sfOwnerNode); } @@ -121,19 +124,8 @@ isOwnedByAccount( } else if (sle->getType() == ltSIGNER_LIST) { - auto hint = sle->getFieldU64(sfOwnerNode); - auto const rootIndex = keylet::ownerDir(accountID); - auto const pageIndex = keylet::page(rootIndex, hint); - - auto directory = ledger.read({ltDIR_NODE, pageIndex.key}); - - if (!directory) - return false; - - auto const& hashes = directory->getFieldV256(sfIndexes); - auto iter = std::find(hashes.begin(), hashes.end(), sle->key()); - - return iter != hashes.end(); + Keylet const accountSignerList = keylet::signers(accountID); + return sle->key() == accountSignerList.key; } return false; diff --git a/src/ripple/rpc/impl/RPCHelpers.h b/src/ripple/rpc/impl/RPCHelpers.h index 5d744bad9ed..e3d44c2e730 100644 --- a/src/ripple/rpc/impl/RPCHelpers.h +++ b/src/ripple/rpc/impl/RPCHelpers.h @@ -82,7 +82,7 @@ getStartHint(std::shared_ptr const& sle, AccountID const& accountID); * Tests if a SLE is owned by accountID. * @param ledger - The ledger used to search for the sle. * @param sle - The SLE to test for ownership. - * @param account - The account that supposedly owns the SLE. + * @param account - The account being tested for SLE ownership. */ bool isOwnedByAccount( diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp index 2e25b90a5a6..92aca63929c 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLinesRPC_test.cpp @@ -363,6 +363,74 @@ class AccountLinesRPC_test : public beast::unit_test::suite } } + void + testAccountLinesMarker() + { + testcase("Entry pointed to by marker is not owned by account"); + using namespace test::jtx; + Env env(*this); + + // The goal of this test is observe account_lines RPC calls return an + // error message when the SLE pointed to by the marker is not owned by + // the Account being traversed. + // + // To start, we'll create an environment with some trust lines, offers + // and a signers list. + Account const alice{"alice"}; + Account const becky{"becky"}; + Account const cheri{"cheri"}; + Account const gw1{"gw1"}; + Account const gw2{"gw2"}; + env.fund(XRP(10000), alice, becky, cheri, gw1, gw2); + env.close(); + + auto const USD = gw1["USD"]; + auto const AUD = gw1["AUD"]; + auto const EUR = gw2["EUR"]; + env(trust(alice, USD(200))); + env(trust(alice, AUD(200))); + env(trust(becky, EUR(200))); + env(trust(cheri, EUR(200))); + env.close(); + + // Give alice a SignerList. + Account const bogie{"bogie"}; + + Json::Value const aliceSigners = signers(alice, 2, {{bogie, 3}}); + env(aliceSigners); + + // Get account_lines for alice. Limit at 1, so we get a marker pointing + // to a SignerList. + auto const aliceLines = env.rpc( + "json", + "account_lines", + R"({"account": ")" + alice.human() + + R"(", )" + R"("limit": 1})"); + BEAST_EXPECT(aliceLines[jss::result].isMember(jss::marker)); + + // When we fetch Alice's remaining lines, we should see only one more. + auto const aliceMarker = env.rpc( + "json", + "account_lines", + R"({"account": ")" + alice.human() + + R"(", )" + R"("marker": ")" + + aliceLines[jss::result][jss::marker].asString() + R"("})"); + BEAST_EXPECT(aliceMarker[jss::result][jss::lines].size() == 1); + + // Get account lines for beckys account, using alices SignerList as a + // marker. This should cause an error. + auto const beckyLines = env.rpc( + "json", + "account_lines", + R"({"account": ")" + becky.human() + + R"(", )" + R"("marker": ")" + + aliceLines[jss::result][jss::marker].asString() + R"("})"); + BEAST_EXPECT(beckyLines[jss::result].isMember(jss::error_message)); + } + void testAccountLineDelete() { @@ -1147,6 +1215,7 @@ class AccountLinesRPC_test : public beast::unit_test::suite run() override { testAccountLines(); + testAccountLinesMarker(); testAccountLineDelete(); testAccountLines2(); testAccountLineDelete2(); From 1a2b5158cdf311842e3cb1a80b6b3eb652afdf4a Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 15 Dec 2021 15:33:29 -0800 Subject: [PATCH 5/5] Verify testAccountLinesMarker() exercises the signer list case --- src/test/rpc/AccountLinesRPC_test.cpp | 79 ++++++++++++++++----------- 1 file changed, 48 insertions(+), 31 deletions(-) diff --git a/src/test/rpc/AccountLinesRPC_test.cpp b/src/test/rpc/AccountLinesRPC_test.cpp index 92aca63929c..bdd376b3aae 100644 --- a/src/test/rpc/AccountLinesRPC_test.cpp +++ b/src/test/rpc/AccountLinesRPC_test.cpp @@ -378,56 +378,73 @@ class AccountLinesRPC_test : public beast::unit_test::suite // and a signers list. Account const alice{"alice"}; Account const becky{"becky"}; - Account const cheri{"cheri"}; Account const gw1{"gw1"}; - Account const gw2{"gw2"}; - env.fund(XRP(10000), alice, becky, cheri, gw1, gw2); - env.close(); - - auto const USD = gw1["USD"]; - auto const AUD = gw1["AUD"]; - auto const EUR = gw2["EUR"]; - env(trust(alice, USD(200))); - env(trust(alice, AUD(200))); - env(trust(becky, EUR(200))); - env(trust(cheri, EUR(200))); + env.fund(XRP(10000), alice, becky, gw1); env.close(); // Give alice a SignerList. Account const bogie{"bogie"}; + env(signers(alice, 2, {{bogie, 3}})); + env.close(); - Json::Value const aliceSigners = signers(alice, 2, {{bogie, 3}}); - env(aliceSigners); + auto const EUR = gw1["EUR"]; + env(trust(alice, EUR(200))); + env(trust(becky, EUR(200))); + env.close(); - // Get account_lines for alice. Limit at 1, so we get a marker pointing - // to a SignerList. - auto const aliceLines = env.rpc( + // Get all account objects for alice and verify that her + // signerlist is first. This is only a (reliable) coincidence of + // object naming. So if any of alice's objects are renamed this + // may fail. + Json::Value const aliceObjects = env.rpc( "json", - "account_lines", + "account_objects", R"({"account": ")" + alice.human() + R"(", )" - R"("limit": 1})"); - BEAST_EXPECT(aliceLines[jss::result].isMember(jss::marker)); + R"("limit": 10})"); + Json::Value const& aliceSignerList = + aliceObjects[jss::result][jss::account_objects][0u]; + if (!(aliceSignerList[sfLedgerEntryType.jsonName] == jss::SignerList)) + { + fail( + "alice's account objects are misordered. " + "Please reorder the objects so the SignerList is first.", + __FILE__, + __LINE__); + return; + } - // When we fetch Alice's remaining lines, we should see only one more. - auto const aliceMarker = env.rpc( + // Get account_lines for alice. Limit at 1, so we get a marker + // pointing to her SignerList. + auto const aliceLines1 = env.rpc( "json", "account_lines", - R"({"account": ")" + alice.human() + - R"(", )" - R"("marker": ")" + - aliceLines[jss::result][jss::marker].asString() + R"("})"); - BEAST_EXPECT(aliceMarker[jss::result][jss::lines].size() == 1); + R"({"account": ")" + alice.human() + R"(", "limit": 1})"); + BEAST_EXPECT(aliceLines1[jss::result].isMember(jss::marker)); + + // Verify that the marker points at the signer list. + std::string const aliceMarker = + aliceLines1[jss::result][jss::marker].asString(); + std::string const markerIndex = + aliceMarker.substr(0, aliceMarker.find(',')); + BEAST_EXPECT(markerIndex == aliceSignerList[jss::index].asString()); + + // When we fetch Alice's remaining lines we should find one and no more. + auto const aliceLines2 = env.rpc( + "json", + "account_lines", + R"({"account": ")" + alice.human() + R"(", "marker": ")" + + aliceMarker + R"("})"); + BEAST_EXPECT(aliceLines2[jss::result][jss::lines].size() == 1); + BEAST_EXPECT(!aliceLines2[jss::result].isMember(jss::marker)); // Get account lines for beckys account, using alices SignerList as a // marker. This should cause an error. auto const beckyLines = env.rpc( "json", "account_lines", - R"({"account": ")" + becky.human() + - R"(", )" - R"("marker": ")" + - aliceLines[jss::result][jss::marker].asString() + R"("})"); + R"({"account": ")" + becky.human() + R"(", "marker": ")" + + aliceMarker + R"("})"); BEAST_EXPECT(beckyLines[jss::result].isMember(jss::error_message)); }