From 1127a03c323f8bce7a942864590cc0955c56e607 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:05:11 +0100 Subject: [PATCH 1/6] Move and rename roles_and_permissions.py This file does not represent a model, but rather a set of utilities that are specific to user permissions (vs. service permissions). --- app/main/forms.py | 6 +----- app/main/views/manage_users.py | 2 +- app/models/user.py | 8 ++++---- app/notify_client/invite_api_client.py | 4 ++-- app/notify_client/user_api_client.py | 4 ++-- .../user_permissions.py} | 0 tests/app/main/test_permissions.py | 2 +- 7 files changed, 11 insertions(+), 15 deletions(-) rename app/{models/roles_and_permissions.py => utils/user_permissions.py} (100%) diff --git a/app/main/forms.py b/app/main/forms.py index b8af1c9dc..bd8f572dd 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -56,13 +56,9 @@ from app.main.validators import ( ) from app.models.feedback import PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE from app.models.organisation import Organisation -from app.models.roles_and_permissions import ( - broadcast_permissions, - permissions, - roles, -) from app.utils import merge_jsonlike from app.utils.user import distinct_email_addresses +from app.utils.user_permissions import broadcast_permissions, permissions, roles def get_time_value_and_label(future_time): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 78e7a0f46..13327ded2 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -28,9 +28,9 @@ from app.main.forms import ( PermissionsForm, SearchUsersForm, ) -from app.models.roles_and_permissions import broadcast_permissions, permissions from app.models.user import InvitedUser, User from app.utils.user import is_gov_user, user_has_permissions +from app.utils.user_permissions import broadcast_permissions, permissions @main.route("/services//users") diff --git a/app/models/user.py b/app/models/user.py index a03ecf712..a1d3e03e0 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -10,16 +10,16 @@ from app.event_handlers import ( ) from app.models import JSONModel, ModelList from app.models.organisation import Organisation -from app.models.roles_and_permissions import ( - all_permissions, - translate_permissions_from_db_to_admin_roles, -) from app.models.webauthn_credential import WebAuthnCredentials from app.notify_client import InviteTokenError from app.notify_client.invite_api_client import invite_api_client from app.notify_client.org_invite_api_client import org_invite_api_client from app.notify_client.user_api_client import user_api_client from app.utils.user import is_gov_user +from app.utils.user_permissions import ( + all_permissions, + translate_permissions_from_db_to_admin_roles, +) def _get_service_id_from_view_args(): diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index f77b9f5c1..531488a96 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,8 +1,8 @@ -from app.models.roles_and_permissions import ( +from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache +from app.utils.user_permissions import ( roles, translate_permissions_from_admin_roles_to_db, ) -from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class InviteApiClient(NotifyAdminAPIClient): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index b65836878..f89de2df9 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,9 +1,9 @@ from notifications_python_client.errors import HTTPError -from app.models.roles_and_permissions import ( +from app.notify_client import NotifyAdminAPIClient, cache +from app.utils.user_permissions import ( translate_permissions_from_admin_roles_to_db, ) -from app.notify_client import NotifyAdminAPIClient, cache ALLOWED_ATTRIBUTES = { 'name', diff --git a/app/models/roles_and_permissions.py b/app/utils/user_permissions.py similarity index 100% rename from app/models/roles_and_permissions.py rename to app/utils/user_permissions.py diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index c089907fa..7244d91bd 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -5,7 +5,7 @@ import re import pytest from flask import current_app -from app.models.roles_and_permissions import ( +from app.utils.user_permissions import ( translate_permissions_from_admin_roles_to_db, translate_permissions_from_db_to_admin_roles, ) From f5580b87dcccd3448f25a28ad560fcdb82d9e44c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:07:41 +0100 Subject: [PATCH 2/6] Move tests to match where the code is located These tests are unrelated to the others in test_permissions.py. We should try and structure our tests the same as the code under test so that it's clear where new tests should go. --- tests/app/main/test_permissions.py | 40 ---------------------- tests/app/utils/test_user_permissions.py | 42 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 40 deletions(-) create mode 100644 tests/app/utils/test_user_permissions.py diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 7244d91bd..9c64f2204 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -5,10 +5,6 @@ import re import pytest from flask import current_app -from app.utils.user_permissions import ( - translate_permissions_from_admin_roles_to_db, - translate_permissions_from_db_to_admin_roles, -) from tests import service_json from tests.conftest import ( ORGANISATION_ID, @@ -18,42 +14,6 @@ from tests.conftest import ( ) -@pytest.mark.parametrize('db_roles,admin_roles', [ - ( - ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], - {'approve_broadcasts'}, - ), - ( - ['manage_templates', 'create_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], - {'create_broadcasts', 'manage_templates'}, - ), - ( - ['manage_templates'], - {'manage_templates'}, - ), - ( - ['create_broadcasts'], - set(), - ), - ( - ['send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'], - {'send_messages', 'manage_templates', 'some_unknown_permission'}, - ), -]) -def test_translate_permissions_from_db_to_admin_roles( - db_roles, - admin_roles, -): - roles = translate_permissions_from_db_to_admin_roles(db_roles) - assert roles == admin_roles - - -def test_translate_permissions_from_admin_roles_to_db(): - roles = ['send_messages', 'manage_templates', 'some_unknown_permission'] - db_perms = translate_permissions_from_admin_roles_to_db(roles) - assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'} - - @pytest.mark.parametrize( 'user_services, user_organisations, expected_status, organisation_checked', ( diff --git a/tests/app/utils/test_user_permissions.py b/tests/app/utils/test_user_permissions.py new file mode 100644 index 000000000..9ad639022 --- /dev/null +++ b/tests/app/utils/test_user_permissions.py @@ -0,0 +1,42 @@ +import pytest + +from app.utils.user_permissions import ( + translate_permissions_from_admin_roles_to_db, + translate_permissions_from_db_to_admin_roles, +) + + +@pytest.mark.parametrize('db_roles,admin_roles', [ + ( + ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], + {'approve_broadcasts'}, + ), + ( + ['manage_templates', 'create_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], + {'create_broadcasts', 'manage_templates'}, + ), + ( + ['manage_templates'], + {'manage_templates'}, + ), + ( + ['create_broadcasts'], + set(), + ), + ( + ['send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'], + {'send_messages', 'manage_templates', 'some_unknown_permission'}, + ), +]) +def test_translate_permissions_from_db_to_admin_roles( + db_roles, + admin_roles, +): + roles = translate_permissions_from_db_to_admin_roles(db_roles) + assert roles == admin_roles + + +def test_translate_permissions_from_admin_roles_to_db(): + roles = ['send_messages', 'manage_templates', 'some_unknown_permission'] + db_perms = translate_permissions_from_admin_roles_to_db(roles) + assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'} From a38baa0bd8a9f4164c42c9eda1d54bcd31d31b7e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:15:24 +0100 Subject: [PATCH 3/6] Rename unclear "permissions" attributes These are more than a list of permissions: each item includes the label to use when displaying it as an option on a form. Switching to a name that reflects how the attributes are used will help to avoid confusion when we rename some of the other attributes in the same file in later commits. --- app/main/forms.py | 16 ++++++++++------ app/main/views/manage_users.py | 7 +++++-- app/utils/user_permissions.py | 4 ++-- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index bd8f572dd..9f45bead9 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -58,7 +58,11 @@ from app.models.feedback import PROBLEM_TICKET_TYPE, QUESTION_TICKET_TYPE from app.models.organisation import Organisation from app.utils import merge_jsonlike from app.utils.user import distinct_email_addresses -from app.utils.user_permissions import broadcast_permissions, permissions, roles +from app.utils.user_permissions import ( + broadcast_permission_options, + permission_options, + roles, +) def get_time_value_and_label(future_time): @@ -946,15 +950,15 @@ def filter_by_permissions(valuelist): if valuelist is None: return None else: - return [entry for entry in valuelist if any(entry in role for role in permissions)] + return [entry for entry in valuelist if any(entry in role for role in permission_options)] -# guard against data entries that aren't a role in broadcast_permissions +# guard against data entries that aren't a role in broadcast_permission_options def filter_by_broadcast_permissions(valuelist): if valuelist is None: return None else: - return [entry for entry in valuelist if any(entry in role for role in broadcast_permissions)] + return [entry for entry in valuelist if any(entry in role for role in broadcast_permission_options)] class BasePermissionsForm(StripWhitespaceForm): @@ -985,7 +989,7 @@ class BasePermissionsForm(StripWhitespaceForm): 'Permissions', filters=[filter_by_permissions], choices=[ - (value, label) for value, label in permissions + (value, label) for value, label in permission_options ], param_extensions={ "hint": {"text": "All team members can see sent messages."} @@ -1023,7 +1027,7 @@ class BroadcastPermissionsForm(BasePermissionsForm): permissions_field = GovukCheckboxesField( 'Permissions', choices=[ - (value, label) for value, label in broadcast_permissions + (value, label) for value, label in broadcast_permission_options ], filters=[filter_by_broadcast_permissions], param_extensions={ diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 13327ded2..42d5c5bba 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -30,7 +30,10 @@ from app.main.forms import ( ) from app.models.user import InvitedUser, User from app.utils.user import is_gov_user, user_has_permissions -from app.utils.user_permissions import broadcast_permissions, permissions +from app.utils.user_permissions import ( + broadcast_permission_options, + permission_options, +) @main.route("/services//users") @@ -43,7 +46,7 @@ def manage_users(service_id): show_search_box=(len(current_service.team_members) > 7), form=SearchUsersForm(), permissions=( - broadcast_permissions if current_service.has_permission('broadcast') else permissions + broadcast_permission_options if current_service.has_permission('broadcast') else permission_options ), ) diff --git a/app/utils/user_permissions.py b/app/utils/user_permissions.py index 124b55e8b..c484d0ff1 100644 --- a/app/utils/user_permissions.py +++ b/app/utils/user_permissions.py @@ -13,7 +13,7 @@ roles = { all_permissions = set(roles.keys()) all_database_permissions = set(chain(*roles.values())) -permissions = ( +permission_options = ( ('view_activity', 'See dashboard'), ('send_messages', 'Send messages'), ('manage_templates', 'Add and edit templates'), @@ -21,7 +21,7 @@ permissions = ( ('manage_api_keys', 'Manage API integration'), ) -broadcast_permissions = ( +broadcast_permission_options = ( ('manage_templates', 'Add and edit templates'), ('create_broadcasts', 'Create new alerts'), ('approve_broadcasts', 'Approve alerts'), From ba9865e62e3d447cdbb735d084d6514347243256 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:25:22 +0100 Subject: [PATCH 4/6] Start to remove use of the term "roles" We don't use this term consistently and it's not defined anywhere. Since most of the Admin app deals with user-facing permssions, it's OK to just use the term "permissions". Where both types of permission are present in the same file, we can more clearly distinguish them as "UI permissions" and "DB permissions". --- app/main/forms.py | 4 ++-- app/models/user.py | 4 ++-- app/notify_client/invite_api_client.py | 4 ++-- app/utils/user_permissions.py | 12 ++++++------ 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 9f45bead9..bbb3d91bd 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -59,9 +59,9 @@ from app.models.organisation import Organisation from app.utils import merge_jsonlike from app.utils.user import distinct_email_addresses from app.utils.user_permissions import ( + all_ui_permissions, broadcast_permission_options, permission_options, - roles, ) @@ -1006,7 +1006,7 @@ class BasePermissionsForm(StripWhitespaceForm): **kwargs, **{ "permissions_field": [ - role for role in roles.keys() if user.has_permission_for_service(service_id, role)] + role for role in all_ui_permissions if user.has_permission_for_service(service_id, role)] }, login_authentication=user.auth_type ) diff --git a/app/models/user.py b/app/models/user.py index a1d3e03e0..cd62db52a 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -17,7 +17,7 @@ from app.notify_client.org_invite_api_client import org_invite_api_client from app.notify_client.user_api_client import user_api_client from app.utils.user import is_gov_user from app.utils.user_permissions import ( - all_permissions, + all_ui_permissions, translate_permissions_from_db_to_admin_roles, ) @@ -201,7 +201,7 @@ class User(JSONModel, UserMixin): return self._platform_admin and not session.get('disable_platform_admin_view', False) def has_permissions(self, *permissions, restrict_admin_usage=False, allow_org_user=False): - unknown_permissions = set(permissions) - all_permissions + unknown_permissions = set(permissions) - all_ui_permissions if unknown_permissions: raise TypeError('{} are not valid permissions'.format(list(unknown_permissions))) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 531488a96..b76d7d279 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,6 +1,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache from app.utils.user_permissions import ( - roles, + all_ui_permissions, translate_permissions_from_admin_roles_to_db, ) @@ -48,7 +48,7 @@ class InviteApiClient(NotifyAdminAPIClient): )['data'] def get_count_of_invites_with_permission(self, service_id, permission): - if permission not in roles.keys(): + if permission not in all_ui_permissions: raise TypeError('{} is not a valid permission'.format(permission)) return len([ invited_user for invited_user in self.get_invites_for_service(service_id) diff --git a/app/utils/user_permissions.py b/app/utils/user_permissions.py index c484d0ff1..b9944d532 100644 --- a/app/utils/user_permissions.py +++ b/app/utils/user_permissions.py @@ -1,6 +1,6 @@ from itertools import chain -roles = { +permission_mappings = { 'send_messages': ['send_texts', 'send_emails', 'send_letters'], 'manage_templates': ['manage_templates'], 'manage_service': ['manage_users', 'manage_settings'], @@ -10,8 +10,8 @@ roles = { 'approve_broadcasts': ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], } -all_permissions = set(roles.keys()) -all_database_permissions = set(chain(*roles.values())) +all_ui_permissions = set(permission_mappings.keys()) +all_db_permissions = set(chain(*permission_mappings.values())) permission_options = ( ('view_activity', 'See dashboard'), @@ -35,10 +35,10 @@ def translate_permissions_from_db_to_admin_roles(permissions): A role is returned if all of its database permissions are in the permission list that is passed in. Any permissions in the list that are not database permissions are also returned. """ - unknown_database_permissions = {p for p in permissions if p not in all_database_permissions} + unknown_database_permissions = {p for p in permissions if p not in all_db_permissions} return { - admin_role for admin_role, db_role_list in roles.items() + admin_role for admin_role, db_role_list in permission_mappings.items() if set(db_role_list) <= set(permissions) } | unknown_database_permissions @@ -49,4 +49,4 @@ def translate_permissions_from_admin_roles_to_db(permissions): Looks them up in the roles dict, falling back to just passing through if they're not recognised. """ - return set(chain.from_iterable(roles.get(permission, [permission]) for permission in permissions)) + return set(chain.from_iterable(permission_mappings.get(permission, [permission]) for permission in permissions)) From dcfff87cc056a6560e5fe2e0264c8a36a80b70fb Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:38:45 +0100 Subject: [PATCH 5/6] Continue to remove "roles" terminology This renames the two functions we have to translate between UI and DB permissions, as well as some of their associated variables to make it clearer which kind of permission they contain. --- app/models/user.py | 6 +++--- app/notify_client/invite_api_client.py | 4 ++-- app/notify_client/user_api_client.py | 8 +++---- app/utils/user_permissions.py | 24 +++++++++++---------- tests/app/utils/test_user_permissions.py | 27 +++++++++++++----------- 5 files changed, 36 insertions(+), 33 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index cd62db52a..95278abb6 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -18,7 +18,7 @@ from app.notify_client.user_api_client import user_api_client from app.utils.user import is_gov_user from app.utils.user_permissions import ( all_ui_permissions, - translate_permissions_from_db_to_admin_roles, + translate_permissions_from_db_to_ui, ) @@ -103,7 +103,7 @@ class User(JSONModel, UserMixin): them out later, we'll need to rework this function. """ self._permissions = { - service: translate_permissions_from_db_to_admin_roles(permissions) + service: translate_permissions_from_db_to_ui(permissions) for service, permissions in permissions_by_service.items() } @@ -505,7 +505,7 @@ class InvitedUser(JSONModel): self._permissions = permissions else: self._permissions = permissions.split(',') - self._permissions = translate_permissions_from_db_to_admin_roles(self.permissions) + self._permissions = translate_permissions_from_db_to_ui(self.permissions) @property def from_user(self): diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index b76d7d279..464c3ba9c 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,7 +1,7 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache from app.utils.user_permissions import ( all_ui_permissions, - translate_permissions_from_admin_roles_to_db, + translate_permissions_from_ui_to_db, ) @@ -23,7 +23,7 @@ class InviteApiClient(NotifyAdminAPIClient): 'service': service_id, 'email_address': email_address, 'from_user': invite_from_id, - 'permissions': ','.join(sorted(translate_permissions_from_admin_roles_to_db(permissions))), + 'permissions': ','.join(sorted(translate_permissions_from_ui_to_db(permissions))), 'auth_type': auth_type, 'invite_link_host': self.admin_url, 'folder_permissions': folder_permissions, diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index f89de2df9..379d4b9b7 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,9 +1,7 @@ from notifications_python_client.errors import HTTPError from app.notify_client import NotifyAdminAPIClient, cache -from app.utils.user_permissions import ( - translate_permissions_from_admin_roles_to_db, -) +from app.utils.user_permissions import translate_permissions_from_ui_to_db ALLOWED_ATTRIBUTES = { 'name', @@ -152,7 +150,7 @@ class UserApiClient(NotifyAdminAPIClient): # permissions passed in are the combined admin roles, not db permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) data = { - 'permissions': [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)], + 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], 'folder_permissions': folder_permissions, } @@ -168,7 +166,7 @@ class UserApiClient(NotifyAdminAPIClient): def set_user_permissions(self, user_id, service_id, permissions, folder_permissions=None): # permissions passed in are the combined admin roles, not db permissions data = { - 'permissions': [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)], + 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], } if folder_permissions is not None: diff --git a/app/utils/user_permissions.py b/app/utils/user_permissions.py index b9944d532..cddd87ca4 100644 --- a/app/utils/user_permissions.py +++ b/app/utils/user_permissions.py @@ -28,25 +28,27 @@ broadcast_permission_options = ( ) -def translate_permissions_from_db_to_admin_roles(permissions): +def translate_permissions_from_db_to_ui(db_permissions): """ - Given a list of database permissions, return a set of roles + Given a list of database permissions, return a set of UI permissions - A role is returned if all of its database permissions are in the permission list that is passed in. - Any permissions in the list that are not database permissions are also returned. + A UI permission is returned if all of its DB permissions are in the permission list that is passed in. + Any DB permissions in the list that are not known permissions are also returned. """ - unknown_database_permissions = {p for p in permissions if p not in all_db_permissions} + unknown_database_permissions = set(db_permissions) - all_db_permissions return { - admin_role for admin_role, db_role_list in permission_mappings.items() - if set(db_role_list) <= set(permissions) + ui_permission for ui_permission, db_permissions_for_ui_permission in permission_mappings.items() + if set(db_permissions_for_ui_permission) <= set(db_permissions) } | unknown_database_permissions -def translate_permissions_from_admin_roles_to_db(permissions): +def translate_permissions_from_ui_to_db(ui_permissions): """ - Given a list of admin roles (ie: checkboxes on a permissions edit page for example), return a set of db permissions + Given a list of UI permissions (ie: checkboxes on a permissions edit page), return a set of DB permissions - Looks them up in the roles dict, falling back to just passing through if they're not recognised. + Looks them up in the mapping, falling back to just passing through if they're not recognised. """ - return set(chain.from_iterable(permission_mappings.get(permission, [permission]) for permission in permissions)) + return set(chain.from_iterable( + permission_mappings.get(ui_permission, [ui_permission]) for ui_permission in ui_permissions + )) diff --git a/tests/app/utils/test_user_permissions.py b/tests/app/utils/test_user_permissions.py index 9ad639022..b40994597 100644 --- a/tests/app/utils/test_user_permissions.py +++ b/tests/app/utils/test_user_permissions.py @@ -1,12 +1,12 @@ import pytest from app.utils.user_permissions import ( - translate_permissions_from_admin_roles_to_db, - translate_permissions_from_db_to_admin_roles, + translate_permissions_from_db_to_ui, + translate_permissions_from_ui_to_db, ) -@pytest.mark.parametrize('db_roles,admin_roles', [ +@pytest.mark.parametrize('db_permissions,expected_ui_permissions', [ ( ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], {'approve_broadcasts'}, @@ -28,15 +28,18 @@ from app.utils.user_permissions import ( {'send_messages', 'manage_templates', 'some_unknown_permission'}, ), ]) -def test_translate_permissions_from_db_to_admin_roles( - db_roles, - admin_roles, +def test_translate_permissions_from_db_to_ui( + db_permissions, + expected_ui_permissions, ): - roles = translate_permissions_from_db_to_admin_roles(db_roles) - assert roles == admin_roles + ui_permissions = translate_permissions_from_db_to_ui(db_permissions) + assert ui_permissions == expected_ui_permissions -def test_translate_permissions_from_admin_roles_to_db(): - roles = ['send_messages', 'manage_templates', 'some_unknown_permission'] - db_perms = translate_permissions_from_admin_roles_to_db(roles) - assert db_perms == {'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'} +def test_translate_permissions_from_ui_to_db(): + ui_permissions = ['send_messages', 'manage_templates', 'some_unknown_permission'] + db_permissions = translate_permissions_from_ui_to_db(ui_permissions) + + assert db_permissions == { + 'send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission' + } From 354cd8bb16c7745bb6a76f5644afbff8b413f15c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 22 Jul 2021 14:51:52 +0100 Subject: [PATCH 6/6] Replace remaining uses of the term "role" In one case I did this by refactoring the code to avoid the need for the "role" variable in the first place. --- app/main/forms.py | 14 ++++++++------ app/notify_client/user_api_client.py | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index bbb3d91bd..15dddedec 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -945,20 +945,20 @@ class GovukRadiosFieldWithNoneOption(FieldWithNoneOption, GovukRadiosField): pass -# guard against data entries that aren't a role in permissions +# guard against data entries that aren't a known permission def filter_by_permissions(valuelist): if valuelist is None: return None else: - return [entry for entry in valuelist if any(entry in role for role in permission_options)] + return [entry for entry in valuelist if any(entry in option for option in permission_options)] -# guard against data entries that aren't a role in broadcast_permission_options +# guard against data entries that aren't a known broadcast permission def filter_by_broadcast_permissions(valuelist): if valuelist is None: return None else: - return [entry for entry in valuelist if any(entry in role for role in broadcast_permission_options)] + return [entry for entry in valuelist if any(entry in option for option in broadcast_permission_options)] class BasePermissionsForm(StripWhitespaceForm): @@ -1005,8 +1005,10 @@ class BasePermissionsForm(StripWhitespaceForm): form = cls( **kwargs, **{ - "permissions_field": [ - role for role in all_ui_permissions if user.has_permission_for_service(service_id, role)] + "permissions_field": ( + user.permissions_for_service(service_id) & all_ui_permissions + ) + }, login_authentication=user.auth_type ) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 379d4b9b7..7360e2209 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -147,7 +147,7 @@ class UserApiClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-template-folders') @cache.delete('user-{user_id}') def add_user_to_service(self, service_id, user_id, permissions, folder_permissions): - # permissions passed in are the combined admin roles, not db permissions + # permissions passed in are the combined UI permissions, not DB permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) data = { 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], @@ -164,7 +164,7 @@ class UserApiClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-template-folders') @cache.delete('user-{user_id}') def set_user_permissions(self, user_id, service_id, permissions, folder_permissions=None): - # permissions passed in are the combined admin roles, not db permissions + # permissions passed in are the combined UI permissions, not DB permissions data = { 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], }