diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index 6618a3e16..a808e0a29 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -93,7 +93,7 @@ def get_broadcast_dashboard_partials(service_id): @main.route('/services//new-broadcast', methods=['GET', 'POST']) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def new_broadcast(service_id): form = NewBroadcastForm() @@ -116,7 +116,7 @@ def new_broadcast(service_id): @main.route('/services//write-new-broadcast', methods=['GET', 'POST']) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def write_new_broadcast(service_id): form = BroadcastTemplateForm() @@ -140,7 +140,7 @@ def write_new_broadcast(service_id): @main.route('/services//new-broadcast/') -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def broadcast(service_id, template_id): return redirect(url_for( @@ -154,7 +154,7 @@ def broadcast(service_id, template_id): @main.route('/services//broadcast//areas') -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def preview_broadcast_areas(service_id, broadcast_message_id): broadcast_message = BroadcastMessage.from_id( @@ -181,7 +181,7 @@ def preview_broadcast_areas(service_id, broadcast_message_id): @main.route('/services//broadcast//libraries') -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def choose_broadcast_library(service_id, broadcast_message_id): return render_template( @@ -198,7 +198,7 @@ def choose_broadcast_library(service_id, broadcast_message_id): '/services//broadcast//libraries/', methods=['GET', 'POST'], ) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def choose_broadcast_area(service_id, broadcast_message_id, library_slug): broadcast_message = BroadcastMessage.from_id( @@ -258,7 +258,7 @@ def _get_broadcast_sub_area_back_link(service_id, broadcast_message_id, library_ '/services//broadcast//libraries//', methods=['GET', 'POST'], ) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def choose_broadcast_sub_area(service_id, broadcast_message_id, library_slug, area_slug): broadcast_message = BroadcastMessage.from_id( @@ -310,7 +310,7 @@ def choose_broadcast_sub_area(service_id, broadcast_message_id, library_slug, ar @main.route('/services//broadcast//remove/') -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def remove_broadcast_area(service_id, broadcast_message_id, area_slug): BroadcastMessage.from_id( @@ -330,7 +330,7 @@ def remove_broadcast_area(service_id, broadcast_message_id, area_slug): '/services//broadcast//preview', methods=['GET', 'POST'], ) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def preview_broadcast_message(service_id, broadcast_message_id): broadcast_message = BroadcastMessage.from_id( @@ -405,7 +405,7 @@ def view_broadcast(service_id, broadcast_message_id): @main.route('/services//current-alerts/', methods=['POST']) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def approve_broadcast_message(service_id, broadcast_message_id): @@ -438,7 +438,7 @@ def approve_broadcast_message(service_id, broadcast_message_id): @main.route('/services//broadcast//reject') -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=True) @service_has_permission('broadcast') def reject_broadcast_message(service_id, broadcast_message_id): @@ -466,7 +466,7 @@ def reject_broadcast_message(service_id, broadcast_message_id): '/services//broadcast//cancel', methods=['GET', 'POST'], ) -@user_has_permissions('send_messages') +@user_has_permissions('send_messages', restrict_admin_usage=False) @service_has_permission('broadcast') def cancel_broadcast_message(service_id, broadcast_message_id): broadcast_message = BroadcastMessage.from_id( diff --git a/app/templates/views/broadcast/dashboard.html b/app/templates/views/broadcast/dashboard.html index a0695de8e..5ebf590a7 100644 --- a/app/templates/views/broadcast/dashboard.html +++ b/app/templates/views/broadcast/dashboard.html @@ -17,7 +17,7 @@ 'current_broadcasts' ) }} - {% if current_user.has_permissions('send_messages') %} + {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) %}
{{ govukButton({ "element": "a", diff --git a/app/templates/views/broadcast/previous-broadcasts.html b/app/templates/views/broadcast/previous-broadcasts.html index 835c1a216..93416b670 100644 --- a/app/templates/views/broadcast/previous-broadcasts.html +++ b/app/templates/views/broadcast/previous-broadcasts.html @@ -13,7 +13,7 @@ {% include('views/broadcast/partials/dashboard-table.html') %} - {% if current_user.has_permissions('send_messages') %} + {% if current_user.has_permissions('send_messages', restrict_admin_usage=True) %}
{{ govukButton({ "element": "a", diff --git a/app/templates/views/broadcast/view-message.html b/app/templates/views/broadcast/view-message.html index cca6309ee..b9de5bcf2 100644 --- a/app/templates/views/broadcast/view-message.html +++ b/app/templates/views/broadcast/view-message.html @@ -19,9 +19,9 @@ {% block service_page_title %} {% if broadcast_message.status == 'pending-approval' %} - {% if broadcast_message.created_by and broadcast_message.created_by == current_user and current_user.has_permissions('send_messages') %} + {% if broadcast_message.created_by and broadcast_message.created_by == current_user and current_user.has_permissions('send_messages', restrict_admin_usage=True) %} {{ broadcast_message.template.name }} is waiting for approval - {% elif current_user.has_permissions('send_messages') %} + {% elif current_user.has_permissions('send_messages', restrict_admin_usage=True) %} {% if broadcast_message.created_by %} {{ broadcast_message.created_by.name }} {% else %} @@ -42,7 +42,7 @@ {{ govukBackLink({ "href": back_link }) }} {% if broadcast_message.status == 'pending-approval' %} - {% if broadcast_message.created_by and broadcast_message.created_by == current_user and current_user.has_permissions('send_messages') %} + {% if broadcast_message.created_by and broadcast_message.created_by == current_user and current_user.has_permissions('send_messages', restrict_admin_usage=True) %} - {% elif current_user.has_permissions('send_messages') %} + {% elif current_user.has_permissions('send_messages', restrict_admin_usage=True) %} {% call form_wrapper(class="banner govuk-!-margin-bottom-6") %}

{% if broadcast_message.created_by %} diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 58671a4ee..406619e4d 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -94,6 +94,7 @@ def test_broadcast_pages_403_without_permission( ) +@pytest.mark.parametrize('user_is_platform_admin', [True, False]) @pytest.mark.parametrize('endpoint, extra_args, expected_get_status, expected_post_status', ( ( '.new_broadcast', {}, @@ -128,23 +129,27 @@ def test_broadcast_pages_403_without_permission( '.preview_broadcast_message', {'broadcast_message_id': sample_uuid}, 403, 403, ), - ( - '.cancel_broadcast_message', {'broadcast_message_id': sample_uuid}, - 403, 403, - ), )) def test_broadcast_pages_403_for_user_without_permission( mocker, client_request, service_one, active_user_view_permissions, + platform_admin_user_no_service_permissions, endpoint, extra_args, expected_get_status, expected_post_status, + user_is_platform_admin ): + """ + Checks that users without permissions, including admin users, cannot create or edit broadcasts. + """ service_one['permissions'] += ['broadcast'] - mocker.patch('app.user_api_client.get_user', return_value=active_user_view_permissions) + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + else: + client_request.login(active_user_view_permissions) client_request.get( endpoint, service_id=SERVICE_ONE_ID, @@ -159,6 +164,78 @@ def test_broadcast_pages_403_for_user_without_permission( ) +@pytest.mark.parametrize('user_is_platform_admin', [True, False]) +def test_user_cannot_accept_broadcast_without_permission( + mocker, + client_request, + service_one, + active_user_view_permissions, + platform_admin_user_no_service_permissions, + user_is_platform_admin +): + service_one['permissions'] += ['broadcast'] + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + else: + client_request.login(active_user_view_permissions) + + client_request.post( + '.approve_broadcast_message', + service_id=SERVICE_ONE_ID, + broadcast_message_id=sample_uuid, + _expected_status=403, + ) + + +@pytest.mark.parametrize('user_is_platform_admin', [True, False]) +def test_user_cannot_reject_broadcast_without_permission( + mocker, + client_request, + service_one, + active_user_view_permissions, + platform_admin_user_no_service_permissions, + user_is_platform_admin +): + service_one['permissions'] += ['broadcast'] + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + else: + client_request.login(active_user_view_permissions) + + client_request.get( + '.reject_broadcast_message', + service_id=SERVICE_ONE_ID, + broadcast_message_id=sample_uuid, + _expected_status=403, + ) + + +def test_cancel_broadcast_page_403_for_user_without_permission( + mocker, + client_request, + service_one, + active_user_view_permissions, +): + """ + separate test for cancel_broadcast endpoint, because admin users are allowed to cancel broadcasts + """ + service_one['permissions'] += ['broadcast'] + + mocker.patch('app.user_api_client.get_user', return_value=active_user_view_permissions) + client_request.get( + '.cancel_broadcast_message', + service_id=SERVICE_ONE_ID, + _expected_status=403, + **{'broadcast_message_id': sample_uuid} + ) + client_request.post( + '.cancel_broadcast_message', + service_id=SERVICE_ONE_ID, + _expected_status=403, + **{'broadcast_message_id': sample_uuid} + ) + + @pytest.mark.parametrize('step_index, expected_link_text, expected_link_href', ( (1, 'Continue', partial(url_for, '.broadcast_tour', step_index=2)), (2, 'Continue', partial(url_for, '.broadcast_tour', step_index=3)), @@ -358,6 +435,7 @@ def test_broadcast_dashboard( ) +@pytest.mark.parametrize("user_is_platform_admin", [True, False]) @pytest.mark.parametrize('endpoint', ( '.broadcast_dashboard', '.broadcast_dashboard_previous', '.broadcast_dashboard_rejected', )) @@ -365,11 +443,17 @@ def test_broadcast_dashboard_does_not_have_button_for_view_only_user( client_request, service_one, active_user_view_permissions, + platform_admin_user_no_service_permissions, mock_get_broadcast_messages, endpoint, + user_is_platform_admin ): + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + else: + client_request.login(active_user_view_permissions) + service_one['permissions'] += ['broadcast'] - client_request.login(active_user_view_permissions) page = client_request.get( endpoint, service_id=SERVICE_ONE_ID, @@ -1835,14 +1919,17 @@ def test_can_approve_own_broadcast_in_trial_mode( @freeze_time('2020-02-22T22:22:22.000000') +@pytest.mark.parametrize("user_is_platform_admin", [True, False]) def test_view_only_user_cant_approve_broadcast( mocker, client_request, service_one, active_user_with_permissions, active_user_view_permissions, + platform_admin_user_no_service_permissions, mock_get_broadcast_template, fake_uuid, + user_is_platform_admin ): mocker.patch( 'app.broadcast_message_api_client.get_broadcast_message', @@ -1855,8 +1942,12 @@ def test_view_only_user_cant_approve_broadcast( status='pending-approval', ), ) + if user_is_platform_admin: + current_user = platform_admin_user_no_service_permissions + else: + current_user = active_user_view_permissions mocker.patch('app.user_api_client.get_user', side_effect=[ - active_user_view_permissions, # Current user + current_user, # Current user active_user_with_permissions, # User who created broadcast ]) service_one['permissions'] += ['broadcast'] @@ -2076,15 +2167,25 @@ def test_no_view_page_for_draft( ) +@pytest.mark.parametrize("user_is_platform_admin", [True, False]) def test_cancel_broadcast( client_request, service_one, mock_get_live_broadcast_message, mock_get_broadcast_template, mock_update_broadcast_message_status, + platform_admin_user_no_service_permissions, fake_uuid, + user_is_platform_admin ): + """ + users with 'send_messages' permissions and platform admins should be able to cancel broadcasts. + """ service_one['permissions'] += ['broadcast'] + + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + page = client_request.get( '.cancel_broadcast_message', service_id=SERVICE_ONE_ID, @@ -2108,15 +2209,25 @@ def test_cancel_broadcast( ) not in page +@pytest.mark.parametrize("user_is_platform_admin", [True, False]) def test_confirm_cancel_broadcast( client_request, service_one, mock_get_live_broadcast_message, mock_get_broadcast_template, mock_update_broadcast_message_status, + platform_admin_user_no_service_permissions, fake_uuid, + user_is_platform_admin ): + """ + users with 'send_messages' permissions and platform admins should be able to cancel broadcasts. + """ service_one['permissions'] += ['broadcast'] + + if user_is_platform_admin: + client_request.login(platform_admin_user_no_service_permissions) + client_request.post( '.cancel_broadcast_message', service_id=SERVICE_ONE_ID, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e41561e60..9b148c175 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -39,7 +39,6 @@ from tests.conftest import ( create_active_user_with_permissions, create_multiple_email_reply_to_addresses, create_multiple_sms_senders, - create_platform_admin_user, create_template, mock_get_service_email_template, mock_get_service_letter_template, @@ -1614,13 +1613,14 @@ def test_no_link_to_use_existing_list_for_service_without_lists( client_request, mock_get_service_template, mock_has_jobs, + platform_admin_user, fake_uuid, ): mocker.patch( 'app.models.contact_list.ContactLists.client_method', return_value=[], ) - client_request.login(create_platform_admin_user()) + client_request.login(platform_admin_user) page = client_request.get( 'main.send_one_off', service_id=SERVICE_ONE_ID, diff --git a/tests/conftest.py b/tests/conftest.py index ba4ba6527..c20d6d38b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1082,30 +1082,25 @@ def api_user_pending(fake_uuid): @pytest.fixture(scope='function') def platform_admin_user(fake_uuid): - user_data = {'id': fake_uuid, - 'name': 'Platform admin user', - 'password': 'somepassword', - 'email_address': 'platform@admin.gov.uk', - 'mobile_number': '07700 900762', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {SERVICE_ONE_ID: ['send_texts', - 'send_emails', - 'send_letters', - 'manage_users', - 'manage_templates', - 'manage_settings', - 'manage_api_keys', - 'view_activity']}, - 'platform_admin': True, - 'auth_type': 'sms_auth', - 'password_changed_at': str(datetime.utcnow()), - 'services': [], - 'organisations': [], - 'current_session_id': None, - 'logged_in_at': None, - } - return user_data + return create_platform_admin_user(permissions={SERVICE_ONE_ID: [ + 'send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity' + ]}) + + +@pytest.fixture(scope='function') +def platform_admin_user_no_service_permissions(): + """ + this fixture is for situations where we want to test that platform admin can access + an endpoint even though they have no explicit permissions for that service. + """ + return create_platform_admin_user() @pytest.fixture(scope='function') @@ -3973,7 +3968,7 @@ def create_active_user_manage_template_permissions(with_unique_id=False): } -def create_platform_admin_user(with_unique_id=False): +def create_platform_admin_user(with_unique_id=False, permissions=None): return { 'id': str(uuid4()) if with_unique_id else sample_uuid(), 'name': 'Platform admin user', @@ -3982,14 +3977,7 @@ def create_platform_admin_user(with_unique_id=False): 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, - 'permissions': {SERVICE_ONE_ID: ['send_texts', - 'send_emails', - 'send_letters', - 'manage_users', - 'manage_templates', - 'manage_settings', - 'manage_api_keys', - 'view_activity']}, + 'permissions': permissions or {}, 'platform_admin': True, 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()),