From 1cc4a3df04cdb1d15f3ed14c67f84bcbb21ae83b Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 4 Aug 2021 14:19:54 +0100 Subject: [PATCH 1/7] Check that a appservice protocol has at least one service --- changelog.d/10532.bugfix | 1 + synapse/handlers/appservice.py | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelog.d/10532.bugfix diff --git a/changelog.d/10532.bugfix b/changelog.d/10532.bugfix new file mode 100644 index 000000000000..5cea4bbbcaa2 --- /dev/null +++ b/changelog.d/10532.bugfix @@ -0,0 +1 @@ +Only report protocols that are implemented by at least one service, as required by the spec when handling `GET /_matrix/client/r0/thirdparty/protocols`. \ No newline at end of file diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 21a17cd2e834..7ab558912eab 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -406,7 +406,11 @@ def _merge_instances(infos: List[JsonDict]) -> JsonDict: return combined - return {p: _merge_instances(protocols[p]) for p in protocols.keys()} + return { + p: _merge_instances(protocols[p]) + for p in protocols.keys() + if len(protocols[p]) > 0 + } async def _get_services_for_event( self, event: EventBase From 67527f625eb43af1144d3510f792135816efbbf4 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 4 Aug 2021 14:52:30 +0100 Subject: [PATCH 2/7] Remove len --- synapse/handlers/appservice.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 7ab558912eab..f52e0b5e1c54 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -393,7 +393,7 @@ async def get_3pe_protocols( def _merge_instances(infos: List[JsonDict]) -> JsonDict: if not infos: - return {} + return None # Merge the 'instances' lists of multiple results, but just take # the other fields from the first as they ought to be identical @@ -409,7 +409,7 @@ def _merge_instances(infos: List[JsonDict]) -> JsonDict: return { p: _merge_instances(protocols[p]) for p in protocols.keys() - if len(protocols[p]) > 0 + if protocols[p] } async def _get_services_for_event( From 91c4d3f2bc3efd55a4e16b92b54ab4e64eea0fbe Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 4 Aug 2021 14:52:55 +0100 Subject: [PATCH 3/7] Remove redundant if check --- synapse/handlers/appservice.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index f52e0b5e1c54..3d61c8642f12 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -392,9 +392,6 @@ async def get_3pe_protocols( protocols[p].append(info) def _merge_instances(infos: List[JsonDict]) -> JsonDict: - if not infos: - return None - # Merge the 'instances' lists of multiple results, but just take # the other fields from the first as they ought to be identical # copy the result so as not to corrupt the cached one From aeb8046a8fac8843c9232b6ce2bd624b1f63c3d5 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 4 Aug 2021 10:03:04 -0400 Subject: [PATCH 4/7] Lint --- synapse/handlers/appservice.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/synapse/handlers/appservice.py b/synapse/handlers/appservice.py index 3d61c8642f12..4ab4046650b8 100644 --- a/synapse/handlers/appservice.py +++ b/synapse/handlers/appservice.py @@ -404,9 +404,7 @@ def _merge_instances(infos: List[JsonDict]) -> JsonDict: return combined return { - p: _merge_instances(protocols[p]) - for p in protocols.keys() - if protocols[p] + p: _merge_instances(protocols[p]) for p in protocols.keys() if protocols[p] } async def _get_services_for_event( From d5e863659592a50bee3bef5c18d4c6612fd11a56 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 4 Aug 2021 16:04:04 +0100 Subject: [PATCH 5/7] Add tests for get_3pe_protocols --- tests/handlers/test_appservice.py | 122 +++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py index 024c5e963cd2..0e3cc2d7572f 100644 --- a/tests/handlers/test_appservice.py +++ b/tests/handlers/test_appservice.py @@ -133,11 +133,131 @@ def test_query_room_alias_exists(self): self.assertEquals(result.room_id, room_id) self.assertEquals(result.servers, servers) - def _mkservice(self, is_interested): + def test_get_3pe_protocols_no_appservices(self): + self.mock_store.get_app_services.return_value = [] + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols("my-protocol")) + ) + self.mock_as_api.get_3pe_protocol.assert_not_called() + self.assertEquals(response, {}) + + def test_get_3pe_protocols_no_protocols(self): + service = self._mkservice(False, []) + self.mock_store.get_app_services.return_value = [service] + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols()) + ) + self.mock_as_api.get_3pe_protocol.assert_not_called() + self.assertEquals(response, {}) + + def test_get_3pe_protocols_protocol_no_response(self): + service = self._mkservice(False, ["my-protocol"]) + self.mock_store.get_app_services.return_value = [service] + self.mock_as_api.get_3pe_protocol.return_value = make_awaitable(None) + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols()) + ) + self.mock_as_api.get_3pe_protocol.assert_called_once_with( + service, "my-protocol" + ) + self.assertEquals(response, {}) + + def test_get_3pe_protocols_select_one_protocol(self): + service = self._mkservice(False, ["my-protocol"]) + self.mock_store.get_app_services.return_value = [service] + self.mock_as_api.get_3pe_protocol.return_value = make_awaitable( + {"x-protocol-data": 42, "instances": []} + ) + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols("my-protocol")) + ) + self.mock_as_api.get_3pe_protocol.assert_called_once_with( + service, "my-protocol" + ) + self.assertEquals( + response, {"my-protocol": {"x-protocol-data": 42, "instances": []}} + ) + + def test_get_3pe_protocols_one_protocol(self): + service = self._mkservice(False, ["my-protocol"]) + self.mock_store.get_app_services.return_value = [service] + self.mock_as_api.get_3pe_protocol.return_value = make_awaitable( + {"x-protocol-data": 42, "instances": []} + ) + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols()) + ) + self.mock_as_api.get_3pe_protocol.assert_called_once_with( + service, "my-protocol" + ) + self.assertEquals( + response, {"my-protocol": {"x-protocol-data": 42, "instances": []}} + ) + + def test_get_3pe_protocols_multiple_protocol(self): + service_one = self._mkservice(False, ["my-protocol"]) + service_two = self._mkservice(False, ["other-protocol"]) + self.mock_store.get_app_services.return_value = [service_one, service_two] + self.mock_as_api.get_3pe_protocol.return_value = make_awaitable( + {"x-protocol-data": 42, "instances": []} + ) + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols()) + ) + self.mock_as_api.get_3pe_protocol.assert_called() + self.assertEquals( + response, + { + "my-protocol": {"x-protocol-data": 42, "instances": []}, + "other-protocol": {"x-protocol-data": 42, "instances": []}, + }, + ) + + def test_get_3pe_protocols_multiple_info(self): + service_one = self._mkservice(False, ["my-protocol"]) + service_two = self._mkservice(False, ["my-protocol"]) + + async def someFunc(service, unusedProtocol): + if service == service_one: + return { + "x-protocol-data": 42, + "instances": [{"desc": "Alice's service"}], + } + if service == service_two: + return { + "x-protocol-data": 36, + "x-not-used": 45, + "instances": [{"desc": "Bob's service"}], + } + raise Exception("Unexpected service") + + self.mock_store.get_app_services.return_value = [service_one, service_two] + self.mock_as_api.get_3pe_protocol = someFunc + response = self.successResultOf( + defer.ensureDeferred(self.handler.get_3pe_protocols()) + ) + # It's expected that the second service's data doesn't appear in the response + self.assertEquals( + response, + { + "my-protocol": { + "x-protocol-data": 42, + "instances": [ + { + "desc": "Alice's service", + }, + {"desc": "Bob's service"}, + ], + }, + }, + ) + + def _mkservice(self, is_interested, protocols=None): service = Mock() service.is_interested.return_value = make_awaitable(is_interested) service.token = "mock_service_token" service.url = "mock_service_url" + service.protocols = protocols return service def _mkservice_alias(self, is_interested_in_alias): From cced86ed419deadbc63668b0db9271d4399484a1 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 4 Aug 2021 16:07:43 +0100 Subject: [PATCH 6/7] Name function s/someFunc/get_3pe_protocol --- tests/handlers/test_appservice.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py index 0e3cc2d7572f..43998020b2eb 100644 --- a/tests/handlers/test_appservice.py +++ b/tests/handlers/test_appservice.py @@ -217,7 +217,7 @@ def test_get_3pe_protocols_multiple_info(self): service_one = self._mkservice(False, ["my-protocol"]) service_two = self._mkservice(False, ["my-protocol"]) - async def someFunc(service, unusedProtocol): + async def get_3pe_protocol(service, unusedProtocol): if service == service_one: return { "x-protocol-data": 42, @@ -232,7 +232,7 @@ async def someFunc(service, unusedProtocol): raise Exception("Unexpected service") self.mock_store.get_app_services.return_value = [service_one, service_two] - self.mock_as_api.get_3pe_protocol = someFunc + self.mock_as_api.get_3pe_protocol = get_3pe_protocol response = self.successResultOf( defer.ensureDeferred(self.handler.get_3pe_protocols()) ) From 8eb7479385d37c8705cf12000a1384a7e2d76285 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Thu, 5 Aug 2021 11:12:45 +0100 Subject: [PATCH 7/7] Update changelog.d/10532.bugfix Co-authored-by: Patrick Cloke --- changelog.d/10532.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/10532.bugfix b/changelog.d/10532.bugfix index 5cea4bbbcaa2..d95e3d9b5963 100644 --- a/changelog.d/10532.bugfix +++ b/changelog.d/10532.bugfix @@ -1 +1 @@ -Only report protocols that are implemented by at least one service, as required by the spec when handling `GET /_matrix/client/r0/thirdparty/protocols`. \ No newline at end of file +Fix a long-standing bug where protocols which are not implemented by any appservices were incorrectly returned via `GET /_matrix/client/r0/thirdparty/protocols`.