previously we'd skip the template page entirely if someone didnt have
manage templates/api keys permission. however, if the template is
deleted you'd then go through the flow entering placeholders and stuff
before it would then crash when trying to send.
instead, just bounce the user to the template page. It has the content
and says when the template was deleted.
This is useful information to store for the event, which would be
lost if someone subsequently changed them.
Rather than updating lots of mock assertions, I've replaced them
with a single test / assert at a lower level, which is consistent
with auditing being a non-critical function.
I've used the term "admin_roles" in the event data to try and show
that these are not the permissions we store in the DB. This is the
name we use for the abstracted form of permissions in the Admin app.
While we could store the DB permissions, that would be a bit more
effort and arguably it's clearer to keep the event data consistent
with the options the user actually saw / chose.
Usually we have imports at the top. It looks like the reason for
them being inline was to avoid a circular import, but we can also
avoid this by not importing everything from the app module.
Since we're about to add more imports from event_handlers, now is
a good time to refactor them. Note this matches how we import the
event handlers in every other module.
The buttons and links on this page now work with the original
permissions and the two new broadcast permissions. Since the new
broadcast permissions have the effect of splitting the `send_messages`
permission this means that additional sections of if/else logic were
required.
The broadcast dashboards contain a button to create a new broadcast.
This adds the new `create_broadcasts` permission as one of the
permissions needed to see the button.
We've added new broadcast roles in the database (`create_broadcasts` and
`approve_broadcasts`).
Adding these has meant we've needed to do a bit of a rewrite of the roles and
permissions code since this had been based on the assumption that each
database permission only belongs to one admin role - this is no longer true.
This means that flipping the roles dict round to create a dict which
contains database permissions as the keys is no longer possible. We can't
necessarily tell which admin role someone has given a database permission.
To check if a user has an admin role given a list of database permissions,
the user must now have ALL the database permissions mapped to that role
(instead of just one). This works because no one has the `manage_users`
permission without also having the `manage_settings` (and similar for
the other admin roles which map to multiple database permissions).
Some test data was changed because it was using admin roles where
database permissions are actually used when the app is running. I've kept
the functionality of the `translate_permissions_from_db_to_admin_roles`
function passing through any unknown roles it is passed as an argument.
This is not necessary, so can be changed later if we decide it will not
ever be used. However, removing it would require updating a lot of
tests since the tests rely on this behaviour.
Added two new permissions - `create_broadcasts` and
`approve_broadcasts`. These new permissions get added to the
`has_permissions` decorator of the broadcast routes to allow the routes
to be accessed with either the old permissions on the new ones while we
switch over.
We were using the `send_messages` permission for the broadcast routes.
By having two new permissions we can allow a more granular control of
these routes.
We don’t want a single person to have two accounts on an emergency
alerts service because it would let them circumvent the two eyes
approval process.
We can go some way to mitigating against this by stopping people using
common methods that email providers use to alias email addresses. These
are:
- being case insensitive
- being insensitive to the position or number of dots in the local part
of an email address
- using ‘plus addressing’
We already prevent the first one, this commit adds normalisation which
strip out the second two before doing the comparision with the current
user’s email address.
Previously the backend would never validate permissions because the
"not service.active" part would (usually) fail. I've updated it to
match the (inverse of the) conditional we have in the HTML [1].
[1]: 6ac593aa5f/app/templates/views/service-settings.html (L455)
If a job exceeds the daily sending limit, show that on the job page. The job is only created if the sending limit has been reached when the delivery app is processing the job, usually this error is caught at the time the CSV is uploaded and the job is not created.
When the broadcast service settings form is submitted it now removes all
permissions for users in notifications-api. This means it should be
clearing the user cache.
This makes the tests consistent with those for suspend / archive,
logging in with different users to make it clearer who can/not do
this action in the backend.
Note that we think this functionality may be too permissive [1],
so we may restrict it in a future PR.
[1]: https://github.com/alphagov/notifications-admin/pull/3959#issuecomment-878291295
For most events this makes the purpose of each argument clearer at
the point the event is called. It's still worth having a function
for each event type, as this abstracts knowledge of the event label.
Using a schema approach will make adding new events easier.
In the next commit we'll DRY-up the duplication in the tests as well.
Just like the new tests for suspending and archiving services, we
can use the 'client' fixture instead of a context, which avoids
the extra nesting. Note that the first event handler doesn't use
its first argument, which I've tried to indicate with a string.
Previously these only tested with a Platform Admin user, but service
admins can suspend a service too. I've rewritten the tests to match
the 'archive_service' ones, which use the client_request fixture to
make changing the user easier.
Note that the return value of the service API client wasn't used for
anything, so it's safe to remove it from the mock.
This is particularly important for broadcast services, where a rogue
service or platform admin could launch a DoS attack by suspending a
service at a critical moment when it needs to send alerts.
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.
Previously we only had one test for SMS auth not being disabled on
the invite version of the form. This modifies that test to fully
check what's displayed, and adds two more for the edit version of
the form.
Previously we applied this restriction to Platform Admins, on the
assumption that all of them use a security key to log in. Rather
than making that assumption, we can explicitly check their login
method, which also supports rolling out the feature to more users.
We put some content in the go live ticket which is for our benefit, for
example notes about the organisation.
It’s hard for us to be able to say what we want here if we know that the
person making a go live request is going to see those notes.
This commit changes go live requests so that the initial content of the
ticket is hidden from the person raising it (in Zendesk it will appear
as an ‘internal note’, rather than a ‘public reply’).
---
Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/877/files
We give estimates of the area for those who can’t see the map. These
estimates were needlessly precise, gave a false sense of accuracy and
were causing intermittent test failures between different environments.
This commit rounds them in the same way that we round the count of
phones.
