Restrict email reply-to domains to domains used by current team membe…

…rs (#2060) * Restrict email reply-tos to domains of current team members * Fix translations * Allow .gc.ca and canada.ca domains for reply-to addresses * Update localization and FR translations * Fix tests * Add missing   to fr translations * Update safelist * Fix domain ordering in error message and fix tests
cds-snc · Feb 24, 2025 · 1351852 · 1351852
1 parent 9937327
commit 1351852
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 18 deletions.
diff --git a/app/config.py b/app/config.py
@@ -69,6 +69,7 @@ class Config(object):
     DEFAULT_SERVICE_LIMIT = env.int("DEFAULT_SERVICE_LIMIT", 50)
     DEFAULT_SMS_DAILY_LIMIT = env.int("DEFAULT_SMS_DAILY_LIMIT", 50)
     DOCUMENTATION_DOMAIN = os.getenv("DOCUMENTATION_DOMAIN", "documentation.notification.canada.ca")
+    REPLY_TO_DOMAINS_SAFELIST = ["canada.ca", "gc.ca", "2Vandenbos.org"]
     EMAIL_2FA_EXPIRY_SECONDS = 1_800  # 30 Minutes
     EMAIL_EXPIRY_SECONDS = 3600  # 1 hour
 

diff --git a/app/main/forms.py b/app/main/forms.py
@@ -53,6 +53,7 @@
     ValidCallbackUrl,
     ValidEmail,
     ValidGovEmail,
+    ValidTeamMemberDomain,
     validate_email_from,
     validate_service_name,
 )
@@ -124,7 +125,7 @@ class MultiCheckboxField(SelectMultipleField):
     option_widget = CheckboxInput()
 
 
-def email_address(label=_l("Email address"), gov_user=True, required=True):
+def email_address(label=_l("Email address"), gov_user=True, only_team_member_domains=False, required=True):
     if label == "email address":
         label = _l("email address")
     elif label == "phone number":
@@ -140,6 +141,9 @@ def email_address(label=_l("Email address"), gov_user=True, required=True):
     if required:
         validators.append(DataRequired(message=_l("Enter an email address")))
 
+    if only_team_member_domains:
+        validators.append(ValidTeamMemberDomain())
+
     return EmailField(label, validators, render_kw={"spellcheck": "false"})
 
 
@@ -1103,7 +1107,7 @@ def __init__(self, *args, **kwargs):
 
 class ServiceReplyToEmailForm(StripWhitespaceForm):
     label_text = _l("Reply-to email address")
-    email_address = email_address(label=_l(label_text), gov_user=False)
+    email_address = email_address(label=_l(label_text), only_team_member_domains=True, gov_user=False)
     is_default = BooleanField(_l("Make this email address the default"))
 
 

diff --git a/app/main/validators.py b/app/main/validators.py
@@ -62,6 +62,24 @@ def __call__(self, form, field):
             )
 
 
+class ValidTeamMemberDomain:
+    def __call__(self, form, field):
+        if not field.data:
+            return
+        email_domain = field.data.split("@")[-1]
+        # Merge domains from team members and safelisted domains, use set to ensure no duplicates
+        safelisted_domains = set(current_app.config.get("REPLY_TO_DOMAINS_SAFELIST", set()))
+        valid_domains = g.team_member_email_domains.union(safelisted_domains)
+
+        if email_domain not in valid_domains:
+            safelist_domains_to_display = ["canada.ca", "gc.ca"]
+            safelist_domains_to_display.extend(g.team_member_email_domains)
+            message = _("{} is not a government or team email address</br>Use one of the following domains:</br>{}").format(
+                email_domain, "<br>".join([f"@{domain}" for domain in safelist_domains_to_display])
+            )
+            raise ValidationError(message)
+
+
 class ValidGovEmail:
     def __call__(self, form, field):
         if not field.data:

diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py
@@ -5,6 +5,7 @@
     abort,
     current_app,
     flash,
+    g,
     jsonify,
     redirect,
     render_template,
@@ -580,6 +581,7 @@ def service_add_email_reply_to(service_id):
     form = ServiceReplyToEmailForm()
     first_email_address = current_service.count_email_reply_to_addresses == 0
     is_default = first_email_address if first_email_address else form.is_default.data
+    g.team_member_email_domains = set([team_member.email_domain for team_member in current_service.team_members])
     if form.validate_on_submit():
         try:
             notification_id = service_api_client.verify_reply_to_email_address(service_id, form.email_address.data)["data"]["id"]
@@ -696,6 +698,7 @@ def get_service_verify_reply_to_address_partials(service_id, notification_id):
 def service_edit_email_reply_to(service_id, reply_to_email_id):
     form = ServiceReplyToEmailForm()
     reply_to_email_address = current_service.get_email_reply_to_address(reply_to_email_id)
+    g.team_member_email_domains = set([team_member.email_domain for team_member in current_service.team_members])
     if request.method == "GET":
         form.email_address.data = reply_to_email_address["email_address"]
         form.is_default.data = reply_to_email_address["is_default"]

diff --git a/app/translations/csv/fr.csv b/app/translations/csv/fr.csv
@@ -2103,6 +2103,7 @@
 "SVG Images only!","Images SVG uniquement"
 "English Government of Canada signature and custom logo","Signature du gouvernement du Canada en anglais et logo personnalisé"
 "Upload a PNG logo","Téléverser un logo PNG"
+"{} is not a government or team email address</br>Use one of the following domains:</br>{}","{} n'est pas une adresse courriel du gouvernement ou de l’équipe</br>Veuillez utiliser l’un des domaines suivants&nbsp;:</br>{}"
 "Some communications are prohibited during a federal election. Check with your communications branch if you’re not sure you can send an announcement or other message. For more information, read the <a href='https://www.canada.ca/en/privy-council/services/publications/guidelines-conduct-ministers-state-exempt-staff-public-servants-election.html'>Guidelines</a>.","Certaines communications sont interdites en période d’élection fédérale. Si vous n’avez pas la certitude de pouvoir envoyer une annonce ou un message, veuillez vous renseigner auprès de la direction des communications de votre organisme. Pour en savoir plus, consultez les lignes <a href='https://www.canada.ca/fr/conseil-prive/services/publications/lignes-directrices-regissant-conduite-ministres-etat-membres-personnel-exonere-fonctionnaires-periode-electorale.html'>directrices</a>."
 "Communicating during caretaker period","Communications durant la période de transition"
 "Information","Information"

diff --git a/tests/__init__.py b/tests/__init__.py
@@ -94,6 +94,7 @@ def user_json(
     id_="1234",
     name="Test User",
     email_address="[email protected]",
+    email_domain=None,
     mobile_number="+16502532222",
     blocked=False,
     password_changed_at=None,
@@ -123,6 +124,7 @@ def user_json(
         "id": id_,
         "name": name,
         "email_address": email_address,
+        "email_domain": email_address.split("@")[-1],
         "mobile_number": mobile_number,
         "password_changed_at": password_changed_at,
         "permissions": permissions,

diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py
@@ -1,9 +1,10 @@
 from unittest.mock import Mock
 
 import pytest
+from flask import g
 from wtforms import ValidationError
 
-from app.main.forms import RegisterUserForm, ServiceSmsSenderForm
+from app.main.forms import RegisterUserForm, ServiceSmsSenderForm, ValidTeamMemberDomain
 from app.main.validators import NoCommasInPlaceHolders, OnlySMSCharacters, ValidGovEmail
 
 
@@ -88,6 +89,22 @@ def test_valid_list_of_white_list_email_domains(
     email_domain_validators(None, _gen_mock_field(email))
 
 
+@pytest.mark.parametrize(
+    "email, raises", [("[email protected]", False), ("[email protected]", False), ("[email protected]", True)]
+)
+def test_valid_team_only_email_domains(email, app_, raises):
+    with app_.app_context(), app_.test_request_context():
+        g.team_member_email_domains = set(["team-domain.ca", "cds-snc.ca"])
+
+        team_only_domain_validator = ValidTeamMemberDomain()
+
+        if raises:
+            with pytest.raises(ValidationError):
+                team_only_domain_validator(None, _gen_mock_field(email))
+        else:
+            team_only_domain_validator(None, _gen_mock_field(email))
+
+
 @pytest.mark.parametrize(
     "email",
     [

diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py
@@ -780,7 +780,7 @@ def test_should_raise_duplicate_name_handled(
 
 
 @pytest.mark.parametrize(
-    ("count_of_users_with_manage_service," "count_of_invites_with_manage_service," "expected_user_checklist_item"),
+    ("count_of_users_with_manage_service,count_of_invites_with_manage_service,expected_user_checklist_item"),
     [
         (1, 0, "Add a team member who can manage settings Not completed"),
         (2, 0, "Add a team member who can manage settings Completed"),
@@ -1560,7 +1560,9 @@ def test_no_senders_message_shows(
         ("testtest", "Enter a valid email address"),
     ],
 )
-def test_incorrect_reply_to_email_address_input(reply_to_input, expected_error, client_request, no_reply_to_email_addresses):
+def test_incorrect_reply_to_email_address_input(
+    reply_to_input, expected_error, client_request, mock_team_members, no_reply_to_email_addresses, app_
+):
     page = client_request.post(
         "main.service_add_email_reply_to",
         service_id=SERVICE_ONE_ID,
@@ -1571,6 +1573,25 @@ def test_incorrect_reply_to_email_address_input(reply_to_input, expected_error,
     assert normalize_spaces(page.select_one(".error-message").text) == expected_error
 
 
+def test_incorrect_reply_to_domain_not_in_team_member_list(
+    client_request, mock_team_members, no_reply_to_email_addresses, app_, mocker
+):
+    page = client_request.post(
+        "main.service_add_email_reply_to",
+        service_id=SERVICE_ONE_ID,
+        _data={"email_address": "[email protected]"},
+        _expected_status=200,
+    )
+
+    valid_domains = ["canada.ca", "gc.ca"]
+    valid_domains.extend([member.email_domain for member in mock_team_members])
+
+    errorMsg = normalize_spaces(page.select_one(".error-message").text)
+    assert "not-team-member-domain.ca is not a government or team email addressUse one of the following domains:" in errorMsg
+    for domain in valid_domains:
+        assert f"@{domain}" in errorMsg
+
+
 @pytest.mark.parametrize(
     "contact_block_input, expected_error",
     [
@@ -1640,8 +1661,11 @@ def test_incorrect_sms_sender_input(
         (create_multiple_email_reply_to_addresses(), {"is_default": "y"}, True),
     ],
 )
-def test_add_reply_to_email_address_sends_test_notification(mocker, client_request, reply_to_addresses, data, api_default_args):
+def test_add_reply_to_email_address_sends_test_notification(
+    mocker, client_request, reply_to_addresses, mock_team_members, data, api_default_args
+):
     mocker.patch("app.service_api_client.get_reply_to_email_addresses", return_value=reply_to_addresses)
+
     data["email_address"] = "[email protected]"
     mock_verify = mocker.patch(
         "app.service_api_client.verify_reply_to_email_address",
@@ -1856,7 +1880,9 @@ def test_add_sms_sender(sms_senders, data, api_default_args, mocker, client_requ
         ),
     ],
 )
-def test_default_box_doesnt_show_on_first_sender(sender_page, function_to_mock, mocker, data, checkbox_present, client_request):
+def test_default_box_doesnt_show_on_first_sender(
+    sender_page, function_to_mock, mock_team_members, mocker, data, checkbox_present, client_request
+):
     mocker.patch(function_to_mock, side_effect=lambda service_id: data)
 
     page = client_request.get(sender_page, service_id=SERVICE_ONE_ID)
@@ -1877,6 +1903,7 @@ def test_edit_reply_to_email_address_sends_verification_notification_if_address_
     reply_to_address,
     data,
     api_default_args,
+    mock_team_members,
     mocker,
     fake_uuid,
     client_request,
@@ -1886,14 +1913,14 @@ def test_edit_reply_to_email_address_sends_verification_notification_if_address_
         return_value={"data": {"id": "123"}},
     )
     mocker.patch("app.service_api_client.get_reply_to_email_address", return_value=reply_to_address)
-    data["email_address"] = "[email protected]"
+    data["email_address"] = "[email protected]"
     client_request.post(
         "main.service_edit_email_reply_to",
         service_id=SERVICE_ONE_ID,
         reply_to_email_id=fake_uuid,
         _data=data,
     )
-    mock_verify.assert_called_once_with(SERVICE_ONE_ID, "[email protected]")
+    mock_verify.assert_called_once_with(SERVICE_ONE_ID, "[email protected]")
 
 
 @pytest.mark.parametrize(
@@ -1912,6 +1939,7 @@ def test_edit_reply_to_email_address_goes_straight_to_update_if_address_not_chan
     mocker,
     fake_uuid,
     client_request,
+    mock_team_members,
     mock_update_reply_to_email_address,
 ):
     mocker.patch("app.service_api_client.get_reply_to_email_address", return_value=reply_to_address)
@@ -1941,6 +1969,7 @@ def test_edit_reply_to_email_address_goes_straight_to_update_if_address_not_chan
 def test_always_shows_delete_link_for_email_reply_to_address(
     mocker: MockerFixture,
     sender_details,
+    mock_team_members,
     fake_uuid,
     client_request,
 ):
@@ -1967,7 +1996,9 @@ def test_always_shows_delete_link_for_email_reply_to_address(
     assert link["href"] == partial_href(service_id=SERVICE_ONE_ID)
 
 
-def test_confirm_delete_reply_to_email_address(fake_uuid, client_request, get_non_default_reply_to_email_address):
+def test_confirm_delete_reply_to_email_address(
+    fake_uuid, client_request, mock_team_members, get_non_default_reply_to_email_address
+):
     page = client_request.get(
         "main.service_confirm_delete_email_reply_to",
         service_id=SERVICE_ONE_ID,
@@ -1976,14 +2007,14 @@ def test_confirm_delete_reply_to_email_address(fake_uuid, client_request, get_no
     )
 
     assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
-        "Are you sure you want to delete this reply-to email address? " "Yes, delete"
+        "Are you sure you want to delete this reply-to email address? Yes, delete"
     )
     assert "action" not in page.select_one(".banner-dangerous form")
     assert page.select_one(".banner-dangerous form")["method"] == "post"
 
 
 def test_confirm_delete_default_reply_to_email_address(
-    mocker: MockerFixture, fake_uuid, client_request, get_default_reply_to_email_address
+    mocker: MockerFixture, fake_uuid, client_request, mock_team_members, get_default_reply_to_email_address
 ):
     reply_tos = create_multiple_email_reply_to_addresses()
     reply_to_for_deletion = reply_tos[1]
@@ -1998,7 +2029,7 @@ def test_confirm_delete_default_reply_to_email_address(
     )
 
     assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
-        "Are you sure you want to delete this reply-to email address? " "Yes, delete"
+        "Are you sure you want to delete this reply-to email address? Yes, delete"
     )
     assert "action" not in page.select_one(".banner-dangerous form")
     assert page.select_one(".banner-dangerous form")["method"] == "post"
@@ -2100,7 +2131,7 @@ def test_confirm_delete_letter_contact_block(
     )
 
     assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
-        "Are you sure you want to delete this contact block? " "Yes, delete"
+        "Are you sure you want to delete this contact block? Yes, delete"
     )
     assert "action" not in page.select_one(".banner-dangerous form")
     assert page.select_one(".banner-dangerous form")["method"] == "post"
@@ -2233,6 +2264,7 @@ def test_default_box_shows_on_non_default_sender_details_while_editing(
     sender_details,
     client_request,
     platform_admin_user,
+    mock_team_members,
     default_message,
     checkbox_present,
     is_platform_admin,
@@ -2321,7 +2353,7 @@ def test_confirm_delete_sms_sender(
     )
 
     assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
-        "Are you sure you want to delete this text message sender? " "Yes, delete"
+        "Are you sure you want to delete this text message sender? Yes, delete"
     )
     assert "action" not in page.select_one(".banner-dangerous form")
     assert page.select_one(".banner-dangerous form")["method"] == "post"
@@ -3307,7 +3339,7 @@ def test_switch_service_enable_letters(
 
 
 @pytest.mark.parametrize(
-    ("initial_permissions," "expected_initial_value," "posted_value," "expected_updated_permissions"),
+    ("initial_permissions,expected_initial_value,posted_value,expected_updated_permissions"),
     [
         (
             ["email", "sms"],
@@ -3566,7 +3598,7 @@ def test_archive_service_prompts_user(
         service_id=SERVICE_ONE_ID,
     )
     assert normalize_spaces(delete_page.select_one(".banner-dangerous").text) == (
-        "Are you sure you want to delete ‘service one’? " "There’s no way to undo this. " "Yes, delete"
+        "Are you sure you want to delete ‘service one’? There’s no way to undo this. Yes, delete"
     )
     assert mocked_fn.called is False
 
@@ -4252,7 +4284,7 @@ def test_submit_email_branding_request(
         tags=["notify_action_add_branding"],
     )
     assert normalize_spaces(page.select_one(".banner-default").text) == (
-        "Thanks for your branding request. We’ll get back to you " "within one working day."
+        "Thanks for your branding request. We’ll get back to you within one working day."
     )
 
 

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -91,6 +91,22 @@ def service_two(api_user_active):
     return service_json(SERVICE_TWO_ID, "service two", [api_user_active["id"]])
 
 
+@pytest.fixture(scope="function")
+def service_multi_member(api_user_active):
+    return service_json(
+        SERVICE_MULTI_MEMBER_ID,
+        "service multi-team-member",
+        [api_user_active, user_json(email_address="[email protected]"), user_json(email_address="[email protected]")],
+    )
+
+
+@pytest.fixture(scope="function")
+def mock_team_members(mocker, service_multi_member):
+    mock_team_members = [Mock(email_domain=team_member["email_domain"]) for team_member in service_multi_member.users]
+    mocker.patch("app.models.service.Service.team_members", mock_team_members)
+    return mock_team_members
+
+
 @pytest.fixture(scope="function")
 def multiple_reply_to_email_addresses(mocker):
     def _get(service_id):
@@ -793,6 +809,7 @@ def _update(service_id, **kwargs):
 
 SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb"
 SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52"
+SERVICE_MULTI_MEMBER_ID = "ca1a6d94-cca9-476a-9d5a-49c21a79d938"
 ORGANISATION_ID = "c011fa40-4cbe-4524-b415-dde2f421bd9c"
 ORGANISATION_TWO_ID = "d9b5be73-0b36-4210-9d89-8f1a5c2fef26"
 TEMPLATE_ONE_ID = "b22d7d94-2197-4a7d-a8e7-fd5f9770bf48"
@@ -1432,6 +1449,7 @@ def api_user_active(fake_uuid):
         "name": "Test User",
         "password": "somepassword",
         "email_address": "[email protected]",
+        "email_domain": "user.canada.ca",
         "mobile_number": "6502532222",
         "blocked": False,
         "state": "active",