-
Notifications
You must be signed in to change notification settings - Fork 383
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
MSC2477: User-defined ephemeral events in rooms #2477
base: old_master
Are you sure you want to change the base?
Conversation
13a999d
to
aad3e26
Compare
Co-Authored-By: Sorunome <mail@sorunome.de>
As there's no similar requirement for PDUs, adding one for EDUs might be a bit overkill.
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.
overall this looks to be in the right direction!
### Addition of an ephemeral event sending endpoint to the Client-Server API | ||
|
||
The suggested addition to the CS API is the endpoint | ||
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost |
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.
worth noting that this limits custom EDUs to rooms compared to something like presence which is user based instead of room based.
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.
The second line in my proposal block already notes this, as I didn't think the usefulness of custom non-room EDUs merited the work of designing some whole new kind of permission system for sending them. Though perhaps a reference to user EDUs built on the Profile-as-a-Room MSC1769 could be added.
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.
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost | |
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost |
Following the new per-endpoint versioning spec.
### Extension of the ephemeral data received in /sync responses | ||
|
||
Because the user-defined ephemeral events can't be aggregated and massaged by Synapse in a simple | ||
manner, this then suggests instead adding a few more (**optional** but suggested for `m.*`, |
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.
"optional but suggested" is not really a state we can support. If we're bumping room versions, we should make it required.
It's also unclear how this would apply to something like presence which is not bound to a room and already duplicates this information.
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 proposal was by design only made for room-specific ephemeral events, so presence - as a non-room-bound EDU - shouldn't be affected. I'll try to make this more clear.
Co-authored-by: Šimon Brandner <simon.bra.ag@gmail.com>
This could do with sanity review so an implementation can be written relatively safely. |
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 really exciting feature, and I'd love to have it personally.
The below are my thoughts on all currently outstanding issues.
### Addition of an ephemeral event sending endpoint to the Client-Server API | ||
|
||
The suggested addition to the CS API is the endpoint | ||
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost |
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.
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost | |
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost |
Following the new per-endpoint versioning spec.
|
||
Status code 400: | ||
|
||
The user tried to send a `m.*` EDU, instead of using the type-specific endpoint |
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'm not sure if we should make this off-limits to anything m.*
. In fact, I wonder if we should go the opposite way and transition all room-based ephemeral messages to this endpoint - so read receipts and typing notifications.
At the moment, typing notifications are using:
PUT /_matrix/client/v3/rooms/{roomId}/typing/{userId}
{
"timeout": 30000,
"typing": true
}
and read markers are sent with:
POST /_matrix/client/v3/rooms/{roomId}/read_markers
{
"m.fully_read": "$somewhere:example.org",
"m.read": "$elsewhere:example.org"
}
The {userId}
bit of the typing endpoint is especially useless, and could be cleaned up in the same move.
Presence and to-device messages would be excluded from this, as they are not bound to a room.
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.
Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?
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 wrote this MSC on the assumption that the custom EDUs would be a separate beast to the built-in EDUs, but I guess it could just as well be done to instead have the server handle well-defined EDUs in a certain manner. So that you - as a client dev/user - could just post m.typing
or m.receipt
/m.fully_read
/m.read
EDUs instead of calling the specific endpoints for them.
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.
Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?
Good point. m.fully_read
does not fit this model, as its purpose is to store your read-up-to position, which lives in the current user's room account data. The linked endpoint just happens to allow optionally sending an m.read
receipt in the same call.
Instead, we should be thinking about whether to replicate the behaviour of POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId}
with this then.
With that, I'm not so sure. They do fall under the banner of EDU, but a receipt is always tied to an event ID (e.g. I read up to this event ID). We'd need to include that event ID in the body of the request for m.read
or for any other type of receipt. That can work, though I suppose it comes down to how much we'd like to differentiate receipts as their own thing, versus just another type of EDU.
To reduce the complexity of the change, this proposal suggests to - for the time being - only limit | ||
the user-defined types by these power levels changes. The default values for `m.*` specified here in | ||
`ephemeral` would then only be expected to be informational in purpose. |
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.
As mentioned earlier, any change to the auth rules requires a new room version. If we wanted to have these only apply to user-defined types initially, and then fully apply later, that would take two room versions.
Given rooms will need to be recreated anyways, it seems sensible to simply have homeservers pre-fill 0
for m.typing
and m.fully_read
entries when creating a room to preserve existing behaviour?
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 used m.receipt
as the type when I was writing, as I wasn't sure about m.read
(and m.fully_read
). The resulting event is m.receipt
, and the endpoint for sending the information is rooms/{roomId}/receipt
. But the receipt type as defined in the content is m.read
.
m.fully_read
is in the spec handled in a separate endpoint, but includes a link to receipts as you can include m.read
in the request to combine the two into a single request. Not sure how that would be combined into things.
For now I think I'm going to keep m.receipt
as the general EDU type, since it's the type that - for now - is seen in an actual ephemeral event. (after all m.fully_read
only ends up in the account data, and m.read
are agglomerated into m.receipt
events)
] | ||
}, | ||
"type": "m.typing", | ||
"room_id": "!636q39766251:example.com" |
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.
room_id
isn't part of the spec for ephemeral event bodies down /sync.
Likewise below.
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.
The spec entry you linked includes an m.typing
event in the example response for code 200, which does include room_id
in the data;
# ...
"join": {
"!726s6s6q:example.com": {
"account_data": {
"events": [
{
"content": {
"tags": {
"u.work": {
"order": 0.9
}
}
},
"type": "m.tag"
},
{
"content": {
"custom_config_key": "custom_config_value"
},
"type": "org.example.custom.room.config"
}
]
},
"ephemeral": {
"events": [
{
"content": {
"user_ids": [
"@alice:matrix.org",
"@bob:example.com"
]
},
"room_id": "!jEsUZKDJdhlrceRyVU:example.org",
"type": "m.typing"
}
]
},
# ...
I think that's where that came from, since I tried to base my examples directly on the spec examples.
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 see, thanks for pointing that out! I've made a PR to fix that, as I believe that's a spec bug: #3679.
This could cause some clients to break though, if they expect the well-defined objects to keep to | ||
their specced forms. Additionally, it might be hard for the server to assign a correct sender and | ||
timestamp to events if they are aggregated from multiple sources - e.g. typing notices and read | ||
receipt lists. |
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 could cause some clients to break though, if they expect the well-defined objects to keep to
their specced forms.
If I understand correctly, this is referring to adding the sender
and origin_server_ts
fields to a JSON object. In the past I believe we've considered adding keys to JSON to not break backwards compatibility, as clients should just be pulling out the keys they need anyhow - and these events are not signed or otherwise wholly checked for integrity.
Additionally, it might be hard for the server to assign a correct sender and
timestamp to events if they are aggregated from multiple sources - e.g. typing notices and read
receipt lists.
This would have to be a problem that we solve anyways, regardless of backwards-compatibility periods.
I'm also not clear why it would be difficult to populate these fields for typing and read receipt lists.
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.
As one example on the second point; m.typing
events are aggregated down from the source data into a single event with a list of user_ids
that are currently typing. If they are to have correct timestamps and sender data attached to match the EDU type as defined in this MSC, then they'd have to be split apart into separate objects each with their own metadata. I'm not really sure if that's desirable.
|
||
Status code 400: | ||
|
||
The user tried to send a `m.*` EDU, instead of using the type-specific endpoint |
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.
Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?
Good point. m.fully_read
does not fit this model, as its purpose is to store your read-up-to position, which lives in the current user's room account data. The linked endpoint just happens to allow optionally sending an m.read
receipt in the same call.
Instead, we should be thinking about whether to replicate the behaviour of POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId}
with this then.
With that, I'm not so sure. They do fall under the banner of EDU, but a receipt is always tied to an event ID (e.g. I read up to this event ID). We'd need to include that event ID in the body of the request for m.read
or for any other type of receipt. That can work, though I suppose it comes down to how much we'd like to differentiate receipts as their own thing, versus just another type of EDU.
These new keys are to function in an identical manner to the already existing `events` and | ||
`events_default` keys, with the assumed default value for `ephemeral_default` - if there is no | ||
`ephemeral_default` in the `m.room.power_levels` event - being 50, while the default values for | ||
`ephemeral` - if there is no `ephemeral` in the `m.room.power_levels` event - would consider all types | ||
to be `ephemeral_default`, or 0 if there is no `m.room.power_levels` event - which would then not allow | ||
any ephemeral events to be sent. |
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 found this paragraph a bit difficult to parse. Partly because it's all one sentence. How about:
These new keys are to function in an identical manner to the already existing
events
and
events_default
keys. The default value forephemeral_default
is 50, while the default value for the
ephemeral
dictionary would be{}
or an empty mapping. This would imply that all ephemeral event types
would require a power level of at leastephemeral_default
to send.
I think the last line about a missing m.room.power_levels
state event is not necessary. According to the spec, rooms must contain an m.room.power_levels
when created: https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3createroom
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.
Decided to rewrite it in a slightly different manner, should hopefully be more easily parsed now.
Thanks for working on this! Definitely a promising feature.
This triggered my attention. Performance-wise, I think servers should take into account that there can be many (orders of magnitude larger than regular events) ephemeral events sent for certain scenarios. This rules out any kind of temporary storage I think. For me at least, that would be a useful requirement (scenario I'm thinking of is to keep track of a user's mouse position and sharing that as ephemeral events). Do you think this should be supported, and if so, made explicit in the MSC? |
I was debating offering a list of possible delivery guarantees as part of the MSC, but I feel that the necessary language, permissions, and special cases necessary for supporting such would end up rather complex - as these are entirely user-specified events which would basically require such a thing to be a per-event choice, which would mean a lot of cases for servers to consider. Of the example scenarios I provide in the MSC itself, the live-location feature, would also result in a large amount of events if a large group of people are using it to try and meet up, or if it's used to track a fleet of commercial vessels. MSC3672 has appeared as a more fully-realized version of that particular example scenario now, and I'm curious to see how the discussion will evolve in regards to this. |
the regular Matrix types as well, which would remove the need for optional fields - but could in | ||
return impact the federation between servers, if they're built to only handle the exact requirements | ||
of the spec. | ||
|
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.
Unstable prefixes will need to be defined in order for an implementation of the MSC to be carried out. I suggest implementation replace:
ephemeral
andephemeral_default
fields in them.room.power_levels
state event withorg.matrix.msc2477.ephemeral
andorg.matrix.msc2477.ephemeral_default
respectively.- the
PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}
endpoint withPUT /_matrix/client/unstable/org.matrix.msc2477/rooms/{roomId}/ephemeral/{eventType}/{txnId}
.
And an experimental room version with name org.matrix.msc2477
should be used where this feature is enabled.
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.
And a whole lot later than it should've been, but I've added a section on an unstable prefix.
Wasn't sure about adding blurbs on when the m.room.power_levels
keys should be allowed, but I figured that it'd a bit superfluous since the only method that will act on them is limited to the experimental room version.
### Addition of an ephemeral event sending endpoint to the Client-Server API | ||
|
||
The addition to the CS API is the endpoint | ||
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost |
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.
Personal nit: wouldn't something like /send_ephemeral
or /send/...?ephemeral=true
make more sense? An endpoint just named "ephemeral" doesn't really indicate that it sends an 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.
I feel it makes more sense to fully separate ephemeral events under their own endpoint, since they're handled completely different from timeline events - which is what /send
sends, as well as state events - which are sent by /state
.
But an argument could definitely be made towards handling them like device-specific events, with a /sendEphemeral
endpoint.
|
||
The current ephemeral event system in Matrix is built on a sort-of guaranteed delivery - albeit with | ||
mutation/consolidation - of the events. This might not be desirable with custom ephemeral events, as | ||
they could contain volumes of data that's not as easy to keep around for a guaranteed delivery. |
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 wonder if it might make more sense to remove the mutation/consolidation expectation?
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.
Yes, replacing the built-in m.read
/m.presence
/etc with proper types that aren't munged by the server is indeed something that could be done, but I'm writing this MSC entirely towards the addition of user-defined EDUs and nothing else - leaving the already existing EDU types as they are.
|
||
As the messages in question are ephemeral, I think the only guarantee that should be required is that | ||
all users that are online when the message is sent will receive it. Anything above that should be | ||
commended, but not required. |
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.
How does one define an "online" user?
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 tried to avoid putting any specific requirements/definitions on what the best-effort guarantee should mean, hence why I'm using some more vague terms like "online".
I originally wrote the best-effort definition to be for any user that has an active sync request, but it ended up feeling rather arbitrary (not to mention leaving open the question of what would happen to EDUs that arrive while the user's client is busy processing the sync response), and also didn't really match with some of the user stories I had in mind while writing the MSC.
If I were to write my own Matrix server, with custom EDUs, I'd probably consider a size and time gated buffer for them, where all user sessions with an active sync request are guaranteed to receive the EDU, and any user session that starts a new sync request within the time gate - or before the buffer rolls over - may also receive it.
But that's probably quite different from how other servers would do it.
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.
When I was thinking about how to implement this a year or so ago, I had come up with the idea to let the sender of the EDU choose their delivery guarantee from a pre-defined set. The homeserver could either store it indefinitely until the receiver comes online (think to-device messages), or allow the receiver to miss it if they're not currently syncing (i.e. ephemeral live location events). In the latter case, the homeserver may only hold that data for 1m or so before expiring it. The same would apply to federation if the remote homeserver were down.
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 the only guarantee that should be required is that
all users that are online when the message is sent will receive it.
This is a weak guarantee which will make a bad UX as if you join a room sending print updates, you need to wait N seconds for it to appear, assuming perfect network connectivity. Having a stronger guarantee of "last write is shown" feels like a better UX for the majority of use cases which want custom EDUs (e.g location sharing). This is bounded data if you replace the EDU based on (sender, type) and have a restricted set of types in your power levels event.
Why would anyone store, or even deliver, all updates on this? Maybe it would make sens for EDUs to only transmit the last value per EDU type. So, a server could store the contents of an EDU type until it can deliver to the recipient, and overwrite the content for any subsequent updates on the EDU received from the sender. |
of the spec. | ||
|
||
|
||
## Unstable Prefix |
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.
Missing "Securitiy Considerations".
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.
There's significant questions here around authenticity as EDUs are not signed.
admins to handle abuse that occur through the use of the feature. | ||
The proposal's suggested changes to power levels - and limitation of what event types can be sent - | ||
should mitigate the potential of abuse from the feature though, as long as admins don't allow any | ||
user-defined ephemeral types to be sent by regular users. |
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 feels suboptimal because it means the room admin needs to know the entire superset of features of all potential clients which may join the room. Consider:
- I create a room on a terminal client that has no knowledge of any of these custom EDUs.
- 6 people join all on clients capable of showing printer status updates.
- See how sad they become as they can't use the feature.
To resolve this requires a fair bit of social work as either:
- someone asks the admin to allow the custom type (assuming the client they are using let's them do this)
- someone asks the admin to make one of the 6 users a mod/admin/to have enough power for their client to automatically (or manually) enable the sending of that custom type.
Rendered