crypto: withhold outgoing messages to unsigned dehydrated devices #4551
Conversation
richvdh
force-pushed
the
rav/withheld_to_unverified_dehydrated
branch
from
001dcf6 to
cac1067
Compare
| // We only need the user identity if `only_allow_trusted_devices` or | ||
| // `error_on_verified_user_problem` is set. | ||
| let device_owner_identity = | ||
| if only_allow_trusted_devices || error_on_verified_user_problem { | ||
| store.get_user_identity(user_id).await? | ||
| } else { | ||
| None | ||
| }; | ||
| let device_owner_identity = store.get_user_identity(user_id).await?; |
There was a problem hiding this comment.
We now also need the identity if the user has a dehydrated device. Trying to optimise away this call in the cases where we weren't going to need the identity was getting too complicated, so let's just do it every time.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4551 +/- ##
==========================================
- Coverage 85.40% 85.39% -0.02%
==========================================
Files 285 285
Lines 32213 32217 +4
==========================================
Hits 27513 27513
- Misses 4700 4704 +4 ☔ View full report in Codecov by Sentry. |
|
I need to update this in view of #4313 (comment) |
* Split the existing `set_up_test_machine` into two parts, so we can set up the test OlmMachine without importing data for other users * Add some helpers for creating common `EncryptionSettings`
richvdh
force-pushed
the
rav/withheld_to_unverified_dehydrated
branch
from
cac1067 to
aa82b96
Compare
Per #4313, we should not send outgoing messages to dehydrated devices that are not signed by the current pinned/verified identity.
richvdh
force-pushed
the
rav/withheld_to_unverified_dehydrated
branch
from
aa82b96 to
919f073
Compare
(Now done) |
| @@ -406,23 +398,50 @@ fn split_devices_for_user( | |||
| for d in user_devices.into_values() { | |||
| if d.is_blacklisted() { | |||
| recipient_devices.denied_devices_with_code.push((d, WithheldCode::Blacklisted)); | |||
| } else if d.local_trust_state() == LocalTrust::Ignored { | |||
| continue; | |||
There was a problem hiding this comment.
The logic in the loop has, due to the additions of various new settings, grown a bit into a maze, it's quite hard to enumerate that all the different settings and combinations thereof don't have some logic flaw in them.
I think this needs to be put into a separate method which would then allow us to easier test the logic as well.
Something like:
enum Decision {
Ok,
Withhold(WithheldCode),
Err,
}
fn decide_what_to_do_with_device(device: &Device) -> Decision {
...
}Then write a match statement per setting, i.e.
if only_allow_trusted {
match (d.is_verified(), d.is_dehydrated)
...
}
}Or something along those lines which would allow us to easily see if our logic is exhaustive and we didn't forget a case.
Per #4313, we should not send outgoing messages to dehydrated devices that are not signed by the user's identity
Fixes #4313