Add tests

Author: babolivier

synapse/handlers/profile.py

@@ -293,7 +293,7 @@ async def set_avatar_url(
                 400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,)
             )
 
-        await self.check_avatar_size_and_content_type(new_avatar_url)
+        await self.check_avatar_size_and_mime_type(new_avatar_url)
 
         avatar_url_to_set: Optional[str] = new_avatar_url
         if new_avatar_url == "":
@@ -316,7 +316,7 @@ async def set_avatar_url(
 
         await self._update_join_states(requester, target_user)
 
-    async def check_avatar_size_and_content_type(self, mxc: str) -> None:
+    async def check_avatar_size_and_mime_type(self, mxc: str) -> None:
         """Check that the size and content type of the avatar at the given MXC URI are
         within the configured limits.
 
@@ -327,11 +327,11 @@ async def check_avatar_size_and_content_type(self, mxc: str) -> None:
             SynapseError with an M_FORBIDDEN error code if the avatar doesn't fit within
             the limits allowed by the configuration.
         """
-        if not await self._check_avatar_size_and_content_type(mxc):
+        if not await self._check_avatar_size_and_mime_type(mxc):
             raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN)
 
     @cached()
-    async def _check_avatar_size_and_content_type(self, mxc: str) -> bool:
+    async def _check_avatar_size_and_mime_type(self, mxc: str) -> bool:
         """Check that the size and content type of the avatar at the given MXC URI are
         within the configured limits.
 

synapse/handlers/room_member.py

@@ -591,8 +591,8 @@ async def update_membership_locked(
             )
 
         if "avatar_url" in content:
-            await self.profile_handler.check_avatar_size_and_content_type(
-                content["avatar_url"],
+            await self.profile_handler.check_avatar_size_and_mime_type(
+                content["avatar_url"]
             )
 
         # The event content should *not* include the authorising user as

tests/handlers/test_profile.py

@@ -11,12 +11,13 @@
 # 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 Any, Dict
 from unittest.mock import Mock
 
 import synapse.types
 from synapse.api.errors import AuthError, SynapseError
 from synapse.rest import admin
+from synapse.server import HomeServer
 from synapse.types import UserID
 
 from tests import unittest
@@ -46,7 +47,7 @@ def register_query_handler(query_type, handler):
         )
         return hs
 
-    def prepare(self, reactor, clock, hs):
+    def prepare(self, reactor, clock, hs: HomeServer):
         self.store = hs.get_datastore()
 
         self.frank = UserID.from_string("@1234abcd:test")
@@ -248,3 +249,104 @@ def test_set_my_avatar_if_disabled(self):
             ),
             SynapseError,
         )
+
+    def test_avatar_constraints_no_config(self):
+        """Tests that the method to check an avatar against configured constraints skips
+        all of its check if no constraint is configured.
+        """
+        # The first check that's done by this method is whether the file exists; if we
+        # don't get an error on a non-existing file then it means all of the checks were
+        # successfully skipped.
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/unknown_file")
+        )
+        self.assertTrue(allowed)
+
+    @unittest.override_config(
+        {
+            "max_avatar_size": 50,
+        }
+    )
+    def test_avatar_constraints_missing(self):
+        """Tests that an avatar isn't allowed if the file at the given MXC URI couldn't
+        be found.
+        """
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/unknown_file")
+        )
+        self.assertFalse(allowed)
+
+    @unittest.override_config(
+        {
+            "max_avatar_size": 50,
+        }
+    )
+    def test_avatar_constraints_file_size(self):
+        """Tests that a file that's above the allowed file size is forbidden but one
+        that's below it is allowed.
+        """
+        self._setup_local_files(
+            {
+                "small": {"size": 40},
+                "big": {"size": 60},
+            }
+        )
+
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/small")
+        )
+        self.assertTrue(allowed)
+
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/big")
+        )
+        self.assertFalse(allowed)
+
+    @unittest.override_config(
+        {
+            "allowed_avatar_mimetypes": ["image/png"],
+        }
+    )
+    def test_avatar_constraint_mime_type(self):
+        """Tests that a file with an unauthorised MIME type is forbidden but one with
+        an authorised content type is allowed.
+        """
+        self._setup_local_files(
+            {
+                "good": {"mimetype": "image/png"},
+                "bad": {"mimetype": "application/octet-stream"},
+            }
+        )
+
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/good")
+        )
+        self.assertTrue(allowed)
+
+        allowed = self.get_success(
+            self.handler._check_avatar_size_and_mime_type("mxc://test/bad")
+        )
+        self.assertFalse(allowed)
+
+    def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]):
+        """Stores metadata about files in the database.
+
+        Args:
+            names_and_props: A dictionary with one entry per file, with the key being the
+                file's name, and the value being a dictionary of properties. Supported
+                properties are "mimetype" (for the file's type) and "size" (for the
+                file's size).
+        """
+        store = self.hs.get_datastore()
+
+        for name, props in names_and_props.items():
+            self.get_success(
+                store.store_local_media(
+                    media_id=name,
+                    media_type=props.get("mimetype", "image/png"),
+                    time_now_ms=self.clock.time_msec(),
+                    upload_name=None,
+                    media_length=props.get("size", 50),
+                    user_id=UserID.from_string("@rin:test"),
+                )
+            )

tests/rest/client/test_profile.py

@@ -13,8 +13,12 @@
 # limitations under the License.
 
 """Tests REST events for /profile paths."""
+from typing import Any, Dict
+
+from synapse.api.errors import Codes
 from synapse.rest import admin
 from synapse.rest.client import login, profile, room
+from synapse.types import UserID
 
 from tests import unittest
 
@@ -25,6 +29,7 @@ class ProfileTestCase(unittest.HomeserverTestCase):
         admin.register_servlets_for_client_rest_resource,
         login.register_servlets,
         profile.register_servlets,
+        room.register_servlets,
     ]
 
     def make_homeserver(self, reactor, clock):
@@ -150,6 +155,173 @@ def _get_avatar_url(self, name=None):
         self.assertEqual(channel.code, 200, channel.result)
         return channel.json_body.get("avatar_url")
 
+    @unittest.override_config(
+        {
+            "max_avatar_size": 50,
+        }
+    )
+    def test_avatar_size_limit_global(self):
+        """Tests that the maximum size limit for avatars is enforced when updating a
+        global profile.
+        """
+        self._setup_local_files(
+            {
+                "small": {"size": 40},
+                "big": {"size": 60},
+            }
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/profile/%s/avatar_url" % (self.owner,),
+            content={"avatar_url": "mxc://test/big"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 403, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/profile/%s/avatar_url" % (self.owner,),
+            content={"avatar_url": "mxc://test/small"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+    @unittest.override_config(
+        {
+            "max_avatar_size": 50,
+        }
+    )
+    def test_avatar_size_limit_per_room(self):
+        """Tests that the maximum size limit for avatars is enforced when updating a
+        per-room profile.
+        """
+        self._setup_local_files(
+            {
+                "small": {"size": 40},
+                "big": {"size": 60},
+            }
+        )
+
+        room_id = self.helper.create_room_as(tok=self.owner_tok)
+
+        channel = self.make_request(
+            "PUT",
+            "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner),
+            content={"membership": "join", "avatar_url": "mxc://test/big"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 403, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner),
+            content={"membership": "join", "avatar_url": "mxc://test/small"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+    @unittest.override_config(
+        {
+            "allowed_avatar_mimetypes": ["image/png"],
+        }
+    )
+    def test_avatar_allowed_mime_type_global(self):
+        """Tests that the MIME type whitelist for avatars is enforced when updating a
+        global profile.
+        """
+        self._setup_local_files(
+            {
+                "good": {"mimetype": "image/png"},
+                "bad": {"mimetype": "application/octet-stream"},
+            }
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/profile/%s/avatar_url" % (self.owner,),
+            content={"avatar_url": "mxc://test/bad"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 403, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/profile/%s/avatar_url" % (self.owner,),
+            content={"avatar_url": "mxc://test/good"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+    @unittest.override_config(
+        {
+            "allowed_avatar_mimetypes": ["image/png"],
+        }
+    )
+    def test_avatar_allowed_mime_type_per_room(self):
+        """Tests that the MIME type whitelist for avatars is enforced when updating a
+        per-room profile.
+        """
+        self._setup_local_files(
+            {
+                "good": {"mimetype": "image/png"},
+                "bad": {"mimetype": "application/octet-stream"},
+            }
+        )
+
+        room_id = self.helper.create_room_as(tok=self.owner_tok)
+
+        channel = self.make_request(
+            "PUT",
+            "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner),
+            content={"membership": "join", "avatar_url": "mxc://test/bad"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 403, channel.result)
+        self.assertEqual(
+            channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body
+        )
+
+        channel = self.make_request(
+            "PUT",
+            "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner),
+            content={"membership": "join", "avatar_url": "mxc://test/good"},
+            access_token=self.owner_tok,
+        )
+        self.assertEqual(channel.code, 200, channel.result)
+
+    def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]):
+        """Stores metadata about files in the database.
+
+        Args:
+            names_and_props: A dictionary with one entry per file, with the key being the
+                file's name, and the value being a dictionary of properties. Supported
+                properties are "mimetype" (for the file's type) and "size" (for the
+                file's size).
+        """
+        store = self.hs.get_datastore()
+
+        for name, props in names_and_props.items():
+            self.get_success(
+                store.store_local_media(
+                    media_id=name,
+                    media_type=props.get("mimetype", "image/png"),
+                    time_now_ms=self.clock.time_msec(),
+                    upload_name=None,
+                    media_length=props.get("size", 50),
+                    user_id=UserID.from_string("@rin:test"),
+                )
+            )
+
 
 class ProfilesRestrictedTestCase(unittest.HomeserverTestCase):