Test room alias deletion (#11327)

* Prefer `HTTPStatus` over plain `int`

This is an Opinion that no-one has seemed to object to yet.

* `--disallow-untyped-defs` for `tests.rest.client.test_directory`
* Improve synapse's annotations for deleting aliases
* Test case for deleting a room alias
* Changelog
This commit is contained in:
David Robertson 2021-11-12 19:56:00 +00:00 committed by GitHub
parent 0bcae8ad56
commit bea815cec8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 91 additions and 31 deletions

1
changelog.d/11327.misc Normal file
View File

@ -0,0 +1 @@
Test that room alias deletion works as intended.

View File

@ -271,6 +271,9 @@ disallow_untyped_defs = True
[mypy-tests] [mypy-tests]
disallow_untyped_defs = True disallow_untyped_defs = True
[mypy-tests.rest.client.test_directory]
disallow_untyped_defs = True
;; Dependencies without annotations ;; Dependencies without annotations
;; Before ignoring a module, check to see if type stubs are available. ;; Before ignoring a module, check to see if type stubs are available.
;; The `typeshed` project maintains stubs here: ;; The `typeshed` project maintains stubs here:

View File

@ -204,6 +204,10 @@ class DirectoryHandler:
) )
room_id = await self._delete_association(room_alias) room_id = await self._delete_association(room_alias)
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: try:
await self._update_canonical_alias(requester, user_id, room_id, room_alias) await self._update_canonical_alias(requester, user_id, room_id, room_alias)
@ -225,7 +229,7 @@ class DirectoryHandler:
) )
await self._delete_association(room_alias) 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): if not self.hs.is_mine(room_alias):
raise SynapseError(400, "Room alias must be local") raise SynapseError(400, "Room alias must be local")

View File

@ -17,6 +17,7 @@ from typing import Iterable, List, Optional
from synapse.api.errors import SynapseError from synapse.api.errors import SynapseError
from synapse.storage._base import SQLBaseStore from synapse.storage._base import SQLBaseStore
from synapse.storage.database import LoggingTransaction
from synapse.types import RoomAlias from synapse.types import RoomAlias
from synapse.util.caches.descriptors import cached from synapse.util.caches.descriptors import cached
@ -126,14 +127,16 @@ class DirectoryWorkerStore(SQLBaseStore):
class DirectoryStore(DirectoryWorkerStore): 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( room_id = await self.db_pool.runInteraction(
"delete_room_alias", self._delete_room_alias_txn, room_alias "delete_room_alias", self._delete_room_alias_txn, room_alias
) )
return room_id 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( txn.execute(
"SELECT room_id FROM room_aliases WHERE room_alias = ?", "SELECT room_id FROM room_aliases WHERE room_alias = ?",
(room_alias.to_string(),), (room_alias.to_string(),),

View File

@ -11,12 +11,16 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and # See the License for the specific language governing permissions and
# limitations under the License. # limitations under the License.
import json import json
from http import HTTPStatus
from twisted.test.proto_helpers import MemoryReactor
from synapse.rest import admin from synapse.rest import admin
from synapse.rest.client import directory, login, room from synapse.rest.client import directory, login, room
from synapse.server import HomeServer
from synapse.types import RoomAlias from synapse.types import RoomAlias
from synapse.util import Clock
from synapse.util.stringutils import random_string from synapse.util.stringutils import random_string
from tests import unittest from tests import unittest
@ -32,7 +36,7 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
room.register_servlets, room.register_servlets,
] ]
def make_homeserver(self, reactor, clock): def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:
config = self.default_config() config = self.default_config()
config["require_membership_for_aliases"] = True config["require_membership_for_aliases"] = True
@ -40,7 +44,11 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
return self.hs return self.hs
def prepare(self, reactor, 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 = self.register_user("room_owner", "test")
self.room_owner_tok = self.login("room_owner", "test") self.room_owner_tok = self.login("room_owner", "test")
@ -51,39 +59,39 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
self.user = self.register_user("user", "test") self.user = self.register_user("user", "test")
self.user_tok = self.login("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.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): def test_directory_endpoint_not_in_room(self) -> None:
self.ensure_user_left_room() 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): def test_state_event_in_room_too_long(self) -> None:
self.ensure_user_joined_room() 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): def test_directory_in_room_too_long(self) -> None:
self.ensure_user_joined_room() 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}) @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""" """Test that a regular user can add alias events before room v6"""
self.ensure_user_joined_room() 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}) @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""" """Test that a regular user can *not* add alias events from room v6"""
self.ensure_user_joined_room() 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): def test_directory_in_room(self) -> None:
self.ensure_user_joined_room() 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): def test_room_creation_too_long(self) -> None:
url = "/_matrix/client/r0/createRoom" url = "/_matrix/client/r0/createRoom"
# We use deliberately a localpart under the length threshold so # We use deliberately a localpart under the length threshold so
@ -93,9 +101,9 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
channel = self.make_request( channel = self.make_request(
"POST", url, request_data, access_token=self.user_tok "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): def test_room_creation(self) -> None:
url = "/_matrix/client/r0/createRoom" url = "/_matrix/client/r0/createRoom"
# Check with an alias of allowed length. There should already be # Check with an alias of allowed length. There should already be
@ -106,9 +114,46 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
channel = self.make_request( channel = self.make_request(
"POST", url, request_data, access_token=self.user_tok "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): 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:
url = "/_matrix/client/r0/rooms/%s/state/m.room.aliases/%s" % ( url = "/_matrix/client/r0/rooms/%s/state/m.room.aliases/%s" % (
self.room_id, self.room_id,
self.hs.hostname, self.hs.hostname,
@ -122,8 +167,11 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
) )
self.assertEqual(channel.code, expected_code, channel.result) self.assertEqual(channel.code, expected_code, channel.result)
def set_alias_via_directory(self, expected_code, alias_length=5): def set_alias_via_directory(
url = "/_matrix/client/r0/directory/room/%s" % self.random_alias(alias_length) self, expected_code: HTTPStatus, alias_length: int = 5
) -> str:
alias = self.random_alias(alias_length)
url = "/_matrix/client/r0/directory/room/%s" % alias
data = {"room_id": self.room_id} data = {"room_id": self.room_id}
request_data = json.dumps(data) request_data = json.dumps(data)
@ -131,17 +179,18 @@ class DirectoryTestCase(unittest.HomeserverTestCase):
"PUT", url, request_data, access_token=self.user_tok "PUT", url, request_data, access_token=self.user_tok
) )
self.assertEqual(channel.code, expected_code, channel.result) self.assertEqual(channel.code, expected_code, channel.result)
return alias
def random_alias(self, length): def random_alias(self, length: int) -> str:
return RoomAlias(random_string(length), self.hs.hostname).to_string() 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") self.ensure_membership("leave")
def ensure_user_joined_room(self): def ensure_user_joined_room(self) -> None:
self.ensure_membership("join") self.ensure_membership("join")
def ensure_membership(self, membership): def ensure_membership(self, membership: str) -> None:
try: try:
if membership == "leave": if membership == "leave":
self.helper.leave(room=self.room_id, user=self.user, tok=self.user_tok) self.helper.leave(room=self.room_id, user=self.user, tok=self.user_tok)