From 367817324816abf48905a30fea58e236461eb0e2 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 28 Sep 2021 02:50:31 -0400 Subject: [PATCH 01/11] Don't set new room alias before potential 403 (#10929) Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/room.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 8fede5e935d0..a1725b825831 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -752,6 +752,15 @@ async def create_room( if not allowed_by_third_party_rules: raise SynapseError(403, "Room visibility value not allowed.") + if is_public: + if not self.config.roomdirectory.is_publishing_room_allowed( + user_id, room_id, room_alias + ): + # Lets just return a generic message, as there may be all sorts of + # reasons why we said no. TODO: Allow configurable error messages + # per alias creation rule? + raise SynapseError(403, "Not allowed to publish room") + directory_handler = self.hs.get_directory_handler() if room_alias: await directory_handler.create_association( @@ -762,15 +771,6 @@ async def create_room( check_membership=False, ) - if is_public: - if not self.config.roomdirectory.is_publishing_room_allowed( - user_id, room_id, room_alias - ): - # Lets just return a generic message, as there may be all sorts of - # reasons why we said no. TODO: Allow configurable error messages - # per alias creation rule? - raise SynapseError(403, "Not allowed to publish room") - preset_config = config.get( "preset", RoomCreationPreset.PRIVATE_CHAT From 4387c69c80dda15087948afd125ed6e283d01516 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 28 Sep 2021 03:39:28 -0400 Subject: [PATCH 02/11] Add changelog.d Signed-off-by: Andrew Ferrazzutti --- changelog.d/10929.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10929.bugfix diff --git a/changelog.d/10929.bugfix b/changelog.d/10929.bugfix new file mode 100644 index 000000000000..756bfe91071e --- /dev/null +++ b/changelog.d/10929.bugfix @@ -0,0 +1 @@ +Newly-created public rooms are now only assigned an alias if the room's creation has not been blocked by permission settings. Contributed by @AndrewFerr. From 507f6a681f0598073611e12890a98998a65b512c Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Tue, 28 Sep 2021 19:03:28 -0400 Subject: [PATCH 03/11] Fix changelog.d entry number Signed-off-by: Andrew Ferrazzutti --- changelog.d/{10929.bugfix => 10930.bugfix} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{10929.bugfix => 10930.bugfix} (100%) diff --git a/changelog.d/10929.bugfix b/changelog.d/10930.bugfix similarity index 100% rename from changelog.d/10929.bugfix rename to changelog.d/10930.bugfix From 749355efc54d07024b0287e70ba32c96972a68ec Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 29 Sep 2021 01:52:31 -0400 Subject: [PATCH 04/11] Add test for fix to #10929 Signed-off-by: Andrew Ferrazzutti --- tests/handlers/test_directory.py | 44 ++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 6a2e76ca4ac3..e2b0aab994b5 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -432,6 +432,50 @@ def test_allowed(self): self.assertEquals(200, channel.code, channel.result) +class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): + user_id = "@test:test" + data_template = '{"room_alias_name": "%%23unofficial_test%%3Atest", "visibility": "%s"}' + + servlets = [directory.register_servlets, room.register_servlets] + + def prepare(self, reactor, clock, hs): + # This time we add custom room list publication rules + config = {} + config["alias_creation_rules"] = [] + config["room_list_publication_rules"] = [ + {"user_id": "*", "alias": "*", "action": "deny"} + ] + + rd_config = RoomDirectoryConfig() + rd_config.read_config(config) + + self.hs.config.roomdirectory.is_publishing_room_allowed = ( + rd_config.is_publishing_room_allowed + ) + + return hs + + def test_denied(self): + channel = self.make_request( + "POST", + "createRoom", + (self.data_template % ("public",)).encode("ascii") + ) + self.assertEquals(403, channel.code, channel.result) + + def test_allowed(self): + channel = self.make_request( + "POST", + "createRoom", + (self.data_template % ("private",)).encode("ascii") + ) + self.assertEquals(200, channel.code, channel.result) + + def test_denied_then_allowed(self): + self.test_denied() + self.test_allowed() + + class TestRoomListSearchDisabled(unittest.HomeserverTestCase): user_id = "@test:test" From 061c3912668b52f344276f8fdc8b50d0aa3700f7 Mon Sep 17 00:00:00 2001 From: AndrewFerr Date: Wed, 29 Sep 2021 02:11:22 -0400 Subject: [PATCH 05/11] Fix comment typo in synapse/handlers/room.py Co-authored-by: David Robertson --- synapse/handlers/room.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index a1725b825831..d5423d7b7bbc 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -756,7 +756,7 @@ async def create_room( if not self.config.roomdirectory.is_publishing_room_allowed( user_id, room_id, room_alias ): - # Lets just return a generic message, as there may be all sorts of + # Let's just return a generic message, as there may be all sorts of # reasons why we said no. TODO: Allow configurable error messages # per alias creation rule? raise SynapseError(403, "Not allowed to publish room") From 7760ccc4339ea80fdf0cd30cf3108e6b9cb24a24 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 29 Sep 2021 02:17:48 -0400 Subject: [PATCH 06/11] Accept linter formatting Signed-off-by: Andrew Ferrazzutti --- tests/handlers/test_directory.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index e2b0aab994b5..f2701ddc5f76 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -434,7 +434,9 @@ def test_allowed(self): class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): user_id = "@test:test" - data_template = '{"room_alias_name": "%%23unofficial_test%%3Atest", "visibility": "%s"}' + data_template = ( + '{"room_alias_name": "%%23unofficial_test%%3Atest", "visibility": "%s"}' + ) servlets = [directory.register_servlets, room.register_servlets] @@ -457,17 +459,13 @@ def prepare(self, reactor, clock, hs): def test_denied(self): channel = self.make_request( - "POST", - "createRoom", - (self.data_template % ("public",)).encode("ascii") + "POST", "createRoom", (self.data_template % ("public",)).encode("ascii") ) self.assertEquals(403, channel.code, channel.result) def test_allowed(self): channel = self.make_request( - "POST", - "createRoom", - (self.data_template % ("private",)).encode("ascii") + "POST", "createRoom", (self.data_template % ("private",)).encode("ascii") ) self.assertEquals(200, channel.code, channel.result) From 5c93b8f6efdd27f25972c2879feacbee14560ac1 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 29 Sep 2021 18:41:25 -0400 Subject: [PATCH 07/11] Add & improve room publishing tests Signed-off-by: Andrew Ferrazzutti --- tests/handlers/test_directory.py | 40 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index f2701ddc5f76..28c51fc4ee83 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -433,10 +433,9 @@ def test_allowed(self): class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): - user_id = "@test:test" - data_template = ( - '{"room_alias_name": "%%23unofficial_test%%3Atest", "visibility": "%s"}' - ) + user_id = "@admin:test" + denied_user_id = "@test:test" + data = {"room_alias_name": "unofficial_test"} servlets = [directory.register_servlets, room.register_servlets] @@ -445,7 +444,8 @@ def prepare(self, reactor, clock, hs): config = {} config["alias_creation_rules"] = [] config["room_list_publication_rules"] = [ - {"user_id": "*", "alias": "*", "action": "deny"} + {"user_id": "*", "alias": "*", "action": "deny"}, + {"user_id": "@admin:test", "alias": "*", "action": "allow"}, ] rd_config = RoomDirectoryConfig() @@ -458,20 +458,32 @@ def prepare(self, reactor, clock, hs): return hs def test_denied(self): - channel = self.make_request( - "POST", "createRoom", (self.data_template % ("public",)).encode("ascii") + # NOTE Setting is_public=True isn't enough + self.data["visibility"] = "public" + self.helper.create_room_as( + self.denied_user_id, extra_content=self.data, expect_code=403 ) - self.assertEquals(403, channel.code, channel.result) - def test_allowed(self): - channel = self.make_request( - "POST", "createRoom", (self.data_template % ("private",)).encode("ascii") + def test_allowed_without_publish(self): + self.helper.create_room_as( + self.denied_user_id, + extra_content=self.data, + is_public=False, + expect_code=200, ) - self.assertEquals(200, channel.code, channel.result) - def test_denied_then_allowed(self): + def test_allowed_as_allowed(self): + self.helper.create_room_as( + self.user_id, extra_content=self.data, is_public=False, expect_code=200 + ) + + def test_denied_then_retry_without_publish(self): + self.test_denied() + self.test_allowed_without_publish() + + def test_denied_then_retry_as_allowed(self): self.test_denied() - self.test_allowed() + self.test_allowed_as_allowed() class TestRoomListSearchDisabled(unittest.HomeserverTestCase): From 58729e5fa3936283d8da029be2135e220ad4de48 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 29 Sep 2021 18:41:45 -0400 Subject: [PATCH 08/11] Fix more comment typos Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/directory.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 5cfba3c8176f..f65c6d5f3123 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -146,7 +146,7 @@ async def create_association( if not self.config.roomdirectory.is_alias_creation_allowed( user_id, room_id, room_alias_str ): - # Lets just return a generic message, as there may be all sorts of + # Let's just return a generic message, as there may be all sorts of # reasons why we said no. TODO: Allow configurable error messages # per alias creation rule? raise SynapseError(403, "Not allowed to create alias") @@ -462,7 +462,7 @@ async def edit_published_room_list( if not self.config.roomdirectory.is_publishing_room_allowed( user_id, room_id, room_aliases ): - # Lets just return a generic message, as there may be all sorts of + # Let's just return a generic message, as there may be all sorts of # reasons why we said no. TODO: Allow configurable error messages # per alias creation rule? raise SynapseError(403, "Not allowed to publish room") From d97145203389f510ed1acb50ceb2646bdd6f91bc Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Fri, 1 Oct 2021 01:33:24 -0400 Subject: [PATCH 09/11] Docstrings and better setup for room publishing tests Signed-off-by: Andrew Ferrazzutti --- tests/handlers/test_directory.py | 88 ++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 21 deletions(-) diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 28c51fc4ee83..1bf7e61d2726 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -15,7 +15,7 @@ from unittest.mock import Mock -import synapse +import synapse.rest.admin import synapse.api.errors from synapse.api.constants import EventTypes from synapse.config.room_directory import RoomDirectoryConfig @@ -433,19 +433,29 @@ def test_allowed(self): class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): - user_id = "@admin:test" - denied_user_id = "@test:test" data = {"room_alias_name": "unofficial_test"} - servlets = [directory.register_servlets, room.register_servlets] + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + login.register_servlets, + directory.register_servlets, + room.register_servlets + ] + 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") + + self.denied_user_id = self.register_user("denied", "pass") + self.denied_access_token = self.login("denied", "pass") + # 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": "@admin:test", "alias": "*", "action": "allow"}, + {"user_id": self.allowed_user_id, "alias": "*", "action": "allow"}, ] rd_config = RoomDirectoryConfig() @@ -457,33 +467,69 @@ def prepare(self, reactor, clock, hs): return hs - def test_denied(self): - # NOTE Setting is_public=True isn't enough - self.data["visibility"] = "public" + def test_denied_without_publication_permission(self): + """ + Try to create a room, register an alias for it, and publish it, + as a user without permission to publish rooms. + (This is used as both a standalone test & as a helper function.) + """ self.helper.create_room_as( - self.denied_user_id, extra_content=self.data, expect_code=403 + self.denied_user_id, + tok=self.denied_access_token, + extra_content=self.data, + is_public=True, + expect_code=403, ) - def test_allowed_without_publish(self): + def test_allowed_when_creating_private_room(self): + """ + Try to create a room, register an alias for it, and NOT publish it, + as a user without permission to publish rooms. + (This is used as both a standalone test & as a helper function.) + """ self.helper.create_room_as( self.denied_user_id, + tok=self.denied_access_token, extra_content=self.data, is_public=False, expect_code=200, ) - def test_allowed_as_allowed(self): + def test_allowed_with_publication_permission(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.user_id, extra_content=self.data, is_public=False, expect_code=200 - ) - - def test_denied_then_retry_without_publish(self): - self.test_denied() - self.test_allowed_without_publish() - - def test_denied_then_retry_as_allowed(self): - self.test_denied() - self.test_allowed_as_allowed() + self.allowed_user_id, + tok=self.allowed_access_token, + extra_content=self.data, + is_public=False, + expect_code=200 + ) + + 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, + retry as the same user, but without publishing the room. + + This should pass, but used to fail because the alias was registered by the first + request, even though the room creation was denied. + """ + self.test_denied_without_publication_permission() + self.test_allowed_when_creating_private_room() + + def test_can_create_with_permission_after_rejection(self): + """ + After failing to publish a room with an alias as a user without publish permission, + retry as someone with permission, using the same alias. + + This also used to fail because of the alias having been registered by the first + request, leaving it unavailable for any other user's new rooms. + """ + self.test_denied_without_publication_permission() + self.test_allowed_with_publication_permission() class TestRoomListSearchDisabled(unittest.HomeserverTestCase): From 837e39afbfb480d7c1b8750a63c7655dca9857de Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 4 Oct 2021 13:32:28 -0400 Subject: [PATCH 10/11] Accept linter formatting Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/saml.py | 2 +- tests/handlers/test_directory.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/saml.py b/synapse/handlers/saml.py index 2fed9f377a5e..fc4500617821 100644 --- a/synapse/handlers/saml.py +++ b/synapse/handlers/saml.py @@ -16,10 +16,10 @@ from typing import TYPE_CHECKING, Callable, Dict, Optional, Set, Tuple import attr -import saml2 import saml2.response from saml2.client import Saml2Client +import saml2 from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.handlers._base import BaseHandler diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 1bf7e61d2726..be008227df59 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -15,8 +15,8 @@ from unittest.mock import Mock -import synapse.rest.admin 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 @@ -439,7 +439,7 @@ class TestCreatePublishedRoomACL(unittest.HomeserverTestCase): synapse.rest.admin.register_servlets_for_client_rest_resource, login.register_servlets, directory.register_servlets, - room.register_servlets + room.register_servlets, ] hijack_auth = False @@ -506,7 +506,7 @@ def test_allowed_with_publication_permission(self): tok=self.allowed_access_token, extra_content=self.data, is_public=False, - expect_code=200 + expect_code=200, ) def test_can_create_as_private_room_after_rejection(self): From d450c7924b6e013dc66c8a9eac4fbce910cb7953 Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 4 Oct 2021 14:21:19 -0400 Subject: [PATCH 11/11] Exclude linter formatting for saml.py Signed-off-by: Andrew Ferrazzutti --- synapse/handlers/saml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/saml.py b/synapse/handlers/saml.py index fc4500617821..2fed9f377a5e 100644 --- a/synapse/handlers/saml.py +++ b/synapse/handlers/saml.py @@ -16,10 +16,10 @@ from typing import TYPE_CHECKING, Callable, Dict, Optional, Set, Tuple import attr +import saml2 import saml2.response from saml2.client import Saml2Client -import saml2 from synapse.api.errors import SynapseError from synapse.config import ConfigError from synapse.handlers._base import BaseHandler