diff --git a/app/config.py b/app/config.py index 5dd33557e..12a3f8d09 100644 --- a/app/config.py +++ b/app/config.py @@ -2,12 +2,13 @@ from datetime import timedelta from celery.schedules import crontab from kombu import Exchange, Queue import os - +from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST if os.environ.get('VCAP_SERVICES'): # on cloudfoundry, config is a json blob in VCAP_SERVICES - unpack it, and populate # standard environment variables from it from app.cloudfoundry_config import extract_cloudfoundry_config + extract_cloudfoundry_config() @@ -176,6 +177,21 @@ class Config(object): DVLA_UPLOAD_BUCKET_NAME = "{}-dvla-file-per-job".format(os.getenv('NOTIFY_ENVIRONMENT')) + API_KEY_LIMITS = { + KEY_TYPE_TEAM: { + "limit": 3000, + "interval": 60 + }, + KEY_TYPE_NORMAL: { + "limit": 3000, + "interval": 60 + }, + KEY_TYPE_TEST: { + "limit": 3000, + "interval": 60 + } + } + ###################### # Config overrides ### @@ -223,6 +239,21 @@ class Test(Config): REDIS_ENABLED = True API_HOST_NAME = "http://localhost:6011" + API_KEY_LIMITS = { + KEY_TYPE_TEAM: { + "limit": 1, + "interval": 2 + }, + KEY_TYPE_NORMAL: { + "limit": 10, + "interval": 20 + }, + KEY_TYPE_TEST: { + "limit": 100, + "interval": 200 + } + } + class Preview(Config): NOTIFY_EMAIL_DOMAIN = 'notify.works' diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 57e1efeed..e325e3ae0 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -21,9 +21,9 @@ from app.notifications.process_client_response import ( from app.notifications.process_notifications import (persist_notification, send_notification_to_queue, simulated_recipient) -from app.notifications.validators import (check_service_message_limit, +from app.notifications.validators import (check_service_over_daily_message_limit, check_template_is_for_notification_type, - check_template_is_active) + check_template_is_active, check_rate_limiting) from app.schemas import ( email_notification_schema, sms_template_notification_schema, @@ -107,7 +107,7 @@ def send_notification(notification_type): if errors: raise InvalidRequest(errors, status_code=400) - check_service_message_limit(api_user.key_type, service) + check_rate_limiting(service, api_user) template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification_form['template'], service_id=service.id) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index aefee34e2..f826e1e0e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -4,12 +4,26 @@ from notifications_utils.recipients import validate_and_format_phone_number, val from app.dao import services_dao from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE from app.service.utils import service_allowed_to_send_to -from app.v2.errors import TooManyRequestsError, BadRequestError +from app.v2.errors import TooManyRequestsError, BadRequestError, RateLimitError from app import redis_store from notifications_utils.clients import redis -def check_service_message_limit(key_type, service): +def check_service_over_api_rate_limit(service, api_key): + cache_key = redis.rate_limit_cache_key(service.id, api_key.key_type) + rate_limit = current_app.config['API_KEY_LIMITS'][api_key.key_type]['limit'] + interval = current_app.config['API_KEY_LIMITS'][api_key.key_type]['interval'] + if redis_store.exceeded_rate_limit( + cache_key, + rate_limit, + interval): + raise RateLimitError( + rate_limit, + interval, + api_key.key_type) + + +def check_service_over_daily_message_limit(key_type, service): if key_type != KEY_TYPE_TEST: cache_key = redis.daily_limit_cache_key(service.id) service_stats = redis_store.get(cache_key) @@ -20,6 +34,11 @@ def check_service_message_limit(key_type, service): raise TooManyRequestsError(service.message_limit) +def check_rate_limiting(service, api_key): + check_service_over_api_rate_limit(service, api_key) + check_service_over_daily_message_limit(api_key.key_type, service) + + def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: message = "{0} template is not suitable for {1} notification".format(template_type, @@ -55,8 +74,6 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type) def check_sms_content_char_count(content_count): char_count_limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT') - if ( - content_count > char_count_limit - ): + if content_count > char_count_limit: message = 'Content for template has a character count greater than the limit of {}'.format(char_count_limit) raise BadRequestError(message=message) diff --git a/app/v2/errors.py b/app/v2/errors.py index 77958962b..cb441deb9 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -16,6 +16,19 @@ class TooManyRequestsError(InvalidRequest): self.message = self.message_template.format(sending_limit) +class RateLimitError(InvalidRequest): + status_code = 429 + message_template = 'Exceeded rate limit for key type {} of {} requests per {} seconds' + + def __init__(self, sending_limit, interval, key_type): + # normal keys are spoken of as "live" in the documentation + # so using this in the error messaging + if key_type == 'normal': + key_type = 'live' + + self.message = self.message_template.format(key_type.upper(), sending_limit, interval) + + class BadRequestError(InvalidRequest): status_code = 400 message = "An error occurred" diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 5f046edb0..78aa4fa91 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -4,21 +4,25 @@ from sqlalchemy.orm.exc import NoResultFound from app import api_user from app.dao import services_dao, templates_dao from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY -from app.notifications.process_notifications import (create_content_for_notification, - persist_notification, - send_notification_to_queue, - simulated_recipient) -from app.notifications.validators import (check_service_message_limit, - check_template_is_for_notification_type, - check_template_is_active, - check_sms_content_char_count, - validate_and_format_recipient) +from app.notifications.process_notifications import ( + create_content_for_notification, + persist_notification, + send_notification_to_queue, + simulated_recipient) +from app.notifications.validators import ( + check_template_is_for_notification_type, + check_template_is_active, + check_sms_content_char_count, + validate_and_format_recipient, + check_rate_limiting) from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint -from app.v2.notifications.notification_schemas import (post_sms_request, - create_post_sms_response_from_notification, post_email_request, - create_post_email_response_from_notification) +from app.v2.notifications.notification_schemas import ( + post_sms_request, + create_post_sms_response_from_notification, + post_email_request, + create_post_email_response_from_notification) @v2_notification_blueprint.route('/', methods=['POST']) @@ -28,7 +32,9 @@ def post_notification(notification_type): else: form = validate(request.get_json(), post_sms_request) service = services_dao.dao_fetch_service_by_id(api_user.service_id) - check_service_message_limit(api_user.key_type, service) + + check_rate_limiting(service, api_user) + form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] send_to = validate_and_format_recipient(send_to=form_send_to, key_type=api_user.key_type, @@ -39,6 +45,7 @@ def post_notification(notification_type): # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(send_to, notification_type) + notification = persist_notification(template_id=template.id, template_version=template.version, recipient=send_to, @@ -49,6 +56,7 @@ def post_notification(notification_type): key_type=api_user.key_type, client_reference=form.get('reference', None), simulated=simulated) + if not simulated: queue_name = 'priority' if template.process_type == PRIORITY else None send_notification_to_queue(notification=notification, research_mode=service.research_mode, queue=queue_name) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index d6a4e9c31..9599d2f9a 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -13,6 +13,7 @@ from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, No from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.dao.services_dao import dao_update_service from app.dao.api_key_dao import save_model_api_key +from app.v2.errors import RateLimitError from tests import create_authorization_header from tests.app.conftest import ( sample_notification as create_sample_notification, @@ -1046,3 +1047,49 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +@pytest.mark.parametrize( + "notification_type, send_to", + [("sms", "07700 900 855"), ("email", "sample@email.com")] +) +def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( + client, + notify_db, + notify_db_session, + mocker, + notification_type, + send_to +): + sample = create_sample_template( + notify_db, + notify_db_session, + template_type=notification_type + ) + persist_mock = mocker.patch('app.notifications.rest.persist_notification') + deliver_mock = mocker.patch('app.notifications.rest.send_notification_to_queue') + + mocker.patch( + 'app.notifications.rest.check_rate_limiting', + side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE")) + + data = { + 'to': send_to, + 'template': str(sample.id) + } + + auth_header = create_authorization_header(service_id=sample.service_id) + + response = client.post( + path='/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + message = json.loads(response.data)['message'] + result = json.loads(response.data)['result'] + assert response.status_code == 429 + assert result == 'error' + assert message == 'Exceeded rate limit for key type TYPE of LIMIT requests per INTERVAL seconds' + + assert not persist_mock.called + assert not deliver_mock.called diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 951a43280..40ce22b0f 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -3,21 +3,21 @@ from freezegun import freeze_time import app from app.notifications.validators import ( - check_service_message_limit, + check_service_over_daily_message_limit, check_template_is_for_notification_type, check_template_is_active, service_can_send_to_recipient, - check_sms_content_char_count -) + check_sms_content_char_count, + check_service_over_api_rate_limit) from app.v2.errors import ( BadRequestError, - TooManyRequestsError -) + TooManyRequestsError, + RateLimitError) from tests.app.conftest import ( sample_notification as create_notification, sample_service as create_service, - sample_service_whitelist -) + sample_service_whitelist, + sample_api_key) @pytest.mark.parametrize('key_type', ['team', 'normal']) @@ -37,7 +37,7 @@ def test_exception_thrown_by_redis_store_get_should_not_be_fatal( create_notification(notify_db, notify_db_session, service=service) with pytest.raises(TooManyRequestsError) as e: - check_service_message_limit(key_type, service) + check_service_over_daily_message_limit(key_type, service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -53,7 +53,7 @@ def test_exception_thown_by_redis_store_set_should_not_be_fatal( mocker): mocker.patch('app.notifications.validators.redis_store.redis_store.set', side_effect=Exception("broken redis")) mocker.patch('app.notifications.validators.redis_store.get', return_value=None) - assert not check_service_message_limit(key_type, sample_service) + assert not check_service_over_daily_message_limit(key_type, sample_service) @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) @@ -65,7 +65,7 @@ def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allow mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') - check_service_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, sample_service) app.notifications.validators.redis_store.set.assert_not_called() assert not app.notifications.validators.services_dao.mock_calls @@ -78,14 +78,14 @@ def test_check_service_message_limit_in_cache_under_message_limit_passes( mocker.patch('app.notifications.validators.redis_store.get', return_value=1) mocker.patch('app.notifications.validators.redis_store.set') mocker.patch('app.notifications.validators.services_dao') - check_service_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, sample_service) app.notifications.validators.redis_store.set.assert_not_called() 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_message_limit('test', sample_service) + check_service_over_daily_message_limit('test', sample_service) assert not app.notifications.validators.redis_store.mock_calls @@ -102,7 +102,7 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set( create_notification(notify_db, notify_db_session, service=sample_service) mocker.patch('app.notifications.validators.redis_store.get', return_value=None) mocker.patch('app.notifications.validators.redis_store.set') - check_service_message_limit(key_type, sample_service) + check_service_over_daily_message_limit(key_type, sample_service) app.notifications.validators.redis_store.set.assert_called_with( str(sample_service.id) + "-2016-01-01-count", 5, ex=3600 ) @@ -118,7 +118,7 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, notify_d for x in range(5): create_notification(notify_db, notify_db_session, service=service) with pytest.raises(TooManyRequestsError) as e: - check_service_message_limit(key_type, service) + check_service_over_daily_message_limit(key_type, service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -140,7 +140,7 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails( service = create_service(notify_db, notify_db_session, restricted=True, limit=4) with pytest.raises(TooManyRequestsError) as e: - check_service_message_limit(key_type, service) + check_service_over_daily_message_limit(key_type, service) assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -262,3 +262,61 @@ def test_check_sms_content_char_count_fails(char_count, notify_api): assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( notify_api.config['SMS_CHAR_COUNT_LIMIT']) assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type, limit, interval', [('team', 1, 2), ('live', 10, 20), ('test', 100, 200)]) +def test_that_when_exceed_rate_limit_request_fails( + notify_db, + notify_db_session, + key_type, + limit, + interval, + mocker): + with freeze_time("2016-01-01 12:00:00.000000"): + + if key_type == 'live': + api_key_type = 'normal' + else: + api_key_type = key_type + + mocker.patch('app.redis_store.exceeded_rate_limit', return_value=True) + mocker.patch('app.notifications.validators.services_dao') + + service = create_service(notify_db, notify_db_session, restricted=True) + api_key = sample_api_key(notify_db, notify_db_session, service=service, key_type=api_key_type) + with pytest.raises(RateLimitError) as e: + check_service_over_api_rate_limit(service, api_key) + + assert app.redis_store.exceeded_rate_limit.called_with( + "{}-{}".format(str(service.id), api_key.key_type), + limit, + interval + ) + assert e.value.status_code == 429 + assert e.value.message == 'Exceeded rate limit for key type {} of {} requests per {} seconds'.format( + key_type.upper(), limit, interval + ) + assert e.value.fields == [] + + +@pytest.mark.parametrize('key_type, limit, interval', [('team', 1, 2), ('normal', 10, 20), ('test', 100, 200)]) +def test_that_when_not_exceeded_rate_limit_request_succeeds( + notify_db, + notify_db_session, + key_type, + limit, + interval, + mocker): + with freeze_time("2016-01-01 12:00:00.000000"): + mocker.patch('app.redis_store.exceeded_rate_limit', return_value=False) + mocker.patch('app.notifications.validators.services_dao') + + service = create_service(notify_db, notify_db_session, restricted=True) + api_key = sample_api_key(notify_db, notify_db_session, service=service, key_type=key_type) + + check_service_over_api_rate_limit(service, api_key) + assert app.redis_store.exceeded_rate_limit.called_with( + "{}-{}".format(str(service.id), api_key.key_type), + limit, + interval + ) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bf15dd8e3..fd6b0cc78 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -2,6 +2,7 @@ import uuid import pytest from flask import json from app.models import Notification +from app.v2.errors import RateLimitError from tests import create_authorization_header from tests.app.conftest import sample_template as create_sample_template @@ -224,3 +225,51 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori assert response.status_code == 201 mocked.assert_called_once_with([notification_id], queue='priority') + + +@pytest.mark.parametrize( + "notification_type, key_send_to, send_to", + [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")] +) +def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( + client, + notify_db, + notify_db_session, + mocker, + notification_type, + key_send_to, + send_to +): + sample = create_sample_template( + notify_db, + notify_db_session, + template_type=notification_type + ) + persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification') + deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue') + mocker.patch( + 'app.v2.notifications.post_notifications.check_rate_limiting', + side_effect=RateLimitError("LIMIT", "INTERVAL", "TYPE")) + + data = { + key_send_to: send_to, + 'template_id': str(sample.id) + } + + auth_header = create_authorization_header(service_id=sample.service_id) + + response = client.post( + path='/v2/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + error = json.loads(response.data)['errors'][0]['error'] + message = json.loads(response.data)['errors'][0]['message'] + status_code = json.loads(response.data)['status_code'] + assert response.status_code == 429 + assert error == 'RateLimitError' + assert message == 'Exceeded rate limit for key type TYPE of LIMIT requests per INTERVAL seconds' + assert status_code == 429 + + assert not persist_mock.called + assert not deliver_mock.called