Commit Graph

10 Commits

Author SHA1 Message Date
Leo Hemsted
e765f98a02 use mockImplementationOnce to define separate return vals for fetch
rather than having a gross if/else, we can define separately. This means
we can separate the asserts and test setups for the first fetch (get)
and the second fetch (post), which means we can arrange all the mocks in
the order they're called in the function, significantly enhancing
legibility of the tests
2021-06-02 11:54:18 +01:00
Ben Thorner
8502827afb Handle errors when registration fails
Previously we would raise a 500 error in a variety of cases:

- If a second key was being registered simultaneously (e.g. in a
separate tab), which means the registration state could be missing
after the first registration completes. That smells like an attack.

- If the server-side verification failed e.g. origin verification,
challenge verification, etc. The library seems to use 'ValueError'
for all such errors [1] (after auditing its 'raise' statements, and
excluding AttestationError [2], since we're not doing that).

- If a key is used that attempts to sign with an unsupported
algorithm. This would normally raise a NotImplemented error as part
of verifying attestation [3], but we don't do that, so we need to
verify the algorithm is supported by the library manually.

This adds error handling to return a 400 response and error message
in these cases, since the error is not unexpected (i.e. not a 500).
A 400 seems more appropriate than a 403, since in many cases it's
not clear if the request data is valid.

I've used CBOR for the transport encoding, to match the successful
request / response encoding. Note that the ordering of then/catch
matters in JS - we don't want to catch our own throws!

[1]: 142587b3e6/fido2/server.py (L255)
[2]: c42d9628a4/fido2/attestation/base.py (L39)
[3]: c42d9628a4/fido2/cose.py (L92)
2021-05-17 12:18:24 +01:00
Ben Thorner
bcf2f7dccd Fix errorType parameter being null
The previous syntax expected the argument would be passed as an
object like { errorType: <type> }.
2021-05-14 17:31:43 +01:00
Ben Thorner
f4ab8776ef Fix confusing error messages when debugging
Previously a bug in the first test would lead to a 'not implemented'
console error, which isn't the actual problem. This ensures alert()
is just a simple no-op, so we can concentrate on actual errors.
2021-05-13 10:22:29 +01:00
Ben Thorner
35507683ee Switch to jest.spyOn() for window.location
The previous comment was incorrect, so there's no need to use the
defineObject hack, or to populate the object beforehand.
2021-05-13 10:22:27 +01:00
Ben Thorner
aae01bf8e2 Switch to jest.spyOn for navigator.credentials
This follows the same approach as for window.fetch, using the Jest
before/afterAll() blocks to handle the idiosynchrosies of whether
the object/function is defined in the test environment.
2021-05-13 10:22:26 +01:00
Ben Thorner
9ee01a2567 Check for response.ok in fetch calls
It's possible for a call to fetch to trigger then "then" callback
even thought the response is an error [1]. We should test for both
scenarios, since they are handled differently. To avoid duplicating
the tests, I've used Jest's parameterisation feature [2].

[1]: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
[2]: https://jestjs.io/docs/api#testeachtablename-fn-timeout
2021-05-13 10:22:26 +01:00
Ben Thorner
6948f5f003 Switch to window.fetch for AJAX calls
In response to [1]. Using window.fetch means we don't get console
logs on errors, so this simplifies the error handling, although we
need to account for some errors not being a standard error object,
such as the string we get by doing Promise.reject('error').

In making this change, I've also started addressing another comment
in the PR [2], so that we reset mocked objects after the tests.

This also switches the ordering of done(), so that it's the last
statement (in response to [3]).

In the next commit we'll check for 'response.ok', but I wanted to
keep this one simple, as it's quite a large change.

[1]: https://github.com/alphagov/notifications-admin/pull/3878#discussion_r631054187
[2]: https://github.com/alphagov/notifications-admin/pull/3878#discussion_r631060116
[3]: https://github.com/alphagov/notifications-admin/pull/3878#discussion_r631061628
2021-05-13 10:22:25 +01:00
Ben Thorner
e2cf3e2c70 Support registering a new authenticator
This adds Yubico's FIDO2 library and two APIs for working with the
"navigator.credentials.create()" function in JavaScript. The GET
API uses the library to generate options for the "create()" function,
and the POST API decodes and verifies the resulting credential. While
the options and response are dict-like, CBOR is necessary to encode
some of the byte-level values, which can't be represented in JSON.

Much of the code here is based on the Yubico library example [1][2].

Implementation notes:

- There are definitely better ways to alert the user about failure, but
window.alert() will do for the time being. Using location.reload() is
also a bit jarring if the page scrolls, but not a major issue.

- Ideally we would use window.fetch() to do AJAX calls, but we don't
have a polyfill for this, and we use $.ajax() elsewhere [3]. We need
to do a few weird tricks [6] to stop jQuery trashing the data.

- The FIDO2 server doesn't serve web requests; it's just a "server" in
the sense of WebAuthn terminology. It lives in its own module, since it
needs to be initialised with the app / config.

- $.ajax returns a promise-like object. Although we've used ".fail()"
elsewhere [3], I couldn't find a stub object that supports it, so I've
gone for ".catch()", and used a Promise stub object in tests.

- WebAuthn only works over HTTPS, but there's an exception for "localhost"
[4].  However, the library is a bit too strict [5], so we have to disable
origin verification to avoid needing HTTPS for dev work.

[1]: c42d9628a4/examples/server/server.py
[2]: c42d9628a4/examples/server/static/register.html
[3]: 91453d3639/app/assets/javascripts/updateContent.js (L33)
[4]: https://stackoverflow.com/questions/55971593/navigator-credentials-is-null-on-local-server
[5]: c42d9628a4/fido2/rpid.py (L69)
[6]: https://stackoverflow.com/questions/12394622/does-jquery-ajax-or-load-allow-for-responsetype-arraybuffer
2021-05-13 10:22:23 +01:00
Ben Thorner
ebb82b2e80 Add page for security keys with stubbed data
This adds a new platform admin settings row, leading a page which
shows any existing keys and allows a new one to be registered. Until
the APIs for this are implemented, the user API client just returns
some stubbed data for manual testing.

This also includes a basic JavaScript module to do the main work of
registering a new authenticator, to be implemented in the next commits.

Some more minor notes:

- Setting the headings in the mapping_table is necessary to get the
horizontal rule along the top (to match the design).

- Setting caption to False in the mapping_table is necessary to stop
an extra margin appearing at the top.
2021-05-12 13:41:53 +01:00