From 27f5e4cfa16aa5d157eb4ce983e7bff609a3d6fa Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Thu, 12 Dec 2024 16:28:15 +0000 Subject: [PATCH 1/6] feat(get domains): add a cached utility method that queries AWS for sending domains --- app/utils.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app/utils.py b/app/utils.py index e9de092796..474dad4e4f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -905,3 +905,24 @@ def get_limit_reset_time_et() -> dict[str, str]: limit_reset_time_et = {"en": next_midnight_utc_in_et.strftime("%-I%p"), "fr": next_midnight_utc_in_et.strftime("%H")} return limit_reset_time_et + + +@cache.memoize(timeout=5 * 60) +def get_verified_ses_domains(): + """Query AWS SES for verified domain identities""" + ses = boto3.client("ses", region_name="ca-central-1") + + # Get all identities + identities = ses.list_identities( + IdentityType="Domain" # Only get domains, not email addresses + ) + + # Get verification status for each domain + verification_attrs = ses.get_identity_verification_attributes(Identities=identities["Identities"])["VerificationAttributes"] + + # Format results + domains = [ + domain for domain in identities["Identities"] if verification_attrs.get(domain, {}).get("VerificationStatus") == "Success" + ] + + return domains From 21f0d5b976db7333676457df88e52483b25c6203 Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Thu, 12 Dec 2024 16:28:49 +0000 Subject: [PATCH 2/6] feat(domain setting): use verified list of domains on sending domain settings page --- app/main/forms.py | 6 +++++- app/main/views/service_settings.py | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 0d6761f7d7..72925c5616 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -553,7 +553,11 @@ def __init__(self, *args, **kwargs): class SendingDomainForm(StripWhitespaceForm): - sending_domain = StringField(_l("Sending domain"), validators=[]) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.sending_domain.choices = kwargs["sending_domain_choices"] + + sending_domain = SelectField(_l("Sending domain"), validators=[]) class RenameOrganisationForm(StripWhitespaceForm): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index fd7dd363fe..3d9a09ca5c 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -72,6 +72,7 @@ email_safe, get_logo_cdn_domain, get_new_default_reply_to_address, + get_verified_ses_domains, user_has_permissions, user_is_gov_user, user_is_platform_admin, @@ -549,7 +550,7 @@ def service_set_reply_to_email(service_id): @main.route("/services//service-settings/sending-domain", methods=["GET", "POST"]) @user_is_platform_admin def service_sending_domain(service_id): - form = SendingDomainForm() + form = SendingDomainForm(sending_domain_choices=get_verified_ses_domains()) if request.method == "GET": form.sending_domain.data = current_service.sending_domain From 0debc256579804532b69d480628d012188056022 Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Thu, 12 Dec 2024 17:00:31 +0000 Subject: [PATCH 3/6] test(ses sending domains): add some tests around verified domains --- tests/app/test_utils.py | 51 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index d44860d5ab..47ecfc9767 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -2,6 +2,7 @@ from csv import DictReader from io import StringIO from pathlib import Path +from unittest.mock import Mock, patch from urllib.parse import unquote import pytest @@ -23,6 +24,7 @@ get_new_default_reply_to_address, get_remote_addr, get_template, + get_verified_ses_domains, printing_today_or_tomorrow, report_security_finding, ) @@ -759,3 +761,52 @@ def test_get_new_default_reply_to_address_returns_none_if_one_reply_to(mocker: M new_default = get_new_default_reply_to_address(email_reply_tos, reply_to_1) # type: ignore assert new_default is None + + +class TestGetSESDomains: + @pytest.fixture + def mock_ses_client(self): + with patch("boto3.client") as mock_client: + mock_ses = Mock() + mock_client.return_value = mock_ses + yield mock_ses + + @pytest.fixture(autouse=True) + def mock_cache_decorator(self): + with patch("app.utils.cache.memoize", lambda *args, **kwargs: lambda f: f): + yield + + def test_get_verified_ses_domains(self, app_, mock_ses_client): + # Setup mock return values + mock_ses_client.list_identities.return_value = {"Identities": ["domain1.com", "domain2.com", "domain3.com"]} + + mock_ses_client.get_identity_verification_attributes.return_value = { + "VerificationAttributes": { + "domain1.com": {"VerificationStatus": "Success"}, + "domain2.com": {"VerificationStatus": "Failed"}, + "domain3.com": {"VerificationStatus": "Pending"}, + } + } + + # Execute within app context + with app_.test_request_context(): + result = get_verified_ses_domains() + + # Assert + assert result == ["domain1.com"] + mock_ses_client.list_identities.assert_called_once_with(IdentityType="Domain") + mock_ses_client.get_identity_verification_attributes.assert_called_once_with( + Identities=["domain1.com", "domain2.com", "domain3.com"] + ) + + def test_get_verified_ses_domains_no_domains(self, app_, mock_ses_client): + # Setup empty response + mock_ses_client.list_identities.return_value = {"Identities": []} + mock_ses_client.get_identity_verification_attributes.return_value = {"VerificationAttributes": {}} + + # Execute within app context + with app_.test_request_context(): + result = get_verified_ses_domains() + + # Assert + assert result == [] From c23ecde472e369ef99a76aae9f0756eeeca0e7a1 Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Thu, 12 Dec 2024 18:43:36 +0000 Subject: [PATCH 4/6] test(sending domain): add some ui tests --- tests/app/main/views/test_service_settings.py | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 2ce8210a5d..5eba8e834a 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4377,3 +4377,69 @@ def test_should_suspend_service_callback_api(self, client_request, platform_admi _data={"updated_by_id": platform_admin_user["id"], "suspend_unsuspend": False}, _expected_status=200, ) + + +class TestSendingDomain: + def test_sending_domain_page_shows_dropdown_of_verified_domains( + self, client_request, platform_admin_user, mock_get_service_settings_page_common, mocker + ): + client_request.login(platform_admin_user) + mock_get_domains = mocker.patch( + "app.main.views.service_settings.get_verified_ses_domains", return_value=["domain1.com", "domain2.com"] + ) + + page = client_request.get("main.service_sending_domain", service_id=SERVICE_ONE_ID, _test_page_title=False) + + assert [option["value"] for option in page.select("select[name=sending_domain] option")] == ["domain1.com", "domain2.com"] + + mock_get_domains.assert_called_once() + + def test_sending_domain_page_populates_with_current_domain( + self, client_request, platform_admin_user, mock_get_service_settings_page_common, mocker, service_one + ): + service_one["sending_domain"] = "domain1.com" + mocker.patch("app.main.views.service_settings.get_verified_ses_domains", return_value=["domain1.com", "domain2.com"]) + + client_request.login(platform_admin_user) + page = client_request.get("main.service_sending_domain", service_id=SERVICE_ONE_ID, _test_page_title=False) + + assert page.select_one("select[name=sending_domain] option[selected]")["value"] == "domain1.com" + + def test_sending_domain_page_updates_domain_and_redirects_when_posted( + self, client_request, platform_admin_user, mock_get_service_settings_page_common, mocker, service_one, mock_update_service + ): + mocker.patch("app.main.views.service_settings.get_verified_ses_domains", return_value=["domain1.com", "domain2.com"]) + + client_request.login(platform_admin_user) + client_request.post( + "main.service_sending_domain", + service_id=SERVICE_ONE_ID, + _data={"sending_domain": "domain2.com"}, + _expected_redirect=url_for( + "main.service_settings", + service_id=SERVICE_ONE_ID, + ), + _test_page_title=False, + ) + + mock_update_service.assert_called_once_with(service_one["id"], sending_domain="domain2.com") + + def test_sending_domain_page_doesnt_update_if_domain_not_in_allowed_list( + self, client_request, platform_admin_user, mock_get_service_settings_page_common, mocker, service_one, mock_update_service + ): + mocker.patch("app.main.views.service_settings.get_verified_ses_domains", return_value=["domain1.com", "domain2.com"]) + + client_request.login(platform_admin_user) + page = client_request.post( + "main.service_sending_domain", + service_id=SERVICE_ONE_ID, + _data={"sending_domain": "domain3.com"}, + _expected_status=200, + _test_page_title=False, + ) + + assert mock_update_service.called is False + assert "Not a valid choice" in page.text + + def test_sending_domain_page_404s_for_non_platform_admin(self, client_request, mock_get_service_settings_page_common, mocker): + client_request.get("main.service_sending_domain", service_id=SERVICE_ONE_ID, _expected_status=403) From 2820c093d921ad49c1bb1243dcf505db6f8d4bf1 Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Mon, 16 Dec 2024 13:37:00 +0000 Subject: [PATCH 5/6] test: temporarily add keys to PR review env to see if everything works --- .github/workflows/test-admin-deploy.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test-admin-deploy.yaml b/.github/workflows/test-admin-deploy.yaml index 4428177b02..89ee045fee 100644 --- a/.github/workflows/test-admin-deploy.yaml +++ b/.github/workflows/test-admin-deploy.yaml @@ -124,6 +124,8 @@ jobs: API_HOST_NAME=https://api.staging.notification.cdssandbox.xyz,\ ADMIN_BASE_URL=$URL,\ AWS_XRAY_SDK_ENABLED=False + AWS_ACCESS_KEY_ID=${{ secrets.STAGING_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY=${{ secrets.STAGING_AWS_SECRET_ACCESS_KEY }} }" > /dev/null 2>&1 aws logs create-log-group --log-group-name /aws/lambda/$FUNCTION_NAME-$PR_NUMBER > /dev/null 2>&1 From 5c3bb9a494b07df8eea9cff1bff0f19359a900f2 Mon Sep 17 00:00:00 2001 From: Andrew Leith Date: Mon, 16 Dec 2024 13:48:14 +0000 Subject: [PATCH 6/6] fix: undo test --- .github/workflows/test-admin-deploy.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/test-admin-deploy.yaml b/.github/workflows/test-admin-deploy.yaml index 89ee045fee..4428177b02 100644 --- a/.github/workflows/test-admin-deploy.yaml +++ b/.github/workflows/test-admin-deploy.yaml @@ -124,8 +124,6 @@ jobs: API_HOST_NAME=https://api.staging.notification.cdssandbox.xyz,\ ADMIN_BASE_URL=$URL,\ AWS_XRAY_SDK_ENABLED=False - AWS_ACCESS_KEY_ID=${{ secrets.STAGING_AWS_ACCESS_KEY_ID }} - AWS_SECRET_ACCESS_KEY=${{ secrets.STAGING_AWS_SECRET_ACCESS_KEY }} }" > /dev/null 2>&1 aws logs create-log-group --log-group-name /aws/lambda/$FUNCTION_NAME-$PR_NUMBER > /dev/null 2>&1