address review comments

changelog.d/12115.bugfix

@@ -1,2 +1 @@
-Fix bug: Server notices user profile (display_name/avatar_url) are not updated when changed after server restart.
-Contributed by Jorge Florian.
+Fix a long-standing bug that updating the server notices user profile (display name/avatar URL) in the configuration would not be applied to pre-existing rooms. Contributed by Jorge Florian.

synapse/server_notices/resource_limits_server_notices.py

@@ -71,10 +71,7 @@ async def maybe_send_server_notice_to_user(self, user_id: str) -> None:
             # In practice, not sure we can ever get here
             return
 
-        (
-            room_id,
-            _,
-        ) = await self._server_notices_manager.get_or_create_notice_room_for_user(
+        room_id = await self._server_notices_manager.get_or_create_notice_room_for_user(
             user_id
         )
 

synapse/server_notices/server_notices_manager.py

@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Optional, Tuple
+from typing import TYPE_CHECKING, Optional
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
 from synapse.events import EventBase
@@ -65,22 +65,14 @@ async def send_notice(
             is_state_event: Is the event a state event
             txn_id: The transaction ID.
         """
-        (room_id, room_created) = await self.get_or_create_notice_room_for_user(user_id)
+        room_id = await self.get_or_create_notice_room_for_user(user_id)
         await self.maybe_invite_user_to_room(user_id, room_id)
 
         assert self.server_notices_mxid is not None
         requester = create_requester(
             self.server_notices_mxid, authenticated_entity=self._server_name
         )
 
-        if not room_created:
-            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,
-            )
-
         logger.info("Sending server notice to %s", user_id)
 
         event_dict = {
@@ -99,9 +91,7 @@ async def send_notice(
         return event
 
     @cached()
-    async def get_or_create_notice_room_for_user(
-        self, user_id: str
-    ) -> Tuple[str, bool]:
+    async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
         """Get the room for notices for a given user
 
         If we have not yet created a notice room for this user, create it, but don't
@@ -111,13 +101,17 @@ async def get_or_create_notice_room_for_user(
             user_id: complete user id for the user we want a room for
 
         Returns:
-            room id of notice room and flag indicating weather room was created.
+            room id of notice room.
         """
         if self.server_notices_mxid is None:
             raise Exception("Server notices not enabled")
 
         assert self._is_mine_id(user_id), "Cannot send server notices to remote users"
 
+        requester = create_requester(
+            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]
         )
@@ -136,7 +130,13 @@ async def get_or_create_notice_room_for_user(
                     room.room_id,
                     user_id,
                 )
-                return room.room_id, False
+                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
         logger.info("Creating server notices room for %s", user_id)
@@ -154,9 +154,6 @@ async def get_or_create_notice_room_for_user(
                 "avatar_url": self._config.servernotices.server_notices_mxid_avatar_url,
             }
 
-        requester = create_requester(
-            self.server_notices_mxid, authenticated_entity=self._server_name
-        )
         info, _ = await self._room_creation_handler.create_room(
             requester,
             config={
@@ -175,7 +172,7 @@ async def get_or_create_notice_room_for_user(
         self._notifier.on_new_event("account_data_key", max_id, users=[user_id])
 
         logger.info("Created server notices room %s for %s", room_id, user_id)
-        return room_id, True
+        return room_id
 
     async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None:
         """Invite the given user to the given server room, unless the user has already

tests/rest/admin/test_server_notice.py

@@ -91,7 +91,14 @@ def test_user_does_not_exist(self) -> None:
         self.assertEqual(HTTPStatus.NOT_FOUND, channel.code, msg=channel.json_body)
         self.assertEqual(Codes.NOT_FOUND, channel.json_body["errcode"])
 
-    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "new display name",
+            }
+        }
+    )
     def test_user_is_not_local(self) -> None:
         """
         Tests that a lookup for a user that is not a local returns a HTTPStatus.BAD_REQUEST
@@ -242,7 +249,14 @@ def test_send_server_notice(self) -> None:
         self.assertEqual(messages[1]["content"]["body"], "test msg two")
         self.assertEqual(messages[1]["sender"], "@notices:test")
 
-    @override_config({"server_notices": {"system_mxid_localpart": "notices"}})
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+            }
+        }
+    )
     def test_send_server_notice_leave_room(self) -> None:
         """
         Tests that sending a server notices is successfully.
@@ -415,6 +429,110 @@ def test_send_server_notice_delete_room(self) -> None:
         # second room has new ID
         self.assertNotEqual(first_room_id, second_room_id)
 
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+                "system_mxid_avatar_url": "test/url",
+            }
+        }
+    )
+    def test_update_notice_user_name_when_changed(self) -> None:
+        """
+        Tests that existing server notices user name in room is updated after
+        server notice config changes.
+        """
+        server_notice_request_content = {
+            "user_id": self.other_user,
+            "content": {"msgtype": "m.text", "body": "test msg one"},
+        }
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        # simulates a change in server config after a server restart.
+        new_display_name = "new display name"
+        self.server_notices_manager._config.servernotices.server_notices_mxid_display_name = (
+            new_display_name
+        )
+        self.server_notices_manager.get_or_create_notice_room_for_user.cache.invalidate_all()
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        invited_rooms = self._check_invite_and_join_status(self.other_user, 1, 0)
+        notice_room_id = invited_rooms[0].room_id
+        self.helper.join(
+            room=notice_room_id, user=self.other_user, tok=self.other_user_token
+        )
+
+        member_states = self._sync_and_get_member_state(
+            notice_room_id, self.other_user_token, "@notices:test"
+        )
+
+        self.assertEqual(member_states[0]["content"]["displayname"], new_display_name)
+
+    @override_config(
+        {
+            "server_notices": {
+                "system_mxid_localpart": "notices",
+                "system_mxid_display_name": "display name",
+                "system_mxid_avatar_url": "test/url",
+            }
+        }
+    )
+    def test_update_notice_user_avatar_when_changed(self) -> None:
+        """
+        Tests that existing server notices user avatar in room is updated when is
+        different from the one in homeserver config.
+        """
+        server_notice_request_content = {
+            "user_id": self.other_user,
+            "content": {"msgtype": "m.text", "body": "test msg one"},
+        }
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        # simulates a change in server config after a server restart.
+        new_avatar_url = "test/new-url"
+        self.server_notices_manager._config.servernotices.server_notices_mxid_avatar_url = (
+            new_avatar_url
+        )
+        self.server_notices_manager.get_or_create_notice_room_for_user.cache.invalidate_all()
+
+        self.make_request(
+            "POST",
+            self.url,
+            access_token=self.admin_user_tok,
+            content=server_notice_request_content,
+        )
+
+        invited_rooms = self._check_invite_and_join_status(self.other_user, 1, 0)
+        notice_room_id = invited_rooms[0].room_id
+        self.helper.join(
+            room=notice_room_id, user=self.other_user, tok=self.other_user_token
+        )
+
+        member_states = self._sync_and_get_member_state(
+            notice_room_id, self.other_user_token, "@notices:test"
+        )
+
+        self.assertEqual(member_states[0]["content"]["avatar_url"], new_avatar_url)
+
     def _check_invite_and_join_status(
         self, user_id: str, expected_invites: int, expected_memberships: int
     ) -> List[RoomsForUser]:
@@ -460,3 +578,18 @@ def _sync_and_get_messages(self, room_id: str, token: str) -> List[JsonDict]:
             x for x in room["timeline"]["events"] if x["type"] == "m.room.message"
         ]
         return messages
+
+    def _sync_and_get_member_state(
+        self, room_id: str, token: str, member_id: str
+    ) -> List[JsonDict]:
+        channel = self.make_request(
+            "GET", "/_matrix/client/r0/sync", access_token=token
+        )
+        self.assertEqual(channel.code, HTTPStatus.OK)
+
+        room = channel.json_body["rooms"]["join"][room_id]
+        return [
+            x
+            for x in room["timeline"]["events"]
+            if x["type"] == "m.room.member" and x["state_key"] == member_id
+        ]