diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a042d7f0a..23101dccd 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -63,6 +63,7 @@ from app.models import ( SMS_TYPE, ) from app.notifications.process_notifications import persist_notification +from app.notifications.notifications_ses_callback import process_ses_response from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd from notifications_utils.s3 import s3upload @@ -547,3 +548,12 @@ def process_incomplete_job(job_id): process_row(row_number, recipient, personalisation, template, job, job.service) job_complete(job, job.service, template, resumed=True) + + +@notify_celery.task(bind=True, name="process-ses-result", max_retries=5, default_retry_delay=300) +@statsd(namespace="tasks") +def process_ses_results(self, response): + errors = process_ses_response(response) + if errors: + current_app.logger.error(errors) + self.retry(queue=QueueNames.RETRY, exc="SES responses processed with error") diff --git a/app/notifications/notifications_ses_callback.py b/app/notifications/notifications_ses_callback.py index b1d0ba589..2ed34037b 100644 --- a/app/notifications/notifications_ses_callback.py +++ b/app/notifications/notifications_ses_callback.py @@ -15,7 +15,6 @@ from app.dao import ( ) from app.celery.statistics_tasks import create_outcome_notification_statistic_tasks from app.notifications.process_client_response import validate_callback_data -from app.notifications.utils import autoconfirm_subscription ses_callback_blueprint = Blueprint('notifications_ses_callback', __name__) @@ -27,26 +26,33 @@ register_errors(ses_callback_blueprint) @ses_callback_blueprint.route('/notifications/email/ses', methods=['POST']) -def process_ses_response(): +def sns_callback_handler(): + errors = process_ses_response(json.loads(request.data)) + if errors: + current_app.logger.error(errors) + raise InvalidRequest(errors, 400) + + return jsonify( + result="success", message="SES callback succeeded" + ), 200 + + +def process_ses_response(ses_request): client_name = 'SES' try: - ses_request = json.loads(request.data) - subscribed_topic = autoconfirm_subscription(ses_request) - if subscribed_topic: - current_app.logger.info("Automatically subscribed to topic: {}".format(subscribed_topic)) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + # TODO: remove this check once the sns_callback_handler is removed + if not isinstance(ses_request, dict): + ses_request = json.loads(ses_request) errors = validate_callback_data(data=ses_request, fields=['Message'], client_name=client_name) if errors: - raise InvalidRequest(errors, status_code=400) + return errors ses_message = json.loads(ses_request['Message']) errors = validate_callback_data(data=ses_message, fields=['notificationType'], client_name=client_name) if errors: - raise InvalidRequest(errors, status_code=400) + return errors notification_type = ses_message['notificationType'] if notification_type == 'Bounce': @@ -58,7 +64,7 @@ def process_ses_response(): aws_response_dict = get_aws_responses(notification_type) except KeyError: error = "{} callback failed: status {} not found".format(client_name, notification_type) - raise InvalidRequest(error, status_code=400) + return error notification_status = aws_response_dict['notification_status'] @@ -71,7 +77,7 @@ def process_ses_response(): if not notification: error = "SES callback failed: notification either not found or already updated " \ "from sending. Status {} for notification reference {}".format(notification_status, reference) - raise InvalidRequest(error, status_code=404) + return error if not aws_response_dict['success']: current_app.logger.info( @@ -96,14 +102,12 @@ def process_ses_response(): create_outcome_notification_statistic_tasks(notification) - return jsonify( - result="success", message="SES callback succeeded" - ), 200 + return except KeyError: - message = "SES callback failed: messageId missing" - raise InvalidRequest(message, status_code=400) + error = "SES callback failed: messageId missing" + return error - except ValueError as ex: + except ValueError: error = "{} callback failed: invalid json".format(client_name) - raise InvalidRequest(error, status_code=400) + return error diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e018a4f32..39bce8e69 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,3 +1,4 @@ +import codecs import json import uuid from datetime import datetime, timedelta @@ -26,8 +27,8 @@ from app.celery.tasks import ( process_incomplete_jobs, get_template_class, s3, - send_inbound_sms_to_service -) + send_inbound_sms_to_service, + process_ses_results) from app.config import QueueNames from app.dao import jobs_dao, services_dao from app.models import ( @@ -1381,3 +1382,50 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template): assert completed_job.job_status == JOB_STATUS_FINISHED assert mock_letter_saver.call_count == 8 + + +def test_process_ses_results(notify_db, notify_db_session, sample_email_template): + + create_sample_notification( + notify_db, + notify_db_session, + template=sample_email_template, + reference='ref1', + sent_at=datetime.utcnow(), + status='sending') + + response = json.loads(ses_notification_callback()) + assert process_ses_results(response=response) is None + + +def test_process_ses_results_retry_called(notify_db, mocker): + mocked = mocker.patch('app.celery.tasks.process_ses_results.retry') + response = json.loads(ses_notification_callback()) + process_ses_results(response=response) + assert mocked.call_count != 0 + + +def ses_notification_callback(): + return '{\n "Type" : "Notification",\n "MessageId" : "ref1",' \ + '\n "TopicArn" : "arn:aws:sns:eu-west-1:123456789012:testing",' \ + '\n "Message" : "{\\"notificationType\\":\\"Delivery\\",' \ + '\\"mail\\":{\\"timestamp\\":\\"2016-03-14T12:35:25.909Z\\",' \ + '\\"source\\":\\"test@test-domain.com\\",' \ + '\\"sourceArn\\":\\"arn:aws:ses:eu-west-1:123456789012:identity/testing-notify\\",' \ + '\\"sendingAccountId\\":\\"123456789012\\",' \ + '\\"messageId\\":\\"ref1\\",' \ + '\\"destination\\":[\\"testing@digital.cabinet-office.gov.uk\\"]},' \ + '\\"delivery\\":{\\"timestamp\\":\\"2016-03-14T12:35:26.567Z\\",' \ + '\\"processingTimeMillis\\":658,' \ + '\\"recipients\\":[\\"testing@digital.cabinet-office.gov.uk\\"],' \ + '\\"smtpResponse\\":\\"250 2.0.0 OK 1457958926 uo5si26480932wjc.221 - gsmtp\\",' \ + '\\"reportingMTA\\":\\"a6-238.smtp-out.eu-west-1.amazonses.com\\"}}",' \ + '\n "Timestamp" : "2016-03-14T12:35:26.665Z",\n "SignatureVersion" : "1",' \ + '\n "Signature" : "X8d7eTAOZ6wlnrdVVPYanrAlsX0SMPfOzhoTEBnQqYkrNWTqQY91C0f3bxtPdUhUt' \ + 'OowyPAOkTQ4KnZuzphfhVb2p1MyVYMxNKcBFB05/qaCX99+92fjw4x9LeUOwyGwMv5F0Vkfi5qZCcEw69uVrhYL' \ + 'VSTFTrzi/yCtru+yFULMQ6UhbY09GwiP6hjxZMVr8aROQy5lLHglqQzOuSZ4KeD85JjifHdKzlx8jjQ+uj+FLzHXPMA' \ + 'PmPU1JK9kpoHZ1oPshAFgPDpphJe+HwcJ8ezmk+3AEUr3wWli3xF+49y8Z2anASSVp6YI2YP95UT8Rlh3qT3T+V9V8rbSVislxA==",' \ + '\n "SigningCertURL" : "https://sns.eu-west-1.amazonaws.com/SimpleNotificationService-bb750' \ + 'dd426d95ee9390147a5624348ee.pem",' \ + '\n "UnsubscribeURL" : "https://sns.eu-west-1.amazonaws.com/?Action=Unsubscribe&S' \ + 'ubscriptionArn=arn:aws:sns:eu-west-1:302763885840:preview-emails:d6aad3ef-83d6-4cf3-a470-54e2e75916da"\n}' diff --git a/tests/app/notifications/test_notifications_ses_callback.py b/tests/app/notifications/test_notifications_ses_callback.py index 0a02b5d58..cfcb4a4b8 100644 --- a/tests/app/notifications/test_notifications_ses_callback.py +++ b/tests/app/notifications/test_notifications_ses_callback.py @@ -3,11 +3,10 @@ from unittest.mock import call from flask import json from freezegun import freeze_time -from requests import HTTPError -import pytest from app import statsd_client from app.dao.notifications_dao import get_notification_by_id +from app.notifications.notifications_ses_callback import process_ses_response from tests.app.conftest import sample_notification as create_sample_notification @@ -17,75 +16,15 @@ def test_ses_callback_should_not_need_auth(client): data=ses_notification_callback(), headers=[('Content-Type', 'text/plain; charset=UTF-8')] ) - assert response.status_code == 404 + assert response.status_code == 400 def test_ses_callback_should_fail_if_invalid_json(client, mocker): stats_mock = mocker.patch( 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - - response = client.post( - path='/notifications/email/ses', - data="nonsense", - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: invalid json' - stats_mock.assert_not_called() - - -def test_ses_callback_should_autoconfirm_subscriptions(client, rmock, mocker): - stats_mock = mocker.patch( - 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' - ) - - endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] - rmock.request( - "GET", - endpoint, - json={"status": "success"}, - status_code=200) - - response = client.post( - path='/notifications/email/ses', - data=ses_confirmation_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - - assert rmock.called - assert rmock.request_history[0].url == endpoint - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' - stats_mock.assert_not_called() - - -def test_ses_callback_autoconfirm_raises_exception_if_not_200(client, rmock, mocker): - stats_mock = mocker.patch( - 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' - ) - - endpoint = json.loads(ses_confirmation_callback())['SubscribeURL'] - rmock.request( - "GET", - endpoint, - json={"status": "not allowed"}, - status_code=405) - - with pytest.raises(HTTPError) as exc: - client.post( - path='/notifications/email/ses', - data=ses_confirmation_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - - assert rmock.called - assert rmock.request_history[0].url == endpoint - assert exc.value.response.status_code == 405 + errors = process_ses_response('nonsense') + assert errors == 'SES callback failed: invalid json' stats_mock.assert_not_called() @@ -94,15 +33,8 @@ def test_ses_callback_should_fail_if_invalid_notification_type(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_invalid_notification_type_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: status Unknown not found' + errors = process_ses_response(json.loads(ses_invalid_notification_type_callback())) + assert errors == 'SES callback failed: status Unknown not found' stats_mock.assert_not_called() @@ -111,15 +43,8 @@ def test_ses_callback_should_fail_if_missing_message_id(client, mocker): 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_missing_notification_id_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: messageId missing' + errors = process_ses_response(json.loads(ses_missing_notification_id_callback())) + assert errors == 'SES callback failed: messageId missing' stats_mock.assert_not_called() @@ -128,15 +53,8 @@ def test_ses_callback_should_fail_if_notification_cannot_be_found(notify_db, not 'app.notifications.notifications_ses_callback.create_outcome_notification_statistic_tasks' ) - response = client.post( - path='/notifications/email/ses', - data=ses_invalid_notification_id_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: notification either not found or already updated from sending. Status delivered for notification reference missing' # noqa + errors = process_ses_response(json.loads(ses_invalid_notification_id_callback())) + assert errors == 'SES callback failed: notification either not found or already updated from sending. Status delivered for notification reference missing' # noqa stats_mock.assert_not_called() @@ -164,15 +82,8 @@ def test_ses_callback_should_update_notification_status( assert get_notification_by_id(notification.id).status == 'sending' - response = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + errors = process_ses_response(json.loads(ses_notification_callback())) + assert errors is None assert get_notification_by_id(notification.id).status == 'delivered' statsd_client.timing_with_dates.assert_any_call( "callback.ses.elapsed-time", datetime.utcnow(), notification.sent_at @@ -216,25 +127,10 @@ def test_ses_callback_should_update_multiple_notification_status_sent( sent_at=datetime.utcnow(), status='sending') - resp1 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref1'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - resp2 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref2'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - resp3 = client.post( - path='/notifications/email/ses', - data=ses_notification_callback(ref='ref3'), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) + assert process_ses_response(json.loads(ses_notification_callback(ref='ref1'))) is None + assert process_ses_response(json.loads(ses_notification_callback(ref='ref2'))) is None + assert process_ses_response(json.loads(ses_notification_callback(ref='ref3'))) is None - assert resp1.status_code == 200 - assert resp2.status_code == 200 - assert resp3.status_code == 200 stats_mock.assert_has_calls([ call(notification1), call(notification2), @@ -261,16 +157,7 @@ def test_ses_callback_should_set_status_to_temporary_failure(client, sent_at=datetime.utcnow() ) assert get_notification_by_id(notification.id).status == 'sending' - - response = client.post( - path='/notifications/email/ses', - data=ses_soft_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + assert process_ses_response(json.loads(ses_soft_bounce_callback())) is None assert get_notification_by_id(notification.id).status == 'temporary-failure' stats_mock.assert_called_once_with(notification) @@ -294,16 +181,8 @@ def test_ses_callback_should_not_set_status_once_status_is_delivered(client, ) assert get_notification_by_id(notification.id).status == 'delivered' - - response = client.post( - path='/notifications/email/ses', - data=ses_soft_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'SES callback failed: notification either not found or already updated from sending. Status temporary-failure for notification reference ref' # noqa + error = process_ses_response(json.loads(ses_soft_bounce_callback())) + assert error == 'SES callback failed: notification either not found or already updated from sending. Status temporary-failure for notification reference ref' # noqa assert get_notification_by_id(notification.id).status == 'delivered' stats_mock.assert_not_called() @@ -327,16 +206,7 @@ def test_ses_callback_should_set_status_to_permanent_failure(client, ) assert get_notification_by_id(notification.id).status == 'sending' - - response = client.post( - path='/notifications/email/ses', - data=ses_hard_bounce_callback(), - headers=[('Content-Type', 'text/plain; charset=UTF-8')] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert json_resp['result'] == 'success' - assert json_resp['message'] == 'SES callback succeeded' + assert process_ses_response(json.loads(ses_hard_bounce_callback())) is None assert get_notification_by_id(notification.id).status == 'permanent-failure' stats_mock.assert_called_once_with(notification)