From e569c54f45a6f0a13a78a9f976811a25119eeaaa Mon Sep 17 00:00:00 2001 From: Jenny Duckett Date: Fri, 9 Dec 2016 14:55:24 +0000 Subject: [PATCH] Send Notify's 2FA codes via only the `notify` queue This means that these codes won't be delayed by large jobs going through the send-sms/email queues. send_user_sms_code now works much more like the endpoints for sending notifications, by persisting the notification and only using the deliver_sms task (instead of using send_sms as well). The workers consuming the `notify` queue should be able to handle the deliver task as well, so no change should be needed to the celery workers to support this. I think there's also a change in behaviour here: previously, if the Notify service was in research mode, 2FA codes would not have been sent out, making it impossible to log into the admin. Now, a call to this endpoint will always send out the notification even if we've put the Notify service into research mode, since we set the notification's key type to normal and ignore the service's research mode setting when sending the notification to the queue. --- app/user/rest.py | 34 +++++++++------ tests/app/user/test_rest_verify.py | 70 +++++++++++++++--------------- 2 files changed, 55 insertions(+), 49 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index a188f162e..1844c7dc6 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -18,7 +18,11 @@ from app.dao.users_dao import ( from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id -from app.models import SMS_TYPE +from app.models import SMS_TYPE, KEY_TYPE_NORMAL +from app.notifications.process_notifications import ( + persist_notification, + send_notification_to_queue +) from app.schemas import ( email_data_request_schema, user_schema, @@ -143,20 +147,22 @@ def send_user_sms_code(user_id): mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') sms_code_template_id = current_app.config['SMS_CODE_TEMPLATE_ID'] sms_code_template = dao_get_template_by_id(sms_code_template_id) - verification_message = encryption.encrypt({ - 'template': sms_code_template_id, - 'template_version': sms_code_template.version, - 'to': mobile, - 'personalisation': { - 'verify_code': secret_code - } + notify_service_id = current_app.config['NOTIFY_SERVICE_ID'] - }) - send_sms.apply_async([current_app.config['NOTIFY_SERVICE_ID'], - str(uuid.uuid4()), - verification_message, - datetime.utcnow().strftime(DATETIME_FORMAT) - ], queue='notify') + saved_notification = persist_notification( + template_id=sms_code_template_id, + template_version=sms_code_template.version, + recipient=mobile, + service_id=notify_service_id, + personalisation={'verify_code': secret_code}, + notification_type=SMS_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL + ) + # Assume that we never want to observe the Notify service's research mode + # setting for this notification - we still need to be able to log into the + # admin even if we're doing user research using this service: + send_notification_to_queue(saved_notification, False, queue='notify') return jsonify({}), 204 diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 006247292..b948bf76e 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -1,5 +1,6 @@ import json import moto +import pytest from datetime import ( datetime, @@ -7,9 +8,11 @@ from datetime import ( ) from flask import url_for, current_app +from app.dao.services_dao import dao_update_service, dao_fetch_service_by_id from app.models import ( VerifyCode, - User + User, + Notification ) from app import db, encryption @@ -214,41 +217,47 @@ def test_user_verify_password_missing_password(notify_api, assert 'Required field missing data' in json_resp['message']['password'] +@pytest.mark.parametrize('research_mode', [True, False]) @freeze_time("2016-01-01 11:09:00.061258") def test_send_user_sms_code(notify_api, sample_user, sms_code_template, - mocker): + mocker, + research_mode): """ Tests POST endpoint /user//sms-code """ with notify_api.test_request_context(): with notify_api.test_client() as client: + if research_mode: + notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID']) + notify_service.research_mode = True + dao_update_service(notify_service) + data = json.dumps({}) auth_header = create_authorization_header() mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + resp = client.post( url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 + assert mocked.call_count == 1 - encrypted = encryption.encrypt({ - 'template': current_app.config['SMS_CODE_TEMPLATE_ID'], - 'template_version': 1, - 'to': sample_user.mobile_number, - 'personalisation': { - 'verify_code': '11111' - } - }) - app.celery.tasks.send_sms.apply_async.assert_called_once_with( - ([current_app.config['NOTIFY_SERVICE_ID'], - "some_uuid", - encrypted, - "2016-01-01T11:09:00.061258Z"]), + assert VerifyCode.query.count() == 1 + assert VerifyCode.query.first().check_code('11111') + + assert Notification.query.count() == 1 + notification = Notification.query.first() + assert notification.personalisation == {'verify_code': '11111'} + assert notification.to == sample_user.mobile_number + assert str(notification.service_id) == current_app.config['NOTIFY_SERVICE_ID'] + + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( + ([str(notification.id)]), queue="notify" ) @@ -257,38 +266,29 @@ def test_send_user_sms_code(notify_api, def test_send_user_code_for_sms_with_optional_to_field(notify_api, sample_user, sms_code_template, - mock_encryption, mocker): """ - Tests POST endpoint '//code' successful sms with optional to field + Tests POST endpoint /user//sms-code with optional to field """ with notify_api.test_request_context(): with notify_api.test_client() as client: + to_number = '+441119876757' mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id - mocker.patch('app.celery.tasks.send_sms.apply_async') - data = json.dumps({'to': '+441119876757'}) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = json.dumps({'to': to_number}) auth_header = create_authorization_header() + resp = client.post( url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - encrypted = encryption.encrypt({ - 'template': current_app.config['SMS_CODE_TEMPLATE_ID'], - 'template_version': 1, - 'to': '+441119876757', - 'personalisation': { - 'verify_code': '11111' - } - }) assert mocked.call_count == 1 - app.celery.tasks.send_sms.apply_async.assert_called_once_with( - ([current_app.config['NOTIFY_SERVICE_ID'], - "some_uuid", - encrypted, - "2016-01-01T11:09:00.061258Z"]), + notification = Notification.query.first() + assert notification.to == to_number + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( + ([str(notification.id)]), queue="notify" )