From f3d129b0d89f659e5a761ed5d90fe8f08b5d9039 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 23 Nov 2017 15:22:18 +0000 Subject: [PATCH 1/2] fix 500s when inbound msgs sent from alphanumerics rather than normal phone numbers --- app/celery/tasks.py | 1 + app/notifications/receive_notifications.py | 8 +++++-- requirements.txt | 2 +- .../test_receive_notification.py | 21 +++++++++++++++++++ 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 436def14d..e96d7e3d8 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -486,6 +486,7 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id): inbound_id=inbound_sms_id) data = { "id": str(inbound_sms.id), + # TODO: should we be validating and formatting the phone number here? "source_number": inbound_sms.user_number, "destination_number": inbound_sms.notify_number, "message": inbound_sms.content, diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 3167134e3..22a7fe61e 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -2,7 +2,7 @@ from urllib.parse import unquote import iso8601 from flask import jsonify, Blueprint, current_app, request, abort -from notifications_utils.recipients import validate_and_format_phone_number +from notifications_utils.recipients import try_validate_and_format_phone_number from app import statsd_client, firetext_client, mmg_client from app.celery import tasks @@ -109,7 +109,11 @@ def format_mmg_datetime(date): def create_inbound_sms_object(service, content, from_number, provider_ref, date_received, provider_name): - user_number = validate_and_format_phone_number(from_number, international=True) + user_number = try_validate_and_format_phone_number( + from_number, + international=True, + log_msg='Invalid from_number received' + ) provider_date = date_received if provider_date: diff --git a/requirements.txt b/requirements.txt index 8d69902c5..8b5cb9f35 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,6 +26,6 @@ notifications-python-client==4.6.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.0.1#egg=notifications-utils==23.0.1 +git+https://github.com/alphagov/notifications-utils.git@23.1.0#egg=notifications-utils==23.1.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 7f784135c..fbd190a50 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -413,3 +413,24 @@ def test_firetext_inbound_sms_auth(notify_db_session, notify_api, client, mocker with set_config(notify_api, 'FIRETEXT_INBOUND_SMS_AUTH', keys): response = firetext_post(client, data, auth=bool(auth), password=auth) assert response.status_code == status_code + + +def test_create_inbound_sms_object_works_with_alphanumeric_sender(sample_service_full_permissions): + data = { + 'Message': 'hello', + 'Number': sample_service_full_permissions.get_inbound_number(), + 'MSISDN': 'ALPHANUM3R1C', + 'DateRecieved': '2017-01-02+03%3A04%3A05', + 'ID': 'bar', + } + + inbound_sms = create_inbound_sms_object( + service=sample_service_full_permissions, + content=format_mmg_message(data["Message"]), + from_number='ALPHANUM3R1C', + provider_ref='foo', + date_received=None, + provider_name="mmg" + ) + + assert inbound_sms.user_number == 'ALPHANUM3R1C' From 3daf039fbe58e8f517537ce49534e23cc10e6bc5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 23 Nov 2017 16:46:39 +0000 Subject: [PATCH 2/2] get_inbound_sms queries from the admin should also allow alphanumerics. Refactored the call to make the POST and the GET versions of the method much more distinct. --- app/inbound_sms/inbound_sms_schemas.py | 2 +- app/inbound_sms/rest.py | 47 +++++---- tests/app/inbound_sms/test_rest.py | 136 +++++++++++-------------- 3 files changed, 89 insertions(+), 96 deletions(-) diff --git a/app/inbound_sms/inbound_sms_schemas.py b/app/inbound_sms/inbound_sms_schemas.py index c0e644d68..24e642f49 100644 --- a/app/inbound_sms/inbound_sms_schemas.py +++ b/app/inbound_sms/inbound_sms_schemas.py @@ -3,7 +3,7 @@ get_inbound_sms_for_service_schema = { "description": "schema for parameters allowed when searching for to field=", "type": "object", "properties": { - "phone_number": {"type": "string", "format": "phone_number"}, + "phone_number": {"type": "string"}, "limit": {"type": ["integer", "null"], "minimum": 1} } } diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index a78e3c237..3e67e4ee1 100644 --- a/app/inbound_sms/rest.py +++ b/app/inbound_sms/rest.py @@ -1,11 +1,10 @@ from flask import ( Blueprint, jsonify, - request, - current_app, json) -from jsonschema import ValidationError + request +) -from notifications_utils.recipients import validate_and_format_phone_number +from notifications_utils.recipients import try_validate_and_format_phone_number from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_for_service, @@ -26,25 +25,31 @@ inbound_sms = Blueprint( register_errors(inbound_sms) -@inbound_sms.route('', methods=['POST', 'GET']) -def get_inbound_sms_for_service(service_id): - - if request.method == 'GET': - limit = request.args.get('limit') - user_number = request.args.get('user_number') - - if user_number: - # we use this to normalise to an international phone number - user_number = validate_and_format_phone_number(user_number, international=True) - - results = dao_get_inbound_sms_for_service(service_id, limit, user_number) - - return jsonify(data=[row.serialize() for row in results]) +@inbound_sms.route('', methods=['POST']) +def post_query_inbound_sms_for_service(service_id): + form = validate(request.get_json(), get_inbound_sms_for_service_schema) + if 'phone_number' in form: + # we use this to normalise to an international phone number - but this may fail if it's an alphanumeric + user_number = try_validate_and_format_phone_number(form['phone_number'], international=True) else: - form = validate(request.get_json(), get_inbound_sms_for_service_schema) - results = dao_get_inbound_sms_for_service(service_id, form.get('limit'), form.get('phone_number')) + user_number = None + results = dao_get_inbound_sms_for_service(service_id, form.get('limit'), user_number) - return jsonify(data=[row.serialize() for row in results]) + return jsonify(data=[row.serialize() for row in results]) + + +@inbound_sms.route('', methods=['GET']) +def get_inbound_sms_for_service(service_id): + limit = request.args.get('limit') + user_number = request.args.get('user_number') + + if user_number: + # we use this to normalise to an international phone number - but this may fail if it's an alphanumeric + user_number = try_validate_and_format_phone_number(user_number, international=True) + + results = dao_get_inbound_sms_for_service(service_id, limit, user_number) + + return jsonify(data=[row.serialize() for row in results]) @inbound_sms.route('/summary') diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index f278251be..a0a6d612b 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -1,28 +1,20 @@ from datetime import datetime import pytest -from flask import json from freezegun import freeze_time -from tests import create_authorization_header from tests.app.db import create_inbound_sms, create_service, create_service_with_inbound_number -def test_get_inbound_sms_with_no_params(client, sample_service): +def test_post_to_get_inbound_sms_with_no_params(admin_request, sample_service): one = create_inbound_sms(sample_service) two = create_inbound_sms(sample_service) - auth_header = create_authorization_header() - - data = {} - - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - sms = json_resp['data'] + sms = admin_request.post( + 'inbound_sms.post_query_inbound_sms_for_service', + service_id=sample_service.id, + _data={} + )['data'] assert len(sms) == 2 assert {inbound['id'] for inbound in sms} == {str(one.id), str(two.id)} @@ -37,40 +29,34 @@ def test_get_inbound_sms_with_no_params(client, sample_service): } -def test_get_inbound_sms_with_limit(client, sample_service): +def test_post_to_get_inbound_sms_with_limit(admin_request, sample_service): with freeze_time('2017-01-01'): one = create_inbound_sms(sample_service) with freeze_time('2017-01-02'): two = create_inbound_sms(sample_service) - auth_header = create_authorization_header() - data = {'limit': 1} - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - sms = json_resp['data'] + sms = admin_request.post( + 'inbound_sms.post_query_inbound_sms_for_service', + service_id=sample_service.id, + _data=data + )['data'] assert len(sms) == 1 assert sms[0]['id'] == str(two.id) -def test_get_inbound_sms_should_error_with_invalid_limit(client, sample_service): - - auth_header = create_authorization_header() - +def test_post_to_get_inbound_sms_should_error_with_invalid_limit(admin_request, sample_service): data = {'limit': 'limit'} - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + error_resp = admin_request.post( + 'inbound_sms.post_query_inbound_sms_for_service', + service_id=sample_service.id, + _data=data, + _expected_status=400 + ) - error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 400 assert error_resp['errors'] == [{ 'error': 'ValidationError', @@ -78,73 +64,61 @@ def test_get_inbound_sms_should_error_with_invalid_limit(client, sample_service) }] -def test_get_inbound_sms_should_error_with_invalid_phone_number(client, sample_service): - - auth_header = create_authorization_header() - - data = {'phone_number': 'invalid phone number'} - - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - error_resp = json.loads(response.get_data(as_text=True)) - assert error_resp['status_code'] == 400 - assert error_resp['errors'] == [{ - 'error': 'ValidationError', - 'message': "phone_number Must not contain letters or symbols" - }] - - @pytest.mark.parametrize('user_number', [ '(07700) 900-001', '+4407700900001', '447700900001', ]) -def test_get_inbound_sms_filters_user_number(client, sample_service, user_number): +def test_post_to_get_inbound_sms_filters_user_number(admin_request, sample_service, user_number): # user_number in the db is international and normalised one = create_inbound_sms(sample_service, user_number='447700900001') two = create_inbound_sms(sample_service, user_number='447700900002') - auth_header = create_authorization_header() - data = { 'limit': 1, 'phone_number': user_number } - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + sms = admin_request.post( + 'inbound_sms.post_query_inbound_sms_for_service', + service_id=sample_service.id, + _data=data + )['data'] - json_resp = json.loads(response.get_data(as_text=True)) - sms = json_resp['data'] assert len(sms) == 1 assert sms[0]['id'] == str(one.id) assert sms[0]['user_number'] == str(one.user_number) -def test_get_inbound_sms_filters_international_user_number(admin_request, sample_service): +def test_post_to_get_inbound_sms_filters_international_user_number(admin_request, sample_service): # user_number in the db is international and normalised one = create_inbound_sms(sample_service, user_number='12025550104') two = create_inbound_sms(sample_service) - auth_header = create_authorization_header() - data = { 'limit': 1, 'phone_number': '+1 (202) 555-0104' } - response = client.post( - path='/service/{}/inbound-sms'.format(sample_service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + sms = admin_request.post( + 'inbound_sms.post_query_inbound_sms_for_service', + service_id=sample_service.id, + _data=data + )['data'] - json_resp = json.loads(response.get_data(as_text=True)) - sms = json_resp['data'] + assert len(sms) == 1 + assert sms[0]['id'] == str(one.id) + assert sms[0]['user_number'] == str(one.user_number) + + +def test_post_to_get_inbound_sms_allows_badly_formatted_number(admin_request, sample_service): + one = create_inbound_sms(sample_service, user_number='ALPHANUM3R1C') + + sms = admin_request.post( + 'inbound_sms.get_inbound_sms_for_service', + service_id=sample_service.id, + _data={'phone_number': 'ALPHANUM3R1C'} + )['data'] assert len(sms) == 1 assert sms[0]['id'] == str(one.id) @@ -156,7 +130,7 @@ def test_get_inbound_sms_filters_international_user_number(admin_request, sample ############################################################## -def test_get_inbound_sms(admin_request, sample_service): +def test_old_get_inbound_sms(admin_request, sample_service): one = create_inbound_sms(sample_service) two = create_inbound_sms(sample_service) @@ -180,7 +154,7 @@ def test_get_inbound_sms(admin_request, sample_service): } -def test_get_inbound_sms_limits(admin_request, sample_service): +def test_old_get_inbound_sms_limits(admin_request, sample_service): with freeze_time('2017-01-01'): one = create_inbound_sms(sample_service) with freeze_time('2017-01-02'): @@ -201,7 +175,7 @@ def test_get_inbound_sms_limits(admin_request, sample_service): '+4407700900001', '447700900001', ]) -def test_get_inbound_sms_filters_user_number(admin_request, sample_service, user_number): +def test_old_get_inbound_sms_filters_user_number(admin_request, sample_service, user_number): # user_number in the db is international and normalised one = create_inbound_sms(sample_service, user_number='447700900001') two = create_inbound_sms(sample_service, user_number='447700900002') @@ -217,7 +191,7 @@ def test_get_inbound_sms_filters_user_number(admin_request, sample_service, user assert sms['data'][0]['user_number'] == str(one.user_number) -def test_get_inbound_sms_filters_international_user_number(admin_request, sample_service): +def test_old_get_inbound_sms_filters_international_user_number(admin_request, sample_service): # user_number in the db is international and normalised one = create_inbound_sms(sample_service, user_number='12025550104') two = create_inbound_sms(sample_service) @@ -233,6 +207,20 @@ def test_get_inbound_sms_filters_international_user_number(admin_request, sample assert sms['data'][0]['user_number'] == str(one.user_number) +def test_old_get_inbound_sms_allows_badly_formatted_number(admin_request, sample_service): + one = create_inbound_sms(sample_service, user_number='ALPHANUM3R1C') + + sms = admin_request.get( + 'inbound_sms.get_inbound_sms_for_service', + service_id=sample_service.id, + user_number='ALPHANUM3R1C', + ) + + assert len(sms['data']) == 1 + assert sms['data'][0]['id'] == str(one.id) + assert sms['data'][0]['user_number'] == str(one.user_number) + + ############################## # End delete section ##############################