From 90db6bcbc1c9669adea5b18501c4767ac0a2eec3 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Mon, 23 Oct 2023 11:29:09 -0400 Subject: [PATCH] feat: add user version of `feature` RPC (#4781) * uses same formatting as admin RPC * hides potentially sensitive data --- API-CHANGELOG.md | 7 +- src/ripple/app/misc/AmendmentTable.h | 4 +- src/ripple/app/misc/impl/AmendmentTable.cpp | 22 +++-- src/ripple/rpc/handlers/Feature1.cpp | 11 ++- src/ripple/rpc/impl/Handler.cpp | 6 +- src/test/app/AmendmentTable_test.cpp | 20 ++++- src/test/rpc/Feature_test.cpp | 93 +++++++++++++++++++-- 7 files changed, 140 insertions(+), 23 deletions(-) diff --git a/API-CHANGELOG.md b/API-CHANGELOG.md index db158cf001c..58ceed0cde1 100644 --- a/API-CHANGELOG.md +++ b/API-CHANGELOG.md @@ -28,6 +28,12 @@ The `network_id` field was added in the `server_info` response in version 1.5.0 ## XRP Ledger server version 2.0.0 +### Additions in 2.2 + +Additions are intended to be non-breaking (because they are purely additive). + +- `feature`: A non-admin mode that uses the same formatting as admin RPC, but hides potentially-sensitive data. + ### Additions in 2.0 Additions are intended to be non-breaking (because they are purely additive). @@ -36,7 +42,6 @@ Additions are intended to be non-breaking (because they are purely additive). - In `Payment` transactions, `DeliverMax` has been added. This is a replacement for the `Amount` field, which should not be used. Typically, the `delivered_amount` (in transaction metadata) should be used. To ease the transition, `DeliverMax` is present regardless of API version, since adding a field is non-breaking. - API version 2 has been moved from beta to supported, meaning that it is generally available (regardless of the `beta_rpc_api` setting). - ## XRP Ledger server version 1.12.0 [Version 1.12.0](https://github.com/XRPLF/rippled/releases/tag/1.12.0) was released on Sep 6, 2023. diff --git a/src/ripple/app/misc/AmendmentTable.h b/src/ripple/app/misc/AmendmentTable.h index d7f7c8b26bb..fef13d50ddb 100644 --- a/src/ripple/app/misc/AmendmentTable.h +++ b/src/ripple/app/misc/AmendmentTable.h @@ -81,11 +81,11 @@ class AmendmentTable firstUnsupportedExpected() const = 0; virtual Json::Value - getJson() const = 0; + getJson(bool isAdmin) const = 0; /** Returns a Json::objectValue. */ virtual Json::Value - getJson(uint256 const& amendment) const = 0; + getJson(uint256 const& amendment, bool isAdmin) const = 0; /** Called when a new fully-validated ledger is accepted. */ void diff --git a/src/ripple/app/misc/impl/AmendmentTable.cpp b/src/ripple/app/misc/impl/AmendmentTable.cpp index 8f4f0321992..8f9556e0714 100644 --- a/src/ripple/app/misc/impl/AmendmentTable.cpp +++ b/src/ripple/app/misc/impl/AmendmentTable.cpp @@ -388,6 +388,7 @@ class AmendmentTableImpl final : public AmendmentTable Json::Value& v, uint256 const& amendment, AmendmentState const& state, + bool isAdmin, std::lock_guard const& lock) const; void @@ -428,9 +429,9 @@ class AmendmentTableImpl final : public AmendmentTable firstUnsupportedExpected() const override; Json::Value - getJson() const override; + getJson(bool isAdmin) const override; Json::Value - getJson(uint256 const&) const override; + getJson(uint256 const&, bool isAdmin) const override; bool needValidatedLedger(LedgerIndex seq) const override; @@ -906,13 +907,14 @@ AmendmentTableImpl::injectJson( Json::Value& v, const uint256& id, const AmendmentState& fs, + bool isAdmin, std::lock_guard const&) const { if (!fs.name.empty()) v[jss::name] = fs.name; v[jss::supported] = fs.supported; - if (!fs.enabled) + if (!fs.enabled && isAdmin) { if (fs.vote == AmendmentVote::obsolete) v[jss::vetoed] = "Obsolete"; @@ -921,7 +923,7 @@ AmendmentTableImpl::injectJson( } v[jss::enabled] = fs.enabled; - if (!fs.enabled && lastVote_) + if (!fs.enabled && lastVote_ && isAdmin) { auto const votesTotal = lastVote_->trustedValidations(); auto const votesNeeded = lastVote_->threshold(); @@ -936,7 +938,7 @@ AmendmentTableImpl::injectJson( } Json::Value -AmendmentTableImpl::getJson() const +AmendmentTableImpl::getJson(bool isAdmin) const { Json::Value ret(Json::objectValue); { @@ -947,6 +949,7 @@ AmendmentTableImpl::getJson() const ret[to_string(e.first)] = Json::objectValue, e.first, e.second, + isAdmin, lock); } } @@ -954,16 +957,19 @@ AmendmentTableImpl::getJson() const } Json::Value -AmendmentTableImpl::getJson(uint256 const& amendmentID) const +AmendmentTableImpl::getJson(uint256 const& amendmentID, bool isAdmin) const { Json::Value ret = Json::objectValue; - Json::Value& jAmendment = (ret[to_string(amendmentID)] = Json::objectValue); { std::lock_guard lock(mutex_); AmendmentState const* a = get(amendmentID, lock); if (a) - injectJson(jAmendment, amendmentID, *a, lock); + { + Json::Value& jAmendment = + (ret[to_string(amendmentID)] = Json::objectValue); + injectJson(jAmendment, amendmentID, *a, isAdmin, lock); + } } return ret; diff --git a/src/ripple/rpc/handlers/Feature1.cpp b/src/ripple/rpc/handlers/Feature1.cpp index 1858a062e53..131a1e5d073 100644 --- a/src/ripple/rpc/handlers/Feature1.cpp +++ b/src/ripple/rpc/handlers/Feature1.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -37,6 +38,7 @@ doFeature(RPC::JsonContext& context) if (context.app.config().reporting()) return rpcError(rpcREPORTING_UNSUPPORTED); + bool const isAdmin = context.role == Role::ADMIN; // Get majority amendment status majorityAmendments_t majorities; @@ -47,7 +49,7 @@ doFeature(RPC::JsonContext& context) if (!context.params.isMember(jss::feature)) { - auto features = table.getJson(); + auto features = table.getJson(isAdmin); for (auto const& [h, t] : majorities) { @@ -69,13 +71,18 @@ doFeature(RPC::JsonContext& context) if (context.params.isMember(jss::vetoed)) { + if (!isAdmin) + return rpcError(rpcNO_PERMISSION); + if (context.params[jss::vetoed].asBool()) table.veto(feature); else table.unVeto(feature); } - Json::Value jvReply = table.getJson(feature); + Json::Value jvReply = table.getJson(feature, isAdmin); + if (!jvReply) + return rpcError(rpcBAD_FEATURE); auto m = majorities.find(feature); if (m != majorities.end()) diff --git a/src/ripple/rpc/impl/Handler.cpp b/src/ripple/rpc/impl/Handler.cpp index b3f11a28040..1fc160dc4db 100644 --- a/src/ripple/rpc/impl/Handler.cpp +++ b/src/ripple/rpc/impl/Handler.cpp @@ -105,6 +105,9 @@ Handler const handlerArray[]{ Role::USER, NO_CONDITION}, {"download_shard", byRef(&doDownloadShard), Role::ADMIN, NO_CONDITION}, + {"feature", byRef(&doFeature), Role::USER, NO_CONDITION}, + {"fee", byRef(&doFee), Role::USER, NEEDS_CURRENT_LEDGER}, + {"fetch_info", byRef(&doFetchInfo), Role::ADMIN, NO_CONDITION}, #ifdef RIPPLED_REPORTING {"gateway_balances", byRef(&doGatewayBalances), Role::ADMIN, NO_CONDITION}, #else @@ -115,9 +118,6 @@ Handler const handlerArray[]{ byRef(&doGetAggregatePrice), Role::USER, NO_CONDITION}, - {"feature", byRef(&doFeature), Role::ADMIN, NO_CONDITION}, - {"fee", byRef(&doFee), Role::USER, NEEDS_CURRENT_LEDGER}, - {"fetch_info", byRef(&doFetchInfo), Role::ADMIN, NO_CONDITION}, {"ledger_accept", byRef(&doLedgerAccept), Role::ADMIN, diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index 64e6a038653..238e05ba523 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -288,7 +288,7 @@ class AmendmentTable_test final : public beast::unit_test::suite uint256 const unsupportedID = amendmentId(unsupported_[0]); { Json::Value const unsupp = - table->getJson(unsupportedID)[to_string(unsupportedID)]; + table->getJson(unsupportedID, true)[to_string(unsupportedID)]; BEAST_EXPECT(unsupp.size() == 0); } @@ -296,7 +296,7 @@ class AmendmentTable_test final : public beast::unit_test::suite table->veto(unsupportedID); { Json::Value const unsupp = - table->getJson(unsupportedID)[to_string(unsupportedID)]; + table->getJson(unsupportedID, true)[to_string(unsupportedID)]; BEAST_EXPECT(unsupp[jss::vetoed].asBool()); } } @@ -638,6 +638,22 @@ class AmendmentTable_test final : public beast::unit_test::suite BEAST_EXPECT(enabled.empty()); BEAST_EXPECT(majority.empty()); + uint256 const unsupportedID = amendmentId(unsupported_[0]); + { + Json::Value const unsupp = + table->getJson(unsupportedID, false)[to_string(unsupportedID)]; + BEAST_EXPECT(unsupp.size() == 0); + } + + table->veto(unsupportedID); + { + Json::Value const unsupp = + table->getJson(unsupportedID, false)[to_string(unsupportedID)]; + BEAST_EXPECT(!unsupp[jss::vetoed].asBool()); + } + + votes.emplace_back(testAmendment, validators.size()); + votes.emplace_back(testAmendment, validators.size()); doRound( diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index dcd95c8a968..fd63aee98e2 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -205,11 +205,94 @@ class Feature_test : public beast::unit_test::suite return cfg; })}; - auto jrr = env.rpc("feature")[jss::result]; - // The current HTTP/S ServerHandler returns an HTTP 403 error code here - // rather than a noPermission JSON error. The JSONRPCClient just eats - // that error and returns an null result. - BEAST_EXPECT(jrr.isNull()); + { + auto result = env.rpc("feature")[jss::result]; + BEAST_EXPECT(result.isMember(jss::features)); + // There should be at least 50 amendments. Don't do exact + // comparison to avoid maintenance as more amendments are added in + // the future. + BEAST_EXPECT(result[jss::features].size() >= 50); + for (auto it = result[jss::features].begin(); + it != result[jss::features].end(); + ++it) + { + uint256 id; + (void)id.parseHex(it.key().asString().c_str()); + if (!BEAST_EXPECT((*it).isMember(jss::name))) + return; + bool expectEnabled = + env.app().getAmendmentTable().isEnabled(id); + bool expectSupported = + env.app().getAmendmentTable().isSupported(id); + BEAST_EXPECTS( + (*it).isMember(jss::enabled) && + (*it)[jss::enabled].asBool() == expectEnabled, + (*it)[jss::name].asString() + " enabled"); + BEAST_EXPECTS( + (*it).isMember(jss::supported) && + (*it)[jss::supported].asBool() == expectSupported, + (*it)[jss::name].asString() + " supported"); + BEAST_EXPECT(!(*it).isMember(jss::vetoed)); + BEAST_EXPECT(!(*it).isMember(jss::majority)); + BEAST_EXPECT(!(*it).isMember(jss::count)); + BEAST_EXPECT(!(*it).isMember(jss::validations)); + BEAST_EXPECT(!(*it).isMember(jss::threshold)); + } + } + + { + Json::Value params; + // invalid feature + params[jss::feature] = + "1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCD" + "EF"; + auto const result = env.rpc( + "json", + "feature", + boost::lexical_cast(params))[jss::result]; + BEAST_EXPECTS( + result[jss::error] == "badFeature", result.toStyledString()); + BEAST_EXPECT( + result[jss::error_message] == "Feature unknown or invalid."); + } + + { + Json::Value params; + params[jss::feature] = + "93E516234E35E08CA689FA33A6D38E103881F8DCB53023F728C307AA89D515" + "A7"; + // invalid param + params[jss::vetoed] = true; + auto const result = env.rpc( + "json", + "feature", + boost::lexical_cast(params))[jss::result]; + BEAST_EXPECTS( + result[jss::error] == "noPermission", + result[jss::error].asString()); + BEAST_EXPECT( + result[jss::error_message] == + "You don't have permission for this command."); + } + + { + std::string const feature = + "C4483A1896170C66C098DEA5B0E024309C60DC960DE5F01CD7AF986AA3D9AD" + "37"; + Json::Value params; + params[jss::feature] = feature; + auto const result = env.rpc( + "json", + "feature", + boost::lexical_cast(params))[jss::result]; + BEAST_EXPECT(result.isMember(feature)); + auto const amendmentResult = result[feature]; + BEAST_EXPECT(amendmentResult[jss::enabled].asBool() == false); + BEAST_EXPECT(amendmentResult[jss::supported].asBool() == true); + BEAST_EXPECT( + amendmentResult[jss::name].asString() == + "fixMasterKeyAsRegularKey"); + } } void