Merge pull request #2648 from JackLivio/fix/allow_user_consent_for_default_groups

Allows User Consent for Default Functional Groups

5 files changed, 49 insertions, 34 deletions

diff --git a/src/components/application_manager/src/policies/policy_handler.cc b/src/components/application_manager/src/policies/policy_handler.cc
index cbc434a485..1b64c5d7b4 100644
--- a/src/components/application_manager/src/policies/policy_handler.cc
+++ b/src/components/application_manager/src/policies/policy_handler.cc
@@ -951,6 +951,7 @@ void PolicyHandler::OnVehicleDataUpdated(
 
 void PolicyHandler::OnPendingPermissionChange(
     const std::string& policy_app_id) {
+  LOG4CXX_AUTO_TRACE(logger_);
   LOG4CXX_DEBUG(logger_,
                 "PolicyHandler::OnPendingPermissionChange for "
                     << policy_app_id);
@@ -989,8 +990,6 @@ void PolicyHandler::OnPendingPermissionChange(
       if (permissions.appPermissionsConsentNeeded) {
         MessageHelper::SendOnAppPermissionsChangedNotification(
             app->app_id(), permissions, application_manager_);
-
-        policy_manager_->RemovePendingPermissionChanges(policy_app_id);
         // "Break" statement has to be here to continue processing in case of
         // there is another "true" flag in permissions struct
         break;
@@ -1000,8 +999,6 @@ void PolicyHandler::OnPendingPermissionChange(
       if (permissions.isAppPermissionsRevoked) {
         MessageHelper::SendOnAppPermissionsChangedNotification(
             app->app_id(), permissions, application_manager_);
-
-        policy_manager_->RemovePendingPermissionChanges(policy_app_id);
       }
       break;
     }
@@ -1022,15 +1019,14 @@ void PolicyHandler::OnPendingPermissionChange(
         commands::Command::SOURCE_SDL);
 
     application_manager_.OnAppUnauthorized(app->app_id());
-
-    policy_manager_->RemovePendingPermissionChanges(policy_app_id);
   }
 
   if (permissions.requestTypeChanged || permissions.requestSubTypeChanged) {
     MessageHelper::SendOnAppPermissionsChangedNotification(
         app->app_id(), permissions, application_manager_);
-    policy_manager_->RemovePendingPermissionChanges(policy_app_id);
   }
+
+  policy_manager_->RemovePendingPermissionChanges(policy_app_id);
 }
 
 bool PolicyHandler::SendMessageToSDK(const BinaryMessage& pt_string,
diff --git a/src/components/application_manager/test/policy_handler_test.cc b/src/components/application_manager/test/policy_handler_test.cc
index 15123385a0..a1471eff36 100644
--- a/src/components/application_manager/test/policy_handler_test.cc
+++ b/src/components/application_manager/test/policy_handler_test.cc
@@ -921,7 +921,7 @@ TEST_F(PolicyHandlerTest,
   EXPECT_CALL(*mock_policy_manager_, GetAppPermissionsChanges(_))
       .WillOnce(Return(permissions));
   EXPECT_CALL(*mock_policy_manager_,
-              RemovePendingPermissionChanges(kPolicyAppId_)).Times(0);
+              RemovePendingPermissionChanges(kPolicyAppId_));
   // Act
   policy_handler_.OnPendingPermissionChange(kPolicyAppId_);
 }
diff --git a/src/components/policy/policy_external/include/policy/policy_helper.h b/src/components/policy/policy_external/include/policy/policy_helper.h
index 6f916b7cd0..e3cb509c15 100644
--- a/src/components/policy/policy_external/include/policy/policy_helper.h
+++ b/src/components/policy/policy_external/include/policy/policy_helper.h
@@ -100,7 +100,8 @@ struct CheckAppPolicy {
    * @param result Result of check of updated policy
    */
   void SetPendingPermissions(const AppPoliciesValueType& app_policy,
-                             PermissionsCheckResult result) const;
+                             PermissionsCheckResult result,
+                             AppPermissions& permissions_diff) const;
   /**
    * @brief Analyzes updated application policy whether any changes received. If
    * yes - provides appropriate result code
@@ -200,6 +201,18 @@ struct CheckAppPolicy {
    */
   bool IsRequestSubTypeChanged(const AppPoliciesValueType& app_policy) const;
 
+  /**
+   * @brief Helper function that inserts permissions into app_permissions_diff_
+   * map.
+   * udpated
+   * @param app_policy Reference to updated application policy
+   * @param permissions_diff Reference to app permissions to be inserted into
+   * map.
+   * @return void
+   */
+  void InsertPermission(const std::string& app_id,
+                        const AppPermissions& permissions_diff);
+
  private:
   PolicyManagerImpl* pm_;
   const std::shared_ptr<policy_table::Table> update_;
diff --git a/src/components/policy/policy_external/src/policy_helper.cc b/src/components/policy/policy_external/src/policy_helper.cc
index 2358c54bef..c9df969bd1 100644
--- a/src/components/policy/policy_external/src/policy_helper.cc
+++ b/src/components/policy/policy_external/src/policy_helper.cc
@@ -299,9 +299,20 @@ void CheckAppPolicy::AddResult(const std::string& app_id,
   out_results_.insert(std::make_pair(app_id, result));
 }
 
+void CheckAppPolicy::InsertPermission(const std::string& app_id,
+                                      const AppPermissions& permissions_diff) {
+  pm_->app_permissions_diff_lock_.Acquire();
+  auto result = pm_->app_permissions_diff_.insert(
+      std::make_pair(app_id, permissions_diff));
+  if (!result.second) {
+    LOG4CXX_ERROR(logger_, "App ID: " << app_id << " already exists in map.");
+  }
+  pm_->app_permissions_diff_lock_.Release();
+}
+
 bool CheckAppPolicy::operator()(const AppPoliciesValueType& app_policy) {
   const std::string app_id = app_policy.first;
-
+  AppPermissions permissions_diff(app_id);
   if (!IsKnownAppication(app_id)) {
     LOG4CXX_WARN(logger_,
                  "Application:" << app_id << " is not present in snapshot.");
@@ -309,14 +320,17 @@ bool CheckAppPolicy::operator()(const AppPoliciesValueType& app_policy) {
   }
 
   if (!IsPredefinedApp(app_policy) && IsAppRevoked(app_policy)) {
-    SetPendingPermissions(app_policy, RESULT_APP_REVOKED);
+    SetPendingPermissions(app_policy, RESULT_APP_REVOKED, permissions_diff);
     AddResult(app_id, RESULT_APP_REVOKED);
+    InsertPermission(app_id, permissions_diff);
     return true;
   }
 
   if (!IsPredefinedApp(app_policy) && !NicknamesMatch(app_policy)) {
-    SetPendingPermissions(app_policy, RESULT_NICKNAME_MISMATCH);
+    SetPendingPermissions(
+        app_policy, RESULT_NICKNAME_MISMATCH, permissions_diff);
     AddResult(app_id, RESULT_NICKNAME_MISMATCH);
+    InsertPermission(app_id, permissions_diff);
     return true;
   }
 
@@ -328,14 +342,20 @@ bool CheckAppPolicy::operator()(const AppPoliciesValueType& app_policy) {
     if (is_request_type_changed) {
       LOG4CXX_TRACE(logger_,
                     "Request types were changed for application: " << app_id);
-      SetPendingPermissions(app_policy, RESULT_REQUEST_TYPE_CHANGED);
+      SetPendingPermissions(
+          app_policy, RESULT_REQUEST_TYPE_CHANGED, permissions_diff);
       AddResult(app_id, RESULT_REQUEST_TYPE_CHANGED);
+      result =
+          (RESULT_NO_CHANGES == result) ? RESULT_REQUEST_TYPE_CHANGED : result;
     }
     if (is_request_subtype_changed) {
       LOG4CXX_TRACE(
           logger_, "Request subtypes were changed for application: " << app_id);
-      SetPendingPermissions(app_policy, RESULT_REQUEST_SUBTYPE_CHANGED);
+      SetPendingPermissions(
+          app_policy, RESULT_REQUEST_SUBTYPE_CHANGED, permissions_diff);
       AddResult(app_id, RESULT_REQUEST_SUBTYPE_CHANGED);
+      result = (RESULT_NO_CHANGES == result) ? RESULT_REQUEST_SUBTYPE_CHANGED
+                                             : result;
     }
   }
 
@@ -352,19 +372,20 @@ bool CheckAppPolicy::operator()(const AppPoliciesValueType& app_policy) {
                                               << " have been changed.");
 
   if (!IsPredefinedApp(app_policy)) {
-    SetPendingPermissions(app_policy, result);
+    SetPendingPermissions(app_policy, result, permissions_diff);
     AddResult(app_id, result);
   }
 
+  InsertPermission(app_id, permissions_diff);
   return true;
 }
 
 void policy::CheckAppPolicy::SetPendingPermissions(
     const AppPoliciesValueType& app_policy,
-    PermissionsCheckResult result) const {
+    PermissionsCheckResult result,
+    AppPermissions& permissions_diff) const {
   using namespace rpc::policy_table_interface_base;
   const std::string app_id = app_policy.first;
-  AppPermissions permissions_diff(app_id);
 
   const std::string priority =
       policy_table::EnumToJsonString(app_policy.second.priority);
@@ -421,10 +442,6 @@ void policy::CheckAppPolicy::SetPendingPermissions(
   if (need_send_priority) {
     permissions_diff.priority = priority;
   }
-
-  pm_->app_permissions_diff_lock_.Acquire();
-  pm_->app_permissions_diff_.insert(std::make_pair(app_id, permissions_diff));
-  pm_->app_permissions_diff_lock_.Release();
 }
 
 PermissionsCheckResult CheckAppPolicy::CheckPermissionsChanges(
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 95304f45a6..a603f122e2 100644
--- a/src/components/policy/policy_external/src/policy_manager_impl.cc
+++ b/src/components/policy/policy_external/src/policy_manager_impl.cc
@@ -1246,10 +1246,6 @@ void PolicyManagerImpl::GetUserConsentForApp(
   FunctionalGroupIDs preconsented_groups = group_types[kTypePreconsented];
   FunctionalGroupIDs consent_allowed_groups = group_types[kTypeAllowed];
   FunctionalGroupIDs consent_disallowed_groups = group_types[kTypeDisallowed];
-  FunctionalGroupIDs default_groups = group_types[kTypeDefault];
-  FunctionalGroupIDs predataconsented_groups =
-      group_types[kTypePreDataConsented];
-  FunctionalGroupIDs device_groups = group_types[kTypeDevice];
 
   // Sorting groups by consent
   FunctionalGroupIDs preconsented_wo_auto =
@@ -1261,15 +1257,8 @@ void PolicyManagerImpl::GetUserConsentForApp(
   FunctionalGroupIDs allowed_groups =
       Merge(consent_allowed_groups, preconsented_wo_disallowed_auto);
 
-  FunctionalGroupIDs merged_stage_1 =
-      Merge(default_groups, predataconsented_groups);
-
-  FunctionalGroupIDs merged_stage_2 = Merge(merged_stage_1, device_groups);
-
-  FunctionalGroupIDs merged_stage_3 =
-      Merge(merged_stage_2, auto_allowed_groups);
-
-  FunctionalGroupIDs excluded_stage_1 = ExcludeSame(all_groups, merged_stage_3);
+  FunctionalGroupIDs excluded_stage_1 =
+      ExcludeSame(all_groups, auto_allowed_groups);
 
   FunctionalGroupIDs excluded_stage_2 =
       ExcludeSame(excluded_stage_1, consent_disallowed_groups);