From 2b292ebd16348acee835617fab7a3d5cfce9aae5 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Jun 2021 17:27:26 +0100 Subject: [PATCH 1/3] Refactor "notify_service" function into a fixture This means we can use it in the next commit. Also, it was surprising for the function to be returning a tuple of values, instead of just the service object. Since the consumers of the function only needed the user as auditing data, it's fine to use the first team member. --- tests/app/conftest.py | 113 ++++++++++++------------------- tests/app/service/test_sender.py | 18 ++--- 2 files changed, 48 insertions(+), 83 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 41e5b8e36..a11bb694e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -602,12 +602,10 @@ def mock_firetext_client(mocker): @pytest.fixture(scope='function') -def sms_code_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def sms_code_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='SMS_CODE_TEMPLATE_ID', content='((verify_code))', template_type='sms' @@ -615,11 +613,10 @@ def sms_code_template(notify_db, @pytest.fixture(scope='function') -def email_2fa_code_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def email_2fa_code_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='EMAIL_2FA_TEMPLATE_ID', content=( 'Hi ((name)),' @@ -633,12 +630,10 @@ def email_2fa_code_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def email_verification_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def email_verification_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='NEW_USER_EMAIL_VERIFICATION_TEMPLATE_ID', content='((user_name)) use ((url)) to complete registration', template_type='email' @@ -646,12 +641,11 @@ def email_verification_template(notify_db, @pytest.fixture(scope='function') -def invitation_email_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def invitation_email_template(notify_service): content = '((user_name)) is invited to Notify by ((service_name)) ((url)) to complete registration', return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='INVITATION_EMAIL_TEMPLATE_ID', content=content, subject='Invitation to ((service_name))', @@ -660,12 +654,11 @@ def invitation_email_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def broadcast_invitation_email_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def broadcast_invitation_email_template(notify_service): content = '((user_name)) is invited to broadcast Notify by ((service_name)) ((url)) to complete registration', return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='BROADCAST_INVITATION_EMAIL_TEMPLATE_ID', content=content, subject='Invitation to ((service_name))', @@ -674,12 +667,10 @@ def broadcast_invitation_email_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def org_invite_email_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def org_invite_email_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='ORGANISATION_INVITATION_EMAIL_TEMPLATE_ID', content='((user_name)) ((organisation_name)) ((url))', subject='Invitation to ((organisation_name))', @@ -688,13 +679,10 @@ def org_invite_email_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def password_reset_email_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def password_reset_email_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='PASSWORD_RESET_TEMPLATE_ID', content='((user_name)) you can reset password by clicking ((url))', subject='Reset your password', @@ -703,12 +691,10 @@ def password_reset_email_template(notify_db, @pytest.fixture(scope='function') -def verify_reply_to_address_email_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def verify_reply_to_address_email_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID', content="Hi,This address has been provided as the reply-to email address so we are verifying if it's working", subject='Your GOV.UK Notify reply-to email address', @@ -717,12 +703,10 @@ def verify_reply_to_address_email_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def team_member_email_edit_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def team_member_email_edit_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='TEAM_MEMBER_EDIT_EMAIL_TEMPLATE_ID', content='Hi ((name)) ((servicemanagername)) changed your email to ((email address))', subject='Your GOV.UK Notify email address has changed', @@ -731,12 +715,10 @@ def team_member_email_edit_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def team_member_mobile_edit_template(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def team_member_mobile_edit_template(notify_service): return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID', content='Your mobile number was changed by ((servicemanagername)).', template_type='sms' @@ -744,14 +726,12 @@ def team_member_mobile_edit_template(notify_db, notify_db_session): @pytest.fixture(scope='function') -def already_registered_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def already_registered_template(notify_service): content = """Sign in here: ((signin_url)) If you’ve forgotten your password, you can reset it here: ((forgot_password_url)) feedback:((feedback_url))""" return create_custom_template( - service=service, user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='ALREADY_REGISTERED_EMAIL_TEMPLATE_ID', content=content, template_type='email' @@ -759,17 +739,15 @@ def already_registered_template(notify_db, @pytest.fixture(scope='function') -def change_email_confirmation_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def change_email_confirmation_template(notify_service): content = """Hi ((name)), Click this link to confirm your new email address: ((url)) If you didn’t try to change the email address for your GOV.UK Notify account, let us know here: ((feedback_url))""" template = create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID', content=content, template_type='email' @@ -778,15 +756,14 @@ def change_email_confirmation_template(notify_db, @pytest.fixture(scope='function') -def mou_signed_templates(notify_db, notify_db_session): - service, user = notify_service(notify_db, notify_db_session) +def mou_signed_templates(notify_service): import importlib alembic_script = importlib.import_module('migrations.versions.0298_add_mou_signed_receipt') return { config_name: create_custom_template( - service, - user, + notify_service, + notify_service.users[0], config_name, 'email', content='\n'.join( @@ -826,10 +803,7 @@ def create_custom_template(service, user, template_config_name, template_type, c @pytest.fixture(scope='function') -def letter_volumes_email_template(notify_db, - notify_db_session): - service, user = notify_service(notify_db, notify_db_session) - +def letter_volumes_email_template(notify_service): email_template_content = '\n'.join([ "((total_volume)) letters (((total_sheets)) sheets) sent via Notify are coming in today''s batch. These include: ", # noqa "", @@ -844,8 +818,8 @@ def letter_volumes_email_template(notify_db, ]) return create_custom_template( - service=service, - user=user, + service=notify_service, + user=notify_service.users[0], template_config_name='LETTERS_VOLUME_EMAIL_TEMPLATE_ID', content=email_template_content, subject="Notify letter volume for ((date)): ((total_volume)) letters, ((total_sheets)) sheets", @@ -853,6 +827,7 @@ def letter_volumes_email_template(notify_db, ) +@pytest.fixture def notify_service(notify_db, notify_db_session): user = create_user() service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) @@ -881,7 +856,7 @@ def notify_service(notify_db, notify_db_session): db.session.add(reply_to) db.session.commit() - return service, user + return service @pytest.fixture(scope='function') diff --git a/tests/app/service/test_sender.py b/tests/app/service/test_sender.py index dc33a6c14..2ce1f1c76 100644 --- a/tests/app/service/test_sender.py +++ b/tests/app/service/test_sender.py @@ -4,7 +4,6 @@ from flask import current_app from app.dao.services_dao import dao_add_user_to_service from app.models import EMAIL_TYPE, SMS_TYPE, Notification from app.service.sender import send_notification_to_service_users -from tests.app.conftest import notify_service as create_notify_service from tests.app.db import create_service, create_template, create_user @@ -13,15 +12,13 @@ from tests.app.db import create_service, create_template, create_user SMS_TYPE ]) def test_send_notification_to_service_users_persists_notifications_correctly( - notify_db, - notify_db_session, + notify_service, notification_type, sample_service, mocker ): mocker.patch('app.service.sender.send_notification_to_queue') - notify_service, _ = create_notify_service(notify_db, notify_db_session) user = sample_service.users[0] template = create_template(sample_service, template_type=notification_type) send_notification_to_service_users(service_id=sample_service.id, template_id=template.id) @@ -39,14 +36,12 @@ def test_send_notification_to_service_users_persists_notifications_correctly( def test_send_notification_to_service_users_sends_to_queue( - notify_db, - notify_db_session, + notify_service, sample_service, mocker ): send_mock = mocker.patch('app.service.sender.send_notification_to_queue') - create_notify_service(notify_db, notify_db_session) template = create_template(sample_service, template_type=EMAIL_TYPE) send_notification_to_service_users(service_id=sample_service.id, template_id=template.id) @@ -55,15 +50,13 @@ def test_send_notification_to_service_users_sends_to_queue( def test_send_notification_to_service_users_includes_user_fields_in_personalisation( - notify_db, - notify_db_session, + notify_service, sample_service, mocker ): persist_mock = mocker.patch('app.service.sender.persist_notification') mocker.patch('app.service.sender.send_notification_to_queue') - create_notify_service(notify_db, notify_db_session) user = sample_service.users[0] template = create_template(sample_service, template_type=EMAIL_TYPE) @@ -84,14 +77,11 @@ def test_send_notification_to_service_users_includes_user_fields_in_personalisat def test_send_notification_to_service_users_sends_to_active_users_only( - notify_db, - notify_db_session, + notify_service, mocker ): mocker.patch('app.service.sender.send_notification_to_queue') - create_notify_service(notify_db, notify_db_session) - first_active_user = create_user(email='foo@bar.com', state='active') second_active_user = create_user(email='foo1@bar.com', state='active') pending_user = create_user(email='foo2@bar.com', state='pending') From 2fa6327efb9014a38b57b7193e38c05e209d161e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 25 Jun 2021 17:29:19 +0100 Subject: [PATCH 2/3] Add flag to say if user is eligible for WebAuthn Currently we have some data-driven roles to say who can use this feature. Adding a flag in the API means we can avoid API calls in the Admin app to determine the same. Allowing members of the GOV.UK Notify service to use the feature is a workaround, so we can avoid making someone a Platform Admin before they've protected their account with it. --- app/models.py | 12 ++++++++++++ tests/app/test_model.py | 14 ++++++++++++++ tests/app/user/test_rest.py | 1 + 3 files changed, 27 insertions(+) diff --git a/app/models.py b/app/models.py index 6f33142d5..dfb70b050 100644 --- a/app/models.py +++ b/app/models.py @@ -140,6 +140,17 @@ class User(db.Model): def password(self): raise AttributeError("Password not readable") + @property + def can_use_webauthn(self): + if self.platform_admin: + return True + + return any( + str(service.organisation_id) == current_app.config['BROADCAST_ORGANISATION_ID'] or + str(service.id) == current_app.config['NOTIFY_SERVICE_ID'] + for service in self.services + ) + @password.setter def password(self, password): self._password = hashpw(password) @@ -179,6 +190,7 @@ class User(db.Model): 'permissions': self.get_permissions(), 'platform_admin': self.platform_admin, 'services': [x.id for x in self.services if x.active], + 'can_use_webauthn': self.can_use_webauthn, 'state': self.state, } diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 0942cdbb9..84e59a19a 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -341,3 +341,17 @@ def test_template_folder_is_parent(sample_service): assert folders[0].is_parent_of(folders[4]) assert folders[1].is_parent_of(folders[2]) assert not folders[1].is_parent_of(folders[0]) + + +@pytest.mark.parametrize('is_platform_admin', (False, True)) +def test_user_can_use_webauthn_returns_false(sample_user, is_platform_admin): + sample_user.platform_admin = is_platform_admin + assert sample_user.can_use_webauthn == is_platform_admin + + +def test_user_can_use_webauthn_if_in_broadcast_org(sample_broadcast_service): + assert sample_broadcast_service.users[0].can_use_webauthn + + +def test_user_can_use_webauthn_if_in_notify_team(notify_service): + assert notify_service.users[0].can_use_webauthn diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 0e3ac3402..290a55782 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -72,6 +72,7 @@ def test_get_user(admin_request, sample_service, sample_organisation): assert fetched['permissions'].keys() == {str(sample_service.id)} assert fetched['services'] == [str(sample_service.id)] assert fetched['organisations'] == [str(sample_organisation.id)] + assert fetched['can_use_webauthn'] is False assert sorted(fetched['permissions'][str(sample_service.id)]) == sorted(expected_permissions) From 6cf24899dd1a0920b180c099797e2d089e29254b Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Wed, 30 Jun 2021 15:41:43 +0100 Subject: [PATCH 3/3] Let existing WebAuthn users continue using it It's not a big deal if a user is no longer eligible to register a security key, so we may as well let them continue using it. This avoids putting them in a limbo state if we don't immediately change their auth type when they're no longer eligible to use the feature. --- app/models.py | 3 +++ tests/app/test_model.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index dfb70b050..8023f8dd7 100644 --- a/app/models.py +++ b/app/models.py @@ -145,6 +145,9 @@ class User(db.Model): if self.platform_admin: return True + if self.auth_type == 'webauthn_auth': + return True + return any( str(service.organisation_id) == current_app.config['BROADCAST_ORGANISATION_ID'] or str(service.id) == current_app.config['NOTIFY_SERVICE_ID'] diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 84e59a19a..5924f0a0b 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -344,11 +344,21 @@ def test_template_folder_is_parent(sample_service): @pytest.mark.parametrize('is_platform_admin', (False, True)) -def test_user_can_use_webauthn_returns_false(sample_user, is_platform_admin): +def test_user_can_use_webauthn_if_platform_admin(sample_user, is_platform_admin): sample_user.platform_admin = is_platform_admin assert sample_user.can_use_webauthn == is_platform_admin +@pytest.mark.parametrize(('auth_type', 'can_use_webauthn'), [ + ('email_auth', False), + ('sms_auth', False), + ('webauthn_auth', True) +]) +def test_user_can_use_webauthn_if_they_login_with_it(sample_user, auth_type, can_use_webauthn): + sample_user.auth_type = auth_type + assert sample_user.can_use_webauthn == can_use_webauthn + + def test_user_can_use_webauthn_if_in_broadcast_org(sample_broadcast_service): assert sample_broadcast_service.users[0].can_use_webauthn