From dbbf926e23f4b746a19b8b68812092df286e09a6 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 16 May 2022 16:06:36 +0100 Subject: [PATCH 01/11] Add authentication to thirdparty bridge APIs --- synapse/appservice/api.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index d19f8dd996b2..d97eda7f2eaa 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -155,6 +155,8 @@ async def query_3pe( if service.url is None: return [] + fields["access_token"] = service.hs_token + uri = "%s%s/thirdparty/%s/%s" % ( service.url, APP_SERVICE_PREFIX, @@ -196,7 +198,7 @@ async def _get() -> Optional[JsonDict]: urllib.parse.quote(protocol), ) try: - info = await self.get_json(uri) + info = await self.get_json(uri, {"access_token": service.hs_token}) if not _is_valid_3pe_metadata(info): logger.warning( From 1474fc23af3ff8891adc5dc2e5421589a088bd5c Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 16 May 2022 16:19:42 +0100 Subject: [PATCH 02/11] Add args as any --- synapse/appservice/api.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index d97eda7f2eaa..80cecfbfb0e6 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -14,7 +14,7 @@ # limitations under the License. import logging import urllib.parse -from typing import TYPE_CHECKING, Dict, Iterable, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, Dict, Iterable, List, Mapping, Optional, Tuple from prometheus_client import Counter from typing_extensions import TypeGuard @@ -155,7 +155,8 @@ async def query_3pe( if service.url is None: return [] - fields["access_token"] = service.hs_token + # This is required by the configuration. + assert service.hs_token is not None uri = "%s%s/thirdparty/%s/%s" % ( service.url, @@ -164,7 +165,11 @@ async def query_3pe( urllib.parse.quote(protocol), ) try: - response = await self.get_json(uri, fields) + args: Mapping[Any, Any] = { + **fields, + b"access_token": [service.hs_token], + } + response = await self.get_json(uri, args=args) if not isinstance(response, list): logger.warning( "query_3pe to %s returned an invalid response %r", uri, response @@ -192,6 +197,8 @@ async def get_3pe_protocol( return {} async def _get() -> Optional[JsonDict]: + # This is required by the configuration. + assert service.hs_token is not None uri = "%s%s/thirdparty/protocol/%s" % ( service.url, APP_SERVICE_PREFIX, From df3554cdf194a62c19748853ea41226994a8b26f Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 16 May 2022 16:23:39 +0100 Subject: [PATCH 03/11] changelog --- changelog.d/12746.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12746.bugfix diff --git a/changelog.d/12746.bugfix b/changelog.d/12746.bugfix new file mode 100644 index 000000000000..0709eab4f262 --- /dev/null +++ b/changelog.d/12746.bugfix @@ -0,0 +1 @@ +Require an `access_token` in `/thirdparty/` requests to appservices, as required by the [Matrix specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks). \ No newline at end of file From fe43fe3f6b38c6f03fa663a118715503f8409a2f Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 16 May 2022 17:37:24 +0100 Subject: [PATCH 04/11] Add tests for api --- synapse/appservice/api.py | 2 +- tests/appservice/test_api.py | 96 ++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 tests/appservice/test_api.py diff --git a/synapse/appservice/api.py b/synapse/appservice/api.py index 80cecfbfb0e6..df1c21446203 100644 --- a/synapse/appservice/api.py +++ b/synapse/appservice/api.py @@ -167,7 +167,7 @@ async def query_3pe( try: args: Mapping[Any, Any] = { **fields, - b"access_token": [service.hs_token], + b"access_token": service.hs_token, } response = await self.get_json(uri, args=args) if not isinstance(response, list): diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py new file mode 100644 index 000000000000..d13cdd896631 --- /dev/null +++ b/tests/appservice/test_api.py @@ -0,0 +1,96 @@ +# Copyright 2022 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from typing import Any, List, Mapping +from unittest.mock import Mock + +from twisted.internet.testing import MemoryReactor + +from synapse.appservice import ApplicationService +from synapse.appservice.api import ApplicationServiceApi +from synapse.server import HomeServer +from synapse.types import JsonDict +from synapse.util import Clock + +from tests import unittest + +PROTOCOL = "myproto" +TOKEN = "myastoken" +URL = "http://mytestservice" +URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}" +URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}" +SUCCESS_RESULT_USER = [ + { + "protocol": PROTOCOL, + "userid": "@a:user", + "fields": { + "more": "fields", + }, + } +] +SUCCESS_RESULT_LOCATION = [ + { + "protocol": PROTOCOL, + "alias": "#a:room", + "fields": { + "more": "fields", + }, + } +] + + +class ApplicationServiceApiTestCase(unittest.HomeserverTestCase): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer): + self.api = ApplicationServiceApi(hs) + + self.request_url = None + self.fields = None + + 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") + + self.assertEqual(args.get(b"access_token"), TOKEN) + self.request_url = url + self.fields = args + if url == URL_USER: + return SUCCESS_RESULT_USER + elif url == URL_LOCATION: + return SUCCESS_RESULT_LOCATION + else: + self.fail("URL provided was invalid") + return [] + + self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] # We assign to a method. + self.service = ApplicationService( + id="unique_identifier", + sender="@as:test", + url=URL, + token="unused", + hs_token=TOKEN, + hostname="myserver", + ) + + def test_query_3pe_authenticates_token(self): + result = self.get_success( + self.api.query_3pe(self.service, "user", PROTOCOL, {b"some": [b"field"]}) + ) + self.assertEqual(self.request_url, URL_USER) + self.assertEqual(result, SUCCESS_RESULT_USER) + result = self.get_success( + self.api.query_3pe( + self.service, "location", PROTOCOL, {b"some": [b"field"]} + ) + ) + self.assertEqual(self.request_url, URL_LOCATION) + self.assertEqual(result, SUCCESS_RESULT_LOCATION) From 4a451618da4fc832008177e25ab5ad509d62233d Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 17 May 2022 13:43:43 +0100 Subject: [PATCH 05/11] Update test_api.py --- tests/appservice/test_api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index d13cdd896631..f2e01f22f068 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -13,8 +13,7 @@ # limitations under the License. from typing import Any, List, Mapping from unittest.mock import Mock - -from twisted.internet.testing import MemoryReactor +from twisted.test.proto_helper import MemoryReactor from synapse.appservice import ApplicationService from synapse.appservice.api import ApplicationServiceApi From b46c626d0d9bd57e75b5e65dcd5defc0e6b15c96 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 17 May 2022 13:43:59 +0100 Subject: [PATCH 06/11] Update changelog.d/12746.bugfix Co-authored-by: Brendan Abolivier --- changelog.d/12746.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12746.bugfix b/changelog.d/12746.bugfix index 0709eab4f262..67e7fc854c4f 100644 --- a/changelog.d/12746.bugfix +++ b/changelog.d/12746.bugfix @@ -1 +1 @@ -Require an `access_token` in `/thirdparty/` requests to appservices, as required by the [Matrix specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks). \ No newline at end of file +Always send an `access_token` in `/thirdparty/` requests to appservices, as required by the [Matrix specification](https://spec.matrix.org/v1.1/application-service-api/#third-party-networks). \ No newline at end of file From 50b04f7b6a7041de8d6ca8472832581a56a20291 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Tue, 17 May 2022 14:19:17 +0100 Subject: [PATCH 07/11] fix lint --- tests/appservice/test_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index f2e01f22f068..5a47a36d6a2f 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -13,6 +13,7 @@ # limitations under the License. from typing import Any, List, Mapping from unittest.mock import Mock + from twisted.test.proto_helper import MemoryReactor from synapse.appservice import ApplicationService From 0520ac7ed54f3ded9cf2950145fc444e28cb44c9 Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Tue, 17 May 2022 14:21:10 +0100 Subject: [PATCH 08/11] fix --- tests/appservice/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index 5a47a36d6a2f..0f2f97c56615 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -14,7 +14,7 @@ from typing import Any, List, Mapping from unittest.mock import Mock -from twisted.test.proto_helper import MemoryReactor +from twisted.test.proto_helpers import MemoryReactor from synapse.appservice import ApplicationService from synapse.appservice.api import ApplicationServiceApi From 512ebcaaea4a47fa5ef5b3049316cf34fff7014e Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Wed, 18 May 2022 11:05:26 +0100 Subject: [PATCH 09/11] Tidy tidy according to review --- tests/appservice/test_api.py | 66 +++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index 0f2f97c56615..c7964eb7338a 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -27,34 +27,45 @@ PROTOCOL = "myproto" TOKEN = "myastoken" URL = "http://mytestservice" -URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}" -URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}" -SUCCESS_RESULT_USER = [ - { - "protocol": PROTOCOL, - "userid": "@a:user", - "fields": { - "more": "fields", - }, - } -] -SUCCESS_RESULT_LOCATION = [ - { - "protocol": PROTOCOL, - "alias": "#a:room", - "fields": { - "more": "fields", - }, - } -] class ApplicationServiceApiTestCase(unittest.HomeserverTestCase): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer): self.api = ApplicationServiceApi(hs) + self.service = ApplicationService( + id="unique_identifier", + sender="@as:test", + url=URL, + token="unused", + hs_token=TOKEN, + hostname="myserver", + ) + + def test_query_3pe_authenticates_token(self): + + SUCCESS_RESULT_USER = [ + { + "protocol": PROTOCOL, + "userid": "@a:user", + "fields": { + "more": "fields", + }, + } + ] + SUCCESS_RESULT_LOCATION = [ + { + "protocol": PROTOCOL, + "alias": "#a:room", + "fields": { + "more": "fields", + }, + } + ] + + URL_USER = f"{URL}/_matrix/app/unstable/thirdparty/user/{PROTOCOL}" + URL_LOCATION = f"{URL}/_matrix/app/unstable/thirdparty/location/{PROTOCOL}" self.request_url = None - self.fields = None async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]: if not args.get(b"access_token"): @@ -62,7 +73,6 @@ async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]: self.assertEqual(args.get(b"access_token"), TOKEN) self.request_url = url - self.fields = args if url == URL_USER: return SUCCESS_RESULT_USER elif url == URL_LOCATION: @@ -71,17 +81,9 @@ async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]: self.fail("URL provided was invalid") return [] - self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] # We assign to a method. - self.service = ApplicationService( - id="unique_identifier", - sender="@as:test", - url=URL, - token="unused", - hs_token=TOKEN, - hostname="myserver", - ) + # We assign to a method, which mypy doesn't like. + self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] - def test_query_3pe_authenticates_token(self): result = self.get_success( self.api.query_3pe(self.service, "user", PROTOCOL, {b"some": [b"field"]}) ) From 8782cdfb3f2a05d24710c9f8e1e6f9c01648b624 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 23 May 2022 09:39:58 +0100 Subject: [PATCH 10/11] Add raise RuntimeError("This should never be seen.") --- tests/appservice/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index c7964eb7338a..f33c90d1d870 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -79,7 +79,7 @@ async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]: return SUCCESS_RESULT_LOCATION else: self.fail("URL provided was invalid") - return [] + raise RuntimeError("This should never be seen.") # We assign to a method, which mypy doesn't like. self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment] From 94e98af695bd0917c2d30facc437c1b2575d524a Mon Sep 17 00:00:00 2001 From: Half-Shot Date: Mon, 23 May 2022 12:34:17 +0100 Subject: [PATCH 11/11] final review bits --- tests/appservice/test_api.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/appservice/test_api.py b/tests/appservice/test_api.py index f33c90d1d870..3e0db4dd9871 100644 --- a/tests/appservice/test_api.py +++ b/tests/appservice/test_api.py @@ -17,7 +17,6 @@ from twisted.test.proto_helpers import MemoryReactor from synapse.appservice import ApplicationService -from synapse.appservice.api import ApplicationServiceApi from synapse.server import HomeServer from synapse.types import JsonDict from synapse.util import Clock @@ -31,7 +30,7 @@ class ApplicationServiceApiTestCase(unittest.HomeserverTestCase): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer): - self.api = ApplicationServiceApi(hs) + self.api = hs.get_application_service_api() self.service = ApplicationService( id="unique_identifier", sender="@as:test", @@ -42,6 +41,10 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer): ) def test_query_3pe_authenticates_token(self): + """ + Tests that 3pe queries to the appservice are authenticated + with the appservice's token. + """ SUCCESS_RESULT_USER = [ { @@ -69,7 +72,7 @@ def test_query_3pe_authenticates_token(self): 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") + raise RuntimeError("Access token not provided") self.assertEqual(args.get(b"access_token"), TOKEN) self.request_url = url @@ -78,8 +81,9 @@ async def get_json(url: str, args: Mapping[Any, Any]) -> List[JsonDict]: elif url == URL_LOCATION: return SUCCESS_RESULT_LOCATION else: - self.fail("URL provided was invalid") - raise RuntimeError("This should never be seen.") + raise RuntimeError( + "URL provided was invalid. This should never be seen." + ) # We assign to a method, which mypy doesn't like. self.api.get_json = Mock(side_effect=get_json) # type: ignore[assignment]