Revert "add back whitelisting and unit tests for it (#2545)"

kdiluca · kdiluca · commit 99bb544e3538 · 2020-09-10T09:15:02.000-04:00
This reverts commit 0cedbb9.
diff --git a/proto/options.proto b/proto/options.proto
@@ -137,14 +137,14 @@ message Options {
   enum Action {
     route = 1;
     locate = 2;
-    sources_to_targets = 3;
-    optimized_route = 4;
-    isochrone = 5;
-    trace_route = 6;
-    trace_attributes = 7;
-    height = 8;
-    transit_available = 9;
-    expansion = 10;
+    sources_to_targets = 6;
+    optimized_route = 7;
+    isochrone = 8;
+    trace_route = 9;
+    trace_attributes = 10;
+    height = 11;
+    transit_available = 12;
+    expansion = 13;
   }
 
   enum DateTimeType {
diff --git a/src/loki/worker.cc b/src/loki/worker.cc
@@ -163,7 +163,6 @@ loki_worker_t::loki_worker_t(const boost::property_tree::ptree& config,
     if (!Options_Action_Enum_Parse(path, &action)) {
       throw std::runtime_error("Action not supported " + path);
     }
-    actions.insert(action);
     action_str.append("'/" + path + "' ");
   }
   // Make sure we have at least something to support!
@@ -265,7 +264,7 @@ loki_worker_t::work(const std::list<zmq::message_t>& job,
     const auto& options = request.options();
 
     // check there is a valid action
-    if (!options.has_action() || actions.find(options.action()) == actions.cend()) {
+    if (!options.has_action()) {
       return jsonify_error({106, action_str}, info, request);
     }
 
diff --git a/test/loki_service.cc b/test/loki_service.cc
@@ -3,9 +3,6 @@
 
 #include "baldr/rapidjson_utils.h"
 #include "midgard/logging.h"
-#include "proto/api.pb.h"
-#include "proto_conversions.h"
-
 #include <boost/property_tree/ptree.hpp>
 #include <prime_server/http_protocol.hpp>
 #include <prime_server/prime_server.hpp>
@@ -330,17 +327,27 @@ const std::vector<std::pair<uint16_t, std::string>> osrm_responses{
     {400,
      R"({"code":"InvalidValue","message":"The successfully parsed query parameters are invalid."})"}};
 
-boost::property_tree::ptree make_config(
-    const std::string& actions =
-        R"([ "locate", "route", "sources_to_targets", "optimized_route", "isochrone", "trace_route", "trace_attributes" ])") {
+zmq::context_t context;
+void start_service() {
+  // server
+  std::thread server(
+      std::bind(&http_server_t::serve,
+                http_server_t(context, "ipc:///tmp/test_loki_server", "ipc:///tmp/test_loki_proxy_in",
+                              "ipc:///tmp/test_loki_results", "ipc:///tmp/test_loki_interrupt")));
+  server.detach();
+
+  // load balancer
+  std::thread proxy(std::bind(&proxy_t::forward, proxy_t(context, "ipc:///tmp/test_loki_proxy_in",
+                                                         "ipc:///tmp/test_loki_proxy_out")));
+  proxy.detach();
+
+  // make the config file
   boost::property_tree::ptree config;
   std::stringstream json;
   json << R"({
       "meili": { "default": { "breakage_distance": 2000} },
       "mjolnir": { "tile_dir": "test/tiles" },
-      "loki": { "actions": )";
-  json << actions;
-  json << R"(,
+      "loki": { "actions": [ "locate", "route", "sources_to_targets", "optimized_route", "isochrone", "trace_route", "trace_attributes" ],
                   "logging": { "long_request": 100.0 },
                   "service": { "proxy": "ipc:///tmp/test_loki_proxy" },
                 "service_defaults": { "minimum_reachability": 50, "radius": 0,"search_cutoff": 35000, "node_snap_tolerance": 5, "street_side_tolerance": 5, "street_side_max_distance": 1000, "heading_tolerance": 60} },
@@ -363,25 +370,6 @@ boost::property_tree::ptree make_config(
       "costing_directions_options": { "auto": {}, "pedestrian": {} }
     })";
   rapidjson::read_json(json, config);
-  return config;
-}
-
-zmq::context_t context;
-void start_service() {
-  // server
-  std::thread server(
-      std::bind(&http_server_t::serve,
-                http_server_t(context, "ipc:///tmp/test_loki_server", "ipc:///tmp/test_loki_proxy_in",
-                              "ipc:///tmp/test_loki_results", "ipc:///tmp/test_loki_interrupt")));
-  server.detach();
-
-  // load balancer
-  std::thread proxy(std::bind(&proxy_t::forward, proxy_t(context, "ipc:///tmp/test_loki_proxy_in",
-                                                         "ipc:///tmp/test_loki_proxy_out")));
-  proxy.detach();
-
-  // make the config file
-  auto config = make_config();
 
   // service worker
   std::thread worker(valhalla::loki::run_service, config);
@@ -440,37 +428,7 @@ TEST(LokiService, test_osrm_failure_requests) {
   run_requests(osrm_requests, osrm_responses);
 }
 
-TEST(LokiService, test_actions_whitelist) {
-  http_request_info_t info{};
-
-  // check that you only get in if your on the configured list
-  for (auto action = Options::Action_MIN; action < Options::Action_ARRAYSIZE;
-       action = static_cast<Options::Action>(static_cast<int>(action) + 1)) {
-
-    auto wrong_action = static_cast<Options::Action>(static_cast<int>(action) +
-                                                     (action == Options::Action_MAX ? -1 : 1));
-    auto whitelist = R"([")" + Options_Action_Enum_Name(wrong_action) + R"("])";
-    auto config = make_config(whitelist);
-    loki::loki_worker_t worker(config);
-    http_request_t request(method_t::GET, "/" + Options_Action_Enum_Name(action));
-    auto req_str = request.to_string();
-    auto msg = zmq::message_t{reinterpret_cast<void*>(&req_str.front()), req_str.size(),
-                              [](void*, void*) {}};
-    auto result = worker.work({msg}, reinterpret_cast<void*>(&info), []() {});
-
-    // failed to find that action in the whitelist
-    EXPECT_TRUE(result.messages.front().find("Try any") != std::string::npos);
-
-    http_request_t request1(method_t::GET, "/" + Options_Action_Enum_Name(wrong_action));
-    req_str = request1.to_string();
-    msg = zmq::message_t{reinterpret_cast<void*>(&req_str.front()), req_str.size(),
-                         [](void*, void*) {}};
-    result = worker.work({msg}, reinterpret_cast<void*>(&info), []() {});
-
-    // found the action this time but failed for no locations
-    EXPECT_TRUE(result.messages.front().find("Try any") == std::string::npos);
-  }
-}
+// todo: test_success_requests??
 
 } // namespace
 
diff --git a/valhalla/loki/worker.h b/valhalla/loki/worker.h
@@ -67,7 +67,6 @@ class loki_worker_t : public service_worker_t {
   sif::cost_ptr_t costing;
   std::shared_ptr<baldr::GraphReader> reader;
   std::shared_ptr<baldr::connectivity_map_t> connectivity_map;
-  std::unordered_set<Options::Action> actions;
   std::string action_str;
   std::unordered_map<std::string, size_t> max_locations;
   std::unordered_map<std::string, float> max_distance;

Original file line number	Diff line number	Diff line change
`@@ -163,7 +163,6 @@ loki_worker_t::loki_worker_t(const boost::property_tree::ptree& config,`
`163`	`163`	`if (!Options_Action_Enum_Parse(path, &action)) {`
`164`	`164`	`throw std::runtime_error("Action not supported " + path);`
`165`	`165`	`}`
`166`		`- actions.insert(action);`
`167`	`166`	`action_str.append("'/" + path + "' ");`
`168`	`167`	`}`
`169`	`168`	`// Make sure we have at least something to support!`
`@@ -265,7 +264,7 @@ loki_worker_t::work(const std::list<zmq::message_t>& job,`
`265`	`264`	`const auto& options = request.options();`
`266`	`265`
`267`	`266`	`// check there is a valid action`
`268`		`- if (!options.has_action() \|\| actions.find(options.action()) == actions.cend()) {`
	`267`	`+ if (!options.has_action()) {`
`269`	`268`	`return jsonify_error({106, action_str}, info, request);`
`270`	`269`	`}`
`271`	`270`