diff --git a/changelog.d/7199.bugfix b/changelog.d/7199.bugfix
new file mode 100644
index 0000000000..b234163ea8
--- /dev/null
+++ b/changelog.d/7199.bugfix
@@ -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.
diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py
index 9fae2e0afe..ce4a828894 100644
--- a/synapse/server_notices/resource_limits_server_notices.py
+++ b/synapse/server_notices/resource_limits_server_notices.py
@@ -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")
diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index f7432c8d2f..bf0943f265 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -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",
+ )
diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index 0d27b92a86..93eb053b8c 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -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
|
