Skip to content

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Sep 12, 2024

Rendered

Homeserver implementations:

Appservice implementations:


In line with matrix-org/matrix-spec#1700, the following disclosure applies:

I am a Software Engineer at Element. This proposal was written and published as an Element employee.


SCT stuff:

checklist

FCP tickyboxes

@sandhose sandhose changed the title Device management for application services MSC4190: Device management for application services Sep 12, 2024
@sandhose sandhose marked this pull request as ready for review September 12, 2024 13:17
@turt2live turt2live added proposal A matrix spec change proposal application services kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Sep 12, 2024
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Overall lgtm, just a couple sticking points.

yingziwu added a commit to yingziwu/synapse that referenced this pull request Dec 20, 2024
This release contains the security fixes from [v1.120.2](https://github.com/element-hq/synapse/releases/tag/v1.120.2).

- Fix release process to not create duplicate releases. ([\#18025](element-hq/synapse#18025))

- Support for [MSC4190](matrix-org/matrix-spec-proposals#4190): device management for Application Services. ([\#17705](element-hq/synapse#17705))
- Update [MSC4186](matrix-org/matrix-spec-proposals#4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members. ([\#17947](element-hq/synapse#17947))
- Use stable `M_USER_LOCKED` error code for locked accounts, as per [Matrix 1.12](https://spec.matrix.org/v1.12/client-server-api/#account-locking). ([\#17965](element-hq/synapse#17965))
- [MSC4076](matrix-org/matrix-spec-proposals#4076): Add `disable_badge_count` to pusher configuration. ([\#17975](element-hq/synapse#17975))

- Fix long-standing bug where read receipts could get overly delayed being sent over federation. ([\#17933](element-hq/synapse#17933))

- Add OIDC example configuration for Forgejo (fork of Gitea). ([\#17872](element-hq/synapse#17872))
- Link to element-docker-demo from contrib/docker*. ([\#17953](element-hq/synapse#17953))

- [MSC4108](matrix-org/matrix-spec-proposals#4108): Add a `Content-Type` header on the `PUT` response to work around a faulty behavior in some caching reverse proxies. ([\#17253](element-hq/synapse#17253))
- Fix incorrect comment in new schema delta. ([\#17936](element-hq/synapse#17936))
- Raise setuptools_rust version cap to 1.10.2. ([\#17944](element-hq/synapse#17944))
- Enable encrypted appservice related experimental features in the complement docker image. ([\#17945](element-hq/synapse#17945))
- Return whether the user is suspended when querying the user account in the Admin API. ([\#17952](element-hq/synapse#17952))
- Fix new scheduled tasks jumping the queue. ([\#17962](element-hq/synapse#17962))
- Bump pyo3 and dependencies to v0.23.2. ([\#17966](element-hq/synapse#17966))
- Update setuptools-rust and fix building abi3 wheels in latest version. ([\#17969](element-hq/synapse#17969))
- Consolidate SSO redirects through `/_matrix/client/v3/login/sso/redirect(/{idpId})`. ([\#17972](element-hq/synapse#17972))
- Fix Docker and Complement config to be able to use `public_baseurl`. ([\#17986](element-hq/synapse#17986))
- Fix building wheels for MacOS which was temporarily disabled in Synapse 1.120.2. ([\#17993](element-hq/synapse#17993))
- Fix release process to not create duplicate releases. ([\#17970](element-hq/synapse#17970), [\#17995](element-hq/synapse#17995))

* Bump bytes from 1.8.0 to 1.9.0. ([\#17982](element-hq/synapse#17982))
* Bump pysaml2 from 7.3.1 to 7.5.0. ([\#17978](element-hq/synapse#17978))
* Bump serde_json from 1.0.132 to 1.0.133. ([\#17939](element-hq/synapse#17939))
* Bump tomli from 2.0.2 to 2.1.0. ([\#17959](element-hq/synapse#17959))
* Bump tomli from 2.1.0 to 2.2.1. ([\#17979](element-hq/synapse#17979))
* Bump tornado from 6.4.1 to 6.4.2. ([\#17955](element-hq/synapse#17955))
@tulir tulir removed the implementation-needs-checking The MSC has an implementation, but the SCT has not yet checked it. label Mar 13, 2025
@tulir
Copy link
Member

tulir commented Apr 2, 2025

MSCs proposed for Final Comment Period (FCP) should meet the requirements outlined in the checklist prior to being accepted into the spec. This checklist is a bit long, but aims to reduce the number of follow-on MSCs after a feature lands.

SCT members: please check off things you check for, and raise a concern against FCP if the checklist is incomplete. If an item doesn't apply, prefer to check it rather than remove it. Unchecking items is encouraged where applicable.

Checklist:

  • Are appropriate implementation(s)
    specified in the MSC’s PR description?
  • Are all MSCs that this MSC depends on already accepted?
  • For each new endpoint that is introduced:
    • Have authentication requirements been specified?
    • Have rate-limiting requirements been specified?
    • Have guest access requirements been specified?
    • Are error responses specified?
      • Does each error case have a specified errcode (e.g. M_FORBIDDEN) and HTTP status code?
        • If a new errcode is introduced, is it clear that it is new?
  • Will the MSC require a new room version, and if so, has that been made clear?
    • Is the reason for a new room version clearly stated? For example,
      modifying the set of redacted fields changes how event IDs are calculated,
      thus requiring a new room version.
  • Are backwards-compatibility concerns appropriately addressed?
  • Are the endpoint conventions honoured?
    • Do HTTP endpoints use_underscores_like_this?
    • Will the endpoint return unbounded data? If so, has pagination been considered?
    • If the endpoint utilises pagination, is it consistent with
      the appendices?
  • An introduction exists and clearly outlines the problem being solved.
    Ideally, the first paragraph should be understandable by a non-technical audience.
  • All outstanding threads are resolved
    • All feedback is incorporated into the proposal text itself, either as a fix or noted as an alternative
  • While the exact sections do not need to be present,
    the details implied by the proposal template are covered. Namely:
    • Introduction
    • Proposal text
    • Potential issues
    • Alternatives
    • Dependencies
  • Stable identifiers are used throughout the proposal, except for the unstable prefix section
    • Unstable prefixes consider the awkward accepted-but-not-merged state
    • Chosen unstable prefixes do not pollute any global namespace (use “org.matrix.mscXXXX”, not “org.matrix”).
  • Changes have applicable Sign Off from all authors/editors/contributors
  • There is a dedicated "Security Considerations" section which detail
    any possible attacks/vulnerabilities this proposal may introduce, even if this is "None.".
    See RFC3552 for things to think about,
    but in particular pay attention to the OWASP Top Ten.

@tulir
Copy link
Member

tulir commented Apr 2, 2025

This is implemented in Synapse and most mautrix bridges. It'd be nice if it was in the same spec release as the rest of next-gen auth

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Apr 2, 2025

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

Concerns:

  • checklist not complete
  • Breaking change on register endpoint
  • This actually depends on MSC3202 device masquerading to be useful
  • lack of clarity over backwards compatibility

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 Apr 2, 2025
@tulir
Copy link
Member

tulir commented Apr 2, 2025

@mscbot concern checklist not complete

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Apr 2, 2025
@turt2live turt2live self-requested a review April 2, 2025 16:29
@github-project-automation github-project-automation bot moved this to Needs idea feedback / initial review in Spec Core Team Workflow Apr 2, 2025
@turt2live turt2live moved this from Needs idea feedback / initial review to Ready for FCP ticks in Spec Core Team Workflow Apr 2, 2025
@tulir
Copy link
Member

tulir commented Apr 5, 2025

@mscbot resolve checklist not complete

@mscbot mscbot removed the unresolved-concerns This proposal has at least one outstanding concern label Apr 5, 2025
@turt2live
Copy link
Member

@mscbot concern Breaking change on register endpoint

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Apr 5, 2025
@tulir
Copy link
Member

tulir commented May 10, 2025

@mscbot concern This actually depends on MSC3202 device masquerading to be useful

@turt2live turt2live moved this from Ready for FCP ticks to Proposed-FCP at risk in Spec Core Team Workflow Jul 8, 2025
@turt2live turt2live moved this from Proposed-FCP at risk to Ready for FCP ticks in Spec Core Team Workflow Jul 8, 2025
@turt2live
Copy link
Member

@mscbot resolve Breaking change on register endpoint

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I guess I should press this button too

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

could do with some updates now that MSC3861 is done, and some clarifications around compatibility

# MSC4190: Device management for application services

[MSC3202] allows application services to handle and send encrypted events.
One part of [MSC3202] is the ability to masquerade devices using the `device_id`
Copy link
Member

Choose a reason for hiding this comment

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

these links are broken

As all changes here only apply to application services, guest access is not
relevant.

### **`PUT /_matrix/client/v3/devices/{deviceId}`**
Copy link
Member

Choose a reason for hiding this comment

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

it would be helpful to link to the existing specs for existing endpoints, so we can see what's changing.

Comment on lines +16 to +17
Furthermore, if [MSC3861] were adopted, the `/login` endpoint would no longer be
available for application services to use.
Copy link
Member

Choose a reason for hiding this comment

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

MSC3861 has been adopted, so this sentence is outdated.

Comment on lines +55 to +57
Currently, the default behavior for `/register` is to create a new device and
access token (i.e. login) in addition to creating the user. Similar to `/login`,
creating an access token would no longer be possible with [MSC3861]. However,
Copy link
Member

Choose a reason for hiding this comment

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

This bit has also been overtaken by the events of MSC3861.

Comment on lines +61 to +63
Therefore, application services MUST call the endpoint with `inhibit_login=true`.
Calls without the parameter, or with a different value than `true`, will return
HTTP 400 with a new `M_APPSERVICE_LOGIN_UNSUPPORTED` error code.
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 see this in the sample implementation, and it feels like an unnecessary breaking change. Are you sure it is necessary to break compatibility with existing app services? Has this been tested in an implementation?

Copy link
Member

Choose a reason for hiding this comment

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

An explicit error rather than silently forcing inhibit_login was added following @turt2live's concern about silent breakage. Looks like it's missing implementation though, so that needs to be fixed in synapse

Comment on lines +67 to +69
The change to `/v3/register` is technically backwards-incompatible, but it will
break when switching to next-gen auth in any case, so a new endpoint version
would not be useful.
Copy link
Member

Choose a reason for hiding this comment

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

ah, so it is already broken on those servers that have switched to next-gen auth?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the endpoint has silently stopped returning access tokens and device ids regardless of whether inhibit_login is set

Copy link
Member

Choose a reason for hiding this comment

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

Actually looks like synapse doesn't have code for that case (MAS enabled + no MSC4190 + no inhibit_login) 🤔 I wonder if it's just exploding or returning broken access tokens instead

Copy link
Member

Choose a reason for hiding this comment

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

I would definitely like to see this compatibility story clarified in the MSC.

Comment on lines +95 to +96
Until this MSC is stable, application services must opt-in to the new behavior
by setting the `io.element.msc4190` flag to `true` in their registration file.
Copy link
Member

Choose a reason for hiding this comment

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

why is opting in necessary?

Copy link
Member

Choose a reason for hiding this comment

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

I think it opts into disabling /login and requiring inhibit_login on /register so it can be tested even without MAS? Not entirely sure, but it says it's only opt-in until the MSC is stable, so probably doesn't matter

Copy link
Member

Choose a reason for hiding this comment

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

It does matter, because the fact that there is an opt-in implies that someone is worried it will cause breakage on existing applications, and that logic still applies after the MSC is stable.

@richvdh
Copy link
Member

richvdh commented Aug 27, 2025

@mscbot concern lack of clarity over backwards compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application services disposition-merge kind:core MSC which is critical to the protocol's success proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.

10 participants