Add some additional FabricTable unit tests (#20534)

* Add some additional FabricTable unit tests

- Some operations have been manually and integration-tested, but
  there are still FabricTable tests with TODO

- This PR implements the following FabricTable unit test
  - TestFabricLabelChange
  - TestAddNocRootCollision
- During development of unit tests, the following fixes were done:
  - Add a `GetFabricLabel(FabricIndex, CharSpan)` method to be symmetrical
    with `SetFabricLabel(FabricIndex, CharSpan)`
  - Fix the error code returned by CommitPendingFabric when a root is
    added, but neither Add was never done
  - Better document the existing methods

Testing done:

- Added unit tests
- All unit tests pass
- Integration tests still pass

* Remove needless comment line

* Address review comment

* Improve trusted-root-only case and metadata deletion logging

src/credentials/FabricTable.cpp

@@ -226,8 +226,18 @@ CHIP_ERROR FabricTable::DeleteMetadataFromStorage(FabricIndex fabricIndex)
 
     if (deleteErr != CHIP_NO_ERROR)
     {
-        ChipLogError(FabricProvisioning, "Error deleting part of fabric %d: %" CHIP_ERROR_FORMAT, fabricIndex, deleteErr.Format());
+        if (deleteErr == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
+        {
+            ChipLogError(FabricProvisioning, "Warning: metadata not found during delete of fabric 0x%x",
+                         static_cast<unsigned>(fabricIndex));
+        }
+        else
+        {
+            ChipLogError(FabricProvisioning, "Error deleting metadata for fabric fabric 0x%x: %" CHIP_ERROR_FORMAT,
+                         static_cast<unsigned>(fabricIndex), deleteErr.Format());
+        }
     }
+
     return deleteErr;
 }
 
@@ -1733,6 +1743,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
     bool isAdding                = mStateFlags.Has(StateFlags::kIsAddPending);
     bool isUpdating              = mStateFlags.Has(StateFlags::kIsUpdatePending);
     bool hasPending              = mStateFlags.Has(StateFlags::kIsPendingFabricDataPresent);
+    bool onlyHaveNewTrustedRoot  = hasPending && haveNewTrustedRoot && !(isAdding || isUpdating);
     bool hasInvalidInternalState = hasPending && (!IsValidFabricIndex(mFabricIndexWithPendingState) || !(isAdding || isUpdating));
 
     FabricIndex fabricIndexBeingCommitted = mFabricIndexWithPendingState;
@@ -1783,20 +1794,20 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
 
     // If there was nothing pending, we are either in a completely OK state, or weird internally inconsistent
     // state. In either case, let's clear all pending state anyway, in case it was partially stale!
-    if (!hasPending || hasInvalidInternalState)
+    if (!hasPending || onlyHaveNewTrustedRoot || hasInvalidInternalState)
     {
         CHIP_ERROR err = CHIP_NO_ERROR;
-        if (hasInvalidInternalState)
-        {
-            ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!");
-            err = CHIP_ERROR_INTERNAL;
-        }
-        else if (haveNewTrustedRoot)
+
+        if (onlyHaveNewTrustedRoot)
         {
             ChipLogError(FabricProvisioning,
                          "Failed to commit: tried to commit with only a new trusted root cert. No data committed.");
-            hasInvalidInternalState = true;
-            err                     = CHIP_ERROR_INCORRECT_STATE;
+            err = CHIP_ERROR_INCORRECT_STATE;
+        }
+        else if (hasInvalidInternalState)
+        {
+            ChipLogError(FabricProvisioning, "Failed to commit: internally inconsistent state!");
+            err = CHIP_ERROR_INTERNAL;
         }
         else
         {
@@ -2005,13 +2016,13 @@ void FabricTable::RevertPendingOpCertsExceptRoot()
 CHIP_ERROR FabricTable::SetFabricLabel(FabricIndex fabricIndex, const CharSpan & fabricLabel)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_FABRIC_INDEX);
 
     ReturnErrorCodeIf(fabricLabel.size() > kFabricLabelMaxLengthInBytes, CHIP_ERROR_INVALID_ARGUMENT);
 
     FabricInfo * fabricInfo  = GetMutableFabricByIndex(fabricIndex);
     bool fabricIsInitialized = (fabricInfo != nullptr) && fabricInfo->IsInitialized();
-    VerifyOrReturnError(fabricIsInitialized, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(fabricIsInitialized, CHIP_ERROR_INVALID_FABRIC_INDEX);
 
     // Update fabric table current in-memory entry, whether pending or not
     ReturnErrorOnFailure(fabricInfo->SetFabricLabel(fabricLabel));
@@ -2025,4 +2036,13 @@ CHIP_ERROR FabricTable::SetFabricLabel(FabricIndex fabricIndex, const CharSpan &
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR FabricTable::GetFabricLabel(FabricIndex fabricIndex, CharSpan & outFabricLabel)
+{
+    const FabricInfo * fabricInfo = FindFabricWithIndex(fabricIndex);
+    VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_INVALID_FABRIC_INDEX);
+
+    outFabricLabel = fabricInfo->GetFabricLabel();
+    return CHIP_NO_ERROR;
+}
+
 } // namespace chip

src/credentials/FabricTable.h

@@ -435,12 +435,37 @@ class DLL_EXPORT FabricTable
     CHIP_ERROR AddFabricDelegate(FabricTable::Delegate * delegate);
     void RemoveFabricDelegate(FabricTable::Delegate * delegate);
 
-    // Set the Fabric Label for the given fabricIndex. If a fabric add/update is pending,
-    // only the pending version will be updated, so that on fail-safe expiry, you would
-    // actually see the only fabric label if Update fails. If the fabric label is
-    // set before UpdateNOC, then the change is immediate.
+    /**
+     * @brief Set the Fabric Label for the fabric referred by `fabricIndex`.
+     *
+     * If a fabric add/update is pending, only the pending version will be updated,
+     * so that on fail-safe expiry, you would actually see the only fabric label if
+     * Update fails. If the fabric label is set before UpdateNOC, then the change is immediate.
+     *
+     * @param fabricIndex - Fabric Index for which to set the label
+     * @param fabricLabel - Label to set on the fabric
+     * @retval CHIP_NO_ERROR on success
+     * @retval CHIP_ERROR_INVALID_FABRIC_INDEX if fabricIndex does not refer to an fabric in the table
+     * @retval CHIP_ERROR_INVALID_ARGUMENT on fabric label error (e.g. too large)
+     * @retval other CHIP_ERROR on internal errors
+     */
     CHIP_ERROR SetFabricLabel(FabricIndex fabricIndex, const CharSpan & fabricLabel);
 
+    /**
+     * @brief Get the Fabric Label for a given fabric
+     *
+     * NOTE: The outFabricLabel argument points to internal memory of the fabric info.
+     *       It may become invalid on the next FabricTable API call due to shadow
+     *       storage of data.
+     *
+     * @param fabricIndex - Fabric index for which to get the label
+     * @param outFabricLabel - char span that will be set to the label value
+     * @retval CHIP_NO_ERROR on success
+     * @retval CHIP_ERROR_INVALID_FABRIC_INDEX on error
+     * @retval other CHIP_ERROR on internal errors
+     */
+    CHIP_ERROR GetFabricLabel(FabricIndex fabricIndex, CharSpan & outFabricLabel);
+
     /**
      * Get the current Last Known Good Time.
      *
@@ -692,10 +717,11 @@ class DLL_EXPORT FabricTable
      * @param vendorId - VendorID to use for the new fabric
      * @param outNewFabricIndex - Pointer where the new fabric index for the fabric just added will be set. Cannot be nullptr.
      *
-     * @retval CHIP_NO_ERROR on success
-     * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order
-     * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending
+     * @retval CHIP_NO_ERROR on success.
+     * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order.
+     * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending.
      * @retval CHIP_ERROR_INVALID_ARGUMENT if any of the arguments are invalid such as too large or out of bounds.
+     * @retval CHIP_ERROR_FABRIC_EXISTS if operational identity collides with one already present.
      * @retval other CHIP_ERROR_* on internal errors or certificate validation errors.
      */
     CHIP_ERROR AddNewPendingFabricWithOperationalKeystore(const ByteSpan & noc, const ByteSpan & icac, uint16_t vendorId,
@@ -725,10 +751,11 @@ class DLL_EXPORT FabricTable
      *                                         copied using P256Keypair::Serialize/Deserialize and owned in heap of a FabricInfo.
      * @param outNewFabricIndex - Pointer where the new fabric index for the fabric just added will be set. Cannot be nullptr.
      *
-     * @retval CHIP_NO_ERROR on success
-     * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order
-     * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending
+     * @retval CHIP_NO_ERROR on success.
+     * @retval CHIP_ERROR_INCORRECT_STATE if this is called in an inconsistent order.
+     * @retval CHIP_ERROR_NO_MEMORY if there is insufficient memory to make the fabric pending.
      * @retval CHIP_ERROR_INVALID_ARGUMENT if any of the arguments are invalid such as too large or out of bounds.
+     * @retval CHIP_ERROR_FABRIC_EXISTS if operational identity collides with one already present.
      * @retval other CHIP_ERROR_* on internal errors or certificate validation errors.
      */
     CHIP_ERROR AddNewPendingFabricWithProvidedOpKey(const ByteSpan & noc, const ByteSpan & icac, uint16_t vendorId,