diff --git a/6053.bugfix b/6053.bugfix new file mode 100644 index 000000000000..56375ce71705 --- /dev/null +++ b/6053.bugfix @@ -0,0 +1 @@ +Fix dummy event insertion consent bug. diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 136c0f955379..3dd20bd1d63a 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -257,6 +257,12 @@ def __init__(self, hs): self._block_events_without_consent_error = ( self.config.block_events_without_consent_error ) + # Some rooms should be excluded from dummy insertion, for instance rooms + # without local users who can send events into the room. + self._rooms_to_exclude_from_dummy_event_insertion = {} + # Rooms can be excluded from dummy event insertion, but should be rechecked + # from time to time. + self._ROOM_EXCLUSION_EXPIRY = 7 * 24 * 60 * 60 * 1000 # 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 @@ -468,6 +474,7 @@ def assert_accepted_privacy_policy(self, requester): return u = yield self.store.get_user_by_id(user_id) + assert u is not None if u["user_type"] in (UserTypes.SUPPORT, UserTypes.BOT): # support and bot users are not required to consent @@ -477,7 +484,6 @@ def assert_accepted_privacy_policy(self, requester): return if u["consent_version"] == self.config.user_consent_version: return - consent_uri = self._consent_uri_builder.build_user_consent_uri( requester.user.localpart ) @@ -888,11 +894,12 @@ def _send_dummy_events_to_fill_extremities(self): """Background task to send dummy events into rooms that have a large number of extremities """ - + self._expire_rooms_to_exclude_from_dummy_event_insertion() room_ids = yield self.store.get_rooms_with_many_extremities( - min_count=10, limit=5 + min_count=10, + limit=5, + room_id_filter=self._rooms_to_exclude_from_dummy_event_insertion.keys() ) - for room_id in room_ids: # For each room we need to find a joined member we can use to send # the dummy event with. @@ -904,28 +911,44 @@ def _send_dummy_events_to_fill_extremities(self): members = yield self.state.get_current_users_in_room( room_id, latest_event_ids=latest_event_ids ) - user_id = None + for member in members: if self.hs.is_mine_id(member): user_id = member requester = create_requester(user_id) - - event, context = yield self.create_event( - requester, - { - "type": "org.matrix.dummy_event", - "content": {}, - "room_id": room_id, - "sender": user_id, - }, - prev_events_and_hashes=prev_events_and_hashes, - ) - - event.internal_metadata.proactively_send = False try: + event, context = yield self.create_event( + requester, + { + "type": "org.matrix.dummy_event", + "content": {}, + "room_id": room_id, + "sender": user_id, + }, + prev_events_and_hashes=prev_events_and_hashes, + ) + + event.internal_metadata.proactively_send = False + yield self.send_nonmember_event(requester, event, context, ratelimit=False) break except ConsentNotGivenError: # Failed to send dummy event due to lack of consent, try another user - pass + user_id = None + + if user_id is None: + # Did not find a valid user in the room, so remove from future attempts + # The store is reset on start up, which creates a crude retry mechanism + now = self.clock.time_msec() + self._rooms_to_exclude_from_dummy_event_insertion[room_id] = now + + def _expire_rooms_to_exclude_from_dummy_event_insertion(self): + expire_before = self.clock.time_msec() - self._ROOM_EXCLUSION_EXPIRY + to_expire = set() + for room_id, time in self._rooms_to_exclude_from_dummy_event_insertion.items(): + if time < expire_before: + to_expire.add(room_id) + for room_id in to_expire: + logger.debug("Expiring room id %s", room_id) + del self._rooms_to_exclude_from_dummy_event_insertion[room_id] diff --git a/synapse/storage/event_federation.py b/synapse/storage/event_federation.py index 4f500d893e74..f4c9635f3daa 100644 --- a/synapse/storage/event_federation.py +++ b/synapse/storage/event_federation.py @@ -190,7 +190,7 @@ def get_latest_event_ids_and_hashes_in_room(self, room_id): room_id, ) - def get_rooms_with_many_extremities(self, min_count, limit): + def get_rooms_with_many_extremities(self, min_count, limit, room_id_filter): """Get the top rooms with at least N extremities. Args: @@ -203,15 +203,33 @@ def get_rooms_with_many_extremities(self, min_count, limit): """ def _get_rooms_with_many_extremities_txn(txn): - sql = """ + base_sql_pre = """ SELECT room_id FROM event_forward_extremities GROUP BY room_id HAVING count(*) > ? + """ + base_sql_post = """ ORDER BY count(*) DESC LIMIT ? """ - - txn.execute(sql, (min_count, limit)) + query_args = [limit] + # Need if/else since 'AND room_id NOT IN ({})' fails on Postgres + # when len(room_id_filter) == 0. Works fine on sqlite. + if len(room_id_filter) > 0: + # questionmarks is a hack to overcome sqlite not supporting + # tuples in 'WHERE IN %s' + questionmarks = "?" * len(room_id_filter) + + query_args.extend(room_id_filter) + + sql = base_sql_pre + """ AND room_id NOT IN ({})""".format( + ",".join(questionmarks) + ) + " " + base_sql_post + else: + sql = base_sql_pre + base_sql_post + + query_args.append(limit) + txn.execute(sql, query_args) return [room_id for room_id, in txn] return self.runInteraction( diff --git a/tests/storage/test_cleanup_extrems.py b/tests/storage/test_cleanup_extrems.py index e9e2d5337c20..5afadfc1ff75 100644 --- a/tests/storage/test_cleanup_extrems.py +++ b/tests/storage/test_cleanup_extrems.py @@ -15,6 +15,10 @@ import os.path +from mock import Mock + +import synapse.rest.admin +from synapse.rest.client.v1 import login, room from synapse.storage import prepare_database from synapse.types import Requester, UserID @@ -225,6 +229,14 @@ def test_forked_graph_cleanup(self): class CleanupExtremDummyEventsTestCase(HomeserverTestCase): + CONSENT_VERSION = "1" + EXTREMITIES_COUNT = 50 + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + login.register_servlets, + room.register_servlets, + ] + def make_homeserver(self, reactor, clock): config = self.default_config() config["cleanup_extremities_with_dummy_events"] = True @@ -233,33 +245,102 @@ def make_homeserver(self, reactor, clock): def prepare(self, reactor, clock, homeserver): self.store = homeserver.get_datastore() self.room_creator = homeserver.get_room_creation_handler() + self.event_creator_handler = homeserver.get_event_creation_handler() # Create a test user and room - self.user = UserID("alice", "test") + self.user = UserID.from_string(self.register_user("user1", "password")) self.requester = Requester(self.user, None, False, None, None) info = self.get_success(self.room_creator.create_room(self.requester, {})) self.room_id = info["room_id"] + self.event_creator = homeserver.get_event_creation_handler() + homeserver.config.user_consent_version = self.CONSENT_VERSION def test_send_dummy_event(self): - # Create a bushy graph with 50 extremities. + self._create_extremity_rich_graph() - event_id_start = self.create_and_send_event(self.room_id, self.user) - - for _ in range(50): - self.create_and_send_event( - self.room_id, self.user, prev_event_ids=[event_id_start] - ) + # Pump the reactor repeatedly so that the background updates have a + # chance to run. + self.pump(10 * 60) latest_event_ids = self.get_success( self.store.get_latest_event_ids_in_room(self.room_id) ) - self.assertEqual(len(latest_event_ids), 50) + self.assertTrue(len(latest_event_ids) < 10, len(latest_event_ids)) + + def test_send_dummy_event_without_consent(self): + self._create_extremity_rich_graph() + self._enable_consent_checking() # Pump the reactor repeatedly so that the background updates have a - # chance to run. + # chance to run. Attempt to add dummy event with user that has not consented + # Check that dummy event send fails. + self.pump(10 * 60) + latest_event_ids = self.get_success( + self.store.get_latest_event_ids_in_room(self.room_id) + ) + self.assertTrue(len(latest_event_ids) == self.EXTREMITIES_COUNT) + + # Room will have been black listed, so force flush of cache + self.event_creator_handler._ROOM_EXCLUSION_EXPIRY = 0 + self.pump(10 * 60) + + # Create new user, and add consent + user2 = self.register_user("user2", "password") + token2 = self.login("user2", "password") + self.get_success(self.store.user_set_consent_version(user2, self.CONSENT_VERSION)) + self.helper.join(self.room_id, user2, tok=token2) + + # Background updates should now cause a dummy event to be added to the graph self.pump(10 * 60) latest_event_ids = self.get_success( self.store.get_latest_event_ids_in_room(self.room_id) ) self.assertTrue(len(latest_event_ids) < 10, len(latest_event_ids)) + + def test_expiry_logic(self): + """Simple test to ensure that _expire_rooms_to_exclude_from_dummy_event_insertion() + expires old entries correctly. + """ + self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion['1'] = 100000 + self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion['2'] = 200000 + self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion['3'] = 300000 + self.event_creator_handler._ROOM_EXCLUSION_EXPIRY = 250 + self.event_creator_handler._expire_rooms_to_exclude_from_dummy_event_insertion() + # All entries within time frame + self.assertEqual( + len(self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion), 3 + ) + # Oldest room to expire + self.pump(1) + self.event_creator_handler._expire_rooms_to_exclude_from_dummy_event_insertion() + self.assertEqual( + len(self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion), 2 + ) + # All rooms to expire + self.pump(2) + self.assertEqual( + len(self.event_creator_handler._rooms_to_exclude_from_dummy_event_insertion), 0 + ) + + def _create_extremity_rich_graph(self): + """Helper method to create bushy graph on demand""" + + event_id_start = self.create_and_send_event(self.room_id, self.user) + + for _ in range(self.EXTREMITIES_COUNT): + self.create_and_send_event( + self.room_id, self.user, prev_event_ids=[event_id_start] + ) + + latest_event_ids = self.get_success( + self.store.get_latest_event_ids_in_room(self.room_id) + ) + self.assertEqual(len(latest_event_ids), 50) + + def _enable_consent_checking(self): + """Helper method to enable consent checking""" + self.event_creator._block_events_without_consent_error = "No consent from user" + consent_uri_builder = Mock() + consent_uri_builder.build_user_consent_uri.return_value = "http://example.com" + self.event_creator._consent_uri_builder = consent_uri_builder diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py index 86c7ac350d49..7f1480d6c81f 100644 --- a/tests/storage/test_event_federation.py +++ b/tests/storage/test_event_federation.py @@ -75,3 +75,36 @@ def insert_event(txn, i): el = r[i] depth = el[2] self.assertLessEqual(5, depth) + + @defer.inlineCallbacks + def test_get_rooms_with_many_extremities(self): + + def insert_event(txn, i, room_id): + event_id = "$event_%i:local" % i + + txn.execute( + ( + "INSERT INTO event_forward_extremities (room_id, event_id) " + "VALUES (?, ?)" + ), + (room_id, event_id), + ) + for i in range(0, 20): + yield self.store.runInteraction("insert", insert_event, i, '#room1') + yield self.store.runInteraction("insert", insert_event, i, '#room2') + yield self.store.runInteraction("insert", insert_event, i, '#room3') + + # Test simple case + r = yield self.store.get_rooms_with_many_extremities(5, 5, []) + self.assertEqual(len(r), 3) + + # Does filter work? + r = yield self.store.get_rooms_with_many_extremities(5, 5, ['#room1']) + self.assertEqual(r, ['#room2', '#room3']) + + r = yield self.store.get_rooms_with_many_extremities(5, 5, ['#room1', '#room2']) + self.assertEqual(r, ['#room3']) + + # Does filter and limit work? + r = yield self.store.get_rooms_with_many_extremities(5, 1, ['#room1']) + self.assertEqual(r, ['#room2'] or ['#room3'])