From 4a4d62bdf3373a92e8948a37a298bdddc37ae37a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 11:34:13 +0000 Subject: [PATCH 1/7] Prefer `HTTPStatus` over plain `int` This is an Opinion that no-one has seemed to object to yet. --- tests/rest/client/test_directory.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/rest/client/test_directory.py b/tests/rest/client/test_directory.py index d2181ea9070f..6e5cbb3e805c 100644 --- a/tests/rest/client/test_directory.py +++ b/tests/rest/client/test_directory.py @@ -11,6 +11,7 @@ # 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 http import HTTPStatus import json @@ -53,35 +54,35 @@ def prepare(self, reactor, clock, homeserver): def test_state_event_not_in_room(self): self.ensure_user_left_room() - self.set_alias_via_state_event(403) + self.set_alias_via_state_event(HTTPStatus.FORBIDDEN) def test_directory_endpoint_not_in_room(self): self.ensure_user_left_room() - self.set_alias_via_directory(403) + self.set_alias_via_directory(HTTPStatus.FORBIDDEN) def test_state_event_in_room_too_long(self): self.ensure_user_joined_room() - self.set_alias_via_state_event(400, alias_length=256) + self.set_alias_via_state_event(HTTPStatus.BAD_REQUEST, alias_length=256) def test_directory_in_room_too_long(self): self.ensure_user_joined_room() - self.set_alias_via_directory(400, alias_length=256) + self.set_alias_via_directory(HTTPStatus.BAD_REQUEST, alias_length=256) @override_config({"default_room_version": 5}) def test_state_event_user_in_v5_room(self): """Test that a regular user can add alias events before room v6""" self.ensure_user_joined_room() - self.set_alias_via_state_event(200) + self.set_alias_via_state_event(HTTPStatus.OK) @override_config({"default_room_version": 6}) def test_state_event_v6_room(self): """Test that a regular user can *not* add alias events from room v6""" self.ensure_user_joined_room() - self.set_alias_via_state_event(403) + self.set_alias_via_state_event(HTTPStatus.FORBIDDEN) def test_directory_in_room(self): self.ensure_user_joined_room() - self.set_alias_via_directory(200) + self.set_alias_via_directory(HTTPStatus.OK) def test_room_creation_too_long(self): url = "/_matrix/client/r0/createRoom" @@ -93,7 +94,7 @@ def test_room_creation_too_long(self): channel = self.make_request( "POST", url, request_data, access_token=self.user_tok ) - self.assertEqual(channel.code, 400, channel.result) + self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) def test_room_creation(self): url = "/_matrix/client/r0/createRoom" @@ -106,7 +107,7 @@ def test_room_creation(self): channel = self.make_request( "POST", url, request_data, access_token=self.user_tok ) - self.assertEqual(channel.code, 200, channel.result) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) def set_alias_via_state_event(self, expected_code, alias_length=5): url = "/_matrix/client/r0/rooms/%s/state/m.room.aliases/%s" % ( @@ -122,7 +123,7 @@ def set_alias_via_state_event(self, expected_code, alias_length=5): ) self.assertEqual(channel.code, expected_code, channel.result) - def set_alias_via_directory(self, expected_code, alias_length=5): + def set_alias_via_directory(self, expected_code: HTTPStatus, alias_length=5) -> None: url = "/_matrix/client/r0/directory/room/%s" % self.random_alias(alias_length) data = {"room_id": self.room_id} request_data = json.dumps(data) From f33f171fa53c9a44e8ad20f07703b016c50db17d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 11:38:15 +0000 Subject: [PATCH 2/7] `--disallow-untyped-defs` for `tests.rest.client.test_directory` --- mypy.ini | 3 ++ tests/rest/client/test_directory.py | 43 +++++++++++++++++------------ 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/mypy.ini b/mypy.ini index a06c7fc66b68..0c591879e356 100644 --- a/mypy.ini +++ b/mypy.ini @@ -282,6 +282,9 @@ disallow_untyped_defs = True [mypy-tests.storage.test_user_directory] disallow_untyped_defs = True +[mypy-tests.rest.client.test_directory] +disallow_untyped_defs = True + ;; Dependencies without annotations ;; Before ignoring a module, check to see if type stubs are available. ;; The `typeshed` project maintains stubs here: diff --git a/tests/rest/client/test_directory.py b/tests/rest/client/test_directory.py index 6e5cbb3e805c..fc83fa936a70 100644 --- a/tests/rest/client/test_directory.py +++ b/tests/rest/client/test_directory.py @@ -14,10 +14,13 @@ from http import HTTPStatus import json +from twisted.internet.testing import MemoryReactor from synapse.rest import admin from synapse.rest.client import directory, login, room +from synapse.server import HomeServer from synapse.types import RoomAlias +from synapse.util import Clock from synapse.util.stringutils import random_string from tests import unittest @@ -33,7 +36,7 @@ class DirectoryTestCase(unittest.HomeserverTestCase): room.register_servlets, ] - def make_homeserver(self, reactor, clock): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: config = self.default_config() config["require_membership_for_aliases"] = True @@ -41,7 +44,9 @@ def make_homeserver(self, reactor, clock): return self.hs - def prepare(self, reactor, clock, homeserver): + def prepare( + self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer + ) -> None: self.room_owner = self.register_user("room_owner", "test") self.room_owner_tok = self.login("room_owner", "test") @@ -52,39 +57,39 @@ def prepare(self, reactor, clock, homeserver): self.user = self.register_user("user", "test") self.user_tok = self.login("user", "test") - def test_state_event_not_in_room(self): + def test_state_event_not_in_room(self) -> None: self.ensure_user_left_room() self.set_alias_via_state_event(HTTPStatus.FORBIDDEN) - def test_directory_endpoint_not_in_room(self): + def test_directory_endpoint_not_in_room(self) -> None: self.ensure_user_left_room() self.set_alias_via_directory(HTTPStatus.FORBIDDEN) - def test_state_event_in_room_too_long(self): + def test_state_event_in_room_too_long(self) -> None: self.ensure_user_joined_room() self.set_alias_via_state_event(HTTPStatus.BAD_REQUEST, alias_length=256) - def test_directory_in_room_too_long(self): + def test_directory_in_room_too_long(self) -> None: self.ensure_user_joined_room() self.set_alias_via_directory(HTTPStatus.BAD_REQUEST, alias_length=256) @override_config({"default_room_version": 5}) - def test_state_event_user_in_v5_room(self): + def test_state_event_user_in_v5_room(self) -> None: """Test that a regular user can add alias events before room v6""" self.ensure_user_joined_room() self.set_alias_via_state_event(HTTPStatus.OK) @override_config({"default_room_version": 6}) - def test_state_event_v6_room(self): + def test_state_event_v6_room(self) -> None: """Test that a regular user can *not* add alias events from room v6""" self.ensure_user_joined_room() self.set_alias_via_state_event(HTTPStatus.FORBIDDEN) - def test_directory_in_room(self): + def test_directory_in_room(self) -> None: self.ensure_user_joined_room() self.set_alias_via_directory(HTTPStatus.OK) - def test_room_creation_too_long(self): + def test_room_creation_too_long(self) -> None: url = "/_matrix/client/r0/createRoom" # We use deliberately a localpart under the length threshold so @@ -96,7 +101,7 @@ def test_room_creation_too_long(self): ) self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result) - def test_room_creation(self): + def test_room_creation(self) -> None: url = "/_matrix/client/r0/createRoom" # Check with an alias of allowed length. There should already be @@ -109,7 +114,9 @@ def test_room_creation(self): ) self.assertEqual(channel.code, HTTPStatus.OK, channel.result) - def set_alias_via_state_event(self, expected_code, alias_length=5): + def set_alias_via_state_event( + self, expected_code: HTTPStatus, alias_length: int = 5 + ) -> None: url = "/_matrix/client/r0/rooms/%s/state/m.room.aliases/%s" % ( self.room_id, self.hs.hostname, @@ -123,7 +130,9 @@ def set_alias_via_state_event(self, expected_code, alias_length=5): ) self.assertEqual(channel.code, expected_code, channel.result) - def set_alias_via_directory(self, expected_code: HTTPStatus, alias_length=5) -> None: + def set_alias_via_directory( + self, expected_code: HTTPStatus, alias_length: int = 5 + ) -> None: url = "/_matrix/client/r0/directory/room/%s" % self.random_alias(alias_length) data = {"room_id": self.room_id} request_data = json.dumps(data) @@ -133,16 +142,16 @@ def set_alias_via_directory(self, expected_code: HTTPStatus, alias_length=5) -> ) self.assertEqual(channel.code, expected_code, channel.result) - def random_alias(self, length): + def random_alias(self, length: int) -> str: return RoomAlias(random_string(length), self.hs.hostname).to_string() - def ensure_user_left_room(self): + def ensure_user_left_room(self) -> None: self.ensure_membership("leave") - def ensure_user_joined_room(self): + def ensure_user_joined_room(self) -> None: self.ensure_membership("join") - def ensure_membership(self, membership): + def ensure_membership(self, membership: str) -> None: try: if membership == "leave": self.helper.leave(room=self.room_id, user=self.user, tok=self.user_tok) From 533faee70d922196c7760bba3d70cc1b25c15952 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 15:58:14 +0000 Subject: [PATCH 3/7] Improve synapse's annotations for deleting aliases --- synapse/handlers/directory.py | 6 +++++- synapse/storage/databases/main/directory.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 8ca5f60b1cf5..5a504ef5f9ac 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -204,6 +204,10 @@ async def delete_association( ) room_id = await self._delete_association(room_alias) + # The call to `_user_can_delete_alias` above ensures the alias exists. + # `_delete_association` returns `None` only if the alias doesn't exist. + # We leave the assert here for mypy's benefit. + assert room_id is not None try: await self._update_canonical_alias(requester, user_id, room_id, room_alias) @@ -225,7 +229,7 @@ async def delete_appservice_association( ) await self._delete_association(room_alias) - async def _delete_association(self, room_alias: RoomAlias) -> str: + async def _delete_association(self, room_alias: RoomAlias) -> Optional[str]: if not self.hs.is_mine(room_alias): raise SynapseError(400, "Room alias must be local") diff --git a/synapse/storage/databases/main/directory.py b/synapse/storage/databases/main/directory.py index 6daf8b8ffb58..06e2e59d91d3 100644 --- a/synapse/storage/databases/main/directory.py +++ b/synapse/storage/databases/main/directory.py @@ -126,7 +126,7 @@ def alias_txn(txn): class DirectoryStore(DirectoryWorkerStore): - async def delete_room_alias(self, room_alias: RoomAlias) -> str: + async def delete_room_alias(self, room_alias: RoomAlias) -> Optional[str]: room_id = await self.db_pool.runInteraction( "delete_room_alias", self._delete_room_alias_txn, room_alias ) From 6ccc1568bb46d346c485af8c181648b3b29b3496 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 15:57:22 +0000 Subject: [PATCH 4/7] Test case for deleting a room alias --- tests/rest/client/test_directory.py | 45 +++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_directory.py b/tests/rest/client/test_directory.py index fc83fa936a70..98d8b224db70 100644 --- a/tests/rest/client/test_directory.py +++ b/tests/rest/client/test_directory.py @@ -11,9 +11,9 @@ # 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. +import json from http import HTTPStatus -import json from twisted.internet.testing import MemoryReactor from synapse.rest import admin @@ -47,6 +47,8 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: def prepare( self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer ) -> None: + """Create two local users and access tokens for them. + One of them creates a room.""" self.room_owner = self.register_user("room_owner", "test") self.room_owner_tok = self.login("room_owner", "test") @@ -114,6 +116,41 @@ def test_room_creation(self) -> None: ) self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + def test_deleting_alias_via_directory(self) -> None: + # Add an alias for the room. We must be joined to do so. + self.ensure_user_joined_room() + alias = self.set_alias_via_directory(HTTPStatus.OK) + + # Then try to remove the alias + channel = self.make_request( + "DELETE", + f"/_matrix/client/r0/directory/room/{alias}", + access_token=self.user_tok, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + def test_deleting_nonexistant_alias(self) -> None: + # Check that no alias exists + alias = "#potato:test" + channel = self.make_request( + "GET", + f"/_matrix/client/r0/directory/room/{alias}", + access_token=self.user_tok, + ) + self.assertEqual(channel.code, HTTPStatus.NOT_FOUND, channel.result) + self.assertIn("error", channel.json_body, channel.json_body) + self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND", channel.json_body) + + # Then try to remove the alias + channel = self.make_request( + "DELETE", + f"/_matrix/client/r0/directory/room/{alias}", + access_token=self.user_tok, + ) + self.assertEqual(channel.code, HTTPStatus.NOT_FOUND, channel.result) + self.assertIn("error", channel.json_body, channel.json_body) + self.assertEqual(channel.json_body["errcode"], "M_NOT_FOUND", channel.json_body) + def set_alias_via_state_event( self, expected_code: HTTPStatus, alias_length: int = 5 ) -> None: @@ -132,8 +169,9 @@ def set_alias_via_state_event( def set_alias_via_directory( self, expected_code: HTTPStatus, alias_length: int = 5 - ) -> None: - url = "/_matrix/client/r0/directory/room/%s" % self.random_alias(alias_length) + ) -> str: + alias = self.random_alias(alias_length) + url = "/_matrix/client/r0/directory/room/%s" % alias data = {"room_id": self.room_id} request_data = json.dumps(data) @@ -141,6 +179,7 @@ def set_alias_via_directory( "PUT", url, request_data, access_token=self.user_tok ) self.assertEqual(channel.code, expected_code, channel.result) + return alias def random_alias(self, length: int) -> str: return RoomAlias(random_string(length), self.hs.hostname).to_string() From ec1ad88d40eb44f85c8db3407c127425292b6554 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 16:05:12 +0000 Subject: [PATCH 5/7] Changelog --- changelog.d/11327.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11327.misc diff --git a/changelog.d/11327.misc b/changelog.d/11327.misc new file mode 100644 index 000000000000..389e3604570d --- /dev/null +++ b/changelog.d/11327.misc @@ -0,0 +1 @@ +Test that room alias deletion works as intended. \ No newline at end of file From bc4bf29a3a2caeba32a2a3fa9eb418f7ea64083c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 18:04:27 +0000 Subject: [PATCH 6/7] Handle the race condition properly This isn't something we'd've spotted if I hadn't gone mypying this. Thanks mypy! Thypy. --- synapse/handlers/directory.py | 8 ++++---- synapse/storage/databases/main/directory.py | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 5a504ef5f9ac..7ee5c47fd96b 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -204,10 +204,10 @@ async def delete_association( ) room_id = await self._delete_association(room_alias) - # The call to `_user_can_delete_alias` above ensures the alias exists. - # `_delete_association` returns `None` only if the alias doesn't exist. - # We leave the assert here for mypy's benefit. - assert room_id is not None + if room_id is None: + # It's possible someone else deleted the association after the + # checks above, but before we did the deletion. + raise NotFoundError("Unknown room alias") try: await self._update_canonical_alias(requester, user_id, room_id, room_alias) diff --git a/synapse/storage/databases/main/directory.py b/synapse/storage/databases/main/directory.py index 06e2e59d91d3..25131b1ea957 100644 --- a/synapse/storage/databases/main/directory.py +++ b/synapse/storage/databases/main/directory.py @@ -17,6 +17,7 @@ from synapse.api.errors import SynapseError from synapse.storage._base import SQLBaseStore +from synapse.storage.database import LoggingTransaction from synapse.types import RoomAlias from synapse.util.caches.descriptors import cached @@ -133,7 +134,9 @@ async def delete_room_alias(self, room_alias: RoomAlias) -> Optional[str]: return room_id - def _delete_room_alias_txn(self, txn, room_alias: RoomAlias) -> str: + def _delete_room_alias_txn( + self, txn: LoggingTransaction, room_alias: RoomAlias + ) -> Optional[str]: txn.execute( "SELECT room_id FROM room_aliases WHERE room_alias = ?", (room_alias.to_string(),), From 18b82e0e7dd75b692b402d9a9ae2debdc065a09e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 12 Nov 2021 19:14:32 +0000 Subject: [PATCH 7/7] Use import compatible with old twisted --- tests/rest/client/test_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_directory.py b/tests/rest/client/test_directory.py index 98d8b224db70..aca03afd0e52 100644 --- a/tests/rest/client/test_directory.py +++ b/tests/rest/client/test_directory.py @@ -14,7 +14,7 @@ import json from http import HTTPStatus -from twisted.internet.testing import MemoryReactor +from twisted.test.proto_helpers import MemoryReactor from synapse.rest import admin from synapse.rest.client import directory, login, room