This changeset pulls in all of the notification_utils code directly into the admin and removes it as an external dependency. We are doing this to cut down on operational maintenance of the project and will begin removing parts of it no longer needed for the admin.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
This changeset removes webauthn from the Notify.gov admin app. We are not using webauthn at all in our implementation and will be looking at an entirely different authentication system in the near future.
Signed-off-by: Carlo Costino <carlo.costino@gsa.gov>
* Updated header and footer
* Moved files around and updated gulpfile to correct the build process when it goes to production
* Updated fonts
* Adjusted grid templating
* Adding images to assets
* Updated account pages, dashboard, and pages in message sending flow
* Updated the styling for the landing pages in the account section once logged in
It looks like, by default, Flask no longer makes full URLs, for example
`https://example.com/path`. Instead it does `/path`. This will still
work fine, and if anything is better because it reduces the number of
bytes of HTML we are sending.
It won’t mean that requests go over `http` instead of `https` without
the protocol because we set the appropriate HSTS header here:
0c57da7781/ansible/roles/paas-proxy/templates/admin.conf.j2 (L11)
This commit changes all our tests to reflect that URLs no longer have
the protocol and domain in them. `_external=True` is Flask’s way of
saying whether a URL should be generated with the domain and protocol
(`True`) or without it (`False`).
Again, I can’t find the changelog or diff where this was introuduced,
but if you’d like to go spelunking then here’s a starting point:
50374e3cfe/src/flask/helpers.py (L192)
Users who have no mobile number set, users who are not on email auth
and users who are not on "Change mobile number" page should not
see the delete link.
Sometimes users ask us to delete their mobile numbers for them.
If those users are on email auth, they should be able to
delete their number themselves.
This will save them writing a support ticket and save us
going into the database.
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 `logged_in_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 using `logged_in_client` to:
use `client_request` instead.
We use the `ChangeEmailForm` if you want to change your own email
address or someone else's email address. This has various validators
which get run. We check if the email address is valid (by using a
function from utils) and if the email address is already in use
(by calling API).
If the email address is not valid, we should not call API to see if it's
already in use because this will cause an exception in API leading to a
`500` in admin. We now only call API if there were no other errors with
the email address.
(The `test_should_redirect_after_name_change` test didn't need the
`mock_email_is_not_already_in_use` fixture, so this has been removed.)
If there were multiple errors, this widget was joining the messages
together and displaying all error messages. If a text input field does
have more than one validation error, we only want to show one.
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.
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.
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 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.
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)
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 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.
The `post` method of the `client_request` fixture has an argument called
`_data`. There were a few places where we had used an argument of `data`
instead by mistake.
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.
so that platform admins (us) can view pages as regular users do easily.
Simply adds a flag in the session cookie that overrides the actual
platform admin flag on the user model if set. This way it's safe, since
this only downgrades existing functionality, so if someone managed to
alter it they could only get less permissions, not more.
You can change this value from the user profile page if either:
* you're a platform admin
* the flag is set (to any value) on the cookie.
This slightly weird check means that we don't check the underlying
`user._platform_admin` flag anywhere in the code, even when toggling
the suppression.
The data flow of other bits of our application looks like this:
```
API (returns JSON)
⬇
API client (returns a built in type, usually `dict`)
⬇
Model (returns an instance, eg of type `Service`)
⬇
View (returns HTML)
```
The user API client was architected weirdly, in that it returned a model
directly, like this:
```
API (returns JSON)
⬇
API client (returns a model, of type `User`, `InvitedUser`, etc)
⬇
View (returns HTML)
```
This mixing of different layers of the application is bad because it
makes it hard to write model code that doesn’t have circular
dependencies. As our application gets more complicated we will be
relying more on models to manage this complexity, so we should make it
easy, not hard to write them.
It also means that most of our mocking was of the User model, not just
the underlying JSON. So it would have been easy to introduce subtle bugs
to the user model, because it wasn’t being comprehensively tested. A lot
of the changed lines of code in this commit mean changing the tests to
mock only the JSON, which means that the model layer gets implicitly
tested.
For those reasons this commit changes the user API client to return
JSON, not an instance of `User` or other models.
At the moment we have to update a YAML file and deploy the change to get
a new domain whitelisted.
We already have a thing for adding new domains – the organisation stuff.
This commit extends the validation to look in the `domains` table on the
API if it can’t find anything in the YAML whitelist.
This has the advantage of:
- not having to deploy code to whitelist a new domain
- forcing us to create new organisations as they come along, so that
users’ services automatically get allocated to the organisation once
their domain is whitelisted
