Merge pull request #3889 from alphagov/webauthn-errors

Handle errors when registration fails

tests/app/main/views/test_webauthn_credentials.py

@@ -2,7 +2,7 @@ import pytest
 from fido2 import cbor
 from flask import url_for
 
-from app import webauthn_server
+from app.models.webauthn_credential import RegistrationError
 
 
 @pytest.mark.parametrize('endpoint', [
@@ -20,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
 
@@ -49,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(
@@ -132,3 +124,37 @@ def test_complete_register_clears_session(
 
     with platform_admin_client.session_transaction() as session:
         assert 'webauthn_registration_state' not in session
+
+
+def test_complete_register_handles_library_errors(
+    platform_admin_client,
+    mocker,
+):
+    with platform_admin_client.session_transaction() as session:
+        session['webauthn_registration_state'] = 'state'
+
+    mocker.patch(
+        'app.models.webauthn_credential.WebAuthnCredential.from_registration',
+        side_effect=RegistrationError('error')
+    )
+
+    response = platform_admin_client.post(
+        url_for('main.webauthn_complete_register'),
+        data=cbor.encode('public_key_credential'),
+    )
+
+    assert response.status_code == 400
+    assert cbor.decode(response.data) == 'error'
+
+
+def test_complete_register_handles_missing_state(
+    platform_admin_client,
+    mocker,
+):
+    response = platform_admin_client.post(
+        url_for('main.webauthn_complete_register'),
+        data=cbor.encode('public_key_credential'),
+    )
+
+    assert response.status_code == 400
+    assert cbor.decode(response.data) == 'No registration in progress'

tests/app/models/test_webauthn_credential.py

@@ -4,8 +4,7 @@ import pytest
 from fido2 import cbor
 from fido2.cose import ES256
 
-from app import webauthn_server
-from app.models.webauthn_credential import WebAuthnCredential
+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
 SESSION_STATE = {'challenge': 'bPzpX3hHQtsp9evyKYkaZtVc9UN07PUdJ22vZUdDp94', 'user_verification': 'discouraged'}
@@ -14,21 +13,11 @@ CLIENT_DATA_JSON = b'{"type": "webauthn.create", "clientExtensions": {}, "challe
 # had to use the cbor2 library to re-encode the attestationObject due to implementation differences
 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+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+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_)
+# manually adapted by working out which character in the encoded CBOR corresponds to the public key algorithm ID
+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
 
 
-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,
@@ -44,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,
@@ -56,3 +45,27 @@ def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verificati
 
     assert type(serialized_credential['credential_data']) == str
     assert type(serialized_credential['registration_response']) == str
+
+
+def test_from_registration_handles_library_errors(app_):
+    registration_response = {
+       'clientDataJSON': CLIENT_DATA_JSON,
+       'attestationObject': ATTESTATION_OBJECT,
+    }
+
+    with pytest.raises(RegistrationError) as exc_info:
+        WebAuthnCredential.from_registration(SESSION_STATE, registration_response)
+
+    assert 'Invalid origin' in str(exc_info.value)
+
+
+def test_from_registration_handles_unsupported_keys(webauthn_dev_server):
+    registration_response = {
+       'clientDataJSON': CLIENT_DATA_JSON,
+       'attestationObject': UNSUPPORTED_ATTESTATION_OBJECT,
+    }
+
+    with pytest.raises(RegistrationError) as exc_info:
+        WebAuthnCredential.from_registration(SESSION_STATE, registration_response)
+
+    assert 'Encryption algorithm not supported' in str(exc_info.value)

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'