Fix/sdl 3620 SDL does not respond with NACK to start RPC service request with invalid data

src/components/connection_handler/src/connection_handler_impl.cc

@@ -547,11 +547,16 @@ void ConnectionHandlerImpl::OnSessionStartedCallback(
         session_key,
         service_type,
         params);
-  } else {
+  }
+#ifdef BUILD_TESTS
+  else {
+    // FIXME (VSemenyuk): This code is only used in unit tests, so should be
+    // removed. ConnectionHandler unit tests should be fixed.
     if (protocol_handler_) {
       protocol_handler_->NotifySessionStarted(context, rejected_params);
     }
   }
+#endif
 }
 
 void ConnectionHandlerImpl::NotifyServiceStartedResult(
@@ -589,17 +594,20 @@ void ConnectionHandlerImpl::NotifyServiceStartedResult(
 
   if (!result) {
     SDL_LOG_WARN("Service starting forbidden by connection_handler_observer");
+    context.is_start_session_failed_ = true;
+  }
+
+  if (protocol_handler_) {
+    protocol_handler_->NotifySessionStarted(context, rejected_params, reason);
+  }
+
+  if (context.is_start_session_failed_) {
     if (protocol_handler::kRpc == context.service_type_) {
       connection->RemoveSession(context.new_session_id_);
     } else {
       connection->RemoveService(context.initial_session_id_,
                                 context.service_type_);
     }
-    context.new_session_id_ = 0;
-  }
-
-  if (protocol_handler_ != NULL) {
-    protocol_handler_->NotifySessionStarted(context, rejected_params, reason);
   }
 }
 

src/components/connection_handler/test/connection_handler_impl_test.cc

@@ -56,6 +56,7 @@ using namespace ::connection_handler;
 using ::protocol_handler::ServiceType;
 using namespace ::protocol_handler;
 using ::testing::_;
+using ::testing::An;
 using ::testing::ByRef;
 using ::testing::DoAll;
 using ::testing::InSequence;
@@ -127,7 +128,8 @@ class ConnectionHandlerTest : public ::testing::Test {
   void AddTestSession() {
     protocol_handler_test::MockProtocolHandler temp_protocol_handler;
     connection_handler_->set_protocol_handler(&temp_protocol_handler);
-    EXPECT_CALL(temp_protocol_handler, NotifySessionStarted(_, _, _))
+    EXPECT_CALL(temp_protocol_handler,
+                NotifySessionStarted(An<SessionContext&>(), _, _))
         .WillOnce(SaveArg<0>(&out_context_));
 
     connection_handler_->OnSessionStartedCallback(
@@ -164,7 +166,8 @@ class ConnectionHandlerTest : public ::testing::Test {
     SessionContext context;
     protocol_handler_test::MockProtocolHandler temp_protocol_handler;
     connection_handler_->set_protocol_handler(&temp_protocol_handler);
-    EXPECT_CALL(temp_protocol_handler, NotifySessionStarted(_, _, _))
+    EXPECT_CALL(temp_protocol_handler,
+                NotifySessionStarted(An<SessionContext&>(), _, _))
         .WillOnce(SaveArg<0>(&context));
 
     connection_handler_->OnSessionStartedCallback(uid_,
@@ -371,7 +374,8 @@ TEST_F(ConnectionHandlerTest, StartSession_NoConnection) {
   protocol_handler::SessionContext context;
 
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&context));
 
   connection_handler_->OnSessionStartedCallback(
@@ -1268,7 +1272,8 @@ TEST_F(ConnectionHandlerTest, StartService_withServices) {
 
   SessionContext audio_context, video_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&audio_context))
       .WillOnce(SaveArg<0>(&video_context));
 
@@ -1309,7 +1314,8 @@ TEST_F(ConnectionHandlerTest, StartService_withServices_withParams) {
   std::vector<std::string> empty;
   BsonObject* dummy_param = reinterpret_cast<BsonObject*>(&dummy);
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, empty, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), empty, _))
       .WillOnce(SaveArg<0>(&video_context));
 
   connection_handler_->OnSessionStartedCallback(uid_,
@@ -1354,7 +1360,8 @@ TEST_F(ConnectionHandlerTest, ServiceStop) {
 
   SessionContext audio_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillRepeatedly(SaveArg<0>(&audio_context));
 
   // Check ignoring hash_id on stop non-rpc service
@@ -1445,7 +1452,8 @@ TEST_F(ConnectionHandlerTest, SessionStarted_WithRpc) {
           reason));
 
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&out_context_));
 
   // Start new session with RPC service
@@ -1485,7 +1493,8 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_SUCCESS) {
 
   // confirm that NotifySessionStarted() is called
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, empty, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), empty, _))
       .WillOnce(SaveArg<0>(&out_context_));
 
   connection_handler_->OnSessionStartedCallback(uid_,
@@ -1527,7 +1536,8 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_FAILURE) {
 
   // confirm that NotifySessionStarted() is called
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, empty, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), empty, _))
       .WillOnce(SaveArg<0>(&out_context_));
 
   connection_handler_->OnSessionStartedCallback(uid_,
@@ -1536,7 +1546,7 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_FAILURE) {
                                                 PROTECTION_OFF,
                                                 dummy_params);
 
-  EXPECT_EQ(0u, out_context_.new_session_id_);
+  EXPECT_TRUE(out_context_.is_start_session_failed_);
 }
 
 /*
@@ -1551,7 +1561,8 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_Multiple) {
 
   protocol_handler_test::MockProtocolHandler temp_protocol_handler;
   connection_handler_->set_protocol_handler(&temp_protocol_handler);
-  EXPECT_CALL(temp_protocol_handler, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(temp_protocol_handler,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&context_first))
       .WillOnce(SaveArg<0>(&context_second));
 
@@ -1622,7 +1633,8 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_Multiple) {
   // verify that connection handler will not mix up the two results
   SessionContext new_context_first, new_context_second;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, empty, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), empty, _))
       .WillOnce(SaveArg<0>(&new_context_second))
       .WillOnce(SaveArg<0>(&new_context_first));
 
@@ -1637,8 +1649,10 @@ TEST_F(ConnectionHandlerTest, ServiceStarted_Video_Multiple) {
                                                 PROTECTION_OFF,
                                                 dummy_params);
 
-  EXPECT_NE(0u, new_context_first.new_session_id_);   // result is positive
-  EXPECT_EQ(0u, new_context_second.new_session_id_);  // result is negative
+  EXPECT_FALSE(
+      new_context_first.is_start_session_failed_);  // result is positive
+  EXPECT_TRUE(
+      new_context_second.is_start_session_failed_);  // result is negative
 }
 
 TEST_F(ConnectionHandlerTest,
@@ -1654,7 +1668,8 @@ TEST_F(ConnectionHandlerTest,
   SessionContext fail_context;
   SessionContext positive_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&fail_context))
       .WillOnce(SaveArg<0>(&positive_context));
 
@@ -1697,7 +1712,8 @@ TEST_F(ConnectionHandlerTest,
   SessionContext fail_context;
   SessionContext positive_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&fail_context))
       .WillOnce(SaveArg<0>(&positive_context));
 
@@ -1742,7 +1758,8 @@ TEST_F(ConnectionHandlerTest,
 
   SessionContext context_first, context_second;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&context_first))
       .WillOnce(SaveArg<0>(&context_second));
 
@@ -1797,7 +1814,8 @@ TEST_F(ConnectionHandlerTest,
 
   SessionContext rejected_context, positive_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&rejected_context))
       .WillOnce(SaveArg<0>(&positive_context));
 
@@ -1840,7 +1858,8 @@ TEST_F(ConnectionHandlerTest, SessionStarted_DelayProtect) {
 
   SessionContext context_new, context_second, context_third;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&context_new))
       .WillOnce(SaveArg<0>(&context_second))
       .WillOnce(SaveArg<0>(&context_third));
@@ -1895,7 +1914,8 @@ TEST_F(ConnectionHandlerTest, SessionStarted_DelayProtectBulk) {
 
   SessionContext new_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&new_context));
   connection_handler_->OnSessionStartedCallback(uid_,
                                                 out_context_.new_session_id_,
@@ -2001,7 +2021,8 @@ TEST_F(ConnectionHandlerTest, GetSSLContext_ByProtectedService) {
 
   SessionContext new_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&new_context));
 
   // Open kAudio service
@@ -2038,7 +2059,8 @@ TEST_F(ConnectionHandlerTest, GetSSLContext_ByDealyProtectedRPC) {
 
   SessionContext new_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&new_context));
 
   // Protect kRpc (Bulk will be protect also)
@@ -2078,7 +2100,8 @@ TEST_F(ConnectionHandlerTest, GetSSLContext_ByDealyProtectedBulk) {
 
   SessionContext new_context;
   connection_handler_->set_protocol_handler(&mock_protocol_handler_);
-  EXPECT_CALL(mock_protocol_handler_, NotifySessionStarted(_, _, _))
+  EXPECT_CALL(mock_protocol_handler_,
+              NotifySessionStarted(An<SessionContext&>(), _, _))
       .WillOnce(SaveArg<0>(&new_context));
 
   // Protect Bulk (kRpc will be protected also)

src/components/include/protocol_handler/protocol_handler.h

@@ -137,11 +137,27 @@ class ProtocolHandler {
    * @param err_reason string with NACK reason. Only valid when
    * generated_session_id is 0.
    */
+  DEPRECATED
   virtual void NotifySessionStarted(
       const SessionContext& context,
       std::vector<std::string>& rejected_params,
       const std::string err_reason = std::string()) = 0;
 
+  /**
+   * @brief Called by connection handler to notify the context of
+   * OnSessionStartedCallback().
+   * @param context reference to structure with started session data
+   * @param rejected_params list of parameters name that are rejected.
+   * Only valid when generated_session_id is 0. Note, even if
+   * generated_session_id is 0, the list may be empty.
+   * @param err_reason string with NACK reason. Only valid when
+   * generated_session_id is 0.
+   */
+  virtual void NotifySessionStarted(
+      SessionContext& context,
+      std::vector<std::string>& rejected_params,
+      const std::string err_reason = std::string()) = 0;
+
   virtual bool IsRPCServiceSecure(const uint32_t connection_key) const = 0;
 
   virtual void ProcessFailedPTU() = 0;

src/components/include/protocol_handler/session_observer.h

@@ -69,6 +69,7 @@ struct SessionContext {
   uint32_t hash_id_;
   bool is_protected_;
   bool is_new_service_;
+  bool is_start_session_failed_;
 
   /**
    * @brief Constructor
@@ -81,7 +82,8 @@ struct SessionContext {
       , service_type_(protocol_handler::kInvalidServiceType)
       , hash_id_(0)
       , is_protected_(false)
-      , is_new_service_(false) {}
+      , is_new_service_(false)
+      , is_start_session_failed_(false) {}
 
   /**
    * @brief Constructor
@@ -111,7 +113,8 @@ struct SessionContext {
       , service_type_(service_type)
       , hash_id_(hash_id)
       , is_protected_(is_protected)
-      , is_new_service_(false) {}
+      , is_new_service_(false)
+      , is_start_session_failed_(false) {}
 };
 
 /**

src/components/include/test/protocol_handler/mock_protocol_handler.h

@@ -64,6 +64,10 @@ class MockProtocolHandler : public ::protocol_handler::ProtocolHandler {
   MOCK_CONST_METHOD0(get_settings,
                      const ::protocol_handler::ProtocolHandlerSettings&());
   MOCK_METHOD0(get_session_observer, protocol_handler::SessionObserver&());
+  MOCK_METHOD3(NotifySessionStarted,
+               void(::protocol_handler::SessionContext& context,
+                    std::vector<std::string>& rejected_params,
+                    const std::string err_reason));
   MOCK_METHOD3(NotifySessionStarted,
                void(const ::protocol_handler::SessionContext& context,
                     std::vector<std::string>& rejected_params,