From fed40326da944e3a6e763c2920a09bb98e80b9cb Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 4 Mar 2021 16:54:41 +0000 Subject: [PATCH 1/2] Fix test assertion It wasn't varying the email address seen in the header based on the two parametrized cases. Not the end of the world, but we should make it correct --- tests/app/main/views/test_manage_users.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 83e09232e..f5ae9b2e8 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1004,7 +1004,7 @@ def test_invite_user( mock_get_template_folders, mock_get_organisations, ): - sample_invite['email_address'] = 'test@example.gov.uk' + sample_invite['email_address'] = email_address assert is_gov_user(email_address) == gov_user mocker.patch('app.models.user.InvitedUsers.client_method', return_value=[sample_invite]) @@ -1027,7 +1027,7 @@ def test_invite_user( ) assert page.h1.string.strip() == 'Team members' flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() - assert flash_banner == 'Invite sent to test@example.gov.uk' + assert flash_banner == f"Invite sent to {email_address}" expected_permissions = {'manage_api_keys', 'manage_service', 'manage_templates', 'send_messages', 'view_activity'} From 7ae4017d50f83f1e3edb3f2d43ab89b753cc9afa Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 5 Mar 2021 12:19:36 +0000 Subject: [PATCH 2/2] Add audit event for inviting users to a service --- app/event_handlers.py | 9 +++++++++ app/main/views/invites.py | 1 + app/main/views/verify.py | 7 ++++++- app/models/user.py | 9 ++++++++- tests/app/main/views/test_accept_invite.py | 23 +++++++++++++++++++++- tests/app/main/views/test_register.py | 9 +++++++++ tests/app/test_event_handlers.py | 21 ++++++++++++++++++++ 7 files changed, 76 insertions(+), 3 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 2ac6ca687..526b57481 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -34,6 +34,15 @@ def create_remove_user_from_service_event(user_id, removed_by_id, service_id): ) +def create_add_user_to_service_event(user_id, invited_by_id, service_id): + _send_event( + 'add_user_to_service', + user_id=user_id, + invited_by_id=invited_by_id, + service_id=service_id + ) + + def create_archive_user_event(user_id, archived_by_id): _send_event( 'archive_user', diff --git a/app/main/views/invites.py b/app/main/views/invites.py index a35b8c1f7..329f1d722 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -68,6 +68,7 @@ def accept_invite(token): service_id=invited_user.service, permissions=invited_user.permissions, folder_permissions=invited_user.folder_permissions, + invited_by_id=invited_user.from_user.id, ) if service.has_permission('broadcast'): return redirect(url_for('main.broadcast_tour', service_id=service.id, step_index=1)) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index bb90f1de5..61c9b64f2 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -97,5 +97,10 @@ def _add_invited_user_to_service(invited_user): invitation = InvitedUser(invited_user) user = User.from_id(session['user_id']) service_id = invited_user['service'] - user.add_to_service(service_id, invitation.permissions, invitation.folder_permissions) + user.add_to_service( + service_id, + invitation.permissions, + invitation.folder_permissions, + invitation.from_user.id, + ) return service_id diff --git a/app/models/user.py b/app/models/user.py index d3fd5a3aa..ff93e1eb9 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -405,7 +405,9 @@ class User(JSONModel, UserMixin): self.current_session_id = user_api_client.get_user(self.id).get('current_session_id') session['current_session_id'] = self.current_session_id - def add_to_service(self, service_id, permissions, folder_permissions): + def add_to_service(self, service_id, permissions, folder_permissions, invited_by_id): + from app.event_handlers import create_add_user_to_service_event + try: user_api_client.add_user_to_service( service_id, @@ -413,6 +415,11 @@ class User(JSONModel, UserMixin): permissions, folder_permissions, ) + create_add_user_to_service_event( + user_id=self.id, + invited_by_id=invited_by_id, + service_id=service_id, + ) except HTTPError as exception: if exception.status_code == 400 and 'already part of service' in exception.message: pass diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index ed3c2e0e3..7fe84def9 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -27,9 +27,12 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( mock_add_user_to_service, mock_get_service, mocker, + mock_events, + mock_get_user, ): expected_service = service_one['id'] expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} + mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -45,6 +48,11 @@ 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=USER_ONE_ID, + ) def test_existing_user_with_no_permissions_or_folder_permissions_accept_invite( @@ -58,6 +66,8 @@ def test_existing_user_with_no_permissions_or_folder_permissions_accept_invite( mock_get_users_by_service, mock_add_user_to_service, mock_get_service, + mock_events, + mock_get_user, ): expected_service = service_one['id'] sample_invite['permissions'] = '' @@ -204,7 +214,8 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa sample_invite, mock_accept_invite, mock_get_service, - mock_get_users_by_service + mock_get_users_by_service, + mock_get_user, ): sample_invite['email_address'] = api_user_active['email_address'] @@ -212,6 +223,7 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa # `existing_user in Users(invited_user.service)` returns False and the right code path is tested mocker.patch('app.user_api_client.get_user_by_email', return_value=create_api_user_active(with_unique_id=True)) mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) + mock_audit_event = mocker.patch('app.event_handlers.create_add_user_to_service_event') mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( response=Mock( @@ -226,6 +238,7 @@ 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( @@ -240,6 +253,8 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( mock_accept_invite, mock_get_service, mocker, + mock_events, + mock_get_user, ): expected_service = service_one['id'] expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} @@ -513,6 +528,8 @@ def test_new_invited_user_verifies_and_added_to_service( mock_create_event, mocker, ): + mock_audit_event = mocker.patch('app.event_handlers.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 @@ -547,6 +564,10 @@ 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 618de2856..576c4257e 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -365,7 +365,12 @@ def test_register_from_email_auth_invite( mock_add_user_to_service, mock_get_service, invite_email_address, + service_one, + fake_uuid, + mocker, ): + mock_audit_event = mocker.patch('app.event_handlers.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: @@ -401,6 +406,10 @@ 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'] == sample_invite diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 254931117..d02bf36c9 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -2,6 +2,7 @@ import uuid from unittest.mock import ANY from app.event_handlers import ( + create_add_user_to_service_event, create_archive_user_event, create_broadcast_account_type_change_event, create_email_change_event, @@ -39,6 +40,26 @@ def test_create_email_change_event_calls_events_api(app_, mock_events): 'new_email_address': 'new@example.com'}) +def test_create_add_user_to_service_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + invited_by_id = str(uuid.uuid4()) + service_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_add_user_to_service_event(user_id, invited_by_id, service_id) + + mock_events.assert_called_with( + 'add_user_to_service', + { + 'browser_fingerprint': {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'invited_by_id': invited_by_id, + 'service_id': service_id, + } + ) + + def test_create_remove_user_from_service_event_calls_events_api(app_, mock_events): user_id = str(uuid.uuid4()) removed_by_id = str(uuid.uuid4())