From a01828a6d0b72d226321bd65e22707818eb47299 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Feb 2016 11:15:41 +0000 Subject: [PATCH 1/5] Return notification_id on create notification endpoints - /notification/sms - /notification/email - /notificaiton/sms/service/ Update message attribute on SQS to notification_id from message_id --- app/aws_sqs.py | 5 +++-- app/notifications/rest.py | 15 ++++++--------- tests/app/notifications/test_rest.py | 9 ++++++--- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/app/aws_sqs.py b/app/aws_sqs.py index 86b28eab8..ea362024c 100644 --- a/app/aws_sqs.py +++ b/app/aws_sqs.py @@ -10,11 +10,12 @@ def add_notification_to_queue(service_id, template_id, type_, notification): ).create_queue(QueueName="{}_{}".format( current_app.config['NOTIFICATION_QUEUE_PREFIX'], str(service_id))) - message_id = str(uuid.uuid4()) + notification_id = str(uuid.uuid4()) serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) q.send_message(MessageBody=encrypted, MessageAttributes={'type': {'StringValue': type_, 'DataType': 'String'}, - 'message_id': {'StringValue': message_id, 'DataType': 'String'}, + 'notification_id': {'StringValue': notification_id, 'DataType': 'String'}, 'service_id': {'StringValue': str(service_id), 'DataType': 'String'}, 'template_id': {'StringValue': str(template_id), 'DataType': 'String'}}) + return notification_id diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 0c3cdf569..b596e15b1 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -29,9 +29,8 @@ def create_sms_notification(): if errors: return jsonify(result="error", message=errors), 400 - add_notification_to_queue(api_user['client'], notification['template'], 'sms', notification) - # TODO data to be returned - return jsonify({}), 204 + notification_id = add_notification_to_queue(api_user['client'], notification['template'], 'sms', notification) + return jsonify({'notification_id': notification_id}), 201 @notifications.route('/email', methods=['POST']) @@ -40,9 +39,8 @@ def create_email_notification(): notification, errors = email_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 - add_notification_to_queue(api_user['client'], "admin", 'email', notification) - # TODO data to be returned - return jsonify({}), 204 + notification_id = add_notification_to_queue(api_user['client'], "admin", 'email', notification) + return jsonify({'notification_id': notification_id}), 201 @notifications.route('/sms/service/', methods=['POST']) @@ -66,6 +64,5 @@ def create_sms_for_service(service_id): message = "Invalid template: id {} for service id: {}".format(template.id, service_id) return jsonify(result="error", message=message), 400 - add_notification_to_queue(service_id, template_id, 'sms', notification) - # TODO data to be returned - return jsonify({}), 204 + notification_id = add_notification_to_queue(service_id, template_id, 'sms', notification) + return jsonify({'notification_id': notification_id}), 201 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index b506bb10a..d3c2170e8 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -199,7 +199,8 @@ def test_should_allow_valid_message(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 204 + assert response.status_code == 201 + assert json.loads(response.data)['notification_id'] is not None @moto.mock_sqs @@ -232,7 +233,8 @@ def test_send_email_valid_data(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 204 + assert response.status_code == 201 + assert json.loads(response.data)['notification_id'] is not None @moto.mock_sqs @@ -263,7 +265,8 @@ def test_valid_message_with_service_id(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 204 + assert response.status_code == 201 + assert json.loads(response.data)['notification_id'] is not None @moto.mock_sqs From 2db0f9737ead488db8d36ae73bc4c295a1931360 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Feb 2016 12:24:10 +0000 Subject: [PATCH 2/5] Added a test to check endpoint works with an id in payload. --- tests/app/job/test_job_rest.py | 35 ++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index a10c004da..9957f3257 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -251,6 +251,41 @@ def test_add_notification(notify_api, notify_db, notify_db_session, sample_job): assert 'sent' == resp_json['data']['status'] +def test_add_notification_with_id(notify_api, notify_db, notify_db_session, sample_job): + notification_id = str(uuid.uuid4()) + to = '+44709123456' + data = { + 'id': notification_id, + 'to': to, + 'job': str(sample_job.id), + 'service': str(sample_job.service.id), + 'template': sample_job.template.id + } + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = url_for('job.create_notification_for_job', + service_id=sample_job.service.id, + job_id=sample_job.id) + + auth_header = create_authorization_header(service_id=sample_job.service.id, + path=path, + method='POST', + request_body=json.dumps(data)) + + headers = [('Content-Type', 'application/json'), auth_header] + + response = client.post(path, headers=headers, data=json.dumps(data)) + + resp_json = json.loads(response.get_data(as_text=True)) + + assert resp_json['data']['id'] == notification_id + assert data['to'] == resp_json['data']['to'] + assert data['service'] == resp_json['data']['service'] + assert data['template'] == resp_json['data']['template'] + assert data['job'] == resp_json['data']['job'] + assert 'sent' == resp_json['data']['status'] + + def test_update_notification(notify_api, notify_db, notify_db_session, sample_notification): assert sample_notification.status == 'sent' From 0cd5fa278bbeb02f8336622c7725e69a805888ac Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 10 Feb 2016 13:09:36 +0000 Subject: [PATCH 3/5] Fix a intermittent test. Removed the need for sample_admin_service_id in service/test_rest --- tests/app/service/test_rest.py | 123 ++++++++++++++------------------- 1 file changed, 52 insertions(+), 71 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cf21ade81..11a0a68a3 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -10,33 +10,31 @@ from tests.app.conftest import sample_user as create_sample_user from tests.app.conftest import sample_service as create_sample_service -def test_get_service_list(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_get_service_list(notify_api, notify_db, notify_db_session, sample_service): """ Tests GET endpoint '/' to retrieve entire service list. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.get_service'), + auth_header = create_authorization_header(path=url_for('service.get_service'), method='GET') response = client.get(url_for('service.get_service'), headers=[auth_header]) assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) # TODO assert correct json returned - assert len(json_resp['data']) == 2 + assert len(json_resp['data']) == 1 assert json_resp['data'][0]['name'] == sample_service.name assert json_resp['data'][0]['id'] == str(sample_service.id) -def test_get_service(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_get_service(notify_api, notify_db, notify_db_session, sample_service): """ Tests GET endpoint '/' to retrieve a single service. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.get_service', service_id=sample_service.id), + auth_header = create_authorization_header(path=url_for('service.get_service', service_id=sample_service.id), method='GET') resp = client.get(url_for('service.get_service', service_id=sample_service.id), @@ -67,21 +65,20 @@ def test_get_service_for_user(notify_api, notify_db, notify_db_session, sample_s assert 'Second Service' not in [x.get('name') for x in json_resp['data']] -def test_post_service(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): +def test_post_service(notify_api, notify_db, notify_db_session, sample_user): """ Tests POST endpoint '/' to create a service. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Service.query.count() == 1 + assert Service.query.count() == 0 data = { 'name': 'created service', 'users': [sample_user.id], 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_service'), + auth_header = create_authorization_header(path=url_for('service.create_service'), method='POST', request_body=json.dumps(data)) headers = [('Content-Type', 'application/json'), auth_header] @@ -96,7 +93,7 @@ def test_post_service(notify_api, notify_db, notify_db_session, sample_user, sam assert json_resp['data']['limit'] == service.limit -def test_post_service_multiple_users(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): +def test_post_service_multiple_users(notify_api, notify_db, notify_db_session, sample_user): """ Tests POST endpoint '/' to create a service with multiple users. """ @@ -106,15 +103,14 @@ def test_post_service_multiple_users(notify_api, notify_db, notify_db_session, s notify_db, notify_db_session, "new@digital.cabinet-office.gov.uk") - assert Service.query.count() == 1 + assert Service.query.count() == 0 data = { 'name': 'created service', 'users': [sample_user.id, another_user.id], 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_service'), + auth_header = create_authorization_header(path=url_for('service.create_service'), method='POST', request_body=json.dumps(data)) headers = [('Content-Type', 'application/json'), auth_header] @@ -130,20 +126,19 @@ def test_post_service_multiple_users(notify_api, notify_db, notify_db_session, s assert len(service.users) == 2 -def test_post_service_without_users_attribute(notify_api, notify_db, notify_db_session, sample_admin_service_id): +def test_post_service_without_users_attribute(notify_api, notify_db, notify_db_session): """ Tests POST endpoint '/' to create a service without 'users' attribute. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Service.query.count() == 1 + assert Service.query.count() == 0 data = { 'name': 'created service', 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_service'), + auth_header = create_authorization_header(path=url_for('service.create_service'), method='POST', request_body=json.dumps(data)) headers = [('Content-Type', 'application/json'), auth_header] @@ -152,18 +147,18 @@ def test_post_service_without_users_attribute(notify_api, notify_db, notify_db_s data=json.dumps(data), headers=headers) assert resp.status_code == 400 - assert Service.query.count() == 1 + assert Service.query.count() == 0 json_resp = json.loads(resp.get_data(as_text=True)) assert json_resp['message'] == '{"users": ["Missing data for required attribute"]}' -def test_put_service(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_put_service(notify_api, notify_db, notify_db_session, sample_service): """ Tests PUT endpoint '/' to edit a service. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Service.query.count() == 2 + assert Service.query.count() == 1 new_name = 'updated service' data = { 'name': new_name, @@ -171,8 +166,7 @@ def test_put_service(notify_api, notify_db, notify_db_session, sample_service, s 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=sample_service.id), method='PUT', request_body=json.dumps(data)) @@ -181,7 +175,7 @@ def test_put_service(notify_api, notify_db, notify_db_session, sample_service, s url_for('service.update_service', service_id=sample_service.id), data=json.dumps(data), headers=headers) - assert Service.query.count() == 2 + assert Service.query.count() == 1 assert resp.status_code == 200 updated_service = Service.query.get(sample_service.id) json_resp = json.loads(resp.get_data(as_text=True)) @@ -190,7 +184,7 @@ def test_put_service(notify_api, notify_db, notify_db_session, sample_service, s assert updated_service.name == new_name -def test_put_service_not_exists(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_put_service_not_exists(notify_api, notify_db, notify_db_session, sample_service): """ Tests PUT endpoint '/' service doesn't exist. """ @@ -205,8 +199,7 @@ def test_put_service_not_exists(notify_api, notify_db, notify_db_session, sample 'restricted': False, 'active': False} missing_service_id = uuid.uuid4() - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=missing_service_id), method='PUT', request_body=json.dumps(data)) @@ -219,13 +212,13 @@ def test_put_service_not_exists(notify_api, notify_db, notify_db_session, sample assert Service.query.first().name != new_name -def test_put_service_add_user(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_put_service_add_user(notify_api, notify_db, notify_db_session, sample_service): """ Tests PUT endpoint '/' add user to the service. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Service.query.count() == 2 + assert Service.query.count() == 1 another_user = create_sample_user( notify_db, notify_db_session, @@ -238,8 +231,7 @@ def test_put_service_add_user(notify_api, notify_db, notify_db_session, sample_s 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=sample_service.id), method='PUT', request_body=json.dumps(data)) @@ -248,7 +240,7 @@ def test_put_service_add_user(notify_api, notify_db, notify_db_session, sample_s url_for('service.update_service', service_id=sample_service.id), data=json.dumps(data), headers=headers) - assert Service.query.count() == 2 + assert Service.query.count() == 1 assert resp.status_code == 200 updated_service = Service.query.get(sample_service.id) json_resp = json.loads(resp.get_data(as_text=True)) @@ -259,7 +251,7 @@ def test_put_service_add_user(notify_api, notify_db, notify_db_session, sample_s assert set(updated_service.users) == set([sample_user, another_user]) -def test_put_service_remove_user(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_put_service_remove_user(notify_api, notify_db, notify_db_session, sample_service): """ Tests PUT endpoint '/' add user to the service. """ @@ -277,11 +269,10 @@ def test_put_service_remove_user(notify_api, notify_db, notify_db_session, sampl 'restricted': sample_service.restricted, 'active': sample_service.active} save_model_service(sample_service, update_dict=data) - assert Service.query.count() == 2 + assert Service.query.count() == 1 data['users'] = [another_user.id] - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=sample_service.id), method='PUT', request_body=json.dumps(data)) @@ -290,7 +281,7 @@ def test_put_service_remove_user(notify_api, notify_db, notify_db_session, sampl url_for('service.update_service', service_id=sample_service.id), data=json.dumps(data), headers=headers) - assert Service.query.count() == 2 + assert Service.query.count() == 1 assert resp.status_code == 200 updated_service = Service.query.get(sample_service.id) json_resp = json.loads(resp.get_data(as_text=True)) @@ -301,14 +292,13 @@ def test_put_service_remove_user(notify_api, notify_db, notify_db_session, sampl assert another_user in updated_service.users -def test_delete_service(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_delete_service(notify_api, notify_db, notify_db_session, sample_service): """ Tests DELETE endpoint '/' delete service. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=sample_service.id), method='DELETE') resp = client.delete( @@ -317,40 +307,38 @@ def test_delete_service(notify_api, notify_db, notify_db_session, sample_service assert resp.status_code == 202 json_resp = json.loads(resp.get_data(as_text=True)) json_resp['data']['name'] == sample_service.name - assert Service.query.count() == 1 + assert Service.query.count() == 0 -def test_delete_service_not_exists(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_delete_service_not_exists(notify_api, notify_db, notify_db_session, sample_service): """ Tests DELETE endpoint '/' delete service doesn't exist. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Service.query.count() == 2 + assert Service.query.count() == 1 missing_service_id = uuid.uuid4() - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_service', + auth_header = create_authorization_header(path=url_for('service.update_service', service_id=missing_service_id), method='DELETE') resp = client.delete( url_for('service.update_service', service_id=missing_service_id), headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 404 - assert Service.query.count() == 2 + assert Service.query.count() == 1 -def test_create_service_should_create_new_service_for_user(notify_api, notify_db, notify_db_session, sample_user, - sample_admin_service_id): +def test_create_service_should_create_new_service_for_user(notify_api, notify_db, notify_db_session, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: + assert Service.query.count() == 0 data = { 'name': 'created service', 'users': [sample_user.id], 'limit': 1000, 'restricted': False, 'active': False} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_service'), + auth_header = create_authorization_header(path=url_for('service.create_service'), method='POST', request_body=json.dumps(data)) headers = [('Content-Type', 'application/json'), auth_header] @@ -358,9 +346,10 @@ def test_create_service_should_create_new_service_for_user(notify_api, notify_db data=json.dumps(data), headers=headers) assert resp.status_code == 201 + assert Service.query.count() == 1 -def test_create_template(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): +def test_create_template(notify_api, notify_db, notify_db_session, sample_service): """ Tests POST endpoint '//template' a template can be created from a service. @@ -377,8 +366,7 @@ def test_create_template(notify_api, notify_db, notify_db_session, sample_servic 'content': template_content, 'service': str(sample_service.id) } - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_template', + auth_header = create_authorization_header(path=url_for('service.create_template', service_id=sample_service.id), method='POST', request_body=json.dumps(data)) @@ -394,8 +382,7 @@ def test_create_template(notify_api, notify_db, notify_db_session, sample_servic assert json_resp['data']['content'] == template_content -def test_create_template_service_not_exists(notify_api, notify_db, notify_db_session, sample_service, - sample_admin_service_id): +def test_create_template_service_not_exists(notify_api, notify_db, notify_db_session, sample_service): """ Tests POST endpoint '//template' a template can be created from a service. @@ -413,8 +400,7 @@ def test_create_template_service_not_exists(notify_api, notify_db, notify_db_ses 'service': str(sample_service.id) } missing_service_id = uuid.uuid4() - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_template', + auth_header = create_authorization_header(path=url_for('service.create_template', service_id=missing_service_id), method='POST', request_body=json.dumps(data)) @@ -428,7 +414,7 @@ def test_create_template_service_not_exists(notify_api, notify_db, notify_db_ses assert "Service not found" in json_resp['message'] -def test_update_template(notify_api, notify_db, notify_db_session, sample_template, sample_admin_service_id): +def test_update_template(notify_api, notify_db, notify_db_session, sample_template): """ Tests PUT endpoint '//template/' a template can be updated. @@ -447,8 +433,7 @@ def test_update_template(notify_api, notify_db, notify_db_session, sample_templa 'content': template_content, 'service': str(sample_service.id) } - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_template', + auth_header = create_authorization_header(path=url_for('service.update_template', service_id=sample_service.id, template_id=sample_template.id), method='PUT', @@ -469,7 +454,7 @@ def test_update_template(notify_api, notify_db, notify_db_session, sample_templa def test_update_template_service_not_exists(notify_api, notify_db, notify_db_session, - sample_template, sample_admin_service_id): + sample_template): """ Tests PUT endpoint '//template/' a 404 if service doesn't exist. @@ -487,8 +472,7 @@ def test_update_template_service_not_exists(notify_api, notify_db, notify_db_ses 'service': str(sample_template.service_id) } missing_service_id = uuid.uuid4() - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_template', + auth_header = create_authorization_header(path=url_for('service.update_template', service_id=missing_service_id, template_id=sample_template.id), method='PUT', @@ -506,7 +490,7 @@ def test_update_template_service_not_exists(notify_api, notify_db, notify_db_ses def test_update_template_template_not_exists(notify_api, notify_db, notify_db_session, - sample_template, sample_admin_service_id): + sample_template): """ Tests PUT endpoint '//template/' a 404 if template doesn't exist. @@ -524,8 +508,7 @@ def test_update_template_template_not_exists(notify_api, notify_db, notify_db_se 'content': template_content, 'service': str(sample_service.id) } - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.update_template', + auth_header = create_authorization_header(path=url_for('service.update_template', service_id=sample_service.id, template_id="123"), method='PUT', @@ -542,8 +525,7 @@ def test_update_template_template_not_exists(notify_api, notify_db, notify_db_se assert template_name != sample_template.name -def test_create_template_unicode_content(notify_api, notify_db, notify_db_session, sample_service, - sample_admin_service_id): +def test_create_template_unicode_content(notify_api, notify_db, notify_db_session, sample_service): """ Tests POST endpoint '//template/' a template is created and the content encoding is respected after saving and loading @@ -561,8 +543,7 @@ def test_create_template_unicode_content(notify_api, notify_db, notify_db_sessio 'content': template_content, 'service': str(sample_service.id) } - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.create_template', + auth_header = create_authorization_header(path=url_for('service.create_template', service_id=sample_service.id), method='POST', request_body=json.dumps(data)) From 918c5617267774a4707aea16635a6037e517bcb0 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Fri, 12 Feb 2016 11:13:54 +0000 Subject: [PATCH 4/5] Code added to now check service id matches the authorization token service for sending an sms. --- app/schemas.py | 11 ++++++++- tests/app/notifications/test_rest.py | 34 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 3a0ab41df..83b9a764e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,4 +1,5 @@ import re +from flask import current_app from flask_marshmallow.fields import fields from . import ma from . import models @@ -102,12 +103,15 @@ class SmsTemplateNotificationSchema(SmsNotificationSchema): @validates_schema def validate_schema(self, data): """ - Validate the to field is valid for this template + Validate the to field is valid for this notification """ + from app import api_user template_id = data.get('template', None) template = models.Template.query.filter_by(id=template_id).first() if template: service = template.service + # Validate restricted service, + # restricted services can only send to one of its users. if service.restricted: valid = False for usr in service.users: @@ -116,6 +120,11 @@ class SmsTemplateNotificationSchema(SmsNotificationSchema): break if not valid: raise ValidationError('Invalid phone number for restricted service', 'restricted') + # Assert the template is valid for the service which made the request. + service = api_user['client'] + if service != current_app.config.get('ADMIN_CLIENT_USER_NAME'): + if template.service != models.Service.query.filter_by(id=service).first(): + raise ValidationError('Invalid template', 'restricted') class SmsAdminNotificationSchema(SmsNotificationSchema): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index d3c2170e8..33cef53b0 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -4,6 +4,8 @@ import uuid from tests import create_authorization_header from flask import url_for, json from app.models import Service +from tests.app.conftest import sample_service as create_sample_service +from tests.app.conftest import sample_template as create_sample_template def test_get_notifications( @@ -172,6 +174,38 @@ def test_send_notification_invalid_template_id(notify_api, assert 'Template not found' in json_resp['message']['template'] +@moto.mock_sqs +def test_should_not_allow_template_from_other_service(notify_api, + notify_db, + notify_db_session, + sample_template, + sample_admin_service_id, + mocker): + """ + Tests POST endpoint '/sms' with notifications. + """ + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + 'to': '+441234123123', + 'template': sample_template.id + } + auth_header = create_authorization_header( + service_id=sample_admin_service_id, + request_body=json.dumps(data), + path=url_for('notifications.create_sms_notification'), + method='POST') + + response = client.post( + url_for('notifications.create_sms_notification'), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert 'Invalid template' in json_resp['message']['restricted'] + + @moto.mock_sqs def test_should_allow_valid_message(notify_api, notify_db, From 1eb18e7f0766678cb3a2d758f0527dd59f47b91c Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Fri, 12 Feb 2016 14:08:48 +0000 Subject: [PATCH 5/5] Code review fix. --- app/schemas.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 83b9a764e..050e35d48 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -122,9 +122,9 @@ class SmsTemplateNotificationSchema(SmsNotificationSchema): raise ValidationError('Invalid phone number for restricted service', 'restricted') # Assert the template is valid for the service which made the request. service = api_user['client'] - if service != current_app.config.get('ADMIN_CLIENT_USER_NAME'): - if template.service != models.Service.query.filter_by(id=service).first(): - raise ValidationError('Invalid template', 'restricted') + if (service != current_app.config.get('ADMIN_CLIENT_USER_NAME') and + template.service != models.Service.query.filter_by(id=service).first()): + raise ValidationError('Invalid template', 'restricted') class SmsAdminNotificationSchema(SmsNotificationSchema):