diff --git a/app/main/forms.py b/app/main/forms.py index b8af1c9dc..15dddedec 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -56,13 +56,13 @@ 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 ( + all_ui_permissions, + broadcast_permission_options, + permission_options, +) def get_time_value_and_label(future_time): @@ -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 permissions)] + 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_permissions +# 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_permissions)] + return [entry for entry in valuelist if any(entry in option for option in broadcast_permission_options)] class BasePermissionsForm(StripWhitespaceForm): @@ -989,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."} @@ -1005,8 +1005,10 @@ class BasePermissionsForm(StripWhitespaceForm): form = cls( **kwargs, **{ - "permissions_field": [ - role for role in roles.keys() 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 ) @@ -1027,7 +1029,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 78e7a0f46..42d5c5bba 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -28,9 +28,12 @@ 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_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/models/roles_and_permissions.py b/app/models/roles_and_permissions.py deleted file mode 100644 index 124b55e8b..000000000 --- a/app/models/roles_and_permissions.py +++ /dev/null @@ -1,52 +0,0 @@ -from itertools import chain - -roles = { - 'send_messages': ['send_texts', 'send_emails', 'send_letters'], - 'manage_templates': ['manage_templates'], - 'manage_service': ['manage_users', 'manage_settings'], - 'manage_api_keys': ['manage_api_keys'], - 'view_activity': ['view_activity'], - 'create_broadcasts': ['create_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], - 'approve_broadcasts': ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], -} - -all_permissions = set(roles.keys()) -all_database_permissions = set(chain(*roles.values())) - -permissions = ( - ('view_activity', 'See dashboard'), - ('send_messages', 'Send messages'), - ('manage_templates', 'Add and edit templates'), - ('manage_service', 'Manage settings, team and usage'), - ('manage_api_keys', 'Manage API integration'), -) - -broadcast_permissions = ( - ('manage_templates', 'Add and edit templates'), - ('create_broadcasts', 'Create new alerts'), - ('approve_broadcasts', 'Approve alerts'), -) - - -def translate_permissions_from_db_to_admin_roles(permissions): - """ - Given a list of database permissions, return a set of roles - - 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} - - return { - admin_role for admin_role, db_role_list in roles.items() - if set(db_role_list) <= set(permissions) - } | unknown_database_permissions - - -def translate_permissions_from_admin_roles_to_db(permissions): - """ - Given a list of admin roles (ie: checkboxes on a permissions edit page for example), return a set of 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)) diff --git a/app/models/user.py b/app/models/user.py index a03ecf712..95278abb6 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_ui_permissions, + translate_permissions_from_db_to_ui, +) def _get_service_id_from_view_args(): @@ -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() } @@ -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))) @@ -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 f77b9f5c1..464c3ba9c 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 ( - roles, - translate_permissions_from_admin_roles_to_db, -) from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache +from app.utils.user_permissions import ( + all_ui_permissions, + translate_permissions_from_ui_to_db, +) class InviteApiClient(NotifyAdminAPIClient): @@ -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, @@ -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/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index b65836878..7360e2209 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.models.roles_and_permissions import ( - translate_permissions_from_admin_roles_to_db, -) from app.notify_client import NotifyAdminAPIClient, cache +from app.utils.user_permissions import translate_permissions_from_ui_to_db ALLOWED_ATTRIBUTES = { 'name', @@ -149,10 +147,10 @@ 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_admin_roles_to_db(permissions)], + 'permissions': [{'permission': x} for x in translate_permissions_from_ui_to_db(permissions)], 'folder_permissions': folder_permissions, } @@ -166,9 +164,9 @@ 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_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 new file mode 100644 index 000000000..cddd87ca4 --- /dev/null +++ b/app/utils/user_permissions.py @@ -0,0 +1,54 @@ +from itertools import chain + +permission_mappings = { + 'send_messages': ['send_texts', 'send_emails', 'send_letters'], + 'manage_templates': ['manage_templates'], + 'manage_service': ['manage_users', 'manage_settings'], + 'manage_api_keys': ['manage_api_keys'], + 'view_activity': ['view_activity'], + 'create_broadcasts': ['create_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], + 'approve_broadcasts': ['approve_broadcasts', 'reject_broadcasts', 'cancel_broadcasts'], +} + +all_ui_permissions = set(permission_mappings.keys()) +all_db_permissions = set(chain(*permission_mappings.values())) + +permission_options = ( + ('view_activity', 'See dashboard'), + ('send_messages', 'Send messages'), + ('manage_templates', 'Add and edit templates'), + ('manage_service', 'Manage settings, team and usage'), + ('manage_api_keys', 'Manage API integration'), +) + +broadcast_permission_options = ( + ('manage_templates', 'Add and edit templates'), + ('create_broadcasts', 'Create new alerts'), + ('approve_broadcasts', 'Approve alerts'), +) + + +def translate_permissions_from_db_to_ui(db_permissions): + """ + Given a list of database permissions, return a set of UI permissions + + 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 = set(db_permissions) - all_db_permissions + + return { + 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_ui_to_db(ui_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 mapping, falling back to just passing through if they're not recognised. + """ + return set(chain.from_iterable( + permission_mappings.get(ui_permission, [ui_permission]) for ui_permission in ui_permissions + )) diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index c089907fa..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.models.roles_and_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..b40994597 --- /dev/null +++ b/tests/app/utils/test_user_permissions.py @@ -0,0 +1,45 @@ +import pytest + +from app.utils.user_permissions import ( + translate_permissions_from_db_to_ui, + translate_permissions_from_ui_to_db, +) + + +@pytest.mark.parametrize('db_permissions,expected_ui_permissions', [ + ( + ['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_ui( + db_permissions, + expected_ui_permissions, +): + ui_permissions = translate_permissions_from_db_to_ui(db_permissions) + assert ui_permissions == expected_ui_permissions + + +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' + }