From 2de602f3a5f930f0987ae6bb675931ebaf685b7a Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 20 Nov 2024 13:06:58 -0800 Subject: [PATCH 01/45] create class to consolidate validation of web user invitation fields --- corehq/apps/registration/validation.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 corehq/apps/registration/validation.py diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py new file mode 100644 index 000000000000..3a3a10010899 --- /dev/null +++ b/corehq/apps/registration/validation.py @@ -0,0 +1,20 @@ +from memoized import memoized + +from corehq.apps.user_importer.validation import RoleValidator + + +class AdminInvitesUserValidator(): + def __init__(self, domain, upload_user): + self.domain = domain + self.upload_user = upload_user + + @property + @memoized + def roles_by_name(self): + from corehq.apps.users.views.utils import get_editable_role_choices + return {role[1]: role[0] for role in get_editable_role_choices(self.domain, self.upload_user, + allow_admin_role=True)} + + def validate_role(self, role): + spec = {'role': role} + return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) From 1f42d9591ccea3d72ec83216016236da38dfcb52 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 21 Nov 2024 13:06:20 -0800 Subject: [PATCH 02/45] this validation is already done as part of django form from CustomDataEditor Form --- corehq/apps/registration/forms.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 0caeffc1cd8c..04e4e7572858 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -589,13 +589,6 @@ def __init__(self, data=None, excluded_emails=None, is_add_user=None, ), ) - def _validate_profile(self, profile_id): - valid_profile_ids = {choice[0] for choice in self.custom_data.form.fields[PROFILE_SLUG].widget.choices} - if profile_id and profile_id not in valid_profile_ids: - raise forms.ValidationError( - _('Invalid profile selected. Please select a valid profile.'), - ) - def clean_email(self): email = self.cleaned_data['email'].strip() if email.lower() in self.excluded_emails: @@ -631,7 +624,6 @@ def clean(self): if prefixed_profile_key in custom_user_data: profile_id = custom_user_data.pop(prefixed_profile_key) - self._validate_profile(profile_id) cleaned_data['profile'] = profile_id cleaned_data['custom_user_data'] = get_prefixed(custom_user_data, self.custom_data.prefix) From b4c260c96c106ec3fb4b7ca7ec87a688c41243f6 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 21 Nov 2024 16:01:12 -0800 Subject: [PATCH 03/45] refactor for readability --- corehq/apps/user_importer/validation.py | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 8b411c8b8ef1..5f2d030ccf3e 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -342,13 +342,16 @@ def validate_spec(self, spec): if spec_profile_name and spec_profile_name not in self.all_user_profile_ids_by_name.keys(): return self.error_message_nonexisting_profile.format(spec_profile_name) - user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) - original_profile_id = None - if user_result.invitation: - original_profile_id = user_result.invitation.profile.id if user_result.invitation.profile else None - elif user_result.editable_user: - original_profile_id = user_result.editable_user.get_user_data(self.domain).profile_id + profile_assignment_required_error = self._validate_profile_assignment_required(spec_profile_name) + if profile_assignment_required_error: + return profile_assignment_required_error + original_profile_id = self._get_original_profile_id(spec) + profile_access_error = self._validate_profile_access(original_profile_id, spec_profile_name) + if profile_access_error: + return profile_access_error + + def _validate_profile_assignment_required(self, spec_profile_name): profile_required_for_user_type_list = CustomDataFieldsDefinition.get_profile_required_for_user_type_list( self.domain, UserFieldsView.field_type @@ -362,6 +365,15 @@ def validate_spec(self, spec): [self.user_types[required_for] for required_for in profile_required_for_user_type_list] )) + def _get_original_profile_id(self, spec): + user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) + if user_result.invitation: + return user_result.invitation.profile.id if user_result.invitation.profile else None + elif user_result.editable_user: + return user_result.editable_user.get_user_data(self.domain).profile_id + return None + + def _validate_profile_access(self, original_profile_id, spec_profile_name): spec_profile_id = self.all_user_profile_ids_by_name.get(spec_profile_name) spec_profile_same_as_original = original_profile_id == spec_profile_id if spec_profile_same_as_original: @@ -370,8 +382,10 @@ def validate_spec(self, spec): upload_user_accessible_profiles = ( UserFieldsView.get_user_accessible_profiles(self.domain, self.upload_user)) accessible_profile_ids = {p.id for p in upload_user_accessible_profiles} + if original_profile_id and original_profile_id not in accessible_profile_ids: return self.error_message_original_user_profile_access + if spec_profile_id and spec_profile_id not in accessible_profile_ids: return self.error_message_new_user_profile_access.format(spec_profile_name) From 2873fcb29a2ff7551ebbfa0a004ba2734ba40e49 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 21 Nov 2024 16:19:29 -0800 Subject: [PATCH 04/45] adds function to validate profile --- corehq/apps/registration/validation.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 3a3a10010899..60a690799e18 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -1,6 +1,9 @@ from memoized import memoized from corehq.apps.user_importer.validation import RoleValidator +from corehq.apps.user_importer.validation import ProfileValidator + +from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition class AdminInvitesUserValidator(): @@ -15,6 +18,25 @@ def roles_by_name(self): return {role[1]: role[0] for role in get_editable_role_choices(self.domain, self.upload_user, allow_admin_role=True)} + @property + @memoized + def profiles_by_name(self): + from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView + definition = CustomDataFieldsDefinition.get(self.domain, UserFieldsView.field_type) + if definition: + profiles = definition.get_profiles() + return { + profile.name: profile + for profile in profiles + } + else: + return {} + def validate_role(self, role): spec = {'role': role} return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) + + def validate_profile(self, new_profile_name): + profile_validator = ProfileValidator(self.domain, self.upload_user, True, self.profiles_by_name()) + spec = {'user_profile': new_profile_name} + return profile_validator.validate_spec(spec) From 06e60e149adcd34d026f0542902fd7b5f9862bba Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 21 Nov 2024 17:46:13 -0800 Subject: [PATCH 05/45] refactor validation of email to consolidate --- corehq/apps/registration/forms.py | 17 ++++++-------- corehq/apps/registration/tests/test_forms.py | 1 - corehq/apps/registration/validation.py | 24 ++++++++++++++++++-- corehq/apps/users/views/__init__.py | 3 --- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 04e4e7572858..6ddfd313606a 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -24,7 +24,7 @@ from corehq.apps.programs.models import Program from corehq.toggles import WEB_USER_INVITE_ADDITIONAL_FIELDS from corehq.apps.users.forms import SelectUserLocationForm, BaseTableauUserForm -from corehq.apps.users.models import CouchUser, WebUser +from corehq.apps.users.models import CouchUser class RegisterWebUserForm(forms.Form): @@ -492,7 +492,7 @@ class AdminInvitesUserForm(SelectUserLocationForm): max_length=User._meta.get_field('email').max_length) role = forms.ChoiceField(choices=(), label="Project Role") - def __init__(self, data=None, excluded_emails=None, is_add_user=None, + def __init__(self, data=None, is_add_user=None, role_choices=(), should_show_location=False, can_edit_tableau_config=False, custom_data=None, *, domain, **kwargs): self.custom_data = custom_data @@ -501,6 +501,8 @@ def __init__(self, data=None, excluded_emails=None, is_add_user=None, custom_data_post_dict = self.custom_data.form.data data.update({k: v for k, v in custom_data_post_dict.items() if k not in data}) self.request = kwargs.get('request') + from corehq.apps.registration.validation import AdminInvitesUserValidator + self._validator = AdminInvitesUserValidator(domain, self.request.couch_user) super(AdminInvitesUserForm, self).__init__(domain=domain, data=data, **kwargs) self.can_edit_tableau_config = can_edit_tableau_config domain_obj = Domain.get_by_name(domain) @@ -520,8 +522,6 @@ def __init__(self, data=None, excluded_emails=None, is_add_user=None, choices = [('', '')] + list((prog.get_id, prog.name) for prog in programs) self.fields['program'].choices = choices - self.excluded_emails = [x.lower() for x in excluded_emails] if excluded_emails else [] - if self.can_edit_tableau_config: self._initialize_tableau_fields(data, domain) @@ -591,12 +591,9 @@ def __init__(self, data=None, excluded_emails=None, is_add_user=None, def clean_email(self): email = self.cleaned_data['email'].strip() - if email.lower() in self.excluded_emails: - raise forms.ValidationError(_("A user with this email address is already in " - "this project or has a pending invitation.")) - web_user = WebUser.get_by_username(email) - if web_user and not web_user.is_active: - raise forms.ValidationError(_("A user with this email address is deactivated. ")) + errors = self._validator.validate_email(email, self.request.method == 'POST') + if errors: + raise forms.ValidationError(errors) return email def clean(self): diff --git a/corehq/apps/registration/tests/test_forms.py b/corehq/apps/registration/tests/test_forms.py index 11c2e0025640..f4dc896a2beb 100644 --- a/corehq/apps/registration/tests/test_forms.py +++ b/corehq/apps/registration/tests/test_forms.py @@ -60,7 +60,6 @@ def create_form(data=None, **kw): defaults = { "domain": "test", "request": request, - "excluded_emails": [], "role_choices": [("admin", "admin")], } return AdminInvitesUserForm(request.POST, **(defaults | kw)) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 60a690799e18..2182b8b0a343 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -1,9 +1,13 @@ from memoized import memoized -from corehq.apps.user_importer.validation import RoleValidator -from corehq.apps.user_importer.validation import ProfileValidator +from django.utils.translation import gettext_lazy as _ from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition +from corehq.apps.user_importer.validation import ( + RoleValidator, + ProfileValidator, +) +from corehq.apps.users.models import Invitation, WebUser class AdminInvitesUserValidator(): @@ -32,6 +36,13 @@ def profiles_by_name(self): else: return {} + @property + @memoized + def current_users_and_pending_invites(self): + current_users = [user.username.lower() for user in WebUser.by_domain(self.domain)] + pending_invites = [di.email.lower() for di in Invitation.by_domain(self.domain)] + return current_users + pending_invites + def validate_role(self, role): spec = {'role': role} return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) @@ -40,3 +51,12 @@ def validate_profile(self, new_profile_name): profile_validator = ProfileValidator(self.domain, self.upload_user, True, self.profiles_by_name()) spec = {'user_profile': new_profile_name} return profile_validator.validate_spec(spec) + + def validate_email(self, email, is_post): + if is_post: + if email.lower() in self.current_users_and_pending_invites: + return _("A user with this email address is already in " + "this project or has a pending invitation.") + web_user = WebUser.get_by_username(email) + if web_user and not web_user.is_active: + return _("A user with this email address is deactivated. ") diff --git a/corehq/apps/users/views/__init__.py b/corehq/apps/users/views/__init__.py index c76e8fd404dd..5139e88db203 100644 --- a/corehq/apps/users/views/__init__.py +++ b/corehq/apps/users/views/__init__.py @@ -1146,11 +1146,8 @@ def invite_web_user_form(self): can_edit_tableau_config = (self.request.couch_user.has_permission(self.domain, 'edit_user_tableau_config') and toggles.TABLEAU_USER_SYNCING.enabled(self.domain)) if self.request.method == 'POST': - current_users = [user.username for user in WebUser.by_domain(self.domain)] - pending_invites = [di.email for di in Invitation.by_domain(self.domain)] return AdminInvitesUserForm( self.request.POST, - excluded_emails=current_users + pending_invites, role_choices=role_choices, domain=self.domain, is_add_user=is_add_user, From 591245a6a3eec7c8249c799a7661d6bbf8b99e03 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 21 Nov 2024 18:26:49 -0800 Subject: [PATCH 06/45] do same validation as done for bulk user import in preparation of validating Invitation creation via API --- corehq/apps/registration/validation.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 2182b8b0a343..38f4db58a9f1 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -6,6 +6,7 @@ from corehq.apps.user_importer.validation import ( RoleValidator, ProfileValidator, + EmailValidator ) from corehq.apps.users.models import Invitation, WebUser @@ -60,3 +61,7 @@ def validate_email(self, email, is_post): web_user = WebUser.get_by_username(email) if web_user and not web_user.is_active: return _("A user with this email address is deactivated. ") + + email_validator = EmailValidator(self.domain, 'email') + spec = {'email': email} + return email_validator.validate_spec(spec) From 3d99f3552673ce484492e77188ad6878504e3f4c Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 22 Nov 2024 11:47:13 -0800 Subject: [PATCH 07/45] refactor: move validating tableau role and tableau group header to check_headers --- corehq/apps/user_importer/importer.py | 28 ++++------- .../apps/user_importer/tests/test_importer.py | 32 ------------- corehq/apps/users/tests/test_views.py | 47 +++++++++++++++++++ corehq/apps/users/views/__init__.py | 11 +++-- corehq/apps/users/views/mobile/users.py | 7 ++- 5 files changed, 71 insertions(+), 54 deletions(-) diff --git a/corehq/apps/user_importer/importer.py b/corehq/apps/user_importer/importer.py index cb8e7a84d790..3cd461a0e489 100644 --- a/corehq/apps/user_importer/importer.py +++ b/corehq/apps/user_importer/importer.py @@ -4,7 +4,6 @@ import random from collections import defaultdict from datetime import datetime -from typing import List from corehq.util.soft_assert.api import soft_assert from memoized import memoized @@ -74,7 +73,7 @@ } -def check_headers(user_specs, domain, is_web_upload=False): +def check_headers(user_specs, domain, upload_couch_user, is_web_upload=False): messages = [] headers = set(user_specs.fieldnames) @@ -92,9 +91,16 @@ def check_headers(user_specs, domain, is_web_upload=False): if not is_web_upload and EnterpriseMobileWorkerSettings.is_domain_using_custom_deactivation(domain): allowed_headers.add('deactivate_after') - - if TABLEAU_USER_SYNCING.enabled(domain): + if TABLEAU_USER_SYNCING.enabled(domain) and upload_couch_user.has_permission( + domain, + get_permission_name(HqPermissions.edit_user_tableau_config) + ): allowed_headers.update({'tableau_role', 'tableau_groups'}) + elif "tableau_role" in headers or "tableau_groups" in headers: + messages.append(_( + "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau" + "User Syncing is enabled can upload files with 'Tableau Role and/or 'Tableau Groups' fields." + )) illegal_headers = headers - allowed_headers @@ -876,8 +882,6 @@ def domain_info(self, domain): def run(self): ret = {"errors": [], "rows": []} - column_headers = self.user_specs[0].keys() if self.user_specs else [] - check_field_edit_permissions(column_headers, self.upload_user, self.upload_domain) for i, row in enumerate(self.user_specs): if self.update_progress: self.update_progress(i) @@ -1027,18 +1031,6 @@ def create_or_update_web_users(upload_domain, user_specs, upload_user, upload_re ).run() -def check_field_edit_permissions(field_names: List, upload_couch_user, domain: str): - if "tableau_role" in field_names or "tableau_groups" in field_names: - if not upload_couch_user.has_permission( - domain, - get_permission_name(HqPermissions.edit_user_tableau_config) - ): - raise UserUploadError(_( - "Only users with 'Manage Tableau Configuration' edit permission can upload files with" - "'Tableau Role and/or 'Tableau Groups' fields. Please remove those fields from your file." - )) - - def check_user_role(username, role): if not role: raise UserUploadError(_( diff --git a/corehq/apps/user_importer/tests/test_importer.py b/corehq/apps/user_importer/tests/test_importer.py index 4c6c2f5c35df..1e28054570b3 100644 --- a/corehq/apps/user_importer/tests/test_importer.py +++ b/corehq/apps/user_importer/tests/test_importer.py @@ -2146,38 +2146,6 @@ def test_tableau_users(self, mock_request): TableauUser.Roles.UNLICENSED.value) local_tableau_users.get(username='george@eliot.com') - # Test user without permission to edit Tableau Configs - self.uploading_user.is_superuser = False - role_with_upload_permission = UserRole.create( - self.domain, 'edit-web-users', permissions=HqPermissions(edit_web_users=True) - ) - self.uploading_user.set_role(self.domain_name, role_with_upload_permission.get_qualified_id()) - self.uploading_user.save() - with self.assertRaises(UserUploadError): - import_users_and_groups( - self.domain.name, - [ - self._get_spec( - username='edith@wharton.com', - tableau_role=TableauUser.Roles.EXPLORER.value, - tableau_groups="""group1,group2""" - ), - ], - [], - self.uploading_user.get_id, - self.upload_record.pk, - True - ) - - # Test user with permission to edit Tableau Configs - role_with_upload_and_edit_tableau_permission = UserRole.create( - self.domain, 'edit-tableau', permissions=HqPermissions(edit_web_users=True, - edit_user_tableau_config=True) - ) - self.uploading_user.set_role(self.domain_name, - role_with_upload_and_edit_tableau_permission.get_qualified_id()) - self.uploading_user.save() - import_users_and_groups( self.domain.name, [ diff --git a/corehq/apps/users/tests/test_views.py b/corehq/apps/users/tests/test_views.py index a43d945e7627..4e183e4e6957 100644 --- a/corehq/apps/users/tests/test_views.py +++ b/corehq/apps/users/tests/test_views.py @@ -541,6 +541,53 @@ def test_invalid_workbook_headers(self): 'success': False }) + @flag_enabled('TABLEAU_USER_SYNCING') + def test_tableau_role_and_groups_headers(self): + workbook = Workbook() + users_sheet = workbook.create_sheet(title='users') + users_sheet.append(['username', 'email', 'password', 'tableau_role', 'tableau_groups']) + users_sheet.append(['test_user', 'test@example.com', 'password', 'fakerole', 'fakegroup']) + + file = BytesIO() + workbook.save(file) + file.seek(0) + file.name = 'users.xlsx' + + # Test user with permission to edit Tableau Configs + self.user.is_superuser = False + role_with_upload_and_edit_tableau_permission = UserRole.create( + self.domain, 'edit-tableau', permissions=HqPermissions(edit_web_users=True, + edit_user_tableau_config=True) + ) + self.user.set_role(self.domain_name, + role_with_upload_and_edit_tableau_permission.get_qualified_id()) + self.user.save() + + with patch('corehq.apps.users.views.mobile.users.BaseUploadUser.upload_users'): + response = self._make_post_request(file) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.json(), {'success': True}) + + # Test user without permission to edit Tableau Configs + role_with_upload_permission = UserRole.create( + self.domain, 'edit-web-users', permissions=HqPermissions(edit_web_users=True) + ) + self.user.set_role(self.domain_name, role_with_upload_permission.get_qualified_id()) + self.user.save() + + file.seek(0) + response = self._make_post_request(file) + self.assertEqual(response.status_code, 400) + expected_response = { + 'success': False, + 'message': ( + "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau " + "User Syncing is enabled can upload files with 'Tableau Role' and/or 'Tableau Groups' fields." + "\nThe following are illegal column headers: tableau_groups, tableau_role." + ), + } + self.assertEqual(response.json(), expected_response) + @patch('corehq.apps.users.views.mobile.users.BaseUploadUser.upload_users') def test_user_upload_error(self, mock_upload_users): mock_upload_users.side_effect = UserUploadError('User upload error') diff --git a/corehq/apps/users/views/__init__.py b/corehq/apps/users/views/__init__.py index 5139e88db203..662989233b8d 100644 --- a/corehq/apps/users/views/__init__.py +++ b/corehq/apps/users/views/__init__.py @@ -1277,7 +1277,12 @@ def post(self, request, *args, **kwargs): """View's dispatch method automatically calls this""" try: workbook = get_workbook(request.FILES.get("bulk_upload_file")) - user_specs, group_specs = self.process_workbook(workbook, self.domain, self.is_web_upload) + user_specs, group_specs = self.process_workbook( + workbook, + self.domain, + self.is_web_upload, + request.couch_user + ) task_ref = self.upload_users( request, user_specs, group_specs, self.domain, self.is_web_upload) return self._get_success_response(request, task_ref) @@ -1291,7 +1296,7 @@ def post(self, request, *args, **kwargs): return HttpResponseRedirect(reverse(self.urlname, args=[self.domain])) @staticmethod - def process_workbook(workbook, domain, is_web_upload): + def process_workbook(workbook, domain, is_web_upload, upload_user): from corehq.apps.user_importer.importer import check_headers try: @@ -1302,7 +1307,7 @@ def process_workbook(workbook, domain, is_web_upload): except WorksheetNotFound as e: raise WorksheetNotFound("Workbook has no worksheets") from e - check_headers(user_specs, domain, is_web_upload=is_web_upload) + check_headers(user_specs, domain, upload_couch_user=upload_user, is_web_upload=is_web_upload) try: group_specs = workbook.get_worksheet(title="groups") diff --git a/corehq/apps/users/views/mobile/users.py b/corehq/apps/users/views/mobile/users.py index 28e9603bfa3e..61de4a7e5606 100644 --- a/corehq/apps/users/views/mobile/users.py +++ b/corehq/apps/users/views/mobile/users.py @@ -1613,7 +1613,12 @@ def bulk_user_upload_api(request, domain): if file is None: raise UserUploadError(_('no file uploaded')) workbook = get_workbook(file) - user_specs, group_specs = BaseUploadUser.process_workbook(workbook, domain, is_web_upload=False) + user_specs, group_specs = BaseUploadUser.process_workbook( + workbook, + domain, + is_web_upload=False, + upload_user=request.couch_user + ) BaseUploadUser.upload_users(request, user_specs, group_specs, domain, is_web_upload=False) return json_response({'success': True}) except (WorkbookJSONError, WorksheetNotFound, UserUploadError) as e: From 9e801bedfe5efec2bd0698ccaa40e5d5e72bb2a4 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 22 Nov 2024 13:40:06 -0800 Subject: [PATCH 08/45] prefactor - consolidate validation to be used for invitation creation / web user edit API --- corehq/apps/registration/forms.py | 8 +++----- corehq/apps/registration/validation.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 6ddfd313606a..752bc499a94a 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -598,11 +598,6 @@ def clean_email(self): def clean(self): cleaned_data = super(AdminInvitesUserForm, self).clean() - - if (('tableau_role' in cleaned_data or 'tableau_group_indices' in cleaned_data) - and not self.can_edit_tableau_config): - raise forms.ValidationError(_("You do not have permission to edit Tableau Configuraion.")) - if 'tableau_group_indices' in cleaned_data: cleaned_data['tableau_group_ids'] = [ self.tableau_form.allowed_tableau_groups[int(i)].id @@ -624,6 +619,9 @@ def clean(self): cleaned_data['profile'] = profile_id cleaned_data['custom_user_data'] = get_prefixed(custom_user_data, self.custom_data.prefix) + errors = self._validator.validate_parameters(cleaned_data.keys()) + if errors: + raise forms.ValidationError(errors) return cleaned_data def _initialize_tableau_fields(self, data, domain): diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 38f4db58a9f1..824ca09f8933 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -2,6 +2,8 @@ from django.utils.translation import gettext_lazy as _ +from corehq import privileges +from corehq.apps.accounting.utils import domain_has_privilege from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition from corehq.apps.user_importer.validation import ( RoleValidator, @@ -9,6 +11,7 @@ EmailValidator ) from corehq.apps.users.models import Invitation, WebUser +from corehq.toggles import TABLEAU_USER_SYNCING class AdminInvitesUserValidator(): @@ -44,6 +47,17 @@ def current_users_and_pending_invites(self): pending_invites = [di.email.lower() for di in Invitation.by_domain(self.domain)] return current_users + pending_invites + def validate_parameters(self, parameters): + can_edit_tableau_config = (self.upload_user.has_permission(self.domain, 'edit_user_tableau_config') + and TABLEAU_USER_SYNCING.enabled(self.domain)) + if (('tableau_role' in parameters or 'tableau_group_indices' in parameters) + and not can_edit_tableau_config): + return _("You do not have permission to edit Tableau Configuraion.") + + has_profile_privilege = domain_has_privilege(self.domain, privileges.APP_USER_PROFILES) + if 'profile' in parameters and not has_profile_privilege: + return _("This domain does not have user profile privileges.") + def validate_role(self, role): spec = {'role': role} return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) From a0119f62c3eac0c5566f35e43d4fe5f053aafab9 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 22 Nov 2024 14:03:26 -0800 Subject: [PATCH 09/45] refactor: extract required values from spec in higher level function to make it easier to tell what values are needed --- corehq/apps/user_importer/validation.py | 41 +++++++++++++------------ 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 5f2d030ccf3e..60cc9578a8bd 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -505,17 +505,28 @@ def __init__(self, domain, upload_user, location_cache, is_web_user_import): self.location_cache = location_cache self.is_web_user_import = is_web_user_import - def _get_locs_being_assigned(self, spec): + def validate_spec(self, spec): + locs_being_assigned = self._get_locs_ids_being_assigned(spec) + user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) + + user_access_error = self._validate_uploading_user_access(locs_being_assigned, user_result) + location_cannot_have_users_error = None + if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): + location_cannot_have_users_error = self._validate_location_has_users(locs_being_assigned) + return user_access_error or location_cannot_have_users_error + + def _get_locs_ids_being_assigned(self, spec): from corehq.apps.user_importer.importer import find_location_id - location_codes = (spec['location_code'] if isinstance(spec['location_code'], list) - else [spec['location_code']]) - locs_ids_being_assigned = find_location_id(location_codes, self.location_cache) + locs_ids_being_assigned = [] + if 'location_code' in spec: + location_codes = (spec['location_code'] if isinstance(spec['location_code'], list) + else [spec['location_code']]) + locs_ids_being_assigned = find_location_id(location_codes, self.location_cache) return locs_ids_being_assigned - def _validate_uploading_user_access(self, spec): + def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): # 1. Get current locations for user or user invitation and ensure user can edit it current_locs = [] - user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) if user_result.invitation: if not user_can_access_invite(self.domain, self.upload_user, user_result.invitation): return self.error_message_user_access.format(user_result.invitation.email) @@ -527,31 +538,23 @@ def _validate_uploading_user_access(self, spec): # 2. Ensure the user is only adding the user to/removing from *new locations* that they have permission # to access. - if 'location_code' in spec: - locs_being_assigned = self._get_locs_being_assigned(spec) + if locs_ids_being_assigned: problem_location_ids = user_can_change_locations(self.domain, self.upload_user, - current_locs, locs_being_assigned) + current_locs, locs_ids_being_assigned) if problem_location_ids: return self.error_message_location_access.format( ', '.join(SQLLocation.objects.filter( location_id__in=problem_location_ids).values_list('site_code', flat=True))) - def _validate_location_has_users(self, spec): - if 'location_code' not in spec: + def _validate_location_has_users(self, locs_ids_being_assigned): + if not locs_ids_being_assigned: return - locs_being_assigned = SQLLocation.objects.filter(location_id__in=self._get_locs_being_assigned(spec)) + locs_being_assigned = SQLLocation.objects.filter(location_id__in=locs_ids_being_assigned) problem_locations = locs_being_assigned.filter(location_type__has_users=False) if problem_locations: return self.error_message_location_not_has_users.format( ', '.join(problem_locations.values_list('site_code', flat=True))) - def validate_spec(self, spec): - user_access_error = self._validate_uploading_user_access(spec) - location_cannot_have_users_error = None - if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): - location_cannot_have_users_error = self._validate_location_has_users(spec) - return user_access_error or location_cannot_have_users_error - class UserRetrievalResult(NamedTuple): invitation: Optional[Invitation] = None From 0fc4ddd7967d5107ee7a06a1d5d2c247603b18de Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 22 Nov 2024 16:38:44 -0800 Subject: [PATCH 10/45] create validation for location in preparetion for create invitation/update web user API --- corehq/apps/registration/validation.py | 34 +++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 824ca09f8933..63d1bfffd06b 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -8,7 +8,9 @@ from corehq.apps.user_importer.validation import ( RoleValidator, ProfileValidator, - EmailValidator + EmailValidator, + LocationValidator, + SiteCodeToLocationCache, ) from corehq.apps.users.models import Invitation, WebUser from corehq.toggles import TABLEAU_USER_SYNCING @@ -47,6 +49,11 @@ def current_users_and_pending_invites(self): pending_invites = [di.email.lower() for di in Invitation.by_domain(self.domain)] return current_users + pending_invites + @property + @memoized + def location_cache(self): + return SiteCodeToLocationCache(self.domain) + def validate_parameters(self, parameters): can_edit_tableau_config = (self.upload_user.has_permission(self.domain, 'edit_user_tableau_config') and TABLEAU_USER_SYNCING.enabled(self.domain)) @@ -58,6 +65,11 @@ def validate_parameters(self, parameters): if 'profile' in parameters and not has_profile_privilege: return _("This domain does not have user profile privileges.") + has_locations_privilege = domain_has_privilege(self.domain, privileges.LOCATIONS) + if (('primary_location' in parameters or 'assigned_locations' in parameters) + and not has_locations_privilege): + return _("This domain does not have locations privileges.") + def validate_role(self, role): spec = {'role': role} return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) @@ -79,3 +91,23 @@ def validate_email(self, email, is_post): email_validator = EmailValidator(self.domain, 'email') spec = {'email': email} return email_validator.validate_spec(spec) + + def validate_locations(self, editable_user, assigned_location_codes, primary_location_code): + if primary_location_code: + if primary_location_code not in assigned_location_codes: + return ( + 'primary_location', + _("Primary location must be one of the user's locations") + ) + if assigned_location_codes and not primary_location_code: + return ( + 'primary_location', + _("Primary location can't be empty if the user has any " + "locations set") + ) + + location_validator = LocationValidator(self.domain, self.upload_user, self.location_cache, True) + location_codes = assigned_location_codes + [primary_location_code] + spec = {'location_code': location_codes, + 'username': editable_user} + return location_validator.validate_spec(spec) From fc6ced85b781434254535ed77e5bef61f74898ef Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 22 Nov 2024 19:55:32 -0800 Subject: [PATCH 11/45] refactor: location_ids is the equivalent return --- corehq/apps/users/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/users/forms.py b/corehq/apps/users/forms.py index 89619fdb586c..e7b778d6d964 100644 --- a/corehq/apps/users/forms.py +++ b/corehq/apps/users/forms.py @@ -1121,7 +1121,7 @@ def clean_assigned_locations(self): if locations.filter(location_type__has_users=False).exists(): raise forms.ValidationError( _('One or more of the locations you specified cannot have users assigned.')) - return [location.location_id for location in locations] + return location_ids def _user_has_permission_to_access_locations(self, new_location_ids): assigned_locations = SQLLocation.objects.filter(location_id__in=new_location_ids) From 30a3214af04284a8eb55b5b43e763247f5b8eec2 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 11:23:40 -0800 Subject: [PATCH 12/45] refactor: consolidate validation for location having users --- corehq/apps/user_importer/validation.py | 7 ++----- corehq/apps/users/forms.py | 14 ++++---------- corehq/apps/users/validation.py | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 60cc9578a8bd..41451f4fd3cb 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -22,6 +22,7 @@ from corehq.apps.users.forms import get_mobile_worker_max_username_length from corehq.apps.users.models import CouchUser, Invitation from corehq.apps.users.util import normalize_username, raw_username +from corehq.apps.users.validation import validate_assigned_locations_has_users from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView from corehq.apps.users.views.utils import ( user_can_access_invite @@ -549,11 +550,7 @@ def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): def _validate_location_has_users(self, locs_ids_being_assigned): if not locs_ids_being_assigned: return - locs_being_assigned = SQLLocation.objects.filter(location_id__in=locs_ids_being_assigned) - problem_locations = locs_being_assigned.filter(location_type__has_users=False) - if problem_locations: - return self.error_message_location_not_has_users.format( - ', '.join(problem_locations.values_list('site_code', flat=True))) + return validate_assigned_locations_has_users(self.domain, locs_ids_being_assigned) class UserRetrievalResult(NamedTuple): diff --git a/corehq/apps/users/forms.py b/corehq/apps/users/forms.py index e7b778d6d964..51a4f9d88145 100644 --- a/corehq/apps/users/forms.py +++ b/corehq/apps/users/forms.py @@ -1110,17 +1110,11 @@ def __init__(self, domain: str, *args, **kwargs): ) def clean_assigned_locations(self): - from corehq.apps.locations.models import SQLLocation - from corehq.apps.locations.util import get_locations_from_ids - + from corehq.apps.users.validation import validate_assigned_locations_has_users location_ids = self.data.getlist('assigned_locations') - try: - locations = get_locations_from_ids(location_ids, self.domain) - except SQLLocation.DoesNotExist: - raise forms.ValidationError(_('One or more of the locations was not found.')) - if locations.filter(location_type__has_users=False).exists(): - raise forms.ValidationError( - _('One or more of the locations you specified cannot have users assigned.')) + error = validate_assigned_locations_has_users(self.domain, location_ids) + if error: + raise forms.ValidationError(error) return location_ids def _user_has_permission_to_access_locations(self, new_location_ids): diff --git a/corehq/apps/users/validation.py b/corehq/apps/users/validation.py index 4771e8c5ecbf..634a46f1f967 100644 --- a/corehq/apps/users/validation.py +++ b/corehq/apps/users/validation.py @@ -4,6 +4,8 @@ from corehq.apps.users.util import is_username_available from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition +from corehq.apps.locations.models import SQLLocation +from corehq.apps.locations.util import get_locations_from_ids from corehq.apps.users.views.mobile import BAD_MOBILE_USERNAME_REGEX from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView @@ -59,3 +61,16 @@ def validate_profile_required(profile_name, domain): raise ValidationError( _("A profile assignment is required for Mobile Workers.") ) + + +def validate_assigned_locations_has_users(domain, assigned_location_ids): + error_message_location_not_has_users = _("These locations cannot have users assigned because of their " + "organization level settings: {}.") + try: + locs_being_assigned = get_locations_from_ids(assigned_location_ids, domain) + except SQLLocation.DoesNotExist: + return _('One or more of the locations was not found.') + problem_locations = locs_being_assigned.filter(location_type__has_users=False) + if problem_locations: + return error_message_location_not_has_users.format( + ', '.join(problem_locations.values_list('site_code', flat=True))) From 2e1de11a8c94d78813b83d89ea5e895860a8f3a2 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 11:33:00 -0800 Subject: [PATCH 13/45] refactor: consolidate validating that primary location is one of the assigned locations or that primary location is defined if assigned locations is defined --- corehq/apps/registration/validation.py | 16 ++++------------ corehq/apps/users/forms.py | 17 +++++------------ corehq/apps/users/validation.py | 8 ++++++++ 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 63d1bfffd06b..7686e37b986b 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -13,6 +13,7 @@ SiteCodeToLocationCache, ) from corehq.apps.users.models import Invitation, WebUser +from corehq.apps.users.validation import validate_primary_location_assignment from corehq.toggles import TABLEAU_USER_SYNCING @@ -93,18 +94,9 @@ def validate_email(self, email, is_post): return email_validator.validate_spec(spec) def validate_locations(self, editable_user, assigned_location_codes, primary_location_code): - if primary_location_code: - if primary_location_code not in assigned_location_codes: - return ( - 'primary_location', - _("Primary location must be one of the user's locations") - ) - if assigned_location_codes and not primary_location_code: - return ( - 'primary_location', - _("Primary location can't be empty if the user has any " - "locations set") - ) + error = validate_primary_location_assignment(primary_location_code, assigned_location_codes) + if error: + return error location_validator = LocationValidator(self.domain, self.upload_user, self.location_cache, True) location_codes = assigned_location_codes + [primary_location_code] diff --git a/corehq/apps/users/forms.py b/corehq/apps/users/forms.py index 51a4f9d88145..f6a8b2edb51a 100644 --- a/corehq/apps/users/forms.py +++ b/corehq/apps/users/forms.py @@ -1132,18 +1132,11 @@ def clean(self): 'assigned_locations', _("You do not have permissions to assign one of those locations.") ) - if primary_location_id: - if primary_location_id not in assigned_location_ids: - self.add_error( - 'primary_location', - _("Primary location must be one of the user's locations") - ) - if assigned_location_ids and not primary_location_id: - self.add_error( - 'primary_location', - _("Primary location can't be empty if the user has any " - "locations set") - ) + from corehq.apps.users.validation import validate_primary_location_assignment + error = validate_primary_location_assignment(primary_location_id, assigned_location_ids) + if error: + self.add_error('primary_location', error) + return self.cleaned_data diff --git a/corehq/apps/users/validation.py b/corehq/apps/users/validation.py index 634a46f1f967..42ee7eb49358 100644 --- a/corehq/apps/users/validation.py +++ b/corehq/apps/users/validation.py @@ -74,3 +74,11 @@ def validate_assigned_locations_has_users(domain, assigned_location_ids): if problem_locations: return error_message_location_not_has_users.format( ', '.join(problem_locations.values_list('site_code', flat=True))) + + +def validate_primary_location_assignment(primary_location, assigned_locations): + if primary_location: + if primary_location not in assigned_locations: + return _("Primary location must be one of the user's locations") + if assigned_locations and not primary_location: + return _("Primary location can't be empty if the user has any locations set") From 7584aea4b007df3c6f01c1d4097d698351b6c5c0 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 13:12:23 -0800 Subject: [PATCH 14/45] refactor to extract out validation of tableau groups from the expected format from spec --- corehq/apps/user_importer/validation.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 41451f4fd3cb..ff6fbd8a1299 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -172,12 +172,16 @@ def validate_spec(self, spec): tableau_groups = spec.get('tableau_groups') or [] if tableau_groups: tableau_groups = tableau_groups.split(',') + return self.validate_tableau_groups(self.allowed_groups_for_domain, tableau_groups) + + @classmethod + def validate_tableau_groups(cls, allowed_groups_for_domain, tableau_groups): invalid_groups = [] for group in tableau_groups: - if group not in self.allowed_groups_for_domain: + if group not in allowed_groups_for_domain: invalid_groups.append(group) if invalid_groups: - return self._error_message.format(', '.join(invalid_groups), ', '.join(self.allowed_groups_for_domain)) + return cls._error_message.format(', '.join(invalid_groups), ', '.join(allowed_groups_for_domain)) class DuplicateValidator(ImportValidator): From 21fd7ac2941549a7045ef291a236ebb1968f5e9e Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 13:15:13 -0800 Subject: [PATCH 15/45] add validation of tableau group to Admin Invites User Validator to be used for invite creation API --- corehq/apps/registration/validation.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 7686e37b986b..7c8df89d7fb8 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -5,12 +5,14 @@ from corehq import privileges from corehq.apps.accounting.utils import domain_has_privilege from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition +from corehq.apps.reports.util import get_allowed_tableau_groups_for_domain from corehq.apps.user_importer.validation import ( RoleValidator, ProfileValidator, EmailValidator, LocationValidator, SiteCodeToLocationCache, + TableauGroupsValidator ) from corehq.apps.users.models import Invitation, WebUser from corehq.apps.users.validation import validate_primary_location_assignment @@ -103,3 +105,7 @@ def validate_locations(self, editable_user, assigned_location_codes, primary_loc spec = {'location_code': location_codes, 'username': editable_user} return location_validator.validate_spec(spec) + + def validate_tableau_group(self, tableau_groups): + allowed_groups_for_domain = get_allowed_tableau_groups_for_domain(self.domain) or [] + return TableauGroupsValidator.validate_tableau_groups(allowed_groups_for_domain, tableau_groups) From ff73910b941e06f6180851376ec0acb9965b5e61 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 16:17:56 -0800 Subject: [PATCH 16/45] prefactor: extract out the validation of the tableau_role independent of the spec format in preparation for reuse --- corehq/apps/user_importer/validation.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index ff6fbd8a1299..1f107535ab76 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -151,12 +151,16 @@ class TableauRoleValidator(ImportValidator): def __init__(self, domain): super().__init__(domain) - self.valid_role_options = [e.value for e in TableauUser.Roles] def validate_spec(self, spec): tableau_role = spec.get('tableau_role') - if tableau_role is not None and tableau_role not in self.valid_role_options: - return self._error_message.format(tableau_role, ', '.join(self.valid_role_options)) + return self.validate_tableau_role(tableau_role) + + @classmethod + def validate_tableau_role(cls, tableau_role): + valid_role_options = [e.value for e in TableauUser.Roles] + if tableau_role is not None and tableau_role not in valid_role_options: + return cls._error_message.format(tableau_role, ', '.join(valid_role_options)) class TableauGroupsValidator(ImportValidator): From cd7912941ad32e4c402beb5ce69d20945095a980 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 16:20:52 -0800 Subject: [PATCH 17/45] add validation for tableau role --- corehq/apps/registration/validation.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 7c8df89d7fb8..e1de5f1de908 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -12,7 +12,8 @@ EmailValidator, LocationValidator, SiteCodeToLocationCache, - TableauGroupsValidator + TableauGroupsValidator, + TableauRoleValidator ) from corehq.apps.users.models import Invitation, WebUser from corehq.apps.users.validation import validate_primary_location_assignment @@ -109,3 +110,6 @@ def validate_locations(self, editable_user, assigned_location_codes, primary_loc def validate_tableau_group(self, tableau_groups): allowed_groups_for_domain = get_allowed_tableau_groups_for_domain(self.domain) or [] return TableauGroupsValidator.validate_tableau_groups(allowed_groups_for_domain, tableau_groups) + + def validate_tableau_role(self, tableau_role): + return TableauRoleValidator.validate_tableau_role(tableau_role) From 062dde561b13d751bdef6143172a431611095e34 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 16:43:24 -0800 Subject: [PATCH 18/45] add validation for custom data in preparation for web user creation API --- corehq/apps/registration/validation.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index e1de5f1de908..1000c80dfd65 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -13,7 +13,8 @@ LocationValidator, SiteCodeToLocationCache, TableauGroupsValidator, - TableauRoleValidator + TableauRoleValidator, + CustomDataValidator, ) from corehq.apps.users.models import Invitation, WebUser from corehq.apps.users.validation import validate_primary_location_assignment @@ -83,6 +84,11 @@ def validate_profile(self, new_profile_name): spec = {'user_profile': new_profile_name} return profile_validator.validate_spec(spec) + def validate_custom_data(self, custom_data, profile_name): + custom_data_validator = CustomDataValidator(self.domain, self.profiles_by_name()) + spec = {'data': custom_data, 'user_profile': profile_name} + return custom_data_validator.validate_spec(spec) + def validate_email(self, email, is_post): if is_post: if email.lower() in self.current_users_and_pending_invites: From f0cd397182d7110a868ac44154c0d85dba248057 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 16:55:32 -0800 Subject: [PATCH 19/45] remove unnecessary caching --- corehq/apps/registration/validation.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 1000c80dfd65..544b91d71bad 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -27,7 +27,6 @@ def __init__(self, domain, upload_user): self.upload_user = upload_user @property - @memoized def roles_by_name(self): from corehq.apps.users.views.utils import get_editable_role_choices return {role[1]: role[0] for role in get_editable_role_choices(self.domain, self.upload_user, @@ -48,14 +47,12 @@ def profiles_by_name(self): return {} @property - @memoized def current_users_and_pending_invites(self): current_users = [user.username.lower() for user in WebUser.by_domain(self.domain)] pending_invites = [di.email.lower() for di in Invitation.by_domain(self.domain)] return current_users + pending_invites @property - @memoized def location_cache(self): return SiteCodeToLocationCache(self.domain) From ef2b0e6376b698e7d32d801c470e66bcb3d74c04 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Mon, 25 Nov 2024 17:17:08 -0800 Subject: [PATCH 20/45] refactor: DRY function --- corehq/apps/custom_data_fields/models.py | 12 ++++++++++++ corehq/apps/registration/validation.py | 10 +--------- corehq/apps/user_importer/importer.py | 10 +--------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/corehq/apps/custom_data_fields/models.py b/corehq/apps/custom_data_fields/models.py index f026b0ae4b56..be4df133b6cb 100644 --- a/corehq/apps/custom_data_fields/models.py +++ b/corehq/apps/custom_data_fields/models.py @@ -120,6 +120,18 @@ def get_profile_required_for_user_type_list(cls, domain, field_type): return profile_required_for_user_type_list return None + @classmethod + def get_profiles_by_name(cls, domain, field_type): + definition = cls.get(domain, field_type) + if definition: + profiles = definition.get_profiles() + return { + profile.name: profile + for profile in profiles + } + else: + return {} + class FieldFilterConfig: def __init__(self, required_only=False, is_required_check_func=None): self.required_only = required_only diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 544b91d71bad..59e73009dad2 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -36,15 +36,7 @@ def roles_by_name(self): @memoized def profiles_by_name(self): from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView - definition = CustomDataFieldsDefinition.get(self.domain, UserFieldsView.field_type) - if definition: - profiles = definition.get_profiles() - return { - profile.name: profile - for profile in profiles - } - else: - return {} + return CustomDataFieldsDefinition.get_profiles_by_name(self.domain, UserFieldsView.field_type) @property def current_users_and_pending_invites(self): diff --git a/corehq/apps/user_importer/importer.py b/corehq/apps/user_importer/importer.py index 3cd461a0e489..d54ca8fe8a93 100644 --- a/corehq/apps/user_importer/importer.py +++ b/corehq/apps/user_importer/importer.py @@ -963,15 +963,7 @@ def location_cache(self): @memoized def profiles_by_name(self): from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView - definition = CustomDataFieldsDefinition.get(self.domain, UserFieldsView.field_type) - if definition: - profiles = definition.get_profiles() - return { - profile.name: profile - for profile in profiles - } - else: - return {} + return CustomDataFieldsDefinition.get_profiles_by_name(self.domain, UserFieldsView.field_type) @property @memoized From 64c904afcaa1cb18cec9b9d35b293e055c10cc11 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Tue, 26 Nov 2024 13:21:21 -0800 Subject: [PATCH 21/45] refactor: The initial purpose of the validation class was to be used for web user API. However, there wasn't as much overlap with validation used for AdminInvitesUserForm. So move validation for Web User Resource its own file and create a class to organize the validations relevant to AdminInvitesUserForm --- corehq/apps/api/validation.py | 75 +++++++++++++++++++ corehq/apps/registration/forms.py | 13 ++-- corehq/apps/registration/validation.py | 99 +++++--------------------- 3 files changed, 101 insertions(+), 86 deletions(-) create mode 100644 corehq/apps/api/validation.py diff --git a/corehq/apps/api/validation.py b/corehq/apps/api/validation.py new file mode 100644 index 000000000000..ba010bc256b6 --- /dev/null +++ b/corehq/apps/api/validation.py @@ -0,0 +1,75 @@ +from memoized import memoized + +from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition +from corehq.apps.reports.util import get_allowed_tableau_groups_for_domain +from corehq.apps.user_importer.validation import ( + RoleValidator, + ProfileValidator, + LocationValidator, + SiteCodeToLocationCache, + TableauGroupsValidator, + TableauRoleValidator, + CustomDataValidator, +) +from corehq.apps.users.validation import validate_primary_location_assignment +from corehq.apps.registration.validation import AdminInvitesUserFormValidator + + +class WebUserResourceValidator(): + def __init__(self, domain, requesting_user): + self.domain = domain + self.requesting_user = requesting_user + + @property + def roles_by_name(self): + from corehq.apps.users.views.utils import get_editable_role_choices + return {role[1]: role[0] for role in get_editable_role_choices(self.domain, self.requesting_user, + allow_admin_role=True)} + + @property + @memoized + def profiles_by_name(self): + from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView + return CustomDataFieldsDefinition.get_profiles_by_name(self.domain, UserFieldsView.field_type) + + @property + def location_cache(self): + return SiteCodeToLocationCache(self.domain) + + def validate_parameters(self, parameters): + AdminInvitesUserFormValidator.validate_parameters(self.domain, self.requesting_user, parameters) + + def validate_role(self, role): + spec = {'role': role} + return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) + + def validate_profile(self, new_profile_name): + profile_validator = ProfileValidator(self.domain, self.requesting_user, True, self.profiles_by_name()) + spec = {'user_profile': new_profile_name} + return profile_validator.validate_spec(spec) + + def validate_custom_data(self, custom_data, profile_name): + custom_data_validator = CustomDataValidator(self.domain, self.profiles_by_name()) + spec = {'data': custom_data, 'user_profile': profile_name} + return custom_data_validator.validate_spec(spec) + + def validate_email(self, email, is_post): + return AdminInvitesUserFormValidator.validate_email(self.domain, email, is_post) + + def validate_locations(self, editable_user, assigned_location_codes, primary_location_code): + error = validate_primary_location_assignment(primary_location_code, assigned_location_codes) + if error: + return error + + location_validator = LocationValidator(self.domain, self.requesting_user, self.location_cache, True) + location_codes = assigned_location_codes + [primary_location_code] + spec = {'location_code': location_codes, + 'username': editable_user} + return location_validator.validate_spec(spec) + + def validate_tableau_group(self, tableau_groups): + allowed_groups_for_domain = get_allowed_tableau_groups_for_domain(self.domain) or [] + return TableauGroupsValidator.validate_tableau_groups(allowed_groups_for_domain, tableau_groups) + + def validate_tableau_role(self, tableau_role): + return TableauRoleValidator.validate_tableau_role(tableau_role) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 752bc499a94a..64d68a467e25 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -501,8 +501,6 @@ def __init__(self, data=None, is_add_user=None, custom_data_post_dict = self.custom_data.form.data data.update({k: v for k, v in custom_data_post_dict.items() if k not in data}) self.request = kwargs.get('request') - from corehq.apps.registration.validation import AdminInvitesUserValidator - self._validator = AdminInvitesUserValidator(domain, self.request.couch_user) super(AdminInvitesUserForm, self).__init__(domain=domain, data=data, **kwargs) self.can_edit_tableau_config = can_edit_tableau_config domain_obj = Domain.get_by_name(domain) @@ -591,7 +589,9 @@ def __init__(self, data=None, is_add_user=None, def clean_email(self): email = self.cleaned_data['email'].strip() - errors = self._validator.validate_email(email, self.request.method == 'POST') + + from corehq.apps.registration.validation import AdminInvitesUserFormValidator + errors = AdminInvitesUserFormValidator.validate_email(self.domain, email, self.request.method == 'POST') if errors: raise forms.ValidationError(errors) return email @@ -619,7 +619,12 @@ def clean(self): cleaned_data['profile'] = profile_id cleaned_data['custom_user_data'] = get_prefixed(custom_user_data, self.custom_data.prefix) - errors = self._validator.validate_parameters(cleaned_data.keys()) + from corehq.apps.registration.validation import AdminInvitesUserFormValidator + errors = AdminInvitesUserFormValidator.validate_parameters( + self.domain, + self.request.couch_user, + cleaned_data.keys() + ) if errors: raise forms.ValidationError(errors) return cleaned_data diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 59e73009dad2..b7376e87586d 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -1,110 +1,45 @@ -from memoized import memoized - from django.utils.translation import gettext_lazy as _ from corehq import privileges from corehq.apps.accounting.utils import domain_has_privilege -from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition -from corehq.apps.reports.util import get_allowed_tableau_groups_for_domain -from corehq.apps.user_importer.validation import ( - RoleValidator, - ProfileValidator, - EmailValidator, - LocationValidator, - SiteCodeToLocationCache, - TableauGroupsValidator, - TableauRoleValidator, - CustomDataValidator, -) -from corehq.apps.users.models import Invitation, WebUser -from corehq.apps.users.validation import validate_primary_location_assignment +from corehq.apps.user_importer.validation import EmailValidator +from corehq.apps.users.models import WebUser, Invitation from corehq.toggles import TABLEAU_USER_SYNCING -class AdminInvitesUserValidator(): - def __init__(self, domain, upload_user): - self.domain = domain - self.upload_user = upload_user - - @property - def roles_by_name(self): - from corehq.apps.users.views.utils import get_editable_role_choices - return {role[1]: role[0] for role in get_editable_role_choices(self.domain, self.upload_user, - allow_admin_role=True)} - - @property - @memoized - def profiles_by_name(self): - from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView - return CustomDataFieldsDefinition.get_profiles_by_name(self.domain, UserFieldsView.field_type) - - @property - def current_users_and_pending_invites(self): - current_users = [user.username.lower() for user in WebUser.by_domain(self.domain)] - pending_invites = [di.email.lower() for di in Invitation.by_domain(self.domain)] - return current_users + pending_invites +class AdminInvitesUserFormValidator(): - @property - def location_cache(self): - return SiteCodeToLocationCache(self.domain) - - def validate_parameters(self, parameters): - can_edit_tableau_config = (self.upload_user.has_permission(self.domain, 'edit_user_tableau_config') - and TABLEAU_USER_SYNCING.enabled(self.domain)) + @staticmethod + def validate_parameters(domain, upload_user, parameters): + can_edit_tableau_config = (upload_user.has_permission(domain, 'edit_user_tableau_config') + and TABLEAU_USER_SYNCING.enabled(domain)) if (('tableau_role' in parameters or 'tableau_group_indices' in parameters) and not can_edit_tableau_config): return _("You do not have permission to edit Tableau Configuraion.") - has_profile_privilege = domain_has_privilege(self.domain, privileges.APP_USER_PROFILES) + has_profile_privilege = domain_has_privilege(domain, privileges.APP_USER_PROFILES) if 'profile' in parameters and not has_profile_privilege: return _("This domain does not have user profile privileges.") - has_locations_privilege = domain_has_privilege(self.domain, privileges.LOCATIONS) + has_locations_privilege = domain_has_privilege(domain, privileges.LOCATIONS) if (('primary_location' in parameters or 'assigned_locations' in parameters) and not has_locations_privilege): return _("This domain does not have locations privileges.") - def validate_role(self, role): - spec = {'role': role} - return RoleValidator(self.domain, self.roles_by_name()).validate_spec(spec) - - def validate_profile(self, new_profile_name): - profile_validator = ProfileValidator(self.domain, self.upload_user, True, self.profiles_by_name()) - spec = {'user_profile': new_profile_name} - return profile_validator.validate_spec(spec) - - def validate_custom_data(self, custom_data, profile_name): - custom_data_validator = CustomDataValidator(self.domain, self.profiles_by_name()) - spec = {'data': custom_data, 'user_profile': profile_name} - return custom_data_validator.validate_spec(spec) - - def validate_email(self, email, is_post): + @staticmethod + def validate_email(domain, email, is_post): if is_post: - if email.lower() in self.current_users_and_pending_invites: + current_users = [user.username.lower() for user in WebUser.by_domain(domain)] + pending_invites = [di.email.lower() for di in Invitation.by_domain(domain)] + current_users_and_pending_invites = current_users + pending_invites + + if email.lower() in current_users_and_pending_invites: return _("A user with this email address is already in " "this project or has a pending invitation.") web_user = WebUser.get_by_username(email) if web_user and not web_user.is_active: return _("A user with this email address is deactivated. ") - email_validator = EmailValidator(self.domain, 'email') + email_validator = EmailValidator(domain, 'email') spec = {'email': email} return email_validator.validate_spec(spec) - - def validate_locations(self, editable_user, assigned_location_codes, primary_location_code): - error = validate_primary_location_assignment(primary_location_code, assigned_location_codes) - if error: - return error - - location_validator = LocationValidator(self.domain, self.upload_user, self.location_cache, True) - location_codes = assigned_location_codes + [primary_location_code] - spec = {'location_code': location_codes, - 'username': editable_user} - return location_validator.validate_spec(spec) - - def validate_tableau_group(self, tableau_groups): - allowed_groups_for_domain = get_allowed_tableau_groups_for_domain(self.domain) or [] - return TableauGroupsValidator.validate_tableau_groups(allowed_groups_for_domain, tableau_groups) - - def validate_tableau_role(self, tableau_role): - return TableauRoleValidator.validate_tableau_role(tableau_role) From a043006bb75c51753b8f3a3a91ad8fd40925e03d Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Tue, 26 Nov 2024 14:22:35 -0800 Subject: [PATCH 22/45] perform permission and privilege check only if the parameter exists --- corehq/apps/registration/validation.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index b7376e87586d..3480018bcf01 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -11,19 +11,19 @@ class AdminInvitesUserFormValidator(): @staticmethod def validate_parameters(domain, upload_user, parameters): - can_edit_tableau_config = (upload_user.has_permission(domain, 'edit_user_tableau_config') - and TABLEAU_USER_SYNCING.enabled(domain)) - if (('tableau_role' in parameters or 'tableau_group_indices' in parameters) - and not can_edit_tableau_config): - return _("You do not have permission to edit Tableau Configuraion.") - - has_profile_privilege = domain_has_privilege(domain, privileges.APP_USER_PROFILES) - if 'profile' in parameters and not has_profile_privilege: + if 'tableau_role' in parameters or 'tableau_group_indices' in parameters: + can_edit_tableau_config = ( + upload_user.has_permission(domain, 'edit_user_tableau_config') + and TABLEAU_USER_SYNCING.enabled(domain) + ) + if not can_edit_tableau_config: + return _("You do not have permission to edit Tableau Configuration.") + + if 'profile' in parameters and not domain_has_privilege(domain, privileges.APP_USER_PROFILES): return _("This domain does not have user profile privileges.") - has_locations_privilege = domain_has_privilege(domain, privileges.LOCATIONS) if (('primary_location' in parameters or 'assigned_locations' in parameters) - and not has_locations_privilege): + and not domain_has_privilege(domain, privileges.LOCATIONS)): return _("This domain does not have locations privileges.") @staticmethod From cca40983ba23a7b333101d88d8e16c368a265ec4 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Tue, 26 Nov 2024 16:37:22 -0800 Subject: [PATCH 23/45] remove location fields from backend if location shouldn't be shown --- corehq/apps/registration/forms.py | 3 +++ corehq/apps/users/forms.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 64d68a467e25..09f6f25f6e20 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -552,6 +552,9 @@ def __init__(self, data=None, is_add_user=None, 'primary_location', ) ) + else: + self.fields.pop('assigned_locations', None) + self.fields.pop('primary_location', None) if self.can_edit_tableau_config: fields.append( crispy.Fieldset( diff --git a/corehq/apps/users/forms.py b/corehq/apps/users/forms.py index f6a8b2edb51a..d47c4c73119b 100644 --- a/corehq/apps/users/forms.py +++ b/corehq/apps/users/forms.py @@ -1125,7 +1125,7 @@ def _user_has_permission_to_access_locations(self, new_location_ids): def clean(self): self.cleaned_data = super(SelectUserLocationForm, self).clean() - primary_location_id = self.cleaned_data['primary_location'] + primary_location_id = self.cleaned_data.get('primary_location', '') assigned_location_ids = self.cleaned_data.get('assigned_locations', []) if not self._user_has_permission_to_access_locations(assigned_location_ids): self.add_error( From 7f3807e859908f6863ac94c8e4996b3f6480b812 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Tue, 26 Nov 2024 17:42:05 -0800 Subject: [PATCH 24/45] update test and adds test for validating if invitation email is an existing webuser --- corehq/apps/registration/tests/test_forms.py | 46 +++++++++++++++++--- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/corehq/apps/registration/tests/test_forms.py b/corehq/apps/registration/tests/test_forms.py index f4dc896a2beb..289475e92bf8 100644 --- a/corehq/apps/registration/tests/test_forms.py +++ b/corehq/apps/registration/tests/test_forms.py @@ -4,7 +4,7 @@ from testil import Regex from ..forms import AdminInvitesUserForm -from corehq.apps.users.models import WebUser +from corehq.apps.users.models import WebUser, Invitation patch_query = patch.object( @@ -19,21 +19,50 @@ domain="test-domain", ) +mock_existing_user = WebUser( + username="existinguser@test.com", + _id="existinguser123", + domain="test-domain", +) + +mock_existing_pending_invitation = Invitation( + email="existinguser@test.com", + domain="test-domain", +) + @patch_query @patch("corehq.apps.users.models.WebUser.get_by_username", return_value=None) -def test_minimal_valid_form(mock_web_user): +@patch("corehq.apps.users.models.WebUser.by_domain") +@patch("corehq.apps.users.models.Invitation.by_domain") +def test_minimal_valid_form(mock_web_user, mock_by_domain, mock_invitation): form = create_form() - assert form.is_valid(), form.errors @patch_query @patch("corehq.apps.users.models.WebUser.get_by_username", return_value=None) -def test_form_is_invalid_when_invite_existing_email_with_case_mismatch(mock_web_user): +@patch("corehq.apps.users.models.WebUser.by_domain") +@patch("corehq.apps.users.models.Invitation.by_domain", return_value=[mock_existing_pending_invitation]) +def test_form_is_invalid_when_invite_existing_email_with_case_mismatch( + mock_web_user, mock_by_domain, mock_invitation +): form = create_form( - {"email": "test@TEST.com"}, - excluded_emails=["TEST@test.com"], + {"email": "existinguser@TEST.com"}, + ) + + msg = "this email address is already in this project" + assert not form.is_valid() + assert form.errors["email"] == [Regex(msg)], form.errors + + +@patch_query +@patch("corehq.apps.users.models.WebUser.get_by_username", return_value=None) +@patch("corehq.apps.users.models.WebUser.by_domain", return_value=[mock_existing_user]) +@patch("corehq.apps.users.models.Invitation.by_domain") +def test_form_is_invalid_when_existing_user_with_case_mismatch(mock_web_user, mock_by_domain, mock_invitation): + form = create_form( + {"email": "existinguser@TEST.com"}, ) msg = "this email address is already in this project" @@ -43,7 +72,9 @@ def test_form_is_invalid_when_invite_existing_email_with_case_mismatch(mock_web_ @patch_query @patch("corehq.apps.users.models.WebUser.get_by_username", return_value=mock_couch_user) -def test_form_is_invalid_when_invite_deactivated_user(mock_web_user): +@patch("corehq.apps.users.models.WebUser.by_domain") +@patch("corehq.apps.users.models.Invitation.by_domain") +def test_form_is_invalid_when_invite_deactivated_user(mock_web_user, mock_by_domain, mock_invitation): mock_web_user.is_active = False form = create_form( {"email": "test@TEST.com"}, @@ -57,6 +88,7 @@ def test_form_is_invalid_when_invite_deactivated_user(mock_web_user): def create_form(data=None, **kw): form_defaults = {"email": "test@test.com", "role": "admin"} request = RequestFactory().post("/", form_defaults | (data or {})) + request.couch_user = mock_couch_user defaults = { "domain": "test", "request": request, From 8fe97bca86076e26c19d92205c5ee9043cabdbb9 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Tue, 26 Nov 2024 17:48:43 -0800 Subject: [PATCH 25/45] rename since only one error is returned, not multiple --- corehq/apps/registration/forms.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 09f6f25f6e20..92c23c52c25a 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -594,9 +594,9 @@ def clean_email(self): email = self.cleaned_data['email'].strip() from corehq.apps.registration.validation import AdminInvitesUserFormValidator - errors = AdminInvitesUserFormValidator.validate_email(self.domain, email, self.request.method == 'POST') - if errors: - raise forms.ValidationError(errors) + error = AdminInvitesUserFormValidator.validate_email(self.domain, email, self.request.method == 'POST') + if error: + raise forms.ValidationError(error) return email def clean(self): @@ -623,13 +623,13 @@ def clean(self): cleaned_data['custom_user_data'] = get_prefixed(custom_user_data, self.custom_data.prefix) from corehq.apps.registration.validation import AdminInvitesUserFormValidator - errors = AdminInvitesUserFormValidator.validate_parameters( + error = AdminInvitesUserFormValidator.validate_parameters( self.domain, self.request.couch_user, cleaned_data.keys() ) - if errors: - raise forms.ValidationError(errors) + if error: + raise forms.ValidationError(error) return cleaned_data def _initialize_tableau_fields(self, data, domain): From 998a2213a40025b74d9b22dacfdd5df475cfc8d0 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 27 Nov 2024 18:02:39 -0800 Subject: [PATCH 26/45] separate conditionally allowed headers since allowed_headers is globally defined and the conditionally allowed headers should not be preserved upon additional calls to check_header --- corehq/apps/user_importer/importer.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/corehq/apps/user_importer/importer.py b/corehq/apps/user_importer/importer.py index d54ca8fe8a93..9e8a6fe7ab1c 100644 --- a/corehq/apps/user_importer/importer.py +++ b/corehq/apps/user_importer/importer.py @@ -76,6 +76,7 @@ def check_headers(user_specs, domain, upload_couch_user, is_web_upload=False): messages = [] headers = set(user_specs.fieldnames) + conditionally_allowed_headers = set() # Backwards warnings for (old_name, new_name) in old_headers.items(): @@ -87,22 +88,22 @@ def check_headers(user_specs, domain, upload_couch_user, is_web_upload=False): headers.discard(old_name) if DOMAIN_PERMISSIONS_MIRROR.enabled(domain): - allowed_headers.add('domain') + conditionally_allowed_headers.add('domain') if not is_web_upload and EnterpriseMobileWorkerSettings.is_domain_using_custom_deactivation(domain): - allowed_headers.add('deactivate_after') + conditionally_allowed_headers.add('deactivate_after') if TABLEAU_USER_SYNCING.enabled(domain) and upload_couch_user.has_permission( domain, get_permission_name(HqPermissions.edit_user_tableau_config) ): - allowed_headers.update({'tableau_role', 'tableau_groups'}) + conditionally_allowed_headers.update({'tableau_role', 'tableau_groups'}) elif "tableau_role" in headers or "tableau_groups" in headers: messages.append(_( "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau" "User Syncing is enabled can upload files with 'Tableau Role and/or 'Tableau Groups' fields." )) - illegal_headers = headers - allowed_headers + illegal_headers = headers - allowed_headers - conditionally_allowed_headers if is_web_upload: missing_headers = web_required_headers - headers From 6366ebf3ef386ef7ecd15d066ecaac738d171496 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 27 Nov 2024 18:04:12 -0800 Subject: [PATCH 27/45] fix typo --- corehq/apps/user_importer/importer.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/user_importer/importer.py b/corehq/apps/user_importer/importer.py index 9e8a6fe7ab1c..4974c5d96c64 100644 --- a/corehq/apps/user_importer/importer.py +++ b/corehq/apps/user_importer/importer.py @@ -99,8 +99,8 @@ def check_headers(user_specs, domain, upload_couch_user, is_web_upload=False): conditionally_allowed_headers.update({'tableau_role', 'tableau_groups'}) elif "tableau_role" in headers or "tableau_groups" in headers: messages.append(_( - "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau" - "User Syncing is enabled can upload files with 'Tableau Role and/or 'Tableau Groups' fields." + "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau " + "User Syncing is enabled can upload files with 'Tableau Role' and/or 'Tableau Groups' fields." )) illegal_headers = headers - allowed_headers - conditionally_allowed_headers From 34e4dc726f5ac5fd64f3861c1ca6e963bc599ef6 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 29 Nov 2024 11:58:20 -0800 Subject: [PATCH 28/45] update test couch_user is already included in the request so the test is updated to reflect that. The couch_user is now used in `bulk_user_upload_api` so the test needs to be updated --- corehq/apps/users/tests/test_views.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/corehq/apps/users/tests/test_views.py b/corehq/apps/users/tests/test_views.py index 4e183e4e6957..8bf670eff5c0 100644 --- a/corehq/apps/users/tests/test_views.py +++ b/corehq/apps/users/tests/test_views.py @@ -629,6 +629,13 @@ def test_cant_upload_multiple_files(self): class BaseUploadUserTest(TestCase): + + mock_couch_user = WebUser( + username="testuser", + _id="user123", + domain="test-domain", + ) + def setUp(self): self.domain = 'test-domain' self.factory = RequestFactory() @@ -651,6 +658,7 @@ def test_post_success(self, mock_get_workbook, mock_process_workbook, mock_uploa mock_reverse.return_value = '/success/' request = self.factory.post('/', {'bulk_upload_file': Mock()}) + request.couch_user = self.mock_couch_user response = self.view.post(request) mock_reverse.assert_called_once_with( From 8d30494213c69cf30d86867e3ae1ec3e70feba8d Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 29 Nov 2024 15:05:11 -0800 Subject: [PATCH 29/45] test TableauRoleValidation and TableauGroupsValidation --- .../user_importer/tests/test_validators.py | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index bfa01ae02a5c..61bce2c6aeb8 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -8,6 +8,7 @@ from corehq.apps.domain.shortcuts import create_domain from corehq.apps.locations.tests.util import LocationHierarchyTestCase, restrict_user_by_location +from corehq.apps.reports.models import TableauUser, TableauServer from corehq.apps.user_importer.exceptions import UserUploadError from corehq.apps.user_importer.importer import SiteCodeToLocationCache from corehq.apps.user_importer.validation import ( @@ -27,6 +28,8 @@ LocationValidator, _get_invitation_or_editable_user, CustomDataValidator, + TableauRoleValidator, + TableauGroupsValidator, ) from corehq.apps.users.dbaccessors import delete_all_users from corehq.apps.users.models import CommCareUser, HqPermissions, Invitation, WebUser @@ -689,3 +692,63 @@ def test_get_invitation_or_editable_user(self): spec = {} self.assertEqual(None, _get_invitation_or_editable_user(spec, True, self.domain).editable_user) self.assertEqual(None, _get_invitation_or_editable_user(spec, False, self.domain).editable_user) + + +class TestTableauRoleValidator(TestCase): + domain = 'test-domain' + + def test_valid_role(self): + validator = TableauRoleValidator(self.domain) + spec = {'tableau_role': TableauUser.Roles.EXPLORER.value} + self.assertIsNone(validator.validate_spec(spec)) + + def test_invalid_role(self): + validator = TableauRoleValidator(self.domain) + spec = {'tableau_role': 'invalid_role'} + expected_error = TableauRoleValidator._error_message.format( + 'invalid_role', ', '.join([e.value for e in TableauUser.Roles]) + ) + self.assertEqual(validator.validate_spec(spec), expected_error) + + def test_no_role(self): + validator = TableauRoleValidator(self.domain) + spec = {} + self.assertIsNone(validator.validate_spec(spec)) + + +class TestTableauGroupsValidator(TestCase): + domain = 'test-domain' + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.allowed_groups = ['group1', 'group2'] + cls.tableau_server = TableauServer.objects.create( + domain=cls.domain, + allowed_tableau_groups=cls.allowed_groups + ) + cls.addClassCleanup(cls.tableau_server.delete) + cls.all_specs = [{'tableau_groups': 'group1,group2'}] + + def test_valid_groups(self): + validator = TableauGroupsValidator(self.domain, self.all_specs) + spec = {'tableau_groups': 'group1,group2'} + self.assertIsNone(validator.validate_spec(spec)) + + def test_invalid_groups(self): + validator = TableauGroupsValidator(self.domain, self.all_specs) + spec = {'tableau_groups': 'group1,invalid_group'} + expected_error = TableauGroupsValidator._error_message.format( + 'invalid_group', ', '.join(self.allowed_groups) + ) + self.assertEqual(validator.validate_spec(spec), expected_error) + + def test_no_groups(self): + validator = TableauGroupsValidator(self.domain, self.all_specs) + spec = {} + self.assertIsNone(validator.validate_spec(spec)) + + def test_empty_groups(self): + validator = TableauGroupsValidator(self.domain, self.all_specs) + spec = {'tableau_groups': ''} + self.assertIsNone(validator.validate_spec(spec)) From eba62f332958a3b10cdc917b605f6feaf325fdda Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 29 Nov 2024 15:19:38 -0800 Subject: [PATCH 30/45] fix import --- corehq/apps/api/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/api/validation.py b/corehq/apps/api/validation.py index ba010bc256b6..a0e24dd40ea2 100644 --- a/corehq/apps/api/validation.py +++ b/corehq/apps/api/validation.py @@ -2,11 +2,11 @@ from corehq.apps.custom_data_fields.models import CustomDataFieldsDefinition from corehq.apps.reports.util import get_allowed_tableau_groups_for_domain +from corehq.apps.user_importer.importer import SiteCodeToLocationCache from corehq.apps.user_importer.validation import ( RoleValidator, ProfileValidator, LocationValidator, - SiteCodeToLocationCache, TableauGroupsValidator, TableauRoleValidator, CustomDataValidator, From d9f55a177d6d39110e468b516655337688deb8be Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 29 Nov 2024 20:40:16 -0800 Subject: [PATCH 31/45] test: new tests for validation of Web User Resourcec --- corehq/apps/api/tests/test_validation.py | 107 +++++++++++++++++++++++ corehq/apps/api/validation.py | 9 +- 2 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 corehq/apps/api/tests/test_validation.py diff --git a/corehq/apps/api/tests/test_validation.py b/corehq/apps/api/tests/test_validation.py new file mode 100644 index 000000000000..ba20f84ed5eb --- /dev/null +++ b/corehq/apps/api/tests/test_validation.py @@ -0,0 +1,107 @@ +from django.test import TestCase +from unittest.mock import patch + +from corehq.apps.api.validation import WebUserResourceValidator +from corehq.apps.domain.models import Domain +from corehq.apps.users.models import WebUser +from corehq.util.test_utils import flag_enabled, flag_disabled + + +class TestWebUserResourceValidator(TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.domain = Domain(name="test-domain", is_active=True) + cls.domain.save() + cls.addClassCleanup(cls.domain.delete) + cls.requesting_user = WebUser.create(cls.domain.name, "test@example.com", "123", None, None) + cls.validator = WebUserResourceValidator(cls.domain.name, cls.requesting_user) + + @classmethod + def tearDownClass(cls): + cls.requesting_user.delete(None, None) + super().tearDownClass() + + def test_validate_parameters(self): + params = {"email": "test@example.com", "role": "Admin"} + self.assertIsNone(self.validator.validate_parameters(params)) + + invalid_params = {"invalid_param": "value"} + self.assertEqual(self.validator.validate_parameters(invalid_params), "Invalid parameter(s): invalid_param") + + @flag_enabled('TABLEAU_USER_SYNCING') + @patch('corehq.apps.users.models.WebUser.has_permission', return_value=True) + def test_validate_parameters_with_tableau_edit_permission(self, mock_has_permission): + params = {"email": "test@example.com", "role": "Admin", "tableau_role": "Viewer"} + self.assertIsNone(self.validator.validate_parameters(params)) + + @flag_disabled('TABLEAU_USER_SYNCING') + @patch('corehq.apps.users.models.WebUser.has_permission', return_value=False) + def test_validate_parameters_without_tableau_edit_permission(self, mock_has_permission): + params = {"email": "test@example.com", "role": "Admin", "tableau_role": "Viewer"} + self.assertEqual(self.validator.validate_parameters(params), + "You do not have permission to edit Tableau Configuration.") + + @patch('corehq.apps.registration.validation.domain_has_privilege', return_value=True) + def test_validate_parameters_with_profile_permission(self, mock_domain_has_privilege): + params = {"email": "test@example.com", "role": "Admin", "profile": "some_profile"} + self.assertIsNone(self.validator.validate_parameters(params)) + + @patch('corehq.apps.accounting.utils.domain_has_privilege', return_value=False) + def test_validate_parameters_without_profile_permission(self, mock_domain_has_privilege): + params = {"email": "test@example.com", "role": "Admin", "profile": "some_profile"} + self.assertEqual(self.validator.validate_parameters(params), + "This domain does not have user profile privileges.") + + @patch('corehq.apps.registration.validation.domain_has_privilege', return_value=True) + def test_validate_parameters_with_location_privilege(self, mock_domain_has_privilege): + params = {"email": "test@example.com", "role": "Admin", "primary_location": "some_location"} + self.assertIsNone(self.validator.validate_parameters(params)) + params = {"email": "test@example.com", "role": "Admin", "assigned_locations": "some_location"} + self.assertIsNone(self.validator.validate_parameters(params)) + + @patch('corehq.apps.registration.validation.domain_has_privilege', return_value=False) + def test_validate_parameters_without_location_privilege(self, mock_domain_has_privilege): + params = {"email": "test@example.com", "role": "Admin", "primary_location": "some_location"} + self.assertEqual(self.validator.validate_parameters(params), + "This domain does not have locations privileges.") + + params = {"email": "test@example.com", "role": "Admin", "assigned_locations": "some_location"} + self.assertEqual(self.validator.validate_parameters(params), + "This domain does not have locations privileges.") + + def test_validate_email(self): + self.assertIsNone(self.validator.validate_email("newtest@example.com", True)) + + self.assertEqual(self.validator.validate_email("test@example.com", True), + "A user with this email address is already in " + "this project or has a pending invitation.") + + deactivated_user = WebUser.create(self.domain.name, "deactivated@example.com", "123", None, None) + deactivated_user.is_active = False + deactivated_user.save() + self.assertEqual(self.validator.validate_email("deactivated@example.com", True), + "A user with this email address is deactivated. ") + + def test_validate_locations(self): + with patch('corehq.apps.user_importer.validation.LocationValidator.validate_spec') as mock_validate_spec: + mock_validate_spec.return_value = None + self.assertIsNone(self.validator.validate_locations(self.requesting_user.username, + ["loc1", "loc2"], "loc1")) + + expected_spec = { + 'location_code': ["loc1", "loc2"], + 'username': self.requesting_user.username + } + mock_validate_spec.assert_called_once_with(expected_spec) + + self.assertEqual( + self.validator.validate_locations(self.requesting_user.username, ["loc1", "loc2"], "loc3"), + "Primary location must be one of the user's locations" + ) + + self.assertEqual( + self.validator.validate_locations(self.requesting_user.username, ["loc1", "loc2"], ""), + "Primary location can't be empty if the user has any locations set" + ) diff --git a/corehq/apps/api/validation.py b/corehq/apps/api/validation.py index a0e24dd40ea2..6c57ce85ccd2 100644 --- a/corehq/apps/api/validation.py +++ b/corehq/apps/api/validation.py @@ -37,7 +37,12 @@ def location_cache(self): return SiteCodeToLocationCache(self.domain) def validate_parameters(self, parameters): - AdminInvitesUserFormValidator.validate_parameters(self.domain, self.requesting_user, parameters) + allowed_params = ['email', 'role', 'primary_location', 'assigned_locations', + 'profile', 'custom_user_data', 'tableau_role', 'tableau_groups'] + invalid_params = [param for param in parameters if param not in allowed_params] + if invalid_params: + return f"Invalid parameter(s): {', '.join(invalid_params)}" + return AdminInvitesUserFormValidator.validate_parameters(self.domain, self.requesting_user, parameters) def validate_role(self, role): spec = {'role': role} @@ -62,7 +67,7 @@ def validate_locations(self, editable_user, assigned_location_codes, primary_loc return error location_validator = LocationValidator(self.domain, self.requesting_user, self.location_cache, True) - location_codes = assigned_location_codes + [primary_location_code] + location_codes = list(set(assigned_location_codes + [primary_location_code])) spec = {'location_code': location_codes, 'username': editable_user} return location_validator.validate_spec(spec) From 427420a786b86c79eeeda68ac0e9f5af7542f678 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Fri, 29 Nov 2024 22:47:07 -0800 Subject: [PATCH 32/45] update test so that order doesn't matter for the listed "illegal column headers" --- corehq/apps/users/tests/test_views.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/corehq/apps/users/tests/test_views.py b/corehq/apps/users/tests/test_views.py index 8bf670eff5c0..0ef718b55c6f 100644 --- a/corehq/apps/users/tests/test_views.py +++ b/corehq/apps/users/tests/test_views.py @@ -4,6 +4,7 @@ from io import BytesIO from openpyxl import Workbook from unittest.mock import patch, Mock +import re from django.http import Http404, HttpResponseRedirect from django.test import TestCase, Client @@ -578,15 +579,14 @@ def test_tableau_role_and_groups_headers(self): file.seek(0) response = self._make_post_request(file) self.assertEqual(response.status_code, 400) - expected_response = { - 'success': False, - 'message': ( - "Only users with 'Manage Tableau Configuration' edit permission in domains where Tableau " - "User Syncing is enabled can upload files with 'Tableau Role' and/or 'Tableau Groups' fields." - "\nThe following are illegal column headers: tableau_groups, tableau_role." - ), - } - self.assertEqual(response.json(), expected_response) + + expected_pattern = re.compile( + r"Only users with 'Manage Tableau Configuration' edit permission in domains " + r"where Tableau User Syncing is enabled can upload files with 'Tableau Role' " + r"and/or 'Tableau Groups' fields\.\nThe following are illegal column headers: " + r"(?:tableau_groups, tableau_role|tableau_role, tableau_groups)\.", + ) + self.assertRegex(response.json()['message'], expected_pattern) @patch('corehq.apps.users.views.mobile.users.BaseUploadUser.upload_users') def test_user_upload_error(self, mock_upload_users): From c2df91611cfc75af5392e71486b3711f26525965 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Sat, 30 Nov 2024 19:01:16 -0800 Subject: [PATCH 33/45] update test so it doesn't matter the order of the arguments --- corehq/apps/api/tests/test_validation.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/corehq/apps/api/tests/test_validation.py b/corehq/apps/api/tests/test_validation.py index ba20f84ed5eb..32a79d48bf64 100644 --- a/corehq/apps/api/tests/test_validation.py +++ b/corehq/apps/api/tests/test_validation.py @@ -90,11 +90,9 @@ def test_validate_locations(self): self.assertIsNone(self.validator.validate_locations(self.requesting_user.username, ["loc1", "loc2"], "loc1")) - expected_spec = { - 'location_code': ["loc1", "loc2"], - 'username': self.requesting_user.username - } - mock_validate_spec.assert_called_once_with(expected_spec) + actual_spec = mock_validate_spec.call_args[0][0] + self.assertEqual(actual_spec['username'], self.requesting_user.username) + self.assertCountEqual(actual_spec['location_code'], ["loc1", "loc2"]) self.assertEqual( self.validator.validate_locations(self.requesting_user.username, ["loc1", "loc2"], "loc3"), From d3b2d3a38f9574d0bf65625d547c341cd6b4b88f Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 11:46:46 -0800 Subject: [PATCH 34/45] delete unused error message. It is now handled in validate_assigned_locations_has_users --- corehq/apps/user_importer/validation.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 1f107535ab76..5edfc78e12e8 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -505,8 +505,6 @@ class LocationValidator(ImportValidator): error_message_user_access = _("Based on your locations you do not have permission to edit this user or user " "invitation") error_message_location_access = _("You do not have permission to assign or remove these locations: {}") - error_message_location_not_has_users = _("These locations cannot have users assigned because of their " - "organization level settings: {}.") def __init__(self, domain, upload_user, location_cache, is_web_user_import): super().__init__(domain) From eff4a17b1c6fce40da85ae684f55fdd5ad80857a Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 12:10:13 -0800 Subject: [PATCH 35/45] correct patch, path should be where function is used --- corehq/apps/api/tests/test_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/api/tests/test_validation.py b/corehq/apps/api/tests/test_validation.py index 32a79d48bf64..46f20537b696 100644 --- a/corehq/apps/api/tests/test_validation.py +++ b/corehq/apps/api/tests/test_validation.py @@ -48,7 +48,7 @@ def test_validate_parameters_with_profile_permission(self, mock_domain_has_privi params = {"email": "test@example.com", "role": "Admin", "profile": "some_profile"} self.assertIsNone(self.validator.validate_parameters(params)) - @patch('corehq.apps.accounting.utils.domain_has_privilege', return_value=False) + @patch('corehq.apps.registration.validation.domain_has_privilege', return_value=False) def test_validate_parameters_without_profile_permission(self, mock_domain_has_privilege): params = {"email": "test@example.com", "role": "Admin", "profile": "some_profile"} self.assertEqual(self.validator.validate_parameters(params), From 6f0ec77e5a09ff9b76e0f79a70aa54cfe1ca9af7 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 12:13:56 -0800 Subject: [PATCH 36/45] For form validation, clean only occurs if request is post so the "is_post" parameter is redundant. Also, EmailField already builds in EmailValidator. So the additional validation check done in the bulk import EmailValidator is redundant when cleaning email from the form --- corehq/apps/api/validation.py | 9 ++++++++- corehq/apps/registration/forms.py | 2 +- corehq/apps/registration/validation.py | 28 ++++++++++---------------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/corehq/apps/api/validation.py b/corehq/apps/api/validation.py index 6c57ce85ccd2..65a77b0d78a2 100644 --- a/corehq/apps/api/validation.py +++ b/corehq/apps/api/validation.py @@ -10,6 +10,7 @@ TableauGroupsValidator, TableauRoleValidator, CustomDataValidator, + EmailValidator, ) from corehq.apps.users.validation import validate_primary_location_assignment from corehq.apps.registration.validation import AdminInvitesUserFormValidator @@ -59,7 +60,13 @@ def validate_custom_data(self, custom_data, profile_name): return custom_data_validator.validate_spec(spec) def validate_email(self, email, is_post): - return AdminInvitesUserFormValidator.validate_email(self.domain, email, is_post) + if is_post: + error = AdminInvitesUserFormValidator.validate_email(self.domain, email, is_post) + if error: + return error + email_validator = EmailValidator(self.domain, 'email') + spec = {'email': email} + return email_validator.validate_spec(spec) def validate_locations(self, editable_user, assigned_location_codes, primary_location_code): error = validate_primary_location_assignment(primary_location_code, assigned_location_codes) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index 92c23c52c25a..c570db0839b6 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -594,7 +594,7 @@ def clean_email(self): email = self.cleaned_data['email'].strip() from corehq.apps.registration.validation import AdminInvitesUserFormValidator - error = AdminInvitesUserFormValidator.validate_email(self.domain, email, self.request.method == 'POST') + error = AdminInvitesUserFormValidator.validate_email(self.domain, email) if error: raise forms.ValidationError(error) return email diff --git a/corehq/apps/registration/validation.py b/corehq/apps/registration/validation.py index 3480018bcf01..9287c941cef9 100644 --- a/corehq/apps/registration/validation.py +++ b/corehq/apps/registration/validation.py @@ -2,7 +2,6 @@ from corehq import privileges from corehq.apps.accounting.utils import domain_has_privilege -from corehq.apps.user_importer.validation import EmailValidator from corehq.apps.users.models import WebUser, Invitation from corehq.toggles import TABLEAU_USER_SYNCING @@ -27,19 +26,14 @@ def validate_parameters(domain, upload_user, parameters): return _("This domain does not have locations privileges.") @staticmethod - def validate_email(domain, email, is_post): - if is_post: - current_users = [user.username.lower() for user in WebUser.by_domain(domain)] - pending_invites = [di.email.lower() for di in Invitation.by_domain(domain)] - current_users_and_pending_invites = current_users + pending_invites - - if email.lower() in current_users_and_pending_invites: - return _("A user with this email address is already in " - "this project or has a pending invitation.") - web_user = WebUser.get_by_username(email) - if web_user and not web_user.is_active: - return _("A user with this email address is deactivated. ") - - email_validator = EmailValidator(domain, 'email') - spec = {'email': email} - return email_validator.validate_spec(spec) + def validate_email(domain, email): + current_users = [user.username.lower() for user in WebUser.by_domain(domain)] + pending_invites = [di.email.lower() for di in Invitation.by_domain(domain)] + current_users_and_pending_invites = current_users + pending_invites + + if email.lower() in current_users_and_pending_invites: + return _("A user with this email address is already in " + "this project or has a pending invitation.") + web_user = WebUser.get_by_username(email) + if web_user and not web_user.is_active: + return _("A user with this email address is deactivated. ") From ab7e8a7f9eb90fd2945da9210054b5040be79641 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 13:33:07 -0800 Subject: [PATCH 37/45] fix argument --- corehq/apps/api/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/corehq/apps/api/validation.py b/corehq/apps/api/validation.py index 65a77b0d78a2..e0a1dad0deec 100644 --- a/corehq/apps/api/validation.py +++ b/corehq/apps/api/validation.py @@ -61,7 +61,7 @@ def validate_custom_data(self, custom_data, profile_name): def validate_email(self, email, is_post): if is_post: - error = AdminInvitesUserFormValidator.validate_email(self.domain, email, is_post) + error = AdminInvitesUserFormValidator.validate_email(self.domain, email) if error: return error email_validator = EmailValidator(self.domain, 'email') From d698b22f463e8de7dbb117e13cf54e39b2f5faa0 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 14:15:10 -0800 Subject: [PATCH 38/45] fix test - property was moved out of the class --- corehq/apps/user_importer/tests/test_validators.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index 61bce2c6aeb8..0ef2202e0bf3 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -407,7 +407,11 @@ def test_location_not_has_users(self): 'location_code': [self.locations['Cambridge'].site_code, self.locations['Middlesex'].site_code]} validation_result = self.validator.validate_spec(user_spec) - assert validation_result == self.validator.error_message_location_not_has_users.format( + error_message_location_not_has_users = ( + "These locations cannot have users assigned because of their " + "organization level settings: {}." + ) + assert validation_result == error_message_location_not_has_users.format( self.locations['Cambridge'].site_code) @classmethod From 11edd769a1c338a0241457d7e77deeaad0e8bed0 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 14:51:33 -0800 Subject: [PATCH 39/45] avoid regenerating list of roles on each call to reduce memory --- corehq/apps/user_importer/validation.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 5edfc78e12e8..4c8e031d3616 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -148,6 +148,7 @@ def validate_spec(self, spec): class TableauRoleValidator(ImportValidator): _error_message = _("Invalid tableau role: '{}'. Please choose one of the following: {}") + valid_role_options = [e.value for e in TableauUser.Roles] def __init__(self, domain): super().__init__(domain) @@ -158,9 +159,8 @@ def validate_spec(self, spec): @classmethod def validate_tableau_role(cls, tableau_role): - valid_role_options = [e.value for e in TableauUser.Roles] - if tableau_role is not None and tableau_role not in valid_role_options: - return cls._error_message.format(tableau_role, ', '.join(valid_role_options)) + if tableau_role is not None and tableau_role not in cls.valid_role_options: + return cls._error_message.format(tableau_role, ', '.join(cls.valid_role_options)) class TableauGroupsValidator(ImportValidator): From a5ee2070e26869c807406c7c234104833dad4b76 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 15:25:45 -0800 Subject: [PATCH 40/45] bug: needs to check user has access to existing locations when all locations are being removed locs_ids_being_assigned is empty/falsy) --- .../user_importer/tests/test_validators.py | 9 +++++ corehq/apps/user_importer/validation.py | 36 +++++++++---------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index 0ef2202e0bf3..9afe1158715d 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -398,6 +398,15 @@ def test_cant_remove_location(self): assert validation_result == self.validator.error_message_location_access.format( self.locations['Suffolk'].site_code) + def test_cant_remove_all_locations(self): + self.editable_user.reset_locations(self.domain, [self.locations['Suffolk'].location_id, + self.locations['Cambridge'].location_id]) + user_spec = {'username': self.editable_user.username, + 'location_code': []} + validation_result = self.validator.validate_spec(user_spec) + assert validation_result == self.validator.error_message_location_access.format( + self.locations['Suffolk'].site_code) + @flag_enabled('USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION') def test_location_not_has_users(self): self.editable_user.reset_locations(self.domain, [self.locations['Middlesex'].location_id]) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 4c8e031d3616..39b6941ad2a5 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -513,22 +513,21 @@ def __init__(self, domain, upload_user, location_cache, is_web_user_import): self.is_web_user_import = is_web_user_import def validate_spec(self, spec): - locs_being_assigned = self._get_locs_ids_being_assigned(spec) - user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) + if 'location_code' in spec: + locs_being_assigned = self._get_locs_ids_being_assigned(spec) + user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) - user_access_error = self._validate_uploading_user_access(locs_being_assigned, user_result) - location_cannot_have_users_error = None - if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): - location_cannot_have_users_error = self._validate_location_has_users(locs_being_assigned) - return user_access_error or location_cannot_have_users_error + user_access_error = self._validate_uploading_user_access(locs_being_assigned, user_result) + location_cannot_have_users_error = None + if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): + location_cannot_have_users_error = self._validate_location_has_users(locs_being_assigned) + return user_access_error or location_cannot_have_users_error def _get_locs_ids_being_assigned(self, spec): from corehq.apps.user_importer.importer import find_location_id - locs_ids_being_assigned = [] - if 'location_code' in spec: - location_codes = (spec['location_code'] if isinstance(spec['location_code'], list) - else [spec['location_code']]) - locs_ids_being_assigned = find_location_id(location_codes, self.location_cache) + location_codes = (spec['location_code'] if isinstance(spec['location_code'], list) + else [spec['location_code']]) + locs_ids_being_assigned = find_location_id(location_codes, self.location_cache) return locs_ids_being_assigned def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): @@ -545,13 +544,12 @@ def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): # 2. Ensure the user is only adding the user to/removing from *new locations* that they have permission # to access. - if locs_ids_being_assigned: - problem_location_ids = user_can_change_locations(self.domain, self.upload_user, - current_locs, locs_ids_being_assigned) - if problem_location_ids: - return self.error_message_location_access.format( - ', '.join(SQLLocation.objects.filter( - location_id__in=problem_location_ids).values_list('site_code', flat=True))) + problem_location_ids = user_can_change_locations(self.domain, self.upload_user, + current_locs, locs_ids_being_assigned) + if problem_location_ids: + return self.error_message_location_access.format( + ', '.join(SQLLocation.objects.filter( + location_id__in=problem_location_ids).values_list('site_code', flat=True))) def _validate_location_has_users(self, locs_ids_being_assigned): if not locs_ids_being_assigned: From 33159f52758a9cc37b356648b81ea17e5d0e0059 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 15:30:47 -0800 Subject: [PATCH 41/45] rename function for clarity --- corehq/apps/user_importer/validation.py | 8 ++++---- corehq/apps/users/forms.py | 4 ++-- corehq/apps/users/validation.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 39b6941ad2a5..0e140e436efa 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -22,7 +22,7 @@ from corehq.apps.users.forms import get_mobile_worker_max_username_length from corehq.apps.users.models import CouchUser, Invitation from corehq.apps.users.util import normalize_username, raw_username -from corehq.apps.users.validation import validate_assigned_locations_has_users +from corehq.apps.users.validation import validate_assigned_locations_allow_users from corehq.apps.users.views.mobile.custom_data_fields import UserFieldsView from corehq.apps.users.views.utils import ( user_can_access_invite @@ -520,7 +520,7 @@ def validate_spec(self, spec): user_access_error = self._validate_uploading_user_access(locs_being_assigned, user_result) location_cannot_have_users_error = None if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): - location_cannot_have_users_error = self._validate_location_has_users(locs_being_assigned) + location_cannot_have_users_error = self._validate_locations_allow_users(locs_being_assigned) return user_access_error or location_cannot_have_users_error def _get_locs_ids_being_assigned(self, spec): @@ -551,10 +551,10 @@ def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): ', '.join(SQLLocation.objects.filter( location_id__in=problem_location_ids).values_list('site_code', flat=True))) - def _validate_location_has_users(self, locs_ids_being_assigned): + def _validate_locations_allow_users(self, locs_ids_being_assigned): if not locs_ids_being_assigned: return - return validate_assigned_locations_has_users(self.domain, locs_ids_being_assigned) + return validate_assigned_locations_allow_users(self.domain, locs_ids_being_assigned) class UserRetrievalResult(NamedTuple): diff --git a/corehq/apps/users/forms.py b/corehq/apps/users/forms.py index d47c4c73119b..dad316a36dd2 100644 --- a/corehq/apps/users/forms.py +++ b/corehq/apps/users/forms.py @@ -1110,9 +1110,9 @@ def __init__(self, domain: str, *args, **kwargs): ) def clean_assigned_locations(self): - from corehq.apps.users.validation import validate_assigned_locations_has_users + from corehq.apps.users.validation import validate_assigned_locations_allow_users location_ids = self.data.getlist('assigned_locations') - error = validate_assigned_locations_has_users(self.domain, location_ids) + error = validate_assigned_locations_allow_users(self.domain, location_ids) if error: raise forms.ValidationError(error) return location_ids diff --git a/corehq/apps/users/validation.py b/corehq/apps/users/validation.py index 42ee7eb49358..c498bea1fd0c 100644 --- a/corehq/apps/users/validation.py +++ b/corehq/apps/users/validation.py @@ -63,7 +63,7 @@ def validate_profile_required(profile_name, domain): ) -def validate_assigned_locations_has_users(domain, assigned_location_ids): +def validate_assigned_locations_allow_users(domain, assigned_location_ids): error_message_location_not_has_users = _("These locations cannot have users assigned because of their " "organization level settings: {}.") try: From da73efa366909699486826da8d0a2a2ca675721a Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 15:48:31 -0800 Subject: [PATCH 42/45] remove unneccessary model deletion from test database --- corehq/apps/user_importer/tests/test_validators.py | 1 - 1 file changed, 1 deletion(-) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index 9afe1158715d..e70b009a2064 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -740,7 +740,6 @@ def setUpClass(cls): domain=cls.domain, allowed_tableau_groups=cls.allowed_groups ) - cls.addClassCleanup(cls.tableau_server.delete) cls.all_specs = [{'tableau_groups': 'group1,group2'}] def test_valid_groups(self): From 06dd23401ef7ff306d49b4bf2a9b4184ef8d51b6 Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Wed, 4 Dec 2024 17:12:06 -0800 Subject: [PATCH 43/45] clarify naming to distinguish from the form to invite web users --- corehq/apps/registration/forms.py | 2 +- corehq/apps/users/tests/test_user_invitation.py | 10 +++++----- corehq/apps/users/views/web.py | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/corehq/apps/registration/forms.py b/corehq/apps/registration/forms.py index c570db0839b6..b420a3c9e7b3 100644 --- a/corehq/apps/registration/forms.py +++ b/corehq/apps/registration/forms.py @@ -433,7 +433,7 @@ def clean_eula_confirmed(self): return data -class WebUserInvitationForm(BaseUserInvitationForm): +class AcceptedWebUserInvitationForm(BaseUserInvitationForm): """ Form for a brand new user, before they've created a domain or done anything on CommCare HQ. """ diff --git a/corehq/apps/users/tests/test_user_invitation.py b/corehq/apps/users/tests/test_user_invitation.py index 4603ee62db52..89ebbfc9ae20 100644 --- a/corehq/apps/users/tests/test_user_invitation.py +++ b/corehq/apps/users/tests/test_user_invitation.py @@ -3,7 +3,7 @@ from unittest.mock import Mock, patch from corehq.apps.users.models import Invitation, WebUser -from corehq.apps.users.views.web import UserInvitationView, WebUserInvitationForm +from corehq.apps.users.views.web import UserInvitationView, AcceptedWebUserInvitationForm from django import forms from django.contrib.auth.models import AnonymousUser @@ -17,7 +17,7 @@ class InvitationTestException(Exception): pass -class StubbedWebUserInvitationForm(WebUserInvitationForm): +class StubbedAcceptedWebUserInvitationForm(AcceptedWebUserInvitationForm): def __init__(self, *args, **kwargs): self.request_email = kwargs.pop('request_email', False) @@ -76,7 +76,7 @@ def test_redirect_if_invite_is_already_accepted(self): self.assertEqual("/accounts/login/", response.url) def test_redirect_if_invite_email_does_not_match(self): - form = StubbedWebUserInvitationForm( + form = StubbedAcceptedWebUserInvitationForm( { "email": "other_test@dimagi.com", "full_name": "asdf", @@ -95,7 +95,7 @@ def test_redirect_if_invite_email_does_not_match(self): str(ve.exception), "['You can only sign up with the email address your invitation was sent to.']") - form = WebUserInvitationForm( + form = AcceptedWebUserInvitationForm( { "email": "other_test@dimagi.com", "full_name": "asdf", @@ -110,7 +110,7 @@ def test_redirect_if_invite_email_does_not_match(self): print(form.errors) self.assertTrue(form.is_valid()) - form = WebUserInvitationForm( + form = AcceptedWebUserInvitationForm( { "email": "test@dimagi.com", "full_name": "asdf", diff --git a/corehq/apps/users/views/web.py b/corehq/apps/users/views/web.py index 66b1ca7c6923..e6bae2248e12 100644 --- a/corehq/apps/users/views/web.py +++ b/corehq/apps/users/views/web.py @@ -32,7 +32,7 @@ from corehq.apps.domain.models import Domain from corehq.apps.hqwebapp.views import BasePageView, logout from corehq.apps.locations.permissions import location_restricted_response, location_safe -from corehq.apps.registration.forms import WebUserInvitationForm +from corehq.apps.registration.forms import AcceptedWebUserInvitationForm from corehq.apps.registration.utils import activate_new_user_via_reg_form from corehq.apps.users.audit.change_messages import UserChangeMessage from corehq.apps.users.decorators import require_can_edit_web_users @@ -156,7 +156,7 @@ def __call__(self, request, uuid, **kwargs): idp = IdentityProvider.get_required_identity_provider(invitation.email) if request.method == "POST": - form = WebUserInvitationForm( + form = AcceptedWebUserInvitationForm( request.POST, is_sso=idp is not None, allow_invite_email_only=allow_invite_email_only, @@ -212,7 +212,7 @@ def __call__(self, request, uuid, **kwargs): f"?next={accept_invitation_url}" f"&username={invitation.email}" ) - form = WebUserInvitationForm( + form = AcceptedWebUserInvitationForm( initial={ 'email': invitation.email, }, From 8fee5fe9462dd5d79b35c6ac5837d01cb0797eff Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 5 Dec 2024 10:17:51 -0800 Subject: [PATCH 44/45] correct bug where regardless if locations are being changed, validation needs to be done that uploading user can access the current locations of the editable user or invitation. --- .../user_importer/tests/test_validators.py | 8 +++++ corehq/apps/user_importer/validation.py | 30 +++++++++++++------ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index e70b009a2064..2bdf1f82b831 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -355,6 +355,10 @@ def test_cant_edit_web_user(self): validation_result = self.validator.validate_spec(user_spec) assert validation_result == self.validator.error_message_user_access + user_spec = {'username': self.editable_user.username} + validation_result = self.validator.validate_spec(user_spec) + assert validation_result == self.validator.error_message_user_access + def test_cant_edit_commcare_user(self): self.cc_user_validator = LocationValidator(self.domain, self.upload_user, SiteCodeToLocationCache(self.domain), False) @@ -366,6 +370,10 @@ def test_cant_edit_commcare_user(self): validation_result = self.cc_user_validator.validate_spec(user_spec) assert validation_result == self.validator.error_message_user_access + user_spec = {'user_id': self.editable_cc_user._id} + validation_result = self.cc_user_validator.validate_spec(user_spec) + assert validation_result == self.validator.error_message_user_access + def test_cant_edit_invitation(self): self.invitation = Invitation.objects.create( domain=self.domain, diff --git a/corehq/apps/user_importer/validation.py b/corehq/apps/user_importer/validation.py index 0e140e436efa..0cb2c2bc1ac2 100644 --- a/corehq/apps/user_importer/validation.py +++ b/corehq/apps/user_importer/validation.py @@ -513,15 +513,20 @@ def __init__(self, domain, upload_user, location_cache, is_web_user_import): self.is_web_user_import = is_web_user_import def validate_spec(self, spec): + user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) + user_access_error = self._validate_uploading_user_access_to_editable_user_or_invitation(user_result) + if user_access_error: + return user_access_error + if 'location_code' in spec: locs_being_assigned = self._get_locs_ids_being_assigned(spec) - user_result = _get_invitation_or_editable_user(spec, self.is_web_user_import, self.domain) + current_locs = self._get_current_locs(user_result) - user_access_error = self._validate_uploading_user_access(locs_being_assigned, user_result) + user_location_access_error = self._validate_user_location_permission(current_locs, locs_being_assigned) location_cannot_have_users_error = None if toggles.USH_RESTORE_FILE_LOCATION_CASE_SYNC_RESTRICTION.enabled(self.domain): location_cannot_have_users_error = self._validate_locations_allow_users(locs_being_assigned) - return user_access_error or location_cannot_have_users_error + return user_location_access_error or location_cannot_have_users_error def _get_locs_ids_being_assigned(self, spec): from corehq.apps.user_importer.importer import find_location_id @@ -530,20 +535,27 @@ def _get_locs_ids_being_assigned(self, spec): locs_ids_being_assigned = find_location_id(location_codes, self.location_cache) return locs_ids_being_assigned - def _validate_uploading_user_access(self, locs_ids_being_assigned, user_result): - # 1. Get current locations for user or user invitation and ensure user can edit it + def _get_current_locs(self, user_result): current_locs = [] + if user_result.invitation: + current_locs = user_result.invitation.assigned_locations.all() + elif user_result.editable_user: + current_locs = user_result.editable_user.get_location_ids(self.domain) + return current_locs + + def _validate_uploading_user_access_to_editable_user_or_invitation(self, user_result): + # Get current locations for editable user or user invitation and ensure uploading user + # can access those locations if user_result.invitation: if not user_can_access_invite(self.domain, self.upload_user, user_result.invitation): return self.error_message_user_access.format(user_result.invitation.email) - current_locs = user_result.invitation.assigned_locations.all() elif user_result.editable_user: if not user_can_access_other_user(self.domain, self.upload_user, user_result.editable_user): return self.error_message_user_access.format(user_result.editable_user.username) - current_locs = user_result.editable_user.get_location_ids(self.domain) - # 2. Ensure the user is only adding the user to/removing from *new locations* that they have permission - # to access. + def _validate_user_location_permission(self, current_locs, locs_ids_being_assigned): + # Ensure the uploading user is only adding the user to/removing from *new locations* that + # the uploading user has permission to access. problem_location_ids = user_can_change_locations(self.domain, self.upload_user, current_locs, locs_ids_being_assigned) if problem_location_ids: From 3dfa1577aa301a71525ea56d6bf41efafd5b071c Mon Sep 17 00:00:00 2001 From: Jonathan Tang Date: Thu, 5 Dec 2024 10:21:49 -0800 Subject: [PATCH 45/45] test: tests that invitiation access is checked even if no locations are changed --- corehq/apps/user_importer/tests/test_validators.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/corehq/apps/user_importer/tests/test_validators.py b/corehq/apps/user_importer/tests/test_validators.py index 2bdf1f82b831..8342cfb68e62 100644 --- a/corehq/apps/user_importer/tests/test_validators.py +++ b/corehq/apps/user_importer/tests/test_validators.py @@ -388,6 +388,10 @@ def test_cant_edit_invitation(self): validation_result = self.validator.validate_spec(user_spec) assert validation_result == self.validator.error_message_user_access + user_spec = {'username': self.invitation.email} + validation_result = self.validator.validate_spec(user_spec) + assert validation_result == self.validator.error_message_user_access + def test_cant_add_location(self): self.editable_user.reset_locations(self.domain, [self.locations['Cambridge'].location_id]) user_spec = {'username': self.editable_user.username,