From fd6329b92efee8f351dd445d3984f7c3e6f5e522 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 17 May 2021 11:37:47 +0100 Subject: [PATCH] Fix app config leaking between tests We need to re-initialise the webauthn_server module with original app config, since this state is global across all tests. Since the behaviour of the original fixture wasn't specific to verifying the origin, I've renamed the fixture as part of making it global. In order to keep the fixture simple, I've rewritten the test for the webauthn_server module, so they don't touch the app fixture. --- .../main/views/test_webauthn_credentials.py | 15 ++------ tests/app/models/test_webauthn_credential.py | 23 ++---------- tests/app/test_webauthn_server.py | 35 ++++++++++--------- tests/conftest.py | 16 ++++++++- 4 files changed, 40 insertions(+), 49 deletions(-) diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index bf96e6d45..769725634 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -2,7 +2,6 @@ import pytest from fido2 import cbor from flask import url_for -from app import webauthn_server from app.models.webauthn_credential import RegistrationError @@ -21,18 +20,10 @@ def test_begin_register_returns_encoded_options( mocker, platform_admin_user, platform_admin_client, + webauthn_dev_server, ): - # override base URL so it's consistent on CI and locally - mocker.patch.dict( - app_.config, - values={'ADMIN_BASE_URL': 'http://localhost:6012'} - ) - webauthn_server.init_app(app_) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) - - response = platform_admin_client.get( - url_for('main.webauthn_begin_register') - ) + response = platform_admin_client.get(url_for('main.webauthn_begin_register')) assert response.status_code == 200 @@ -50,7 +41,7 @@ def test_begin_register_returns_encoded_options( relying_party_options = webauthn_options['rp'] assert relying_party_options['name'] == 'GOV.UK Notify' - assert relying_party_options['id'] == 'localhost' + assert relying_party_options['id'] == 'webauthn.io' def test_begin_register_includes_existing_credentials( diff --git a/tests/app/models/test_webauthn_credential.py b/tests/app/models/test_webauthn_credential.py index 44294b8d4..07a7a2006 100644 --- a/tests/app/models/test_webauthn_credential.py +++ b/tests/app/models/test_webauthn_credential.py @@ -4,7 +4,6 @@ import pytest from fido2 import cbor from fido2.cose import ES256 -from app import webauthn_server from app.models.webauthn_credential import RegistrationError, WebAuthnCredential # noqa adapted from https://github.com/duo-labs/py_webauthn/blob/90e3d97e0182899a35a70fc510280b4082cce19b/tests/test_webauthn.py#L14-L24 @@ -18,20 +17,7 @@ ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgw UNSUPPORTED_ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgwRgIhAI1qbvWibQos/t3zsTU05IXw1Ek3SDApATok09uc4UBwAiEAv0fB/lgb5Ot3zJ691Vje6iQLAtLhJDiA8zDxaGjcE3hjeDVjgVkCUzCCAk8wggE3oAMCAQICBDxoKU0wDQYJKoZIhvcNAQELBQAwLjEsMCoGA1UEAxMjWXViaWNvIFUyRiBSb290IENBIFNlcmlhbCA0NTcyMDA2MzEwIBcNMTQwODAxMDAwMDAwWhgPMjA1MDA5MDQwMDAwMDBaMDExLzAtBgNVBAMMJll1YmljbyBVMkYgRUUgU2VyaWFsIDIzOTI1NzM0ODExMTE3OTAxMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvd9nk9t3lMNQMXHtLE1FStlzZnUaSLql2fm1ajoggXlrTt8rzXuSehSTEPvEaEdv/FeSqX22L6Aoa8ajIAIOY6M7MDkwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjUwEwYLKwYBBAGC5RwCAQEEBAMCBSAwDQYJKoZIhvcNAQELBQADggEBAKrADVEJfuwVpIazebzEg0D4Z9OXLs5qZ/ukcONgxkRZ8K04QtP/CB5x6olTlxsj+SXArQDCRzEYUgbws6kZKfuRt2a1P+EzUiqDWLjRILSr+3/o7yR7ZP/GpiFKwdm+czb94POoGD+TS1IYdfXj94mAr5cKWx4EKjh210uovu/pLdLjc8xkQciUrXzZpPR9rT2k/q9HkZhHU+NaCJzky+PTyDbq0KKnzqVhWtfkSBCGw3ezZkTS+5lrvOKbIa24lfeTgu7FST5OwTPCFn8HcfWZMXMSD/KNU+iBqJdAwTLPPDRoLLvPTl29weCAIh+HUpmBQd0UltcPOrA/LFvAf61oYXV0aERhdGFYwnSm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wQQAAAAAAAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyUhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6') # noqa -@pytest.fixture -def disable_webauthn_origin_verification(app_, mocker): - mocker.patch.dict( - app_.config, values={ - 'NOTIFY_ENVIRONMENT': 'development', - 'ADMIN_BASE_URL': 'https://webauthn.io', - } - ) - - # disable origin verification for non-HTTPS test - webauthn_server.init_app(app_) - - -def test_from_registration_verifies_response(disable_webauthn_origin_verification): +def test_from_registration_verifies_response(webauthn_dev_server): registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': ATTESTATION_OBJECT, @@ -47,7 +33,7 @@ def test_from_registration_verifies_response(disable_webauthn_origin_verificatio assert credential_data.public_key[3] == ES256.ALGORITHM -def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verification): +def test_from_registration_encodes_as_unicode(webauthn_dev_server): registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': ATTESTATION_OBJECT, @@ -62,9 +48,6 @@ def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verificati def test_from_registration_handles_library_errors(app_): - # enable origin verification for non-HTTPS test - webauthn_server.init_app(app_) - registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': ATTESTATION_OBJECT, @@ -76,7 +59,7 @@ def test_from_registration_handles_library_errors(app_): assert 'Invalid origin' in str(exc_info.value) -def test_from_registration_handles_unsupported_keys(disable_webauthn_origin_verification): +def test_from_registration_handles_unsupported_keys(webauthn_dev_server): registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': UNSUPPORTED_ATTESTATION_OBJECT, diff --git a/tests/app/test_webauthn_server.py b/tests/app/test_webauthn_server.py index 23093378b..c2bc70381 100644 --- a/tests/app/test_webauthn_server.py +++ b/tests/app/test_webauthn_server.py @@ -3,33 +3,36 @@ import pytest from app import webauthn_server +@pytest.fixture +def app_with_mock_config(mocker): + app = mocker.Mock() + + app.config = { + 'ADMIN_BASE_URL': 'https://www.notify.works', + 'NOTIFY_ENVIRONMENT': 'development' + } + + return app + + @pytest.mark.parametrize(('environment, allowed'), [ ('development', True), ('production', False) ]) def test_server_origin_verification( - app_, - mocker, + app_with_mock_config, environment, allowed ): - mocker.patch.dict( - app_.config, - values={'NOTIFY_ENVIRONMENT': environment} - ) - webauthn_server.init_app(app_) - assert app_.webauthn_server._verify('fake-domain') == allowed + app_with_mock_config.config['NOTIFY_ENVIRONMENT'] = environment + webauthn_server.init_app(app_with_mock_config) + assert app_with_mock_config.webauthn_server._verify('fake-domain') == allowed def test_server_relying_party_id( - app_, + app_with_mock_config, mocker, ): - mocker.patch.dict( - app_.config, - values={'ADMIN_BASE_URL': 'https://www.notify.works'} - ) - - webauthn_server.init_app(app_) - assert app_.webauthn_server.rp.id == 'www.notify.works' + webauthn_server.init_app(app_with_mock_config) + assert app_with_mock_config.webauthn_server.rp.id == 'www.notify.works' diff --git a/tests/conftest.py b/tests/conftest.py index 36950f562..c9fe05cac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,7 +13,7 @@ from flask import Flask, url_for from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_token -from app import create_app +from app import create_app, webauthn_server from . import ( TestClient, @@ -3245,6 +3245,20 @@ def set_config_values(app, dict): app.config[key] = old_values[key] +@pytest.fixture +def webauthn_dev_server(app_, mocker): + overrides = { + 'NOTIFY_ENVIRONMENT': 'development', + 'ADMIN_BASE_URL': 'https://webauthn.io', + } + + with set_config_values(app_, overrides): + webauthn_server.init_app(app_) + yield + + webauthn_server.init_app(app_) + + @pytest.fixture(scope='function') def valid_token(app_, fake_uuid): return generate_token(