From e6d7f7ebeb31f24456a4f2dd39383bb9653fbf63 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 11 Mar 2019 14:13:56 +0000 Subject: [PATCH] Add a user method to check folder permission User model is the most natural place for a permission check method, however this means that we need to pass the full user object to service model methods and TemplateList instead of user_id. --- app/main/views/conversation.py | 2 +- app/main/views/manage_users.py | 2 +- app/main/views/templates.py | 8 ++++---- app/models/service.py | 28 ++++++++++++++-------------- app/models/template_list.py | 28 ++++++++++++++-------------- app/models/user.py | 6 ++++++ tests/app/models/test_service.py | 4 ++-- 7 files changed, 42 insertions(+), 36 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 83a06cfb7..e9d5dfeb6 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -52,7 +52,7 @@ def conversation_reply( templates_and_folders=TemplateList( current_service, template_folder_id=from_folder, - user_id=current_user.id, + user=current_user, template_type='sms' ), template_folder_path=current_service.get_template_folder_path(from_folder), diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 1a45f3aed..30cab629a 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -100,7 +100,7 @@ def edit_user_permissions(service_id, user_id): service_id, folder_permissions=[ f['id'] for f in current_service.all_template_folders - if user_id in f.get('users_with_permission', []) + if user.has_template_folder_permission(f) ], all_template_folders=current_service.all_template_folders ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index cd26a1806..2e2629a09 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -111,10 +111,10 @@ def start_tour(service_id, template_id): @user_has_permissions() def choose_template(service_id, template_type='all', template_folder_id=None): - template_list = TemplateList(current_service, template_type, template_folder_id, current_user.id) + template_list = TemplateList(current_service, template_type, template_folder_id, current_user) templates_and_folders_form = TemplateAndFoldersSelectionForm( - all_template_folders=current_service.get_user_template_folders(current_user.id), + all_template_folders=current_service.get_user_template_folders(current_user), template_list=template_list, template_type=template_type, allow_adding_letter_template=current_service.has_permission('letter'), @@ -348,7 +348,7 @@ def choose_template_to_copy( services_templates_and_folders=TemplateList( service, template_folder_id=from_folder, - user_id=current_user.id + user=current_user ), template_folder_path=service.get_template_folder_path(from_folder), from_service=service, @@ -361,7 +361,7 @@ def choose_template_to_copy( services_templates_and_folders=TemplateLists([ Service(service) for service in user_api_client.get_services_for_user(current_user) - ], user_id=current_user.id), + ], user=current_user), search_form=SearchByNameForm(), ) diff --git a/app/models/service.py b/app/models/service.py index 613cc07aa..f3839b4f9 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -155,10 +155,10 @@ class Service(): def all_template_ids(self): return {template['id'] for template in self.all_templates} - def get_templates(self, template_type='all', template_folder_id=None, user_id=None): - if user_id and template_folder_id and self.has_permission('edit_folder_permissions'): + def get_templates(self, template_type='all', template_folder_id=None, user=None): + if user and template_folder_id and self.has_permission('edit_folder_permissions'): folder = self.get_template_folder(template_folder_id) - if user_id not in folder.get("users_with_permission", []): + if not user.has_template_folder_permission(folder): return [] if isinstance(template_type, str): @@ -403,7 +403,7 @@ class Service(): def all_template_folder_ids(self): return {folder['id'] for folder in self.all_template_folders} - def get_user_template_folders(self, user_id): + def get_user_template_folders(self, user): """Returns a modified list of folders a user has permission to view For each folder, we do the following: @@ -422,10 +422,10 @@ class Service(): user_folders = [] for folder in self.all_template_folders: - if user_id not in folder.get("users_with_permission", []): + if not user.has_template_folder_permission(folder): continue parent = self.get_template_folder(folder["parent_id"]) - if user_id in parent.get("users_with_permission", []): + if user.has_template_folder_permission(parent): user_folders.append(folder) else: folder_attrs = { @@ -439,14 +439,14 @@ class Service(): else: parent = self.get_template_folder(parent["parent_id"]) folder_attrs["parent_id"] = parent.get("id", None) - if user_id in parent.get("users_with_permission", []): + if user.has_template_folder_permission(parent): break user_folders.append(folder_attrs) return user_folders - def get_template_folders(self, template_type='all', parent_folder_id=None, user_id=None): - if user_id: - folders = self.get_user_template_folders(user_id) + def get_template_folders(self, template_type='all', parent_folder_id=None, user=None): + if user: + folders = self.get_user_template_folders(user) else: folders = self.all_template_folders if parent_folder_id: @@ -456,7 +456,7 @@ class Service(): folder for folder in folders if ( folder['parent_id'] == parent_folder_id - and self.is_folder_visible(folder['id'], template_type, user_id) + and self.is_folder_visible(folder['id'], template_type, user) ) ] @@ -469,7 +469,7 @@ class Service(): } return self._get_by_id(self.all_template_folders, folder_id) - def is_folder_visible(self, template_folder_id, template_type='all', user_id=None): + def is_folder_visible(self, template_folder_id, template_type='all', user=None): if template_type == 'all': return True @@ -478,8 +478,8 @@ class Service(): return True if any( - self.is_folder_visible(child_folder['id'], template_type, user_id) - for child_folder in self.get_template_folders(template_type, template_folder_id, user_id) + self.is_folder_visible(child_folder['id'], template_type, user) + for child_folder in self.get_template_folders(template_type, template_folder_id, user) ): return True diff --git a/app/models/template_list.py b/app/models/template_list.py index ad479b115..678e72285 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -5,28 +5,28 @@ class TemplateList(): service, template_type='all', template_folder_id=None, - user_id=None, + user=None, ): self.service = service self.template_type = template_type self.template_folder_id = template_folder_id - self.user_id = user_id + self.user = user def __iter__(self): for item in self.get_templates_and_folders( - self.template_type, self.template_folder_id, self.user_id, ancestors=[] + self.template_type, self.template_folder_id, self.user, ancestors=[] ): yield item - def get_templates_and_folders(self, template_type, template_folder_id, user_id, ancestors): + def get_templates_and_folders(self, template_type, template_folder_id, user, ancestors): for item in self.service.get_template_folders( - template_type, template_folder_id, user_id, + template_type, template_folder_id, user, ): yield TemplateListFolder( item, folders=self.service.get_template_folders( - template_type, item['id'], user_id + template_type, item['id'], user ), templates=self.service.get_templates( template_type, item['id'] @@ -35,12 +35,12 @@ class TemplateList(): service_id=self.service.id, ) for sub_item in self.get_templates_and_folders( - template_type, item['id'], user_id, ancestors + [item] + template_type, item['id'], user, ancestors + [item] ): yield sub_item for item in self.service.get_templates( - template_type, template_folder_id, user_id + template_type, template_folder_id, user ): yield TemplateListTemplate( item, @@ -59,24 +59,24 @@ class TemplateList(): @property def folder_is_empty(self): return not any(self.get_templates_and_folders( - 'all', self.template_folder_id, self.user_id, [] + 'all', self.template_folder_id, self.user, [] )) class TemplateLists(): - def __init__(self, services, user_id=None): + def __init__(self, services, user=None): self.services = sorted( services, key=lambda service: service.name.lower(), ) - self.user_id = user_id + self.user = user def __iter__(self): if len(self.services) == 1: - for template_or_folder in TemplateList(self.services[0], user_id=self.user_id): + for template_or_folder in TemplateList(self.services[0], user=self.user): yield template_or_folder return @@ -88,11 +88,11 @@ class TemplateLists(): yield template_list_service for service_templates_and_folders in TemplateList( - service, user_id=self.user_id + service, user=self.user ).get_templates_and_folders( template_type='all', template_folder_id=None, - user_id=self.user_id, + user=self.user, ancestors=[template_list_service], ): yield service_templates_and_folders diff --git a/app/models/user.py b/app/models/user.py index ba314471a..ffc88d99d 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -151,6 +151,12 @@ class User(UserMixin): def has_permission_for_service(self, service_id, permission): return permission in self._permissions.get(service_id, []) + def has_template_folder_permission(self, template_folder): + if self.platform_admin: + return True + + return self.id in template_folder.get("users_with_permission", []) + def belongs_to_service(self, service_id): return str(service_id) in self.services diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py index 3ba9063b4..d5ac253c2 100644 --- a/tests/app/models/test_service.py +++ b/tests/app/models/test_service.py @@ -76,7 +76,7 @@ def test_get_user_template_folders_only_returns_folders_visible_to_user( mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service_one['permissions'] = ['edit_folder_permissions'] service = Service(service_one) - result = service.get_user_template_folders(active_user_with_permissions.id) + result = service.get_user_template_folders(active_user_with_permissions) assert result == [ { 'name': "Parent 1 - invisible / 1's Visible child", @@ -120,7 +120,7 @@ def test_get_template_folders_shows_user_folders_when_user_id_passed_in( mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service_one['permissions'] = ['edit_folder_permissions'] service = Service(service_one) - result = service.get_template_folders(user_id=active_user_with_permissions.id) + result = service.get_template_folders(user=active_user_with_permissions) assert result == [ { 'name': "Parent 1 - invisible / 1's Visible child",