darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-06-06 14:30:57 -04:00
Code Issues Packages Projects Releases Wiki Activity
Files
3c778de760e4858dd7abf15b00a86cdff599d190
notifications-admin/tests/app/notify_client/test_user_client.py

347 lines
12 KiB
Python
Raw Normal View History

Delete cached template-folders when adding user to service The api endpoint to get all template folders also returns the users who can see each folder. We need to clear the template-folder cache when adding a user to a service so that we are not using out of date data about who can see each folder.
2019-03-19 11:49:10 +00:00
import uuid
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
from unittest.mock import Mock, call
Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
import pytest
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
from notifications_python_client.errors import HTTPError
Enforce order and style of imports Done using isort[1], with the following command: ``` isort -rc ./app ./tests ``` Adds linting to the `run_tests.sh` script to stop badly-sorted imports getting re-introduced. Chosen style is ‘Vertical Hanging Indent’ with trailing commas, because I think it gives the cleanest diffs, eg: ``` from third_party import ( lib1, lib2, lib3, lib4, ) ``` 1. https://pypi.python.org/pypi/isort
2018-02-20 11:22:17 +00:00
Delete caches when user accepts invite Accepting an invite changes: - the `user_to_service` list of users returned by `GET /service/<id>` - the `services` list return by `GET /user/<id>` The latter change is causing the functional tests to fail.
2018-04-19 13:15:52 +01:00
from app import invite_api_client, service_api_client, user_api_client
Support registering a new authenticator This adds Yubico's FIDO2 library and two APIs for working with the "navigator.credentials.create()" function in JavaScript. The GET API uses the library to generate options for the "create()" function, and the POST API decodes and verifies the resulting credential. While the options and response are dict-like, CBOR is necessary to encode some of the byte-level values, which can't be represented in JSON. Much of the code here is based on the Yubico library example [1][2]. Implementation notes: - There are definitely better ways to alert the user about failure, but window.alert() will do for the time being. Using location.reload() is also a bit jarring if the page scrolls, but not a major issue. - Ideally we would use window.fetch() to do AJAX calls, but we don't have a polyfill for this, and we use $.ajax() elsewhere [3]. We need to do a few weird tricks [6] to stop jQuery trashing the data. - The FIDO2 server doesn't serve web requests; it's just a "server" in the sense of WebAuthn terminology. It lives in its own module, since it needs to be initialised with the app / config. - $.ajax returns a promise-like object. Although we've used ".fail()" elsewhere [3], I couldn't find a stub object that supports it, so I've gone for ".catch()", and used a Promise stub object in tests. - WebAuthn only works over HTTPS, but there's an exception for "localhost" [4]. However, the library is a bit too strict [5], so we have to disable origin verification to avoid needing HTTPS for dev work. [1]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/server.py [2]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/static/register.html [3]: https://github.com/alphagov/notifications-admin/blob/91453d36395b7a0cf2998dfb8a5f52cc9e96640f/app/assets/javascripts/updateContent.js#L33 [4]: https://stackoverflow.com/questions/55971593/navigator-credentials-is-null-on-local-server [5]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/fido2/rpid.py#L69 [6]: https://stackoverflow.com/questions/12394622/does-jquery-ajax-or-load-allow-for-responsetype-arraybuffer
2021-05-07 18:10:07 +01:00
from app.models.webauthn_credential import WebAuthnCredential
Stop calling `fake_uuid` fixture directly Pytest is deprecating the direct calling of fixtures. One fixture that we call directly quite a lot is `fake_uuid`. Since it just returns the value of `sample_uuid()` we can either call that instead (where we need a fixed value) or generate a new UUID each time (where a fixed value is not needed).
2018-09-26 16:41:04 +01:00
from tests import sample_uuid
Reduce the errors related to user fixtures 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.
2019-12-19 16:59:07 +00:00
from tests.conftest import SERVICE_ONE_ID
Add test for existing get users client method Want to make sure it’s doing what I expect before I build another method that calls through to it.
2018-02-21 09:26:42 +00:00
Stop calling `fake_uuid` fixture directly Pytest is deprecating the direct calling of fixtures. One fixture that we call directly quite a lot is `fake_uuid`. Since it just returns the value of `sample_uuid()` we can either call that instead (where we need a fixed value) or generate a new UUID each time (where a fixed value is not needed).
2018-09-26 16:41:04 +01:00
user_id = sample_uuid()
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
Add test for existing get users client method Want to make sure it’s doing what I expect before I build another method that calls through to it.
2018-02-21 09:26:42 +00:00
def test_client_gets_all_users_for_service(
mocker,
fake_uuid,
):
user_api_client.max_failed_login_count = 99 # doesn't matter for this test
mock_get = mocker.patch(
'app.notify_client.user_api_client.UserApiClient.get',
return_value={'data': [
{'id': fake_uuid},
]}
)
users = user_api_client.get_users_for_service(SERVICE_ONE_ID)
mock_get.assert_called_once_with('/service/{}/users'.format(SERVICE_ONE_ID))
assert len(users) == 1
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
assert users[0]['id'] == fake_uuid
Extend user client to count users with permission One of the things we need to know for a service to go live is whether they have at least two users with the ‘manage service’ permission. So this commit adds a method to the client to count how many users have a given permission. We can do logic on this count later. But having the counting done in the client feels like a cleaner separation of concerns. Meant some refactoring of the way `service_id` is extracted from the request, in order to make it easier to mock.
2018-02-21 10:18:56 +00:00
Admin uses correct endpoint for getting user by email address.
2016-02-23 12:47:48 +00:00
def test_client_uses_correct_find_by_email(mocker, api_user_active):
expected_url = '/user/email'
fix syntax
2021-03-05 15:05:48 +00:00
expected_data = {'email': api_user_active['email_address']}
Admin uses correct endpoint for getting user by email address.
2016-02-23 12:47:48 +00:00
Use client from `app` in tests Means we don’t have to instantiate it in every test, keeps things consistent with parent commit.
2018-02-09 15:04:52 +00:00
user_api_client.max_failed_login_count = 1 # doesn't matter for this test
post to get user by email that way we won't store any PII in logs
2021-03-05 12:43:15 +00:00
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
Admin uses correct endpoint for getting user by email address.
2016-02-23 12:47:48 +00:00
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
user_api_client.get_user_by_email(api_user_active['email_address'])
Admin uses correct endpoint for getting user by email address.
2016-02-23 12:47:48 +00:00
fix syntax
2021-03-05 15:05:48 +00:00
mock_post.assert_called_once_with(expected_url, data=expected_data)
Revert original update user method and add new attribute update (with strict checking)
2016-11-09 15:06:02 +00:00
def test_client_only_updates_allowed_attributes(mocker):
mocker.patch('app.notify_client.current_user', id='1')
with pytest.raises(TypeError) as error:
Use client from `app` in tests Means we don’t have to instantiate it in every test, keeps things consistent with parent commit.
2018-02-09 15:04:52 +00:00
user_api_client.update_user_attribute('user_id', id='1')
Revert original update user method and add new attribute update (with strict checking)
2016-11-09 15:06:02 +00:00
assert str(error.value) == 'Not allowed to update user attributes: id'
Change user api client to update password with new endpoint
2017-02-07 13:31:46 +00:00
def test_client_updates_password_separately(mocker, api_user_active):
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
expected_url = '/user/{}/update-password'.format(api_user_active['id'])
Change user api client to update password with new endpoint
2017-02-07 13:31:46 +00:00
expected_params = {'_password': 'newpassword'}
Use client from `app` in tests Means we don’t have to instantiate it in every test, keeps things consistent with parent commit.
2018-02-09 15:04:52 +00:00
user_api_client.max_failed_login_count = 1 # doesn't matter for this test
Change user api client to update password with new endpoint
2017-02-07 13:31:46 +00:00
mock_update_password = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
user_api_client.update_password(api_user_active['id'], expected_params['_password'])
Change user api client to update password with new endpoint
2017-02-07 13:31:46 +00:00
mock_update_password.assert_called_once_with(expected_url, data=expected_params)
use active-endpoint
2017-11-09 14:37:33 +00:00
def test_client_activates_if_pending(mocker, api_user_pending):
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
Use client from `app` in tests Means we don’t have to instantiate it in every test, keeps things consistent with parent commit.
2018-02-09 15:04:52 +00:00
user_api_client.max_failed_login_count = 1 # doesn't matter for this test
use active-endpoint
2017-11-09 14:37:33 +00:00
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
user_api_client.activate_user(api_user_pending['id'])
use active-endpoint
2017-11-09 14:37:33 +00:00
Make user API client return JSON, not a model 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.
2019-05-23 15:27:35 +01:00
mock_post.assert_called_once_with('/user/{}/activate'.format(api_user_pending['id']), data=None)
Tell API what URL to use for email auth links So that we can keep people on the prototype URL when doing user research. Depends on: - [ ] https://github.com/alphagov/notifications-api/pull/1645
2018-02-09 15:01:20 +00:00
def test_client_passes_admin_url_when_sending_email_auth(
Rename "app_" fixture to "notify_admin" 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]: https://github.com/alphagov/notifications-admin/commit/3b1d521c10a515dab85700a1103912cf47ed414c [2]: https://github.com/alphagov/notifications-admin/blob/78824f54fdf7288172387f8ed3b332d26d74e083/tests/app/main/views/test_forgot_password.py#L5
2021-05-12 14:57:21 +01:00
notify_admin,
Tell API what URL to use for email auth links So that we can keep people on the prototype URL when doing user research. Depends on: - [ ] https://github.com/alphagov/notifications-api/pull/1645
2018-02-09 15:01:20 +00:00
mocker,
fake_uuid,
):
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
user_api_client.send_verify_code(fake_uuid, 'email', 'ignored@example.com')
mock_post.assert_called_once_with(
'/user/{}/email-code'.format(fake_uuid),
data={
'to': 'ignored@example.com',
'email_auth_link_host': 'http://localhost:6012',
}
)
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
Rename "app_" fixture to "notify_admin" 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]: https://github.com/alphagov/notifications-admin/commit/3b1d521c10a515dab85700a1103912cf47ed414c [2]: https://github.com/alphagov/notifications-admin/blob/78824f54fdf7288172387f8ed3b332d26d74e083/tests/app/main/views/test_forgot_password.py#L5
2021-05-12 14:57:21 +01:00
def test_client_converts_admin_permissions_to_db_permissions_on_edit(notify_admin, mocker):
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
user_api_client.set_user_permissions('user_id', 'service_id', permissions={'send_messages', 'view_activity'})
Change the format of the user_permissions data that gets sent The endpoint for setting permissions in api will now be used for both user permissions and a user's folder permissions, so this changes the format of the data we pass through.
2019-02-21 15:06:41 +00:00
assert sorted(mock_post.call_args[1]['data']['permissions'], key=lambda x: x['permission']) == sorted([
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
{'permission': 'send_texts'},
{'permission': 'send_emails'},
{'permission': 'send_letters'},
{'permission': 'view_activity'},
], key=lambda x: x['permission'])
Rename "app_" fixture to "notify_admin" 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]: https://github.com/alphagov/notifications-admin/commit/3b1d521c10a515dab85700a1103912cf47ed414c [2]: https://github.com/alphagov/notifications-admin/blob/78824f54fdf7288172387f8ed3b332d26d74e083/tests/app/main/views/test_forgot_password.py#L5
2021-05-12 14:57:21 +01:00
def test_client_converts_admin_permissions_to_db_permissions_on_add_to_service(notify_admin, mocker):
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post', return_value={'data': {}})
Set folder permissions when creating and accepting invites to services 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.
2019-03-15 14:57:39 +00:00
user_api_client.add_user_to_service('service_id',
'user_id',
permissions={'send_messages', 'view_activity'},
folder_permissions=[])
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
Change format of data sent to api when adding user to service The endpoint for adding a user to a service in api will now deal with both user permissions and a user's folder permissions, so this changes the format of the data we pass through.
2019-03-12 16:54:48 +00:00
assert sorted(mock_post.call_args[1]['data']['permissions'], key=lambda x: x['permission']) == sorted([
map roles and db permissions in the db, we have several rows for single permissions - we separate `send_messages` into `send_texts`, `send_emails` and `send_letters`, and also `manage_service` into `manage_users` and `manage_settings`. But on the front end we don't do anything with this distinction. It's unhelpful for us to have to think about permissions as groups of things when we can never split them up at all. So we should combine them. This commit makes sure: * when user models are read (from JSON direct from the API), we should transform them from db permissions into roles. * when permissions are persisted (editing permissions, and creating invites), we should send db permissions to the API. All other interaction with permissions (should just be the endpoint decorator and checks in html templates generally) should use admin roles.
2018-02-28 15:37:49 +00:00
{'permission': 'send_texts'},
{'permission': 'send_emails'},
{'permission': 'send_letters'},
{'permission': 'view_activity'},
], key=lambda x: x['permission'])
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
@pytest.mark.parametrize(
(
'expected_cache_get_calls,'
'cache_value,'
'expected_api_calls,'
'expected_cache_set_calls,'
'expected_return_value,'
),
[
(
[
call('user-{}'.format(user_id))
],
b'{"data": "from cache"}',
[],
[],
'from cache',
),
(
[
call('user-{}'.format(user_id))
],
None,
[
call('/user/{}'.format(user_id))
],
[
call(
'user-{}'.format(user_id),
'{"data": "from api"}',
Make Redis hold onto cached API responses longer Redis is giving us a big performance boost (it’s roughly halved the median request time on the admin app). Once we’re confident that it’s working properly[1] we can eke out a bit more performance from it by keeping the caches alive for longer. As far as I can tell we’re still using Redis in a very low-volume way[2], so increasing the number of things we’re storing shouldn’t start taxing our Redis server at all. But reducing the number of times we have to hit the API to refresh the cache _should_ result in some performance increase. --- 1. ie we’re not seeing instances of stale caches not being invalidated 2. We have 2.5G of available space in Redis. Here is our current usage: ``` used_memory:7728960 used_memory_human:7.37M used_memory_rss:7728960 used_memory_peak:16563776 used_memory_peak_human:15.79M used_memory_lua:37888 ```
2018-04-23 17:07:41 +01:00
ex=604800
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
)
],
'from api',
),
]
)
def test_returns_value_from_cache(
Rename "app_" fixture to "notify_admin" 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]: https://github.com/alphagov/notifications-admin/commit/3b1d521c10a515dab85700a1103912cf47ed414c [2]: https://github.com/alphagov/notifications-admin/blob/78824f54fdf7288172387f8ed3b332d26d74e083/tests/app/main/views/test_forgot_password.py#L5
2021-05-12 14:57:21 +01:00
notify_admin,
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
mocker,
expected_cache_get_calls,
cache_value,
expected_return_value,
expected_api_calls,
expected_cache_set_calls,
):
mock_redis_get = mocker.patch(
move (non-api) clients (inc redis) from app/__init__.py to extensions when clients are defined in app/__init__.py, it increases the chance of cyclical imports. By moving module level client singletons out to a separate extensions file, we stop cyclical imports, but keep the same code flow - the clients are still initialised in `create_app` in `__init__.py`. The redis client in particular is no longer separate - previously redis was set up on the `NotifyAdminAPIClient` base class, but now there's one singleton in `app.extensions`. This was done so that we can access redis from outside of the existing clients.
2019-02-14 14:25:31 +00:00
'app.extensions.RedisClient.get',
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
return_value=cache_value,
)
mock_api_get = mocker.patch(
'app.notify_client.NotifyAdminAPIClient.get',
return_value={'data': 'from api'},
)
mock_redis_set = mocker.patch(
move (non-api) clients (inc redis) from app/__init__.py to extensions when clients are defined in app/__init__.py, it increases the chance of cyclical imports. By moving module level client singletons out to a separate extensions file, we stop cyclical imports, but keep the same code flow - the clients are still initialised in `create_app` in `__init__.py`. The redis client in particular is no longer separate - previously redis was set up on the `NotifyAdminAPIClient` base class, but now there's one singleton in `app.extensions`. This was done so that we can access redis from outside of the existing clients.
2019-02-14 14:25:31 +00:00
'app.extensions.RedisClient.set',
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
)
user_api_client.get_user(user_id)
assert mock_redis_get.call_args_list == expected_cache_get_calls
assert mock_api_get.call_args_list == expected_api_calls
assert mock_redis_set.call_args_list == expected_cache_set_calls
@pytest.mark.parametrize('client, method, extra_args, extra_kwargs', [
Set folder permissions when creating and accepting invites to services 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.
2019-03-15 14:57:39 +00:00
(user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, sample_uuid(), [], []], {}),
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
(user_api_client, 'update_user_attribute', [user_id], {}),
(user_api_client, 'reset_failed_login_count', [user_id], {}),
(user_api_client, 'update_user_attribute', [user_id], {}),
(user_api_client, 'update_password', [user_id, 'hunter2'], {}),
(user_api_client, 'verify_password', [user_id, 'hunter2'], {}),
(user_api_client, 'check_verify_code', [user_id, '', ''], {}),
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
(user_api_client, 'complete_webauthn_login_attempt', [user_id], {'is_successful': True}),
Set folder permissions when creating and accepting invites to services 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.
2019-03-15 14:57:39 +00:00
(user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, user_id, [], []], {}),
Stop calling `fake_uuid` fixture directly Pytest is deprecating the direct calling of fixtures. One fixture that we call directly quite a lot is `fake_uuid`. Since it just returns the value of `sample_uuid()` we can either call that instead (where we need a fixed value) or generate a new UUID each time (where a fixed value is not needed).
2018-09-26 16:41:04 +01:00
(user_api_client, 'add_user_to_organisation', [sample_uuid(), user_id], {}),
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
(user_api_client, 'set_user_permissions', [user_id, SERVICE_ONE_ID, []], {}),
Reduce the errors related to user fixtures 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.
2019-12-19 16:59:07 +00:00
(user_api_client, 'activate_user', [user_id], {}),
Add page to archive user Users can only be archived by Platform Admin from the user page (/users/<user_id>). This removes them from all services and orgs and updates their details.
2019-05-22 11:38:47 +01:00
(user_api_client, 'archive_user', [user_id], {}),
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
(service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, user_id], {}),
Stop sending service domain to the API The API doesn’t look at it, and the rest of the admin code isn’t calling the method with this argument any more.
2019-04-12 17:00:11 +01:00
(service_api_client, 'create_service', ['', '', 0, False, user_id, sample_uuid()], {}),
Delete caches when user accepts invite Accepting an invite changes: - the `user_to_service` list of users returned by `GET /service/<id>` - the `services` list return by `GET /user/<id>` The latter change is causing the functional tests to fail.
2018-04-19 13:15:52 +01:00
(invite_api_client, 'accept_invite', [SERVICE_ONE_ID, user_id], {}),
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
])
def test_deletes_user_cache(
Rename "app_" fixture to "notify_admin" 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]: https://github.com/alphagov/notifications-admin/commit/3b1d521c10a515dab85700a1103912cf47ed414c [2]: https://github.com/alphagov/notifications-admin/blob/78824f54fdf7288172387f8ed3b332d26d74e083/tests/app/main/views/test_forgot_password.py#L5
2021-05-12 14:57:21 +01:00
notify_admin,
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
mock_get_user,
mocker,
client,
method,
extra_args,
extra_kwargs,
):
mocker.patch('app.notify_client.current_user', id='1')
move (non-api) clients (inc redis) from app/__init__.py to extensions when clients are defined in app/__init__.py, it increases the chance of cyclical imports. By moving module level client singletons out to a separate extensions file, we stop cyclical imports, but keep the same code flow - the clients are still initialised in `create_app` in `__init__.py`. The redis client in particular is no longer separate - previously redis was set up on the `NotifyAdminAPIClient` base class, but now there's one singleton in `app.extensions`. This was done so that we can access redis from outside of the existing clients.
2019-02-14 14:25:31 +00:00
mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete')
Cache `GET /user` response in Redis In the same way, and for the same reasons that we’re caching the service object. Here’s a sample of the data returned by the API – so we should make sure that any changes to this data invalidate the cache. If we ever change a user’s phone number (for example) directly in the database, then we will need to invalidate this cache manually. ```python {      'data':{         'organisations':[            '4c707b81-4c6d-4d33-9376-17f0de6e0405'       ],       'logged_in_at':'2018-04-10T11:41:03.781990Z',       'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',       'email_address':'test@example.gov.uk',       'platform_admin':False,       'password_changed_at':'2018-01-01 10:10:10.100000',       'permissions':{            '42a9d4f2-1444-4e22-9133-52d9e406213f':[               'manage_api_keys',             'send_letters',             'manage_users',             'manage_templates',             'view_activity',             'send_texts',             'send_emails',             'manage_settings'          ],          'a928eef8-0f25-41ca-b480-0447f29b2c20':[               'manage_users',             'manage_templates',             'manage_settings',             'send_texts',             'send_emails',             'send_letters',             'manage_api_keys',             'view_activity'          ],       },       'state':'active',       'mobile_number':'07700900123',       'failed_login_count':0,       'name':'Example',       'services':[            '6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',          '6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',       ],       'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',       'auth_type':'sms_auth'    } } ```
2018-04-10 13:30:52 +01:00
mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request')
getattr(client, method)(*extra_args, **extra_kwargs)
assert call('user-{}'.format(user_id)) in mock_redis_delete.call_args_list
assert len(mock_request.call_args_list) == 1
Delete cached template-folders when adding user to service The api endpoint to get all template folders also returns the users who can see each folder. We need to clear the template-folder cache when adding a user to a service so that we are not using out of date data about who can see each folder.
2019-03-19 11:49:10 +00:00
def test_add_user_to_service_calls_correct_endpoint_and_deletes_keys_from_cache(mocker):
mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete')
service_id = uuid.uuid4()
user_id = uuid.uuid4()
folder_id = uuid.uuid4()
expected_url = '/service/{}/users/{}'.format(service_id, user_id)
data = {'permissions': [], 'folder_permissions': [folder_id]}
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
user_api_client.add_user_to_service(service_id, user_id, [], [folder_id])
mock_post.assert_called_once_with(expected_url, data=data)
assert mock_redis_delete.call_args_list == [
call('user-{user_id}'.format(user_id=user_id)),
Change code order for Redis delete decorator Before, the delete decorator would delete the keys from Redis and then we made the request to api to change the data. However, it is possible that the cache could get re-populated in between these two things happening, and so would cache outdated data. This changes the order to send the api request first. We then always delete the specified keys from Redis. Changing the order of the code in the decorator changes the order in which the cache keys get deleted, so the tests have been updated.
2019-07-26 16:19:03 +01:00
call('service-{service_id}-template-folders'.format(service_id=service_id)),
call('service-{service_id}'.format(service_id=service_id)),
Delete cached template-folders when adding user to service The api endpoint to get all template folders also returns the users who can see each folder. We need to clear the template-folder cache when adding a user to a service so that we are not using out of date data about who can see each folder.
2019-03-19 11:49:10 +00:00
]
Add page for security keys with stubbed data 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.
2021-05-07 15:00:01 +01:00
Integrate with the API for adding and getting webauthn creds 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>
2021-05-13 15:54:05 +01:00
def test_get_webauthn_credentials_for_user(mocker, webauthn_credential, fake_uuid):
Support registering a new authenticator This adds Yubico's FIDO2 library and two APIs for working with the "navigator.credentials.create()" function in JavaScript. The GET API uses the library to generate options for the "create()" function, and the POST API decodes and verifies the resulting credential. While the options and response are dict-like, CBOR is necessary to encode some of the byte-level values, which can't be represented in JSON. Much of the code here is based on the Yubico library example [1][2]. Implementation notes: - There are definitely better ways to alert the user about failure, but window.alert() will do for the time being. Using location.reload() is also a bit jarring if the page scrolls, but not a major issue. - Ideally we would use window.fetch() to do AJAX calls, but we don't have a polyfill for this, and we use $.ajax() elsewhere [3]. We need to do a few weird tricks [6] to stop jQuery trashing the data. - The FIDO2 server doesn't serve web requests; it's just a "server" in the sense of WebAuthn terminology. It lives in its own module, since it needs to be initialised with the app / config. - $.ajax returns a promise-like object. Although we've used ".fail()" elsewhere [3], I couldn't find a stub object that supports it, so I've gone for ".catch()", and used a Promise stub object in tests. - WebAuthn only works over HTTPS, but there's an exception for "localhost" [4]. However, the library is a bit too strict [5], so we have to disable origin verification to avoid needing HTTPS for dev work. [1]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/server.py [2]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/static/register.html [3]: https://github.com/alphagov/notifications-admin/blob/91453d36395b7a0cf2998dfb8a5f52cc9e96640f/app/assets/javascripts/updateContent.js#L33 [4]: https://stackoverflow.com/questions/55971593/navigator-credentials-is-null-on-local-server [5]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/fido2/rpid.py#L69 [6]: https://stackoverflow.com/questions/12394622/does-jquery-ajax-or-load-allow-for-responsetype-arraybuffer
2021-05-07 18:10:07 +01:00
Integrate with the API for adding and getting webauthn creds 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>
2021-05-13 15:54:05 +01:00
mock_get = mocker.patch(
'app.notify_client.user_api_client.UserApiClient.get',
return_value={'data': [webauthn_credential]}
)
credentials = user_api_client.get_webauthn_credentials_for_user(fake_uuid)
mock_get.assert_called_once_with(f'/user/{fake_uuid}/webauthn')
assert len(credentials) == 1
assert credentials[0]['name'] == 'Test credential'
Support registering a new authenticator This adds Yubico's FIDO2 library and two APIs for working with the "navigator.credentials.create()" function in JavaScript. The GET API uses the library to generate options for the "create()" function, and the POST API decodes and verifies the resulting credential. While the options and response are dict-like, CBOR is necessary to encode some of the byte-level values, which can't be represented in JSON. Much of the code here is based on the Yubico library example [1][2]. Implementation notes: - There are definitely better ways to alert the user about failure, but window.alert() will do for the time being. Using location.reload() is also a bit jarring if the page scrolls, but not a major issue. - Ideally we would use window.fetch() to do AJAX calls, but we don't have a polyfill for this, and we use $.ajax() elsewhere [3]. We need to do a few weird tricks [6] to stop jQuery trashing the data. - The FIDO2 server doesn't serve web requests; it's just a "server" in the sense of WebAuthn terminology. It lives in its own module, since it needs to be initialised with the app / config. - $.ajax returns a promise-like object. Although we've used ".fail()" elsewhere [3], I couldn't find a stub object that supports it, so I've gone for ".catch()", and used a Promise stub object in tests. - WebAuthn only works over HTTPS, but there's an exception for "localhost" [4]. However, the library is a bit too strict [5], so we have to disable origin verification to avoid needing HTTPS for dev work. [1]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/server.py [2]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/static/register.html [3]: https://github.com/alphagov/notifications-admin/blob/91453d36395b7a0cf2998dfb8a5f52cc9e96640f/app/assets/javascripts/updateContent.js#L33 [4]: https://stackoverflow.com/questions/55971593/navigator-credentials-is-null-on-local-server [5]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/fido2/rpid.py#L69 [6]: https://stackoverflow.com/questions/12394622/does-jquery-ajax-or-load-allow-for-responsetype-arraybuffer
2021-05-07 18:10:07 +01:00
Integrate with the API for adding and getting webauthn creds 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>
2021-05-13 15:54:05 +01:00
def test_create_webauthn_credential_for_user(mocker, webauthn_credential, fake_uuid):
Support registering a new authenticator This adds Yubico's FIDO2 library and two APIs for working with the "navigator.credentials.create()" function in JavaScript. The GET API uses the library to generate options for the "create()" function, and the POST API decodes and verifies the resulting credential. While the options and response are dict-like, CBOR is necessary to encode some of the byte-level values, which can't be represented in JSON. Much of the code here is based on the Yubico library example [1][2]. Implementation notes: - There are definitely better ways to alert the user about failure, but window.alert() will do for the time being. Using location.reload() is also a bit jarring if the page scrolls, but not a major issue. - Ideally we would use window.fetch() to do AJAX calls, but we don't have a polyfill for this, and we use $.ajax() elsewhere [3]. We need to do a few weird tricks [6] to stop jQuery trashing the data. - The FIDO2 server doesn't serve web requests; it's just a "server" in the sense of WebAuthn terminology. It lives in its own module, since it needs to be initialised with the app / config. - $.ajax returns a promise-like object. Although we've used ".fail()" elsewhere [3], I couldn't find a stub object that supports it, so I've gone for ".catch()", and used a Promise stub object in tests. - WebAuthn only works over HTTPS, but there's an exception for "localhost" [4]. However, the library is a bit too strict [5], so we have to disable origin verification to avoid needing HTTPS for dev work. [1]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/server.py [2]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/examples/server/static/register.html [3]: https://github.com/alphagov/notifications-admin/blob/91453d36395b7a0cf2998dfb8a5f52cc9e96640f/app/assets/javascripts/updateContent.js#L33 [4]: https://stackoverflow.com/questions/55971593/navigator-credentials-is-null-on-local-server [5]: https://github.com/Yubico/python-fido2/blob/c42d9628a4f33d20c4401096fa8d3fc466d5b77f/fido2/rpid.py#L69 [6]: https://stackoverflow.com/questions/12394622/does-jquery-ajax-or-load-allow-for-responsetype-arraybuffer
2021-05-07 18:10:07 +01:00
credential = WebAuthnCredential(webauthn_credential)
Integrate with the API for adding and getting webauthn creds 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>
2021-05-13 15:54:05 +01:00
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
expected_url = f'/user/{fake_uuid}/webauthn'
user_api_client.create_webauthn_credential_for_user(fake_uuid, credential)
mock_post.assert_called_once_with(expected_url, data=credential.serialize())
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
def test_complete_webauthn_login_attempt_returns_true_and_no_message_normally(fake_uuid, mocker):
mock_post = mocker.patch('app.notify_client.user_api_client.UserApiClient.post')
resp = user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True)
expected_data = {'successful': True}
rename verify to complete in api endpoint it was changed in this PR: https://github.com/alphagov/notifications-api/pull/3260
2021-06-03 12:46:31 +01:00
mock_post.assert_called_once_with(f'/user/{fake_uuid}/complete/webauthn-login', data=expected_data)
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
assert resp == (True, '')
def test_complete_webauthn_login_attempt_returns_false_and_message_on_403(fake_uuid, mocker):
mock_post = mocker.patch(
'app.notify_client.user_api_client.UserApiClient.post',
side_effect=HTTPError(
response=Mock(
status_code=403,
json=Mock(
return_value={'message': 'forbidden'}
)
)
)
)
resp = user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True)
expected_data = {'successful': True}
rename verify to complete in api endpoint it was changed in this PR: https://github.com/alphagov/notifications-api/pull/3260
2021-06-03 12:46:31 +01:00
mock_post.assert_called_once_with(f'/user/{fake_uuid}/complete/webauthn-login', data=expected_data)
redirect on login; flash errors on failure the js `fetch` function will follow redirects blindly and return you the final 200 response. when there's an error, we don't want to go anywhere, and we want to use the flask `flash` functionality to pop up an error page (the likely reason for seeing this is using a yubikey that isn't associated with your user). using `flash` and then `window.location.reload()` handles this fine. However, when the user does log in succesfully we need to properly log them in - this includes: * checking their account isn't over the max login count * resetting failed login count to 0 if not * setting a new session id in the database (so other browser windows are logged out) * checking if they need to revalidate their email access (every 90 days) * clearing old user out of the cache This code all happens in the ajax function rather than being in a separate redirect, so that you can't just navigate to the login flow. I wasn't able to unit test that function due how it uses the session and other flask globals, so moved the auth into its own function so it's easy to stub out all that CBOR nonsense. TODO: We still need to pass any `next` URLs through the chain from login page all the way through the javascript AJAX calls and redirects to the log_in_user function
2021-05-17 15:56:15 +01:00
assert resp == (False, 'forbidden')
def test_complete_webauthn_login_attempt_raises_on_api_error(fake_uuid, mocker):
mocker.patch(
'app.notify_client.user_api_client.UserApiClient.post',
side_effect=HTTPError(response=Mock(status_code=503, message='error'))
)
with pytest.raises(HTTPError):
user_api_client.complete_webauthn_login_attempt(fake_uuid, is_successful=True)
Pass admin URL to API when resetting password This follows the pattern for invite emails where the admin app tells the API which domain to use when generating the link.
2022-02-02 16:24:03 +00:00
def test_reset_password(
mocker,
fake_uuid,
):
mock_post = mocker.patch(
'app.notify_client.user_api_client.UserApiClient.post'
)
user_api_client.send_reset_password_url('test@example.com')
mock_post.assert_called_once_with(
'/user/reset-password',
data={
'email': 'test@example.com',
'admin_base_url': 'http://localhost:6012',
},
)
Pass admin URL to API for registration email This follows the pattern for invite emails where the admin app tells the API which domain to use when generating the link.
2022-03-07 15:11:50 +00:00
def test_send_registration_email(
mocker,
fake_uuid,
):
mock_post = mocker.patch(
'app.notify_client.user_api_client.UserApiClient.post'
)
user_api_client.send_verify_email(fake_uuid, 'test@example.com')
mock_post.assert_called_once_with(
f'/user/{fake_uuid}/email-verification',
data={
'to': 'test@example.com',
'admin_base_url': 'http://localhost:6012',
},
)
Reference in New Issue Copy Permalink