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>
Daily volumes report: total volumes across the platform aggregated by whole business day (bst_date)
Volumes by service report: total volumes per service aggregated by the date range given.
NB: start and end dates are inclusive
This is consistent with all other methods: we clear the cache after
the actual change, not before it.
Since the new version of -utils, we're now catching redis exceptions
on delete, so this change has little effect on behaviour.
This stops adding the current user to the data sent to the API when
removing a user from an organisation. API only needs to know the
organisation_id and the id of the user we want to remove from
the organisation, so we don't need to pass through the id of the
current user too.
The other change made is to clear the user cache of the user who has
been removed from the org. We don't need to clear any of the organisation
caches, since these values don't contain lists of users for the orgs.
It’s weird to have a method just for updating one attribute. I think
the reason for doing this was to only invalidate the
`organisation-{}-name` cache when absolutely necessary, but:
- we don’t need a separate method to check whether it’s the name being
updated
- it was easy to get around this by calling
`OrganisationsClient.update_organisation` directly, leaving a stale
value in the cache
This updates the `get_notifications_for_service` of the
`NotificationApiClient` to make POST request to the api when the `to`
field is provided. This is done so that we avoid logging personal
details such as email addreses.
If the `to` field is not present, the method will make a GET reqeust and
there will be no change to how it works.
In
a9617d4df6
we upgraded the version of utils to 49.1 which brought in a renamed
`TTL` as `DEFAULT_TTL`.
However, not only did it change the name, it also changed its type
from an `int` to a `float`:
https://github.com/alphagov/notifications-utils/pull/923/files
We thought that would be OK as in the utils, we moved the conversion
to an integer to happen in the `set` method but it turns out that
caused an issue in the admin app where setting the `has_jobs...`
redis keys will error:
```
Redis error performing set on has_jobs-4bd11cb2-cc17-44e1-b241-8547990db245
...
...
redis.exceptions.ResponseError: value is not an integer or out of range
```
It looks like this is because we are passing a float instead of an
int to `ex`
See a similar post describing the importance of ints rather than
floats for other parameters:
https://developpaper.com/question/redis-err-value-is-not-an-integer-or-out-of-range/
An interesting note is our test
`test_client_creates_job_data_correctly` didn't catch this because
`float(604800) == int(604800`.
I've gone for the quickest solution which is to wrap `DEFAULT_TTL`
in an int. The reason I've done this now is that to do the long
term and more durable fix is to add this fix to utils, however
there are several breaking changes infront of it that would take
me a while to bring in to the admin app first. I've checked the
admin and API apps and this is the only place we are directly
using `DEFAULT_TTL`.
This stat is shown on a few of our pages, such as our homepage,
the performance page and also a platform admin page
and is currently catched for the
default TTL of 1 week. I think there is no reason we can't make
this only cache once an hour and give slightly more up to date stats
which will update more regularly.
This mimics the approach and also the TTL choice of 1 hour that has
been added for the performance page (although there is no
particular bug here to fix, it is just nice to have slightly more up
up to date data).
Note, the API call only takes about 0.3 seconds at the moment
so it is not particularly intensive on the DB to run this more
regularly.
I came across a bug where the performance page might be visited
on say the 22nd and we expect it to show data for yesterday and
the past 7 days (so the 21st and back 7 days) but instead it
only shows it for the 20th and then back 6 days.
The cause is that we have nightly tasks that run to
calculate the number of notifications sent per service (the
ft_notification_status table)
calculate the number of notifications sent within 10 seconds
(the ft_processing_time table)
Both of these tables get filled between the hours of midnight and
2:30am. The first seems to be filled by about 12:30 every night
whereas the processing stats seems to be filled about 2am every
night.
The admin app currently caches the results of the call to the API
(https://github.com/alphagov/notifications-api/blob/master/app/performance_dashboard/rest.py#L24)
to get this data with the key
performance-stats-{start_date}-to-{end_date}.
Now the issue here is that if the performance page is visited at
00:01 on the 23rd, it will call the API for data on the 22nd and
backwards 7 days. The data it gets at 00:01 will not yet have the
data for the 22nd, it will only go as far as the 21st, because
the overnight jobs to put data in the tables have not run yet. The
admin app then caches this data, meaning that for the rest of the
day the page will be missing data for the 22nd, even though it
becomes available after approx 2am. We have seen it a few times
in production where the performance page has indeed been visited
before say 2 am, leaving the page with missing data.
In terms of solutions, there are several potential options.
1. Remove the caching that we do in redis. The downside of this is
that we will hit the API more (currently once a day for the first
visitor to the performance page). We have had 255 requests
to the performance page in the last week and when there is no value
in redis, the call to the API takes approximately 1-2 seconds.
Removing the caching would against why the caching was originally
added which was to act as a basic protection against malicious
DOS attempts.
2. Make the admin app only set the cache value if it is after
2:30am. This one feels a bit gross and snowflakey.
3. The API flushes the redis cache after it has finished its nightly
tasks. We don't have that much precedent of the API interacting
with redis, it is mostly the admin app that does but this is still
a valid option.
4. Cache in redis the data for 2 days ago to 7 days ago and then
get the data for yesterday freshly every time. This would mean
some things are still cached but the smallest bit that we can't rely
on can be gathered fresh.
5. Remove the caching we do in redis but then try and optimise the
API endpoint to return results even faster and with less
interaction with the DB. My hypothesis is that the slowest part
is where the API has to calculate how many notifications Notify
has ever sent
(https://github.com/alphagov/notifications-api/blob/master/app/performance_dashboard/rest.py#L36)
and so we could potentially try and store these totals in the DB
every night and update them nightly rather than having to query all
the data in the table to figure them out every time.
6. Reduce the expiry time of the cache key so although the bug will
still exist, it will only exist for a much shorter time.
In the end, we've gone for option 6. The user impact of
this bug sticking around for an hour is minimal, in particular
because that would be in the early hours of the morning. The solution
is also quick to implement and keeps the protection against a DOS attack.
The value of 1 hour for expiry was a fairly abitrary choice, it could
have been as little as 1 minute or as much as 6 hours.
Previously we were passing a flag to the API which handled this. Now
we are doing it at the time of clicking the link, not at the time of
storing the new password. We don’t need to update the timestamp twice,
so this commit removes the code which tells the API to do it.
When someone uses a fresh password reset link they have proved that they
have access to their inbox.
At the moment, when revalidating a user’s email address we wait until
after they’ve put in the 2FA code before updating the timestamp which
records when they last validated their email address[1].
We can’t think of a good reason that we need the extra assurance of a
valid 2FA code to assert that the user has access to their email –
they’ve done that just by clicking the link. When the user clicks the
link we already update their failed login count before they 2fa. Think
it makes sense to handle `email_access_validated_at` then too.
As a bonus, the functional tests never go as far as getting a 2FA code
after a password reset[2], so the functional test user never gets its
timestamp updated. This causes the functional tests start failing after
90 days. By moving the update to this point we ensure that the
functional tests will keep passing indefinitely.
1. This code in the API (91542ad33e/app/dao/users_dao.py (L131))
which is called by this code in the admin app (9ba37249a4/app/utils/login.py (L26))
2. 5837eb01dc/tests/functional/preview_and_dev/test_email_auth.py (L43-L46)
