From a8d3b0952feacc28e78428ef8ad97021c7643c88 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 14 Dec 2017 13:35:13 +0000 Subject: [PATCH 1/3] Add MMG_INBOUND_SMS_AUTH config variable and auth check Checks authentication header value on inbound SMS requests from MMG against a list of allowed API keys set in the application config. At the moment, we're only logging the attempts without aborting the requests. Once this is rolled out to production and we've checked the logs we'll switch on the aborts and add the tests for 401 and 403 responses. This work has already been done for Firetext in a previous PR: https://github.com/alphagov/notifications-api/pull/1409 --- app/cloudfoundry_config.py | 1 + app/config.py | 2 + app/notifications/receive_notifications.py | 20 ++++++-- .../test_receive_notification.py | 46 ++++++++++++++++--- tests/app/test_cloudfoundry_config.py | 8 ++++ 5 files changed, 67 insertions(+), 10 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 45f4eeee7..79af3317a 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -48,6 +48,7 @@ def extract_notify_config(notify_config): os.environ['DANGEROUS_SALT'] = notify_config['credentials']['dangerous_salt'] os.environ['SMS_INBOUND_WHITELIST'] = json.dumps(notify_config['credentials']['allow_ip_inbound_sms']) os.environ['FIRETEXT_INBOUND_SMS_AUTH'] = json.dumps(notify_config['credentials']['firetext_inbound_sms_auth']) + os.environ['MMG_INBOUND_SMS_AUTH'] = json.dumps(notify_config['credentials']['mmg_inbound_sms_auth']) os.environ['ROUTE_SECRET_KEY_1'] = notify_config['credentials']['route_secret_key_1'] os.environ['ROUTE_SECRET_KEY_2'] = notify_config['credentials']['route_secret_key_2'] diff --git a/app/config.py b/app/config.py index f2d37a769..e34f565a5 100644 --- a/app/config.py +++ b/app/config.py @@ -300,6 +300,7 @@ class Config(object): SMS_INBOUND_WHITELIST = json.loads(os.environ.get('SMS_INBOUND_WHITELIST', '[]')) FIRETEXT_INBOUND_SMS_AUTH = json.loads(os.environ.get('FIRETEXT_INBOUND_SMS_AUTH', '[]')) + MMG_INBOUND_SMS_AUTH = json.loads(os.environ.get('MMG_INBOUND_SMS_AUTH', '[]')) ROUTE_SECRET_KEY_1 = os.environ.get('ROUTE_SECRET_KEY_1', '') ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '') @@ -376,6 +377,7 @@ class Test(Config): SMS_INBOUND_WHITELIST = ['203.0.113.195'] FIRETEXT_INBOUND_SMS_AUTH = ['testkey'] + MMG_INBOUND_SMS_AUTH = ['testkey'] TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index f44aa17d8..0192bf6fd 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -30,6 +30,15 @@ def receive_mmg_sms(): """ post_data = request.get_json() + auth = request.authorization + + if not auth: + current_app.logger.warning("Inbound sms (MMG) no auth header") + # abort(401) + elif auth.username != 'mmg_co_2017' or auth.password not in current_app.config['MMG_INBOUND_SMS_AUTH']: + current_app.logger.warning("Inbound sms (MMG) incorrect username ({}) or password".format(auth.username)) + # abort(403) + inbound_number = strip_leading_forty_four(post_data['Number']) service = fetch_potential_service(inbound_number, 'mmg') @@ -49,20 +58,23 @@ def receive_mmg_sms(): tasks.send_inbound_sms_to_service.apply_async([str(inbound.id), str(service.id)], queue=QueueNames.NOTIFY) - return 'RECEIVED', 200 + current_app.logger.info( + '{} received inbound SMS with reference {} from MMG'.format(service.id, inbound.provider_reference)) + return jsonify({ + "status": "ok" + }), 200 @receive_notifications_blueprint.route('/notifications/sms/receive/firetext', methods=['POST']) def receive_firetext_sms(): post_data = request.form - # This is pre-implementation test code to validate the provider is basic auth headers. auth = request.authorization if not auth: - current_app.logger.warning("Inbound sms no auth header") + current_app.logger.warning("Inbound sms (Firetext) no auth header") abort(401) elif auth.username != 'notify' or auth.password not in current_app.config['FIRETEXT_INBOUND_SMS_AUTH']: - current_app.logger.warning("Inbound sms incorrect username ({}) or password".format(auth.username)) + current_app.logger.warning("Inbound sms (Firetext) incorrect username ({}) or password".format(auth.username)) abort(403) inbound_number = strip_leading_forty_four(post_data['destination']) diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 982cd0ac1..4642c433d 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -37,14 +37,21 @@ def firetext_post(client, data, auth=True, password='testkey'): ) -def mmg_post(client, data): +def mmg_post(client, data, auth=True, password='testkey'): + headers = [ + ('Content-Type', 'application/json'), + ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') + ] + + if auth: + auth_value = base64.b64encode("notify:{}".format(password).encode('utf-8')).decode('utf-8') + headers.append(('Authorization', 'Basic ' + auth_value)) + return client.post( path='/notifications/sms/receive/mmg', data=json.dumps(data), - headers=[ - ('Content-Type', 'application/json'), - ('X-Forwarded-For', '203.0.113.195, 70.41.3.18, 150.172.238.178') - ]) + headers=headers + ) def test_receive_notification_returns_received_to_mmg(client, mocker, sample_service_full_permissions): @@ -61,7 +68,9 @@ def test_receive_notification_returns_received_to_mmg(client, mocker, sample_ser response = mmg_post(client, data) assert response.status_code == 200 - assert response.get_data(as_text=True) == 'RECEIVED' + result = json.loads(response.get_data(as_text=True)) + assert result['status'] == 'ok' + inbound_sms_id = InboundSms.query.all()[0].id mocked.assert_called_once_with( [str(inbound_sms_id), str(sample_service_full_permissions.id)], queue="notify-internal-tasks") @@ -411,6 +420,31 @@ def test_firetext_inbound_sms_auth(notify_db_session, notify_api, client, mocker assert response.status_code == status_code +@pytest.mark.parametrize("auth, keys, status_code", [ + ["testkey", ["testkey"], 200], + ["", ["testkey"], 401], + ["wrong", ["testkey"], 403], + ["testkey1", ["testkey1", "testkey2"], 200], + ["testkey2", ["testkey1", "testkey2"], 200], + ["wrong", ["testkey1", "testkey2"], 403], + ["", [], 401], + ["testkey", [], 403], +]) +@pytest.mark.skip(reason="aborts are disabled at the moment") +def test_mmg_inbound_sms_auth(notify_db_session, notify_api, client, mocker, auth, keys, status_code): + mocker.patch("app.notifications.receive_notifications.tasks.send_inbound_sms_to_service.apply_async") + + create_service_with_inbound_number( + service_name='b', inbound_number='07111111111', service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE] + ) + + data = "source=07999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" + + with set_config(notify_api, 'MMG_INBOUND_SMS_AUTH', keys): + response = mmg_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', diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index cfa8f8875..fa802afca 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -18,6 +18,7 @@ def notify_config(): 'dangerous_salt': 'dangerous salt', 'allow_ip_inbound_sms': ['111.111.111.111', '100.100.100.100'], 'firetext_inbound_sms_auth': ['testkey'], + 'mmg_inbound_sms_auth': ['testkey'], 'route_secret_key_1': "key_1", 'route_secret_key_2': "" } @@ -232,6 +233,13 @@ def test_firetext_inbound_sms_auth_config(): assert os.environ['FIRETEXT_INBOUND_SMS_AUTH'] == json.dumps(['testkey']) +@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') +def test_mmg_inbound_sms_auth_config(): + extract_cloudfoundry_config() + + assert os.environ['MMG_INBOUND_SMS_AUTH'] == json.dumps(['testkey']) + + @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') def test_performance_platform_config(): extract_cloudfoundry_config() From 1ca252dcf91175e9ba547ce7bc1dbf5028d429bb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 12 Dec 2017 17:28:17 +0000 Subject: [PATCH 2/3] put pdfs in tomorrow's dvla bucket after 17:30 So if someone sends a letter in the evening, it gets picked up the next day --- app/aws/s3.py | 9 +++++++-- tests/app/aws/test_s3.py | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 4551c8c72..5380544c8 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, time from flask import current_app @@ -83,8 +83,13 @@ def remove_transformed_dvla_file(job_id): def upload_letters_pdf(reference, crown, filedata): now = datetime.utcnow() + + print_datetime = now + if now.time() > time(17, 30): + print_datetime = now + timedelta(days=1) + upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=now.date().isoformat(), + folder=print_datetime.date(), reference=reference, duplex="D", letter_class="2", diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index e610560db..23f890b9f 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -147,7 +147,7 @@ def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, (True, 'C'), (False, 'N'), ]) -@freeze_time("2017-12-04 15:00:00") +@freeze_time("2017-12-04 17:29:00") def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( notify_api, mocker, crown_flag, expected_crown_text): s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') @@ -157,5 +157,19 @@ def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( filedata='some_data', region='eu-west-1', bucket_name='test-letters-pdf', - file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204150000.PDF'.format(expected_crown_text) + file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) + ) + + +@freeze_time("2017-12-04 17:31:00") +def test_upload_letters_pdf_puts_in_tomorrows_bucket_after_half_five(notify_api, mocker): + s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') + upload_letters_pdf(reference='foo', crown=True, filedata='some_data') + + s3_upload_mock.assert_called_with( + filedata='some_data', + region='eu-west-1', + bucket_name='test-letters-pdf', + # in tomorrow's folder, but still has this evening's timestamp + file_location='2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' ) From ab66f5c0acae3698f688f5d7c72134c8595c0b75 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 15 Dec 2017 12:19:58 +0000 Subject: [PATCH 3/3] Change MMG username to look at env variable --- app/cloudfoundry_config.py | 1 + app/config.py | 2 ++ app/notifications/receive_notifications.py | 3 ++- tests/app/notifications/test_receive_notification.py | 12 ++++++++++-- tests/app/test_cloudfoundry_config.py | 8 ++++++++ 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/cloudfoundry_config.py b/app/cloudfoundry_config.py index 79af3317a..3a21a7a29 100644 --- a/app/cloudfoundry_config.py +++ b/app/cloudfoundry_config.py @@ -49,6 +49,7 @@ def extract_notify_config(notify_config): os.environ['SMS_INBOUND_WHITELIST'] = json.dumps(notify_config['credentials']['allow_ip_inbound_sms']) os.environ['FIRETEXT_INBOUND_SMS_AUTH'] = json.dumps(notify_config['credentials']['firetext_inbound_sms_auth']) os.environ['MMG_INBOUND_SMS_AUTH'] = json.dumps(notify_config['credentials']['mmg_inbound_sms_auth']) + os.environ['MMG_INBOUND_SMS_USERNAME'] = json.dumps(notify_config['credentials']['mmg_inbound_sms_username']) os.environ['ROUTE_SECRET_KEY_1'] = notify_config['credentials']['route_secret_key_1'] os.environ['ROUTE_SECRET_KEY_2'] = notify_config['credentials']['route_secret_key_2'] diff --git a/app/config.py b/app/config.py index 356b0cad2..0c4b477c6 100644 --- a/app/config.py +++ b/app/config.py @@ -301,6 +301,7 @@ class Config(object): SMS_INBOUND_WHITELIST = json.loads(os.environ.get('SMS_INBOUND_WHITELIST', '[]')) FIRETEXT_INBOUND_SMS_AUTH = json.loads(os.environ.get('FIRETEXT_INBOUND_SMS_AUTH', '[]')) MMG_INBOUND_SMS_AUTH = json.loads(os.environ.get('MMG_INBOUND_SMS_AUTH', '[]')) + MMG_INBOUND_SMS_USERNAME = json.loads(os.environ.get('MMG_INBOUND_SMS_USERNAME', '[]')) ROUTE_SECRET_KEY_1 = os.environ.get('ROUTE_SECRET_KEY_1', '') ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '') @@ -378,6 +379,7 @@ class Test(Config): SMS_INBOUND_WHITELIST = ['203.0.113.195'] FIRETEXT_INBOUND_SMS_AUTH = ['testkey'] MMG_INBOUND_SMS_AUTH = ['testkey'] + MMG_INBOUND_SMS_USERNAME = ['username'] TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 0192bf6fd..c6bf80811 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -35,7 +35,8 @@ def receive_mmg_sms(): if not auth: current_app.logger.warning("Inbound sms (MMG) no auth header") # abort(401) - elif auth.username != 'mmg_co_2017' or auth.password not in current_app.config['MMG_INBOUND_SMS_AUTH']: + elif auth.username not in current_app.config['MMG_INBOUND_SMS_USERNAME'] \ + or auth.password not in current_app.config['MMG_INBOUND_SMS_AUTH']: current_app.logger.warning("Inbound sms (MMG) incorrect username ({}) or password".format(auth.username)) # abort(403) diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 4642c433d..02898d564 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -44,7 +44,7 @@ def mmg_post(client, data, auth=True, password='testkey'): ] if auth: - auth_value = base64.b64encode("notify:{}".format(password).encode('utf-8')).decode('utf-8') + auth_value = base64.b64encode("username:{}".format(password).encode('utf-8')).decode('utf-8') headers.append(('Authorization', 'Basic ' + auth_value)) return client.post( @@ -438,7 +438,15 @@ def test_mmg_inbound_sms_auth(notify_db_session, notify_api, client, mocker, aut service_name='b', inbound_number='07111111111', service_permissions=[EMAIL_TYPE, SMS_TYPE, INBOUND_SMS_TYPE] ) - data = "source=07999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" + data = { + "ID": "1234", + "MSISDN": "07111111111", + "Message": "Some message to notify", + "Trigger": "Trigger?", + "Number": "testing", + "Channel": "SMS", + "DateRecieved": "2012-06-27 12:33:00" + } with set_config(notify_api, 'MMG_INBOUND_SMS_AUTH', keys): response = mmg_post(client, data, auth=bool(auth), password=auth) diff --git a/tests/app/test_cloudfoundry_config.py b/tests/app/test_cloudfoundry_config.py index fa802afca..aafd894c7 100644 --- a/tests/app/test_cloudfoundry_config.py +++ b/tests/app/test_cloudfoundry_config.py @@ -19,6 +19,7 @@ def notify_config(): 'allow_ip_inbound_sms': ['111.111.111.111', '100.100.100.100'], 'firetext_inbound_sms_auth': ['testkey'], 'mmg_inbound_sms_auth': ['testkey'], + 'mmg_inbound_sms_username': ['username'], 'route_secret_key_1': "key_1", 'route_secret_key_2': "" } @@ -240,6 +241,13 @@ def test_mmg_inbound_sms_auth_config(): assert os.environ['MMG_INBOUND_SMS_AUTH'] == json.dumps(['testkey']) +@pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') +def test_mmg_inbound_sms_username_config(): + extract_cloudfoundry_config() + + assert os.environ['MMG_INBOUND_SMS_USERNAME'] == json.dumps(['username']) + + @pytest.mark.usefixtures('os_environ', 'cloudfoundry_environ') def test_performance_platform_config(): extract_cloudfoundry_config()