From c400ecca8c82fbcdadfe58a6d0cf325b1629d8e8 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 14 Sep 2023 11:46:58 -0700 Subject: [PATCH 01/56] Add comments --- src/registrar/models/domain.py | 43 +++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4e6b96de2..d0e0a914b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -228,6 +228,7 @@ class Domain(TimeStampedModel, DomainHelper): """ try: hosts = self._get_property("hosts") + # PRINT THIS -- host response object? except Exception as err: # Don't throw error as this is normal for a new domain # TODO - 433 error handling ticket should address this @@ -246,10 +247,13 @@ class Domain(TimeStampedModel, DomainHelper): def _check_host(self, hostnames: list[str]): """check if host is available, True if available returns boolean""" + # Double check this implementation is needed bc it's untested code + # Check if the IP address is available/real checkCommand = commands.CheckHost(hostnames) try: response = registry.send(checkCommand, cleaned=True) return response.res_data[0].avail + # there will be a .available property on object -- boolean except RegistryError as err: logger.warning( "Couldn't check hosts %s. Errorcode was %s, error was %s", @@ -266,11 +270,14 @@ class Domain(TimeStampedModel, DomainHelper): returns ErrorCode (int)""" logger.info("Creating host") if addrs is not None: + # UNIT TEST: make sure to have 1 with ip address + 1 without addresses = [epp.Ip(addr=addr) for addr in addrs] request = commands.CreateHost(name=host, addrs=addresses) else: + # ip is a specification within the nameserver request = commands.CreateHost(name=host) + # if you talk to registry you MUST do try/except try: logger.info("_create_host()-> sending req as %s" % request) response = registry.send(request, cleaned=True) @@ -288,24 +295,43 @@ class Domain(TimeStampedModel, DomainHelper): # must delete nameservers as well or update # ip version checking may need to be added in a different ticket + # We currently don't have IP address functionality + # We can have multiple IP addresses + # If you have a dotgov, then you HAVE to have at least 1 IP address (can have multiple) + # Nameservers already have IP addresses, these IP addresses are additionals + if len(hosts) > 13: raise ValueError( "Too many hosts provided, you may not have more than 13 nameservers." ) logger.info("Setting nameservers") logger.info(hosts) + + # currenthosts = self.nameservers + # that way you have current hosts + for hostTuple in hosts: - host = hostTuple[0] + host = hostTuple[0].strip() # for removing empty string -- do we need strip? addrs = None if len(hostTuple) > 1: - addrs = hostTuple[1:] + addrs = hostTuple[1:] # list of all the ip address + # do we want to clean the addresses (strip it if not null?) + # is the host a .gov (do .split on the last item), isdotgov can be a boolean + # if you are dotgov and don't have an IP address then raise error + # TRY logger.info() or print() avail = self._check_host([host]) - if avail: - createdCode = self._create_host(host=host, addrs=addrs) + if avail: + createdCode = self._create_host(host=host, addrs=addrs) # creates in registry + # DOUBLE CHECK: _create_host should handle duplicates? # update the domain obj + # if createdCode == ErrorCode.OBJECT_EXISTS: + # duplication check if it's already on the domain -- self.nameservers + # Is it possible for a nameserver to exist and not be on a domain yet? (can one have duplicate name servers) + # NOTE TO ANSWER THIS ON SLACK + # if it isn't in the domain - set a flag so that createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - # add host to domain + # add host to domain (domain already created, just adding to it) request = commands.UpdateDomain( name=self.name, add=[epp.HostObjSet([host])] ) @@ -319,6 +345,8 @@ class Domain(TimeStampedModel, DomainHelper): ) try: + # should we check for contacts? + # check if there are 2 or more name servers or 13 (inclusive) self.ready() self.save() except Exception as err: @@ -714,7 +742,7 @@ class Domain(TimeStampedModel, DomainHelper): req = commands.InfoDomain(name=self.name) domainInfo = registry.send(req, cleaned=True).res_data[0] exitEarly = True - return domainInfo + return domainInfo except RegistryError as e: count += 1 @@ -805,6 +833,7 @@ class Domain(TimeStampedModel, DomainHelper): source=[State.DNS_NEEDED], target=State.READY, ) + # 811 -- Rachid look at constraint on a transition, could just be a function def ready(self): """Transition to the ready state domain should have nameservers and all contacts @@ -816,6 +845,7 @@ class Domain(TimeStampedModel, DomainHelper): # within the transistion itself nameserverList = self.nameservers logger.info("Changing to ready state") + # TEST THIS -- assertValue or print (trigger this) if len(nameserverList) < 2 or len(nameserverList) > 13: raise ValueError("Not ready to become created, cannot transition yet") logger.info("able to transition to ready state") @@ -928,6 +958,7 @@ class Domain(TimeStampedModel, DomainHelper): raise e def _update_or_create_host(self, host): + # maybe take out current code and put here raise NotImplementedError() def _delete_host(self, host): From f2948a77dec33f4702617cb875d4c36fc1d5f9a3 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 21 Sep 2023 16:31:47 -0700 Subject: [PATCH 02/56] Update one nameserver test --- src/registrar/models/domain.py | 24 +++++++++++-------- src/registrar/tests/common.py | 9 +++++++ src/registrar/tests/test_models_domain.py | 29 +++++++++++++++++++---- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 730c9357d..f0567104c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -309,7 +309,7 @@ class Domain(TimeStampedModel, DomainHelper): # currenthosts = self.nameservers # that way you have current hosts - + count = 0 for hostTuple in hosts: host = hostTuple[0].strip() # for removing empty string -- do we need strip? addrs = None @@ -320,7 +320,7 @@ class Domain(TimeStampedModel, DomainHelper): # if you are dotgov and don't have an IP address then raise error # TRY logger.info() or print() avail = self._check_host([host]) - + if avail: createdCode = self._create_host(host=host, addrs=addrs) # creates in registry # DOUBLE CHECK: _create_host should handle duplicates? @@ -328,8 +328,10 @@ class Domain(TimeStampedModel, DomainHelper): # if createdCode == ErrorCode.OBJECT_EXISTS: # duplication check if it's already on the domain -- self.nameservers # Is it possible for a nameserver to exist and not be on a domain yet? (can one have duplicate name servers) - # NOTE TO ANSWER THIS ON SLACK - # if it isn't in the domain - set a flag so that createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + + # TODO: There could be an error here??? + count += 1 + # host can be used by multiple domains if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: # add host to domain (domain already created, just adding to it) request = commands.UpdateDomain( @@ -338,17 +340,19 @@ class Domain(TimeStampedModel, DomainHelper): try: registry.send(request, cleaned=True) + # count += 1 except RegistryError as e: logger.error( "Error adding nameserver, code was %s error was %s" % (e.code, e) ) + # elif createdCode == ErrorCode.OBJECT_EXISTS: + # count += 1 try: - # should we check for contacts? - # check if there are 2 or more name servers or 13 (inclusive) - self.ready() - self.save() + if len(count) >= 2 or len(count) <= 13: + self.ready() + self.save() except Exception as err: logger.info( "nameserver setter checked for create state " @@ -847,8 +851,8 @@ class Domain(TimeStampedModel, DomainHelper): nameserverList = self.nameservers logger.info("Changing to ready state") # TEST THIS -- assertValue or print (trigger this) - if len(nameserverList) < 2 or len(nameserverList) > 13: - raise ValueError("Not ready to become created, cannot transition yet") + # if len(nameserverList) < 2 or len(nameserverList) > 13: + # raise ValueError("Not ready to become created, cannot transition yet") logger.info("able to transition to ready state") def _disclose_fields(self, contact: PublicContact): diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 66d9c2db1..f0ef47223 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -554,12 +554,14 @@ class MockEppLib(TestCase): contacts=..., hosts=..., statuses=..., + avail=..., ): self.auth_info = auth_info self.cr_date = cr_date self.contacts = contacts self.hosts = hosts self.statuses = statuses + self.avail = avail mockDataInfoDomain = fakedEppObject( "fakepw", @@ -583,6 +585,9 @@ class MockEppLib(TestCase): mockDataInfoHosts = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) + mockDataCheckHosts = fakedEppObject( + "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), avail=True, + ) def mockSend(self, _request, cleaned): """Mocks the registry.send function used inside of domain.py @@ -603,6 +608,10 @@ class MockEppLib(TestCase): # use this for when a contact is being updated # sets the second send() to fail raise RegistryError(code=ErrorCode.OBJECT_EXISTS) + elif (isinstance(_request, commands.CheckHost)): + return MagicMock(res_data=[self.mockDataCheckHosts]) + elif (isinstance(_request, commands.CreateHost)): + return MagicMock(res_data=[self.mockDataCheckHosts], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index d35b0ba96..7ff5cecae 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -523,7 +523,7 @@ class TestRegistrantContacts(MockEppLib): raise -class TestRegistrantNameservers(TestCase): +class TestRegistrantNameservers(MockEppLib): """Rule: Registrants may modify their nameservers""" def setUp(self): @@ -532,9 +532,9 @@ class TestRegistrantNameservers(TestCase): Given the registrant is logged in And the registrant is the admin on a domain """ - pass + super().setUp() + self.domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.DNS_NEEDED) - @skip("not implemented yet") def test_user_adds_one_nameserver(self): """ Scenario: Registrant adds a single nameserver @@ -544,7 +544,28 @@ class TestRegistrantNameservers(TestCase): to the registry And `domain.is_active` returns False """ - raise + + # set 1 nameserver + nameserver = "ns1.my-nameserver.com" + self.domain.nameservers = [(nameserver,)] + + # when you create a host, you also have to update at same time + created_host = commands.CreateHost(nameserver) + update_domain_with_created = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host.name])]) + + # checking if commands were sent (commands have to be sent in order) + expectedCalls = [ + call( + commands.CheckHost([created_host.name]), cleaned=True + ), + call(created_host, cleaned=True), + call(update_domain_with_created, cleaned=True), + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls) + + # check that status is still NOT READY + self.assertFalse(self.domain.is_active()) @skip("not implemented yet") def test_user_adds_two_nameservers(self): From 803a469ba003211408ea846bd859e5b361513d8c Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 21 Sep 2023 22:09:23 -0700 Subject: [PATCH 03/56] Add to tests --- src/registrar/models/domain.py | 58 +++---- src/registrar/tests/test_models_domain.py | 191 +++++++++++++++++++++- 2 files changed, 217 insertions(+), 32 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index f0567104c..b09567372 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -228,18 +228,19 @@ class Domain(TimeStampedModel, DomainHelper): """ try: hosts = self._get_property("hosts") - # PRINT THIS -- host response object? + print("HOST IS ") + print(hosts) except Exception as err: - # Don't throw error as this is normal for a new domain - # TODO - 433 error handling ticket should address this + # TODO-848: Check/add to error handling ticket if it's not addressed + # (Don't throw error as this is normal for a new domain?) logger.info("Domain is missing nameservers %s" % err) return [] hostList = [] for host in hosts: - # TODO - this should actually have a second tuple value with the ip address + # TODO-848: This should actually have a second tuple value with the ip address # ignored because uncertain if we will even have a way to display mult. - # and adresses can be a list of mult address + # and adresses can be a list of mult address hostList.append((host["name"],)) return hostList @@ -247,13 +248,12 @@ class Domain(TimeStampedModel, DomainHelper): def _check_host(self, hostnames: list[str]): """check if host is available, True if available returns boolean""" - # Double check this implementation is needed bc it's untested code + # TODO-848: Double check this implementation is needed bc it's untested code # Check if the IP address is available/real checkCommand = commands.CheckHost(hostnames) try: response = registry.send(checkCommand, cleaned=True) return response.res_data[0].avail - # there will be a .available property on object -- boolean except RegistryError as err: logger.warning( "Couldn't check hosts %s. Errorcode was %s, error was %s", @@ -270,14 +270,14 @@ class Domain(TimeStampedModel, DomainHelper): returns ErrorCode (int)""" logger.info("Creating host") if addrs is not None: - # UNIT TEST: make sure to have 1 with ip address + 1 without + # TODO-848: Make sure to have 1 with ip address + 1 without addresses = [epp.Ip(addr=addr) for addr in addrs] request = commands.CreateHost(name=host, addrs=addresses) else: - # ip is a specification within the nameserver + # NOTE-848: ip is a specification within the nameserver request = commands.CreateHost(name=host) - # if you talk to registry you MUST do try/except + # NOTE-848: if you talk to registry you MUST do try/except try: logger.info("_create_host()-> sending req as %s" % request) response = registry.send(request, cleaned=True) @@ -291,9 +291,8 @@ class Domain(TimeStampedModel, DomainHelper): """host should be a tuple of type str, str,... where the elements are Fully qualified host name, addresses associated with the host example: [(ns1.okay.gov, 127.0.0.1, others ips)]""" - # TODO: ticket #848 finish this implementation - # must delete nameservers as well or update - # ip version checking may need to be added in a different ticket + # TODO-848: Finish this implementation of delete + update nameserver + # TODO-848: ip version checking may need to be added in a different ticket # We currently don't have IP address functionality # We can have multiple IP addresses @@ -315,25 +314,23 @@ class Domain(TimeStampedModel, DomainHelper): addrs = None if len(hostTuple) > 1: addrs = hostTuple[1:] # list of all the ip address - # do we want to clean the addresses (strip it if not null?) - # is the host a .gov (do .split on the last item), isdotgov can be a boolean - # if you are dotgov and don't have an IP address then raise error - # TRY logger.info() or print() + # TODO-848: Do we want to clean the addresses (strip it if not null?) + # TODO-848: Check if the host a .gov (do .split on the last item), isdotgov can be a boolean function + # TODO-848: if you are dotgov and don't have an IP address then raise error + # NOTE-848: TRY logger.info() or print() avail = self._check_host([host]) if avail: - createdCode = self._create_host(host=host, addrs=addrs) # creates in registry - # DOUBLE CHECK: _create_host should handle duplicates? - # update the domain obj - # if createdCode == ErrorCode.OBJECT_EXISTS: - # duplication check if it's already on the domain -- self.nameservers - # Is it possible for a nameserver to exist and not be on a domain yet? (can one have duplicate name servers) + # TODO-848: Go through code flow to figure out why count is not incrementing + + createdCode = self._create_host(host=host, addrs=addrs) # creates in registry + # TODO-848: Double check if _create_host should handle duplicates + update domain obj? + # NOTE-848: if createdCode == ErrorCode.OBJECT_EXISTS: --> self.nameservers - # TODO: There could be an error here??? count += 1 - # host can be used by multiple domains + # NOTE-848: Host can be used by multiple domains if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - # add host to domain (domain already created, just adding to it) + # NOTE-848: Add host to domain (domain already created, just adding to it) request = commands.UpdateDomain( name=self.name, add=[epp.HostObjSet([host])] ) @@ -350,6 +347,8 @@ class Domain(TimeStampedModel, DomainHelper): # count += 1 try: + print("COUNT IS ") + print(count) if len(count) >= 2 or len(count) <= 13: self.ready() self.save() @@ -358,8 +357,7 @@ class Domain(TimeStampedModel, DomainHelper): "nameserver setter checked for create state " "and it did not succeed. Error: %s" % err ) - # TODO - handle removed nameservers here will need to change the state - # then go back to DNS_NEEDED + # TODO-848: Handle removed nameservers here, will need to change the state then go back to DNS_NEEDED @Cache def statuses(self) -> list[str]: @@ -967,6 +965,10 @@ class Domain(TimeStampedModel, DomainHelper): raise NotImplementedError() def _delete_host(self, host): + # if len(nameserver_list) < 2: + # change from READY to DNS_NEEDED state + + # Check host to nameserver list, and then use delete command? raise NotImplementedError() def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 7ff5cecae..ab5235136 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -567,7 +567,6 @@ class TestRegistrantNameservers(MockEppLib): # check that status is still NOT READY self.assertFalse(self.domain.is_active()) - @skip("not implemented yet") def test_user_adds_two_nameservers(self): """ Scenario: Registrant adds 2 or more nameservers, thereby activating the domain @@ -577,9 +576,39 @@ class TestRegistrantNameservers(MockEppLib): to the registry And `domain.is_active` returns True """ - raise - @skip("not implemented yet") + # set 2 nameservers + nameserver1 = "ns1.my-nameserver-1.com" + nameserver2 = "ns1.my-nameserver-2.com" + self.domain.nameservers = [(nameserver1,), (nameserver2,)] + + # when you create a host, you also have to update at same time + created_host1 = commands.CreateHost(nameserver1) + update_domain_with_created1 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host1.name])]) + + created_host2 = commands.CreateHost(nameserver2) + update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) + + # checking if commands were sent (commands have to be sent in order) + expectedCalls = [ + call( + commands.CheckHost([created_host1.name]), cleaned=True + ), + call(created_host1, cleaned=True), + call(update_domain_with_created1, cleaned=True), + call( + commands.CheckHost([created_host2.name]), cleaned=True + ), + call(created_host2, cleaned=True), + call(update_domain_with_created2, cleaned=True), + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls) + + # check that status is READY + # TO-FIX: This is currently failing because we are not incrementing count? + self.assertTrue(self.domain.is_active()) + def test_user_adds_too_many_nameservers(self): """ Scenario: Registrant adds 14 or more nameservers @@ -587,7 +616,161 @@ class TestRegistrantNameservers(MockEppLib): When `domain.nameservers` is set to an array of length 14 Then Domain raises a user-friendly error """ - raise + + # set 13+ nameservers + nameserver1 = "ns1.cats-are-superior1.com" + nameserver2 = "ns1.cats-are-superior2.com" + nameserver3 = "ns1.cats-are-superior3.com" + nameserver4 = "ns1.cats-are-superior4.com" + nameserver5 = "ns1.cats-are-superior5.com" + nameserver6 = "ns1.cats-are-superior6.com" + nameserver7 = "ns1.cats-are-superior7.com" + nameserver8 = "ns1.cats-are-superior8.com" + nameserver9 = "ns1.cats-are-superior9.com" + nameserver10 = "ns1.cats-are-superior10.com" + nameserver11 = "ns1.cats-are-superior11.com" + nameserver12 = "ns1.cats-are-superior12.com" + nameserver13 = "ns1.cats-are-superior13.com" + nameserver14 = "ns1.cats-are-superior14.com" + + self.domain.nameservers = [(nameserver1,), (nameserver2,), (nameserver3,), (nameserver4,), + (nameserver5,), (nameserver6,), (nameserver7,), (nameserver8,), (nameserver9), (nameserver10,), + (nameserver11,), (nameserver12,), (nameserver13,), (nameserver14,)] + + # when you create a host, you also have to update at same time + created_host1 = commands.CreateHost(nameserver1) + update_domain_with_created1 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host1.name])]) + + created_host2 = commands.CreateHost(nameserver2) + update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) + + created_host3 = commands.CreateHost(nameserver3) + update_domain_with_created3 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host3.name])]) + + created_host4 = commands.CreateHost(nameserver4) + update_domain_with_created4 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host4.name])]) + + created_host5 = commands.CreateHost(nameserver5) + update_domain_with_created5 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host5.name])]) + + created_host6 = commands.CreateHost(nameserver6) + update_domain_with_created6 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host6.name])]) + + created_host7 = commands.CreateHost(nameserver7) + update_domain_with_created7 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host7.name])]) + + created_host8 = commands.CreateHost(nameserver8) + update_domain_with_created8 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host8.name])]) + + created_host9 = commands.CreateHost(nameserver9) + update_domain_with_created9 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host5.name])]) + + created_host10 = commands.CreateHost(nameserver10) + update_domain_with_created10 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host10.name])]) + + created_host11 = commands.CreateHost(nameserver11) + update_domain_with_created11 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host11.name])]) + + created_host12 = commands.CreateHost(nameserver12) + update_domain_with_created12 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host12.name])]) + + created_host13 = commands.CreateHost(nameserver13) + update_domain_with_created13 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host13.name])]) + + created_host14 = commands.CreateHost(nameserver14) + update_domain_with_created14 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host14.name])]) + + # checking if commands were sent (commands have to be sent in order) + expectedCalls = [ + call( + commands.CheckHost([created_host1.name]), cleaned=True + ), + call(created_host1, cleaned=True), + call(update_domain_with_created1, cleaned=True), + + call( + commands.CheckHost([created_host2.name]), cleaned=True + ), + call(created_host2, cleaned=True), + call(update_domain_with_created2, cleaned=True), + + call( + commands.CheckHost([created_host3.name]), cleaned=True + ), + call(created_host3, cleaned=True), + call(update_domain_with_created3, cleaned=True), + + call( + commands.CheckHost([created_host4.name]), cleaned=True + ), + call(created_host4, cleaned=True), + call(update_domain_with_created4, cleaned=True), + + call( + commands.CheckHost([created_host5.name]), cleaned=True + ), + call(created_host5, cleaned=True), + call(update_domain_with_created5, cleaned=True), + + call( + commands.CheckHost([created_host6.name]), cleaned=True + ), + call(created_host6, cleaned=True), + call(update_domain_with_created6, cleaned=True), + + call( + commands.CheckHost([created_host7.name]), cleaned=True + ), + call(created_host7, cleaned=True), + call(update_domain_with_created7, cleaned=True), + + call( + commands.CheckHost([created_host8.name]), cleaned=True + ), + call(created_host8, cleaned=True), + call(update_domain_with_created8, cleaned=True), + + call( + commands.CheckHost([created_host9.name]), cleaned=True + ), + call(created_host9, cleaned=True), + call(update_domain_with_created9, cleaned=True), + + call( + commands.CheckHost([created_host10.name]), cleaned=True + ), + call(created_host10, cleaned=True), + call(update_domain_with_created10, cleaned=True), + + call( + commands.CheckHost([created_host11.name]), cleaned=True + ), + call(created_host11, cleaned=True), + call(update_domain_with_created11, cleaned=True), + + call( + commands.CheckHost([created_host12.name]), cleaned=True + ), + call(created_host12, cleaned=True), + call(update_domain_with_created12, cleaned=True), + + call( + commands.CheckHost([created_host13.name]), cleaned=True + ), + call(created_host13, cleaned=True), + call(update_domain_with_created13, cleaned=True), + + call( + commands.CheckHost([created_host14.name]), cleaned=True + ), + call(created_host14, cleaned=True), + call(update_domain_with_created14, cleaned=True), + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls) + + # TO-FIX: This is borked because it hits the error as soon as we set up 14 + self.assertRaises(ValueError, namservers) @skip("not implemented yet") def test_user_removes_some_nameservers(self): From c747be074c3b391eb4fb5adf51f379d1d50df22d Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 22 Sep 2023 08:29:15 -0700 Subject: [PATCH 04/56] Fix tests for unsuccessful if less than 2 nameservers, successful if betweeen 2 and 13 nameservers, and unsuccessful if more than 13 nameservers --- src/registrar/models/domain.py | 2 +- src/registrar/tests/test_models_domain.py | 149 ++-------------------- 2 files changed, 13 insertions(+), 138 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index b09567372..9fe52b7df 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -349,7 +349,7 @@ class Domain(TimeStampedModel, DomainHelper): try: print("COUNT IS ") print(count) - if len(count) >= 2 or len(count) <= 13: + if count >= 2 and count <= 13: self.ready() self.save() except Exception as err: diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index ab5235136..f4d44d20d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -603,10 +603,12 @@ class TestRegistrantNameservers(MockEppLib): call(update_domain_with_created2, cleaned=True), ] + print("self.mockedSendFunction.call_args_list is ") + print(self.mockedSendFunction.call_args_list) + self.mockedSendFunction.assert_has_calls(expectedCalls) # check that status is READY - # TO-FIX: This is currently failing because we are not incrementing count? self.assertTrue(self.domain.is_active()) def test_user_adds_too_many_nameservers(self): @@ -632,145 +634,18 @@ class TestRegistrantNameservers(MockEppLib): nameserver12 = "ns1.cats-are-superior12.com" nameserver13 = "ns1.cats-are-superior13.com" nameserver14 = "ns1.cats-are-superior14.com" - - self.domain.nameservers = [(nameserver1,), (nameserver2,), (nameserver3,), (nameserver4,), - (nameserver5,), (nameserver6,), (nameserver7,), (nameserver8,), (nameserver9), (nameserver10,), - (nameserver11,), (nameserver12,), (nameserver13,), (nameserver14,)] - - # when you create a host, you also have to update at same time - created_host1 = commands.CreateHost(nameserver1) - update_domain_with_created1 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host1.name])]) - - created_host2 = commands.CreateHost(nameserver2) - update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) - - created_host3 = commands.CreateHost(nameserver3) - update_domain_with_created3 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host3.name])]) - - created_host4 = commands.CreateHost(nameserver4) - update_domain_with_created4 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host4.name])]) - - created_host5 = commands.CreateHost(nameserver5) - update_domain_with_created5 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host5.name])]) - - created_host6 = commands.CreateHost(nameserver6) - update_domain_with_created6 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host6.name])]) - - created_host7 = commands.CreateHost(nameserver7) - update_domain_with_created7 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host7.name])]) - - created_host8 = commands.CreateHost(nameserver8) - update_domain_with_created8 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host8.name])]) - - created_host9 = commands.CreateHost(nameserver9) - update_domain_with_created9 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host5.name])]) - - created_host10 = commands.CreateHost(nameserver10) - update_domain_with_created10 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host10.name])]) - created_host11 = commands.CreateHost(nameserver11) - update_domain_with_created11 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host11.name])]) - - created_host12 = commands.CreateHost(nameserver12) - update_domain_with_created12 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host12.name])]) - - created_host13 = commands.CreateHost(nameserver13) - update_domain_with_created13 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host13.name])]) - - created_host14 = commands.CreateHost(nameserver14) - update_domain_with_created14 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host14.name])]) - - # checking if commands were sent (commands have to be sent in order) - expectedCalls = [ - call( - commands.CheckHost([created_host1.name]), cleaned=True - ), - call(created_host1, cleaned=True), - call(update_domain_with_created1, cleaned=True), - - call( - commands.CheckHost([created_host2.name]), cleaned=True - ), - call(created_host2, cleaned=True), - call(update_domain_with_created2, cleaned=True), - - call( - commands.CheckHost([created_host3.name]), cleaned=True - ), - call(created_host3, cleaned=True), - call(update_domain_with_created3, cleaned=True), - - call( - commands.CheckHost([created_host4.name]), cleaned=True - ), - call(created_host4, cleaned=True), - call(update_domain_with_created4, cleaned=True), - - call( - commands.CheckHost([created_host5.name]), cleaned=True - ), - call(created_host5, cleaned=True), - call(update_domain_with_created5, cleaned=True), - - call( - commands.CheckHost([created_host6.name]), cleaned=True - ), - call(created_host6, cleaned=True), - call(update_domain_with_created6, cleaned=True), - - call( - commands.CheckHost([created_host7.name]), cleaned=True - ), - call(created_host7, cleaned=True), - call(update_domain_with_created7, cleaned=True), - - call( - commands.CheckHost([created_host8.name]), cleaned=True - ), - call(created_host8, cleaned=True), - call(update_domain_with_created8, cleaned=True), - - call( - commands.CheckHost([created_host9.name]), cleaned=True - ), - call(created_host9, cleaned=True), - call(update_domain_with_created9, cleaned=True), - - call( - commands.CheckHost([created_host10.name]), cleaned=True - ), - call(created_host10, cleaned=True), - call(update_domain_with_created10, cleaned=True), - - call( - commands.CheckHost([created_host11.name]), cleaned=True - ), - call(created_host11, cleaned=True), - call(update_domain_with_created11, cleaned=True), - - call( - commands.CheckHost([created_host12.name]), cleaned=True - ), - call(created_host12, cleaned=True), - call(update_domain_with_created12, cleaned=True), - - call( - commands.CheckHost([created_host13.name]), cleaned=True - ), - call(created_host13, cleaned=True), - call(update_domain_with_created13, cleaned=True), - - call( - commands.CheckHost([created_host14.name]), cleaned=True - ), - call(created_host14, cleaned=True), - call(update_domain_with_created14, cleaned=True), - ] - - self.mockedSendFunction.assert_has_calls(expectedCalls) + def _get_14_nameservers(): + self.domain.nameservers = [(nameserver1,), (nameserver2,), (nameserver3,), (nameserver4,), + (nameserver5,), (nameserver6,), (nameserver7,), (nameserver8,), (nameserver9), (nameserver10,), + (nameserver11,), (nameserver12,), (nameserver13,), (nameserver14,)] + print("!! Hello I am in _get_14_nameservers!") # TO-FIX: This is borked because it hits the error as soon as we set up 14 - self.assertRaises(ValueError, namservers) + self.assertRaises(ValueError, _get_14_nameservers) + print("self.mockedSendFunction.call_args_list is ") + print(self.mockedSendFunction.call_args_list) + self.assertEqual(self.mockedSendFunction.call_count, 0) @skip("not implemented yet") def test_user_removes_some_nameservers(self): From 1c6b1b0e2a513d24e34ec49aec569724c50b62ca Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 25 Sep 2023 10:25:07 -0700 Subject: [PATCH 05/56] removed checkhost, added strip on field, added getNameserverChanges --- src/registrar/forms/domain.py | 4 +- src/registrar/models/domain.py | 114 ++++++++++++++-------- src/registrar/tests/common.py | 10 +- src/registrar/tests/test_models_domain.py | 30 +++--- 4 files changed, 93 insertions(+), 65 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index f14448bcf..e3f14c464 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -20,8 +20,8 @@ class DomainNameserverForm(forms.Form): """Form for changing nameservers.""" - server = forms.CharField(label="Name server") - + server = forms.CharField(label="Name server", strip=True) + #when adding IPs to this form ensure they are stripped as well NameserverFormset = formset_factory( DomainNameserverForm, diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 9fe52b7df..0ad6731ad 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -241,27 +241,28 @@ class Domain(TimeStampedModel, DomainHelper): # TODO-848: This should actually have a second tuple value with the ip address # ignored because uncertain if we will even have a way to display mult. # and adresses can be a list of mult address + hostList.append((host["name"],)) return hostList - def _check_host(self, hostnames: list[str]): - """check if host is available, True if available - returns boolean""" - # TODO-848: Double check this implementation is needed bc it's untested code - # Check if the IP address is available/real - checkCommand = commands.CheckHost(hostnames) - try: - response = registry.send(checkCommand, cleaned=True) - return response.res_data[0].avail - except RegistryError as err: - logger.warning( - "Couldn't check hosts %s. Errorcode was %s, error was %s", - hostnames, - err.code, - err, - ) - return False + # def _check_host(self, hostnames: list[str]): + # """check if host is available, True if available + # returns boolean""" + # # TODO-848: Double check this implementation is needed bc it's untested code + # # Check if the IP address is available/real + # checkCommand = commands.CheckHost(hostnames) + # try: + # response = registry.send(checkCommand, cleaned=True) + # return response.res_data[0].avail + # except RegistryError as err: + # logger.warning( + # "Couldn't check hosts %s. Errorcode was %s, error was %s", + # hostnames, + # err.code, + # err, + # ) + # return False def _create_host(self, host, addrs): """Call _check_host first before using this function, @@ -285,6 +286,35 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error("Error _create_host, code was %s error was %s" % (e.code, e)) return e.code + def getNameserverChanges(self, hosts:list[tuple[str]]): + """ + calls self.nameserver, it should pull from cache but may result + in an epp call + returns tuple of four values as follows: + deleted_values: + updated_values: + new_values: + oldNameservers:""" + oldNameservers=self.nameservers + + previousHostDict = {tup[0]: tup[1:] for tup in oldNameservers} + newHostDict = {tup[0]: tup[1:] for tup in hosts} + + deleted_values = [] + updated_values = [] + new_values = [] + + for key in previousHostDict: + if key not in newHostDict: + deleted_values.append(previousHostDict[key]) + elif newHostDict[key] != previousHostDict[key]: + updated_values.append(newHostDict[key]) + + for key in newHostDict: + if key not in previousHostDict: + new_values.append(newHostDict[key]) + + return (deleted_values,updated_values,new_values, oldNameservers) @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str]]): @@ -306,46 +336,44 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Setting nameservers") logger.info(hosts) - # currenthosts = self.nameservers - # that way you have current hosts + #get the changes made by user and old nameserver values + deleted_values,updated_values,new_values, oldNameservers=self.getNameserverChanges(hosts=hosts) + count = 0 for hostTuple in hosts: - host = hostTuple[0].strip() # for removing empty string -- do we need strip? addrs = None + host=hostTuple[0] if len(hostTuple) > 1: addrs = hostTuple[1:] # list of all the ip address - # TODO-848: Do we want to clean the addresses (strip it if not null?) + # TODO-848: Check if the host a .gov (do .split on the last item), isdotgov can be a boolean function # TODO-848: if you are dotgov and don't have an IP address then raise error # NOTE-848: TRY logger.info() or print() - avail = self._check_host([host]) - if avail: - # TODO-848: Go through code flow to figure out why count is not incrementing - createdCode = self._create_host(host=host, addrs=addrs) # creates in registry - # TODO-848: Double check if _create_host should handle duplicates + update domain obj? - # NOTE-848: if createdCode == ErrorCode.OBJECT_EXISTS: --> self.nameservers + createdCode = self._create_host(host=host, addrs=addrs) # creates in registry + # TODO-848: Double check if _create_host should handle duplicates + update domain obj? + # NOTE-848: if createdCode == ErrorCode.OBJECT_EXISTS: --> self.nameservers - count += 1 - # NOTE-848: Host can be used by multiple domains - if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - # NOTE-848: Add host to domain (domain already created, just adding to it) - request = commands.UpdateDomain( - name=self.name, add=[epp.HostObjSet([host])] + count += 1 + # NOTE-848: Host can be used by multiple domains + if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + # NOTE-848: Add host to domain (domain already created, just adding to it) + request = commands.UpdateDomain( + name=self.name, add=[epp.HostObjSet([host])] + ) + + try: + registry.send(request, cleaned=True) + # count += 1 + except RegistryError as e: + logger.error( + "Error adding nameserver, code was %s error was %s" + % (e.code, e) ) - - try: - registry.send(request, cleaned=True) - # count += 1 - except RegistryError as e: - logger.error( - "Error adding nameserver, code was %s error was %s" - % (e.code, e) - ) # elif createdCode == ErrorCode.OBJECT_EXISTS: # count += 1 - + # unchangedValuesCount=len(oldNameservers)-len(deleted_values)+addedNameservers try: print("COUNT IS ") print(count) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index f0ef47223..c86c6f2ba 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -561,7 +561,7 @@ class MockEppLib(TestCase): self.contacts = contacts self.hosts = hosts self.statuses = statuses - self.avail = avail + self.avail = avail #use for CheckDomain mockDataInfoDomain = fakedEppObject( "fakepw", @@ -585,8 +585,8 @@ class MockEppLib(TestCase): mockDataInfoHosts = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) - mockDataCheckHosts = fakedEppObject( - "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), avail=True, + mockDataCreateHost =fakedEppObject( + "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) def mockSend(self, _request, cleaned): @@ -608,10 +608,8 @@ class MockEppLib(TestCase): # use this for when a contact is being updated # sets the second send() to fail raise RegistryError(code=ErrorCode.OBJECT_EXISTS) - elif (isinstance(_request, commands.CheckHost)): - return MagicMock(res_data=[self.mockDataCheckHosts]) elif (isinstance(_request, commands.CreateHost)): - return MagicMock(res_data=[self.mockDataCheckHosts], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) + return MagicMock(res_data=[self.mockDataCreateHost], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index f4d44d20d..e49b69837 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -534,7 +534,22 @@ class TestRegistrantNameservers(MockEppLib): """ super().setUp() self.domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.DNS_NEEDED) - + def test_get_nameserver_changes(self): + self.domain._cache["hosts"]=[{ + "name": "ns1.example.com", + "addrs": None + }, + {"name": "ns2.example.com", + "addrs": ["1.2.3"] + }, + {"name": "ns3.example.com", + "addrs": None + } + ] + newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] + retTuple=self.domain.getNameserverChanges(newChanges) + print(retTuple) + def test_user_adds_one_nameserver(self): """ Scenario: Registrant adds a single nameserver @@ -555,9 +570,6 @@ class TestRegistrantNameservers(MockEppLib): # checking if commands were sent (commands have to be sent in order) expectedCalls = [ - call( - commands.CheckHost([created_host.name]), cleaned=True - ), call(created_host, cleaned=True), call(update_domain_with_created, cleaned=True), ] @@ -591,14 +603,8 @@ class TestRegistrantNameservers(MockEppLib): # checking if commands were sent (commands have to be sent in order) expectedCalls = [ - call( - commands.CheckHost([created_host1.name]), cleaned=True - ), call(created_host1, cleaned=True), call(update_domain_with_created1, cleaned=True), - call( - commands.CheckHost([created_host2.name]), cleaned=True - ), call(created_host2, cleaned=True), call(update_domain_with_created2, cleaned=True), ] @@ -639,12 +645,8 @@ class TestRegistrantNameservers(MockEppLib): self.domain.nameservers = [(nameserver1,), (nameserver2,), (nameserver3,), (nameserver4,), (nameserver5,), (nameserver6,), (nameserver7,), (nameserver8,), (nameserver9), (nameserver10,), (nameserver11,), (nameserver12,), (nameserver13,), (nameserver14,)] - print("!! Hello I am in _get_14_nameservers!") - # TO-FIX: This is borked because it hits the error as soon as we set up 14 self.assertRaises(ValueError, _get_14_nameservers) - print("self.mockedSendFunction.call_args_list is ") - print(self.mockedSendFunction.call_args_list) self.assertEqual(self.mockedSendFunction.call_count, 0) @skip("not implemented yet") From 9841100a8dd2dac044eaf89d5664b46f6bce3e3e Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 25 Sep 2023 17:12:27 -0700 Subject: [PATCH 06/56] deleted and updated list not correct, needs fixing --- src/registrar/models/domain.py | 38 ++++++++++++++++------- src/registrar/tests/test_models_domain.py | 14 +++++++-- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 0ad6731ad..92505654e 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -241,8 +241,8 @@ class Domain(TimeStampedModel, DomainHelper): # TODO-848: This should actually have a second tuple value with the ip address # ignored because uncertain if we will even have a way to display mult. # and adresses can be a list of mult address - - hostList.append((host["name"],)) + + hostList.append((host["name"],host["addrs"])) return hostList @@ -298,22 +298,36 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers=self.nameservers previousHostDict = {tup[0]: tup[1:] for tup in oldNameservers} - newHostDict = {tup[0]: tup[1:] for tup in hosts} - + print(previousHostDict) + #when slicing the tuple at tup[1:] it is causing it to be a tuple instead of a list + newHostDict = {tup[0]: tup[1:] for tup in hosts} #TODO-when slicing for addresses it should be a list or None + print(f" new host dict {newHostDict}") deleted_values = [] updated_values = [] new_values = [] - for key in previousHostDict: - if key not in newHostDict: - deleted_values.append(previousHostDict[key]) - elif newHostDict[key] != previousHostDict[key]: - updated_values.append(newHostDict[key]) + for prevHost in previousHostDict: + addrs=previousHostDict[prevHost] + # get deleted values-which are values in previous nameserver list + # but are not in the list of new host values + if prevHost not in newHostDict: + deleted_values.append((prevHost,addrs)) + #if the host exists in both, check if the addresses changed + else: + print(f"value in newHostDict[prevHost]{newHostDict[prevHost]}") + print(f"prevhost {prevHost}") + #not right updated_values: [(), (['1.2.4'],)] + + if newHostDict[prevHost] != addrs: + updated_values.append((prevHost,newHostDict[prevHost])) - for key in newHostDict: - if key not in previousHostDict: - new_values.append(newHostDict[key]) + #new value is one that is not in the previous dict + # for key in newHostDict: + # if key not in previousHostDict: + # new_values.append(newHostDict[key]) + + new_values=set(newHostDict)-set(previousHostDict) return (deleted_values,updated_values,new_values, oldNameservers) @nameservers.setter # type: ignore diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index e49b69837..239c75462 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -547,9 +547,17 @@ class TestRegistrantNameservers(MockEppLib): } ] newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] - retTuple=self.domain.getNameserverChanges(newChanges) - print(retTuple) - + deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) + print(f"deleted: {deleted_values}\n") + print(f"updated_values: {updated_values}\n") #has an extra, why? + print(f"new_values: {new_values}\n")# good + print(f"oldNameservers: {oldNameservers}\n") #good + #expecting: + # updated_values==1 -- which is "ns3.example.com" + # newvalues==1 -- which is "ns4.example.com" + # deleted==1 --which is "ns2.example.com" + + # self.assertTrue() def test_user_adds_one_nameserver(self): """ Scenario: Registrant adds a single nameserver From 46f6042bf7495c9eadfb06bb842ae762bad87fc0 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 25 Sep 2023 17:46:19 -0700 Subject: [PATCH 07/56] fixed getNameserverChanges --- src/registrar/models/domain.py | 25 +++++++++++++---------- src/registrar/tests/test_models_domain.py | 9 ++++---- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 92505654e..29ee64612 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -286,6 +286,15 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error("Error _create_host, code was %s error was %s" % (e.code, e)) return e.code + def _convert_list_to_dict(self, listToConvert: list[tuple[str]]): + newDict={} + for tup in listToConvert: + if len(tup) == 1: + newDict[tup[0]] = None + else: + newDict[tup[0]] = tup[1] + return newDict + def getNameserverChanges(self, hosts:list[tuple[str]]): """ calls self.nameserver, it should pull from cache but may result @@ -296,11 +305,11 @@ class Domain(TimeStampedModel, DomainHelper): new_values: oldNameservers:""" oldNameservers=self.nameservers - - previousHostDict = {tup[0]: tup[1:] for tup in oldNameservers} - print(previousHostDict) - #when slicing the tuple at tup[1:] it is causing it to be a tuple instead of a list - newHostDict = {tup[0]: tup[1:] for tup in hosts} #TODO-when slicing for addresses it should be a list or None + + previousHostDict = self._convert_list_to_dict(oldNameservers) + print("previousHostDict {previousHostDict}") + + newHostDict = self._convert_list_to_dict(hosts) print(f" new host dict {newHostDict}") deleted_values = [] updated_values = [] @@ -320,12 +329,6 @@ class Domain(TimeStampedModel, DomainHelper): if newHostDict[prevHost] != addrs: updated_values.append((prevHost,newHostDict[prevHost])) - - - #new value is one that is not in the previous dict - # for key in newHostDict: - # if key not in previousHostDict: - # new_values.append(newHostDict[key]) new_values=set(newHostDict)-set(previousHostDict) return (deleted_values,updated_values,new_values, oldNameservers) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 239c75462..d37a41e02 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -549,15 +549,16 @@ class TestRegistrantNameservers(MockEppLib): newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) print(f"deleted: {deleted_values}\n") - print(f"updated_values: {updated_values}\n") #has an extra, why? - print(f"new_values: {new_values}\n")# good - print(f"oldNameservers: {oldNameservers}\n") #good + print(f"updated_values: {updated_values}\n") + print(f"new_values: {new_values}\n") + print(f"oldNameservers: {oldNameservers}\n") + print("CHANGE") + print(self.domain._convert_list_to_dict(newChanges)) #expecting: # updated_values==1 -- which is "ns3.example.com" # newvalues==1 -- which is "ns4.example.com" # deleted==1 --which is "ns2.example.com" - # self.assertTrue() def test_user_adds_one_nameserver(self): """ Scenario: Registrant adds a single nameserver From 07aa476b6a6ef8ac18fa6caba152101fb600a35b Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Mon, 25 Sep 2023 18:09:25 -0700 Subject: [PATCH 08/56] Add assertion statements and remove cleared todos and some print statements --- src/registrar/models/domain.py | 10 ---------- src/registrar/tests/test_models_domain.py | 19 +++++++++++++------ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 29ee64612..307d2711b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -238,10 +238,6 @@ class Domain(TimeStampedModel, DomainHelper): hostList = [] for host in hosts: - # TODO-848: This should actually have a second tuple value with the ip address - # ignored because uncertain if we will even have a way to display mult. - # and adresses can be a list of mult address - hostList.append((host["name"],host["addrs"])) return hostList @@ -307,10 +303,8 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers=self.nameservers previousHostDict = self._convert_list_to_dict(oldNameservers) - print("previousHostDict {previousHostDict}") newHostDict = self._convert_list_to_dict(hosts) - print(f" new host dict {newHostDict}") deleted_values = [] updated_values = [] new_values = [] @@ -323,10 +317,6 @@ class Domain(TimeStampedModel, DomainHelper): deleted_values.append((prevHost,addrs)) #if the host exists in both, check if the addresses changed else: - print(f"value in newHostDict[prevHost]{newHostDict[prevHost]}") - print(f"prevhost {prevHost}") - #not right updated_values: [(), (['1.2.4'],)] - if newHostDict[prevHost] != addrs: updated_values.append((prevHost,newHostDict[prevHost])) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index d37a41e02..915094e15 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -548,17 +548,24 @@ class TestRegistrantNameservers(MockEppLib): ] newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) - print(f"deleted: {deleted_values}\n") - print(f"updated_values: {updated_values}\n") - print(f"new_values: {new_values}\n") - print(f"oldNameservers: {oldNameservers}\n") - print("CHANGE") - print(self.domain._convert_list_to_dict(newChanges)) + # print(f"deleted: {deleted_values}\n") + # print(f"updated_values: {updated_values}\n") + # print(f"new_values: {new_values}\n") + # print(f"oldNameservers: {oldNameservers}\n") + # print("CHANGE") + # print(self.domain._convert_list_to_dict(newChanges)) + + # Q: This is asserting that 1.2.3 was deleted vs updated -- is this ok or a bug + self.assertEqual(deleted_values, [('ns2.example.com', ['1.2.3'])]) + self.assertTrue(updated_values, [('ns3.example.com', ['1.2.4'])]) + self.assertTrue(new_values, {'ns4.example.com'}) + self.assertTrue(oldNameservers, [('ns1.example.com', None), ('ns2.example.com', ['1.2.3']), ('ns3.example.com', None)]) #expecting: # updated_values==1 -- which is "ns3.example.com" # newvalues==1 -- which is "ns4.example.com" # deleted==1 --which is "ns2.example.com" + def test_user_adds_one_nameserver(self): """ Scenario: Registrant adds a single nameserver From 5523d06e59a7f8357a467ccbaa511d43ebdfe73d Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 26 Sep 2023 13:45:02 -0700 Subject: [PATCH 09/56] Fix from asserttrue to assertequal and update 1 test case --- src/registrar/tests/test_models_domain.py | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 915094e15..6a77c2505 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -534,6 +534,7 @@ class TestRegistrantNameservers(MockEppLib): """ super().setUp() self.domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.DNS_NEEDED) + def test_get_nameserver_changes(self): self.domain._cache["hosts"]=[{ "name": "ns1.example.com", @@ -543,27 +544,16 @@ class TestRegistrantNameservers(MockEppLib): "addrs": ["1.2.3"] }, {"name": "ns3.example.com", - "addrs": None + "addrs": ["1.2.3"] } ] newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) - # print(f"deleted: {deleted_values}\n") - # print(f"updated_values: {updated_values}\n") - # print(f"new_values: {new_values}\n") - # print(f"oldNameservers: {oldNameservers}\n") - # print("CHANGE") - # print(self.domain._convert_list_to_dict(newChanges)) - # Q: This is asserting that 1.2.3 was deleted vs updated -- is this ok or a bug self.assertEqual(deleted_values, [('ns2.example.com', ['1.2.3'])]) - self.assertTrue(updated_values, [('ns3.example.com', ['1.2.4'])]) - self.assertTrue(new_values, {'ns4.example.com'}) - self.assertTrue(oldNameservers, [('ns1.example.com', None), ('ns2.example.com', ['1.2.3']), ('ns3.example.com', None)]) - #expecting: - # updated_values==1 -- which is "ns3.example.com" - # newvalues==1 -- which is "ns4.example.com" - # deleted==1 --which is "ns2.example.com" + self.assertEqual(updated_values, [('ns3.example.com', ['1.2.4'])]) + self.assertEqual(new_values, {'ns4.example.com'}) + self.assertEqual(oldNameservers, [('ns1.example.com', None), ('ns2.example.com', ['1.2.3']), ('ns3.example.com', ['1.2.3'])]) def test_user_adds_one_nameserver(self): From d5dce308bbc9ad88a8f107917cd25256142e5599 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 26 Sep 2023 16:54:01 -0700 Subject: [PATCH 10/56] Looping through changes made by user and deleting and creating and updating plus adding in ip addr work --- src/registrar/models/domain.py | 163 ++++++++++++++-------- src/registrar/tests/common.py | 4 +- src/registrar/tests/test_models_domain.py | 7 +- 3 files changed, 114 insertions(+), 60 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 307d2711b..18cc97c4c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -282,6 +282,7 @@ class Domain(TimeStampedModel, DomainHelper): except RegistryError as e: logger.error("Error _create_host, code was %s error was %s" % (e.code, e)) return e.code + def _convert_list_to_dict(self, listToConvert: list[tuple[str]]): newDict={} for tup in listToConvert: @@ -298,7 +299,7 @@ class Domain(TimeStampedModel, DomainHelper): returns tuple of four values as follows: deleted_values: updated_values: - new_values: + new_values: dict oldNameservers:""" oldNameservers=self.nameservers @@ -319,9 +320,11 @@ class Domain(TimeStampedModel, DomainHelper): else: if newHostDict[prevHost] != addrs: updated_values.append((prevHost,newHostDict[prevHost])) - - new_values=set(newHostDict)-set(previousHostDict) - return (deleted_values,updated_values,new_values, oldNameservers) + + new_values=set(newHostDict)-set(previousHostDict) #returns actually a set + + final_new_values = dict.fromkeys(new_values, None) + return (deleted_values,updated_values,final_new_values, previousHostDict) @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str]]): @@ -344,54 +347,65 @@ class Domain(TimeStampedModel, DomainHelper): logger.info(hosts) #get the changes made by user and old nameserver values - deleted_values,updated_values,new_values, oldNameservers=self.getNameserverChanges(hosts=hosts) - - count = 0 - for hostTuple in hosts: - addrs = None - host=hostTuple[0] - if len(hostTuple) > 1: - addrs = hostTuple[1:] # list of all the ip address + deleted_values, updated_values, new_values, oldNameservers=self.getNameserverChanges(hosts=hosts) + successDeletedCount = 0 + successCreatedCount = 0 - # TODO-848: Check if the host a .gov (do .split on the last item), isdotgov can be a boolean function - # TODO-848: if you are dotgov and don't have an IP address then raise error - # NOTE-848: TRY logger.info() or print() - + for hostTuple in deleted_values: + deleted_response_code = self._delete_host(hostTuple[0]) + if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + successDeletedCount += 1 - createdCode = self._create_host(host=host, addrs=addrs) # creates in registry + for hostTuple in updated_values: + updated_response_code = self._updated_host(hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0])) + + for key, value in new_values.items(): + print("HELLO THERE KEY, VALUE PAIR") + print(key) + print(value) + createdCode = self._create_host(host=key, addrs=value) # creates in registry # TODO-848: Double check if _create_host should handle duplicates + update domain obj? # NOTE-848: if createdCode == ErrorCode.OBJECT_EXISTS: --> self.nameservers - count += 1 # NOTE-848: Host can be used by multiple domains - if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - # NOTE-848: Add host to domain (domain already created, just adding to it) + if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY or createdCode == ErrorCode.OBJECT_EXISTS: request = commands.UpdateDomain( - name=self.name, add=[epp.HostObjSet([host])] + name=self.name, add=[epp.HostObjSet([key])] ) - try: registry.send(request, cleaned=True) - # count += 1 + successCreatedCount += 1 except RegistryError as e: logger.error( "Error adding nameserver, code was %s error was %s" % (e.code, e) ) - # elif createdCode == ErrorCode.OBJECT_EXISTS: - # count += 1 - # unchangedValuesCount=len(oldNameservers)-len(deleted_values)+addedNameservers - try: - print("COUNT IS ") - print(count) - if count >= 2 and count <= 13: + + successTotalNameservers = len(oldNameservers) - successDeletedCount+ successCreatedCount + + + print("SUCCESSTOTALNAMESERVERS IS ") + print(successTotalNameservers) + if successTotalNameservers < 2: + try: + print("DNS_NEEDED: We have less than 2 nameservers") + self.dns_needed() + self.save() + except Exception as err: + logger.info( + "nameserver setter checked for dns_needed state " + "and it did not succeed. Error: %s" % err + ) + elif successTotalNameservers >= 2 and successTotalNameservers <= 13: + try: + print("READY/SAVE: We are in happy path where btwen 2 and 13 inclusive ns") self.ready() self.save() - except Exception as err: - logger.info( - "nameserver setter checked for create state " - "and it did not succeed. Error: %s" % err - ) + except Exception as err: + logger.info( + "nameserver setter checked for create state " + "and it did not succeed. Error: %s" % err + ) # TODO-848: Handle removed nameservers here, will need to change the state then go back to DNS_NEEDED @Cache @@ -792,7 +806,7 @@ class Domain(TimeStampedModel, DomainHelper): if e.code == ErrorCode.OBJECT_DOES_NOT_EXIST: # avoid infinite loop already_tried_to_create = True - self.pendingCreate() + self.dns_needed_from_unknown() self.save() else: logger.error(e) @@ -806,7 +820,7 @@ class Domain(TimeStampedModel, DomainHelper): return registrant.registry_id @transition(field="state", source=State.UNKNOWN, target=State.DNS_NEEDED) - def pendingCreate(self): + def dns_needed_from_unknown(self): logger.info("Changing to dns_needed") registrantID = self.addRegistrant() @@ -862,32 +876,45 @@ class Domain(TimeStampedModel, DomainHelper): # a child host is being used by # another .gov domains. The host must be first removed # and/or renamed before the parent domain may be deleted. - logger.info("pendingCreate()-> inside pending create") + logger.info("dns_needed_from_unknown()-> inside pending create") self._delete_domain() # TODO - delete ticket any additional error handling here + def is_dns_needed(self): + self._invalidate_cache() + nameserverList = self.nameservers + return len(nameserverList) < 2 + @transition( field="state", source=[State.DNS_NEEDED], target=State.READY, + conditions=[lambda x : not is_dns_needed] ) - # 811 -- Rachid look at constraint on a transition, could just be a function def ready(self): """Transition to the ready state domain should have nameservers and all contacts and now should be considered live on a domain """ - # TODO - in nameservers tickets 848 and 562 - # check here if updates need to be made - # consider adding these checks as constraints - # within the transistion itself - nameserverList = self.nameservers logger.info("Changing to ready state") - # TEST THIS -- assertValue or print (trigger this) - # if len(nameserverList) < 2 or len(nameserverList) > 13: - # raise ValueError("Not ready to become created, cannot transition yet") logger.info("able to transition to ready state") + @transition( + field="state", + source=[State.READY], + target=State.DNS_NEEDED, + conditions=[is_dns_needed] + ) + def dns_needed(self): + """Transition to the DNS_NEEDED state + domain should NOT have nameservers but + SHOULD have all contacts + Going to check nameservers and will + result in an EPP call + """ + logger.info("Changing to DNS_NEEDED state") + logger.info("able to transition to DNS_NEEDED state") + def _disclose_fields(self, contact: PublicContact): """creates a disclose object that can be added to a contact Create using .disclose= on the command before sending. @@ -995,17 +1022,43 @@ class Domain(TimeStampedModel, DomainHelper): raise e - def _update_or_create_host(self, host): - # maybe take out current code and put here - raise NotImplementedError() + # TODO: Need to implement this + def is_ipv6(self, ip: str): + return True - def _delete_host(self, host): - # if len(nameserver_list) < 2: - # change from READY to DNS_NEEDED state + def _convert_ips(self, ip_list: list[str]): + edited_ip_list = [] + for ip_addr in ip_list: + if is_ipv6: + edited_ip_list.append(command.Ip(addr=ip_addr, ip="v6")) + else: # default ip addr is v4 + edited_ip_list.append(command.Ip(addr=ip_addr)) + return edited_ip_list - # Check host to nameserver list, and then use delete command? - raise NotImplementedError() + def _update_host(self, nameserver: str, ip_list: list[str], old_ip_list: list[str]): + try: + if len(ip_list) == 0: + return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY + + request = commands.UpdateHost(name=nameserver, add=self._convert_ips(ip_list), rem=self._convert_ips(old_ip_list)) + response = registry.send(request, cleaned=True) + logger.info("_update_host()-> sending req as %s" % request) + return response.code + except RegistryError as e: + logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + return e.code + + def _delete_host(self, nameserver: str): + try: + request = commands.DeleteHost(name=nameserver) + response = registry.send(request, cleaned=True) + logger.info("_delete_host()-> sending req as %s" % request) + return response.code + except RegistryError as e: + logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + return e.code + def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" try: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index c86c6f2ba..5d6a49dae 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -555,6 +555,7 @@ class MockEppLib(TestCase): hosts=..., statuses=..., avail=..., + addrs=..., ): self.auth_info = auth_info self.cr_date = cr_date @@ -562,6 +563,7 @@ class MockEppLib(TestCase): self.hosts = hosts self.statuses = statuses self.avail = avail #use for CheckDomain + self.addrs = addrs mockDataInfoDomain = fakedEppObject( "fakepw", @@ -583,7 +585,7 @@ class MockEppLib(TestCase): "anotherPw", cr_date=datetime.datetime(2023, 7, 25, 19, 45, 35) ) mockDataInfoHosts = fakedEppObject( - "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) + "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), addrs=["1.2.3", "2.3.4"] ) mockDataCreateHost =fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 6a77c2505..d4940bdc0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -291,7 +291,7 @@ class TestRegistrantContacts(MockEppLib): expectedSecContact = PublicContact.get_default_security() expectedSecContact.domain = self.domain - self.domain.pendingCreate() + self.domain.dns_needed_from_unknown() self.assertEqual(self.mockedSendFunction.call_count, 8) self.assertEqual(PublicContact.objects.filter(domain=self.domain).count(), 4) @@ -334,7 +334,7 @@ class TestRegistrantContacts(MockEppLib): created contact of type 'security' """ # make a security contact that is a PublicContact - self.domain.pendingCreate() # make sure a security email already exists + self.domain.dns_needed_from_unknown() # make sure a security email already exists expectedSecContact = PublicContact.get_default_security() expectedSecContact.domain = self.domain expectedSecContact.email = "newEmail@fake.com" @@ -553,8 +553,7 @@ class TestRegistrantNameservers(MockEppLib): self.assertEqual(deleted_values, [('ns2.example.com', ['1.2.3'])]) self.assertEqual(updated_values, [('ns3.example.com', ['1.2.4'])]) self.assertEqual(new_values, {'ns4.example.com'}) - self.assertEqual(oldNameservers, [('ns1.example.com', None), ('ns2.example.com', ['1.2.3']), ('ns3.example.com', ['1.2.3'])]) - + self.assertEqual(oldNameservers, {'ns1.example.com': None, 'ns2.example.com': ['1.2.3'], 'ns3.example.com': ['1.2.3']}) def test_user_adds_one_nameserver(self): """ From 989aad54aa7401c919fffb27f05121d29feb4100 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Wed, 27 Sep 2023 08:59:49 -0700 Subject: [PATCH 11/56] Fix delete host count --- src/registrar/models/domain.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 18cc97c4c..89a2bb502 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -351,7 +351,20 @@ class Domain(TimeStampedModel, DomainHelper): successDeletedCount = 0 successCreatedCount = 0 + print("deleted_values") + print(deleted_values) + print("updated_values") + print(updated_values) + print("new_values") + print(new_values) + print("oldNameservers") + print(oldNameservers) + + for hostTuple in deleted_values: + print("hostTuple in deleted_values") + print(hostTuple) + print(deleted_values) deleted_response_code = self._delete_host(hostTuple[0]) if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: successDeletedCount += 1 @@ -381,7 +394,17 @@ class Domain(TimeStampedModel, DomainHelper): % (e.code, e) ) + print("") successTotalNameservers = len(oldNameservers) - successDeletedCount+ successCreatedCount + + print("len(oldNameservers) IS ") + print(len(oldNameservers)) + print("successDeletedCount IS ") + print(successDeletedCount) + print("successCreatedCount IS ") + print(successCreatedCount) + + print("SUCCESSTOTALNAMESERVERS IS ") From 06dd9a4f19a1a151eb221871bae94c76a448baa3 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 27 Sep 2023 12:34:41 -0700 Subject: [PATCH 12/56] _is_dns_needed not working --- src/registrar/models/domain.py | 33 ++++++++++++++++------- src/registrar/tests/common.py | 8 ++++-- src/registrar/tests/test_models_domain.py | 25 ++++++++++++----- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 89a2bb502..a1303369a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -318,6 +318,8 @@ class Domain(TimeStampedModel, DomainHelper): deleted_values.append((prevHost,addrs)) #if the host exists in both, check if the addresses changed else: + #TODO - host is being updated when previous was None and new is an empty list + #add check here if newHostDict[prevHost] != addrs: updated_values.append((prevHost,newHostDict[prevHost])) @@ -370,8 +372,9 @@ class Domain(TimeStampedModel, DomainHelper): successDeletedCount += 1 for hostTuple in updated_values: - updated_response_code = self._updated_host(hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0])) - + updated_response_code = self._update_host(hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0])) + if updated_response_code not in [ ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, ErrorCode.OBJECT_EXISTS]: + logger.warning("Could not update host %s. Error code was: %s " % (hostTuple[0], updated_response_code)) for key, value in new_values.items(): print("HELLO THERE KEY, VALUE PAIR") print(key) @@ -1052,10 +1055,10 @@ class Domain(TimeStampedModel, DomainHelper): def _convert_ips(self, ip_list: list[str]): edited_ip_list = [] for ip_addr in ip_list: - if is_ipv6: - edited_ip_list.append(command.Ip(addr=ip_addr, ip="v6")) + if self.is_ipv6(): + edited_ip_list.append(epp.Ip(addr=ip_addr, ip="v6")) else: # default ip addr is v4 - edited_ip_list.append(command.Ip(addr=ip_addr)) + edited_ip_list.append(epp.Ip(addr=ip_addr)) return edited_ip_list def _update_host(self, nameserver: str, ip_list: list[str], old_ip_list: list[str]): @@ -1074,13 +1077,23 @@ class Domain(TimeStampedModel, DomainHelper): def _delete_host(self, nameserver: str): try: - request = commands.DeleteHost(name=nameserver) - response = registry.send(request, cleaned=True) - logger.info("_delete_host()-> sending req as %s" % request) + updateReq = commands.UpdateDomain( + name=self.name, rem=[epp.HostObjSet([nameserver])] + ) + response=registry.send(updateReq, cleaned=True) + + logger.info("_delete_host()-> sending update domain req as %s" % updateReq) + + deleteHostReq = commands.DeleteHost(name=nameserver) + response = registry.send(deleteHostReq, cleaned=True) + logger.info("_delete_host()-> sending delete host req as %s" % deleteHostReq) return response.code except RegistryError as e: - logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) - return e.code + if e.code==ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + logger.info("Did not remove host %s because it is in use on another domain." % nameserver) + else: + logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + return e.code def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 5d6a49dae..a177ae1e8 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -587,7 +587,7 @@ class MockEppLib(TestCase): mockDataInfoHosts = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), addrs=["1.2.3", "2.3.4"] ) - mockDataCreateHost =fakedEppObject( + mockDataHostChange =fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) @@ -611,7 +611,11 @@ class MockEppLib(TestCase): # sets the second send() to fail raise RegistryError(code=ErrorCode.OBJECT_EXISTS) elif (isinstance(_request, commands.CreateHost)): - return MagicMock(res_data=[self.mockDataCreateHost], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) + return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) + elif (isinstance(_request, commands.UpdateHost)): + return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) + elif (isinstance(_request, commands.DeleteHost)): + return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index d4940bdc0..da95b724c 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -533,6 +533,10 @@ class TestRegistrantNameservers(MockEppLib): And the registrant is the admin on a domain """ super().setUp() + self.nameserver1 = "ns1.my-nameserver-1.com" + self.nameserver2 = "ns1.my-nameserver-2.com" + self.nameserver3 = "ns1.cats-are-superior3.com" + self.domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.DNS_NEEDED) def test_get_nameserver_changes(self): @@ -549,10 +553,10 @@ class TestRegistrantNameservers(MockEppLib): ] newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) - + print(oldNameservers) self.assertEqual(deleted_values, [('ns2.example.com', ['1.2.3'])]) self.assertEqual(updated_values, [('ns3.example.com', ['1.2.4'])]) - self.assertEqual(new_values, {'ns4.example.com'}) + self.assertEqual(new_values, {'ns4.example.com':None}) self.assertEqual(oldNameservers, {'ns1.example.com': None, 'ns2.example.com': ['1.2.3'], 'ns3.example.com': ['1.2.3']}) def test_user_adds_one_nameserver(self): @@ -595,15 +599,14 @@ class TestRegistrantNameservers(MockEppLib): """ # set 2 nameservers - nameserver1 = "ns1.my-nameserver-1.com" - nameserver2 = "ns1.my-nameserver-2.com" - self.domain.nameservers = [(nameserver1,), (nameserver2,)] + + self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] # when you create a host, you also have to update at same time - created_host1 = commands.CreateHost(nameserver1) + created_host1 = commands.CreateHost(self.nameserver1) update_domain_with_created1 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host1.name])]) - created_host2 = commands.CreateHost(nameserver2) + created_host2 = commands.CreateHost(self.nameserver2) update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) # checking if commands were sent (commands have to be sent in order) @@ -664,6 +667,14 @@ class TestRegistrantNameservers(MockEppLib): to the registry And `domain.is_active` returns True """ + #Given the domain has 3 nameservers + self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,),(self.nameserver3,)] + + #now remove one + self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] + + #assert updatedomain called + #assert call deletehost? raise @skip("not implemented yet") From 026860c88aff480df587ade0d7a1c81b50492aea Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 27 Sep 2023 16:57:03 -0700 Subject: [PATCH 13/56] switches states and updates, deletes and creates hosts --- src/registrar/models/domain.py | 57 ++++++++++++--- src/registrar/tests/common.py | 28 ++++++++ src/registrar/tests/test_models_domain.py | 86 ++++++++++++++++++----- 3 files changed, 144 insertions(+), 27 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a1303369a..a2fd96770 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -292,6 +292,15 @@ class Domain(TimeStampedModel, DomainHelper): newDict[tup[0]] = tup[1] return newDict + def isDotGov(self, nameserver): + return nameserver.find(".gov")!=-1 + + def checkHostIPCombo(self, nameserver:str, ip:list): + if ( self.isDotGov(nameserver) and (ip is None or + ip==[]) ): + raise ValueError("Nameserver %s needs to have an ip address", nameserver) + return None + def getNameserverChanges(self, hosts:list[tuple[str]]): """ calls self.nameserver, it should pull from cache but may result @@ -316,16 +325,25 @@ class Domain(TimeStampedModel, DomainHelper): # but are not in the list of new host values if prevHost not in newHostDict: deleted_values.append((prevHost,addrs)) - #if the host exists in both, check if the addresses changed + # if the host exists in both, check if the addresses changed else: #TODO - host is being updated when previous was None and new is an empty list - #add check here - if newHostDict[prevHost] != addrs: + # add check here + if (newHostDict[prevHost] != addrs + and newHostDict[prevHost] is not None): + # could raise error here if new value is empty and is a dotgov + self.checkHostIPCombo(nameserver=prevHost, ip=newHostDict[prevHost]) updated_values.append((prevHost,newHostDict[prevHost])) new_values=set(newHostDict)-set(previousHostDict) #returns actually a set final_new_values = dict.fromkeys(new_values, None) + # loop in final new values to check for .gov and missing addresses + for nameserver, ip in final_new_values.items(): + # check the new values for missing IPs + # raise error if missing + self.checkHostIPCombo(nameserver=nameserver,ip=ip) + return (deleted_values,updated_values,final_new_values, previousHostDict) @nameservers.setter # type: ignore @@ -333,7 +351,7 @@ class Domain(TimeStampedModel, DomainHelper): """host should be a tuple of type str, str,... where the elements are Fully qualified host name, addresses associated with the host example: [(ns1.okay.gov, 127.0.0.1, others ips)]""" - # TODO-848: Finish this implementation of delete + update nameserver + # TODO-848: ip version checking may need to be added in a different ticket # We currently don't have IP address functionality @@ -906,16 +924,29 @@ class Domain(TimeStampedModel, DomainHelper): self._delete_domain() # TODO - delete ticket any additional error handling here - def is_dns_needed(self): - self._invalidate_cache() - nameserverList = self.nameservers - return len(nameserverList) < 2 + # def is_dns_needed(self): + # """Commented out and kept in the codebase + # as this call should be made, but adds + # a lot of processing time + # when EPP calling is made more efficient + # this should be added back in + # The goal is to double check that + # the nameservers we set are in fact + # on the registry + # """ + # self._invalidate_cache() + # nameserverList = self.nameservers + # return len(nameserverList) < 2 + + # def dns_not_needed(self): + # return not self.is_dns_needed() + @transition( field="state", source=[State.DNS_NEEDED], target=State.READY, - conditions=[lambda x : not is_dns_needed] + # conditions=[dns_not_needed] ) def ready(self): """Transition to the ready state @@ -929,7 +960,7 @@ class Domain(TimeStampedModel, DomainHelper): field="state", source=[State.READY], target=State.DNS_NEEDED, - conditions=[is_dns_needed] + # conditions=[is_dns_needed] ) def dns_needed(self): """Transition to the DNS_NEEDED state @@ -1054,17 +1085,21 @@ class Domain(TimeStampedModel, DomainHelper): def _convert_ips(self, ip_list: list[str]): edited_ip_list = [] + if ip_list is None: + return [] + for ip_addr in ip_list: if self.is_ipv6(): edited_ip_list.append(epp.Ip(addr=ip_addr, ip="v6")) else: # default ip addr is v4 edited_ip_list.append(epp.Ip(addr=ip_addr)) + return edited_ip_list def _update_host(self, nameserver: str, ip_list: list[str], old_ip_list: list[str]): try: - if len(ip_list) == 0: + if ip_list is None or len(ip_list) == 0 and isinstance(old_ip_list,list) and len(old_ip_list)!=0 : return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY request = commands.UpdateHost(name=nameserver, add=self._convert_ips(ip_list), rem=self._convert_ips(old_ip_list)) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index a177ae1e8..9da6c5e00 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -581,15 +581,36 @@ class MockEppLib(TestCase): contacts=[], hosts=["fake.host.com"], ) + infoDomainThreeHosts =fakedEppObject( + "my-nameserver.gov", + cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + contacts=[], + hosts=["ns1.my-nameserver-1.com","ns1.my-nameserver-2.com","ns1.cats-are-superior3.com"], + ) + infoDomainNoHost =fakedEppObject( + "my-nameserver.gov", + cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + contacts=[], + hosts=[], + ) + infoDomainTwoHosts =fakedEppObject( + "my-nameserver.gov", + cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + contacts=[], + hosts=["ns1.my-nameserver-1.com","ns1.my-nameserver-2.com"], + ) mockDataInfoContact = fakedEppObject( "anotherPw", cr_date=datetime.datetime(2023, 7, 25, 19, 45, 35) ) mockDataInfoHosts = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), addrs=["1.2.3", "2.3.4"] ) + mockDataHostChange =fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) + + extendedValues=False def mockSend(self, _request, cleaned): """Mocks the registry.send function used inside of domain.py @@ -599,6 +620,13 @@ class MockEppLib(TestCase): if isinstance(_request, commands.InfoDomain): if getattr(_request, "name", None) == "security.gov": return MagicMock(res_data=[self.infoDomainNoContact]) + elif getattr(_request, "name", None) == "my-nameserver.gov": + if self.extendedValues: + return MagicMock(res_data=[self.infoDomainThreeHosts]) + elif self.mockedSendFunction.call_count==5: ## remove this breaks anything? + return MagicMock(res_data=[self.infoDomainTwoHosts]) + else: + return MagicMock(res_data=[self.infoDomainNoHost]) return MagicMock(res_data=[self.mockDataInfoDomain]) elif isinstance(_request, commands.InfoContact): return MagicMock(res_data=[self.mockDataInfoContact]) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index da95b724c..d9538baca 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -599,7 +599,7 @@ class TestRegistrantNameservers(MockEppLib): """ # set 2 nameservers - + print("DOCKER DIDNT SUCK THIS TIME") self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] # when you create a host, you also have to update at same time @@ -609,8 +609,10 @@ class TestRegistrantNameservers(MockEppLib): created_host2 = commands.CreateHost(self.nameserver2) update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) + infoDomain = commands.InfoDomain(name='my-nameserver.gov', auth_info=None) # checking if commands were sent (commands have to be sent in order) expectedCalls = [ + call(infoDomain, cleaned=True), call(created_host1, cleaned=True), call(update_domain_with_created1, cleaned=True), call(created_host2, cleaned=True), @@ -620,8 +622,8 @@ class TestRegistrantNameservers(MockEppLib): print("self.mockedSendFunction.call_args_list is ") print(self.mockedSendFunction.call_args_list) - self.mockedSendFunction.assert_has_calls(expectedCalls) - + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertEqual(5, self.mockedSendFunction.call_count) # check that status is READY self.assertTrue(self.domain.is_active()) @@ -657,7 +659,6 @@ class TestRegistrantNameservers(MockEppLib): self.assertRaises(ValueError, _get_14_nameservers) self.assertEqual(self.mockedSendFunction.call_count, 0) - @skip("not implemented yet") def test_user_removes_some_nameservers(self): """ Scenario: Registrant removes some nameservers, while keeping at least 2 @@ -667,29 +668,59 @@ class TestRegistrantNameservers(MockEppLib): to the registry And `domain.is_active` returns True """ - #Given the domain has 3 nameservers - self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,),(self.nameserver3,)] - #now remove one + # Mock is set to return 3 nameservers on infodomain + self.extendedValues=True self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] + expectedCalls=[ + # calls info domain, and info on all hosts + # to get past values + # then removes the single host and updates domain + call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), + call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.cats-are-superior3.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), + call(commands.DeleteHost(name='ns1.cats-are-superior3.com'), cleaned=True)] - #assert updatedomain called - #assert call deletehost? - raise + print("self.mockedSendFunction.call_args_list is ") + print(self.mockedSendFunction.call_args_list) + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertTrue(self.domain.is_active()) - @skip("not implemented yet") + def test_user_removes_too_many_nameservers(self): """ Scenario: Registrant removes some nameservers, bringing the total to less than 2 - Given the domain has 3 nameservers + Given the domain has 2 nameservers When `domain.nameservers` is set to an array containing nameserver #1 Then `commands.UpdateDomain` and `commands.DeleteHost` is sent to the registry And `domain.is_active` returns False + """ - raise + self.extendedValues=True + print("domain state") + print(self.domain.state) + self.domain.ready() + print("Domain state is now") + print(self.domain.state) + self.domain.nameservers = [(self.nameserver1,)] + print("self.mockedSendFunction.call_args_list is ") + print(self.mockedSendFunction.call_args_list) + expectedCalls=[call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), + call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.my-nameserver-2.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), + call(commands.DeleteHost(name='ns1.my-nameserver-2.com'), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.cats-are-superior3.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), + call(commands.DeleteHost(name='ns1.cats-are-superior3.com'), cleaned=True)] + + + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertFalse(self.domain.is_active()) - @skip("not implemented yet") def test_user_replaces_nameservers(self): """ Scenario: Registrant simultaneously adds and removes some nameservers @@ -700,9 +731,27 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - raise + self.extendedValues=True + self.domain.ready() + self.domain.nameservers=[(self.nameserver1,), ("ns1.cats-are-superior1.com",), ("ns1.cats-are-superior2.com",)] + print("self.mockedSendFunction.call_args_list is ") + print(self.mockedSendFunction.call_args_list) + expectedCalls=[call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), + call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), + call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.my-nameserver-2.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), + call(commands.DeleteHost(name='ns1.my-nameserver-2.com'), cleaned=True), + call(commands.CreateHost(name='ns1.cats-are-superior1.com', addrs=[]), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[common.HostObjSet(hosts=['ns1.cats-are-superior1.com'])], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), + call(commands.CreateHost(name='ns1.cats-are-superior2.com', addrs=[]), cleaned=True), + call(commands.UpdateDomain(name='my-nameserver.gov', add=[common.HostObjSet(hosts=['ns1.cats-are-superior2.com'])], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True) + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertTrue(self.domain.is_active()) + - @skip("not implemented yet") def test_user_cannot_add_subordinate_without_ip(self): """ Scenario: Registrant adds a nameserver which is a subdomain of their .gov @@ -711,6 +760,8 @@ class TestRegistrantNameservers(MockEppLib): with a subdomain of the domain and no IP addresses Then Domain raises a user-friendly error """ + #add a nameserver with a .gov and no ip + ##assertRaises error raise @skip("not implemented yet") @@ -758,6 +809,9 @@ class TestRegistrantNameservers(MockEppLib): """ raise + def tearDown(self): + self.extendedValues=False + return super().tearDown() class TestRegistrantDNSSEC(TestCase): """Rule: Registrants may modify their secure DNS data""" From 1f149700e3711f73696f54c369dd3c512f11031f Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 28 Sep 2023 15:55:54 -0700 Subject: [PATCH 14/56] Checking for subdomains, fixing more ip logic stuff and tests --- src/registrar/forms/domain.py | 3 +- src/registrar/models/domain.py | 215 ++++++++------- src/registrar/tests/common.py | 63 +++-- src/registrar/tests/test_models_domain.py | 316 ++++++++++++++++------ 4 files changed, 401 insertions(+), 196 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index e3f14c464..a905b58ae 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -21,7 +21,8 @@ class DomainNameserverForm(forms.Form): """Form for changing nameservers.""" server = forms.CharField(label="Name server", strip=True) - #when adding IPs to this form ensure they are stripped as well + # when adding IPs to this form ensure they are stripped as well + NameserverFormset = formset_factory( DomainNameserverForm, diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a2fd96770..825503026 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -228,8 +228,6 @@ class Domain(TimeStampedModel, DomainHelper): """ try: hosts = self._get_property("hosts") - print("HOST IS ") - print(hosts) except Exception as err: # TODO-848: Check/add to error handling ticket if it's not addressed # (Don't throw error as this is normal for a new domain?) @@ -238,7 +236,7 @@ class Domain(TimeStampedModel, DomainHelper): hostList = [] for host in hosts: - hostList.append((host["name"],host["addrs"])) + hostList.append((host["name"], host["addrs"])) return hostList @@ -267,14 +265,11 @@ class Domain(TimeStampedModel, DomainHelper): returns ErrorCode (int)""" logger.info("Creating host") if addrs is not None: - # TODO-848: Make sure to have 1 with ip address + 1 without addresses = [epp.Ip(addr=addr) for addr in addrs] request = commands.CreateHost(name=host, addrs=addresses) else: - # NOTE-848: ip is a specification within the nameserver request = commands.CreateHost(name=host) - # NOTE-848: if you talk to registry you MUST do try/except try: logger.info("_create_host()-> sending req as %s" % request) response = registry.send(request, cleaned=True) @@ -284,35 +279,41 @@ class Domain(TimeStampedModel, DomainHelper): return e.code def _convert_list_to_dict(self, listToConvert: list[tuple[str]]): - newDict={} + newDict = {} + + # TODO-848: If duplicated nameserver names, throw error for tup in listToConvert: if len(tup) == 1: newDict[tup[0]] = None else: newDict[tup[0]] = tup[1] return newDict - - def isDotGov(self, nameserver): - return nameserver.find(".gov")!=-1 - - def checkHostIPCombo(self, nameserver:str, ip:list): - if ( self.isDotGov(nameserver) and (ip is None or - ip==[]) ): - raise ValueError("Nameserver %s needs to have an ip address", nameserver) + + def isSubdomain(self, nameserver): + return nameserver.find(self.name) != -1 + + def checkHostIPCombo(self, nameserver: str, ip: list): + if self.isSubdomain(nameserver) and (ip is None or ip == []): + raise ValueError("Nameserver %s needs to have an ip address" % nameserver) + elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): + raise ValueError( + "Nameserver %s cannot be linked " + "because %s is not a subdomain" % (nameserver, ip) + ) return None - def getNameserverChanges(self, hosts:list[tuple[str]]): - """ + def getNameserverChanges(self, hosts: list[tuple[str]]): + """ calls self.nameserver, it should pull from cache but may result in an epp call - returns tuple of four values as follows: + returns tuple of four values as follows: deleted_values: - updated_values: + updated_values: new_values: dict oldNameservers:""" - oldNameservers=self.nameservers + oldNameservers = self.nameservers - previousHostDict = self._convert_list_to_dict(oldNameservers) + previousHostDict = self._convert_list_to_dict(oldNameservers) newHostDict = self._convert_list_to_dict(hosts) deleted_values = [] @@ -320,44 +321,39 @@ class Domain(TimeStampedModel, DomainHelper): new_values = [] for prevHost in previousHostDict: - addrs=previousHostDict[prevHost] + addrs = previousHostDict[prevHost] # get deleted values-which are values in previous nameserver list # but are not in the list of new host values if prevHost not in newHostDict: - deleted_values.append((prevHost,addrs)) + deleted_values.append((prevHost, addrs)) # if the host exists in both, check if the addresses changed else: - #TODO - host is being updated when previous was None and new is an empty list + # TODO - host is being updated when previous was None and new is an empty list # add check here - if (newHostDict[prevHost] != addrs - and newHostDict[prevHost] is not None): - # could raise error here if new value is empty and is a dotgov + if newHostDict[prevHost] is not None and set( + newHostDict[prevHost] + ) != set(addrs): self.checkHostIPCombo(nameserver=prevHost, ip=newHostDict[prevHost]) - updated_values.append((prevHost,newHostDict[prevHost])) + updated_values.append((prevHost, newHostDict[prevHost])) - new_values=set(newHostDict)-set(previousHostDict) #returns actually a set + new_values = { + key: newHostDict.get(key) + for key in newHostDict + if key not in previousHostDict + } - final_new_values = dict.fromkeys(new_values, None) - # loop in final new values to check for .gov and missing addresses - for nameserver, ip in final_new_values.items(): - # check the new values for missing IPs - # raise error if missing - self.checkHostIPCombo(nameserver=nameserver,ip=ip) - - return (deleted_values,updated_values,final_new_values, previousHostDict) + for nameserver, ip in new_values.items(): + self.checkHostIPCombo(nameserver=nameserver, ip=ip) + + return (deleted_values, updated_values, new_values, previousHostDict) @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str]]): """host should be a tuple of type str, str,... where the elements are Fully qualified host name, addresses associated with the host example: [(ns1.okay.gov, 127.0.0.1, others ips)]""" - - # TODO-848: ip version checking may need to be added in a different ticket - # We currently don't have IP address functionality - # We can have multiple IP addresses - # If you have a dotgov, then you HAVE to have at least 1 IP address (can have multiple) - # Nameservers already have IP addresses, these IP addresses are additionals + # TODO-848: ip version checking may need to be added in a different ticket if len(hosts) > 13: raise ValueError( @@ -366,9 +362,14 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Setting nameservers") logger.info(hosts) - #get the changes made by user and old nameserver values - deleted_values, updated_values, new_values, oldNameservers=self.getNameserverChanges(hosts=hosts) - successDeletedCount = 0 + # get the changes made by user and old nameserver values + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.getNameserverChanges(hosts=hosts) + successDeletedCount = 0 successCreatedCount = 0 print("deleted_values") @@ -380,29 +381,31 @@ class Domain(TimeStampedModel, DomainHelper): print("oldNameservers") print(oldNameservers) - for hostTuple in deleted_values: - print("hostTuple in deleted_values") - print(hostTuple) - print(deleted_values) deleted_response_code = self._delete_host(hostTuple[0]) if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - successDeletedCount += 1 + successDeletedCount += 1 for hostTuple in updated_values: - updated_response_code = self._update_host(hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0])) - if updated_response_code not in [ ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, ErrorCode.OBJECT_EXISTS]: - logger.warning("Could not update host %s. Error code was: %s " % (hostTuple[0], updated_response_code)) + updated_response_code = self._update_host( + hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) + ) + if updated_response_code not in [ + ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ErrorCode.OBJECT_EXISTS, + ]: + logger.warning( + "Could not update host %s. Error code was: %s " + % (hostTuple[0], updated_response_code) + ) for key, value in new_values.items(): - print("HELLO THERE KEY, VALUE PAIR") - print(key) - print(value) - createdCode = self._create_host(host=key, addrs=value) # creates in registry - # TODO-848: Double check if _create_host should handle duplicates + update domain obj? - # NOTE-848: if createdCode == ErrorCode.OBJECT_EXISTS: --> self.nameservers - - # NOTE-848: Host can be used by multiple domains - if createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY or createdCode == ErrorCode.OBJECT_EXISTS: + createdCode = self._create_host( + host=key, addrs=value + ) # creates in registry + if ( + createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY + or createdCode == ErrorCode.OBJECT_EXISTS + ): request = commands.UpdateDomain( name=self.name, add=[epp.HostObjSet([key])] ) @@ -415,9 +418,10 @@ class Domain(TimeStampedModel, DomainHelper): % (e.code, e) ) - print("") - successTotalNameservers = len(oldNameservers) - successDeletedCount+ successCreatedCount - + successTotalNameservers = ( + len(oldNameservers) - successDeletedCount + successCreatedCount + ) + print("len(oldNameservers) IS ") print(len(oldNameservers)) print("successDeletedCount IS ") @@ -425,9 +429,6 @@ class Domain(TimeStampedModel, DomainHelper): print("successCreatedCount IS ") print(successCreatedCount) - - - print("SUCCESSTOTALNAMESERVERS IS ") print(successTotalNameservers) if successTotalNameservers < 2: @@ -438,17 +439,19 @@ class Domain(TimeStampedModel, DomainHelper): except Exception as err: logger.info( "nameserver setter checked for dns_needed state " - "and it did not succeed. Error: %s" % err - ) + "and it did not succeed. Warning: %s" % err + ) elif successTotalNameservers >= 2 and successTotalNameservers <= 13: try: - print("READY/SAVE: We are in happy path where btwen 2 and 13 inclusive ns") + print( + "READY/SAVE: We are in happy path where btwen 2 and 13 inclusive ns" + ) self.ready() self.save() except Exception as err: logger.info( "nameserver setter checked for create state " - "and it did not succeed. Error: %s" % err + "and it did not succeed. Warning: %s" % err ) # TODO-848: Handle removed nameservers here, will need to change the state then go back to DNS_NEEDED @@ -838,7 +841,7 @@ class Domain(TimeStampedModel, DomainHelper): req = commands.InfoDomain(name=self.name) domainInfo = registry.send(req, cleaned=True).res_data[0] exitEarly = True - return domainInfo + return domainInfo except RegistryError as e: count += 1 @@ -931,17 +934,17 @@ class Domain(TimeStampedModel, DomainHelper): # when EPP calling is made more efficient # this should be added back in - # The goal is to double check that + # The goal is to double check that # the nameservers we set are in fact # on the registry # """ # self._invalidate_cache() # nameserverList = self.nameservers # return len(nameserverList) < 2 - + # def dns_not_needed(self): # return not self.is_dns_needed() - + @transition( field="state", source=[State.DNS_NEEDED], @@ -964,10 +967,10 @@ class Domain(TimeStampedModel, DomainHelper): ) def dns_needed(self): """Transition to the DNS_NEEDED state - domain should NOT have nameservers but + domain should NOT have nameservers but SHOULD have all contacts - Going to check nameservers and will - result in an EPP call + Going to check nameservers and will + result in an EPP call """ logger.info("Changing to DNS_NEEDED state") logger.info("able to transition to DNS_NEEDED state") @@ -1087,22 +1090,33 @@ class Domain(TimeStampedModel, DomainHelper): edited_ip_list = [] if ip_list is None: return [] - + for ip_addr in ip_list: - if self.is_ipv6(): + if self.is_ipv6(ip_addr): edited_ip_list.append(epp.Ip(addr=ip_addr, ip="v6")) - else: # default ip addr is v4 + else: # default ip addr is v4 edited_ip_list.append(epp.Ip(addr=ip_addr)) return edited_ip_list def _update_host(self, nameserver: str, ip_list: list[str], old_ip_list: list[str]): try: - - if ip_list is None or len(ip_list) == 0 and isinstance(old_ip_list,list) and len(old_ip_list)!=0 : + if ( + ip_list is None + or len(ip_list) == 0 + and isinstance(old_ip_list, list) + and len(old_ip_list) != 0 + ): return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY - - request = commands.UpdateHost(name=nameserver, add=self._convert_ips(ip_list), rem=self._convert_ips(old_ip_list)) + + added_ip_list = set(ip_list) - set(old_ip_list) + removed_ip_list = set(old_ip_list) - set(ip_list) + + request = commands.UpdateHost( + name=nameserver, + add=self._convert_ips(list(added_ip_list)), + rem=self._convert_ips(list(removed_ip_list)), + ) response = registry.send(request, cleaned=True) logger.info("_update_host()-> sending req as %s" % request) return response.code @@ -1113,23 +1127,30 @@ class Domain(TimeStampedModel, DomainHelper): def _delete_host(self, nameserver: str): try: updateReq = commands.UpdateDomain( - name=self.name, rem=[epp.HostObjSet([nameserver])] - ) - response=registry.send(updateReq, cleaned=True) - + name=self.name, rem=[epp.HostObjSet([nameserver])] + ) + response = registry.send(updateReq, cleaned=True) + logger.info("_delete_host()-> sending update domain req as %s" % updateReq) - + deleteHostReq = commands.DeleteHost(name=nameserver) response = registry.send(deleteHostReq, cleaned=True) - logger.info("_delete_host()-> sending delete host req as %s" % deleteHostReq) + logger.info( + "_delete_host()-> sending delete host req as %s" % deleteHostReq + ) return response.code except RegistryError as e: - if e.code==ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: - logger.info("Did not remove host %s because it is in use on another domain." % nameserver) + if e.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: + logger.info( + "Did not remove host %s because it is in use on another domain." + % nameserver + ) else: - logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + logger.error( + "Error _delete_host, code was %s error was %s" % (e.code, e) + ) return e.code - + def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" try: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 9da6c5e00..67ba62b0a 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -562,7 +562,7 @@ class MockEppLib(TestCase): self.contacts = contacts self.hosts = hosts self.statuses = statuses - self.avail = avail #use for CheckDomain + self.avail = avail # use for CheckDomain self.addrs = addrs mockDataInfoDomain = fakedEppObject( @@ -581,36 +581,52 @@ class MockEppLib(TestCase): contacts=[], hosts=["fake.host.com"], ) - infoDomainThreeHosts =fakedEppObject( + infoDomainThreeHosts = fakedEppObject( "my-nameserver.gov", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), contacts=[], - hosts=["ns1.my-nameserver-1.com","ns1.my-nameserver-2.com","ns1.cats-are-superior3.com"], + hosts=[ + "ns1.my-nameserver-1.com", + "ns1.my-nameserver-2.com", + "ns1.cats-are-superior3.com", + ], ) - infoDomainNoHost =fakedEppObject( + infoDomainNoHost = fakedEppObject( "my-nameserver.gov", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), contacts=[], hosts=[], ) - infoDomainTwoHosts =fakedEppObject( + infoDomainTwoHosts = fakedEppObject( "my-nameserver.gov", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), contacts=[], - hosts=["ns1.my-nameserver-1.com","ns1.my-nameserver-2.com"], + hosts=["ns1.my-nameserver-1.com", "ns1.my-nameserver-2.com"], ) mockDataInfoContact = fakedEppObject( "anotherPw", cr_date=datetime.datetime(2023, 7, 25, 19, 45, 35) ) mockDataInfoHosts = fakedEppObject( - "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), addrs=["1.2.3", "2.3.4"] + "lastPw", + cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), + addrs=["1.2.3", "2.3.4"], ) - mockDataHostChange =fakedEppObject( + mockDataHostChange = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35) ) - - extendedValues=False + infoDomainHasIP = fakedEppObject( + "nameserverwithip.gov", + cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + contacts=[], + hosts=[ + "ns1.nameserverwithip.gov", + "ns2.nameserverwithip.gov", + "ns3.nameserverwithip.gov", + ], + ) + + extendedValues = False def mockSend(self, _request, cleaned): """Mocks the registry.send function used inside of domain.py @@ -623,10 +639,14 @@ class MockEppLib(TestCase): elif getattr(_request, "name", None) == "my-nameserver.gov": if self.extendedValues: return MagicMock(res_data=[self.infoDomainThreeHosts]) - elif self.mockedSendFunction.call_count==5: ## remove this breaks anything? + elif ( + self.mockedSendFunction.call_count == 5 + ): ## remove this breaks anything? return MagicMock(res_data=[self.infoDomainTwoHosts]) else: return MagicMock(res_data=[self.infoDomainNoHost]) + elif getattr(_request, "name", None) == "nameserverwithip.gov": + return MagicMock(res_data=[self.infoDomainHasIP]) return MagicMock(res_data=[self.mockDataInfoDomain]) elif isinstance(_request, commands.InfoContact): return MagicMock(res_data=[self.mockDataInfoContact]) @@ -638,12 +658,21 @@ class MockEppLib(TestCase): # use this for when a contact is being updated # sets the second send() to fail raise RegistryError(code=ErrorCode.OBJECT_EXISTS) - elif (isinstance(_request, commands.CreateHost)): - return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) - elif (isinstance(_request, commands.UpdateHost)): - return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) - elif (isinstance(_request, commands.DeleteHost)): - return MagicMock(res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY) + elif isinstance(_request, commands.CreateHost): + return MagicMock( + res_data=[self.mockDataHostChange], + code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ) + elif isinstance(_request, commands.UpdateHost): + return MagicMock( + res_data=[self.mockDataHostChange], + code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ) + elif isinstance(_request, commands.DeleteHost): + return MagicMock( + res_data=[self.mockDataHostChange], + code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ) return MagicMock(res_data=[self.mockDataInfoHosts]) def setUp(self): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index d9538baca..5f556d8eb 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -537,27 +537,39 @@ class TestRegistrantNameservers(MockEppLib): self.nameserver2 = "ns1.my-nameserver-2.com" self.nameserver3 = "ns1.cats-are-superior3.com" - self.domain, _ = Domain.objects.get_or_create(name="my-nameserver.gov", state=Domain.State.DNS_NEEDED) + self.domain, _ = Domain.objects.get_or_create( + name="my-nameserver.gov", state=Domain.State.DNS_NEEDED + ) def test_get_nameserver_changes(self): - self.domain._cache["hosts"]=[{ - "name": "ns1.example.com", - "addrs": None - }, - {"name": "ns2.example.com", - "addrs": ["1.2.3"] - }, - {"name": "ns3.example.com", - "addrs": ["1.2.3"] - } + self.domain._cache["hosts"] = [ + {"name": "ns1.example.com", "addrs": None}, + {"name": "ns2.example.com", "addrs": ["1.2.3"]}, + {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]}, ] - newChanges=[("ns1.example.com",),("ns3.example.com",["1.2.4"]),("ns4.example.com",)] - deleted_values,updated_values,new_values, oldNameservers=self.domain.getNameserverChanges(newChanges) + newChanges = [ + ("ns1.example.com",), + ("ns3.my-nameserver.gov", ["1.2.4"]), + ("ns4.example.com",), + ] + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.domain.getNameserverChanges(newChanges) print(oldNameservers) - self.assertEqual(deleted_values, [('ns2.example.com', ['1.2.3'])]) - self.assertEqual(updated_values, [('ns3.example.com', ['1.2.4'])]) - self.assertEqual(new_values, {'ns4.example.com':None}) - self.assertEqual(oldNameservers, {'ns1.example.com': None, 'ns2.example.com': ['1.2.3'], 'ns3.example.com': ['1.2.3']}) + self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])]) + self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])]) + self.assertEqual(new_values, {"ns4.example.com": None}) + self.assertEqual( + oldNameservers, + { + "ns1.example.com": None, + "ns2.example.com": ["1.2.3"], + "ns3.my-nameserver.gov": ["1.2.3"], + }, + ) def test_user_adds_one_nameserver(self): """ @@ -571,11 +583,13 @@ class TestRegistrantNameservers(MockEppLib): # set 1 nameserver nameserver = "ns1.my-nameserver.com" - self.domain.nameservers = [(nameserver,)] + self.domain.nameservers = [(nameserver,)] # when you create a host, you also have to update at same time created_host = commands.CreateHost(nameserver) - update_domain_with_created = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host.name])]) + update_domain_with_created = commands.UpdateDomain( + name=self.domain.name, add=[common.HostObjSet([created_host.name])] + ) # checking if commands were sent (commands have to be sent in order) expectedCalls = [ @@ -600,16 +614,20 @@ class TestRegistrantNameservers(MockEppLib): # set 2 nameservers print("DOCKER DIDNT SUCK THIS TIME") - self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] + self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] # when you create a host, you also have to update at same time created_host1 = commands.CreateHost(self.nameserver1) - update_domain_with_created1 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host1.name])]) + update_domain_with_created1 = commands.UpdateDomain( + name=self.domain.name, add=[common.HostObjSet([created_host1.name])] + ) created_host2 = commands.CreateHost(self.nameserver2) - update_domain_with_created2 = commands.UpdateDomain(name=self.domain.name, add=[common.HostObjSet([created_host2.name])]) + update_domain_with_created2 = commands.UpdateDomain( + name=self.domain.name, add=[common.HostObjSet([created_host2.name])] + ) - infoDomain = commands.InfoDomain(name='my-nameserver.gov', auth_info=None) + infoDomain = commands.InfoDomain(name="my-nameserver.gov", auth_info=None) # checking if commands were sent (commands have to be sent in order) expectedCalls = [ call(infoDomain, cleaned=True), @@ -619,9 +637,6 @@ class TestRegistrantNameservers(MockEppLib): call(update_domain_with_created2, cleaned=True), ] - print("self.mockedSendFunction.call_args_list is ") - print(self.mockedSendFunction.call_args_list) - self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) self.assertEqual(5, self.mockedSendFunction.call_count) # check that status is READY @@ -650,11 +665,24 @@ class TestRegistrantNameservers(MockEppLib): nameserver12 = "ns1.cats-are-superior12.com" nameserver13 = "ns1.cats-are-superior13.com" nameserver14 = "ns1.cats-are-superior14.com" - + def _get_14_nameservers(): - self.domain.nameservers = [(nameserver1,), (nameserver2,), (nameserver3,), (nameserver4,), - (nameserver5,), (nameserver6,), (nameserver7,), (nameserver8,), (nameserver9), (nameserver10,), - (nameserver11,), (nameserver12,), (nameserver13,), (nameserver14,)] + self.domain.nameservers = [ + (nameserver1,), + (nameserver2,), + (nameserver3,), + (nameserver4,), + (nameserver5,), + (nameserver6,), + (nameserver7,), + (nameserver8,), + (nameserver9), + (nameserver10,), + (nameserver11,), + (nameserver12,), + (nameserver13,), + (nameserver14,), + ] self.assertRaises(ValueError, _get_14_nameservers) self.assertEqual(self.mockedSendFunction.call_count, 0) @@ -670,25 +698,37 @@ class TestRegistrantNameservers(MockEppLib): """ # Mock is set to return 3 nameservers on infodomain - self.extendedValues=True + self.extendedValues = True self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] - expectedCalls=[ + expectedCalls = [ # calls info domain, and info on all hosts # to get past values # then removes the single host and updates domain - call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), - call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.cats-are-superior3.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), - call(commands.DeleteHost(name='ns1.cats-are-superior3.com'), cleaned=True)] + call( + commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), + call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[], + rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + call(commands.DeleteHost(name="ns1.cats-are-superior3.com"), cleaned=True), + ] - print("self.mockedSendFunction.call_args_list is ") - print(self.mockedSendFunction.call_args_list) self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) self.assertTrue(self.domain.is_active()) - def test_user_removes_too_many_nameservers(self): """ Scenario: Registrant removes some nameservers, bringing the total to less than 2 @@ -697,27 +737,51 @@ class TestRegistrantNameservers(MockEppLib): Then `commands.UpdateDomain` and `commands.DeleteHost` is sent to the registry And `domain.is_active` returns False - + """ - self.extendedValues=True + self.extendedValues = True print("domain state") print(self.domain.state) self.domain.ready() print("Domain state is now") print(self.domain.state) - self.domain.nameservers = [(self.nameserver1,)] - print("self.mockedSendFunction.call_args_list is ") - print(self.mockedSendFunction.call_args_list) - expectedCalls=[call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), - call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.my-nameserver-2.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), - call(commands.DeleteHost(name='ns1.my-nameserver-2.com'), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.cats-are-superior3.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), - call(commands.DeleteHost(name='ns1.cats-are-superior3.com'), cleaned=True)] - - + self.domain.nameservers = [(self.nameserver1,)] + expectedCalls = [ + call( + commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), + call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[], + rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[], + rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + call(commands.DeleteHost(name="ns1.cats-are-superior3.com"), cleaned=True), + ] + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) self.assertFalse(self.domain.is_active()) @@ -731,27 +795,72 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - self.extendedValues=True + self.extendedValues = True self.domain.ready() - self.domain.nameservers=[(self.nameserver1,), ("ns1.cats-are-superior1.com",), ("ns1.cats-are-superior2.com",)] - print("self.mockedSendFunction.call_args_list is ") - print(self.mockedSendFunction.call_args_list) - expectedCalls=[call(commands.InfoDomain(name='my-nameserver.gov', auth_info=None), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-1.com'), cleaned=True), - call(commands.InfoHost(name='ns1.my-nameserver-2.com'), cleaned=True), - call(commands.InfoHost(name='ns1.cats-are-superior3.com'), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[], rem=[common.HostObjSet(hosts=['ns1.my-nameserver-2.com'])], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), - call(commands.DeleteHost(name='ns1.my-nameserver-2.com'), cleaned=True), - call(commands.CreateHost(name='ns1.cats-are-superior1.com', addrs=[]), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[common.HostObjSet(hosts=['ns1.cats-are-superior1.com'])], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True), - call(commands.CreateHost(name='ns1.cats-are-superior2.com', addrs=[]), cleaned=True), - call(commands.UpdateDomain(name='my-nameserver.gov', add=[common.HostObjSet(hosts=['ns1.cats-are-superior2.com'])], rem=[], nsset=None, keyset=None, registrant=None, auth_info=None), cleaned=True) - ] - + self.domain.nameservers = [ + (self.nameserver1,), + ("ns1.cats-are-superior1.com",), + ("ns1.cats-are-superior2.com",), + ] + + expectedCalls = [ + call( + commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), + call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[], + rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call( + commands.CreateHost(name="ns1.cats-are-superior1.com", addrs=[]), + cleaned=True, + ), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[common.HostObjSet(hosts=["ns1.cats-are-superior1.com"])], + rem=[], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + call( + commands.CreateHost(name="ns1.cats-are-superior2.com", addrs=[]), + cleaned=True, + ), + call( + commands.UpdateDomain( + name="my-nameserver.gov", + add=[common.HostObjSet(hosts=["ns1.cats-are-superior2.com"])], + rem=[], + nsset=None, + keyset=None, + registrant=None, + auth_info=None, + ), + cleaned=True, + ), + ] + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) self.assertTrue(self.domain.is_active()) - def test_user_cannot_add_subordinate_without_ip(self): """ Scenario: Registrant adds a nameserver which is a subdomain of their .gov @@ -760,11 +869,12 @@ class TestRegistrantNameservers(MockEppLib): with a subdomain of the domain and no IP addresses Then Domain raises a user-friendly error """ - #add a nameserver with a .gov and no ip - ##assertRaises error - raise - @skip("not implemented yet") + dotgovnameserver = "my-nameserver.gov" + + with self.assertRaises(ValueError): + self.domain.nameservers = [(dotgovnameserver,)] + def test_user_updates_ips(self): """ Scenario: Registrant changes IP addresses for a nameserver @@ -774,9 +884,49 @@ class TestRegistrantNameservers(MockEppLib): with a different IP address(es) Then `commands.UpdateHost` is sent to the registry """ - raise + domain, _ = Domain.objects.get_or_create( + name="nameserverwithip.gov", state=Domain.State.READY + ) + domain.nameservers = [ + ("ns1.nameserverwithip.gov", ["2.3.4", "1.2.3"]), + ("ns2.nameserverwithip.gov", ["1.2.3", "2.3.4", "3.4.5"]), + ("ns3.nameserverwithip.gov", ["2.3.4"]), + ] + + # print("self.mockedSendFunction.call_args_list is ") + # print(self.mockedSendFunction.call_args_list) + + expectedCalls = [ + call( + commands.InfoDomain(name="nameserverwithip.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoHost(name="ns1.nameserverwithip.gov"), cleaned=True), + call(commands.InfoHost(name="ns2.nameserverwithip.gov"), cleaned=True), + call(commands.InfoHost(name="ns3.nameserverwithip.gov"), cleaned=True), + call( + commands.UpdateHost( + name="ns2.nameserverwithip.gov", + add=[common.Ip(addr="3.4.5", ip="v6")], + rem=[], + chg=None, + ), + cleaned=True, + ), + call( + commands.UpdateHost( + name="ns3.nameserverwithip.gov", + add=[], + rem=[common.Ip(addr="1.2.3", ip="v6")], + chg=None, + ), + cleaned=True, + ), + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertTrue(domain.is_active()) - @skip("not implemented yet") def test_user_cannot_add_non_subordinate_with_ip(self): """ Scenario: Registrant adds a nameserver which is NOT a subdomain of their .gov @@ -785,7 +935,10 @@ class TestRegistrantNameservers(MockEppLib): which is not a subdomain of the domain and has IP addresses Then Domain raises a user-friendly error """ - raise + dotgovnameserver = "mynameserverdotgov.gov" + + with self.assertRaises(ValueError): + self.domain.nameservers = [(dotgovnameserver, ["1.2.3"])] @skip("not implemented yet") def test_nameservers_are_idempotent(self): @@ -810,9 +963,10 @@ class TestRegistrantNameservers(MockEppLib): raise def tearDown(self): - self.extendedValues=False + self.extendedValues = False return super().tearDown() + class TestRegistrantDNSSEC(TestCase): """Rule: Registrants may modify their secure DNS data""" From ef294594216aebf5cd57266ffbae1d1f369f9cc8 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 28 Sep 2023 17:41:19 -0700 Subject: [PATCH 15/56] Nameserver fixes along with linter fixes --- src/registrar/models/domain.py | 115 +++++++++++----------- src/registrar/tests/common.py | 44 ++++++--- src/registrar/tests/test_models_domain.py | 54 +++++++--- 3 files changed, 126 insertions(+), 87 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 825503026..4a5306752 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -234,6 +234,10 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Domain is missing nameservers %s" % err) return [] + # TODO-848: Fix the output + # ('ns1.therealslimhsiehdy.com',) + # ('ns2.therealslimhsiehdy.com',) + # ('ns3.therealslimhsiehdy.com',) hostList = [] for host in hosts: hostList.append((host["name"], host["addrs"])) @@ -328,7 +332,7 @@ class Domain(TimeStampedModel, DomainHelper): deleted_values.append((prevHost, addrs)) # if the host exists in both, check if the addresses changed else: - # TODO - host is being updated when previous was None and new is an empty list + # TODO - host is being updated when previous was None+new is empty list # add check here if newHostDict[prevHost] is not None and set( newHostDict[prevHost] @@ -347,6 +351,49 @@ class Domain(TimeStampedModel, DomainHelper): return (deleted_values, updated_values, new_values, previousHostDict) + # TODO-848: Rename later - was getting complex err + def _loop_through(self, deleted_values, updated_values, new_values, oldNameservers): + successDeletedCount = 0 + successCreatedCount = 0 + for hostTuple in deleted_values: + deleted_response_code = self._delete_host(hostTuple[0]) + if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + successDeletedCount += 1 + + for hostTuple in updated_values: + updated_response_code = self._update_host( + hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) + ) + if updated_response_code not in [ + ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ErrorCode.OBJECT_EXISTS, + ]: + logger.warning( + "Could not update host %s. Error code was: %s " + % (hostTuple[0], updated_response_code) + ) + + for key, value in new_values.items(): + createdCode = self._create_host( + host=key, addrs=value + ) # creates in registry + if ( + createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY + or createdCode == ErrorCode.OBJECT_EXISTS + ): + request = commands.UpdateDomain( + name=self.name, add=[epp.HostObjSet([key])] + ) + try: + registry.send(request, cleaned=True) + successCreatedCount += 1 + except RegistryError as e: + logger.error( + "Error adding nameserver, code was %s error was %s" + % (e.code, e) + ) + return len(oldNameservers) - successDeletedCount + successCreatedCount + @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str]]): """host should be a tuple of type str, str,... where the elements are @@ -369,68 +416,15 @@ class Domain(TimeStampedModel, DomainHelper): new_values, oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - successDeletedCount = 0 - successCreatedCount = 0 - print("deleted_values") - print(deleted_values) - print("updated_values") - print(updated_values) - print("new_values") - print(new_values) - print("oldNameservers") - print(oldNameservers) - - for hostTuple in deleted_values: - deleted_response_code = self._delete_host(hostTuple[0]) - if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - successDeletedCount += 1 - - for hostTuple in updated_values: - updated_response_code = self._update_host( - hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) - ) - if updated_response_code not in [ - ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, - ErrorCode.OBJECT_EXISTS, - ]: - logger.warning( - "Could not update host %s. Error code was: %s " - % (hostTuple[0], updated_response_code) - ) - for key, value in new_values.items(): - createdCode = self._create_host( - host=key, addrs=value - ) # creates in registry - if ( - createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY - or createdCode == ErrorCode.OBJECT_EXISTS - ): - request = commands.UpdateDomain( - name=self.name, add=[epp.HostObjSet([key])] - ) - try: - registry.send(request, cleaned=True) - successCreatedCount += 1 - except RegistryError as e: - logger.error( - "Error adding nameserver, code was %s error was %s" - % (e.code, e) - ) - - successTotalNameservers = ( - len(oldNameservers) - successDeletedCount + successCreatedCount + # TODO-848: Fix name here + successTotalNameservers = self._loop_through( + deleted_values, updated_values, new_values, oldNameservers ) - print("len(oldNameservers) IS ") - print(len(oldNameservers)) - print("successDeletedCount IS ") - print(successDeletedCount) - print("successCreatedCount IS ") - print(successCreatedCount) + # print("SUCCESSTOTALNAMESERVERS IS ") + # print(successTotalNameservers) - print("SUCCESSTOTALNAMESERVERS IS ") - print(successTotalNameservers) if successTotalNameservers < 2: try: print("DNS_NEEDED: We have less than 2 nameservers") @@ -453,7 +447,8 @@ class Domain(TimeStampedModel, DomainHelper): "nameserver setter checked for create state " "and it did not succeed. Warning: %s" % err ) - # TODO-848: Handle removed nameservers here, will need to change the state then go back to DNS_NEEDED + # TODO-848: Handle removed nameservers here, + # will need to change the state then go back to DNS_NEEDED @Cache def statuses(self) -> list[str]: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 67ba62b0a..6f9c8aa56 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -581,6 +581,13 @@ class MockEppLib(TestCase): contacts=[], hosts=["fake.host.com"], ) + # infoDomainUpdateFail = fakedEppObject( + # "security", + # cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + # contacts=[], + # hosts=["ns1.failednameserver.gov"], + # addrs=["1.2.3"], + # ) infoDomainThreeHosts = fakedEppObject( "my-nameserver.gov", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), @@ -628,26 +635,31 @@ class MockEppLib(TestCase): extendedValues = False + # TODO-848: Rename later - was getting complex err + def _getattrshelper(self, _request): + if getattr(_request, "name", None) == "security.gov": + return MagicMock(res_data=[self.infoDomainNoContact]) + elif getattr(_request, "name", None) == "my-nameserver.gov": + if self.extendedValues: + return MagicMock(res_data=[self.infoDomainThreeHosts]) + elif self.mockedSendFunction.call_count == 5: + return MagicMock(res_data=[self.infoDomainTwoHosts]) + else: + return MagicMock(res_data=[self.infoDomainNoHost]) + elif getattr(_request, "name", None) == "nameserverwithip.gov": + return MagicMock(res_data=[self.infoDomainHasIP]) + # elif getattr(_request, "name", None) == "failednameserver.gov": + # return MagicMock(res_data=[self.infoDomainUpdateFail]) + # return MagicMock(res_data=[self.mockDataInfoDomain]) + def mockSend(self, _request, cleaned): """Mocks the registry.send function used inside of domain.py registry is imported from epplibwrapper returns objects that simulate what would be in a epp response but only relevant pieces for tests""" if isinstance(_request, commands.InfoDomain): - if getattr(_request, "name", None) == "security.gov": - return MagicMock(res_data=[self.infoDomainNoContact]) - elif getattr(_request, "name", None) == "my-nameserver.gov": - if self.extendedValues: - return MagicMock(res_data=[self.infoDomainThreeHosts]) - elif ( - self.mockedSendFunction.call_count == 5 - ): ## remove this breaks anything? - return MagicMock(res_data=[self.infoDomainTwoHosts]) - else: - return MagicMock(res_data=[self.infoDomainNoHost]) - elif getattr(_request, "name", None) == "nameserverwithip.gov": - return MagicMock(res_data=[self.infoDomainHasIP]) - return MagicMock(res_data=[self.mockDataInfoDomain]) + # TODO-848: Fix name here + return self._getattrshelper(_request) elif isinstance(_request, commands.InfoContact): return MagicMock(res_data=[self.mockDataInfoContact]) elif ( @@ -663,7 +675,9 @@ class MockEppLib(TestCase): res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, ) - elif isinstance(_request, commands.UpdateHost): + # elif isinstance(_request, commands.UpdateHost): + # if getattr(_request, "name", None) == "ns1.failednameserver.gov": + # raise RegistryError(code=ErrorCode.OBJECT_EXISTS) return MagicMock( res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 5f556d8eb..ef2f6aab1 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -20,6 +20,7 @@ from .common import MockEppLib from epplibwrapper import ( commands, common, + RegistryError, ) @@ -334,7 +335,8 @@ class TestRegistrantContacts(MockEppLib): created contact of type 'security' """ # make a security contact that is a PublicContact - self.domain.dns_needed_from_unknown() # make sure a security email already exists + # make sure a security email already exists + self.domain.dns_needed_from_unknown() expectedSecContact = PublicContact.get_default_security() expectedSecContact.domain = self.domain expectedSecContact.email = "newEmail@fake.com" @@ -558,7 +560,6 @@ class TestRegistrantNameservers(MockEppLib): new_values, oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - print(oldNameservers) self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])]) self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])]) self.assertEqual(new_values, {"ns4.example.com": None}) @@ -613,7 +614,6 @@ class TestRegistrantNameservers(MockEppLib): """ # set 2 nameservers - print("DOCKER DIDNT SUCK THIS TIME") self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] # when you create a host, you also have to update at same time @@ -740,11 +740,7 @@ class TestRegistrantNameservers(MockEppLib): """ self.extendedValues = True - print("domain state") - print(self.domain.state) self.domain.ready() - print("Domain state is now") - print(self.domain.state) self.domain.nameservers = [(self.nameserver1,)] expectedCalls = [ call( @@ -893,9 +889,6 @@ class TestRegistrantNameservers(MockEppLib): ("ns3.nameserverwithip.gov", ["2.3.4"]), ] - # print("self.mockedSendFunction.call_args_list is ") - # print(self.mockedSendFunction.call_args_list) - expectedCalls = [ call( commands.InfoDomain(name="nameserverwithip.gov", auth_info=None), @@ -940,7 +933,6 @@ class TestRegistrantNameservers(MockEppLib): with self.assertRaises(ValueError): self.domain.nameservers = [(dotgovnameserver, ["1.2.3"])] - @skip("not implemented yet") def test_nameservers_are_idempotent(self): """ Scenario: Registrant adds a set of nameservers twice, due to a UI glitch @@ -951,6 +943,31 @@ class TestRegistrantNameservers(MockEppLib): # implementation note: this requires seeing what happens when these are actually # sent like this, and then implementing appropriate mocks for any errors the # registry normally sends in this case + + self.extendedValues = True + + # Checking that it doesn't create or update even if out of order + self.domain.nameservers = [ + (self.nameserver3,), + (self.nameserver1,), + (self.nameserver2,), + ] + + expectedCalls = [ + call( + commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), + call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), + call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), + ] + + self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) + self.assertEqual(self.mockedSendFunction.call_count, 4) + + @skip("not implemented yet") + def test_caching_issue(self): raise @skip("not implemented yet") @@ -959,8 +976,21 @@ class TestRegistrantNameservers(MockEppLib): Scenario: An update to the nameservers is unsuccessful When an error is returned from epplibwrapper Then a user-friendly error message is returned for displaying on the web + + Note: TODO 433 -- we will perform correct error handling and complete + this ticket. We want to raise an error for update/create/delete, but + don't want to lose user info (and exit out too early) """ - raise + + domain, _ = Domain.objects.get_or_create( + name="failednameserver.gov", state=Domain.State.READY + ) + + with self.assertRaises(RegistryError): + domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] + + # print("self.mockedSendFunction.call_args_list is ") + # print(self.mockedSendFunction.call_args_list) def tearDown(self): self.extendedValues = False From f704fe7184ed49b5c07fe5b3e9c30d84158f0296 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 28 Sep 2023 20:27:26 -0700 Subject: [PATCH 16/56] Fix conflict --- src/registrar/tests/test_models_domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index ef2f6aab1..7aaafe582 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -21,6 +21,7 @@ from epplibwrapper import ( commands, common, RegistryError, + ErrorCode, ) From ba72f8df8261b9c42c3cfbfa3717155c0501b78f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:38:12 -0600 Subject: [PATCH 17/56] Fixed invitation bug --- src/registrar/fixtures.py | 4 ++-- src/registrar/views/domain.py | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index a4e75dd2e..434cd28e4 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -192,8 +192,8 @@ class UserFixture: user.is_superuser = False user.first_name = staff["first_name"] user.last_name = staff["last_name"] - if "email" in admin.keys(): - user.email = admin["email"] + if "email" in staff.keys(): + user.email = staff["email"] user.is_staff = True user.is_active = True diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index ca7cee4ac..86455a038 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -315,7 +315,6 @@ class DomainAddUserView(DomainPermissionView, FormMixin): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.pk}) - def post(self, request, *args, **kwargs): self.object = self.get_object() form = self.get_form() @@ -377,7 +376,7 @@ class DomainAddUserView(DomainPermissionView, FormMixin): def form_valid(self, form): """Add the specified user on this domain.""" - requested_email = form.cleaned_data["email"] + requested_email = form.cleaned_data.get("email", "") # look up a user with that email try: requested_user = User.objects.get(email=requested_email) From 6fea4caa9a18969e228604f52cb574d54ebee37b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:43:51 -0600 Subject: [PATCH 18/56] Update domain.py --- src/registrar/views/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 86455a038..d88e0d443 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -315,6 +315,7 @@ class DomainAddUserView(DomainPermissionView, FormMixin): def get_success_url(self): return reverse("domain-users", kwargs={"pk": self.object.pk}) + def post(self, request, *args, **kwargs): self.object = self.get_object() form = self.get_form() From 71826a7a7ab91855f6091c448ad46f05075f1d4c Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 29 Sep 2023 16:15:05 -0700 Subject: [PATCH 19/56] Address feedback from Neil --- src/registrar/models/domain.py | 21 +++---- src/registrar/tests/common.py | 2 + src/registrar/tests/test_models_domain.py | 68 ++++++++++++++++++++--- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4a5306752..ff8952643 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -294,11 +294,12 @@ class Domain(TimeStampedModel, DomainHelper): return newDict def isSubdomain(self, nameserver): - return nameserver.find(self.name) != -1 + return self.name in nameserver def checkHostIPCombo(self, nameserver: str, ip: list): if self.isSubdomain(nameserver) and (ip is None or ip == []): - raise ValueError("Nameserver %s needs to have an ip address" % nameserver) + raise ValueError("Nameserver %s needs to have an " + "ip address because it is a subdomain" % nameserver) elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): raise ValueError( "Nameserver %s cannot be linked " @@ -306,15 +307,15 @@ class Domain(TimeStampedModel, DomainHelper): ) return None - def getNameserverChanges(self, hosts: list[tuple[str]]): + def getNameserverChanges(self, hosts: list[tuple[str]]) -> tuple[list, list, dict, list]: """ calls self.nameserver, it should pull from cache but may result in an epp call returns tuple of four values as follows: - deleted_values: - updated_values: + deleted_values: list + updated_values: list new_values: dict - oldNameservers:""" + oldNameservers: list""" oldNameservers = self.nameservers previousHostDict = self._convert_list_to_dict(oldNameservers) @@ -417,7 +418,7 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - # TODO-848: Fix name here + # TODO-848: Fix name successTotalNameservers = self._loop_through( deleted_values, updated_values, new_values, oldNameservers ) @@ -1103,9 +1104,9 @@ class Domain(TimeStampedModel, DomainHelper): and len(old_ip_list) != 0 ): return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY - - added_ip_list = set(ip_list) - set(old_ip_list) - removed_ip_list = set(old_ip_list) - set(ip_list) + + added_ip_list = set(ip_list).difference(old_ip_list) + removed_ip_list = set(old_ip_list).difference(ip_list) request = commands.UpdateHost( name=nameserver, diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 6f9c8aa56..d87757f26 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -631,8 +631,10 @@ class MockEppLib(TestCase): "ns2.nameserverwithip.gov", "ns3.nameserverwithip.gov", ], + addrs=["1.2.3", "2.3.4"], ) + # TODO-848: Fix naming extendedValues = False # TODO-848: Rename later - was getting complex err diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 7aaafe582..85968db01 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -544,15 +544,64 @@ class TestRegistrantNameservers(MockEppLib): name="my-nameserver.gov", state=Domain.State.DNS_NEEDED ) - def test_get_nameserver_changes(self): + def test_get_nameserver_changes_success_deleted_vals(self): + # Testing only deleting and no other changes self.domain._cache["hosts"] = [ {"name": "ns1.example.com", "addrs": None}, {"name": "ns2.example.com", "addrs": ["1.2.3"]}, - {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]}, ] newChanges = [ ("ns1.example.com",), + ] + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.domain.getNameserverChanges(newChanges) + + self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])]) + self.assertEqual(updated_values, []) + self.assertEqual(new_values, {}) + self.assertEqual( + oldNameservers, + { + 'ns1.example.com': None, 'ns2.example.com': ['1.2.3'] + }, + ) + + def test_get_nameserver_changes_success_updated_vals(self): + # Testing only updating no other changes + self.domain._cache["hosts"] = [ + {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]}, + ] + newChanges = [ ("ns3.my-nameserver.gov", ["1.2.4"]), + ] + ( + deleted_values, + updated_values, + new_values, + oldNameservers, + ) = self.domain.getNameserverChanges(newChanges) + + self.assertEqual(deleted_values, []) + self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])]) + self.assertEqual(new_values, {}) + self.assertEqual( + oldNameservers, + { + "ns3.my-nameserver.gov": ["1.2.3"] + }, + ) + + def test_get_nameserver_changes_success_new_vals(self): + # Testing only creating no other changes + self.domain._cache["hosts"] = [ + {"name": "ns1.example.com", "addrs": None}, + ] + newChanges = [ + ("ns1.example.com",), ("ns4.example.com",), ] ( @@ -561,15 +610,19 @@ class TestRegistrantNameservers(MockEppLib): new_values, oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])]) - self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])]) + + + print("deleted vals is ", deleted_values) + print("updated vals is ", updated_values) + print("new vals is ", new_values) + print("old nameserver is ", oldNameservers) + self.assertEqual(deleted_values, []) + self.assertEqual(updated_values, []) self.assertEqual(new_values, {"ns4.example.com": None}) self.assertEqual( oldNameservers, { "ns1.example.com": None, - "ns2.example.com": ["1.2.3"], - "ns3.my-nameserver.gov": ["1.2.3"], }, ) @@ -601,7 +654,8 @@ class TestRegistrantNameservers(MockEppLib): self.mockedSendFunction.assert_has_calls(expectedCalls) - # check that status is still NOT READY + # check that status is still NOT READY + # as you have less than 2 nameservers self.assertFalse(self.domain.is_active()) def test_user_adds_two_nameservers(self): From 77878b9bb99b082445ec9a7d00b405c9d8ab7866 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 29 Sep 2023 16:16:06 -0700 Subject: [PATCH 20/56] Addressing feedback from Neil --- src/registrar/tests/test_models_domain.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 85968db01..e6494c6a8 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -611,11 +611,6 @@ class TestRegistrantNameservers(MockEppLib): oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - - print("deleted vals is ", deleted_values) - print("updated vals is ", updated_values) - print("new vals is ", new_values) - print("old nameserver is ", oldNameservers) self.assertEqual(deleted_values, []) self.assertEqual(updated_values, []) self.assertEqual(new_values, {"ns4.example.com": None}) From 13039b63d9427da61effda5d7545b9b4c3c5b5b0 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Fri, 29 Sep 2023 17:22:48 -0700 Subject: [PATCH 21/56] Add in additional checks for ip validity --- src/registrar/models/domain.py | 30 ++++++++++++++++++----- src/registrar/tests/common.py | 6 ++--- src/registrar/tests/test_models_domain.py | 29 ++++++++++++---------- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index ff8952643..040f68b4e 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1,4 +1,5 @@ import logging +import ipaddress from datetime import date from string import digits @@ -298,8 +299,10 @@ class Domain(TimeStampedModel, DomainHelper): def checkHostIPCombo(self, nameserver: str, ip: list): if self.isSubdomain(nameserver) and (ip is None or ip == []): - raise ValueError("Nameserver %s needs to have an " - "ip address because it is a subdomain" % nameserver) + raise ValueError( + "Nameserver %s needs to have an " + "ip address because it is a subdomain" % nameserver + ) elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): raise ValueError( "Nameserver %s cannot be linked " @@ -307,7 +310,22 @@ class Domain(TimeStampedModel, DomainHelper): ) return None - def getNameserverChanges(self, hosts: list[tuple[str]]) -> tuple[list, list, dict, list]: + # TODO-848: We are checking for valid ip address format + # Need to use before checkHostIPCombo, or discuss where best fit + # And confirm if AddressValueError is best choice of error to raise + # def _valid_ip_addr(self): + # if ipaddress.IPv6Address(ip) or ipaddress.IPv4Address(ip): + # return True + # else: + # # We will need to import this error + # raise AddressValueError( + # "IP Address is in an invalid format." + # ) + # return None + + def getNameserverChanges( + self, hosts: list[tuple[str]] + ) -> tuple[list, list, dict, list]: """ calls self.nameserver, it should pull from cache but may result in an epp call @@ -1078,9 +1096,9 @@ class Domain(TimeStampedModel, DomainHelper): raise e - # TODO: Need to implement this def is_ipv6(self, ip: str): - return True + ip_addr = ipaddress.ip_address(ip) + return ip_addr.version == 6 def _convert_ips(self, ip_list: list[str]): edited_ip_list = [] @@ -1104,7 +1122,7 @@ class Domain(TimeStampedModel, DomainHelper): and len(old_ip_list) != 0 ): return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY - + added_ip_list = set(ip_list).difference(old_ip_list) removed_ip_list = set(old_ip_list).difference(ip_list) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index d87757f26..bd11edfdb 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -616,7 +616,7 @@ class MockEppLib(TestCase): mockDataInfoHosts = fakedEppObject( "lastPw", cr_date=datetime.datetime(2023, 8, 25, 19, 45, 35), - addrs=["1.2.3", "2.3.4"], + addrs=["1.2.3.4", "2.3.4.5"], ) mockDataHostChange = fakedEppObject( @@ -631,10 +631,10 @@ class MockEppLib(TestCase): "ns2.nameserverwithip.gov", "ns3.nameserverwithip.gov", ], - addrs=["1.2.3", "2.3.4"], + addrs=["1.2.3.4", "2.3.4.5"], ) - # TODO-848: Fix naming + # TODO-848: Fix naming extendedValues = False # TODO-848: Rename later - was getting complex err diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index e6494c6a8..8be0c3d1d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -565,9 +565,7 @@ class TestRegistrantNameservers(MockEppLib): self.assertEqual(new_values, {}) self.assertEqual( oldNameservers, - { - 'ns1.example.com': None, 'ns2.example.com': ['1.2.3'] - }, + {"ns1.example.com": None, "ns2.example.com": ["1.2.3"]}, ) def test_get_nameserver_changes_success_updated_vals(self): @@ -590,9 +588,7 @@ class TestRegistrantNameservers(MockEppLib): self.assertEqual(new_values, {}) self.assertEqual( oldNameservers, - { - "ns3.my-nameserver.gov": ["1.2.3"] - }, + {"ns3.my-nameserver.gov": ["1.2.3"]}, ) def test_get_nameserver_changes_success_new_vals(self): @@ -635,7 +631,7 @@ class TestRegistrantNameservers(MockEppLib): nameserver = "ns1.my-nameserver.com" self.domain.nameservers = [(nameserver,)] - # when you create a host, you also have to update at same time + # when we create a host, we should've updated at the same time created_host = commands.CreateHost(nameserver) update_domain_with_created = commands.UpdateDomain( name=self.domain.name, add=[common.HostObjSet([created_host.name])] @@ -649,7 +645,7 @@ class TestRegistrantNameservers(MockEppLib): self.mockedSendFunction.assert_has_calls(expectedCalls) - # check that status is still NOT READY + # check that status is still NOT READY # as you have less than 2 nameservers self.assertFalse(self.domain.is_active()) @@ -934,9 +930,12 @@ class TestRegistrantNameservers(MockEppLib): name="nameserverwithip.gov", state=Domain.State.READY ) domain.nameservers = [ - ("ns1.nameserverwithip.gov", ["2.3.4", "1.2.3"]), - ("ns2.nameserverwithip.gov", ["1.2.3", "2.3.4", "3.4.5"]), - ("ns3.nameserverwithip.gov", ["2.3.4"]), + ("ns1.nameserverwithip.gov", ["2.3.4.5", "1.2.3.4"]), + ( + "ns2.nameserverwithip.gov", + ["1.2.3.4", "2.3.4.5", "2001:0db8:85a3:0000:0000:8a2e:0370:7334"], + ), + ("ns3.nameserverwithip.gov", ["2.3.4.5"]), ] expectedCalls = [ @@ -950,7 +949,11 @@ class TestRegistrantNameservers(MockEppLib): call( commands.UpdateHost( name="ns2.nameserverwithip.gov", - add=[common.Ip(addr="3.4.5", ip="v6")], + add=[ + common.Ip( + addr="2001:0db8:85a3:0000:0000:8a2e:0370:7334", ip="v6" + ) + ], rem=[], chg=None, ), @@ -960,7 +963,7 @@ class TestRegistrantNameservers(MockEppLib): commands.UpdateHost( name="ns3.nameserverwithip.gov", add=[], - rem=[common.Ip(addr="1.2.3", ip="v6")], + rem=[common.Ip(addr="1.2.3.4", ip=None)], chg=None, ), cleaned=True, From ed3d8ccf726cc1c146fdc588c629e286776d75dd Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 2 Oct 2023 09:21:03 -0700 Subject: [PATCH 22/56] fixed failing tests --- src/registrar/tests/common.py | 8 +++----- src/registrar/tests/test_models_domain.py | 1 + 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 98e9287ed..1cb0f7893 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -638,8 +638,7 @@ class MockEppLib(TestCase): # TODO-848: Fix naming extendedValues = False - # TODO-848: Rename later - was getting complex err - def _getattrshelper(self, _request): + def _getattrInfoDomain(self, _request): if getattr(_request, "name", None) == "security.gov": return MagicMock(res_data=[self.infoDomainNoContact]) elif getattr(_request, "name", None) == "my-nameserver.gov": @@ -653,7 +652,7 @@ class MockEppLib(TestCase): return MagicMock(res_data=[self.infoDomainHasIP]) # elif getattr(_request, "name", None) == "failednameserver.gov": # return MagicMock(res_data=[self.infoDomainUpdateFail]) - # return MagicMock(res_data=[self.mockDataInfoDomain]) + return MagicMock(res_data=[self.mockDataInfoDomain]) def mockSend(self, _request, cleaned): """Mocks the registry.send function used inside of domain.py @@ -661,8 +660,7 @@ class MockEppLib(TestCase): returns objects that simulate what would be in a epp response but only relevant pieces for tests""" if isinstance(_request, commands.InfoDomain): - # TODO-848: Fix name here - return self._getattrshelper(_request) + return self._getattrInfoDomain(_request) elif isinstance(_request, commands.InfoContact): return MagicMock(res_data=[self.mockDataInfoContact]) elif ( diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 49264cd67..7a7056a26 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -90,6 +90,7 @@ class TestDomainCache(MockEppLib): } expectedHostsDict = { "name": self.mockDataInfoDomain.hosts[0], + "addrs":self.mockDataInfoHosts.addrs, "cr_date": self.mockDataInfoHosts.cr_date, } From d63d6622cc19d4e9e1df6acb940c5f1333a8bebb Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 2 Oct 2023 10:12:52 -0700 Subject: [PATCH 23/56] fixed linter errors --- src/registrar/models/domain.py | 66 +++++++++++------------ src/registrar/tests/test_models_domain.py | 4 +- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 7339b72d1..5a08d3c43 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -6,7 +6,7 @@ from string import digits from django_fsm import FSMField, transition # type: ignore from django.db import models - +from typing import Any from epplibwrapper import ( CLIENT as registry, commands, @@ -216,13 +216,13 @@ class Domain(TimeStampedModel, DomainHelper): raise NotImplementedError() @Cache - def nameservers(self) -> list[tuple[str]]: + def nameservers(self) -> list[tuple[str, list]]: """ Get or set a complete list of nameservers for this domain. Hosts are provided as a list of tuples, e.g. - [("ns1.example.com",), ("ns1.example.gov", "0.0.0.0")] + [("ns1.example.com",), ("ns1.example.gov", ["0.0.0.0"])] Subordinate hosts (something.your-domain.gov) MUST have IP addresses, while non-subordinate hosts MUST NOT. @@ -283,14 +283,14 @@ class Domain(TimeStampedModel, DomainHelper): logger.error("Error _create_host, code was %s error was %s" % (e.code, e)) return e.code - def _convert_list_to_dict(self, listToConvert: list[tuple[str]]): - newDict = {} + def _convert_list_to_dict(self, listToConvert: list[tuple[str, list]]): + newDict: dict[str, Any] = {} # TODO-848: If duplicated nameserver names, throw error for tup in listToConvert: if len(tup) == 1: newDict[tup[0]] = None - else: + elif len(tup) == 2: newDict[tup[0]] = tup[1] return newDict @@ -301,31 +301,32 @@ class Domain(TimeStampedModel, DomainHelper): if self.isSubdomain(nameserver) and (ip is None or ip == []): raise ValueError( "Nameserver %s needs to have an " - "ip address because it is a subdomain" % nameserver + "IP address because it is a subdomain" % nameserver ) elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): raise ValueError( "Nameserver %s cannot be linked " "because %s is not a subdomain" % (nameserver, ip) ) + elif ip is not None and ip != []: + for addr in ip: + if not self._valid_ip_addr(addr): + raise ValueError( + "Nameserver %s has an invalid IP address: %s" % (nameserver, ip) + ) return None - # TODO-848: We are checking for valid ip address format - # Need to use before checkHostIPCombo, or discuss where best fit - # And confirm if AddressValueError is best choice of error to raise - # def _valid_ip_addr(self): - # if ipaddress.IPv6Address(ip) or ipaddress.IPv4Address(ip): - # return True - # else: - # # We will need to import this error - # raise AddressValueError( - # "IP Address is in an invalid format." - # ) - # return None + def _valid_ip_addr(self, ip): + try: + ip = ipaddress.ip_address(ip) + return ip.version == 6 or ip.version == 4 + + except ValueError: + return False def getNameserverChanges( - self, hosts: list[tuple[str]] - ) -> tuple[list, list, dict, list]: + self, hosts: list[tuple[str, list]] + ) -> tuple[list, list, dict, dict]: """ calls self.nameserver, it should pull from cache but may result in an epp call @@ -333,15 +334,17 @@ class Domain(TimeStampedModel, DomainHelper): deleted_values: list updated_values: list new_values: dict - oldNameservers: list""" + prevHostDict: dict""" + oldNameservers = self.nameservers previousHostDict = self._convert_list_to_dict(oldNameservers) newHostDict = self._convert_list_to_dict(hosts) deleted_values = [] + # TODO-currently a list of tuples, why not dict? for consistency updated_values = [] - new_values = [] + new_values = {} for prevHost in previousHostDict: addrs = previousHostDict[prevHost] @@ -370,8 +373,9 @@ class Domain(TimeStampedModel, DomainHelper): return (deleted_values, updated_values, new_values, previousHostDict) - # TODO-848: Rename later - was getting complex err - def _loop_through(self, deleted_values, updated_values, new_values, oldNameservers): + def _update_delete_create_hosts( + self, deleted_values, updated_values, new_values, oldNameservers + ): successDeletedCount = 0 successCreatedCount = 0 for hostTuple in deleted_values: @@ -414,12 +418,10 @@ class Domain(TimeStampedModel, DomainHelper): return len(oldNameservers) - successDeletedCount + successCreatedCount @nameservers.setter # type: ignore - def nameservers(self, hosts: list[tuple[str]]): + def nameservers(self, hosts: list[tuple[str, list]]): """host should be a tuple of type str, str,... where the elements are Fully qualified host name, addresses associated with the host - example: [(ns1.okay.gov, 127.0.0.1, others ips)]""" - - # TODO-848: ip version checking may need to be added in a different ticket + example: [(ns1.okay.gov, [127.0.0.1, others ips])]""" if len(hosts) > 13: raise ValueError( @@ -436,14 +438,10 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - # TODO-848: Fix name - successTotalNameservers = self._loop_through( + successTotalNameservers = self._update_delete_create_hosts( deleted_values, updated_values, new_values, oldNameservers ) - # print("SUCCESSTOTALNAMESERVERS IS ") - # print(successTotalNameservers) - if successTotalNameservers < 2: try: print("DNS_NEEDED: We have less than 2 nameservers") diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 7a7056a26..4b8b3020d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -90,7 +90,7 @@ class TestDomainCache(MockEppLib): } expectedHostsDict = { "name": self.mockDataInfoDomain.hosts[0], - "addrs":self.mockDataInfoHosts.addrs, + "addrs": self.mockDataInfoHosts.addrs, "cr_date": self.mockDataInfoHosts.cr_date, } @@ -146,7 +146,7 @@ class TestDomainCreation(MockEppLib): application.status = DomainApplication.SUBMITTED # transition to approve state application.approve() - # should hav information present for this domain + # should have information present for this domain domain = Domain.objects.get(name="igorville.gov") self.assertTrue(domain) self.mockedSendFunction.assert_not_called() From f228ca88790cc755a01560a50a4b08b4629c6d5a Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 2 Oct 2023 10:18:31 -0700 Subject: [PATCH 24/56] added comment for ticket 687 --- src/registrar/templates/includes/summary_item.html | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index a2035b227..6fcad0650 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -43,6 +43,7 @@ {% else %} {% include "includes/contact.html" with contact=value %} {% endif %} + {% elif list %} {% if value|length == 1 %} {% if users %} From 1cd4893b546494916b31b2e6f720b282be09556d Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 2 Oct 2023 10:18:48 -0700 Subject: [PATCH 25/56] code cleanup --- src/registrar/models/domain.py | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 5a08d3c43..abec7a04a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -235,34 +235,13 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("Domain is missing nameservers %s" % err) return [] - # TODO-848: Fix the output - # ('ns1.therealslimhsiehdy.com',) - # ('ns2.therealslimhsiehdy.com',) - # ('ns3.therealslimhsiehdy.com',) + # TODO-687 fix this return value hostList = [] for host in hosts: hostList.append((host["name"], host["addrs"])) return hostList - # def _check_host(self, hostnames: list[str]): - # """check if host is available, True if available - # returns boolean""" - # # TODO-848: Double check this implementation is needed bc it's untested code - # # Check if the IP address is available/real - # checkCommand = commands.CheckHost(hostnames) - # try: - # response = registry.send(checkCommand, cleaned=True) - # return response.res_data[0].avail - # except RegistryError as err: - # logger.warning( - # "Couldn't check hosts %s. Errorcode was %s, error was %s", - # hostnames, - # err.code, - # err, - # ) - # return False - def _create_host(self, host, addrs): """Call _check_host first before using this function, This creates the host object in the registry From 095f7ca56f7de0ee9b16722ae032f4339d63d825 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Mon, 2 Oct 2023 14:02:07 -0700 Subject: [PATCH 26/56] Refactor _update_delete_create_hosts function --- src/registrar/models/domain.py | 44 +++++++++++++++++++---- src/registrar/tests/test_models_domain.py | 14 ++++---- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index abec7a04a..57944a35e 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -352,16 +352,17 @@ class Domain(TimeStampedModel, DomainHelper): return (deleted_values, updated_values, new_values, previousHostDict) - def _update_delete_create_hosts( - self, deleted_values, updated_values, new_values, oldNameservers - ): + def _deleted_host_values(self, deleted_values) -> int: successDeletedCount = 0 - successCreatedCount = 0 + for hostTuple in deleted_values: deleted_response_code = self._delete_host(hostTuple[0]) if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: successDeletedCount += 1 + return successDeletedCount + + def _update_host_values(self, updated_values, oldNameservers): for hostTuple in updated_values: updated_response_code = self._update_host( hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) @@ -375,6 +376,8 @@ class Domain(TimeStampedModel, DomainHelper): % (hostTuple[0], updated_response_code) ) + def _new_host_values(self, new_values) -> int: + successCreatedCount = 0 for key, value in new_values.items(): createdCode = self._create_host( host=key, addrs=value @@ -394,7 +397,24 @@ class Domain(TimeStampedModel, DomainHelper): "Error adding nameserver, code was %s error was %s" % (e.code, e) ) - return len(oldNameservers) - successDeletedCount + successCreatedCount + return successCreatedCount + + # def _update_delete_create_hosts( + # self, deleted_values, updated_values, new_values, oldNameservers + # ): + + # for hostTuple in updated_values: + # updated_response_code = self._update_host( + # hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) + # ) + # if updated_response_code not in [ + # ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + # ErrorCode.OBJECT_EXISTS, + # ]: + # logger.warning( + # "Could not update host %s. Error code was: %s " + # % (hostTuple[0], updated_response_code) + # ) @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str, list]]): @@ -417,10 +437,20 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - successTotalNameservers = self._update_delete_create_hosts( - deleted_values, updated_values, new_values, oldNameservers + successDeletedCount = self._deleted_host_values(deleted_values) # returns value + _ = self._update_host_values( + updated_values, oldNameservers + ) # returns nothing, just need to be run and errors + successCreatedCount = self._new_host_values(new_values) + + successTotalNameservers = ( + len(oldNameservers) - successDeletedCount + successCreatedCount ) + # successTotalNameservers = self._update_delete_create_hosts( + # deleted_values, updated_values, new_values, oldNameservers + # ) + if successTotalNameservers < 2: try: print("DNS_NEEDED: We have less than 2 nameservers") diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 4b8b3020d..c0fcc2fb6 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -663,7 +663,7 @@ class TestRegistrantNameservers(MockEppLib): # Testing only deleting and no other changes self.domain._cache["hosts"] = [ {"name": "ns1.example.com", "addrs": None}, - {"name": "ns2.example.com", "addrs": ["1.2.3"]}, + {"name": "ns2.example.com", "addrs": ["1.2.3.4"]}, ] newChanges = [ ("ns1.example.com",), @@ -675,21 +675,21 @@ class TestRegistrantNameservers(MockEppLib): oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3"])]) + self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3.4"])]) self.assertEqual(updated_values, []) self.assertEqual(new_values, {}) self.assertEqual( oldNameservers, - {"ns1.example.com": None, "ns2.example.com": ["1.2.3"]}, + {"ns1.example.com": None, "ns2.example.com": ["1.2.3.4"]}, ) def test_get_nameserver_changes_success_updated_vals(self): # Testing only updating no other changes self.domain._cache["hosts"] = [ - {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3"]}, + {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3.4"]}, ] newChanges = [ - ("ns3.my-nameserver.gov", ["1.2.4"]), + ("ns3.my-nameserver.gov", ["1.2.4.5"]), ] ( deleted_values, @@ -699,11 +699,11 @@ class TestRegistrantNameservers(MockEppLib): ) = self.domain.getNameserverChanges(newChanges) self.assertEqual(deleted_values, []) - self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4"])]) + self.assertEqual(updated_values, [("ns3.my-nameserver.gov", ["1.2.4.5"])]) self.assertEqual(new_values, {}) self.assertEqual( oldNameservers, - {"ns3.my-nameserver.gov": ["1.2.3"]}, + {"ns3.my-nameserver.gov": ["1.2.3.4"]}, ) def test_get_nameserver_changes_success_new_vals(self): From 24f4346e55c7f9162dc46c401996dc3224979538 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Mon, 2 Oct 2023 14:10:22 -0700 Subject: [PATCH 27/56] Remove commented out test, my bad --- src/registrar/models/domain.py | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 57944a35e..19854854d 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -399,23 +399,6 @@ class Domain(TimeStampedModel, DomainHelper): ) return successCreatedCount - # def _update_delete_create_hosts( - # self, deleted_values, updated_values, new_values, oldNameservers - # ): - - # for hostTuple in updated_values: - # updated_response_code = self._update_host( - # hostTuple[0], hostTuple[1], oldNameservers.get(hostTuple[0]) - # ) - # if updated_response_code not in [ - # ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, - # ErrorCode.OBJECT_EXISTS, - # ]: - # logger.warning( - # "Could not update host %s. Error code was: %s " - # % (hostTuple[0], updated_response_code) - # ) - @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str, list]]): """host should be a tuple of type str, str,... where the elements are From d532c389c081243b6ced312c0fc9321e0c746622 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Mon, 2 Oct 2023 22:07:26 -0700 Subject: [PATCH 28/56] Add tests for subdomain and ip logic --- src/registrar/tests/common.py | 11 ++++++++ src/registrar/tests/test_models_domain.py | 34 +++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 7a0d2ec59..5a3887f3b 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -634,6 +634,15 @@ class MockEppLib(TestCase): ], addrs=["1.2.3.4", "2.3.4.5"], ) + infoDomainCheckHostIPCombo = fakedEppObject( + "nameserversubdomain.gov", + cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), + contacts=[], + hosts=[ + "ns1.nameserversubdomain.gov", + "ns2.nameserversubdomain.gov", + ], + ) # TODO-848: Fix naming extendedValues = False @@ -650,6 +659,8 @@ class MockEppLib(TestCase): return MagicMock(res_data=[self.infoDomainNoHost]) elif getattr(_request, "name", None) == "nameserverwithip.gov": return MagicMock(res_data=[self.infoDomainHasIP]) + elif getattr(_request, "name", None) == "namerserversubdomain.gov": + return MagicMock(res_data=[self.infoDomainCheckHostIPCombo]) # elif getattr(_request, "name", None) == "failednameserver.gov": # return MagicMock(res_data=[self.infoDomainUpdateFail]) return MagicMock(res_data=[self.mockDataInfoDomain]) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 701f9e081..c0eac751d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -1134,6 +1134,40 @@ class TestRegistrantNameservers(MockEppLib): self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) self.assertEqual(self.mockedSendFunction.call_count, 4) + def test_is_subdomain_with_no_ip(self): + domain, _ = Domain.objects.get_or_create( + name="nameserversubdomain.gov", state=Domain.State.READY + ) + + with self.assertRaises(ValueError): + domain.nameservers = [ + ("ns1.nameserversubdomain.gov",), + ("ns2.nameserversubdomain.gov",), + ] + + @skip("not implemented yet") + def test_not_subdomain_but_has_ip(self): + # TO FIX - Logic + # domain, _ = Domain.objects.get_or_create( + # name="nameserversubdomain.gov", state=Domain.State.READY + # ) + + # with self.assertRaises(ValueError): + # domain.nameservers = [("ns1.cats-da-best.gov", ["1.2.3.4"]), + # ("ns2.cats-da-best.gov", ["2.3.4.5"]),] + raise + + def test_is_subdomain_but_ip_addr_not_valid(self): + domain, _ = Domain.objects.get_or_create( + name="nameserversubdomain.gov", state=Domain.State.READY + ) + + with self.assertRaises(ValueError): + domain.nameservers = [ + ("ns1.nameserversubdomain.gov", ["1.2.3"]), + ("ns2.nameserversubdomain.gov", ["2.3.4"]), + ] + @skip("not implemented yet") def test_caching_issue(self): raise From 5fcae0cb80792a9d2aab28a8164d8c6e96bab98e Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 3 Oct 2023 10:45:03 -0700 Subject: [PATCH 29/56] Add logic for is not subdomain but has ip test --- src/registrar/models/domain.py | 2 ++ src/registrar/tests/test_models_domain.py | 17 ++++++++--------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6e3a2304f..d758e1569 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -277,12 +277,14 @@ class Domain(TimeStampedModel, DomainHelper): return self.name in nameserver def checkHostIPCombo(self, nameserver: str, ip: list): + print("Do I come into checkHostIPCombo") if self.isSubdomain(nameserver) and (ip is None or ip == []): raise ValueError( "Nameserver %s needs to have an " "IP address because it is a subdomain" % nameserver ) elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): + print("In 2nd else statement") raise ValueError( "Nameserver %s cannot be linked " "because %s is not a subdomain" % (nameserver, ip) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index c0eac751d..e78a5a55b 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -1145,17 +1145,16 @@ class TestRegistrantNameservers(MockEppLib): ("ns2.nameserversubdomain.gov",), ] - @skip("not implemented yet") def test_not_subdomain_but_has_ip(self): - # TO FIX - Logic - # domain, _ = Domain.objects.get_or_create( - # name="nameserversubdomain.gov", state=Domain.State.READY - # ) + domain, _ = Domain.objects.get_or_create( + name="nameserversubdomain.gov", state=Domain.State.READY + ) - # with self.assertRaises(ValueError): - # domain.nameservers = [("ns1.cats-da-best.gov", ["1.2.3.4"]), - # ("ns2.cats-da-best.gov", ["2.3.4.5"]),] - raise + with self.assertRaises(ValueError): + domain.nameservers = [ + ("ns1.cats-da-best.gov", ["1.2.3.4"]), + ("ns2.cats-da-best.gov", ["2.3.4.5"]), + ] def test_is_subdomain_but_ip_addr_not_valid(self): domain, _ = Domain.objects.get_or_create( From fbad2abd9d228114241c2d107948d780b8b61d95 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Wed, 4 Oct 2023 09:14:09 -0700 Subject: [PATCH 30/56] Remove extraneous print statements and rename extendedValues to threeNS --- src/registrar/models/domain.py | 2 -- src/registrar/tests/common.py | 7 ++++--- src/registrar/tests/test_models_domain.py | 10 +++++----- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index d758e1569..6e3a2304f 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -277,14 +277,12 @@ class Domain(TimeStampedModel, DomainHelper): return self.name in nameserver def checkHostIPCombo(self, nameserver: str, ip: list): - print("Do I come into checkHostIPCombo") if self.isSubdomain(nameserver) and (ip is None or ip == []): raise ValueError( "Nameserver %s needs to have an " "IP address because it is a subdomain" % nameserver ) elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): - print("In 2nd else statement") raise ValueError( "Nameserver %s cannot be linked " "because %s is not a subdomain" % (nameserver, ip) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 5a3887f3b..dce377188 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -644,14 +644,15 @@ class MockEppLib(TestCase): ], ) - # TODO-848: Fix naming - extendedValues = False + # Bc we have multiple tests utilizing 3 nameservers, + # easier to set a flag for it + threeNS = False def _getattrInfoDomain(self, _request): if getattr(_request, "name", None) == "security.gov": return MagicMock(res_data=[self.infoDomainNoContact]) elif getattr(_request, "name", None) == "my-nameserver.gov": - if self.extendedValues: + if self.threeNS: return MagicMock(res_data=[self.infoDomainThreeHosts]) elif self.mockedSendFunction.call_count == 5: return MagicMock(res_data=[self.infoDomainTwoHosts]) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index e78a5a55b..4026eabab 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -859,7 +859,7 @@ class TestRegistrantNameservers(MockEppLib): """ # Mock is set to return 3 nameservers on infodomain - self.extendedValues = True + self.threeNS = True self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] expectedCalls = [ # calls info domain, and info on all hosts @@ -900,7 +900,7 @@ class TestRegistrantNameservers(MockEppLib): And `domain.is_active` returns False """ - self.extendedValues = True + self.threeNS = True self.domain.ready() self.domain.nameservers = [(self.nameserver1,)] expectedCalls = [ @@ -952,7 +952,7 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - self.extendedValues = True + self.threeNS = True self.domain.ready() self.domain.nameservers = [ (self.nameserver1,), @@ -1112,7 +1112,7 @@ class TestRegistrantNameservers(MockEppLib): # sent like this, and then implementing appropriate mocks for any errors the # registry normally sends in this case - self.extendedValues = True + self.threeNS = True # Checking that it doesn't create or update even if out of order self.domain.nameservers = [ @@ -1194,7 +1194,7 @@ class TestRegistrantNameservers(MockEppLib): # print(self.mockedSendFunction.call_args_list) def tearDown(self): - self.extendedValues = False + self.threeNS = False return super().tearDown() From be78b87c4a64816fec0be6e1f438e52d078fe99d Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Wed, 4 Oct 2023 09:32:19 -0700 Subject: [PATCH 31/56] Remove extraneous prints --- src/registrar/models/domain.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 9f62d61fe..81104d3bb 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -452,13 +452,9 @@ class Domain(TimeStampedModel, DomainHelper): len(oldNameservers) - successDeletedCount + successCreatedCount ) - # successTotalNameservers = self._update_delete_create_hosts( - # deleted_values, updated_values, new_values, oldNameservers - # ) if successTotalNameservers < 2: try: - print("DNS_NEEDED: We have less than 2 nameservers") self.dns_needed() self.save() except Exception as err: From fdaee606bbb1022fdb4a6fe0fe301bb87333b4fb Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 4 Oct 2023 13:43:40 -0600 Subject: [PATCH 32/56] Fixed Gabys email --- src/registrar/fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index 434cd28e4..6b18f37e1 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -46,6 +46,7 @@ class UserFixture: "username": "70488e0a-e937-4894-a28c-16f5949effd4", "first_name": "Gaby", "last_name": "DiSarli", + "email": "gaby@truss.works", }, { "username": "83c2b6dd-20a2-4cac-bb40-e22a72d2955c", @@ -126,7 +127,6 @@ class UserFixture: "username": "0eb6f326-a3d4-410f-a521-aa4c1fad4e47", "first_name": "Gaby-Analyst", "last_name": "DiSarli-Analyst", - "email": "gaby@truss.works", }, { "username": "cfe7c2fc-e24a-480e-8b78-28645a1459b3", From 6944b31ba0672d177d207147415a126b6ae88cbe Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 4 Oct 2023 17:57:16 -0700 Subject: [PATCH 33/56] added nameserver error class --- .../models/utility/nameserver_error.py | 49 +++++++++++++++++++ src/registrar/tests/test_nameserver_error.py | 37 ++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 src/registrar/models/utility/nameserver_error.py create mode 100644 src/registrar/tests/test_nameserver_error.py diff --git a/src/registrar/models/utility/nameserver_error.py b/src/registrar/models/utility/nameserver_error.py new file mode 100644 index 000000000..075494a1c --- /dev/null +++ b/src/registrar/models/utility/nameserver_error.py @@ -0,0 +1,49 @@ +from enum import IntEnum + + +class NameserverErrorCodes(IntEnum): + """Used in the NameserverError class for + error mapping. + Overview of nameserver error codes: + - + """ + + MISSING_IP = 1 + GLUE_RECORD_NOT_ALLOWED = 2 + INVALID_IP = 3 + TOO_MANY_HOSTS=4 + + + +class NameserverError(Exception): + """ + Overview of contact error codes: + + """ + + # For linter + + _error_mapping = { + NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an " + "IP address because it is a subdomain", + NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked " + "because it is not a subdomain", + NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", + NameserverErrorCodes.TOO_MANY_HOSTS: "Too many hosts provided, you may not have more than 13 nameservers.", + + } + + def __init__(self, *args, code=None,nameserver=None,ip=None, **kwargs): + super().__init__(*args, **kwargs) + self.code = code + if self.code in self._error_mapping: + self.message = self._error_mapping.get(self.code) + if nameserver is not None and ip is not None: + self.message=self.message.format(str(nameserver),str(ip)) + elif nameserver is not None: + self.message=self.message.format(str(nameserver)) + elif ip is not None: + self.message=self.message.format(str(ip)) + + def __str__(self): + return f"{self.message}" \ No newline at end of file diff --git a/src/registrar/tests/test_nameserver_error.py b/src/registrar/tests/test_nameserver_error.py new file mode 100644 index 000000000..91f452f91 --- /dev/null +++ b/src/registrar/tests/test_nameserver_error.py @@ -0,0 +1,37 @@ +from django.test import TestCase + +from registrar.models.utility.nameserver_error import ( + NameserverError, + NameserverErrorCodes as nsErrorCodes) + + +class TestNameserverError(TestCase): + + def test_with_no_ip(self): + """Test NameserverError when no ip address is passed""" + nameserver="nameserver val" + expected=f"Nameserver {nameserver} needs to have an "\ + "IP address because it is a subdomain" + + nsException=NameserverError(code=nsErrorCodes.MISSING_IP,nameserver=nameserver) + self.assertEqual(nsException.message,expected) + self.assertEqual(nsException.code,nsErrorCodes.MISSING_IP) + + def test_with_only_code(self): + """Test NameserverError when no ip address or nameserver is passed, only the code value""" + nameserver="nameserver val" + expected="Too many hosts provided, you may not have more than 13 nameservers." + + nsException=NameserverError(code=nsErrorCodes.TOO_MANY_HOSTS,nameserver=nameserver) + self.assertEqual(nsException.message,expected) + self.assertEqual(nsException.code,nsErrorCodes.TOO_MANY_HOSTS) + + def test_with_ip_nameserver(self): + """Test NameserverError when ip and nameserver are passed""" + ip="ip val" + nameserver="nameserver val" + + expected=f"Nameserver {nameserver} has an invalid IP address: {ip}" + nsException=NameserverError(code=nsErrorCodes.INVALID_IP,nameserver=nameserver, ip=ip) + self.assertEqual(nsException.message,expected) + self.assertEqual(nsException.code,nsErrorCodes.INVALID_IP) \ No newline at end of file From f7e168de199540c5842aca50fde2ad190bea6a1d Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 4 Oct 2023 17:58:33 -0700 Subject: [PATCH 34/56] added nameserver error class to be used in domain --- src/registrar/models/domain.py | 30 +++++----- .../models/utility/nameserver_error.py | 26 ++++----- src/registrar/tests/test_models_domain.py | 15 ++--- src/registrar/tests/test_nameserver_error.py | 58 +++++++++++-------- 4 files changed, 64 insertions(+), 65 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 81104d3bb..be7d8ee59 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -15,7 +15,10 @@ from epplibwrapper import ( RegistryError, ErrorCode, ) - +from registrar.models.utility.nameserver_error import ( + NameserverError, + NameserverErrorCodes as nsErrorCodes, +) from .utility.domain_field import DomainField from .utility.domain_helper import DomainHelper from .utility.time_stamped_model import TimeStampedModel @@ -231,8 +234,9 @@ class Domain(TimeStampedModel, DomainHelper): try: hosts = self._get_property("hosts") except Exception as err: - # TODO-848: Check/add to error handling ticket if it's not addressed - # (Don't throw error as this is normal for a new domain?) + # Do not raise error when missing nameservers + # this is a standard occurence when a domain + # is first created logger.info("Domain is missing nameservers %s" % err) return [] @@ -279,20 +283,17 @@ class Domain(TimeStampedModel, DomainHelper): def checkHostIPCombo(self, nameserver: str, ip: list): if self.isSubdomain(nameserver) and (ip is None or ip == []): - raise ValueError( - "Nameserver %s needs to have an " - "IP address because it is a subdomain" % nameserver - ) + raise NameserverError(code=nsErrorCodes.MISSING_IP, nameserver=nameserver) + elif not self.isSubdomain(nameserver) and (ip is not None and ip != []): - raise ValueError( - "Nameserver %s cannot be linked " - "because %s is not a subdomain" % (nameserver, ip) + raise NameserverError( + code=nsErrorCodes.GLUE_RECORD_NOT_ALLOWED, nameserver=nameserver, ip=ip ) elif ip is not None and ip != []: for addr in ip: if not self._valid_ip_addr(addr): - raise ValueError( - "Nameserver %s has an invalid IP address: %s" % (nameserver, ip) + raise NameserverError( + code=nsErrorCodes.INVALID_IP, nameserver=nameserver, ip=ip ) return None @@ -428,9 +429,7 @@ class Domain(TimeStampedModel, DomainHelper): example: [(ns1.okay.gov, [127.0.0.1, others ips])]""" if len(hosts) > 13: - raise ValueError( - "Too many hosts provided, you may not have more than 13 nameservers." - ) + raise NameserverError(code=nsErrorCodes.TOO_MANY_HOSTS) logger.info("Setting nameservers") logger.info(hosts) @@ -452,7 +451,6 @@ class Domain(TimeStampedModel, DomainHelper): len(oldNameservers) - successDeletedCount + successCreatedCount ) - if successTotalNameservers < 2: try: self.dns_needed() diff --git a/src/registrar/models/utility/nameserver_error.py b/src/registrar/models/utility/nameserver_error.py index 075494a1c..32735a286 100644 --- a/src/registrar/models/utility/nameserver_error.py +++ b/src/registrar/models/utility/nameserver_error.py @@ -5,45 +5,43 @@ class NameserverErrorCodes(IntEnum): """Used in the NameserverError class for error mapping. Overview of nameserver error codes: - - + - """ MISSING_IP = 1 GLUE_RECORD_NOT_ALLOWED = 2 INVALID_IP = 3 - TOO_MANY_HOSTS=4 - + TOO_MANY_HOSTS = 4 class NameserverError(Exception): """ Overview of contact error codes: - + """ # For linter _error_mapping = { NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an " - "IP address because it is a subdomain", + "IP address because it is a subdomain", NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked " - "because it is not a subdomain", - NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", - NameserverErrorCodes.TOO_MANY_HOSTS: "Too many hosts provided, you may not have more than 13 nameservers.", - + "because it is not a subdomain", + NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", + NameserverErrorCodes.TOO_MANY_HOSTS: "Too many hosts provided, you may not have more than 13 nameservers.", } - def __init__(self, *args, code=None,nameserver=None,ip=None, **kwargs): + def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs): super().__init__(*args, **kwargs) self.code = code if self.code in self._error_mapping: self.message = self._error_mapping.get(self.code) if nameserver is not None and ip is not None: - self.message=self.message.format(str(nameserver),str(ip)) + self.message = self.message.format(str(nameserver), str(ip)) elif nameserver is not None: - self.message=self.message.format(str(nameserver)) + self.message = self.message.format(str(nameserver)) elif ip is not None: - self.message=self.message.format(str(ip)) + self.message = self.message.format(str(ip)) def __str__(self): - return f"{self.message}" \ No newline at end of file + return f"{self.message}" diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 78e217098..6e3b8420e 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -16,6 +16,7 @@ from registrar.models.domain_information import DomainInformation from registrar.models.draft_domain import DraftDomain from registrar.models.public_contact import PublicContact from registrar.models.user import User +from registrar.models.utility.nameserver_error import NameserverError from .common import MockEppLib from django_fsm import TransitionNotAllowed # type: ignore from epplibwrapper import ( @@ -1141,7 +1142,7 @@ class TestRegistrantNameservers(MockEppLib): name="nameserversubdomain.gov", state=Domain.State.READY ) - with self.assertRaises(ValueError): + with self.assertRaises(NameserverError): domain.nameservers = [ ("ns1.nameserversubdomain.gov",), ("ns2.nameserversubdomain.gov",), @@ -1152,7 +1153,7 @@ class TestRegistrantNameservers(MockEppLib): name="nameserversubdomain.gov", state=Domain.State.READY ) - with self.assertRaises(ValueError): + with self.assertRaises(NameserverError): domain.nameservers = [ ("ns1.cats-da-best.gov", ["1.2.3.4"]), ("ns2.cats-da-best.gov", ["2.3.4.5"]), @@ -1163,16 +1164,12 @@ class TestRegistrantNameservers(MockEppLib): name="nameserversubdomain.gov", state=Domain.State.READY ) - with self.assertRaises(ValueError): + with self.assertRaises(NameserverError): domain.nameservers = [ ("ns1.nameserversubdomain.gov", ["1.2.3"]), ("ns2.nameserversubdomain.gov", ["2.3.4"]), ] - @skip("not implemented yet") - def test_caching_issue(self): - raise - @skip("not implemented yet") def test_update_is_unsuccessful(self): """ @@ -1192,11 +1189,9 @@ class TestRegistrantNameservers(MockEppLib): with self.assertRaises(RegistryError): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] - # print("self.mockedSendFunction.call_args_list is ") - # print(self.mockedSendFunction.call_args_list) - def tearDown(self): self.threeNS = False + Domain.objects.all().delete() return super().tearDown() diff --git a/src/registrar/tests/test_nameserver_error.py b/src/registrar/tests/test_nameserver_error.py index 91f452f91..7398dbba7 100644 --- a/src/registrar/tests/test_nameserver_error.py +++ b/src/registrar/tests/test_nameserver_error.py @@ -1,37 +1,45 @@ from django.test import TestCase from registrar.models.utility.nameserver_error import ( - NameserverError, - NameserverErrorCodes as nsErrorCodes) + NameserverError, + NameserverErrorCodes as nsErrorCodes, +) class TestNameserverError(TestCase): - def test_with_no_ip(self): """Test NameserverError when no ip address is passed""" - nameserver="nameserver val" - expected=f"Nameserver {nameserver} needs to have an "\ - "IP address because it is a subdomain" - - nsException=NameserverError(code=nsErrorCodes.MISSING_IP,nameserver=nameserver) - self.assertEqual(nsException.message,expected) - self.assertEqual(nsException.code,nsErrorCodes.MISSING_IP) - + nameserver = "nameserver val" + expected = ( + f"Nameserver {nameserver} needs to have an " + "IP address because it is a subdomain" + ) + + nsException = NameserverError( + code=nsErrorCodes.MISSING_IP, nameserver=nameserver + ) + self.assertEqual(nsException.message, expected) + self.assertEqual(nsException.code, nsErrorCodes.MISSING_IP) + def test_with_only_code(self): """Test NameserverError when no ip address or nameserver is passed, only the code value""" - nameserver="nameserver val" - expected="Too many hosts provided, you may not have more than 13 nameservers." - - nsException=NameserverError(code=nsErrorCodes.TOO_MANY_HOSTS,nameserver=nameserver) - self.assertEqual(nsException.message,expected) - self.assertEqual(nsException.code,nsErrorCodes.TOO_MANY_HOSTS) - + nameserver = "nameserver val" + expected = "Too many hosts provided, you may not have more than 13 nameservers." + + nsException = NameserverError( + code=nsErrorCodes.TOO_MANY_HOSTS, nameserver=nameserver + ) + self.assertEqual(nsException.message, expected) + self.assertEqual(nsException.code, nsErrorCodes.TOO_MANY_HOSTS) + def test_with_ip_nameserver(self): """Test NameserverError when ip and nameserver are passed""" - ip="ip val" - nameserver="nameserver val" - - expected=f"Nameserver {nameserver} has an invalid IP address: {ip}" - nsException=NameserverError(code=nsErrorCodes.INVALID_IP,nameserver=nameserver, ip=ip) - self.assertEqual(nsException.message,expected) - self.assertEqual(nsException.code,nsErrorCodes.INVALID_IP) \ No newline at end of file + ip = "ip val" + nameserver = "nameserver val" + + expected = f"Nameserver {nameserver} has an invalid IP address: {ip}" + nsException = NameserverError( + code=nsErrorCodes.INVALID_IP, nameserver=nameserver, ip=ip + ) + self.assertEqual(nsException.message, expected) + self.assertEqual(nsException.code, nsErrorCodes.INVALID_IP) From f15adde7120dfda99cb07fe3168540830c651553 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 4 Oct 2023 18:05:51 -0700 Subject: [PATCH 35/56] added more comments --- src/registrar/models/utility/nameserver_error.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/utility/nameserver_error.py b/src/registrar/models/utility/nameserver_error.py index 32735a286..df04fb7eb 100644 --- a/src/registrar/models/utility/nameserver_error.py +++ b/src/registrar/models/utility/nameserver_error.py @@ -5,7 +5,11 @@ class NameserverErrorCodes(IntEnum): """Used in the NameserverError class for error mapping. Overview of nameserver error codes: - - + - 1 MISSING_IP ip address is missing for a nameserver + - 2 GLUE_RECORD_NOT_ALLOWED a host has a nameserver + value but is not a subdomain + - 3 INVALID_IP invalid ip address format or invalid version + - 4 TOO_MANY_HOSTS more than the max allowed host values """ MISSING_IP = 1 @@ -16,12 +20,10 @@ class NameserverErrorCodes(IntEnum): class NameserverError(Exception): """ - Overview of contact error codes: - + NameserverError class used when to raise exceptions on + the nameserver getter """ - # For linter - _error_mapping = { NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an " "IP address because it is a subdomain", From 851c58953b52bbf738becb4c03fb24432988fe98 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 4 Oct 2023 18:21:19 -0700 Subject: [PATCH 36/56] updated tests to include the nameservererror class, though they are still failing --- src/registrar/models/utility/nameserver_error.py | 6 ++++-- src/registrar/tests/test_models_domain.py | 6 +++--- src/registrar/tests/test_nameserver_error.py | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/utility/nameserver_error.py b/src/registrar/models/utility/nameserver_error.py index df04fb7eb..96d4b5af3 100644 --- a/src/registrar/models/utility/nameserver_error.py +++ b/src/registrar/models/utility/nameserver_error.py @@ -20,7 +20,7 @@ class NameserverErrorCodes(IntEnum): class NameserverError(Exception): """ - NameserverError class used when to raise exceptions on + NameserverError class used to raise exceptions on the nameserver getter """ @@ -30,7 +30,9 @@ class NameserverError(Exception): NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked " "because it is not a subdomain", NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", - NameserverErrorCodes.TOO_MANY_HOSTS: "Too many hosts provided, you may not have more than 13 nameservers.", + NameserverErrorCodes.TOO_MANY_HOSTS: ( + "Too many hosts provided, you may not have more than " "13 nameservers." + ), } def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 6e3b8420e..1f4aaed5a 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -848,7 +848,7 @@ class TestRegistrantNameservers(MockEppLib): (nameserver14,), ] - self.assertRaises(ValueError, _get_14_nameservers) + self.assertRaises(NameserverError, _get_14_nameservers) self.assertEqual(self.mockedSendFunction.call_count, 0) def test_user_removes_some_nameservers(self): @@ -1032,7 +1032,7 @@ class TestRegistrantNameservers(MockEppLib): dotgovnameserver = "my-nameserver.gov" - with self.assertRaises(ValueError): + with self.assertRaises(NameserverError): self.domain.nameservers = [(dotgovnameserver,)] def test_user_updates_ips(self): @@ -1101,7 +1101,7 @@ class TestRegistrantNameservers(MockEppLib): """ dotgovnameserver = "mynameserverdotgov.gov" - with self.assertRaises(ValueError): + with self.assertRaises(NameserverError): self.domain.nameservers = [(dotgovnameserver, ["1.2.3"])] def test_nameservers_are_idempotent(self): diff --git a/src/registrar/tests/test_nameserver_error.py b/src/registrar/tests/test_nameserver_error.py index 7398dbba7..2f11977ac 100644 --- a/src/registrar/tests/test_nameserver_error.py +++ b/src/registrar/tests/test_nameserver_error.py @@ -22,7 +22,8 @@ class TestNameserverError(TestCase): self.assertEqual(nsException.code, nsErrorCodes.MISSING_IP) def test_with_only_code(self): - """Test NameserverError when no ip address or nameserver is passed, only the code value""" + """Test NameserverError when no ip address + and no nameserver is passed""" nameserver = "nameserver val" expected = "Too many hosts provided, you may not have more than 13 nameservers." From 01ad6b5523193279ededac80108fa747102e03d2 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Thu, 5 Oct 2023 17:33:50 -0700 Subject: [PATCH 37/56] moved nameserver error to utility --- src/registrar/models/domain.py | 2 +- .../models/utility/nameserver_error.py | 51 ------------------ src/registrar/tests/test_models_domain.py | 2 +- src/registrar/tests/test_nameserver_error.py | 2 +- src/registrar/utility/errors.py | 53 +++++++++++++++++++ 5 files changed, 56 insertions(+), 54 deletions(-) delete mode 100644 src/registrar/models/utility/nameserver_error.py diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index be7d8ee59..025a7392f 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -15,7 +15,7 @@ from epplibwrapper import ( RegistryError, ErrorCode, ) -from registrar.models.utility.nameserver_error import ( +from registrar.utility.errors import ( NameserverError, NameserverErrorCodes as nsErrorCodes, ) diff --git a/src/registrar/models/utility/nameserver_error.py b/src/registrar/models/utility/nameserver_error.py deleted file mode 100644 index 96d4b5af3..000000000 --- a/src/registrar/models/utility/nameserver_error.py +++ /dev/null @@ -1,51 +0,0 @@ -from enum import IntEnum - - -class NameserverErrorCodes(IntEnum): - """Used in the NameserverError class for - error mapping. - Overview of nameserver error codes: - - 1 MISSING_IP ip address is missing for a nameserver - - 2 GLUE_RECORD_NOT_ALLOWED a host has a nameserver - value but is not a subdomain - - 3 INVALID_IP invalid ip address format or invalid version - - 4 TOO_MANY_HOSTS more than the max allowed host values - """ - - MISSING_IP = 1 - GLUE_RECORD_NOT_ALLOWED = 2 - INVALID_IP = 3 - TOO_MANY_HOSTS = 4 - - -class NameserverError(Exception): - """ - NameserverError class used to raise exceptions on - the nameserver getter - """ - - _error_mapping = { - NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an " - "IP address because it is a subdomain", - NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked " - "because it is not a subdomain", - NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", - NameserverErrorCodes.TOO_MANY_HOSTS: ( - "Too many hosts provided, you may not have more than " "13 nameservers." - ), - } - - def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs): - super().__init__(*args, **kwargs) - self.code = code - if self.code in self._error_mapping: - self.message = self._error_mapping.get(self.code) - if nameserver is not None and ip is not None: - self.message = self.message.format(str(nameserver), str(ip)) - elif nameserver is not None: - self.message = self.message.format(str(nameserver)) - elif ip is not None: - self.message = self.message.format(str(ip)) - - def __str__(self): - return f"{self.message}" diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 1f4aaed5a..9864c7f1a 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -16,7 +16,7 @@ from registrar.models.domain_information import DomainInformation from registrar.models.draft_domain import DraftDomain from registrar.models.public_contact import PublicContact from registrar.models.user import User -from registrar.models.utility.nameserver_error import NameserverError +from registrar.utility.errors import NameserverError from .common import MockEppLib from django_fsm import TransitionNotAllowed # type: ignore from epplibwrapper import ( diff --git a/src/registrar/tests/test_nameserver_error.py b/src/registrar/tests/test_nameserver_error.py index 2f11977ac..c64717eb5 100644 --- a/src/registrar/tests/test_nameserver_error.py +++ b/src/registrar/tests/test_nameserver_error.py @@ -1,6 +1,6 @@ from django.test import TestCase -from registrar.models.utility.nameserver_error import ( +from registrar.utility.errors import ( NameserverError, NameserverErrorCodes as nsErrorCodes, ) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 3b17a17c7..98c2e6667 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -1,3 +1,6 @@ +from enum import IntEnum + + class BlankValueError(ValueError): pass @@ -8,3 +11,53 @@ class ExtraDotsError(ValueError): class DomainUnavailableError(ValueError): pass + + +class NameserverErrorCodes(IntEnum): + """Used in the NameserverError class for + error mapping. + Overview of nameserver error codes: + - 1 MISSING_IP ip address is missing for a nameserver + - 2 GLUE_RECORD_NOT_ALLOWED a host has a nameserver + value but is not a subdomain + - 3 INVALID_IP invalid ip address format or invalid version + - 4 TOO_MANY_HOSTS more than the max allowed host values + """ + + MISSING_IP = 1 + GLUE_RECORD_NOT_ALLOWED = 2 + INVALID_IP = 3 + TOO_MANY_HOSTS = 4 + + +class NameserverError(Exception): + """ + NameserverError class used to raise exceptions on + the nameserver getter + """ + + _error_mapping = { + NameserverErrorCodes.MISSING_IP: "Nameserver {} needs to have an " + "IP address because it is a subdomain", + NameserverErrorCodes.GLUE_RECORD_NOT_ALLOWED: "Nameserver {} cannot be linked " + "because it is not a subdomain", + NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", + NameserverErrorCodes.TOO_MANY_HOSTS: ( + "Too many hosts provided, you may not have more than " "13 nameservers." + ), + } + + def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs): + super().__init__(*args, **kwargs) + self.code = code + if self.code in self._error_mapping: + self.message = self._error_mapping.get(self.code) + if nameserver is not None and ip is not None: + self.message = self.message.format(str(nameserver), str(ip)) + elif nameserver is not None: + self.message = self.message.format(str(nameserver)) + elif ip is not None: + self.message = self.message.format(str(ip)) + + def __str__(self): + return f"{self.message}" From 8b17d184ebd3ec46111ab10b17b433d0ced257df Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 6 Oct 2023 08:53:21 -0700 Subject: [PATCH 38/56] Ran formatter --- src/registrar/models/domain.py | 1 + src/registrar/tests/common.py | 5 ++--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 73b502913..eabbfcd74 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1366,6 +1366,7 @@ class Domain(TimeStampedModel, DomainHelper): def is_ipv6(self, ip: str): ip_addr = ipaddress.ip_address(ip) return ip_addr.version == 6 + def _fetch_hosts(self, host_data): """Fetch host info.""" hosts = [] diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 6050ef8b1..a444522c1 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -680,7 +680,6 @@ class MockEppLib(TestCase): "ns1.my-nameserver-2.com", "ns1.cats-are-superior3.com", ], - ) infoDomainNoHost = fakedEppObject( "my-nameserver.gov", @@ -717,7 +716,7 @@ class MockEppLib(TestCase): ], addrs=["1.2.3.4", "2.3.4.5"], ) - + infoDomainCheckHostIPCombo = fakedEppObject( "nameserversubdomain.gov", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), @@ -759,7 +758,7 @@ class MockEppLib(TestCase): but only relevant pieces for tests""" if isinstance(_request, commands.InfoDomain): return self._getattrInfoDomain(_request) - + elif isinstance(_request, commands.InfoContact): mocked_result: info.InfoContactResultData From a4057a669f689d4bf809525c3ee86c510bba2d4e Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 6 Oct 2023 11:48:45 -0700 Subject: [PATCH 39/56] updated test to remove theeNS and stop settings if not ready or DNS needed --- src/registrar/models/domain.py | 8 ++-- src/registrar/tests/common.py | 12 ++--- src/registrar/tests/test_models_domain.py | 55 ++++++++++++----------- src/registrar/utility/errors.py | 7 +++ 4 files changed, 44 insertions(+), 38 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index eabbfcd74..ba4db0ef9 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -17,6 +17,7 @@ from epplibwrapper import ( ErrorCode, ) from registrar.utility.errors import ( + ActionNotAllowed, NameserverError, NameserverErrorCodes as nsErrorCodes, ) @@ -433,6 +434,10 @@ class Domain(TimeStampedModel, DomainHelper): if len(hosts) > 13: raise NameserverError(code=nsErrorCodes.TOO_MANY_HOSTS) + + if self.state not in [self.State.DNS_NEEDED, self.State.READY]: + raise ActionNotAllowed("Nameservers can not be " "set in the current state") + logger.info("Setting nameservers") logger.info(hosts) @@ -465,9 +470,6 @@ class Domain(TimeStampedModel, DomainHelper): ) elif successTotalNameservers >= 2 and successTotalNameservers <= 13: try: - print( - "READY/SAVE: We are in happy path where btwen 2 and 13 inclusive ns" - ) self.ready() self.save() except Exception as err: diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index a444522c1..b0bd701aa 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -727,17 +727,11 @@ class MockEppLib(TestCase): ], ) - # Bc we have multiple tests utilizing 3 nameservers, - # easier to set a flag for it - threeNS = False - def _getattrInfoDomain(self, _request): if getattr(_request, "name", None) == "security.gov": return MagicMock(res_data=[self.infoDomainNoContact]) elif getattr(_request, "name", None) == "my-nameserver.gov": - if self.threeNS: - return MagicMock(res_data=[self.infoDomainThreeHosts]) - elif self.mockedSendFunction.call_count == 5: + if self.mockedSendFunction.call_count == 5: return MagicMock(res_data=[self.infoDomainTwoHosts]) else: return MagicMock(res_data=[self.infoDomainNoHost]) @@ -747,8 +741,8 @@ class MockEppLib(TestCase): return MagicMock(res_data=[self.infoDomainCheckHostIPCombo]) elif getattr(_request, "name", None) == "freeman.gov": return MagicMock(res_data=[self.InfoDomainWithContacts]) - # elif getattr(_request, "name", None) == "failednameserver.gov": - # return MagicMock(res_data=[self.infoDomainUpdateFail]) + elif getattr(_request, "name", None) == "threenameserversDomain.gov": + return MagicMock(res_data=[self.infoDomainThreeHosts]) return MagicMock(res_data=[self.mockDataInfoDomain]) def mockSend(self, _request, cleaned): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index c1107a590..5cdd3c79d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -16,7 +16,7 @@ from registrar.models.domain_information import DomainInformation from registrar.models.draft_domain import DraftDomain from registrar.models.public_contact import PublicContact from registrar.models.user import User -from registrar.utility.errors import NameserverError +from registrar.utility.errors import ActionNotAllowed, NameserverError from .common import MockEppLib from django_fsm import TransitionNotAllowed # type: ignore from epplibwrapper import ( @@ -862,6 +862,9 @@ class TestRegistrantNameservers(MockEppLib): self.domain, _ = Domain.objects.get_or_create( name="my-nameserver.gov", state=Domain.State.DNS_NEEDED ) + self.domainWithThreeNS, _ = Domain.objects.get_or_create( + name="threenameserversDomain.gov", state=Domain.State.READY + ) def test_get_nameserver_changes_success_deleted_vals(self): # Testing only deleting and no other changes @@ -1063,14 +1066,13 @@ class TestRegistrantNameservers(MockEppLib): """ # Mock is set to return 3 nameservers on infodomain - self.threeNS = True - self.domain.nameservers = [(self.nameserver1,), (self.nameserver2,)] + self.domainWithThreeNS.nameservers = [(self.nameserver1,), (self.nameserver2,)] expectedCalls = [ # calls info domain, and info on all hosts # to get past values # then removes the single host and updates domain call( - commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + commands.InfoDomain(name=self.domainWithThreeNS.name, auth_info=None), cleaned=True, ), call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), @@ -1078,7 +1080,7 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[], rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], nsset=None, @@ -1104,12 +1106,11 @@ class TestRegistrantNameservers(MockEppLib): And `domain.is_active` returns False """ - self.threeNS = True - self.domain.ready() - self.domain.nameservers = [(self.nameserver1,)] + + self.domainWithThreeNS.nameservers = [(self.nameserver1,)] expectedCalls = [ call( - commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + commands.InfoDomain(name=self.domainWithThreeNS.name, auth_info=None), cleaned=True, ), call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), @@ -1117,7 +1118,7 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[], rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], nsset=None, @@ -1130,7 +1131,7 @@ class TestRegistrantNameservers(MockEppLib): call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[], rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], nsset=None, @@ -1156,9 +1157,8 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - self.threeNS = True - self.domain.ready() - self.domain.nameservers = [ + + self.domainWithThreeNS.nameservers = [ (self.nameserver1,), ("ns1.cats-are-superior1.com",), ("ns1.cats-are-superior2.com",), @@ -1166,7 +1166,7 @@ class TestRegistrantNameservers(MockEppLib): expectedCalls = [ call( - commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + commands.InfoDomain(name=self.domainWithThreeNS.name, auth_info=None), cleaned=True, ), call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), @@ -1174,7 +1174,7 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[], rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], nsset=None, @@ -1191,7 +1191,7 @@ class TestRegistrantNameservers(MockEppLib): ), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[common.HostObjSet(hosts=["ns1.cats-are-superior1.com"])], rem=[], nsset=None, @@ -1207,7 +1207,7 @@ class TestRegistrantNameservers(MockEppLib): ), call( commands.UpdateDomain( - name="my-nameserver.gov", + name=self.domainWithThreeNS.name, add=[common.HostObjSet(hosts=["ns1.cats-are-superior2.com"])], rem=[], nsset=None, @@ -1312,14 +1312,9 @@ class TestRegistrantNameservers(MockEppLib): to the registry twice with identical data Then no errors are raised in Domain """ - # implementation note: this requires seeing what happens when these are actually - # sent like this, and then implementing appropriate mocks for any errors the - # registry normally sends in this case - - self.threeNS = True # Checking that it doesn't create or update even if out of order - self.domain.nameservers = [ + self.domainWithThreeNS.nameservers = [ (self.nameserver3,), (self.nameserver1,), (self.nameserver2,), @@ -1327,7 +1322,7 @@ class TestRegistrantNameservers(MockEppLib): expectedCalls = [ call( - commands.InfoDomain(name="my-nameserver.gov", auth_info=None), + commands.InfoDomain(name=self.domainWithThreeNS.name, auth_info=None), cleaned=True, ), call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), @@ -1371,6 +1366,15 @@ class TestRegistrantNameservers(MockEppLib): ("ns2.nameserversubdomain.gov", ["2.3.4"]), ] + def test_setting_not_allowed(self): + """Scenario: A domain state is not Ready or DNS Needed + then setting nameservers is not allowed""" + domain, _ = Domain.objects.get_or_create( + name="onholdDomain.gov", state=Domain.State.ON_HOLD + ) + with self.assertRaises(ActionNotAllowed): + domain.nameservers = [self.nameserver1, self.nameserver2] + @skip("not implemented yet") def test_update_is_unsuccessful(self): """ @@ -1391,7 +1395,6 @@ class TestRegistrantNameservers(MockEppLib): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] def tearDown(self): - self.threeNS = False Domain.objects.all().delete() return super().tearDown() diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 98c2e6667..5eb92750e 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -13,6 +13,13 @@ class DomainUnavailableError(ValueError): pass +class ActionNotAllowed(Exception): + """User accessed an action that is not + allowed by the current state""" + + pass + + class NameserverErrorCodes(IntEnum): """Used in the NameserverError class for error mapping. From e7094e070fc934ab9eca36019ede6de89019386a Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 6 Oct 2023 16:31:50 -0700 Subject: [PATCH 40/56] remove test bug --- src/registrar/models/domain.py | 2 +- src/registrar/tests/test_models_domain.py | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index ba4db0ef9..6cd8f4f54 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -449,11 +449,11 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) + successCreatedCount = self._new_host_values(new_values) successDeletedCount = self._deleted_host_values(deleted_values) # returns value _ = self._update_host_values( updated_values, oldNameservers ) # returns nothing, just need to be run and errors - successCreatedCount = self._new_host_values(new_values) successTotalNameservers = ( len(oldNameservers) - successDeletedCount + successCreatedCount diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 5cdd3c79d..60f7a3e14 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -1094,7 +1094,7 @@ class TestRegistrantNameservers(MockEppLib): ] self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) - self.assertTrue(self.domain.is_active()) + self.assertTrue(self.domainWithThreeNS.is_active()) def test_user_removes_too_many_nameservers(self): """ @@ -1145,7 +1145,7 @@ class TestRegistrantNameservers(MockEppLib): ] self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) - self.assertFalse(self.domain.is_active()) + self.assertFalse(self.domainWithThreeNS.is_active()) def test_user_replaces_nameservers(self): """ @@ -1157,7 +1157,7 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - + print("in second failing test") self.domainWithThreeNS.nameservers = [ (self.nameserver1,), ("ns1.cats-are-superior1.com",), @@ -1220,7 +1220,7 @@ class TestRegistrantNameservers(MockEppLib): ] self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) - self.assertTrue(self.domain.is_active()) + self.assertTrue(self.domainWithThreeNS.is_active()) def test_user_cannot_add_subordinate_without_ip(self): """ @@ -1395,6 +1395,7 @@ class TestRegistrantNameservers(MockEppLib): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] def tearDown(self): + print("in tear down") Domain.objects.all().delete() return super().tearDown() From 0e9b777ab506d061cf77267fed9ec907ce460aab Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 6 Oct 2023 17:05:45 -0700 Subject: [PATCH 41/56] updated comments --- src/registrar/models/domain.py | 96 +++++++++++++++++------ src/registrar/tests/test_models_domain.py | 4 +- 2 files changed, 75 insertions(+), 25 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6cd8f4f54..2157b304d 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -272,9 +272,15 @@ class Domain(TimeStampedModel, DomainHelper): return e.code def _convert_list_to_dict(self, listToConvert: list[tuple[str, list]]): + """converts a list of hosts into a dictionary + Args: + list[tuple[str, list]]: such as [("123",["1","2","3"])] + This is the list of hosts to convert + + returns: + convertDict (dict(str,list))- such as{"123":["1","2","3"]}""" newDict: dict[str, Any] = {} - # TODO-848: If duplicated nameserver names, throw error for tup in listToConvert: if len(tup) == 1: newDict[tup[0]] = None @@ -282,10 +288,24 @@ class Domain(TimeStampedModel, DomainHelper): newDict[tup[0]] = tup[1] return newDict - def isSubdomain(self, nameserver): + def isSubdomain(self, nameserver: str): + """Returns boolean if the domain name is found in the argument passed""" return self.name in nameserver - def checkHostIPCombo(self, nameserver: str, ip: list): + def checkHostIPCombo(self, nameserver: str, ip: list[str]): + """Checks the parameters past for a valid combination + raises error if: + - nameserver is a subdomain but is missing ip + - nameserver is not a subdomain but is missing ip + - nameserver is a subdomain but an ip passed is invalid + + Args: + hostname (str)- nameserver or subdomain + ip (list[str])-list of ip strings + Throws: + NameserverError (if exception hit) + Returns: + None""" if self.isSubdomain(nameserver) and (ip is None or ip == []): raise NameserverError(code=nsErrorCodes.MISSING_IP, nameserver=nameserver) @@ -301,7 +321,11 @@ class Domain(TimeStampedModel, DomainHelper): ) return None - def _valid_ip_addr(self, ip): + def _valid_ip_addr(self, ip: str): + """returns boolean if valid ip address string + We currently only accept v4 or v6 ips + returns: + isValid (boolean)-True for valid ip address""" try: ip = ipaddress.ip_address(ip) return ip.version == 6 or ip.version == 4 @@ -315,11 +339,17 @@ class Domain(TimeStampedModel, DomainHelper): """ calls self.nameserver, it should pull from cache but may result in an epp call - returns tuple of four values as follows: - deleted_values: list - updated_values: list - new_values: dict - prevHostDict: dict""" + Args: + hosts: list[tuple[str, list]] such as [("123",["1","2","3"])] + Throws: + NameserverError (if exception hit) + Returns: + tuple[list, list, dict, dict] + These four tuple values as follows: + deleted_values: list[str] + updated_values: list[str] + new_values: dict(str,list) + prevHostDict: dict(str,list)""" oldNameservers = self.nameservers @@ -428,7 +458,7 @@ class Domain(TimeStampedModel, DomainHelper): @nameservers.setter # type: ignore def nameservers(self, hosts: list[tuple[str, list]]): - """host should be a tuple of type str, str,... where the elements are + """Host should be a tuple of type str, str,... where the elements are Fully qualified host name, addresses associated with the host example: [(ns1.okay.gov, [127.0.0.1, others ips])]""" @@ -477,8 +507,6 @@ class Domain(TimeStampedModel, DomainHelper): "nameserver setter checked for create state " "and it did not succeed. Warning: %s" % err ) - # TODO-848: Handle removed nameservers here, - # will need to change the state then go back to DNS_NEEDED @Cache def statuses(self) -> list[str]: @@ -1154,17 +1182,6 @@ class Domain(TimeStampedModel, DomainHelper): self._remove_client_hold() # TODO -on the client hold ticket any additional error handling here - @transition(field="state", source=State.ON_HOLD, target=State.DELETED) - def deleted(self): - """domain is deleted in epp but is saved in our database""" - # TODO Domains may not be deleted if: - # a child host is being used by - # another .gov domains. The host must be first removed - # and/or renamed before the parent domain may be deleted. - logger.info("dns_needed_from_unknown()-> inside pending create") - self._delete_domain() - # TODO - delete ticket any additional error handling here - @transition( field="state", source=[State.ON_HOLD, State.DNS_NEEDED], target=State.DELETED ) @@ -1387,6 +1404,16 @@ class Domain(TimeStampedModel, DomainHelper): return hosts def _convert_ips(self, ip_list: list[str]): + """Convert Ips to a list of epp.Ip objects + use when sending update host command. + if there are no ips an empty list will be returned + + Args: + ip_list (list[str]): the new list of ips, may be empty + Returns: + edited_ip_list (list[epp.Ip]): list of epp.ip objects ready to + be sent to the registry + """ edited_ip_list = [] if ip_list is None: return [] @@ -1400,6 +1427,17 @@ class Domain(TimeStampedModel, DomainHelper): return edited_ip_list def _update_host(self, nameserver: str, ip_list: list[str], old_ip_list: list[str]): + """Update an existing host object in EPP. Sends the update host command + can result in a RegistryError + Args: + nameserver (str): nameserver or subdomain + ip_list (list[str]): the new list of ips, may be empty + old_ip_list (list[str]): the old ip list, may also be empty + + Returns: + errorCode (int): one of ErrorCode enum type values + + """ try: if ( ip_list is None @@ -1425,6 +1463,18 @@ class Domain(TimeStampedModel, DomainHelper): return e.code def _delete_host(self, nameserver: str): + """Remove this host from the domain and delete the host object in registry, + will only delete the host object, if it's not being used by another domain + Performs two epp calls and can result in a RegistryError + Args: + nameserver (str): nameserver or subdomain + ip_list (list[str]): the new list of ips, may be empty + old_ip_list (list[str]): the old ip list, may also be empty + + Returns: + errorCode (int): one of ErrorCode enum type values + + """ try: updateReq = commands.UpdateDomain( name=self.name, rem=[epp.HostObjSet([nameserver])] diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 60f7a3e14..34ba15cd5 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -867,7 +867,7 @@ class TestRegistrantNameservers(MockEppLib): ) def test_get_nameserver_changes_success_deleted_vals(self): - # Testing only deleting and no other changes + """Testing only deleting and no other changes""" self.domain._cache["hosts"] = [ {"name": "ns1.example.com", "addrs": None}, {"name": "ns2.example.com", "addrs": ["1.2.3.4"]}, @@ -891,7 +891,7 @@ class TestRegistrantNameservers(MockEppLib): ) def test_get_nameserver_changes_success_updated_vals(self): - # Testing only updating no other changes + """Testing only updating no other changes""" self.domain._cache["hosts"] = [ {"name": "ns3.my-nameserver.gov", "addrs": ["1.2.3.4"]}, ] From 65cf774e84a8cf5feed93b066d65411955b41367 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Fri, 6 Oct 2023 17:23:17 -0700 Subject: [PATCH 42/56] fix linter issue --- src/registrar/models/domain.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2157b304d..c905ab1ce 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -321,13 +321,13 @@ class Domain(TimeStampedModel, DomainHelper): ) return None - def _valid_ip_addr(self, ip: str): + def _valid_ip_addr(self, ipToTest: str): """returns boolean if valid ip address string We currently only accept v4 or v6 ips returns: isValid (boolean)-True for valid ip address""" try: - ip = ipaddress.ip_address(ip) + ip = ipaddress.ip_address(ipToTest) return ip.version == 6 or ip.version == 4 except ValueError: From ffede9817fda1a4a185a9a692f73cbc564c3e1b3 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Mon, 9 Oct 2023 21:13:49 -0700 Subject: [PATCH 43/56] changed UpdateDomain on nameserver.setter to be called with all the added/removed hosts in one call --- src/registrar/models/domain.py | 135 ++++++++++++++-------- src/registrar/tests/common.py | 9 +- src/registrar/tests/test_models_domain.py | 78 +++++-------- src/registrar/utility/errors.py | 5 + 4 files changed, 122 insertions(+), 105 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c905ab1ce..439f705f3 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -366,7 +366,7 @@ class Domain(TimeStampedModel, DomainHelper): # get deleted values-which are values in previous nameserver list # but are not in the list of new host values if prevHost not in newHostDict: - deleted_values.append((prevHost, addrs)) + deleted_values.append(prevHost) # if the host exists in both, check if the addresses changed else: # TODO - host is being updated when previous was None+new is empty list @@ -388,16 +388,6 @@ class Domain(TimeStampedModel, DomainHelper): return (deleted_values, updated_values, new_values, previousHostDict) - def _deleted_host_values(self, deleted_values) -> int: - successDeletedCount = 0 - - for hostTuple in deleted_values: - deleted_response_code = self._delete_host(hostTuple[0]) - if deleted_response_code == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: - successDeletedCount += 1 - - return successDeletedCount - def _update_host_values(self, updated_values, oldNameservers): for hostTuple in updated_values: updated_response_code = self._update_host( @@ -412,8 +402,15 @@ class Domain(TimeStampedModel, DomainHelper): % (hostTuple[0], updated_response_code) ) - def _new_host_values(self, new_values) -> int: - successCreatedCount = 0 + def createNewHostList(self, new_values: dict) -> list: + """convert the dictionary of new values to a list of HostObjSet + for use in the UpdateDomain epp message + Args: + new_values: dict(str,list)- dict of {nameserver:ips} to add to domain + Returns: + list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + """ + addToDomainList = [] for key, value in new_values.items(): createdCode = self._create_host( host=key, addrs=value @@ -422,18 +419,21 @@ class Domain(TimeStampedModel, DomainHelper): createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY or createdCode == ErrorCode.OBJECT_EXISTS ): - request = commands.UpdateDomain( - name=self.name, add=[epp.HostObjSet([key])] - ) - try: - registry.send(request, cleaned=True) - successCreatedCount += 1 - except RegistryError as e: - logger.error( - "Error adding nameserver, code was %s error was %s" - % (e.code, e) - ) - return successCreatedCount + addToDomainList.append(epp.HostObjSet([key])) + + return addToDomainList + + def createDeleteHostList(self, hostsToDelete: list[str]): + """ + Args: + hostsToDelete (list[str])- list of nameserver/host names to remove + Returns: + list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + """ + deleteList = [] + for nameserver in hostsToDelete: + deleteList.append(epp.HostObjSet([nameserver])) + return deleteList @Cache def dnssecdata(self) -> extensions.DNSSECExtension: @@ -479,16 +479,23 @@ class Domain(TimeStampedModel, DomainHelper): oldNameservers, ) = self.getNameserverChanges(hosts=hosts) - successCreatedCount = self._new_host_values(new_values) - successDeletedCount = self._deleted_host_values(deleted_values) # returns value _ = self._update_host_values( updated_values, oldNameservers ) # returns nothing, just need to be run and errors - - successTotalNameservers = ( - len(oldNameservers) - successDeletedCount + successCreatedCount + addToDomainList = self.createNewHostList(new_values) + deleteHostList = self.createDeleteHostList(deleted_values) + responseCode = self.addAndRemoveHostsFromDomain( + hostsToAdd=addToDomainList, hostsToDelete=deleteHostList ) + # if unable to update domain raise error and stop + if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: + raise NameserverError(code=nsErrorCodes.UNABLE_TO_UPDATE_DOMAIN) + + successTotalNameservers = ( + len(oldNameservers) - len(deleteHostList) + len(addToDomainList) + ) + self._delete_hosts_if_not_used(hostsToDelete=deleted_values) if successTotalNameservers < 2: try: self.dns_needed() @@ -1459,36 +1466,64 @@ class Domain(TimeStampedModel, DomainHelper): logger.info("_update_host()-> sending req as %s" % request) return response.code except RegistryError as e: - logger.error("Error _delete_host, code was %s error was %s" % (e.code, e)) + logger.error("Error _update_host, code was %s error was %s" % (e.code, e)) return e.code - def _delete_host(self, nameserver: str): - """Remove this host from the domain and delete the host object in registry, - will only delete the host object, if it's not being used by another domain - Performs two epp calls and can result in a RegistryError + def addAndRemoveHostsFromDomain( + self, hostsToAdd: list[str], hostsToDelete: list[str] + ): + """sends an UpdateDomain message to the registry with the hosts provided Args: - nameserver (str): nameserver or subdomain - ip_list (list[str]): the new list of ips, may be empty - old_ip_list (list[str]): the old ip list, may also be empty - + hostsToDelete (list[epp.HostObjSet])- list of host objects to delete + hostsToAdd (list[epp.HostObjSet])- list of host objects to add Returns: - errorCode (int): one of ErrorCode enum type values - + response code (int)- RegistryErrorCode integer value + defaults to return COMMAND_COMPLETED_SUCCESSFULLY + if there is nothing to add or delete """ + + if hostsToAdd == [] and hostsToDelete == []: + return ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY + try: updateReq = commands.UpdateDomain( - name=self.name, rem=[epp.HostObjSet([nameserver])] + name=self.name, rem=hostsToDelete, add=hostsToAdd ) response = registry.send(updateReq, cleaned=True) - logger.info("_delete_host()-> sending update domain req as %s" % updateReq) - - deleteHostReq = commands.DeleteHost(name=nameserver) - response = registry.send(deleteHostReq, cleaned=True) logger.info( - "_delete_host()-> sending delete host req as %s" % deleteHostReq + "addAndRemoveHostsFromDomain()-> sending update domain req as %s" + % updateReq ) - return response.code + + except RegistryError as e: + logger.error( + "Error addAndRemoveHostsFromDomain, code was %s error was %s" + % (e.code, e) + ) + + return response.code + + def _delete_hosts_if_not_used(self, hostsToDelete: list[str]): + """delete the host object in registry, + will only delete the host object, if it's not being used by another domain + Performs just the DeleteHost epp call + Supresses regstry error, as registry can disallow delete for various reasons + Args: + hostsToDelete (list[str])- list of nameserver/host names to remove + Returns: + None + + """ + try: + for nameserver in hostsToDelete: + deleteHostReq = commands.DeleteHost(name=nameserver) + registry.send(deleteHostReq, cleaned=True) + logger.info( + "_delete_hosts_if_not_used()-> sending delete host req as %s" + % deleteHostReq + ) + except RegistryError as e: if e.code == ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION: logger.info( @@ -1497,9 +1532,9 @@ class Domain(TimeStampedModel, DomainHelper): ) else: logger.error( - "Error _delete_host, code was %s error was %s" % (e.code, e) + "Error _delete_hosts_if_not_used, code was %s error was %s" + % (e.code, e) ) - return e.code def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index b0bd701aa..4c532947f 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -784,9 +784,12 @@ class MockEppLib(TestCase): res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, ) - # elif isinstance(_request, commands.UpdateHost): - # if getattr(_request, "name", None) == "ns1.failednameserver.gov": - # raise RegistryError(code=ErrorCode.OBJECT_EXISTS) + elif isinstance(_request, commands.UpdateHost): + return MagicMock( + res_data=[self.mockDataHostChange], + code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, + ) + elif isinstance(_request, commands.UpdateDomain): return MagicMock( res_data=[self.mockDataHostChange], code=ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY, diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 34ba15cd5..f018d647a 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -882,7 +882,7 @@ class TestRegistrantNameservers(MockEppLib): oldNameservers, ) = self.domain.getNameserverChanges(newChanges) - self.assertEqual(deleted_values, [("ns2.example.com", ["1.2.3.4"])]) + self.assertEqual(deleted_values, ["ns2.example.com"]) self.assertEqual(updated_values, []) self.assertEqual(new_values, {}) self.assertEqual( @@ -956,7 +956,9 @@ class TestRegistrantNameservers(MockEppLib): # when we create a host, we should've updated at the same time created_host = commands.CreateHost(nameserver) update_domain_with_created = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host.name])] + name=self.domain.name, + add=[common.HostObjSet([created_host.name])], + rem=[], ) # checking if commands were sent (commands have to be sent in order) @@ -986,13 +988,15 @@ class TestRegistrantNameservers(MockEppLib): # when you create a host, you also have to update at same time created_host1 = commands.CreateHost(self.nameserver1) - update_domain_with_created1 = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host1.name])] - ) - created_host2 = commands.CreateHost(self.nameserver2) - update_domain_with_created2 = commands.UpdateDomain( - name=self.domain.name, add=[common.HostObjSet([created_host2.name])] + + update_domain_with_created = commands.UpdateDomain( + name=self.domain.name, + add=[ + common.HostObjSet([created_host1.name]), + common.HostObjSet([created_host2.name]), + ], + rem=[], ) infoDomain = commands.InfoDomain(name="my-nameserver.gov", auth_info=None) @@ -1000,13 +1004,12 @@ class TestRegistrantNameservers(MockEppLib): expectedCalls = [ call(infoDomain, cleaned=True), call(created_host1, cleaned=True), - call(update_domain_with_created1, cleaned=True), call(created_host2, cleaned=True), - call(update_domain_with_created2, cleaned=True), + call(update_domain_with_created, cleaned=True), ] self.mockedSendFunction.assert_has_calls(expectedCalls, any_order=True) - self.assertEqual(5, self.mockedSendFunction.call_count) + self.assertEqual(4, self.mockedSendFunction.call_count) # check that status is READY self.assertTrue(self.domain.is_active()) @@ -1116,24 +1119,15 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[], - rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), call( commands.UpdateDomain( name=self.domainWithThreeNS.name, add=[], - rem=[common.HostObjSet(hosts=["ns1.cats-are-superior3.com"])], + rem=[ + common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + ], nsset=None, keyset=None, registrant=None, @@ -1157,7 +1151,6 @@ class TestRegistrantNameservers(MockEppLib): And `commands.UpdateDomain` is sent to add #4 and #5 plus remove #2 and #3 And `commands.DeleteHost` is sent to delete #2 and #3 """ - print("in second failing test") self.domainWithThreeNS.nameservers = [ (self.nameserver1,), ("ns1.cats-are-superior1.com",), @@ -1172,35 +1165,11 @@ class TestRegistrantNameservers(MockEppLib): call(commands.InfoHost(name="ns1.my-nameserver-1.com"), cleaned=True), call(commands.InfoHost(name="ns1.my-nameserver-2.com"), cleaned=True), call(commands.InfoHost(name="ns1.cats-are-superior3.com"), cleaned=True), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[], - rem=[common.HostObjSet(hosts=["ns1.my-nameserver-2.com"])], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call(commands.DeleteHost(name="ns1.my-nameserver-2.com"), cleaned=True), call( commands.CreateHost(name="ns1.cats-are-superior1.com", addrs=[]), cleaned=True, ), - call( - commands.UpdateDomain( - name=self.domainWithThreeNS.name, - add=[common.HostObjSet(hosts=["ns1.cats-are-superior1.com"])], - rem=[], - nsset=None, - keyset=None, - registrant=None, - auth_info=None, - ), - cleaned=True, - ), call( commands.CreateHost(name="ns1.cats-are-superior2.com", addrs=[]), cleaned=True, @@ -1208,8 +1177,14 @@ class TestRegistrantNameservers(MockEppLib): call( commands.UpdateDomain( name=self.domainWithThreeNS.name, - add=[common.HostObjSet(hosts=["ns1.cats-are-superior2.com"])], - rem=[], + add=[ + common.HostObjSet(hosts=["ns1.cats-are-superior1.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior2.com"]), + ], + rem=[ + common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), + common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + ], nsset=None, keyset=None, registrant=None, @@ -1395,7 +1370,6 @@ class TestRegistrantNameservers(MockEppLib): domain.nameservers = [("ns1.failednameserver.gov", ["4.5.6"])] def tearDown(self): - print("in tear down") Domain.objects.all().delete() return super().tearDown() diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 5eb92750e..39d24caee 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -35,6 +35,7 @@ class NameserverErrorCodes(IntEnum): GLUE_RECORD_NOT_ALLOWED = 2 INVALID_IP = 3 TOO_MANY_HOSTS = 4 + UNABLE_TO_UPDATE_DOMAIN = 5 class NameserverError(Exception): @@ -52,6 +53,10 @@ class NameserverError(Exception): NameserverErrorCodes.TOO_MANY_HOSTS: ( "Too many hosts provided, you may not have more than " "13 nameservers." ), + NameserverErrorCodes.UNABLE_TO_UPDATE_DOMAIN: ( + "Unable to update domain, changes were not applied." + "Check logs as a Registry Error is the likely cause" + ), } def __init__(self, *args, code=None, nameserver=None, ip=None, **kwargs): From 4050803baaeda983f488bb38329ee2f95bf2df6d Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 10 Oct 2023 17:01:24 -0700 Subject: [PATCH 44/56] Fix spacing issue --- src/registrar/utility/errors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 39d24caee..f7bc743d6 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -51,7 +51,7 @@ class NameserverError(Exception): "because it is not a subdomain", NameserverErrorCodes.INVALID_IP: "Nameserver {} has an invalid IP address: {}", NameserverErrorCodes.TOO_MANY_HOSTS: ( - "Too many hosts provided, you may not have more than " "13 nameservers." + "Too many hosts provided, you may not have more than 13 nameservers." ), NameserverErrorCodes.UNABLE_TO_UPDATE_DOMAIN: ( "Unable to update domain, changes were not applied." From b2cfc4e8bce4787f686389532546815edbd1e283 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Tue, 10 Oct 2023 17:33:41 -0700 Subject: [PATCH 45/56] fixed reference before assignment error --- src/registrar/models/domain.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 439f705f3..d6151fea6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1489,20 +1489,21 @@ class Domain(TimeStampedModel, DomainHelper): updateReq = commands.UpdateDomain( name=self.name, rem=hostsToDelete, add=hostsToAdd ) - response = registry.send(updateReq, cleaned=True) logger.info( "addAndRemoveHostsFromDomain()-> sending update domain req as %s" % updateReq ) + response = registry.send(updateReq, cleaned=True) + return response.code except RegistryError as e: logger.error( "Error addAndRemoveHostsFromDomain, code was %s error was %s" % (e.code, e) ) - - return response.code + return e.code + def _delete_hosts_if_not_used(self, hostsToDelete: list[str]): """delete the host object in registry, From 36c9cbdaa36a41b939d10d62cf1ce9406932635c Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Tue, 10 Oct 2023 17:59:53 -0700 Subject: [PATCH 46/56] Fix linter spacing --- src/registrar/models/domain.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index d6151fea6..cb62f335a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1503,7 +1503,6 @@ class Domain(TimeStampedModel, DomainHelper): % (e.code, e) ) return e.code - def _delete_hosts_if_not_used(self, hostsToDelete: list[str]): """delete the host object in registry, From c8c624e985db32fb40a49531990d5db992486bc0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 11 Oct 2023 09:04:46 -0600 Subject: [PATCH 47/56] Fix gaby email --- 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 6b6e191d8..dfe51785b 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -39,6 +39,7 @@ class UserFixture: "username": "70488e0a-e937-4894-a28c-16f5949effd4", "first_name": "Gaby", "last_name": "DiSarli", + "email": "gaby@truss.works", }, { "username": "83c2b6dd-20a2-4cac-bb40-e22a72d2955c", @@ -129,7 +130,7 @@ class UserFixture: "username": "0eb6f326-a3d4-410f-a521-aa4c1fad4e47", "first_name": "Gaby-Analyst", "last_name": "DiSarli-Analyst", - "email": "gaby@truss.works", + "email": "gaby+1@truss.works", }, { "username": "cfe7c2fc-e24a-480e-8b78-28645a1459b3", From c66813100eb673453e4b255472e20f64d68e593b Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 11 Oct 2023 08:12:53 -0700 Subject: [PATCH 48/56] changed host object set to contain the list off add hosts instead of using multiple objects --- src/registrar/models/domain.py | 46 ++++++++++++++--------- src/registrar/tests/test_models_domain.py | 27 +++++++++---- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb62f335a..9b51e48d6 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -252,8 +252,7 @@ class Domain(TimeStampedModel, DomainHelper): return hostList def _create_host(self, host, addrs): - """Call _check_host first before using this function, - This creates the host object in the registry + """Creates the host object in the registry doesn't add the created host to the domain returns ErrorCode (int)""" logger.info("Creating host") @@ -380,7 +379,7 @@ class Domain(TimeStampedModel, DomainHelper): new_values = { key: newHostDict.get(key) for key in newHostDict - if key not in previousHostDict + if key not in previousHostDict and key.strip() != "" } for nameserver, ip in new_values.items(): @@ -402,15 +401,19 @@ class Domain(TimeStampedModel, DomainHelper): % (hostTuple[0], updated_response_code) ) - def createNewHostList(self, new_values: dict) -> list: + def createNewHostList(self, new_values: dict): """convert the dictionary of new values to a list of HostObjSet for use in the UpdateDomain epp message Args: new_values: dict(str,list)- dict of {nameserver:ips} to add to domain Returns: - list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + tuple [list[epp.HostObjSet], int] + list[epp.HostObjSet]-epp object for use in the UpdateDomain epp message + defaults to empty list + int-number of items being created default 0 """ - addToDomainList = [] + + hostStringList = [] for key, value in new_values.items(): createdCode = self._create_host( host=key, addrs=value @@ -419,21 +422,31 @@ class Domain(TimeStampedModel, DomainHelper): createdCode == ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY or createdCode == ErrorCode.OBJECT_EXISTS ): - addToDomainList.append(epp.HostObjSet([key])) + hostStringList.append(key) + if hostStringList == []: + return [], 0 - return addToDomainList + addToDomainObject = epp.HostObjSet(hosts=hostStringList) + return [addToDomainObject], len(hostStringList) def createDeleteHostList(self, hostsToDelete: list[str]): """ Args: hostsToDelete (list[str])- list of nameserver/host names to remove Returns: - list[epp.HostObjSet]-epp object list for use in the UpdateDomain epp message + tuple [list[epp.HostObjSet], int] + list[epp.HostObjSet]-epp object for use in the UpdateDomain epp message + defaults to empty list + int-number of items being created default 0 """ - deleteList = [] + deleteStrList = [] for nameserver in hostsToDelete: - deleteList.append(epp.HostObjSet([nameserver])) - return deleteList + deleteStrList.append(nameserver) + if deleteStrList == []: + return [], 0 + deleteObj = epp.HostObjSet(hosts=hostsToDelete) + + return [deleteObj], len(deleteStrList) @Cache def dnssecdata(self) -> extensions.DNSSECExtension: @@ -482,8 +495,8 @@ class Domain(TimeStampedModel, DomainHelper): _ = self._update_host_values( updated_values, oldNameservers ) # returns nothing, just need to be run and errors - addToDomainList = self.createNewHostList(new_values) - deleteHostList = self.createDeleteHostList(deleted_values) + addToDomainList, addToDomainCount = self.createNewHostList(new_values) + deleteHostList, deleteCount = self.createDeleteHostList(deleted_values) responseCode = self.addAndRemoveHostsFromDomain( hostsToAdd=addToDomainList, hostsToDelete=deleteHostList ) @@ -492,9 +505,8 @@ class Domain(TimeStampedModel, DomainHelper): if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY: raise NameserverError(code=nsErrorCodes.UNABLE_TO_UPDATE_DOMAIN) - successTotalNameservers = ( - len(oldNameservers) - len(deleteHostList) + len(addToDomainList) - ) + successTotalNameservers = len(oldNameservers) - deleteCount + addToDomainCount + self._delete_hosts_if_not_used(hostsToDelete=deleted_values) if successTotalNameservers < 2: try: diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index f018d647a..99d7a4c8d 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -993,8 +993,7 @@ class TestRegistrantNameservers(MockEppLib): update_domain_with_created = commands.UpdateDomain( name=self.domain.name, add=[ - common.HostObjSet([created_host1.name]), - common.HostObjSet([created_host2.name]), + common.HostObjSet([created_host1.name, created_host2.name]), ], rem=[], ) @@ -1125,8 +1124,12 @@ class TestRegistrantNameservers(MockEppLib): name=self.domainWithThreeNS.name, add=[], rem=[ - common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), - common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + common.HostObjSet( + hosts=[ + "ns1.my-nameserver-2.com", + "ns1.cats-are-superior3.com", + ] + ), ], nsset=None, keyset=None, @@ -1178,12 +1181,20 @@ class TestRegistrantNameservers(MockEppLib): commands.UpdateDomain( name=self.domainWithThreeNS.name, add=[ - common.HostObjSet(hosts=["ns1.cats-are-superior1.com"]), - common.HostObjSet(hosts=["ns1.cats-are-superior2.com"]), + common.HostObjSet( + hosts=[ + "ns1.cats-are-superior1.com", + "ns1.cats-are-superior2.com", + ] + ), ], rem=[ - common.HostObjSet(hosts=["ns1.my-nameserver-2.com"]), - common.HostObjSet(hosts=["ns1.cats-are-superior3.com"]), + common.HostObjSet( + hosts=[ + "ns1.my-nameserver-2.com", + "ns1.cats-are-superior3.com", + ] + ), ], nsset=None, keyset=None, From 72c238c00ef262963262f4ded834b04462858b19 Mon Sep 17 00:00:00 2001 From: Alysia Broddrick Date: Wed, 11 Oct 2023 09:13:00 -0700 Subject: [PATCH 49/56] changed comment --- src/registrar/models/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 9b51e48d6..8397ea35c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -295,7 +295,7 @@ class Domain(TimeStampedModel, DomainHelper): """Checks the parameters past for a valid combination raises error if: - nameserver is a subdomain but is missing ip - - nameserver is not a subdomain but is missing ip + - nameserver is not a subdomain but has ip - nameserver is a subdomain but an ip passed is invalid Args: From de87f0c7730634f9449cad64d1d3648fbec822d2 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 11 Oct 2023 11:59:52 -0700 Subject: [PATCH 50/56] Add domain invitation permissions to analyst --- docs/django-admin/roles.md | 5 +-- .../migrations/0038_create_groups_v02.py | 36 +++++++++++++++++++ src/registrar/models/user_group.py | 5 +++ src/registrar/tests/test_migrations.py | 4 ++- 4 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 src/registrar/migrations/0038_create_groups_v02.py diff --git a/docs/django-admin/roles.md b/docs/django-admin/roles.md index 91c2949eb..458029e07 100644 --- a/docs/django-admin/roles.md +++ b/docs/django-admin/roles.md @@ -13,7 +13,8 @@ For more details, refer to the [user group model](../../src/registrar/models/use We can edit and deploy new group permissions by: -1. editing `user_group` then: +1. Editing `user_group` then: 2. Duplicating migration `0036_create_groups_01` and running migrations (append the name with a version number -to help django detect the migration eg 0037_create_groups_02) \ No newline at end of file +to help django detect the migration eg 0037_create_groups_02) +3. Making sure to update the dependency on the new migration with the previous migration \ No newline at end of file diff --git a/src/registrar/migrations/0038_create_groups_v02.py b/src/registrar/migrations/0038_create_groups_v02.py new file mode 100644 index 000000000..80752f31a --- /dev/null +++ b/src/registrar/migrations/0038_create_groups_v02.py @@ -0,0 +1,36 @@ +# This migration creates the create_full_access_group and create_cisa_analyst_group groups +# It is dependent on 0035 (which populates ContentType and Permissions) +# If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS +# in the user_group model then: +# [NOT RECOMMENDED] +# step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions +# step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups +# step 3: fake run the latest migration in the migrations list +# [RECOMMENDED] +# Alternatively: +# Only step: duplicate the migration that loads data and run: docker-compose exec app ./manage.py migrate + +from django.db import migrations +from registrar.models import UserGroup +from typing import Any + + +# For linting: RunPython expects a function reference, +# so let's give it one +def create_groups(apps, schema_editor) -> Any: + UserGroup.create_cisa_analyst_group(apps, schema_editor) + UserGroup.create_full_access_group(apps, schema_editor) + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0037_create_groups_v01"), + ] + + operations = [ + migrations.RunPython( + create_groups, + reverse_code=migrations.RunPython.noop, + atomic=True, + ), + ] diff --git a/src/registrar/models/user_group.py b/src/registrar/models/user_group.py index b6f5b41b2..5cdb1f2ec 100644 --- a/src/registrar/models/user_group.py +++ b/src/registrar/models/user_group.py @@ -51,6 +51,11 @@ class UserGroup(Group): "model": "user", "permissions": ["analyst_access_permission", "change_user"], }, + { + "app_label": "registrar", + "model": "domaininvitation", + "permissions": ["add_domaininvitation", "view_domaininvitation"], + }, ] # Avoid error: You can't execute queries until the end diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index f98e876d7..14ab36e70 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -31,7 +31,7 @@ class TestGroups(TestCase): UserGroup.objects.filter(name="full_access_group"), [full_access_group] ) - # Test permissions for cisa_analysts_group + # Test permissions data migrations for cisa_analysts_group ran as expected # Define the expected permission codenames expected_permissions = [ "view_logentry", @@ -42,6 +42,8 @@ class TestGroups(TestCase): "change_draftdomain", "analyst_access_permission", "change_user", + "add_domaininvitation", + "view_domaininvitation" ] # Get the codenames of actual permissions associated with the group From 67dacffa9f3bdf51dd51c040da944d5f53ec90b9 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:01:06 -0700 Subject: [PATCH 51/56] Update 0037_create_groups_v01 migration documentation --- src/registrar/migrations/0037_create_groups_v01.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/migrations/0037_create_groups_v01.py b/src/registrar/migrations/0037_create_groups_v01.py index 27a14f8b9..69a4c02aa 100644 --- a/src/registrar/migrations/0037_create_groups_v01.py +++ b/src/registrar/migrations/0037_create_groups_v01.py @@ -2,11 +2,13 @@ # It is dependent on 0035 (which populates ContentType and Permissions) # If permissions on the groups need changing, edit CISA_ANALYST_GROUP_PERMISSIONS # in the user_group model then: +# [NOT RECOMMENDED] # step 1: docker-compose exec app ./manage.py migrate --fake registrar 0035_contenttypes_permissions # step 2: docker-compose exec app ./manage.py migrate registrar 0036_create_groups # step 3: fake run the latest migration in the migrations list +# [RECOMMENDED] # Alternatively: -# Only step: duplicate the migtation that loads data and run: docker-compose exec app ./manage.py migrate +# Only step: duplicate the migration that loads data and run: docker-compose exec app ./manage.py migrate from django.db import migrations from registrar.models import UserGroup From 816e678bf370188204d390b2c5edb6c856f1d5a1 Mon Sep 17 00:00:00 2001 From: Rebecca Hsieh Date: Thu, 12 Oct 2023 09:26:51 -0700 Subject: [PATCH 52/56] Add in regex for subdomain checking --- src/registrar/models/domain.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index e75692bf7..7aaeebe21 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1,6 +1,7 @@ from itertools import zip_longest import logging import ipaddress +import re from datetime import date from string import digits from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore @@ -292,7 +293,10 @@ class Domain(TimeStampedModel, DomainHelper): def isSubdomain(self, nameserver: str): """Returns boolean if the domain name is found in the argument passed""" - return self.name in nameserver + subdomain_pattern = r"([\w-]+\.)*" + full_pattern = subdomain_pattern + self.name + regex = re.compile(full_pattern) + return bool(regex.match(nameserver)) def checkHostIPCombo(self, nameserver: str, ip: list[str]): """Checks the parameters past for a valid combination From 81c37b15ca4ba384ccdb4e97269d0d14e7d442ee Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 12 Oct 2023 10:39:05 -0600 Subject: [PATCH 53/56] Update domain.py --- src/registrar/views/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index 442057d02..d8c3c80fa 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -381,7 +381,7 @@ class DomainAddUserView(DomainPermissionView, FormMixin): def form_valid(self, form): """Add the specified user on this domain.""" - requested_email = form.cleaned_data.get("email", "") + requested_email = form.cleaned_data["email"] # look up a user with that email try: requested_user = User.objects.get(email=requested_email) From ecc4d6c89eaaeb7a87ba1d87c66e6369f034effb Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Thu, 12 Oct 2023 09:56:18 -0700 Subject: [PATCH 54/56] Run linter and formatting changes --- src/registrar/admin.py | 6 ++++++ src/registrar/migrations/0037_create_groups_v01.py | 3 ++- src/registrar/migrations/0038_create_groups_v02.py | 3 ++- src/registrar/tests/test_migrations.py | 5 +++-- 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 174500f28..8d0ed8c2e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -342,6 +342,12 @@ class DomainInvitationAdmin(ListHeaderAdmin): ] search_help_text = "Search by email or domain." + # Mark the FSM field 'status' as readonly + # to allow admin users to create Domain Invitations + # without triggering the FSM Transition Not Allowed + # error. + readonly_fields = ["status"] + class DomainInformationAdmin(ListHeaderAdmin): """Customize domain information admin class.""" diff --git a/src/registrar/migrations/0037_create_groups_v01.py b/src/registrar/migrations/0037_create_groups_v01.py index 69a4c02aa..3540ea2f3 100644 --- a/src/registrar/migrations/0037_create_groups_v01.py +++ b/src/registrar/migrations/0037_create_groups_v01.py @@ -8,7 +8,8 @@ # step 3: fake run the latest migration in the migrations list # [RECOMMENDED] # Alternatively: -# Only step: duplicate the migration that loads data and run: docker-compose exec app ./manage.py migrate +# step 1: duplicate the migration that loads data +# step 2: docker-compose exec app ./manage.py migrate from django.db import migrations from registrar.models import UserGroup diff --git a/src/registrar/migrations/0038_create_groups_v02.py b/src/registrar/migrations/0038_create_groups_v02.py index 80752f31a..fc61db3c0 100644 --- a/src/registrar/migrations/0038_create_groups_v02.py +++ b/src/registrar/migrations/0038_create_groups_v02.py @@ -8,7 +8,8 @@ # step 3: fake run the latest migration in the migrations list # [RECOMMENDED] # Alternatively: -# Only step: duplicate the migration that loads data and run: docker-compose exec app ./manage.py migrate +# step 1: duplicate the migration that loads data +# step 2: docker-compose exec app ./manage.py migrate from django.db import migrations from registrar.models import UserGroup diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index 14ab36e70..2ea1962a1 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -31,7 +31,8 @@ class TestGroups(TestCase): UserGroup.objects.filter(name="full_access_group"), [full_access_group] ) - # Test permissions data migrations for cisa_analysts_group ran as expected + # Test data migrations for cisa_analysts_group permission + # ran as expected. # Define the expected permission codenames expected_permissions = [ "view_logentry", @@ -43,7 +44,7 @@ class TestGroups(TestCase): "analyst_access_permission", "change_user", "add_domaininvitation", - "view_domaininvitation" + "view_domaininvitation", ] # Get the codenames of actual permissions associated with the group From bd1b5654ef0d6a8d328a338c46b480184198b358 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Thu, 12 Oct 2023 10:25:54 -0700 Subject: [PATCH 55/56] Modify test case to reflect order of expected permissions --- src/registrar/tests/test_migrations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index 2ea1962a1..0b71f3bf6 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -40,11 +40,11 @@ class TestGroups(TestCase): "view_domain", "change_domainapplication", "change_domaininformation", + "add_domaininvitation", + "view_domaininvitation", "change_draftdomain", "analyst_access_permission", "change_user", - "add_domaininvitation", - "view_domaininvitation", ] # Get the codenames of actual permissions associated with the group From 02c929ddd391b357a5fbb608e5ae087b68d5ecc4 Mon Sep 17 00:00:00 2001 From: Erin <121973038+erinysong@users.noreply.github.com> Date: Thu, 12 Oct 2023 10:34:08 -0700 Subject: [PATCH 56/56] Implement format feedback --- src/registrar/tests/test_migrations.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_migrations.py b/src/registrar/tests/test_migrations.py index 0b71f3bf6..95e5853ff 100644 --- a/src/registrar/tests/test_migrations.py +++ b/src/registrar/tests/test_migrations.py @@ -31,8 +31,8 @@ class TestGroups(TestCase): UserGroup.objects.filter(name="full_access_group"), [full_access_group] ) - # Test data migrations for cisa_analysts_group permission - # ran as expected. + # Test permissions for cisa_analysts_group + # Verifies permission data migrations ran as expected. # Define the expected permission codenames expected_permissions = [ "view_logentry",