mirror of
https://github.com/GSA/notifications-admin.git
synced 2026-05-21 17:23:06 -04:00
Prevent auth type changing for any WebAuthn user
Previously we applied this restriction to Platform Admins, on the assumption that all of them use a security key to log in. Rather than making that assumption, we can explicitly check their login method, which also supports rolling out the feature to more users.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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))
|
||||
|
||||
|
||||
@@ -11,9 +11,9 @@
|
||||
{% endif %}
|
||||
|
||||
{% if service_has_email_auth %}
|
||||
{% if user.platform_admin %}
|
||||
{% if user.webauthn_auth %}
|
||||
<p class="bottom-gutter">
|
||||
Platform admin users will login with a security key.
|
||||
This user will login with a security key.
|
||||
</p>
|
||||
{% elif not mobile_number %}
|
||||
{{ radios(
|
||||
|
||||
@@ -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=[],
|
||||
|
||||
Reference in New Issue
Block a user