Merge pull request #1423 from alphagov/fix-firetext-inbound-500s

fix 500s when inbound msgs sent from alphanumerics rather than normal…

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,

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}
     }
 }

app/inbound_sms/rest.py

@@ -1,11 +1,10 @@
 from flask import (
     Blueprint,
     jsonify,
-    request,
+    request
-    current_app, json)
+)
-from jsonschema import ValidationError
 
-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'])
+@inbound_sms.route('', methods=['POST'])
-def get_inbound_sms_for_service(service_id):
+def post_query_inbound_sms_for_service(service_id):
-
+    form = validate(request.get_json(), get_inbound_sms_for_service_schema)
-    if request.method == 'GET':
+    if 'phone_number' in form:
-        limit = request.args.get('limit')
+        # we use this to normalise to an international phone number - but this may fail if it's an alphanumeric
-        user_number = request.args.get('user_number')
+        user_number = try_validate_and_format_phone_number(form['phone_number'], international=True)
-
-        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])
     else:
-        form = validate(request.get_json(), get_inbound_sms_for_service_schema)
+        user_number = None
-        results = dao_get_inbound_sms_for_service(service_id, form.get('limit'), form.get('phone_number'))
+    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')

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:

requirements.txt

@@ -25,6 +25,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

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()
+    sms = admin_request.post(
-
+        'inbound_sms.post_query_inbound_sms_for_service',
-    data = {}
+        service_id=sample_service.id,
-
+        _data={}
-    response = client.post(
+    )['data']
-        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']
 
     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(
+    sms = admin_request.post(
-        path='/service/{}/inbound-sms'.format(sample_service.id),
+        'inbound_sms.post_query_inbound_sms_for_service',
-        data=json.dumps(data),
+        service_id=sample_service.id,
-        headers=[('Content-Type', 'application/json'), auth_header])
+        _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(two.id)
 
 
-def test_get_inbound_sms_should_error_with_invalid_limit(client, sample_service):
+def test_post_to_get_inbound_sms_should_error_with_invalid_limit(admin_request, sample_service):
-
-    auth_header = create_authorization_header()
-
     data = {'limit': 'limit'}
 
-    response = client.post(
+    error_resp = admin_request.post(
-        path='/service/{}/inbound-sms'.format(sample_service.id),
+        'inbound_sms.post_query_inbound_sms_for_service',
-        data=json.dumps(data),
+        service_id=sample_service.id,
-        headers=[('Content-Type', 'application/json'), auth_header])
+        _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(
+    sms = admin_request.post(
-        path='/service/{}/inbound-sms'.format(sample_service.id),
+        'inbound_sms.post_query_inbound_sms_for_service',
-        data=json.dumps(data),
+        service_id=sample_service.id,
-        headers=[('Content-Type', 'application/json'), auth_header])
+        _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(
+    sms = admin_request.post(
-        path='/service/{}/inbound-sms'.format(sample_service.id),
+        'inbound_sms.post_query_inbound_sms_for_service',
-        data=json.dumps(data),
+        service_id=sample_service.id,
-        headers=[('Content-Type', 'application/json'), auth_header])
+        _data=data
+    )['data']
 
-    json_resp = json.loads(response.get_data(as_text=True))
+    assert len(sms) == 1
-    sms = json_resp['data']
+    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
 ##############################

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'