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

MSC3848: Introduce errcodes for specific event sending failures #3848

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Jul 19, 2022

@Half-Shot Half-Shot changed the title MSC0000: Introduce errcodes for specific membership change failures MSC3848: Introduce errcodes for specific membership change failures Jul 19, 2022
@Half-Shot Half-Shot force-pushed the hs/errcode-specific-failure branch from 41b8a74 to a570f6a Compare July 19, 2022 16:22
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Jul 19, 2022
Note that it would not cover endpoints where trying to join a room when the
user is already joined would no-op, like `POST /_matrix/client/v3/join/{roomIdOrAlias}`.

`M_INSUFFICIENT_POWER` would be when your user does not have the specific required power level to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, this came up when thinking about other kinds of explicit errors we'd like when dealing with membership (as it would be nice to know if your user was denied inviting another user due to PLs).

Powerlevels extend beyond membership though, as they also control who can send events / redact and other fun things so it might be worth slightly extending the scope of the MSC to cover any event sending endpoints which might return a power level failure.

@Half-Shot Half-Shot changed the title MSC3848: Introduce errcodes for specific membership change failures MSC3848: Introduce errcodes for specific event sending failures Jul 20, 2022
tulir added a commit to mautrix/python that referenced this pull request Jul 21, 2022
Also includes `M_NOT_JOINED` which is not yet written in the MSC

matrix-org/matrix-spec-proposals#3848
@tulir
Copy link
Member

tulir commented Jul 21, 2022

mautrix-python implementation (includes M_NOT_JOINED): mautrix/python@d71b149

perform an action in the room.
`M_NOT_JOINED` would be when the authenticated user is not joined to a room, but attempts to perform
an action in it.
Both errcodes would cover endpoints:
Copy link
Member

Choose a reason for hiding this comment

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

Synapse also returns not in room when /leaveing a room you're not in (I'm not sure if that should actually just be a no-op without erroring, but if it returns an error, it should be included in endpoints that return M_NOT_JOINED)

Copy link
Contributor Author

@Half-Shot Half-Shot Jul 22, 2022

Choose a reason for hiding this comment

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

I tested this on Synapse and found:

  • Leaving a room that doesn't exist on the host returns "M_UNKNOWN" -> Not a known room
  • Leaving a room that I was previously in will always no-op with a 200.
  • Leaving a room on the host that I was never in will result in a M_FORBIDDEN.

I think we should consistently define the behavior for trying to leave a room you are not in, rather than having 3 different error responses (differing messages are fine) for effectively the same state. My gut feeling is that we should probably be a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


## Unstable prefix

While this MSC is not considered stable for implementation, implementations should use `org.matrix.unstable.errcode`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikjohnston raised the point in the Synapse PR that we should still namespace this field, so implementations will need to adapt.

Copy link
Member

Choose a reason for hiding this comment

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

A generic unstable errcode field would be nice so each MSC improving errors wouldn't need a different one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The counterpoint made was that other MSCs can re-use this MSC's key matrix-org/synapse#13343 (comment)

Copy link
Member

@tulir tulir Jul 27, 2022

Choose a reason for hiding this comment

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

I guess that makes sense, hopefully we remember to use that key (although a generic key could be forgot too, so namespacing makes sense either way)

Co-authored-by: Christian Paul <christianp@element.io>
@richvdh richvdh removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jul 27, 2022
@richvdh
Copy link
Member

richvdh commented Jul 27, 2022

@Half-Shot says this has an implementation and is ready for review

@richvdh richvdh added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jul 27, 2022
@richvdh
Copy link
Member

richvdh commented Jul 27, 2022

(I haven't checked the implementation)

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.

per comments, I don't believe this is ready for FCP given how quickly it is trying to move at the moment.

@@ -0,0 +1,77 @@
# MSC3848: Introduce errcodes for specific event sending failures.
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 concerned that this MSC is trying to move faster than safety allows for. While implementations can (and should) move faster than the spec, and the spec should not arbitrarily delay process, this is the sort of MSC which needs time to settle and think about the problem at hand.

Specifically, I'm concerned that this MSC hasn't taken into consideration backwards compatibility of the ecosystem: we have a very long tail of old clients which will not have had a chance to upgrade, and may never will. To help move the spec along faster than the eternal tide of old clients, we'd effectively have to consider this a breaking change.

Breaking changes to endpoints are relatively easy, but also a bit messy in implementation: we'd bump the endpoint version and declare that the error codes are only valid on the new endpoint version. Thus, clients opt-in to using the error codes. We would also deprecate the now-old endpoints to force clients to consider using the new endpoints. Eventually, we'd remove the old ones.

The complexity for implementations is that clients would need to version check the server (or possibly rely on 404-like semantics), and servers would need to specifically handle their error codes per endpoint version (it would not be legal to return the new error codes on the old endpoint version).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention isn't to move particularly fast (and doesn't have an urgent project backing it), but while there is spare bandwidth going it felt wise to keep moving on an MSC before it gets forgotten about. That said, happy to have this sit for a bit and have implementations play with it so we can get a wider understanding of whether the MSC works.

I can happily include a passage about requiring v4 (or vNext) endpoints for this errcode to be returned, otherwise servers will continue to respond with M_FORBIDDEN.

Wrt implementations, would it be better for us to continue to have an experimental errcode key returned (like the proposal currently suggests) or use a unstable version prefix like /_matrix/client/org.matrix.msc3848/room/{rId}/invite which would probably match the reality of what clients have to do?

Copy link
Member

Choose a reason for hiding this comment

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

The MSC would likely claim v4, and if for some reason something else beats it to it then we might consider bundling the two MSCs into one version, or ask that this MSC use v5 or whatever. For now though, it's much simpler to assume v4 will be the thing.

I'd encourage both implementations, I think. Not at the same time mind you, but it can be worth seeing which of the approaches works "better" for clients and what their concerns are. We might also explore alternative error code formats, as suggested in the SCT office room (array of error codes?).

Copy link
Member

Choose a reason for hiding this comment

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

+1 - I don't think we can, in good conscience, change the error codes on existing endpoints. It's frustrating, because this is very much unspecced behaviour that servers should be within their rights to change, but our continual failure to spec useful error codes means that the existing error codes become part of the de-facto spec.

I'd be more relaxed about replacing M_UNKNOWN. Anyone attaching semantic meaning to that deserves to keep both pieces when it breaks.

So yeah, new endpoint versions here, please, unless you can justify any particular change as being ok.

(OTOH: I very much applaud efforts to make the error codes more useful.)

new proposed errcode under its own field. This gives clients a chance to adapt to the
new errcode / ensure their behaviours with unexpected errcodes are acceptable.

## Alternatives
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative is making errcode an array of errors, which the client can pick from based on what errcodes it's aware of.

The drawbacks are that this is still a breaking change today, and potentially over-engineering the problem. If we expect errcodes to change infrequently, it might be better to continue to version bump the spec. The other problem is maintaining a potentially long tail of errcodes for older and older generations of the spec.

@Half-Shot Half-Shot self-assigned this Jul 28, 2022
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.

generally 👍.


## Proposal

New `errcode` would be introduced into the error body of a response
Copy link
Member

Choose a reason for hiding this comment

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

this sounds like we're adding a whole new property, which I don't think is the case?

@@ -0,0 +1,77 @@
# MSC3848: Introduce errcodes for specific event sending failures.
Copy link
Member

Choose a reason for hiding this comment

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

+1 - I don't think we can, in good conscience, change the error codes on existing endpoints. It's frustrating, because this is very much unspecced behaviour that servers should be within their rights to change, but our continual failure to spec useful error codes means that the existing error codes become part of the de-facto spec.

I'd be more relaxed about replacing M_UNKNOWN. Anyone attaching semantic meaning to that deserves to keep both pieces when it breaks.

So yeah, new endpoint versions here, please, unless you can justify any particular change as being ok.

(OTOH: I very much applaud efforts to make the error codes more useful.)

New `errcode` would be introduced into the error body of a response
(https://spec.matrix.org/v1.3/client-server-api/#standard-error-response).

`M_ALREADY_JOINED` would be fired when a membership action fails when the authenticated user
Copy link
Member

Choose a reason for hiding this comment

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

could you maybe structure this so that it's clearer which text applies to which error code? Maybe subsection headers?

Fizzadar added a commit to beeper/synapse-legacy-fork that referenced this pull request Aug 25, 2022
Synapse 1.65.0 (2022-08-16)
===========================

No significant changes since 1.65.0rc2.

Synapse 1.65.0rc2 (2022-08-11)
==============================

Internal Changes
----------------

- Revert 'Remove the unspecced `room_id` field in the `/hierarchy` response. ([\matrix-org#13365](matrix-org#13365))' to give more time for clients to update. ([\matrix-org#13501](matrix-org#13501))

Synapse 1.65.0rc1 (2022-08-09)
==============================

Features
--------

- Add support for stable prefixes for [MSC2285 (private read receipts)](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13273](matrix-org#13273))
- Add new unstable error codes `ORG.MATRIX.MSC3848.ALREADY_JOINED`, `ORG.MATRIX.MSC3848.NOT_JOINED`, and `ORG.MATRIX.MSC3848.INSUFFICIENT_POWER` described in [MSC3848](matrix-org/matrix-spec-proposals#3848). ([\matrix-org#13343](matrix-org#13343))
- Use stable prefixes for [MSC3827](matrix-org/matrix-spec-proposals#3827). ([\matrix-org#13370](matrix-org#13370))
- Add a new module API method to translate a room alias into a room ID. ([\matrix-org#13428](matrix-org#13428))
- Add a new module API method to create a room. ([\matrix-org#13429](matrix-org#13429))
- Add remote join capability to the module API's `update_room_membership` method (in a backwards compatible manner). ([\matrix-org#13441](matrix-org#13441))

Bugfixes
--------

- Update the version of the LDAP3 auth provider module included in the `matrixdotorg/synapse` DockerHub images and the Debian packages hosted on packages.matrix.org to 0.2.2. This version fixes a regression in the module. ([\matrix-org#13470](matrix-org#13470))
- Fix a bug introduced in Synapse v1.41.0 where the `/hierarchy` API returned non-standard information (a `room_id` field under each entry in `children_state`). ([\matrix-org#13365](matrix-org#13365))
- Fix a bug introduced in Synapse 0.24.0 that would respond with the wrong error status code to `/joined_members` requests when the requester is not a current member of the room. Contributed by @andrewdoh. ([\matrix-org#13374](matrix-org#13374))
- Fix bug in handling of typing events for appservices. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13392](matrix-org#13392))
- Fix a bug introduced in Synapse 1.57.0 where rooms listed in `exclude_rooms_from_sync` in the configuration file would not be properly excluded from incremental syncs. ([\matrix-org#13408](matrix-org#13408))
- Fix a bug in the experimental faster-room-joins support which could cause it to get stuck in an infinite loop. ([\matrix-org#13353](matrix-org#13353))
- Faster room joins: fix a bug which caused rejected events to become un-rejected during state syncing. ([\matrix-org#13413](matrix-org#13413))
- Faster room joins: fix error when running out of servers to sync partial state with, so that Synapse raises the intended error instead. ([\matrix-org#13432](matrix-org#13432))

Updates to the Docker image
---------------------------

- Make Docker images build on armv7 by installing cryptography dependencies in the 'requirements' stage. Contributed by Jasper Spaans. ([\matrix-org#13372](matrix-org#13372))

Improved Documentation
----------------------

- Update the 'registration tokens' page to acknowledge that the relevant MSC was merged into version 1.2 of the Matrix specification. Contributed by @moan0s. ([\matrix-org#11897](matrix-org#11897))
- Document which HTTP resources support gzip compression. ([\matrix-org#13221](matrix-org#13221))
- Add steps describing how to elevate an existing user to administrator by manipulating the database. ([\matrix-org#13230](matrix-org#13230))
- Fix wrong headline for `url_preview_accept_language` in documentation. ([\matrix-org#13437](matrix-org#13437))
- Remove redundant 'Contents' section from the Configuration Manual. Contributed by @dklimpel. ([\matrix-org#13438](matrix-org#13438))
- Update documentation for config setting `macaroon_secret_key`. ([\matrix-org#13443](matrix-org#13443))
- Update outdated information on `sso_mapping_providers` documentation. ([\matrix-org#13449](matrix-org#13449))
- Fix example code in module documentation of `password_auth_provider_callbacks`. ([\matrix-org#13450](matrix-org#13450))
- Make the configuration for the cache clearer. ([\matrix-org#13481](matrix-org#13481))

Internal Changes
----------------

- Extend the release script to automatically push a new SyTest branch, rather than having that be a manual process. ([\matrix-org#12978](matrix-org#12978))
- Make minor clarifications to the error messages given when we fail to join a room via any server. ([\matrix-org#13160](matrix-org#13160))
- Enable Complement CI tests in the 'latest deps' test run. ([\matrix-org#13213](matrix-org#13213))
- Fix long-standing bugged logic which was never hit in `get_pdu` asking every remote destination even after it finds an event. ([\matrix-org#13346](matrix-org#13346))
- Faster room joins: avoid blocking when pulling events with partially missing prev events. ([\matrix-org#13355](matrix-org#13355))
- Instrument `/messages` for understandable traces in Jaeger. ([\matrix-org#13368](matrix-org#13368))
- Remove an unused argument to `get_relations_for_event`. ([\matrix-org#13383](matrix-org#13383))
- Add a `merge-back` command to the release script, which automates merging the correct branches after a release. ([\matrix-org#13393](matrix-org#13393))
- Adding missing type hints to tests. ([\matrix-org#13397](matrix-org#13397))
- Faster Room Joins: don't leave a stuck room partial state flag if the join fails. ([\matrix-org#13403](matrix-org#13403))
- Refactor `_resolve_state_at_missing_prevs` to compute an `EventContext` instead. ([\matrix-org#13404](matrix-org#13404), [\matrix-org#13431](matrix-org#13431))
- Faster Room Joins: prevent Synapse from answering federated join requests for a room which it has not fully joined yet. ([\matrix-org#13416](matrix-org#13416))
- Re-enable running Complement tests against Synapse with workers. ([\matrix-org#13420](matrix-org#13420))
- Prevent unnecessary lookups to any external `get_event` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13435](matrix-org#13435))
- Add some tracing to give more insight into local room joins. ([\matrix-org#13439](matrix-org#13439))
- Rename class `RateLimitConfig` to `RatelimitSettings` and `FederationRateLimitConfig` to `FederationRatelimitSettings`. ([\matrix-org#13442](matrix-org#13442))
- Add some comments about how event push actions are stored. ([\matrix-org#13445](matrix-org#13445), [\matrix-org#13455](matrix-org#13455))
- Improve rebuild speed for the "synapse-workers" docker image. ([\matrix-org#13447](matrix-org#13447))
- Fix `@tag_args` being off-by-one with the arguments when tagging a span (tracing). ([\matrix-org#13452](matrix-org#13452))
- Update type of `EventContext.rejected`. ([\matrix-org#13460](matrix-org#13460))
- Use literals in place of `HTTPStatus` constants in tests. ([\matrix-org#13463](matrix-org#13463), [\matrix-org#13469](matrix-org#13469))
- Correct a misnamed argument in state res v2 internals. ([\matrix-org#13467](matrix-org#13467))
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 4, 2022
packaging changes:

summary of upstream changes:

Synapse 1.65.0 (2022-08-16)
===========================

Features
--------

- Add support for stable prefixes for [MSC2285 (private read receipts)](matrix-org/matrix-spec-proposals#2285). ([\#13273](matrix-org/synapse#13273))

- Add new unstable error codes `ORG.MATRIX.MSC3848.ALREADY_JOINED`,
  `ORG.MATRIX.MSC3848.NOT_JOINED`, and
  `ORG.MATRIX.MSC3848.INSUFFICIENT_POWER` described in
  [MSC3848](matrix-org/matrix-spec-proposals#3848).
  ([\#13343](matrix-org/synapse#13343))

- Use stable prefixes for
  [MSC3827](matrix-org/matrix-spec-proposals#3827).
  ([\#13370](matrix-org/synapse#13370))

- Add a new module API method to translate a room alias into a room
  ID. ([\#13428](matrix-org/synapse#13428))

- Add a new module API method to create a
  room. ([\#13429](matrix-org/synapse#13429))

- Add remote join capability to the module API's
  `update_room_membership` method (in a backwards compatible
  manner). ([\#13441](matrix-org/synapse#13441))
@turt2live
Copy link
Member

@Half-Shot I believe this has received the sanity review this MSC was looking for and therefore am removing it from the SCT board. If this is false, please raise it in the SCT office for reconsideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specific errcode for when a user invited to a room is already present in the room
5 participants