See c31264d4c for why ‘whitelist’ should be avoided. The use of
whitelist here was not referring to the user-maintained list, but to
mean ‘not a government’ email address. This commit renames these tests
to make that difference clear.
If someone enters an email address from a domain we don’t recognise we
direct them straight to our support channel. This is causing increased
contact from suppliers and members of the public.
Now that we have a page which explains who can use Notify, let’s direct
people there first. Then if they really do need to contact support
(because we don’t recognise their organisation) then they can do so from
that page.
The property doesn’t represent the whole client, but just one method on
it. So this commit renames the property to better describe what it is
designed to store.
The session key we use is global.
This means if you open the edit page for two different users in two
different tabs the session for the first tab is overwritten with the
session from the second tab. This means the two users are both set to
the same email address, which causes an exception (email addresses are
unique).
This commit fixes that bug by including the user ID in the session ID.
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.
GOV.UK Design System recommends:
> You should also set the autocomplete attribute to email. This lets
> browsers autofill the email address on a user’s behalf if they’ve
> entered it previously.
Only doing this on the register and sign in forms because it’s unlikely
to be helpful where a user is trying to enter someone else’s email
address.
the api always returns exactly:
```
id
name
email_address
auth_type
current_session_id
failed_login_count
logged_in_at
mobile_number
organisations
password_changed_at
permissions
platform_admin
services
state
```
it does this through `models.py::User.serialize` – there is an old
Marshmallow `user_schema` in `schemas.py` but this isn’t used for
dumping return data, only parsing the json in the create user rest
endpoint.
This means we can rely on these keys always being in the dictionary.
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
This removes the edit_folder_permission checks from the code, enabling
the folder permissions for all services.
This also fixes folder-related tests to set up appropriate user
permissions.
This should only be merged right after alphagov/notifications-api#2428,
when all other permission stories are done.
Platform admin users can access all template folders, so the folder
permissions form always displays everything as checked for them,
which makes it look like the form isn't actually working. We could
do the check based on folder data, but the field still wouldn't
have any effect on permissions. So instead, we hide it completely
for platform admin users.
Submitting the form will remove any folder permissions from the DB
for the platform admin user (which can still be created by changing
permissions on the template folder 'Manage' page), but that's only
relevant if a user stops being a platform admin but keeps their
Notify services.
When a user's email address is updated, we not allowing it to be changed
to a non-government email address. We now allow a non-gov email address
to be changed to another non-gov email address. Government email
addresses still cannot be changed to non-government email addresses.
Also fixes the link in the error message on the ChangeEmailAddress form -
this was being escaped before.
We should audit when a service manager changes a user profile that is not
their own. This can be recorded in our events table, which is currently
only used to record successful logins.
This adds two new types of event, `update_user_email` and
`update_user_mobile_number` which store the
- browser fingerprint
- IP address
- user id of the user being updated
- user id of the service manager making the change
- original email address and new email address (for `update_user_email`
events)
- original mobile number and new mobile number (for
`update_user_mobile_number` events)
Shows a count of how many folders that user can see - this doesn't do
anything smart with parent folder stuff, it's just "how many checkboxes
are ticked on the edit page".
* doesn't show if service has no folders
* doesn't show if service hasn't got folder permissions enabled
It:
- saves repetetive boilerplate code
- does some extra checks (eg checking for a `200` response)
- makes the codebase less confusing to consistently do the same thing in
the same way
remove `confirm` from `confirm_remove_user_from_service` as there's
only one action now that the initial confirmation prompt takes place
on the edit permissions page
when you hit the delete button, it flashes the delete button and takes
you to the `/service/../user/../delete` url. If you then click the save
button, it would make a POST to the delete URL... and delete the user.
now the page stays on the edit url, but adds a `?delete=yes` query
string. The dangerous flash banner now has an action field which
defines where the browser will make the POST to (which remains at
/delete).
If a new user is being invited for a service which doesn't have edit
folder permissions turned on, we want to send all folders for that
service to api.
Added a folder permissions form to the page to invite users to services.
This only shows if the service has 'edit_folder_permissions' enabled,
and all folder checkboxes are checked by default. This change means that
InviteApiClient.create_invite now sends folder_permissions through to
notifications_api (so invites get created with folder permissions).
Started passing the folder_permissions through to notifications-api when
accepting an invite. This changes UserApiClient.add_user_to_service to
send folder_permissions to notifications_api so that new users get folder
permissions when they are added to the service.
Integrates the folder permissions form with the updated API endpoint
to store changes in the user folders.
Since user folder permissions are returned in the full list of template
folders for the service we need to invalidate the cache key for it each
time we update user permissions.
We're reusing the logic for the `move_to` nested radios field for the
user folder permissions nested checkboxes.
The main difference between the two forms (aside from the different
input type) is that "Move" form contains the root "Templates" as an
option, whereas the folder permissions doesn't.
It turns out that, because of the way NestedFieldMixin.children and
select_nested macro are implemented the easiest way to get the desired
folder permissions behaviour is to add the root folder as a choice with
a `None` value and `NONE_OPTION_VALUE = None` set on the field, which
allows the `child_map` to be constructed but doesn't display the root
folder checkbox itself since it gets overwritten in the final `child_map`.
Currently when you load the ‘edit user’ page (which has a URL like
`/service/<service_id>/users/<user_id>`) we check that:
- you belong to the service represented by `service_id`
- you have permission to edit users on this service
We don’t check that:
- the user represented by `user_id` belongs to this service
This means that if you could somehow determine another user’s `user_id`
(which I don’t think is possible if you don’t already have the manage
service permission for that service) then you could:
- edit their permissions on your service (weird, but wouldn’t have any
effect)
- change their email address (bad)
This commit adds checks to return a `404` any time you’re looking at a
service and trying to do stuff to a user who doesn’t belong to that
service.
We can’t add this check to the API easily because there are still times
that we want to get/modify users outside the context of a service (eg
platform admin pages, or users who have no services).
When updating a user’s email address you currently get an validation
error if you save without changing it. Instead it should just obey your
command. And no need for the confirmation step because nothing is
actually changing.
