Ignore to-device messages from dehydrated devices #4569
Conversation
andybalaam
requested review from
Hywan and
richvdh
and removed request for
a team
| // Handle the decrypted event, e.g. fetch out Megolm sessions out of | ||
| // the event. | ||
| self.handle_decrypted_to_device_event(transaction.cache(), &mut decrypted, changes) | ||
| .await?; |
There was a problem hiding this comment.
I could not find a way to test that the side effects of this call are suppressed when sending from a dehydrated device. I think the only observable side effects happen when we send a SecretSend to-device event, and when I looked at how to do that it looked like I would need an existing SecretRequest for anything to happen, and that looked complex to set up.
There was a problem hiding this comment.
(To try to clarify what I mean: my tests would still pass if this if did not exist.)
| // know better. | ||
| } | ||
|
|
||
| // Do we already know about this device? |
There was a problem hiding this comment.
My tests would still pass if the rest of this function did not exist.
I looked into how to construct a to-device message that was from a known dehydrated device, but did not itself contain device_keys and it just looked too complex. I even got part-way in to an implementation pairing with Rich, but it just seemed so much!
andybalaam
force-pushed
the
andybalaam/drop-to-device-from-dehydrated
branch
from
69389e6 to
c1c2b18
Compare
|
Any help with how to construct test cases that exercise the other parts of this code would be much appreciated! I have essentially given up because it seemed too hard. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4569 +/- ##
==========================================
+ Coverage 85.39% 85.41% +0.01%
==========================================
Files 285 285
Lines 32215 32235 +20
==========================================
+ Hits 27510 27533 +23
+ Misses 4705 4702 -3 ☔ View full report in Codecov by Sentry. |
| async fn send_room_key_to_device( | ||
| alice: &OlmMachine, | ||
| bob: &OlmMachine, | ||
| room_id: &RoomId, | ||
| ) -> OlmResult<(Vec<Raw<AnyToDeviceEvent>>, Vec<RoomKeyInfo>)> { |
There was a problem hiding this comment.
might be worth a bit of documentation saying this sends from alice to bob. (Alternatively, I guess you could use better names for the users.)
Could you also document what it returns?
| warn!( | ||
| sender = ?e.sender, | ||
| session = ?decrypted.session, | ||
| "Received a to-device event from a dehydrated device. This is unexpected" |
There was a problem hiding this comment.
| "Received a to-device event from a dehydrated device. This is unexpected" | |
| "Received a to-device event from a dehydrated device. This is unexpected: ignoring event" |
| Some(raw_event) | ||
| } | ||
|
|
||
| async fn event_is_from_dehydrated_device( |
There was a problem hiding this comment.
| async fn event_is_from_dehydrated_device( | |
| async fn to_device_event_is_from_dehydrated_device( |
| async fn event_is_from_dehydrated_device( | ||
| &self, | ||
| decrypted: &OlmDecryptionInfo, | ||
| sender_user_id: &UserId, | ||
| ) -> bool { |
There was a problem hiding this comment.
it might come as a bit of a surprise that this accepts an OlmDecryptionInfo rather than, say, an AnyToDeviceEvent. Might be worth adding a doc-comment that explains that it works on decrypted to-device events.
| if let Some(device) = device { | ||
| device.is_dehydrated() | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
light suggestion, no strong feelings here
| if let Some(device) = device { | |
| device.is_dehydrated() | |
| } else { | |
| false | |
| } | |
| device.is_some_and(Device::is_dehydrated) |
| // No - we received a to-device message from a device we don't know about. We | ||
| // continue to process it, but this is unexpected so we log an error. | ||
| warn!( | ||
| error = ?e, | ||
| "Received a to-device message from a device we don't recognise", | ||
| ); |
There was a problem hiding this comment.
I think if we just don't recognise the device, get_device_from_curve_key will return Ok(None). This suggests a problem with the store.
It's probably still ok to return false here; but the comment and warning could do with a tweak.
| } | ||
| Err(e) => { | ||
| // No - we received a to-device message from a device we don't know about. We | ||
| // continue to process it, but this is unexpected so we log an error. |
There was a problem hiding this comment.
we log an error
We are logging a warning, not an error.
Fixes #4466