From 6d0d9d46f736f7d50eb73e9fc58ab680fd87a5e5 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 15:58:33 +0100 Subject: [PATCH 1/7] Prevent switching auth type for Platform Admins This closes a security loophole, where the auth type of a Platform Admin could be unwittingly changed when they accept an invite, or by an admin of a service they are a member of. --- app/main/views/invites.py | 19 +++++----- app/main/views/manage_users.py | 4 +- .../views/manage-users/permissions.html | 6 ++- tests/app/main/views/test_accept_invite.py | 38 +++++++++++++++++++ tests/app/main/views/test_manage_users.py | 36 ++++++++++++++++++ 5 files changed, 92 insertions(+), 11 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 1c4446235..baa61ece6 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -57,15 +57,16 @@ def accept_invite(token): return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) else: service = Service.from_id(invited_user.service) - # if the service you're being added to can modify auth type, then check if this is relevant - if service.has_permission('email_auth') and ( - # they have a phone number, we want them to start using it. if they dont have a mobile we just - # ignore that option of the invite - (existing_user.mobile_number and invited_user.auth_type == 'sms_auth') or - # we want them to start sending emails. it's always valid, so lets always update - invited_user.auth_type == 'email_auth' - ): - existing_user.update(auth_type=invited_user.auth_type) + # if the service you're being added to can modify auth type, then check if we can do this; + # if the user is a Platform Admin, we silently leave this unchanged to prevent a security + # issue where someone could switch their auth type to something less secure + if service.has_permission('email_auth') and not existing_user.platform_admin: + if invited_user.auth_type == 'email_auth' or ( + # they have a phone number, we want them to start using it. + # if they dont have a mobile we just ignore that option of the invite + existing_user.mobile_number and invited_user.auth_type == 'sms_auth' + ): + existing_user.update(auth_type=invited_user.auth_type) existing_user.add_to_service( service_id=invited_user.service, permissions=invited_user.permissions, diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 0253ae1a9..8140288bc 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -142,7 +142,9 @@ def edit_user_permissions(service_id, user_id): permissions=form.permissions, folder_permissions=form.folder_permissions.data, ) - if service_has_email_auth: + # only change the auth type if this is supported for a service; for Platform Admin users, + # we avoid changing the auth type to prevent it being switched to something less secure + if service_has_email_auth and not user.platform_admin: user.update(auth_type=form.login_authentication.data) return redirect(url_for('.manage_users', service_id=service_id)) diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 77189dcb0..db64051c9 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -11,7 +11,11 @@ {% endif %} {% if service_has_email_auth %} - {% if not mobile_number %} + {% if user.platform_admin %} +

+ Platform admin users will login with a security key. +

+ {% elif not mobile_number %} {{ radios( form.login_authentication, disable=['sms_auth'], diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 693e9b24f..0ccc7fb31 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -1,4 +1,5 @@ from unittest.mock import ANY, Mock +from uuid import uuid4 import pytest from bs4 import BeautifulSoup @@ -690,6 +691,43 @@ def test_existing_user_accepts_and_sets_email_auth( mock_add_user_to_service.assert_called_once_with(ANY, USER_ONE_ID, ANY, ANY) +def test_platform_admin_user_accepts_and_preserves_auth( + client_request, + platform_admin_user, + service_one, + sample_invite, + mock_get_users_by_service, + mock_accept_invite, + mock_update_user_attribute, + mock_add_user_to_service, + mocker +): + sample_invite['email_address'] = platform_admin_user['email_address'] + sample_invite['auth_type'] = 'email_auth' + service_one['permissions'].append('email_auth') + + # mock_get_users_by_service uses the same global UUID as the platform admin user, + # so we need to reset it to pretend this user isn't a member of the service + platform_admin_user['id'] = uuid4() + platform_admin_user['auth_type'] = 'webauthn_auth' + + # mock_get_unknown_user_by_email returns the active_api_user, which we don't want + mocker.patch('app.user_api_client.get_user_by_email', return_value=platform_admin_user) + mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) + + client_request.login(platform_admin_user) + + client_request.get( + 'main.accept_invite', + token='thisisnotarealtoken', + _expected_status=302, + _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), + ) + + assert not mock_update_user_attribute.called + assert mock_add_user_to_service.called + + def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( client_request, api_user_active, diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 186eac9d6..44fbddb0f 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -795,6 +795,42 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service( ) +@pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth']) +def test_edit_user_permissions_preserves_auth_type_for_platform_admin( + client_request, + service_one, + platform_admin_user, + mock_get_users_by_service, + mock_get_invites_for_service, + mock_set_user_permissions, + mock_update_user_attribute, + auth_type, + mock_get_template_folders +): + service_one['permissions'].append('email_auth') + client_request.login(platform_admin_user) + + client_request.post( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=platform_admin_user['id'], + _data={ + 'email_address': platform_admin_user['email_address'], + 'permissions_field': [], + 'login_authentication': auth_type, + }, + _expected_status=302, + ) + + mock_set_user_permissions.assert_called_with( + str(platform_admin_user['id']), + SERVICE_ONE_ID, + permissions=set(), + folder_permissions=[], + ) + mock_update_user_attribute.assert_not_called() + + def test_should_show_page_for_inviting_user( client_request, mock_get_template_folders, From 1dcfd5ba955da394f3c33ca6f720c18457328dbf Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 16:54:37 +0100 Subject: [PATCH 2/7] Refactor accept invite test to avoid override This replaces the original fixture with a more explicit one, noting that none of the tests rely on this fixture as part of testing the scenarios when a user is already a member of the service. --- tests/app/main/views/test_accept_invite.py | 42 +++++++++++----------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 0ccc7fb31..36c2ec1a2 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -1,5 +1,4 @@ from unittest.mock import ANY, Mock -from uuid import uuid4 import pytest from bs4 import BeautifulSoup @@ -18,13 +17,18 @@ from tests.conftest import ( ) +@pytest.fixture() +def mock_no_users_for_service(mocker): + mocker.patch('app.models.user.Users.client_method', return_value=[]) + + def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( client, service_one, api_user_active, mock_check_invite_token, mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, mock_get_service, @@ -66,7 +70,7 @@ def test_broadcast_service_shows_tour( service_one, mock_check_invite_token, mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, mocker, @@ -103,7 +107,7 @@ def test_existing_user_with_no_permissions_or_folder_permissions_accept_invite( sample_invite, mock_check_invite_token, mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_add_user_to_service, mock_get_service, mock_events, @@ -153,7 +157,7 @@ def test_invite_goes_in_session( api_user_active, mock_check_invite_token, mock_get_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_add_user_to_service, mock_accept_invite, ): @@ -188,7 +192,7 @@ def test_accepting_invite_removes_invite_from_session( service_one, mock_check_invite_token, mock_get_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_add_user_to_service, mock_accept_invite, mock_get_service_templates, @@ -255,7 +259,7 @@ 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_no_users_for_service, mock_get_user, ): sample_invite['email_address'] = api_user_active['email_address'] @@ -289,7 +293,7 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( sample_invite, mock_check_invite_token, mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_add_user_to_service, mock_accept_invite, mock_get_service, @@ -327,7 +331,7 @@ def test_new_user_accept_invite_calls_api_and_redirects_to_registration( mock_check_invite_token, mock_dont_get_user_by_email, mock_add_user_to_service, - mock_get_users_by_service, + mock_no_users_for_service, mock_get_service, mocker, ): @@ -349,7 +353,7 @@ def test_new_user_accept_invite_calls_api_and_views_registration_page( mock_dont_get_user_by_email, mock_get_invited_user_by_id, mock_add_user_to_service, - mock_get_users_by_service, + mock_no_users_for_service, mock_get_service, mocker, ): @@ -446,7 +450,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( mock_send_verify_code, mock_get_invited_user_by_id, mock_accept_invite, - mock_get_users_by_service, + mock_no_users_for_service, mock_add_user_to_service, mock_get_service, mocker, @@ -559,7 +563,7 @@ def test_new_invited_user_verifies_and_added_to_service( mock_get_template_statistics, mock_has_no_jobs, mock_has_permissions, - mock_get_users_by_service, + mock_no_users_for_service, mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, @@ -667,7 +671,7 @@ def test_existing_user_accepts_and_sets_email_auth( service_one, sample_invite, mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -696,7 +700,7 @@ def test_platform_admin_user_accepts_and_preserves_auth( platform_admin_user, service_one, sample_invite, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -705,10 +709,6 @@ def test_platform_admin_user_accepts_and_preserves_auth( sample_invite['email_address'] = platform_admin_user['email_address'] sample_invite['auth_type'] = 'email_auth' service_one['permissions'].append('email_auth') - - # mock_get_users_by_service uses the same global UUID as the platform admin user, - # so we need to reset it to pretend this user isn't a member of the service - platform_admin_user['id'] = uuid4() platform_admin_user['auth_type'] = 'webauthn_auth' # mock_get_unknown_user_by_email returns the active_api_user, which we don't want @@ -734,7 +734,7 @@ def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( service_one, sample_invite, mock_get_user_by_email, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -762,7 +762,7 @@ def test_existing_email_auth_user_without_phone_cannot_set_sms_auth( api_user_active, service_one, sample_invite, - mock_get_users_by_service, + mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -794,7 +794,7 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( api_user_active, service_one, sample_invite, - mock_get_users_by_service, + mock_no_users_for_service, mock_get_unknown_user_by_email, mock_accept_invite, mock_update_user_attribute, From ef2996d56a843e254a022fabcccc5af7bdac197b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 17:00:45 +0100 Subject: [PATCH 3/7] Localise fixture to the only test that uses it --- tests/app/main/views/test_accept_invite.py | 11 +++++++++++ tests/conftest.py | 11 ----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 36c2ec1a2..452ebba4e 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -22,6 +22,17 @@ def mock_no_users_for_service(mocker): mocker.patch('app.models.user.Users.client_method', return_value=[]) +@pytest.fixture(scope='function') +def mock_get_unknown_user_by_email(mocker, api_user_active): + api_user_active['id'] = USER_ONE_ID + + def _get_user(email_address): + api_user_active['email_address'] = email_address + return api_user_active + + return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) + + def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index 62fd5166a..85bd085af 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1500,17 +1500,6 @@ def mock_get_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) -@pytest.fixture(scope='function') -def mock_get_unknown_user_by_email(mocker, api_user_active): - api_user_active['id'] = USER_ONE_ID - - def _get_user(email_address): - api_user_active['email_address'] = email_address - return api_user_active - - return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) - - @pytest.fixture(scope='function') def mock_dont_get_user_by_email(mocker): def _get_user(email_address): From c696693785732305740a7ded3e0884d96c1c40b2 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 17:20:04 +0100 Subject: [PATCH 4/7] Simplify mocking and asserting the existing user Previously we made surprising changed to the invited user as part of the mock, and then surprising assertions that its ID matched USER_ONE_ID. This simplifies the mock to do what it says, so that we can test for the original ID of the existing user.* *this does still differ from the ID of the sample_invite, which is also hard-coded to USER_ONE_ID. However, this isn't relevant in any of the tests, so doesn't seem to much of an issue. --- tests/app/main/views/test_accept_invite.py | 53 ++++++++++------------ 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 452ebba4e..c26b5b5da 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -9,7 +9,6 @@ import app from tests import service_json from tests.conftest import ( SERVICE_ONE_ID, - USER_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, create_api_user_active, @@ -23,14 +22,8 @@ def mock_no_users_for_service(mocker): @pytest.fixture(scope='function') -def mock_get_unknown_user_by_email(mocker, api_user_active): - api_user_active['id'] = USER_ONE_ID - - def _get_user(email_address): - api_user_active['email_address'] = email_address - return api_user_active - - return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) +def mock_get_existing_user_by_email(mocker, api_user_active): + return mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_active) def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( @@ -38,7 +31,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( service_one, api_user_active, mock_check_invite_token, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, @@ -54,11 +47,11 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) mock_check_invite_token.assert_called_with('thisisnotarealtoken') - mock_get_unknown_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_get_existing_user_by_email.assert_called_with('invited_user@test.gov.uk') assert mock_accept_invite.call_count == 1 mock_add_user_to_service.assert_called_with( expected_service, - USER_ONE_ID, + api_user_active['id'], expected_permissions, [], ) @@ -68,7 +61,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( mock_audit_event.assert_called_once_with( invited_by_id=service_one['users'][0], service_id=SERVICE_ONE_ID, - user_id=USER_ONE_ID, + user_id=api_user_active['id'], ) @@ -80,7 +73,7 @@ def test_broadcast_service_shows_tour( client, service_one, mock_check_invite_token, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, @@ -117,7 +110,7 @@ def test_existing_user_with_no_permissions_or_folder_permissions_accept_invite( api_user_active, sample_invite, mock_check_invite_token, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_no_users_for_service, mock_add_user_to_service, mock_get_service, @@ -132,7 +125,7 @@ def test_existing_user_with_no_permissions_or_folder_permissions_accept_invite( response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) mock_add_user_to_service.assert_called_with(expected_service, - USER_ONE_ID, + api_user_active['id'], expected_permissions, expected_folder_permissions) @@ -303,7 +296,7 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( api_user_active, sample_invite, mock_check_invite_token, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_no_users_for_service, mock_add_user_to_service, mock_accept_invite, @@ -318,9 +311,9 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) mock_check_invite_token.assert_called_with('thisisnotarealtoken') - mock_get_unknown_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_get_existing_user_by_email.assert_called_with('invited_user@test.gov.uk') mock_add_user_to_service.assert_called_with(expected_service, - USER_ONE_ID, + api_user_active['id'], expected_permissions, sample_invite['folder_permissions']) assert mock_accept_invite.call_count == 1 @@ -360,6 +353,7 @@ def test_new_user_accept_invite_calls_api_and_redirects_to_registration( def test_new_user_accept_invite_calls_api_and_views_registration_page( client, service_one, + sample_invite, mock_check_invite_token, mock_dont_get_user_by_email, mock_get_invited_user_by_id, @@ -372,7 +366,7 @@ def test_new_user_accept_invite_calls_api_and_views_registration_page( mock_check_invite_token.assert_called_with('thisisnotarealtoken') mock_dont_get_user_by_email.assert_called_with('invited_user@test.gov.uk') - mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) + mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id']) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -472,7 +466,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( with client.session_transaction() as session: assert response.status_code == 302 assert response.location == expected_redirect_location - assert session.get('invited_user_id') == USER_ONE_ID + assert session.get('invited_user_id') == sample_invite['id'] data = {'service': sample_invite['service'], 'email_address': sample_invite['email_address'], @@ -489,7 +483,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( assert response.location == expected_redirect_location mock_send_verify_code.assert_called_once_with(ANY, 'sms', data['mobile_number']) - mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) + mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id']) mock_register_user.assert_called_with(data['name'], data['email_address'], @@ -681,7 +675,7 @@ def test_existing_user_accepts_and_sets_email_auth( api_user_active, service_one, sample_invite, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, @@ -701,9 +695,9 @@ def test_existing_user_accepts_and_sets_email_auth( _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), ) - mock_get_unknown_user_by_email.assert_called_once_with('test@user.gov.uk') - mock_update_user_attribute.assert_called_once_with(USER_ONE_ID, auth_type='email_auth') - mock_add_user_to_service.assert_called_once_with(ANY, USER_ONE_ID, ANY, ANY) + mock_get_existing_user_by_email.assert_called_once_with('test@user.gov.uk') + mock_update_user_attribute.assert_called_once_with(api_user_active['id'], auth_type='email_auth') + mock_add_user_to_service.assert_called_once_with(ANY, api_user_active['id'], ANY, ANY) def test_platform_admin_user_accepts_and_preserves_auth( @@ -722,7 +716,6 @@ def test_platform_admin_user_accepts_and_preserves_auth( service_one['permissions'].append('email_auth') platform_admin_user['auth_type'] = 'webauthn_auth' - # mock_get_unknown_user_by_email returns the active_api_user, which we don't want mocker.patch('app.user_api_client.get_user_by_email', return_value=platform_admin_user) mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) @@ -806,7 +799,7 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( service_one, sample_invite, mock_no_users_for_service, - mock_get_unknown_user_by_email, + mock_get_existing_user_by_email, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -825,5 +818,5 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), ) - mock_get_unknown_user_by_email.assert_called_once_with(sample_invite['email_address']) - mock_update_user_attribute.assert_called_once_with(USER_ONE_ID, auth_type='sms_auth') + mock_get_existing_user_by_email.assert_called_once_with(sample_invite['email_address']) + mock_update_user_attribute.assert_called_once_with(api_user_active['id'], auth_type='sms_auth') From eb343e493745ea4f2ebd49974edcfc437b35fb99 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 17:27:30 +0100 Subject: [PATCH 5/7] Simplify test for API error with existing user This is now covered since we use 'mock_no_users_for_service'. --- tests/app/main/views/test_accept_invite.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index c26b5b5da..8a3a6229c 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -11,7 +11,6 @@ from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, - create_api_user_active, normalize_spaces, ) @@ -261,16 +260,12 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa mocker, api_user_active, sample_invite, + mock_get_existing_user_by_email, mock_accept_invite, mock_get_service, mock_no_users_for_service, mock_get_user, ): - sample_invite['email_address'] = api_user_active['email_address'] - - # This mock needs to return a user with a different ID to the invited user so that - # `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') From 754f4e3753b4046c351656e681ca7f3941cd28b1 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 17:54:37 +0100 Subject: [PATCH 6/7] Use mock_check_invite_token consistently In some tests the mock was already used, and then overridden but without any change to the behaviour. --- tests/app/main/views/test_accept_invite.py | 23 ++++++++++------------ 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 8a3a6229c..f3b3524e1 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -135,10 +135,10 @@ def test_if_existing_user_accepts_twice_they_redirect_to_sign_in( client, mocker, sample_invite, + mock_check_invite_token, mock_get_service, ): sample_invite['status'] = 'accepted' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) assert response.status_code == 200 @@ -165,7 +165,6 @@ def test_invite_goes_in_session( mock_accept_invite, ): sample_invite['email_address'] = 'test@user.gov.uk' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.get( 'main.accept_invite', @@ -215,7 +214,6 @@ def test_accepting_invite_removes_invite_from_session( ): sample_invite['email_address'] = user['email_address'] - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.login(user) page = client_request.get( @@ -236,10 +234,10 @@ def test_existing_user_of_service_get_redirected_to_signin( sample_invite, mock_get_service, mock_get_user_by_email, + mock_check_invite_token, mock_accept_invite, ): sample_invite['email_address'] = api_user_active['email_address'] - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active]) response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) @@ -261,12 +259,12 @@ def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_pa api_user_active, sample_invite, mock_get_existing_user_by_email, + mock_check_invite_token, mock_accept_invite, mock_get_service, mock_no_users_for_service, mock_get_user, ): - 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( @@ -493,12 +491,12 @@ def test_signed_in_existing_user_cannot_use_anothers_invite( client_request, mocker, api_user_active, + mock_check_invite_token, sample_invite, mock_get_user, mock_accept_invite, mock_get_service, ): - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) mocker.patch('app.user_api_client.get_users_for_service', return_value=[api_user_active]) page = client_request.get( @@ -523,11 +521,11 @@ def test_accept_invite_does_not_treat_email_addresses_as_case_sensitive( api_user_active, sample_invite, mock_accept_invite, + mock_check_invite_token, mock_get_user_by_email ): # the email address of api_user_active is 'test@user.gov.uk' sample_invite['email_address'] = 'TEST@user.gov.uk' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) mocker.patch('app.models.user.Users.client_method', return_value=[api_user_active]) client_request.get( @@ -673,6 +671,7 @@ def test_existing_user_accepts_and_sets_email_auth( mock_get_existing_user_by_email, mock_no_users_for_service, mock_accept_invite, + mock_check_invite_token, mock_update_user_attribute, mock_add_user_to_service, mocker @@ -681,7 +680,6 @@ def test_existing_user_accepts_and_sets_email_auth( service_one['permissions'].append('email_auth') sample_invite['auth_type'] = 'email_auth' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.get( 'main.accept_invite', @@ -700,6 +698,7 @@ def test_platform_admin_user_accepts_and_preserves_auth( platform_admin_user, service_one, sample_invite, + mock_check_invite_token, mock_no_users_for_service, mock_accept_invite, mock_update_user_attribute, @@ -712,7 +711,6 @@ def test_platform_admin_user_accepts_and_preserves_auth( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user_by_email', return_value=platform_admin_user) - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.login(platform_admin_user) @@ -734,6 +732,7 @@ def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( sample_invite, mock_get_user_by_email, mock_no_users_for_service, + mock_check_invite_token, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -744,7 +743,6 @@ def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( assert 'email_auth' not in service_one['permissions'] sample_invite['auth_type'] = 'email_auth' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.get( 'main.accept_invite', @@ -762,6 +760,7 @@ def test_existing_email_auth_user_without_phone_cannot_set_sms_auth( service_one, sample_invite, mock_no_users_for_service, + mock_check_invite_token, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -776,7 +775,6 @@ def test_existing_email_auth_user_without_phone_cannot_set_sms_auth( sample_invite['auth_type'] = 'sms_auth' mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_active) - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) client_request.get( 'main.accept_invite', @@ -795,6 +793,7 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( sample_invite, mock_no_users_for_service, mock_get_existing_user_by_email, + mock_check_invite_token, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -804,8 +803,6 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( service_one['permissions'].append('email_auth') sample_invite['auth_type'] = 'sms_auth' - mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) - client_request.get( 'main.accept_invite', token='thisisnotarealtoken', From 71cbc00a3d0f802c4a6fe09201bfc6334e15728b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 25 May 2021 17:55:52 +0100 Subject: [PATCH 7/7] Localise and simplify fixture to invite tests This isn't used anywhere else. --- tests/app/main/views/test_accept_invite.py | 5 +++++ tests/conftest.py | 8 -------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index f3b3524e1..1fff1a06f 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -25,6 +25,11 @@ def mock_get_existing_user_by_email(mocker, api_user_active): return mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_active) +@pytest.fixture(scope='function') +def mock_check_invite_token(mocker, sample_invite): + return mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) + + def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index 85bd085af..ad3e3d02f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2482,14 +2482,6 @@ def mock_get_invites_without_manage_permission(mocker, service_one, sample_invit return mocker.patch('app.models.user.InvitedUsers.client_method', side_effect=_get_invites) -@pytest.fixture(scope='function') -def mock_check_invite_token(mocker, sample_invite): - def _check_token(token): - return sample_invite - - return mocker.patch('app.invite_api_client.check_token', side_effect=_check_token) - - @pytest.fixture(scope='function') def mock_accept_invite(mocker, sample_invite): def _accept(service_id, invite_id):