From 88e9a0ff6162f3b0694746a5741c96db21dbd593 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 3 Apr 2019 11:47:46 +0100 Subject: [PATCH 1/2] Add audit event when a service manager changes someones profile We should audit when a service manager changes a user profile that is not their own. This can be recorded in our events table, which is currently only used to record successful logins. This adds two new types of event, `update_user_email` and `update_user_mobile_number` which store the - browser fingerprint - IP address - user id of the user being updated - user id of the service manager making the change - original email address and new email address (for `update_user_email` events) - original mobile number and new mobile number (for `update_user_mobile_number` events) --- app/event_handlers.py | 35 ++++++++++++-- app/main/views/manage_users.py | 8 ++++ tests/app/main/views/test_manage_users.py | 56 ++++++++++++++++++----- tests/app/test_event_handlers.py | 41 ++++++++++++++++- 4 files changed, 123 insertions(+), 17 deletions(-) diff --git a/app/event_handlers.py b/app/event_handlers.py index 650d72d52..6a6124513 100644 --- a/app/event_handlers.py +++ b/app/event_handlers.py @@ -3,18 +3,43 @@ from flask import request from app import events_api_client -def on_user_logged_in(sender, user): - _send_event(sender, event_type='sucessful_login', user=user) +def on_user_logged_in(_sender, user): + _send_event(event_type='sucessful_login', user_id=user.id) -def _send_event(sender, **kwargs): +def create_email_change_event(user_id, updated_by_id, original_email_address, new_email_address): + _send_event( + event_type='update_user_email', + user_id=user_id, + updated_by_id=updated_by_id, + original_email_address=original_email_address, + new_email_address=new_email_address) + + +def create_mobile_number_change_event(user_id, updated_by_id, original_mobile_number, new_mobile_number): + _send_event( + event_type='update_user_mobile_number', + user_id=user_id, + updated_by_id=updated_by_id, + original_mobile_number=original_mobile_number, + new_mobile_number=new_mobile_number) + + +def _send_event(**kwargs): if not kwargs.get('event_type'): return event_data = _construct_event_data(request) + event_fields = ('user_id', + 'updated_by_id', + 'original_email_address', + 'new_email_address', + 'original_mobile_number', + 'new_mobile_number') - if kwargs.get('user'): - event_data['user_id'] = kwargs.get('user').id + for field in event_fields: + if kwargs.get(field): + event_data[field] = kwargs[field] events_api_client.create_event(kwargs['event_type'], event_data) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 30cab629a..7dd69bcde 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -16,6 +16,10 @@ from app import ( service_api_client, user_api_client, ) +from app.event_handlers import ( + create_email_change_event, + create_mobile_number_change_event, +) from app.main import main from app.main.forms import ( ChangeEmailForm, @@ -196,6 +200,8 @@ def confirm_edit_user_email(service_id, user_id): user_api_client.update_user_attribute(str(user_id), email_address=new_email, updated_by=current_user.id) except HTTPError as e: abort(500, e) + else: + create_email_change_event(user.id, current_user.id, user.email_address, new_email) finally: session.pop('team_member_email_change', None) @@ -254,6 +260,8 @@ def confirm_edit_user_mobile_number(service_id, user_id): user_api_client.update_user_attribute(str(user_id), mobile_number=new_number, updated_by=current_user.id) except HTTPError as e: abort(500, e) + else: + create_mobile_number_change_event(user.id, current_user.id, user.mobile_number, new_number) finally: session.pop('team_member_mobile_change', None) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 5450e647d..fa4007a01 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1112,19 +1112,30 @@ def test_confirm_edit_user_email_page_404s_for_non_team_member( def test_confirm_edit_user_email_changes_user_email( client_request, active_user_with_permissions, - mock_get_users_by_service, + api_user_active, service_one, mocker, - mock_get_user, - mock_update_user_attribute + mock_update_user_attribute, ): + # We want active_user_with_permissions (the current user) to update the email address for api_user_active + # By default both users would have the same id, so we change the id of api_user_active + api_user_active.id = str(uuid.uuid4()) + mocker.patch('app.user_api_client.get_users_for_service', + return_value=[api_user_active, active_user_with_permissions]) + # get_user gets called twice - first to check if current user can see the page, then to see if the team member + # whose email address we're changing belongs to the service + mocker.patch('app.models.service.user_api_client.get_user', + side_effect=[active_user_with_permissions, api_user_active]) + mock_event_handler = mocker.patch('app.main.views.manage_users.create_email_change_event') + new_email = 'new_email@gov.uk' with client_request.session_transaction() as session: session['team_member_email_change'] = new_email + client_request.post( 'main.confirm_edit_user_email', service_id=service_one['id'], - user_id=active_user_with_permissions.id, + user_id=api_user_active.id, _expected_status=302, _expected_redirect=url_for( 'main.manage_users', @@ -1132,11 +1143,17 @@ def test_confirm_edit_user_email_changes_user_email( _external=True, ), ) + mock_update_user_attribute.assert_called_once_with( - active_user_with_permissions.id, + api_user_active.id, email_address=new_email, - updated_by=mocker.ANY + updated_by=active_user_with_permissions.id ) + mock_event_handler.assert_called_once_with( + api_user_active.id, + active_user_with_permissions.id, + api_user_active.email_address, + new_email) def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( @@ -1313,19 +1330,31 @@ def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty( def test_confirm_edit_user_mobile_number_changes_user_mobile_number( client_request, active_user_with_permissions, - mock_get_users_by_service, + api_user_active, service_one, mocker, - mock_get_user, mock_update_user_attribute ): + # We want active_user_with_permissions (the current user) to update the mobile number for api_user_active + # By default both users would have the same id, so we change the id of api_user_active + api_user_active.id = str(uuid.uuid4()) + + mocker.patch('app.user_api_client.get_users_for_service', + return_value=[api_user_active, active_user_with_permissions]) + # get_user gets called twice - first to check if current user can see the page, then to see if the team member + # whose mobile number we're changing belongs to the service + mocker.patch('app.models.service.user_api_client.get_user', + side_effect=[active_user_with_permissions, api_user_active]) + mock_event_handler = mocker.patch('app.main.views.manage_users.create_mobile_number_change_event') + new_number = '07554080636' with client_request.session_transaction() as session: session['team_member_mobile_change'] = new_number + client_request.post( 'main.confirm_edit_user_mobile_number', service_id=SERVICE_ONE_ID, - user_id=active_user_with_permissions.id, + user_id=api_user_active.id, _expected_status=302, _expected_redirect=url_for( 'main.manage_users', @@ -1334,10 +1363,15 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( ), ) mock_update_user_attribute.assert_called_once_with( - active_user_with_permissions.id, + api_user_active.id, mobile_number=new_number, - updated_by=mocker.ANY + updated_by=active_user_with_permissions.id ) + mock_event_handler.assert_called_once_with( + api_user_active.id, + active_user_with_permissions.id, + api_user_active.mobile_number, + new_number) def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member( diff --git a/tests/app/test_event_handlers.py b/tests/app/test_event_handlers.py index 7f1ae1fb7..22d398186 100644 --- a/tests/app/test_event_handlers.py +++ b/tests/app/test_event_handlers.py @@ -1,6 +1,11 @@ +import uuid from unittest.mock import ANY -from app.event_handlers import on_user_logged_in +from app.event_handlers import ( + create_email_change_event, + create_mobile_number_change_event, + on_user_logged_in, +) def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): @@ -11,3 +16,37 @@ def test_on_user_logged_in_calls_events_api(app_, api_user_active, mock_events): {'browser_fingerprint': {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, 'ip_address': ANY, 'user_id': str(api_user_active.id)}) + + +def test_create_email_change_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + updated_by_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_email_change_event(user_id, updated_by_id, 'original@example.com', 'new@example.com') + + mock_events.assert_called_with('update_user_email', + {'browser_fingerprint': + {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'updated_by_id': updated_by_id, + 'original_email_address': 'original@example.com', + 'new_email_address': 'new@example.com'}) + + +def test_create_mobile_number_change_event_calls_events_api(app_, mock_events): + user_id = str(uuid.uuid4()) + updated_by_id = str(uuid.uuid4()) + + with app_.test_request_context(): + create_mobile_number_change_event(user_id, updated_by_id, '07700900000', '07700900999') + + mock_events.assert_called_with('update_user_mobile_number', + {'browser_fingerprint': + {'browser': ANY, 'version': ANY, 'platform': ANY, 'user_agent_string': ''}, + 'ip_address': ANY, + 'user_id': user_id, + 'updated_by_id': updated_by_id, + 'original_mobile_number': '07700900000', + 'new_mobile_number': '07700900999'}) From 19fe587640cf880cf4601984444bbeeb9e743b4e Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 3 Apr 2019 15:53:30 +0100 Subject: [PATCH 2/2] Make InvitedUser class always expect folder_permissions to be returned InvitedUsers now always have folder_permissions (if they can't view any folders their folder permissions will be `[]`). --- app/models/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.py b/app/models/user.py index b74c8c2af..e54315ed5 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -240,7 +240,7 @@ class InvitedUser(object): self.created_at = created_at self.auth_type = auth_type self.permissions = translate_permissions_from_db_to_admin_roles(self.permissions) - self.folder_permissions = folder_permissions or [] + self.folder_permissions = folder_permissions def has_permissions(self, *permissions): if self.status == 'cancelled':