Don't deliver announcement rooms to excluded users in legacy sync

synapse/config/server_announcements.py

@@ -21,7 +21,7 @@ incomplete.
 ## server_announcements
 
 This feature allows homeserver admins to create and manage "announcement" rooms. These
-rooms and their events will be visible to all users on the homeserver, except those
+rooms and their events are delivered to all users on the homeserver, except those
 listed in the room's `excluded_user_ids`.
 
 Read-only users don't *actually* join announcement rooms, but they will see it in their
@@ -76,8 +76,8 @@ Options for each entry:
   rooms are not federated. Defaults to an empty list, allowing no users to send
   announcements. Sender accounts must already exist and must not be deactivated.
 * `excluded_user_ids` (array): Full Matrix user IDs excluded from automatically
-  receiving announcements from this room, for example appservice users or audit
-  bots.
+  receiving announcements from this room, for example bots that act on the messages
+  they receive.
 
   Each must be a valid, local user, and must not be in `allowed_user_ids`: senders
   are joined to the room, so they always receive its events. Defaults to an empty
@@ -87,6 +87,11 @@ Options for each entry:
   rooms manually (as they are world-readable). But they will not automatically receive
   announcement events.
 
+  Changes to this list do not update rooms that clients already have. Clients of a
+  newly excluded user keep the room but stop receiving its events. A user removed
+  from the list receives new events but not the room's state until their client does
+  an initial sync.
+
 The internal room manager user (which is used to create announcement rooms and update
 their room state) is configured with `room_management_localpart` below. It is a
 management identity, not a user-visible sender, and must not be included in

synapse/handlers/sync.py

@@ -2371,7 +2371,7 @@ class SyncHandler:
         synthetic_join_announcement_room_ids = frozenset(
             room_id
             for room_id in sync_config.filter_collection.filter_rooms(
-                await self.store.get_server_announcement_room_ids()
+                await self.store.get_server_announcement_room_ids_for_user(user_id)
             )
             if room_id not in mutable_joined_room_ids
             and room_id not in mutable_rooms_to_exclude

synapse/storage/databases/main/cache.py

@@ -612,7 +612,9 @@ class CacheInvalidationWorkerStore(SQLBaseStore):
         self._attempt_to_invalidate_cache("get_room_version_id", (room_id,))
         self._attempt_to_invalidate_cache("get_room_type", (room_id,))
         self._attempt_to_invalidate_cache("get_room_encryption", (room_id,))
-        self._attempt_to_invalidate_cache("get_server_announcement_room_ids", None)
+        self._attempt_to_invalidate_cache(
+            "get_server_announcement_config_ids_by_room_id", None
+        )
 
         self._attempt_to_invalidate_cache("_get_max_event_pos", (room_id,))
 

synapse/storage/databases/main/server_announcements.py

@@ -12,6 +12,8 @@
 # <https://www.gnu.org/licenses/agpl-3.0.html>.
 #
 
+from typing import Mapping
+
 from synapse.storage.database import LoggingTransaction
 from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore
 from synapse.util.caches.descriptors import cached
@@ -23,7 +25,8 @@ class ServerAnnouncementsStore(CacheInvalidationWorkerStore):
     The unique announcement `id` identifies the same room even when its configured
     details change. It is separate from the actual room ID.
 
-    Only the set of configured room IDs is cached, as it is read on every sync.
+    Only the mapping from room ID to config ID is cached, as it is read on every
+    sync.
     """
 
     async def get_server_announcement_room(self, config_id: str) -> str | None:
@@ -47,8 +50,11 @@ class ServerAnnouncementsStore(CacheInvalidationWorkerStore):
         )
 
     @cached(num_args=0)
-    async def get_server_announcement_room_ids(self) -> frozenset[str]:
-        """Return the room IDs of the currently configured announcement rooms.
+    async def get_server_announcement_config_ids_by_room_id(
+        self,
+    ) -> Mapping[str, str]:
+        """Return the config ID of each currently configured announcement room,
+        keyed by room ID.
 
         Rooms whose config ID is no longer in the config are left out, as
         are configured rooms that have not been created yet.
@@ -59,10 +65,36 @@ class ServerAnnouncementsStore(CacheInvalidationWorkerStore):
             iterable=[
                 room.config_id for room in self.hs.config.serverannouncements.rooms
             ],
-            retcols=("room_id",),
-            desc="get_server_announcement_room_ids",
+            retcols=("room_id", "config_id"),
+            desc="get_server_announcement_config_ids_by_room_id",
+        )
+        return dict(rows)
+
+    async def get_server_announcement_room_ids(self) -> frozenset[str]:
+        """Return the room IDs of the currently configured announcement rooms,
+        as in `get_server_announcement_config_ids_by_room_id`.
+        """
+        return frozenset(await self.get_server_announcement_config_ids_by_room_id())
+
+    async def get_server_announcement_room_ids_for_user(
+        self, user_id: str
+    ) -> frozenset[str]:
+        """Return the room IDs of the announcement rooms delivered to a user:
+        the currently configured ones, minus those listing the user in
+        `excluded_user_ids`.
+        """
+        excluded_config_ids = {
+            room.config_id
+            for room in self.hs.config.serverannouncements.rooms
+            if user_id in room.excluded_user_ids
+        }
+        return frozenset(
+            room_id
+            for room_id, config_id in (
+                await self.get_server_announcement_config_ids_by_room_id()
+            ).items()
+            if config_id not in excluded_config_ids
         )
-        return frozenset(room_id for (room_id,) in rows)
 
     async def store_server_announcement_room(
         self, config_id: str, room_id: str
@@ -76,7 +108,7 @@ class ServerAnnouncementsStore(CacheInvalidationWorkerStore):
                 values={"config_id": config_id, "room_id": room_id},
             )
             self._invalidate_cache_and_stream(
-                txn, self.get_server_announcement_room_ids, ()
+                txn, self.get_server_announcement_config_ids_by_room_id, ()
             )
 
         await self.db_pool.runInteraction(

tests/rest/client/test_sync.py

@@ -2486,6 +2486,48 @@ class ServerAnnouncementsSyncTestCase(unittest.HomeserverTestCase):
             self._sync(self.bob_tok, since, full_state=True, filter=filter)
         )
 
+    def test_excluded_user(self) -> None:
+        """A user in `excluded_user_ids` is not delivered the room, while other
+        non-members still are.
+        """
+        carol = self.register_user("carol", "pass")
+        carol_tok = self.login("carol", "pass")
+        self.config.rooms = [
+            attr.evolve(self.config.rooms[0], excluded_user_ids=[self.bob])
+        ]
+        self._announce("hello")
+
+        since = self._sync(self.bob_tok)["next_batch"]
+        self._announce("again")
+
+        self._assert_not_delivered(self._sync(self.bob_tok))
+        self._assert_not_delivered(self._sync(self.bob_tok, since))
+        self._assert_not_delivered(self._sync(self.bob_tok, since, full_state=True))
+        self._assert_delivered(self._sync(carol_tok), carol)
+
+    def test_excluded_former_member(self) -> None:
+        """A member removed from the room by the config and excluded from it
+        gets their real leave instead of the synthetic join.
+        """
+        since = self._sync(self.former_tok)["next_batch"]
+
+        self.config.rooms = [
+            attr.evolve(
+                self.config.rooms[0],
+                allowed_user_ids=[self.announcer],
+                excluded_user_ids=[self.former],
+            )
+        ]
+        self.get_success(self.handler.reconcile_rooms())
+        self._announce("hello")
+
+        for response in (
+            self._sync(self.former_tok, since),
+            self._sync(self.former_tok),
+        ):
+            self.assertNotIn(self.room_id, response["rooms"].get("join", {}))
+            self.assertIn(self.room_id, response["rooms"]["leave"])
+
     def test_removed_member(self) -> None:
         """A member removed from the room by the config switches straight to the
         synthetic join and the room's current power levels, then gets later

tests/storage/test_server_announcements.py

@@ -16,6 +16,7 @@ from parameterized import parameterized
 
 from twisted.internet.testing import MemoryReactor
 
+from synapse.config.server_announcements import ServerAnnouncementRoom
 from synapse.rest.client import room
 from synapse.server import HomeServer
 from synapse.storage.databases.main.server_announcements import ServerAnnouncementsStore
@@ -126,3 +127,57 @@ class ServerAnnouncementsStoreTestCase(HomeserverTestCase):
         self.assertIsNone(
             self.get_success(reader.get_server_announcement_avatar(self.config_id))
         )
+
+    def test_room_ids_for_user_leave_out_excluded_rooms(self) -> None:
+        """A user listed in one room's `excluded_user_ids` gets every other
+        configured room, and a newly stored room shows up for both users.
+        """
+        excluded_user_id = "@bot:test"
+        self.hs.config.serverannouncements.rooms = [
+            ServerAnnouncementRoom(
+                config_id=config_id,
+                room_name=config_id,
+                room_topic=None,
+                room_avatar_url=None,
+                room_avatar_path=None,
+                allowed_user_ids=[],
+                excluded_user_ids=excluded_user_ids,
+            )
+            for config_id, excluded_user_ids in (
+                ("news", [excluded_user_id]),
+                ("maintenance", []),
+                ("incidents", []),
+            )
+        ]
+        self.get_success(
+            self.store.store_server_announcement_room("news", "!news:test")
+        )
+        self.get_success(
+            self.store.store_server_announcement_room(
+                "maintenance", "!maintenance:test"
+            )
+        )
+
+        self.assertEqual(
+            self.get_success(
+                self.store.get_server_announcement_room_ids_for_user(excluded_user_id)
+            ),
+            {"!maintenance:test"},
+        )
+        self.assertEqual(
+            self.get_success(
+                self.store.get_server_announcement_room_ids_for_user(self.user_id)
+            ),
+            {"!news:test", "!maintenance:test"},
+        )
+
+        self.get_success(
+            self.store.store_server_announcement_room("incidents", "!incidents:test")
+        )
+
+        self.assertEqual(
+            self.get_success(
+                self.store.get_server_announcement_room_ids_for_user(excluded_user_id)
+            ),
+            {"!maintenance:test", "!incidents:test"},
+        )