From 7fafc18fb3a0e9282c4a2af6e05a6e14c99f0fdc Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 14:54:20 +0100 Subject: [PATCH 1/5] Remove unnecessary restriction for 2FA WebAuthn It should be enough to check the user has it set as their auth type. Even if a user is no longer eligible to register a security key, it should still be OK for them to continue using the feature. --- app/main/views/two_factor.py | 3 +-- tests/app/main/views/test_two_factor.py | 14 -------------- 2 files changed, 1 insertion(+), 16 deletions(-) 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/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, From 43afcd1064343ac0114da9e9927ab093fb243950 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 14:58:08 +0100 Subject: [PATCH 2/5] Remove redundant restrictions for WebAuthn feature Since the register and authentication APIs work in pairs, we can just put the restrictions on the "begin" API. We weren't testing the restrictions on the "complete" API anyway. For authentication, it's also enough to check if the user has WebAuthn as their auth type, as it's not a big deal if a user continues to login with a security key indefinitely. --- app/main/views/webauthn_credentials.py | 10 ---------- tests/app/main/views/test_webauthn_credentials.py | 11 ----------- 2 files changed, 21 deletions(-) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 2161edf01..ca2aaad7b 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -38,7 +38,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 +80,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 +101,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/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index 645ad766e..03fb7855f 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -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) From a1b4ccc246faf6ddd96da7bfb7c39352b7e6c94c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 15:15:00 +0100 Subject: [PATCH 3/5] 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. --- app/main/forms.py | 12 +++------ app/main/views/manage_users.py | 6 ++--- .../views/manage-users/permissions.html | 4 +-- tests/app/main/views/test_manage_users.py | 26 +++++++------------ 4 files changed, 19 insertions(+), 29 deletions(-) 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=[], From 1a4dd04ab1b1c5d4efa6b7aab563664e6d33a7ee Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 15:29:02 +0100 Subject: [PATCH 4/5] Add missing tests for authentication radios Previously we only had one test for SMS auth not being disabled on the invite version of the form. This modifies that test to fully check what's displayed, and adds two more for the edit version of the form. --- tests/app/main/views/test_manage_users.py | 49 ++++++++++++++++++++++- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 6f690be85..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( @@ -794,6 +797,48 @@ def test_edit_user_permissions_including_authentication_with_email_auth_service( ) +def test_edit_user_permissions_shows_authentication_for_email_auth_service( + client_request, + service_one, + 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, From 4c2915ce86398976032136bb83a4970d61320a8d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 15:30:29 +0100 Subject: [PATCH 5/5] Use API flag to give users access to WebAuthn This allows us to roll out the feature to other users. Note that the flag is also "True" if the user has "webauthn_auth" as their auth type, so this is compatible with the more fine-grained check we have on the authentication parts of the feature. We could do a more explicit "can_use_webauthn or webauthn_auth" check here, but the idea is that we'll be able to get rid of this flag eventually, so I've optimised for brevity instead. I've modified a couple of the unhappy-path tests to make it more explicit that the flag is false, since it can be true for Platform Admins and "normal users" alike. --- app/main/views/user_profile.py | 17 ++++++++------- app/main/views/webauthn_credentials.py | 5 +++-- app/models/user.py | 1 + app/templates/views/user-profile.html | 2 +- tests/app/main/views/test_user_profile.py | 21 +++++++++++++++---- .../main/views/test_webauthn_credentials.py | 12 +++++------ tests/conftest.py | 4 +++- 7 files changed, 40 insertions(+), 22 deletions(-) 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 ca2aaad7b..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( diff --git a/app/models/user.py b/app/models/user.py index 0d95e1624..e7d341eab 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/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_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 03fb7855f..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( 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