Crypto: Take into account pending to-device room key sharing requests when collecting devices

[procr1337]

Attempted fix for #2729 based on previous discussion in #4954.

To summarize, the situation of pending room key sharing requests prior to calls to GroupSessionManager::share_room_key can indeed occur if matrix-sdk-crypto is used directly. In the case where matrix-sdk wraps it, Room::share_room_key makes sure to send pending requests right after, as well as mark them as sent which will cause the corresponding entry in to_share_with_set to be moved to shared_with_set. If there is a failure, the room key is discarded right after (to avoid UTD errors from missing keys).

This is of course racy, since this entire sequence is not atomic. After an interruption and restart, we can end up back in GroupSessionManager::share_room_key with pending requests (which after all is the point of persisting the pending requests in the first place).

It seems ugly to export shared_with_set and to_share_with_set. It feels like there should be abstractions similar to is_shared_with that provide a semantic view into GroupSessionManager's state carrying sufficient information for all existing uses of shared_with_set and to_share_with_set outside of the impl (all of which are in share_strategy.rs). If you prefer this approach let me know.

Signed-off-by: Niklas Baumstark [email protected]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.88%. Comparing base (91d085c) to head (e267f5e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4975      +/-   ##
==========================================
- Coverage   85.88%   85.88%   -0.01%     
==========================================
  Files         325      325              
  Lines       35699    35712      +13     
==========================================
+ Hits        30661    30671      +10     
- Misses       5038     5041       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[poljar]

I think that this looks good and indeed should close #2729.

[poljar]

A bit of a shame that we can't reuse OutboundGroupSession::is_shared_with or at least put this into a new method.

But that would introduce unnecessary allocations due to the locking.

Oh well...

[poljar]

It seems ugly to export shared_with_set and to_share_with_set. It feels like there should be abstractions similar to is_shared_with that provide a semantic view into GroupSessionManager's state carrying sufficient information for all existing uses of shared_with_set and to_share_with_set outside of the impl (all of which are in share_strategy.rs). If you prefer this approach let me know.

I was thinking that it would be nice to have something like OutboundGroupSession::devices_shared_with(&self, user_id: &UserId) -> BTreeSet<&DeviceId>.

But that is tricky to add due to the locks protecting the sets. We would need to combine the return value with the lock guards, or we would need to clone the device IDs.

So I think the current approach is fine.

[procr1337]

@poljar In my limited understanding of rust patterns I guess the typical approach would be to return an impl Iterator<&DeviceId> which holds the read locks - thanks for your feedback, it has been a pleasant experience contributing to this code base so far.

[poljar]

@poljar In my limited understanding of rust patterns I guess the typical approach would be to return an impl Iterator<&DeviceId> which holds the read locks.

Yeah that would likely work but as you have to take ownership of the guard and borrow a value behind the guard this gets quite tricky.

I'm not sure if the borrow checker is smart enough to know that you should be able to do this, i.e. not sure if you can get away without using unsafe.

I would say it's not really worth it to spend time on this, but if you manage to get it work without unsafe I wouldn't object to merging it.

Thanks for your feedback, it has been a pleasant experience contributing to this code base so far.

Thank you for the patches and for the patience while waiting for the review.