This table does not have a description yet.
{% endif %} diff --git a/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html b/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html new file mode 100644 index 000000000..4bac06437 --- /dev/null +++ b/src/registrar/templates/django/admin/includes/descriptions/allowed_email_description.html @@ -0,0 +1,6 @@ +This table is an email whitelist for non-production environments.
++ If an email is sent out and the email does not exist within this table (or is not a subset of it), + then no email will be sent. +
+If this table is populated in a production environment, no change will occur as it will simply be ignored.
\ No newline at end of file From 056a3ecf36cd1d81461e3ec355c713183051d2ab Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 13:26:32 -0600 Subject: [PATCH 03/22] Add to fixtures and add whitelist logic --- src/registrar/fixtures_users.py | 38 +++++++++++++++++++++++++++ src/registrar/models/allowed_email.py | 31 +++++++++++++++++++++- src/registrar/utility/email.py | 25 +++++++++++++----- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 0fc203248..ecc30db91 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -6,6 +6,7 @@ User, UserGroup, ) +from registrar.models.allowed_email import AllowedEmail fake = Faker() @@ -240,6 +241,11 @@ class UserFixture: }, ] + # Additional emails to add to the AllowedEmail whitelist + ADDITIONAL_ALLOWED_EMAILS = [ + "zander.adkinson@ecstech.com" + ] + def load_users(cls, users, group_name, are_superusers=False): logger.info(f"Going to load {len(users)} users in group {group_name}") for user_data in users: @@ -264,6 +270,34 @@ def load_users(cls, users, group_name, are_superusers=False): logger.warning(e) logger.info(f"All users in group {group_name} loaded.") + def load_allowed_emails(cls, users, additional_emails): + """Populates a whitelist of allowed emails (as defined in this list)""" + logger.info(f"Going to load allowed emails for {len(users)} users") + if additional_emails: + logger.info(f"Going to load {len(additional_emails)} additional allowed emails") + + allowed_emails = [] + + # Load user emails + for user_data in users: + user_email = user_data.get("email") + if user_email and user_email not in allowed_emails: + allowed_emails.append(AllowedEmail(email=user_email)) + else: + first_name = user_data.get("first_name") + last_name = user_data.get("last_name") + logger.warning(f"Could not load email for {first_name} {last_name}: No email exists.") + + # Load additional emails + for email in additional_emails: + allowed_emails.append(AllowedEmail(email=email)) + + if allowed_emails: + AllowedEmail.objects.bulk_create(allowed_emails) + logger.info(f"Loaded {len(allowed_emails)} allowed emails") + else: + logger.info("No allowed emails to load") + @classmethod def load(cls): # Lumped under .atomic to ensure we don't make redundant DB calls. @@ -275,3 +309,7 @@ def load(cls): with transaction.atomic(): cls.load_users(cls, cls.ADMINS, "full_access_group", are_superusers=True) cls.load_users(cls, cls.STAFF, "cisa_analysts_group") + + # Combine ADMINS and STAFF lists + all_users = cls.ADMINS + cls.STAFF + cls.load_allowed_emails(cls, all_users, additional_emails=cls.ADDITIONAL_ALLOWED_EMAILS) diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 90105debe..796f4b556 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -1,5 +1,6 @@ from django.db import models - +from django.db.models import Q +import re from .utility.time_stamped_model import TimeStampedModel @@ -16,5 +17,33 @@ class AllowedEmail(TimeStampedModel): max_length=320, ) + @classmethod + def is_allowed_email(cls, email): + """Given an email, check if this email exists within our AllowEmail whitelist""" + print(f"the email is: {email}") + if not email: + return False + + # Split the email into a local part and a domain part + local, domain = email.split('@') + + # Check if there's a '+' in the local part + if "+" in local: + base_local = local.split("+")[0] + base_email = f"{base_local}@{domain}" + allowed_emails = cls.objects.filter(email__iexact=base_email) + + # The string must start with the local, and the plus must be a digit + # and occur immediately after the local. The domain should still exist in the email. + pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + + # If the base email exists AND the email matches our expected regex, + # then we can let the email through. + return allowed_emails.exists() and re.match(pattern, email) + else: + # If no '+' exists in the email, just do an exact match + allowed_emails = cls.objects.filter(email__iexact=email) + return allowed_emails.exists() + def __str__(self): return str(self.email) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index e274893a2..8e40d4397 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -4,6 +4,7 @@ import logging import textwrap from datetime import datetime +from django.apps import apps from django.conf import settings from django.template.loader import get_template from email.mime.application import MIMEApplication @@ -27,7 +28,7 @@ def send_templated_email( to_address: str, bcc_address="", context={}, - attachment_file: str = None, + attachment_file = None, wrap_email=False, ): """Send an email built from a template to one email address. @@ -39,9 +40,21 @@ def send_templated_email( Raises EmailSendingError if SES client could not be accessed """ - if flag_is_active(None, "disable_email_sending") and not settings.IS_PRODUCTION: # type: ignore - message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." - raise EmailSendingError(message) + + + if not settings.IS_PRODUCTION: # type: ignore + if flag_is_active(None, "disable_email_sending"): # type: ignore + message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." + raise EmailSendingError(message) + else: + # Raise an email sending error if these doesn't exist within our whitelist. + # If these emails don't exist, this function can handle that elsewhere. + AllowedEmail = apps.get_model('registrar', 'AllowedEmail') + message = "Could not send email. The email '{}' does not exist within the whitelist." + if to_address and not AllowedEmail.is_allowed_email(to_address): + raise EmailSendingError(message.format(to_address)) + if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): + raise EmailSendingError(message.format(bcc_address)) template = get_template(template_name) email_body = template.render(context=context) @@ -63,7 +76,7 @@ def send_templated_email( ) logger.info(f"An email was sent! Template name: {template_name} to {to_address}") except Exception as exc: - logger.debug("E-mail unable to send! Could not access the SES client.") + logger.debug("An email was unable to send! Could not access the SES client.") raise EmailSendingError("Could not access the SES client.") from exc destination = {"ToAddresses": [to_address]} @@ -71,7 +84,7 @@ def send_templated_email( destination["BccAddresses"] = [bcc_address] try: - if attachment_file is None: + if not attachment_file: # Wrap the email body to a maximum width of 80 characters per line. # Not all email clients support CSS to do this, and our .txt files require parsing. if wrap_email: From 7fb186e24b32cc9582f9f4484bbfd60ce874e011 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 22 Aug 2024 14:11:28 -0600 Subject: [PATCH 04/22] Display logic in admin --- src/registrar/admin.py | 26 ++++++++++++++++++++++++++ src/registrar/models/domain_request.py | 12 ++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9e5fb71aa..995a00fde 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -7,6 +7,7 @@ from django.db.models import Value, CharField, Q from django.db.models.functions import Concat, Coalesce from django.http import HttpResponseRedirect +from django.conf import settings from django.shortcuts import redirect from django_fsm import get_available_FIELD_transitions, FSMField from registrar.models.domain_information import DomainInformation @@ -306,6 +307,7 @@ def clean(self): return cleaned_data + def _check_for_valid_rejection_reason(self, rejection_reason) -> bool: """ Checks if the rejection_reason field is not none. @@ -1914,6 +1916,19 @@ def save_model(self, request, obj, form, change): else: obj.action_needed_reason_email = default_email + if obj.status in DomainRequest.get_statuses_that_send_emails(): + if not settings.IS_PRODUCTION: + profile_flag = flag_is_active(None, "profile_feature") + if profile_flag and hasattr(obj, "creator"): + recipient = obj.creator + elif not profile_flag and hasattr(obj, "submitter"): + recipient = obj.submitter + else + recipient = None + + if recipient and recipient.email: + self._check_for_valid_email(request, recipient.email) + # == Handle status == # if obj.status == original_obj.status: # If the status hasn't changed, let the base function take care of it @@ -1926,6 +1941,17 @@ def save_model(self, request, obj, form, change): if should_save: return super().save_model(request, obj, form, change) + def _check_for_valid_email(self, request, email): + """Certain emails are whitelisted in non-production environments, + so we should display that information using this function. + + """ + + allowed = models.AllowedEmail.is_allowed_email(email) + error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + if not allowed: + messages.warning(request, error_message) + def _handle_status_change(self, request, obj, original_obj): """ Checks for various conditions when a status change is triggered. diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 966c880d7..812f8e582 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -577,6 +577,18 @@ def get_action_needed_reason_label(cls, action_needed_reason: str): blank=True, ) + @classmethod + def get_statuses_that_send_emails(cls): + """Returns a list of statuses that send an email to the user""" + excluded_statuses = [ + cls.DomainRequestStatus.INELIGIBLE, + cls.DomainRequestStatus.IN_REVIEW + ] + return [ + status for status in cls.DomainRequestStatus + if status not in excluded_statuses + ] + def sync_organization_type(self): """ Updates the organization_type (without saving) to match From c1378acd2c5ae5fa7dfa8034ed2cee1c6071cf7f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 10:46:22 -0600 Subject: [PATCH 05/22] minor fixes + add notes --- src/registrar/admin.py | 7 ++++- src/registrar/fixtures_users.py | 5 ++-- src/registrar/models/allowed_email.py | 5 +++- src/registrar/tests/test_models.py | 38 +++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 995a00fde..6fe7f9849 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1923,9 +1923,11 @@ def save_model(self, request, obj, form, change): recipient = obj.creator elif not profile_flag and hasattr(obj, "submitter"): recipient = obj.submitter - else + else: recipient = None + # Displays a warning in admin when an email cannot be sent, + # Or a success message if it was. if recipient and recipient.email: self._check_for_valid_email(request, recipient.email) @@ -1949,8 +1951,11 @@ def _check_for_valid_email(self, request, email): allowed = models.AllowedEmail.is_allowed_email(email) error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + success_message = f"An email to '{email}' was sent!" if not allowed: messages.warning(request, error_message) + else: + messages.success(request, success_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index ecc30db91..5f57dc93b 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -241,7 +241,8 @@ class UserFixture: }, ] - # Additional emails to add to the AllowedEmail whitelist + # Additional emails to add to the AllowedEmail whitelist. + # The format should be as follows: ["email@igorville.gov", "email2@igorville.gov"] ADDITIONAL_ALLOWED_EMAILS = [ "zander.adkinson@ecstech.com" ] @@ -286,7 +287,7 @@ def load_allowed_emails(cls, users, additional_emails): else: first_name = user_data.get("first_name") last_name = user_data.get("last_name") - logger.warning(f"Could not load email for {first_name} {last_name}: No email exists.") + logger.warning(f"Could add email to whitelist for {first_name} {last_name}: No email exists.") # Load additional emails for email in additional_emails: diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 796f4b556..7cf3df277 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -31,7 +31,10 @@ def is_allowed_email(cls, email): if "+" in local: base_local = local.split("+")[0] base_email = f"{base_local}@{domain}" - allowed_emails = cls.objects.filter(email__iexact=base_email) + allowed_emails = cls.objects.filter( + Q(email__iexact=base_email) | + Q(email__iexact=email) + ) # The string must start with the local, and the plus must be a digit # and occur immediately after the local. The domain should still exist in the email. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index f4e998fff..8c7225841 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2320,3 +2320,41 @@ def test_can_add_urbanization_field(self): self.assertEqual(portfolio.urbanization, "test123") self.assertEqual(portfolio.state_territory, DomainRequest.StateTerritoryChoices.PUERTO_RICO) + + +class TestAllowedEmail(TestCase): + """Tests our allowed email whitelist""" + + @less_console_noise_decorator + def setUp(self): + self.email = "mayor@igorville.gov" + self.domain_name = "igorvilleInTransition.gov" + self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") + self.user, _ = User.objects.get_or_create(email=self.email) + + def tearDown(self): + super().tearDown() + Domain.objects.all().delete() + DomainInvitation.objects.all().delete() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + DraftDomain.objects.all().delete() + TransitionDomain.objects.all().delete() + Portfolio.objects.all().delete() + User.objects.all().delete() + UserDomainRole.objects.all().delete() + + + # Test for a normal email defined in the whitelist + # Test for a normal email NOT defined in the whitelist + + # Test for a +1 email defined in the whitelist + # Test for a +1 email NOT defined in the whitelist + + # Test for a +1 email NOT defined in the whitelist, but the normal is defined + # Test for a +1 email defined in the whitelist, but the normal is NOT defined + # Test for a +1 email NOT defined in the whitelist and NOT defined in the normal + + # Test for an invalid email that contains a '+' + + # TODO: We need a small test for domain request admin \ No newline at end of file From 416633a3ce4392ab566ff52e4fae54a67b284ba4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:25:49 -0600 Subject: [PATCH 06/22] Model unit test --- src/registrar/fixtures_users.py | 2 + src/registrar/models/allowed_email.py | 38 +++++---- src/registrar/tests/test_models.py | 115 +++++++++++++++++++++----- 3 files changed, 116 insertions(+), 39 deletions(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 5f57dc93b..f50afec8f 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -48,6 +48,7 @@ class UserFixture: "username": "eb2214cd-fc0c-48c0-9dbd-bc4cd6820c74", "first_name": "Alysia", "last_name": "Broddrick", + "email": "abroddrick@truss.works", }, { "username": "8f8e7293-17f7-4716-889b-1990241cbd39", @@ -64,6 +65,7 @@ class UserFixture: "username": "83c2b6dd-20a2-4cac-bb40-e22a72d2955c", "first_name": "Cameron", "last_name": "Dixon", + "email": "cameron.dixon@cisa.dhs.gov", }, { "username": "0353607a-cbba-47d2-98d7-e83dcd5b90ea", diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 7cf3df277..7910caf48 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -20,33 +20,37 @@ class AllowedEmail(TimeStampedModel): @classmethod def is_allowed_email(cls, email): """Given an email, check if this email exists within our AllowEmail whitelist""" - print(f"the email is: {email}") + if not email: return False # Split the email into a local part and a domain part - local, domain = email.split('@') + local, domain = email.split("@") + + # If the email exists within the whitelist, then do nothing else. + email_exists = cls.objects.filter(email__iexact=email).exists() + if email_exists: + return True # Check if there's a '+' in the local part if "+" in local: base_local = local.split("+")[0] - base_email = f"{base_local}@{domain}" - allowed_emails = cls.objects.filter( - Q(email__iexact=base_email) | - Q(email__iexact=email) - ) - - # The string must start with the local, and the plus must be a digit - # and occur immediately after the local. The domain should still exist in the email. - pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + base_email_exists = cls.objects.filter( + Q(email__iexact=f"{base_local}@{domain}") + ).exists() - # If the base email exists AND the email matches our expected regex, - # then we can let the email through. - return allowed_emails.exists() and re.match(pattern, email) + # Given an example email, such as "joe.smoe+1@igorville.com" + # The full regex statement will be: "^joe.smoe\\+\\d+@igorville.com$" + pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + return base_email_exists and re.match(pattern, email) else: - # If no '+' exists in the email, just do an exact match - allowed_emails = cls.objects.filter(email__iexact=email) - return allowed_emails.exists() + # Edge case, the +1 record exists but the base does not, + # and the record we are checking is the base record. + pattern = f'^{re.escape(local)}\\+\\d+@{re.escape(domain)}$' + plus_email_exists = cls.objects.filter( + Q(email__iregex=pattern) + ).exists() + return plus_email_exists def __str__(self): return str(self.email) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8c7225841..be5f6da9b 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -17,6 +17,7 @@ DomainInvitation, UserDomainRole, FederalAgency, + AllowedEmail, ) import boto3_mocking @@ -2328,33 +2329,103 @@ class TestAllowedEmail(TestCase): @less_console_noise_decorator def setUp(self): self.email = "mayor@igorville.gov" - self.domain_name = "igorvilleInTransition.gov" - self.domain, _ = Domain.objects.get_or_create(name="igorville.gov") - self.user, _ = User.objects.get_or_create(email=self.email) + self.email_2 = "cake@igorville.gov" + self.plus_email = "mayor+1@igorville.gov" + self.invalid_plus_email = "1+mayor@igorville.gov" def tearDown(self): super().tearDown() - Domain.objects.all().delete() - DomainInvitation.objects.all().delete() - DomainInformation.objects.all().delete() - DomainRequest.objects.all().delete() - DraftDomain.objects.all().delete() - TransitionDomain.objects.all().delete() - Portfolio.objects.all().delete() - User.objects.all().delete() - UserDomainRole.objects.all().delete() - + AllowedEmail.objects.all().delete() + + def test_email_in_whitelist(self): + """Test for a normal email defined in the whitelist""" + AllowedEmail.objects.create(email=self.email) + is_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(is_allowed) + + def test_email_not_in_whitelist(self): + """Test for a normal email NOT defined in the whitelist""" + # Check a email not in the list + is_allowed = AllowedEmail.is_allowed_email(self.email_2) + self.assertFalse(AllowedEmail.objects.filter(email=self.email_2).exists()) + self.assertFalse(is_allowed) + + def test_plus_email_in_whitelist(self): + """Test for a +1 email defined in the whitelist""" + AllowedEmail.objects.create(email=self.plus_email) + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) + + def test_plus_email_not_in_whitelist(self): + """Test for a +1 email not defined in the whitelist""" + # This email should not be allowed. + # Checks that we do more than just a regex check on the record. + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertFalse(plus_email_allowed) + + def test_plus_email_not_in_whitelist_but_base_email_is(self): + """ + Test for a +1 email NOT defined in the whitelist, but the normal one is defined. + Example: + normal (in whitelist) - joe@igorville.com + +1 email (not in whitelist) - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.email) + base_email_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(base_email_allowed) + + # The plus email should also be allowed + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) - # Test for a normal email defined in the whitelist - # Test for a normal email NOT defined in the whitelist + # This email shouldn't exist in the DB + self.assertFalse(AllowedEmail.objects.filter(email=self.plus_email).exists()) - # Test for a +1 email defined in the whitelist - # Test for a +1 email NOT defined in the whitelist + def test_plus_email_in_whitelist_but_base_email_is_not(self): + """ + Test for a +1 email defined in the whitelist, but the normal is NOT defined. + Example: + normal (not in whitelist) - joe@igorville.com + +1 email (in whitelist) - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.plus_email) + plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) + self.assertTrue(plus_email_allowed) - # Test for a +1 email NOT defined in the whitelist, but the normal is defined - # Test for a +1 email defined in the whitelist, but the normal is NOT defined - # Test for a +1 email NOT defined in the whitelist and NOT defined in the normal + # The base email should also be allowed + base_email_allowed = AllowedEmail.is_allowed_email(self.email) + self.assertTrue(base_email_allowed) - # Test for an invalid email that contains a '+' + # This email shouldn't exist in the DB + self.assertFalse(AllowedEmail.objects.filter(email=self.email).exists()) - # TODO: We need a small test for domain request admin \ No newline at end of file + def test_invalid_regex_for_plus_email(self): + """ + Test for an invalid email that contains a '+'. + This base email should still pass, but the regex rule should not. + + Our regex should only pass for emails that end with a '+' + Example: + Invalid email - 1+joe@igorville.com + Valid email: - joe+1@igorville.com + """ + AllowedEmail.objects.create(email=self.invalid_plus_email) + invalid_plus_email = AllowedEmail.is_allowed_email(self.invalid_plus_email) + # We still expect that this will pass, it exists in the db + self.assertTrue(invalid_plus_email) + + # The base email SHOULD NOT pass, as it doesn't match our regex + base_email = AllowedEmail.is_allowed_email(self.email) + self.assertFalse(base_email) + + # For good measure, also check the other plus email + regular_plus_email = AllowedEmail.is_allowed_email(self.plus_email) + self.assertFalse(regular_plus_email) + + # TODO: We need a small test for domain request admin + # We also need a basic test in test_emails based off the mocked is_allowed_email value. + # This will be simpler + # def test_email_in_whitelist_in_prod(self): + # """Tests that the whitelist does nothing when we are in production""" + # allowed_email = AllowedEmail.objects.create(email=self.email) + # self.assertEqual(allowed_email.is_allowed_email(), True) \ No newline at end of file From d95be51ebf7fe39c5b19f47bdb02d2fcd59d6dd7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:30:20 -0600 Subject: [PATCH 07/22] Update test_models.py --- src/registrar/tests/test_models.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index be5f6da9b..3a78fa6df 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -236,7 +236,7 @@ def check_email_sent( self, domain_request, msg, action, expected_count, expected_content=None, expected_email="mayor@igorville.com" ): """Check if an email was sent after performing an action.""" - + email_allowed = AllowedEmail.objects.get_or_create(email=expected_email) with self.subTest(msg=msg, action=action): with boto3_mocking.clients.handler_for("sesv2", self.mock_client): # Perform the specified action @@ -255,6 +255,8 @@ def check_email_sent( email_content = sent_emails[0]["kwargs"]["Content"]["Simple"]["Body"]["Text"]["Data"] self.assertIn(expected_content, email_content) + email_allowed.delete() + @override_flag("profile_feature", active=False) @less_console_noise_decorator def test_submit_from_started_sends_email(self): From 86f520c3f6833857a587962cea8dc1e9617fa997 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 12:48:36 -0600 Subject: [PATCH 08/22] Update test_models.py --- src/registrar/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 3a78fa6df..77f908cf3 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -236,7 +236,7 @@ def check_email_sent( self, domain_request, msg, action, expected_count, expected_content=None, expected_email="mayor@igorville.com" ): """Check if an email was sent after performing an action.""" - email_allowed = AllowedEmail.objects.get_or_create(email=expected_email) + email_allowed, _ = AllowedEmail.objects.get_or_create(email=expected_email) with self.subTest(msg=msg, action=action): with boto3_mocking.clients.handler_for("sesv2", self.mock_client): # Perform the specified action From 866204cb8dccb26308c5d350d460a9eb3eb700ef Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:34:33 -0600 Subject: [PATCH 09/22] fix tests --- src/registrar/admin.py | 3 --- src/registrar/tests/test_admin_request.py | 14 ++++++++++++- src/registrar/tests/test_emails.py | 24 +++++++++++++++++++++++ src/registrar/tests/test_views_domain.py | 3 +++ 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6fe7f9849..f80a2da9e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1951,11 +1951,8 @@ def _check_for_valid_email(self, request, email): allowed = models.AllowedEmail.is_allowed_email(email) error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." - success_message = f"An email to '{email}' was sent!" if not allowed: messages.warning(request, error_message) - else: - messages.success(request, success_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index c4fc8bcee..9b911e415 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -22,6 +22,7 @@ Contact, Website, SeniorOfficial, + AllowedEmail, ) from .common import ( MockSESClient, @@ -52,6 +53,10 @@ class TestDomainRequestAdmin(MockEppLib): tests have available staffuser, superuser, client, admin and test_helper """ + @classmethod + def tearDownClass(cls): + super().tearDownClass() + @classmethod def setUpClass(self): super().setUpClass() @@ -84,6 +89,7 @@ def tearDown(self): def tearDownClass(self): super().tearDownClass() User.objects.all().delete() + AllowedEmail.objects.all().delete() @less_console_noise_decorator def test_domain_request_senior_official_is_alphabetically_sorted(self): @@ -597,7 +603,8 @@ def assert_email_is_accurate( ): """Helper method for the email test cases. email_index is the index of the email in mock_client.""" - + allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) + allowed_bcc_email, _ = AllowedEmail.objects.get_or_create(email=bcc_email_address) with less_console_noise(): # Access the arguments passed to send_email call_args = self.mock_client.EMAILS_SENT @@ -624,6 +631,9 @@ def assert_email_is_accurate( if bcc_email_address: bcc_email = kwargs["Destination"]["BccAddresses"][0] self.assertEqual(bcc_email, bcc_email_address) + + allowed_email.delete() + allowed_bcc_email.delete() @override_settings(IS_PRODUCTION=True) @less_console_noise_decorator @@ -1686,6 +1696,8 @@ def custom_is_active(self): # Patch Domain.is_active and django.contrib.messages.error simultaneously stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) + stack.enter_context(patch.object(messages, "success")) domain_request.status = another_state diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 8cf707004..5fcf7c7df 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -7,6 +7,7 @@ from registrar.utility import email from registrar.utility.email import send_templated_email from .common import completed_domain_request +from registrar.models import AllowedEmail from api.tests.common import less_console_noise_decorator from datetime import datetime @@ -14,9 +15,32 @@ class TestEmails(TestCase): + + @classmethod + def setUpClass(cls): + super().setUpClass() + allowed_emails = [ + AllowedEmail(email="doesnotexist@igorville.com"), + AllowedEmail(email="testy@town.com"), + AllowedEmail(email="mayor@igorville.gov"), + AllowedEmail(email="testy2@town.com"), + AllowedEmail(email="cisaRep@igorville.gov"), + AllowedEmail(email="sender@example.com"), + AllowedEmail(email="recipient@example.com"), + ] + AllowedEmail.objects.bulk_create(allowed_emails) + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + AllowedEmail.objects.all().delete() + def setUp(self): self.mock_client_class = MagicMock() self.mock_client = self.mock_client_class.return_value + + def tearDown(self): + super().tearDown() @boto3_mocking.patching @override_flag("disable_email_sending", active=True) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 3a90543a2..15c2ff92b 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -27,6 +27,7 @@ Domain, DomainInformation, DomainInvitation, + AllowedEmail, Contact, PublicContact, Host, @@ -460,6 +461,7 @@ def test_domain_invitation_email_sent(self): """Inviting a non-existent user sends them an email.""" # make sure there is no user with this email email_address = "mayor@igorville.gov" + allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) User.objects.filter(email=email_address).delete() self.domain_information, _ = DomainInformation.objects.get_or_create(creator=self.user, domain=self.domain) @@ -479,6 +481,7 @@ def test_domain_invitation_email_sent(self): Destination={"ToAddresses": [email_address]}, Content=ANY, ) + allowed_email.delete() @boto3_mocking.patching @less_console_noise_decorator From 9f6d1324d9263a32d2c77e0ef7e58d45b36d28cd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 13:54:16 -0600 Subject: [PATCH 10/22] Fix tests not within helpers --- src/registrar/tests/test_admin_request.py | 15 ++++++++++----- src/registrar/tests/test_views_domain.py | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 9b911e415..27971fc7c 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -576,7 +576,8 @@ def transition_state_and_send_email( ): """Helper method for the email test cases.""" - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), ExitStack() as stack: + stack.enter_context(patch.object(messages, "warning")) # Create a mock request request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) @@ -1155,6 +1156,7 @@ def test_transition_to_rejected_without_rejection_reason_does_trigger_error(self with ExitStack() as stack: stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) domain_request.status = DomainRequest.DomainRequestStatus.REJECTED self.admin.save_model(request, domain_request, None, True) @@ -1183,6 +1185,7 @@ def test_transition_to_rejected_with_rejection_reason_does_not_trigger_error(sel with ExitStack() as stack: stack.enter_context(patch.object(messages, "error")) + stack.enter_context(patch.object(messages, "warning")) domain_request.status = DomainRequest.DomainRequestStatus.REJECTED domain_request.rejection_reason = DomainRequest.RejectionReasons.CONTACTS_OR_ORGANIZATION_LEGITIMACY @@ -1234,11 +1237,13 @@ def test_save_model_sets_approved_domain(self): request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - # Modify the domain request's property - domain_request.status = DomainRequest.DomainRequestStatus.APPROVED + with ExitStack() as stack: + stack.enter_context(patch.object(messages, "warning")) + # Modify the domain request's property + domain_request.status = DomainRequest.DomainRequestStatus.APPROVED - # Use the model admin's save_model method - self.admin.save_model(request, domain_request, form=None, change=True) + # Use the model admin's save_model method + self.admin.save_model(request, domain_request, form=None, change=True) # Test that approved domain exists and equals requested domain self.assertEqual(domain_request.requested_domain.name, domain_request.approved_domain.name) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 15c2ff92b..0db47fb8f 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -345,6 +345,22 @@ def test_domain_readonly_on_detail_page(self): class TestDomainManagers(TestDomainOverview): + @classmethod + def setUpClass(cls): + super().setUpClass() + allowed_emails = [ + AllowedEmail(email=""), + AllowedEmail(email="testy@town.com"), + AllowedEmail(email="mayor@igorville.gov"), + AllowedEmail(email="testy2@town.com"), + ] + AllowedEmail.objects.bulk_create(allowed_emails) + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + AllowedEmail.objects.all().delete() + def tearDown(self): """Ensure that the user has its original permissions""" super().tearDown() @@ -567,6 +583,7 @@ def test_domain_invitation_email_has_email_as_requestor_staff(self): """Inviting a user sends them an email, with email as the name.""" # Create a fake user object email_address = "mayor@igorville.gov" + allowed_email = AllowedEmail.objects.get_or_create(email=email_address) User.objects.get_or_create(email=email_address, username="fakeuser@fakeymail.com") # Make sure the user is staff From 056df7151dbfc79462279904385824f1139a37ad Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:45:44 -0600 Subject: [PATCH 11/22] Fix remaining tests (hopefully) --- src/registrar/admin.py | 9 +++++---- src/registrar/fixtures_users.py | 6 ++---- src/registrar/models/allowed_email.py | 12 ++++-------- src/registrar/models/domain_request.py | 10 ++-------- src/registrar/tests/test_admin_request.py | 12 ++++++------ src/registrar/tests/test_emails.py | 6 +++--- src/registrar/tests/test_models.py | 6 +++--- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/utility/email.py | 6 ++---- 9 files changed, 28 insertions(+), 41 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f80a2da9e..48a5b61c3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -307,7 +307,6 @@ def clean(self): return cleaned_data - def _check_for_valid_rejection_reason(self, rejection_reason) -> bool: """ Checks if the rejection_reason field is not none. @@ -1923,7 +1922,7 @@ def save_model(self, request, obj, form, change): recipient = obj.creator elif not profile_flag and hasattr(obj, "submitter"): recipient = obj.submitter - else: + else: recipient = None # Displays a warning in admin when an email cannot be sent, @@ -1944,9 +1943,9 @@ def save_model(self, request, obj, form, change): return super().save_model(request, obj, form, change) def _check_for_valid_email(self, request, email): - """Certain emails are whitelisted in non-production environments, + """Certain emails are whitelisted in non-production environments, so we should display that information using this function. - + """ allowed = models.AllowedEmail.is_allowed_email(email) @@ -3197,6 +3196,7 @@ def change_view(self, request, object_id, form_url="", extra_context=None): extra_context = {"domain_requests": domain_requests, "domains": domains} return super().change_view(request, object_id, form_url, extra_context) + class AllowedEmailAdmin(ListHeaderAdmin): class Meta: model = models.AllowedEmail @@ -3206,6 +3206,7 @@ class Meta: search_help_text = "Search by email." ordering = ["email"] + admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index f50afec8f..8476c72eb 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -245,9 +245,7 @@ class UserFixture: # Additional emails to add to the AllowedEmail whitelist. # The format should be as follows: ["email@igorville.gov", "email2@igorville.gov"] - ADDITIONAL_ALLOWED_EMAILS = [ - "zander.adkinson@ecstech.com" - ] + ADDITIONAL_ALLOWED_EMAILS = ["zander.adkinson@ecstech.com"] def load_users(cls, users, group_name, are_superusers=False): logger.info(f"Going to load {len(users)} users in group {group_name}") @@ -290,7 +288,7 @@ def load_allowed_emails(cls, users, additional_emails): first_name = user_data.get("first_name") last_name = user_data.get("last_name") logger.warning(f"Could add email to whitelist for {first_name} {last_name}: No email exists.") - + # Load additional emails for email in additional_emails: allowed_emails.append(AllowedEmail(email=email)) diff --git a/src/registrar/models/allowed_email.py b/src/registrar/models/allowed_email.py index 7910caf48..6622bcc55 100644 --- a/src/registrar/models/allowed_email.py +++ b/src/registrar/models/allowed_email.py @@ -35,21 +35,17 @@ def is_allowed_email(cls, email): # Check if there's a '+' in the local part if "+" in local: base_local = local.split("+")[0] - base_email_exists = cls.objects.filter( - Q(email__iexact=f"{base_local}@{domain}") - ).exists() + base_email_exists = cls.objects.filter(Q(email__iexact=f"{base_local}@{domain}")).exists() # Given an example email, such as "joe.smoe+1@igorville.com" # The full regex statement will be: "^joe.smoe\\+\\d+@igorville.com$" - pattern = f'^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$' + pattern = f"^{re.escape(base_local)}\\+\\d+@{re.escape(domain)}$" return base_email_exists and re.match(pattern, email) else: # Edge case, the +1 record exists but the base does not, # and the record we are checking is the base record. - pattern = f'^{re.escape(local)}\\+\\d+@{re.escape(domain)}$' - plus_email_exists = cls.objects.filter( - Q(email__iregex=pattern) - ).exists() + pattern = f"^{re.escape(local)}\\+\\d+@{re.escape(domain)}$" + plus_email_exists = cls.objects.filter(Q(email__iregex=pattern)).exists() return plus_email_exists def __str__(self): diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 812f8e582..02457a539 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -580,14 +580,8 @@ def get_action_needed_reason_label(cls, action_needed_reason: str): @classmethod def get_statuses_that_send_emails(cls): """Returns a list of statuses that send an email to the user""" - excluded_statuses = [ - cls.DomainRequestStatus.INELIGIBLE, - cls.DomainRequestStatus.IN_REVIEW - ] - return [ - status for status in cls.DomainRequestStatus - if status not in excluded_statuses - ] + excluded_statuses = [cls.DomainRequestStatus.INELIGIBLE, cls.DomainRequestStatus.IN_REVIEW] + return [status for status in cls.DomainRequestStatus if status not in excluded_statuses] def sync_organization_type(self): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 27971fc7c..d82826f33 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -56,7 +56,8 @@ class TestDomainRequestAdmin(MockEppLib): @classmethod def tearDownClass(cls): super().tearDownClass() - + AllowedEmail.objects.all.delete() + @classmethod def setUpClass(self): super().setUpClass() @@ -74,6 +75,8 @@ def setUpClass(self): model=DomainRequest, ) self.mock_client = MockSESClient() + allowed_emails = [AllowedEmail(email="mayor@igorville.gov"), AllowedEmail(email="help@get.gov")] + AllowedEmail.objects.bulk_create(allowed_emails) def tearDown(self): super().tearDown() @@ -604,8 +607,8 @@ def assert_email_is_accurate( ): """Helper method for the email test cases. email_index is the index of the email in mock_client.""" - allowed_email, _ = AllowedEmail.objects.get_or_create(email=email_address) - allowed_bcc_email, _ = AllowedEmail.objects.get_or_create(email=bcc_email_address) + AllowedEmail.objects.get_or_create(email=email_address) + AllowedEmail.objects.get_or_create(email=bcc_email_address) with less_console_noise(): # Access the arguments passed to send_email call_args = self.mock_client.EMAILS_SENT @@ -632,9 +635,6 @@ def assert_email_is_accurate( if bcc_email_address: bcc_email = kwargs["Destination"]["BccAddresses"][0] self.assertEqual(bcc_email, bcc_email_address) - - allowed_email.delete() - allowed_bcc_email.delete() @override_settings(IS_PRODUCTION=True) @less_console_noise_decorator diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index 5fcf7c7df..a98c16604 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -15,7 +15,7 @@ class TestEmails(TestCase): - + @classmethod def setUpClass(cls): super().setUpClass() @@ -29,7 +29,7 @@ def setUpClass(cls): AllowedEmail(email="recipient@example.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) - + @classmethod def tearDownClass(cls): super().tearDownClass() @@ -38,7 +38,7 @@ def tearDownClass(cls): def setUp(self): self.mock_client_class = MagicMock() self.mock_client = self.mock_client_class.return_value - + def tearDown(self): super().tearDown() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 77f908cf3..8ee5dac3d 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2360,7 +2360,7 @@ def test_plus_email_in_whitelist(self): def test_plus_email_not_in_whitelist(self): """Test for a +1 email not defined in the whitelist""" - # This email should not be allowed. + # This email should not be allowed. # Checks that we do more than just a regex check on the record. plus_email_allowed = AllowedEmail.is_allowed_email(self.plus_email) self.assertFalse(plus_email_allowed) @@ -2426,8 +2426,8 @@ def test_invalid_regex_for_plus_email(self): # TODO: We need a small test for domain request admin # We also need a basic test in test_emails based off the mocked is_allowed_email value. - # This will be simpler + # This will be simpler # def test_email_in_whitelist_in_prod(self): # """Tests that the whitelist does nothing when we are in production""" # allowed_email = AllowedEmail.objects.create(email=self.email) - # self.assertEqual(allowed_email.is_allowed_email(), True) \ No newline at end of file + # self.assertEqual(allowed_email.is_allowed_email(), True) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 0db47fb8f..0f8b59995 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -355,7 +355,7 @@ def setUpClass(cls): AllowedEmail(email="testy2@town.com"), ] AllowedEmail.objects.bulk_create(allowed_emails) - + @classmethod def tearDownClass(cls): super().tearDownClass() diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 8e40d4397..d77de3ed0 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -28,7 +28,7 @@ def send_templated_email( to_address: str, bcc_address="", context={}, - attachment_file = None, + attachment_file=None, wrap_email=False, ): """Send an email built from a template to one email address. @@ -40,8 +40,6 @@ def send_templated_email( Raises EmailSendingError if SES client could not be accessed """ - - if not settings.IS_PRODUCTION: # type: ignore if flag_is_active(None, "disable_email_sending"): # type: ignore message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." @@ -49,7 +47,7 @@ def send_templated_email( else: # Raise an email sending error if these doesn't exist within our whitelist. # If these emails don't exist, this function can handle that elsewhere. - AllowedEmail = apps.get_model('registrar', 'AllowedEmail') + AllowedEmail = apps.get_model("registrar", "AllowedEmail") message = "Could not send email. The email '{}' does not exist within the whitelist." if to_address and not AllowedEmail.is_allowed_email(to_address): raise EmailSendingError(message.format(to_address)) From e84f346e4bed7ff933dc8657eaee824e24cef71b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Aug 2024 14:57:49 -0600 Subject: [PATCH 12/22] linting --- src/registrar/admin.py | 39 +++++++++++------------ src/registrar/tests/test_admin_request.py | 5 --- src/registrar/tests/test_views_domain.py | 2 +- src/registrar/utility/email.py | 29 ++++++++++------- 4 files changed, 37 insertions(+), 38 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 48a5b61c3..0fe3a2c38 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1915,20 +1915,8 @@ def save_model(self, request, obj, form, change): else: obj.action_needed_reason_email = default_email - if obj.status in DomainRequest.get_statuses_that_send_emails(): - if not settings.IS_PRODUCTION: - profile_flag = flag_is_active(None, "profile_feature") - if profile_flag and hasattr(obj, "creator"): - recipient = obj.creator - elif not profile_flag and hasattr(obj, "submitter"): - recipient = obj.submitter - else: - recipient = None - - # Displays a warning in admin when an email cannot be sent, - # Or a success message if it was. - if recipient and recipient.email: - self._check_for_valid_email(request, recipient.email) + if obj.status in DomainRequest.get_statuses_that_send_emails() and not settings.IS_PRODUCTION: + self._check_for_valid_email(request, obj) # == Handle status == # if obj.status == original_obj.status: @@ -1942,16 +1930,27 @@ def save_model(self, request, obj, form, change): if should_save: return super().save_model(request, obj, form, change) - def _check_for_valid_email(self, request, email): + def _check_for_valid_email(self, request, obj): """Certain emails are whitelisted in non-production environments, so we should display that information using this function. """ - - allowed = models.AllowedEmail.is_allowed_email(email) - error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." - if not allowed: - messages.warning(request, error_message) + profile_flag = flag_is_active(request, "profile_feature") + if profile_flag and hasattr(obj, "creator"): + recipient = obj.creator + elif not profile_flag and hasattr(obj, "submitter"): + recipient = obj.submitter + else: + recipient = None + + # Displays a warning in admin when an email cannot be sent, + # Or a success message if it was. + if recipient and recipient.email: + email = recipient.email + allowed = models.AllowedEmail.is_allowed_email(email) + error_message = f"Could not send email. The email '{email}' does not exist within the whitelist." + if not allowed: + messages.warning(request, error_message) def _handle_status_change(self, request, obj, original_obj): """ diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index d82826f33..46b7c22f2 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -53,11 +53,6 @@ class TestDomainRequestAdmin(MockEppLib): tests have available staffuser, superuser, client, admin and test_helper """ - @classmethod - def tearDownClass(cls): - super().tearDownClass() - AllowedEmail.objects.all.delete() - @classmethod def setUpClass(self): super().setUpClass() diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index 0f8b59995..ae3689703 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -583,7 +583,7 @@ def test_domain_invitation_email_has_email_as_requestor_staff(self): """Inviting a user sends them an email, with email as the name.""" # Create a fake user object email_address = "mayor@igorville.gov" - allowed_email = AllowedEmail.objects.get_or_create(email=email_address) + AllowedEmail.objects.get_or_create(email=email_address) User.objects.get_or_create(email=email_address, username="fakeuser@fakeymail.com") # Make sure the user is staff diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index d77de3ed0..533bd9e99 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -41,18 +41,7 @@ def send_templated_email( """ if not settings.IS_PRODUCTION: # type: ignore - if flag_is_active(None, "disable_email_sending"): # type: ignore - message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." - raise EmailSendingError(message) - else: - # Raise an email sending error if these doesn't exist within our whitelist. - # If these emails don't exist, this function can handle that elsewhere. - AllowedEmail = apps.get_model("registrar", "AllowedEmail") - message = "Could not send email. The email '{}' does not exist within the whitelist." - if to_address and not AllowedEmail.is_allowed_email(to_address): - raise EmailSendingError(message.format(to_address)) - if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): - raise EmailSendingError(message.format(bcc_address)) + _can_send_email(to_address, bcc_address) template = get_template(template_name) email_body = template.render(context=context) @@ -113,6 +102,22 @@ def send_templated_email( raise EmailSendingError("Could not send SES email.") from exc +def _can_send_email(to_address, bcc_address): + """Raises an error if we cannot send an error""" + if flag_is_active(None, "disable_email_sending"): # type: ignore + message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." + raise EmailSendingError(message) + else: + # Raise an email sending error if these doesn't exist within our whitelist. + # If these emails don't exist, this function can handle that elsewhere. + AllowedEmail = apps.get_model("registrar", "AllowedEmail") + message = "Could not send email. The email '{}' does not exist within the whitelist." + if to_address and not AllowedEmail.is_allowed_email(to_address): + raise EmailSendingError(message.format(to_address)) + if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): + raise EmailSendingError(message.format(bcc_address)) + + def wrap_text_and_preserve_paragraphs(text, width): """ Wraps text to `width` preserving newlines; splits on '\n', wraps segments, rejoins with '\n'. From 66dcb038f93363ae7dd3263c2a454bad1cf6b9c0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:08:54 -0600 Subject: [PATCH 13/22] Add documentation + unit test --- docs/developer/README.md | 16 +++++++++ src/registrar/fixtures_users.py | 2 +- src/registrar/tests/test_emails.py | 58 +++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/docs/developer/README.md b/docs/developer/README.md index 358df649c..8e9dbcd40 100644 --- a/docs/developer/README.md +++ b/docs/developer/README.md @@ -97,6 +97,7 @@ While on production (the sandbox referred to as `stable`), an existing analyst o "username": "If this table is populated in a production environment, no change will occur as it will simply be ignored.
\ No newline at end of file +If this table is populated in a production environment, no change will occur as it will simply be ignored.
diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8ee5dac3d..b2c4452f6 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -2423,11 +2423,3 @@ def test_invalid_regex_for_plus_email(self): # For good measure, also check the other plus email regular_plus_email = AllowedEmail.is_allowed_email(self.plus_email) self.assertFalse(regular_plus_email) - - # TODO: We need a small test for domain request admin - # We also need a basic test in test_emails based off the mocked is_allowed_email value. - # This will be simpler - # def test_email_in_whitelist_in_prod(self): - # """Tests that the whitelist does nothing when we are in production""" - # allowed_email = AllowedEmail.objects.create(email=self.email) - # self.assertEqual(allowed_email.is_allowed_email(), True) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 533bd9e99..3d32213a0 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -41,6 +41,9 @@ def send_templated_email( """ if not settings.IS_PRODUCTION: # type: ignore + # Split into a function: C901 'send_templated_email' is too complex. + # Raises an error if we cannot send an email (due to restrictions). + # Does nothing otherwise. _can_send_email(to_address, bcc_address) template = get_template(template_name) @@ -63,7 +66,7 @@ def send_templated_email( ) logger.info(f"An email was sent! Template name: {template_name} to {to_address}") except Exception as exc: - logger.debug("An email was unable to send! Could not access the SES client.") + logger.debug("E-mail unable to send! Could not access the SES client.") raise EmailSendingError("Could not access the SES client.") from exc destination = {"ToAddresses": [to_address]} @@ -103,7 +106,8 @@ def send_templated_email( def _can_send_email(to_address, bcc_address): - """Raises an error if we cannot send an error""" + """Raises an EmailSendingError if we cannot send an email. Does nothing otherwise.""" + if flag_is_active(None, "disable_email_sending"): # type: ignore message = "Could not send email. Email sending is disabled due to flag 'disable_email_sending'." raise EmailSendingError(message) @@ -114,6 +118,7 @@ def _can_send_email(to_address, bcc_address): message = "Could not send email. The email '{}' does not exist within the whitelist." if to_address and not AllowedEmail.is_allowed_email(to_address): raise EmailSendingError(message.format(to_address)) + if bcc_address and not AllowedEmail.is_allowed_email(bcc_address): raise EmailSendingError(message.format(bcc_address)) From 8ff93fade16fbcf87a1275496ba7f9bf7cad6fdd Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 28 Aug 2024 09:28:19 -0600 Subject: [PATCH 16/22] lint --- src/registrar/utility/email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/utility/email.py b/src/registrar/utility/email.py index 3d32213a0..1fe1be596 100644 --- a/src/registrar/utility/email.py +++ b/src/registrar/utility/email.py @@ -42,7 +42,7 @@ def send_templated_email( if not settings.IS_PRODUCTION: # type: ignore # Split into a function: C901 'send_templated_email' is too complex. - # Raises an error if we cannot send an email (due to restrictions). + # Raises an error if we cannot send an email (due to restrictions). # Does nothing otherwise. _can_send_email(to_address, bcc_address) From fb36141c6ee5ed4565881c1ac8df78bc8c9679dc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:09:35 -0600 Subject: [PATCH 17/22] Add rebecca --- src/registrar/fixtures_users.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index e7a71af5d..a45cd2a3d 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -86,6 +86,7 @@ class UserFixture: "username": "2a88a97b-be96-4aad-b99e-0b605b492c78", "first_name": "Rebecca", "last_name": "Hsieh", + "email": "rebecca.hsieh@truss.works", }, { "username": "fa69c8e8-da83-4798-a4f2-263c9ce93f52", From 779ac92ce0a802050580285d8ed82ae4da3c2ba0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:17:45 -0600 Subject: [PATCH 18/22] fix migrations after merge --- .../migrations/{0119_allowedemail.py => 0121_allowedemail.py} | 4 ++-- .../{0120_create_groups_v16.py => 0122_create_groups_v16.py} | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename src/registrar/migrations/{0119_allowedemail.py => 0121_allowedemail.py} (82%) rename src/registrar/migrations/{0120_create_groups_v16.py => 0122_create_groups_v16.py} (96%) diff --git a/src/registrar/migrations/0119_allowedemail.py b/src/registrar/migrations/0121_allowedemail.py similarity index 82% rename from src/registrar/migrations/0119_allowedemail.py rename to src/registrar/migrations/0121_allowedemail.py index 9d63d6973..ebed1ac15 100644 --- a/src/registrar/migrations/0119_allowedemail.py +++ b/src/registrar/migrations/0121_allowedemail.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.10 on 2024-08-22 17:16 +# Generated by Django 4.2.10 on 2024-08-29 18:16 from django.db import migrations, models @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("registrar", "0118_alter_portfolio_options_alter_portfolio_creator_and_more"), + ("registrar", "0120_add_domainrequest_submission_dates"), ] operations = [ diff --git a/src/registrar/migrations/0120_create_groups_v16.py b/src/registrar/migrations/0122_create_groups_v16.py similarity index 96% rename from src/registrar/migrations/0120_create_groups_v16.py rename to src/registrar/migrations/0122_create_groups_v16.py index f08e20805..82c750976 100644 --- a/src/registrar/migrations/0120_create_groups_v16.py +++ b/src/registrar/migrations/0122_create_groups_v16.py @@ -25,7 +25,7 @@ def create_groups(apps, schema_editor) -> Any: class Migration(migrations.Migration): dependencies = [ - ("registrar", "0119_allowedemail"), + ("registrar", "0121_allowedemail"), ] operations = [ From 5a4e9fbe594844095273be064989b0cb0dd96f7f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:39:22 -0600 Subject: [PATCH 19/22] Add more emails --- src/registrar/fixtures_users.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index a45cd2a3d..118881e4f 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -33,6 +33,7 @@ class UserFixture: "username": "aad084c3-66cc-4632-80eb-41cdf5c5bcbf", "first_name": "Aditi", "last_name": "Green", + "email": "aditidevelops+01@gmail.com", }, { "username": "be17c826-e200-4999-9389-2ded48c43691", @@ -43,6 +44,7 @@ class UserFixture: "username": "5f283494-31bd-49b5-b024-a7e7cae00848", "first_name": "Rachid", "last_name": "Mrad", + "email": "rachid.mrad@associates.cisa.dhs.gov ", }, { "username": "eb2214cd-fc0c-48c0-9dbd-bc4cd6820c74", @@ -92,6 +94,7 @@ class UserFixture: "username": "fa69c8e8-da83-4798-a4f2-263c9ce93f52", "first_name": "David", "last_name": "Kennedy", + "email": "david.kennedy@ecstech.com", }, { "username": "f14433d8-f0e9-41bf-9c72-b99b110e665d", @@ -145,6 +148,7 @@ class UserFixture: "username": "ffec5987-aa84-411b-a05a-a7ee5cbcde54", "first_name": "Aditi-Analyst", "last_name": "Green-Analyst", + "email": "aditidevelops+02@gmail.com", }, { "username": "d6bf296b-fac5-47ff-9c12-f88ccc5c1b99", @@ -187,6 +191,7 @@ class UserFixture: "username": "5dc6c9a6-61d9-42b4-ba54-4beff28bac3c", "first_name": "David-Analyst", "last_name": "Kennedy-Analyst", + "email": "david.kennedy@associates.cisa.dhs.gov", }, { "username": "0eb6f326-a3d4-410f-a521-aa4c1fad4e47", @@ -245,7 +250,7 @@ class UserFixture: ] # Additional emails to add to the AllowedEmail whitelist. - ADDITIONAL_ALLOWED_EMAILS: list[str] = [] + ADDITIONAL_ALLOWED_EMAILS: list[str] = ["davekenn4242@gmail.com", "rachid_mrad@hotmail.com"] def load_users(cls, users, group_name, are_superusers=False): logger.info(f"Going to load {len(users)} users in group {group_name}") From 4e08e92bea27ef34b4614e32c213ef4680dc76df Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:42:20 -0600 Subject: [PATCH 20/22] Update fixtures_users.py --- src/registrar/fixtures_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 118881e4f..b3aa237f8 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -44,7 +44,7 @@ class UserFixture: "username": "5f283494-31bd-49b5-b024-a7e7cae00848", "first_name": "Rachid", "last_name": "Mrad", - "email": "rachid.mrad@associates.cisa.dhs.gov ", + "email": "rachid.mrad@associates.cisa.dhs.gov", }, { "username": "eb2214cd-fc0c-48c0-9dbd-bc4cd6820c74", From 6b08c0706905979c3e7c25921f98ae565b59ca3c Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 12:54:59 -0600 Subject: [PATCH 21/22] PR suggestion --- docs/developer/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/developer/README.md b/docs/developer/README.md index 8e9dbcd40..9ddb35352 100644 --- a/docs/developer/README.md +++ b/docs/developer/README.md @@ -134,7 +134,7 @@ Analysts are a variant of the admin role with limited permissions. The process f Do note that if you wish to have both an analyst and admin account, append `-Analyst` to your first and last name, or use a completely different first/last name to avoid confusion. Example: `Bob-Analyst` ## Adding an email address to the email whitelist (sandboxes only) -On all non-production environments, we use an email whitelist table (called `AllowEmail`). This whitelist is not case sensitive, and it provides an inclusion for +1 emails (like example.person+1@igorville.gov). The content after the `+` can be any _digit_. The whitelist checks for the "base" email (example.person) so even if you only have the +1 email defined, an email will still be sent assuming that it follows those conventions. +On all non-production environments, we use an email whitelist table (called `Allowed emails`). This whitelist is not case sensitive, and it provides an inclusion for +1 emails (like example.person+1@igorville.gov). The content after the `+` can be any _digit_. The whitelist checks for the "base" email (example.person) so even if you only have the +1 email defined, an email will still be sent assuming that it follows those conventions. To add yourself to this, you can go about it in three ways. @@ -143,7 +143,7 @@ Permanent (all sandboxes): 2. In src/registrar/fixtures_users.py, add the desired email address to the `ADDITIONAL_ALLOWED_EMAILS` list. This route is suggested for product. Sandbox specific (wiped when the db is reset): -3. Create a new record on the `AllowEmail` table with your email address. This can be done through django admin. +3. Create a new record on the `Allowed emails` table with your email address. This can be done through django admin. More detailed instructions regarding #3 can be found [here](https://docs.google.com/document/d/1ebIz4PcUuoiT7LlVy83EAyHAk_nWPEc99neMp4QjzDs). From b5a6bb69cfe956939b5883659bee9c524133e1c5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 29 Aug 2024 14:29:03 -0600 Subject: [PATCH 22/22] Update fixtures_users.py --- src/registrar/fixtures_users.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index b3aa237f8..8be3e13a2 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -100,6 +100,7 @@ class UserFixture: "username": "f14433d8-f0e9-41bf-9c72-b99b110e665d", "first_name": "Nicolle", "last_name": "LeClair", + "email": "nicolle.leclair@ecstech.com", }, { "username": "24840450-bf47-4d89-8aa9-c612fe68f9da", @@ -203,7 +204,7 @@ class UserFixture: "username": "cfe7c2fc-e24a-480e-8b78-28645a1459b3", "first_name": "Nicolle-Analyst", "last_name": "LeClair-Analyst", - "email": "nicolle.leclair@ecstech.com", + "email": "nicolle.leclair@gmail.com", }, { "username": "378d0bc4-d5a7-461b-bd84-3ae6f6864af9",