Clean up partial fabric data after device is reset during CommitPendingFabricData (#37819)

* Always clean up all data in FabricTable::Delete

Always try to cleanup all fabric data regardless if fabric was
initialized, as there may be some partial data stored if a device was
reset during committing changes to storage.

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

* Test FabricTable Delete

Test if Delete invokes delegates, as this is needed to cleanup all
fabric related settings after device is reset during committing changes
to storage.

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

* Fix test

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

* Fix return value

Make Delete return the same errors as before the changes.

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

---------

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

Author: adigie

src/credentials/FabricTable.cpp

@@ -1002,39 +1002,35 @@ CHIP_ERROR FabricTable::Delete(FabricIndex fabricIndex)
         }
     }
 
-    if (!fabricIsInitialized)
+    if (fabricIsInitialized)
     {
-        // Make sure to return the error our API promises, not whatever storage
-        // chose to return.
-        return CHIP_ERROR_NOT_FOUND;
-    }
-
-    // Since fabricIsInitialized was true, fabric is not null.
-    fabricInfo->Reset();
-
-    if (!mNextAvailableFabricIndex.HasValue())
-    {
-        // We must have been in a situation where CHIP_CONFIG_MAX_FABRICS is 254
-        // and our fabric table was full, so there was no valid next index.  We
-        // have a single available index now, though; use it as
-        // mNextAvailableFabricIndex.
-        mNextAvailableFabricIndex.SetValue(fabricIndex);
-    }
-    // If StoreFabricIndexInfo fails here, that's probably OK.  When we try to
-    // read things from storage later we will realize there is nothing for this
-    // index.
-    StoreFabricIndexInfo();
+        // Since fabricIsInitialized was true, fabric is not null.
+        fabricInfo->Reset();
 
-    // If we ever start moving the FabricInfo entries around in the array on
-    // delete, we should update DeleteAllFabrics to handle that.
-    if (mFabricCount == 0)
-    {
-        ChipLogError(FabricProvisioning, "Trying to delete a fabric, but the current fabric count is already 0");
-    }
-    else
-    {
-        mFabricCount--;
-        ChipLogProgress(FabricProvisioning, "Fabric (0x%x) deleted.", static_cast<unsigned>(fabricIndex));
+        if (!mNextAvailableFabricIndex.HasValue())
+        {
+            // We must have been in a situation where CHIP_CONFIG_MAX_FABRICS is 254
+            // and our fabric table was full, so there was no valid next index.  We
+            // have a single available index now, though; use it as
+            // mNextAvailableFabricIndex.
+            mNextAvailableFabricIndex.SetValue(fabricIndex);
+        }
+        // If StoreFabricIndexInfo fails here, that's probably OK.  When we try to
+        // read things from storage later we will realize there is nothing for this
+        // index.
+        StoreFabricIndexInfo();
+
+        // If we ever start moving the FabricInfo entries around in the array on
+        // delete, we should update DeleteAllFabrics to handle that.
+        if (mFabricCount == 0)
+        {
+            ChipLogError(FabricProvisioning, "Trying to delete a fabric, but the current fabric count is already 0");
+        }
+        else
+        {
+            mFabricCount--;
+            ChipLogProgress(FabricProvisioning, "Fabric (0x%x) deleted.", static_cast<unsigned>(fabricIndex));
+        }
     }
 
     if (mDelegateListRoot != nullptr)
@@ -1050,12 +1046,17 @@ CHIP_ERROR FabricTable::Delete(FabricIndex fabricIndex)
         }
     }
 
-    // Only return error after trying really hard to remove everything we could
-    ReturnErrorOnFailure(metadataErr);
-    ReturnErrorOnFailure(opKeyErr);
-    ReturnErrorOnFailure(opCertsErr);
+    if (fabricIsInitialized)
+    {
+        // Only return error after trying really hard to remove everything we could
+        ReturnErrorOnFailure(metadataErr);
+        ReturnErrorOnFailure(opKeyErr);
+        ReturnErrorOnFailure(opCertsErr);
 
-    return CHIP_NO_ERROR;
+        return CHIP_NO_ERROR;
+    }
+
+    return CHIP_ERROR_NOT_FOUND;
 }
 
 void FabricTable::DeleteAllFabrics()

src/credentials/FabricTable.h

@@ -412,7 +412,15 @@ class DLL_EXPORT FabricTable
         No
     };
 
-    // Returns CHIP_ERROR_NOT_FOUND if there is no fabric for that index.
+    /**
+     * @brief Delete the fabric with given `fabricIndex`.
+     *
+     * @param fabricIndex - Index of fabric for deletion
+     * @retval CHIP_NO_ERROR on success
+     * @retval CHIP_ERROR_NOT_FOUND if there is no fabric for that index
+     * @retval CHIP_ERROR_INVALID_ARGUMENT if any of the arguments are invalid such as too large or out of bounds
+     * @retval other CHIP_ERROR on internal errors
+     */
     CHIP_ERROR Delete(FabricIndex fabricIndex);
     void DeleteAllFabrics();
 

src/credentials/tests/TestFabricTable.cpp

@@ -72,6 +72,16 @@ class ScopedFabricTable
         return mFabricTable.Init(initParams);
     }
 
+    CHIP_ERROR ReinitFabricTable(chip::TestPersistentStorageDelegate * storage)
+    {
+        chip::FabricTable::InitParams initParams;
+        initParams.storage             = storage;
+        initParams.operationalKeystore = &mOpKeyStore;
+        initParams.opCertStore         = &mOpCertStore;
+
+        return mFabricTable.Init(initParams);
+    }
+
     FabricTable & GetFabricTable() { return mFabricTable; }
 
 private:
@@ -2998,4 +3008,58 @@ TEST_F(TestFabricTable, TestCommitMarker)
 #endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST
 }
 
+class TestFabricTableDelegate : public FabricTable::Delegate
+{
+public:
+    void FabricWillBeRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override { willBeRemovedCalled = true; }
+
+    void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override { onRemovedCalled = true; }
+
+    bool willBeRemovedCalled = false;
+    bool onRemovedCalled     = false;
+};
+
+TEST_F(TestFabricTable, Delete)
+{
+    Credentials::TestOnlyLocalCertificateAuthority fabricCertAuthority;
+    EXPECT_TRUE(fabricCertAuthority.Init().IsSuccess());
+
+    chip::TestPersistentStorageDelegate storage;
+    ScopedFabricTable fabricTableHolder;
+    EXPECT_EQ(fabricTableHolder.Init(&storage), CHIP_NO_ERROR);
+
+    FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+    FabricId fabricId         = 1;
+    NodeId nodeId             = 10;
+
+    // Simulate AddNOC
+    uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size];
+    MutableByteSpan csrSpan{ csrBuf };
+    EXPECT_EQ(fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan), CHIP_NO_ERROR);
+
+    EXPECT_EQ(fabricCertAuthority.SetIncludeIcac(true).GenerateNocChain(fabricId, nodeId, csrSpan).GetStatus(), CHIP_NO_ERROR);
+    ByteSpan rcac = fabricCertAuthority.GetRcac();
+    ByteSpan icac = fabricCertAuthority.GetIcac();
+    ByteSpan noc  = fabricCertAuthority.GetNoc();
+
+    fabricTable.AddNewPendingTrustedRootCert(rcac);
+
+    constexpr uint16_t kVendorId = 0xFFF1u;
+    FabricIndex newFabricIndex   = kUndefinedFabricIndex;
+    EXPECT_EQ(fabricTable.AddNewPendingFabricWithOperationalKeystore(noc, icac, kVendorId, &newFabricIndex), CHIP_NO_ERROR);
+
+    // Reinitialize FabricTable to simulate device reboot
+    EXPECT_EQ(fabricTableHolder.ReinitFabricTable(&storage), CHIP_NO_ERROR);
+
+    TestFabricTableDelegate fabricDelegate;
+    fabricTable.AddFabricDelegate(&fabricDelegate);
+
+    // Check if calling Delete invokes OnFabricRemoved on delegates
+    fabricTable.Delete(newFabricIndex);
+    EXPECT_TRUE(fabricDelegate.willBeRemovedCalled);
+    EXPECT_TRUE(fabricDelegate.onRemovedCalled);
+
+    fabricTable.RemoveFabricDelegate(&fabricDelegate);
+}
+
 } // namespace