Don't create an empty room when checking for MAU limits (#12713)

This commit is contained in:
Brendan Abolivier 2022-05-13 15:30:15 +02:00 committed by GitHub
parent aec69d2481
commit 9013104429
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 66 additions and 56 deletions

1
changelog.d/12713.bugfix Normal file
View File

@ -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.

View File

@ -21,7 +21,6 @@ from synapse.api.constants import (
ServerNoticeMsgType, ServerNoticeMsgType,
) )
from synapse.api.errors import AuthError, ResourceLimitError, SynapseError from synapse.api.errors import AuthError, ResourceLimitError, SynapseError
from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG
if TYPE_CHECKING: if TYPE_CHECKING:
from synapse.server import HomeServer from synapse.server import HomeServer
@ -71,18 +70,19 @@ class ResourceLimitsServerNotices:
# In practice, not sure we can ever get here # In practice, not sure we can ever get here
return 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 user_id
) )
if not room_id: if room_id is not None:
logger.warning("Failed to get server notices room") # Determine current state of room
return currently_blocked, ref_events = await self._is_room_currently_blocked(
room_id
await self._check_and_set_tags(user_id, room_id) )
else:
# Determine current state of room currently_blocked = False
currently_blocked, ref_events = await self._is_room_currently_blocked(room_id) ref_events = []
limit_msg = None limit_msg = None
limit_type = None limit_type = None
@ -161,26 +161,6 @@ class ResourceLimitsServerNotices:
user_id, content, EventTypes.Pinned, "" 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]]: async def _is_room_currently_blocked(self, room_id: str) -> Tuple[bool, List[str]]:
""" """
Determines if the room is currently blocked Determines if the room is currently blocked

View File

@ -90,6 +90,35 @@ class ServerNoticesManager:
) )
return event 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() @cached()
async def get_or_create_notice_room_for_user(self, user_id: str) -> str: async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
"""Get the room for notices for a given user """Get the room for notices for a given user
@ -112,31 +141,20 @@ class ServerNoticesManager:
self.server_notices_mxid, authenticated_entity=self._server_name self.server_notices_mxid, authenticated_entity=self._server_name
) )
rooms = await self._store.get_rooms_for_local_user_where_membership_is( room_id = await self.maybe_get_notice_room_for_user(user_id)
user_id, [Membership.INVITE, Membership.JOIN] if room_id is not None:
) logger.info(
for room in rooms: "Using existing server notices room %s for user %s",
# it's worth noting that there is an asymmetry here in that we room_id,
# expect the user to be invited or joined, but the system user must user_id,
# 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 await self._update_notice_user_profile_if_changed(
# the server notices room. requester,
user_ids = await self._store.get_users_in_room(room.room_id) room_id,
if len(user_ids) <= 2 and self.server_notices_mxid in user_ids: self._config.servernotices.server_notices_mxid_display_name,
# we found a room which our user shares with the system notice self._config.servernotices.server_notices_mxid_avatar_url,
# user )
logger.info( return room_id
"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
# apparently no existing notice room: create a new one # apparently no existing notice room: create a new one
logger.info("Creating server notices room for %s", user_id) logger.info("Creating server notices room for %s", user_id)
@ -166,6 +184,8 @@ class ServerNoticesManager:
) )
room_id = info["room_id"] 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( max_id = await self._account_data_handler.add_tag_to_room(
user_id, room_id, SERVER_NOTICE_ROOM_TAG, {} user_id, room_id, SERVER_NOTICE_ROOM_TAG, {}
) )

View File

@ -75,6 +75,9 @@ class TestResourceLimitsServerNotices(unittest.HomeserverTestCase):
self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock( self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock(
return_value=make_awaitable("!something:localhost") 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.add_tag_to_room = Mock(return_value=make_awaitable(None))
self._rlsn._store.get_tags_for_room = Mock(return_value=make_awaitable({})) 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)) 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 # 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() self._send_notice.assert_called_once()
def test_maybe_send_server_notice_to_user_remove_blocked_notice_noop(self): 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 hasn't been reached (since it's the only user and the limit is 5), so users
shouldn't receive a server notice. 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") tok = self.login("user", "password")
channel = self.make_request("GET", "/sync?timeout=0", access_token=tok) 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" "rooms", channel.json_body, "Got invites without server notice"
) )
m.assert_called_once_with(user_id)
def test_invite_with_notice(self): def test_invite_with_notice(self):
"""Tests that, if the MAU limit is hit, the server notices user invites each user """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. to a room in which it has sent a notice.