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.
At the moment if you’re invited to a live broadcast service you get the
training mode tour. This is misleading, and could make people think they
weren’t in danger of sending a real alert.
This commit adds a short, 2 step tour for users invited to a live
broadcast service.
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
Only the test channel has the option to isolate messages to one network.
This commits makes the choices less confusing by only showing the
network choice to those who have selected the test channel.
It feels quite dangerous that it’s just one click to make an emergency
alerts service live.
This commit adds a confirmation step which explains the consequences of
what you’re about to do.
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.
For someone who has retrieved a template ID from their system the only
way to find it in Notify is:
- hack the URL
- click through every template, visually inspecting the ID shown on the
page until you find the right one
Neither of these is ideal.
This commit adds searching by ID, for those services who have an API
integration. This means we don’t need to confuse teams who aren’t using
the API by talking about IDs.
This is similar to how we let these teams search for notifications by
reference[1]
1. https://github.com/alphagov/notifications-admin/pull/3223/files
Previously each navigation class had a list of endpoint to "exclude",
which was only used in tests to ensure that all endpoints in the app
were covered: either they are present in navigation, or excluded.
However, over time the "exclude" lists have grown long and repetitive,
and maintaining each of them individually adds extra work [1][2]. This
switches to a more DRY approach, where the list of excluded endpoints
is defined once, close to the single point of use in the test.
Note the resulting test is _slightly_ less prescriptive, as it will now
pass if an endpoint exist one in navigation, even if it should also
exist in another. This seems a reasonable compromise.
[1]: https://github.com/alphagov/notifications-admin/pull/3788/files#r572809972
[2]: https://github.com/alphagov/notifications-admin/pull/3794/files#diff-39387df3a9f89b313976957e7b5457be20deab1017b2d895541b142b957f1972
The dashboard for normal services is quite general, because it tells
you a bit about channels, templates and spend.
What is now the dashboard for broadcast services is much more specific,
therefore less like a dashboard. We can reflect this by giving it a more
specific name. This should reduce the amount of navigation surfing
people need to do in order to find the thing they’re looking for.
Previous alerts are much less important than ones that are live or
waiting for approval.
Therefore we can make the dashboard more focused by moving previous
alerts to their own page.
Services doing broadcasts wont:
- incur costs, so don’t need to see the usage page
- be sending anything by uploading, so don’t need to see the uploads
page
- (for now) be sending anything using the API, so don’t need to see the
API integration page
We’ve done this already for services with the upload letters permission.
And all services can upload letters now.
But we’re still returning it in the JSON response we use to AJAX-ify the
page.
Since the jobs response can query stats for up to 50 jobs at a time this
puts some load on the API/database. Hopefully this might drop that load
a bit.
We were using user fixtures in a lot of parameterized tests, but this is
no longer allowed in Pytest 5. To avoid having to split up the parametrized
tests (which would make the test files a lot longer and slightly more
difficult to read) this commit creates functions which return various types
of user json so that we can use these as the test parameters instead.
Flask will pick the first route that matches. Decorators get applied
from innermost to outermost.
So if the same endpoint is served at `/abc` and `/123` the one used
when `url_for` is generating a URL is whichever decorator is lowest
(in terms of line number).
It doesn’t functionally make a difference, but it’s causing the
functional tests to fail at the moment. And shorter URLs are nicer, so
I think it makes sense to change here, rather than change the tests.
Organisation team members only have access to the dashboard if they’re
also a member of that service.
They always have access to the usage page, so let’s link there instead.
Our research and prototyping around ‘basic view’ found that:
- a lot of users who send messages rarely or never look at the dashboard
(yet it’s the first page they see when they sign in)
- team managers like the idea of taking away things that users don’t
need in order to make the interface simpler
We’ve disentangled the simpler way of sending messages from being part
of ‘basic view’. This means we can give managers the option of taking
away the dashboard as an independent choice, not something that’s
wrapped up in a separate ‘view’.
I think that this checkbox is a more straightforward proposition than
‘basic view’ ever was (despite all the work we did to explain it and
develop the nested checkbox pattern). In research users would often
explain the feature back to us as being about hiding the dashboard – we
should try to make Notify operate in terms of concepts that come
naturally to people wherever possible.
There are some teams who send jobs on a daily/weekly basis. They have
team members who only use Notify for this purpose. So they would
probably benefit from basic view, because they don’t need to see the
dashboard.
This commit:
- adds a new item (uploaded files) to the basic view navigation for
teams that have sent at least one job
- makes the job pages visible to basic view users
I think we should do this now, rather than as a later enhancement to
basic view. We only have one chance to announce the feature, so teams
who do send jobs may otherwise discount it as not useful for them and
the opportunity to have them use it is lost.
From Karl:
> Templates – this should be consistent with Admin view. Users may
> switch from Basic to Admin view (or vice versa), they will also
> interact with users who have a different view or permissions to them.
> Neither should have to learn new interfaces and language if possible.
> ‘Send a message’ was a nice, active label – but Notify options aren’t
> usually actions. If we’re going to change this we should be consistent
> across both Admin and Basic views.
> For the same reason, I have rejected ‘see’, ‘search’ and ‘view sent
> messages’. It will be interesting to see in user testing whether users
> read ‘sent messages’ as ‘send messages’.
The other task that caseworkers have to do (much less often than sending
messages) is look at the messages which they’ve sent. The reason for
doing this is usually to find a specific message which someone has
complained about.
This commit adds:
- a page where they can do that
- a navigation item so they can get to that page
We reckon that because this is about finding specific messages, not
reporting that it’s fine to mush all the channels (email, text, letter)
into one table.
The main task that we think ‘caseworker’ users do is send one off
messages.
So this commit:
- makes sure users who don’t have the `view_activity` permission (ie
not ‘admin’ users) can still send messages
- adds navigation so that these users have a place to go from which to
start the process of sending a one off message
It is standard practice when using GOV.UK template to highlight the
selected navigation item in the propositional navigation (black bar) by
colouring it blue.
This commit adds a new subclass of `Navigation` with the mapping needed
to decide which pages belong to which item in the navigation (or none
at all).
Because we have multiple navigations, which will share the same methods
(by subclassing) but different mappings of navigation items to endpoints
by overriding the `.mapping` and `.exclude` attributes.
In research I’ve sometimes seen people click the wrong nav item. I
reckon that people’s concept of which pages live behind which navigation
items isn’t very strong.
We can reinforce this relationship by showing, for every page, which is
the corresponding nav item. The conventional way of doing this is either
with some kind of emphasis, typically colour or bold. I’ve gone for bold
because colour would be weird.
---
The implementation of this is quite loosely coupled to our application
code because:
- our application code is not well structured (eg we don’t make any use
of blueprints)
- spreading this change across lots of files in our application would
make it harder to test without actually hitting each endpoints; such
tests would be slow and verbose
So I’ve gone for more of a meta approach. Rather than testing that each
endpoint has a specific navigation item selected, I’ve gone for
validating that:
- all endpoints being mapped to are real
- all endpoints have _a_ selected navigation item (or are specifically
excluded)
This means that it’s impossible to add, change or remove an endpoint
without also updating which navigation item should be selected. And the
actual mapping is so declarative that it testing it would be redundant.
