Update server notices user profile in room if changed

changelog.d/12115.bugfix

@@ -0,0 +1,2 @@
+Fix bug: Server notices user profile (display_name/avatar_url) are not updated when changed after server restart.
+Contributed by Jorge Florian.

synapse/server_notices/resource_limits_server_notices.py

@@ -71,7 +71,10 @@ 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,11 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import TYPE_CHECKING, Optional
+from typing import TYPE_CHECKING, Optional, Tuple
 
 from synapse.api.constants import EventTypes, Membership, RoomCreationPreset
 from synapse.events import EventBase
-from synapse.types import UserID, create_requester
+from synapse.types import Requester, UserID, create_requester
 from synapse.util.caches.descriptors import cached
 
 if TYPE_CHECKING:
@@ -35,6 +35,7 @@ def __init__(self, hs: "HomeServer"):
         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._message_handler = hs.get_message_handler()
         self._is_mine_id = hs.is_mine_id
         self._server_name = hs.hostname
 
@@ -64,14 +65,22 @@ async def send_notice(
             is_state_event: Is the event a state event
             txn_id: The transaction ID.
         """
-        room_id = await self.get_or_create_notice_room_for_user(user_id)
+        (room_id, room_created) = 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 = {
@@ -90,7 +99,9 @@ async def send_notice(
         return event
 
     @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
+    ) -> Tuple[str, bool]:
         """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
@@ -100,7 +111,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
             user_id: complete user id for the user we want a room for
 
         Returns:
-            room id of notice room.
+            room id of notice room and flag indicating weather room was created.
         """
         if self.server_notices_mxid is None:
             raise Exception("Server notices not enabled")
@@ -125,7 +136,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
                     room.room_id,
                     user_id,
                 )
-                return room.room_id
+                return room.room_id, False
 
         # apparently no existing notice room: create a new one
         logger.info("Creating server notices room for %s", user_id)
@@ -164,7 +175,7 @@ async def get_or_create_notice_room_for_user(self, user_id: str) -> str:
         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
+        return room_id, True
 
     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
@@ -194,3 +205,46 @@ async def maybe_invite_user_to_room(self, user_id: str, room_id: str) -> None:
             room_id=room_id,
             action="invite",
         )
+
+    async def _update_notice_user_profile_if_changed(
+        self,
+        requester: Requester,
+        room_id: str,
+        display_name: Optional[str],
+        avatar_url: Optional[str],
+    ) -> None:
+        """
+        Updates notice user profile if it's different from what it's saved in the room.
+
+        Args:
+            requester: The user who is performing the update.
+            room_id: The ID of the server notice room
+            display_name: The displayname of the server notice user
+            avatar_url: The avatar url of the server notice user
+        """
+        logger.info("Checking whether notice user profile has changed", room_id)
+
+        assert self.server_notices_mxid is not None
+
+        notice_user_data_in_room = await self._message_handler.get_room_data(
+            self.server_notices_mxid,
+            room_id,
+            EventTypes.Member,
+            self.server_notices_mxid,
+        )
+
+        assert notice_user_data_in_room is not None
+
+        notice_user_profile_changed = (
+            display_name != notice_user_data_in_room.content.get("displayname")
+            or avatar_url != notice_user_data_in_room.content.get("avatar_url")
+        )
+        if notice_user_profile_changed:
+            logger.info("Updating notice user profile in room %s", room_id)
+            await self._room_member_handler.update_membership(
+                requester=requester,
+                target=UserID.from_string(self.server_notices_mxid),
+                room_id=room_id,
+                action="join",
+                content={"displayname": display_name, "avatar_url": avatar_url},
+            )

tests/server_notices/test_resource_limits_server_notices.py

@@ -75,7 +75,7 @@ def prepare(self, reactor, clock, hs):
         self.user_id = "@user_id:test"
 
         self._rlsn._server_notices_manager.get_or_create_notice_room_for_user = Mock(
-            return_value=defer.succeed("!something:localhost")
+            return_value=defer.succeed(("!something:localhost", False))
         )
         self._rlsn._store.add_tag_to_room = Mock(return_value=defer.succeed(None))
         self._rlsn._store.get_tags_for_room = Mock(return_value=make_awaitable({}))

tests/server_notices/test_server_notices_manager.py

@@ -0,0 +1,195 @@
+# Copyright 2018, 2019 New Vector Ltd
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from typing import cast
+from unittest.mock import Mock
+
+from twisted.internet import defer
+
+from synapse.api.constants import EventTypes, Membership
+from synapse.api.room_versions import RoomVersions
+from synapse.events import FrozenEvent
+from synapse.server_notices.server_notices_manager import ServerNoticesManager
+from synapse.storage.roommember import RoomsForUser
+
+from tests import unittest
+from tests.unittest import override_config
+from tests.utils import default_config
+
+server_notice_config = {
+    "system_mxid_localpart": "server",
+    "system_mxid_display_name": "display name",
+    "system_mxid_avatar_url": "test/url",
+    "room_name": "Server Notices",
+}
+
+
+class TestServiceNoticeManager(unittest.HomeserverTestCase):
+    def default_config(self):
+        config = cast(dict, default_config("test"))
+        config.update({"server_notices": server_notice_config})
+
+        # apply any additional config which was specified via the override_config
+        # decorator.
+        if self._extra_config is not None:
+            config.update(self._extra_config)
+
+        return config
+
+    def prepare(self, reactor, clock, hs):
+        self.server_notices_manager = self.hs.get_server_notices_manager()
+
+        self.server_notices_manager._event_creation_handler.create_and_send_nonmember_event = Mock(
+            return_value=defer.succeed(
+                (FrozenEvent({"event_id": "$notice_event123"}, RoomVersions.V1), 0)
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership = Mock(
+            return_value=defer.succeed(("$updated_membership123", 1))
+        )
+
+        self.user_id = "@user_id:test"
+        self.notice_message = {"content": "a new message from server", type: "m.text"}
+
+    @override_config(
+        {
+            "server_notices": {
+                **server_notice_config,
+                "system_mxid_display_name": "new display name",
+            }
+        }
+    )
+    def test_update_notice_user_name_when_changed(self):
+        """
+        Tests that existing server notices user name in room is updated when is
+        different from the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
+        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
+            "content"
+        ]
+
+        assert call_args_content == {
+            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
+            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
+        }
+
+    @override_config(
+        {
+            "server_notices": {
+                **server_notice_config,
+                "system_mxid_avatar_url": "test/new-url",
+            }
+        }
+    )
+    def test_update_notice_user_avatar_when_changed(self):
+        """
+        Tests that existing server notices user avatar in room is updated when is
+        different from the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_called_once()
+        call_args_content = self.server_notices_manager._room_member_handler.update_membership.call_args.kwargs[
+            "content"
+        ]
+
+        assert call_args_content == {
+            "displayname": self.hs.config.servernotices.server_notices_mxid_display_name,
+            "avatar_url": self.hs.config.servernotices.server_notices_mxid_avatar_url,
+        }
+
+    def test_doesnt_update_notice_user_profile_when_not_changed(self):
+        """
+        Tests that existing server notices profile in room is not updated when is
+        equal to the one in homeserver config.
+        """
+        self._mock_existing_notice_room(
+            self.server_notices_manager, server_notice_config
+        )
+
+        self.get_success(
+            self.server_notices_manager.send_notice(
+                self.user_id, self.notice_message, EventTypes.Message
+            )
+        )
+
+        self.server_notices_manager._room_member_handler.update_membership.assert_not_called()
+
+    def _mock_existing_notice_room(
+        self, server_notices_manager: ServerNoticesManager, server_notice_config: dict
+    ):
+        """
+        Mocks ServerNoticesManager dependencies used for reading info
+        about existing server notice room.
+        """
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._store.get_rooms_for_local_user_where_membership_is = Mock(  # type: ignore
+            return_value=defer.succeed(
+                [
+                    RoomsForUser(
+                        room_id="!something:test",
+                        event_id="$abc123",
+                        membership=Membership.JOIN,
+                        sender="@server:test",
+                        room_version_id="1",
+                        stream_ordering=0,
+                    )
+                ]
+            )
+        )
+
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._store.get_users_in_room = Mock(  # type: ignore
+            return_value=defer.succeed([server_notices_manager.server_notices_mxid])
+        )
+
+        notice_user_profile_in_room = {
+            "displayname": server_notice_config["system_mxid_display_name"],
+            "avatar_url": server_notice_config["system_mxid_avatar_url"],
+        }
+        # Ignored type error: Cannot assign to a method
+        #  https://github.com/python/mypy/issues/2427
+        server_notices_manager._message_handler.get_room_data = Mock(  # type: ignore
+            return_value=defer.succeed(
+                FrozenEvent(
+                    {
+                        "event_id": "$notice_user_state_123",
+                        "content": notice_user_profile_in_room,
+                    },
+                    RoomVersions.V1,
+                )
+            )
+        )