fix(MSC3911): Handle profile avatars that are unknown gracefully

Author: jason-famedly

synapse/handlers/profile.py

@@ -327,8 +327,22 @@ async def validate_avatar_url(self, avatar_url: str, requester: Requester) -> No
             media_info = await self.hs.get_datastores().main.get_cached_remote_media(
                 mxc_uri.server_name, mxc_uri.media_id
             )
+            # When our local user has attempted to set a profile avatar to a remote
+            # piece of media, but the local server has not actually seen it, only
+            # complain if unrestricted media is disabled, otherwise just allow it. Later
+            # when/if the new profile data is propagated to each room's membership
+            # event, it will either be copied/passed along/dropped depending on the
+            # above circumstances
             if not media_info:
-                # There is no data on this, not much can be done until it comes along
+                if self.disable_unrestricted_media:
+                    # The user should have done a COPY on this media previous to this
+                    # attempt to set
+                    raise SynapseError(
+                        HTTPStatus.NOT_FOUND,
+                        "Profile request to update avatar to remote media can not proceed, a /copy request should have happened first",
+                        errcode=Codes.NOT_FOUND,
+                    )
+                # For backwards compatible behavior, treat the media as unrestricted
                 return
 
         if self.disable_unrestricted_media and not media_info.restricted:

synapse/handlers/room_member.py

@@ -110,6 +110,9 @@ def __init__(self, hs: "HomeServer"):
         self.event_auth_handler = hs.get_event_auth_handler()
         self._worker_lock_handler = hs.get_worker_locks_handler()
         self.enable_restricted_media = hs.config.experimental.msc3911_enabled
+        self.allow_legacy_media = (
+            not hs.config.experimental.msc3911_unrestricted_media_upload_disabled
+        )
 
         self._membership_types_to_include_profile_data_in = {
             Membership.JOIN,
@@ -863,21 +866,83 @@ async def update_membership_locked(
                 logger.info("Failed to get profile information for %r: %s", target, e)
 
             if self.enable_restricted_media and not media_info_for_attachment:
-                # Other than membership
-                avatar_url = content.get("avatar_url")
+                # This code path should only be taken for memberships updating an
+                # avatar url
+                avatar_url = content.get(EventContentFields.MEMBERSHIP_AVATAR_URL)
                 if avatar_url:
                     # Something about the MediaRepository does not like being part of
                     # the initialization code of the RoomMemberHandler, so just import
                     # it on the spot instead.
                     media_repo = self.hs.get_media_repository()
 
-                    new_mxc_uri = await media_repo.copy_media(
-                        MXCUri.from_str(avatar_url), requester.user, 20_000
-                    )
-                    media_object = await media_repo.get_media_info(new_mxc_uri)
-                    assert isinstance(media_object, LocalMedia)
-                    media_info_for_attachment = {media_object}
-                    content[EventContentFields.MEMBERSHIP_AVATAR_URL] = str(new_mxc_uri)
+                    try:
+                        # Run a preflight that this media exists. The http replication
+                        # call should not be a thundering herd of guaranteed http fails.
+                        existing_mxc_uri = MXCUri.from_str(avatar_url)
+                        # The actual info is irrelevant at this stage, just ignore. This
+                        # will raise if the media does not exist
+                        _ = await media_repo.get_media_info(existing_mxc_uri)
+
+                        new_mxc_uri = await media_repo.copy_media(
+                            existing_mxc_uri, requester.user, 20_000
+                        )
+                    except SynapseError:
+                        # The only kind of media copying that should fail is from a
+                        # remote item that has not been seen and cached previously. This
+                        # should already be guarded from in the only circumstances we
+                        # could think of: setting a profile avatar.
+                        #
+                        # If legacy unrestricted media is allowed, this can still be
+                        # triggered. If this happens, ignore it. This allows the avatar
+                        # url to be faithfully set to the given url. All we can do is
+                        # hope that it is not restricted when it finally shows up.
+                        # Guard against other potentials escaping by raising if it
+                        # should occur when unrestricted media begins to be disallowed.
+                        if self.allow_legacy_media:
+                            logger.debug(
+                                "Ignoring media copy request; the media is unknown and "
+                                "will not be treated as restricted"
+                            )
+
+                        else:
+                            # Don't fail the new membership event just because the media
+                            # was not found. Specifically, for:
+                            # * Outgoing remote invites
+                            # * Joins in the context of a new room
+                            # Just remove it, and let the client deal with the lack of
+                            # hint
+                            if effective_membership_state == Membership.INVITE or (
+                                effective_membership_state == Membership.JOIN
+                                and new_room
+                            ):
+                                logger.warning(
+                                    "Unknown media (%s) can not be restricted to "
+                                    "membership event for '%s', dropping avatar from "
+                                    "event",
+                                    avatar_url,
+                                    target,
+                                )
+                                del content[EventContentFields.MEMBERSHIP_AVATAR_URL]
+
+                            else:
+                                # All other types of membership should raise. For
+                                # whatever reason, the media is missing so this should
+                                # not continue
+                                logger.warning(
+                                    "Unknown media (%s) can not be restricted to "
+                                    "membership event for '%s'",
+                                    avatar_url,
+                                    target,
+                                )
+                                raise
+
+                    else:
+                        media_object = await media_repo.get_media_info(new_mxc_uri)
+                        assert isinstance(media_object, LocalMedia)
+                        media_info_for_attachment = {media_object}
+                        content[EventContentFields.MEMBERSHIP_AVATAR_URL] = str(
+                            new_mxc_uri
+                        )
 
         # if this is a join with a 3pid signature, we may need to turn a 3pid
         # invite into a normal invite before we can handle the join.

synapse/media/media_repository.py

@@ -153,9 +153,13 @@ async def get_media_info(self, mxc_uri: MXCUri) -> Union[LocalMedia, RemoteMedia
                 mxc_uri.server_name, mxc_uri.media_id
             )
         if not media_info:
-            raise SynapseError(404, "Media not found", errcode="M_NOT_FOUND")
+            raise SynapseError(
+                HTTPStatus.NOT_FOUND, "Media not found", errcode=Codes.NOT_FOUND
+            )
         if media_info.quarantined_by:
-            raise SynapseError(404, "Media not found", errcode="M_NOT_FOUND")
+            raise SynapseError(
+                HTTPStatus.NOT_FOUND, "Media not found", errcode=Codes.NOT_FOUND
+            )
         return media_info
 
     async def create_or_update_content(

tests/rest/client/test_profile.py

@@ -1015,7 +1015,7 @@ def test_can_attach_media_to_profile_update(self) -> None:
         assert restrictions.event_id is None
         assert restrictions.profile_user_id == UserID.from_string(self.user)
 
-    def test_attaching_nonexistent_media_to_profile_fails(self) -> None:
+    def test_attaching_nonexistent_local_media_to_profile_fails(self) -> None:
         """
         Test that media that does not exist is not allowed to be attached to a user profile.
         """
@@ -1032,6 +1032,42 @@ def test_attaching_nonexistent_media_to_profile_fails(self) -> None:
         assert channel.json_body["errcode"] == Codes.INVALID_PARAM
         assert "does not exist" in channel.json_body["error"]
 
+    def test_attaching_unreachable_remote_media_to_profile_might_succeed(self) -> None:
+        """
+        Test that media that can not be retrieved can be attached to a user profile, if
+        legacy unrestricted media is allowed.
+        """
+        # Generate non-existing media.
+        nonexistent_mxc_uri = MXCUri.from_str("mxc://remote/fakeMediaId")
+        channel = self.make_request(
+            "PUT",
+            f"/_matrix/client/v3/profile/{self.user}/avatar_url",
+            access_token=self.tok,
+            content={"avatar_url": str(nonexistent_mxc_uri)},
+        )
+
+        assert channel.code == HTTPStatus.OK, channel.json_body
+
+    @override_config(
+        {"experimental_features": {"msc3911_unrestricted_media_upload_disabled": True}}
+    )
+    def test_attaching_unreachable_remote_media_to_profile_fails(self) -> None:
+        """
+        Test that media that can not be retrieved will fail to be attached to a user
+        profile when legacy unrestricted media is disabled.
+        """
+        # Generate non-existing media.
+        nonexistent_mxc_uri = MXCUri.from_str("mxc://remote/fakeMediaId_2")
+        channel = self.make_request(
+            "PUT",
+            f"/_matrix/client/v3/profile/{self.user}/avatar_url",
+            access_token=self.tok,
+            content={"avatar_url": str(nonexistent_mxc_uri)},
+        )
+
+        assert channel.code == HTTPStatus.NOT_FOUND, channel.json_body
+        assert channel.json_body["errcode"] == Codes.NOT_FOUND
+
     def test_attaching_unrestricted_media_to_profile(self) -> None:
         """
         Test that attaching unrestricted media to user profile also works

tests/rest/client/test_rooms.py

@@ -5269,6 +5269,53 @@ def test_create_room_fails_with_malformed_room_avatar_url(self) -> None:
         room_id = self.create_room_with_avatar(avatar_mxc="junk", expected_code=400)
         assert room_id is None
 
+    @override_config(
+        {"experimental_features": {"msc3911_unrestricted_media_upload_disabled": True}}
+    )
+    def test_create_room_with_missing_profile_avatar_media_succeeds(self) -> None:
+        """
+        Test that a profile avatar that should automatically be included in a room
+        creator's join event does not break the room when the actual media of the avatar
+        is missing.
+        """
+        # First inject a profile avatar url directly into the database. The handler
+        # functions for such can not be used as they do validation, and it would fail as
+        # the media does not actually exist.
+        avatar_mxc_uri = MXCUri.from_str("mxc://fake-domain/whatever")
+        # Make sure to add the restrictions too
+        self.get_success_or_raise(
+            self.hs.get_datastores().main.set_media_restricted_to_user_profile(
+                avatar_mxc_uri.server_name,
+                avatar_mxc_uri.media_id,
+                self.user,
+            )
+        )
+        self.get_success_or_raise(
+            self.store.set_profile_avatar_url(
+                UserID.from_string(self.user), str(avatar_mxc_uri)
+            )
+        )
+
+        # try and create room. This should succeed, but the avatar will have been
+        # stripped from the join event of the creator
+        room_id = self.helper.create_room_as(
+            self.user,
+            tok=self.tok,
+        )
+        assert room_id is not None
+        # Make sure the avatar is not on the event
+        membership_as_set = self.get_success_or_raise(
+            self.store.get_membership_event_ids_for_user(self.user, room_id)
+        )
+        join_event_id = membership_as_set.pop()
+        join_event = self.get_success_or_raise(self.store.get_event(join_event_id))
+        assert join_event.content["membership"] == Membership.JOIN
+        assert "avatar_url" not in join_event.content
+
+        # The display name would have been added, see if that is still there
+        assert "displayname" in join_event.content
+        assert join_event.content["displayname"] == "david"
+
 
 class RoomMemberEventMediaAttachmentTestCase(unittest.HomeserverTestCase):
     servlets = [
@@ -5489,8 +5536,6 @@ def test_invite_with_media_get_copied_and_attached_to_event(self) -> None:
         assert channel.code == 200, channel.result["body"]
 
         # Get the member event of the invite just occurred.
-        # creator_event_ids = self.get_success(self.store.get_membership_event_ids_for_user(self.user, room_id))
-        # assert len(creator_event_ids) == 1
         invitee_event_ids = self.get_success(
             self.store.get_membership_event_ids_for_user(self.other_user, room_id)
         )
@@ -5502,7 +5547,7 @@ def test_invite_with_media_get_copied_and_attached_to_event(self) -> None:
         assert event.type == EventTypes.Member
 
         # Verify that this event a different mxc
-        assert event.content.get(EventContentFields.MEMBERSHIP_DISPLAYNAME) != str(
+        assert event.content.get(EventContentFields.MEMBERSHIP_AVATAR_URL) != str(
             invitee_avatar_mxc_uri
         )