From 0ce539704e7d3698ee33ecb17eaeba7e6356e68f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 25 Feb 2021 10:27:39 +0000 Subject: [PATCH] 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(