diff --git a/changelog.d/19398.bugfix b/changelog.d/19398.bugfix new file mode 100644 index 00000000000..07679b31aec --- /dev/null +++ b/changelog.d/19398.bugfix @@ -0,0 +1 @@ +Allow user requested erasure to succeed even if Synapse has disabled profile changes. Contributed by Famedly. diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 8f16ae6dece..36748b1edc8 100644 --- a/synapse/handlers/profile.py +++ b/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( diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index 7a7f803ebd5..753060b583a 100644 --- a/tests/handlers/test_profile.py +++ b/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") diff --git a/tests/rest/client/test_account.py b/tests/rest/client/test_account.py index ffa96c7840a..5d028152e93 100644 --- a/tests/rest/client/test_account.py +++ b/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