Flake8 Bugbear checks for some extra things that aren’t code style
errors, but are likely to introduce bugs or unexpected behaviour. A
good example is having mutable default function arguments, which get
shared between every call to the function and therefore mutating a value
in one place can unexpectedly cause it to change in another.
This commit enables all the extra warnings provided by Flake8 Bugbear,
except for:
- the line length one (because we already lint for that separately)
- B903 Data class should either be immutable or use `__slots__` because
this seems to false-positive on some of our custom exceptions
- B902 Invalid first argument 'cls' used for instance method because
some SQLAlchemy decorators (eg `declared_attr`) make things that
aren’t formally class methods take a class not an instance as their
first argument
It disables:
- _B306: BaseException.message is removed in Python 3_ because I think
our exceptions have a custom structure that means the `.message`
attribute is still present
Matches the work done in other repos:
- https://github.com/alphagov/notifications-admin/pull/3172/files
Same as we’ve done for templates.
For high volume services this should mean avoiding calls to external
services, either the database or Redis.
TTL is set to 2 seconds, so that’s the maximum time it will take for
revoking an API key or renaming a service to propagate.
Some of the tests created services with the same service ID. This
caused intermittent failures because the cache relies on unique service
IDs (like we have in the real world) to key itself.
By serialising these straight away we can:
- not go back to the database later, potentially closing the connection
sooner
- potentially cache the serialised data, meaning we don’t touch the
database at all
This will allow us to accept two different ones and therefore allow us
to rotate the secret that the admin client is sending to the API
Due to how the notifications-python-client throws exceptions, we run
into exactly the same issue with not being able to distinguish if a
`TokenDecodeError` is thrown because the token was encrypted with a
different secret key or if because there was a different error when
decoding. I've copied the TODO from `requires_auth` as this is exactly
the same issue.
I've also added a test case for functionality that was missing for an
out of date admin token (old IAT).
And support this change across our code. Note, this is a halfway step
where it is not a list rather than a string but still only supports a
single secret, ie one item in the list.
The main drive behind this is to allow us to enable http healthchecks on
the `/_status` endpoint. The healthcheck requests are happening directly
on the instances without going to the proxy to get the header properly
set.
In any case, endpoints like `/_status` should be generally accessible by
anything without requiring any form of authorization.
while it doesn't strictly make sense for the error situations, these
are not typical end user errors - they're about malformed requests.
The typical use case is "api key was revoked" or similar - so that
should be the default error message
example log line:
```
API AuthError: AuthError({'token': ['Invalid token: signature, api token is not valid']}, 403, service_id=3e1ed7ea-8a05-4b4e-93ec-d7bebfea6cae, api_key_id=None)"
```
There are three authentication methods:
- requires_no_auth - public endpoint that does not require an Authorisation header
- requires_auth - public endpoints that need an API key in the Authorisation header
- requires_admin_auth - private endpoint that requires an Authorisation header which contains the API key for the defined as the client admin user
Started with adding a before_request event to the service_blueprint, which executes the requires_admin_auth method rather than the require_auth method.
Obviously this is not done but want to get this in front of people to get an opinion.
We’ve seen quite a few developers encounter the `Invalid token: expired`
error message when they’re getting started using the Notify API. When
this happens they either raise a support ticket or ask for help on
Slack.
In every case this has been because the clock on their
machine/environment/container isn’t accurate. The error message doesn’t
help them figure this out.
This commit adds extra detail to the error message so they can fix the
problem without having to come to us for help.
cleaned up some auth code to marginally improve efficiency of error checking
and hopefully make it easier to read
fixed some incorrect auth headers in the deactivate tests
If you sign a token with a service ID that doesn’t exist (say, for
example, that you get service ID and API key mixed up) then you get
an error saying that “no API keys exist for the service”. This is wrong
because the service doesn’t even exist.
This commit adds:
- code to check if the service does exist
- a specific error message for this case
The check does mean an extra database call to look up the service.
However this only happens _after_ looping through all the API keys. So
it shouldn’t have a performance implication for anyone using a valid API
key.
If you create a token signed with a service ID that doesn’t exist, you
will get an error (as you should).
However we didn’t have a test that explicitly checks for this. This
commit adds one.
