Check client_id in sensor data handlers (#1266)

cartographer/cloud/internal/handlers/add_fixed_frame_pose_data_handler.cc

@@ -29,8 +29,8 @@ namespace cartographer {
 namespace cloud {
 namespace handlers {
 
-void AddFixedFramePoseDataHandler::OnRequest(
+void AddFixedFramePoseDataHandler::OnSensorData(
-    const proto::AddFixedFramePoseDataRequest &request) {
+    const proto::AddFixedFramePoseDataRequest& request) {
   // The 'BlockingQueue' returned by 'sensor_data_queue()' is already
   // thread-safe. Therefore it suffices to get an unsynchronized reference to
   // the 'MapBuilderContext'.
@@ -55,10 +55,6 @@ void AddFixedFramePoseDataHandler::OnRequest(
   }
 }
 
-void AddFixedFramePoseDataHandler::OnReadsDone() {
-  Send(common::make_unique<google::protobuf::Empty>());
-}
-
 }  // namespace handlers
 }  // namespace cloud
 }  // namespace cartographer

cartographer/cloud/internal/handlers/add_fixed_frame_pose_data_handler.h

@@ -18,6 +18,7 @@
 #define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_FIXED_FRAME_POSE_DATA_HANDLER_H
 
 #include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h"
 #include "cartographer/cloud/proto/map_builder_service.pb.h"
 #include "google/protobuf/empty.pb.h"
 
@@ -32,10 +33,10 @@ DEFINE_HANDLER_SIGNATURE(
     "/cartographer.cloud.proto.MapBuilderService/AddFixedFramePoseData")
 
 class AddFixedFramePoseDataHandler
-    : public async_grpc::RpcHandler<AddFixedFramePoseDataSignature> {
+    : public AddSensorDataHandlerBase<AddFixedFramePoseDataSignature> {
  public:
-  void OnRequest(const proto::AddFixedFramePoseDataRequest &request) override;
+  void OnSensorData(
-  void OnReadsDone() override;
+      const proto::AddFixedFramePoseDataRequest& request) override;
 };
 
 }  // namespace handlers

cartographer/cloud/internal/handlers/add_fixed_frame_pose_data_handler_test.cc

@@ -56,6 +56,11 @@ TEST_F(AddFixedFramePoseDataHandlerTest, NoLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetNoLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),
@@ -70,6 +75,11 @@ TEST_F(AddFixedFramePoseDataHandlerTest, WithMockLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetMockLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),

cartographer/cloud/internal/handlers/add_imu_data_handler.cc

@@ -29,7 +29,7 @@ namespace cartographer {
 namespace cloud {
 namespace handlers {
 
-void AddImuDataHandler::OnRequest(const proto::AddImuDataRequest &request) {
+void AddImuDataHandler::OnSensorData(const proto::AddImuDataRequest& request) {
   // The 'BlockingQueue' returned by 'sensor_data_queue()' is already
   // thread-safe. Therefore it suffices to get an unsynchronized reference to
   // the 'MapBuilderContext'.
@@ -52,10 +52,6 @@ void AddImuDataHandler::OnRequest(const proto::AddImuDataRequest &request) {
   }
 }
 
-void AddImuDataHandler::OnReadsDone() {
-  Send(common::make_unique<google::protobuf::Empty>());
-}
-
 }  // namespace handlers
 }  // namespace cloud
 }  // namespace cartographer

cartographer/cloud/internal/handlers/add_imu_data_handler.h

@@ -18,6 +18,7 @@
 #define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_IMU_DATA_HANDLER_H
 
 #include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h"
 #include "cartographer/cloud/proto/map_builder_service.pb.h"
 #include "google/protobuf/empty.pb.h"
 
@@ -30,10 +31,9 @@ DEFINE_HANDLER_SIGNATURE(
     google::protobuf::Empty,
     "/cartographer.cloud.proto.MapBuilderService/AddImuData")
 
-class AddImuDataHandler : public async_grpc::RpcHandler<AddImuDataSignature> {
+class AddImuDataHandler : public AddSensorDataHandlerBase<AddImuDataSignature> {
  public:
-  void OnRequest(const proto::AddImuDataRequest &request) override;
+  void OnSensorData(const proto::AddImuDataRequest& request) override;
-  void OnReadsDone() override;
 };
 
 }  // namespace handlers

cartographer/cloud/internal/handlers/add_imu_data_handler_test.cc

@@ -53,6 +53,11 @@ TEST_F(AddImuDataHandlerTest, NoLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetNoLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),
@@ -67,6 +72,11 @@ TEST_F(AddImuDataHandlerTest, WithMockLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetMockLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),

cartographer/cloud/internal/handlers/add_landmark_data_handler.cc

@@ -29,8 +29,8 @@ namespace cartographer {
 namespace cloud {
 namespace handlers {
 
-void AddLandmarkDataHandler::OnRequest(
+void AddLandmarkDataHandler::OnSensorData(
-    const proto::AddLandmarkDataRequest &request) {
+    const proto::AddLandmarkDataRequest& request) {
   // The 'BlockingQueue' returned by 'sensor_data_queue()' is already
   // thread-safe. Therefore it suffices to get an unsynchronized reference to
   // the 'MapBuilderContext'.
@@ -53,10 +53,6 @@ void AddLandmarkDataHandler::OnRequest(
   }
 }
 
-void AddLandmarkDataHandler::OnReadsDone() {
-  Send(common::make_unique<google::protobuf::Empty>());
-}
-
 }  // namespace handlers
 }  // namespace cloud
 }  // namespace cartographer

cartographer/cloud/internal/handlers/add_landmark_data_handler.h

@@ -18,6 +18,7 @@
 #define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_LANDMARK_DATA_HANDLER_H
 
 #include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h"
 #include "cartographer/cloud/proto/map_builder_service.pb.h"
 #include "google/protobuf/empty.pb.h"
 
@@ -31,10 +32,9 @@ DEFINE_HANDLER_SIGNATURE(
     "/cartographer.cloud.proto.MapBuilderService/AddLandmarkData")
 
 class AddLandmarkDataHandler
-    : public async_grpc::RpcHandler<AddLandmarkDataSignature> {
+    : public AddSensorDataHandlerBase<AddLandmarkDataSignature> {
  public:
-  void OnRequest(const proto::AddLandmarkDataRequest &request) override;
+  void OnSensorData(const proto::AddLandmarkDataRequest& request) override;
-  void OnReadsDone() override;
 };
 
 }  // namespace handlers

cartographer/cloud/internal/handlers/add_landmark_data_handler_test.cc

@@ -60,6 +60,11 @@ TEST_F(AddLandmarkDataHandlerTest, NoLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetNoLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),
@@ -74,6 +79,11 @@ TEST_F(AddLandmarkDataHandlerTest, WithMockLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetMockLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),

cartographer/cloud/internal/handlers/add_odometry_data_handler.cc

@@ -29,8 +29,8 @@ namespace cartographer {
 namespace cloud {
 namespace handlers {
 
-void AddOdometryDataHandler::OnRequest(
+void AddOdometryDataHandler::OnSensorData(
-    const proto::AddOdometryDataRequest &request) {
+    const proto::AddOdometryDataRequest& request) {
   // The 'BlockingQueue' returned by 'sensor_data_queue()' is already
   // thread-safe. Therefore it suffices to get an unsynchronized reference to
   // the 'MapBuilderContext'.
@@ -53,10 +53,6 @@ void AddOdometryDataHandler::OnRequest(
   }
 }
 
-void AddOdometryDataHandler::OnReadsDone() {
-  Send(common::make_unique<google::protobuf::Empty>());
-}
-
 }  // namespace handlers
 }  // namespace cloud
 }  // namespace cartographer

cartographer/cloud/internal/handlers/add_odometry_data_handler.h

@@ -18,6 +18,7 @@
 #define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_ODOMETRY_DATA_HANDLER_H
 
 #include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h"
 #include "cartographer/cloud/proto/map_builder_service.pb.h"
 #include "google/protobuf/empty.pb.h"
 
@@ -31,10 +32,9 @@ DEFINE_HANDLER_SIGNATURE(
     "/cartographer.cloud.proto.MapBuilderService/AddOdometryData")
 
 class AddOdometryDataHandler
-    : public async_grpc::RpcHandler<AddOdometryDataSignature> {
+    : public AddSensorDataHandlerBase<AddOdometryDataSignature> {
  public:
-  void OnRequest(const proto::AddOdometryDataRequest &request) override;
+  void OnSensorData(const proto::AddOdometryDataRequest& request) override;
-  void OnReadsDone() override;
 };
 
 }  // namespace handlers

cartographer/cloud/internal/handlers/add_odometry_data_handler_test.cc

@@ -55,6 +55,11 @@ TEST_F(AddOdometryDataHandlerTest, NoLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetNoLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),
@@ -69,6 +74,11 @@ TEST_F(AddOdometryDataHandlerTest, WithMockLocalSlamUploader) {
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
   SetMockLocalTrajectoryUploader();
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),

cartographer/cloud/internal/handlers/add_rangefinder_data_handler.cc

@@ -28,8 +28,8 @@ namespace cartographer {
 namespace cloud {
 namespace handlers {
 
-void AddRangefinderDataHandler::OnRequest(
+void AddRangefinderDataHandler::OnSensorData(
-    const proto::AddRangefinderDataRequest &request) {
+    const proto::AddRangefinderDataRequest& request) {
   // The 'BlockingQueue' returned by 'sensor_data_queue()' is already
   // thread-safe. Therefore it suffices to get an unsynchronized reference to
   // the 'MapBuilderContext'.
@@ -40,10 +40,6 @@ void AddRangefinderDataHandler::OnRequest(
           sensor::FromProto(request.timed_point_cloud_data())));
 }
 
-void AddRangefinderDataHandler::OnReadsDone() {
-  Send(common::make_unique<google::protobuf::Empty>());
-}
-
 }  // namespace handlers
 }  // namespace cloud
 }  // namespace cartographer

cartographer/cloud/internal/handlers/add_rangefinder_data_handler.h

@@ -18,6 +18,7 @@
 #define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_RANGEFINDER_DATA_HANDLER_H
 
 #include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h"
 #include "cartographer/cloud/proto/map_builder_service.pb.h"
 #include "google/protobuf/empty.pb.h"
 
@@ -32,10 +33,9 @@ DEFINE_HANDLER_SIGNATURE(
     "/cartographer.cloud.proto.MapBuilderService/AddRangefinderData")
 
 class AddRangefinderDataHandler
-    : public async_grpc::RpcHandler<AddRangefinderDataSignature> {
+    : public AddSensorDataHandlerBase<AddRangefinderDataSignature> {
  public:
-  void OnRequest(const proto::AddRangefinderDataRequest &request) override;
+  void OnSensorData(const proto::AddRangefinderDataRequest& request) override;
-  void OnReadsDone() override;
 };
 
 }  // namespace handlers

cartographer/cloud/internal/handlers/add_rangefinder_data_handler_test.cc

@@ -53,6 +53,11 @@ TEST_F(AddRangefinderDataHandlerTest, NoLocalSlamUploader) {
   proto::AddRangefinderDataRequest request;
   EXPECT_TRUE(
       google::protobuf::TextFormat::ParseFromString(kMessage, &request));
+  EXPECT_CALL(
+      *mock_map_builder_context_,
+      CheckClientIdForTrajectory(Eq(request.sensor_metadata().client_id()),
+                                 Eq(request.sensor_metadata().trajectory_id())))
+      .WillOnce(::testing::Return(true));
   EXPECT_CALL(*mock_map_builder_context_,
               DoEnqueueSensorData(
                   Eq(request.sensor_metadata().trajectory_id()),

cartographer/cloud/internal/handlers/add_sensor_data_batch_handler.cc

@@ -32,6 +32,16 @@ namespace handlers {
 void AddSensorDataBatchHandler::OnRequest(
     const proto::AddSensorDataBatchRequest& request) {
   for (const proto::SensorData& sensor_data : request.sensor_data()) {
+    if (!GetContext<MapBuilderContextInterface>()->CheckClientIdForTrajectory(
+            sensor_data.sensor_metadata().client_id(),
+            sensor_data.sensor_metadata().trajectory_id())) {
+      LOG(ERROR) << "Unknown trajectory with ID "
+                 << sensor_data.sensor_metadata().trajectory_id()
+                 << " and client_id "
+                 << sensor_data.sensor_metadata().client_id();
+      Finish(::grpc::Status(::grpc::NOT_FOUND, "Unknown trajectory"));
+      return;
+    }
     switch (sensor_data.sensor_data_case()) {
       case proto::SensorData::kOdometryData:
         GetUnsynchronizedContext<MapBuilderContextInterface>()

cartographer/cloud/internal/handlers/add_sensor_data_handler_base.h

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2018 The Cartographer Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_SENSOR_DATA_HANDLER_BASE_H
+#define CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_SENSOR_DATA_HANDLER_BASE_H
+
+#include "async_grpc/rpc_handler.h"
+#include "cartographer/cloud/internal/map_builder_context_interface.h"
+
+namespace cartographer {
+namespace cloud {
+namespace handlers {
+
+template <typename HandlerSignatureType>
+class AddSensorDataHandlerBase
+    : public async_grpc::RpcHandler<HandlerSignatureType> {
+ public:
+  using SensorDataType =
+      async_grpc::StripStream<typename HandlerSignatureType::IncomingType>;
+
+  void OnRequest(const SensorDataType& request) override {
+    if (!this->template GetContext<
+                 cartographer::cloud::MapBuilderContextInterface>()
+             ->CheckClientIdForTrajectory(
+                 request.sensor_metadata().client_id(),
+                 request.sensor_metadata().trajectory_id())) {
+      LOG(ERROR) << "Unknown trajectory with ID "
+                 << request.sensor_metadata().trajectory_id()
+                 << " and client_id " << request.sensor_metadata().client_id();
+      this->template Finish(
+          ::grpc::Status(::grpc::NOT_FOUND, "Unknown trajectory"));
+      return;
+    }
+    OnSensorData(request);
+  }
+
+  virtual void OnSensorData(const SensorDataType& request) = 0;
+
+  void OnReadsDone() override {
+    this->template Send(common::make_unique<google::protobuf::Empty>());
+  }
+};
+
+}  // namespace handlers
+}  // namespace cloud
+}  // namespace cartographer
+
+#endif  // CARTOGRAPHER_CLOUD_INTERNAL_HANDLERS_ADD_SENSOR_DATA_HANDLER_BASE_H