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

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented Jan 14, 2020

@ericmigi
Copy link

bumping this up as it would help out of a ton of mx-puppet-slack users! :)

@MRAAGH
Copy link

MRAAGH commented Mar 3, 2020

I really miss this while using mx-puppet-discord. Typing notifications add so much extra value to a chat!

@auscompgeek
Copy link
Contributor

It seems to me that it would be worth making appservices opt into receiving EDUs to avoid pushing EDUs to an appservice that doesn't care about them.

@Half-Shot
Copy link
Contributor

@auscompgeek would be even better if appservices could add a filter to their registration data to filter out anything they don't care about.

@turt2live turt2live self-requested a review March 27, 2020 02:49
@Sorunome
Copy link
Contributor Author

Sorunome commented Apr 3, 2020

@Half-Shot maybe in general add the filter object to the registration file? That way PDUs could also be filtered. That sounds like it should be a separate MSC, though

@Half-Shot
Copy link
Contributor

@Half-Shot maybe in general add the filter object to the registration file? That way PDUs could also be filtered. That sounds like it should be a separate MSC, though

It was one from a while ago, but we dropped at the time as the performance gains from doing so weren't worth it.

@Sorunome
Copy link
Contributor Author

Sorunome commented Apr 3, 2020

@Half-Shot maybe in general add the filter object to the registration file? That way PDUs could also be filtered. That sounds like it should be a separate MSC, though

It was one from a while ago, but we dropped at the time as the performance gains from doing so weren't worth it.

Does it sound worthy to bring that up again, in relation with this MSC, and then leave filtering to, well, the filters?

@Half-Shot
Copy link
Contributor

Yup. I think it's worth noting that we might need filters, but at this point I think that's a seperate concern. This MSC is facilitating the ability for ASes to recieve EDU traffic, another MSC can handle filtering of that traffic.

@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
proposals/2409-appservice-edus.md Outdated Show resolved Hide resolved
proposals/2409-appservice-edus.md Outdated Show resolved Hide resolved
proposals/2409-appservice-edus.md Outdated Show resolved Hide resolved
proposals/2409-appservice-edus.md Outdated Show resolved Hide resolved
@Half-Shot
Copy link
Contributor

Half-Shot commented Oct 1, 2020

Implementing backend at matrix-org/synapse#8437 (available in 1.22.0)
Implementing bridge side at matrix-org/matrix-appservice-node#32 (available in 0.6.0)

@tulir
Copy link
Member

tulir commented Oct 18, 2020

mautrix-python impl: mautrix/python@ee74e17

yingziwu added a commit to yingziwu/synapse that referenced this pull request Oct 3, 2023
No significant changes since 1.93.0rc1.

The following issues are fixed in 1.93.0 (and RCs).

- [GHSA-4f74-84v3-j9q5](GHSA-4f74-84v3-j9q5) / [CVE-2023-41335](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-41335) — Low Severity

  Temporary storage of plaintext passwords during password changes.

- [GHSA-7565-cq32-vx2x](GHSA-7565-cq32-vx2x) / [CVE-2023-42453](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-42453) — Low Severity

  Improper validation of receipts allows forged read receipts.

See the advisories for more details. If you have any questions, email security@matrix.org.

- Add automatic purge after all users have forgotten a room. ([\matrix-org#15488](matrix-org#15488))
- Restore room purge/shutdown after a Synapse restart. ([\matrix-org#15488](matrix-org#15488))
- Support resolving homeservers using `matrix-fed` DNS SRV records from [MSC4040](matrix-org/matrix-spec-proposals#4040). ([\matrix-org#16137](matrix-org#16137))
- Add the ability to use `G` (GiB) and `T` (TiB) suffixes in configuration options that refer to numbers of bytes. ([\matrix-org#16219](matrix-org#16219))
- Add span information to requests sent to appservices. Contributed by MTRNord. ([\matrix-org#16227](matrix-org#16227))
- Add the ability to enable/disable registrations when using CAS. Contributed by Aurélien Grimpard. ([\matrix-org#16262](matrix-org#16262))
- Allow the `/notifications` endpoint to be routed to workers. ([\matrix-org#16265](matrix-org#16265))
- Enable users to easily unsubscribe to notifications emails via the `List-Unsubscribe` header. ([\matrix-org#16274](matrix-org#16274))
- Report whether a user is `locked` in the [List Accounts admin API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#list-accounts), and exclude locked users by default. ([\matrix-org#16328](matrix-org#16328))

- Fix a long-standing bug where multi-device accounts could cause high load due to presence. ([\matrix-org#16066](matrix-org#16066), [\matrix-org#16170](matrix-org#16170), [\matrix-org#16171](matrix-org#16171), [\matrix-org#16172](matrix-org#16172), [\matrix-org#16174](matrix-org#16174))
- Fix a long-standing bug where appservices using [MSC2409](matrix-org/matrix-spec-proposals#2409) to receive `to_device` messages would only get messages for one user. ([\matrix-org#16251](matrix-org#16251))
- Fix bug when using workers where Synapse could end up re-requesting the same remote device repeatedly. ([\matrix-org#16252](matrix-org#16252))
- Fix long-standing bug where we kept re-requesting a remote server's key repeatedly, potentially causing delays in receiving events over federation. ([\matrix-org#16257](matrix-org#16257))
- Avoid temporary storage of sensitive information. ([\matrix-org#16272](matrix-org#16272))
- Fix bug introduced in Synapse 1.49.0 when using dehydrated devices ([MSC2697](matrix-org/matrix-spec-proposals#2697)) and refresh tokens. Contributed by Hanadi. ([\matrix-org#16288](matrix-org#16288))
- Fix a long-standing bug where invalid receipts would be accepted. ([\matrix-org#16327](matrix-org#16327))
- Use standard name for UTF-8 charset in emails. ([\matrix-org#16329](matrix-org#16329))
- Don't try refetching device lists for users on remote hosts that are marked as "down". ([\matrix-org#16298](matrix-org#16298))

- Fix typos in the documentation. ([\matrix-org#16282](matrix-org#16282))
- Link to the Alpine Linux community package for Synapse. ([\matrix-org#16304](matrix-org#16304))
- Use string for `federation_client_minimum_tls_version` documentation examples. Contributed by @jcgruenhage. ([\matrix-org#16353](matrix-org#16353))

- Allow modules to delete rooms. ([\matrix-org#15997](matrix-org#15997))
- Add GCC and GNU Make to the Nix flake development environment so that `ruff` can be compiled. ([\matrix-org#16090](matrix-org#16090), [\matrix-org#16263](matrix-org#16263))
- Fix type checking when using the new version of Twisted. ([\matrix-org#16235](matrix-org#16235))
- Delete device messages asynchronously and in staged batches using the task scheduler. ([\matrix-org#16240](matrix-org#16240), [\matrix-org#16311](matrix-org#16311), [\matrix-org#16312](matrix-org#16312), [\matrix-org#16313](matrix-org#16313))
- Bump minimum supported Rust version to 1.61.0. ([\matrix-org#16248](matrix-org#16248))
- Update rust to version 1.71.1 in the nix development environment. ([\matrix-org#16260](matrix-org#16260))
- Simplify server key storage. ([\matrix-org#16261](matrix-org#16261))
- Reduce CPU overhead of change password endpoint. ([\matrix-org#16264](matrix-org#16264))
- Stop purging from tables slated for removal. ([\matrix-org#16273](matrix-org#16273))
- Improve type hints. ([\matrix-org#16276](matrix-org#16276), [\matrix-org#16301](matrix-org#16301), [\matrix-org#16325](matrix-org#16325), [\matrix-org#16326](matrix-org#16326))
- Raise `setuptools_rust` version cap to 1.7.0. ([\matrix-org#16277](matrix-org#16277))
- Fix using the new task scheduler causing lots of CPU to be used. ([\matrix-org#16278](matrix-org#16278))
- Upgrade CI run of Python 3.12 from rc1 to rc2. ([\matrix-org#16280](matrix-org#16280))
- Include values in SQL debug when using `execute_values` with Postgres. ([\matrix-org#16281](matrix-org#16281))
- Enable additional linting checks. ([\matrix-org#16283](matrix-org#16283))
- Refactor `receipts_graph` Postgres transactions to stop error messages. ([\matrix-org#16299](matrix-org#16299))
- Small improvements to logging in replication code. ([\matrix-org#16309](matrix-org#16309))
- Remove a reference cycle in background processes. ([\matrix-org#16314](matrix-org#16314))
- Only use literal strings for background process names. ([\matrix-org#16315](matrix-org#16315))
- Refactor `get_user_by_id`. ([\matrix-org#16316](matrix-org#16316))
- Speed up task to delete to-device messages. ([\matrix-org#16318](matrix-org#16318))
- Avoid patching code in tests. ([\matrix-org#16349](matrix-org#16349))
- Test against PostgreSQL 16. ([\matrix-org#16351](matrix-org#16351))

* Bump mypy from 1.4.1 to 1.5.1. ([\matrix-org#16300](matrix-org#16300))
* Bump black from 23.7.0 to 23.9.1. ([\matrix-org#16295](matrix-org#16295))
* Bump docker/build-push-action from 4 to 5. ([\matrix-org#16336](matrix-org#16336))
* Bump docker/login-action from 2 to 3. ([\matrix-org#16339](matrix-org#16339))
* Bump docker/metadata-action from 4 to 5. ([\matrix-org#16337](matrix-org#16337))
* Bump docker/setup-qemu-action from 2 to 3. ([\matrix-org#16338](matrix-org#16338))
* Bump furo from 2023.8.19 to 2023.9.10. ([\matrix-org#16340](matrix-org#16340))
* Bump gitpython from 3.1.32 to 3.1.35. ([\matrix-org#16267](matrix-org#16267), [\matrix-org#16279](matrix-org#16279))
* Bump mypy-zope from 1.0.0 to 1.0.1. ([\matrix-org#16291](matrix-org#16291))
* Bump pillow from 10.0.0 to 10.0.1. ([\matrix-org#16344](matrix-org#16344))
* Bump regex from 1.9.4 to 1.9.5. ([\matrix-org#16233](matrix-org#16233))
* Bump ruff from 0.0.286 to 0.0.290. ([\matrix-org#16342](matrix-org#16342))
* Bump serde_json from 1.0.105 to 1.0.107. ([\matrix-org#16296](matrix-org#16296), [\matrix-org#16345](matrix-org#16345))
* Bump twisted from 22.10.0 to 23.8.0. ([\matrix-org#16235](matrix-org#16235))
* Bump types-pillow from 10.0.0.2 to 10.0.0.3. ([\matrix-org#16293](matrix-org#16293))
* Bump types-setuptools from 68.0.0.3 to 68.2.0.0. ([\matrix-org#16292](matrix-org#16292))
* Bump typing-extensions from 4.7.1 to 4.8.0. ([\matrix-org#16341](matrix-org#16341))
@Half-Shot
Copy link
Contributor

Hey, it's 2023 (well, it's nearly 2024 but don't let that get you down)!

I'm thinking of picking this one up, as it's a few years later and we're (matrix.org bridges / Element integrations) are using this MSC a lot, which is generally a good indicator that we like what it does and it works well enough.

I think there are probably two tracks here to be cleaned up, which are:

Are we happy with the general format of the transaction body (type v.s. edu_type).

I'm generally of the opinion that since this landed in Synapse experimentally, we've managed to implement this without much pain and what is spec'd seems to work.

Is it performant?

This is harder to answer, but we did run the libera.chat bridge for a number of years with this enabled and Element runs several servers with this feature turned on so I think in a lot of cases it can be performant. There are a few outstanding Synapse bugs, but my feeling is these are implementation problems / details and do not pertain to the described spec.

In short, I think we're in a place to start considering this as a proper spec now. We've got implementations, we've got happy usage out of it. The next steps I believe are to hear what the consensus is from the spec team and wider.

@tulir
Copy link
Member

tulir commented Oct 20, 2023

I'm generally of the opinion that since this landed in Synapse experimentally, we've managed to implement this without much pain and what is spec'd seems to work.

It's implemented as type everywhere, but the proposal text still says edu_type 🤔

@Half-Shot
Copy link
Contributor

I don't think it's worth trying to rewrite the world, let's just fix the proposal.

Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request Oct 27, 2023
No significant changes since 1.93.0rc1.

The following issues are fixed in 1.93.0 (and RCs).

- [GHSA-4f74-84v3-j9q5](GHSA-4f74-84v3-j9q5) / [CVE-2023-41335](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-41335) — Low Severity

  Temporary storage of plaintext passwords during password changes.

- [GHSA-7565-cq32-vx2x](GHSA-7565-cq32-vx2x) / [CVE-2023-42453](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-42453) — Low Severity

  Improper validation of receipts allows forged read receipts.

See the advisories for more details. If you have any questions, email security@matrix.org.

- Add automatic purge after all users have forgotten a room. ([\matrix-org#15488](matrix-org#15488))
- Restore room purge/shutdown after a Synapse restart. ([\matrix-org#15488](matrix-org#15488))
- Support resolving homeservers using `matrix-fed` DNS SRV records from [MSC4040](matrix-org/matrix-spec-proposals#4040). ([\matrix-org#16137](matrix-org#16137))
- Add the ability to use `G` (GiB) and `T` (TiB) suffixes in configuration options that refer to numbers of bytes. ([\matrix-org#16219](matrix-org#16219))
- Add span information to requests sent to appservices. Contributed by MTRNord. ([\matrix-org#16227](matrix-org#16227))
- Add the ability to enable/disable registrations when using CAS. Contributed by Aurélien Grimpard. ([\matrix-org#16262](matrix-org#16262))
- Allow the `/notifications` endpoint to be routed to workers. ([\matrix-org#16265](matrix-org#16265))
- Enable users to easily unsubscribe to notifications emails via the `List-Unsubscribe` header. ([\matrix-org#16274](matrix-org#16274))
- Report whether a user is `locked` in the [List Accounts admin API](https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#list-accounts), and exclude locked users by default. ([\matrix-org#16328](matrix-org#16328))

- Fix a long-standing bug where multi-device accounts could cause high load due to presence. ([\matrix-org#16066](matrix-org#16066), [\matrix-org#16170](matrix-org#16170), [\matrix-org#16171](matrix-org#16171), [\matrix-org#16172](matrix-org#16172), [\matrix-org#16174](matrix-org#16174))
- Fix a long-standing bug where appservices using [MSC2409](matrix-org/matrix-spec-proposals#2409) to receive `to_device` messages would only get messages for one user. ([\matrix-org#16251](matrix-org#16251))
- Fix bug when using workers where Synapse could end up re-requesting the same remote device repeatedly. ([\matrix-org#16252](matrix-org#16252))
- Fix long-standing bug where we kept re-requesting a remote server's key repeatedly, potentially causing delays in receiving events over federation. ([\matrix-org#16257](matrix-org#16257))
- Avoid temporary storage of sensitive information. ([\matrix-org#16272](matrix-org#16272))
- Fix bug introduced in Synapse 1.49.0 when using dehydrated devices ([MSC2697](matrix-org/matrix-spec-proposals#2697)) and refresh tokens. Contributed by Hanadi. ([\matrix-org#16288](matrix-org#16288))
- Fix a long-standing bug where invalid receipts would be accepted. ([\matrix-org#16327](matrix-org#16327))
- Use standard name for UTF-8 charset in emails. ([\matrix-org#16329](matrix-org#16329))
- Don't try refetching device lists for users on remote hosts that are marked as "down". ([\matrix-org#16298](matrix-org#16298))

- Fix typos in the documentation. ([\matrix-org#16282](matrix-org#16282))
- Link to the Alpine Linux community package for Synapse. ([\matrix-org#16304](matrix-org#16304))
- Use string for `federation_client_minimum_tls_version` documentation examples. Contributed by @jcgruenhage. ([\matrix-org#16353](matrix-org#16353))

- Allow modules to delete rooms. ([\matrix-org#15997](matrix-org#15997))
- Add GCC and GNU Make to the Nix flake development environment so that `ruff` can be compiled. ([\matrix-org#16090](matrix-org#16090), [\matrix-org#16263](matrix-org#16263))
- Fix type checking when using the new version of Twisted. ([\matrix-org#16235](matrix-org#16235))
- Delete device messages asynchronously and in staged batches using the task scheduler. ([\matrix-org#16240](matrix-org#16240), [\matrix-org#16311](matrix-org#16311), [\matrix-org#16312](matrix-org#16312), [\matrix-org#16313](matrix-org#16313))
- Bump minimum supported Rust version to 1.61.0. ([\matrix-org#16248](matrix-org#16248))
- Update rust to version 1.71.1 in the nix development environment. ([\matrix-org#16260](matrix-org#16260))
- Simplify server key storage. ([\matrix-org#16261](matrix-org#16261))
- Reduce CPU overhead of change password endpoint. ([\matrix-org#16264](matrix-org#16264))
- Stop purging from tables slated for removal. ([\matrix-org#16273](matrix-org#16273))
- Improve type hints. ([\matrix-org#16276](matrix-org#16276), [\matrix-org#16301](matrix-org#16301), [\matrix-org#16325](matrix-org#16325), [\matrix-org#16326](matrix-org#16326))
- Raise `setuptools_rust` version cap to 1.7.0. ([\matrix-org#16277](matrix-org#16277))
- Fix using the new task scheduler causing lots of CPU to be used. ([\matrix-org#16278](matrix-org#16278))
- Upgrade CI run of Python 3.12 from rc1 to rc2. ([\matrix-org#16280](matrix-org#16280))
- Include values in SQL debug when using `execute_values` with Postgres. ([\matrix-org#16281](matrix-org#16281))
- Enable additional linting checks. ([\matrix-org#16283](matrix-org#16283))
- Refactor `receipts_graph` Postgres transactions to stop error messages. ([\matrix-org#16299](matrix-org#16299))
- Small improvements to logging in replication code. ([\matrix-org#16309](matrix-org#16309))
- Remove a reference cycle in background processes. ([\matrix-org#16314](matrix-org#16314))
- Only use literal strings for background process names. ([\matrix-org#16315](matrix-org#16315))
- Refactor `get_user_by_id`. ([\matrix-org#16316](matrix-org#16316))
- Speed up task to delete to-device messages. ([\matrix-org#16318](matrix-org#16318))
- Avoid patching code in tests. ([\matrix-org#16349](matrix-org#16349))
- Test against PostgreSQL 16. ([\matrix-org#16351](matrix-org#16351))

* Bump mypy from 1.4.1 to 1.5.1. ([\matrix-org#16300](matrix-org#16300))
* Bump black from 23.7.0 to 23.9.1. ([\matrix-org#16295](matrix-org#16295))
* Bump docker/build-push-action from 4 to 5. ([\matrix-org#16336](matrix-org#16336))
* Bump docker/login-action from 2 to 3. ([\matrix-org#16339](matrix-org#16339))
* Bump docker/metadata-action from 4 to 5. ([\matrix-org#16337](matrix-org#16337))
* Bump docker/setup-qemu-action from 2 to 3. ([\matrix-org#16338](matrix-org#16338))
* Bump furo from 2023.8.19 to 2023.9.10. ([\matrix-org#16340](matrix-org#16340))
* Bump gitpython from 3.1.32 to 3.1.35. ([\matrix-org#16267](matrix-org#16267), [\matrix-org#16279](matrix-org#16279))
* Bump mypy-zope from 1.0.0 to 1.0.1. ([\matrix-org#16291](matrix-org#16291))
* Bump pillow from 10.0.0 to 10.0.1. ([\matrix-org#16344](matrix-org#16344))
* Bump regex from 1.9.4 to 1.9.5. ([\matrix-org#16233](matrix-org#16233))
* Bump ruff from 0.0.286 to 0.0.290. ([\matrix-org#16342](matrix-org#16342))
* Bump serde_json from 1.0.105 to 1.0.107. ([\matrix-org#16296](matrix-org#16296), [\matrix-org#16345](matrix-org#16345))
* Bump twisted from 22.10.0 to 23.8.0. ([\matrix-org#16235](matrix-org#16235))
* Bump types-pillow from 10.0.0.2 to 10.0.0.3. ([\matrix-org#16293](matrix-org#16293))
* Bump types-setuptools from 68.0.0.3 to 68.2.0.0. ([\matrix-org#16292](matrix-org#16292))
* Bump typing-extensions from 4.7.1 to 4.8.0. ([\matrix-org#16341](matrix-org#16341))

# -----BEGIN PGP SIGNATURE-----
#
# iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmUS8iEQHGVyaWtAbWF0
# cml4Lm9yZwAKCRClQuTtGw+sCXFgB/912+T+BydS290UECCXp9kpRB5xo3aWe8mX
# NCx9Oor1TRLBpLhlQWk786gP1Q9JAQpmA4z6kovjKaLG1b4oLbZNjbPG4hEYc8ow
# /rVzGor52pfyS7uS5GW+rRmapcw4AYND6hA9XGELupf2joC8LXioSCEVG4cxwD8E
# IgIbLc87C7KpaUkNbDEz3jzZ3/BVRGcIYyhF3zTK2ZApvH2qsegq8wKYx4EYJnfh
# 87DXtTCNwA+bW6XZYPtUwPKjZ+TGB11IizxmQySGLbAxvH+GUan8X8TizGyxaqaA
# FDk3yMBbUo0R7ljDgL5YsZXT6qsZz+IBz/bsMzSbZ39f/yEUqHak
# =1/pL
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue Sep 26 16:00:49 2023 BST
# gpg:                using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09
# gpg:                issuer "erik@matrix.org"
# gpg: Can't check signature: No public key

# Conflicts:
#	.github/workflows/docker.yml
#	.github/workflows/push_complement_image.yml
#	.github/workflows/release-artifacts.yml
#	.github/workflows/tests.yml
#	poetry.lock
#	synapse/appservice/scheduler.py
#	synapse/handlers/pagination.py
#	synapse/handlers/room.py
#	synapse/rest/client/account_data.py
#	tests/rest/client/test_receipts.py
@turt2live
Copy link
Member

Let's get this into the feedback engine :)

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Dec 1, 2023

This FCP proposal has been cancelled by #2409 (comment).

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Dec 1, 2023
@turt2live turt2live added this to Ready for FCP ticks in Spec Core Team Backlog Dec 21, 2023
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Dec 21, 2023
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.

}
```

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.

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

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

### 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:
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).

### 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:
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.

Comment on lines +68 to +69
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
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?


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
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.

### 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:
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?

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

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.

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

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.

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.

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.

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:

Comment on lines +188 to +192
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.
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)


## Security considerations

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

Comment on lines +227 to +228
any metadata about users. Particularly `m.presence` could be tricky, as no `room_id` is present in
that EDU.
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?

Comment on lines +205 to +207
* 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.
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

@Half-Shot
Copy link
Contributor

Just for explicitness: I'm happy to shepherd this MSC.

@turt2live
Copy link
Member

Canceling FCP while this remains a work in progress.

@mscbot fcp cancel

@mscbot mscbot added proposal-in-review and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Apr 23, 2024
@turt2live turt2live removed this from Ready for FCP ticks in Spec Core Team Backlog Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review
Projects
Status: Scheduled - v1.10
Development

Successfully merging this pull request may close these issues.

None yet