Add authentication to thirdparty bridge APIs #12746

Half-Shot · 2022-05-16T15:08:03Z

As per the spec, these APIs should have been authenticated from the beginning https://spec.matrix.org/v1.1/application-service-api/#third-party-networks

This change makes Synapse provide the bridge token to the appservice so the appservice can properly authenticate requests, so this shouldn't break any existing bridge implementations.

babolivier

Can you write a test so we ensure this doesn't break in the future please?
Also, is this going to break community bridges? (if so we need to add something in the upgrade notes)

changelog.d/12746.bugfix

Half-Shot · 2022-05-16T15:34:03Z

Also, is this going to break community bridges? (if so we need to add something in the upgrade notes)

I don't think that's likely. This is providing more information to the bridges, so they can choose to authenticate. It's only going to break if a bridge requires auth (as they should do today) and Synapse doesn't provide the information (as it does now).

Can you write a test so we ensure this doesn't break in the future please?

I couldn't find any prior art for the appservice 3pid stuff, but can make an attempt.

babolivier · 2022-05-16T16:34:19Z

I don't think that's likely. This is providing more information to the bridges, so they can choose to authenticate. It's only going to break if a bridge requires auth (as they should do today) and Synapse doesn't provide the information (as it does now).

Makes sense.

I couldn't find any prior art for the appservice 3pid stuff, but can make an attempt.

Thanks! Talking about CI, it looks like Sytest also needs fixing :/

tests/appservice/test_api.py

Co-authored-by: Brendan Abolivier <babolivier@matrix.org>

tests/appservice/test_api.py

babolivier · 2022-05-17T14:35:02Z

tests/appservice/test_api.py

+                self.fail("URL provided was invalid")
+                return []
+
+        self.api.get_json = Mock(side_effect=get_json)  # type: ignore[assignment]  # We assign to a method.


Just to line wrap and explain a bit what the comment refers to.

Suggested change

self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] # We assign to a method.

# We assign to a method, which mypy doesn't like.

self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment]

babolivier · 2022-05-17T14:45:00Z

tests/appservice/test_api.py

+        self.service = ApplicationService(
+            id="unique_identifier",
+            sender="@as:test",
+            url=URL,


Looks like URL could just be inlined here?

It's a bit tricky. It's used by template strings URL_USER and URL_LOCATION, but I suppose I could hardcode those to use URL. It just felt a bit more...safe?

babolivier · 2022-05-17T14:46:59Z

tests/appservice/test_api.py

+                self.fail("URL provided was invalid")
+                return []
+
+        self.api.get_json = Mock(side_effect=get_json)  # type: ignore[assignment]  # We assign to a method.


Looks like this (and the definition of get_json should probably happen within the test? It feels pretty specific to what we want to test in test_query_3pe_authenticates_token.
In this case most of the constants used can become local to test_query_3pe_authenticates_token.

babolivier

Almost good now!

babolivier · 2022-05-23T09:06:18Z

tests/appservice/test_api.py

+            elif url == URL_LOCATION:
+                return SUCCESS_RESULT_LOCATION
+            else:
+                self.fail("URL provided was invalid")


Note that this is now a bit unnecessary since raising the RuntimeError will automatically fail the test.

babolivier · 2022-05-23T09:07:24Z

tests/appservice/test_api.py

+
+        async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]:
+            if not args.get(b"access_token"):
+                raise Exception("Access token not provided")


Suggested change

raise Exception("Access token not provided")

raise RuntimeError("Access token not provided")

For consistency

babolivier · 2022-05-23T09:13:34Z

tests/appservice/test_api.py

+
+class ApplicationServiceApiTestCase(unittest.HomeserverTestCase):
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
+        self.api = ApplicationServiceApi(hs)


Suggested change

self.api = ApplicationServiceApi(hs)

self.api = hs.get_application_service_api()

It probably doesn't make that much of a difference, but it's more consistent with how we access this kind of objects in tests.

tests/appservice/test_api.py

@olmari

Synapse 1.61.0 (2022-06-14) =========================== This release removes support for the non-standard feature known both as 'groups' and as 'communities', which have been superseded by *Spaces*. See [the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1610) for more details. Improved Documentation ---------------------- - Mention removed community/group worker endpoints in [upgrade.md](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1610s). Contributed by @olmari. ([\matrix-org#13023](matrix-org#13023)) Synapse 1.61.0rc1 (2022-06-07) ============================== Features -------- - Add new `media_retention` options to the homeserver config for routinely cleaning up non-recently accessed media. ([\matrix-org#12732](matrix-org#12732), [\matrix-org#12972](matrix-org#12972), [\matrix-org#12977](matrix-org#12977)) - Experimental support for [MSC3772](matrix-org/matrix-spec-proposals#3772): Push rule for mutually related events. ([\matrix-org#12740](matrix-org#12740), [\matrix-org#12859](matrix-org#12859)) - Update to the `check_event_for_spam` module callback: Deprecate the current callback signature, replace it with a new signature that is both less ambiguous (replacing booleans with explicit allow/block) and more powerful (ability to return explicit error codes). ([\matrix-org#12808](matrix-org#12808)) - Add storage and module API methods to get monthly active users (and their corresponding appservices) within an optionally specified time range. ([\matrix-org#12838](matrix-org#12838), [\matrix-org#12917](matrix-org#12917)) - Support the new error code `ORG.MATRIX.MSC3823.USER_ACCOUNT_SUSPENDED` from [MSC3823](matrix-org/matrix-spec-proposals#3823). ([\matrix-org#12845](matrix-org#12845), [\matrix-org#12923](matrix-org#12923)) - Add a configurable background job to delete stale devices. ([\matrix-org#12855](matrix-org#12855)) - Improve URL previews for pages with empty elements. ([\matrix-org#12951](matrix-org#12951)) - Allow updating a user's password using the admin API without logging out their devices. Contributed by @jcgruenhage. ([\matrix-org#12952](matrix-org#12952)) Bugfixes -------- - Always send an `access_token` in `/thirdparty/` requests to appservices, as required by the [Application Service API specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks). ([\matrix-org#12746](matrix-org#12746)) - Implement [MSC3816](matrix-org/matrix-spec-proposals#3816): sending the root event in a thread should count as having 'participated' in it. ([\matrix-org#12766](matrix-org#12766)) - Delete events from the `federation_inbound_events_staging` table when a room is purged through the admin API. ([\matrix-org#12784](matrix-org#12784)) - Fix a bug where we did not correctly handle invalid device list updates over federation. Contributed by Carl Bordum Hansen. ([\matrix-org#12829](matrix-org#12829)) - Fix a bug which allowed multiple async operations to access database locks concurrently. Contributed by @sumnerevans @ Beeper. ([\matrix-org#12832](matrix-org#12832)) - Fix an issue introduced in Synapse 0.34 where the `/notifications` endpoint would only return notifications if a user registered at least one pusher. Contributed by Famedly. ([\matrix-org#12840](matrix-org#12840)) - Fix a bug where servers using a Postgres database would fail to backfill from an insertion event when MSC2716 is enabled (`experimental_features.msc2716_enabled`). ([\matrix-org#12843](matrix-org#12843)) - Fix [MSC3787](matrix-org/matrix-spec-proposals#3787) rooms being omitted from room directory, room summary and space hierarchy responses. ([\matrix-org#12858](matrix-org#12858)) - Fix a bug introduced in Synapse 1.54.0 which could sometimes cause exceptions when handling federated traffic. ([\matrix-org#12877](matrix-org#12877)) - Fix a bug introduced in Synapse 1.59.0 which caused room deletion to fail with a foreign key violation error. ([\matrix-org#12889](matrix-org#12889)) - Fix a long-standing bug which caused the `/messages` endpoint to return an incorrect `end` attribute when there were no more events. Contributed by @Vetchu. ([\matrix-org#12903](matrix-org#12903)) - Fix a bug introduced in Synapse 1.58.0 where `/sync` would fail if the most recent event in a room was a redaction of an event that has since been purged. ([\matrix-org#12905](matrix-org#12905)) - Fix a potential memory leak when generating thumbnails. ([\matrix-org#12932](matrix-org#12932)) - Fix a long-standing bug where a URL preview would break if the image failed to download. ([\matrix-org#12950](matrix-org#12950)) Improved Documentation ---------------------- - Fix typographical errors in documentation. ([\matrix-org#12863](matrix-org#12863)) - Fix documentation incorrectly stating the `sendToDevice` endpoint can be directed at generic workers. Contributed by Nick @ Beeper. ([\matrix-org#12867](matrix-org#12867)) Deprecations and Removals ------------------------- - Remove support for the non-standard groups/communities feature from Synapse. ([\matrix-org#12553](matrix-org#12553), [\matrix-org#12558](matrix-org#12558), [\matrix-org#12563](matrix-org#12563), [\matrix-org#12895](matrix-org#12895), [\matrix-org#12897](matrix-org#12897), [\matrix-org#12899](matrix-org#12899), [\matrix-org#12900](matrix-org#12900), [\matrix-org#12936](matrix-org#12936), [\matrix-org#12966](matrix-org#12966)) - Remove contributed `kick_users.py` script. This is broken under Python 3, and is not added to the environment when `pip install`ing Synapse. ([\matrix-org#12908](matrix-org#12908)) - Remove `contrib/jitsimeetbridge`. This was an unused experiment that hasn't been meaningfully changed since 2014. ([\matrix-org#12909](matrix-org#12909)) - Remove unused `contrib/experiements/cursesio.py` script, which fails to run under Python 3. ([\matrix-org#12910](matrix-org#12910)) - Remove unused `contrib/experiements/test_messaging.py` script. This fails to run on Python 3. ([\matrix-org#12911](matrix-org#12911)) Internal Changes ---------------- - Test Synapse against Complement with workers. ([\matrix-org#12810](matrix-org#12810), [\matrix-org#12933](matrix-org#12933)) - Reduce the amount of state we pull from the DB. ([\matrix-org#12811](matrix-org#12811), [\matrix-org#12964](matrix-org#12964)) - Try other homeservers when re-syncing state for rooms with partial state. ([\matrix-org#12812](matrix-org#12812)) - Resume state re-syncing for rooms with partial state after a Synapse restart. ([\matrix-org#12813](matrix-org#12813)) - Remove Mutual Rooms' ([MSC2666](matrix-org/matrix-spec-proposals#2666)) endpoint dependency on the User Directory. ([\matrix-org#12836](matrix-org#12836)) - Experimental: expand `check_event_for_spam` with ability to return additional fields. This enables spam-checker implementations to experiment with mechanisms to give users more information about why they are blocked and whether any action is needed from them to be unblocked. ([\matrix-org#12846](matrix-org#12846)) - Remove `dont_notify` from the `.m.rule.room.server_acl` rule. ([\matrix-org#12849](matrix-org#12849)) - Remove the unstable `/hierarchy` endpoint from [MSC2946](matrix-org/matrix-spec-proposals#2946). ([\matrix-org#12851](matrix-org#12851)) - Pull out less state when handling gaps in room DAG. ([\matrix-org#12852](matrix-org#12852), [\matrix-org#12904](matrix-org#12904)) - Clean-up the push rules datastore. ([\matrix-org#12856](matrix-org#12856)) - Correct a type annotation in the URL preview source code. ([\matrix-org#12860](matrix-org#12860)) - Update `pyjwt` dependency to [2.4.0](https://github.com/jpadilla/pyjwt/releases/tag/2.4.0). ([\matrix-org#12865](matrix-org#12865)) - Enable the `/account/whoami` endpoint on synapse worker processes. Contributed by Nick @ Beeper. ([\matrix-org#12866](matrix-org#12866)) - Enable the `batch_send` endpoint on synapse worker processes. Contributed by Nick @ Beeper. ([\matrix-org#12868](matrix-org#12868)) - Don't generate empty AS transactions when the AS is flagged as down. Contributed by Nick @ Beeper. ([\matrix-org#12869](matrix-org#12869)) - Fix up the variable `state_store` naming. ([\matrix-org#12871](matrix-org#12871)) - Faster room joins: when querying the current state of the room, wait for state to be populated. ([\matrix-org#12872](matrix-org#12872)) - Avoid running queries which will never result in deletions. ([\matrix-org#12879](matrix-org#12879)) - Use constants for EDU types. ([\matrix-org#12884](matrix-org#12884)) - Reduce database load of `/sync` when presence is enabled. ([\matrix-org#12885](matrix-org#12885)) - Refactor `have_seen_events` to reduce memory consumed when processing federation traffic. ([\matrix-org#12886](matrix-org#12886)) - Refactor receipt linearization code. ([\matrix-org#12888](matrix-org#12888)) - Add type annotations to `synapse.logging.opentracing`. ([\matrix-org#12894](matrix-org#12894)) - Remove PyNaCl occurrences directly used in Synapse code. ([\matrix-org#12902](matrix-org#12902)) - Bump types-jsonschema from 4.4.1 to 4.4.6. ([\matrix-org#12912](matrix-org#12912)) - Rename storage classes. ([\matrix-org#12913](matrix-org#12913)) - Preparation for database schema simplifications: stop reading from `event_edges.room_id`. ([\matrix-org#12914](matrix-org#12914)) - Check if we are in a virtual environment before overriding the `PYTHONPATH` environment variable in the demo script. ([\matrix-org#12916](matrix-org#12916)) - Improve the logging when signature checks on events fail. ([\matrix-org#12925](matrix-org#12925)) # -----BEGIN PGP SIGNATURE----- # # iQFEBAABCgAuFiEEBTGR3/RnAzBGUif3pULk7RsPrAkFAmKoaa0QHGVyaWtAbWF0 # cml4Lm9yZwAKCRClQuTtGw+sCVn3B/sF8hdhrZ7hWW40ST3eG9cEKNFrj/xZXiaI # zho3ryrxaQF68BSKot15AvZSprEdwBXWrb8WeTjyw+QH7vTKrCQDZ0p7qubn10Z7 # BuKq9hyYjyCLjBZrgy8d4U3Y8gsSByuO59YKHNLn+UTJLOs5GTH8Wprwh4mpU3Jl # +o+cC+lMSVcyZij2hihFymcSxWq/I9WL0dsjRif8x0BUQwRXwmXc6+mhlgLBe2Zs # 2dUouzJ8NVZcjfWvsg4noXPrNQ/IiyCVZlSIgaDftDIxVSPk5/rXiUUNex8Tn1+I # TnOgnXhpOQD1vwVGYS8LrcfA0ubSili7xUJ8k2e5TkCjpkaVnYNu # =q+7N # -----END PGP SIGNATURE----- # gpg: Signature made Tue Jun 14 11:57:49 2022 BST # gpg: using RSA key 053191DFF4670330465227F7A542E4ED1B0FAC09 # gpg: issuer "erik@matrix.org" # gpg: WARNING: server 'dirmngr' is older than us (2.2.34 < 2.3.6) # gpg: Note: Outdated servers may lack important security fixes. # gpg: Note: Use the command "gpgconf --kill all" to restart them. # gpg: Can't check signature: No public key # Conflicts: # docs/workers.md # synapse/handlers/message.py # synapse/handlers/pagination.py # synapse/push/bulk_push_rule_evaluator.py # synapse/push/push_rule_evaluator.py # synapse/rest/client/receipts.py # synapse/storage/databases/main/appservice.py # tests/push/test_push_rule_evaluator.py

Add authentication to thirdparty bridge APIs

dbbf926

Half-Shot requested a review from a team as a code owner May 16, 2022 15:08

Half-Shot added 2 commits May 16, 2022 16:19

Add args as any

1474fc2

changelog

df3554c

babolivier suggested changes May 16, 2022

View reviewed changes

changelog.d/12746.bugfix Outdated Show resolved Hide resolved

Add tests for api

fe43fe3

Half-Shot mentioned this pull request May 16, 2022

Add authentication to thirdparty bridge APIs: Check access_token matrix-org/sytest#1253

Merged

DMRobertson reviewed May 17, 2022

View reviewed changes

tests/appservice/test_api.py Outdated Show resolved Hide resolved

Half-Shot and others added 4 commits May 17, 2022 13:43

Update test_api.py

4a45161

Update changelog.d/12746.bugfix

b46c626

Co-authored-by: Brendan Abolivier <babolivier@matrix.org>

fix lint

50b04f7

fix

0520ac7

Half-Shot requested a review from babolivier May 17, 2022 13:27

babolivier suggested changes May 17, 2022

View reviewed changes

Tidy tidy according to review

512ebca

Half-Shot requested a review from babolivier May 18, 2022 10:20

Add raise RuntimeError("This should never be seen.")

8782cdf

babolivier suggested changes May 23, 2022

View reviewed changes

final review bits

94e98af

Half-Shot requested a review from babolivier May 23, 2022 11:34

babolivier approved these changes May 24, 2022

View reviewed changes

babolivier merged commit 6855024 into develop May 24, 2022

babolivier deleted the hs/add-authentication-to-3pid branch May 24, 2022 14:39

This was referenced May 25, 2022

Enable authenticateThirdpartyEndpoints matrix-org/matrix-appservice-irc#1566

Closed

Remove option to disable authentication of the 3pid endpoints matrix-org/matrix-appservice-bridge#409

Merged

Half-Shot mentioned this pull request Dec 2, 2022

Appservice /transactions requests are queued behind requests to the AS /users/ (and maybe other) #14152

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication to thirdparty bridge APIs #12746

Add authentication to thirdparty bridge APIs #12746

Half-Shot commented May 16, 2022 •

edited

Loading

babolivier left a comment

Half-Shot commented May 16, 2022

babolivier commented May 16, 2022 •

edited

Loading

babolivier May 17, 2022

babolivier May 17, 2022

Half-Shot May 18, 2022

babolivier May 17, 2022

babolivier left a comment

babolivier May 23, 2022

babolivier May 23, 2022

babolivier May 23, 2022

	self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] # We assign to a method.
	# We assign to a method, which mypy doesn't like.
	self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment]

	raise Exception("Access token not provided")
	raise RuntimeError("Access token not provided")

	self.api = ApplicationServiceApi(hs)
	self.api = hs.get_application_service_api()

Add authentication to thirdparty bridge APIs #12746

Add authentication to thirdparty bridge APIs #12746

Conversation

Half-Shot commented May 16, 2022 • edited Loading

babolivier left a comment

Choose a reason for hiding this comment

Half-Shot commented May 16, 2022

babolivier commented May 16, 2022 • edited Loading

babolivier May 17, 2022

Choose a reason for hiding this comment

babolivier May 17, 2022

Choose a reason for hiding this comment

Half-Shot May 18, 2022

Choose a reason for hiding this comment

babolivier May 17, 2022

Choose a reason for hiding this comment

babolivier left a comment

Choose a reason for hiding this comment

babolivier May 23, 2022

Choose a reason for hiding this comment

babolivier May 23, 2022

Choose a reason for hiding this comment

babolivier May 23, 2022

Choose a reason for hiding this comment

Half-Shot commented May 16, 2022 •

edited

Loading

babolivier commented May 16, 2022 •

edited

Loading