fix: Avoid a M_FORBIDDEN response when a user tries to erase their account and profile updates are disabled

changelog.d/19398.bugfix

@@ -0,0 +1 @@
+Allow user requested erasure to succeed even if Synapse has disabled profile changes. Contributed by Famedly.

synapse/handlers/profile.py

@@ -193,7 +193,12 @@ async def set_displayname(
         if not by_admin and target_user != requester.user:
             raise AuthError(400, "Cannot set another user's displayname")
 
-        if not by_admin and not self.hs.config.registration.enable_set_displayname:
+        # Bypass forbidding the change to displayname if this is a deactivation request
+        # that explicitly removes the displayname. This is probably an erasure. This
+        # helps with GDPR compliance.
+        if (
+            not by_admin and not self.hs.config.registration.enable_set_displayname
+        ) and not (deactivation and new_displayname == ""):
             profile = await self.store.get_profileinfo(target_user)
             if profile.display_name:
                 raise SynapseError(
@@ -297,7 +302,12 @@ async def set_avatar_url(
         if not by_admin and target_user != requester.user:
             raise AuthError(400, "Cannot set another user's avatar_url")
 
-        if not by_admin and not self.hs.config.registration.enable_set_avatar_url:
+        # Bypass forbidding the change to avatar url if this is a deactivation request
+        # that explicitly removes the avatar url. This is probably an erasure. This
+        # helps with GDPR compliance.
+        if (
+            not by_admin and not self.hs.config.registration.enable_set_avatar_url
+        ) and not (deactivation and new_avatar_url == ""):
             profile = await self.store.get_profileinfo(target_user)
             if profile.avatar_url:
                 raise SynapseError(

tests/handlers/test_profile.py

@@ -33,6 +33,7 @@
 from synapse.util.clock import Clock
 
 from tests import unittest
+from tests.unittest import override_config
 
 
 class ProfileTestCase(unittest.HomeserverTestCase):
@@ -113,9 +114,8 @@ def test_set_my_name(self) -> None:
             self.get_success(self.store.get_profile_displayname(self.frank))
         )
 
-    def test_set_my_name_if_disabled(self) -> None:
-        self.hs.config.registration.enable_set_displayname = False
-
+    @override_config({"enable_set_displayname": False})
+    def test_set_displayname_if_disabled(self) -> None:
         # Setting displayname for the first time is allowed
         self.get_success(self.store.set_profile_displayname(self.frank, "Frank"))
 
@@ -234,9 +234,8 @@ def test_set_my_avatar(self) -> None:
             (self.get_success(self.store.get_profile_avatar_url(self.frank))),
         )
 
-    def test_set_my_avatar_if_disabled(self) -> None:
-        self.hs.config.registration.enable_set_avatar_url = False
-
+    @override_config({"enable_set_avatar_url": False})
+    def test_set_avatar_url_if_disabled(self) -> None:
         # Setting displayname for the first time is allowed
         self.get_success(
             self.store.set_profile_avatar_url(self.frank, "http://my.server/me.png")

tests/rest/client/test_account.py

@@ -38,7 +38,7 @@
 from synapse.rest.synapse.client.password_reset import PasswordResetSubmitTokenResource
 from synapse.server import HomeServer
 from synapse.storage._base import db_to_json
-from synapse.types import JsonDict, UserID
+from synapse.types import JsonDict, UserID, create_requester
 from synapse.util.clock import Clock
 
 from tests import unittest
@@ -500,6 +500,97 @@ def test_deactivate_account(self) -> None:
         channel = self.make_request("GET", "account/whoami", access_token=tok)
         self.assertEqual(channel.code, 401)
 
+    def test_deactivate_erase_account(self) -> None:
+        """
+        Test that a user account can be signaled for erasure on the Matrix spec endpoint
+        for client access, `/account/deactivate` and that profile data is erased as part
+        of the process
+        """
+        mxid = self.register_user("kermit", "test")
+        user_id = UserID.from_string(mxid)
+        tok = self.login("kermit", "test")
+
+        profile_handler = self.hs.get_profile_handler()
+
+        # Set some profile data that can be checked for after the user is erased
+        self.get_success(
+            profile_handler.set_displayname(
+                user_id, create_requester(user_id), "Kermit the Frog"
+            )
+        )
+        self.get_success(
+            profile_handler.set_avatar_url(
+                user_id, create_requester(user_id), "http://test/Kermit.jpg"
+            )
+        )
+
+        # Request erasure of the user
+        self.deactivate(mxid, tok, erase=True)
+
+        store = self.hs.get_datastores().main
+
+        # Check that the user has been marked as deactivated.
+        self.assertTrue(self.get_success(store.get_user_deactivated_status(mxid)))
+
+        # On deactivation with 'erase', a displayname and avatar_url are set to an empty
+        # string through the handler, but are turned into `None` for the database
+        display_name = self.get_success(profile_handler.get_displayname(user_id))
+        assert display_name is None, f"{display_name}"
+
+        avatar_url = self.get_success(profile_handler.get_avatar_url(user_id))
+        assert avatar_url is None, f"{avatar_url}"
+
+        # Check that this access token has been invalidated.
+        channel = self.make_request("GET", "account/whoami", access_token=tok)
+        self.assertEqual(channel.code, 401)
+
+    @override_config({"enable_set_displayname": False, "enable_set_avatar_url": False})
+    def test_deactivate_erase_account_with_disabled_profile_changes(self) -> None:
+        """
+        Test the same erasure process as `test_deactivate_erase_account` above, but have
+        the homeserver configuration disable user ability to update profile data
+        """
+        mxid = self.register_user("kermit", "test")
+        user_id = UserID.from_string(mxid)
+        tok = self.login("kermit", "test")
+
+        profile_handler = self.hs.get_profile_handler()
+
+        # Can not use the profile handler to set a display name when it is disabled. Use
+        # the database directly
+        store = self.hs.get_datastores().main
+        self.get_success(store.set_profile_displayname(user_id, "Kermit the Frog"))
+
+        self.assertEqual(
+            (self.get_success(store.get_profile_displayname(user_id))),
+            "Kermit the Frog",
+        )
+        self.get_success(
+            store.set_profile_avatar_url(user_id, "http://test/Kermit.jpg")
+        )
+        self.assertEqual(
+            (self.get_success(store.get_profile_avatar_url(user_id))),
+            "http://test/Kermit.jpg",
+        )
+
+        # Request erasure of the user
+        self.deactivate(mxid, tok, erase=True)
+
+        # Check that the user has been marked as deactivated.
+        self.assertTrue(self.get_success(store.get_user_deactivated_status(mxid)))
+
+        # On deactivation with 'erase', a displayname and avatar_url are set to an empty
+        # string through the handler, but are turned into `None` for the database
+        display_name = self.get_success(profile_handler.get_displayname(user_id))
+        assert display_name is None, f"{display_name}"
+
+        avatar_url = self.get_success(profile_handler.get_avatar_url(user_id))
+        assert avatar_url is None, f"{avatar_url}"
+
+        # Check that this access token has been invalidated.
+        channel = self.make_request("GET", "account/whoami", access_token=tok)
+        self.assertEqual(channel.code, 401)
+
     def test_pending_invites(self) -> None:
         """Tests that deactivating a user rejects every pending invite for them."""
         store = self.hs.get_datastores().main
@@ -698,14 +789,23 @@ def test_background_update_deletes_deactivated_users_server_side_backup_keys(
         )
         self.assertEqual(len(res2), 4)
 
-    def deactivate(self, user_id: str, tok: str) -> None:
+    def deactivate(self, user_id: str, tok: str, erase: bool = False) -> None:
+        """
+        Helper to deactivate a user using the /account/deactivate endpoint, optionally
+        with erasure
+
+        Args:
+            user_id: the string formatted mxid(not a UserID)
+            tok: the user's access token
+            erase: bool of if this should be a full erasure request
+        """
         request_data = {
             "auth": {
                 "type": "m.login.password",
                 "user": user_id,
                 "password": "test",
             },
-            "erase": False,
+            "erase": erase,
         }
         channel = self.make_request(
             "POST", "account/deactivate", request_data, access_token=tok