mirrors/synapse
1
0
Fork 0
mirror of https://github.com/element-hq/synapse.git synced 2024-11-21 17:15:38 +03:00
Code Issues Projects Releases Packages Wiki Activity

Server notices: Dissociate room creation/lookup from invite (#7199)

Browse source 
Fixes #6815

Before figuring out whether we should alert a user on MAU, we call get_notice_room_for_user to get some info on the existing server notices room for this user. This function, if the room doesn't exist, creates it and invites the user in it. This means that, if we decide later that no server notice is needed, the user gets invited in a room with no message in it. This happens at every restart of the server, since the room ID returned by get_notice_room_for_user is cached.

This PR fixes that by moving the inviting bit to a dedicated function, that's only called when the server actually needs to send a notice to the user. A potential issue with this approach is that the room that's created by get_notice_room_for_user doesn't match how that same function looks for an existing room (i.e. it creates a room that doesn't have an invite or a join for the current user in it, so it could lead to a new room being created each time a user syncs), but I'm not sure this is a problem given it's cached until the server restarts, so that function won't run very often.

It also renames get_notice_room_for_user into get_or_create_notice_room_for_user to make what it does clearer.
This commit is contained in:
Brendan Abolivier 2020-04-04 17:27:45 +02:00 committed by GitHub
parent 694d8bed0e
commit d73bf18d13
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 154 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
changelog.d/7199.bugfix Normal file
View file

@ -0,0 +1 @@
Fix a bug that could cause a user to be invited to a server notices (aka System Alerts) room without any notice being sent.

4
synapse/server_notices/resource_limits_server_notices.py
View file

@ -80,7 +80,9 @@ class ResourceLimitsServerNotices(object):
# In practice, not sure we can ever get here
return
room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id)
room_id = yield self._server_notices_manager.get_or_create_notice_room_for_user(
user_id
)
if not room_id:
logger.warning("Failed to get server notices room")

51
synapse/server_notices/server_notices_manager.py
View file

@ -17,7 +17,7 @@ import logging
from twisted.internet import defer
from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
from synapse.types import create_requester
from synapse.types import UserID, create_requester
from synapse.util.caches.descriptors import cachedInlineCallbacks
logger = logging.getLogger(__name__)
@ -36,10 +36,12 @@ class ServerNoticesManager(object):
self._store = hs.get_datastore()
self._config = hs.config
self._room_creation_handler = hs.get_room_creation_handler()
self._room_member_handler = hs.get_room_member_handler()
self._event_creation_handler = hs.get_event_creation_handler()
self._is_mine_id = hs.is_mine_id
self._notifier = hs.get_notifier()
self.server_notices_mxid = self._config.server_notices_mxid
def is_enabled(self):
"""Checks if server notices are enabled on this server.
@ -66,7 +68,8 @@ class ServerNoticesManager(object):
Returns:
Deferred[FrozenEvent]
"""
room_id = yield self.get_notice_room_for_user(user_id)
room_id = yield self.get_or_create_notice_room_for_user(user_id)
yield self.maybe_invite_user_to_room(user_id, room_id)
system_mxid = self._config.server_notices_mxid
requester = create_requester(system_mxid)
@ -89,10 +92,11 @@ class ServerNoticesManager(object):
return res
@cachedInlineCallbacks()
def get_notice_room_for_user(self, user_id):
def get_or_create_notice_room_for_user(self, user_id):
"""Get the room for notices for a given user
If we have not yet created a notice room for this user, create it
If we have not yet created a notice room for this user, create it, but don't
invite the user to it.
Args:
user_id (str): complete user id for the user we want a room for
@ -108,7 +112,6 @@ class ServerNoticesManager(object):
rooms = yield self._store.get_rooms_for_local_user_where_membership_is(
user_id, [Membership.INVITE, Membership.JOIN]
)
system_mxid = self._config.server_notices_mxid
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
@ -116,10 +119,14 @@ class ServerNoticesManager(object):
# manages to invite the system user to a room, that doesn't make it
# the server notices room.
user_ids = yield self._store.get_users_in_room(room.room_id)
if system_mxid in user_ids:
if self.server_notices_mxid in user_ids:
# we found a room which our user shares with the system notice
# user
logger.info("Using room %s", room.room_id)
logger.info(
"Using existing server notices room %s for user %s",
room.room_id,
user_id,
)
return room.room_id
# apparently no existing notice room: create a new one
@ -138,14 +145,13 @@ class ServerNoticesManager(object):
"avatar_url": self._config.server_notices_mxid_avatar_url,
}
requester = create_requester(system_mxid)
requester = create_requester(self.server_notices_mxid)
info = yield self._room_creation_handler.create_room(
requester,
config={
"preset": RoomCreationPreset.PRIVATE_CHAT,
"name": self._config.server_notices_room_name,
"power_level_content_override": {"users_default": -10},
"invite": (user_id,),
},
ratelimit=False,
creator_join_profile=join_profile,
@ -159,3 +165,30 @@ class ServerNoticesManager(object):
logger.info("Created server notices room %s for %s", room_id, user_id)
return room_id
@defer.inlineCallbacks
def maybe_invite_user_to_room(self, user_id: str, room_id: str):
"""Invite the given user to the given server room, unless the user has already
joined or been invited to it.
Args:
user_id: The ID of the user to invite.
room_id: The ID of the room to invite the user to.
"""
requester = create_requester(self.server_notices_mxid)
# Check whether the user has already joined or been invited to this room. If
# that's the case, there is no need to re-invite them.
joined_rooms = yield self._store.get_rooms_for_local_user_where_membership_is(
user_id, [Membership.INVITE, Membership.JOIN]
)
for room in joined_rooms:
if room.room_id == room_id:
return
yield self._room_member_handler.update_membership(
requester=requester,
target=UserID.from_string(user_id),
room_id=room_id,
action="invite",
)

120
tests/server_notices/test_resource_limits_server_notices.py
View file

@ -19,6 +19,9 @@ from twisted.internet import defer
from synapse.api.constants import EventTypes, LimitBlockingTypes, ServerNoticeMsgType
from synapse.api.errors import ResourceLimitError
from synapse.rest import admin
from synapse.rest.client.v1 import login, room
from synapse.rest.client.v2_alpha import sync
from synapse.server_notices.resource_limits_server_notices import (
ResourceLimitsServerNotices,
)
@ -67,7 +70,7 @@ class TestResourceLimitsServerNotices(unittest.HomeserverTestCase):
# self.server_notices_mxid_avatar_url = None
# self.server_notices_room_name = "Server Notices"
self._rlsn._server_notices_manager.get_notice_room_for_user = Mock(
self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock(
returnValue=""
)
self._rlsn._store.add_tag_to_room = Mock()
@ -215,6 +218,26 @@ class TestResourceLimitsServerNotices(unittest.HomeserverTestCase):
class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
servlets = [
admin.register_servlets,
login.register_servlets,
room.register_servlets,
sync.register_servlets,
]
def default_config(self):
c = super().default_config()
c["server_notices"] = {
"system_mxid_localpart": "server",
"system_mxid_display_name": None,
"system_mxid_avatar_url": None,
"room_name": "Test Server Notice Room",
}
c["limit_usage_by_mau"] = True
c["max_mau_value"] = 5
c["admin_contact"] = "mailto:user@test.com"
return c
def prepare(self, reactor, clock, hs):
self.store = self.hs.get_datastore()
self.server_notices_sender = self.hs.get_server_notices_sender()
@ -228,18 +251,8 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
if not isinstance(self._rlsn, ResourceLimitsServerNotices):
raise Exception("Failed to find reference to ResourceLimitsServerNotices")
self.hs.config.limit_usage_by_mau = True
self.hs.config.hs_disabled = False
self.hs.config.max_mau_value = 5
self.hs.config.server_notices_mxid = "@server:test"
self.hs.config.server_notices_mxid_display_name = None
self.hs.config.server_notices_mxid_avatar_url = None
self.hs.config.server_notices_room_name = "Test Server Notice Room"
self.user_id = "@user_id:test"
self.hs.config.admin_contact = "mailto:user@test.com"
def test_server_notice_only_sent_once(self):
self.store.get_monthly_active_count = Mock(return_value=1000)
@ -253,7 +266,7 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
# Now lets get the last load of messages in the service notice room and
# check that there is only one server notice
room_id = self.get_success(
self.server_notices_manager.get_notice_room_for_user(self.user_id)
self.server_notices_manager.get_or_create_notice_room_for_user(self.user_id)
)
token = self.get_success(self.event_source.get_current_token())
@ -273,3 +286,86 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
count += 1
self.assertEqual(count, 1)
def test_no_invite_without_notice(self):
"""Tests that a user doesn't get invited to a server notices room without a
server notice being sent.
The scenario for this test is a single user on a server where the MAU limit
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")
tok = self.login("user", "password")
request, channel = self.make_request("GET", "/sync?timeout=0", access_token=tok)
self.render(request)
invites = channel.json_body["rooms"]["invite"]
self.assertEqual(len(invites), 0, invites)
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.
"""
user_id, tok, room_id = self._trigger_notice_and_join()
# Sync again to retrieve the events in the room, so we can check whether this
# room has a notice in it.
request, channel = self.make_request("GET", "/sync?timeout=0", access_token=tok)
self.render(request)
# Scan the events in the room to search for a message from the server notices
# user.
events = channel.json_body["rooms"]["join"][room_id]["timeline"]["events"]
notice_in_room = False
for event in events:
if (
event["type"] == EventTypes.Message
and event["sender"] == self.hs.config.server_notices_mxid
):
notice_in_room = True
self.assertTrue(notice_in_room, "No server notice in room")
def _trigger_notice_and_join(self):
"""Creates enough active users to hit the MAU limit and trigger a system notice
about it, then joins the system notices room with one of the users created.
Returns:
user_id (str): The ID of the user that joined the room.
tok (str): The access token of the user that joined the room.
room_id (str): The ID of the room that's been joined.
"""
user_id = None
tok = None
invites = []
# Register as many users as the MAU limit allows.
for i in range(self.hs.config.max_mau_value):
localpart = "user%d" % i
user_id = self.register_user(localpart, "password")
tok = self.login(localpart, "password")
# Sync with the user's token to mark the user as active.
request, channel = self.make_request(
"GET", "/sync?timeout=0", access_token=tok,
)
self.render(request)
# Also retrieves the list of invites for this user. We don't care about that
# one except if we're processing the last user, which should have received an
# invite to a room with a server notice about the MAU limit being reached.
# We could also pick another user and sync with it, which would return an
# invite to a system notices room, but it doesn't matter which user we're
# using so we use the last one because it saves us an extra sync.
invites = channel.json_body["rooms"]["invite"]
# Make sure we have an invite to process.
self.assertEqual(len(invites), 1, invites)
# Join the room.
room_id = list(invites.keys())[0]
self.helper.join(room=room_id, user=user_id, tok=tok)
return user_id, tok, room_id