-
Notifications
You must be signed in to change notification settings - Fork 285
feat(WidgetDriver): Add to-device support (without encryption) #4963
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
base: main
Are you sure you want to change the base?
Conversation
9f38056
to
9504745
Compare
f6bd24a
to
16171b6
Compare
16171b6
to
3445127
Compare
d1f9f49
to
50c0c2a
Compare
3445127
to
bb14445
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4963 +/- ##
==========================================
+ Coverage 85.86% 85.87% +0.01%
==========================================
Files 325 325
Lines 35851 35954 +103
==========================================
+ Hits 30783 30876 +93
- Misses 5068 5078 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c70a005
to
64c5426
Compare
A new widget filter is required to add support for to-device events. This allows to let the widget only send and receive to-device events it has negotiated capabilities for.
It consists of the following changes: - add a `NotifyNewToDeviceEvent` ToWidget request (a request that will be sent to the widget from the client when the client receives a widget action over the widget api) - add the `SendToDeviceRequest` (driver request that will be sent from the widget and asks the driver to send a ToDevice event) - add the ToDeviceActions to the required enums: `IncomingMessage`(machine), `MatrixDriverResponse`, `FromWidgetResponse`, `FromWidgetRequest`, `MatrixDriverRequestData`
5d3078c
to
39ccdca
Compare
I'm going to let poljar handle this next week as I have very little context on it. |
… events via cs api) This also hooks up the widget via the machine actions. And adds toDevice events to the subscription.
9ff12fa
to
35199a7
Compare
…ity. This needs to be part of the send/read capabilities so that to-device keys can be used.
ad770d9
to
aa35aad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has a couple of flaws that need to be addressed, the biggest concern here is about the security when sending and receiving to-device messages.
I also left a bunch of less important nits, mainly to get the terminology consistent.
Please take a look at our contributing guide for our preferred commit styiling: https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#commit-message-format.
The first commit also doesn't compile on its own, unless you want to fix that we'll have to squash merge this PR, otherwise using git bisect
(if we ever need that tool) becomes hard.
Finally, I think that this deserves a changelog entry. Our contributing guide should help you with that as well: https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#writing-changelog-entries.
} | ||
} | ||
|
||
impl ToDeviceEventFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the separate impl
block?
State(FilterInputState<'a>), | ||
// only then we can check if we can deserialize as a message like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// only then we can check if we can deserialize as a message like. | |
// only then we can check if we can deserialize as a message-like. |
MessageLike(FilterInputMessageLike<'a>), | ||
// ToDevice will need to be done explicitly since it looks the same as a message like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// ToDevice will need to be done explicitly since it looks the same as a message like. | |
// ToDevice will need to be done explicitly since it looks the same as a message-like. |
@@ -188,10 +228,35 @@ impl<'a> TryFrom<&'a Raw<AnyTimelineEvent>> for FilterInput<'a> { | |||
type Error = serde_json::Error; | |||
|
|||
fn try_from(raw_event: &'a Raw<AnyTimelineEvent>) -> Result<Self, Self::Error> { | |||
// FilterInput first checks if it can deserialize as a state event (state_key | |||
// exists) and then as a message like event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// exists) and then as a message like event. | |
// exists) and then as a message-like event. |
impl fmt::Display for ToDeviceEventFilter { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!(f, "{}", self.event_type) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that Display
implementation used for something? A bit of a surprising choice to just print the event type.
/// This is a "response" to the widget subscribing to the events in the room. | ||
#[derive(Serialize)] | ||
#[serde(transparent)] | ||
pub(crate) struct NotifyNewToDeviceEvent(pub(crate) Raw<AnyToDeviceEvent>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the compilation but I think the rename makes sense:
pub(crate) struct NotifyNewToDeviceEvent(pub(crate) Raw<AnyToDeviceEvent>); | |
pub(crate) struct NotifyNewToDeviceMessage(pub(crate) Raw<AnyToDeviceEvent>); |
let _ = _tx.send(event_with_room_id.cast()); | ||
} | ||
Err(e) => { | ||
error!("Failed to attach room id to message like event: {}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error!("Failed to attach room id to message like event: {}", e); | |
error!("Failed to attach room id to message-like event: {}", e); |
impl<'a> TryFrom<&'a Raw<AnyToDeviceEvent>> for FilterInput<'a> { | ||
type Error = serde_json::Error; | ||
fn try_from(raw_event: &'a Raw<AnyToDeviceEvent>) -> Result<Self, Self::Error> { | ||
// deserialize_as::<FilterInput> will first try state, message like and then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// deserialize_as::<FilterInput> will first try state, message like and then | |
// deserialize_as::<FilterInput> will first try state, message-like and then |
/// Attach a room id to the event. This is needed because the widget API | ||
/// requires the room id to be present in the event. | ||
/// | ||
/// Attach the `ecryption` flag to the event. This is needed so the widget gets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Attach the `ecryption` flag to the event. This is needed so the widget gets | |
/// Attach the `encryption` flag to the event. This is needed so the widget gets |
Since you seem to be a typo susceptible creature, not unlike myself, let me share a small secret we have going on.
We're using typos
to spell check the source tree. When we discover a new typo that somebody (mostly me) is making regularly, we head over to the non-critical typos issue on the typos
repo and add it there: crate-ci/typos#1290 (comment).
This has fixed a lot of typos in the repo 😅.
/// requires the room id to be present in the event. | ||
/// | ||
/// Attach the `ecryption` flag to the event. This is needed so the widget gets | ||
/// informed if an event is encrypted or not. Since the client is responsible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a particularly dangerous pattern.
Generally inserting additional metadata into the same scope that the homeserver or another user are part of is always dangerous.
One could easily imagine that the homeserver might just insert such a value themselves, or the sender themselves.
Now you'll argue that this is fine, since you're going to overwrite any value that was there. This might be true for some serializers but duplicate keys are allowed by the JSON spec, and the handling of them depends on the implementation.
It is doubly dangerous because multiple JSON implementations will be involved here, serde
on the Rust side and whichever implementation the various browsers that might run the widget.
What you need to do is the following, have a struct like this:
struct ToDeviceFoo {
message: Raw<AnyToDevice...>,
metadata_i_add: bool,
some_more_things: String
}
This way the scopes we modify and the homeserver or sender control are cleanly separated.
It is fine to do this for the room_info
because it's metadata we are putting back the homeserver omitted for performance reasons.
The widget Driver should be able to send and receive to-device events. This is useful for element call encryption keys.
This PR focusses on the widget driver and machine logic. To send/communicate the events from the widget to the driver.
It skips any encryption logic. Some of the encryption logic will be part of crypto crate and the code in the widget driver crate should be kept minimal once the crypto crate is ready.