Reset forgotten status when rejoining a room

Fix #17781

synapse/storage/databases/main/events.py

@@ -1863,10 +1863,10 @@ def _update_current_state_txn(
             txn.execute_batch(
                 f"""
                 INSERT INTO sliding_sync_membership_snapshots
-                    (room_id, user_id, sender, membership_event_id, membership, event_stream_ordering, event_instance_name
+                    (room_id, user_id, sender, membership_event_id, membership, forgotten, event_stream_ordering, event_instance_name
                     {("," + ", ".join(sliding_sync_snapshot_keys)) if sliding_sync_snapshot_keys else ""})
                 VALUES (
-                    ?, ?, ?, ?, ?,
+                    ?, ?, ?, ?, ?, ?,
                     (SELECT stream_ordering FROM events WHERE event_id = ?),
                     (SELECT COALESCE(instance_name, 'master') FROM events WHERE event_id = ?)
                     {("," + ", ".join("?" for _ in sliding_sync_snapshot_values)) if sliding_sync_snapshot_values else ""}
@@ -1876,6 +1876,7 @@ def _update_current_state_txn(
                     sender = EXCLUDED.sender,
                     membership_event_id = EXCLUDED.membership_event_id,
                     membership = EXCLUDED.membership,
+                    forgotten = EXCLUDED.forgotten,
                     event_stream_ordering = EXCLUDED.event_stream_ordering
                     {("," + ", ".join(f"{key} = EXCLUDED.{key}" for key in sliding_sync_snapshot_keys)) if sliding_sync_snapshot_keys else ""}
                 """,
@@ -1886,6 +1887,9 @@ def _update_current_state_txn(
                         membership_info.sender,
                         membership_info.membership_event_id,
                         membership_info.membership,
+                        # Since this is a new membership, it isn't forgotten anymore (which
+                        # matches how Synapse currently thinks about the forgotten status)
+                        False,
                         # XXX: We do not use `membership_info.membership_event_stream_ordering` here
                         # because it is an unreliable value. See XXX note above.
                         membership_info.membership_event_id,
@@ -2901,6 +2905,9 @@ def _store_room_members_txn(
                     "sender": event.sender,
                     "membership_event_id": event.event_id,
                     "membership": event.membership,
+                    # Since this is a new membership, it isn't forgotten anymore (which
+                    # matches how Synapse currently thinks about the forgotten status)
+                    "forgotten": False,
                     "event_stream_ordering": event.internal_metadata.stream_ordering,
                     "event_instance_name": event.internal_metadata.instance_name,
                 }

synapse/storage/databases/main/roommember.py

@@ -1375,6 +1375,7 @@ def f(txn: LoggingTransaction) -> None:
                 keyvalues={"user_id": user_id, "room_id": room_id},
                 updatevalues={"forgotten": 1},
             )
+            # Handle updating the `sliding_sync_membership_snapshots` table
             self.db_pool.simple_update_txn(
                 txn,
                 table="sliding_sync_membership_snapshots",

tests/rest/client/sliding_sync/test_sliding_sync.py

@@ -240,6 +240,7 @@ def _create_remote_invite_room_for_user(
         self,
         invitee_user_id: str,
         unsigned_invite_room_state: Optional[List[StrippedStateEvent]],
+        invite_room_id: Optional[str] = None,
     ) -> str:
         """
         Create a fake invite for a remote room and persist it.
@@ -252,19 +253,23 @@ def _create_remote_invite_room_for_user(
             invitee_user_id: The person being invited
             unsigned_invite_room_state: List of stripped state events to assist the
                 receiver in identifying the room.
+            invite_room_id: Optional remote room ID to be invited to. When unset, we
+                will generate one.
 
         Returns:
             The room ID of the remote invite room
         """
         store = self.hs.get_datastores().main
 
-        invite_room_id = f"!test_room{self._remote_invite_count}:remote_server"
+        if invite_room_id is None:
+            invite_room_id = f"!test_room{self._remote_invite_count}:remote_server"
 
         invite_event_dict = {
             "room_id": invite_room_id,
             "sender": "@inviter:remote_server",
             "state_key": invitee_user_id,
-            "depth": 1,
+            # Just keep advancing the depth
+            "depth": self._remote_invite_count,
             "origin_server_ts": 1,
             "type": EventTypes.Member,
             "content": {"membership": Membership.INVITE},
@@ -679,6 +684,112 @@ def test_forgotten_up_to_date(self) -> None:
             exact=True,
         )
 
+    def test_rejoin_forgotten_room(self) -> None:
+        """
+        Make sure we can see a forgotten room again if we rejoin (or any new membership
+        like an invite) (no longer forgotten)
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+        user2_id = self.register_user("user2", "pass")
+        user2_tok = self.login(user2_id, "pass")
+
+        room_id = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True)
+        # User1 joins the room
+        self.helper.join(room_id, user1_id, tok=user1_tok)
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 99]],
+                    "required_state": [],
+                    "timeline_limit": 0,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+        # We should see the room (like normal)
+        self.assertIncludes(
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
+            {room_id},
+            exact=True,
+        )
+
+        # Leave and forget the room
+        self.helper.leave(room_id, user1_id, tok=user1_tok)
+        # User1 forgets the room
+        channel = self.make_request(
+            "POST",
+            f"/_matrix/client/r0/rooms/{room_id}/forget",
+            content={},
+            access_token=user1_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        # Re-join the room
+        self.helper.join(room_id, user1_id, tok=user1_tok)
+
+        # We should see the room again after re-joining
+        response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
+        self.assertIncludes(
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
+            {room_id},
+            exact=True,
+        )
+
+    def test_invited_to_forgotten_remote_room(self) -> None:
+        """
+        Make sure we can see a forgotten room again if we are invited again
+        (remote/federated out-of-band memberships)
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        # Create a remote room invite (out-of-band membership)
+        room_id = self._create_remote_invite_room_for_user(user1_id, None)
+
+        # Make the Sliding Sync request
+        sync_body = {
+            "lists": {
+                "foo-list": {
+                    "ranges": [[0, 99]],
+                    "required_state": [],
+                    "timeline_limit": 0,
+                }
+            }
+        }
+        response_body, from_token = self.do_sync(sync_body, tok=user1_tok)
+        # We should see the room (like normal)
+        self.assertIncludes(
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
+            {room_id},
+            exact=True,
+        )
+
+        # Leave and forget the room
+        self.helper.leave(room_id, user1_id, tok=user1_tok)
+        # User1 forgets the room
+        channel = self.make_request(
+            "POST",
+            f"/_matrix/client/r0/rooms/{room_id}/forget",
+            content={},
+            access_token=user1_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+        # Get invited to the room again
+        # self.helper.join(room_id, user1_id, tok=user1_tok)
+        self._create_remote_invite_room_for_user(user1_id, None, invite_room_id=room_id)
+
+        # We should see the room again after re-joining
+        response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
+        self.assertIncludes(
+            set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]),
+            {room_id},
+            exact=True,
+        )
+
     def test_ignored_user_invites_initial_sync(self) -> None:
         """
         Make sure we ignore invites if they are from one of the `m.ignored_user_list` on

tests/rest/client/test_rooms.py

@@ -2894,6 +2894,66 @@ def _check_for_reason(self, reason: str) -> None:
         self.assertEqual(event_content.get("reason"), reason, channel.result)
 
 
+class RoomForgottenTestCase(unittest.HomeserverTestCase):
+    """
+    Test forget/forgotten rooms
+    """
+
+    servlets = [
+        synapse.rest.admin.register_servlets,
+        room.register_servlets,
+        login.register_servlets,
+    ]
+
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
+        self.store = hs.get_datastores().main
+
+    def test_room_not_forgotten_after_unban(self) -> None:
+        """
+        Test what happens when someone is banned from a room, they forget the room, and
+        some time later are unbanned.
+
+        Currently, when they are unbanned, the room isn't forgotten anymore which may or
+        may not be expected.
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+        user2_id = self.register_user("user2", "pass")
+        user2_tok = self.login(user2_id, "pass")
+
+        room_id = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True)
+        self.helper.join(room_id, user1_id, tok=user1_tok)
+
+        # User1 is banned and forgets the room
+        self.helper.ban(room_id, src=user2_id, targ=user1_id, tok=user2_tok)
+        # User1 forgets the room
+        self.get_success(self.store.forget(user1_id, room_id))
+
+        # The room should show up as forgotten
+        forgotten_room_ids = self.get_success(
+            self.store.get_forgotten_rooms_for_user(user1_id)
+        )
+        self.assertIncludes(forgotten_room_ids, {room_id}, exact=True)
+
+        # Unban user1
+        self.helper.change_membership(
+            room=room_id,
+            src=user2_id,
+            targ=user1_id,
+            membership=Membership.LEAVE,
+            tok=user2_tok,
+        )
+
+        # Room is no longer forgotten because it's a new membership
+        #
+        # XXX: Is this correct? Should the room forgotten state only be reset
+        # when the user decides to join again (or is invited/knocks)
+        forgotten_room_ids = self.get_success(
+            self.store.get_forgotten_rooms_for_user(user1_id)
+        )
+        self.assertIncludes(forgotten_room_ids, set(), exact=True)
+
+
 class LabelsTestCase(unittest.HomeserverTestCase):
     servlets = [
         synapse.rest.admin.register_servlets_for_client_rest_resource,