diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 849cb6100..8773f7ef8 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3835,6 +3835,9 @@ class PublicContactAdmin(ListHeaderAdmin, ImportExportModelAdmin): change_form_template = "django/admin/email_clipboard_change_form.html" autocomplete_fields = ["domain"] + list_display = ("name", "contact_type", "email", "domain", "registry_id") + search_fields = ["email", "name", "registry_id"] + search_help_text = "Search by email, name or registry id." def changeform_view(self, request, object_id=None, form_url="", extra_context=None): if extra_context is None: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6bd8278a1..f7e76cbc4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -4,9 +4,9 @@ import re from datetime import date, timedelta from typing import Optional +from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore - -from django.db import models +from django.db import models, IntegrityError from django.utils import timezone from typing import Any from registrar.models.host import Host @@ -1329,7 +1329,7 @@ def get_default_security_contact(self): def get_default_administrative_contact(self): """Gets the default administrative contact.""" - logger.info("get_default_security_contact() -> Adding administrative security contact") + logger.info("get_default_administrative_contact() -> Adding administrative security contact") contact = PublicContact.get_default_administrative() contact.domain = self return contact @@ -1665,28 +1665,6 @@ def _make_contact_in_registry(self, contact: PublicContact): ) return err.code - def _fetch_contacts(self, contact_data): - """Fetch contact info.""" - choices = PublicContact.ContactTypeChoices - # We expect that all these fields get populated, - # so we can create these early, rather than waiting. - contacts_dict = { - choices.ADMINISTRATIVE: None, - choices.SECURITY: None, - choices.TECHNICAL: None, - } - for domainContact in contact_data: - req = commands.InfoContact(id=domainContact.contact) - data = registry.send(req, cleaned=True).res_data[0] - - # Map the object we recieved from EPP to a PublicContact - mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) - - # Find/create it in the DB - in_db = self._get_or_create_public_contact(mapped_object) - contacts_dict[in_db.contact_type] = in_db.registry_id - return contacts_dict - def _get_or_create_contact(self, contact: PublicContact): """Try to fetch info about a contact. Create it if it does not exist.""" logger.info("_get_or_create_contact() -> Fetching contact info") @@ -1848,7 +1826,6 @@ def _fix_unknown_state(self, cleaned): """ try: self._add_missing_contacts_if_unknown(cleaned) - except Exception as e: logger.error( "%s couldn't _add_missing_contacts_if_unknown, error was %s." @@ -1866,11 +1843,12 @@ def _add_missing_contacts_if_unknown(self, cleaned): is in an UNKNOWN state, that is an error state) Note: The transition state change happens at the end of the function """ - missingAdmin = True missingSecurity = True missingTech = True + # Potential collision - mismatch between _contacts and contacts? + # But the ID wouldnt match in this case because the default is being grabbed? if len(cleaned.get("_contacts")) < 3: for contact in cleaned.get("_contacts"): if contact.type == PublicContact.ContactTypeChoices.ADMINISTRATIVE: @@ -1890,6 +1868,11 @@ def _add_missing_contacts_if_unknown(self, cleaned): if missingTech: technical_contact = self.get_default_technical_contact() technical_contact.save() + + logger.info( + "_add_missing_contacts_if_unknown => In function. Values are " + f"missingAdmin: {missingAdmin}, missingSecurity: {missingSecurity}, missingTech: {missingTech}" + ) def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" @@ -1956,6 +1939,8 @@ def _update_hosts_and_contacts(self, cleaned, fetch_hosts, fetch_contacts): Additionally, capture and cache old hosts and contacts from cache if they don't exist in cleaned """ + + # object reference issue between self._cache vs cleaned? old_cache_hosts = self._cache.get("hosts") old_cache_contacts = self._cache.get("contacts") @@ -2067,6 +2052,30 @@ def _get_contacts(self, contacts): if contacts and isinstance(contacts, list) and len(contacts) > 0: cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts + + def _fetch_contacts(self, contact_data): + """Fetch contact info.""" + choices = PublicContact.ContactTypeChoices + # We expect that all these fields get populated, + # so we can create these early, rather than waiting. + contacts_dict = { + choices.ADMINISTRATIVE: None, + choices.SECURITY: None, + choices.TECHNICAL: None, + } + for domainContact in contact_data: + req = commands.InfoContact(id=domainContact.contact) + data = registry.send(req, cleaned=True).res_data[0] + logger.info(f"_fetch_contacts => this is the data: {data}") + + # Map the object we recieved from EPP to a PublicContact + mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) + logger.info(f"_fetch_contacts => mapped_object: {mapped_object}") + + # Find/create it in the DB + in_db = self._get_or_create_public_contact(mapped_object) + contacts_dict[in_db.contact_type] = in_db.registry_id + return contacts_dict def _get_hosts(self, hosts): cleaned_hosts = [] @@ -2077,11 +2086,13 @@ def _get_hosts(self, hosts): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" + logger.info(f"in function") db_contact = PublicContact.objects.filter( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) + logger.info(f"db_contact {db_contact}") # If we find duplicates, log it and delete the oldest ones. if db_contact.count() > 1: @@ -2104,8 +2115,24 @@ def _get_or_create_public_contact(self, public_contact: PublicContact): # Save to DB if it doesn't exist already. if db_contact.count() == 0: # Doesn't run custom save logic, just saves to DB - public_contact.save(skip_epp_save=True) - logger.info(f"Created a new PublicContact: {public_contact}") + try: + with transaction.atomic(): + public_contact.save(skip_epp_save=True) + logger.info(f"Created a new PublicContact: {public_contact}") + # In rare cases, _add_missing_contacts_if_unknown will cause a race condition with this function. + # This is because it calls .save(), which is called here. + # + except IntegrityError as err: + logger.error( + "_get_or_create_public_contact() => tried to create a duplicate public contact: " + f"{err}", exc_info=True + ) + return PublicContact.objects.get( + registry_id=public_contact.registry_id, + contact_type=public_contact.contact_type, + domain=self, + ) + # Append the item we just created return public_contact diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 15a88a608..8fde89fd0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -349,6 +349,68 @@ def test_fix_unknown_to_dns_needed_state(self): class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" + @less_console_noise_decorator + def test_get_security_email_race_condition(self): + """ + Scenario: Two processes try to create the same security contact simultaneously + Given a domain in UNKNOWN state + When a race condition occurs during contact creation + Then no IntegrityError is raised + And only one security contact exists in database + And the correct security email is returned + """ + domain, _ = Domain.objects.get_or_create(name="defaultsecurity.gov") + + # Store original method + original_filter = PublicContact.objects.filter + self.first_call = True + def mock_filter(*args, **kwargs): + """ Simulates the race condition by creating a + duplicate contact between filter check and save + """ + result = original_filter(*args, **kwargs) + + # Return empty queryset for first call. Otherwise just proceed as normal. + if self.first_call: + self.first_call = False + duplicate = PublicContact( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY, + registry_id="defaultSec", + email="dotgov@cisa.dhs.gov", + name="Registry Customer Service" + ) + duplicate.save(skip_epp_save=True) + return PublicContact.objects.none() + + return result + + with patch.object(PublicContact.objects, 'filter', side_effect=mock_filter): + try: + public_contact = PublicContact( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY, + registry_id="defaultSec", + email="dotgov@cisa.dhs.gov", + name="Registry Customer Service" + ) + returned_public_contact = domain._get_or_create_public_contact(public_contact) + except IntegrityError: + self.fail( + "IntegrityError was raised during contact creation due to a race condition. " + "This indicates that concurrent contact creation is not working in some cases. " + "The error occurs when two processes try to create the same contact simultaneously. " + "Expected behavior: gracefully handle duplicate creation and return existing contact." + ) + + # Verify only one contact exists + security_contacts = PublicContact.objects.filter( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY + ) + self.assertEqual(security_contacts.count(), 1) + self.assertEqual(returned_public_contact, security_contacts.get()) + @boto3_mocking.patching def test_approved_domain_request_creates_domain_locally(self): """