diff options
author | Andrii Kalinich (GitHub) <AKalinich@luxoft.com> | 2019-10-01 10:23:11 -0400 |
---|---|---|
committer | Jacob Keeler <jacob.keeler@livioradio.com> | 2019-10-01 10:23:11 -0400 |
commit | 8994b210fe5346d533c780993053fb9aa0b0d8f4 (patch) | |
tree | a803811dce9fe7efd45a7f336abf3177045c28b8 /src/components/policy/policy_regular | |
parent | 66888d419aeb753d7e323d31dc25e59ce4eab6e7 (diff) | |
download | sdl_core-8994b210fe5346d533c780993053fb9aa0b0d8f4.tar.gz |
Fix PTU notifications order (#3051)
* Fix order of actions taken during PTU
The existing order of actions were changed to
make it more obvious for external systems like
HMI and mobile apps. The main concern was that
SDL is sending OnStatusUpdate(UP_TO_DATE) when
policy table update was not actually done.
The order of actions done during PTU was updated
to perform sending of UP_TO_DATE only when all
policy actions were actually done.
Related unit tests were updated respectively.
Diffstat (limited to 'src/components/policy/policy_regular')
3 files changed, 179 insertions, 88 deletions
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 542f33794b..178de4a8f0 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 @@ -129,8 +129,10 @@ class PolicyManagerImpl : public PolicyManager { * @param pt_content PTU as binary string * @return true if successfully */ - bool LoadPT(const std::string& file, - const BinaryMessage& pt_content) OVERRIDE; + PtProcessingResult LoadPT(const std::string& file, + const BinaryMessage& pt_content) OVERRIDE; + + void OnPTUFinished(const PtProcessingResult ptu_result) OVERRIDE; typedef policy_table::ApplicationPolicies::value_type AppPoliciesValueType; @@ -152,6 +154,12 @@ class PolicyManagerImpl : public PolicyManager { const AppPoliciesValueType& app_policy); /** + * @brief Resumes all policy actions for all apps, suspended during + * PTU applying + */ + void ResumePendingAppPolicyActions(); + + /** * @brief Resets Policy Table * @param file_name Path to preloaded PT file * @return true if successfully @@ -1167,11 +1175,6 @@ class PolicyManagerImpl : public PolicyManager { RetrySequenceURL retry_sequence_url_; /** - * @brief Flag for notifying that invalid PTU was received - */ - bool wrong_ptu_update_received_; - - /** * @brief Flag for notifying that PTU was started */ bool send_on_update_sent_out_; @@ -1180,6 +1183,21 @@ class PolicyManagerImpl : public PolicyManager { * @brief Flag for notifying that invalid PTU should be triggered */ bool trigger_ptu_; + + typedef std::list<std::pair<std::string, AppPoliciesValueType> > + PendingAppPolicyActionsList; + + /** + * @brief List containing device_id and pending permissions structure pairs + * which can be processed later on demand + */ + PendingAppPolicyActionsList notify_system_list_; + + /** + * @brief List containing device_id and pending permissions structure pairs + * which can be processed later on demand + */ + PendingAppPolicyActionsList send_permissions_list_; }; } // namespace policy 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 2eaa517381..4c3f0763bc 100644 --- a/src/components/policy/policy_regular/src/policy_manager_impl.cc +++ b/src/components/policy/policy_regular/src/policy_manager_impl.cc @@ -76,7 +76,6 @@ PolicyManagerImpl::PolicyManagerImpl() this, &PolicyManagerImpl::OnPTUIterationTimeout)) , ignition_check(true) , retry_sequence_url_(0, 0, "") - , wrong_ptu_update_received_(false) , send_on_update_sent_out_(false) , trigger_ptu_(false) {} @@ -315,8 +314,8 @@ void FilterPolicyTable( } } -bool PolicyManagerImpl::LoadPT(const std::string& file, - const BinaryMessage& pt_content) { +PolicyManager::PtProcessingResult PolicyManagerImpl::LoadPT( + const std::string& file, const BinaryMessage& pt_content) { LOG4CXX_INFO(logger_, "LoadPT of size " << pt_content.size()); LOG4CXX_DEBUG( logger_, @@ -333,9 +332,9 @@ bool PolicyManagerImpl::LoadPT(const std::string& file, std::shared_ptr<policy_table::Table> pt_update = ParseArray(pt_content); #endif if (!pt_update) { - LOG4CXX_WARN(logger_, "Parsed table pointer is 0."); - update_status_manager_.OnWrongUpdateReceived(); - return false; + LOG4CXX_WARN(logger_, "Parsed table pointer is NULL."); + ; + return PtProcessingResult::kWrongPtReceived; } file_system::DeleteFile(file); @@ -344,83 +343,99 @@ bool PolicyManagerImpl::LoadPT(const std::string& file, FilterPolicyTable(pt_update->policy_table, current_vd_items); if (!IsPTValid(pt_update, policy_table::PT_UPDATE)) { - wrong_ptu_update_received_ = true; - update_status_manager_.OnWrongUpdateReceived(); - return false; + LOG4CXX_WARN(logger_, "Received policy table update is not valid"); + return PtProcessingResult::kWrongPtReceived; } - update_status_manager_.OnValidUpdateReceived(); - cache_->SaveUpdateRequired(false); - // Update finished, no need retry if (timer_retry_sequence_.is_running()) { LOG4CXX_INFO(logger_, "Stop retry sequence"); timer_retry_sequence_.Stop(); } - { - sync_primitives::AutoLock lock(apps_registration_lock_); + cache_->SaveUpdateRequired(false); - // Get current DB data, since it could be updated during awaiting of PTU - std::shared_ptr<policy_table::Table> policy_table_snapshot = - cache_->GenerateSnapshot(); - if (!policy_table_snapshot) { - LOG4CXX_ERROR( - logger_, - "Failed to create snapshot of policy table, trying another exchange"); - ForcePTExchange(); - return false; - } + sync_primitives::AutoLock lock(apps_registration_lock_); - // 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)) { - LOG4CXX_WARN( - logger_, - "Unsuccessful save of updated policy table, trying another exchange"); - ForcePTExchange(); - return false; - } - CheckPermissionsChangesAfterUpdate(*pt_update, *policy_table_snapshot); + // Get current DB data, since it could be updated during awaiting of PTU + auto policy_table_snapshot = cache_->GenerateSnapshot(); + if (!policy_table_snapshot) { + LOG4CXX_ERROR( + logger_, + "Failed to create snapshot of policy table, trying another exchange"); + return PtProcessingResult::kNewPtRequired; + } - ProcessAppPolicyCheckResults( - results, 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); - listener_->OnCertificateUpdated( - *(pt_update->policy_table.module_config.certificate)); + // Replace current data with updated + if (!cache_->ApplyUpdate(*pt_update)) { + LOG4CXX_WARN( + logger_, + "Unsuccessful save of updated policy table, trying another exchange"); + return PtProcessingResult::kNewPtRequired; + } + CheckPermissionsChangesAfterUpdate(*pt_update, *policy_table_snapshot); - std::map<std::string, StringArray> app_hmi_types; - cache_->GetHMIAppTypeAfterUpdate(app_hmi_types); - if (!app_hmi_types.empty()) { - LOG4CXX_INFO(logger_, "app_hmi_types is full calling OnUpdateHMIAppType"); - listener_->OnUpdateHMIAppType(app_hmi_types); - } else { - LOG4CXX_INFO(logger_, "app_hmi_types empty" << pt_content.size()); - } + ProcessAppPolicyCheckResults( + results, pt_update->policy_table.app_policies_section.apps); - std::vector<std::string> enabled_apps; - cache_->GetEnabledCloudApps(enabled_apps); - for (auto it = enabled_apps.begin(); it != enabled_apps.end(); ++it) { - SendAuthTokenUpdated(*it); - } + listener_->OnCertificateUpdated( + *(pt_update->policy_table.module_config.certificate)); + + std::map<std::string, StringArray> app_hmi_types; + cache_->GetHMIAppTypeAfterUpdate(app_hmi_types); + if (!app_hmi_types.empty()) { + LOG4CXX_INFO(logger_, "app_hmi_types is full calling OnUpdateHMIAppType"); + listener_->OnUpdateHMIAppType(app_hmi_types); + } else { + LOG4CXX_INFO(logger_, "app_hmi_types empty" << pt_content.size()); } + std::vector<std::string> enabled_apps; + cache_->GetEnabledCloudApps(enabled_apps); + for (auto it = enabled_apps.begin(); it != enabled_apps.end(); ++it) { + SendAuthTokenUpdated(*it); + } + + return PtProcessingResult::kSuccess; +} + +void PolicyManagerImpl::OnPTUFinished(const PtProcessingResult ptu_result) { + LOG4CXX_AUTO_TRACE(logger_); + + if (PtProcessingResult::kWrongPtReceived == ptu_result) { + LOG4CXX_DEBUG(logger_, "Wrong PT was received"); + update_status_manager_.OnWrongUpdateReceived(); + return; + } + + update_status_manager_.OnValidUpdateReceived(); + + if (PtProcessingResult::kNewPtRequired == ptu_result) { + LOG4CXX_DEBUG(logger_, "New PTU interation is required"); + ForcePTExchange(); + return; + } + + ResumePendingAppPolicyActions(); + // If there was a user request for policy table update, it should be started // right after current update is finished if (update_status_manager_.IsUpdateRequired()) { + LOG4CXX_DEBUG(logger_, + "PTU was successful and new PTU iteration was scheduled"); StartPTExchange(); - return true; + return; } RefreshRetrySequence(); - return true; } void PolicyManagerImpl::ProcessAppPolicyCheckResults( @@ -439,6 +454,9 @@ void PolicyManagerImpl::ProcessAppPolicyCheckResults( void PolicyManagerImpl::ProcessActionsForAppPolicies( const ApplicationsPoliciesActions& actions, const policy_table::ApplicationPolicies& app_policies) { + notify_system_list_.clear(); + send_permissions_list_.clear(); + ApplicationsPoliciesActions::const_iterator it_actions = actions.begin(); for (; it_actions != actions.end(); ++it_actions) { auto app_policy = app_policies.find(it_actions->first); @@ -463,15 +481,31 @@ void PolicyManagerImpl::ProcessActionsForAppPolicies( } } if (it_actions->second.is_notify_system) { - NotifySystem(device_id, *app_policy); + notify_system_list_.push_back(std::make_pair(device_id, *app_policy)); } if (it_actions->second.is_send_permissions_to_app) { - SendPermissionsToApp(device_id, *app_policy); + send_permissions_list_.push_back( + std::make_pair(device_id, *app_policy)); } } } } +void PolicyManagerImpl::ResumePendingAppPolicyActions() { + LOG4CXX_AUTO_TRACE(logger_); + + for (auto& notify_system_params : notify_system_list_) { + NotifySystem(notify_system_params.first, notify_system_params.second); + } + notify_system_list_.clear(); + + for (auto& send_permissions_params : send_permissions_list_) { + SendPermissionsToApp(send_permissions_params.first, + send_permissions_params.second); + } + send_permissions_list_.clear(); +} + void PolicyManagerImpl::NotifySystem( const std::string& device_id, const PolicyManagerImpl::AppPoliciesValueType& app_policy) const { @@ -1227,7 +1261,7 @@ void PolicyManagerImpl::StopRetrySequence() { timer_retry_sequence_.Stop(); } - if (update_status_manager_.IsUpdateRequired()) { + if (cache_->UpdateRequired()) { ResetRetrySequence(ResetRetryCountType::kResetWithStatusUpdate); } } 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 273656565e..e1f916e72f 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 @@ -314,7 +314,9 @@ class PolicyManagerImplTest2 : public ::testing::Test { ifile.close(); ::policy::BinaryMessage msg(json.begin(), json.end()); // Load Json to cache - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + EXPECT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); return root; } @@ -609,8 +611,11 @@ TEST_F(PolicyManagerImplTest2, GetNotificationsNumberAfterPTUpdate) { // Act const std::string json = table.toStyledString(); ::policy::BinaryMessage msg(json.begin(), json.end()); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + EXPECT_CALL(listener, OnUpdateStatusChanged(_)); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); std::string priority = "EMERGENCY"; uint32_t notif_number = manager->GetNotificationsNumber(priority); @@ -651,7 +656,10 @@ TEST_F(PolicyManagerImplTest2, IsAppRevoked_SetRevokedAppID_ExpectAppRevoked) { ifile.close(); ::policy::BinaryMessage msg(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); + EXPECT_TRUE(manager->IsApplicationRevoked(app_id1)); } @@ -705,7 +713,9 @@ TEST_F(PolicyManagerImplTest2, ifile.close(); ::policy::BinaryMessage msg(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); manager->CheckPermissions( dev_id1, app_id1, std::string("FULL"), "Alert", input_params, output); @@ -746,7 +756,9 @@ TEST_F( AddWidgetSupportToFunctionalGroups(&root, rpc_name, hmi_level); ::policy::BinaryMessage msg(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); ::policy::RPCParams input_params; ::policy::CheckPermissionResult output; @@ -760,7 +772,9 @@ TEST_F( // Act json = AddWidgetSupportToPt(&root, app_id1, group_number); msg = BinaryMessage(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); output.hmi_level_permitted = ::policy::kRpcDisallowed; manager->CheckPermissions( @@ -802,7 +816,9 @@ TEST_F( AddWidgetSupportToFunctionalGroups(&root, rpc_name, hmi_level); ::policy::BinaryMessage msg(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); ::policy::RPCParams input_params; ::policy::CheckPermissionResult output; @@ -821,7 +837,9 @@ TEST_F( Json::Value("Base-4"); json = root.toStyledString(); msg = BinaryMessage(json.begin(), json.end()); - ASSERT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); manager->CheckPermissions( dev_id1, app_id1, hmi_level, rpc_name, input_params, output); @@ -886,7 +904,9 @@ TEST_F(PolicyManagerImplTest2, ::policy::BinaryMessage msg(json.begin(), json.end()); // Load Json to cache - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); policy_table::RpcParameters rpc_parameters; rpc_parameters.hmi_levels.push_back(policy_table::HL_FULL); @@ -984,9 +1004,13 @@ TEST_F(PolicyManagerImplTest, LoadPT_SetPT_PTIsLoaded) { EXPECT_CALL(listener, GetDevicesIds("1234")) .WillRepeatedly(Return(transport_manager::DeviceList())); EXPECT_CALL(*cache_manager, SaveUpdateRequired(false)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + EXPECT_CALL(*cache_manager, TimeoutResponse()); EXPECT_CALL(*cache_manager, SecondsBetweenRetries(_)); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + EXPECT_CALL(listener, OnUpdateStatusChanged(_)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_removeRPC_SendUpdate) { @@ -1018,7 +1042,9 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_removeRPC_SendUpdate) { EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); ExpectOnPermissionsUpdated(); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, @@ -1053,7 +1079,9 @@ TEST_F(PolicyManagerImplTest, EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); ExpectOnPermissionsUpdated(); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, @@ -1088,7 +1116,9 @@ TEST_F(PolicyManagerImplTest, EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); ExpectOnPermissionsUpdated(); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, @@ -1124,7 +1154,9 @@ TEST_F(PolicyManagerImplTest, EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); ExpectOnPermissionsUpdated(); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_addRPCParams_SendUpdate) { @@ -1159,7 +1191,9 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_addRPCParams_SendUpdate) { EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); ExpectOnPermissionsUpdated(); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_NoUpdate_DONT_SendUpdate) { @@ -1186,7 +1220,9 @@ TEST_F(PolicyManagerImplTest, LoadPT_FunctionalGroup_NoUpdate_DONT_SendUpdate) { .WillOnce(Return(std::vector<policy_table::VehicleDataItem>())); EXPECT_CALL(*cache_manager, ApplyUpdate(_)).WillOnce(Return(true)); - EXPECT_TRUE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kSuccess, + manager->LoadPT("file_pt_update.json", msg)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kSuccess); } TEST_F(PolicyManagerImplTest, LoadPT_SetInvalidUpdatePT_PTIsNotLoaded) { @@ -1214,11 +1250,14 @@ TEST_F(PolicyManagerImplTest, LoadPT_SetInvalidUpdatePT_PTIsNotLoaded) { // Assert EXPECT_CALL(*cache_manager, ApplyUpdate(_)).Times(0); EXPECT_CALL(listener, GetAppName(_)).Times(0); - EXPECT_CALL(listener, OnUpdateStatusChanged(_)).Times(1); EXPECT_CALL(*cache_manager, SaveUpdateRequired(false)).Times(0); EXPECT_CALL(*cache_manager, TimeoutResponse()).Times(0); EXPECT_CALL(*cache_manager, SecondsBetweenRetries(_)).Times(0); - EXPECT_FALSE(manager->LoadPT("file_pt_update.json", msg)); + ASSERT_EQ(PolicyManager::PtProcessingResult::kWrongPtReceived, + manager->LoadPT("file_pt_update.json", msg)); + + EXPECT_CALL(listener, OnUpdateStatusChanged(_)); + manager->OnPTUFinished(PolicyManager::PtProcessingResult::kWrongPtReceived); } TEST_F(PolicyManagerImplTest2, |