Skip to content

MSC4075: MatrixRTC invites and notifications - #4075

Open
toger5 wants to merge 54 commits into
mainfrom
toger5/matrixrtc-call-ringing
Open

MSC4075: MatrixRTC invites and notifications#4075
toger5 wants to merge 54 commits into
mainfrom
toger5/matrixrtc-call-ringing

Conversation

@toger5

@toger5 toger5 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

Rendered

To-do:

Dependencies:

Implementations:

Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
@turt2live turt2live changed the title MatrixRTC Call Ringing MSC4075: MatrixRTC Call Ringing Nov 8, 2023
Comment thread proposals/4075-call-notify-event.md Outdated
@turt2live turt2live added A-VOIP Any VoIP or MatrixRTC related proposals proposal A matrix spec change proposal. Process state. kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Nov 8, 2023
Signed-off-by: Timo K <toger5@hotmail.de>
Comment thread proposals/4075-call-notify-event.md Outdated
Comment thread proposals/4075-call-notify-event.md Outdated
Comment thread proposals/4075-call-notify-event.md Outdated
Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 22, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 22, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 24, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 24, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 24, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
toger5 added a commit to toger5/ruma that referenced this pull request Nov 24, 2023
See: [MSC4075]( matrix-org/matrix-spec-proposals#4075)

Signed-off-by: Timo K <toger5@hotmail.de>
@uhoreg

uhoreg commented Jan 24, 2024

Copy link
Copy Markdown
Member

@toger5 You've added some implementations in the PR description. Do these implement the MSC in full, or are there parts of the MSC that are still lacking implementation?

@toger5

toger5 commented Jan 24, 2024

Copy link
Copy Markdown
Contributor Author

Yes it's implemented in full. At least on the platforms shown in the description (EW).

There are optional configurations that are still missing. (Allowing to send a notification to a specific subset of users) but the API for this is already part of the SDK. There is no ui for this however and we are not sure we want that in EW.

Comment thread proposals/4075-call-notify-event.md Outdated
Signed-off-by: Timo K <toger5@hotmail.de>
Comment thread proposals/4075-call-notify-event.md Outdated
Comment thread proposals/4075-call-notify-event.md Outdated
Comment thread proposals/4075-call-notify-event.md Outdated
Comment thread proposals/4075-rtc-notification-event.md Outdated
Comment thread proposals/4075-rtc-notification-event.md Outdated
Comment on lines +118 to +121
- The `lifetime`, as measured from `sender_ts` and capped to 2 minutes, has not elapsed. If
`sender_ts` is more than 20 seconds ahead of `origin_server_ts`, the `lifetime` SHOULD be
measured from `origin_server_ts` instead. This limits the impact of a malicious user faking
`sender_ts` to trigger long-lived notifications.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the remote server could choose to put origin_server_ts in the future as well, so I think this ultimately needs to be limited by the receipt time on the client. (Ideally it would be limited by the receipt time on the local server, so there's no risk of a notification that "comes back from the dead" every time you log in a new device… but an MSC fixing age / introducing a new received_ts thing is of course a whole nother project)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good point. The 2 minute cap would still apply there, too, though. So I think both malicious users and malicious servers could only cause 2-minute rings – which I think is fine?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 minute cap would still apply there, too, though.

Ah, I guess I can read "The lifetime, as measured from sender_ts and capped to 2 minutes, has not elapsed" that way, yes. My initial reading led me to think this was instructing to use stop_ringing_ts = origin_server_ts + min(lifetime, 2 minutes) rather than ring_duration = min(origin_server_ts + lifetime - now, 2 minutes).

Comment on lines +122 to +123
- `m.mentions` is either empty, missing or contains the client's user ID (either directly or
through a room mention).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this redundant to the "push rules produce an action of notify" condition?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, something is wrong here. 🤔

I think we should separate validating invites from invite notifications. The latter only determine whether you get a ping / ring. But you should be able to also act on invites in a muted room when you observe them. So the validity rules here should only determine whether an invite is actionable for you.

For that, I think, we'd need the m.mentions condition because an invited directed at another user should not be actionable to you. But we should drop the notify condition from above because you that only determines whether you were notified.

Does that make sense?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense!

Comment on lines +108 to +134
In line with the expected behaviour of sending clients that was outlined in the previous section,
a receiving client SHOULD only consider an invite valid as long as all of the following conditions
apply:

- The invite is the current invite entry in the ephemeral sticky events map for the sender
and slot and not a withdrawal (that is, an invite event whose `content` is empty except
for `sticky_key`).
- The client's current [push rules] produce an action of `notify` for the event.
- An `m.rtc.slot` event with `state_key = slot_id` and `status = "open"` exists in the room
where the invite was received.
- The `lifetime`, as measured from `sender_ts` and capped to 2 minutes, has not elapsed. If
`sender_ts` is more than 20 seconds ahead of `origin_server_ts`, the `lifetime` SHOULD be
measured from `origin_server_ts` instead. This limits the impact of a malicious user faking
`sender_ts` to trigger long-lived notifications.
- `m.mentions` is either empty, missing or contains the client's user ID (either directly or
through a room mention).
- The user is not already joined to the same slot via a corresponding `m.rtc.member` event.

If the invite is valid, the receiving client has three options:

1. It can accept the invite by joining the slot with an appropriate `m.rtc.member` event as
per [MSC4143]. Once the event is observed by other devices of the user, it invalidates the
invite.
1. It can decline the invite by sending an `m.rtc.decline` event. Again, once the event is
observed by other devices of the user, it invalidates the invite.
1. It can ignore the event by doing nothing. The invite will remain valid until either
the user accepts or declines the invite on another device or its `lifetime` has elapsed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With legacy VoIP, Element clients had a variety of issues with stale invites coming back from the dead on client startup or in cases of poor push/sync performance. I see a potential for client devs to fall into very similar traps with m.rtc.invite if they implement the above bullet list naively, but I think we could significantly improve the odds of a decent implementation if we essentially fold the "three options" specification at the end (join/decline/timeout) into the bullet list, and then make the "three options" explanation non-normative.

Specifically the problem I see is that clients can have gappy syncs, syncs that contain not only an invite but also a join event followed by a leave event, etc., and it's not clear what it means to "observe" a join or decline event in all these cases. Therefore it might be better to phrase the conditions in terms of "what's currently sticky" and "what's in the ephemeral map", which are more well-defined.

I had written out more of my thinking here, but it's probably easiest if I send a PR to show what I mean?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had written out more of my thinking here, but it's probably easiest if I send a PR to show what I mean?

Yes, that would be awesome. I think I haven't yet fully understood the change you propose but it'll probably get clearer then.

Comment thread proposals/4075-rtc-notification-event.md
Comment on lines +182 to +183
`.m.rule.rtc.invite_for_room` matches `m.rtc.invite` events with the `room` property of `m.mentions`
set to `true` (provided that the sender has the proper power level to trigger room notifications).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A downside of reusing m.mentions for invites is that the distinction between a room: true invite and an invite without m.mentions is pretty subtle, I don't know how you'd explain that in a user interface 😕

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah. I think we'd have the same problem if we introduced a separate invitees property (similar to legacy VoIP's invitee)?

I would naively say that without mentions, a call shouldn't ring whereas with mentions it should. Maybe we could achieve that by removing the ring tweak from the underride rule? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switched to an invitees property, then we could rule out the existence of something like an m.mentions: { room: true } invite though, no? (Or are you imagining invitees would exist in addition to m.mentions?)

In my mind it would be ideal to have just two variants: invites with specific invitees, and invites directed at the general room. We shouldn't need a third variant that's more 'passive' than a room invite, because you can always just start a session without sending any invites. I'm not experienced enough with push rules to know whether that's doable…

Comment on lines +251 to +259
Finally, in rooms that are neither set to muted nor mentions-only, `m.rtc.invite` events with `m.mentions`
notify if the user is validly targeted via the event's `m.mentions` (via one of the two override rules)
and also if the event has no or empty `m.mentions` (via the underride rule).

| Push rule configuration | Invite with room mention | Invite with user mention | Invite without mention |
| ----------------------- | ------------------------ | ------------------------ | ---------------------- |
| Default | ✅ Notifies | ✅ Notifies | ✅ Notifies |
| Mentions-only | ✅ Notifies | ✅ Notifies | ❌ Silent |
| Muted | ❌ Silent | ❌ Silent | ❌ Silent |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would a client implement a "mentions and calls only" setting for a room?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd just need a single room-kind rule that mutes anything that isn't caught by override rules. Then the two default override rules for m.rtc.invite and the two default override rules for m.mentions catch both calls and normal mentions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also achieve calls but no mentions otherwise by using the same room-kind rule and disabling the two default override rules for m.mentions.

Or, thirdly, no calls but using the room-kind rule and setting the actions on the two override rules for m.rtc.invite to [] (which means stop processing).

Do you think it would help to spell these combinations out in the proposal?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sounds plausible, I just wanted to check that it was possible with this push rule design, as I haven't personally studied up on push rules. Unless someone else has the same question, don't think there's a particular need to include this.

Comment on lines +31 to +33
- `m.mentions`: (optional, object): A [mentions] object to optionally direct the invite at a set
of users in the room. If omitted or empty, the event is not targeted at specific users and may
be acted upon by any room member.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmartinesp raised the question of how clients are supposed to decide if and which mentions to include.

Naively, I think in DMs, you would probably always include the other user in the mention because you want to ping/ring them. In rooms, you'd use either a room-mention (if you want to ping/ring the room) or no mention at all (if you want to achieve a non-ringing / banner notification at most).

Allowing no mentions at all also helps still allow calls and invites when users lack the power level for room mentions.

All of this should probably be added to the proposal text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-VOIP Any VoIP or MatrixRTC related proposals kind:feature MSC for not-core and not-maintenance stuff matrix-2.0 Required for Matrix 2.0 (note: do not rename - used in reports/links) needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal. Process state.

Projects

Status: Tracking for review

Development

Successfully merging this pull request may close these issues.