Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance UX and add features to the Callback pages #1855

Merged
merged 29 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1eb677c
Add basic validation to callbacks URLs
whabanks May 29, 2024
36fc3d3
Added dynamic hint text to the url field
whabanks May 30, 2024
90694c1
Merge branch 'main' into task/verify-callback-url
whabanks Jun 6, 2024
17edaeb
Various fixes
whabanks Jun 7, 2024
dd8c154
Merge branch 'main' into task/verify-callback-url
whabanks Jun 11, 2024
a1ae8b2
Fix tests
whabanks Jun 11, 2024
f0c7683
Merge branch 'main' into task/verify-callback-url
whabanks Jun 11, 2024
40c743a
Add placeholder translations
whabanks Jun 11, 2024
9927046
Merge branch 'main' into task/verify-callback-url
whabanks Jul 30, 2024
8c79c8f
Merge branch 'main' into task/verify-callback-url
whabanks Aug 12, 2024
0046a7d
Consider 5xx responses as valid
whabanks Aug 12, 2024
d8ea5cb
Merge branch 'main' into task/verify-callback-url
whabanks Aug 13, 2024
144e65c
Improve the callback config UX
whabanks Aug 13, 2024
ab221c6
Merge remote-tracking branch 'origin/main' into task/verify-callback-url
whabanks Sep 4, 2024
eec3e7c
Add callback test button
whabanks Sep 9, 2024
ba989b9
Unify delivery-status-callback and received-text-messages-callback pages
whabanks Sep 9, 2024
193e263
formatting fixes
whabanks Sep 9, 2024
f3f3857
Fix tests
whabanks Sep 11, 2024
541b010
Merge branch 'main' into task/verify-callback-url
whabanks Sep 11, 2024
104bfca
Fix updated translations in code
whabanks Sep 11, 2024
a7d565b
Merge branch 'main' into task/verify-callback-url
whabanks Sep 17, 2024
dd83ae9
Add & fix tests
whabanks Sep 17, 2024
1468f21
Update delete message
whabanks Sep 17, 2024
7164b9e
Update french translations
whabanks Sep 18, 2024
3897819
Merge branch 'main' into task/verify-callback-url
jzbahrai Sep 19, 2024
53f1f24
Update translations
whabanks Sep 23, 2024
807433d
Merge branch 'main' into task/verify-callback-url
whabanks Sep 23, 2024
71c4ba5
Translations & refresh lock file
whabanks Sep 23, 2024
f2330a2
Fix code QL issues
whabanks Sep 23, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions app/main/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
SelectField,
SelectMultipleField,
StringField,
SubmitField,
TextAreaField,
ValidationError,
validators,
Expand All @@ -49,6 +50,7 @@
LettersNumbersAndFullStopsOnly,
NoCommasInPlaceHolders,
OnlySMSCharacters,
ValidCallbackUrl,
ValidEmail,
ValidGovEmail,
validate_email_from,
Expand Down Expand Up @@ -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


Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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):
Expand Down Expand Up @@ -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"))],
)

Expand Down
60 changes: 57 additions & 3 deletions app/main/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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:
Expand Down Expand Up @@ -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):
Copy link
Member

@andrewleith andrewleith Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in slack, this might be an issue since here it expects the callback servers to respond with a 200 when we call them with {"health_check": "true"}. If we want to do this we would probably have to advertise this in the docs so users are aware.

"""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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to say I was concerned that using the flask global object might leak the value here to other users, but just learned that values stored here are request-specific and last for only one request. Good to know!


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
Expand Down
Loading
Loading