Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Half-Shot · 2022-07-20T17:35:10Z

Implements MSC3848. There isn't too much to say on this one other than I've tried to make this fairly backwards compatible as per the spec.

I haven't made this configurable, so the extra key on the error responses will always be present. I can add some logic to do that though if we think it's a hard requirement though I'm not sure how to do so in a non-invasive manner (passing a config flag to all functions that might raise this error feels clunky)

This is now configurable via the usual config flags.

synapse/api/errors.py

Half-Shot · 2022-07-20T18:10:29Z

I don't know what's in here that specifically upsets a Synapse using postgres, with workers, on Debian buster specifically but there you go.

erikjohnston

I think this makes sense to me. Will check with the team about whether we should make this configurable

erikjohnston · 2022-07-25T09:03:28Z

synapse/api/errors.py

+        return cs_error(
+            self.msg,
+            self.previous_errcode,
+            **{"org.matrix.unstable.errcode": self.errcode},


Should this key also not be namespaced to the MSC?

It could be, my main concern was the maintenance burden it puts on the client (and servers) to maintain a set of keys and values to namespace, which makes things like these error classes more cumbersome (you could argue there is a benefit to making unstable things more cumbersome though).

If we think there is a real danger of clashing with other MSCs or it just being too unusual, I can namespace them.

Well, future MSCs could reuse the MSC namespaced key, it just makes it explicitly clear where to go to find out what the key means. (Otherwise different MSCs might use subtly different definitions of the key).

Done in d1a66cb

erikjohnston · 2022-07-25T14:40:30Z

@Half-Shot so I think we do want to have this behind a config flag. I think we can add a hs: HomeServer param to error_dict, as that doesn't get called many places and where it is called have easy access to the home server object.

Half-Shot · 2022-07-25T15:59:36Z

@Half-Shot so I think we do want to have this behind a config flag. I think we can add a hs: HomeServer param to error_dict, as that doesn't get called many places and where it is called have easy access to the home server object.

This should be done, albit a using a boolean rather than the full homeserver object (to make it easier to default to False should a value not be provided)

erikjohnston

Sorry for missing this yesterday 🤦

erikjohnston · 2022-07-27T09:31:37Z

synapse/federation/federation_server.py

@@ -469,7 +470,12 @@ async def process_pdus_for_room(room_id: str) -> None:
                    )
                    for pdu in pdus_by_room[room_id]:
                        event_id = pdu.event_id
-                        pdu_results[event_id] = e.error_dict()
+                        if isinstance(e, UnstableSpecAuthError):


Bleurgh, I'm not a fan of using isinstance TBH, its a bit of a smell. I'd rather we changed the signature of error_dict across all the errors to avoid this. At which point you probably do want to pass in either the config or HS object to make the error_dict API sane.

Bleurgh, I'm not a fan of using isinstance TBH, its a bit of a smell. I'd rather we changed the signature of error_dict across all the errors to avoid this.

Happy to 👍

At which point you probably do want to pass in either the config or HS object to make the error_dict API sane.

I'll try it with the config. I could see that coming up again. It might just be prior experience but I have a natural aversion to passing in chunky classes into smaller classes.

Half-Shot · 2022-07-27T11:45:51Z

synapse/http/server.py

@@ -459,7 +453,7 @@ def _send_error_response(
        request: SynapseRequest,
    ) -> None:
        """Implements _AsyncResource._send_error_response"""
-        return_json_error(f, request)
+        return_json_error(f, request, None)


I couldn't figure out a non-invasive way of providing hs to this class so instead here we just don't support unstable error codes on anything directly subclassing DirectServeJsonResource.

A cursory glance at the code seems to indicates this isn't used by any REST API classes that interact with the auth parts of Synapse, so it's unlikely we would raise an unstable error in those parts. Most APIs directly or indirectly subclass JsonResource, which does provide a hs object.

erikjohnston

Thank you! Sorry for the faff.

…ures (matrix-org#13343) Implements MSC3848

@andrewdoh

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

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

Half-Shot requested a review from a team as a code owner July 20, 2022 17:35

Half-Shot commented Jul 20, 2022

View reviewed changes

synapse/api/errors.py Show resolved Hide resolved

Half-Shot mentioned this pull request Jul 20, 2022

MSC3848: Introduce errcodes for specific event sending failures matrix-org/matrix-spec-proposals#3848

Open

Half-Shot changed the title ~~Implement MSC3848: Introduce errcodes for specific membership change failures~~ Implement MSC3848: Introduce errcodes for specific event sending failures Jul 20, 2022

erikjohnston reviewed Jul 25, 2022

View reviewed changes

Half-Shot added 10 commits July 25, 2022 16:54

Saving changes

472d91d

Add unstable spec auth errors

366bc6d

lint

60c9cf2

prefixing

4cdb8a6

notes

f3defc6

Drop mistakenly committed changes

8d6bafa

Add NOT_JOINED

b302d34

Use msc namespace

8d08d0f

namespace

ae053f4

Make errors configurable

eb870dd

Half-Shot force-pushed the hs/already-joined-fix branch from fa91feb to eb870dd Compare July 25, 2022 15:54

tidy up if

1a97541

make lint happy

1d34f6b

Half-Shot requested a review from erikjohnston July 26, 2022 17:36

erikjohnston reviewed Jul 27, 2022

View reviewed changes

Pass in config to all error types

4f7629b

Half-Shot requested a review from erikjohnston July 27, 2022 11:17

Half-Shot added 5 commits July 27, 2022 12:31

Appease python

5b3ef13

reorder

780ed56

lint again

7ff61c9

Make homeserver config optional

584351b

Fix optionals

1c19e5f

Half-Shot commented Jul 27, 2022

View reviewed changes

mark as optional to appease code

c430045

erikjohnston approved these changes Jul 27, 2022

View reviewed changes

erikjohnston merged commit 502f075 into develop Jul 27, 2022

erikjohnston deleted the hs/already-joined-fix branch July 27, 2022 12:44

azmeuk pushed a commit to azmeuk/synapse that referenced this pull request Aug 8, 2022

Implement MSC3848: Introduce errcodes for specific event sending fail…

1bac869

…ures (matrix-org#13343) Implements MSC3848

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Half-Shot commented Jul 20, 2022 •

edited

Loading

Half-Shot commented Jul 20, 2022

erikjohnston left a comment

erikjohnston Jul 25, 2022

Half-Shot Jul 25, 2022

erikjohnston Jul 25, 2022

Half-Shot Jul 25, 2022

erikjohnston commented Jul 25, 2022

Half-Shot commented Jul 25, 2022

erikjohnston left a comment

erikjohnston Jul 27, 2022

Half-Shot Jul 27, 2022

Half-Shot Jul 27, 2022 •

edited

Loading

erikjohnston left a comment

Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Implement MSC3848: Introduce errcodes for specific event sending failures #13343

Conversation

Half-Shot commented Jul 20, 2022 • edited Loading

Half-Shot commented Jul 20, 2022

erikjohnston left a comment

Choose a reason for hiding this comment

erikjohnston Jul 25, 2022

Choose a reason for hiding this comment

Half-Shot Jul 25, 2022

Choose a reason for hiding this comment

erikjohnston Jul 25, 2022

Choose a reason for hiding this comment

Half-Shot Jul 25, 2022

Choose a reason for hiding this comment

erikjohnston commented Jul 25, 2022

Half-Shot commented Jul 25, 2022

erikjohnston left a comment

Choose a reason for hiding this comment

erikjohnston Jul 27, 2022

Choose a reason for hiding this comment

Half-Shot Jul 27, 2022

Choose a reason for hiding this comment

Half-Shot Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

Half-Shot commented Jul 20, 2022 •

edited

Loading

Half-Shot Jul 27, 2022 •

edited

Loading