Avoid a M_FORBIDDEN response when a user tries to erase their account and profile updates are disabled (#19398)

Currently synapse returns `M_FORBIDDEN` when trying to use the account
deactivation API, if the server admin disabled displayname changes. This
is undesirable, since it prevents GDPR erasure without admin
interaction. The admin API seems to work fine though. This also only
seems to affect the deactivate API, when the erase flag is true.

Relevant endpoint:
https://spec.matrix.org/latest/client-server-api/#post_matrixclientv3accountdeactivate

This change only removes the checked for condition that the displayname
and profile avatar are allowed to be changed per the configuration
setting. If a user is deleting themselves, why is that denied?

There did not seem to be a basic test for this endpoint that checks the
`erase` usage, so that was added as well as checking the above mentioned
behavior.

Author: jason-famedly

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

@@ -406,28 +406,6 @@ async def delete_profile_upon_deactivation(
             # have it.
             raise AuthError(400, "Cannot remove another user's profile")
 
-        if not by_admin:
-            current_profile = await self.store.get_profileinfo(target_user)
-            if not self.hs.config.registration.enable_set_displayname:
-                if current_profile.display_name:
-                    # SUSPICIOUS: It seems strange to block deactivation on this,
-                    # though this is preserving previous behaviour.
-                    raise SynapseError(
-                        400,
-                        "Changing display name is disabled on this server",
-                        Codes.FORBIDDEN,
-                    )
-
-            if not self.hs.config.registration.enable_set_avatar_url:
-                if current_profile.avatar_url:
-                    # SUSPICIOUS: It seems strange to block deactivation on this,
-                    # though this is preserving previous behaviour.
-                    raise SynapseError(
-                        400,
-                        "Changing avatar is disabled on this server",
-                        Codes.FORBIDDEN,
-                    )
-
         await self.store.delete_profile(target_user)
 
         await self._third_party_rules.on_profile_update(

tests/handlers/test_profile.py

@@ -38,6 +38,7 @@
 from synapse.util.task_scheduler import TaskStatus
 
 from tests import unittest
+from tests.unittest import override_config
 
 
 class ProfileTestCase(unittest.HomeserverTestCase):
@@ -314,9 +315,8 @@ async def potentially_slow_update_membership(
                 membership[state_tuple].content["displayname"], "Frank Jr."
             )
 
+    @override_config({"enable_set_displayname": False})
     def test_set_my_name_if_disabled(self) -> None:
-        self.hs.config.registration.enable_set_displayname = False
-
         # Setting displayname for the first time is allowed
         self.get_success(self.store.set_profile_displayname(self.frank, "Frank"))
 
@@ -435,9 +435,8 @@ def test_set_my_avatar(self) -> None:
             (self.get_success(self.store.get_profile_avatar_url(self.frank))),
         )
 
+    @override_config({"enable_set_avatar_url": False})
     def test_set_my_avatar_if_disabled(self) -> None:
-        self.hs.config.registration.enable_set_avatar_url = False
-
         # 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

@@ -31,14 +31,14 @@
 
 import synapse.rest.admin
 from synapse.api.constants import LoginType, Membership
-from synapse.api.errors import Codes, HttpResponseException
+from synapse.api.errors import Codes, HttpResponseException, SynapseError
 from synapse.appservice import ApplicationService
 from synapse.rest import admin
 from synapse.rest.client import account, login, register, room
 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,123 @@ 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"
+            )
+        )
+        # Verify it is set
+        self.assertEqual(
+            self.get_success(profile_handler.get_displayname(user_id)),
+            "Kermit the Frog",
+        )
+        self.assertEqual(
+            self.get_success(profile_handler.get_avatar_url(user_id)),
+            "http://test/Kermit.jpg",
+        )
+
+        # Deactivate!
+        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', the entire database row is erased. Both of these
+        # should raise a 404(Not Found) SynapseError
+        display_name_failure = self.get_failure(
+            profile_handler.get_displayname(user_id), SynapseError
+        )
+        assert display_name_failure.value.code == HTTPStatus.NOT_FOUND
+
+        avatar_url_failure = self.get_failure(
+            profile_handler.get_avatar_url(user_id), SynapseError
+        )
+        assert avatar_url_failure.value.code == HTTPStatus.NOT_FOUND
+
+        # 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 that deactivating the user with the 'erase' option will remove existing
+        profile data, even with the Synapse configuration to forbid profile changes
+        """
+        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.get_success(
+            store.set_profile_avatar_url(user_id, "http://test/Kermit.jpg")
+        )
+
+        # Verify it is set
+        self.assertEqual(
+            (self.get_success(store.get_profile_displayname(user_id))),
+            "Kermit the Frog",
+        )
+        self.assertEqual(
+            self.get_success(profile_handler.get_displayname(user_id)),
+            "Kermit the Frog",
+        )
+        self.assertEqual(
+            (self.get_success(store.get_profile_avatar_url(user_id))),
+            "http://test/Kermit.jpg",
+        )
+        self.assertEqual(
+            self.get_success(profile_handler.get_avatar_url(user_id)),
+            "http://test/Kermit.jpg",
+        )
+
+        # Deactivate!
+        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', the entire database row is erased. Both of these
+        # should raise a 404(Not Found) SynapseError
+        display_name_failure = self.get_failure(
+            profile_handler.get_displayname(user_id), SynapseError
+        )
+        assert display_name_failure.value.code == HTTPStatus.NOT_FOUND
+
+        avatar_url_failure = self.get_failure(
+            profile_handler.get_avatar_url(user_id), SynapseError
+        )
+        assert avatar_url_failure.value.code == HTTPStatus.NOT_FOUND
+
+        # 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 +815,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