Skip to content
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

MSC2409: Proposal to send EDUs to appservices #2409

Open
wants to merge 17 commits into
base: old_master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 236 additions & 0 deletions proposals/2409-appservice-edus.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,236 @@
# MSC2409: Proposal to send EDUs to appservices
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved

*Node: This proposal is a continuation of [MSC1888](https://github.com/matrix-org/matrix-doc/pull/1888)
and deprecates that one.*

The [appservice /transactions API](https://matrix.org/docs/spec/application_service/r0.1.2#put-matrix-app-v1-transactions-txnid)
currently supports pushing PDU events (regular message and state events)
however it doesn't provison for EDU events (typing, presence and more). This means that bridges cannot
react to Matrix users who send any typing or presence information in a room the service is part of.

There is an interest amongst the community to have equal bridging on both sides of a bridge, so that
read reciepts and typing notifications can be seen on the remote side. To that end, this proposal
specifies how these can be pushed to an appservice.
turt2live marked this conversation as resolved.
Show resolved Hide resolved

## Proposal

### Changes to the registration file

In order that appservices don't get flooded with EDUs, appservices have to opt-in to receive them by
setting `push_ephemeral` to true. A registration file could look like following:
turt2live marked this conversation as resolved.
Show resolved Hide resolved
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, perhaps, but since this file is describing the app service, should it be, receive_ephemeral rather than 'push'? (indeed, the comment below is phrased in this way too).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% on this but it feels slightly confusing that ephemeral here matches the new ephemeral section the body, but it also controls the presence of the to_device section. Could it be clearer to just have a separate field to control whether the AS wants to_device messages (and maybe this could actually be useful for an AS to be able to subscribe to to_device message without getting presence, etc?)

Copy link
Member

Choose a reason for hiding this comment

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

indeed, maybe we should have separate settings for each of presence, typing, device lists, etc?

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion a filter would be best suited for a future MSC. Appservices already don't have an option to filter what events (PDUs) they receive, but it would be great if they could.

"Ephemeral" here is also inherited from the server-server API rather than client-server API: it includes to-device messages, but changes shape to be more useful as a representation. We could absolutely package it into ephemeral, but appservices will already be translating it to fit their crypto engine shape anyways - might as well give them something directly compatible.

Overall, appservices being closer to federation than clients makes things weird. We should improve them, but I don't think this is the MSC to fix everything.


```yaml
id: "IRC Bridge"
url: "http://127.0.0.1:1234"
as_token: "30c05ae90a248a4188e620216fa72e349803310ec83e2a77b34fe90be6081f46"
hs_token: "312df522183efd404ec1cd22d2ffa4bbc76a8c1ccf541dd692eef281356bb74e"
sender_localpart: "_irc_bot"
# We want to receive EDUs
push_ephemeral: true
namespaces:
users:
- exclusive: true
regex: "@_irc_bridge_.*"
aliases:
- exclusive: false
regex: "#_irc_bridge_.*"
rooms: []
```

### Changes to the /transactions/ API

The `PUT /_matrix/app/v1/transactions/{txnId}` API currently supports sending PDUs
via the `events` array.

```json
{
"events": [
{
"content": {
"membership": "join",
"avatar_url": "mxc://domain.com/SEsfnsuifSDFSSEF#auto",
"displayname": "Alice Margatroid"
},
"type": "m.room.member",
"event_id": "$143273582443PhrSn:domain.com",
"room_id": "!jEsUZKDJdhlrceRyVU:domain.com",
"sender": "@example:domain.com",
"origin_server_ts": 1432735824653,
"unsigned": {
"age": 1234
},
"state_key": "@alice:domain.com"
}
]
}
```

This proposal would extend the `PUT /_matrix/app/v1/transactions/` endpoint to include a new key
`ephemeral` to behave similar to the various sections of the CS API `/sync` endpoint. The `ephemeral` key
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

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

please can to_device be mentioned here (and probably in the example), rather than introduced as a plot twist halfway through the description (which I assumed until then was going to be all about ephemeral)?

Alternatively, introduce the two arrays as separate changes with their own sections in the MSC.

Edit: oh, this has already been mentioned below

Copy link
Member

Choose a reason for hiding this comment

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

Please be explicit about exactly what sort of events are included in ephemeral. AIUI, it's typing and presence? Or are there more? Read receipts? Device lists? Account data?

MAY be omitted entirely if there are ephemeral no events to send.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MAY be omitted entirely if there are ephemeral no events to send.
MAY be omitted entirely if there are no ephemeral events to send.


```json
{
"ephemeral": [
turt2live marked this conversation as resolved.
Show resolved Hide resolved
richvdh marked this conversation as resolved.
Show resolved Hide resolved
{
"type": "m.typing",
turt2live marked this conversation as resolved.
Show resolved Hide resolved
"room_id": "!jEsUZKDJdhlrceRyVU:domain.com",
"content": {
"user_ids": [
"@alice:example.com"
]
}
},
{
"type": "m.receipt",
"room_id": "!jEsUZKDJdhlrceRyVU:domain.com",
"content": {
"$1435641916114394fHBLK:matrix.org": {
"m.read": {
"@rikj:jki.re": {
"ts": 1436451550453
}
}
}
}
}
],
"events": [
// ...
]
}
```

The reason for a new key rather than bundling the events into `events` is that
existing appservices may mistake them for PDUs and might behave erratically.
While `events` may now be a somewhat misleading name, this is an acceptable tradeoff.

Note that the EDU is otherwise formatted as it would for client-server API transport.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that the EDU is otherwise formatted as it would for client-server API transport.
Note that the EDU is otherwise formatted as it would be for client-server API transport.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be the case, based on the example above. For example, the C-S API puts typing notifications under the corresponding room, so doesn't need a separate room_id key in the m.typing notification itself.

I'm not really sure what the word "otherwise" is doing in this sentence.

Please be explicit about exactly what format each type of event should use.


To-device messages are a bit special as they are aimed at a particular user/device ID
combo. These events are annotated by the server with a `to_device_id` and `to_user_id`
Copy link
Member

Choose a reason for hiding this comment

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

Would maybe make more sense to wrap the to_device events in their own object which contains the metadata for this transport layer? ie.

{
  "to_user_id": "@_irc_bob:example.org",
  "to_device_id": "ABCDEF123",
  {
    "type": "org.example.to_device_event_type",
    "sender": "@alice:example.com",
    "content": {
      "hello": "world"
    }
  }
}

...then there's no chance of overlapping keys (although obviously this would be unlikely anyway) and the receiving server can just take the inner object out and pass it on verbatim?

Copy link
Member

Choose a reason for hiding this comment

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

you'd need a name for the property that contains the inner object

field at the top level of the message for transport to the appservice:

```json5
Copy link
Member

Choose a reason for hiding this comment

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

Github doesn't seem to format json5 as prettily as json. Can we use json or jsonc here and below?

{
"type": "org.example.to_device_event_type",
"sender": "@alice:example.com",
"to_user_id": "@_irc_bob:example.org",
"to_device_id": "ABCDEF123",
"content": {
"hello": "world"
}
}
```

Unlike other ephemeral events, to-device messages are included at a top level `to_device`
Copy link
Member

Choose a reason for hiding this comment

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

Could this get a mention back where the ephemeral key is introduced please? ie. give a summary of what's being added and then give the detail on each.

array in the transaction. If there are no messages to be sent, the array can be omitted.
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved
This is primarily due to how to-device messages work over federation: they get wrapped in
Copy link
Member

Choose a reason for hiding this comment

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

If there are no messages to be sent, the array can be omitted.
This is primarily due to how to-device messages work over federation:

I don't the fact that the array can be omitted is anything to do with the way to-device messages work over federation, fwiw.

an EDU (`m.direct_to_device`) but that parent EDU is stripped out before sending the message
off to clients. This can lead to potential conflict where if down the line we support EDUs
and to-device messages with the same event type: consumers would be uncertain as to whether
they are handling an EDU or to-device message.
Comment on lines +128 to +132
Copy link
Member

Choose a reason for hiding this comment

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

TBH in general I think the whole EDU concept was a terrible idea: each so-called EDU has its own special-casing, and the fact that the federation spec dumps them into a single edus property is a source of confusion (I've complained about this previously at matrix-org/matrix-spec#897 (comment)). We don't need to justify the fact that we are not repeating those mistakes here.

Suggest getting rid of this text.


A complete example of the transaction with all 3 arrays populated would be:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A complete example of the transaction with all 3 arrays populated would be:
A complete example of a transaction with all 3 arrays populated would be:


```json5
{
"ephemeral": [
{
"type": "m.typing",
"room_id": "!jEsUZKDJdhlrceRyVU:domain.com",
"content": {
"user_ids": [
"@alice:example.com"
]
}
}
],
"events": [
{
"content": {
"membership": "join",
"avatar_url": "mxc://domain.com/SEsfnsuifSDFSSEF#auto",
"displayname": "Alice Margatroid"
},
"type": "m.room.member",
"event_id": "$143273582443PhrSn:domain.com",
"room_id": "!jEsUZKDJdhlrceRyVU:domain.com",
"sender": "@example:domain.com",
"origin_server_ts": 1432735824653,
"unsigned": {
"age": 1234
},
"state_key": "@alice:domain.com"
}
],
"to_device": [
{
"type": "org.example.to_device_event_type",
"sender": "@alice:example.com",
"to_user_id": "@_irc_bob:example.org",
"to_device_id": "ABCDEF123",
"content": {
"hello": "world"
}
}
]
}
```

### Expectations of when an EDU should be pushed to an appservice

It is not clear at face value what should be pushed to an appservice. Appservices claim
namespaces of users which registers "interest" in the rooms where those users reside, as
well as claiming namespaces of rooms for explicit interest. However, not all EDUs are
associated with a single room (presence, etc).

If the EDU is capable of being associated to a particular room, it should be sent to the
appservice under the same rules as regular events (interest in the room means sending it).
For EDUs which are not associated with a particular room, the appservice receives the EDU
if it contextually *would* apply. For example, a presence update for a user an appservice
shares a room with (or is under the appservice's namespace) would be sent to the appservice.
Comment on lines +188 to +192
Copy link
Member

Choose a reason for hiding this comment

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

Strongly recommend writing explicit rules for each type of event, rather than handwaving about EDUs in general.

(Also, note again that EDUs don't exist as a concept at all outside the federation API)


To-device messages for devices belonging to the appservice's user namespaces should always
be sent.

### Implementation detail: when to delete a to-device message

Not defined by this MSC is an explicit algorithm for when to delete a to-device message (mark
it as sent). This is left as an implementation detail, though a suggested approach is as
follows:

* If the message is sent to a user under an appservice's *exclusive* namespace, mark it as sent
and delete it. Note that retried transactions will still need to include the message.
* If the message is sent to a user under an appservice's *inclusive* namespace, mark it as sent
to the appservice but do not delete it until a `/sync` request is completed which includes the
message. Note that retried transactions will still need to include the message.
Comment on lines +205 to +207
Copy link
Member

Choose a reason for hiding this comment

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

This seems to assume that a client somewhere is /syncing with the same device ID as the appservice? And both devices want to receive the to-device message?

I'll note that this does not yet seem to be implemented (element-hq/synapse#11423). Maybe that's ok since this is not a normative part of the MSC, but I'm a bit troubled that this is left undefined.

Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand why we can only delete messages for users under an appservice's exclusive namespace. There's probably some explanation somewhere in the depths of matrix-org/synapse#10653, but it needs explaining here imho


This approach is largely to align with how namespaces are used by appservices in practice, but
is not applicable to all scenarios (and thus is an implementation detail). The majority of known
appservices use exclusive namespaces, which typically also means that those users will not be
calling `/sync`. Because of this, the server may never get another opportunity to delete the
messages until it has confirmed that the appservice received the transaction successfully. Inclusive
namespaces are typically used when the appservice wants to impact a subset of users, but without
controlling those users explicitly. Typically, inclusive users are also calling `/sync` and so
the appservice should be CC'd on the to-device messages that would normally go down `/sync`.

## Potential issues

Determining which EDUs to transmit to the appservice could lead to quite some overhead on the
homeservers side. Additionally, more network traffic is produced, potentially straining the local
network and the appservice more. As such, appservices have to opt-in to receive EDUs.

## Security considerations
Sorunome marked this conversation as resolved.
Show resolved Hide resolved

The homeserver needs to accuratley determine which EDUs to send to the appservice, as to not leak
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The homeserver needs to accuratley determine which EDUs to send to the appservice, as to not leak
The homeserver needs to accurately determine which EDUs to send to the appservice, as to not leak

any metadata about users. Particularly `m.presence` could be tricky, as no `room_id` is present in
that EDU.
Comment on lines +227 to +228
Copy link
Member

Choose a reason for hiding this comment

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

The same is true of to-device messages?


## Unstable prefix

In the transaction body, instead of `ephemeral`, `de.sorunome.msc2409.ephemeral` is used.

In the transaction body, instead of `to_device`, `de.sorunome.msc2409.to_device` is used.

In the registration file, instead of `push_ephemeral`, `de.sorunome.msc2409.push_ephemeral` is used.