From a65ada0d7ef86c4b25b5ede9baa4a9970d2b443b Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 22 May 2020 11:19:49 +0100 Subject: [PATCH] Delete cached users and templates when archiving a service When the admin app gets user objects from the API, these include a dict of permissions by service for what the user can do to that services. Permissions for inactive services are not included in the response as per: https://github.com/alphagov/notifications-api/blob/87cb6f25974d394a8fa1b8f9cd7abe8aa28e05a7/app/dao/permissions_dao.py#L66 However, this causes a bug where a service is archived but cached user data still tells us that the user has permissions to view the service. This should not be the case and causes errors where users can still see the archived service page, it's settings, and even request to go live for it, because they are using old cached data for the user. We solve this by deleting the users who are part of the service from the cache. We also delete the templates for this service from the cache as the templates are also archived when we ask the API to archive the service as per: https://github.com/alphagov/notifications-api/blob/d95c0131e09efb87e535d3b57e49c502824dcabb/app/service/rest.py#L597 Note, one decision I had to make was whether to delete the user cache for just active team members or also invited users. Assuming an invited user can't see the service until they've accepted their invite anyway, it shouldn't make any difference whether we delete their cache or not. --- app/main/views/service_settings.py | 2 +- app/notify_client/service_api_client.py | 7 ++++++- tests/app/main/views/test_service_settings.py | 4 ++++ tests/app/notify_client/test_service_api_client.py | 6 +++++- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index bb9cb1044..d2ca667e5 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -302,7 +302,7 @@ def archive_service(service_id): ): abort(403) if request.method == 'POST': - service_api_client.archive_service(service_id) + service_api_client.archive_service(service_id, current_service.active_users) flash( '‘{}’ was deleted'.format(current_service.name), 'default_with_tick', diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b0ad13904..7f118fec0 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -129,7 +129,12 @@ class ServiceAPIClient(NotifyAdminAPIClient): return self.update_service(service_id, **properties) @cache.delete('service-{service_id}') - def archive_service(self, service_id): + @cache.delete('service-{service_id}-templates') + def archive_service(self, service_id, service_users): + # We need to purge the cache for the services users as otherwise, although they will have had their permissions + # removed in the DB, they would still have permissions in the cache to view/edit/manage this service + for user in service_users: + cache.delete(f'user-{user.id}') return self.post('/service/{}/archive'.format(service_id), data=None) @cache.delete('service-{service_id}') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 00126e9e8..862d5b13a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3807,9 +3807,11 @@ def test_archive_service_after_confirm( mock_get_organisations, mock_get_service_and_organisation_counts, mock_get_organisations_and_services_for_user, + mock_get_users_by_service, user, ): mocked_fn = mocker.patch('app.service_api_client.post') + cache_delete_mock = mocker.patch('app.notify_client.service_api_client.cache.delete') client_request.login(user) page = client_request.post( 'main.archive_service', @@ -3822,6 +3824,8 @@ def test_archive_service_after_confirm( assert normalize_spaces(page.select_one('.banner-default-with-tick').text) == ( '‘service one’ was deleted' ) + # The one user which is part of this service has the sample_uuid as it's user ID + cache_delete_mock.assert_called_once_with(f"user-{sample_uuid()}") @pytest.mark.parametrize('user', ( diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index ef8dfc33d..281af2ca0 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -389,7 +389,7 @@ def test_returns_value_from_cache( @pytest.mark.parametrize('client, method, extra_args, extra_kwargs', [ (service_api_client, 'update_service', [SERVICE_ONE_ID], {'name': 'foo'}), (service_api_client, 'update_service_with_properties', [SERVICE_ONE_ID], {'properties': {}}), - (service_api_client, 'archive_service', [SERVICE_ONE_ID], {}), + (service_api_client, 'archive_service', [SERVICE_ONE_ID, []], {}), (service_api_client, 'suspend_service', [SERVICE_ONE_ID], {}), (service_api_client, 'resume_service', [SERVICE_ONE_ID], {}), (service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, ''], {}), @@ -458,6 +458,10 @@ def test_deletes_service_cache( 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), + ('archive_service', [SERVICE_ONE_ID, []], [ + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'service-{}'.format(SERVICE_ONE_ID), + ]), ]) def test_deletes_caches_when_modifying_templates( app_,