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/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/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 764a278de..6f690be85 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -742,7 +742,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 +751,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 +794,35 @@ 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( +@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, - platform_admin_user, + 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=[],