[ICD] Improve ICDClientStorage (project-chip#36036) (project-chip#36055)

yunhanw-google · mkardous-silabs · restyled-commits · web-flow · commit 5335054bb013 · 2024-10-15T09:28:56.000-04:00
* 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



* Update DefaultICDClientStorage.cpp



* Update DefaultICDClientStorage.cpp



* Update DefaultICDClientStorage.cpp



* Update DefaultICDClientStorage.cpp



* 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 &lt;84793247+mkardous-silabs@users.noreply.github.com&gt;
Co-authored-by: Restyled.io &lt;commits@restyled.io&gt;
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,7 @@ 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/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",
diff --git a/src/app/tests/TestCheckInHandler.cpp b/src/app/tests/TestCheckInHandler.cpp
diff --git a/src/app/tests/TestDefaultICDClientStorage.cpp b/src/app/tests/TestDefaultICDClientStorage.cpp

Original file line number	Diff line number	Diff line change
`@@ -52,8 +52,12 @@ CHIP_ERROR CheckInHandler::Init(Messaging::ExchangeManager * exchangeManager, IC`
`52`	`52`	`{`
`53`	`53`	`VerifyOrReturnError(exchangeManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT);`
`54`	`54`	`VerifyOrReturnError(clientStorage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);`
	`55`	`+ VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);`
	`56`	`+ VerifyOrReturnError(engine != nullptr, CHIP_ERROR_INVALID_ARGUMENT);`
`55`	`57`	`VerifyOrReturnError(mpExchangeManager == nullptr, CHIP_ERROR_INCORRECT_STATE);`
`56`	`58`	`VerifyOrReturnError(mpICDClientStorage == nullptr, CHIP_ERROR_INCORRECT_STATE);`
	`59`	`+ VerifyOrReturnError(mpCheckInDelegate == nullptr, CHIP_ERROR_INCORRECT_STATE);`
	`60`	`+ VerifyOrReturnError(mpImEngine == nullptr, CHIP_ERROR_INCORRECT_STATE);`
`57`	`61`
`58`	`62`	`mpExchangeManager = exchangeManager;`
`59`	`63`	`mpICDClientStorage = clientStorage;`
`@@ -113,6 +117,7 @@ CHIP_ERROR CheckInHandler::OnMessageReceived(Messaging::ExchangeContext * ec, co`
`113`	`117`
`114`	`118`	`if (refreshKey)`
`115`	`119`	`{`
	`120`	`+ ChipLogProgress(ICD, "Key Refresh is required");`
`116`	`121`	`RefreshKeySender * refreshKeySender = mpCheckInDelegate->OnKeyRefreshNeeded(clientInfo, mpICDClientStorage);`
`117`	`122`	`if (refreshKeySender == nullptr)`
`118`	`123`	`{`