diff --git a/app/main/forms.py b/app/main/forms.py index 1910f7574..af03280d1 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1009,14 +1009,10 @@ class BasePermissionsForm(StripWhitespaceForm): }, login_authentication=user.auth_type ) - # if the user is a platform admin then we don't allow you to change the login type. Unfortunately, since the - # login_authentication field is a radio field, the validation expects it to have a value, and the choices in it - # normally don't allow 'webauthn_auth'. - # If the user has webauthn_auth too, then we'll try and validate against that. We should never be changing - # auth of platform admin users, so just force the choices to be this. - # TODO: if the user is a regular user with webauthn_auth we will still show the radios, so if you edit that - # user's regular permissions you'll necessarily change their auth type as the value will be in the POST - if user.platform_admin: + + # If a user logs in with a security key, we generally don't want a service admin to be able to change this. + # As well as enforcing this in the backend, we need to delete the auth radios to prevent validation errors. + if user.webauthn_auth: del form.login_authentication return form diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 3a75f1359..57e00ac3f 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -142,9 +142,9 @@ def edit_user_permissions(service_id, user_id): permissions=form.permissions, folder_permissions=form.folder_permissions.data, ) - # 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: + # Only change the auth type if this is supported for a service. If a user logs in with a + # security key, we generally don't want them to be able to use something less secure. + if service_has_email_auth and not user.auth_type == 'webauthn_auth': user.update(auth_type=form.login_authentication.data) return redirect(url_for('.manage_users', service_id=service_id)) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 31864075c..231859e47 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -93,8 +93,7 @@ def two_factor_sms(): def two_factor_webauthn(): user_id = session['user_details']['id'] user = User.from_id(user_id) - if not user.platform_admin: - abort(403) + if not user.webauthn_auth: abort(403) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 03ace2424..426852ff4 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -27,11 +27,7 @@ from app.main.forms import ( TwoFactorForm, ) from app.models.user import User -from app.utils.user import ( - user_is_gov_user, - user_is_logged_in, - user_is_platform_admin, -) +from app.utils.user import user_is_gov_user, user_is_logged_in NEW_EMAIL = 'new-email' NEW_MOBILE = 'new-mob' @@ -236,8 +232,10 @@ def user_profile_disable_platform_admin_view(): @main.route("/user-profile/security-keys", methods=['GET']) -@user_is_platform_admin def user_profile_security_keys(): + if not current_user.can_use_webauthn: + abort(403) + return render_template( 'views/user-profile/security-keys.html', ) @@ -253,8 +251,10 @@ def user_profile_security_keys(): methods=['GET'], endpoint="user_profile_confirm_delete_security_key" ) -@user_is_platform_admin def user_profile_manage_security_key(key_id): + if not current_user.can_use_webauthn: + abort(403) + security_key = current_user.webauthn_credentials.by_id(key_id) if not security_key: @@ -282,8 +282,9 @@ def user_profile_manage_security_key(key_id): @main.route("/user-profile/security-keys//delete", methods=['POST']) -@user_is_platform_admin def user_profile_delete_security_key(key_id): + if not current_user.can_use_webauthn: + abort(403) try: user_api_client.delete_webauthn_credential_for_user( diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 2161edf01..4d6d13148 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -14,12 +14,13 @@ from app.utils.login import ( log_in_user, redirect_to_sign_in, ) -from app.utils.user import user_is_platform_admin @main.route('/webauthn/register') -@user_is_platform_admin def webauthn_begin_register(): + if not current_user.can_use_webauthn: + abort(403) + server = current_app.webauthn_server registration_data, state = server.register_begin( @@ -38,7 +39,6 @@ def webauthn_begin_register(): @main.route('/webauthn/register', methods=['POST']) -@user_is_platform_admin def webauthn_complete_register(): if 'webauthn_registration_state' not in session: return cbor.encode("No registration in progress"), 400 @@ -81,9 +81,6 @@ def webauthn_begin_authentication(): if not user_to_login.webauthn_auth: abort(403) - if not user_to_login.platform_admin: - abort(403) - authentication_data, state = current_app.webauthn_server.authenticate_begin( credentials=user_to_login.webauthn_credentials.as_cbor, user_verification="discouraged", # don't ask for PIN @@ -105,12 +102,6 @@ def webauthn_complete_authentication(): user_id = session['user_details']['id'] user_to_login = User.from_id(user_id) - if not user_to_login.webauthn_auth: - abort(403) - - if not user_to_login.platform_admin: - abort(403) - try: _verify_webauthn_authentication(user_to_login) redirect = _complete_webauthn_login_attempt(user_to_login) diff --git a/app/models/user.py b/app/models/user.py index e52661b2b..ff9cc1387 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -31,6 +31,7 @@ class User(JSONModel, UserMixin): MAX_FAILED_LOGIN_COUNT = 10 ALLOWED_PROPERTIES = { + 'can_use_webauthn', 'id', 'name', 'email_address', diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index db64051c9..c5c8dd786 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -11,9 +11,9 @@ {% endif %} {% if service_has_email_auth %} - {% if user.platform_admin %} + {% if user.webauthn_auth %}

- Platform admin users will login with a security key. + This user will login with a security key.

{% elif not mobile_number %} {{ radios( diff --git a/app/templates/views/user-profile.html b/app/templates/views/user-profile.html index 4044a707c..b0680987f 100644 --- a/app/templates/views/user-profile.html +++ b/app/templates/views/user-profile.html @@ -45,7 +45,7 @@ {{ edit_field('Change', url_for('.user_profile_password')) }} {% endcall %} - {% if current_user.platform_admin %} + {% if current_user.can_use_webauthn %} {% call row(id='security-keys') %} {{ text_field('Security keys') }} {{ optional_text_field( diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 764a278de..11a74420c 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -453,8 +453,11 @@ def test_invite_user_allows_to_choose_auth( service_one['permissions'].append('email_auth') page = client_request.get('main.invite_user', service_id=SERVICE_ONE_ID) - sms_auth_radio_button = page.select_one('input[value="sms_auth"]') - assert sms_auth_radio_button.has_attr("disabled") is False + radio_buttons = page.select("input[name=login_authentication]") + values = {button["value"] for button in radio_buttons} + + assert values == {'sms_auth', 'email_auth'} + assert not any(button.has_attr("disabled") for button in radio_buttons) def test_invite_user_has_correct_email_field( @@ -742,7 +745,6 @@ def test_cant_edit_non_member_user_permissions( assert mock_set_user_permissions.called is False -@pytest.mark.parametrize('current_auth_type', ['webauthn_auth', 'email_auth']) def test_edit_user_permissions_including_authentication_with_email_auth_service( client_request, service_one, @@ -752,9 +754,8 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service( mock_set_user_permissions, mock_update_user_attribute, mock_get_template_folders, - current_auth_type, ): - active_user_with_permissions['auth_type'] = current_auth_type + active_user_with_permissions['auth_type'] = 'email_auth' service_one['permissions'].append('email_auth') client_request.post( @@ -796,39 +797,77 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service( ) -@pytest.mark.parametrize('current_auth_type', ['webauthn_auth', 'email_auth']) -def test_edit_user_permissions_preserves_auth_type_for_platform_admin( +def test_edit_user_permissions_shows_authentication_for_email_auth_service( client_request, service_one, - platform_admin_user, + mock_get_users_by_service, + mock_get_template_folders, + active_user_with_permissions, +): + service_one['permissions'].append('email_auth') + + page = client_request.get( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=active_user_with_permissions['id'], + ) + + radio_buttons = page.select("input[name=login_authentication]") + values = {button["value"] for button in radio_buttons} + + assert values == {'sms_auth', 'email_auth'} + assert not any(button.has_attr("disabled") for button in radio_buttons) + + +def test_edit_user_permissions_hides_authentication_for_webauthn_user( + client_request, + service_one, + mock_get_users_by_service, + mock_get_template_folders, + active_user_with_permissions, +): + active_user_with_permissions['auth_type'] = 'webauthn_auth' + service_one['permissions'].append('email_auth') + + page = client_request.get( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=active_user_with_permissions['id'], + ) + + assert 'This user will login with a security key' in str(page) + assert page.select_one('#login_authentication') is None + + +@pytest.mark.parametrize('new_auth_type', ['sms_auth', 'email_auth']) +def test_edit_user_permissions_preserves_auth_type_for_webauthn_user( + client_request, + service_one, + active_user_with_permissions, mock_get_users_by_service, mock_get_invites_for_service, mock_set_user_permissions, mock_update_user_attribute, mock_get_template_folders, - current_auth_type, + new_auth_type, ): - platform_admin_user['auth_type'] = current_auth_type + active_user_with_permissions['auth_type'] = 'webauthn_auth' service_one['permissions'].append('email_auth') - # we're logging in as this user being edited; normally a user can't edit themselves, but since - # the same mock is used to (a) check access and (b) find the user to edit, this is just easier - client_request.login(platform_admin_user) - client_request.post( 'main.edit_user_permissions', service_id=SERVICE_ONE_ID, - user_id=platform_admin_user['id'], + user_id=active_user_with_permissions['id'], _data={ - 'email_address': platform_admin_user['email_address'], + 'email_address': active_user_with_permissions['email_address'], 'permissions_field': [], - 'login_authentication': 'sms_auth', + 'login_authentication': new_auth_type, }, _expected_status=302, ) mock_set_user_permissions.assert_called_with( - str(platform_admin_user['id']), + str(active_user_with_permissions['id']), SERVICE_ONE_ID, permissions=set(), folder_permissions=[], diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 03508c426..e02d7d42a 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -298,20 +298,6 @@ def test_two_factor_webauthn_should_have_auth_signin_button( mock_get_user.assert_called_once_with(platform_admin_user['id']) -def test_two_factor_webauthn_should_reject_non_platform_admins( - client, - api_user_active, - mock_get_user, -): - api_user_active['auth_type'] = 'webauthn_auth' - with client.session_transaction() as session: - session['user_details'] = {'id': api_user_active['id'], 'email': api_user_active['email_address']} - - response = client.get(url_for('main.two_factor_webauthn')) - - assert response.status_code == 403 - - def test_two_factor_webauthn_should_reject_non_webauthn_auth_users( client, platform_admin_user, diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 7e03f6a5a..d18a7f745 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -44,7 +44,7 @@ def test_overview_page_shows_disable_for_platform_admin( (1, 'Security keys 1 registered Change'), (2, 'Security keys 2 registered Change'), ]) -def test_overview_page_shows_security_keys_for_platform_admin( +def test_overview_page_shows_security_keys_if_user_they_can_use_webauthn( mocker, client_request, platform_admin_user, @@ -358,7 +358,13 @@ def test_can_reenable_platform_admin(client_request, platform_admin_user): assert session['disable_platform_admin_view'] is False -def test_normal_user_doesnt_see_security_keys(client_request): +def test_user_doesnt_see_security_keys_unless_they_can_use_webauthn( + client_request, + platform_admin_user +): + platform_admin_user['can_use_webauthn'] = False + client_request.login(platform_admin_user) + client_request.get( '.user_profile_security_keys', _expected_status=403, @@ -455,9 +461,16 @@ def test_manage_security_key_page_404s_when_key_not_found( (".user_profile_confirm_delete_security_key", "post"), (".user_profile_delete_security_key", "post"), ]) -def test_non_platform_admin_user_cant_manage_security_keys( - client_request, webauthn_credential, endpoint, method +def test_cant_manage_security_keys_unless_can_use_webauthn( + client_request, + platform_admin_user, + webauthn_credential, + endpoint, + method ): + platform_admin_user['can_use_webauthn'] = False + client_request.login(platform_admin_user) + if method == "get": client_request.get( endpoint, diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 645ad766e..3106ef79a 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -33,14 +33,14 @@ def webauthn_authentication_post_data(fake_uuid, webauthn_credential, client): }) -@pytest.mark.parametrize('endpoint', [ - 'webauthn_begin_register', -]) -def test_register_forbidden_for_non_platform_admins( +def test_begin_register_forbidden_unless_can_use_webauthn( client_request, - endpoint, + platform_admin_user, + mocker, ): - client_request.get(f'main.{endpoint}', _expected_status=403) + platform_admin_user['can_use_webauthn'] = False + mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) + client_request.get('main.webauthn_begin_register', _expected_status=403) def test_begin_register_returns_encoded_options( @@ -197,17 +197,6 @@ def test_complete_register_handles_missing_state( assert cbor.decode(response.data) == 'No registration in progress' -def test_begin_authentication_forbidden_for_non_platform_admins(client, api_user_active, mock_get_user): - # mock_get_user returns api_user_active so changes to the api user will reflect - api_user_active['auth_type'] = 'webauthn_auth' - - with client.session_transaction() as session: - session['user_details'] = {'id': '1'} - - response = client.get(url_for('main.webauthn_begin_authentication')) - assert response.status_code == 403 - - def test_begin_authentication_forbidden_for_users_without_webauthn(client, mocker, platform_admin_user): platform_admin_user['auth_type'] = 'sms_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) diff --git a/tests/conftest.py b/tests/conftest.py index d7e7959f5..eeeaad4ce 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3636,6 +3636,7 @@ def create_platform_admin_user(with_unique_id=False, auth_type='webauthn_auth', permissions=permissions or {}, platform_admin=True, auth_type=auth_type, + can_use_webauthn=True, ) @@ -3681,7 +3682,8 @@ def create_user(**overrides): 'organisations': [], 'current_session_id': None, 'logged_in_at': None, - 'email_access_validated_at': None + 'email_access_validated_at': None, + 'can_use_webauthn': False, } user_data.update(overrides) return user_data