From 1f7bc45dfffe4a65a283204d87baba9dfa36f47a Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 1 Apr 2020 18:50:38 +0200
Subject: [PATCH 1/8] Dissociate room creation/lookup from invite in server
 notices

This is because the MAU alerting code looks up/creates a room without knowing if it should notify the user.

Also renames `get_notice_room_for_user` into `get_or_create_notice_room_for_user` to clear out confusion over what it does.
---
 .../resource_limits_server_notices.py         |  7 +-
 .../server_notices/server_notices_manager.py  | 84 ++++++++++++++-----
 2 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py
index 9fae2e0afe62..4d23b96a9cbd 100644
--- a/synapse/server_notices/resource_limits_server_notices.py
+++ b/synapse/server_notices/resource_limits_server_notices.py
@@ -80,7 +80,12 @@ def maybe_send_server_notice_to_user(self, user_id):
             # In practice, not sure we can ever get here
             return
 
-        room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id)
+        # Retrieve or create a server notices room for this user. This function is
+        # cached, so if the user hasn't been invited to a room previously created yet,
+        # a new one won't be created.
+        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 f7432c8d2f49..443d8f470cd3 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -13,11 +13,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
+from typing import Optional
 
 from twisted.internet import defer
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
-from synapse.types import create_requester
+from synapse.types import create_requester, UserID
 from synapse.util.caches.descriptors import cachedInlineCallbacks
 
 logger = logging.getLogger(__name__)
@@ -36,10 +37,12 @@ def __init__(self, hs):
         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 +69,8 @@ def send_notice(
         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.invite_user_in_notice_room(user_id, room_id)
 
         system_mxid = self._config.server_notices_mxid
         requester = create_requester(system_mxid)
@@ -89,7 +93,7 @@ def send_notice(
         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
@@ -105,22 +109,9 @@ def get_notice_room_for_user(self, user_id):
 
         assert self._is_mine_id(user_id), "Cannot send server notices to remote users"
 
-        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
-            # 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 = yield self._store.get_users_in_room(room.room_id)
-            if system_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)
-                return room.room_id
+        existing_room_id = yield self._get_existing_notice_room_for_user(user_id)
+        if existing_room_id:
+            return existing_room_id
 
         # apparently no existing notice room: create a new one
         logger.info("Creating server notices room for %s", user_id)
@@ -138,14 +129,13 @@ def get_notice_room_for_user(self, user_id):
                 "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 +149,55 @@ def get_notice_room_for_user(self, user_id):
 
         logger.info("Created server notices room %s for %s", room_id, user_id)
         return room_id
+
+    @defer.inlineCallbacks
+    def _get_existing_notice_room_for_user(self, user_id,):
+        # type: (str) -> defer.Deferred[Optional[str]]
+        """Retrieve the ID for the existing notice room for the given user ID if it
+        exists.
+
+        Returns:
+            The ID of the notice room, None if no room exist for this user.
+        """
+        room_id = None
+
+        rooms = yield 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 = yield self._store.get_users_in_room(room.room_id)
+            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)
+                room_id = room.room_id
+
+        return room_id
+
+    @defer.inlineCallbacks
+    def invite_user_in_notice_room(self, user_id: str, room_id: str):
+        """Invite the given user to the given server notices room.
+
+        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)
+
+        # Make sure a notice room doesn't already exist for this user, otherwise the auth
+        # checks will make the current request (i.e. sync) fail.
+        existing_room_id = yield self._get_existing_notice_room_for_user(user_id)
+        if existing_room_id:
+            return
+
+        yield self._room_member_handler.update_membership(
+            requester=requester,
+            target=UserID.from_string(user_id),
+            room_id=room_id,
+            action="invite",
+        )

From bfd50f136a1f0a1579fc597bf3f26ed9531aba6a Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 1 Apr 2020 18:51:07 +0200
Subject: [PATCH 2/8] Add tests

---
 .../test_resource_limits_server_notices.py    | 123 ++++++++++++++++--
 1 file changed, 111 insertions(+), 12 deletions(-)

diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index 0d27b92a86bb..4520a79c9e3b 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -22,6 +22,9 @@
 from synapse.server_notices.resource_limits_server_notices import (
     ResourceLimitsServerNotices,
 )
+from synapse.rest import admin
+from synapse.rest.client.v1 import login, room
+from synapse.rest.client.v2_alpha import sync
 
 from tests import unittest
 
@@ -67,7 +70,7 @@ def prepare(self, reactor, clock, hs):
         # 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,29 @@ def test_maybe_send_server_notice_when_alerting_suppressed_room_blocked(self):
 
 
 class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
+    servlets = [
+        admin.register_servlets,
+        login.register_servlets,
+        room.register_servlets,
+        sync.register_servlets,
+    ]
+
+    def make_homeserver(self, reactor, clock):
+        hs_config = self.default_config()
+        hs_config["server_notices"] = {
+            "system_mxid_localpart": "server",
+            "system_mxid_display_name": None,
+            "system_mxid_avatar_url": None,
+            "room_name": "Test Server Notice Room",
+        }
+
+        hs_config["limit_usage_by_mau"] = True
+        hs_config["max_mau_value"] = 5
+        hs_config["admin_contact"] = "mailto:user@test.com"
+
+        hs = self.setup_test_homeserver(config=hs_config)
+        return hs
+
     def prepare(self, reactor, clock, hs):
         self.store = self.hs.get_datastore()
         self.server_notices_sender = self.hs.get_server_notices_sender()
@@ -228,18 +254,8 @@ def prepare(self, reactor, clock, hs):
         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 +269,7 @@ def test_server_notice_only_sent_once(self):
         # 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 +289,86 @@ def test_server_notice_only_sent_once(self):
             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

From beeadaa0893c95d0d8cc762059716199678d563c Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 1 Apr 2020 19:07:31 +0200
Subject: [PATCH 3/8] Lint

---
 synapse/server_notices/server_notices_manager.py            | 4 ++--
 tests/server_notices/test_resource_limits_server_notices.py | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index 443d8f470cd3..285522cb1ffb 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -18,7 +18,7 @@
 from twisted.internet import defer
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
-from synapse.types import create_requester, UserID
+from synapse.types import UserID, create_requester
 from synapse.util.caches.descriptors import cachedInlineCallbacks
 
 logger = logging.getLogger(__name__)
@@ -151,7 +151,7 @@ def get_or_create_notice_room_for_user(self, user_id):
         return room_id
 
     @defer.inlineCallbacks
-    def _get_existing_notice_room_for_user(self, user_id,):
+    def _get_existing_notice_room_for_user(self, user_id):
         # type: (str) -> defer.Deferred[Optional[str]]
         """Retrieve the ID for the existing notice room for the given user ID if it
         exists.
diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index 4520a79c9e3b..ce537bcb7a58 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -19,12 +19,12 @@
 
 from synapse.api.constants import EventTypes, LimitBlockingTypes, ServerNoticeMsgType
 from synapse.api.errors import ResourceLimitError
-from synapse.server_notices.resource_limits_server_notices import (
-    ResourceLimitsServerNotices,
-)
 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,
+)
 
 from tests import unittest
 

From 966baa2bf58154240322a9d2c602441a21662340 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 1 Apr 2020 19:08:35 +0200
Subject: [PATCH 4/8] Changelog

---
 changelog.d/7199.bugfix | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/7199.bugfix

diff --git a/changelog.d/7199.bugfix b/changelog.d/7199.bugfix
new file mode 100644
index 000000000000..c0b368d760a3
--- /dev/null
+++ b/changelog.d/7199.bugfix
@@ -0,0 +1 @@
+Fix a bug that could cause a user to be invited in a server notices (aka System Alerts) room without any notice being sent.

From 3da9f9a65367b98854052bd2f4cc752bb6bbd826 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Thu, 2 Apr 2020 19:30:29 +0200
Subject: [PATCH 5/8] Incorporate review

---
 .../resource_limits_server_notices.py         |  3 -
 .../server_notices/server_notices_manager.py  | 75 +++++++++----------
 2 files changed, 34 insertions(+), 44 deletions(-)

diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py
index 4d23b96a9cbd..ce4a828894b8 100644
--- a/synapse/server_notices/resource_limits_server_notices.py
+++ b/synapse/server_notices/resource_limits_server_notices.py
@@ -80,9 +80,6 @@ def maybe_send_server_notice_to_user(self, user_id):
             # In practice, not sure we can ever get here
             return
 
-        # Retrieve or create a server notices room for this user. This function is
-        # cached, so if the user hasn't been invited to a room previously created yet,
-        # a new one won't be created.
         room_id = yield self._server_notices_manager.get_or_create_notice_room_for_user(
             user_id
         )
diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index 285522cb1ffb..a6ae02cf856d 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -70,7 +70,7 @@ def send_notice(
             Deferred[FrozenEvent]
         """
         room_id = yield self.get_or_create_notice_room_for_user(user_id)
-        yield self.invite_user_in_notice_room(user_id, room_id)
+        yield self.invite_user_to_notice_room(user_id, room_id)
 
         system_mxid = self._config.server_notices_mxid
         requester = create_requester(system_mxid)
@@ -96,7 +96,8 @@ def send_notice(
     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
@@ -109,9 +110,25 @@ def get_or_create_notice_room_for_user(self, user_id):
 
         assert self._is_mine_id(user_id), "Cannot send server notices to remote users"
 
-        existing_room_id = yield self._get_existing_notice_room_for_user(user_id)
-        if existing_room_id:
-            return existing_room_id
+        rooms = yield 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 = yield self._store.get_users_in_room(room.room_id)
+            if 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,
+                )
+                return room.room_id
 
         # apparently no existing notice room: create a new one
         logger.info("Creating server notices room for %s", user_id)
@@ -151,37 +168,9 @@ def get_or_create_notice_room_for_user(self, user_id):
         return room_id
 
     @defer.inlineCallbacks
-    def _get_existing_notice_room_for_user(self, user_id):
-        # type: (str) -> defer.Deferred[Optional[str]]
-        """Retrieve the ID for the existing notice room for the given user ID if it
-        exists.
-
-        Returns:
-            The ID of the notice room, None if no room exist for this user.
-        """
-        room_id = None
-
-        rooms = yield 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 = yield self._store.get_users_in_room(room.room_id)
-            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)
-                room_id = room.room_id
-
-        return room_id
-
-    @defer.inlineCallbacks
-    def invite_user_in_notice_room(self, user_id: str, room_id: str):
-        """Invite the given user to the given server notices room.
+    def invite_user_to_notice_room(self, user_id: str, room_id: str):
+        """Invite the given user to the given server notices room, unless the user has
+        already joined or been invited to this room.
 
         Args:
             user_id: The ID of the user to invite.
@@ -189,11 +178,15 @@ def invite_user_in_notice_room(self, user_id: str, room_id: str):
         """
         requester = create_requester(self.server_notices_mxid)
 
-        # Make sure a notice room doesn't already exist for this user, otherwise the auth
-        # checks will make the current request (i.e. sync) fail.
-        existing_room_id = yield self._get_existing_notice_room_for_user(user_id)
-        if existing_room_id:
-            return
+        # Check whether the user has already joined or been invited to this room. If
+        # that's the case, don't invite because otherwise it would make the auth checks
+        # fail.
+        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,

From 7216a7a95fa5ea548fd5d73dfd5965a1c1e177a4 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Thu, 2 Apr 2020 19:42:17 +0200
Subject: [PATCH 6/8] Lint

---
 synapse/server_notices/server_notices_manager.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index a6ae02cf856d..5b3397f7edde 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -13,7 +13,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import Optional
 
 from twisted.internet import defer
 

From f14060a12007c2ade6062c7b57d061f49816fcbc Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Sat, 4 Apr 2020 15:34:17 +0200
Subject: [PATCH 7/8] Update changelog.d/7199.bugfix

Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
---
 changelog.d/7199.bugfix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/changelog.d/7199.bugfix b/changelog.d/7199.bugfix
index c0b368d760a3..b234163ea823 100644
--- a/changelog.d/7199.bugfix
+++ b/changelog.d/7199.bugfix
@@ -1 +1 @@
-Fix a bug that could cause a user to be invited in a server notices (aka System Alerts) room without any notice being sent.
+Fix a bug that could cause a user to be invited to a server notices (aka System Alerts) room without any notice being sent.

From 421324bb65954c71da3fdc65f3f98ac6f67c95e1 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Sat, 4 Apr 2020 15:34:45 +0200
Subject: [PATCH 8/8] Incorporate review

---
 .../server_notices/server_notices_manager.py    | 11 +++++------
 .../test_resource_limits_server_notices.py      | 17 +++++++----------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py
index 5b3397f7edde..bf0943f265d0 100644
--- a/synapse/server_notices/server_notices_manager.py
+++ b/synapse/server_notices/server_notices_manager.py
@@ -69,7 +69,7 @@ def send_notice(
             Deferred[FrozenEvent]
         """
         room_id = yield self.get_or_create_notice_room_for_user(user_id)
-        yield self.invite_user_to_notice_room(user_id, room_id)
+        yield self.maybe_invite_user_to_room(user_id, room_id)
 
         system_mxid = self._config.server_notices_mxid
         requester = create_requester(system_mxid)
@@ -167,9 +167,9 @@ def get_or_create_notice_room_for_user(self, user_id):
         return room_id
 
     @defer.inlineCallbacks
-    def invite_user_to_notice_room(self, user_id: str, room_id: str):
-        """Invite the given user to the given server notices room, unless the user has
-        already joined or been invited to this room.
+    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.
@@ -178,8 +178,7 @@ def invite_user_to_notice_room(self, user_id: str, room_id: str):
         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, don't invite because otherwise it would make the auth checks
-        # fail.
+        # 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]
         )
diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py
index ce537bcb7a58..93eb053b8c05 100644
--- a/tests/server_notices/test_resource_limits_server_notices.py
+++ b/tests/server_notices/test_resource_limits_server_notices.py
@@ -225,21 +225,18 @@ class TestResourceLimitsServerNoticesWithRealRooms(unittest.HomeserverTestCase):
         sync.register_servlets,
     ]
 
-    def make_homeserver(self, reactor, clock):
-        hs_config = self.default_config()
-        hs_config["server_notices"] = {
+    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",
         }
-
-        hs_config["limit_usage_by_mau"] = True
-        hs_config["max_mau_value"] = 5
-        hs_config["admin_contact"] = "mailto:user@test.com"
-
-        hs = self.setup_test_homeserver(config=hs_config)
-        return hs
+        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()