From c07022303ef596fe7f42f6eb7001660a62801715 Mon Sep 17 00:00:00 2001 From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com> Date: Thu, 17 Dec 2020 13:05:39 +0100 Subject: [PATCH] Fix a bug that deactivated users appear in the directory (#8933) Fixes a bug that deactivated users appear in the directory when their profile information was updated. To change profile information of deactivated users is neccesary for example you will remove displayname or avatar. But they should not appear in directory. They are deactivated. Co-authored-by: Erik Johnston --- changelog.d/8933.bugfix | 1 + synapse/handlers/user_directory.py | 8 +++-- tests/handlers/test_user_directory.py | 40 ++++++++++++++++++++- tests/rest/admin/test_user.py | 50 ++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 changelog.d/8933.bugfix diff --git a/changelog.d/8933.bugfix b/changelog.d/8933.bugfix new file mode 100644 index 000000000..295933d6c --- /dev/null +++ b/changelog.d/8933.bugfix @@ -0,0 +1 @@ +Fix a bug where deactivated users appeared in the user directory when their profile information was updated. diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 3d80371f0..7c4eeaaa5 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -113,9 +113,13 @@ class UserDirectoryHandler(StateDeltasHandler): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - is_support = await self.store.is_support_user(user_id) + # Support users are for diagnostics and should not appear in the user directory. - if not is_support: + is_support = await self.store.is_support_user(user_id) + # When change profile information of deactivated user it should not appear in the user directory. + is_deactivated = await self.store.get_user_deactivated_status(user_id) + + if not (is_support or is_deactivated): await self.store.update_profile_in_user_dir( user_id, profile.display_name, profile.avatar_url ) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 1260721db..9c886d671 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -54,6 +54,10 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): user_id=support_user_id, password_hash=None, user_type=UserTypes.SUPPORT ) ) + regular_user_id = "@regular:test" + self.get_success( + self.store.register_user(user_id=regular_user_id, password_hash=None) + ) self.get_success( self.handler.handle_local_profile_change(support_user_id, None) @@ -63,13 +67,47 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): display_name = "display_name" profile_info = ProfileInfo(avatar_url="avatar_url", display_name=display_name) - regular_user_id = "@regular:test" self.get_success( self.handler.handle_local_profile_change(regular_user_id, profile_info) ) profile = self.get_success(self.store.get_user_in_directory(regular_user_id)) self.assertTrue(profile["display_name"] == display_name) + def test_handle_local_profile_change_with_deactivated_user(self): + # create user + r_user_id = "@regular:test" + self.get_success( + self.store.register_user(user_id=r_user_id, password_hash=None) + ) + + # update profile + display_name = "Regular User" + profile_info = ProfileInfo(avatar_url="avatar_url", display_name=display_name) + self.get_success( + self.handler.handle_local_profile_change(r_user_id, profile_info) + ) + + # profile is in directory + profile = self.get_success(self.store.get_user_in_directory(r_user_id)) + self.assertTrue(profile["display_name"] == display_name) + + # deactivate user + self.get_success(self.store.set_user_deactivated_status(r_user_id, True)) + self.get_success(self.handler.handle_user_deactivated(r_user_id)) + + # profile is not in directory + profile = self.get_success(self.store.get_user_in_directory(r_user_id)) + self.assertTrue(profile is None) + + # update profile after deactivation + self.get_success( + self.handler.handle_local_profile_change(r_user_id, profile_info) + ) + + # profile is furthermore not in directory + profile = self.get_success(self.store.get_user_in_directory(r_user_id)) + self.assertTrue(profile is None) + def test_handle_user_deactivated_support_user(self): s_user_id = "@support:test" self.get_success( diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 4f379a5e5..9d6ef0251 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -603,7 +603,7 @@ class UserRestTestCase(unittest.HomeserverTestCase): self.admin_user = self.register_user("admin", "pass", admin=True) self.admin_user_tok = self.login("admin", "pass") - self.other_user = self.register_user("user", "pass") + self.other_user = self.register_user("user", "pass", displayname="User") self.other_user_token = self.login("user", "pass") self.url_other_user = "/_synapse/admin/v2/users/%s" % urllib.parse.quote( self.other_user @@ -1012,6 +1012,54 @@ class UserRestTestCase(unittest.HomeserverTestCase): self.assertEqual("@user:test", channel.json_body["name"]) self.assertEqual(True, channel.json_body["deactivated"]) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) + def test_change_name_deactivate_user_user_directory(self): + """ + Test change profile information of a deactivated user and + check that it does not appear in user directory + """ + + # is in user directory + profile = self.get_success(self.store.get_user_in_directory(self.other_user)) + self.assertTrue(profile["display_name"] == "User") + + # Deactivate user + body = json.dumps({"deactivated": True}) + + request, channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content=body.encode(encoding="utf_8"), + ) + + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(True, channel.json_body["deactivated"]) + + # is not in user directory + profile = self.get_success(self.store.get_user_in_directory(self.other_user)) + self.assertTrue(profile is None) + + # Set new displayname user + body = json.dumps({"displayname": "Foobar"}) + + request, channel = self.make_request( + "PUT", + self.url_other_user, + access_token=self.admin_user_tok, + content=body.encode(encoding="utf_8"), + ) + + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + self.assertEqual("@user:test", channel.json_body["name"]) + self.assertEqual(True, channel.json_body["deactivated"]) + self.assertEqual("Foobar", channel.json_body["displayname"]) + + # is not in user directory + profile = self.get_success(self.store.get_user_in_directory(self.other_user)) + self.assertTrue(profile is None) + def test_reactivate_user(self): """ Test reactivating another user.