Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token` #13241

Vetchu · 2022-07-11T09:36:50Z

Signed-off-by: Jacek Kusnierz [email protected]

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Pull request includes a sign off
Code style is correct
(run the linters)

I'm not sure if allowing the optional and then throwing error is a correct way to do that, but I'll wait on your feedback.

Signed-off-by: Jacek Kusnierz <[email protected]>

DMRobertson

Tagging @richvdh for review when this gets moved forward.

changelog.d/13241.misc

synapse/handlers/identity.py

Vetchu · 2022-07-11T10:35:23Z

I might have went overboard with ask_id_server_for_third_party_invite, but it is two functions downstream from do_3pid_invite, so theoretically it will always receive id_access_token as well.

Signed-off-by: Jacek Kusnierz <[email protected]>

Vetchu · 2022-07-11T12:23:25Z

@DMRobertson I'm not sure how can I pass id_access_token directly to on_POST in room.py: is this present inside "request"? I fixed the tests to workaround this, but I'll revert them once I figure if they are the only blocker (and how to fix this error by getting id_access_token inside the on_POST function).

richvdh

Once again, thank you! A few minor suggestions, but generally looks great.

It would be quite nice to add tests (probably in tests/rest/client/test_rooms.py) that sending an invite or a room creation without id_access_token results in a 400 with M_MISSING_PARAM.

It's worth noting that we cannot merge this until the fixes to element-hq/element-web#22757 and element-hq/element-ios#6385 have had a few weeks to propagate into the ecosystem.

synapse/handlers/identity.py

synapse/handlers/room.py

tests/rest/client/test_identity.py

changelog.d/13241.removal

rich has taken over this

Co-authored-by: Richard van der Hoff <[email protected]>

synapse/handlers/identity.py

synapse/handlers/room.py

tests/rest/client/test_identity.py

synapse/rest/client/room.py

tests/rest/client/test_rooms.py

…misc/drop-v1-lookup

synapse/handlers/identity.py

synapse/rest/client/room.py

tests/rest/client/test_rooms.py

synapse/handlers/identity.py

tests/rest/client/test_rooms.py

tests/rest/client/test_shadow_banned.py

Co-authored-by: Richard van der Hoff <[email protected]>

Vetchu · 2022-08-15T13:38:44Z

@richvdh overall tests on my side pass, only mypy for other files now fails, I'll keep updating branch hoping that it is fixed

Vetchu · 2022-08-15T16:05:07Z

@richvdh tests are done correctly, please review

synapse/handlers/identity.py

synapse/handlers/room.py

synapse/rest/client/room.py

Signed-off-by: Jacek Kusnierz <[email protected]>

richvdh

nearly there I think!

richvdh · 2022-08-26T10:59:11Z

synapse/rest/client/room.py

+            if not all(key in content for key in ("id_server", "id_access_token")):
+                raise SynapseError(
+                    400,
+                    "`id_server` and `id_access_token` are required when doing 3pid invite",
+                    Codes.MISSING_PARAM,
                )


I think we should only do this check if membership_action == "invite"

richvdh · 2022-08-26T11:00:02Z

synapse/rest/client/room.py

+    def _has_3pid_invite_keys(self, content: JsonDict) -> bool:
+        return all(key in content for key in ("medium", "address"))


since you're rewriting this anyway: I think you may as well inline it. Having a separate function just makes it harder to read.

richvdh · 2022-08-26T11:04:57Z

synapse/handlers/room.py

+            if not self._has_all_3pid_keys(invite_3pid):
+                raise SynapseError(
+                    400,
+                    f"`id_server` and `id_access_token` are required when doing 3pid invite, caused by {invite_3pid}",


Suppose we have an invite_3pid with id_server and id_access_token but no medium. We will still log this error. That seems incorrect.

So if I understand correctly, medium is always optional and does not have to be in the invite? Can it be ommited or should another error be raised?

Although I think it would just have to log "all of the (4) fields need to be present in an invite, caused by:" if the if above it is correct

medium is not optional.

Okay, so in newest version all these properties are mentioned as mandatory in error message.

richvdh · 2022-08-26T11:06:36Z

synapse/handlers/room.py

+        for invite_3pid in invite_3pid_list:
+            if not self._has_all_3pid_keys(invite_3pid):
+                raise SynapseError(
+                    400,


generally we try to use http.HTTPStatus constants here (you may need to add an import):

Suggested change

400,

HTTPStatus.BAD_REQUEST,

likewise in the other place you do this.

synapse/handlers/room.py

Vetchu · 2022-08-26T14:43:19Z

@richvdh is there anything that is missing here? I think all the issues that you've mentioned in latest review round are addressed in newest commit.

richvdh

thanks very much for seeing this through!

synapse/handlers/room.py

@Vetchu

Synapse 1.67.0 (2022-09-13) =========================== This release removes using the deprecated direct TCP replication configuration for workers. Server admins should use Redis instead. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670). The minimum version of `poetry` supported for managing source checkouts is now 1.2.0. **Notice:** from the next major release (1.68.0) installing Synapse from a source checkout will require a recent Rust compiler. Those using packages or `pip install matrix-synapse` will not be affected. See the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670). **Notice:** from the next major release (1.68.0), running Synapse with a SQLite database will require SQLite version 3.27.0 or higher. (The [current minimum version is SQLite 3.22.0](https://github.com/matrix-org/synapse/blob/release-v1.67/synapse/storage/engines/sqlite.py#L69-L78).) See [matrix-org#12983](matrix-org#12983) and the [upgrade notes](https://matrix-org.github.io/synapse/v1.67/upgrade.html#upgrading-to-v1670) for more details. No significant changes since 1.67.0rc1. Synapse 1.67.0rc1 (2022-09-06) ============================== Features -------- - Support setting the registration shared secret in a file, via a new `registration_shared_secret_path` configuration option. ([\matrix-org#13614](matrix-org#13614)) - Change the default startup behaviour so that any missing "additional" configuration files (signing key, etc) are generated automatically. ([\matrix-org#13615](matrix-org#13615)) - Improve performance of sending messages in rooms with thousands of local users. ([\matrix-org#13634](matrix-org#13634)) Bugfixes -------- - Fix a bug introduced in Synapse 1.13 where the [List Rooms admin API](https://matrix-org.github.io/synapse/develop/admin_api/rooms.html#list-room-api) would return integers instead of booleans for the `federatable` and `public` fields when using a Sqlite database. ([\matrix-org#13509](matrix-org#13509)) - Fix bug that user cannot `/forget` rooms after the last member has left the room. ([\matrix-org#13546](matrix-org#13546)) - Faster Room Joins: fix `/make_knock` blocking indefinitely when the room in question is a partial-stated room. ([\matrix-org#13583](matrix-org#13583)) - Fix loading the current stream position behind the actual position. ([\matrix-org#13585](matrix-org#13585)) - Fix a longstanding bug in `register_new_matrix_user` which meant it was always necessary to explicitly give a server URL. ([\matrix-org#13616](matrix-org#13616)) - Fix the running of [MSC1763](matrix-org/matrix-spec-proposals#1763) retention purge_jobs in deployments with background jobs running on a worker by forcing them back onto the main worker. Contributed by Brad @ Beeper. ([\matrix-org#13632](matrix-org#13632)) - Fix a long-standing bug that downloaded media for URL previews was not deleted while database background updates were running. ([\matrix-org#13657](matrix-org#13657)) - Fix [MSC3030](matrix-org/matrix-spec-proposals#3030) `/timestamp_to_event` endpoint to return the correct next event when the events have the same timestamp. ([\matrix-org#13658](matrix-org#13658)) - Fix bug where we wedge media plugins if clients disconnect early. Introduced in v1.22.0. ([\matrix-org#13660](matrix-org#13660)) - Fix a long-standing bug which meant that keys for unwhitelisted servers were not returned by `/_matrix/key/v2/query`. ([\matrix-org#13683](matrix-org#13683)) - Fix a bug introduced in Synapse v1.20.0 that would cause the unstable unread counts from [MSC2654](matrix-org/matrix-spec-proposals#2654) to be calculated even if the feature is disabled. ([\matrix-org#13694](matrix-org#13694)) Updates to the Docker image --------------------------- - Update docker image to use a stable version of poetry. ([\matrix-org#13688](matrix-org#13688)) Improved Documentation ---------------------- - Improve the description of the ["chain cover index"](https://matrix-org.github.io/synapse/latest/auth_chain_difference_algorithm.html) used internally by Synapse. ([\matrix-org#13602](matrix-org#13602)) - Document how ["monthly active users"](https://matrix-org.github.io/synapse/latest/usage/administration/monthly_active_users.html) is calculated and used. ([\matrix-org#13617](matrix-org#13617)) - Improve documentation around user registration. ([\matrix-org#13640](matrix-org#13640)) - Remove documentation of legacy `frontend_proxy` worker app. ([\matrix-org#13645](matrix-org#13645)) - Clarify documentation that HTTP replication traffic can be protected with a shared secret. ([\matrix-org#13656](matrix-org#13656)) - Remove unintentional colons from [config manual](https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html) headers. ([\matrix-org#13665](matrix-org#13665)) - Update docs to make enabling metrics more clear. ([\matrix-org#13678](matrix-org#13678)) - Clarify `(room_id, event_id)` global uniqueness and how we should scope our database schemas. ([\matrix-org#13701](matrix-org#13701)) Deprecations and Removals ------------------------- - Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token`, which was not permitted by the spec. Contributed by @Vetchu. ([\matrix-org#13241](matrix-org#13241)) - Remove redundant `_get_joined_users_from_context` cache. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13569](matrix-org#13569)) - Remove the ability to use direct TCP replication with workers. Direct TCP replication was deprecated in Synapse v1.18.0. Workers now require using Redis. ([\matrix-org#13647](matrix-org#13647)) - Remove support for unstable [private read receipts](matrix-org/matrix-spec-proposals#2285). ([\matrix-org#13653](matrix-org#13653), [\matrix-org#13692](matrix-org#13692)) Internal Changes ---------------- - Extend the release script to wait for GitHub Actions to finish and to be usable as a guide for the whole process. ([\matrix-org#13483](matrix-org#13483)) - Add experimental configuration option to allow disabling legacy Prometheus metric names. ([\matrix-org#13540](matrix-org#13540)) - Cache user IDs instead of profiles to reduce cache memory usage. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13573](matrix-org#13573), [\matrix-org#13600](matrix-org#13600)) - Optimize how Synapse calculates domains to fetch from during backfill. ([\matrix-org#13575](matrix-org#13575)) - Comment about a better future where we can get the state diff between two events. ([\matrix-org#13586](matrix-org#13586)) - Instrument `_check_sigs_and_hash_and_fetch` to trace time spent in child concurrent calls for understandable traces in Jaeger. ([\matrix-org#13588](matrix-org#13588)) - Improve performance of `@cachedList`. ([\matrix-org#13591](matrix-org#13591)) - Minor speed up of fetching large numbers of push rules. ([\matrix-org#13592](matrix-org#13592)) - Optimise push action fetching queries. Contributed by Nick @ Beeper (@Fizzadar). ([\matrix-org#13597](matrix-org#13597)) - Rename `event_map` to `unpersisted_events` when computing the auth differences. ([\matrix-org#13603](matrix-org#13603)) - Refactor `get_users_in_room(room_id)` mis-use with dedicated `get_current_hosts_in_room(room_id)` function. ([\matrix-org#13605](matrix-org#13605)) - Use dedicated `get_local_users_in_room(room_id)` function to find local users when calculating `join_authorised_via_users_server` of a `/make_join` request. ([\matrix-org#13606](matrix-org#13606)) - Refactor `get_users_in_room(room_id)` mis-use to lookup single local user with dedicated `check_local_user_in_room(...)` function. ([\matrix-org#13608](matrix-org#13608)) - Drop unused column `application_services_state.last_txn`. ([\matrix-org#13627](matrix-org#13627)) - Improve readability of Complement CI logs by printing failure results last. ([\matrix-org#13639](matrix-org#13639)) - Generalise the `@cancellable` annotation so it can be used on functions other than just servlet methods. ([\matrix-org#13662](matrix-org#13662)) - Introduce a `CommonUsageMetrics` class to share some usage metrics between the Prometheus exporter and the phone home stats. ([\matrix-org#13671](matrix-org#13671)) - Add some logging to help track down matrix-org#13444. ([\matrix-org#13679](matrix-org#13679)) - Update poetry lock file for v1.2.0. ([\matrix-org#13689](matrix-org#13689)) - Add cache to `is_partial_state_room`. ([\matrix-org#13693](matrix-org#13693)) - Update the Grafana dashboard that is included with Synapse in the `contrib` directory. ([\matrix-org#13697](matrix-org#13697)) - Only run trial CI on all python versions on non-PRs. ([\matrix-org#13698](matrix-org#13698)) - Fix typechecking with latest types-jsonschema. ([\matrix-org#13712](matrix-org#13712)) - Reduce number of CI checks we run for PRs. ([\matrix-org#13713](matrix-org#13713)) # -----BEGIN PGP SIGNATURE----- # # iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmMgR2QQHGVyaWtAbWF0 # cml4Lm9yZwAKCRClQuTtGw+sCfG7B/94PwW1ChsaI8hkz/3e+93PEl/mNJ6YFaEB # 5pP4Dh/0dipP/iKbpgNuj5xz/JFnIi8D49A8sKNnku3jk0/8AZHgqDiBgOkrN76z # Y3awo5Q9ag4xww/105V3bhdnX1NrX8Avf6F2jchDv6/9q8wQHGBPg6DMgfZ/m/BL # SB4dypbbNpgLykuwtWxx6YMUYH+trsXJOn/MoAqld3QcZsqkDR25wXCt9+Dr+6AT # dPd/czi8kV8ruU59tf2K5HB7XKzBW9S3Qb3dJJmGOTTJ7ccUkN/XuTwqnII950Mo # bSlMXjY2hqk8rKUNhGZpi9bqUkwNhMgOkZl9A0Y1XtsXx6yjy0T/ # =zSGi # -----END PGP SIGNATURE----- # gpg: Signature made Tue Sep 13 10:03:32 2022 BST # gpg: using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09 # gpg: issuer "[email protected]" # gpg: Can't check signature: No public key # Conflicts: # synapse/config/experimental.py # synapse/push/bulk_push_rule_evaluator.py # synapse/storage/databases/main/event_push_actions.py # synapse/util/caches/descriptors.py

Drop v1 lookup

0f6487e

Signed-off-by: Jacek Kusnierz <[email protected]>

Vetchu requested a review from a team as a code owner July 11, 2022 09:36

Add changelog

ef1c794

Signed-off-by: Jacek Kusnierz <[email protected]>

DMRobertson previously requested changes Jul 11, 2022

View reviewed changes

changelog.d/13241.misc Outdated Show resolved Hide resolved

synapse/handlers/identity.py Outdated Show resolved Hide resolved

synapse/handlers/identity.py Outdated Show resolved Hide resolved

DMRobertson requested a review from richvdh July 11, 2022 09:50

DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jul 11, 2022

Address code review

e9c789a

Vetchu requested a review from DMRobertson July 11, 2022 10:35

Repair tests PoC.

e354ec6

Signed-off-by: Jacek Kusnierz <[email protected]>

richvdh changed the title ~~Drop v1 lookup~~ Drop support for calling /_matrix/client/v3/rooms/{roomId}/invite without an id_access_token Jul 12, 2022

richvdh suggested changes Jul 12, 2022

View reviewed changes

Vetchu and others added 3 commits July 12, 2022 11:46

Update changelog.d/13241.removal

eb5ba09

Co-authored-by: Richard van der Hoff <[email protected]>

merge develop

72313f6

Add tests, address concerns

779e78c

Vetchu requested a review from richvdh July 22, 2022 13:18

Vetchu added 3 commits July 22, 2022 15:20

minify test

5668dcd

missing comma

6e908e4

Merge branch 'develop' into misc/drop-v1-lookup

242fa48

richvdh suggested changes Jul 25, 2022

View reviewed changes

Vetchu added 3 commits July 25, 2022 20:36

Address comments

6698279

Merge branch 'misc/drop-v1-lookup' of github.com:Vetchu/synapse into …

48d88f0

…misc/drop-v1-lookup

Merge branch 'develop' into misc/drop-v1-lookup

6b1413b

Vetchu requested a review from richvdh July 25, 2022 21:18

richvdh suggested changes Jul 26, 2022

View reviewed changes

synapse/handlers/identity.py Outdated Show resolved Hide resolved

synapse/rest/client/room.py Outdated Show resolved Hide resolved

tests/rest/client/test_rooms.py Show resolved Hide resolved

synapse/handlers/identity.py Outdated Show resolved Hide resolved

richvdh reviewed Jul 26, 2022

View reviewed changes

tests/rest/client/test_rooms.py Outdated Show resolved Hide resolved

tests/rest/client/test_shadow_banned.py Outdated Show resolved Hide resolved

Vetchu and others added 2 commits August 11, 2022 16:29

Merge branch 'matrix-org:develop' into misc/drop-v1-lookup

dacd33d

Update tests/rest/client/test_rooms.py

9a04234

Co-authored-by: Richard van der Hoff <[email protected]>

add missing check

f2a74b2

Vetchu requested a review from richvdh August 15, 2022 13:37

Merge branch 'develop' into misc/drop-v1-lookup

bb84fe4

Vetchu added 4 commits August 15, 2022 16:01

Merge branch 'develop' into misc/drop-v1-lookup

8ab4b35

fixed test

f9600bb

catch missing param for 3pid

0073118

synapseerror

ebcdbba

validate earlier

b97b27f

richvdh suggested changes Aug 15, 2022

View reviewed changes

synapse/handlers/identity.py Outdated Show resolved Hide resolved

synapse/handlers/room.py Outdated Show resolved Hide resolved

synapse/handlers/room.py Outdated Show resolved Hide resolved

synapse/rest/client/room.py Outdated Show resolved Hide resolved

merge develop

23be3b9

DMRobertson removed their request for review August 23, 2022 12:07

Vetchu added 5 commits August 23, 2022 16:27

Merge remote-tracking branch 'origin' into misc/drop-v1-lookup

d1b6ae8

Next iteration

21d9be1

Signed-off-by: Jacek Kusnierz <[email protected]>

allow exception to go up

caeee4d

Merge branch 'develop' into misc/drop-v1-lookup

3792e8c

beautify code

3dfd8fb

DMRobertson requested a review from richvdh August 26, 2022 06:02

richvdh suggested changes Aug 26, 2022

View reviewed changes

hopefully addresses the comments

5b237aa

Vetchu requested a review from richvdh August 26, 2022 14:41

DMRobertson removed the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Aug 30, 2022

richvdh approved these changes Aug 31, 2022

View reviewed changes

synapse/handlers/room.py Outdated Show resolved Hide resolved

richvdh added 2 commits August 31, 2022 12:34

Update synapse/handlers/room.py

27951c5

Merge branch 'develop' into misc/drop-v1-lookup

c6aa582

richvdh enabled auto-merge (squash) August 31, 2022 11:35

richvdh merged commit 84ddcd7 into matrix-org:develop Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token` #13241

Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token` #13241

Vetchu commented Jul 11, 2022 •

edited

Loading

DMRobertson left a comment

Vetchu commented Jul 11, 2022

Vetchu commented Jul 11, 2022

richvdh left a comment

Vetchu commented Aug 15, 2022

Vetchu commented Aug 15, 2022

richvdh left a comment

richvdh Aug 26, 2022

richvdh Aug 26, 2022

richvdh Aug 26, 2022

Vetchu Aug 26, 2022

Vetchu Aug 26, 2022

richvdh Aug 26, 2022

Vetchu Aug 26, 2022

richvdh Aug 26, 2022

Vetchu commented Aug 26, 2022

richvdh left a comment

		def _has_3pid_invite_keys(self, content: JsonDict) -> bool:
		return all(key in content for key in ("medium", "address"))

Drop support for calling /_matrix/client/v3/rooms/{roomId}/invite without an id_access_token #13241

Drop support for calling /_matrix/client/v3/rooms/{roomId}/invite without an id_access_token #13241

Conversation

Vetchu commented Jul 11, 2022 • edited Loading

Pull Request Checklist

DMRobertson left a comment

Choose a reason for hiding this comment

Vetchu commented Jul 11, 2022

Vetchu commented Jul 11, 2022

richvdh left a comment

Choose a reason for hiding this comment

Vetchu commented Aug 15, 2022

Vetchu commented Aug 15, 2022

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vetchu commented Aug 26, 2022

richvdh left a comment

Choose a reason for hiding this comment

Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token` #13241

Drop support for calling `/_matrix/client/v3/rooms/{roomId}/invite` without an `id_access_token` #13241

Vetchu commented Jul 11, 2022 •

edited

Loading