Stop storing breadcrumb in persistent storage.

Breadcrumb is not supposed to be stored persistently. Also adds a bunch of tests and fixes various bugs where we were setting breadcrumbs when we should not (e.g. in error conditions) and not setting them when we should (e.g. when the fail-safe expires). Fixes project-chip#17515
bzbarsky-apple · Apr 27, 2022 · 543e8fe · 543e8fe
1 parent 1dd1e80
commit 543e8fe
Show file tree

Hide file tree

Showing 41 changed files with 569 additions and 442 deletions.
diff --git a/examples/chip-tool-darwin/templates/tests/tests.js b/examples/chip-tool-darwin/templates/tests/tests.js
@@ -277,7 +277,8 @@ function getTests()
     'TestDescriptorCluster',
     // TestBasicInformation needs Reboot
     //'TestBasicInformation',
-    'TestGeneralCommissioning',
+    // TestGeneralCommissioning needs Reboot
+    //'TestGeneralCommissioning',
     'TestGroupsCluster',
     'TestGroupKeyManagementCluster',
     'TestIdentifyCluster',

diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp
@@ -68,7 +68,6 @@ class GeneralCommissioningAttrAccess : public AttributeAccessInterface
     GeneralCommissioningAttrAccess() : AttributeAccessInterface(Optional<EndpointId>::Missing(), GeneralCommissioning::Id) {}
 
     CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
-    CHIP_ERROR Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) override;
 
 private:
     CHIP_ERROR ReadIfSupported(CHIP_ERROR (ConfigurationManager::*getter)(uint8_t &), AttributeValueEncoder & aEncoder);
@@ -100,37 +99,13 @@ CHIP_ERROR GeneralCommissioningAttrAccess::Read(const ConcreteReadAttributePath
     case SupportsConcurrentConnection::Id: {
         return ReadSupportsConcurrentConnection(aEncoder);
     }
-    case Breadcrumb::Id: {
-        return aEncoder.Encode(DeviceLayer::DeviceControlServer::DeviceControlSvr().GetBreadcrumb());
-    }
     default: {
         break;
     }
     }
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR GeneralCommissioningAttrAccess::Write(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder)
-{
-    // TODO: There was discussion about moving the breadcrumb to the attribute store, which would make this function obsolete
-
-    if (aPath.mClusterId != GeneralCommissioning::Id)
-    {
-        // We shouldn't have been called at all.
-        return CHIP_ERROR_INVALID_ARGUMENT;
-    }
-
-    switch (aPath.mAttributeId)
-    {
-    case Attributes::Breadcrumb::Id:
-        Attributes::Breadcrumb::TypeInfo::DecodableType value;
-        ReturnErrorOnFailure(aDecoder.Decode(value));
-        return DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(value);
-    default:
-        return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
-    }
-}
-
 CHIP_ERROR GeneralCommissioningAttrAccess::ReadIfSupported(CHIP_ERROR (ConfigurationManager::*getter)(uint8_t &),
                                                            AttributeValueEncoder & aEncoder)
 {
@@ -208,6 +183,8 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
         {
             // Force the timer to expire immediately.
             failSafeContext.ForceFailSafeTimerExpiry();
+            // Don't set the breadcrumb, since expiring the failsafe should
+            // reset it anyway.
             response.errorCode = CommissioningError::kOk;
             commandObj->AddResponse(commandPath, response);
         }
@@ -216,10 +193,10 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
             CheckSuccess(
                 failSafeContext.ArmFailSafe(accessingFabricIndex, System::Clock::Seconds16(commandData.expiryLengthSeconds)),
                 Failure);
+            Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb);
             response.errorCode = CommissioningError::kOk;
             commandObj->AddResponse(commandPath, response);
         }
-        DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(commandData.breadcrumb);
     }
     else
     {
@@ -265,11 +242,11 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
             CheckSuccess(server->CommissioningComplete(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
                          Failure);
 
+            Breadcrumb::Set(commandPath.mEndpointId, 0);
             response.errorCode = CommissioningError::kOk;
         }
     }
 
-    DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(0);
     commandObj->AddResponse(commandPath, response);
 
     return true;
@@ -282,9 +259,8 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
     MATTER_TRACE_EVENT_SCOPE("SetRegulatoryConfig", "GeneralCommissioning");
     DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr();
 
-    CheckSuccess(server->SetRegulatoryConfig(to_underlying(commandData.newRegulatoryConfig), commandData.countryCode,
-                                             commandData.breadcrumb),
-                 Failure);
+    CheckSuccess(server->SetRegulatoryConfig(to_underlying(commandData.newRegulatoryConfig), commandData.countryCode), Failure);
+    Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb);
 
     Commands::SetRegulatoryConfigResponse::Type response;
     response.errorCode = CommissioningError::kOk;
@@ -293,8 +269,21 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH
     return true;
 }
 
+namespace {
+void OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)
+{
+    if (event->Type == DeviceLayer::DeviceEventType::kFailSafeTimerExpired)
+    {
+        // Spec says to reset Breadcrumb attribute to 0.
+        Breadcrumb::Set(0, 0);
+    }
+}
+
+} // anonymous namespace
+
 void MatterGeneralCommissioningPluginServerInitCallback()
 {
-    DeviceLayer::DeviceControlServer::DeviceControlSvr().SetBreadcrumb(0);
+    Breadcrumb::Set(0, 0);
     registerAttributeAccessOverride(&gAttrAccess);
+    DeviceLayer::PlatformMgrImpl().AddEventHandler(OnPlatformEventHandler);
 }
diff --git a/src/app/tests/suites/TestGeneralCommissioning.yaml b/src/app/tests/suites/TestGeneralCommissioning.yaml
@@ -18,6 +18,12 @@ config:
     nodeId: 0x12344321
     cluster: "General Commissioning"
     endpoint: 0
+    discriminator:
+        type: INT16U
+        defaultValue: 3840
+    payload:
+        type: CHAR_STRING
+        defaultValue: "MT:-24J0AFN00KA0648G00" # This value needs to be generated automatically
 
 tests:
     - label: "Wait for the commissioned device to be retrieved"
@@ -52,6 +58,214 @@ tests:
       response:
           value: 81
 
+    - label: "Reboot to reset Breadcrumb"
+      cluster: "SystemCommands"
+      command: "Reboot"
+
+    - label: "Connect to the device again"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    - label: "Read back Breadcrumb after reboot and ensure it was not persisted"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 0
+
+    - label: "Set Breadcrumb to nonzero value"
+      command: "writeAttribute"
+      attribute: "Breadcrumb"
+      arguments:
+          value: 1
+
+    - label: "Check Breadcrumb set worked"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 1
+
+    - label: "Send CommissioningComplete without armed fail-safe"
+      command: "CommissioningComplete"
+      response:
+          values:
+              - name: errorCode
+                value: 3 # NoFailSafe
+
+    - label: "Check Breadcrumb was not touched by invalid CommissioningComplete"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 1
+
+    - label: "Open Commissioning Window from alpha"
+      cluster: "AdministratorCommissioning"
+      command: "OpenBasicCommissioningWindow"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "CommissioningTimeout"
+                value: 180
+
+    # Arming a fail-safe over CASE while a commissioning window is open should not work.
+    - label: "Try to arm fail-safe"
+      command: "ArmFailSafe"
+      arguments:
+          values:
+              - name: expiryLengthSeconds
+                value: 10
+              - name: breadcrumb
+                value: 5000
+      response:
+          values:
+              - name: errorCode
+                value: 4 # BusyWithOtherAdmin
+
+    - label:
+          "Check Breadcrumb was not touched by ArmFailSafe with commissioning
+          window open"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 1
+
+    - label: "Reset Breadcrumb to 0 so we can commission"
+      command: "writeAttribute"
+      attribute: "Breadcrumb"
+      arguments:
+          value: 0
+
+    - label: "Commission from beta"
+      identity: "beta"
+      cluster: "CommissionerCommands"
+      command: "PairWithQRCode"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: 0x12345
+              - name: "payload"
+                value: payload
+
+    - label: "Wait for the commissioned device to be retrieved for beta"
+      identity: "beta"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: 0x12345
+
+    - label: "Arm fail-safe"
+      command: "ArmFailSafe"
+      arguments:
+          values:
+              - name: expiryLengthSeconds
+                value: 500
+              - name: breadcrumb
+                value: 2
+      response:
+          values:
+              - name: errorCode
+                value: 0 # OK
+
+    - label: "Check Breadcrumb was properly set by ArmFailSafe"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 2
+
+    - label: "Try to arm fail-safe from wrong fabric"
+      command: "ArmFailSafe"
+      identity: "beta"
+      arguments:
+          values:
+              - name: expiryLengthSeconds
+                value: 10
+              - name: breadcrumb
+                value: 5000
+      response:
+          values:
+              - name: errorCode
+                value: 4 # BusyWithOtherAdmin
+
+    - label:
+          "Check Breadcrumb was not touched by ArmFailSafe with existing
+          fail-safe armed"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 2
+
+    - label: "Send CommissioningComplete from wrong fabric"
+      command: "CommissioningComplete"
+      identity: "beta"
+      response:
+          values:
+              - name: errorCode
+                value: 2 # InvalidAuthentication
+
+    - label:
+          "Check Breadcrumb was not touched by CommissioningComplete from wrong
+          fabric"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 2
+
+    - label: "Close out the fail-safe gracefully"
+      command: "CommissioningComplete"
+      response:
+          values:
+              - name: errorCode
+                value: 0 # Ok
+
+    - label: "Check Breadcrumb was reset to 0 by CommissioningComplete"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 0
+
+    - label: "Arm fail-safe again"
+      command: "ArmFailSafe"
+      arguments:
+          values:
+              - name: expiryLengthSeconds
+                value: 500
+              - name: breadcrumb
+                value: 3
+      response:
+          values:
+              - name: errorCode
+                value: 0 # OK
+
+    - label: "Check Breadcrumb was set by arming fail-safe again"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 3
+
+    - label: "Force-expire the fail-safe"
+      command: "ArmFailSafe"
+      arguments:
+          values:
+              - name: expiryLengthSeconds
+                value: 0
+              - name: breadcrumb
+                value: 4
+      response:
+          values:
+              - name: errorCode
+                value: 0 # OK
+
+    - label: "Check Breadcrumb was reset by expiring the fail-safe"
+      command: "readAttribute"
+      attribute: "Breadcrumb"
+      response:
+          value: 0
+
     - label: "Validate presence of SupportsConcurrentConnection"
       command: "readAttribute"
       attribute: "SupportsConcurrentConnection"

diff --git a/src/darwin/Framework/CHIP/templates/tests/tests.js b/src/darwin/Framework/CHIP/templates/tests/tests.js
@@ -273,7 +273,9 @@ function getTests()
     'TestDescriptorCluster',
     // TestBasicInformation needs support for Reboot with no discriminator
     //'TestBasicInformation',
-    'TestGeneralCommissioning',
+    // TestGeneralCommissioning does reboots that need to have side-effects, so
+    // can't be tested in this test framework.
+    //'TestGeneralCommissioning',
     'TestGroupsCluster',
     'TestGroupKeyManagementCluster',
     'TestIdentifyCluster',