From 0a5382062c0121980204eedcaf7c53298675afa4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 18 Mar 2019 18:30:07 +0000 Subject: [PATCH 1/9] broken registration test, for neil to look at --- synapse/handlers/message.py | 13 ++++++++++--- tests/handlers/test_register.py | 6 ++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index c762b58902c8..55787563c001 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -243,7 +243,14 @@ def __init__(self, hs): self.spam_checker = hs.get_spam_checker() - if self.config.block_events_without_consent_error is not None: + self._block_events_without_consent_error = ( + self.config.block_events_without_consent_error + ) + + # we need to construct a ConsentURIBuilder here, as it checks that the necessary + # config options, but *only* if we have a configuration for which we are + # going to need it. + if self._block_events_without_consent_error: self._consent_uri_builder = ConsentURIBuilder(self.config) @defer.inlineCallbacks @@ -378,7 +385,7 @@ def assert_accepted_privacy_policy(self, requester): Raises: ConsentNotGivenError: if the user has not given consent yet """ - if self.config.block_events_without_consent_error is None: + if self._block_events_without_consent_error is None: return # exempt AS users from needing consent @@ -405,7 +412,7 @@ def assert_accepted_privacy_policy(self, requester): consent_uri = self._consent_uri_builder.build_user_consent_uri( requester.user.localpart, ) - msg = self.config.block_events_without_consent_error % { + msg = self._block_events_without_consent_error % { 'consent_uri': consent_uri, } raise ConsentNotGivenError( diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index c9c150627305..ef6ad127fcd8 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -187,6 +187,12 @@ def test_auto_create_auto_join_rooms_when_support_user_exists(self): @defer.inlineCallbacks def test_auto_create_auto_join_where_no_consent(self): + """XXX what is this trying to test? I *think* it is trying to test + that we are not auto-joined to the server notices room if + block_events_without_consent_error is set, but (a) that doesn't seem to be + a sensible thing to test for, and (b) it doesn't work anyway because you can't + change the config after the EventCreationHandler has been instantiated. + """ self.hs.config.user_consent_at_registration = True self.hs.config.block_events_without_consent_error = "Error" room_alias_str = "#room:test" From 9ad68163bd3224dcbf8b7dd7c7e65a35b5cd3f73 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:19:53 +0000 Subject: [PATCH 2/9] fix test_auto_create_auto_join_where_no_consent --- synapse/handlers/register.py | 6 ++++++ tests/handlers/test_register.py | 24 ++++++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 0ec16b1d2e3d..eb81556080a5 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -23,11 +23,13 @@ from synapse.api.errors import ( AuthError, Codes, + ConsentNotGivenError, InvalidCaptchaError, LimitExceededError, RegistrationError, SynapseError, ) + from synapse.config.server import is_threepid_reserved from synapse.http.client import CaptchaServerHttpClient from synapse.http.servlet import assert_params_in_dict @@ -311,6 +313,10 @@ def _auto_join_rooms(self, user_id): ) else: yield self._join_user_to_room(fake_requester, r) + except ConsentNotGivenError as e: + # Technically not necessary to pull out this error though + # moving away from bare excepts is a good thing to do. + logger.error("Failed to join new user to %r: %r", r, e) except Exception as e: logger.error("Failed to join new user to %r: %r", r, e) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index ef6ad127fcd8..001761f2b1e4 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -21,7 +21,7 @@ from synapse.api.errors import ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester - +from synapse.api.urls import ConsentURIBuilder from tests.utils import setup_test_homeserver from .. import unittest @@ -187,18 +187,30 @@ def test_auto_create_auto_join_rooms_when_support_user_exists(self): @defer.inlineCallbacks def test_auto_create_auto_join_where_no_consent(self): - """XXX what is this trying to test? I *think* it is trying to test - that we are not auto-joined to the server notices room if - block_events_without_consent_error is set, but (a) that doesn't seem to be - a sensible thing to test for, and (b) it doesn't work anyway because you can't - change the config after the EventCreationHandler has been instantiated. + """Test to ensure that the first user is not auto-joined to a room if + they have not given general consent. """ self.hs.config.user_consent_at_registration = True self.hs.config.block_events_without_consent_error = "Error" + + # Given:- + # * a user must give consent, + # * they have not given that consent + # * The server is configured to auto-join to a room + # (and autocreate if necessary) + event_creation_handler = self.hs.get_event_creation_handler() + event_creation_handler._block_events_without_consent_error = ( + self.hs.config.block_events_without_consent_error + ) + event_creation_handler._consent_uri_builder = Mock() room_alias_str = "#room:test" self.hs.config.auto_join_rooms = [room_alias_str] + + # When the user is registered, and post consent actions are called res = yield self.handler.register(localpart='jeff') yield self.handler.post_consent_actions(res[0]) + + # Ensure that they have not been joined to the room rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) From 4bacb07a456b1364b519e3a1f01890d5b5060e13 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:38:33 +0000 Subject: [PATCH 3/9] remove redundant code and improve comments --- 4887.bugfix | 1 + synapse/handlers/register.py | 1 - tests/handlers/test_register.py | 18 ++++++++++-------- 3 files changed, 11 insertions(+), 9 deletions(-) create mode 100644 4887.bugfix diff --git a/4887.bugfix b/4887.bugfix new file mode 100644 index 000000000000..f7c953aef42b --- /dev/null +++ b/4887.bugfix @@ -0,0 +1 @@ +fix test_auto_create_auto_join_where_no_consent diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index eb81556080a5..68f73d37935f 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -29,7 +29,6 @@ RegistrationError, SynapseError, ) - from synapse.config.server import is_threepid_reserved from synapse.http.client import CaptchaServerHttpClient from synapse.http.servlet import assert_params_in_dict diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 001761f2b1e4..010e65829ed1 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -21,7 +21,7 @@ from synapse.api.errors import ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester -from synapse.api.urls import ConsentURIBuilder + from tests.utils import setup_test_homeserver from .. import unittest @@ -190,27 +190,29 @@ def test_auto_create_auto_join_where_no_consent(self): """Test to ensure that the first user is not auto-joined to a room if they have not given general consent. """ - self.hs.config.user_consent_at_registration = True - self.hs.config.block_events_without_consent_error = "Error" # Given:- # * a user must give consent, # * they have not given that consent # * The server is configured to auto-join to a room # (and autocreate if necessary) + event_creation_handler = self.hs.get_event_creation_handler() - event_creation_handler._block_events_without_consent_error = ( - self.hs.config.block_events_without_consent_error - ) + # (Messing with the internals of event_creation_handler is fragile + # but can't see a better way to do this. One option could be to subclass + # the test with custom config.) + event_creation_handler._block_events_without_consent_error = ("Error") event_creation_handler._consent_uri_builder = Mock() room_alias_str = "#room:test" self.hs.config.auto_join_rooms = [room_alias_str] - # When the user is registered, and post consent actions are called + # When:- + # * the user is registered and post consent actions are called res = yield self.handler.register(localpart='jeff') yield self.handler.post_consent_actions(res[0]) - # Ensure that they have not been joined to the room + # Then:- + # * Ensure that they have not been joined to the room rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) From 7480fd3c3798a4bc73aa0f950dff6a04a05cec46 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:41:32 +0000 Subject: [PATCH 4/9] use the correct PR no. --- 4886.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 4886.bugfix diff --git a/4886.bugfix b/4886.bugfix new file mode 100644 index 000000000000..f7c953aef42b --- /dev/null +++ b/4886.bugfix @@ -0,0 +1 @@ +fix test_auto_create_auto_join_where_no_consent From 97bd4669a16c34ec906cd2ace5e4aa701c2498a6 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:44:16 +0000 Subject: [PATCH 5/9] put it in the right directory ... --- changelog.d/4886.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4886.bugfix diff --git a/changelog.d/4886.bugfix b/changelog.d/4886.bugfix new file mode 100644 index 000000000000..f7c953aef42b --- /dev/null +++ b/changelog.d/4886.bugfix @@ -0,0 +1 @@ +fix test_auto_create_auto_join_where_no_consent From c7238134df5e6e44dfcfb7b237fad6b265ce9ba5 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:47:39 +0000 Subject: [PATCH 6/9] add a . --- 4886.bugfix | 1 - 1 file changed, 1 deletion(-) delete mode 100644 4886.bugfix diff --git a/4886.bugfix b/4886.bugfix deleted file mode 100644 index f7c953aef42b..000000000000 --- a/4886.bugfix +++ /dev/null @@ -1 +0,0 @@ -fix test_auto_create_auto_join_where_no_consent From 6e54268bb18de3e5ee86a50e7c07cff3816b9abe Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:50:20 +0000 Subject: [PATCH 7/9] add a . --- changelog.d/4886.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/4886.bugfix b/changelog.d/4886.bugfix index f7c953aef42b..b17aa92485d4 100644 --- a/changelog.d/4886.bugfix +++ b/changelog.d/4886.bugfix @@ -1 +1 @@ -fix test_auto_create_auto_join_where_no_consent +fix test_auto_create_auto_join_where_no_consent. From 195c2b6d393a2b593ae4254be451bf4ab8e3b495 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 10:51:50 +0000 Subject: [PATCH 8/9] delete --- 4887.bugfix | 1 - 1 file changed, 1 deletion(-) delete mode 100644 4887.bugfix diff --git a/4887.bugfix b/4887.bugfix deleted file mode 100644 index f7c953aef42b..000000000000 --- a/4887.bugfix +++ /dev/null @@ -1 +0,0 @@ -fix test_auto_create_auto_join_where_no_consent From c61bcae886cfa5558e2577952ce560ca2326e6be Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 19 Mar 2019 11:23:18 +0000 Subject: [PATCH 9/9] rename --- changelog.d/4886.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4886.misc diff --git a/changelog.d/4886.misc b/changelog.d/4886.misc new file mode 100644 index 000000000000..b17aa92485d4 --- /dev/null +++ b/changelog.d/4886.misc @@ -0,0 +1 @@ +fix test_auto_create_auto_join_where_no_consent.