Implement required check for adding an already-existing fabric. (#16404)

Per spec, AddNOC should fail if the fabric id and root public key of
the new NOC match an existing fabric.  We were not implementing this
check.

examples/chip-tool-darwin/templates/partials/test_cluster.zapt

@@ -107,7 +107,8 @@ class {{filename}}: public TestCommandBridge
           {{else}}
           {{#if (isString type)}}@"{{/if}}{{definedValue}}{{#if (isString type)}}"{{/if}}
           {{~/if~}}
-        {{/chip_tests_item_parameters}});
+        {{/chip_tests_item_parameters}}
+        {{additionalArguments}});
         {{else}}
         CHIPDevice * device = GetConnectedDevice();
         CHIPTest{{asUpperCamelCase cluster}} * cluster = [[CHIPTest{{asUpperCamelCase cluster}} alloc] initWithDevice:device endpoint:{{endpoint}} queue:mCallbackQueue];

examples/chip-tool/templates/tests/partials/test_cluster.zapt

@@ -203,7 +203,8 @@ class {{filename}}Suite: public TestCommand
           {{~else if (isString type)}}"{{definedValue}}"
           {{else}}{{definedValue}}
           {{~/if~}}
-          {{/chip_tests_item_parameters}});
+          {{/chip_tests_item_parameters}}
+          {{additionalArguments}});
     }
     {{else if isWait}}
     CHIP_ERROR {{>testCommand}}()

examples/platform/linux/LinuxCommissionableDataProvider.h

@@ -53,7 +53,7 @@ class LinuxCommissionableDataProvider : public chip::DeviceLayer::Commissionable
      *                                    PASE verifier if `serializedSpake2pVerifier` argument empty.
      * @param discriminator             - Discriminator to use for advertising.
      * @return CHIP_ERROR_OK on success, CHIP_ERROR_INVALID_ARGUMENT on any invalid argument combinations,
-     *         CHIP_ERROR_INVALID_STATE if already initialized, or other CHIP_ERROR values if inner
+     *         CHIP_ERROR_INCORRECT_STATE if already initialized, or other CHIP_ERROR values if inner
      *         implementation dependencies fail.
      */
     CHIP_ERROR Init(chip::Optional<std::vector<uint8_t>> serializedSpake2pVerifier,

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -564,6 +564,10 @@ OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err)
     {
         return OperationalCertStatus::kTableFull;
     }
+    else if (err == CHIP_ERROR_FABRIC_EXISTS)
+    {
+        return OperationalCertStatus::kFabricConflict;
+    }
 
     return OperationalCertStatus::kInvalidNOC;
 }

src/app/tests/suites/TestMultiAdmin.yaml

@@ -16,6 +16,9 @@ name: Test Multi Admin
 
 config:
     nodeId: 0x12344321
+    nodeIdForDuplicateCommissioning:
+        type: NODE_ID
+        defaultValue: 0x11
     nodeId2:
         type: NODE_ID
         defaultValue: 0xCAFE
@@ -56,6 +59,41 @@ tests:
               - name: "CommissioningTimeout"
                 value: 120
 
+    - label: "Commission from alpha again"
+      identity: "alpha"
+      cluster: "CommissionerCommands"
+      command: "PairWithQRCode"
+      additionalArguments: ", CHIP_ERROR_FABRIC_EXISTS"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeIdForDuplicateCommissioning
+              - name: "payload"
+                value: payload
+
+    - label: "Check that we just have the one fabric and did not add a new one"
+      identity: "alpha"
+      cluster: "Operational Credentials"
+      command: "readAttribute"
+      attribute: "Fabrics"
+      fabricFiltered: false
+      response:
+          value: [{ "nodeID": nodeId }]
+
+    - label: "Close Commissioning Window after failed commissioning"
+      cluster: "AdministratorCommissioning"
+      command: "RevokeCommissioning"
+      timedInteractionTimeoutMs: 10000
+
+    - label: "Open Commissioning Window from alpha again"
+      cluster: "AdministratorCommissioning"
+      command: "OpenBasicCommissioningWindow"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "CommissioningTimeout"
+                value: 120
+
     - label: "Commission from beta"
       identity: "beta"
       cluster: "CommissionerCommands"

src/app/tests/suites/commands/commissioner/CommissionerCommands.cpp

@@ -20,10 +20,12 @@
 
 constexpr uint16_t kPayloadMaxSize = 64;
 
-CHIP_ERROR CommissionerCommands::PairWithQRCode(chip::NodeId nodeId, const chip::CharSpan payload)
+CHIP_ERROR CommissionerCommands::PairWithQRCode(chip::NodeId nodeId, const chip::CharSpan payload, CHIP_ERROR expectedStatus)
 {
     VerifyOrReturnError(payload.size() > 0 && payload.size() < kPayloadMaxSize, CHIP_ERROR_INVALID_ARGUMENT);
 
+    mExpectedStatus = expectedStatus;
+
     GetCurrentCommissioner().RegisterPairingDelegate(this);
 
     char qrCode[kPayloadMaxSize];
@@ -35,6 +37,8 @@ CHIP_ERROR CommissionerCommands::PairWithManualCode(chip::NodeId nodeId, const c
 {
     VerifyOrReturnError(payload.size() > 0 && payload.size() < kPayloadMaxSize, CHIP_ERROR_INVALID_ARGUMENT);
 
+    mExpectedStatus = CHIP_NO_ERROR;
+
     GetCurrentCommissioner().RegisterPairingDelegate(this);
 
     char manualCode[kPayloadMaxSize];
@@ -44,6 +48,8 @@ CHIP_ERROR CommissionerCommands::PairWithManualCode(chip::NodeId nodeId, const c
 
 CHIP_ERROR CommissionerCommands::Unpair(chip::NodeId nodeId)
 {
+    mExpectedStatus = CHIP_NO_ERROR;
+
     return GetCurrentCommissioner().UnpairDevice(nodeId);
 }
 
@@ -71,19 +77,45 @@ void CommissionerCommands::OnPairingComplete(CHIP_ERROR err)
 
 void CommissionerCommands::OnPairingDeleted(CHIP_ERROR err)
 {
-    if (CHIP_NO_ERROR != err)
+    if (mExpectedStatus != err)
     {
-        ChipLogError(chipTool, "Pairing Delete Failure: %s", ErrorStr(err));
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(chipTool, "Pairing Delete Failure: %s", ErrorStr(err));
+        }
+        else
+        {
+            ChipLogError(chipTool, "Got success but expected: %s", ErrorStr(mExpectedStatus));
+            err = CHIP_ERROR_INCORRECT_STATE;
+        }
+    }
+    else
+    {
+        // Treat as success.
+        err = CHIP_NO_ERROR;
     }
 
     LogErrorOnFailure(ContinueOnChipMainThread(err));
 }
 
 void CommissionerCommands::OnCommissioningComplete(chip::NodeId nodeId, CHIP_ERROR err)
 {
-    if (CHIP_NO_ERROR != err)
+    if (mExpectedStatus != err)
+    {
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(chipTool, "Commissioning Complete Failure: %s", ErrorStr(err));
+        }
+        else
+        {
+            ChipLogError(chipTool, "Got success but expected: %s", ErrorStr(mExpectedStatus));
+            err = CHIP_ERROR_INCORRECT_STATE;
+        }
+    }
+    else
     {
-        ChipLogError(chipTool, "Commissioning Complete Failure: %s", ErrorStr(err));
+        // Treat as success.
+        err = CHIP_NO_ERROR;
     }
 
     LogErrorOnFailure(ContinueOnChipMainThread(err));

src/app/tests/suites/commands/commissioner/CommissionerCommands.h

@@ -30,7 +30,7 @@ class CommissionerCommands : public chip::Controller::DevicePairingDelegate
     virtual CHIP_ERROR ContinueOnChipMainThread(CHIP_ERROR err)             = 0;
     virtual chip::Controller::DeviceCommissioner & GetCurrentCommissioner() = 0;
 
-    CHIP_ERROR PairWithQRCode(chip::NodeId nodeId, const chip::CharSpan payload);
+    CHIP_ERROR PairWithQRCode(chip::NodeId nodeId, const chip::CharSpan payload, CHIP_ERROR expectedStatus = CHIP_NO_ERROR);
     CHIP_ERROR PairWithManualCode(chip::NodeId nodeId, const chip::CharSpan payload);
     CHIP_ERROR Unpair(chip::NodeId nodeId);
 
@@ -39,4 +39,7 @@ class CommissionerCommands : public chip::Controller::DevicePairingDelegate
     void OnPairingComplete(CHIP_ERROR error) override;
     void OnPairingDeleted(CHIP_ERROR error) override;
     void OnCommissioningComplete(chip::NodeId deviceId, CHIP_ERROR error) override;
+
+private:
+    CHIP_ERROR mExpectedStatus = CHIP_NO_ERROR;
 };

src/controller/CHIPDeviceController.cpp

@@ -1293,8 +1293,9 @@ CHIP_ERROR DeviceCommissioner::ConvertFromOperationalCertStatus(OperationalCrede
         return CHIP_ERROR_INCORRECT_STATE;
     case OperationalCertStatus::kTableFull:
         return CHIP_ERROR_NO_MEMORY;
-    case OperationalCertStatus::kInsufficientPrivilege:
     case OperationalCertStatus::kFabricConflict:
+        return CHIP_ERROR_FABRIC_EXISTS;
+    case OperationalCertStatus::kInsufficientPrivilege:
     case OperationalCertStatus::kLabelConflict:
         return CHIP_ERROR_INVALID_ARGUMENT;
     case OperationalCertStatus::kInvalidFabricIndex:

src/credentials/FabricTable.cpp

@@ -677,6 +677,31 @@ CHIP_ERROR FabricTable::AddNewFabric(FabricInfo & newFabric, FabricIndex * outpu
 {
     VerifyOrReturnError(outputIndex != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     static_assert(kMaxValidFabricIndex <= UINT8_MAX, "Cannot create more fabrics than UINT8_MAX");
+
+    // Check whether we already have a matching fabric.  An incoming fabric does
+    // not have its fabric id set yet, so we have to extract it here to do the
+    // comparison.
+    FabricId fabricId;
+    {
+        ByteSpan noc;
+        ReturnErrorOnFailure(newFabric.GetNOCCert(noc));
+        NodeId unused;
+        ReturnErrorOnFailure(ExtractNodeIdFabricIdFromOpCert(noc, &unused, &fabricId));
+    }
+    for (auto & existingFabric : *this)
+    {
+        if (existingFabric.GetFabricId() == fabricId)
+        {
+            P256PublicKeySpan existingRootKey, newRootKey;
+            ReturnErrorOnFailure(existingFabric.GetRootPubkey(existingRootKey));
+            ReturnErrorOnFailure(newFabric.GetRootPubkey(newRootKey));
+            if (existingRootKey.data_equal(newRootKey))
+            {
+                return CHIP_ERROR_FABRIC_EXISTS;
+            }
+        }
+    }
+
     for (FabricIndex i = mNextAvailableFabricIndex; i <= kMaxValidFabricIndex; i++)
     {
         FabricInfo * fabric = FindFabricWithIndex(i);

src/darwin/Framework/CHIP/templates/tests/partials/test_cluster.zapt

@@ -24,7 +24,7 @@ ResponseHandler {{> subscribeDataCallback}} = nil;
 
 {{#if (isTestOnlyCluster cluster)}}
     dispatch_queue_t queue = dispatch_get_main_queue();
-    {{command}}(expectation, queue{{#chip_tests_item_parameters}}, {{#if (isString type)}}@"{{/if}}{{> defined_value}}{{#if (isString type)}}"{{/if}}{{/chip_tests_item_parameters}});
+    {{command}}(expectation, queue{{#chip_tests_item_parameters}}, {{#if (isString type)}}@"{{/if}}{{> defined_value}}{{#if (isString type)}}"{{/if}}{{/chip_tests_item_parameters}}{{additionalArguments}});
 {{else}}
     CHIPDevice * device = GetConnectedDevice();
     dispatch_queue_t queue = dispatch_get_main_queue();

src/lib/core/CHIPError.cpp

@@ -410,29 +410,8 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
     case CHIP_ERROR_DRBG_ENTROPY_SOURCE_FAILED.AsInteger():
         desc = "DRBG entropy source failed to generate entropy data";
         break;
-    case CHIP_ERROR_NO_TAKE_AUTH_DELEGATE.AsInteger():
-        desc = "No TAKE auth delegate set";
-        break;
-    case CHIP_ERROR_TAKE_RECONFIGURE_REQUIRED.AsInteger():
-        desc = "TAKE requires a reconfigure";
-        break;
-    case CHIP_ERROR_TAKE_REAUTH_POSSIBLE.AsInteger():
-        desc = "TAKE can do a reauthentication";
-        break;
-    case CHIP_ERROR_INVALID_TAKE_PARAMETER.AsInteger():
-        desc = "TAKE received an invalid parameter";
-        break;
-    case CHIP_ERROR_UNSUPPORTED_TAKE_CONFIGURATION.AsInteger():
-        desc = "TAKE Unsupported configuration";
-        break;
-    case CHIP_ERROR_TAKE_TOKEN_IDENTIFICATION_FAILED.AsInteger():
-        desc = "TAKE token identification failed";
-        break;
-    case CHIP_ERROR_INVALID_TOKENPAIRINGBUNDLE.AsInteger():
-        desc = "Invalid Token Pairing Bundle";
-        break;
-    case CHIP_ERROR_UNSUPPORTED_TOKENPAIRINGBUNDLE_VERSION.AsInteger():
-        desc = "Unsupported Token Pairing Bundle version";
+    case CHIP_ERROR_FABRIC_EXISTS.AsInteger():
+        desc = "Trying to add a NOC for a fabric that already exists";
         break;
     case CHIP_ERROR_KEY_NOT_FOUND_FROM_PEER.AsInteger():
         desc = "Key not found error code received from peer";

src/lib/core/CHIPError.h

@@ -1500,77 +1500,28 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_TLV_TAG_NOT_FOUND                           CHIP_CORE_ERROR(0x76)
 
-/**
- *  @def CHIP_ERROR_INVALID_TOKENPAIRINGBUNDLE
- *
- *  @brief
- *    A token pairing bundle is invalid.
- *
- */
-#define CHIP_ERROR_INVALID_TOKENPAIRINGBUNDLE                  CHIP_CORE_ERROR(0x77)
+// unused CHIP_CORE_ERROR(0x77)
 
-/**
- *  @def CHIP_ERROR_UNSUPPORTED_TOKENPAIRINGBUNDLE_VERSION
- *
- *  @brief
- *    A token pairing bundle is invalid.
- *
- */
-#define CHIP_ERROR_UNSUPPORTED_TOKENPAIRINGBUNDLE_VERSION      CHIP_CORE_ERROR(0x78)
+// unused CHIP_CORE_ERROR(0x78)
 
-/**
- *  @def CHIP_ERROR_NO_TAKE_AUTH_DELEGATE
- *
- *  @brief
- *    No TAKE authentication delegate is set.
- *
- */
-#define CHIP_ERROR_NO_TAKE_AUTH_DELEGATE                       CHIP_CORE_ERROR(0x79)
+// unused CHIP_CORE_ERROR(0x79)
 
-/**
- *  @def CHIP_ERROR_TAKE_RECONFIGURE_REQUIRED
- *
- *  @brief
- *    TAKE requires a reconfigure.
- *
- */
-#define CHIP_ERROR_TAKE_RECONFIGURE_REQUIRED                   CHIP_CORE_ERROR(0x7a)
+// unused CHIP_CORE_ERROR(0x7a)
 
-/**
- *  @def CHIP_ERROR_TAKE_REAUTH_POSSIBLE
- *
- *  @brief
- *    TAKE can do a reauthentication.
- *
- */
-#define CHIP_ERROR_TAKE_REAUTH_POSSIBLE                        CHIP_CORE_ERROR(0x7b)
+// unused CHIP_CORE_ERROR(0x7b)
 
-/**
- *  @def CHIP_ERROR_INVALID_TAKE_PARAMETER
- *
- *  @brief
- *    Received an invalid TAKE parameter.
- *
- */
-#define CHIP_ERROR_INVALID_TAKE_PARAMETER                      CHIP_CORE_ERROR(0x7c)
+// unused CHIP_CORE_ERROR(0x7c)
 
-/**
- *  @def CHIP_ERROR_UNSUPPORTED_TAKE_CONFIGURATION
- *
- *  @brief
- *    This configuration is not supported by TAKE.
- *
- */
-#define CHIP_ERROR_UNSUPPORTED_TAKE_CONFIGURATION              CHIP_CORE_ERROR(0x7d)
+// unused CHIP_CORE_ERROR(0x7d)
 
 /**
- *  @def CHIP_ERROR_TAKE_TOKEN_IDENTIFICATION_FAILED
+ *  @def CHIP_ERROR_FABRIC_EXISTS
  *
  *  @brief
- *    The TAKE Token Identification failed.
+ *    The fabric with the given fabric id and root public key already exists.
  *
  */
-#define CHIP_ERROR_TAKE_TOKEN_IDENTIFICATION_FAILED            CHIP_CORE_ERROR(0x7e)
+#define CHIP_ERROR_FABRIC_EXISTS                               CHIP_CORE_ERROR(0x7e)
 
 /**
  *  @def CHIP_ERROR_KEY_NOT_FOUND_FROM_PEER

src/lib/core/tests/TestCHIPErrorStr.cpp

@@ -164,14 +164,7 @@ static const CHIP_ERROR kTestElements[] =
     CHIP_ERROR_INVALID_FABRIC_ID,
     CHIP_ERROR_DRBG_ENTROPY_SOURCE_FAILED,
     CHIP_ERROR_TLV_TAG_NOT_FOUND,
-    CHIP_ERROR_INVALID_TOKENPAIRINGBUNDLE,
-    CHIP_ERROR_UNSUPPORTED_TOKENPAIRINGBUNDLE_VERSION,
-    CHIP_ERROR_NO_TAKE_AUTH_DELEGATE,
-    CHIP_ERROR_TAKE_RECONFIGURE_REQUIRED,
-    CHIP_ERROR_TAKE_REAUTH_POSSIBLE,
-    CHIP_ERROR_INVALID_TAKE_PARAMETER,
-    CHIP_ERROR_UNSUPPORTED_TAKE_CONFIGURATION,
-    CHIP_ERROR_TAKE_TOKEN_IDENTIFICATION_FAILED,
+    CHIP_ERROR_FABRIC_EXISTS,
     CHIP_ERROR_KEY_NOT_FOUND_FROM_PEER,
     CHIP_ERROR_WRONG_ENCRYPTION_TYPE_FROM_PEER,
     CHIP_ERROR_UNKNOWN_KEY_TYPE_FROM_PEER,

src/lib/support/CodeUtils.h

@@ -341,7 +341,7 @@ constexpr inline const _T & max(const _T & a, const _T & b)
  *
  *  @code
  *    ReturnErrorCodeIf(state == kInitialized, CHIP_NO_ERROR);
- *    ReturnErrorCodeIf(state == kInitialized, CHIP_ERROR_INVALID_STATE);
+ *    ReturnErrorCodeIf(state == kInitialized, CHIP_ERROR_INCORRECT_STATE);
  *  @endcode
  *
  *  @param[in]  expr        A Boolean expression to be evaluated.