From 67d618e111bd586fb0b4d6c92c9d43b1174b0f42 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 16:50:05 +0000 Subject: [PATCH 01/14] Allow blocking a room multiple times --- synapse/storage/room.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/storage/room.py b/synapse/storage/room.py index 41c65e112..33870b585 100644 --- a/synapse/storage/room.py +++ b/synapse/storage/room.py @@ -500,10 +500,12 @@ class RoomStore(RoomWorkerStore, SearchStore): @defer.inlineCallbacks def block_room(self, room_id, user_id): - yield self._simple_insert( + yield self._simple_upsert( table="blocked_rooms", - values={ + keyvalues={ "room_id": room_id, + }, + insertion_values={ "user_id": user_id, }, desc="block_room", From 74c46d81fa7c3e4f1cfc3688d9ce3f46d35ee5a5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 16:50:23 +0000 Subject: [PATCH 02/14] Only require consent for events with an associated request There are a number of instances where a server or admin may puppet a user to join/leave rooms, which we don't want to fail if the user has not consented to the privacy policy. We fix this by adding a check to test if the requester has an associated access_token, which is used as a proxy to answer the question of whether the action is being done on behalf of a real request from the user. --- synapse/handlers/message.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 55787563c..ac9d9c1a8 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -316,8 +316,12 @@ class EventCreationHandler(object): target, e ) + # Check if the user has accepted the privacy policy. We only do this if + # the requester has an associated access_token_id, which indicates that + # this action came from a user request rather than an automatice server + # or admin action. is_exempt = yield self._is_exempt_from_privacy_policy(builder, requester) - if not is_exempt: + if requester.access_token_id and not is_exempt: yield self.assert_accepted_privacy_policy(requester) if token_id is not None: From 6b28890543cfd128a05c3e05ad53ea1e36c932fa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 16:52:28 +0000 Subject: [PATCH 03/14] Log new room ID --- synapse/rest/client/v1/admin.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/v1/admin.py b/synapse/rest/client/v1/admin.py index 2a29f0c2a..56c253cc9 100644 --- a/synapse/rest/client/v1/admin.py +++ b/synapse/rest/client/v1/admin.py @@ -490,8 +490,13 @@ class ShutdownRoomRestServlet(ClientV1RestServlet): requester_user_id = requester.user.to_string() - logger.info("Shutting down room %r", room_id) + logger.info( + "Shutting down room %r, joining to new room: %r", + room_id, new_room_id, + ) + # This will work even if the room is already blocked, but that is + # desirable in case the first attempt at blocking the room failed below. yield self.store.block_room(room_id, requester_user_id) users = yield self.state.get_current_user_in_room(room_id) From 72a14860abadf6c8cee8960c4699f7d15da428d0 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 16:54:00 +0000 Subject: [PATCH 04/14] Gracefully handle failing to kick user --- synapse/rest/client/v1/admin.py | 46 +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/synapse/rest/client/v1/admin.py b/synapse/rest/client/v1/admin.py index 56c253cc9..56ad65515 100644 --- a/synapse/rest/client/v1/admin.py +++ b/synapse/rest/client/v1/admin.py @@ -501,34 +501,41 @@ class ShutdownRoomRestServlet(ClientV1RestServlet): users = yield self.state.get_current_user_in_room(room_id) kicked_users = [] + failed_to_kick_users = [] for user_id in users: if not self.hs.is_mine_id(user_id): continue logger.info("Kicking %r from %r...", user_id, room_id) - target_requester = create_requester(user_id) - yield self.room_member_handler.update_membership( - requester=target_requester, - target=target_requester.user, - room_id=room_id, - action=Membership.LEAVE, - content={}, - ratelimit=False - ) + try: + target_requester = create_requester(user_id) + yield self.room_member_handler.update_membership( + requester=target_requester, + target=target_requester.user, + room_id=room_id, + action=Membership.LEAVE, + content={}, + ratelimit=False + ) - yield self.room_member_handler.forget(target_requester.user, room_id) + yield self.room_member_handler.forget(target_requester.user, room_id) - yield self.room_member_handler.update_membership( - requester=target_requester, - target=target_requester.user, - room_id=new_room_id, - action=Membership.JOIN, - content={}, - ratelimit=False - ) + yield self.room_member_handler.update_membership( + requester=target_requester, + target=target_requester.user, + room_id=new_room_id, + action=Membership.JOIN, + content={}, + ratelimit=False + ) - kicked_users.append(user_id) + kicked_users.append(user_id) + except Exception: + logger.exception( + "Failed to leave old room and join new room for %r", user_id, + ) + failed_to_kick_users.append(user_id) yield self.event_creation_handler.create_and_send_nonmember_event( room_creator_requester, @@ -549,6 +556,7 @@ class ShutdownRoomRestServlet(ClientV1RestServlet): defer.returnValue((200, { "kicked_users": kicked_users, + "failed_to_kick_users": failed_to_kick_users, "local_aliases": aliases_for_room, "new_room_id": new_room_id, })) From 30e69ff9b6c1bcabc6d2dc66ce401912dc6e9d55 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 16:56:55 +0000 Subject: [PATCH 05/14] Newsfile --- changelog.d/4904.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4904.bugfix diff --git a/changelog.d/4904.bugfix b/changelog.d/4904.bugfix new file mode 100644 index 000000000..5c2d7f3cf --- /dev/null +++ b/changelog.d/4904.bugfix @@ -0,0 +1 @@ +Fix bug in shutdown room admin API where it would fail if a user in the room hadn't consented to the privacy policy. From 7d47cc1305e1504af78ff13c49b43b81b1ac5791 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 17:08:36 +0000 Subject: [PATCH 06/14] Move requester check into assert_accepted_privacy_policy --- synapse/handlers/message.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index ac9d9c1a8..345a3e0ec 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -316,12 +316,8 @@ class EventCreationHandler(object): target, e ) - # Check if the user has accepted the privacy policy. We only do this if - # the requester has an associated access_token_id, which indicates that - # this action came from a user request rather than an automatice server - # or admin action. is_exempt = yield self._is_exempt_from_privacy_policy(builder, requester) - if requester.access_token_id and not is_exempt: + if not is_exempt: yield self.assert_accepted_privacy_policy(requester) if token_id is not None: @@ -396,6 +392,13 @@ class EventCreationHandler(object): if requester.app_service is not None: return + # Check if the user has accepted the privacy policy. We only do this if + # the requester has an associated access_token_id, which indicates that + # this action came from a user request rather than an automatice server + # or admin action. + if requester.access_token_id is None: + return + user_id = requester.user.to_string() # exempt the system notices user From aa959a6c0705067cd01d1fd0ba42f51f320ed51b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 17:39:29 +0000 Subject: [PATCH 07/14] Use flags --- synapse/handlers/_base.py | 1 + synapse/handlers/deactivate_account.py | 1 + synapse/handlers/message.py | 18 +++++------------- synapse/handlers/room_member.py | 6 ++++++ synapse/rest/client/v1/admin.py | 6 ++++-- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/synapse/handlers/_base.py b/synapse/handlers/_base.py index d8d86d6ff..ac09d03ba 100644 --- a/synapse/handlers/_base.py +++ b/synapse/handlers/_base.py @@ -165,6 +165,7 @@ class BaseHandler(object): member_event.room_id, "leave", ratelimit=False, + require_consent=False, ) except Exception as e: logger.exception("Error kicking guest user: %s" % (e,)) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index 75fe50c42..97d3f31d9 100644 --- a/synapse/handlers/deactivate_account.py +++ b/synapse/handlers/deactivate_account.py @@ -164,6 +164,7 @@ class DeactivateAccountHandler(BaseHandler): room_id, "leave", ratelimit=False, + require_consent=False, ) except Exception: logger.exception( diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 345a3e0ec..587fbfbe8 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -255,7 +255,7 @@ class EventCreationHandler(object): @defer.inlineCallbacks def create_event(self, requester, event_dict, token_id=None, txn_id=None, - prev_events_and_hashes=None): + prev_events_and_hashes=None, require_consent=True): """ Given a dict from a client, create a new event. @@ -276,6 +276,9 @@ class EventCreationHandler(object): where *hashes* is a map from algorithm to hash. If None, they will be requested from the database. + + require_consent (bool): Whether to check if the requester has + consented to privacy policy. Raises: ResourceLimitError if server is blocked to some resource being exceeded @@ -317,7 +320,7 @@ class EventCreationHandler(object): ) is_exempt = yield self._is_exempt_from_privacy_policy(builder, requester) - if not is_exempt: + if require_consent and not is_exempt: yield self.assert_accepted_privacy_policy(requester) if token_id is not None: @@ -388,17 +391,6 @@ class EventCreationHandler(object): if self._block_events_without_consent_error is None: return - # exempt AS users from needing consent - if requester.app_service is not None: - return - - # Check if the user has accepted the privacy policy. We only do this if - # the requester has an associated access_token_id, which indicates that - # this action came from a user request rather than an automatice server - # or admin action. - if requester.access_token_id is None: - return - user_id = requester.user.to_string() # exempt the system notices user diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index aead9e460..71ce5b54e 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -160,6 +160,7 @@ class RoomMemberHandler(object): txn_id=None, ratelimit=True, content=None, + require_consent=True, ): user_id = target.to_string() @@ -185,6 +186,7 @@ class RoomMemberHandler(object): token_id=requester.access_token_id, txn_id=txn_id, prev_events_and_hashes=prev_events_and_hashes, + require_consent=require_consent, ) # Check if this event matches the previous membership event for the user. @@ -305,6 +307,7 @@ class RoomMemberHandler(object): third_party_signed=None, ratelimit=True, content=None, + require_consent=True, ): key = (room_id,) @@ -319,6 +322,7 @@ class RoomMemberHandler(object): third_party_signed=third_party_signed, ratelimit=ratelimit, content=content, + require_consent=require_consent, ) defer.returnValue(result) @@ -335,6 +339,7 @@ class RoomMemberHandler(object): third_party_signed=None, ratelimit=True, content=None, + require_consent=True, ): content_specified = bool(content) if content is None: @@ -516,6 +521,7 @@ class RoomMemberHandler(object): ratelimit=ratelimit, prev_events_and_hashes=prev_events_and_hashes, content=content, + require_consent=require_consent, ) defer.returnValue(res) diff --git a/synapse/rest/client/v1/admin.py b/synapse/rest/client/v1/admin.py index 56ad65515..e78876963 100644 --- a/synapse/rest/client/v1/admin.py +++ b/synapse/rest/client/v1/admin.py @@ -516,7 +516,8 @@ class ShutdownRoomRestServlet(ClientV1RestServlet): room_id=room_id, action=Membership.LEAVE, content={}, - ratelimit=False + ratelimit=False, + require_consent=False, ) yield self.room_member_handler.forget(target_requester.user, room_id) @@ -527,7 +528,8 @@ class ShutdownRoomRestServlet(ClientV1RestServlet): room_id=new_room_id, action=Membership.JOIN, content={}, - ratelimit=False + ratelimit=False, + require_consent=False, ) kicked_users.append(user_id) From 8d8834d3e7d6c0c4086ea47664c922bfa91757d4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 17:49:56 +0000 Subject: [PATCH 08/14] comment block_room --- synapse/storage/room.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/synapse/storage/room.py b/synapse/storage/room.py index 33870b585..abc9786a9 100644 --- a/synapse/storage/room.py +++ b/synapse/storage/room.py @@ -500,6 +500,15 @@ class RoomStore(RoomWorkerStore, SearchStore): @defer.inlineCallbacks def block_room(self, room_id, user_id): + """Marks the room as blocked. Can be called multiple times. + + Args: + room_id (str): Room to block + user_id (str): Who blocked it + + Returns: + Deferred + """ yield self._simple_upsert( table="blocked_rooms", keyvalues={ From cd62981a6a083945547100c8d0f30380ea17c6e3 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 20 Mar 2019 17:51:27 +0000 Subject: [PATCH 09/14] Revert spurious delete --- synapse/handlers/message.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 587fbfbe8..9b41c7b20 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -391,6 +391,10 @@ class EventCreationHandler(object): if self._block_events_without_consent_error is None: return + # exempt AS users from needing consent + if requester.app_service is not None: + return + user_id = requester.user.to_string() # exempt the system notices user From 3ecec5ede2dfaf82f9587b4740d51288d43e7be1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 10:21:15 +0000 Subject: [PATCH 10/14] Fix upsert --- synapse/storage/room.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/storage/room.py b/synapse/storage/room.py index abc9786a9..a979d4860 100644 --- a/synapse/storage/room.py +++ b/synapse/storage/room.py @@ -514,6 +514,7 @@ class RoomStore(RoomWorkerStore, SearchStore): keyvalues={ "room_id": room_id, }, + values={}, insertion_values={ "user_id": user_id, }, From 5c6f61f81c966d34d76362eb2af4c8701b3bb46b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 10:51:21 +0000 Subject: [PATCH 11/14] Add tests --- tests/rest/client/v1/test_admin.py | 67 +++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index ea03b7e52..b3ab5642b 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -20,7 +20,7 @@ import json from mock import Mock from synapse.api.constants import UserTypes -from synapse.rest.client.v1 import admin, login +from synapse.rest.client.v1 import admin, login, room from tests import unittest @@ -353,3 +353,68 @@ class UserRegisterTestCase(unittest.HomeserverTestCase): self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"]) self.assertEqual('Invalid user type', channel.json_body["error"]) + + +class ShutdownRoomTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.event_creation_handler = hs.get_event_creation_handler() + hs.config.user_consent_version = "1" + + self._consent_uri_builder = Mock() + self._consent_uri_builder.build_user_consent_uri.return_value = ( + "http://example.com" + ) + + self.store = hs.get_datastore() + + @unittest.DEBUG + def test_shutdown_room_conset(self): + admin_user = self.register_user("admin", "pass", admin=True) + admin_user_tok = self.login("admin", "pass") + + other_user = self.register_user("user", "pass") + other_user_token = self.login("user", "pass") + + room_id = self.helper.create_room_as(other_user, tok=other_user_token) + + # Assert one user in room + users_in_room = self.get_success( + self.store.get_users_in_room(room_id), + ) + self.assertEqual([other_user], users_in_room) + + # Enable require consent to send events + self.event_creation_handler._block_events_without_consent_error = "Error" + self.event_creation_handler._consent_uri_builder = self._consent_uri_builder + + # Assert that the user is getting consent error + self.helper.send(room_id, body="foo", tok=other_user_token, expect_code=403) + + # Mark the admin user as having consented + self.get_success( + self.store.user_set_consent_version(admin_user, "1"), + ) + + # Test that the admin can still send shutdown + url = "admin/shutdown_room/" + room_id + request, channel = self.make_request( + "POST", + url.encode('ascii'), + json.dumps({"new_room_user_id": admin_user}), + access_token=admin_user_tok, + ) + self.render(request) + + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Assert there is now no longer anyone in the room + users_in_room = self.get_success( + self.store.get_users_in_room(room_id), + ) + self.assertEqual([], users_in_room) From 9c9e618b93f9f6d9b72d79343b3a8977447b4f61 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 10:58:56 +0000 Subject: [PATCH 12/14] Remove debug --- tests/rest/client/v1/test_admin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index b3ab5642b..2b7ade9f6 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -373,7 +373,6 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): self.store = hs.get_datastore() - @unittest.DEBUG def test_shutdown_room_conset(self): admin_user = self.register_user("admin", "pass", admin=True) admin_user_tok = self.login("admin", "pass") From 4a8a1ac962091aa305f3f7d448a24c9e2cd138bb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 11:02:11 +0000 Subject: [PATCH 13/14] Rejig testcase to make it more extensible --- tests/rest/client/v1/test_admin.py | 39 +++++++++++++++++------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index 2b7ade9f6..fb4ac6b95 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -366,38 +366,43 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): self.event_creation_handler = hs.get_event_creation_handler() hs.config.user_consent_version = "1" - self._consent_uri_builder = Mock() - self._consent_uri_builder.build_user_consent_uri.return_value = ( + consent_uri_builder = Mock() + consent_uri_builder.build_user_consent_uri.return_value = ( "http://example.com" ) + self.event_creation_handler._consent_uri_builder = consent_uri_builder self.store = hs.get_datastore() + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + self.other_user = self.register_user("user", "pass") + self.other_user_token = self.login("user", "pass") + + # Mark the admin user as having consented + self.get_success( + self.store.user_set_consent_version(self.admin_user, "1"), + ) + def test_shutdown_room_conset(self): - admin_user = self.register_user("admin", "pass", admin=True) - admin_user_tok = self.login("admin", "pass") + self.event_creation_handler._block_events_without_consent_error = None - other_user = self.register_user("user", "pass") - other_user_token = self.login("user", "pass") - - room_id = self.helper.create_room_as(other_user, tok=other_user_token) + room_id = self.helper.create_room_as(self.other_user, tok=self.other_user_token) # Assert one user in room users_in_room = self.get_success( self.store.get_users_in_room(room_id), ) - self.assertEqual([other_user], users_in_room) + self.assertEqual([self.other_user], users_in_room) # Enable require consent to send events self.event_creation_handler._block_events_without_consent_error = "Error" - self.event_creation_handler._consent_uri_builder = self._consent_uri_builder # Assert that the user is getting consent error - self.helper.send(room_id, body="foo", tok=other_user_token, expect_code=403) - - # Mark the admin user as having consented - self.get_success( - self.store.user_set_consent_version(admin_user, "1"), + self.helper.send( + room_id, + body="foo", tok=self.other_user_token, expect_code=403, ) # Test that the admin can still send shutdown @@ -405,8 +410,8 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): request, channel = self.make_request( "POST", url.encode('ascii'), - json.dumps({"new_room_user_id": admin_user}), - access_token=admin_user_tok, + json.dumps({"new_room_user_id": self.admin_user}), + access_token=self.admin_user_tok, ) self.render(request) From cd80cbffea0e4dc28e01d46b6a87915e7b58244d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 21 Mar 2019 11:22:26 +0000 Subject: [PATCH 14/14] Fix typo and add description --- tests/rest/client/v1/test_admin.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_admin.py b/tests/rest/client/v1/test_admin.py index fb4ac6b95..0caa4aa80 100644 --- a/tests/rest/client/v1/test_admin.py +++ b/tests/rest/client/v1/test_admin.py @@ -385,7 +385,11 @@ class ShutdownRoomTestCase(unittest.HomeserverTestCase): self.store.user_set_consent_version(self.admin_user, "1"), ) - def test_shutdown_room_conset(self): + def test_shutdown_room_consent(self): + """Test that we can shutdown rooms with local users who have not + yet accepted the privacy policy. This used to fail when we tried to + force part the user from the old room. + """ self.event_creation_handler._block_events_without_consent_error = None room_id = self.helper.create_room_as(self.other_user, tok=self.other_user_token)