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..1fff1a06f 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -9,21 +9,34 @@ 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, normalize_spaces, ) +@pytest.fixture() +def mock_no_users_for_service(mocker): + mocker.patch('app.models.user.Users.client_method', return_value=[]) + + +@pytest.fixture(scope='function') +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, api_user_active, mock_check_invite_token, - mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_get_existing_user_by_email, + mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, mock_get_service, @@ -38,11 +51,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, [], ) @@ -52,7 +65,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'], ) @@ -64,8 +77,8 @@ def test_broadcast_service_shows_tour( client, service_one, mock_check_invite_token, - mock_get_unknown_user_by_email, - mock_get_users_by_service, + mock_get_existing_user_by_email, + mock_no_users_for_service, mock_accept_invite, mock_add_user_to_service, mocker, @@ -101,8 +114,8 @@ 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_users_by_service, + mock_get_existing_user_by_email, + mock_no_users_for_service, mock_add_user_to_service, mock_get_service, mock_events, @@ -116,7 +129,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) @@ -127,10 +140,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 @@ -152,12 +165,11 @@ 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, ): 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', @@ -187,7 +199,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, @@ -207,7 +219,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( @@ -228,10 +239,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) @@ -252,17 +263,13 @@ 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_check_invite_token, 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'] - - # 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') mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( @@ -287,8 +294,8 @@ 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_users_by_service, + mock_get_existing_user_by_email, + mock_no_users_for_service, mock_add_user_to_service, mock_accept_invite, mock_get_service, @@ -302,9 +309,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 @@ -326,7 +333,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, ): @@ -344,11 +351,12 @@ 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, mock_add_user_to_service, - mock_get_users_by_service, + mock_no_users_for_service, mock_get_service, mocker, ): @@ -356,7 +364,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') @@ -445,7 +453,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, @@ -456,7 +464,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'], @@ -473,7 +481,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'], @@ -488,12 +496,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( @@ -518,11 +526,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( @@ -558,7 +566,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, @@ -665,9 +673,10 @@ def test_existing_user_accepts_and_sets_email_auth( api_user_active, service_one, sample_invite, - mock_get_unknown_user_by_email, - mock_get_users_by_service, + 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 @@ -676,7 +685,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', @@ -685,9 +693,41 @@ 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( + client_request, + platform_admin_user, + service_one, + sample_invite, + mock_check_invite_token, + mock_no_users_for_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') + platform_admin_user['auth_type'] = 'webauthn_auth' + + mocker.patch('app.user_api_client.get_user_by_email', return_value=platform_admin_user) + + 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( @@ -696,7 +736,8 @@ 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_check_invite_token, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -707,7 +748,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', @@ -724,7 +764,8 @@ 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_check_invite_token, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, @@ -739,7 +780,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', @@ -756,8 +796,9 @@ 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_get_unknown_user_by_email, + 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, @@ -767,8 +808,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', @@ -776,5 +815,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') 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, diff --git a/tests/conftest.py b/tests/conftest.py index 62fd5166a..ad3e3d02f 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): @@ -2493,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):