Some tests use the `client` fixture but don’t call any of its methods.
The reason for doing this is because the test depends on something in
the request context.
This commit replaces all those instances with `client_request`, which
also sets the request context.
These tests are the last ones that still use the `client` fixture. By
replacing it with `client_request` we will be able to say that no tests
should be using the `client` fixture directly.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
Lots of our tests still use an older fixture called `client`. This is
not as good because it:
- returns a raw `Response` object
- doesn’t do the additional checks
- means our tests contain a lot of repetetive boilerplate like `page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')`
This commit converts all the tests which had a `client.get(…)` or
`client.post(…)` statement to use their equivalents on `client_request`
instead.
Subsequent commits will remove uses of `client` in other tests, but
doing it this way means the work can be broken up into more manageable
chunks.
We added a new argument to `client_request.get` and
`client_request.post` to specify that it should return a raw `Response`
object rather than an instance of `BeautifulSoup`.
This is useful because sometimes we need to look at stuff like the
response headers.
However it turns out we already have a separate method for this, so
rather than invent something new I think it’s better to stick with the
thing we already have.
We have a `client_request` fixture which does a bunch of useful stuff
like:
- checking the status code of the response
- returning a `BeautifulSoup` object
For most tests of a platform admin view we used `platform_admin_client`
instead. This is not as good because it returns a raw `Response` object
and doesn’t do the additional checks.
This commit converts all the tests using `platform_admin_client` to:
use new `client_request` and log in as `platform_admin_user` before
making any requests.
This is also nice because it makes any test easy to parametrize with
additional users, for example to test differences in behaviour dependant
on being platform admin or not.
since we are hard-coding a generic error message on the front-end, we
have no need to do anything on the back end. This is also nice as it
standardises the two flows to behave more like each other (rather than
previously where one would `flash` an error message and the other would
return CBOR for the js to decode).
Note that the register flow returns 400 while the auth flow returns 403.
The js for both just checks `response.ok` so will handle both. The JS
completely discards any body returned if the status isn't 200 now.
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.
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.
Previously this was duplicated between the "two_factor" and the
"webauthn" views, and required more test setup. This DRYs up the
check and tests it once, using mocks to simplify the view tests.
As part of DRYing up the check into a util module, I've also moved
the "is_less_than_days_ago" function it uses.
This saves a bit of repetition, and lets us attach other methods to the
collection, rather than having multiple methods on the user object
prefixed with the same name, or random functions floating about.
the next url comes from sign in via a query param, and needs to go to
the POST /webauthn/authenticate endpoint. That endpoint logs the user
in and returns the redirect to the browser, and will take the next from
the request query params to get there.
also moving the window mocks to beforeEach/afterEach ensures that
promise callbacks from previous tests aren't still associated in future
tests to ensure good test isolation.
unfortunately i couldn't get mocking location for a single js test to
work, but by changing the global config i was able to add some query
params that i can expect to be passed through. Don't love this at all
but not quite sure of a good way round this. I think we're not
practicing very good hygiene and best practices with our mocking and
it's really confounding me here.
_complete_webauthn_authentication -> _verify_webauthn_authentication
This function just does verification of the actual auth process -
checking the challenge is correct, the signature matches the public key
we have stored in our database, etc.
verify_webauthn_login -> _complete_webauthn_login_attempt
This function doesn't do any actual verification, we've already verified
the user is who they say they are (or not), it's about marking the
attempt, either unsuccessful (we bump the failed_login_count in the db)
or successful (we set the logged_in_at and current_session_id in the
db).
This change also informs changes to the names of methods on the user
model and in user_api_client.
flashes are consumed by the jinja template calling get_flashed_messages
in flash_messages.html.
When you call `abort(403)` the 403 error page is rendered, with the
flashed message on it. However, the webauthn endpoints just return that
page to the ajax `fetch`, which ignores the response and just reloads
the page.
Instead of calling abort, we can just return an empty response body and
the 403 error code, so that the flashed messages stay in the session and
will be rendered when the `GET /two-factor-webauthn` request happens
after the js reloads the page.
the js fetch function is really not designed to work with 302s. when it
receives a 302, it automatically follows it and fetches the next page.
This is awkward because I don't want js to do all this in ajax, I want
the browser to get the new URL so it can load the page.
A better approach is to view the admin endpoint as a more pure API: the
js sends a request for authentication to the admin app, and the admin
app responds with a 200 indicating success, and then a payload of
relevant data with that.
The relevant data in this case is "Which URL should I redirect to", it
might be the user's list of services page, or it might be a page telling
them that their email needs revalidating.
this doesn't include timeouts or other errors on the browser side - the
main thing this catches is if the token doesn't belong to the user.
However I'm not entirely clear if that's something that will be caught
at this point, or if the browser would reject that key as it's not in
the credentials passed in to the begin_authentication process.
the js `fetch` function will follow redirects blindly and return you the
final 200 response. when there's an error, we don't want to go anywhere,
and we want to use the flask `flash` functionality to pop up an error
page (the likely reason for seeing this is using a yubikey that isn't
associated with your user). using `flash` and then
`window.location.reload()` handles this fine.
However, when the user does log in succesfully we need to properly log
them in - this includes:
* checking their account isn't over the max login count
* resetting failed login count to 0 if not
* setting a new session id in the database (so other browser windows are
logged out)
* checking if they need to revalidate their email access (every 90 days)
* clearing old user out of the cache
This code all happens in the ajax function rather than being in a
separate redirect, so that you can't just navigate to the login flow. I
wasn't able to unit test that function due how it uses the session and
other flask globals, so moved the auth into its own function so it's
easy to stub out all that CBOR nonsense.
TODO: We still need to pass any `next` URLs through the chain from login
page all the way through the javascript AJAX calls and redirects to the
log_in_user function
this is a bit complex, but essentially we're using the test variables
defined in the duolabs py_webauthn library [1]. We're already using
their test variables in tests/app/models/test_webauthn_credential.py and
in the webauthn_credential fixture in conftest.py. By using sample
signature, authenticatordata and clientdatajson from the same key we can
test that the library correctly verifies the signed challenge matches
the original.
We needed to transform some of this data as the yubico/fido2 library we
use has a slightly different way of formatting the fields for the
request body, which is why we're doing things like base64 decoding and
converting from hex to bytes in the post data.
The pytest fixture has changed - before it was incomplete/corrupted and
would error when trying to verify the signature. We took the
credential_data from the pytest fixture, converted it to an
AttestedCredentialData using WebauthnCredential.to_credential_data,
modified the public_key private dictionary to add `public_key[-1]: 1`,
and then called `AttestedCredentialData.create` to re-CBOR-encode the
blob.
The `-1: 1` is the numeric ID of the "SECP256R1" elliptic curve
algorithm. The py_webauthn library forces this particular algorithm,
which differs from the sample creds we took from the fido2 lib tests,
which is why we've had to update our data.
[1] https://github.com/duo-labs/py_webauthn/blob/master/tests/test_webauthn.py#L13-L32
The flow of the code is roughly as follows:
user clicks button on webauthn page
js sends GET request
python reads GET request, sets up login challenge
python returns login challenge in response
js reads GET response, passes login challenge to browser
browser asks user to touch yubikey
browser returns yubikey challenge response data to js
js sends POST request with yubikey challenge response data
python reads yubikey challenge and compares with users creds from db
if its a match, python signs user in
The login challenge is a PublicKeyCredentialRequestOptions: [1]
The browser function we call is navigator.credentials.get(): [2]
The response to the challenge from the browser is a PublicKeyCredential: [3]
The python server does all the work setting those up and tearing them
back down again (and checking them against the values we have stored in
the database), but we need to do work to convert them to-and-from CBOR.
[1] https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialRequestOptions
[2] https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/get
[3] https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredential
This naming was introduced in 2016 without explanation [1]. I find it
confusing because:
- It's reminiscent of "_app", which is a Python convention indicating
the variable is internal, so maybe avoid using it.
- It suggests there's some other "app" fixture I should be using (there
isn't, though).
The Python style guide describes using an underscore suffix to avoid
clashes with inbuilt names [1], which is sort of applicable if we need
to import the "app" module [2]. However, we can also avoid clashes by
choosing a different name, without the strange underscore.
[1]: 3b1d521c10
[2]: 78824f54fd/tests/app/main/views/test_forgot_password.py (L5)
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.
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)
This links up the `get_webauthn_credentials_for_user` and
`create_webauthn_credential_for_user` methods of the user api client to
notifications-api.
To send data to the API we need strings to be unicode, so we call
decode('utf-8') on base64 objects.
Co-authored-by: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
This passes existing credentials in the server response, to allow
the browser to prevent re-registering the same key for the same
user. Registering the same key multiple times doesn't seem to be
an issue technically; the user has likely got their keys mixed up.
- Chrome says "you don't need to register it again".
- Safari exits with an InvalidStateError.
- Firefox exits with a DOMException.
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
