diff --git a/Makefile b/Makefile index bebfe42280..8833fd26e4 100644 --- a/Makefile +++ b/Makefile @@ -58,6 +58,7 @@ run-dev: .PHONY: format format: ruff check --select I --fix . + ruff check ruff format . mypy ./ npx prettier --write app/assets/javascripts app/assets/stylesheets tests_cypress/cypress/e2e diff --git a/app/main/forms.py b/app/main/forms.py index 6effcd55b6..809a1f3b67 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -23,6 +23,7 @@ SelectField, SelectMultipleField, StringField, + SubmitField, TextAreaField, ValidationError, validators, @@ -49,6 +50,7 @@ LettersNumbersAndFullStopsOnly, NoCommasInPlaceHolders, OnlySMSCharacters, + ValidCallbackUrl, ValidEmail, ValidGovEmail, validate_email_from, @@ -342,7 +344,8 @@ def bind_field(self, form, unbound_field, options): filters = [strip_whitespace] if not issubclass(unbound_field.field_class, no_filter_fields) else [] filters += unbound_field.kwargs.get("filters", []) bound = unbound_field.bind(form=form, filters=filters, **options) - bound.get_form = weakref.ref(form) # GC won't collect the form if we don't use a weakref + # GC won't collect the form if we don't use a weakref + bound.get_form = weakref.ref(form) return bound @@ -1407,7 +1410,7 @@ def __init__(self, *args, **kwargs): class CallbackForm(StripWhitespaceForm): def validate(self, extra_validators=None): - return super().validate(extra_validators) or self.url.data == "" + return super().validate(extra_validators) class ServiceReceiveMessagesCallbackForm(CallbackForm): @@ -1415,7 +1418,8 @@ class ServiceReceiveMessagesCallbackForm(CallbackForm): "URL", validators=[ DataRequired(message=_l("This cannot be empty")), - Regexp(regex="^https.*", message=_l("Must be a valid https URL")), + Regexp(regex="^https.*", message=_l("Enter a URL that starts with https://")), + ValidCallbackUrl(), ], ) bearer_token = PasswordFieldShowHasContent( @@ -1432,7 +1436,8 @@ class ServiceDeliveryStatusCallbackForm(CallbackForm): "URL", validators=[ DataRequired(message=_l("This cannot be empty")), - Regexp(regex="^https.*", message=_l("Must be a valid https URL")), + Regexp(regex="^https.*", message=_l("Enter a URL that starts with https://")), + ValidCallbackUrl(), ], ) bearer_token = PasswordFieldShowHasContent( @@ -1442,6 +1447,7 @@ class ServiceDeliveryStatusCallbackForm(CallbackForm): Length(min=10, message=_l("Must be at least 10 characters")), ], ) + test_response_time = SubmitField() class InternationalSMSForm(StripWhitespaceForm): @@ -1885,7 +1891,8 @@ class BrandingPoolForm(StripWhitespaceForm): pool_branding = RadioField( _l("Select alternate logo"), - choices=[], # Choices by default, override to get more refined options. + # Choices by default, override to get more refined options. + choices=[], validators=[DataRequired(message=_l("You must select an option to continue"))], ) diff --git a/app/main/validators.py b/app/main/validators.py index 238e68cc91..77ad8365b4 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -2,7 +2,9 @@ import time import pwnedpasswords -from flask import current_app +import requests +import validators +from flask import current_app, g from flask_babel import _ from flask_babel import lazy_gettext as _l from notifications_utils.field import Field @@ -11,7 +13,7 @@ from wtforms import ValidationError from wtforms.validators import Email -from app import formatted_list, service_api_client +from app import current_service, formatted_list, service_api_client from app.main._blocked_passwords import blocked_passwords from app.utils import Spreadsheet, email_safe, email_safe_name, is_gov_user @@ -25,7 +27,8 @@ def __init__(self, message=None): def __call__(self, form, field): if current_app.config.get("HIPB_ENABLED", None): hibp_bad_password_found = False - for i in range(0, 3): # Try 3 times. If the HIPB API is down then fall back to the old banlist. + # Try 3 times. If the HIPB API is down then fall back to the old banlist. + for i in range(0, 3): try: response = pwnedpasswords.check(field.data) if response > 0: @@ -141,6 +144,57 @@ def __call__(self, form, field): raise ValidationError(self.message) +class ValidCallbackUrl: + def __init__(self, message="Enter a URL that starts with https://"): + self.message = message + + def __call__(self, form, field): + if field.data: + validate_callback_url(field.data, form.bearer_token.data) + + +def validate_callback_url(service_callback_url, bearer_token): + """Validates a callback URL, checking that it is https and by sending a POST request to the URL with a health_check parameter. + 4xx responses are considered invalid. 5xx responses are considered valid as it indicates there is at least a service running + at the URL, and we are sending a payload that the service will not understand. + + Args: + service_callback_url (str): The url to validate. + bearer_token (str): The bearer token to use in the request, specified by the user requesting callbacks. + + Raises: + ValidationError: If the URL is not HTTPS or the http response is 4xx. + """ + if not validators.url(service_callback_url): + current_app.logger.warning( + f"Unable to create callback for service: {current_service.id}. Error: Invalid callback URL format: URL: {service_callback_url}" + ) + raise ValidationError(_l("Enter a URL that starts with https://")) + + try: + response = requests.post( + url=service_callback_url, + allow_redirects=True, + data={"health_check": "true"}, + headers={"Content-Type": "application/json", "Authorization": f"Bearer {bearer_token}"}, + timeout=2, + ) + + g.callback_response_time = response.elapsed.total_seconds() + + if response.status_code < 500 and response.status_code >= 400: + current_app.logger.warning( + f"Unable to create callback for service: {current_service.id} Error: Callback URL not reachable URL: {service_callback_url}" + ) + raise ValidationError(_l("Check your service is running and not using a proxy we cannot access")) + + except requests.RequestException as e: + current_app.logger.warning( + f"Unable to create callback for service: {current_service.id} Error: Callback URL not reachable URL: {service_callback_url} Exception: {e}" + ) + raise ValidationError(_l("Check your service is running and not using a proxy we cannot access")) + + def validate_email_from(form, field): if email_safe(field.data) != field.data.lower(): # fix their data instead of only warning them diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index d3c9a35277..b50286dce2 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -1,4 +1,4 @@ -from flask import Markup, abort, flash, redirect, render_template, request, url_for +from flask import Markup, abort, flash, g, redirect, render_template, request, url_for from flask_babel import lazy_gettext as _l from flask_login import current_user @@ -28,11 +28,13 @@ @main.route("/services//api") @user_has_permissions("manage_api_keys") def api_integration(service_id): - callbacks_link = ".api_callbacks" if current_service.has_permission("inbound_sms") else ".delivery_status_callback" + callbacks_link = ".api_callbacks" if current_service.has_permission( + "inbound_sms") 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), + api_notifications=notification_api_client.get_api_notifications_for_service( + service_id), ) @@ -87,7 +89,8 @@ def create_api_key(service_id): disabled_options, option_hints = [], {} if current_service.trial_mode: disabled_options = [KEY_TYPE_NORMAL] - option_hints[KEY_TYPE_NORMAL] = Markup(_l("Not available because your service is in trial mode.")) + option_hints[KEY_TYPE_NORMAL] = Markup( + _l("Not available because your service is in trial mode.")) if current_service.has_permission("letter"): option_hints[KEY_TYPE_TEAM] = "" if form.validate_on_submit(): @@ -119,7 +122,8 @@ def revoke_api_key(service_id, key_id): if request.method == "GET": flash( [ - "{} ‘{}’?".format(_l("Are you sure you want to revoke"), key_name), + "{} ‘{}’?".format( + _l("Are you sure you want to revoke"), key_name), _l("You will not be able to use this API key to connect to GC Notify"), ], "revoke this API key", @@ -137,9 +141,11 @@ 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.service_callback_api[0]) + callback_api = service_api_client.get_service_callback_api( + current_service.id, current_service.service_callback_api[0]) if current_service.inbound_api: - inbound_api = service_api_client.get_service_inbound_api(current_service.id, current_service.inbound_api[0]) + inbound_api = service_api_client.get_service_inbound_api( + current_service.id, current_service.inbound_api[0]) return (callback_api, inbound_api) @@ -161,7 +167,8 @@ def api_callbacks(service_id): return render_template( "views/api/callbacks.html", - received_text_messages_callback=received_text_messages_callback["url"] if received_text_messages_callback else None, + received_text_messages_callback=received_text_messages_callback[ + "url"] if received_text_messages_callback else None, delivery_status_callback=delivery_status_callback["url"] if delivery_status_callback else None, ) @@ -171,6 +178,44 @@ def get_delivery_status_callback_details(): return service_api_client.get_service_callback_api(current_service.id, current_service.service_callback_api[0]) +@main.route("/services//api/callbacks/delivery-status-callback/delete", methods=["GET", "POST"]) +@user_has_permissions("manage_api_keys") +def delete_delivery_status_callback(service_id): + delivery_status_callback = get_delivery_status_callback_details() + back_link = ".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".api_integration" + url_hint_txt = "Must start with https://" + + if request.method == "POST": + if delivery_status_callback: + service_api_client.delete_service_callback_api( + service_id, + delivery_status_callback["id"], + ) + + flash(_l("Your Callback configuration has been deleted."), + "default_with_tick") + return redirect(url_for(back_link, service_id=service_id)) + + flash(["{}".format( + _l("Are you sure you want to delete this callback configuration?"))], "delete") + + form = ServiceDeliveryStatusCallbackForm( + url=delivery_status_callback.get( + "url") if delivery_status_callback else "", + bearer_token=dummy_bearer_token if delivery_status_callback else "", + ) + + return render_template( + "views/api/callbacks/delivery-status-callback.html", + back_link=".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".delivery_status_callback", + hint_text=url_hint_txt, + is_deleting=True, + form=form, + ) + + @main.route( "/services//api/callbacks/delivery-status-callback", methods=["GET", "POST"], @@ -178,28 +223,54 @@ def get_delivery_status_callback_details(): @user_has_permissions("manage_api_keys") def delivery_status_callback(service_id): delivery_status_callback = get_delivery_status_callback_details() - back_link = ".api_callbacks" if current_service.has_permission("inbound_sms") else ".api_integration" + back_link = ".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".api_integration" + url_hint_txt = _l("Must start with https://") form = ServiceDeliveryStatusCallbackForm( - url=delivery_status_callback.get("url") if delivery_status_callback else "", + 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(): + # As part of the ValidCallbackUrl validation, we ping their callback URL to check if it's up and set the response time in g + response_time = "{:.2f}".format(g.callback_response_time) + url_hostname = form.url.data.split("https://")[1] + + # Update existing callback if delivery_status_callback and form.url.data: 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.url.data, - bearer_token=check_token_against_dummy_bearer(form.bearer_token.data), + bearer_token=check_token_against_dummy_bearer( + form.bearer_token.data), user_id=current_user.id, callback_api_id=delivery_status_callback.get("id"), ) - elif delivery_status_callback and not form.url.data: - service_api_client.delete_service_callback_api( - service_id, - delivery_status_callback["id"], - ) + + # If the user is just testing their URL, don't send them back to the API Integration page + if request.form.get("button_pressed") == "test_response_time": + flash( + _l("The service {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + return redirect(url_for("main.delivery_status_callback", service_id=service_id)) + + flash( + _l("We’ve saved your callback configuration. {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + + return redirect(url_for(back_link, service_id=service_id)) + # Create a new callback elif form.url.data: service_api_client.create_service_callback_api( service_id, @@ -207,17 +278,45 @@ def delivery_status_callback(service_id): bearer_token=form.bearer_token.data, user_id=current_user.id, ) + + flash( + _l("We’ve set up your callback configuration. {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + + return redirect(url_for(back_link, service_id=service_id)) else: # If no callback is set up and the user chooses to continue # having no callback (ie both fields empty) then there’s # nothing for us to do here pass - return redirect(url_for(back_link, service_id=service_id)) + if request.form.get("button_pressed") == "test_response_time" and g.callback_response_time >= 1: + flash( + _l("The service {} took longer than 1 second to respond.").format( + url_hostname, + ), + "error", + ) + return redirect(url_for("main.delivery_status_callback", service_id=service_id)) + else: + flash( + _l("The service {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + return redirect(url_for("main.delivery_status_callback", service_id=service_id)) return render_template( "views/api/callbacks/delivery-status-callback.html", + has_callback_config=delivery_status_callback is not None, back_link=back_link, + hint_text=url_hint_txt, form=form, ) @@ -235,23 +334,52 @@ def get_received_text_messages_callback(): def received_text_messages_callback(service_id): if not current_service.has_permission("inbound_sms"): return redirect(url_for(".api_integration", service_id=service_id)) + back_link = ".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".api_integration" received_text_messages_callback = get_received_text_messages_callback() form = ServiceReceiveMessagesCallbackForm( - url=received_text_messages_callback.get("url") if received_text_messages_callback else "", + url=received_text_messages_callback.get( + "url") if received_text_messages_callback else "", bearer_token=dummy_bearer_token if received_text_messages_callback else "", ) + url_hint_txt = _l("Must start with https://") if form.validate_on_submit(): + # As part of the ValidCallbackUrl validation, we ping their callback URL to check if it's up and set the response time in g + response_time = "{:.2f}".format(g.callback_response_time) + url_hostname = form.url.data.split("https://")[1] + if received_text_messages_callback and form.url.data: 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), + 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"), ) + + # If the user is just testing their URL, don't send them back to the API Integration page + if request.form.get("button_pressed") == "test_response_time": + flash( + _l("The service {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + return redirect(url_for("main.received_text_messages_callback", service_id=service_id)) + + flash( + _l("We’ve saved your callback configuration. {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + elif received_text_messages_callback and not form.url.data: service_api_client.delete_service_inbound_api( service_id, @@ -264,8 +392,75 @@ def received_text_messages_callback(service_id): bearer_token=form.bearer_token.data, user_id=current_user.id, ) - return redirect(url_for(".api_callbacks", service_id=service_id)) + flash( + _l("We’ve set up your callback configuration. {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + + return redirect(url_for(back_link, service_id=service_id)) + + if request.form.get("button_pressed") == "test_response_time" and g.callback_response_time >= 1: + flash( + _l("The service {} took longer than 1 second to respond.").format( + url_hostname, + ), + "error", + ) + return redirect(url_for("main.received_text_messages_callback", service_id=service_id)) + else: + flash( + _l("The service {} responded in {} seconds.").format( + url_hostname, + response_time, + ), + "default_with_tick", + ) + return redirect(url_for("main.received_text_messages_callback", service_id=service_id)) + return render_template( "views/api/callbacks/received-text-messages-callback.html", + has_callback_config=received_text_messages_callback is not None, + form=form, + hint_text=url_hint_txt, + ) + + +@main.route("/services//api/callbacks/received-text-messages-callback/delete", methods=["GET", "POST"]) +@user_has_permissions("manage_api_keys") +def delete_received_text_messages_callback(service_id): + received_text_messages_callback = get_received_text_messages_callback() + back_link = ".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".api_integration" + url_hint_txt = "Must start with https://" + + if request.method == "POST": + if received_text_messages_callback: + service_api_client.delete_service_inbound_api( + service_id, + received_text_messages_callback["id"], + ) + + flash(_l("Your Callback configuration has been deleted."), + "default_with_tick") + return redirect(url_for(back_link, service_id=service_id)) + + flash(["{}".format( + _l("Are you sure you want to delete this callback configuration?"))], "delete") + + form = ServiceReceiveMessagesCallbackForm( + url=received_text_messages_callback.get( + "url") if delivery_status_callback else "", + bearer_token=dummy_bearer_token if received_text_messages_callback else "", + ) + + return render_template( + "views/api/callbacks/delivery-status-callback.html", + back_link=".api_callbacks" if current_service.has_permission( + "inbound_sms") else ".delivery_status_callback", + hint_text=url_hint_txt, + is_deleting=True, form=form, ) diff --git a/app/templates/components/page-footer.html b/app/templates/components/page-footer.html index 13929520a3..3cf2ba666a 100644 --- a/app/templates/components/page-footer.html +++ b/app/templates/components/page-footer.html @@ -67,3 +67,49 @@ {% endmacro %} + +{% macro sticky_page_footer_two_submit_buttons_and_delete_link( + button1_text="b1", + button1_value="b1", + button2_text=None, + button2_value=None, + delete_link=False, + delete_link_text=_("Delete")) %} +
+
+ + + + {% if button2_text and button2_value %} + + {% endif %} + + {% if delete_link %} + + {{ delete_link_text }} + + {% endif %} +
+
+{% endmacro %} \ No newline at end of file diff --git a/app/templates/views/api/callbacks.html b/app/templates/views/api/callbacks.html index 6831405e9b..9b5b503d50 100644 --- a/app/templates/views/api/callbacks.html +++ b/app/templates/views/api/callbacks.html @@ -24,13 +24,13 @@ {% call row() %} {{ text_field('Delivery receipts') }} {{ optional_text_field(delivery_status_callback, truncate=false) }} - {{ edit_field('Change', url_for('.delivery_status_callback', service_id=current_service.id)) }} + {{ edit_field('Change', url_for('.delivery_status_callback', service_id=current_service.id), attributes="data-testid='change-delivery-receipts'") }} {% endcall %} {% call row() %} {{ text_field('Received text messages') }} {{ optional_text_field(received_text_messages_callback, truncate=false) }} - {{ edit_field('Change', url_for('.received_text_messages_callback', service_id=current_service.id)) }} + {{ edit_field('Change', url_for('.received_text_messages_callback', service_id=current_service.id), attributes="data-testid='change-received-text-messages'") }} {% endcall %} {% endcall %} diff --git a/app/templates/views/api/callbacks/delivery-status-callback.html b/app/templates/views/api/callbacks/delivery-status-callback.html index ed20b80b18..988466b5c3 100644 --- a/app/templates/views/api/callbacks/delivery-status-callback.html +++ b/app/templates/views/api/callbacks/delivery-status-callback.html @@ -1,8 +1,9 @@ {% extends "admin_template.html" %} {% from "components/textbox.html" import textbox %} {% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import page_footer %} +{% from "components/page-footer.html" import sticky_page_footer_two_submit_buttons_and_delete_link %} {% from "components/form.html" import form_wrapper %} +{% from "components/ajax-block.html" import ajax_block %} {% block service_page_title %} {{ _('Callbacks for delivery receipts') }} @@ -23,12 +24,12 @@

{{ _('Add an endpoint where GC Notify should send POST requests') }}

- {% set hint_txt = _('Must start with https://') %} {% call form_wrapper() %} {{ textbox( form.url, width='w-full', - hint=hint_txt + hint=hint_text, + testid='url', ) }}

{{ _('Add a secret value for authentication') }} @@ -38,12 +39,24 @@

form.bearer_token, width='w-full', hint=hint_txt, - autocomplete='new-password' + autocomplete='new-password', + testid='bearer-token', ) }} - {% set button_txt = _('Save') %} - {{ page_footer(button_txt) }} + {% set test_response_txt = _('Test response time') if has_callback_config else None %} + {% set test_response_value = _('test_response_time') if has_callback_config else None %} + {% set delete_link = url_for('.delete_delivery_status_callback', service_id=current_service.id) if has_callback_config else None %} + {% set display_footer = is_deleting if is_deleting else False %} + {% if not display_footer %} + {{ sticky_page_footer_two_submit_buttons_and_delete_link( + button1_text=_('Save'), + button1_value=_('save'), + button2_text=test_response_txt, + button2_value=test_response_value, + delete_link=delete_link, + delete_link_text=_('Delete') + ) }} + {% endif %} {% endcall %} - {% 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 index 488dad293f..49fab276b6 100644 --- a/app/templates/views/api/callbacks/received-text-messages-callback.html +++ b/app/templates/views/api/callbacks/received-text-messages-callback.html @@ -1,7 +1,7 @@ {% extends "admin_template.html" %} {% from "components/textbox.html" import textbox %} {% from "components/page-header.html" import page_header %} -{% from "components/page-footer.html" import page_footer %} +{% from "components/page-footer.html" import sticky_page_footer_two_submit_buttons_and_delete_link %} {% from "components/form.html" import form_wrapper %} {% block service_page_title %} @@ -18,13 +18,16 @@

{{ _('When you receive a text message in GC Notify, we can forward it to your system. Learn more in the') }} {{ _('callbacks documentation') }} .

- {% set hint_txt = _('Must start with https://') %} + {% call form_wrapper() %} {{ textbox( form.url, width='w-full', - hint=hint_txt + hint=hint_text ) }} +

+ {{ _('Add a secret value for authentication') }} +

{% set hint_txt = _('At least 10 characters') %} {{ textbox( form.bearer_token, @@ -32,8 +35,19 @@ hint=hint_txt, autocomplete='new-password' ) }} - {% set button_txt = _('Save') %} - {{ page_footer(button_txt) }} + {% set test_response_txt = _('Test response time') if has_callback_config else None %} + {% set test_response_value = _('test_response_time') if has_callback_config else None %} + {% set display_footer = is_deleting if is_deleting else False %} + {% if not display_footer %} + {{ sticky_page_footer_two_submit_buttons_and_delete_link( + button1_text=_('Save'), + button1_value=_('save'), + button2_text=test_response_txt, + button2_value=test_response_value, + delete_link=url_for('.delete_received_text_messages_callback', service_id=current_service.id), + delete_link_text=_('Delete') + ) }} + {% endif %} {% endcall %} diff --git a/app/templates/views/api/index.html b/app/templates/views/api/index.html index 7d084f4ade..47918e5ce0 100644 --- a/app/templates/views/api/index.html +++ b/app/templates/views/api/index.html @@ -21,7 +21,7 @@