From c01394d04575adb09fc0d8137d843bf49a172b11 Mon Sep 17 00:00:00 2001 From: James Swan <122404367+swan-amazon@users.noreply.github.com> Date: Tue, 7 May 2024 18:32:57 +0000 Subject: [PATCH] Terms and Conditions functional check during CompleteCommissioning This update improves the robustness of the commissioning process by enforcing the acceptance of required terms and conditions before commissioning can be completed, addressing potential legal and compliance issues. - Refactored the GeneralCommissioningServer to use a more robust method for checking terms and conditions acceptance before commissioning is considered complete. - Added methods for retrieving and setting terms and conditions versions and acknowledgments. - Updated the handling logic to check both the acknowledged versions against the minimum required versions and whether all required acknowledgments have been met before proceeding. - Introduced a TermsAndConditionsManager class that centralizes the management of terms and conditions related data. - Simplified related methods in DeviceControlServer to use the new manager class, ensuring all terms and conditions related operations are consistent and centralized. --- .../general-commissioning-server.cpp | 92 ++++++++++--------- src/app/server/BUILD.gn | 4 +- src/app/server/Server.h | 2 +- src/app/server/TermsAndConditionsManager.cpp | 68 +++++++++++++- src/app/server/TermsAndConditionsManager.h | 7 +- .../GenericConfigurationManagerImpl.h | 2 +- src/platform/DeviceControlServer.cpp | 15 --- 7 files changed, 124 insertions(+), 66 deletions(-) diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp index e9ece1fbf18927..e5d94e9e8f137b 100644 --- a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp +++ b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp @@ -1,6 +1,6 @@ /** * - * Copyright (c) 2021 Project CHIP Authors + * Copyright (c) 2021-2024 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -255,53 +255,60 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback( response.errorCode = CommissioningErrorEnum::kInvalidAuthentication; ChipLogError(FailSafe, "GeneralCommissioning: Got commissioning complete in invalid security context"); } - else if (!termsAndConditionsManager.HasRequiredTermsAndConditionsBeenAcknowledged()) - { - ChipLogError(AppServer, "Required terms and conditions have not been accepted"); - - /* - * Pass fabric of commissioner to DeviceControlSvr. - * This allows device to send messages back to commissioner. - * Once bindings are implemented, this may no longer be needed. - */ - failSafe.DisarmFailSafe(); - CheckSuccess( - devCtrl->PostCommissioningCompleteEvent(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()), - Failure); - - Breadcrumb::Set(commandPath.mEndpointId, 0); - response.errorCode = CommissioningErrorEnum::kRequiredTCNotAccepted; - } else { - if (failSafe.NocCommandHasBeenInvoked()) + CHIP_ERROR err; + + // Fetch the terms and conditions acknowledgements for verification + uint16_t tcAcceptedVersion = 0U, tcMinRequiredVersion = 0U, tcAcknowledgements = 0U, tcAcknowledgementsRequired = 0U; + + err = termsAndConditionsManager.GetTCAcceptedVersion(tcAcceptedVersion); + CheckSuccess(err, Failure); + + err = termsAndConditionsManager.GetTCMinRequiredVersion(tcMinRequiredVersion); + CheckSuccess(err, Failure); + + err = termsAndConditionsManager.GetTCAcknowledgements(tcAcknowledgements); + CheckSuccess(err, Failure); + + err = termsAndConditionsManager.GetTCAcknowledgementsRequired(tcAcknowledgementsRequired); + CheckSuccess(err, Failure); + + if (tcAcknowledgementsRequired != (tcAcknowledgementsRequired & tcAcknowledgements)) { - CHIP_ERROR err = fabricTable.CommitPendingFabricData(); - if (err != CHIP_NO_ERROR) - { - // No need to revert on error: CommitPendingFabricData always reverts if not fully successful. - ChipLogError(FailSafe, "GeneralCommissioning: Failed to commit pending fabric data: %" CHIP_ERROR_FORMAT, - err.Format()); - } - else + ChipLogError(AppServer, "Required terms and conditions have not been accepted"); + Breadcrumb::Set(commandPath.mEndpointId, 0); + response.errorCode = CommissioningErrorEnum::kRequiredTCNotAccepted; + } + + else if (tcAcceptedVersion < tcMinRequiredVersion) + { + ChipLogError(AppServer, "Minimium terms and conditions version has not been accepted"); + Breadcrumb::Set(commandPath.mEndpointId, 0); + response.errorCode = CommissioningErrorEnum::kTCMinVersionNotMet; + } + + else + { + if (failSafe.NocCommandHasBeenInvoked()) { + err = fabricTable.CommitPendingFabricData(); + CheckSuccess(err, Failure); ChipLogProgress(FailSafe, "GeneralCommissioning: Successfully commited pending fabric data"); } - CheckSuccess(err, Failure); - } - /* - * Pass fabric of commissioner to DeviceControlSvr. - * This allows device to send messages back to commissioner. - * Once bindings are implemented, this may no longer be needed. - */ - failSafe.DisarmFailSafe(); - CheckSuccess( - devCtrl->PostCommissioningCompleteEvent(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()), - Failure); + /* + * Pass fabric of commissioner to DeviceControlSvr. + * This allows device to send messages back to commissioner. + * Once bindings are implemented, this may no longer be needed. + */ + failSafe.DisarmFailSafe(); + err = devCtrl->PostCommissioningCompleteEvent(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()); + CheckSuccess(err, Failure); - Breadcrumb::Set(commandPath.mEndpointId, 0); - response.errorCode = CommissioningErrorEnum::kOk; + Breadcrumb::Set(commandPath.mEndpointId, 0); + response.errorCode = CommissioningErrorEnum::kOk; + } } } @@ -368,10 +375,11 @@ bool emberAfGeneralCommissioningClusterSetTCAcknowledgementsCallback( const chip::app::Clusters::GeneralCommissioning::Commands::SetTCAcknowledgements::DecodableType & commandData) { MATTER_TRACE_SCOPE("SetTCAcknowledgements", "GeneralCommissioning"); - DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr(); + auto & termsAndConditionsManager = Server::GetInstance().GetTermsAndConditionsManager(); + Commands::SetTCAcknowledgementsResponse::Type response; - CheckSuccess(server->SetTCAcknowledgements(commandData.TCVersion, commandData.TCUserResponse), Failure); + CheckSuccess(termsAndConditionsManager.SetTCAcknowledgements(commandData.TCVersion, commandData.TCUserResponse), Failure); response.errorCode = CommissioningErrorEnum::kOk; commandObj->AddResponse(commandPath, response); diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn index 7c661464bbaea3..268201b12b1725 100644 --- a/src/app/server/BUILD.gn +++ b/src/app/server/BUILD.gn @@ -1,4 +1,4 @@ -# Copyright (c) 2020 Project CHIP Authors +# Copyright (c) 2020-2024 Project CHIP Authors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -44,6 +44,8 @@ static_library("server") { "OnboardingCodesUtil.h", "Server.cpp", "Server.h", + "TermsAndConditionsManager.cpp", + "TermsAndConditionsManager.h", ] public_configs = [ ":server_config" ] diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 3685538f0489ac..f672a85f74e8e8 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -358,7 +358,7 @@ class Server app::FailSafeContext & GetFailSafeContext() { return mFailSafeContext; } - app::TermsAndConditionsManager & GetTermsAndConditionsManager() const { return mTermsAndConditionsManager; } + app::TermsAndConditionsManager & GetTermsAndConditionsManager() { return mTermsAndConditionsManager; } TestEventTriggerDelegate * GetTestEventTriggerDelegate() { return mTestEventTriggerDelegate; } diff --git a/src/app/server/TermsAndConditionsManager.cpp b/src/app/server/TermsAndConditionsManager.cpp index c80eafabac65a2..c303dfc92e3cb9 100644 --- a/src/app/server/TermsAndConditionsManager.cpp +++ b/src/app/server/TermsAndConditionsManager.cpp @@ -1,20 +1,78 @@ #include "TermsAndConditionsManager.h" -CHIP_ERROR chip::app::TermsAndConditionsManager::SetTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse) +#include + +CHIP_ERROR chip::app::TermsAndConditionsManager::GetTCAcceptedVersion(uint16_t & value) { CHIP_ERROR err = CHIP_NO_ERROR; - err = ConfigurationMgr().StoreTCAcknowledgements(tcVersion, tcUserResponse); + err = DeviceLayer::ConfigurationMgr().GetTCAcceptedVersion(value); SuccessOrExit(err); exit: if (err != CHIP_NO_ERROR) { - ChipLogError(DeviceLayer, "SetTCAcknowledgements failed with error: %s", ErrorStr(err)); + ChipLogError(DeviceLayer, "GetTCAcceptedVersion failed with error: %s", ErrorStr(err)); + } + return err; +} + +CHIP_ERROR chip::app::TermsAndConditionsManager::GetTCMinRequiredVersion(uint16_t & value) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + err = DeviceLayer::ConfigurationMgr().GetTCMinRequiredVersion(value); + SuccessOrExit(err); + +exit: + if (err != CHIP_NO_ERROR) + { + ChipLogError(DeviceLayer, "GetTCMinRequiredVersion failed with error: %s", ErrorStr(err)); } return err; } -bool chip::app::TermsAndConditionsManager::HasRequiredTermsAndConditionsBeenAcknowledged() { - return false; +CHIP_ERROR chip::app::TermsAndConditionsManager::GetTCAcknowledgements(uint16_t & value) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + err = DeviceLayer::ConfigurationMgr().GetTCAcknowledgements(value); + SuccessOrExit(err); + +exit: + if (err != CHIP_NO_ERROR) + { + ChipLogError(DeviceLayer, "GetTCAcknowledgements failed with error: %s", ErrorStr(err)); + } + return err; +} + +CHIP_ERROR chip::app::TermsAndConditionsManager::GetTCAcknowledgementsRequired(uint16_t & value) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + err = DeviceLayer::ConfigurationMgr().GetTCAcknowledgementsRequired(value); + SuccessOrExit(err); + +exit: + if (err != CHIP_NO_ERROR) + { + ChipLogError(DeviceLayer, "GetTCAcknowledgementsRequired failed with error: %s", ErrorStr(err)); + } + return err; +} + +CHIP_ERROR chip::app::TermsAndConditionsManager::SetTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + + err = DeviceLayer::ConfigurationMgr().StoreTCAcknowledgements(tcVersion, tcUserResponse); + SuccessOrExit(err); + +exit: + if (err != CHIP_NO_ERROR) + { + ChipLogError(DeviceLayer, "SetTCAcknowledgements failed with error: %s", ErrorStr(err)); + } + return err; } diff --git a/src/app/server/TermsAndConditionsManager.h b/src/app/server/TermsAndConditionsManager.h index 29b951adfd2ba3..c2459272dae5a8 100644 --- a/src/app/server/TermsAndConditionsManager.h +++ b/src/app/server/TermsAndConditionsManager.h @@ -2,13 +2,18 @@ #include +#include + namespace chip { namespace app { class TermsAndConditionsManager { public: + CHIP_ERROR GetTCAcceptedVersion(uint16_t &value); + CHIP_ERROR GetTCMinRequiredVersion(uint16_t &value); + CHIP_ERROR GetTCAcknowledgements(uint16_t &value); + CHIP_ERROR GetTCAcknowledgementsRequired(uint16_t &value); CHIP_ERROR SetTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse); - virtual bool HasRequiredTermsAndConditionsBeenAcknowledged() = 0; }; }; // namespace app diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.h b/src/include/platform/internal/GenericConfigurationManagerImpl.h index 5d438f9d822fb1..6ba54a370f0481 100644 --- a/src/include/platform/internal/GenericConfigurationManagerImpl.h +++ b/src/include/platform/internal/GenericConfigurationManagerImpl.h @@ -99,7 +99,7 @@ class GenericConfigurationManagerImpl : public ConfigurationManager CHIP_ERROR GetTCMinRequiredVersion(uint16_t &value) override; CHIP_ERROR GetTCAcknowledgements(uint16_t &value) override; CHIP_ERROR GetTCAcknowledgementsRequired(uint16_t &value) override; - CHIP_ERROR StoreTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse); + CHIP_ERROR StoreTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse) override; CHIP_ERROR GetRebootCount(uint32_t & rebootCount) override; CHIP_ERROR StoreRebootCount(uint32_t rebootCount) override; CHIP_ERROR GetTotalOperationalHours(uint32_t & totalOperationalHours) override; diff --git a/src/platform/DeviceControlServer.cpp b/src/platform/DeviceControlServer.cpp index 25960c8fe95cec..ec348669aec213 100644 --- a/src/platform/DeviceControlServer.cpp +++ b/src/platform/DeviceControlServer.cpp @@ -64,21 +64,6 @@ CHIP_ERROR DeviceControlServer::SetRegulatoryConfig(uint8_t location, const Char return err; } -CHIP_ERROR DeviceControlServer::SetTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse) -{ - CHIP_ERROR err = CHIP_NO_ERROR; - - err = ConfigurationMgr().StoreTCAcknowledgements(tcVersion, tcUserResponse); - SuccessOrExit(err); - -exit: - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "SetTCAcknowledgements failed with error: %s", ErrorStr(err)); - } - return err; -} - CHIP_ERROR DeviceControlServer::PostConnectedToOperationalNetworkEvent(ByteSpan networkID) { ChipDeviceEvent event;