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.
swan-amazon · May 7, 2024 · c01394d · c01394d
1 parent ecb5109
commit c01394d
Show file tree

Hide file tree

Showing 7 changed files with 124 additions and 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
@@ -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
@@ -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
@@ -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
@@ -1,20 +1,78 @@
 #include "TermsAndConditionsManager.h"
 
-CHIP_ERROR chip::app::TermsAndConditionsManager::SetTCAcknowledgements(uint16_t tcVersion, uint16_t tcUserResponse)
+#include <platform/ConfigurationManager.h>
+
+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
@@ -2,13 +2,18 @@
 
 #include <stdint.h>
 
+#include <lib/core/CHIPError.h>
+
 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
@@ -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
@@ -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;