Fix redundant OnPermissionChanged notification (#3007)

* Fix redundant OnPermissionChanged notification

* fixup! Fix redundant OnPermissionChanged notification

6 files changed, 67 insertions, 84 deletions

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 92be87a9fc..9b9d300d78 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -506,9 +506,13 @@ bool PolicyManagerImpl::LoadPT(const std::string& file,
       return false;
     }
 
-    // Replace predefined policies with its actual setting, e.g. "123":"default"
-    // to actual values of default section
-    UnwrapAppPolicies(pt_update->policy_table.app_policies_section.apps);
+    // Checking of difference between PTU and current policy state
+    // Must to be done before PTU applying since it is possible, that functional
+    // groups, which had been present before are absent in PTU and will be
+    // removed after update. So in case of revoked groups system has to know
+    // names and ids of revoked groups before they will be removed.
+    CheckAppPolicyResults results =
+        CheckPermissionsChanges(pt_update, policy_table_snapshot);
 
     // Replace current data with updated
     if (!cache_->ApplyUpdate(*pt_update)) {
@@ -519,14 +523,6 @@ bool PolicyManagerImpl::LoadPT(const std::string& file,
       return false;
     }
 
-    // Checking of difference between PTU and current policy state
-    // Must to be done before PTU applying since it is possible, that functional
-    // groups, which had been present before are absent in PTU and will be
-    // removed after update. So in case of revoked groups system has to know
-    // names and ids of revoked groups before they will be removed.
-    CheckAppPolicyResults results =
-        CheckPermissionsChanges(pt_update, policy_table_snapshot);
-
     ExternalConsentStatus status = cache_->GetExternalConsentStatus();
     GroupsByExternalConsentStatus groups_by_status =
         cache_->GetGroupsWithSameEntities(status);
@@ -574,6 +570,10 @@ CheckAppPolicyResults PolicyManagerImpl::CheckPermissionsChanges(
     const std::shared_ptr<policy_table::Table> snapshot) {
   LOG4CXX_AUTO_TRACE(logger_);
 
+  // Replace predefined policies with its actual setting, e.g. "123":"default"
+  // to actual values of default section
+  UnwrapAppPolicies(pt_update->policy_table.app_policies_section.apps);
+
   CheckAppPolicyResults out_results;
   std::for_each(pt_update->policy_table.app_policies_section.apps.begin(),
                 pt_update->policy_table.app_policies_section.apps.end(),
diff --git a/src/components/policy/policy_regular/include/policy/policy_helper.h b/src/components/policy/policy_regular/include/policy/policy_helper.h
index 628a8c49d1..a8cec41715 100644
--- a/src/components/policy/policy_regular/include/policy/policy_helper.h
+++ b/src/components/policy/policy_regular/include/policy/policy_helper.h
@@ -106,8 +106,6 @@ struct CheckAppPolicy {
       const std::vector<FunctionalGroupPermission>& revoked_groups) const;
   bool IsKnownAppication(const std::string& application_id) const;
   void NotifySystem(const AppPoliciesValueType& app_policy) const;
-  void SendPermissionsToApp(const std::string& app_id,
-                            const policy_table::Strings& groups) const;
   bool IsAppRevoked(const AppPoliciesValueType& app_policy) const;
   bool NicknamesMatch(const AppPoliciesValueType& app_policy) const;
 
diff --git a/src/components/policy/policy_regular/include/policy/policy_manager_impl.h b/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
index c6066a9ae9..0bc311ba9f 100644
--- a/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
+++ b/src/components/policy/policy_regular/include/policy/policy_manager_impl.h
@@ -136,16 +136,20 @@ class PolicyManagerImpl : public PolicyManager {
 
   /**
    * @brief Notifies system by sending OnAppPermissionChanged notification
+   * @param device_id device identifier
    * @param app_policy Reference to application policy
    */
-  void NotifySystem(const AppPoliciesValueType& app_policy) const;
+  void NotifySystem(const std::string& device_id,
+                    const AppPoliciesValueType& app_policy) const;
 
   /**
    * @brief Sends OnPermissionChange notification to application if its
    * currently registered
+   * @param device_id device identifier
    * @param app_policy Reference to application policy
    */
-  void SendPermissionsToApp(const AppPoliciesValueType& app_policy);
+  void SendPermissionsToApp(const std::string& device_id,
+                            const AppPoliciesValueType& app_policy);
 
   /**
    * @brief Resets Policy Table
diff --git a/src/components/policy/policy_regular/src/policy_helper.cc b/src/components/policy/policy_regular/src/policy_helper.cc
index 7338e98cff..c6116584f8 100644
--- a/src/components/policy/policy_regular/src/policy_helper.cc
+++ b/src/components/policy/policy_regular/src/policy_helper.cc
@@ -350,33 +350,6 @@ void policy::CheckAppPolicy::NotifySystem(
   }
 }
 
-void CheckAppPolicy::SendPermissionsToApp(
-    const std::string& app_id, const policy_table::Strings& groups) const {
-  const auto devices_ids = pm_->listener()->GetDevicesIds(app_id);
-  if (devices_ids.empty()) {
-    LOG4CXX_WARN(logger_,
-                 "Couldn't find device info for application id: " << app_id);
-    return;
-  }
-
-  for (const auto& device_id : devices_ids) {
-    std::vector<FunctionalGroupPermission> group_permissons;
-    pm_->GetPermissionsForApp(device_id, app_id, group_permissons);
-
-    Permissions notification_data;
-    pm_->PrepareNotificationData(update_->policy_table.functional_groupings,
-                                 groups,
-                                 group_permissons,
-                                 notification_data);
-
-    LOG4CXX_INFO(logger_, "Send notification for application_id: " << app_id);
-    // Default_hmi is Ford-specific and should not be used with basic policy
-    const std::string default_hmi;
-    pm_->listener()->OnPermissionsUpdated(
-        device_id, app_id, notification_data, default_hmi);
-  }
-}
-
 bool CheckAppPolicy::IsAppRevoked(
     const AppPoliciesValueType& app_policy) const {
   LOG4CXX_AUTO_TRACE(logger_);
@@ -480,12 +453,13 @@ bool CheckAppPolicy::operator()(const AppPoliciesValueType& app_policy) {
     return true;
   }
 
+  SetPendingPermissions(app_policy, result);
   if (RESULT_CONSENT_NOT_REQUIRED != result) {
-    SetPendingPermissions(app_policy, result);
     AddResult(app_id, RESULT_CONSENT_NEEDED);
+    return true;
   }
 
-  SendPermissionsToApp(app_policy.first, app_policy.second.groups);
+  AddResult(app_id, result);
   return true;
 }
 
@@ -545,17 +519,17 @@ void policy::CheckAppPolicy::SetPendingPermissions(
 
 policy::PermissionsCheckResult policy::CheckAppPolicy::CheckPermissionsChanges(
     const policy::AppPoliciesValueType& app_policy) const {
-  bool has_revoked_groups = HasRevokedGroups(app_policy);
+  const bool has_revoked_groups = HasRevokedGroups(app_policy);
+
+  const bool has_consent_needed_groups = HasConsentNeededGroups(app_policy);
 
-  bool has_consent_needed_groups = HasConsentNeededGroups(app_policy);
+  const bool has_new_groups = HasNewGroups(app_policy);
 
-  bool has_new_groups = HasNewGroups(app_policy);
+  const bool has_updated_groups = HasUpdatedGroups(app_policy);
 
   const bool encryption_required_flag_changed =
       IsEncryptionRequiredFlagChanged(app_policy);
 
-  bool has_updated_groups = HasUpdatedGroups(app_policy);
-
   if (has_revoked_groups && has_consent_needed_groups) {
     return RESULT_PERMISSIONS_REVOKED_AND_CONSENT_NEEDED;
   } else if (has_revoked_groups) {
diff --git a/src/components/policy/policy_regular/src/policy_manager_impl.cc b/src/components/policy/policy_regular/src/policy_manager_impl.cc
index 7e3b7ccbe9..0e1cfd267a 100644
--- a/src/components/policy/policy_regular/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_regular/src/policy_manager_impl.cc
@@ -381,9 +381,13 @@ bool PolicyManagerImpl::LoadPT(const std::string& file,
       return false;
     }
 
-    // Replace predefined policies with its actual setting, e.g. "123":"default"
-    // to actual values of default section
-    UnwrapAppPolicies(pt_update->policy_table.app_policies_section.apps);
+    // Checking of difference between PTU and current policy state
+    // Must to be done before PTU applying since it is possible, that functional
+    // groups, which had been present before are absent in PTU and will be
+    // removed after update. So in case of revoked groups system has to know
+    // names and ids of revoked groups before they will be removed.
+    const auto results =
+        CheckPermissionsChanges(pt_update, policy_table_snapshot);
 
     // Replace current data with updated
     if (!cache_->ApplyUpdate(*pt_update)) {
@@ -395,14 +399,6 @@ bool PolicyManagerImpl::LoadPT(const std::string& file,
     }
     CheckPermissionsChangesAfterUpdate(*pt_update, *policy_table_snapshot);
 
-    // Checking of difference between PTU and current policy state
-    // Must to be done before PTU applying since it is possible, that functional
-    // groups, which had been present before are absent in PTU and will be
-    // removed after update. So in case of revoked groups system has to know
-    // names and ids of revoked groups before they will be removed.
-    const auto results =
-        CheckPermissionsChanges(pt_update, policy_table_snapshot);
-
     ProcessAppPolicyCheckResults(
         results, pt_update->policy_table.app_policies_section.apps);
 
@@ -459,40 +455,45 @@ void PolicyManagerImpl::ProcessActionsForAppPolicies(
       continue;
     }
 
-    if (it_actions->second.is_consent_needed) {
-      // Post-check after ExternalConsent consent changes
-      const std::string& policy_app_id = app_policy->first;
-      if (!IsConsentNeeded(last_device_id_, policy_app_id)) {
-        sync_primitives::AutoLock lock(app_permissions_diff_lock_);
+    const auto devices_ids = listener()->GetDevicesIds(app_policy->first);
+    for (const auto& device_id : devices_ids) {
+      if (it_actions->second.is_consent_needed) {
+        // Post-check after ExternalConsent consent changes
+        const std::string& policy_app_id = app_policy->first;
+        if (!IsConsentNeeded(device_id, policy_app_id)) {
+          sync_primitives::AutoLock lock(app_permissions_diff_lock_);
 
-        PendingPermissions::iterator app_id_diff =
-            app_permissions_diff_.find(policy_app_id);
+          PendingPermissions::iterator app_id_diff =
+              app_permissions_diff_.find(policy_app_id);
 
-        if (app_permissions_diff_.end() != app_id_diff) {
-          app_id_diff->second.appPermissionsConsentNeeded = false;
+          if (app_permissions_diff_.end() != app_id_diff) {
+            app_id_diff->second.appPermissionsConsentNeeded = false;
+          }
         }
       }
-    }
-    if (it_actions->second.is_notify_system) {
-      NotifySystem(*app_policy);
-    }
-    if (it_actions->second.is_send_permissions_to_app) {
-      SendPermissionsToApp(*app_policy);
+      if (it_actions->second.is_notify_system) {
+        NotifySystem(device_id, *app_policy);
+      }
+      if (it_actions->second.is_send_permissions_to_app) {
+        SendPermissionsToApp(device_id, *app_policy);
+      }
     }
   }
 }
 
 void PolicyManagerImpl::NotifySystem(
+    const std::string& device_id,
     const PolicyManagerImpl::AppPoliciesValueType& app_policy) const {
-  listener()->OnPendingPermissionChange(last_device_id_, app_policy.first);
+  listener()->OnPendingPermissionChange(device_id, app_policy.first);
 }
 
 void PolicyManagerImpl::SendPermissionsToApp(
+    const std::string& device_id,
     const PolicyManagerImpl::AppPoliciesValueType& app_policy) {
   const std::string app_id = app_policy.first;
 
   std::vector<FunctionalGroupPermission> group_permissons;
-  GetPermissionsForApp(last_device_id_, app_id, group_permissons);
+  GetPermissionsForApp(device_id, app_id, group_permissons);
 
   Permissions notification_data;
 
@@ -508,7 +509,7 @@ void PolicyManagerImpl::SendPermissionsToApp(
   std::string default_hmi;
   default_hmi = "NONE";
   listener()->OnPermissionsUpdated(
-      last_device_id_, app_id, notification_data, default_hmi);
+      device_id, app_id, notification_data, default_hmi);
 }
 
 CheckAppPolicyResults PolicyManagerImpl::CheckPermissionsChanges(
diff --git a/src/components/policy/policy_regular/test/policy_manager_impl_test.cc b/src/components/policy/policy_regular/test/policy_manager_impl_test.cc
index ce0d3f5941..273656565e 100644
--- a/src/components/policy/policy_regular/test/policy_manager_impl_test.cc
+++ b/src/components/policy/policy_regular/test/policy_manager_impl_test.cc
@@ -1013,7 +1013,8 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_removeRPC_SendUpdate) {
   // Assert
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));
   ExpectOnPermissionsUpdated();
 
@@ -1045,7 +1046,8 @@ TEST_F(PolicyManagerImplTest,
   ::policy::BinaryMessage msg(json.begin(), json.end());
 
   // Assert
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));
@@ -1079,7 +1081,8 @@ TEST_F(PolicyManagerImplTest,
   ::policy::BinaryMessage msg(json.begin(), json.end());
 
   // Assert
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));
@@ -1114,7 +1117,8 @@ TEST_F(PolicyManagerImplTest,
   ::policy::BinaryMessage msg(json.begin(), json.end());
 
   // Assert
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));
@@ -1148,7 +1152,8 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_addRPCParams_SendUpdate) {
   ::policy::BinaryMessage msg(json.begin(), json.end());
 
   // Assert
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));
@@ -1175,7 +1180,8 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_NoUpdate_DONT_SendUpdate) {
   ::policy::BinaryMessage msg(json.begin(), json.end());
 
   // Assert
-  EXPECT_CALL(*cache_manager, GenerateSnapshot()).WillOnce(Return(snapshot));
+  EXPECT_CALL(*cache_manager, GenerateSnapshot())
+      .WillRepeatedly(Return(snapshot));
   EXPECT_CALL(*cache_manager, GetVehicleDataItems())
       .WillOnce(Return(std::vector<policy_table::VehicleDataItem>()));
   EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true));