From 17061e0d06ea15b1ba30381226aa95325089665d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 28 Feb 2018 15:37:49 +0000 Subject: [PATCH] map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles. --- app/main/views/manage_users.py | 19 +++---- app/notify_client/invite_api_client.py | 8 +-- app/notify_client/models.py | 58 +++++++++++++++++++-- app/notify_client/user_api_client.py | 11 ++-- tests/__init__.py | 1 + tests/app/main/test_permissions.py | 16 ++++++ tests/app/notify_client/test_user_client.py | 26 +++++++++ 7 files changed, 117 insertions(+), 22 deletions(-) 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'])