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 project-chip#15114
bzbarsky-apple · Feb 12, 2022 · 4b2fc4e · 4b2fc4e
1 parent 6a291ca
commit 4b2fc4e
Show file tree

Hide file tree

Showing 70 changed files with 500 additions and 199 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/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>

diff --git a/src/controller/data_model/controller-clusters.matter b/src/controller/data_model/controller-clusters.matter
@@ -2738,7 +2738,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;

diff --git a/src/controller/java/zap-generated/CHIPAttributeTLVValueDecoder.cpp b/src/controller/java/zap-generated/CHIPAttributeTLVValueDecoder.cpp