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,