Update RPC handler role/usage (RIPD-557):

* Properly use the RPC method to determine required role for HTTP/S RPC calls.
* Charge for malformed RPC calls over HTTP/S

Builds/VisualStudio2015/RippleD.vcxproj

@@ -4620,6 +4620,10 @@
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
     </ClCompile>
+    <ClCompile Include="..\..\src\test\rpc\RPCOverload_test.cpp">
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
+      <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
+    </ClCompile>
     <ClCompile Include="..\..\src\test\rpc\ServerInfo_test.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>

Builds/VisualStudio2015/RippleD.vcxproj.filters

@@ -5262,6 +5262,9 @@
     <ClCompile Include="..\..\src\test\rpc\RobustTransaction_test.cpp">
       <Filter>test\rpc</Filter>
     </ClCompile>
+    <ClCompile Include="..\..\src\test\rpc\RPCOverload_test.cpp">
+      <Filter>test\rpc</Filter>
+    </ClCompile>
     <ClCompile Include="..\..\src\test\rpc\ServerInfo_test.cpp">
       <Filter>test\rpc</Filter>
     </ClCompile>

src/ripple/rpc/impl/ServerHandlerImp.cpp

@@ -402,6 +402,8 @@ ServerHandlerImp::processSession(
     if (is->getConsumer().disconnect())
     {
         session->close();
+        // FIX: This rpcError is not delivered since the session
+        // was just closed.
         return rpcError(rpcSLOW_DOWN);
     }
 
@@ -438,6 +440,7 @@ ServerHandlerImp::processSession(
         is->user());
     if (Role::FORBID == role)
     {
+        loadType = Resource::feeInvalidRPC;
         jr[jss::result] = rpcError (rpcFORBIDDEN);
     }
     else
@@ -550,26 +553,12 @@ ServerHandlerImp::processRequest (Port const& port,
         }
     }
 
-    // Parse id now so errors from here on will have the id
+    /* ---------------------------------------------------------------------- */
-    //
+    // Determine role/usage so we can charge for invalid requests
-    // VFALCO NOTE Except that "id" isn't included in the following errors.
-    //
-    Json::Value const& id = jsonRPC [jss::id];
     Json::Value const& method = jsonRPC [jss::method];
 
-    if (! method) {
-        HTTPReply (400, "Null method", output, rpcJ);
-        return;
-    }
-
-    if (!method.isString ()) {
-        HTTPReply (400, "method is not string", output, rpcJ);
-        return;
-    }
-
-    /* ---------------------------------------------------------------------- */
     auto role = Role::FORBID;
-    auto required = RPC::roleRequired(id.asString());
+    auto required = RPC::roleRequired(method.asString());
     if (jsonRPC.isMember(jss::params) &&
         jsonRPC[jss::params].isArray() &&
         jsonRPC[jss::params].size() > 0 &&
@@ -584,16 +573,6 @@ ServerHandlerImp::processRequest (Port const& port,
             remoteIPAddress, user);
     }
 
-    /**
-     * Clear header-assigned values if not positively identified from a
-     * secure_gateway.
-     */
-    if (role != Role::IDENTIFIED)
-    {
-        forwardedFor.clear();
-        user.clear();
-    }
-
     Resource::Consumer usage;
     if (isUnlimited(role))
     {
@@ -610,9 +589,31 @@ ServerHandlerImp::processRequest (Port const& port,
         }
     }
 
+    if (role == Role::FORBID)
+    {
+        usage.charge(Resource::feeInvalidRPC);
+        HTTPReply (403, "Forbidden", output, rpcJ);
+        return;
+    }
+
+    if (! method)
+    {
+        usage.charge(Resource::feeInvalidRPC);
+        HTTPReply (400, "Null method", output, rpcJ);
+        return;
+    }
+
+    if (! method.isString ())
+    {
+        usage.charge(Resource::feeInvalidRPC);
+        HTTPReply (400, "method is not string", output, rpcJ);
+        return;
+    }
+
     std::string strMethod = method.asString ();
     if (strMethod.empty())
     {
+        usage.charge(Resource::feeInvalidRPC);
         HTTPReply (400, "method is empty", output, rpcJ);
         return;
     }
@@ -630,6 +631,7 @@ ServerHandlerImp::processRequest (Port const& port,
 
     else if (!params.isArray () || params.size() != 1)
     {
+        usage.charge(Resource::feeInvalidRPC);
         HTTPReply (400, "params unparseable", output, rpcJ);
         return;
     }
@@ -638,20 +640,20 @@ ServerHandlerImp::processRequest (Port const& port,
         params = std::move (params[0u]);
         if (!params.isObject())
         {
+            usage.charge(Resource::feeInvalidRPC);
             HTTPReply (400, "params unparseable", output, rpcJ);
             return;
         }
     }
 
-    // VFALCO TODO Shouldn't we handle this earlier?
+    /**
-    //
+     * Clear header-assigned values if not positively identified from a
-    if (role == Role::FORBID)
+     * secure_gateway.
+     */
+    if (role != Role::IDENTIFIED)
     {
-        // VFALCO TODO Needs implementing
+        forwardedFor.clear();
-        // FIXME Needs implementing
+        user.clear();
-        // XXX This needs rate limiting to prevent brute forcing password.
-        HTTPReply (403, "Forbidden", output, rpcJ);
-        return;
     }
 
     JLOG(m_journal.debug()) << "Query: " << strMethod << params;
@@ -684,6 +686,10 @@ ServerHandlerImp::processRequest (Port const& port,
         result[jss::status]  = jss::success;
     }
 
+    usage.charge (loadType);
+    if (usage.warn())
+        result[jss::warning] = jss::load;
+
     Json::Value reply (Json::objectValue);
     reply[jss::result] = std::move (result);
     if (jsonRPC.isMember(jss::jsonrpc))
@@ -702,7 +708,6 @@ ServerHandlerImp::processRequest (Port const& port,
         response.size ()));
 
     response += '\n';
-    usage.charge (loadType);
 
     if (auto stream = m_journal.debug())
     {

src/ripple/server/impl/JSONRPCUtil.cpp

@@ -94,6 +94,7 @@ void HTTPReply (
     case 403: output ("HTTP/1.1 403 Forbidden\r\n"); break;
     case 404: output ("HTTP/1.1 404 Not Found\r\n"); break;
     case 500: output ("HTTP/1.1 500 Internal Server Error\r\n"); break;
+    case 503: output ("HTTP/1.1 503 Server is overloaded\r\n"); break;
     }
 
     output (getHTTPHeaderTimestamp ());

src/test/rpc/LedgerRequestRPC_test.cpp

@@ -294,10 +294,12 @@ public:
         env.fund(XRP(100000), gw);
         env.close();
 
-        auto const result = env.rpc ( "ledger_request", "1" ) [jss::result];
+        auto const result = env.rpc ( "ledger_request", "1" )  [jss::result];
-        BEAST_EXPECT(result[jss::error]         == "noPermission");
+        // The current HTTP/S ServerHandler returns an HTTP 403 error code here
-        BEAST_EXPECT(result[jss::status]        == "error");
+        // rather than a noPermission JSON error.  The JSONRPCClient just eats that
-        BEAST_EXPECT(result[jss::error_message] == "You don't have permission for this command.");
+        // error and returns an null result.
+        BEAST_EXPECT(result.type() == Json::nullValue);
+
     }
 
     void run ()

src/test/rpc/RPCOverload_test.cpp

@@ -0,0 +1,90 @@
+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2012, 2013 Ripple Labs Inc.
+    Permission to use, copy, modify, and/or distribute this software for any
+    purpose  with  or without fee is hereby granted, provided that the above
+    copyright notice and this permission notice appear in all copies.
+    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
+    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
+    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+//==============================================================================
+
+#include <BeastConfig.h>
+#include <ripple/protocol/JsonFields.h>
+#include <test/support/WSClient.h>
+#include <test/support/JSONRPCClient.h>
+#include <test/support/jtx.h>
+#include <ripple/beast/unit_test.h>
+
+namespace ripple {
+namespace test {
+
+class RPCOverload_test : public beast::unit_test::suite
+{
+public:
+    void testOverload(bool useWS)
+    {
+        testcase << "Overload " << (useWS ? "WS" : "HTTP") << " RPC client";
+        using namespace jtx;
+        Env env(*this, []()
+            {
+                auto p = std::make_unique<Config>();
+                setupConfigForUnitTests(*p);
+                (*p)["port_rpc"].set("admin","");
+                (*p)["port_ws"].set("admin","");
+                return p;
+            }());
+
+        Account const alice {"alice"};
+        Account const bob {"bob"};
+        env.fund (XRP (10000), alice, bob);
+
+        std::unique_ptr<AbstractClient> client = useWS ?
+              makeWSClient(env.app().config())
+            : makeJSONRPCClient(env.app().config());
+
+        Json::Value tx = Json::objectValue;
+        tx[jss::tx_json] = pay(alice, bob, XRP(1));
+        tx[jss::secret] = toBase58(generateSeed("alice"));
+
+        // Ask the server to repeatedly sign this transaction
+        // Signing is a resource heavy transaction, so we want the server
+        // to warn and eventually boot us.
+        bool warned = false, booted = false;
+        for(int i = 0 ; i < 500 && !booted; ++i)
+        {
+            auto jv = client->invoke("sign", tx);
+            if(!useWS)
+                jv = jv[jss::result];
+            // When booted, we just get a null json response
+            if(jv.isNull())
+                booted = true;
+            else
+                BEAST_EXPECT(jv.isMember(jss::status)
+                             && (jv[jss::status] == "success"));
+
+            if(jv.isMember(jss::warning))
+                warned = jv[jss::warning] == jss::load;
+        }
+        BEAST_EXPECT(warned && booted);
+    }
+
+
+
+    void run() override
+    {
+        testOverload(false /* http */);
+        testOverload(true /* ws */);
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(RPCOverload,app,ripple);
+
+} // test
+} // ripple

src/test/support/impl/WSClient.cpp

@@ -27,6 +27,7 @@
 #include <beast/core/placeholders.hpp>
 #include <beast/core/streambuf.hpp>
 #include <beast/websocket.hpp>
+
 #include <condition_variable>
 
 #include <ripple/beast/unit_test.h>
@@ -97,6 +98,8 @@ class WSClientImpl : public WSClient
     beast::websocket::opcode op_;
     beast::streambuf rb_;
 
+    bool peerClosed_ = false;
+
     // synchronize destructor
     bool b0_ = false;
     std::mutex m0_;
@@ -112,7 +115,8 @@ class WSClientImpl : public WSClient
     void cleanup()
     {
         error_code ec;
-        ws_.close({}, ec);
+		if(!peerClosed_)
+        	ws_.close({}, ec);
         stream_.close(ec);
         work_ = boost::none;
         thread_.join();
@@ -255,7 +259,12 @@ private:
     on_read_msg(error_code const& ec)
     {
         if(ec)
+        {
+            if(ec == beast::websocket::error::closed)
+               peerClosed_ = true;
             return;
+        }
+
         Json::Value jv;
         Json::Reader jr;
         jr.parse(buffer_string(rb_.data()), jv);

src/test/unity/rpc_test_unity.cpp

@@ -33,6 +33,7 @@
 #include <test/rpc/LedgerRequestRPC_test.cpp>
 #include <test/rpc/NoRipple_test.cpp>
 #include <test/rpc/RobustTransaction_test.cpp>
+#include <test/rpc/RPCOverload_test.cpp>
 #include <test/rpc/ServerInfo_test.cpp>
 #include <test/rpc/Status_test.cpp>
 #include <test/rpc/Subscribe_test.cpp>