fix respond on SDL.GetListOfPermission request (#3627)

* fix respond on SDL.GetListOfPermission request

* Error response in case when we could not collect permissions

Co-authored-by: Jacob Keeler <jacob.keeler@livioradio.com>

9 files changed, 127 insertions, 57 deletions

diff --git a/src/components/application_manager/include/application_manager/message_helper.h b/src/components/application_manager/include/application_manager/message_helper.h
index 9797442e34..4aea48c086 100644
--- a/src/components/application_manager/include/application_manager/message_helper.h
+++ b/src/components/application_manager/include/application_manager/message_helper.h
@@ -490,18 +490,22 @@ class MessageHelper {
  * @param permissions Array of groups permissions
  * @param external_consent_status External user consent status
  * @param correlation_id Correlation id of request
+ * @param app_mngr ApplicationManager instance
+ * @param success_flag Indication that we were able to collect permissions
  */
 #ifdef EXTERNAL_PROPRIETARY_MODE
   static void SendGetListOfPermissionsResponse(
       const std::vector<policy::FunctionalGroupPermission>& permissions,
       const policy::ExternalConsentStatus& external_consent_status,
       const uint32_t correlation_id,
-      ApplicationManager& app_mngr);
+      ApplicationManager& app_mngr,
+      const bool success_flag = true);
 #else
   static void SendGetListOfPermissionsResponse(
       const std::vector<policy::FunctionalGroupPermission>& permissions,
       const uint32_t correlation_id,
-      ApplicationManager& app_mngr);
+      ApplicationManager& app_mngr,
+      const bool success_flag = true);
 #endif  // EXTERNAL_PROPRIETARY_MODE
 
   /**
diff --git a/src/components/application_manager/include/application_manager/policies/policy_handler.h b/src/components/application_manager/include/application_manager/policies/policy_handler.h
index 0a815b4c5b..acf4ca7819 100644
--- a/src/components/application_manager/include/application_manager/policies/policy_handler.h
+++ b/src/components/application_manager/include/application_manager/policies/policy_handler.h
@@ -880,19 +880,21 @@ class PolicyHandler : public PolicyHandlerInterface,
   /**
    * @brief Collects permissions for all currently registered applications on
    * all devices
-   * @return consolidated permissions list or empty list if no
-   * applications/devices currently present
+   * @param out_permissions output ref on list of application permissions
+   * @return true if app connection information is valid, otherwise - false
    */
-  std::vector<FunctionalGroupPermission> CollectRegisteredAppsPermissions();
+  bool CollectRegisteredAppsPermissions(
+      std::vector<FunctionalGroupPermission>& out_permissions);
 
   /**
    * @brief Collects permissions for application with certain connection key
    * @param connection_key Connection key of application to look for
-   * @return list of application permissions or empty list if no such
-   * application found
+   * @param out_permissions output ref on list of application permissions
+   * @return true if app connection information is valid, otherwise - false
    */
-  std::vector<FunctionalGroupPermission> CollectAppPermissions(
-      const uint32_t connection_key);
+  bool CollectAppPermissions(
+      const uint32_t connection_key,
+      std::vector<policy::FunctionalGroupPermission>& out_permissions);
 
  private:
   static const std::string kLibrary;
diff --git a/src/components/application_manager/src/message_helper/message_helper.cc b/src/components/application_manager/src/message_helper/message_helper.cc
index 488a5f6eef..c0f8a5dcfb 100644
--- a/src/components/application_manager/src/message_helper/message_helper.cc
+++ b/src/components/application_manager/src/message_helper/message_helper.cc
@@ -2333,7 +2333,8 @@ void MessageHelper::SendGetListOfPermissionsResponse(
     const std::vector<policy::FunctionalGroupPermission>& permissions,
     const policy::ExternalConsentStatus& external_consent_status,
     uint32_t correlation_id,
-    ApplicationManager& app_mngr) {
+    ApplicationManager& app_mngr,
+    const bool success_flag) {
   using namespace smart_objects;
   using namespace hmi_apis;
 
@@ -2345,7 +2346,8 @@ void MessageHelper::SendGetListOfPermissionsResponse(
   params[strings::function_id] = FunctionID::SDL_GetListOfPermissions;
   params[strings::message_type] = MessageType::kResponse;
   params[strings::correlation_id] = correlation_id;
-  params[hmi_response::code] = static_cast<int32_t>(Common_Result::SUCCESS);
+  params[hmi_response::code] = static_cast<int32_t>(
+      success_flag ? Common_Result::SUCCESS : Common_Result::GENERIC_ERROR);
 
   SmartObject& msg_params = (*message)[strings::msg_params];
 
@@ -2375,7 +2377,8 @@ void MessageHelper::SendGetListOfPermissionsResponse(
 void MessageHelper::SendGetListOfPermissionsResponse(
     const std::vector<policy::FunctionalGroupPermission>& permissions,
     uint32_t correlation_id,
-    ApplicationManager& app_mngr) {
+    ApplicationManager& app_mngr,
+    const bool success_flag) {
   using namespace smart_objects;
   using namespace hmi_apis;
 
@@ -2387,7 +2390,8 @@ void MessageHelper::SendGetListOfPermissionsResponse(
   params[strings::function_id] = FunctionID::SDL_GetListOfPermissions;
   params[strings::message_type] = MessageType::kResponse;
   params[strings::correlation_id] = correlation_id;
-  params[hmi_response::code] = static_cast<int32_t>(Common_Result::SUCCESS);
+  params[hmi_response::code] = static_cast<int32_t>(
+      success_flag ? Common_Result::SUCCESS : Common_Result::GENERIC_ERROR);
 
   SmartObject& msg_params = (*message)[strings::msg_params];
 
diff --git a/src/components/application_manager/src/policies/policy_handler.cc b/src/components/application_manager/src/policies/policy_handler.cc
index bacd478138..dd5ec0d52d 100644
--- a/src/components/application_manager/src/policies/policy_handler.cc
+++ b/src/components/application_manager/src/policies/policy_handler.cc
@@ -866,12 +866,11 @@ void PolicyHandler::ExchangePolicyManager(
   atomic_policy_manager_.swap(policy_manager);
 }
 
-std::vector<policy::FunctionalGroupPermission>
-PolicyHandler::CollectRegisteredAppsPermissions() {
+bool PolicyHandler::CollectRegisteredAppsPermissions(
+    std::vector<FunctionalGroupPermission>& out_permissions) {
   SDL_LOG_AUTO_TRACE();
   const auto policy_manager = LoadPolicyManager();
-  POLICY_LIB_CHECK_OR_RETURN(policy_manager,
-                             std::vector<policy::FunctionalGroupPermission>());
+  POLICY_LIB_CHECK_OR_RETURN(policy_manager, false);
   // If no specific app was passed, get permissions for all currently registered
   // applications
   sync_primitives::AutoLock lock(app_to_device_link_lock_);
@@ -887,14 +886,17 @@ PolicyHandler::CollectRegisteredAppsPermissions() {
         it->first, it->second, group_permissions);
     consolidator.Consolidate(group_permissions);
   }
-  return consolidator.GetConsolidatedPermissions();
+
+  out_permissions = consolidator.GetConsolidatedPermissions();
+  return true;
 }
 
-std::vector<FunctionalGroupPermission> PolicyHandler::CollectAppPermissions(
-    const uint32_t connection_key) {
+bool PolicyHandler::CollectAppPermissions(
+    const uint32_t connection_key,
+    std::vector<policy::FunctionalGroupPermission>& out_permissions) {
   std::vector<FunctionalGroupPermission> group_permissions;
   const auto policy_manager = LoadPolicyManager();
-  POLICY_LIB_CHECK_OR_RETURN(policy_manager, group_permissions);
+  POLICY_LIB_CHECK_OR_RETURN(policy_manager, false);
 
   // Single app only
   ApplicationSharedPtr app = application_manager_.application(connection_key);
@@ -905,7 +907,7 @@ std::vector<FunctionalGroupPermission> PolicyHandler::CollectAppPermissions(
                  << "' "
                     "not found within registered applications.");
 
-    return group_permissions;
+    return false;
   }
 
   DeviceParams device_params = GetDeviceParams(
@@ -914,14 +916,13 @@ std::vector<FunctionalGroupPermission> PolicyHandler::CollectAppPermissions(
 
   if (device_params.device_mac_address.empty()) {
     SDL_LOG_WARN("Couldn't find device, which hosts application.");
-    return group_permissions;
+    return false;
   }
 
-  policy_manager->GetUserConsentForApp(device_params.device_mac_address,
-                                       app->policy_app_id(),
-                                       group_permissions);
+  policy_manager->GetUserConsentForApp(
+      device_params.device_mac_address, app->policy_app_id(), out_permissions);
 
-  return group_permissions;
+  return true;
 }
 
 void PolicyHandler::OnGetListOfPermissions(const uint32_t connection_key,
@@ -935,15 +936,12 @@ void PolicyHandler::OnGetListOfPermissions(const uint32_t connection_key,
   const bool is_app_registered = NULL != app.get();
   const bool is_connection_key_valid = is_app_registered && connection_key;
 
-  const std::vector<policy::FunctionalGroupPermission> permissions =
-      is_connection_key_valid ? CollectAppPermissions(connection_key)
-                              : CollectRegisteredAppsPermissions();
+  std::vector<policy::FunctionalGroupPermission> permissions;
 
-  if (permissions.empty() && is_connection_key_valid) {
-    SDL_LOG_ERROR("No permissions found for application with connection key:"
-                  << connection_key);
-    return;
-  }
+  const bool collect_result =
+      is_connection_key_valid
+          ? CollectAppPermissions(connection_key, permissions)
+          : CollectRegisteredAppsPermissions(permissions);
 
   MessageHelper::SendGetListOfPermissionsResponse(
       permissions,
@@ -951,7 +949,14 @@ void PolicyHandler::OnGetListOfPermissions(const uint32_t connection_key,
       policy_manager->GetExternalConsentStatus(),
 #endif  // EXTERNAL_PROPRIETARY_MODE
       correlation_id,
-      application_manager_);
+      application_manager_,
+      collect_result);
+
+  if (!collect_result) {
+    SDL_LOG_ERROR(
+        "Permissions collection failed for application with connection key:"
+        << connection_key);
+  }
 }
 
 void PolicyHandler::LinkAppsToDevice() {
diff --git a/src/components/application_manager/test/include/application_manager/mock_message_helper.h b/src/components/application_manager/test/include/application_manager/mock_message_helper.h
index 89b2d125e5..dd84ce4c40 100644
--- a/src/components/application_manager/test/include/application_manager/mock_message_helper.h
+++ b/src/components/application_manager/test/include/application_manager/mock_message_helper.h
@@ -139,18 +139,20 @@ class MockMessageHelper {
                void(const std::string& file_name,
                     ApplicationManager& app_mngr));
 #ifdef EXTERNAL_PROPRIETARY_MODE
-  MOCK_METHOD4(
+  MOCK_METHOD5(
       SendGetListOfPermissionsResponse,
       void(const std::vector<policy::FunctionalGroupPermission>& permissions,
            const policy::ExternalConsentStatus& external_consent_status,
            uint32_t correlation_id,
-           ApplicationManager& app_mngr));
+           ApplicationManager& app_mngr,
+           const bool success_flag));
 #else
-  MOCK_METHOD3(
+  MOCK_METHOD4(
       SendGetListOfPermissionsResponse,
       void(const std::vector<policy::FunctionalGroupPermission>& permissions,
            uint32_t correlation_id,
-           ApplicationManager& app_mngr));
+           ApplicationManager& app_mngr,
+           const bool success_flag));
 #endif  // #ifdef EXTERNAL_PROPRIETARY_MODE
   MOCK_METHOD4(SendOnPermissionsChangeNotification,
                void(uint32_t connection_key,
diff --git a/src/components/application_manager/test/message_helper/message_helper_test.cc b/src/components/application_manager/test/message_helper/message_helper_test.cc
index f6ee2b654a..d2dc8f3528 100644
--- a/src/components/application_manager/test/message_helper/message_helper_test.cc
+++ b/src/components/application_manager/test/message_helper/message_helper_test.cc
@@ -963,7 +963,8 @@ TEST_F(MessageHelperTest, SendGetListOfPermissionsResponse_SUCCESS) {
   MessageHelper::SendGetListOfPermissionsResponse(permissions,
                                                   external_consent_status,
                                                   correlation_id,
-                                                  mock_application_manager_);
+                                                  mock_application_manager_,
+                                                  true);
 
   ASSERT_TRUE(result.get());
 
@@ -1003,7 +1004,8 @@ TEST_F(MessageHelperTest,
   MessageHelper::SendGetListOfPermissionsResponse(permissions,
                                                   external_consent_status,
                                                   correlation_id,
-                                                  mock_application_manager_);
+                                                  mock_application_manager_,
+                                                  true);
 
   ASSERT_TRUE(result.get());
 
diff --git a/src/components/application_manager/test/mock_message_helper.cc b/src/components/application_manager/test/mock_message_helper.cc
index 554b99ff72..f96a0eca5c 100644
--- a/src/components/application_manager/test/mock_message_helper.cc
+++ b/src/components/application_manager/test/mock_message_helper.cc
@@ -170,14 +170,16 @@ void MessageHelper::SendGetListOfPermissionsResponse(
     const policy::ExternalConsentStatus& external_consent_status,
 #endif  // EXTERNAL_PROPRIETARY_MODE
     uint32_t correlation_id,
-    ApplicationManager& app_mngr) {
+    ApplicationManager& app_mngr,
+    const bool success_flag) {
   MockMessageHelper::message_helper_mock()->SendGetListOfPermissionsResponse(
       permissions,
 #ifdef EXTERNAL_PROPRIETARY_MODE
       external_consent_status,
 #endif  // EXTERNAL_PROPRIETARY_MODE
       correlation_id,
-      app_mngr);
+      app_mngr,
+      success_flag);
 }
 
 void MessageHelper::SendOnPermissionsChangeNotification(
diff --git a/src/components/application_manager/test/policy_handler_test.cc b/src/components/application_manager/test/policy_handler_test.cc
index 1d764526b9..1836e2e87f 100644
--- a/src/components/application_manager/test/policy_handler_test.cc
+++ b/src/components/application_manager/test/policy_handler_test.cc
@@ -292,6 +292,10 @@ class PolicyHandlerTest : public ::testing::Test {
   }
 };
 
+ACTION_P(SetDeviceParamsMacAdress, mac_adress) {
+  *arg3 = mac_adress;
+}
+
 TEST_F(PolicyHandlerTest, LoadPolicyLibrary_Method_ExpectLibraryLoaded) {
   // Check before policy enabled from ini file
   EXPECT_CALL(policy_settings_, enable_policy()).WillRepeatedly(Return(false));
@@ -1583,6 +1587,56 @@ TEST_F(PolicyHandlerTest, OnGetListOfPermissions) {
               GetDataOnDeviceID(
                   testing::An<transport_manager::DeviceHandle>(), _, _, _, _));
 
+#ifdef EXTERNAL_PROPRIETARY_MODE
+  policy::ExternalConsentStatus external_consent_status =
+      policy::ExternalConsentStatus();
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(
+                  _, external_consent_status, corr_id, _, true));
+  EXPECT_CALL(*mock_policy_manager_, GetExternalConsentStatus())
+      .WillOnce(Return(external_consent_status));
+#else
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(_, corr_id, _, true));
+#endif  // #ifdef EXTERNAL_PROPRIETARY_MODE
+
+  policy_handler_.OnGetListOfPermissions(app_id, corr_id);
+}
+
+TEST_F(PolicyHandlerTest, OnGetListOfPermissions_CollectResult_false) {
+  // Arrange
+  EnablePolicyAndPolicyManagerMock();
+
+  const uint32_t app_id = 10u;
+  const uint32_t corr_id = 1u;
+  test_app.insert(mock_app_);
+
+  EXPECT_CALL(app_manager_, application(app_id))
+      .WillRepeatedly(Return(mock_app_));
+  EXPECT_CALL(conn_handler, get_session_observer())
+      .WillOnce(ReturnRef(mock_session_observer));
+  EXPECT_CALL(*mock_app_, device()).WillOnce(Return(0));
+  EXPECT_CALL(mock_session_observer,
+              GetDataOnDeviceID(
+                  testing::An<transport_manager::DeviceHandle>(), _, _, _, _))
+      .WillRepeatedly(
+          DoAll(SetDeviceParamsMacAdress(std::string()), (Return(1u))));
+
+#ifdef EXTERNAL_PROPRIETARY_MODE
+  policy::ExternalConsentStatus external_consent_status =
+      policy::ExternalConsentStatus();
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(
+                  _, external_consent_status, corr_id, _, false))
+      .WillOnce(Return());
+  EXPECT_CALL(*mock_policy_manager_, GetExternalConsentStatus())
+      .WillOnce(Return(external_consent_status));
+#else
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(_, corr_id, _, false))
+      .WillOnce(Return());
+#endif  // #ifdef EXTERNAL_PROPRIETARY_MODE
+
   policy_handler_.OnGetListOfPermissions(app_id, corr_id);
 }
 
@@ -1614,14 +1668,14 @@ TEST_F(PolicyHandlerTest, OnGetListOfPermissions_WithoutConnectionKey) {
 #ifdef EXTERNAL_PROPRIETARY_MODE
   policy::ExternalConsentStatus external_consent_status =
       policy::ExternalConsentStatus();
-  EXPECT_CALL(
-      mock_message_helper_,
-      SendGetListOfPermissionsResponse(_, external_consent_status, corr_id, _));
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(
+                  _, external_consent_status, corr_id, _, true));
   EXPECT_CALL(*mock_policy_manager_, GetExternalConsentStatus())
       .WillOnce(Return(external_consent_status));
 #else
   EXPECT_CALL(mock_message_helper_,
-              SendGetListOfPermissionsResponse(_, corr_id, _));
+              SendGetListOfPermissionsResponse(_, corr_id, _, true));
 #endif  // #ifdef EXTERNAL_PROPRIETARY_MODE
 
   policy_handler_.OnGetListOfPermissions(app_id, corr_id);
@@ -1690,14 +1744,14 @@ TEST_F(PolicyHandlerTest, OnGetListOfPermissions_GroupPermissions_SUCCESS) {
 #ifdef EXTERNAL_PROPRIETARY_MODE
   policy::ExternalConsentStatus external_consent_status =
       policy::ExternalConsentStatus();
-  EXPECT_CALL(
-      mock_message_helper_,
-      SendGetListOfPermissionsResponse(_, external_consent_status, corr_id, _));
+  EXPECT_CALL(mock_message_helper_,
+              SendGetListOfPermissionsResponse(
+                  _, external_consent_status, corr_id, _, true));
   EXPECT_CALL(*mock_policy_manager_, GetExternalConsentStatus())
       .WillOnce(Return(external_consent_status));
 #else
   EXPECT_CALL(mock_message_helper_,
-              SendGetListOfPermissionsResponse(_, corr_id, _));
+              SendGetListOfPermissionsResponse(_, corr_id, _, true));
 #endif  // #ifdef EXTERNAL_PROPRIETARY_MODE
 
   policy_handler_.OnGetListOfPermissions(app_id, corr_id);
@@ -2318,10 +2372,6 @@ TEST_F(PolicyHandlerTest,
   EXPECT_FALSE(waiter->WaitFor(kCallsCount_, kTimeout_));
 }
 
-ACTION_P(SetDeviceParamsMacAdress, mac_adress) {
-  *arg3 = mac_adress;
-}
-
 TEST_F(PolicyHandlerTest,
        OnAppPermissionConsentInternal_ExistAppsPreviouslyStored_SUCCESS) {
   EnablePolicyAndPolicyManagerMock();
diff --git a/src/components/policy/policy_external/src/policy_manager_impl.cc b/src/components/policy/policy_external/src/policy_manager_impl.cc
index d6aa007ac1..b1e1c85490 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -1596,7 +1596,6 @@ void PolicyManagerImpl::GetPermissionsForApp(
     FillFunctionalGroupPermissions(
         all_disallowed, group_names, kGroupDisallowed, permissions);
   }
-  return;
 }
 
 std::string& PolicyManagerImpl::GetCurrentDeviceId(