From d6830b63ab52739ec9bbcfd482e5eb3682f21433 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 21 Mar 2022 19:02:56 +0000 Subject: [PATCH 1/5] Test case for the issue at large. --- tests/rest/admin/test_user.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index a60ea0a56305..bef911d5df41 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1050,6 +1050,25 @@ def test_deactivate_user_erase_true(self) -> None: self._is_erased("@user:test", True) + @override_config({"max_avatar_size": 1234}) + def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None: + """Check we can erase a user whose avatar is the empty string. + + Reproduces #12257. + """ + # Patch `self.other_user` to have an empty string as their avatar. + self.get_success(self.store.set_profile_avatar_url("user", "")) + + # Check we can still erase them. + channel = self.make_request( + "POST", + self.url, + access_token=self.admin_user_tok, + content={"erase": True}, + ) + self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body) + self._is_erased("@user:test", True) + def test_deactivate_user_erase_false(self) -> None: """ Test deactivating a user and set `erase` to `false` From 94e8a5f69ec9ec9720805cdbf93030b85e570b16 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 21 Mar 2022 19:06:08 +0000 Subject: [PATCH 2/5] Targetted test case --- tests/handlers/test_profile.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 08733a9f2d42..1ec105c37398 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -267,6 +267,12 @@ def test_avatar_constraints_no_config(self) -> None: ) self.assertTrue(res) + @unittest.override_config({"max_avatar_size": 50}) + def test_avatar_constraints_allow_empty_avatar_url(self) -> None: + """An empty avatar is always permitted.""" + res = self.get_success(self.handler.check_avatar_size_and_mime_type("")) + self.assertTrue(res) + @unittest.override_config({"max_avatar_size": 50}) def test_avatar_constraints_missing(self) -> None: """Tests that an avatar isn't allowed if the file at the given MXC URI couldn't From ba5fd0f1cf00aaa4e441eda3739874cc85015450 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 21 Mar 2022 19:01:01 +0000 Subject: [PATCH 3/5] Don't reject an empty string as an invalid avatar Fixes #12257. --- synapse/handlers/profile.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 6554c0d3c209..239b0aa7447a 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -336,12 +336,18 @@ 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. + If the given `mxc` is empty, no checks are performed. (Users are always able to + unset their avatar.) + Args: mxc: The MXC URI at which the avatar can be found. Returns: A boolean indicating whether the file can be allowed to be set as an avatar. """ + if mxc == "": + return True + if not self.max_avatar_size and not self.allowed_avatar_mimetypes: return True From b0c5365cc44a51cc661cfcafeee288969920f6fb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 21 Mar 2022 19:13:41 +0000 Subject: [PATCH 4/5] Changelog --- changelog.d/12261.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/12261.bugfix diff --git a/changelog.d/12261.bugfix b/changelog.d/12261.bugfix new file mode 100644 index 000000000000..4702f1afa7f1 --- /dev/null +++ b/changelog.d/12261.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.52 where admins could not deactivate and GDPR-eerase a user if Synapse was configured with limits on avatars. From e130f4441f1e439965a692761091d17f54229df4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 25 Mar 2022 13:02:38 +0000 Subject: [PATCH 5/5] Typo fix Co-authored-by: Patrick Cloke --- changelog.d/12261.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/12261.bugfix b/changelog.d/12261.bugfix index 4702f1afa7f1..1bfde4c38055 100644 --- a/changelog.d/12261.bugfix +++ b/changelog.d/12261.bugfix @@ -1 +1 @@ -Fix a bug introduced in Synapse 1.52 where admins could not deactivate and GDPR-eerase a user if Synapse was configured with limits on avatars. +Fix a bug introduced in Synapse 1.52 where admins could not deactivate and GDPR-erase a user if Synapse was configured with limits on avatars.