[ICD] Improve ICDClientStorage (#36036)

* Improve ICDClientStorage -- when DeleteAllEntries is triggered, and all fabric are removed, we need remove ICDFabricList from persistent storage, if there is at least 1 fabric in table, we would update fabricList vector and persistent storage. -- when fabric does not exist, storeEntry needs to return invalid fabric error, and deleteEntry or deleteAllEntries needs to return no error. -- Add multiple unit tests to cover DeleteAllEntries/StoreEntry/CheckInHandling for multiple fabrics * Update DefaultICDClientStorage.cpp Co-authored-by: mkardous-silabs <[email protected]> * Update DefaultICDClientStorage.cpp Co-authored-by: mkardous-silabs <[email protected]> * Update DefaultICDClientStorage.cpp Co-authored-by: mkardous-silabs <[email protected]> * Update DefaultICDClientStorage.cpp Co-authored-by: mkardous-silabs <[email protected]> * Update DefaultICDClientStorage.cpp Co-authored-by: mkardous-silabs <[email protected]> * address comments * Restyled by whitespace * Restyled by clang-format * address comments * address comments and add more tests * Restyled by whitespace * Restyled by clang-format --------- Co-authored-by: mkardous-silabs <[email protected]> Co-authored-by: Restyled.io <[email protected]>
project-chip · Oct 12, 2024 · 286f03f · 286f03f
1 parent dd6f974
commit 286f03f
Show file tree

Hide file tree

Showing 6 changed files with 532 additions and 8 deletions.
diff --git a/src/app/icd/client/CheckInHandler.cpp b/src/app/icd/client/CheckInHandler.cpp
@@ -52,8 +52,12 @@ CHIP_ERROR CheckInHandler::Init(Messaging::ExchangeManager * exchangeManager, IC
 {
     VerifyOrReturnError(exchangeManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(clientStorage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(engine != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(mpExchangeManager == nullptr, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(mpICDClientStorage == nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mpCheckInDelegate == nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mpImEngine == nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     mpExchangeManager  = exchangeManager;
     mpICDClientStorage = clientStorage;
@@ -113,6 +117,7 @@ CHIP_ERROR CheckInHandler::OnMessageReceived(Messaging::ExchangeContext * ec, co
 
     if (refreshKey)
     {
+        ChipLogProgress(ICD, "Key Refresh is required");
         RefreshKeySender * refreshKeySender = mpCheckInDelegate->OnKeyRefreshNeeded(clientInfo, mpICDClientStorage);
         if (refreshKeySender == nullptr)
         {

diff --git a/src/app/icd/client/DefaultICDClientStorage.cpp b/src/app/icd/client/DefaultICDClientStorage.cpp
@@ -51,6 +51,11 @@ CHIP_ERROR DefaultICDClientStorage::UpdateFabricList(FabricIndex fabricIndex)
 
     mFabricList.push_back(fabricIndex);
 
+    return StoreFabricList();
+}
+
+CHIP_ERROR DefaultICDClientStorage::StoreFabricList()
+{
     Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;
     size_t counter = mFabricList.size();
     size_t total   = kFabricIndexTlvSize * counter + kArrayOverHead;
@@ -68,7 +73,7 @@ CHIP_ERROR DefaultICDClientStorage::UpdateFabricList(FabricIndex fabricIndex)
     const auto len = writer.GetLengthWritten();
     VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    writer.Finalize(backingBuffer);
+    ReturnErrorOnFailure(writer.Finalize(backingBuffer));
     return mpClientInfoStore->SyncSetKeyValue(DefaultStorageKeyAllocator::ICDFabricList().KeyName(), backingBuffer.Get(),
                                               static_cast<uint16_t>(len));
 }
@@ -211,6 +216,7 @@ CHIP_ERROR DefaultICDClientStorage::Load(FabricIndex fabricIndex, std::vector<IC
 {
     size_t count = 0;
     ReturnErrorOnFailure(LoadCounter(fabricIndex, count, clientInfoSize));
+    VerifyOrReturnError(count > 0, CHIP_NO_ERROR);
     size_t len = clientInfoSize * count + kArrayOverHead;
     Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;
     VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
@@ -344,8 +350,21 @@ CHIP_ERROR DefaultICDClientStorage::SerializeToTlv(TLV::TLVWriter & writer, cons
     return writer.EndContainer(arrayType);
 }
 
+bool DefaultICDClientStorage::FabricExists(FabricIndex fabricIndex)
+{
+    for (auto & fabric_idx : mFabricList)
+    {
+        if (fabric_idx == fabricIndex)
+        {
+            return true;
+        }
+    }
+    return false;
+}
+
 CHIP_ERROR DefaultICDClientStorage::StoreEntry(const ICDClientInfo & clientInfo)
 {
+    VerifyOrReturnError(FabricExists(clientInfo.peer_node.GetFabricIndex()), CHIP_ERROR_INVALID_FABRIC_INDEX);
     std::vector<ICDClientInfo> clientInfoVector;
     size_t clientInfoSize = MaxICDClientInfoSize();
     ReturnErrorOnFailure(Load(clientInfo.peer_node.GetFabricIndex(), clientInfoVector, clientInfoSize));
@@ -359,7 +378,6 @@ CHIP_ERROR DefaultICDClientStorage::StoreEntry(const ICDClientInfo & clientInfo)
             break;
         }
     }
-
     clientInfoVector.push_back(clientInfo);
     size_t total = clientInfoSize * clientInfoVector.size() + kArrayOverHead;
     Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;
@@ -371,7 +389,7 @@ CHIP_ERROR DefaultICDClientStorage::StoreEntry(const ICDClientInfo & clientInfo)
     const auto len = writer.GetLengthWritten();
     VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    writer.Finalize(backingBuffer);
+    ReturnErrorOnFailure(writer.Finalize(backingBuffer));
     ReturnErrorOnFailure(mpClientInfoStore->SyncSetKeyValue(
         DefaultStorageKeyAllocator::ICDClientInfoKey(clientInfo.peer_node.GetFabricIndex()).KeyName(), backingBuffer.Get(),
         static_cast<uint16_t>(len)));
@@ -416,17 +434,19 @@ CHIP_ERROR DefaultICDClientStorage::UpdateEntryCountForFabric(FabricIndex fabric
 
     const auto len = writer.GetLengthWritten();
     VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
-    writer.Finalize(backingBuffer);
+    ReturnErrorOnFailure(writer.Finalize(backingBuffer));
 
     return mpClientInfoStore->SyncSetKeyValue(DefaultStorageKeyAllocator::FabricICDClientInfoCounter(fabricIndex).KeyName(),
                                               backingBuffer.Get(), static_cast<uint16_t>(len));
 }
 
 CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
 {
+    VerifyOrReturnError(FabricExists(peerNode.GetFabricIndex()), CHIP_NO_ERROR);
     size_t clientInfoSize = 0;
     std::vector<ICDClientInfo> clientInfoVector;
     ReturnErrorOnFailure(Load(peerNode.GetFabricIndex(), clientInfoVector, clientInfoSize));
+    VerifyOrReturnError(clientInfoVector.size() > 0, CHIP_NO_ERROR);
 
     for (auto it = clientInfoVector.begin(); it != clientInfoVector.end(); it++)
     {
@@ -440,7 +460,6 @@ CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
 
     ReturnErrorOnFailure(
         mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::ICDClientInfoKey(peerNode.GetFabricIndex()).KeyName()));
-
     size_t total = clientInfoSize * clientInfoVector.size() + kArrayOverHead;
     Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;
     ReturnErrorCodeIf(!backingBuffer.Calloc(total), CHIP_ERROR_NO_MEMORY);
@@ -451,7 +470,7 @@ CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
     const auto len = writer.GetLengthWritten();
     VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    writer.Finalize(backingBuffer);
+    ReturnErrorOnFailure(writer.Finalize(backingBuffer));
     ReturnErrorOnFailure(
         mpClientInfoStore->SyncSetKeyValue(DefaultStorageKeyAllocator::ICDClientInfoKey(peerNode.GetFabricIndex()).KeyName(),
                                            backingBuffer.Get(), static_cast<uint16_t>(len)));
@@ -461,6 +480,8 @@ CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
 
 CHIP_ERROR DefaultICDClientStorage::DeleteAllEntries(FabricIndex fabricIndex)
 {
+    VerifyOrReturnError(FabricExists(fabricIndex), CHIP_NO_ERROR);
+
     size_t clientInfoSize = 0;
     std::vector<ICDClientInfo> clientInfoVector;
     ReturnErrorOnFailure(Load(fabricIndex, clientInfoVector, clientInfoSize));
@@ -471,7 +492,23 @@ CHIP_ERROR DefaultICDClientStorage::DeleteAllEntries(FabricIndex fabricIndex)
     }
     ReturnErrorOnFailure(
         mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::ICDClientInfoKey(fabricIndex).KeyName()));
-    return mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricICDClientInfoCounter(fabricIndex).KeyName());
+    ReturnErrorOnFailure(
+        mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::FabricICDClientInfoCounter(fabricIndex).KeyName()));
+
+    for (auto fabric = mFabricList.begin(); fabric != mFabricList.end(); fabric++)
+    {
+        if (*fabric == fabricIndex)
+        {
+            mFabricList.erase(fabric);
+            break;
+        }
+    }
+
+    if (mFabricList.size() == 0)
+    {
+        return mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::ICDFabricList().KeyName());
+    }
+    return StoreFabricList();
 }
 
 CHIP_ERROR DefaultICDClientStorage::ProcessCheckInPayload(const ByteSpan & payload, ICDClientInfo & clientInfo,

diff --git a/src/app/icd/client/DefaultICDClientStorage.h b/src/app/icd/client/DefaultICDClientStorage.h
@@ -120,6 +120,12 @@ class DefaultICDClientStorage : public ICDClientStorage
     CHIP_ERROR ProcessCheckInPayload(const ByteSpan & payload, ICDClientInfo & clientInfo,
                                      Protocols::SecureChannel::CounterType & counter) override;
 
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+    size_t GetFabricListSize() { return mFabricList.size(); }
+
+    PersistentStorageDelegate * GetClientInfoStore() { return mpClientInfoStore; }
+#endif // CONFIG_BUILD_FOR_HOST_UNIT_TEST
+
 protected:
     enum class ClientInfoTag : uint8_t
     {
@@ -173,9 +179,12 @@ class DefaultICDClientStorage : public ICDClientStorage
 
 private:
     friend class ICDClientInfoIteratorImpl;
+    CHIP_ERROR StoreFabricList();
     CHIP_ERROR LoadFabricList();
     CHIP_ERROR LoadCounter(FabricIndex fabricIndex, size_t & count, size_t & clientInfoSize);
 
+    bool FabricExists(FabricIndex fabricIndex);
+
     CHIP_ERROR IncreaseEntryCountForFabric(FabricIndex fabricIndex);
     CHIP_ERROR DecreaseEntryCountForFabric(FabricIndex fabricIndex);
     CHIP_ERROR UpdateEntryCountForFabric(FabricIndex fabricIndex, bool increase);

diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn
@@ -194,6 +194,7 @@ chip_test_suite("tests") {
     "TestBasicCommandPathRegistry.cpp",
     "TestBindingTable.cpp",
     "TestBuilderParser.cpp",
+    "TestCheckInHandler.cpp",
     "TestCommandHandlerInterfaceRegistry.cpp",
     "TestCommandInteraction.cpp",
     "TestCommandPathParams.cpp",
@@ -236,6 +237,8 @@ chip_test_suite("tests") {
     "${chip_root}/src/app",
     "${chip_root}/src/app/codegen-data-model-provider:instance-header",
     "${chip_root}/src/app/common:cluster-objects",
+    "${chip_root}/src/app/data-model-provider/tests:encode-decode",
+    "${chip_root}/src/app/icd/client:handler",
     "${chip_root}/src/app/icd/client:manager",
     "${chip_root}/src/app/tests:helpers",
     "${chip_root}/src/app/util/mock:mock_codegen_data_model",