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
Open
Changes from 3 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
58 changes: 58 additions & 0 deletions proposals/3848-errcode-specific-failure.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# MSC3848: Introduce errcodes for specific membership change failures.

When performing an action on the C-S API, sometimes it will fail if the user is
already joined to the room. In these cases, homeservers will throw a `M_FORBIDDEN`
stating that the action wasn't successful. However, it's difficult to distinguish this
kind of failure from insufficient permission errors (or other kinds of errors). This would be
useful, as the caller can then react to the error e.g. refresh it's membership cache
if it tries to invite a user that is already joined.

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

(https://spec.matrix.org/latest/client-server-api/#standard-error-response).

`M_ALREADY_JOINED` would be fired when a membership action fails when the user
is already joined to the room.
This would cover endpoints:
- [POST /_matrix/client/v3/rooms/{roomId}/invite](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3roomsroomidinvite)
Half-Shot marked this conversation as resolved.
Show resolved Hide resolved
- [POST /_matrix/client/v3/rooms/knock/{roomIdOrAlias}](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3knockroomidoralias)
- [PUT /_matrix/client/v3/rooms/{roomId}/state/{eventType}/{stateKey}](https://spec.matrix.org/latest/client-server-api/#put_matrixclientv3roomsroomidstateeventtypestatekey)
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.

perform an action in the room.
- [POST /_matrix/client/v3/rooms/{roomId}/invite](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3roomsroomidinvite)
- [POST /_matrix/client/v3/rooms/knock/{roomIdOrAlias}](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3knockroomidoralias)
- [POST /_matrix/client/v3/rooms/{roomId}/unban](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3roomsroomidban)
- [POST /_matrix/client/v3/rooms/{roomId}/ban](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3roomsroomidban)
- [POST /_matrix/client/v3/rooms/{roomId}/kick](https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3roomsroomidkick)
- [PUT /_matrix/client/v3/rooms/{roomId}/state/{eventType}/{stateKey}](https://spec.matrix.org/latest/client-server-api/#put_matrixclientv3roomsroomidstateeventtypestatekey)

Half-Shot marked this conversation as resolved.
Show resolved Hide resolved

## Potential issues

Changing long-established error codes in Matrix will be fraught with risk, as many
clients will need updating to support the new error types. Failure to do so might lead
to unexpected behaviours or confusing error messages. However, the alternative is keeping
the non-specific error codes and having the

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


We could introduce a second field to the error body for more specific errors, but this would likely make
error handling in clients much more complicated.

## Security considerations

None.

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

as a prefix to the fields on the error body. `M_FORBIDDEN` will still be emitted as a `errcode` while the
MSC is unstable, and will be replaced when the spec stabilizes.

## Dependencies

None.