From 72c1b3d8a15552f421412792bb3c6d0769c27f29 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 13 Jul 2020 11:10:34 +0100 Subject: [PATCH 1/2] Only show relevant user permissions for broadcast services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For services with the broadcast permission this hides: - the ‘View dashboard’ permission (and defaults it to _checked_) because all users of broadcast services will need to see the dashboard - the ‘Manage API keys’ permission (and defaults it to _not checked_) because we don’t offer an API integration for broadcast services yet – if we do we won’t want existing users to automatically get the permission It relabels: - the ‘Send’ permission to ‘Prepare and approve’ to match the current, slightly clunky language on the templates page - the ‘Manage settings’ label to not refer to ‘usage’ because broadcast services won’t incur cost --- app/main/forms.py | 41 +++++- app/main/views/manage_users.py | 16 ++- app/models/roles_and_permissions.py | 6 + tests/app/main/views/test_manage_users.py | 159 ++++++++++++++++++++++ 4 files changed, 215 insertions(+), 7 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 713e5e036..957e7c11a 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -54,7 +54,11 @@ 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 permissions, roles +from app.models.roles_and_permissions import ( + broadcast_permissions, + permissions, + roles, +) from app.utils import guess_name_from_email_address @@ -493,7 +497,12 @@ PermissionsAbstract = type("PermissionsAbstract", (StripWhitespaceForm,), { }) -class PermissionsForm(PermissionsAbstract): +BroadcastPermissionsAbstract = type("BroadcastPermissionsAbstract", (StripWhitespaceForm,), { + permission: BooleanField(label) for permission, label in broadcast_permissions +}) + + +class BasePermissionsForm(StripWhitespaceForm): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) self.folder_permissions.choices = [] @@ -517,11 +526,14 @@ class PermissionsForm(PermissionsAbstract): @property def permissions(self): - return {role for role in roles.keys() if self[role].data is True} + return {field.id for field in self.permissions_fields if field.data is True} @property def permissions_fields(self): - return (getattr(self, permission) for permission, _ in permissions) + return ( + getattr(self, permission) for permission, field in self.__dict__.items() + if isinstance(field, BooleanField) + ) @classmethod def from_user(cls, user, service_id, **kwargs): @@ -535,7 +547,18 @@ class PermissionsForm(PermissionsAbstract): ) -class InviteUserForm(PermissionsForm): +class PermissionsForm(PermissionsAbstract, BasePermissionsForm): + pass + + +class BroadcastPermissionsForm(BroadcastPermissionsAbstract, BasePermissionsForm): + + @property + def permissions(self): + return {'view_activity'} | super().permissions + + +class BaseInviteUserForm(): email_address = email_address(gov_user=False) def __init__(self, invalid_email_address, *args, **kwargs): @@ -547,6 +570,14 @@ class InviteUserForm(PermissionsForm): raise ValidationError("You cannot send an invitation to yourself") +class InviteUserForm(BaseInviteUserForm, PermissionsForm): + pass + + +class BroadcastInviteUserForm(BaseInviteUserForm, BroadcastPermissionsForm): + pass + + class InviteOrgUserForm(StripWhitespaceForm): email_address = email_address(gov_user=False) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 8102ba521..3f055e695 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -18,6 +18,8 @@ from app.event_handlers import ( ) from app.main import main from app.main.forms import ( + BroadcastInviteUserForm, + BroadcastPermissionsForm, ChangeEmailForm, ChangeMobileNumberForm, ChangeNonGovEmailForm, @@ -47,7 +49,12 @@ def manage_users(service_id): @user_has_permissions('manage_service') def invite_user(service_id): - form = InviteUserForm( + if current_service.has_permission('broadcast'): + form_class = BroadcastInviteUserForm + else: + form_class = InviteUserForm + + form = form_class( invalid_email_address=current_user.email_address, all_template_folders=current_service.all_template_folders, folder_permissions=[f['id'] for f in current_service.all_template_folders] @@ -89,7 +96,12 @@ def edit_user_permissions(service_id, user_id): if user.mobile_number: mobile_number = redact_mobile_number(user.mobile_number, " ") - form = PermissionsForm.from_user( + if current_service.has_permission('broadcast'): + form_class = BroadcastPermissionsForm + else: + form_class = PermissionsForm + + form = form_class.from_user( user, service_id, folder_permissions=None if user.platform_admin else [ diff --git a/app/models/roles_and_permissions.py b/app/models/roles_and_permissions.py index c59526101..ed71774dc 100644 --- a/app/models/roles_and_permissions.py +++ b/app/models/roles_and_permissions.py @@ -25,6 +25,12 @@ permissions = ( ('manage_api_keys', 'Manage API integration'), ) +broadcast_permissions = ( + ('send_messages', 'Prepare and approve broadcasts'), + ('manage_templates', 'Add and edit templates'), + ('manage_service', 'Manage settings and team'), +) + def translate_permissions_from_db_to_admin_roles(permissions): """ diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index eeca85f48..374bbb41d 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -266,6 +266,39 @@ def test_service_without_caseworking_doesnt_show_admin_vs_caseworker( assert page.select('input[type=checkbox]')[4]['name'] == 'manage_api_keys' +@pytest.mark.parametrize('endpoint, extra_args', [ + ( + 'main.edit_user_permissions', + {'user_id': sample_uuid()}, + ), + ( + 'main.invite_user', + {}, + ), +]) +def test_broadcast_service_only_shows_relevant_permissions( + client_request, + service_one, + mock_get_users_by_service, + mock_get_template_folders, + endpoint, + extra_args, +): + service_one['permissions'] = ['broadcast'] + page = client_request.get( + endpoint, + service_id=SERVICE_ONE_ID, + **extra_args + ) + assert [ + field['name'] for field in page.select('input[type=checkbox]') + ] == [ + 'send_messages', + 'manage_templates', + 'manage_service', + ] + + @pytest.mark.parametrize('service_has_email_auth, displays_auth_type', [ (True, True), (False, False) @@ -477,6 +510,76 @@ def test_edit_user_permissions( ) +@pytest.mark.parametrize('submitted_permissions, permissions_sent_to_api', [ + ( + { + 'view_activity': 'y', + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + }, + { + 'view_activity', + 'send_messages', + 'manage_service', + 'manage_templates', + } + ), + ( + { + 'view_activity': 'y', + 'send_messages': 'y', + }, + { + 'view_activity', + 'send_messages', + } + ), + ( + { + }, + { + 'view_activity', + } + ), +]) +def test_edit_user_permissions_for_broadcast_service( + client_request, + service_one, + mocker, + mock_get_users_by_service, + mock_get_invites_for_service, + mock_set_user_permissions, + mock_get_template_folders, + fake_uuid, + submitted_permissions, + permissions_sent_to_api, +): + service_one['permissions'] = 'broadcast' + client_request.post( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + _data=dict( + email_address="test@example.com", + **submitted_permissions + ), + _expected_status=302, + _expected_redirect=url_for( + 'main.manage_users', + service_id=SERVICE_ONE_ID, + _external=True, + ), + ) + mock_set_user_permissions.assert_called_with( + fake_uuid, + SERVICE_ONE_ID, + permissions=permissions_sent_to_api, + folder_permissions=[] + ) + + def test_edit_user_folder_permissions( client_request, mocker, @@ -787,6 +890,62 @@ def test_invite_user_with_email_auth_service( []) +@pytest.mark.parametrize('post_data, expected_permissions_to_api', ( + ( + { + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + }, + { + 'view_activity', + 'send_messages', + 'manage_templates', + 'manage_service', + }, + ), + ( + { + 'view_activity': 'y', + 'manage_api_keys': 'y', + 'foo': 'y', + }, + { + 'view_activity', + }, + ), +)) +def test_invite_user_to_broadcast_service( + client_request, + service_one, + active_user_with_permissions, + mocker, + sample_invite, + mock_get_template_folders, + mock_get_organisations, + post_data, + expected_permissions_to_api, +): + service_one['permissions'] = ['broadcast'] + mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) + mocker.patch('app.models.user.Users.client_method', return_value=[active_user_with_permissions]) + mocker.patch('app.invite_api_client.create_invite', return_value=sample_invite) + post_data['email_address'] = 'broadcast@example.gov.uk' + client_request.post( + 'main.invite_user', + service_id=SERVICE_ONE_ID, + _data=post_data, + ) + app.invite_api_client.create_invite.assert_called_once_with( + sample_invite['from_user'], + sample_invite['service'], + 'broadcast@example.gov.uk', + expected_permissions_to_api, + 'sms_auth', + [], + ) + + def test_cancel_invited_user_cancels_user_invitations( client_request, mock_get_invites_for_service, From 94434b36e73beeed724de8c72d2f94e0de45d406 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 13 Jul 2020 12:04:11 +0100 Subject: [PATCH 2/2] Only show relevant permissions on the team members page Since users of broadcast services will always have the view dashboard permission and never have the API keys permission we can hide these. And we should re-label the permissions to make sense in the context of broadcasting. --- app/main/views/manage_users.py | 6 +++-- tests/app/main/views/test_manage_users.py | 29 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 3f055e695..b4df99416 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -27,7 +27,7 @@ from app.main.forms import ( PermissionsForm, SearchUsersForm, ) -from app.models.roles_and_permissions import permissions +from app.models.roles_and_permissions import broadcast_permissions, permissions from app.models.user import InvitedUser, User from app.utils import is_gov_user, redact_mobile_number, user_has_permissions @@ -41,7 +41,9 @@ def manage_users(service_id): current_user=current_user, show_search_box=(len(current_service.team_members) > 7), form=SearchUsersForm(), - permissions=permissions, + permissions=( + broadcast_permissions if current_service.has_permission('broadcast') else permissions + ), ) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 374bbb41d..34a221d7d 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -193,6 +193,35 @@ def test_should_show_caseworker_on_overview_page( ) +def test_should_show_overview_page_for_broadcast_service( + client_request, + mocker, + mock_get_invites_for_service, + mock_get_template_folders, + service_one, + active_user_view_permissions, + active_user_with_permissions, +): + service_one['permissions'].append('broadcast') + mocker.patch('app.models.user.Users.client_method', return_value=[ + active_user_with_permissions, + active_user_view_permissions, + ]) + page = client_request.get('main.manage_users', service_id=SERVICE_ONE_ID) + assert normalize_spaces(page.select('.user-list-item')[0].text) == ( + 'Test User (you) ' + 'Can Prepare and approve broadcasts ' + 'Can Add and edit templates ' + 'Can Manage settings and team' + ) + assert normalize_spaces(page.select('.user-list-item')[1].text) == ( + 'Test User With Permissions (you) ' + 'Cannot Prepare and approve broadcasts ' + 'Cannot Add and edit templates ' + 'Cannot Manage settings and team' + ) + + @pytest.mark.parametrize('endpoint, extra_args, service_has_email_auth, auth_options_hidden', [ ( 'main.edit_user_permissions',