Fix responses for UpdateFabricLabel to match the spec. (#15122)

* Fix responses for UpdateFabricLabel to match the spec. 1. Send a NOCResponse instead of a status response, like the spec says. 2. Implement the "check for fabric label collision" bit from the spec. 3. Update the name of the "Fabrics" attribute to match the spec. 4. Add a test for UpdateFabricLabel and reading the Fabrics attribute both before and after updating the label. Fixes #15114 * Fix handling of chip_tests_variables_has inside #each. It looks like #each breaks the .parent chain that chip_tests_variables_has relies on. Just manually hook that up.
project-chip · Jan 13, 2024 · 696e964 · 696e964
1 parent 881380c
commit 696e964
Show file tree

Hide file tree

Showing 74 changed files with 504 additions and 203 deletions.
diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter
@@ -2361,7 +2361,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/bridge-app/bridge-common/bridge-app.matter b/examples/bridge-app/bridge-common/bridge-app.matter
@@ -701,7 +701,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/chip-tool/templates/partials/test_cluster_command_value.zapt b/examples/chip-tool/templates/partials/test_cluster_command_value.zapt
@@ -18,7 +18,7 @@
       auto-free setup, so we could guarantee no name collisions. }}
   {{zapTypeToEncodableClusterObjectType type ns=ns forceNotList=true forceNotNullable=true forceNotOptional=true}} {{asLowerCamelCase label}}List_{{depth}}[{{definedValue.length}}];
   {{#each definedValue}}
-  {{>commandValue ns=../ns container=(concat (asLowerCamelCase ../label) "List_" ../depth "[" @index "]") definedValue=. type=../type depth=(incrementDepth ../depth)}}
+  {{>commandValue ns=../ns container=(concat (asLowerCamelCase ../label) "List_" ../depth "[" @index "]") definedValue=. type=../type depth=(incrementDepth ../depth) parent=../parent}}
   {{/each}}
   {{container}} = {{asLowerCamelCase label}}List_{{depth}};
   {{else}}

diff --git a/examples/chip-tool/templates/partials/test_cluster_value_equals.zapt b/examples/chip-tool/templates/partials/test_cluster_value_equals.zapt
@@ -25,7 +25,7 @@
       auto iter_{{depth}} = {{actual}}.begin();
       {{#each expected}}
         VerifyOrReturn(CheckNextListItemDecodes<decltype({{../actual}})>("{{../label}}", iter_{{../depth}}, {{@index}}));
-        {{>valueEquals label=(concat ../label "[" @index "]") actual=(concat "iter_" ../depth ".GetValue()") expected=this isArray=false type=../type chipType=../chipType depth=(incrementDepth depth)}}
+        {{>valueEquals label=(concat ../label "[" @index "]") actual=(concat "iter_" ../depth ".GetValue()") expected=this isArray=false type=../type chipType=../chipType depth=(incrementDepth depth) parent=../parent}}
       {{/each}}
       VerifyOrReturn(CheckNoMoreListItems<decltype({{actual}})>("{{label}}", iter_{{depth}}, {{expected.length}}));
     }

diff --git a/examples/door-lock-app/door-lock-common/door-lock-app.matter b/examples/door-lock-app/door-lock-common/door-lock-app.matter
@@ -1064,7 +1064,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/light-switch-app/light-switch-common/light-switch-app.matter b/examples/light-switch-app/light-switch-common/light-switch-app.matter
@@ -1052,7 +1052,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/lighting-app/lighting-common/lighting-app.matter b/examples/lighting-app/lighting-common/lighting-app.matter
@@ -1274,7 +1274,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/lock-app/lock-common/lock-app.matter b/examples/lock-app/lock-common/lock-app.matter
@@ -693,7 +693,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/log-source-app/log-source-common/log-source-app.matter b/examples/log-source-app/log-source-common/log-source-app.matter
@@ -311,7 +311,7 @@ server cluster OperationalCredentials = 62 {
     CHAR_STRING<32> label = 5;
   }
 
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter b/examples/ota-provider-app/ota-provider-common/ota-provider-app.matter
@@ -456,7 +456,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter b/examples/ota-requestor-app/ota-requestor-common/ota-requestor-app.matter
@@ -463,7 +463,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/placeholder/linux/apps/app1/config.matter b/examples/placeholder/linux/apps/app1/config.matter
@@ -586,7 +586,7 @@ client cluster OperationalCredentials = 62 {
     CHAR_STRING<32> label = 5;
   }
 
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;
@@ -668,7 +668,7 @@ server cluster OperationalCredentials = 62 {
     CHAR_STRING<32> label = 5;
   }
 
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/placeholder/linux/apps/app2/config.matter b/examples/placeholder/linux/apps/app2/config.matter
@@ -586,7 +586,7 @@ client cluster OperationalCredentials = 62 {
     CHAR_STRING<32> label = 5;
   }
 
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;
@@ -668,7 +668,7 @@ server cluster OperationalCredentials = 62 {
     CHAR_STRING<32> label = 5;
   }
 
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/pump-app/pump-common/pump-app.matter b/examples/pump-app/pump-common/pump-app.matter
@@ -612,7 +612,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter b/examples/pump-controller-app/pump-controller-common/pump-controller-app.matter
@@ -632,7 +632,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter b/examples/temperature-measurement-app/esp32/main/temperature-measurement.matter
@@ -518,7 +518,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/thermostat/thermostat-common/thermostat.matter b/examples/thermostat/thermostat-common/thermostat.matter
@@ -767,7 +767,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/tv-app/tv-common/tv-app.matter b/examples/tv-app/tv-common/tv-app.matter
@@ -1463,7 +1463,7 @@ client cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;
@@ -1561,7 +1561,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter b/examples/tv-casting-app/tv-casting-common/tv-casting-app.matter
@@ -2152,7 +2152,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/examples/window-app/common/window-app.matter b/examples/window-app/common/window-app.matter
@@ -539,7 +539,7 @@ server cluster OperationalCredentials = 62 {
   }
 
   readonly attribute NOCStruct NOCs[] = 0;
-  readonly attribute FabricDescriptor fabricsList[] = 1;
+  readonly attribute FabricDescriptor fabrics[] = 1;
   readonly attribute int8u supportedFabrics = 2;
   readonly attribute int8u commissionedFabrics = 3;
   readonly attribute OCTET_STRING trustedRootCertificates[] = 4;

diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -60,6 +60,7 @@ namespace {
 
 CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status,
                            uint8_t index, const CharSpan & debug_text);
+OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err);
 
 constexpr uint8_t kDACCertificate = 1;
 constexpr uint8_t kPAICertificate = 2;
@@ -209,7 +210,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::Read(const ConcreteReadAttributePat
     case Attributes::CommissionedFabrics::Id: {
         return ReadCommissionedFabrics(aPath.mEndpointId, aEncoder);
     }
-    case Attributes::FabricsList::Id: {
+    case Attributes::Fabrics::Id: {
         return ReadFabricsList(aPath.mEndpointId, aEncoder);
     }
     case Attributes::TrustedRootCertificates::Id: {
@@ -270,7 +271,7 @@ void fabricListChanged()
 
     // Currently, we only manage FabricsList attribute in endpoint 0, OperationalCredentials cluster is always required to be on
     // EP0.
-    MatterReportingAttributeChangeCallback(0, OperationalCredentials::Id, OperationalCredentials::Attributes::FabricsList::Id);
+    MatterReportingAttributeChangeCallback(0, OperationalCredentials::Id, OperationalCredentials::Attributes::Fabrics::Id);
     MatterReportingAttributeChangeCallback(0, OperationalCredentials::Id,
                                            OperationalCredentials::Attributes::CommissionedFabrics::Id);
 }
@@ -419,28 +420,39 @@ bool emberAfOperationalCredentialsClusterUpdateFabricLabelCallback(app::CommandH
                                                                    const app::ConcreteCommandPath & commandPath,
                                                                    const Commands::UpdateFabricLabel::DecodableType & commandData)
 {
-    auto & Label = commandData.label;
+    auto & Label        = commandData.label;
+    auto ourFabricIndex = commandObj->GetAccessingFabricIndex();
 
     emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: UpdateFabricLabel");
 
-    EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS;
+    for (auto & fabricInfo : Server::GetInstance().GetFabricTable())
+    {
+        if (fabricInfo.GetFabricLabel().data_equal(Label) && fabricInfo.GetFabricIndex() != ourFabricIndex)
+        {
+            ChipLogError(Zcl, "Fabric label already in use");
+            SendNOCResponse(commandObj, commandPath, OperationalCertStatus::kLabelConflict, ourFabricIndex, CharSpan());
+            return true;
+        }
+    }
+
     CHIP_ERROR err;
 
     // Fetch current fabric
     FabricInfo * fabric = RetrieveCurrentFabric(commandObj);
-    VerifyOrExit(fabric != nullptr, status = EMBER_ZCL_STATUS_FAILURE);
+    VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INVALID_FABRIC_ID);
 
     // Set Label on fabric
     err = fabric->SetFabricLabel(Label);
-    VerifyOrExit(err == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_FAILURE);
+    SuccessOrExit(err);
 
     // Persist updated fabric
     err = Server::GetInstance().GetFabricTable().Store(fabric->GetFabricIndex());
-    VerifyOrExit(err == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_FAILURE);
+    SuccessOrExit(err);
 
 exit:
     fabricListChanged();
-    emberAfSendImmediateDefaultResponse(status);
+
+    SendNOCResponse(commandObj, commandPath, ConvertToNOCResponseStatus(err), ourFabricIndex, CharSpan());
     return true;
 }
 

diff --git a/src/app/tests/suites/TestOperationalCredentialsCluster.yaml b/src/app/tests/suites/TestOperationalCredentialsCluster.yaml
@@ -48,31 +48,85 @@ tests:
       command: "readAttribute"
       attribute: "CurrentFabricIndex"
       response:
+          saveAs: ourFabricIndex
           constraints:
               type: uint8
               # 0 is not a valid value, but past that we have no idea what the
               # other side will claim here.
               minValue: 1
 
-    # This test is currently disabled as it breaks on Darwin.
-    # The test removes the current fabric, and Darwin test runner reuses
-    # the same pairing to run all the tests. Due to that, all subsequent
-    # tests fail.
-    - label: "Remove fabric"
-      disabled: true
+    - label: "Remove nonexistent fabric"
       command: "RemoveFabric"
       arguments:
           values:
               - name: "FabricIndex"
-                value: 1
+                value: 243 # Pretty unlikely to have that here!
+      response:
+          values:
+              - name: "StatusCode"
+                value: 11 # InvalidFabricIndex
 
-    - label: "Remove nonexistent fabric"
-      command: "RemoveFabric"
+    - label: "Read fabric list before setting label"
+      command: "readAttribute"
+      attribute: "Fabrics"
+      response:
+          value: [
+                  {
+                      "FabricIndex": ourFabricIndex,
+                      # Don't know what values to expect for the other bits here
+                      "Label": "",
+                  },
+              ]
+
+    - label: "Set the fabric label"
+      command: "UpdateFabricLabel"
       arguments:
           values:
+              - name: "Label"
+                value: "Batcave"
+      response:
+          values:
+              - name: "StatusCode"
+                value: 0 # Ok
               - name: "FabricIndex"
-                value: 243 # Pretty unlikely to have that here!
+                value: ourFabricIndex
+
+    - label: "Read fabric list after setting label"
+      command: "readAttribute"
+      attribute: "Fabrics"
+      response:
+          value: [
+                  {
+                      "FabricIndex": ourFabricIndex,
+                      # Don't know what values to expect for the other bits here
+                      "Label": "Batcave",
+                  },
+              ]
+
+    # TODO: Once we can commission the device onto a second fabric
+    # here, try setting the fabric label from there to the same value
+    # and ensure it fails.
+    - label: "Set the fabric label"
+      disabled: true
+      command: "UpdateFabricLabel"
+      identity: beta
+      arguments:
+          values:
+              - name: "Label"
+                value: "Batcave"
       response:
           values:
               - name: "StatusCode"
-                value: 11 # InvalidFabricIndex
+                value: 10 # LabelConflict
+
+    # This test is currently disabled as it breaks on Darwin.
+    # The test removes the current fabric, and Darwin test runner reuses
+    # the same pairing to run all the tests. Due to that, all subsequent
+    # tests fail.
+    - label: "Remove fabric"
+      disabled: true
+      command: "RemoveFabric"
+      arguments:
+          values:
+              - name: "FabricIndex"
+                value: 1
diff --git a/src/app/tests/suites/certification/Test_TC_DM_2_2.yaml b/src/app/tests/suites/certification/Test_TC_DM_2_2.yaml
@@ -53,7 +53,7 @@ tests:
 
     - label: "Query fabrics list"
       command: "readAttribute"
-      attribute: "fabrics list"
+      attribute: "Fabrics"
       response:
           value: [
                   {

diff --git a/src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml b/src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml
@@ -56,7 +56,7 @@ limitations under the License.
     <description>This cluster is used to add or remove Operational Credentials on a Commissionee or Node, as well as manage the associated Fabrics.</description>
 
     <attribute side="server" code="0x0000" define="NOCS" type="ARRAY" entryType="NOCStruct" writable="false" optional="false">NOCs</attribute>
-    <attribute side="server" code="0x0001" define="FABRICS" type="ARRAY" entryType="FabricDescriptor" writable="false" optional="false">fabrics list</attribute>
+    <attribute side="server" code="0x0001" define="FABRICS" type="ARRAY" entryType="FabricDescriptor" writable="false" optional="false">Fabrics</attribute>
     <attribute side="server" code="0x0002" define="SUPPORTED_FABRICS" type="INT8U" writable="false" optional="false">SupportedFabrics</attribute>
     <attribute side="server" code="0x0003" define="COMMISSIONED_FABRICS" type="INT8U" writable="false" optional="false">CommissionedFabrics</attribute>
     <attribute side="server" code="0x0004" define="TRUSTED_ROOTS" type="ARRAY" entryType="OCTET_STRING" writable="false" optional="false">TrustedRootCertificates</attribute>