From 9fafc092f72a4fcd554c6edbda48d8f92348100c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 15 Jul 2021 12:13:42 +0100 Subject: [PATCH] Audit permissions when adding a user to a service This is useful information to store for the event, which would be lost if someone subsequently changed them. Rather than updating lots of mock assertions, I've replaced them with a single test / assert at a lower level, which is consistent with auditing being a non-critical function. --- app/event_handlers.py | 2 +- app/models/user.py | 1 + tests/app/main/views/test_accept_invite.py | 15 --------------- tests/app/main/views/test_register.py | 6 ------ tests/app/models/test_user.py | 20 ++++++++++++++++++++ tests/app/test_event_handlers.py | 3 ++- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 63a7acebe..45a6c6f10 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -7,7 +7,7 @@ EVENT_SCHEMAS = { "update_user_email": {"user_id", "updated_by_id", "original_email_address", "new_email_address"}, "update_user_mobile_number": {"user_id", "updated_by_id", "original_mobile_number", "new_mobile_number"}, "remove_user_from_service": {"user_id", "removed_by_id", "service_id"}, - "add_user_to_service": {"user_id", "invited_by_id", "service_id"}, + "add_user_to_service": {"user_id", "invited_by_id", "service_id", "admin_roles"}, "set_user_permissions": {"user_id", "service_id", "original_admin_roles", "new_admin_roles", "set_by_id"}, "archive_user": {"user_id", "archived_by_id"}, "change_broadcast_account_type": {"service_id", "changed_by_id", "service_mode", "broadcast_channel", "provider_restriction"}, # noqa: E501 (length) diff --git a/app/models/user.py b/app/models/user.py index 98c6770d1..fac181cf5 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -425,6 +425,7 @@ class User(JSONModel, UserMixin): user_id=self.id, invited_by_id=invited_by_id, service_id=service_id, + admin_roles=permissions, ) except HTTPError as exception: if exception.status_code == 400 and 'already part of service' in exception.message: diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index cdf71c616..73e8fcd9b 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -46,7 +46,6 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( ): expected_service = service_one['id'] expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -62,11 +61,6 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=expected_service, _external=True) - mock_audit_event.assert_called_once_with( - invited_by_id=service_one['users'][0], - service_id=SERVICE_ONE_ID, - user_id=api_user_active['id'], - ) @pytest.mark.parametrize('trial_mode, expected_endpoint', ( @@ -270,8 +264,6 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa mock_no_users_for_service, mock_get_user, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( response=Mock( status_code=400, @@ -285,7 +277,6 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=False) assert response.location == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID, _external=True) - assert not mock_audit_event.called def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( @@ -574,8 +565,6 @@ def test_new_invited_user_verifies_and_added_to_service( mock_create_event, mocker, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - # visit accept token page response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) assert response.status_code == 302 @@ -611,10 +600,6 @@ def test_new_invited_user_verifies_and_added_to_service( mock_check_verify_code.assert_called_once_with(new_user_id, '12345', 'sms') assert service_one['id'] == session['service_id'] - mock_audit_event.assert_called_once_with(invited_by_id=service_one['users'][0], - service_id=service_one['id'], - user_id=new_user_id) - raw_html = response.data.decode('utf-8') page = BeautifulSoup(raw_html, 'html.parser') assert page.find('h1').text == 'Dashboard' diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 8c83c258d..74e42743d 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -334,8 +334,6 @@ def test_register_from_email_auth_invite( fake_uuid, mocker, ): - mock_audit_event = mocker.patch('app.models.user.create_add_user_to_service_event') - sample_invite['auth_type'] = 'email_auth' sample_invite['email_address'] = invite_email_address with client.session_transaction() as session: @@ -373,10 +371,6 @@ def test_register_from_email_auth_invite( assert current_user.is_authenticated assert mock_add_user_to_service.called - mock_audit_event.assert_called_once_with(invited_by_id=service_one['users'][0], - service_id=sample_invite['service'], - user_id=fake_uuid) - with client.session_transaction() as session: # invited user details are still there so they can get added to the service assert session['invited_user_id'] == sample_invite['id'] diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index fe038b64e..df6fb9c91 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -156,3 +156,23 @@ def test_set_permissions(client, mocker, active_user_view_permissions, fake_uuid new_admin_roles={'manage_templates'}, set_by_id=fake_uuid, ) + + +def test_add_to_service(client, mocker, api_user_active, fake_uuid): + mock_api = mocker.patch('app.models.user.user_api_client.add_user_to_service') + mock_event = mocker.patch('app.models.user.create_add_user_to_service_event') + + User(api_user_active).add_to_service( + service_id=SERVICE_ONE_ID, + permissions={'manage_templates'}, + folder_permissions=[], + invited_by_id=fake_uuid, + ) + + mock_api.assert_called_once() + mock_event.assert_called_once_with( + service_id=SERVICE_ONE_ID, + user_id=api_user_active['id'], + invited_by_id=fake_uuid, + admin_roles={'manage_templates'}, + ) diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 8def2653a..10ff5518e 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -49,7 +49,8 @@ def test_create_add_user_to_service_event_calls_events_api(client, mock_events): kwargs = { "user_id": str(uuid.uuid4()), "invited_by_id": str(uuid.uuid4()), - "service_id": str(uuid.uuid4()) + "service_id": str(uuid.uuid4()), + "admin_roles": {'manage_templates'}, } create_add_user_to_service_event(**kwargs)