From 4b2a0037e340f6f50f5109e2df62ceb2ee200a39 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 23 Jun 2020 17:45:05 +0100 Subject: [PATCH 01/13] Get letter data for provided filenames --- app/commands.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/commands.py b/app/commands.py index 766bc9f2d..02825f227 100644 --- a/app/commands.py +++ b/app/commands.py @@ -773,7 +773,22 @@ def get_letter_details_from_zips_sent_file(file_paths): rows_from_file.extend(json.loads(file_contents)) notification_references = tuple(row[18:34] for row in rows_from_file) + get_letters_data_from_references(notification_references) + +@notify_command(name='get-notification-and-service-ids-for-letters-that-failed-to-print') +@click.option('-f', '--file_name', required=True, + help="""Full path of the file to upload, file should contain letter filenames, one per line""") +def get_notification_and_service_ids_for_letters_that_failed_to_print(file_name): + print("Getting service and notification ids for letter filenames list {}".format(file_name)) + file = open(file_name) + references = tuple([row[7:23] for row in file]) + + get_letters_data_from_references(tuple(references)) + file.close() + + +def get_letters_data_from_references(notification_references): sql = """ SELECT id, service_id, reference, job_id, created_at FROM notifications From d506451d4fb4db49c606a1e59a9cbc845f4aa74d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 24 Jun 2020 12:16:59 +0100 Subject: [PATCH 02/13] Include template id, too --- app/commands.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/commands.py b/app/commands.py index 02825f227..8002e1c02 100644 --- a/app/commands.py +++ b/app/commands.py @@ -790,7 +790,7 @@ def get_notification_and_service_ids_for_letters_that_failed_to_print(file_name) def get_letters_data_from_references(notification_references): sql = """ - SELECT id, service_id, reference, job_id, created_at + SELECT id, service_id, template_id, reference, job_id, created_at FROM notifications WHERE reference IN :notification_references ORDER BY service_id, job_id""" @@ -798,7 +798,7 @@ def get_letters_data_from_references(notification_references): with open('zips_sent_details.csv', 'w') as csvfile: csv_writer = csv.writer(csvfile) - csv_writer.writerow(['notification_id', 'service_id', 'reference', 'job_id', 'created_at']) + csv_writer.writerow(['notification_id', 'service_id', 'template_id', 'reference', 'job_id', 'created_at']) for row in result: csv_writer.writerow(row) From b8fe7b8e61dd7f1162052e6dc7fa2f3ef42aafe7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 13:42:22 +0100 Subject: [PATCH 03/13] Revert "Merge pull request #2902 from alphagov/fix-imports" This reverts commit e00c0355b6b60ba78067d2666b0dc815c9a6def4, reversing changes made to 832b5899800072b3c88d3b5d1b6ba5de04ec5a9c. --- app/serialised_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serialised_models.py b/app/serialised_models.py index e47c8800c..7f88171d2 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -9,6 +9,7 @@ from werkzeug.utils import cached_property from app import db, redis_store +from app.dao import templates_dao from app.dao.api_key_dao import get_model_api_keys from app.dao.services_dao import dao_fetch_service_by_id @@ -100,7 +101,6 @@ class SerialisedTemplate(SerialisedModel): @staticmethod @redis_cache.set('template-{template_id}-version-None') def get_dict(template_id, service_id): - from app.dao import templates_dao from app.schemas import template_schema fetched_template = templates_dao.dao_get_template_by_id_and_service_id( From 7e85e37e1d0cbff0a22db0847111fbb13f44d7f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 13:42:44 +0100 Subject: [PATCH 04/13] Revert "Merge pull request #2887 from alphagov/cache-the-serialised-things" This reverts commit b8c2c6b291c6da689efdce2d796f923f4156c230, reversing changes made to 351aca2c5a8188614377022ebdcf2b702bae4965. --- app/authentication/auth.py | 6 +- app/config.py | 7 +- app/notifications/process_notifications.py | 1 + app/notifications/validators.py | 4 +- app/schemas.py | 2 +- app/serialised_models.py | 84 +------------------ app/service/send_notification.py | 4 +- app/service/utils.py | 6 -- app/template/rest.py | 11 +-- app/v2/notifications/post_notifications.py | 8 +- requirements-app.txt | 1 - requirements.txt | 6 +- .../app/authentication/test_authentication.py | 46 ++-------- tests/app/notifications/test_validators.py | 8 +- .../notifications/test_post_notifications.py | 71 +++++----------- 15 files changed, 50 insertions(+), 215 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index e26edee48..1b7a286ac 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -8,7 +8,7 @@ from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from gds_metrics import Histogram -from app.serialised_models import SerialisedService +from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header' # noqa @@ -94,7 +94,7 @@ def requires_auth(): try: with AUTH_DB_CONNECTION_DURATION_SECONDS.time(): - service = SerialisedService.from_id(issuer) + service = dao_fetch_service_by_id_with_api_keys(issuer) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) except NoResultFound: @@ -129,7 +129,7 @@ def requires_auth(): if api_key.expiry_date: raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) - g.service_id = service.id + g.service_id = api_key.service_id _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key diff --git a/app/config.py b/app/config.py index 257eaea61..7a9f881d3 100644 --- a/app/config.py +++ b/app/config.py @@ -400,12 +400,7 @@ class Test(Development): NOTIFY_ENVIRONMENT = 'test' TESTING = True - HIGH_VOLUME_SERVICE = [ - '941b6f9a-50d7-4742-8d50-f365ca74bf27', - '63f95b86-2d19-4497-b8b2-ccf25457df4e', - '7e5950cb-9954-41f5-8376-962b8c8555cf', - '10d1b9c9-0072-4fa9-ae1c-595e333841da', - ] + HIGH_VOLUME_SERVICE = ['941b6f9a-50d7-4742-8d50-f365ca74bf27'] CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' CONTACT_LIST_BUCKET_NAME = 'test-contact-list' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e5ca0adcc..3f3f86fd6 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -120,6 +120,7 @@ def persist_notification( template_version=template_version, to=recipient, service_id=service.id, + service=service, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, diff --git a/app/notifications/validators.py b/app/notifications/validators.py index d1bf7b589..c505d5a19 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -90,7 +90,7 @@ def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_ def service_has_permission(notify_type, permissions): - return notify_type in permissions + return notify_type in [p.permission for p in permissions] def check_service_has_permission(notify_type, permissions): @@ -131,7 +131,7 @@ def check_if_service_can_send_to_number(service, number): if ( # if number is international and not a crown dependency international_phone_info.international and not international_phone_info.crown_dependency - ) and INTERNATIONAL_SMS_TYPE not in service.permissions: + ) and INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: raise BadRequestError(message="Cannot send to international mobile numbers") else: return international_phone_info diff --git a/app/schemas.py b/app/schemas.py index a44514b5d..c799522c8 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -205,7 +205,7 @@ class ProviderDetailsHistorySchema(BaseSchema): strict = True -class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): +class ServiceSchema(BaseSchema): created_by = field_for(models.Service, 'created_by', required=True) organisation_type = field_for(models.Service, 'organisation_type') diff --git a/app/serialised_models.py b/app/serialised_models.py index 7f88171d2..d544dc478 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -5,13 +5,9 @@ from threading import RLock import cachetools from notifications_utils.clients.redis import RequestCache -from werkzeug.utils import cached_property - -from app import db, redis_store +from app import redis_store from app.dao import templates_dao -from app.dao.api_key_dao import get_model_api_keys -from app.dao.services_dao import dao_fetch_service_by_id caches = defaultdict(partial(cachetools.TTLCache, maxsize=1024, ttl=2)) locks = defaultdict(RLock) @@ -57,29 +53,6 @@ class SerialisedModel(ABC): return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) -class SerialisedModelCollection(ABC): - - """ - A SerialisedModelCollection takes a list of dictionaries, typically - created by serialising database objects. When iterated over it - returns a SerialisedModel instance for each of the items in the list. - """ - - @property - @abstractmethod - def model(self): - pass - - def __init__(self, items): - self.items = items - - def __bool__(self): - return bool(self.items) - - def __getitem__(self, index): - return self.model(self.items[index]) - - class SerialisedTemplate(SerialisedModel): ALLOWED_PROPERTIES = { 'archived', @@ -109,60 +82,5 @@ class SerialisedTemplate(SerialisedModel): ) template_dict = template_schema.dump(fetched_template).data - db.session.commit() return {'data': template_dict} - - -class SerialisedService(SerialisedModel): - ALLOWED_PROPERTIES = { - 'id', - 'active', - 'contact_link', - 'email_from', - 'permissions', - 'research_mode', - 'restricted', - } - - @classmethod - @memory_cache - def from_id(cls, service_id): - return cls(cls.get_dict(service_id)['data']) - - @staticmethod - @redis_cache.set('service-{service_id}') - def get_dict(service_id): - from app.schemas import service_schema - - service_dict = service_schema.dump(dao_fetch_service_by_id(service_id)).data - db.session.commit() - - return {'data': service_dict} - - @cached_property - def api_keys(self): - return SerialisedAPIKeyCollection.from_service_id(self.id) - - -class SerialisedAPIKey(SerialisedModel): - ALLOWED_PROPERTIES = { - 'id', - 'secret', - 'expiry_date', - 'key_type', - } - - -class SerialisedAPIKeyCollection(SerialisedModelCollection): - model = SerialisedAPIKey - - @classmethod - @memory_cache - def from_service_id(cls, service_id): - keys = [ - {k: getattr(key, k) for k in SerialisedAPIKey.ALLOWED_PROPERTIES} - for key in get_model_api_keys(service_id) - ] - db.session.commit() - return cls(keys) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 10ed42ab7..a7886b624 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -137,9 +137,7 @@ def get_reply_to_text(notification_type, sender_id, service, template): def send_pdf_letter_notification(service_id, post_data): service = dao_fetch_service_by_id(service_id) - check_service_has_permission(LETTER_TYPE, [ - p.permission for p in service.permissions - ]) + check_service_has_permission(LETTER_TYPE, service.permissions) check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) validate_created_by(service, post_data['created_by']) validate_and_format_recipient( diff --git a/app/service/utils.py b/app/service/utils.py index c6e0e0476..16521fbbd 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -7,8 +7,6 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL) -from app.dao.services_dao import dao_fetch_service_by_id - def get_recipients_from_request(request_json, key, type): return [(type, recipient) for recipient in request_json.get(key)] @@ -35,10 +33,6 @@ def service_allowed_to_send_to(recipient, service, key_type, allow_whitelisted_r if key_type == KEY_TYPE_NORMAL and not service.restricted: return True - # Revert back to the ORM model here so we can get some things which - # aren’t in the serialised model - service = dao_fetch_service_by_id(service.id) - team_members = itertools.chain.from_iterable( [user.mobile_number, user.email_address] for user in service.users ) diff --git a/app/template/rest.py b/app/template/rest.py index 464fad1c3..da401094b 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -69,9 +69,7 @@ def validate_parent_folder(template_json): def create_template(service_id): fetched_service = dao_fetch_service_by_id(service_id=service_id) # permissions needs to be placed here otherwise marshmallow will interfere with versioning - permissions = [ - p.permission for p in fetched_service.permissions - ] + permissions = fetched_service.permissions template_json = validate(request.get_json(), post_create_template_schema) folder = validate_parent_folder(template_json=template_json) new_template = Template.from_json(template_json, folder) @@ -104,12 +102,7 @@ def create_template(service_id): def update_template(service_id, template_id): fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) - if not service_has_permission( - fetched_template.template_type, - [ - p.permission for p in fetched_template.service.permissions - ] - ): + if not service_has_permission(fetched_template.template_type, fetched_template.service.permissions): message = "Updating {} templates is not allowed".format( get_public_notify_type_text(fetched_template.template_type)) errors = {'template_type': [message]} diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index af0d766ea..cc77be913 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -332,7 +332,7 @@ def process_letter_notification( if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) - if not service.research_mode and service.restricted and api_key.key_type != KEY_TYPE_TEST: + if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) if precompiled: @@ -342,7 +342,7 @@ def process_letter_notification( template=template, reply_to_text=reply_to_text) - validate_address(service, letter_data) + validate_address(api_key, letter_data) test_key = api_key.key_type == KEY_TYPE_TEST @@ -391,10 +391,10 @@ def process_letter_notification( return resp -def validate_address(service, letter_data): +def validate_address(api_key, letter_data): address = PostalAddress.from_personalisation( letter_data['personalisation'], - allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions), + allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS), ) if not address.has_enough_lines: raise ValidationError( diff --git a/requirements-app.txt b/requirements-app.txt index 444164308..57b04538e 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -20,7 +20,6 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 -cachetools==4.1.0 notifications-python-client==5.5.1 diff --git a/requirements.txt b/requirements.txt index c70959d8c..c12e5dc4b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,7 +22,6 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 -cachetools==4.1.0 notifications-python-client==5.5.1 @@ -40,14 +39,15 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.85 +awscli==1.18.84 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.8 +botocore==1.17.7 +cachetools==4.1.0 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 24b212f4c..3b6b086aa 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -8,18 +8,9 @@ import pytest from flask import json, current_app, request from freezegun import freeze_time from notifications_python_client.authentication import create_jwt_token -from unittest.mock import call from app import api_user -from app.dao.api_key_dao import ( - get_unsigned_secrets, - save_model_api_key, - get_unsigned_secret, - expire_api_key, - get_model_api_keys, -) -from app.dao.services_dao import dao_fetch_service_by_id - +from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key from app.models import ApiKey, KEY_TYPE_NORMAL from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE @@ -309,7 +300,7 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ with pytest.raises(AuthError) as exc: requires_auth() assert exc.value.short_message == 'Invalid token: API key revoked' - assert exc.value.service_id == str(expired_api_key.service_id) + assert exc.value.service_id == expired_api_key.service_id assert exc.value.api_key_id == expired_api_key.id @@ -385,7 +376,7 @@ def test_authentication_returns_error_when_service_has_no_secrets(client, with pytest.raises(AuthError) as exc: requires_auth() assert exc.value.short_message == 'Invalid token: service has no API keys' - assert exc.value.service_id == str(sample_service.id) + assert exc.value.service_id == sample_service.id def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key): @@ -396,7 +387,7 @@ def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_ser headers={'Authorization': 'Bearer {}'.format(token)} ) assert response.status_code == 200 - assert str(api_user.id) == str(sample_api_key.id) + assert api_user == sample_api_key def test_should_return_403_when_token_is_expired(client, @@ -408,8 +399,8 @@ def test_should_return_403_when_token_is_expired(client, request.headers = {'Authorization': 'Bearer {}'.format(token)} requires_auth() assert exc.value.short_message == 'Error: Your system clock must be accurate to within 30 seconds' - assert exc.value.service_id == str(sample_api_key.service_id) - assert str(exc.value.api_key_id) == str(sample_api_key.id) + assert exc.value.service_id == sample_api_key.service_id + assert exc.value.api_key_id == sample_api_key.id def __create_token(service_id): @@ -466,28 +457,3 @@ def test_proxy_key_on_admin_auth_endpoint(notify_api, check_proxy_header, header ] ) assert response.status_code == expected_status - - -def test_should_cache_service_and_api_key_lookups(mocker, client, sample_api_key): - - mock_get_api_keys = mocker.patch( - 'app.serialised_models.get_model_api_keys', - wraps=get_model_api_keys, - ) - mock_get_service = mocker.patch( - 'app.serialised_models.dao_fetch_service_by_id', - wraps=dao_fetch_service_by_id, - ) - - for i in range(5): - token = __create_token(sample_api_key.service_id) - client.get('/notifications', headers={ - 'Authorization': f'Bearer {token}' - }) - - assert mock_get_api_keys.call_args_list == [ - call(str(sample_api_key.service_id)) - ] - assert mock_get_service.call_args_list == [ - call(str(sample_api_key.service_id)) - ] diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index ce3b0ce7a..1c4db4190 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import SerialisedService, SerialisedTemplate +from app.serialised_models import SerialisedTemplate from app.utils import get_template_instance from app.v2.errors import ( @@ -439,9 +439,8 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_ notify_db_session, ): service = create_service(service_permissions=[SMS_TYPE]) - service_model = SerialisedService.from_id(service.id) with pytest.raises(BadRequestError) as e: - validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE) + validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE) assert e.value.status_code == 400 assert e.value.message == 'Cannot send to international mobile numbers' assert e.value.fields == [] @@ -450,8 +449,7 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_ @pytest.mark.parametrize('key_type', ['test', 'normal']) def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_sms( key_type, sample_service_full_permissions): - service_model = SerialisedService.from_id(sample_service_full_permissions.id) - result = validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE) + result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service_full_permissions, SMS_TYPE) assert result == '201212341234' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 6d1ea71c2..536c4b51f 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -232,14 +232,14 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template assert mock_get_template.call_count == 1 assert mock_get_template.call_args_list == [ - call(service_id=str(sample_template.service_id), template_id=str(sample_template.id)) + call(service_id=sample_template.service_id, template_id=str(sample_template.id)) ] assert Notification.query.count() == 5 -def test_should_cache_template_and_service_in_redis(mocker, client, sample_template): +def test_should_cache_template_lookups_in_redis(mocker, client, sample_template): - from app.schemas import service_schema, template_schema + from app.schemas import template_schema mock_redis_get = mocker.patch( 'app.redis_store.get', @@ -263,49 +263,34 @@ def test_should_cache_template_and_service_in_redis(mocker, client, sample_templ headers=[('Content-Type', 'application/json'), auth_header] ) - expected_service_key = f'service-{sample_template.service_id}' - expected_templates_key = f'template-{sample_template.id}-version-None' + expected_key = f'template-{sample_template.id}-version-None' - assert mock_redis_get.call_args_list == [ - call(expected_service_key), - call(expected_templates_key), - ] + assert mock_redis_get.call_args_list == [call( + expected_key, + )] - service_dict = service_schema.dump(sample_template.service).data template_dict = template_schema.dump(sample_template).data - assert len(mock_redis_set.call_args_list) == 2 - - service_call, templates_call = mock_redis_set.call_args_list - - assert service_call[0][0] == expected_service_key - assert json.loads(service_call[0][1]) == {'data': service_dict} - assert service_call[1]['ex'] == 604_800 - - assert templates_call[0][0] == expected_templates_key - assert json.loads(templates_call[0][1]) == {'data': template_dict} - assert templates_call[1]['ex'] == 604_800 + assert len(mock_redis_set.call_args_list) == 1 + assert mock_redis_set.call_args[0][0] == expected_key + assert json.loads(mock_redis_set.call_args[0][1]) == { + 'data': template_dict, + } + assert mock_redis_set.call_args[1]['ex'] == 604_800 def test_should_return_template_if_found_in_redis(mocker, client, sample_template): - from app.schemas import service_schema, template_schema - service_dict = service_schema.dump(sample_template.service).data + from app.schemas import template_schema template_dict = template_schema.dump(sample_template).data mocker.patch( 'app.redis_store.get', - side_effect=[ - json.dumps({'data': service_dict}).encode('utf-8'), - json.dumps({'data': template_dict}).encode('utf-8'), - ], + return_value=json.dumps({'data': template_dict}).encode('utf-8') ) mock_get_template = mocker.patch( 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' ) - mock_get_service = mocker.patch( - 'app.dao.services_dao.dao_fetch_service_by_id' - ) mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') @@ -323,7 +308,6 @@ def test_should_return_template_if_found_in_redis(mocker, client, sample_templat assert response.status_code == 201 assert mock_get_template.called is False - assert mock_get_service.called is False @pytest.mark.parametrize("notification_type, key_send_to, send_to", @@ -883,8 +867,8 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke assert validate(resp_json, post_email_response) == resp_json assert document_download_mock.upload_document.call_args_list == [ - call(str(service.id), 'abababab', csv_param.get('is_csv')), - call(str(service.id), 'cdcdcdcd', csv_param.get('is_csv')) + call(service.id, 'abababab', csv_param.get('is_csv')), + call(service.id, 'cdcdcdcd', csv_param.get('is_csv')) ] notification = Notification.query.one() @@ -1033,10 +1017,7 @@ def test_post_notifications_saves_email_to_queue(client, notify_db_session, mock save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service( - service_id=current_app.config['HIGH_VOLUME_SERVICE'][0], - service_name='high volume service', - ) + service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1067,10 +1048,7 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl ) mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service( - service_id=current_app.config['HIGH_VOLUME_SERVICE'][1], - service_name='high volume service', - ) + service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1099,10 +1077,8 @@ def test_post_notifications_doesnt_save_email_to_queue_for_test_emails(client, n save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service( - service_id=current_app.config['HIGH_VOLUME_SERVICE'][2], - service_name='high volume service', - ) + service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + # create_api_key(service=service, key_type='test') template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1131,10 +1107,7 @@ def test_post_notifications_doesnt_save_email_to_queue_for_sms(client, notify_db save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - service = create_service( - service_id=current_app.config['HIGH_VOLUME_SERVICE'][3], - service_name='high volume service', - ) + service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') template = create_template(service=service, content='((message))', template_type=SMS_TYPE) data = { "phone_number": '+447700900855', From 59aba018bd23516ccd3773d524c39bac1cc376c6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 13:46:32 +0100 Subject: [PATCH 05/13] Ensure rate limit is in serialised service Once we start using the serialised service to power the `POST` notifications endpoint it needs to include rate limit to do the rate limit checks. --- app/schemas.py | 1 - tests/app/service/test_rest.py | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index c799522c8..36d9ff815 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -249,7 +249,6 @@ class ServiceSchema(BaseSchema): 'inbound_number', 'inbound_sms', 'letter_logo_filename', - 'rate_limit', 'returned_letters', 'users', 'version', diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index bd1183110..867f66b29 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -233,8 +233,34 @@ def test_get_service_by_id(admin_request, sample_service): assert json_resp['data']['id'] == str(sample_service.id) assert not json_resp['data']['research_mode'] assert json_resp['data']['email_branding'] is None - assert 'branding' not in json_resp['data'] assert json_resp['data']['prefix_sms'] is True + assert json_resp['data'].keys() == { + 'active', + 'consent_to_research', + 'contact_link', + 'count_as_live', + 'created_by', + 'email_branding', + 'email_from', + 'go_live_at', + 'go_live_user', + 'id', + 'inbound_api', + 'letter_branding', + 'message_limit', + 'name', + 'organisation', + 'organisation_type', + 'permissions', + 'prefix_sms', + 'rate_limit', + 'research_mode', + 'restricted', + 'service_callback_api', + 'volume_email', + 'volume_letter', + 'volume_sms', + } @pytest.mark.parametrize('detailed', [True, False]) From 3ffdb3093b4ebb71f586e979afca3b6abdbe2f83 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 14:10:12 +0100 Subject: [PATCH 06/13] Revert "Revert "Merge pull request #2887 from alphagov/cache-the-serialised-things"" This reverts commit 7e85e37e1d0cbff0a22db0847111fbb13f44d7f2. --- app/authentication/auth.py | 6 +- app/config.py | 7 +- app/notifications/process_notifications.py | 1 - app/notifications/validators.py | 4 +- app/schemas.py | 2 +- app/serialised_models.py | 84 ++++++++++++++++++- app/service/send_notification.py | 4 +- app/service/utils.py | 6 ++ app/template/rest.py | 11 ++- app/v2/notifications/post_notifications.py | 8 +- requirements-app.txt | 1 + requirements.txt | 6 +- .../app/authentication/test_authentication.py | 46 ++++++++-- tests/app/notifications/test_validators.py | 8 +- .../notifications/test_post_notifications.py | 71 +++++++++++----- 15 files changed, 215 insertions(+), 50 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 1b7a286ac..e26edee48 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -8,7 +8,7 @@ from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from gds_metrics import Histogram -from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys +from app.serialised_models import SerialisedService GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header' # noqa @@ -94,7 +94,7 @@ def requires_auth(): try: with AUTH_DB_CONNECTION_DURATION_SECONDS.time(): - service = dao_fetch_service_by_id_with_api_keys(issuer) + service = SerialisedService.from_id(issuer) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) except NoResultFound: @@ -129,7 +129,7 @@ def requires_auth(): if api_key.expiry_date: raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) - g.service_id = api_key.service_id + g.service_id = service.id _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key diff --git a/app/config.py b/app/config.py index 7a9f881d3..257eaea61 100644 --- a/app/config.py +++ b/app/config.py @@ -400,7 +400,12 @@ class Test(Development): NOTIFY_ENVIRONMENT = 'test' TESTING = True - HIGH_VOLUME_SERVICE = ['941b6f9a-50d7-4742-8d50-f365ca74bf27'] + HIGH_VOLUME_SERVICE = [ + '941b6f9a-50d7-4742-8d50-f365ca74bf27', + '63f95b86-2d19-4497-b8b2-ccf25457df4e', + '7e5950cb-9954-41f5-8376-962b8c8555cf', + '10d1b9c9-0072-4fa9-ae1c-595e333841da', + ] CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' CONTACT_LIST_BUCKET_NAME = 'test-contact-list' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 3f3f86fd6..e5ca0adcc 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -120,7 +120,6 @@ def persist_notification( template_version=template_version, to=recipient, service_id=service.id, - service=service, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, diff --git a/app/notifications/validators.py b/app/notifications/validators.py index c505d5a19..d1bf7b589 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -90,7 +90,7 @@ def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_ def service_has_permission(notify_type, permissions): - return notify_type in [p.permission for p in permissions] + return notify_type in permissions def check_service_has_permission(notify_type, permissions): @@ -131,7 +131,7 @@ def check_if_service_can_send_to_number(service, number): if ( # if number is international and not a crown dependency international_phone_info.international and not international_phone_info.crown_dependency - ) and INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: + ) and INTERNATIONAL_SMS_TYPE not in service.permissions: raise BadRequestError(message="Cannot send to international mobile numbers") else: return international_phone_info diff --git a/app/schemas.py b/app/schemas.py index 36d9ff815..763f34143 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -205,7 +205,7 @@ class ProviderDetailsHistorySchema(BaseSchema): strict = True -class ServiceSchema(BaseSchema): +class ServiceSchema(BaseSchema, UUIDsAsStringsMixin): created_by = field_for(models.Service, 'created_by', required=True) organisation_type = field_for(models.Service, 'organisation_type') diff --git a/app/serialised_models.py b/app/serialised_models.py index d544dc478..7f88171d2 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -5,9 +5,13 @@ from threading import RLock import cachetools from notifications_utils.clients.redis import RequestCache +from werkzeug.utils import cached_property + +from app import db, redis_store -from app import redis_store from app.dao import templates_dao +from app.dao.api_key_dao import get_model_api_keys +from app.dao.services_dao import dao_fetch_service_by_id caches = defaultdict(partial(cachetools.TTLCache, maxsize=1024, ttl=2)) locks = defaultdict(RLock) @@ -53,6 +57,29 @@ class SerialisedModel(ABC): return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) +class SerialisedModelCollection(ABC): + + """ + A SerialisedModelCollection takes a list of dictionaries, typically + created by serialising database objects. When iterated over it + returns a SerialisedModel instance for each of the items in the list. + """ + + @property + @abstractmethod + def model(self): + pass + + def __init__(self, items): + self.items = items + + def __bool__(self): + return bool(self.items) + + def __getitem__(self, index): + return self.model(self.items[index]) + + class SerialisedTemplate(SerialisedModel): ALLOWED_PROPERTIES = { 'archived', @@ -82,5 +109,60 @@ class SerialisedTemplate(SerialisedModel): ) template_dict = template_schema.dump(fetched_template).data + db.session.commit() return {'data': template_dict} + + +class SerialisedService(SerialisedModel): + ALLOWED_PROPERTIES = { + 'id', + 'active', + 'contact_link', + 'email_from', + 'permissions', + 'research_mode', + 'restricted', + } + + @classmethod + @memory_cache + def from_id(cls, service_id): + return cls(cls.get_dict(service_id)['data']) + + @staticmethod + @redis_cache.set('service-{service_id}') + def get_dict(service_id): + from app.schemas import service_schema + + service_dict = service_schema.dump(dao_fetch_service_by_id(service_id)).data + db.session.commit() + + return {'data': service_dict} + + @cached_property + def api_keys(self): + return SerialisedAPIKeyCollection.from_service_id(self.id) + + +class SerialisedAPIKey(SerialisedModel): + ALLOWED_PROPERTIES = { + 'id', + 'secret', + 'expiry_date', + 'key_type', + } + + +class SerialisedAPIKeyCollection(SerialisedModelCollection): + model = SerialisedAPIKey + + @classmethod + @memory_cache + def from_service_id(cls, service_id): + keys = [ + {k: getattr(key, k) for k in SerialisedAPIKey.ALLOWED_PROPERTIES} + for key in get_model_api_keys(service_id) + ] + db.session.commit() + return cls(keys) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index a7886b624..10ed42ab7 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -137,7 +137,9 @@ def get_reply_to_text(notification_type, sender_id, service, template): def send_pdf_letter_notification(service_id, post_data): service = dao_fetch_service_by_id(service_id) - check_service_has_permission(LETTER_TYPE, service.permissions) + check_service_has_permission(LETTER_TYPE, [ + p.permission for p in service.permissions + ]) check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service) validate_created_by(service, post_data['created_by']) validate_and_format_recipient( diff --git a/app/service/utils.py b/app/service/utils.py index 16521fbbd..c6e0e0476 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -7,6 +7,8 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL) +from app.dao.services_dao import dao_fetch_service_by_id + def get_recipients_from_request(request_json, key, type): return [(type, recipient) for recipient in request_json.get(key)] @@ -33,6 +35,10 @@ def service_allowed_to_send_to(recipient, service, key_type, allow_whitelisted_r if key_type == KEY_TYPE_NORMAL and not service.restricted: return True + # Revert back to the ORM model here so we can get some things which + # aren’t in the serialised model + service = dao_fetch_service_by_id(service.id) + team_members = itertools.chain.from_iterable( [user.mobile_number, user.email_address] for user in service.users ) diff --git a/app/template/rest.py b/app/template/rest.py index da401094b..464fad1c3 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -69,7 +69,9 @@ def validate_parent_folder(template_json): def create_template(service_id): fetched_service = dao_fetch_service_by_id(service_id=service_id) # permissions needs to be placed here otherwise marshmallow will interfere with versioning - permissions = fetched_service.permissions + permissions = [ + p.permission for p in fetched_service.permissions + ] template_json = validate(request.get_json(), post_create_template_schema) folder = validate_parent_folder(template_json=template_json) new_template = Template.from_json(template_json, folder) @@ -102,7 +104,12 @@ def create_template(service_id): def update_template(service_id, template_id): fetched_template = dao_get_template_by_id_and_service_id(template_id=template_id, service_id=service_id) - if not service_has_permission(fetched_template.template_type, fetched_template.service.permissions): + if not service_has_permission( + fetched_template.template_type, + [ + p.permission for p in fetched_template.service.permissions + ] + ): message = "Updating {} templates is not allowed".format( get_public_notify_type_text(fetched_template.template_type)) errors = {'template_type': [message]} diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index cc77be913..af0d766ea 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -332,7 +332,7 @@ def process_letter_notification( if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) - if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: + if not service.research_mode and service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) if precompiled: @@ -342,7 +342,7 @@ def process_letter_notification( template=template, reply_to_text=reply_to_text) - validate_address(api_key, letter_data) + validate_address(service, letter_data) test_key = api_key.key_type == KEY_TYPE_TEST @@ -391,10 +391,10 @@ def process_letter_notification( return resp -def validate_address(api_key, letter_data): +def validate_address(service, letter_data): address = PostalAddress.from_personalisation( letter_data['personalisation'], - allow_international_letters=api_key.service.has_permission(INTERNATIONAL_LETTERS), + allow_international_letters=(INTERNATIONAL_LETTERS in service.permissions), ) if not address.has_enough_lines: raise ValidationError( diff --git a/requirements-app.txt b/requirements-app.txt index 57b04538e..444164308 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -20,6 +20,7 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +cachetools==4.1.0 notifications-python-client==5.5.1 diff --git a/requirements.txt b/requirements.txt index c12e5dc4b..c70959d8c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,7 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +cachetools==4.1.0 notifications-python-client==5.5.1 @@ -39,15 +40,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.84 +awscli==1.18.85 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.7 -cachetools==4.1.0 +botocore==1.17.8 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 3b6b086aa..24b212f4c 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -8,9 +8,18 @@ import pytest from flask import json, current_app, request from freezegun import freeze_time from notifications_python_client.authentication import create_jwt_token +from unittest.mock import call from app import api_user -from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key +from app.dao.api_key_dao import ( + get_unsigned_secrets, + save_model_api_key, + get_unsigned_secret, + expire_api_key, + get_model_api_keys, +) +from app.dao.services_dao import dao_fetch_service_by_id + from app.models import ApiKey, KEY_TYPE_NORMAL from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE @@ -300,7 +309,7 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ with pytest.raises(AuthError) as exc: requires_auth() assert exc.value.short_message == 'Invalid token: API key revoked' - assert exc.value.service_id == expired_api_key.service_id + assert exc.value.service_id == str(expired_api_key.service_id) assert exc.value.api_key_id == expired_api_key.id @@ -376,7 +385,7 @@ def test_authentication_returns_error_when_service_has_no_secrets(client, with pytest.raises(AuthError) as exc: requires_auth() assert exc.value.short_message == 'Invalid token: service has no API keys' - assert exc.value.service_id == sample_service.id + assert exc.value.service_id == str(sample_service.id) def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key): @@ -387,7 +396,7 @@ def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_ser headers={'Authorization': 'Bearer {}'.format(token)} ) assert response.status_code == 200 - assert api_user == sample_api_key + assert str(api_user.id) == str(sample_api_key.id) def test_should_return_403_when_token_is_expired(client, @@ -399,8 +408,8 @@ def test_should_return_403_when_token_is_expired(client, request.headers = {'Authorization': 'Bearer {}'.format(token)} requires_auth() assert exc.value.short_message == 'Error: Your system clock must be accurate to within 30 seconds' - assert exc.value.service_id == sample_api_key.service_id - assert exc.value.api_key_id == sample_api_key.id + assert exc.value.service_id == str(sample_api_key.service_id) + assert str(exc.value.api_key_id) == str(sample_api_key.id) def __create_token(service_id): @@ -457,3 +466,28 @@ def test_proxy_key_on_admin_auth_endpoint(notify_api, check_proxy_header, header ] ) assert response.status_code == expected_status + + +def test_should_cache_service_and_api_key_lookups(mocker, client, sample_api_key): + + mock_get_api_keys = mocker.patch( + 'app.serialised_models.get_model_api_keys', + wraps=get_model_api_keys, + ) + mock_get_service = mocker.patch( + 'app.serialised_models.dao_fetch_service_by_id', + wraps=dao_fetch_service_by_id, + ) + + for i in range(5): + token = __create_token(sample_api_key.service_id) + client.get('/notifications', headers={ + 'Authorization': f'Bearer {token}' + }) + + assert mock_get_api_keys.call_args_list == [ + call(str(sample_api_key.service_id)) + ] + assert mock_get_service.call_args_list == [ + call(str(sample_api_key.service_id)) + ] diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 1c4db4190..ce3b0ce7a 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import SerialisedTemplate +from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import get_template_instance from app.v2.errors import ( @@ -439,8 +439,9 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_ notify_db_session, ): service = create_service(service_permissions=[SMS_TYPE]) + service_model = SerialisedService.from_id(service.id) with pytest.raises(BadRequestError) as e: - validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE) + validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE) assert e.value.status_code == 400 assert e.value.message == 'Cannot send to international mobile numbers' assert e.value.fields == [] @@ -449,7 +450,8 @@ def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_ @pytest.mark.parametrize('key_type', ['test', 'normal']) def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_sms( key_type, sample_service_full_permissions): - result = validate_and_format_recipient('20-12-1234-1234', key_type, sample_service_full_permissions, SMS_TYPE) + service_model = SerialisedService.from_id(sample_service_full_permissions.id) + result = validate_and_format_recipient('20-12-1234-1234', key_type, service_model, SMS_TYPE) assert result == '201212341234' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 536c4b51f..6d1ea71c2 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -232,14 +232,14 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template assert mock_get_template.call_count == 1 assert mock_get_template.call_args_list == [ - call(service_id=sample_template.service_id, template_id=str(sample_template.id)) + call(service_id=str(sample_template.service_id), template_id=str(sample_template.id)) ] assert Notification.query.count() == 5 -def test_should_cache_template_lookups_in_redis(mocker, client, sample_template): +def test_should_cache_template_and_service_in_redis(mocker, client, sample_template): - from app.schemas import template_schema + from app.schemas import service_schema, template_schema mock_redis_get = mocker.patch( 'app.redis_store.get', @@ -263,34 +263,49 @@ def test_should_cache_template_lookups_in_redis(mocker, client, sample_template) headers=[('Content-Type', 'application/json'), auth_header] ) - expected_key = f'template-{sample_template.id}-version-None' + expected_service_key = f'service-{sample_template.service_id}' + expected_templates_key = f'template-{sample_template.id}-version-None' - assert mock_redis_get.call_args_list == [call( - expected_key, - )] + assert mock_redis_get.call_args_list == [ + call(expected_service_key), + call(expected_templates_key), + ] + service_dict = service_schema.dump(sample_template.service).data template_dict = template_schema.dump(sample_template).data - assert len(mock_redis_set.call_args_list) == 1 - assert mock_redis_set.call_args[0][0] == expected_key - assert json.loads(mock_redis_set.call_args[0][1]) == { - 'data': template_dict, - } - assert mock_redis_set.call_args[1]['ex'] == 604_800 + assert len(mock_redis_set.call_args_list) == 2 + + service_call, templates_call = mock_redis_set.call_args_list + + assert service_call[0][0] == expected_service_key + assert json.loads(service_call[0][1]) == {'data': service_dict} + assert service_call[1]['ex'] == 604_800 + + assert templates_call[0][0] == expected_templates_key + assert json.loads(templates_call[0][1]) == {'data': template_dict} + assert templates_call[1]['ex'] == 604_800 def test_should_return_template_if_found_in_redis(mocker, client, sample_template): - from app.schemas import template_schema + from app.schemas import service_schema, template_schema + service_dict = service_schema.dump(sample_template.service).data template_dict = template_schema.dump(sample_template).data mocker.patch( 'app.redis_store.get', - return_value=json.dumps({'data': template_dict}).encode('utf-8') + side_effect=[ + json.dumps({'data': service_dict}).encode('utf-8'), + json.dumps({'data': template_dict}).encode('utf-8'), + ], ) mock_get_template = mocker.patch( 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' ) + mock_get_service = mocker.patch( + 'app.dao.services_dao.dao_fetch_service_by_id' + ) mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') @@ -308,6 +323,7 @@ def test_should_return_template_if_found_in_redis(mocker, client, sample_templat assert response.status_code == 201 assert mock_get_template.called is False + assert mock_get_service.called is False @pytest.mark.parametrize("notification_type, key_send_to, send_to", @@ -867,8 +883,8 @@ def test_post_notification_with_document_upload(client, notify_db_session, mocke assert validate(resp_json, post_email_response) == resp_json assert document_download_mock.upload_document.call_args_list == [ - call(service.id, 'abababab', csv_param.get('is_csv')), - call(service.id, 'cdcdcdcd', csv_param.get('is_csv')) + call(str(service.id), 'abababab', csv_param.get('is_csv')), + call(str(service.id), 'cdcdcdcd', csv_param.get('is_csv')) ] notification = Notification.query.one() @@ -1017,7 +1033,10 @@ def test_post_notifications_saves_email_to_queue(client, notify_db_session, mock save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][0], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1048,7 +1067,10 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl ) mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][1], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1077,8 +1099,10 @@ def test_post_notifications_doesnt_save_email_to_queue_for_test_emails(client, n save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') - # create_api_key(service=service, key_type='test') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][2], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1107,7 +1131,10 @@ def test_post_notifications_doesnt_save_email_to_queue_for_sms(client, notify_db save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][3], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=SMS_TYPE) data = { "phone_number": '+447700900855', From 1f315b06e2eddc11cb7bd0ffafc2483c67ce5c45 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 14:10:21 +0100 Subject: [PATCH 07/13] Revert "Revert "Merge pull request #2902 from alphagov/fix-imports"" This reverts commit b8fe7b8e61dd7f1162052e6dc7fa2f3ef42aafe7. --- app/serialised_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serialised_models.py b/app/serialised_models.py index 7f88171d2..e47c8800c 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -9,7 +9,6 @@ from werkzeug.utils import cached_property from app import db, redis_store -from app.dao import templates_dao from app.dao.api_key_dao import get_model_api_keys from app.dao.services_dao import dao_fetch_service_by_id @@ -101,6 +100,7 @@ class SerialisedTemplate(SerialisedModel): @staticmethod @redis_cache.set('template-{template_id}-version-None') def get_dict(template_id, service_id): + from app.dao import templates_dao from app.schemas import template_schema fetched_template = templates_dao.dao_get_template_by_id_and_service_id( From 9f41e77bf7aa7065bcb3be3afe77e6211fd5da4f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 13:38:58 +0100 Subject: [PATCH 08/13] Add rate_limit and message_limit to SerialisedService The API needs these to check whether a service can send a notification. This commit also updates all the tests in `test_validators.py` to take a serialised service, not a database object. --- app/serialised_models.py | 2 + tests/app/notifications/test_validators.py | 82 ++++++++++++++++------ 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/app/serialised_models.py b/app/serialised_models.py index e47c8800c..2b3af3903 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -120,7 +120,9 @@ class SerialisedService(SerialisedModel): 'active', 'contact_link', 'email_from', + 'message_limit', 'permissions', + 'rate_limit', 'research_mode', 'restricted', } diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index ce3b0ce7a..4e6748917 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import SerialisedService, SerialisedTemplate +from app.serialised_models import SerialisedService, SerialisedTemplate, SerialisedAPIKeyCollection from app.utils import get_template_instance from app.v2.errors import ( @@ -42,6 +42,7 @@ from tests.app.db import ( create_service_whitelist, create_template, ) +from unittest.mock import ANY # all of these tests should have redis enabled (except where we specifically disable it) @@ -56,12 +57,20 @@ def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allow key_type, sample_service, mocker): - mocker.patch('app.notifications.validators.redis_store.get', return_value=1) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[ + None, # The serialised service + 1, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') + serialised_service = SerialisedService.from_id(sample_service.id) - check_service_over_daily_message_limit(key_type, sample_service) - app.notifications.validators.redis_store.set.assert_not_called() + check_service_over_daily_message_limit(key_type, serialised_service) + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls @@ -70,17 +79,27 @@ def test_check_service_message_limit_in_cache_under_message_limit_passes( key_type, sample_service, mocker): - mocker.patch('app.notifications.validators.redis_store.get', return_value=1) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[ + None, # The serialised service + 1, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') - check_service_over_daily_message_limit(key_type, sample_service) - app.notifications.validators.redis_store.set.assert_not_called() + serialised_service = SerialisedService.from_id(sample_service.id) + check_service_over_daily_message_limit(key_type, serialised_service) + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls def test_should_not_interact_with_cache_for_test_key(sample_service, mocker): mocker.patch('app.notifications.validators.redis_store') - check_service_over_daily_message_limit('test', sample_service) + mocker.patch('app.notifications.validators.redis_store.get', side_effect=[None]) + serialised_service = SerialisedService.from_id(sample_service.id) + check_service_over_daily_message_limit('test', serialised_service) assert not app.notifications.validators.redis_store.mock_calls @@ -91,22 +110,24 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set( sample_service, mocker ): + serialised_service = SerialisedService.from_id(sample_service.id) with freeze_time("2016-01-01 12:00:00.000000"): for x in range(5): create_notification(sample_template) mocker.patch('app.notifications.validators.redis_store.get', return_value=None) mocker.patch('app.notifications.validators.redis_store.set') - check_service_over_daily_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, serialised_service) app.notifications.validators.redis_store.set.assert_called_with( str(sample_service.id) + "-2016-01-01-count", 5, ex=3600 ) def test_should_not_access_database_if_redis_disabled(notify_api, sample_service, mocker): + serialised_service = SerialisedService.from_id(sample_service.id) with set_config(notify_api, 'REDIS_ENABLED', False): db_mock = mocker.patch('app.notifications.validators.services_dao') - check_service_over_daily_message_limit('normal', sample_service) + check_service_over_daily_message_limit('normal', serialised_service) assert db_mock.method_calls == [] @@ -120,11 +141,12 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, sample_s sample_service.restricted = True sample_service.message_limit = 4 template = create_template(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) for x in range(5): create_notification(template) with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, serialised_service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -139,17 +161,25 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails( key_type, mocker): with freeze_time("2016-01-01 12:00:00.000000"): - mocker.patch('app.redis_store.get', return_value=5) + mocker.patch('app.redis_store.get', side_effect=[ + None, # The serialised service + 5, # The rolling count + ]) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') service = create_service(restricted=True, message_limit=4) + serialised_service = SerialisedService.from_id(service.id) with pytest.raises(TooManyRequestsError) as e: - check_service_over_daily_message_limit(key_type, service) + check_service_over_daily_message_limit(key_type, serialised_service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] - app.notifications.validators.redis_store.set.assert_not_called() + app.notifications.validators.redis_store.set.assert_called_once_with( + f'service-{serialised_service.id}', + ANY, + ex=ANY, + ) assert not app.notifications.validators.services_dao.mock_calls @@ -194,23 +224,25 @@ def test_check_template_is_active_fails(sample_template): ['test', 'normal']) def test_service_can_send_to_recipient_passes(key_type, notify_db_session): trial_mode_service = create_service(service_name='trial mode', restricted=True) + serialised_service = SerialisedService.from_id(trial_mode_service.id) assert service_can_send_to_recipient(trial_mode_service.users[0].email_address, key_type, - trial_mode_service) is None + serialised_service) is None assert service_can_send_to_recipient(trial_mode_service.users[0].mobile_number, key_type, - trial_mode_service) is None + serialised_service) is None @pytest.mark.parametrize('key_type', ['test', 'normal']) def test_service_can_send_to_recipient_passes_for_live_service_non_team_member(key_type, sample_service): + serialised_service = SerialisedService.from_id(sample_service.id) assert service_can_send_to_recipient("some_other_email@test.com", key_type, - sample_service) is None + serialised_service) is None assert service_can_send_to_recipient('07513332413', key_type, - sample_service) is None + serialised_service) is None def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(sample_service): @@ -383,9 +415,11 @@ def test_that_when_exceed_rate_limit_request_fails( sample_service.restricted = True api_key = create_api_key(sample_service, key_type=api_key_type) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] with pytest.raises(RateLimitError) as e: - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert app.redis_store.exceeded_rate_limit.called_with( "{}-{}".format(str(sample_service.id), api_key.key_type), @@ -408,8 +442,10 @@ def test_that_when_not_exceeded_rate_limit_request_succeeds( sample_service.restricted = True api_key = create_api_key(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert app.redis_store.exceeded_rate_limit.called_with( "{}-{}".format(str(sample_service.id), api_key.key_type), 3000, @@ -427,9 +463,11 @@ def test_should_not_rate_limit_if_limiting_is_disabled( mocker.patch('app.notifications.validators.services_dao') sample_service.restricted = True - api_key = create_api_key(sample_service) + create_api_key(sample_service) + serialised_service = SerialisedService.from_id(sample_service.id) + serialised_api_key = SerialisedAPIKeyCollection.from_service_id(serialised_service.id)[0] - check_service_over_api_rate_limit(sample_service, api_key) + check_service_over_api_rate_limit(serialised_service, serialised_api_key) assert not app.redis_store.exceeded_rate_limit.called From 8def7d0d3b842ca1c4aec672cf99653b5472d2b5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 26 Jun 2020 16:31:49 +0100 Subject: [PATCH 09/13] Fix serialisation of callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because the IDs of our callback and inbound SMS APIs were stored in lists instead of directly on the serialised model they weren’t getting cast to a string before trying to JSONify them. And JSON doesn’t know what to do with a UUID object. For some reason this was only affecting the endpoint for fetching inbound SMS. --- app/schemas.py | 7 ++++++ .../v2/inbound_sms/test_get_inbound_sms.py | 23 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 763f34143..6199c14bd 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -63,9 +63,16 @@ class UUIDsAsStringsMixin: @post_dump() def __post_dump(self, data): for key, value in data.items(): + if isinstance(value, UUID): data[key] = str(value) + if isinstance(value, list): + data[key] = [ + (str(item) if isinstance(item, UUID) else item) + for item in value + ] + class BaseSchema(ma.ModelSchema): diff --git a/tests/app/v2/inbound_sms/test_get_inbound_sms.py b/tests/app/v2/inbound_sms/test_get_inbound_sms.py index dfd4ae0b9..7231ed9b9 100644 --- a/tests/app/v2/inbound_sms/test_get_inbound_sms.py +++ b/tests/app/v2/inbound_sms/test_get_inbound_sms.py @@ -1,7 +1,7 @@ from flask import json, url_for from tests import create_authorization_header -from tests.app.db import create_inbound_sms +from tests.app.db import create_inbound_sms, create_service_inbound_api, create_service_callback_api def test_get_inbound_sms_returns_200( @@ -31,6 +31,27 @@ def test_get_inbound_sms_returns_200( assert json_response == expected_response +def test_get_inbound_sms_returns_200_when_service_has_callbacks( + client, sample_service +): + create_service_inbound_api( + service=sample_service, + url="https://inbound.example.com", + ) + create_service_callback_api( + service=sample_service, + url="https://inbound.example.com", + ) + + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.get( + path='/v2/received-text-messages', + headers=[('Content-Type', 'application/json'), auth_header], + ) + + assert response.status_code == 200 + + def test_get_inbound_sms_generate_page_links(client, sample_service, mocker): mocker.patch.dict( "app.v2.inbound_sms.get_inbound_sms.current_app.config", From 02e5ff61a42459f4d80df22fec40769f0144cebe Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Jun 2020 11:16:29 +0100 Subject: [PATCH 10/13] Test that template history updates created by --- tests/app/template/test_rest.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 5b49a8cd7..59f9fbff3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -340,8 +340,13 @@ def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, def test_update_should_update_a_template(client, sample_user): + service = create_service(service_permissions=[LETTER_TYPE]) template = create_template(service, template_type="letter", postage="second") + + assert template.created_by == service.created_by + assert template.created_by != sample_user + data = { 'content': 'my template has new content, swell!', 'created_by': str(sample_user.id), @@ -366,6 +371,14 @@ def test_update_should_update_a_template(client, sample_user): assert update_json_resp['data']['template_type'] == template.template_type assert update_json_resp['data']['version'] == 2 + assert update_json_resp['data']['created_by'] == str(sample_user.id) + assert [ + template.created_by_id for template in TemplateHistory.query.all() + ] == [ + service.created_by.id, + sample_user.id, + ] + def test_should_be_able_to_archive_template(client, sample_template): data = { From f541ad42d67b8e222997debdbfe647a0bf8c501a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Jun 2020 11:16:37 +0100 Subject: [PATCH 11/13] Revert "Avoid extra query when serialising Template created_by" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 58a9862cd1392c0c3384ce37d4b8eeb51dd7da32. That commit tried to optimize the fetch template query by However it had the side effect of making Marshmallow ignore `created_by` when loading the JSON in the post request. So the field on the model was never being updated, just copied from the original template. The quick way of getting things to work again is to revert this optimisation. There’s probably some Marshmallow magic we could use to avoid the extra query and still have created_by not be ignored. It does mean we’re introducing an extra query, but I’m not too fussed about that since the caching seems to be working well. --- app/schemas.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 6199c14bd..3258c1c45 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -356,9 +356,7 @@ class BaseTemplateSchema(BaseSchema): class TemplateSchema(BaseTemplateSchema, UUIDsAsStringsMixin): - created_by_id = field_for( - models.Template, 'created_by_id', dump_to='created_by', dump_only=True - ) + created_by = field_for(models.Template, 'created_by', required=True) process_type = field_for(models.Template, 'process_type') redact_personalisation = fields.Method("redact") @@ -372,9 +370,6 @@ class TemplateSchema(BaseTemplateSchema, UUIDsAsStringsMixin): if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') - class Meta(BaseTemplateSchema.Meta): - exclude = BaseTemplateSchema.Meta.exclude + ('created_by',) - class TemplateSchemaNoDetail(TemplateSchema): class Meta(TemplateSchema.Meta): From 12f460adc5921288f91fcb59269f16d69f53d80e Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 25 Jun 2020 17:22:08 +0100 Subject: [PATCH 12/13] Turn off statsd wrapper for synchronous statsd calls during POSTs This commit turns off StatsD metrics for the following - the `dao_create_notification` function - the `user-agent` metric - the response times and response codes per flask endpoint This has been done with the purpose of not having the creation of text messages or emails call out to StatsD during the request process. These are the three current metrics that are currently called during the processing of one of those requests and so have been removed from the API. The reason for removing the calls out to StatsD when processing a request to send a notification is that we have seen two incidents recently affected by DNS resolution for StatsD (either by a slow down in resolution time or a failure to resolve). These POST requests are our most critical code path and we don't want them to be affected by any potential unforeseen trouble with StatsD DNS resolution. We are not going to miss the removal of these metrics. - the `dao_create_notification` metric is rarely/never looked at - the `user-agent` metric is rarely/never looked at and can be got from our logs if we want it - the response times and response codes per flask endpoint are already exposed using the gds metrics python library I did not remove the statsd metrics from any other parts of the API because - As the POST notification endpoints are the main source of web traffic, we should have already removed most calls to StatsD which should greatly reduce the chance of their being any further issues with DNS resolution - Some of the other metrics still provide value so no point deleting them if we don't need to - The metrics on celery tasks will not affect any HTTP requests from users as they are async and also we do not currently have the infrastructure set up to replace them with a prometheus HTTP endpoint that can be scraped so this would require more work --- app/__init__.py | 16 ---------------- app/dao/notifications_dao.py | 1 - requirements-app.txt | 2 +- requirements.txt | 10 +++++----- tests/app/test_user_agent_processing.py | 13 ------------- 5 files changed, 6 insertions(+), 36 deletions(-) delete mode 100644 tests/app/test_user_agent_processing.py diff --git a/app/__init__.py b/app/__init__.py index 4563cd473..52a527021 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -269,10 +269,6 @@ def register_v2_blueprints(application): def init_app(app): - @app.before_request - def record_user_agent(): - statsd_client.incr("user-agent.{}".format(process_user_agent(request.headers.get('User-Agent', None)))) - @app.before_request def record_request_details(): CONCURRENT_REQUESTS.inc() @@ -319,18 +315,6 @@ def create_random_identifier(): return ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(16)) -def process_user_agent(user_agent_string): - if user_agent_string and user_agent_string.lower().startswith("notify"): - components = user_agent_string.split("/") - client_name = components[0].lower() - client_version = components[1].replace(".", "-") - return "{}.{}".format(client_name, client_version) - elif user_agent_string and not user_agent_string.lower().startswith("notify"): - return "non-notify-user-agent" - else: - return "unknown" - - def setup_sqlalchemy_events(app): TOTAL_DB_CONNECTIONS = Gauge( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8b0b67568..8bc62657e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -77,7 +77,6 @@ def dao_get_last_date_template_was_used(template_id, service_id): return last_date -@statsd(namespace="dao") @transactional def dao_create_notification(notification): if not notification.id: diff --git a/requirements-app.txt b/requirements-app.txt index 444164308..58add7c6c 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,7 +27,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.7.1#egg=notifications-utils==39.7.1 +git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 diff --git a/requirements.txt b/requirements.txt index c70959d8c..3d1087c14 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.7.1#egg=notifications-utils==39.7.1 +git+https://github.com/alphagov/notifications-utils.git@40.0.0#egg=notifications-utils==40.0.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 @@ -40,14 +40,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.85 +awscli==1.18.89 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.8 +botocore==1.17.12 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 @@ -58,8 +58,8 @@ flask-redis==0.4.0 future==0.18.2 govuk-bank-holidays==0.6 greenlet==0.4.16 -idna==2.9 -importlib-metadata==1.6.1 +idna==2.10 +importlib-metadata==1.7.0 Jinja2==2.11.2 jmespath==0.10.0 kombu==3.0.37 diff --git a/tests/app/test_user_agent_processing.py b/tests/app/test_user_agent_processing.py deleted file mode 100644 index 161b0e475..000000000 --- a/tests/app/test_user_agent_processing.py +++ /dev/null @@ -1,13 +0,0 @@ -from app import process_user_agent - - -def test_can_process_notify_api_user_agent(): - assert "notify-api-python-client.3-0-0" == process_user_agent("NOTIFY-API-PYTHON-CLIENT/3.0.0") - - -def test_can_handle_non_notify_api_user_agent(): - assert "non-notify-user-agent" == process_user_agent("Mozilla/5.0 (iPad; U; CPU OS 3_2_1 like Mac OS X; en-us) AppleWebKit/531.21.10 (KHTML, like Gecko) Mobile/7B405") # noqa - - -def test_handles_null_user_agent(): - assert "unknown" == process_user_agent(None) From bf6e468a7c9b0d79f2b8b14092ced5562f0161cb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 29 Jun 2020 12:48:24 +0100 Subject: [PATCH 13/13] Keep excluding created_by from TemplateSchemaNoDetail --- app/schemas.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/schemas.py b/app/schemas.py index 3258c1c45..a2601ed75 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -377,6 +377,7 @@ class TemplateSchemaNoDetail(TemplateSchema): 'archived', 'content', 'created_at', + 'created_by', 'created_by_id', 'hidden', 'postage',