From 90131044297ac5378fb381050f4068784dc206a8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 13 May 2022 15:30:15 +0200 Subject: [PATCH] Don't create an empty room when checking for MAU limits (#12713) --- changelog.d/12713.bugfix | 1 + .../resource_limits_server_notices.py | 40 +++-------- .../server_notices/server_notices_manager.py | 70 ++++++++++++------- .../test_resource_limits_server_notices.py | 11 ++- 4 files changed, 66 insertions(+), 56 deletions(-) create mode 100644 changelog.d/12713.bugfix diff --git a/changelog.d/12713.bugfix b/changelog.d/12713.bugfix new file mode 100644 index 0000000000..91e70f102c --- /dev/null +++ b/changelog.d/12713.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.30.0 where empty rooms could be automatically created if a monthly active users limit is set. diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 015dd08f05..b5f3a0c74e 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -21,7 +21,6 @@ from synapse.api.constants import ( ServerNoticeMsgType, ) from synapse.api.errors import AuthError, ResourceLimitError, SynapseError -from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG if TYPE_CHECKING: from synapse.server import HomeServer @@ -71,18 +70,19 @@ class ResourceLimitsServerNotices: # In practice, not sure we can ever get here return - room_id = await self._server_notices_manager.get_or_create_notice_room_for_user( + # Check if there's a server notice room for this user. + room_id = await self._server_notices_manager.maybe_get_notice_room_for_user( user_id ) - if not room_id: - logger.warning("Failed to get server notices room") - return - - await self._check_and_set_tags(user_id, room_id) - - # Determine current state of room - currently_blocked, ref_events = await self._is_room_currently_blocked(room_id) + if room_id is not None: + # Determine current state of room + currently_blocked, ref_events = await self._is_room_currently_blocked( + room_id + ) + else: + currently_blocked = False + ref_events = [] limit_msg = None limit_type = None @@ -161,26 +161,6 @@ class ResourceLimitsServerNotices: user_id, content, EventTypes.Pinned, "" ) - async def _check_and_set_tags(self, user_id: str, room_id: str) -> None: - """ - Since server notices rooms were originally not with tags, - important to check that tags have been set correctly - Args: - user_id(str): the user in question - room_id(str): the server notices room for that user - """ - tags = await self._store.get_tags_for_room(user_id, room_id) - need_to_set_tag = True - if tags: - if SERVER_NOTICE_ROOM_TAG in tags: - # tag already present, nothing to do here - need_to_set_tag = False - if need_to_set_tag: - max_id = await self._account_data_handler.add_tag_to_room( - user_id, room_id, SERVER_NOTICE_ROOM_TAG, {} - ) - self._notifier.on_new_event("account_data_key", max_id, users=[user_id]) - async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str]]: """ Determines if the room is currently blocked diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 48eae5fa06..c2c37e1015 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -90,6 +90,35 @@ class ServerNoticesManager: ) return event + @cached() + async def maybe_get_notice_room_for_user(self, user_id: str) -> Optional[str]: + """Try to look up the server notice room for this user if it exists. + + Does not create one if none can be found. + + Args: + user_id: the user we want a server notice room for. + + Returns: + The room's ID, or None if no room could be found. + """ + rooms = await self._store.get_rooms_for_local_user_where_membership_is( + user_id, [Membership.INVITE, Membership.JOIN] + ) + for room in rooms: + # it's worth noting that there is an asymmetry here in that we + # expect the user to be invited or joined, but the system user must + # be joined. This is kinda deliberate, in that if somebody somehow + # manages to invite the system user to a room, that doesn't make it + # the server notices room. + user_ids = await self._store.get_users_in_room(room.room_id) + if len(user_ids) <= 2 and self.server_notices_mxid in user_ids: + # we found a room which our user shares with the system notice + # user + return room.room_id + + return None + @cached() async def get_or_create_notice_room_for_user(self, user_id: str) -> str: """Get the room for notices for a given user @@ -112,31 +141,20 @@ class ServerNoticesManager: self.server_notices_mxid, authenticated_entity=self._server_name ) - rooms = await self._store.get_rooms_for_local_user_where_membership_is( - user_id, [Membership.INVITE, Membership.JOIN] - ) - for room in rooms: - # it's worth noting that there is an asymmetry here in that we - # expect the user to be invited or joined, but the system user must - # be joined. This is kinda deliberate, in that if somebody somehow - # manages to invite the system user to a room, that doesn't make it - # the server notices room. - user_ids = await self._store.get_users_in_room(room.room_id) - if len(user_ids) <= 2 and self.server_notices_mxid in user_ids: - # we found a room which our user shares with the system notice - # user - logger.info( - "Using existing server notices room %s for user %s", - room.room_id, - user_id, - ) - await self._update_notice_user_profile_if_changed( - requester, - room.room_id, - self._config.servernotices.server_notices_mxid_display_name, - self._config.servernotices.server_notices_mxid_avatar_url, - ) - return room.room_id + room_id = await self.maybe_get_notice_room_for_user(user_id) + if room_id is not None: + logger.info( + "Using existing server notices room %s for user %s", + room_id, + user_id, + ) + await self._update_notice_user_profile_if_changed( + requester, + room_id, + self._config.servernotices.server_notices_mxid_display_name, + self._config.servernotices.server_notices_mxid_avatar_url, + ) + return room_id # apparently no existing notice room: create a new one logger.info("Creating server notices room for %s", user_id) @@ -166,6 +184,8 @@ class ServerNoticesManager: ) room_id = info["room_id"] + self.maybe_get_notice_room_for_user.invalidate((user_id,)) + max_id = await self._account_data_handler.add_tag_to_room( user_id, room_id, SERVER_NOTICE_ROOM_TAG, {} ) diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index 9ee9509d3a..07e29788e5 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -75,6 +75,9 @@ class TestResourceLimitsServerNotices(unittest.HomeserverTestCase): self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock( return_value=make_awaitable("!something:localhost") ) + self._rlsn._server_notices_manager.maybe_get_notice_room_for_user = Mock( + return_value=make_awaitable("!something:localhost") + ) self._rlsn._store.add_tag_to_room = Mock(return_value=make_awaitable(None)) self._rlsn._store.get_tags_for_room = Mock(return_value=make_awaitable({})) @@ -102,6 +105,7 @@ class TestResourceLimitsServerNotices(unittest.HomeserverTestCase): ) self.get_success(self._rlsn.maybe_send_server_notice_to_user(self.user_id)) # Would be better to check the content, but once == remove blocking event + self._rlsn._server_notices_manager.maybe_get_notice_room_for_user.assert_called_once() self._send_notice.assert_called_once() def test_maybe_send_server_notice_to_user_remove_blocked_notice_noop(self): @@ -300,7 +304,10 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase): hasn't been reached (since it's the only user and the limit is 5), so users shouldn't receive a server notice. """ - self.register_user("user", "password") + m = Mock(return_value=make_awaitable(None)) + self._rlsn._server_notices_manager.maybe_get_notice_room_for_user = m + + user_id = self.register_user("user", "password") tok = self.login("user", "password") channel = self.make_request("GET", "/sync?timeout=0", access_token=tok) @@ -309,6 +316,8 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase): "rooms", channel.json_body, "Got invites without server notice" ) + m.assert_called_once_with(user_id) + def test_invite_with_notice(self): """Tests that, if the MAU limit is hit, the server notices user invites each user to a room in which it has sent a notice.