fix bug stopping editing of permissions of webauthn platform admins

We hide the radio field in the HTML for platform admins, as we don't
want anyone to be able to change their auth type. However, when the form
is validated, the form has a field called login_authentication that it
expects a value for. It silently fails as it complains that when the
user POSTed they didn't select a value for that radio field, but the
error message is on the radio fields that don't get displayed to the
user so they'd never know.

Fixing this is actually pretty hard.

We use this form in two places, one where we have a user to edit, one
where we are creating an invite from scratch. So sometimes we don't know
about a user's auth type. In addition, radio buttons are mandatory by
design, but now sometimes we don't just want to make it optional but
explicitly ignore the value being passed in? To solve this, remove the
field entirely from the form if the user is a platform admin. This means
that if the code in manage_users.py tries to access the
login_authentication value from the form, it'll error, but I think
that's okay to leave for now given we concede that this isn't a perfect
final solution.

The tests didn't flag this previously as they tried to set from sms_auth
(the default for `platform_admin_user`) TO email_auth or sms_auth. Also,
the diagnosis of this bug was confounded further by the fact that
`mock_get_users_by_service` sets what is returned by the API - the
service model then takes the IDs out of that response and calls
`User.get_user_by_id` for the matching ID (as in, the code only uses
get_users_by_service to ensure the user belongs to that service). This
means that we accidentally set the form editing the current user, as
when we log in we set `get_user_by_id` to return the user of our choice
This commit is contained in:
Leo Hemsted
2021-06-10 23:09:36 +01:00
parent 92b6885224
commit 126f9cf6be
2 changed files with 25 additions and 10 deletions

View File

@@ -742,7 +742,7 @@ def test_cant_edit_non_member_user_permissions(
assert mock_set_user_permissions.called is False
@pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth'])
@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,
@@ -751,9 +751,10 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service(
mock_get_invites_for_service,
mock_set_user_permissions,
mock_update_user_attribute,
auth_type,
mock_get_template_folders
mock_get_template_folders,
current_auth_type,
):
active_user_with_permissions['auth_type'] = current_auth_type
service_one['permissions'].append('email_auth')
client_request.post(
@@ -768,7 +769,7 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service(
'manage_service',
'manage_api_keys',
],
'login_authentication': auth_type,
'login_authentication': 'sms_auth',
},
_expected_status=302,
_expected_redirect=url_for(
@@ -791,11 +792,11 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service(
)
mock_update_user_attribute.assert_called_with(
str(active_user_with_permissions['id']),
auth_type=auth_type
auth_type='sms_auth'
)
@pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth'])
@pytest.mark.parametrize('current_auth_type', ['webauthn_auth', 'email_auth'])
def test_edit_user_permissions_preserves_auth_type_for_platform_admin(
client_request,
service_one,
@@ -804,10 +805,14 @@ def test_edit_user_permissions_preserves_auth_type_for_platform_admin(
mock_get_invites_for_service,
mock_set_user_permissions,
mock_update_user_attribute,
auth_type,
mock_get_template_folders
mock_get_template_folders,
current_auth_type,
):
platform_admin_user['auth_type'] = current_auth_type
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(
@@ -817,7 +822,7 @@ def test_edit_user_permissions_preserves_auth_type_for_platform_admin(
_data={
'email_address': platform_admin_user['email_address'],
'permissions_field': [],
'login_authentication': auth_type,
'login_authentication': 'sms_auth',
},
_expected_status=302,
)