Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReleaseSessionsForFabric takes CompressFabricIndex which is ambiguous #18435

Closed
tcarmelveilleux opened this issue May 13, 2022 · 0 comments · Fixed by #18459
Closed

ReleaseSessionsForFabric takes CompressFabricIndex which is ambiguous #18435

tcarmelveilleux opened this issue May 13, 2022 · 0 comments · Fixed by #18459

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Within the SDK, FabricIndex is the only unambiguous identifier.

Many methods that emanated from FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId compressedFabricId, chip::FabricIndex fabricIndex) perpetuated use of the compressed fabric ID, because at the bottom, we have
CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId compressedFabricId);

This makes the whole process more confusing and overall muddies the water about what CompressedFabricId is for. Furthermore, CompressedFabricId is not unambiguous and this could lead to some unintended behavior.

Now that secure sessions hold their fabric index, we should be able to remove CompressedFabricId from the signatures.

Proposed Solution

  • Remove compressed fabric ID from FabricTableDelegate::OnFabricDeletedFromStorage()
  • Add CASESessionManager::ReleaseSessionsForFabric(FabricIndex)
  • Remove CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId)
  • Do the necessary updates in dependencies to reflect the above.
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 15, 2022
Within the SDK, FabricIndex is the only unambiguous identifier.

Many methods that emanated from
FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId, chip::FabricIndex)
perpetuated use of the compressed fabric ID, because at the bottom, we have
CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId)

- Replace CASESessionManager::ReleaseSessionsForFabric argument from
  CompressedFabricId to FabricIndex.
- Rework FabricTableDelegate to remove need to pass any
  CompressedFabricId.
- Replace all downstream usages of CompressedFabricId with FabricIndex
  and FabricTable reference.
- Make FabricTableDelegate calls symmetrical in arguments
- Make FabricTableDelegate an inner class of FabricTable to remove
  a friend relationship
- Clarify when adding a FabricTableDelegate causes its deletion due
  to ownership changes
- Add session resumption state clearing on fabric removal

Fixes project-chip#18435
Issue project-chip#18436
tcarmelveilleux added a commit that referenced this issue May 17, 2022
* Remove unneeded/ambiguous CompressedFabricId usages

Within the SDK, FabricIndex is the only unambiguous identifier.

Many methods that emanated from
FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId, chip::FabricIndex)
perpetuated use of the compressed fabric ID, because at the bottom, we have
CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId)

- Replace CASESessionManager::ReleaseSessionsForFabric argument from
  CompressedFabricId to FabricIndex.
- Rework FabricTableDelegate to remove need to pass any
  CompressedFabricId.
- Replace all downstream usages of CompressedFabricId with FabricIndex
  and FabricTable reference.
- Make FabricTableDelegate calls symmetrical in arguments
- Make FabricTableDelegate an inner class of FabricTable to remove
  a friend relationship
- Clarify when adding a FabricTableDelegate causes its deletion due
  to ownership changes
- Add session resumption state clearing on fabric removal

Fixes #18435
Issue #18436

* Restyled by clang-format

* Fix clang CI

* Apply review comments

* Restyled by clang-format

* Address review comments

* Remove missed field

* Please the gods of Tidy

Co-authored-by: Restyled.io <commits@restyled.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant