Fix dummy event consent insertion bug #5905

6053.bugfix

@@ -0,0 +1 @@
+Fix dummy event insertion consent bug.

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]

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(

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

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'])