From 5dbd2297818575545b73db65f6ad7cb65782eec5 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 3 Apr 2019 17:20:32 +0100 Subject: [PATCH 1/2] Hide template folder permission editing for platform admin users Platform admin users can access all template folders, so the folder permissions form always displays everything as checked for them, which makes it look like the form isn't actually working. We could do the check based on folder data, but the field still wouldn't have any effect on permissions. So instead, we hide it completely for platform admin users. Submitting the form will remove any folder permissions from the DB for the platform admin user (which can still be created by changing permissions on the template folder 'Manage' page), but that's only relevant if a user stops being a platform admin but keeps their Notify services. --- app/main/views/manage_users.py | 4 +- .../views/manage-users/permissions.html | 4 ++ tests/app/main/views/test_manage_users.py | 49 +++++++++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 314c18755..4c38a79ce 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -103,11 +103,11 @@ def edit_user_permissions(service_id, user_id): form = PermissionsForm.from_user( user, service_id, - folder_permissions=[ + folder_permissions=None if user.platform_admin else [ f['id'] for f in current_service.all_template_folders if user.has_template_folder_permission(f) ], - all_template_folders=current_service.all_template_folders + all_template_folders=None if user.platform_admin else current_service.all_template_folders ) if form.validate_on_submit(): diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 8d839b9aa..a3b0160fd 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -15,6 +15,10 @@ {% if current_service.has_permission("edit_folder_permissions") and form.folder_permissions.all_template_folders %} {{ checkboxes_nested(form.folder_permissions, form.folder_permissions.children(), hide_legend=True, collapsible_opts={ 'field': 'folder' }) }} +{% elif user and user.platform_admin %} +

+ Platform admin users can access all template folders. +

{% endif %} {% if service_has_email_auth %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 2d45c8b02..eb9dbe821 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -17,6 +17,7 @@ from tests.conftest import ( active_user_view_permissions, active_user_with_permissions, normalize_spaces, + platform_admin_user, sample_uuid, ) @@ -500,6 +501,54 @@ def test_edit_user_folder_permissions( ) +def test_cant_edit_user_folder_permissions_for_platform_admin_users( + client_request, + mocker, + service_one, + mock_get_users_by_service, + mock_get_invites_for_service, + mock_set_user_permissions, + mock_get_template_folders, + fake_uuid, +): + service_one['permissions'] = ['edit_folder_permissions'] + mocker.patch( + 'app.user_api_client.get_user', return_value=platform_admin_user(fake_uuid) + ) + mock_get_template_folders.return_value = [ + {'id': 'folder-id-1', 'name': 'folder_one', 'parent_id': None, 'users_with_permission': []}, + {'id': 'folder-id-2', 'name': 'folder_one', 'parent_id': None, 'users_with_permission': []}, + {'id': 'folder-id-3', 'name': 'folder_one', 'parent_id': 'folder-id-1', 'users_with_permission': []}, + ] + page = client_request.get( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + ) + assert page.select_one('main p').text == 'foo' + assert page.select('') is False + client_request.post( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + _data=dict( + folder_permissions=['folder-id-1', 'folder-id-3'] + ), + _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=set(), + folder_permissions=['folder-id-1', 'folder-id-3'] + ) + + def test_cant_edit_non_member_user_permissions( client_request, mocker, From 81b299428f7f07d624c53801f9568a503c267a81 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 17 May 2019 11:03:41 +0100 Subject: [PATCH 2/2] Add tests for editing folder permissions for platform admin users --- app/main/forms.py | 1 + tests/app/main/views/test_manage_users.py | 29 +++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 016534ed4..f22053df7 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -418,6 +418,7 @@ PermissionsAbstract = type("PermissionsAbstract", (StripWhitespaceForm,), { class PermissionsForm(PermissionsAbstract): def __init__(self, all_template_folders=None, *args, **kwargs): super().__init__(*args, **kwargs) + self.folder_permissions.choices = [] if all_template_folders is not None: self.folder_permissions.all_template_folders = all_template_folders self.folder_permissions.choices = [ diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index eb9dbe821..6f2c19f4e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -479,6 +479,18 @@ def test_edit_user_folder_permissions( {'id': 'folder-id-2', 'name': 'folder_one', 'parent_id': None, 'users_with_permission': []}, {'id': 'folder-id-3', 'name': 'folder_one', 'parent_id': 'folder-id-1', 'users_with_permission': []}, ] + + page = client_request.get( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + ) + assert [ + item['value'] for item in page.select('input[name=folder_permissions]') + ] == [ + 'folder-id-1', 'folder-id-3', 'folder-id-2' + ] + client_request.post( 'main.edit_user_permissions', service_id=SERVICE_ONE_ID, @@ -525,15 +537,16 @@ def test_cant_edit_user_folder_permissions_for_platform_admin_users( service_id=SERVICE_ONE_ID, user_id=fake_uuid, ) - assert page.select_one('main p').text == 'foo' - assert page.select('') is False + assert normalize_spaces(page.select('main p')[0].text) == 'platform@admin.gov.uk Change' + assert normalize_spaces(page.select('main p')[2].text) == ( + 'Platform admin users can access all template folders.' + ) + assert page.select('input[name=folder_permissions]') == [] client_request.post( 'main.edit_user_permissions', service_id=SERVICE_ONE_ID, user_id=fake_uuid, - _data=dict( - folder_permissions=['folder-id-1', 'folder-id-3'] - ), + _data={}, _expected_status=302, _expected_redirect=url_for( 'main.manage_users', @@ -544,8 +557,10 @@ def test_cant_edit_user_folder_permissions_for_platform_admin_users( mock_set_user_permissions.assert_called_with( fake_uuid, SERVICE_ONE_ID, - permissions=set(), - folder_permissions=['folder-id-1', 'folder-id-3'] + permissions={ + 'manage_api_keys', 'manage_service', 'manage_templates', 'send_messages', 'view_activity', + }, + folder_permissions=None, )