Key verification events can be in the timeline as well. about ruma HOT 16 CLOSED

Left a review on the MSC, let's see where that goes.

from ruma.

So I think we should go with the separate struct afterall. It was never truly a problem to integrate since AnyEvent doesn't contain to-device events, and probably doesn't even need to exist at all. Even then, there is no ambiguity between to-device and room events; it only was a problem before we started looking at the presence of extra fields rather than the type to identify the event kind.

I think we can tackle this as follows:

• Rename all to-device event content structs from WhateverEventContent to WhateverToDeviceEventContent

• Apply this patch:

  diff --git a/ruma-events-macros/src/event_enum.rs b/ruma-events-macros/src/event_enum.rs
  index 5ab0332d..d823f7b3 100644
  --- a/ruma-events-macros/src/event_enum.rs
  +++ b/ruma-events-macros/src/event_enum.rs
  @@ -818 +818 @@ fn to_event_path(name: &LitStr, struct_name: &Ident, ruma_events: &TokenStream)
  -            let content = format_ident!("{}EventContent", event);
  +            let content = format_ident!("{}ToDeviceEventContent", event);

• For all key verification events:

  • Introduce a new (feature-gated) event content struct with the content struct variation used for rooms (using the previous WhateverEventContent name)
  • In pub type WhateverEvent = BasicEvent<WhateverEventContent>; replace BasicEvent by MessageEvent and feature-gate this type alias

• For all other to-device events, remove the pub type WhateverEvent = BasicEvent<WhateverEventContent>; aliases

from ruma.

What about the events where we do share the content between a message event and a to-device event (i think that's only m.room.encrypted)

I'd say we do pub type EncryptedToDeviceEventContent = EncryptedEventContent;.

Shouldn't this be a bit different:

Oh, of course 😄

from ruma.

@poljar Do you want to take a stab at implementing this? It should be a straight-forward change (move the modules to room/, declare them as room events like the other ones, no changes to the declared fields necessary) if I properly understand this issue.

from ruma.

I'm not sure that just moving them to room events is enough. They can still be a to-device event. But sure, I'll check it out.

from ruma.

So checked it out. The verification events are implemented using the ruma_event! macro. The macro takes an argument for the kind of the event.

Changing this from Event to RoomEvent will make the event_id, sender and the rest appear, but the event will probably fail to be de-serialized if it's send as a to-device event.

So we could:

1. Create a new kind in the macro
2. Define the struct fields manually
3. Create two key verification event implementations one for each event kind

from ruma.

I don't understand. Are these events sometimes room events and sometimes not?

from ruma.

Yeah, when they were first implemented they were sent using the sendToDevice endpoint, those events aren't part of the DAG nor a room, and aren't kept around in the history.

The linked spec proposal adds the ability to send those events using rooms/{room_id}/send API endpoint, making them normal room events.

For backwards compatibility the first method of sending those events won't be dropped.

from ruma.

Okay so I understand this is a proposal, nothing final? In that case, we probably shouldn't do anything yet, but it's good to keep this on our radar nonetheless. I think having separate event types for the room events and send-to-device events would be the way to go once this lands in the next version of the spec.

from ruma.

While it hasn't been merged yet, it's very likely to be final. It has been implement and merged in the js-skd.

I agree that we don't need to rush this out, I will probably need this in a couple of months.

from ruma.

So we have a problem with this. The content between the in-room event differs from the to-device event as well. As can be found here.

The messages used in SAS verification are the same as those currently defined, except that instead of the transaction_id property, an m.relates_to property, as defined above, is used instead.

So the transaction ID for the in-room event will be missing, using the existing contents for in-room events as they are defined currently would result in a de-serialization error. Of course there's also the issue that in-room events still have a completely different thing that is called a transaction id in the unsigned part as well.

So we have a couple of things we could do:

• Make the transaction id optional.
• Create completely separate content structs.
• Something else?

from ruma.

So we have a problem with this. The content between the in-room event differs from the to-device event as well. As can be found here.

The messages used in SAS verification are the same as those currently defined, except that instead of the transaction_id property, an m.relates_to property, as defined above, is used instead.

So the transaction ID for the in-room event will be missing, using the existing contents for in-room events as they are defined currently would result in a de-serialization error. Of course there's also the issue that in-room events still have a completely different thing that is called a transaction id in the unsigned part as well.

🤦

So we have a couple of things we could do:

• Make the transaction id optional.

Generally not what we do.

• Create completely separate content structs.

This will interact badly with the existing enum generation code. I'd like to avoid it if possible...

• Something else?

Can we still turn around the MSC potentially? Or is this "effectively stabilized" by being implemented in Synapse and Element? 🙄

from ruma.

Can we still turn around the MSC potentially? Or is this "effectively stabilized" by being implemented in Synapse and Element?

Technically the MSC is still open and not in the final comment period, but I have my doubts about changing course, since at least 4 different clients implement it. Synapse likely doesn't care what the shape of the content of those events looks like.

from ruma.

Could we move this somehow forward, I would like to add support for in-room verifications in the rust-sdk to finish up the crypto saga there.

Seems like making the transaction id optional and adding the an optional Relation field would be the simplest thing to do. In the future we will likely need to have the ability to define the struct that should be used for the event enum so we can have different versions for to-device vs message events.

Or perhaps we could transform the Relation into the transaction_id. That would leave the content the same and the transaction id would stay non-optional, not sure how hard it would be to keep the serialization/deserialization bijective.

from ruma.

So I think we should go with the separate struct afterall. It was never truly a problem to integrate since AnyEvent doesn't contain to-device events, and probably doesn't even need to exist at all. Even then, there is no ambiguity between to-device and room events; it only was a problem before we started looking at the presence of extra fields rather than the type to identify the event kind.

I think we can tackle this as follows:

* Rename all to-device event content structs from `WhateverEventContent` to `WhateverToDeviceEventContent`

What about the events where we do share the content between a message event and a to-device event (i think that's only m.room.encrypted)

* Apply this patch:

Shouldn't this be a bit different:

diff --git a/ruma-events-macros/src/event_enum.rs b/ruma-events-macros/src/event_enum.rs
index 5ab0332d..93bd229d 100644
--- a/ruma-events-macros/src/event_enum.rs
+++ b/ruma-events-macros/src/event_enum.rs
@@ -809,8 +809,7 @@ fn to_event_path(name: &LitStr, struct_name: &Ident, ruma_events: &TokenStream)
             };
             quote! { #ruma_events::room::redaction::#redaction }
         }
-        "ToDeviceEvent"
-        | "SyncStateEvent"
+        "SyncStateEvent"
         | "StrippedStateEvent"
         | "InitialStateEvent"
         | "SyncMessageEvent"
@@ -818,6 +817,10 @@ fn to_event_path(name: &LitStr, struct_name: &Ident, ruma_events: &TokenStream)
             let content = format_ident!("{}EventContent", event);
             quote! { #ruma_events::#struct_name<#ruma_events::#( #path )::*::#content> }
         }
+        "ToDeviceEvent" => {
+            let content = format_ident!("{}ToDeviceEventContent", event);
+            quote! { #ruma_events::#struct_name<#ruma_events::#( #path )::*::#content> }
+        }
         struct_str if struct_str.contains("Redacted") => {
             let content = format_ident!("Redacted{}EventContent", event);
             quote! { #ruma_events::#struct_name<#ruma_events::#( #path )::*::#content> }

from ruma.

Fixed by #361, which is marked as closed rather than merged because I did the merge on the commandline in a way GitHub didn't understand.

from ruma.