From 78c88530b59ff2bf781304506bbaaea0153d92c3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 10 Aug 2020 12:13:11 +0100 Subject: [PATCH 1/2] Let users without `send_messages` view broadcasts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment viewing a broadcast is limited to those users who have the `send_messages` permission. This doesn’t match how we describe the permissions on the team members page This commit makes it so that any team member can see a broadcast that’s in any state other than `draft`. --- app/main/views/broadcast.py | 2 +- .../views/broadcast/view-message.html | 11 ++++- tests/app/main/views/test_broadcast.py | 44 +++++++++++++++++++ 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/app/main/views/broadcast.py b/app/main/views/broadcast.py index ff35b505d..8069dfeaf 100644 --- a/app/main/views/broadcast.py +++ b/app/main/views/broadcast.py @@ -182,7 +182,7 @@ def preview_broadcast_message(service_id, broadcast_message_id): @main.route('/services//broadcast/') -@user_has_permissions('send_messages') +@user_has_permissions() @service_has_permission('broadcast') def view_broadcast_message(service_id, broadcast_message_id): broadcast_message = BroadcastMessage.from_id( diff --git a/app/templates/views/broadcast/view-message.html b/app/templates/views/broadcast/view-message.html index 474fd10f9..d07ce3052 100644 --- a/app/templates/views/broadcast/view-message.html +++ b/app/templates/views/broadcast/view-message.html @@ -18,7 +18,7 @@ ) }} {% if broadcast_message.status == 'pending-approval' %} - {% if broadcast_message.created_by == current_user %} + {% if broadcast_message.created_by == current_user and current_user.has_permissions('send_messages') %} - {% else %} + {% elif current_user.has_permissions('send_messages') %} {% call form_wrapper(class="banner govuk-!-margin-bottom-6") %}

{{ broadcast_message.created_by.name }} wants to broadcast this @@ -41,6 +41,13 @@ delete_link_text='Reject this broadcast' ) }} {% endcall %} + {% else %} +

{% endif %} {% else %}

diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 0570e1963..8c828c240 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -639,6 +639,50 @@ def test_cant_approve_own_broadcast( ) +@freeze_time('2020-02-22T22:22:22.000000') +def test_view_only_user_cant_approve_broadcast( + mocker, + client_request, + service_one, + active_user_with_permissions, + active_user_view_permissions, + mock_get_broadcast_template, + fake_uuid, +): + mocker.patch( + 'app.broadcast_message_api_client.get_broadcast_message', + return_value=broadcast_message_json( + id_=fake_uuid, + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + created_by_id=fake_uuid, + finishes_at='2020-02-23T23:23:23.000000', + status='pending-approval', + ), + ) + mocker.patch('app.user_api_client.get_user', side_effect=[ + active_user_view_permissions, # Current user + active_user_with_permissions, # User who created broadcast + ]) + service_one['permissions'] += ['broadcast'] + + page = client_request.get( + '.view_broadcast_message', + service_id=SERVICE_ONE_ID, + broadcast_message_id=fake_uuid, + ) + + assert ( + normalize_spaces(page.select_one('.banner').text) + ) == ( + 'This broadcast is waiting for approval ' + 'You don’t have permission to approve broadcasts.' + ) + + assert not page.select_one('form') + assert not page.select_one('.banner a') + + @pytest.mark.parametrize('initial_status, expected_approval', ( ('draft', False,), ('pending-approval', True), From e47dbc0caaa5086bcacd3f71b9fbe7903b7b58cf Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 10 Aug 2020 12:26:33 +0100 Subject: [PATCH 2/2] Add tests to check permission-restricted broadcast pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some pages should only be shown to users who have permission to send or approve broadcasts. This commit adds a test to ensure that this is true, and that we don’t accidentally regress the checks for this permission. --- tests/app/main/views/test_broadcast.py | 59 ++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/app/main/views/test_broadcast.py b/tests/app/main/views/test_broadcast.py index 8c828c240..6c726f0cc 100644 --- a/tests/app/main/views/test_broadcast.py +++ b/tests/app/main/views/test_broadcast.py @@ -75,6 +75,63 @@ def test_broadcast_pages_403_without_permission( ) +@pytest.mark.parametrize('endpoint, extra_args, expected_get_status, expected_post_status', ( + ( + '.broadcast', + {'template_id': sample_uuid}, + 403, 405, + ), + ( + '.preview_broadcast_areas', {'broadcast_message_id': sample_uuid}, + 403, 405, + ), + ( + '.choose_broadcast_library', {'broadcast_message_id': sample_uuid}, + 403, 405, + ), + ( + '.choose_broadcast_area', {'broadcast_message_id': sample_uuid, 'library_slug': 'countries'}, + 403, 403, + ), + ( + '.remove_broadcast_area', {'broadcast_message_id': sample_uuid, 'area_slug': 'england'}, + 403, 405, + ), + ( + '.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, + endpoint, + extra_args, + expected_get_status, + expected_post_status, +): + service_one['permissions'] += ['broadcast'] + mocker.patch('app.user_api_client.get_user', return_value=active_user_view_permissions) + client_request.get( + endpoint, + service_id=SERVICE_ONE_ID, + _expected_status=expected_get_status, + **extra_args + ) + client_request.post( + endpoint, + service_id=SERVICE_ONE_ID, + _expected_status=expected_post_status, + **extra_args + ) + + def test_dashboard_redirects_to_broadcast_dashboard( client_request, service_one, @@ -94,6 +151,7 @@ def test_dashboard_redirects_to_broadcast_dashboard( def test_empty_broadcast_dashboard( client_request, service_one, + active_user_view_permissions, mock_get_no_broadcast_messages, mock_get_service_templates_when_no_templates_exist, ): @@ -157,6 +215,7 @@ def test_broadcast_dashboard( def test_broadcast_dashboard_json( logged_in_client, service_one, + active_user_view_permissions, mock_get_broadcast_messages, ): service_one['permissions'] += ['broadcast']