From 68e35c83bcec16cb2fd32e92202645aba28eaa76 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Wed, 28 Sep 2022 10:58:04 +0400 Subject: [PATCH 01/17] fix incorrect assumption of host as server_name while checking for avatar size and mime type --- synapse/handlers/profile.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index d8ff5289b56f..4bf9a047a3bc 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -307,7 +307,11 @@ async def check_avatar_size_and_mime_type(self, mxc: str) -> bool: if not self.max_avatar_size and not self.allowed_avatar_mimetypes: return True - server_name, _, media_id = parse_and_validate_mxc_uri(mxc) + host, port, media_id = parse_and_validate_mxc_uri(mxc) + if port is not None: + server_name = host + ":" + str(port) + else: + server_name = host if server_name == self.server_name: media_info = await self.store.get_local_media(media_id) From 1ea8fe03e6eed319b9f7b4785f41f79f5e886d76 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Wed, 28 Sep 2022 11:25:31 +0400 Subject: [PATCH 02/17] add changelog file --- changelog.d/13927.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13927.bugfix diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix new file mode 100644 index 000000000000..e075ea7d7571 --- /dev/null +++ b/changelog.d/13927.bugfix @@ -0,0 +1 @@ +Fixes avatar handling check on homeservers running on non-default ports. Contributed by @ashfame From 6bea75a49cfe64889543ea6baa69779dbbbc2564 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Wed, 28 Sep 2022 11:44:14 +0400 Subject: [PATCH 03/17] added missing period in changelog file --- changelog.d/13927.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix index e075ea7d7571..fcd765e24e71 100644 --- a/changelog.d/13927.bugfix +++ b/changelog.d/13927.bugfix @@ -1 +1 @@ -Fixes avatar handling check on homeservers running on non-default ports. Contributed by @ashfame +Fixes avatar handling check on homeservers running on non-default ports. Contributed by @ashfame. From cd10ee6e722e8c62bd3a1dc401f2714ec66c1ee8 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Mon, 3 Oct 2022 16:19:56 +0400 Subject: [PATCH 04/17] better changelog description --- changelog.d/13927.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix index fcd765e24e71..1387ca3cd018 100644 --- a/changelog.d/13927.bugfix +++ b/changelog.d/13927.bugfix @@ -1 +1 @@ -Fixes avatar handling check on homeservers running on non-default ports. Contributed by @ashfame. +Fixes check_avatar_size_and_mime_type() to successfully update avatars on homeservers running on non-default ports which were mistakenly treated as remote homeserver while validating the avatar's size and mime type. Contributed by @ashfame. From 9aeaa2cf4247fa8cbe349b61a516452c3572c0c6 Mon Sep 17 00:00:00 2001 From: Ashish Kumar <ashfame@users.noreply.github.com> Date: Mon, 3 Oct 2022 16:34:43 +0400 Subject: [PATCH 05/17] Update changelog.d/13927.bugfix Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/13927.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix index 1387ca3cd018..2f4c97f2074c 100644 --- a/changelog.d/13927.bugfix +++ b/changelog.d/13927.bugfix @@ -1 +1 @@ -Fixes check_avatar_size_and_mime_type() to successfully update avatars on homeservers running on non-default ports which were mistakenly treated as remote homeserver while validating the avatar's size and mime type. Contributed by @ashfame. +Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name`. Contributed by @ashfame. From f3d8aa57140b679fc338fa63c9eb4ff5e4979085 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 4 Oct 2022 13:35:15 +0400 Subject: [PATCH 06/17] update changelog to be accurate --- changelog.d/13927.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix index 2f4c97f2074c..71ecda2ee673 100644 --- a/changelog.d/13927.bugfix +++ b/changelog.d/13927.bugfix @@ -1 +1 @@ -Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name`. Contributed by @ashfame. +Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and had avatar's size or image type restrictions configured. Contributed by @ashfame. From 227a8b99ee9468c74187ac80fdd0b81a9cb60ae7 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 4 Oct 2022 14:12:04 +0400 Subject: [PATCH 07/17] add unit test --- tests/handlers/test_profile.py | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index f88c725a42c3..cad022c18641 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -327,6 +327,41 @@ def test_avatar_constraint_mime_type(self) -> None: ) self.assertFalse(res) + @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + def test_avatar_fetching_metadata_right_source(self) -> None: + """Tests that local and remote file are rightly fetched for metadata + when checking for avatar size and mime type""" + remote_server = "test:8080" + store = self.hs.get_datastores().main + + self.get_success(store.store_local_media( + media_id="local", + media_type="image/png", + media_length=50, + time_now_ms=self.clock.time_msec(), + upload_name=None, + user_id=UserID.from_string("@whatever:test"), + )) + self.get_success(store.store_cached_remote_media( + media_id="remote", + media_type="image/png", + media_length=50, + origin=remote_server, + time_now_ms=self.clock.time_msec(), + upload_name=None, + filesystem_id="remote" + )) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/local") + ) + self.assertTrue(res) + + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://" + remote_server + "/remote") + ) + self.assertTrue(res) + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): """Stores metadata about files in the database. From 7e784d4edeb59ae3acecb4adedbbcfa906056443 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 4 Oct 2022 14:24:20 +0400 Subject: [PATCH 08/17] make linter happy --- tests/handlers/test_profile.py | 42 +++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index cad022c18641..5f4b3c5e8bb4 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -334,23 +334,27 @@ def test_avatar_fetching_metadata_right_source(self) -> None: remote_server = "test:8080" store = self.hs.get_datastores().main - self.get_success(store.store_local_media( - media_id="local", - media_type="image/png", - media_length=50, - time_now_ms=self.clock.time_msec(), - upload_name=None, - user_id=UserID.from_string("@whatever:test"), - )) - self.get_success(store.store_cached_remote_media( - media_id="remote", - media_type="image/png", - media_length=50, - origin=remote_server, - time_now_ms=self.clock.time_msec(), - upload_name=None, - filesystem_id="remote" - )) + self.get_success( + store.store_local_media( + media_id="local", + media_type="image/png", + media_length=50, + time_now_ms=self.clock.time_msec(), + upload_name=None, + user_id=UserID.from_string("@whatever:test"), + ) + ) + self.get_success( + store.store_cached_remote_media( + media_id="remote", + media_type="image/png", + media_length=50, + origin=remote_server, + time_now_ms=self.clock.time_msec(), + upload_name=None, + filesystem_id="remote", + ) + ) res = self.get_success( self.handler.check_avatar_size_and_mime_type("mxc://test/local") @@ -358,7 +362,9 @@ def test_avatar_fetching_metadata_right_source(self) -> None: self.assertTrue(res) res = self.get_success( - self.handler.check_avatar_size_and_mime_type("mxc://" + remote_server + "/remote") + self.handler.check_avatar_size_and_mime_type( + "mxc://" + remote_server + "/remote" + ) ) self.assertTrue(res) From 213b651d2465c0ce6dffa836c0dc7be8e145efc2 Mon Sep 17 00:00:00 2001 From: Ashish Kumar <ashfame@users.noreply.github.com> Date: Wed, 12 Oct 2022 17:02:45 +0400 Subject: [PATCH 09/17] Update changelog.d/13927.bugfix Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- changelog.d/13927.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13927.bugfix b/changelog.d/13927.bugfix index 71ecda2ee673..119cd128e7a7 100644 --- a/changelog.d/13927.bugfix +++ b/changelog.d/13927.bugfix @@ -1 +1 @@ -Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and had avatar's size or image type restrictions configured. Contributed by @ashfame. +Fix a bug which prevented setting an avatar on homeservers which have an explicit port in their `server_name` and have `max_avatar_size` and/or `allowed_avatar_mimetypes` configuration. Contributed by @ashfame. From 8009eb6a30d81affc0603aea8696db5129678841 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Wed, 12 Oct 2022 17:40:52 +0400 Subject: [PATCH 10/17] better grouping of state setup and its test --- tests/handlers/test_profile.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 5f4b3c5e8bb4..120234d40b92 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -344,6 +344,11 @@ def test_avatar_fetching_metadata_right_source(self) -> None: user_id=UserID.from_string("@whatever:test"), ) ) + res = self.get_success( + self.handler.check_avatar_size_and_mime_type("mxc://test/local") + ) + self.assertTrue(res) + self.get_success( store.store_cached_remote_media( media_id="remote", @@ -355,12 +360,6 @@ def test_avatar_fetching_metadata_right_source(self) -> None: filesystem_id="remote", ) ) - - res = self.get_success( - self.handler.check_avatar_size_and_mime_type("mxc://test/local") - ) - self.assertTrue(res) - res = self.get_success( self.handler.check_avatar_size_and_mime_type( "mxc://" + remote_server + "/remote" From 260d3ed8cc9c59758ce611e6b3667625a012a6a4 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Wed, 12 Oct 2022 18:05:13 +0400 Subject: [PATCH 11/17] add explanation to unit test's intention --- tests/handlers/test_profile.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 120234d40b92..65ed0c8046a5 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -329,8 +329,11 @@ def test_avatar_constraint_mime_type(self) -> None: @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) def test_avatar_fetching_metadata_right_source(self) -> None: - """Tests that local and remote file are rightly fetched for metadata - when checking for avatar size and mime type""" + """Tests that local and remote files are correctly fetched for metadata + when checking for avatar size and mime type.""" + # This test only concerns itself with correctly figuring out the host and port + # based on mxc url so that metadata can be fetched from the right place i.e. + # get_local_media() for local image & get_cached_remote_media() for remote image remote_server = "test:8080" store = self.hs.get_datastores().main From cc4ca9fe6dcacac8caf235fdc16cf230b0adf377 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Thu, 13 Oct 2022 18:28:08 +0400 Subject: [PATCH 12/17] improved unit test with a lot of explanation --- tests/handlers/test_profile.py | 80 +++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 16 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 65ed0c8046a5..062d7bcb6a10 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -329,17 +329,33 @@ def test_avatar_constraint_mime_type(self) -> None: @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) def test_avatar_fetching_metadata_right_source(self) -> None: - """Tests that local and remote files are correctly fetched for metadata - when checking for avatar size and mime type.""" - # This test only concerns itself with correctly figuring out the host and port - # based on mxc url so that metadata can be fetched from the right place i.e. - # get_local_media() for local image & get_cached_remote_media() for remote image - remote_server = "test:8080" + """Tests that server_name is figured out correctly when checking for + avatar size and mime type, by checking against from where it tries to retrieve + the metadata for image i.e. locally for a file on our homeserver and from cache + for a file on a remote server.""" + # server_name for homeserver under unit tests is "test" + # available under "self.hs.config.server.server_name" + # and let us say remote server's server_name is "remoteserver" + remote_server = "remoteserver" + + # so + # local media's mxc urls would be: + local_mxc_good = "mxc://" + self.hs.config.server.server_name + "/localgood" + local_mxc_bad = "mxc://" + self.hs.config.server.server_name + "/localbad" + + # remote media's mxc urls would be: + remote_mxc_good = "mxc://remoteserver/remotegood" + remote_mxc_bad = "mxc://remoteserver/remotebad" + store = self.hs.get_datastores().main + # store all 4 of these images + # storing media itself for the local ones + # storing in cache for the remote ones + self.get_success( store.store_local_media( - media_id="local", + media_id="localgood", media_type="image/png", media_length=50, time_now_ms=self.clock.time_msec(), @@ -347,14 +363,19 @@ def test_avatar_fetching_metadata_right_source(self) -> None: user_id=UserID.from_string("@whatever:test"), ) ) - res = self.get_success( - self.handler.check_avatar_size_and_mime_type("mxc://test/local") + self.get_success( + store.store_local_media( + media_id="localbad", + media_type="image/jpg", + media_length=50, + time_now_ms=self.clock.time_msec(), + upload_name=None, + user_id=UserID.from_string("@whatever:test"), + ) ) - self.assertTrue(res) - self.get_success( store.store_cached_remote_media( - media_id="remote", + media_id="remotegood", media_type="image/png", media_length=50, origin=remote_server, @@ -363,12 +384,39 @@ def test_avatar_fetching_metadata_right_source(self) -> None: filesystem_id="remote", ) ) - res = self.get_success( - self.handler.check_avatar_size_and_mime_type( - "mxc://" + remote_server + "/remote" + self.get_success( + store.store_cached_remote_media( + media_id="remotebad", + media_type="image/jpg", + media_length=50, + origin=remote_server, + time_now_ms=self.clock.time_msec(), + upload_name=None, + filesystem_id="remote", + ) + ) + + # now check for avatar size & mime type restrictions + self.assertTrue( + self.get_success( + self.handler.check_avatar_size_and_mime_type(local_mxc_good) + ) + ) + self.assertFalse( + self.get_success( + self.handler.check_avatar_size_and_mime_type(local_mxc_bad) + ) + ) + self.assertTrue( + self.get_success( + self.handler.check_avatar_size_and_mime_type(remote_mxc_good) + ) + ) + self.assertFalse( + self.get_success( + self.handler.check_avatar_size_and_mime_type(remote_mxc_bad) ) ) - self.assertTrue(res) def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): """Stores metadata about files in the database. From 3801f2037d849391d67b7c985f3c212ec71c8b8e Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 18 Oct 2022 16:00:33 +0400 Subject: [PATCH 13/17] simplify unit test - by removing good bad mxc urls based on allowed mime type, since we already have a separate test for it - by adding a port to the server_name for this test - changing the user_id associated with upload to be consistent --- tests/handlers/test_profile.py | 66 ++++++++-------------------------- 1 file changed, 15 insertions(+), 51 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 062d7bcb6a10..ed3709f28511 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -327,55 +327,40 @@ def test_avatar_constraint_mime_type(self) -> None: ) self.assertFalse(res) - @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + @unittest.override_config({"server_name":"test:8888", "allowed_avatar_mimetypes": ["image/png"]}) def test_avatar_fetching_metadata_right_source(self) -> None: """Tests that server_name is figured out correctly when checking for avatar size and mime type, by checking against from where it tries to retrieve the metadata for image i.e. locally for a file on our homeserver and from cache for a file on a remote server.""" - # server_name for homeserver under unit tests is "test" + # default server_name for homeserver under unit tests is "test" # available under "self.hs.config.server.server_name" + # so we override it to have a port # and let us say remote server's server_name is "remoteserver" - remote_server = "remoteserver" - - # so - # local media's mxc urls would be: - local_mxc_good = "mxc://" + self.hs.config.server.server_name + "/localgood" - local_mxc_bad = "mxc://" + self.hs.config.server.server_name + "/localbad" + remote_server = "remote" - # remote media's mxc urls would be: - remote_mxc_good = "mxc://remoteserver/remotegood" - remote_mxc_bad = "mxc://remoteserver/remotebad" + # so, mxc urls would be + local_mxc = "mxc://" + self.hs.config.server.server_name + "/local" + remote_mxc = "mxc://" + remote_server + "/remote" store = self.hs.get_datastores().main - # store all 4 of these images - # storing media itself for the local ones - # storing in cache for the remote ones - + # store both of these images + # storing media itself for the local one self.get_success( store.store_local_media( - media_id="localgood", + media_id="local", media_type="image/png", media_length=50, time_now_ms=self.clock.time_msec(), upload_name=None, - user_id=UserID.from_string("@whatever:test"), - ) - ) - self.get_success( - store.store_local_media( - media_id="localbad", - media_type="image/jpg", - media_length=50, - time_now_ms=self.clock.time_msec(), - upload_name=None, - user_id=UserID.from_string("@whatever:test"), + user_id=UserID.from_string("@rin:test"), ) ) + # storing in cache for the remote one self.get_success( store.store_cached_remote_media( - media_id="remotegood", + media_id="remote", media_type="image/png", media_length=50, origin=remote_server, @@ -384,37 +369,16 @@ def test_avatar_fetching_metadata_right_source(self) -> None: filesystem_id="remote", ) ) - self.get_success( - store.store_cached_remote_media( - media_id="remotebad", - media_type="image/jpg", - media_length=50, - origin=remote_server, - time_now_ms=self.clock.time_msec(), - upload_name=None, - filesystem_id="remote", - ) - ) # now check for avatar size & mime type restrictions self.assertTrue( self.get_success( - self.handler.check_avatar_size_and_mime_type(local_mxc_good) - ) - ) - self.assertFalse( - self.get_success( - self.handler.check_avatar_size_and_mime_type(local_mxc_bad) + self.handler.check_avatar_size_and_mime_type(local_mxc) ) ) self.assertTrue( self.get_success( - self.handler.check_avatar_size_and_mime_type(remote_mxc_good) - ) - ) - self.assertFalse( - self.get_success( - self.handler.check_avatar_size_and_mime_type(remote_mxc_bad) + self.handler.check_avatar_size_and_mime_type(remote_mxc) ) ) From 01439b8271892225f0bcac154856e0661f9d9619 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 18 Oct 2022 17:11:08 +0400 Subject: [PATCH 14/17] fix formatting --- tests/handlers/test_profile.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index ed3709f28511..90576c898a11 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -327,7 +327,9 @@ def test_avatar_constraint_mime_type(self) -> None: ) self.assertFalse(res) - @unittest.override_config({"server_name":"test:8888", "allowed_avatar_mimetypes": ["image/png"]}) + @unittest.override_config( + {"server_name":"test:8888", "allowed_avatar_mimetypes": ["image/png"]} + ) def test_avatar_fetching_metadata_right_source(self) -> None: """Tests that server_name is figured out correctly when checking for avatar size and mime type, by checking against from where it tries to retrieve From e27fc69548227e5428c441974f49253f27df1fa3 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 18 Oct 2022 18:37:54 +0400 Subject: [PATCH 15/17] fix formatting, again --- tests/handlers/test_profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 90576c898a11..02a53b519777 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -328,7 +328,7 @@ def test_avatar_constraint_mime_type(self) -> None: self.assertFalse(res) @unittest.override_config( - {"server_name":"test:8888", "allowed_avatar_mimetypes": ["image/png"]} + {"server_name": "test:8888", "allowed_avatar_mimetypes": ["image/png"]} ) def test_avatar_fetching_metadata_right_source(self) -> None: """Tests that server_name is figured out correctly when checking for From 25df404acc0732485093434818f734ccff9c31b2 Mon Sep 17 00:00:00 2001 From: Ashfame <mail@ashfame.com> Date: Tue, 18 Oct 2022 18:49:37 +0400 Subject: [PATCH 16/17] fix more formatting, linter style check --- tests/handlers/test_profile.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 02a53b519777..c386c912f5e4 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -374,14 +374,10 @@ def test_avatar_fetching_metadata_right_source(self) -> None: # now check for avatar size & mime type restrictions self.assertTrue( - self.get_success( - self.handler.check_avatar_size_and_mime_type(local_mxc) - ) + self.get_success(self.handler.check_avatar_size_and_mime_type(local_mxc)) ) self.assertTrue( - self.get_success( - self.handler.check_avatar_size_and_mime_type(remote_mxc) - ) + self.get_success(self.handler.check_avatar_size_and_mime_type(remote_mxc)) ) def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): From 198788351b221e08b76924fa13be2746bb9c7026 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <richard@matrix.org> Date: Wed, 26 Oct 2022 14:40:56 +0100 Subject: [PATCH 17/17] Clean up unit tests --- tests/handlers/test_profile.py | 66 ++++++++++++++++------------------ 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index c386c912f5e4..675aa023acec 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -14,6 +14,8 @@ from typing import Any, Awaitable, Callable, Dict from unittest.mock import Mock +from parameterized import parameterized + from twisted.test.proto_helpers import MemoryReactor import synapse.types @@ -330,52 +332,46 @@ def test_avatar_constraint_mime_type(self) -> None: @unittest.override_config( {"server_name": "test:8888", "allowed_avatar_mimetypes": ["image/png"]} ) - def test_avatar_fetching_metadata_right_source(self) -> None: - """Tests that server_name is figured out correctly when checking for - avatar size and mime type, by checking against from where it tries to retrieve - the metadata for image i.e. locally for a file on our homeserver and from cache - for a file on a remote server.""" - # default server_name for homeserver under unit tests is "test" - # available under "self.hs.config.server.server_name" - # so we override it to have a port - # and let us say remote server's server_name is "remoteserver" - remote_server = "remote" - - # so, mxc urls would be - local_mxc = "mxc://" + self.hs.config.server.server_name + "/local" - remote_mxc = "mxc://" + remote_server + "/remote" + def test_avatar_constraint_on_local_server_with_port(self): + """Test that avatar metadata is correctly fetched when the media is on a local + server and the server has an explicit port. - store = self.hs.get_datastores().main + (This was previously a bug) + """ + local_server_name = self.hs.config.server.server_name + media_id = "local" + local_mxc = f"mxc://{local_server_name}/{media_id}" - # store both of these images - # storing media itself for the local one - self.get_success( - store.store_local_media( - media_id="local", - media_type="image/png", - media_length=50, - time_now_ms=self.clock.time_msec(), - upload_name=None, - user_id=UserID.from_string("@rin:test"), - ) + # mock up the existence of the avatar file + self._setup_local_files({media_id: {"mimetype": "image/png"}}) + + # and now check that check_avatar_size_and_mime_type is happy + self.assertTrue( + self.get_success(self.handler.check_avatar_size_and_mime_type(local_mxc)) ) - # storing in cache for the remote one + + @parameterized.expand([("remote",), ("remote:1234",)]) + @unittest.override_config({"allowed_avatar_mimetypes": ["image/png"]}) + def test_check_avatar_on_remote_server(self, remote_server_name: str) -> None: + """Test that avatar metadata is correctly fetched from a remote server""" + media_id = "remote" + remote_mxc = f"mxc://{remote_server_name}/{media_id}" + + # if the media is remote, check_avatar_size_and_mime_type just checks the + # media cache, so we don't need to instantiate a real remote server. It is + # sufficient to poke an entry into the db. self.get_success( - store.store_cached_remote_media( - media_id="remote", + self.hs.get_datastores().main.store_cached_remote_media( + media_id=media_id, media_type="image/png", media_length=50, - origin=remote_server, + origin=remote_server_name, time_now_ms=self.clock.time_msec(), upload_name=None, - filesystem_id="remote", + filesystem_id="xyz", ) ) - # now check for avatar size & mime type restrictions - self.assertTrue( - self.get_success(self.handler.check_avatar_size_and_mime_type(local_mxc)) - ) self.assertTrue( self.get_success(self.handler.check_avatar_size_and_mime_type(remote_mxc)) )