From 0ce539704e7d3698ee33ecb17eaeba7e6356e68f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 25 Feb 2021 10:27:39 +0000 Subject: [PATCH 1/2] Fix bug with removing email auth for broadcast service We accidently were removing the ability for a service to do email auth if it was a broadcast service with email auth. This fixes it. Note, it might be up for debate later whether we let broadcast services use email auth (I think we should) so this might change in time, but we will fix this bug regardless. Note, worth glancing at `SERVICE_PERMISSION_TYPES` which contains a list of permissions that a service might have to make sure I haven't missed any others. The one that looks potentially dodgy is the `EDIT_FOLDER_PERMISSIONS` permission but I can't see this being used anywhere in the API or the admin app so think it is likely now defunct and a user level permission so we don't need to worry about it. --- app/dao/broadcast_service_dao.py | 8 +++++-- tests/app/service/test_rest.py | 36 +++++++++++++++++++++----------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/app/dao/broadcast_service_dao.py b/app/dao/broadcast_service_dao.py index d228bfc31..e4fa73122 100644 --- a/app/dao/broadcast_service_dao.py +++ b/app/dao/broadcast_service_dao.py @@ -3,7 +3,7 @@ from datetime import datetime from flask import current_app from app import db -from app.models import ServiceBroadcastSettings, ServicePermission, Organisation, BROADCAST_TYPE +from app.models import ServiceBroadcastSettings, ServicePermission, Organisation, BROADCAST_TYPE, EMAIL_AUTH_TYPE from app.dao.dao_utils import transactional @@ -20,7 +20,11 @@ def set_broadcast_service_type(service, service_mode, broadcast_channel, provide ServicePermission.query.filter( ServicePermission.service_id == service.id, - ServicePermission.permission != BROADCAST_TYPE + ServicePermission.permission != BROADCAST_TYPE, + # Email auth is an exception to the other service permissions (which relate to what type + # of notifications a service can send) where a broadcast service is allowed to have the + # email auth permission (but doesn't have to) + ServicePermission.permission != EMAIL_AUTH_TYPE ).delete() # Refresh the service object as it has references to the service permissions but we don't yet diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 66712b2c6..0622dca67 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -35,8 +35,10 @@ from app.models import ( INTERNATIONAL_LETTERS, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE, + EMAIL_AUTH_TYPE, NOTIFICATION_RETURNED_LETTER, UPLOAD_LETTERS, + SERVICE_PERMISSION_TYPES, ) from tests import create_authorization_header from tests.app.db import ( @@ -3766,13 +3768,15 @@ def test_set_as_broadcast_service_rejects_if_no_channel( ) -def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_permissions( - admin_request, sample_service, broadcast_organisation +@pytest.mark.parametrize('starting_permissions, ending_permissions', ( + ([], [BROADCAST_TYPE]), + ([EMAIL_AUTH_TYPE], [BROADCAST_TYPE, EMAIL_AUTH_TYPE]), + ([p for p in SERVICE_PERMISSION_TYPES if p != BROADCAST_TYPE], [BROADCAST_TYPE, EMAIL_AUTH_TYPE]), +)) +def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_channel_permissions( + admin_request, broadcast_organisation, starting_permissions, ending_permissions ): - current_permissions = [p.permission for p in sample_service.permissions] - assert len(current_permissions) > 0 - assert BROADCAST_TYPE not in current_permissions - + sample_service = create_service(service_permissions=starting_permissions) data = { 'broadcast_channel': "severe", 'service_mode': 'training', @@ -3784,17 +3788,25 @@ def test_set_as_broadcast_service_gives_broadcast_permission_and_removes_other_p service_id=sample_service.id, _data=data, ) - assert result['data']['permissions'] == [BROADCAST_TYPE] + assert set(result['data']['permissions']) == set(ending_permissions) permissions = ServicePermission.query.filter_by(service_id=sample_service.id).all() - assert [p.permission for p in permissions] == [BROADCAST_TYPE] + assert set([p.permission for p in permissions]) == set(ending_permissions) +@pytest.mark.parametrize('has_email_auth, ending_permissions', ( + (False, [BROADCAST_TYPE]), + (True, [BROADCAST_TYPE, EMAIL_AUTH_TYPE]), +)) def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_broadcast_service( - admin_request, sample_broadcast_service + admin_request, sample_broadcast_service, has_email_auth, ending_permissions ): + if has_email_auth: + service_permission = ServicePermission(service_id=sample_broadcast_service.id, permission=EMAIL_AUTH_TYPE) + sample_broadcast_service.permissions.append(service_permission) + current_permissions = [p.permission for p in sample_broadcast_service.permissions] - assert current_permissions == [BROADCAST_TYPE] + assert set(current_permissions) == set(ending_permissions) data = { 'broadcast_channel': "severe", @@ -3807,10 +3819,10 @@ def test_set_as_broadcast_service_maintains_broadcast_permission_for_existing_br service_id=sample_broadcast_service.id, _data=data, ) - assert result['data']['permissions'] == [BROADCAST_TYPE] + assert set(result['data']['permissions']) == set(ending_permissions) permissions = ServicePermission.query.filter_by(service_id=sample_broadcast_service.id).all() - assert [p.permission for p in permissions] == [BROADCAST_TYPE] + assert set([p.permission for p in permissions]) == set(ending_permissions) def test_set_as_broadcast_service_sets_count_as_live_to_false( From fcf667f09cdc310c4f717f97ba6e256ff5cd6132 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 3 Mar 2021 18:22:41 +0000 Subject: [PATCH 2/2] Add test coverage for setting service permission Spotted that we aren't testing all our permission types here so added this one in. It includes the TODO for allowing the API to give a service the broadcast permission. We don't want this to happen, we want them to use the `set_as_broadcast_service` route instead. We will probably get away with it for the moment for it would be tighter validation we should add to reduce the risk of letting a service get in a dodgy state. --- tests/app/service/test_rest.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 0622dca67..d3db9083b 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -973,6 +973,8 @@ def test_update_service_permissions_will_add_service_permissions(client, sample_ (INTERNATIONAL_SMS_TYPE), (LETTER_TYPE), (INBOUND_SMS_TYPE), + (EMAIL_AUTH_TYPE), + (BROADCAST_TYPE), # TODO: remove this ability to set broadcast permission this way ] ) def test_add_service_permission_will_add_permission(client, service_with_no_permissions, permission_to_add):