From 7d07dc66fdfe2ad9053e7a8b15f3a08dbdca6e3e Mon Sep 17 00:00:00 2001 From: "Hemanth V. Alluri" Date: Tue, 15 Jan 2019 16:51:14 +0530 Subject: [PATCH] custom profile fields: Fix error handling for admin clearing values. This commit fixes an error in the logic for allowing admins to edit any user's CPF (custom profile field) values. The logic allowing users to edit their own CPF values is however sound. What happens is that of all the CPF types, for "choice fields" as well as "URL" and "date fields", when the value is reset/deleted/cleared by the admin in the Admin UI (organization settings), the frontend would send a null (empty string) value to the backend for that custom profile field (as this is, after all, the new value in this case). This would then triggers the backend validators to return an error message. We fix this by using the method check_remove_custom_profile_field_value, that both code paths (user editing their own CPFs and admin editing a user's CPF) can call. --- zerver/tests/test_users.py | 62 ++++++++++++++++++++++++++++++++++++++ zerver/views/users.py | 13 ++++++-- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 02be08a373..6b0c5ee956 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -395,6 +395,68 @@ class PermissionTest(ZulipTestCase): {'profile_data': ujson.dumps(new_profile_data)}) self.assert_json_error(result, error_msg) + # non-existant field and no data + invalid_profile_data = [{ + 'id': 9001, + 'value': '' + }] + result = self.client_patch('/json/users/{}'.format(cordelia.id), + {'profile_data': ujson.dumps(invalid_profile_data)}) + self.assert_json_error(result, 'Field id 9001 not found.') + + # non-existant field and data + invalid_profile_data = [{ + 'id': 9001, + 'value': 'some data' + }] + result = self.client_patch('/json/users/{}'.format(cordelia.id), + {'profile_data': ujson.dumps(invalid_profile_data)}) + self.assert_json_error(result, 'Field id 9001 not found.') + + # Test for clearing/resetting field values. + empty_profile_data = [] + for field_name in fields: + field = CustomProfileField.objects.get(name=field_name, realm=realm) + value = '' # type: Union[str, None, List[Any]] + if field.field_type == CustomProfileField.USER: + value = [] + empty_profile_data.append({ + 'id': field.id, + 'value': value, + }) + result = self.client_patch('/json/users/{}'.format(cordelia.id), + {'profile_data': ujson.dumps(empty_profile_data)}) + self.assert_json_success(result) + for field_dict in cordelia.profile_data: + self.assertEqual(field_dict['value'], None) + + # Test adding some of the field values after removing all. + hamlet = self.example_user("hamlet") + new_fields = { + 'Phone number': None, + 'Biography': 'A test user', + 'Favorite food': None, + 'Favorite editor': None, + 'Birthday': None, + 'GitHub profile': 'https://github.com/DEF', + 'Mentor': [hamlet.id] + } + new_profile_data = [] + for field_name in fields: + field = CustomProfileField.objects.get(name=field_name, realm=realm) + value = None + if new_fields[field_name]: + value = new_fields[field_name] + new_profile_data.append({ + 'id': field.id, + 'value': value, + }) + result = self.client_patch('/json/users/{}'.format(cordelia.id), + {'profile_data': ujson.dumps(new_profile_data)}) + self.assert_json_success(result) + for field_dict in cordelia.profile_data: + self.assertEqual(field_dict['value'], new_fields[str(field_dict['name'])]) + def test_non_admin_user_cannot_change_profile_data(self) -> None: self.login(self.example_email("cordelia")) hamlet = self.example_user("hamlet") diff --git a/zerver/views/users.py b/zerver/views/users.py index bb824c2a8a..8fe529b80d 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -19,7 +19,7 @@ from zerver.lib.actions import do_change_avatar_fields, do_change_bot_owner, \ do_create_user, do_deactivate_user, do_reactivate_user, do_regenerate_api_key, \ check_change_full_name, notify_created_bot, do_update_outgoing_webhook_service, \ do_update_bot_config_data, check_change_bot_full_name, do_change_is_guest, \ - do_update_user_custom_profile_data + do_update_user_custom_profile_data, check_remove_custom_profile_field_value from zerver.lib.avatar import avatar_url, get_gravatar_url, get_avatar_field from zerver.lib.bot_config import set_bot_config from zerver.lib.exceptions import JsonableError, CannotDeactivateLastUserError @@ -113,8 +113,15 @@ def update_user_backend(request: HttpRequest, user_profile: UserProfile, user_id check_change_full_name(target, full_name, user_profile) if profile_data is not None: - validate_user_custom_profile_data(target.realm.id, profile_data) - do_update_user_custom_profile_data(target, profile_data) + clean_profile_data = [] + for entry in profile_data: + if not entry["value"]: + field_id = entry["id"] + check_remove_custom_profile_field_value(target, field_id) + else: + clean_profile_data.append(entry) + validate_user_custom_profile_data(target.realm.id, clean_profile_data) + do_update_user_custom_profile_data(target, clean_profile_data) return json_success()