From bef8e8887bd59f1e630a811e37588bce8756966b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 6 Dec 2017 20:24:25 +0000 Subject: [PATCH 1/7] Strip spaces from email addresses when signing in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you copy and paste an email address into the sign in box, there’s a chance you’ll also copy some leading or trailing spaces. This is especially likely to happen if you’re doing this while using your computer upside down. If this happens, it never even gets as far as looking up the user, because the form validation doesn’t consider a string with a leading space to be a valid email address. This commit makes sure that accidental spaces are handled, by removing them before doing any validation or hitting the API to look up the user. --- app/main/views/sign_in.py | 3 +++ tests/app/main/views/test_sign_in.py | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 8721c7451..fc8e9b049 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -30,6 +30,9 @@ def sign_in(): return redirect(url_for('main.choose_service')) form = LoginForm() + if form.email_address.data: + form.email_address.data = form.email_address.data.strip() + if form.validate_on_submit(): user = user_api_client.get_user_by_email_or_none(form.email_address.data) diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 97b1956ac..dc9d4da29 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -77,6 +77,10 @@ def test_logged_in_user_redirects_to_choose_service( assert response.location == url_for('main.choose_service', _external=True) +@pytest.mark.parametrize('email_address', [ + 'valid@example.gov.uk', + ' valid@example.gov.uk ', +]) def test_process_sms_auth_sign_in_return_2fa_template( client, api_user_active, @@ -84,14 +88,16 @@ def test_process_sms_auth_sign_in_return_2fa_template( mock_get_user, mock_get_user_by_email, mock_verify_password, + email_address, ): response = client.post( url_for('main.sign_in'), data={ - 'email_address': 'valid@example.gov.uk', + 'email_address': email_address, 'password': 'val1dPassw0rd!'}) assert response.status_code == 302 assert response.location == url_for('.two_factor', _external=True) mock_verify_password.assert_called_with(api_user_active.id, 'val1dPassw0rd!') + mock_get_user_by_email.assert_called_with('valid@example.gov.uk') def test_process_email_auth_sign_in_return_2fa_template( From 79912166d75376a77b3013ac05b6a0928bee97e4 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Mon, 11 Dec 2017 10:25:23 +0000 Subject: [PATCH 2/7] 117-119 for Cash Service MK and HMCTS Small Claims Mediation 50 - 51 orgs too --- app/templates/views/signedout.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 0ec9f1878..05bde1b5e 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -120,12 +120,12 @@
-
117
+
119
services
-
50
+
51
organisations
From 34aca4de5bdd2ea783922a3d3842d6f323f2dbf4 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 8 Dec 2017 23:34:02 +0000 Subject: [PATCH 3/7] Update pyexcel-xls from 0.5.4 to 0.5.5 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 455683a0d..5f7fcadd0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,7 +7,7 @@ boto3==1.4.8 blinker==1.4 pyexcel==0.5.6 pyexcel-io==0.5.4 -pyexcel-xls==0.5.4 +pyexcel-xls==0.5.5 pyexcel-xlsx==0.5.4 pyexcel-ods3==0.5.2 pytz==2017.3 From 76117c148cf43e762eb37b32597b8b9605215eae Mon Sep 17 00:00:00 2001 From: chrisw Date: Mon, 4 Dec 2017 15:07:11 +0000 Subject: [PATCH 4/7] Allow service to set callback url for notifications --- app/main/forms.py | 41 ++- app/main/views/api_keys.py | 90 +++++- app/main/views/service_settings.py | 61 ---- app/notify_client/service_api_client.py | 24 ++ .../callbacks.html} | 27 +- app/templates/views/api/index.html | 2 +- app/templates/views/callbacks.html | 32 ++- app/templates/views/service-settings.html | 8 - tests/__init__.py | 2 + tests/app/main/views/test_api_keys.py | 264 ++++++++++++++++++ tests/app/main/views/test_service_settings.py | 178 ------------ tests/conftest.py | 62 ++++ 12 files changed, 523 insertions(+), 268 deletions(-) rename app/templates/views/{service-settings/set-inbound-api.html => api/callbacks.html} (53%) diff --git a/app/main/forms.py b/app/main/forms.py index ca5e9f1ba..47e695ca8 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -717,15 +717,38 @@ class ServiceInboundNumberForm(Form): ) -class ServiceInboundApiForm(Form): - url = StringField("Callback URL", - validators=[DataRequired(message='Can’t be empty'), - Regexp(regex="^https.*", - message='Must be a valid https URL')] - ) - bearer_token = PasswordFieldShowHasContent("Bearer token", - validators=[DataRequired(message='Can’t be empty'), - Length(min=10, message='Must be at least 10 characters')]) +class ServiceCallbacksForm(Form): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.can_receive_inbound = kwargs['can_receive_inbound'] + + inbound_url = StringField("Inbound URL") + inbound_bearer_token = PasswordFieldShowHasContent("Bearer token") + outbound_url = StringField( + "Outbound URL", + validators=[DataRequired(message='Can’t be empty'), + Regexp(regex="^https.*", message='Must be a valid https URL')] + ) + outbound_bearer_token = PasswordFieldShowHasContent( + "Bearer token", + validators=[DataRequired(message='Can’t be empty'), + Length(min=10, message='Must be at least 10 characters')] + ) + + def validate_inbound_url(self, field): + pattern = re.compile("^https.*") + if self.can_receive_inbound: + if not field.data: + raise ValidationError('Can’t be empty') + elif not pattern.match(field.data): + raise ValidationError('Must be a valid https URL') + + def validate_inbound_bearer_token(self, field): + if self.can_receive_inbound: + if not field.data: + raise ValidationError('Can’t be empty') + elif len(field.data) < 10: + raise ValidationError('Must be at least 10 characters') class InternationalSMSForm(Form): diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 14d31bb65..98e886ca6 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -1,11 +1,13 @@ from flask import request, render_template, redirect, url_for, flash, Markup, abort -from flask_login import login_required +from flask_login import login_required, current_user from app.main import main -from app.main.forms import CreateKeyForm, Whitelist +from app.main.forms import CreateKeyForm, Whitelist, ServiceCallbacksForm from app import api_key_api_client, service_api_client, notification_api_client, current_service from app.utils import user_has_permissions, email_safe from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM +dummy_bearer_token = 'bearer_token_set' + @main.route("/services//api") @login_required @@ -113,3 +115,87 @@ def revoke_api_key(service_id, key_id): api_key_api_client.revoke_api_key(service_id=service_id, key_id=key_id) flash('‘{}’ was revoked'.format(key_name), 'default_with_tick') return redirect(url_for('.api_keys', service_id=service_id)) + + +def get_apis(): + callback_api = None + inbound_api = None + if current_service['service_callback_api']: + callback_api = service_api_client.get_service_callback_api( + current_service['id'], + current_service.get('service_callback_api')[0] + ) + if current_service['inbound_api']: + inbound_api = service_api_client.get_service_inbound_api( + current_service['id'], + current_service.get('inbound_api')[0] + ) + + return (callback_api, inbound_api) + + +def check_token_against_dummy_bearer(token): + if token != dummy_bearer_token: + return token + else: + return '' + + +@main.route("/services//api/callbacks", methods=['GET', 'POST']) +@login_required +def api_callbacks(service_id): + callback_api, inbound_api = get_apis() + can_receive_inbound = 'inbound_sms' in current_service['permissions'] + + form = ServiceCallbacksForm( + inbound_url=inbound_api.get('url') if inbound_api else '', + inbound_bearer_token=dummy_bearer_token if inbound_api else '', + outbound_url=callback_api.get('url') if callback_api else '', + outbound_bearer_token=dummy_bearer_token if callback_api else '', + can_receive_inbound=can_receive_inbound, + ) + + if form.validate_on_submit(): + if callback_api: + if (callback_api.get('url') != form.outbound_url.data + or form.outbound_bearer_token.data != dummy_bearer_token): + service_api_client.update_service_callback_api( + service_id, + url=form.outbound_url.data, + bearer_token=check_token_against_dummy_bearer(form.outbound_bearer_token.data), + user_id=current_user.id, + callback_api_id=callback_api.get('id') + ) + else: + service_api_client.create_service_callback_api( + service_id, + url=form.outbound_url.data, + bearer_token=form.outbound_bearer_token.data, + user_id=current_user.id + ) + if can_receive_inbound: + if inbound_api: + if (inbound_api.get('url') != form.inbound_url.data + or form.inbound_bearer_token.data != dummy_bearer_token): + service_api_client.update_service_inbound_api( + service_id, + url=form.inbound_url.data, + bearer_token=check_token_against_dummy_bearer(form.inbound_bearer_token.data), + user_id=current_user.id, + inbound_api_id=inbound_api.get('id') + ) + else: + service_api_client.create_service_inbound_api( + service_id, + url=form.inbound_url.data, + bearer_token=form.inbound_bearer_token.data, + user_id=current_user.id + ) + + return redirect(url_for('.api_integration', service_id=service_id)) + + return render_template( + 'views/api/callbacks.html', + form=form, + can_receive_inbound=can_receive_inbound, + ) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 80bb9d4e9..7631b0f39 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1,5 +1,3 @@ -from urllib.parse import urlparse - import requests from flask import ( render_template, @@ -33,7 +31,6 @@ from app.main.forms import ( ServiceLetterContactBlockForm, ServiceBrandingOrg, LetterBranding, - ServiceInboundApiForm, InternationalSMSForm, OrganisationTypeForm, FreeSMSAllowance, @@ -44,17 +41,6 @@ from app import user_api_client, current_service, organisations_client, inbound_ from notifications_utils.formatters import formatted_list -dummy_bearer_token = 'bearer_token_set' - - -def get_inbound_api(): - if current_service['inbound_api']: - return service_api_client.get_service_inbound_api( - current_service['id'], - current_service.get('inbound_api')[0] - ) - - @main.route("/services//service-settings") @login_required @user_has_permissions('manage_settings', admin_override=True) @@ -65,14 +51,6 @@ def service_settings(service_id): else: organisation = None - inbound_api = get_inbound_api() - if inbound_api: - parsed_url = urlparse(inbound_api.get('url')) if inbound_api else '' - inbound_api_url = '{uri.scheme}://{uri.netloc}{elide_token}'.format( - uri=parsed_url, elide_token='...' if parsed_url.path else '') - else: - inbound_api_url = '' - inbound_number = inbound_number_client.get_inbound_sms_number_for_service(service_id) disp_inbound_number = inbound_number['data'].get('number', '') reply_to_email_addresses = service_api_client.get_reply_to_email_addresses(service_id) @@ -100,7 +78,6 @@ def service_settings(service_id): current_service.get('dvla_organisation', '001') ), can_receive_inbound=('inbound_sms' in current_service['permissions']), - inbound_api_url=inbound_api_url, inbound_number=disp_inbound_number, default_reply_to_email_address=default_reply_to_email_address, reply_to_email_address_count=reply_to_email_address_count, @@ -807,41 +784,3 @@ def get_branding_as_dict(organisations): 'colour': organisation['colour'] } for organisation in organisations } - - -@main.route("/services//service-settings/set-inbound-api", methods=['GET', 'POST']) -@login_required -@user_has_permissions('manage_settings', admin_override=True) -def service_set_inbound_api(service_id): - if 'inbound_sms' not in current_service['permissions']: - abort(403) - - inbound_api = get_inbound_api() - form = ServiceInboundApiForm( - url=inbound_api.get('url') if inbound_api else '', - bearer_token=dummy_bearer_token if inbound_api else '' - ) - - if form.validate_on_submit(): - if inbound_api: - if inbound_api.get('url') != form.url.data or form.bearer_token.data != dummy_bearer_token: - service_api_client.update_service_inbound_api( - service_id, - url=form.url.data, - bearer_token=form.bearer_token.data if form.bearer_token.data != dummy_bearer_token else '', - user_id=current_user.id, - inbound_api_id=inbound_api.get('id') - ) - else: - service_api_client.create_service_inbound_api( - service_id, - url=form.url.data, - bearer_token=form.bearer_token.data, - user_id=current_user.id - ) - return redirect(url_for('.service_settings', service_id=service_id)) - - return render_template( - 'views/service-settings/set-inbound-api.html', - form=form, - ) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 08f78c038..2b7b6ead4 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -387,6 +387,30 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) + def get_service_callback_api(self, service_id, callback_api_id): + return self.get( + "/service/{}/delivery-receipt-api/{}".format( + service_id, callback_api_id + ) + )['data'] + + def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id): + data = { + "url": url, + "updated_by_id": user_id + } + if bearer_token: + data['bearer_token'] = bearer_token + return self.post("/service/{}/delivery-receipt-api/{}".format(service_id, callback_api_id), data) + + def create_service_callback_api(self, service_id, url, bearer_token, user_id): + data = { + "url": url, + "bearer_token": bearer_token, + "updated_by_id": user_id + } + return self.post("/service/{}/delivery-receipt-api".format(service_id), data) + class ServicesBrowsableItem(BrowsableItem): @property diff --git a/app/templates/views/service-settings/set-inbound-api.html b/app/templates/views/api/callbacks.html similarity index 53% rename from app/templates/views/service-settings/set-inbound-api.html rename to app/templates/views/api/callbacks.html index 9530b36d5..4747e510d 100644 --- a/app/templates/views/service-settings/set-inbound-api.html +++ b/app/templates/views/api/callbacks.html @@ -3,33 +3,44 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Callback URL + Callbacks {% endblock %} {% block maincolumn_content %}
-

Callback URL for received text messages

+

Callbacks

- Text messages you receive can be forwarded to your systems with our callback feature. See our documentation on the format of the callback.

+ {% if can_receive_inbound %} + {{ textbox( + form.inbound_url, + width='1-1', + hint='Valid https URL' + ) }} + {{ textbox( + form.inbound_bearer_token, + width='2-3', + hint='At least 10 characters' + ) }} + {% endif %} {{ textbox( - form.url, - width='2-3', + form.outbound_url, + width='1-1', hint='Valid https URL' ) }} {{ textbox( - form.bearer_token, + form.outbound_bearer_token, width='2-3', hint='At least 10 characters' ) }} {{ page_footer( 'Save', - back_link=url_for('.service_settings', service_id=current_service.id), - back_link_text='Back to settings' + back_link=url_for('.api_integration', service_id=current_service.id), + back_link_text='Back to API integration' ) }}
diff --git a/app/templates/views/api/index.html b/app/templates/views/api/index.html index 1b99ff3e1..4ef0ea2cf 100644 --- a/app/templates/views/api/index.html +++ b/app/templates/views/api/index.html @@ -21,7 +21,7 @@ Whitelist
- Documentation + Callback Url
diff --git a/app/templates/views/callbacks.html b/app/templates/views/callbacks.html index 643153911..b61a58755 100644 --- a/app/templates/views/callbacks.html +++ b/app/templates/views/callbacks.html @@ -31,12 +31,14 @@ If you don’t have ‘receive text messages’ enabled for your service, get in touch and we can turn it on for you.

-

Format of the callback

+

Format of the callbacks

The format of the callback message you receive is JSON.

+

Received text messages callback

+
{% call mapping_table( caption='Callback message format', @@ -60,4 +62,32 @@ {% endcall %}
+

Status callback

+ +
+ {% call mapping_table( + caption='Callback message format', + field_headings=['Key', 'Description', 'Format'], + field_headings_visible=True, + caption_visible=False + ) %} + {% for key, description, format in [ + ('id', 'Notify’s id for the status receipts', 'UUID'), + ('reference', 'The reference sent by the service', '12345678'), + ('to', 'The email address of the recipient', 'hello@gov.uk'), + ('status', 'The status of the notification', 'delivered | permanent-failure | temporary-failure | technical-failure'), + ('created_at', 'The time the service sent the request', '2017-05-14T12:15:30.000000Z'), + ('updated_at', 'The last time the status was updated', '2017-05-14T12:15:30.000000Z'), + ('sent_at', 'The time the notification was sent', '2017-05-14T12:15:30.000000Z or nil'), + ('notification_type', 'The notification type', 'email | sms | letter') + ] %} + {% call row() %} + {% call row_heading() %} {{ key }} {% endcall %} + {{ text_field(description) }} + {{ text_field(format) }} + {% endcall %} + {% endfor %} + {% endcall %} +
+ {% endblock %} diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index c4bbb8d6e..51bf2750c 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -120,14 +120,6 @@ {{ edit_field('Change', url_for('.service_set_inbound_sms', service_id=current_service.id)) }} {% endcall %} - {% if can_receive_inbound %} - {% call row() %} - {{ text_field('Callback URL for received text messages') }} - {{ optional_text_field(inbound_api_url) }} - {{ edit_field('Change', url_for('.service_set_inbound_api', service_id=current_service.id)) }} - {% endcall %} - {% endif %} - {% endif %} {% endcall %} diff --git a/tests/__init__.py b/tests/__init__.py index 372d34f6c..262b008e5 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -53,6 +53,7 @@ def service_json( created_at=None, letter_contact_block=None, inbound_api=None, + service_callback_api=None, permissions=None, organisation_type='central', free_sms_fragment_limit=250000, @@ -84,6 +85,7 @@ def service_json( 'dvla_organisation': '001', 'permissions': permissions, 'inbound_api': inbound_api, + 'service_callback_api': service_callback_api, 'prefix_sms': prefix_sms, } diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 412e9cc69..dc1ba9c00 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -391,3 +391,267 @@ def test_should_validate_whitelist_items( assert jump_links[1]['href'] == '#phone_numbers' mock_update_whitelist.assert_not_called() + + +@pytest.mark.parametrize('url, bearer_token, expected_errors', [ + ("", "", "Can’t be empty Can’t be empty"), + ("http://not_https.com", "1234567890", "Must be a valid https URL"), + ("https://test.com", "123456789", "Must be at least 10 characters"), +]) +def test_set_outbound_api_validation( + client_request, + service_one, + url, + bearer_token, + expected_errors, + mock_create_service_callback_api, + mock_update_service_callback_api +): + response = client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data={"outbound_url": url, "outbound_bearer_token": bearer_token}, + _expected_status=200 + ) + error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) + + assert error_msgs == expected_errors + mock_create_service_callback_api.assert_not_called() + mock_update_service_callback_api.assert_not_called() + + +@pytest.mark.parametrize('url, bearer_token, expected_errors', [ + ("", "", "Can’t be empty Can’t be empty Can’t be empty Can’t be empty"), + ("http://not_https.com", "1234567890", "Must be a valid https URL Must be a valid https URL"), + ("https://test.com", "123456789", "Must be at least 10 characters Must be at least 10 characters"), +]) +def test_set_inbound_api_validation( + client_request, + service_one, + url, + bearer_token, + expected_errors, + fake_uuid +): + service_one['permissions'] = ['inbound_sms'] + + data = { + "inbound_url": url, + "inbound_bearer_token": bearer_token, + "outbound_url": url, + "outbound_bearer_token": bearer_token + } + + response = client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=data, + _expected_status=200 + ) + error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) + + assert error_msgs == expected_errors + + +def test_create_new_callback_api_without_inbound_set( + client_request, + service_one, + mock_create_service_callback_api, + mock_get_notifications, + fake_uuid, + mocker, +): + service_one['service_callback_api'] = [] + + callback_api_data = { + 'outbound_url': "https://test.url.com/", + 'outbound_bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=callback_api_data, + _follow_redirects=True, + ) + + callback_api_data['updated_by_id'] = service_one['users'][0] + + mock_create_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid + ) + + +def test_update_new_callback_api_without_inbound_set( + client_request, + service_one, + mock_get_valid_service_callback_api, + mock_update_service_callback_api, + mock_get_notifications, + fake_uuid, + mocker, +): + service_one['service_callback_api'] = [fake_uuid] + + callback_api_data = { + 'outbound_url': "https://test.url.com/", + 'outbound_bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=callback_api_data, + _follow_redirects=True, + ) + + callback_api_data['updated_by_id'] = service_one['users'][0] + + mock_update_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + callback_api_id=fake_uuid + ) + + +def test_inbound_fields_do_not_show_if_inbound_is_disabled( + client_request, + service_one, + mocker +): + response = client_request.get( + 'main.api_callbacks', + service_id=service_one['id'], + _expected_status=200 + ) + assert response.select_one('label[for="inbound_url"]') is None + assert response.select_one('label[for="inbound_bearer_token"]') is None + + +def test_create_inbound_and_outbound_apis( + client_request, + service_one, + mocker, + mock_create_service_callback_api, + mock_create_service_inbound_api, + mock_get_notifications, + fake_uuid, +): + service_one['permissions'] = ['inbound_sms'] + + callback_api_data = { + 'outbound_url': "https://test.url.com/", + 'outbound_bearer_token': '1234567890', + 'inbound_url': "https://test2.url.com/", + 'inbound_bearer_token': '5678901234', + 'user_id': fake_uuid + } + + client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=callback_api_data, + _follow_redirects=True, + ) + + mock_create_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid + ) + + mock_create_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test2.url.com/", + bearer_token="5678901234", + user_id=fake_uuid + ) + + +def test_update_inbound_and_outbound_apis( + client_request, + service_one, + mocker, + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, + mock_update_service_callback_api, + mock_update_service_inbound_api, + mock_get_notifications, + fake_uuid, +): + service_one['service_callback_api'] = [fake_uuid] + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + + callback_api_data = { + 'outbound_url': "https://test.url.com/", + 'outbound_bearer_token': '1234567890', + 'inbound_url': "https://test2.url.com/", + 'inbound_bearer_token': '5678901234', + 'user_id': fake_uuid + } + + client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=callback_api_data, + _follow_redirects=True, + ) + + mock_update_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + callback_api_id=fake_uuid + ) + + mock_update_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test2.url.com/", + bearer_token="5678901234", + user_id=fake_uuid, + inbound_api_id=fake_uuid + ) + + +def test_save_callback_apis_without_changes_does_not_update_callback_apis( + client_request, + service_one, + mocker, + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, + mock_update_service_callback_api, + mock_update_service_inbound_api, + mock_get_notifications, + fake_uuid, +): + service_one['service_callback_api'] = [fake_uuid] + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + + callback_api_data = { + 'outbound_url': "https://hello2.gov.uk", + 'outbound_bearer_token': 'bearer_token_set', + 'inbound_url': "https://hello3.gov.uk", + 'inbound_bearer_token': 'bearer_token_set', + 'user_id': fake_uuid + } + + client_request.post( + 'main.api_callbacks', + service_id=service_one['id'], + _data=callback_api_data, + _follow_redirects=True, + ) + + assert mock_update_service_callback_api.called is False + assert mock_update_service_inbound_api.called is False diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 9f79cfb8d..02d22db84 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -7,7 +7,6 @@ from flask import url_for from werkzeug.exceptions import InternalServerError import app -from app.main.views.service_settings import dummy_bearer_token from app.utils import email_safe from tests import validate_route_permission, service_json from tests.conftest import ( @@ -136,7 +135,6 @@ def test_should_show_overview( 'Text messages start with service name On Change', 'International text messages On Change', 'Receive text messages On Change', - 'Callback URL for received text messages Not set Change', 'Label Value Action', 'Send letters Off Change', @@ -186,47 +184,6 @@ def test_should_show_overview_for_service_with_more_things_set( assert row == " ".join(page.find_all('tr')[index + 1].text.split()) -@pytest.mark.parametrize('url, elided_url', [ - ('https://test.url.com/inbound', 'https://test.url.com...'), - ('https://test.url.com/', 'https://test.url.com...'), - ('https://test.url.com', 'https://test.url.com'), -]) -def test_service_settings_show_elided_api_url_if_needed( - logged_in_platform_admin_client, - service_one, - single_reply_to_email_address, - single_sms_sender, - single_letter_contact_block, - mocker, - fake_uuid, - url, - elided_url, - mock_get_service_settings_page_common, -): - service_one['permissions'] = ['sms', 'email', 'inbound_sms'] - service_one['inbound_api'] = [fake_uuid] - - mocked_get_fn = mocker.patch( - 'app.service_api_client.get', - return_value={'data': {'id': fake_uuid, 'url': url}} - ) - - response = logged_in_platform_admin_client.get( - url_for( - 'main.service_settings', - service_id=service_one['id'] - ) - ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - non_empty_trs = [tr.find_all('td') for tr in page.find_all('tr') if tr.find_all('td')] - api_url = [api_setting[1].text.strip() for api_setting in non_empty_trs - if api_setting[0].text.strip() == 'Callback URL for received text messages'][0] - assert api_url == elided_url - assert mocked_get_fn.called is True - - def test_if_cant_send_letters_then_cant_see_letter_contact_block( logged_in_client, service_one, @@ -1274,34 +1231,6 @@ def test_does_not_show_research_mode_indicator( assert not element -@pytest.mark.parametrize('url, bearer_token, expected_errors', [ - ("", "", "Can’t be empty Can’t be empty"), - ("http://not_https.com", "1234567890", "Must be a valid https URL"), - ("https://test.com", "123456789", "Must be at least 10 characters"), -]) -def test_set_inbound_api_validation( - logged_in_client, - mock_update_service, - service_one, - mock_get_letter_organisations, - url, - bearer_token, - expected_errors, -): - service_one['permissions'] = ['inbound_sms'] - response = logged_in_client.post(url_for( - 'main.service_set_inbound_api', - service_id=service_one['id']), - data={"url": url, "bearer_token": bearer_token} - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - error_msgs = ' '.join(msg.text.strip() for msg in page.select(".error-message")) - - assert response.status_code == 200 - assert error_msgs == expected_errors - assert not mock_update_service.called - - @pytest.mark.parametrize('method', ['get', 'post']) def test_cant_set_letter_contact_block_if_service_cant_send_letters( logged_in_client, @@ -1707,113 +1636,6 @@ def test_switch_service_enable_international_sms( assert mocked_fn.call_args[0][0] == service_one['id'] -def test_set_new_inbound_api_and_valid_bearer_token_calls_create_inbound_api_endpoint( - logged_in_platform_admin_client, - service_one, - mocker, -): - service_one['permissions'] = ['inbound_sms'] - service_one['inbound_api'] = [] - - mocked_post_fn = mocker.patch('app.service_api_client.post', return_value=service_one) - - inbound_api_data = {'url': "https://test.url.com/", 'bearer_token': '1234567890'} - response = logged_in_platform_admin_client.post( - url_for( - 'main.service_set_inbound_api', - service_id=service_one['id'] - ), - data=inbound_api_data - ) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - assert mocked_post_fn.called - - inbound_api_data['updated_by_id'] = service_one['users'][0] - assert mocked_post_fn.call_args == call("/service/{}/inbound-api".format(service_one['id']), inbound_api_data) - - -@pytest.mark.parametrize( - 'inbound_api_data', [ - {'url': "https://test.url.com/inbound", 'bearer_token': dummy_bearer_token}, - {'url': "https://test.url.com/inbound", 'bearer_token': '1234567890'}, - {'url': "https://test.url.com/", 'bearer_token': 'new_1234567890'}, - ] -) -def test_update_inbound_api_and_valid_bearer_token_calls_update_inbound_api_endpoint( - logged_in_platform_admin_client, - service_one, - mocker, - fake_uuid, - inbound_api_data, -): - service_one['permissions'] = ['inbound_sms'] - service_one['inbound_api'] = [fake_uuid] - - initial_api_data = {'data': {'id': fake_uuid, 'url': "https://test.url.com/"}} - - mocked_get_fn = mocker.patch('app.service_api_client.get', return_value=initial_api_data) - mocked_post_fn = mocker.patch('app.service_api_client.post', return_value=service_one) - - response = logged_in_platform_admin_client.get( - url_for( - 'main.service_set_inbound_api', - service_id=service_one['id'] - ) - ) - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert page.find('input', {'id': 'url'}).get('value') == initial_api_data['data']['url'] - assert page.find('input', {'id': 'bearer_token'}).get('value') == dummy_bearer_token - - response = logged_in_platform_admin_client.post( - url_for( - 'main.service_set_inbound_api', - service_id=service_one['id'] - ), - data=inbound_api_data - ) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - assert mocked_get_fn.called is True - assert mocked_post_fn.called is True - - if inbound_api_data['bearer_token'] == dummy_bearer_token: - del inbound_api_data['bearer_token'] - inbound_api_data['updated_by_id'] = service_one['users'][0] - - assert mocked_post_fn.call_args == call( - "/service/{}/inbound-api/{}".format(service_one['id'], fake_uuid), inbound_api_data) - - -def test_save_inbound_api_without_changes_does_not_update_inbound_api( - logged_in_platform_admin_client, - service_one, - mocker, - fake_uuid, -): - service_one['permissions'] = ['inbound_sms'] - service_one['inbound_api'] = [fake_uuid] - - initial_api_data = {'data': {'id': fake_uuid, 'url': "https://test.url.com/"}} - inbound_api_data = {'url': initial_api_data['data']['url'], 'bearer_token': dummy_bearer_token} - - mocked_get_fn = mocker.patch('app.service_api_client.get', return_value=initial_api_data) - mocked_post_fn = mocker.patch('app.service_api_client.post', return_value=service_one) - - response = logged_in_platform_admin_client.post( - url_for( - 'main.service_set_inbound_api', - service_id=service_one['id'] - ), - data=inbound_api_data - ) - assert response.status_code == 302 - assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - assert mocked_get_fn.called is True - assert mocked_post_fn.called is False - - def test_archive_service_after_confirm( logged_in_platform_admin_client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index 004e87052..88ed70fb8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2497,3 +2497,65 @@ def valid_token(app_, fake_uuid): app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT'] ) + + +@pytest.fixture(scope='function') +def mock_get_valid_service_inbound_api(mocker): + def _get(service_id, inbound_api_id): + return { + 'created_at': '2017-12-04T10:52:55.289026Z', + 'updated_by_id': fake_uuid, + 'id': inbound_api_id, + 'url': 'https://hello3.gov.uk', + 'service_id': service_id, + 'updated_at': '2017-12-04T11:28:42.575153Z' + } + + return mocker.patch('app.service_api_client.get_service_inbound_api', side_effect=_get) + + +@pytest.fixture(scope='function') +def mock_get_valid_service_callback_api(mocker): + def _get(service_id, callback_api_id): + return { + 'created_at': '2017-12-04T10:52:55.289026Z', + 'updated_by_id': fake_uuid, + 'id': callback_api_id, + 'url': 'https://hello2.gov.uk', + 'service_id': service_id, + 'updated_at': '2017-12-04T11:28:42.575153Z' + } + + return mocker.patch('app.service_api_client.get_service_callback_api', side_effect=_get) + + +@pytest.fixture(scope='function') +def mock_create_service_inbound_api(mocker): + def _create_service_inbound_api(service_id, url, bearer_token, user_id): + return + + return mocker.patch('app.service_api_client.create_service_inbound_api', side_effect=_create_service_inbound_api) + + +@pytest.fixture(scope='function') +def mock_update_service_inbound_api(mocker): + def _update_service_inbound_api(service_id, url, bearer_token, user_id, inbound_api_id): + return + + return mocker.patch('app.service_api_client.update_service_inbound_api', side_effect=_update_service_inbound_api) + + +@pytest.fixture(scope='function') +def mock_create_service_callback_api(mocker): + def _create_service_callback_api(service_id, url, bearer_token, user_id): + return + + return mocker.patch('app.service_api_client.create_service_callback_api', side_effect=_create_service_callback_api) + + +@pytest.fixture(scope='function') +def mock_update_service_callback_api(mocker): + def _update_service_callback_api(service_id, url, bearer_token, user_id, callback_api_id): + return + + return mocker.patch('app.service_api_client.update_service_callback_api', side_effect=_update_service_callback_api) From df383b5c0f2e6a6a606ba2441b90f23c64a4ca3d Mon Sep 17 00:00:00 2001 From: chrisw Date: Thu, 7 Dec 2017 13:20:33 +0000 Subject: [PATCH 5/7] renamed api_keys file --- app/main/__init__.py | 2 +- app/main/views/{api_keys.py => api_integration.py} | 0 .../main/views/{test_api_keys.py => test_api_integration.py} | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename app/main/views/{api_keys.py => api_integration.py} (100%) rename tests/app/main/views/{test_api_keys.py => test_api_integration.py} (100%) diff --git a/app/main/__init__.py b/app/main/__init__.py index 042aa1e3d..05c023144 100644 --- a/app/main/__init__.py +++ b/app/main/__init__.py @@ -21,7 +21,7 @@ from app.main.views import ( # noqa styleguide, user_profile, choose_service, - api_keys, + api_integration, manage_users, invites, feedback, diff --git a/app/main/views/api_keys.py b/app/main/views/api_integration.py similarity index 100% rename from app/main/views/api_keys.py rename to app/main/views/api_integration.py diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_integration.py similarity index 100% rename from tests/app/main/views/test_api_keys.py rename to tests/app/main/views/test_api_integration.py From cd3d8edccd8fb1b1618728d91fd4dc6e3416eeda Mon Sep 17 00:00:00 2001 From: chrisw Date: Fri, 8 Dec 2017 10:52:18 +0000 Subject: [PATCH 6/7] Allowed custom attributes to be added to textbox component --- app/templates/components/textbox.html | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/templates/components/textbox.html b/app/templates/components/textbox.html index f895dfde6..9d032d1ca 100644 --- a/app/templates/components/textbox.html +++ b/app/templates/components/textbox.html @@ -41,11 +41,12 @@ ' form-control-error' if field.errors else '' ) %} - {{ field(**{ - 'class': field_class, - 'data-module': 'highlight-tags' if highlight_tags else '', - 'rows': rows|string - }) }} + {{ field( + class=field_class, + data_module='highlight-tags' if highlight_tags else '', + rows=rows|string, + **kwargs + ) }} {% if suffix %} {{ suffix }} {% endif %} From 9c0d2c2a39c5e6fa48ae34b78d7bc324aaa861f9 Mon Sep 17 00:00:00 2001 From: chrisw Date: Fri, 8 Dec 2017 10:52:38 +0000 Subject: [PATCH 7/7] refactor of api callbacks admin work --- app/main/forms.py | 38 +- app/main/views/api_integration.py | 137 +++-- app/templates/views/api/callbacks.html | 60 +-- .../callbacks/delivery-status-callback.html | 39 ++ .../received-text-messages-callback.html | 39 ++ app/templates/views/api/index.html | 2 +- app/templates/views/callbacks.html | 40 +- tests/app/main/views/test_api_integration.py | 476 +++++++++--------- 8 files changed, 469 insertions(+), 362 deletions(-) create mode 100644 app/templates/views/api/callbacks/delivery-status-callback.html create mode 100644 app/templates/views/api/callbacks/received-text-messages-callback.html diff --git a/app/main/forms.py b/app/main/forms.py index 47e695ca8..614698989 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -717,38 +717,30 @@ class ServiceInboundNumberForm(Form): ) -class ServiceCallbacksForm(Form): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self.can_receive_inbound = kwargs['can_receive_inbound'] - - inbound_url = StringField("Inbound URL") - inbound_bearer_token = PasswordFieldShowHasContent("Bearer token") - outbound_url = StringField( - "Outbound URL", +class ServiceReceiveMessagesCallbackForm(Form): + url = StringField( + "URL", validators=[DataRequired(message='Can’t be empty'), Regexp(regex="^https.*", message='Must be a valid https URL')] ) - outbound_bearer_token = PasswordFieldShowHasContent( + bearer_token = PasswordFieldShowHasContent( "Bearer token", validators=[DataRequired(message='Can’t be empty'), Length(min=10, message='Must be at least 10 characters')] ) - def validate_inbound_url(self, field): - pattern = re.compile("^https.*") - if self.can_receive_inbound: - if not field.data: - raise ValidationError('Can’t be empty') - elif not pattern.match(field.data): - raise ValidationError('Must be a valid https URL') - def validate_inbound_bearer_token(self, field): - if self.can_receive_inbound: - if not field.data: - raise ValidationError('Can’t be empty') - elif len(field.data) < 10: - raise ValidationError('Must be at least 10 characters') +class ServiceDeliveryStatusCallbackForm(Form): + url = StringField( + "URL", + validators=[DataRequired(message='Can’t be empty'), + Regexp(regex="^https.*", message='Must be a valid https URL')] + ) + bearer_token = PasswordFieldShowHasContent( + "Bearer token", + validators=[DataRequired(message='Can’t be empty'), + Length(min=10, message='Must be at least 10 characters')] + ) class InternationalSMSForm(Form): diff --git a/app/main/views/api_integration.py b/app/main/views/api_integration.py index 98e886ca6..799892c5d 100644 --- a/app/main/views/api_integration.py +++ b/app/main/views/api_integration.py @@ -1,7 +1,12 @@ from flask import request, render_template, redirect, url_for, flash, Markup, abort from flask_login import login_required, current_user from app.main import main -from app.main.forms import CreateKeyForm, Whitelist, ServiceCallbacksForm +from app.main.forms import ( + CreateKeyForm, + Whitelist, + ServiceReceiveMessagesCallbackForm, + ServiceDeliveryStatusCallbackForm +) from app import api_key_api_client, service_api_client, notification_api_client, current_service from app.utils import user_has_permissions, email_safe from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM @@ -13,8 +18,13 @@ dummy_bearer_token = 'bearer_token_set' @login_required @user_has_permissions('manage_api_keys', admin_override=True) def api_integration(service_id): + callbacks_link = ( + '.api_callbacks' if 'inbound_sms' in current_service['permissions'] + else '.delivery_status_callback' + ) return render_template( 'views/api/index.html', + callbacks_link=callbacks_link, api_notifications=notification_api_client.get_api_notifications_for_service(service_id) ) @@ -144,58 +154,107 @@ def check_token_against_dummy_bearer(token): @main.route("/services//api/callbacks", methods=['GET', 'POST']) @login_required def api_callbacks(service_id): - callback_api, inbound_api = get_apis() - can_receive_inbound = 'inbound_sms' in current_service['permissions'] + if 'inbound_sms' not in current_service['permissions']: + return redirect(url_for('.delivery_status_callback', service_id=service_id)) - form = ServiceCallbacksForm( - inbound_url=inbound_api.get('url') if inbound_api else '', - inbound_bearer_token=dummy_bearer_token if inbound_api else '', - outbound_url=callback_api.get('url') if callback_api else '', - outbound_bearer_token=dummy_bearer_token if callback_api else '', - can_receive_inbound=can_receive_inbound, + received_text_messages_callback, delivery_status_callback = get_apis() + + return render_template( + 'views/api/callbacks.html', + received_text_messages_callback=received_text_messages_callback.get('url', "Not Set"), + delivery_status_callback=delivery_status_callback.get('url', "Not Set") + ) + + +def get_delivery_status_callback_details(): + if current_service['service_callback_api']: + return service_api_client.get_service_callback_api( + current_service['id'], + current_service.get('service_callback_api')[0] + ) + + +@main.route("/services//api/callbacks/delivery-status-callback", methods=['GET', 'POST']) +@login_required +def delivery_status_callback(service_id): + delivery_status_callback = get_delivery_status_callback_details() + back_link = ( + '.api_callbacks' if 'inbound_sms' in current_service['permissions'] + else '.api_integration' + ) + + form = ServiceDeliveryStatusCallbackForm( + url=delivery_status_callback.get('url') if delivery_status_callback else '', + bearer_token=dummy_bearer_token if delivery_status_callback else '' ) if form.validate_on_submit(): - if callback_api: - if (callback_api.get('url') != form.outbound_url.data - or form.outbound_bearer_token.data != dummy_bearer_token): + if delivery_status_callback: + if (delivery_status_callback.get('url') != form.url.data + or form.bearer_token.data != dummy_bearer_token): service_api_client.update_service_callback_api( service_id, - url=form.outbound_url.data, - bearer_token=check_token_against_dummy_bearer(form.outbound_bearer_token.data), + url=form.url.data, + bearer_token=check_token_against_dummy_bearer(form.bearer_token.data), user_id=current_user.id, - callback_api_id=callback_api.get('id') + callback_api_id=delivery_status_callback.get('id') ) else: service_api_client.create_service_callback_api( service_id, - url=form.outbound_url.data, - bearer_token=form.outbound_bearer_token.data, + url=form.url.data, + bearer_token=form.bearer_token.data, user_id=current_user.id ) - if can_receive_inbound: - if inbound_api: - if (inbound_api.get('url') != form.inbound_url.data - or form.inbound_bearer_token.data != dummy_bearer_token): - service_api_client.update_service_inbound_api( - service_id, - url=form.inbound_url.data, - bearer_token=check_token_against_dummy_bearer(form.inbound_bearer_token.data), - user_id=current_user.id, - inbound_api_id=inbound_api.get('id') - ) - else: - service_api_client.create_service_inbound_api( - service_id, - url=form.inbound_url.data, - bearer_token=form.inbound_bearer_token.data, - user_id=current_user.id - ) - - return redirect(url_for('.api_integration', service_id=service_id)) + return redirect(url_for(back_link, service_id=service_id)) return render_template( - 'views/api/callbacks.html', + 'views/api/callbacks/delivery-status-callback.html', + back_link=back_link, + form=form, + ) + + +def get_received_text_messages_callback(): + if current_service['inbound_api']: + return service_api_client.get_service_inbound_api( + current_service['id'], + current_service.get('inbound_api')[0] + ) + + +@main.route("/services//api/callbacks/received-text-messages-callback", methods=['GET', 'POST']) +@login_required +def received_text_messages_callback(service_id): + if 'inbound_sms' not in current_service['permissions']: + return redirect(url_for('.api_integration', service_id=service_id)) + + received_text_messages_callback = get_received_text_messages_callback() + form = ServiceReceiveMessagesCallbackForm( + url=received_text_messages_callback.get('url') if received_text_messages_callback else '', + bearer_token=dummy_bearer_token if received_text_messages_callback else '' + ) + + if form.validate_on_submit(): + if received_text_messages_callback: + if (received_text_messages_callback.get('url') != form.url.data + or form.bearer_token.data != dummy_bearer_token): + service_api_client.update_service_inbound_api( + service_id, + url=form.url.data, + bearer_token=check_token_against_dummy_bearer(form.bearer_token.data), + user_id=current_user.id, + inbound_api_id=received_text_messages_callback.get('id') + ) + else: + service_api_client.create_service_inbound_api( + service_id, + url=form.url.data, + bearer_token=form.bearer_token.data, + user_id=current_user.id + ) + return redirect(url_for('.api_callbacks', service_id=service_id)) + return render_template( + 'views/api/callbacks/received-text-messages-callback.html', form=form, - can_receive_inbound=can_receive_inbound, ) diff --git a/app/templates/views/api/callbacks.html b/app/templates/views/api/callbacks.html index 4747e510d..fb0ac84ab 100644 --- a/app/templates/views/api/callbacks.html +++ b/app/templates/views/api/callbacks.html @@ -1,49 +1,33 @@ {% extends "withnav_template.html" %} {% from "components/textbox.html" import textbox %} {% from "components/page-footer.html" import page_footer %} +{% from "components/table.html" import mapping_table, row, text_field, edit_field %} + {% block service_page_title %} Callbacks {% endblock %} {% block maincolumn_content %} -
-
-

Callbacks

-

- See our documentation on the format of the callback. -

- -
- {% if can_receive_inbound %} - {{ textbox( - form.inbound_url, - width='1-1', - hint='Valid https URL' - ) }} - {{ textbox( - form.inbound_bearer_token, - width='2-3', - hint='At least 10 characters' - ) }} - {% endif %} - {{ textbox( - form.outbound_url, - width='1-1', - hint='Valid https URL' - ) }} - {{ textbox( - form.outbound_bearer_token, - width='2-3', - hint='At least 10 characters' - ) }} - {{ page_footer( - 'Save', - back_link=url_for('.api_integration', service_id=current_service.id), - back_link_text='Back to API integration' - ) }} -
-
-
+

Callbacks

+
+ {% call mapping_table( + caption='General', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=False + ) %} + {% call row() %} + {{ text_field('Delivery status callback URL') }} + {{ text_field(received_text_messages_callback) }} + {{ edit_field('Change', url_for('.delivery_status_callback', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} + {{ text_field('Received text messages callback URL') }} + {{ text_field(delivery_status_callback) }} + {{ edit_field('Change', url_for('.received_text_messages_callback', service_id=current_service.id)) }} + {% endcall %} + {% endcall %} +
{% endblock %} diff --git a/app/templates/views/api/callbacks/delivery-status-callback.html b/app/templates/views/api/callbacks/delivery-status-callback.html new file mode 100644 index 000000000..b81effaad --- /dev/null +++ b/app/templates/views/api/callbacks/delivery-status-callback.html @@ -0,0 +1,39 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Callback for delivery receipts +{% endblock %} + +{% block maincolumn_content %} +
+
+

Callback for delivery receipts

+

+ When you send an email or text message, we can tell you if Notify was able to deliver it. + Check the callback documentation for more information. +

+ +
+ {{ textbox( + form.url, + width='2-3', + hint='Valid https URL' + ) }} + {{ textbox( + form.bearer_token, + width='2-3', + hint='At least 10 characters', + autocomplete='new-password' + ) }} + {{ page_footer( + 'Save', + back_link=url_for(back_link, service_id=current_service.id), + back_link_text='Back to settings' + ) }} +
+
+
+ +{% endblock %} diff --git a/app/templates/views/api/callbacks/received-text-messages-callback.html b/app/templates/views/api/callbacks/received-text-messages-callback.html new file mode 100644 index 000000000..423f55fc3 --- /dev/null +++ b/app/templates/views/api/callbacks/received-text-messages-callback.html @@ -0,0 +1,39 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Callback for received text messages +{% endblock %} + +{% block maincolumn_content %} +
+
+

Callback for received text messages

+

+ When you receive a text message in Notify, we can forward it to your system. + Check the callback documentation for more information. +

+ +
+ {{ textbox( + form.url, + width='2-3', + hint='Valid https URL' + ) }} + {{ textbox( + form.bearer_token, + width='2-3', + hint='At least 10 characters', + autocomplete='new-password' + ) }} + {{ page_footer( + 'Save', + back_link=url_for('.api_callbacks', service_id=current_service.id), + back_link_text='Back to settings' + ) }} +
+
+
+ +{% endblock %} diff --git a/app/templates/views/api/index.html b/app/templates/views/api/index.html index 4ef0ea2cf..195f6a62f 100644 --- a/app/templates/views/api/index.html +++ b/app/templates/views/api/index.html @@ -21,7 +21,7 @@ Whitelist
- Callback Url + Callbacks
diff --git a/app/templates/views/callbacks.html b/app/templates/views/callbacks.html index b61a58755..107a01bcf 100644 --- a/app/templates/views/callbacks.html +++ b/app/templates/views/callbacks.html @@ -9,36 +9,14 @@
+

Callbacks for received text messages

+

A callback lets you receive messages from Notify to a URL you choose.

+

You’ll need to provide a bearer token, for security. We’ll add this to the authorisation header of the callback request.

+

The callback message is in JSON.

-

- Text messages you receive can be forwarded to a URL that you specify, using our callback feature. -

- -

- Messages are forwarded as they are received. -

- -

- To protect your service, we require you to provide a bearer token. We put this token in the authorisation header of the callback requests. -

- -

- Once you have ‘receive text messages’ enabled, you can set up your callback on the settings page of your service. -

- -

- If you don’t have ‘receive text messages’ enabled for your service, get in touch and we can turn it on for you. -

- -

Format of the callbacks

- -

- The format of the callback message you receive is JSON. -

- -

Received text messages callback

- +

Text messages you receive

+

If your service receives text messages in Notify, we can forward them to your callback URL as soon as they arrive.

{% call mapping_table( caption='Callback message format', @@ -62,8 +40,8 @@ {% endcall %}
-

Status callback

- +

Email and text message delivery receipts

+

When you send an email or text message through Notify, we can send a receipt to your callback URL to tell you if we were able to deliver it or not.

{% call mapping_table( caption='Callback message format', @@ -77,7 +55,7 @@ ('to', 'The email address of the recipient', 'hello@gov.uk'), ('status', 'The status of the notification', 'delivered | permanent-failure | temporary-failure | technical-failure'), ('created_at', 'The time the service sent the request', '2017-05-14T12:15:30.000000Z'), - ('updated_at', 'The last time the status was updated', '2017-05-14T12:15:30.000000Z'), + ('completed_at', 'The last time the status was updated', '2017-05-14T12:15:30.000000Z'), ('sent_at', 'The time the notification was sent', '2017-05-14T12:15:30.000000Z or nil'), ('notification_type', 'The notification type', 'email | sms | letter') ] %} diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index dc1ba9c00..d594419c1 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -13,6 +13,8 @@ from tests.conftest import ( mock_get_service_with_letters, normalize_spaces, SERVICE_ONE_ID, + mock_get_valid_service_callback_api, + mock_get_valid_service_inbound_api, ) @@ -393,236 +395,6 @@ def test_should_validate_whitelist_items( mock_update_whitelist.assert_not_called() -@pytest.mark.parametrize('url, bearer_token, expected_errors', [ - ("", "", "Can’t be empty Can’t be empty"), - ("http://not_https.com", "1234567890", "Must be a valid https URL"), - ("https://test.com", "123456789", "Must be at least 10 characters"), -]) -def test_set_outbound_api_validation( - client_request, - service_one, - url, - bearer_token, - expected_errors, - mock_create_service_callback_api, - mock_update_service_callback_api -): - response = client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data={"outbound_url": url, "outbound_bearer_token": bearer_token}, - _expected_status=200 - ) - error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) - - assert error_msgs == expected_errors - mock_create_service_callback_api.assert_not_called() - mock_update_service_callback_api.assert_not_called() - - -@pytest.mark.parametrize('url, bearer_token, expected_errors', [ - ("", "", "Can’t be empty Can’t be empty Can’t be empty Can’t be empty"), - ("http://not_https.com", "1234567890", "Must be a valid https URL Must be a valid https URL"), - ("https://test.com", "123456789", "Must be at least 10 characters Must be at least 10 characters"), -]) -def test_set_inbound_api_validation( - client_request, - service_one, - url, - bearer_token, - expected_errors, - fake_uuid -): - service_one['permissions'] = ['inbound_sms'] - - data = { - "inbound_url": url, - "inbound_bearer_token": bearer_token, - "outbound_url": url, - "outbound_bearer_token": bearer_token - } - - response = client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data=data, - _expected_status=200 - ) - error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) - - assert error_msgs == expected_errors - - -def test_create_new_callback_api_without_inbound_set( - client_request, - service_one, - mock_create_service_callback_api, - mock_get_notifications, - fake_uuid, - mocker, -): - service_one['service_callback_api'] = [] - - callback_api_data = { - 'outbound_url': "https://test.url.com/", - 'outbound_bearer_token': '1234567890', - 'user_id': fake_uuid - } - - client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data=callback_api_data, - _follow_redirects=True, - ) - - callback_api_data['updated_by_id'] = service_one['users'][0] - - mock_create_service_callback_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid - ) - - -def test_update_new_callback_api_without_inbound_set( - client_request, - service_one, - mock_get_valid_service_callback_api, - mock_update_service_callback_api, - mock_get_notifications, - fake_uuid, - mocker, -): - service_one['service_callback_api'] = [fake_uuid] - - callback_api_data = { - 'outbound_url': "https://test.url.com/", - 'outbound_bearer_token': '1234567890', - 'user_id': fake_uuid - } - - client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data=callback_api_data, - _follow_redirects=True, - ) - - callback_api_data['updated_by_id'] = service_one['users'][0] - - mock_update_service_callback_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid, - callback_api_id=fake_uuid - ) - - -def test_inbound_fields_do_not_show_if_inbound_is_disabled( - client_request, - service_one, - mocker -): - response = client_request.get( - 'main.api_callbacks', - service_id=service_one['id'], - _expected_status=200 - ) - assert response.select_one('label[for="inbound_url"]') is None - assert response.select_one('label[for="inbound_bearer_token"]') is None - - -def test_create_inbound_and_outbound_apis( - client_request, - service_one, - mocker, - mock_create_service_callback_api, - mock_create_service_inbound_api, - mock_get_notifications, - fake_uuid, -): - service_one['permissions'] = ['inbound_sms'] - - callback_api_data = { - 'outbound_url': "https://test.url.com/", - 'outbound_bearer_token': '1234567890', - 'inbound_url': "https://test2.url.com/", - 'inbound_bearer_token': '5678901234', - 'user_id': fake_uuid - } - - client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data=callback_api_data, - _follow_redirects=True, - ) - - mock_create_service_callback_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid - ) - - mock_create_service_inbound_api.assert_called_once_with( - service_one['id'], - url="https://test2.url.com/", - bearer_token="5678901234", - user_id=fake_uuid - ) - - -def test_update_inbound_and_outbound_apis( - client_request, - service_one, - mocker, - mock_get_valid_service_callback_api, - mock_get_valid_service_inbound_api, - mock_update_service_callback_api, - mock_update_service_inbound_api, - mock_get_notifications, - fake_uuid, -): - service_one['service_callback_api'] = [fake_uuid] - service_one['inbound_api'] = [fake_uuid] - service_one['permissions'] = ['inbound_sms'] - - callback_api_data = { - 'outbound_url': "https://test.url.com/", - 'outbound_bearer_token': '1234567890', - 'inbound_url': "https://test2.url.com/", - 'inbound_bearer_token': '5678901234', - 'user_id': fake_uuid - } - - client_request.post( - 'main.api_callbacks', - service_id=service_one['id'], - _data=callback_api_data, - _follow_redirects=True, - ) - - mock_update_service_callback_api.assert_called_once_with( - service_one['id'], - url="https://test.url.com/", - bearer_token="1234567890", - user_id=fake_uuid, - callback_api_id=fake_uuid - ) - - mock_update_service_inbound_api.assert_called_once_with( - service_one['id'], - url="https://test2.url.com/", - bearer_token="5678901234", - user_id=fake_uuid, - inbound_api_id=fake_uuid - ) - - def test_save_callback_apis_without_changes_does_not_update_callback_apis( client_request, service_one, @@ -655,3 +427,247 @@ def test_save_callback_apis_without_changes_does_not_update_callback_apis( assert mock_update_service_callback_api.called is False assert mock_update_service_inbound_api.called is False + + +@pytest.mark.parametrize('endpoint', [ + ('main.delivery_status_callback'), + ('main.received_text_messages_callback'), +]) +@pytest.mark.parametrize('url, bearer_token, expected_errors', [ + ("", "", "Can’t be empty Can’t be empty"), + ("http://not_https.com", "1234567890", "Must be a valid https URL"), + ("https://test.com", "123456789", "Must be at least 10 characters"), +]) +def test_callback_forms_validation( + client_request, + service_one, + endpoint, + url, + bearer_token, + expected_errors +): + if endpoint == 'main.received_text_messages_callback': + service_one['permissions'] = ['inbound_sms'] + + data = { + "url": url, + "bearer_token": bearer_token, + } + + response = client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + _expected_status=200 + ) + error_msgs = ' '.join(msg.text.strip() for msg in response.select(".error-message")) + + assert error_msgs == expected_errors + + +@pytest.mark.parametrize('has_inbound_sms, expected_link', [ + (True, 'main.api_callbacks'), + (False, 'main.delivery_status_callback'), +]) +def test_callbacks_button_links_straight_to_delivery_status_if_service_has_no_inbound_sms( + client_request, + service_one, + mocker, + mock_get_notifications, + has_inbound_sms, + expected_link +): + if has_inbound_sms: + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get( + 'main.api_integration', + service_id=service_one['id'], + ) + + assert page.select('.pill-separate-item')[2]['href'] == url_for( + expected_link, service_id=service_one['id'] + ) + + +def test_callbacks_page_redirects_to_delivery_status_if_service_has_no_inbound_sms( + client_request, + service_one, + mocker +): + page = client_request.get( + 'main.api_callbacks', + service_id=service_one['id'], + _follow_redirects=True, + ) + + assert normalize_spaces(page.select_one('h1').text) == "Callback for delivery receipts" + + +@pytest.mark.parametrize('has_inbound_sms, expected_link', [ + (True, 'main.api_callbacks'), + (False, 'main.api_integration'), +]) +def test_back_link_directs_to_api_integration_from_delivery_callback_if_no_inbound_sms( + client_request, + service_one, + mocker, + has_inbound_sms, + expected_link +): + if has_inbound_sms: + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get( + 'main.delivery_status_callback', + service_id=service_one['id'], + _follow_redirects=True, + ) + + assert page.select_one('.page-footer-back-link')['href'] == url_for( + expected_link, service_id=service_one['id'] + ) + + +@pytest.mark.parametrize('endpoint', [ + ('main.delivery_status_callback'), + ('main.received_text_messages_callback'), +]) +def test_create_delivery_status_and_receive_text_message_callbacks( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_create_service_inbound_api, + mock_create_service_callback_api, + endpoint, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['permissions'] = ['inbound_sms'] + + data = { + 'url': "https://test.url.com/", + 'bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + mock_create_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + ) + else: + mock_create_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + ) + + +@pytest.mark.parametrize('endpoint, fixture', [ + ('main.delivery_status_callback', mock_get_valid_service_callback_api), + ('main.received_text_messages_callback', mock_get_valid_service_inbound_api), +]) +def test_update_delivery_status_and_receive_text_message_callbacks( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_update_service_inbound_api, + mock_update_service_callback_api, + endpoint, + fixture, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + else: + service_one['service_callback_api'] = [fake_uuid] + + fixture(mocker) + + data = { + 'url': "https://test.url.com/", + 'bearer_token': '1234567890', + 'user_id': fake_uuid + } + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + mock_update_service_inbound_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + inbound_api_id=fake_uuid, + ) + else: + mock_update_service_callback_api.assert_called_once_with( + service_one['id'], + url="https://test.url.com/", + bearer_token="1234567890", + user_id=fake_uuid, + callback_api_id=fake_uuid + ) + + +@pytest.mark.parametrize('endpoint, data, fixture', [ + ( + 'main.delivery_status_callback', + {"url": "https://hello2.gov.uk", "bearer_token": "bearer_token_set"}, + mock_get_valid_service_callback_api + ), + ( + 'main.received_text_messages_callback', + {"url": "https://hello3.gov.uk", "bearer_token": "bearer_token_set"}, + mock_get_valid_service_inbound_api + ), +]) +def test_update_delivery_status_and_receive_text_message_callbacks_without_changes_do_not_update( + client_request, + service_one, + mocker, + mock_get_notifications, + mock_update_service_callback_api, + mock_update_service_inbound_api, + data, + fixture, + endpoint, + fake_uuid, +): + if endpoint == 'main.received_text_messages_callback': + service_one['inbound_api'] = [fake_uuid] + service_one['permissions'] = ['inbound_sms'] + else: + service_one['service_callback_api'] = [fake_uuid] + + fixture(mocker) + + data['user_id'] = fake_uuid + + client_request.post( + endpoint, + service_id=service_one['id'], + _data=data, + ) + + if endpoint == 'main.received_text_messages_callback': + assert mock_update_service_inbound_api.called is False + else: + assert mock_update_service_callback_api.called is False