From d5d47ab72040ce544da07386fe4c64659d823ec2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 18 Nov 2021 14:53:00 -0500 Subject: [PATCH 1/8] Fix checking whether a room can be published on creation. --- synapse/config/room_directory.py | 50 ++++++++++++++++++-------------- synapse/handlers/room.py | 5 +++- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/synapse/config/room_directory.py b/synapse/config/room_directory.py index 56981cac79c2..57316c59b6a0 100644 --- a/synapse/config/room_directory.py +++ b/synapse/config/room_directory.py @@ -1,4 +1,5 @@ # Copyright 2018 New Vector Ltd +# Copyright 2021 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. @@ -12,6 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import List + +from synapse.types import JsonDict from synapse.util import glob_to_regex from ._base import Config, ConfigError @@ -20,7 +24,7 @@ class RoomDirectoryConfig(Config): section = "roomdirectory" - def read_config(self, config, **kwargs): + def read_config(self, config, **kwargs) -> None: self.enable_room_list_search = config.get("enable_room_list_search", True) alias_creation_rules = config.get("alias_creation_rules") @@ -47,7 +51,7 @@ def read_config(self, config, **kwargs): _RoomDirectoryRule("room_list_publication_rules", {"action": "allow"}) ] - def generate_config_section(self, config_dir_path, server_name, **kwargs): + def generate_config_section(self, config_dir_path, server_name, **kwargs) -> str: return """ # Uncomment to disable searching the public room list. When disabled # blocks searching local and remote room lists for local and remote @@ -113,16 +117,16 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # action: allow """ - def is_alias_creation_allowed(self, user_id, room_id, alias): + def is_alias_creation_allowed(self, user_id: str, room_id: str, alias: str) -> bool: """Checks if the given user is allowed to create the given alias Args: - user_id (str) - room_id (str) - alias (str) + user_id: The user to check. + room_id: The room ID for the alias. + alias: The alias being created. Returns: - boolean: True if user is allowed to create the alias + True if user is allowed to create the alias """ for rule in self._alias_creation_rules: if rule.matches(user_id, room_id, [alias]): @@ -130,16 +134,18 @@ def is_alias_creation_allowed(self, user_id, room_id, alias): return False - def is_publishing_room_allowed(self, user_id, room_id, aliases): + def is_publishing_room_allowed( + self, user_id: str, room_id: str, aliases: List[str] + ) -> bool: """Checks if the given user is allowed to publish the room Args: - user_id (str) - room_id (str) - aliases (list[str]): any local aliases associated with the room + user_id: The user ID publishing the room. + room_id: The room being published. + aliases: any local aliases associated with the room Returns: - boolean: True if user can publish room + True if user can publish room """ for rule in self._room_list_publication_rules: if rule.matches(user_id, room_id, aliases): @@ -153,11 +159,11 @@ class _RoomDirectoryRule: creating an alias or publishing a room. """ - def __init__(self, option_name, rule): + def __init__(self, option_name: str, rule: JsonDict): """ Args: - option_name (str): Name of the config option this rule belongs to - rule (dict): The rule as specified in the config + option_name: Name of the config option this rule belongs to + rule: The rule as specified in the config """ action = rule["action"] @@ -181,18 +187,18 @@ def __init__(self, option_name, rule): except Exception as e: raise ConfigError("Failed to parse glob into regex") from e - def matches(self, user_id, room_id, aliases): + def matches(self, user_id: str, room_id: str, aliases: List[str]) -> bool: """Tests if this rule matches the given user_id, room_id and aliases. Args: - user_id (str) - room_id (str) - aliases (list[str]): The associated aliases to the room. Will be a - single element for testing alias creation, and can be empty for - testing room publishing. + user_id: The user ID to check. + room_id: The room ID to check. + aliases: The associated aliases to the room. Will be a single element + for testing alias creation, and can be empty for testing room + publishing. Returns: - boolean + True if the rule matches. """ # Note: The regexes are anchored at both ends diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index f9a099c4f3ef..88053f986997 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -775,8 +775,11 @@ async def create_room( raise SynapseError(403, "Room visibility value not allowed.") if is_public: + room_aliases = [] + if room_alias: + room_aliases.append(room_alias.to_string()) if not self.config.roomdirectory.is_publishing_room_allowed( - user_id, room_id, room_alias + user_id, room_id, room_aliases ): # Let's just return a generic message, as there may be all sorts of # reasons why we said no. TODO: Allow configurable error messages From ff36eb882703f30fe3bea7326542b0c36e7314b6 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 18 Nov 2021 15:00:04 -0500 Subject: [PATCH 2/8] Newsfragment --- changelog.d/11392.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11392.bugfix diff --git a/changelog.d/11392.bugfix b/changelog.d/11392.bugfix new file mode 100644 index 000000000000..fb158003279e --- /dev/null +++ b/changelog.d/11392.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in v1.13.0 where creating and publishing a room could cause errors if `room_list_publication_rules` is configured. From 7d0b096d9e288410bd0d38acb625354ac42e7d9f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:17:53 -0500 Subject: [PATCH 3/8] Clean-up unit tests. --- tests/handlers/test_directory.py | 53 +++++++++++++++----------------- 1 file changed, 24 insertions(+), 29 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index be008227df59..5c1f2ed6f7bb 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -394,22 +394,16 @@ class TestCreateAliasACL(unittest.HomeserverTestCase): servlets = [directory.register_servlets, room.register_servlets] - def prepare(self, reactor, clock, hs): - # We cheekily override the config to add custom alias creation rules - config = {} + def default_config(self): + config = super().default_config() + + # We cheekily override the config to add custom alias creation and + # room publication rules. config["alias_creation_rules"] = [ {"user_id": "*", "alias": "#unofficial_*", "action": "allow"} ] - config["room_list_publication_rules"] = [] - rd_config = RoomDirectoryConfig() - rd_config.read_config(config) - - self.hs.config.roomdirectory.is_alias_creation_allowed = ( - rd_config.is_alias_creation_allowed - ) - - return hs + return config def test_denied(self): room_id = self.helper.create_room_as(self.user_id) @@ -417,7 +411,7 @@ def test_denied(self): channel = self.make_request( "PUT", b"directory/room/%23test%3Atest", - ('{"room_id":"%s"}' % (room_id,)).encode("ascii"), + {"room_id": room_id}, ) self.assertEquals(403, channel.code, channel.result) @@ -427,14 +421,12 @@ def test_allowed(self): channel = self.make_request( "PUT", b"directory/room/%23unofficial_test%3Atest", - ('{"room_id":"%s"}' % (room_id,)).encode("ascii"), + {"room_id": room_id}, ) self.assertEquals(200, channel.code, channel.result) class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): - data = {"room_alias_name": "unofficial_test"} - servlets = [ synapse.rest.admin.register_servlets_for_client_rest_resource, login.register_servlets, @@ -443,27 +435,30 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): ] hijack_auth = False - def prepare(self, reactor, clock, hs): - self.allowed_user_id = self.register_user("allowed", "pass") - self.allowed_access_token = self.login("allowed", "pass") + data = {"room_alias_name": "unofficial_test"} + allowed_localpart = "allowed" - self.denied_user_id = self.register_user("denied", "pass") - self.denied_access_token = self.login("denied", "pass") + def default_config(self): + config = super().default_config() # This time we add custom room list publication rules - config = {} - config["alias_creation_rules"] = [] config["room_list_publication_rules"] = [ {"user_id": "*", "alias": "*", "action": "deny"}, - {"user_id": self.allowed_user_id, "alias": "*", "action": "allow"}, + { + "user_id": "@" + self.allowed_localpart + "*", + "alias": "*", + "action": "allow", + }, ] - rd_config = RoomDirectoryConfig() - rd_config.read_config(config) + return config - self.hs.config.roomdirectory.is_publishing_room_allowed = ( - rd_config.is_publishing_room_allowed - ) + def prepare(self, reactor, clock, hs): + self.allowed_user_id = self.register_user(self.allowed_localpart, "pass") + self.allowed_access_token = self.login(self.allowed_localpart, "pass") + + self.denied_user_id = self.register_user("denied", "pass") + self.denied_access_token = self.login("denied", "pass") return hs From 502e81e4b91a38823a0c4e6457273daccd734095 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:18:26 -0500 Subject: [PATCH 4/8] Additional unit tests for creating aliases. --- tests/handlers/test_directory.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 5c1f2ed6f7bb..6f4e78f5d6ef 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -425,6 +425,29 @@ def test_allowed(self): ) self.assertEquals(200, channel.code, channel.result) + def test_denied_during_creation(self): + """A room alias that is not allowed should be rejected during creation.""" + # Invalid room alias. + self.helper.create_room_as( + self.user_id, + expect_code=403, + extra_content={"room_alias_name": "foo"}, + ) + + def test_allowed_during_creation(self): + """A valid room alias should be allowed during creation.""" + room_id = self.helper.create_room_as( + self.user_id, + extra_content={"room_alias_name": "unofficial_test"}, + ) + + channel = self.make_request( + "GET", + b"directory/room/%23unofficial_test%3Atest", + ) + self.assertEquals(200, channel.code, channel.result) + self.assertEquals(channel.json_body["room_id"], room_id) + class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): servlets = [ From 88e48a4e217ad925d2dc83464d6a73bfd954a5b1 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:18:57 -0500 Subject: [PATCH 5/8] Fix current tests to actually publish room alias. --- tests/handlers/test_directory.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 6f4e78f5d6ef..58c8ea84af9f 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -466,12 +466,12 @@ def default_config(self): # This time we add custom room list publication rules config["room_list_publication_rules"] = [ - {"user_id": "*", "alias": "*", "action": "deny"}, { "user_id": "@" + self.allowed_localpart + "*", - "alias": "*", + "alias": "#unofficial_*", "action": "allow", }, + {"user_id": "*", "alias": "*", "action": "deny"}, ] return config @@ -523,7 +523,7 @@ def test_allowed_with_publication_permission(self): self.allowed_user_id, tok=self.allowed_access_token, extra_content=self.data, - is_public=False, + is_public=True, expect_code=200, ) From 34b7da7059502727fcced42fc6659848708be11e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:19:07 -0500 Subject: [PATCH 6/8] Additional test to skip match-all logic. --- tests/handlers/test_directory.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 58c8ea84af9f..1a4b422179a8 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -527,6 +527,20 @@ def test_allowed_with_publication_permission(self): expect_code=200, ) + def test_denied_publication_with_invalid_alias(self): + """ + Try to create a room, register an alias for it, and publish it, + as a user WITH permission to publish rooms. + (This is used as both a standalone test & as a helper function.) + """ + self.helper.create_room_as( + self.allowed_user_id, + tok=self.allowed_access_token, + extra_content={"room_alias_name": "foo"}, + is_public=True, + expect_code=403, + ) + def test_can_create_as_private_room_after_rejection(self): """ After failing to publish a room with an alias as a user without publish permission, From 06d5a251a530a8f31e11f3d53688efcd60cb6629 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:28:36 -0500 Subject: [PATCH 7/8] Lint --- tests/handlers/test_directory.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 1a4b422179a8..d5f949737e7b 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -1,4 +1,5 @@ # Copyright 2014-2016 OpenMarket Ltd +# Copyright 2021 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. @@ -12,13 +13,11 @@ # See the License for the specific language governing permissions and # limitations under the License. - from unittest.mock import Mock import synapse.api.errors import synapse.rest.admin from synapse.api.constants import EventTypes -from synapse.config.room_directory import RoomDirectoryConfig from synapse.rest.client import directory, login, room from synapse.types import RoomAlias, create_requester From 99a6cdb50594f18b4f2343000faa5b17b4e3ed64 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 19 Nov 2021 09:48:22 -0500 Subject: [PATCH 8/8] Update comments. --- tests/handlers/test_directory.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index d5f949737e7b..0ea4e753e2b2 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -396,8 +396,7 @@ class TestCreateAliasACL(unittest.HomeserverTestCase): def default_config(self): config = super().default_config() - # We cheekily override the config to add custom alias creation and - # room publication rules. + # Add custom alias creation rules to the config. config["alias_creation_rules"] = [ {"user_id": "*", "alias": "#unofficial_*", "action": "allow"} ] @@ -463,7 +462,7 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): def default_config(self): config = super().default_config() - # This time we add custom room list publication rules + # Add custom room list publication rules to the config. config["room_list_publication_rules"] = [ { "user_id": "@" + self.allowed_localpart + "*", @@ -530,7 +529,6 @@ def test_denied_publication_with_invalid_alias(self): """ Try to create a room, register an alias for it, and publish it, as a user WITH permission to publish rooms. - (This is used as both a standalone test & as a helper function.) """ self.helper.create_room_as( self.allowed_user_id,