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
if user has `webauthn_auth` as their auth type, then redirect them to an
interstitial that prompts them to click on a button which right now just
logs to the JS console, but in a future commit will open up the webauthn
browser prompt
content is unsurprisingly not final.
I don’t think we need to say (all networks) in the header. The real
benefit of this change is forcing the platform admin person making the
change to explicitly give their choice of network.
And by not putting it in the label we don’t have future users from other
organisations wondering if there’s some option other than ‘all networks’
they need to think about.
without the need for verification.
This is for when the email takes too long to arrive and the service
users cannot update it as a result.
A more streamlined solution has been proposed where we could send
a link in the verification email to the users and clicking that
link would add/update reply-email-to address.
That would require a bit more work so right now I am proposing this
as a quick stop gap so that we don't have to go to the database
manually to add the reply-to email address.
We can use the `optional_text_field` macro to grey out the text when
nothing is set up. And adding ‘registered’ makes the language consistent
through to the next page.
When referring to something that’s not part of the Notify system, like a
spreadsheet or a paper letter or a security key we’ve found it’s helpful
to give people a visual representation of it. This commit does the same
for security keys.
Previously we made surprising changed to the invited user as part
of the mock, and then surprising assertions that its ID matched
USER_ONE_ID. This simplifies the mock to do what it says, so that
we can test for the original ID of the existing user.*
*this does still differ from the ID of the sample_invite, which is
also hard-coded to USER_ONE_ID. However, this isn't relevant in
any of the tests, so doesn't seem to much of an issue.
This replaces the original fixture with a more explicit one, noting
that none of the tests rely on this fixture as part of testing the
scenarios when a user is already a member of the service.
This closes a security loophole, where the auth type of a Platform
Admin could be unwittingly changed when they accept an invite, or
by an admin of a service they are a member of.
when getting a list of security keys
Also test separately that we are correctly choosing key out of list
of security keys. Previously we have done it as a part
of testing pages where where we were calling API to get a list
of keys, but then choosing one of those keys based on id.
Also remove redundant second test credential after PR review
Also remove redundant return value from mocks in update name tests
When we are unable to delete security key because it's the last
one for that user, API throws an error. Here we catch that error
and display useful message to the user.
Use security key instead of webauthn credential
in user facing message - for consistency and readability.
We use security key term in user facing stuff and webauthn
credential in the code.
Depends on: https://github.com/alphagov/notifications-aws/pull/905
Previously this would print some custom text with each step, and
as optionally loading a virtual environment. This moves the actual
test commands to the Makefile. While this no longer prints custom
text, it does print the command that was run:
Before (skipping other output):
./scripts/run_tests.sh
Code style check passed
Import order check passed
...
JavaScript tests have passed
...
Unit tests have passed
After (skipping other output):
flake8 .
isort --check-only ./app ./tests
npm test
...
py.test -n auto --maxfail=10 tests/
...
I think it's more useful to see the command being run, rather than
having to wait until it succeeds to know what was happening. Having
the command also makes it easier to run it again if it fails, rather
than having to go and find it in a script.
Previously we could only select a provider when using the test
channel, but this is also required for others channels when we
do tests on the production network with individual MNOs.
In order to reduce duplication and improve consistency, I've reused
the new broadcast_service_name_tag macro to show the setting.
This makes the code shareable between:
- the broadcast tour pages
- the broadcast settings platform admin page
- the regular service navigation
On the training mode tour pages we don’t want to confuse people with the
organisation name or _Switch service_ links, so those are omitted and
the code is therefore slightly different.
