diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 98b5fff98..b893a10bb 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -1,5 +1,3 @@ -from itertools import chain - from flask import abort, flash, redirect, render_template, request, url_for from flask_login import current_user, login_required from notifications_python_client.errors import HTTPError @@ -52,7 +50,7 @@ def invite_user(service_id): if form.validate_on_submit(): email_address = form.email_address.data - permissions = ','.join(sorted(get_permissions_from_form(form))) + permissions = get_permissions_from_form(form) invited_user = invite_api_client.create_invite( current_user.id, service_id, @@ -82,7 +80,7 @@ def edit_user_permissions(service_id, user_id): user_has_no_mobile_number = user.mobile_number is None form = PermissionsForm( - **{role: user.has_permissions(*permissions) for role, permissions in roles.items()}, + **{role: user.has_permissions(role) for role in roles.keys()}, login_authentication=user.auth_type ) if form.validate_on_submit(): @@ -111,7 +109,7 @@ def remove_user_from_service(service_id, user_id): # Need to make the email address read only, or a disabled field? # Do it through the template or the form class? form = PermissionsForm(**{ - role: user.has_permissions(*permissions) for role, permissions in roles.items() + role: user.has_permissions(role) for role in roles.keys() }) if request.method == 'POST': @@ -152,11 +150,6 @@ def get_permissions_from_form(form): # view_activity is a default role to be added to all users. # All users will have at minimum view_activity to allow users to see notifications, # templates, team members but no update privileges - selected_permissions = [ - permissions - for role, permissions in roles.items() - if form[role].data is True - ] - selected_permissions = list(chain.from_iterable(selected_permissions)) - selected_permissions.append('view_activity') - return selected_permissions + selected_roles = {role for role in roles.keys() if form[role].data is True} + selected_roles.add('view_activity') + return selected_roles diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 5c3a548ba..1d558454d 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -1,6 +1,8 @@ - from app.notify_client import NotifyAdminAPIClient, _attach_current_user -from app.notify_client.models import InvitedUser +from app.notify_client.models import ( + InvitedUser, + translate_permissions_from_admin_roles_to_db, +) class InviteApiClient(NotifyAdminAPIClient): @@ -17,7 +19,7 @@ class InviteApiClient(NotifyAdminAPIClient): 'service': str(service_id), 'email_address': email_address, 'from_user': invite_from_id, - 'permissions': permissions, + 'permissions': ','.join(sorted(translate_permissions_from_admin_roles_to_db(permissions))), 'auth_type': auth_type, 'invite_link_host': self.admin_url, } diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 1f7b39d8b..d6f88ee95 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -10,13 +10,46 @@ roles = { 'manage_api_keys': ['manage_api_keys'] } -all_permissions = set(chain.from_iterable(roles.values())) | {'view_activity'} +# same dict as above, but flipped round (and with view_activity) +roles_by_permission = { + 'send_texts': 'send_messages', + 'send_emails': 'send_messages', + 'send_letters': 'send_messages', + + 'manage_users': 'manage_service', + 'manage_settings': 'manage_service', + + 'manage_templates': 'manage_templates', + + 'manage_api_keys': 'manage_api_keys', + 'view_activity': 'view_activity', +} + +all_permissions = set(roles_by_permission.values()) def _get_service_id_from_view_args(): return request.view_args.get('service_id', None) +def translate_permissions_from_db_to_admin_roles(permissions): + """ + Given a list of database permissions, return a set of roles + + look them up in roles_by_permission, falling back to just passing through from the api if they aren't in the dict + """ + return {roles_by_permission.get(permission, permission) for permission in 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)) + + class User(UserMixin): def __init__(self, fields, max_failed_login_count=3): self._id = fields.get('id') @@ -24,7 +57,7 @@ class User(UserMixin): self._email_address = fields.get('email_address') self._mobile_number = fields.get('mobile_number') self._password_changed_at = fields.get('password_changed_at') - self._permissions = fields.get('permissions') + self._set_permissions(fields.get('permissions', {})) self._auth_type = fields.get('auth_type') self._failed_login_count = fields.get('failed_login_count') self._state = fields.get('state') @@ -33,6 +66,25 @@ class User(UserMixin): self.current_session_id = fields.get('current_session_id') self._organisations = fields.get('organisations', []) + def _set_permissions(self, permissions_by_service): + """ + Permissions is a dict {'service_id': ['permission a', 'permission b', 'permission c']} + + The api currently returns some granular permissions that we don't set or use separately (but may want + to in the future): + * send_texts, send_letters and send_emails become send_messages + * manage_user and manage_settings become + users either have all three permissions for a service or none of them, they're not helpful to distinguish + between on the front end. So lets collapse them into "send_messages" and "manage_service". If we want to split + them out later, we'll need to rework this function. + """ + + self._permissions = { + service: translate_permissions_from_db_to_admin_roles(permissions) + for service, permissions + in permissions_by_service.items() + } + def get_id(self): return self.id @@ -112,7 +164,7 @@ class User(UserMixin): unknown_permissions = set(permissions) - all_permissions if unknown_permissions: - raise TypeError('{} are not valid permissions'.format(unknown_permissions)) + raise TypeError('{} are not valid permissions'.format(list(unknown_permissions))) # Only available to the platform admin user if admin_override and self.platform_admin: diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index dc45a2b46..8f6872337 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -1,7 +1,10 @@ from notifications_python_client.errors import HTTPError from app.notify_client import NotifyAdminAPIClient -from app.notify_client.models import User +from app.notify_client.models import ( + User, + translate_permissions_from_admin_roles_to_db, +) ALLOWED_ATTRIBUTES = { 'name', @@ -142,8 +145,9 @@ class UserApiClient(NotifyAdminAPIClient): return [User(data) for data in resp['data']] def add_user_to_service(self, service_id, user_id, permissions): + # permissions passed in are the combined admin roles, not db permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) - data = [{'permission': x} for x in permissions] + data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] resp = self.post(endpoint, data=data) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) @@ -152,7 +156,8 @@ class UserApiClient(NotifyAdminAPIClient): return User(resp['data'], max_failed_login_count=self.max_failed_login_count) def set_user_permissions(self, user_id, service_id, permissions): - data = [{'permission': x} for x in permissions] + # permissions passed in are the combined admin roles, not db permissions + data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] endpoint = '/user/{}/service/{}/permission'.format(user_id, service_id) self.post(endpoint, data=data) diff --git a/tests/__init__.py b/tests/__init__.py index 20ca78664..d186968e7 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -47,6 +47,7 @@ def user_json( mobile_number='+447700900986', password_changed_at=None, permissions={generate_uuid(): [ + 'view_activity', 'send_texts', 'send_emails', 'send_letters', diff --git a/tests/app/main/test_permissions.py b/tests/app/main/test_permissions.py index 6811569ed..ee2b68163 100644 --- a/tests/app/main/test_permissions.py +++ b/tests/app/main/test_permissions.py @@ -3,6 +3,10 @@ from flask import request from werkzeug.exceptions import Forbidden, Unauthorized from app.main.views.index import index +from app.notify_client.models import ( + translate_permissions_from_admin_roles_to_db, + translate_permissions_from_db_to_admin_roles, +) from app.utils import user_has_permissions @@ -159,3 +163,15 @@ def _user_with_permissions(): } user = User(user_data) return user + + +def test_translate_permissions_from_db_to_admin_roles(): + db_perms = ['send_texts', 'send_emails', 'send_letters', 'manage_templates', 'some_unknown_permission'] + roles = translate_permissions_from_db_to_admin_roles(db_perms) + assert roles == {'send_messages', 'manage_templates', 'some_unknown_permission'} + + +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'} diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index d98aca621..774c67218 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -131,3 +131,29 @@ def test_client_passes_admin_url_when_sending_email_auth( 'email_auth_link_host': 'http://localhost:6012', } ) + + +def test_client_converts_admin_permissions_to_db_permissions_on_edit(app_, mocker): + mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post') + + user_api_client.set_user_permissions('user_id', 'service_id', permissions={'send_messages', 'view_activity'}) + + assert sorted(mock_post.call_args[1]['data'], key=lambda x: x['permission']) == sorted([ + {'permission': 'send_texts'}, + {'permission': 'send_emails'}, + {'permission': 'send_letters'}, + {'permission': 'view_activity'}, + ], key=lambda x: x['permission']) + + +def test_client_converts_admin_permissions_to_db_permissions_on_add_to_service(app_, mocker): + mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post', return_value={'data': {}}) + + user_api_client.add_user_to_service('service_id', 'user_id', permissions={'send_messages', 'view_activity'}) + + assert sorted(mock_post.call_args[1]['data'], key=lambda x: x['permission']) == sorted([ + {'permission': 'send_texts'}, + {'permission': 'send_emails'}, + {'permission': 'send_letters'}, + {'permission': 'view_activity'}, + ], key=lambda x: x['permission'])