From d6e0a7b979b1aac79d77a0a80fa32efe664dd6d0 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Mon, 12 Jan 2026 17:12:08 -0500 Subject: [PATCH] Change domain update commands to be varipotent by status (#2930) This means that attempting to add a status that is already present will now fail, and attempting to remove a status that is not present will also now fail. This also refactors the existing checks into a single verify method, rather than having to call three separate methods from every callsite. BUG= http://b/474645068 --- .../registry/flows/ResourceFlowUtils.java | 27 ++-- .../flows/domain/DomainFlowUtils.java | 16 +-- .../flows/domain/DomainUpdateFlow.java | 23 ++-- .../registry/flows/host/HostUpdateFlow.java | 12 +- .../model/domain/secdns/DomainDsDataBase.java | 2 +- .../domain/secdns/SecDnsUpdateExtension.java | 13 +- .../flows/domain/DomainUpdateFlowTest.java | 130 +++++++++++------- .../registry/flows/domain/domain_update.xml | 1 - .../domain/domain_update_empty_registrant.xml | 1 - .../domain/domain_update_no_auth_change.xml | 1 - .../flows/domain/domain_update_no_cltrid.xml | 1 - .../domain_update_prohibited_status.xml | 1 - ...update_remove_client_update_prohibited.xml | 16 +++ .../domain/domain_update_remove_contact.xml | 1 - .../flows/domain_update_dsdata_rem.xml | 2 +- 15 files changed, 144 insertions(+), 103 deletions(-) create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_update_remove_client_update_prohibited.xml diff --git a/core/src/main/java/google/registry/flows/ResourceFlowUtils.java b/core/src/main/java/google/registry/flows/ResourceFlowUtils.java index e27127a5d..9ceb7150c 100644 --- a/core/src/main/java/google/registry/flows/ResourceFlowUtils.java +++ b/core/src/main/java/google/registry/flows/ResourceFlowUtils.java @@ -194,27 +194,24 @@ public final class ResourceFlowUtils { } } - /** Check that the same values aren't being added and removed in an update command. */ - public static void checkSameValuesNotAddedAndRemoved( - ImmutableSet fieldsToAdd, ImmutableSet fieldsToRemove) - throws AddRemoveSameValueException { + /** + * Verifies the adds and removes on a resource. + * + *

This throws an exception in three different situations: if the same value is being both + * added and removed, if a value is being added that is already present, or if a value is being + * removed that isn't present. + */ + public static void verifyAddsAndRemoves( + ImmutableSet existingFields, ImmutableSet fieldsToAdd, ImmutableSet fieldsToRemove) + throws AddRemoveSameValueException, + AddExistingValueException, + RemoveNonexistentValueException { if (!intersection(fieldsToAdd, fieldsToRemove).isEmpty()) { throw new AddRemoveSameValueException(); } - } - - /** Check that we aren't adding a value that is already present. */ - public static void checkExistingValueNotAdded( - ImmutableSet fieldsToAdd, ImmutableSet existingFields) - throws AddExistingValueException { if (!intersection(fieldsToAdd, existingFields).isEmpty()) { throw new AddExistingValueException(); } - } - - public static void checkNonexistentValueNotRemoved( - ImmutableSet fieldsToRemove, ImmutableSet existingFields) - throws RemoveNonexistentValueException { if (intersection(fieldsToRemove, existingFields).size() != fieldsToRemove.size()) { throw new RemoveNonexistentValueException(); } diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java index c945cdb4f..08da55a75 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -976,23 +976,21 @@ public class DomainFlowUtils { throw new UrgentAttributeNotSupportedException(); } // There must be at least one of add/rem/chg, and chg isn't actually supported. - if (secDnsUpdate.getChange() != null) { + if (secDnsUpdate.getChange().isPresent()) { // The only thing you can change is maxSigLife, and we don't support that at all. throw new MaxSigLifeChangeNotSupportedException(); } - Add add = secDnsUpdate.getAdd(); - Remove remove = secDnsUpdate.getRemove(); - if (add == null && remove == null) { + Optional add = secDnsUpdate.getAdd(); + Optional remove = secDnsUpdate.getRemove(); + if (add.isEmpty() && remove.isEmpty()) { throw new EmptySecDnsUpdateException(); } - if (remove != null && Boolean.FALSE.equals(remove.getAll())) { + if (remove.isPresent() && Boolean.FALSE.equals(remove.get().getAll())) { throw new SecDnsAllUsageException(); // Explicit all=false is meaningless. } - Set toAdd = (add == null) ? ImmutableSet.of() : add.getDsData(); + Set toAdd = add.map(Add::getDsData).orElse(ImmutableSet.of()); Set toRemove = - (remove == null) - ? ImmutableSet.of() - : (remove.getAll() == null) ? remove.getDsData() : oldDsData; + remove.map(r -> (r.getAll() == null) ? r.getDsData() : oldDsData).orElse(ImmutableSet.of()); // RFC 5910 specifies that removes are processed before adds. return ImmutableSet.copyOf(union(difference(oldDsData, toRemove), toAdd)); } diff --git a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java index 886d6aba7..fc1f494d6 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java @@ -21,10 +21,8 @@ import static com.google.common.collect.Sets.union; import static google.registry.dns.DnsUtils.requestDomainDnsRefresh; import static google.registry.flows.FlowUtils.persistEntityChanges; import static google.registry.flows.FlowUtils.validateRegistrarIsLoggedIn; -import static google.registry.flows.ResourceFlowUtils.checkExistingValueNotAdded; -import static google.registry.flows.ResourceFlowUtils.checkNonexistentValueNotRemoved; -import static google.registry.flows.ResourceFlowUtils.checkSameValuesNotAddedAndRemoved; import static google.registry.flows.ResourceFlowUtils.loadAndVerifyExistence; +import static google.registry.flows.ResourceFlowUtils.verifyAddsAndRemoves; import static google.registry.flows.ResourceFlowUtils.verifyAllStatusesAreClientSettable; import static google.registry.flows.ResourceFlowUtils.verifyNoDisallowedStatuses; import static google.registry.flows.ResourceFlowUtils.verifyOptionalAuthInfo; @@ -77,6 +75,8 @@ import google.registry.model.domain.fee.FeeUpdateCommandExtension; import google.registry.model.domain.metadata.MetadataExtension; import google.registry.model.domain.secdns.DomainDsData; import google.registry.model.domain.secdns.SecDnsUpdateExtension; +import google.registry.model.domain.secdns.SecDnsUpdateExtension.Add; +import google.registry.model.domain.secdns.SecDnsUpdateExtension.Remove; import google.registry.model.domain.superuser.DomainUpdateSuperuserExtension; import google.registry.model.eppcommon.AuthInfo; import google.registry.model.eppcommon.StatusValue; @@ -247,14 +247,19 @@ public final class DomainUpdateFlow implements MutatingFlow { private Domain performUpdate(Update command, Domain domain, DateTime now) throws EppException { AddRemove add = command.getInnerAdd(); AddRemove remove = command.getInnerRemove(); - checkSameValuesNotAddedAndRemoved(add.getNameservers(), remove.getNameservers()); - checkSameValuesNotAddedAndRemoved(add.getContacts(), remove.getContacts()); - checkSameValuesNotAddedAndRemoved(add.getStatusValues(), remove.getStatusValues()); - checkExistingValueNotAdded(add.getNameservers(), domain.getNameservers()); - checkNonexistentValueNotRemoved(remove.getNameservers(), domain.getNameservers()); - Change change = command.getInnerChange(); Optional secDnsUpdate = eppInput.getSingleExtension(SecDnsUpdateExtension.class); + verifyAddsAndRemoves(domain.getNameservers(), add.getNameservers(), remove.getNameservers()); + verifyAddsAndRemoves(domain.getContacts(), add.getContacts(), remove.getContacts()); + verifyAddsAndRemoves(domain.getStatusValues(), add.getStatusValues(), remove.getStatusValues()); + if (secDnsUpdate.isPresent()) { + SecDnsUpdateExtension ext = secDnsUpdate.get(); + verifyAddsAndRemoves( + domain.getDsData(), + ext.getAdd().map(Add::getDsData).orElse(ImmutableSet.of()), + ext.getRemove().map(Remove::getDsData).orElse(ImmutableSet.of())); + } + Change change = command.getInnerChange(); // We have to verify no duplicate contacts _before_ constructing the domain because it is // illegal to construct a domain with duplicate contacts. diff --git a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java index a0d2c79dd..7d537403e 100644 --- a/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/host/HostUpdateFlow.java @@ -20,10 +20,8 @@ import static google.registry.dns.DnsUtils.requestHostDnsRefresh; import static google.registry.dns.RefreshDnsOnHostRenameAction.PARAM_HOST_KEY; import static google.registry.dns.RefreshDnsOnHostRenameAction.QUEUE_HOST_RENAME; import static google.registry.flows.FlowUtils.validateRegistrarIsLoggedIn; -import static google.registry.flows.ResourceFlowUtils.checkExistingValueNotAdded; -import static google.registry.flows.ResourceFlowUtils.checkNonexistentValueNotRemoved; -import static google.registry.flows.ResourceFlowUtils.checkSameValuesNotAddedAndRemoved; import static google.registry.flows.ResourceFlowUtils.loadAndVerifyExistence; +import static google.registry.flows.ResourceFlowUtils.verifyAddsAndRemoves; import static google.registry.flows.ResourceFlowUtils.verifyAllStatusesAreClientSettable; import static google.registry.flows.ResourceFlowUtils.verifyNoDisallowedStatuses; import static google.registry.flows.ResourceFlowUtils.verifyResourceOwnership; @@ -161,10 +159,10 @@ public final class HostUpdateFlow implements MutatingFlow { } AddRemove add = command.getInnerAdd(); AddRemove remove = command.getInnerRemove(); - checkSameValuesNotAddedAndRemoved(add.getStatusValues(), remove.getStatusValues()); - checkSameValuesNotAddedAndRemoved(add.getInetAddresses(), remove.getInetAddresses()); - checkExistingValueNotAdded(add.getInetAddresses(), existingHost.getInetAddresses()); - checkNonexistentValueNotRemoved(remove.getInetAddresses(), existingHost.getInetAddresses()); + verifyAddsAndRemoves( + existingHost.getStatusValues(), add.getStatusValues(), remove.getStatusValues()); + verifyAddsAndRemoves( + existingHost.getInetAddresses(), add.getInetAddresses(), remove.getInetAddresses()); HostFlowUtils.validateInetAddresses(add.getInetAddresses()); VKey newSuperordinateDomainKey = newSuperordinateDomain.map(Domain::createVKey).orElse(null); diff --git a/core/src/main/java/google/registry/model/domain/secdns/DomainDsDataBase.java b/core/src/main/java/google/registry/model/domain/secdns/DomainDsDataBase.java index 4e3bd85c6..82195b819 100644 --- a/core/src/main/java/google/registry/model/domain/secdns/DomainDsDataBase.java +++ b/core/src/main/java/google/registry/model/domain/secdns/DomainDsDataBase.java @@ -31,7 +31,7 @@ import jakarta.xml.bind.annotation.adapters.XmlJavaTypeAdapter; @Access(AccessType.FIELD) public abstract class DomainDsDataBase extends ImmutableObject implements UnsafeSerializable { - @XmlTransient @Transient String domainRepoId; + @XmlTransient @Transient @Insignificant String domainRepoId; /** The identifier for this particular key in the domain. */ @Transient int keyTag; diff --git a/core/src/main/java/google/registry/model/domain/secdns/SecDnsUpdateExtension.java b/core/src/main/java/google/registry/model/domain/secdns/SecDnsUpdateExtension.java index a3c17586b..a9032c277 100644 --- a/core/src/main/java/google/registry/model/domain/secdns/SecDnsUpdateExtension.java +++ b/core/src/main/java/google/registry/model/domain/secdns/SecDnsUpdateExtension.java @@ -24,6 +24,7 @@ import jakarta.xml.bind.annotation.XmlElement; import jakarta.xml.bind.annotation.XmlRootElement; import jakarta.xml.bind.annotation.XmlTransient; import jakarta.xml.bind.annotation.XmlType; +import java.util.Optional; import java.util.Set; /** The EPP secDNS extension that may be present on domain update commands. */ @@ -55,16 +56,16 @@ public class SecDnsUpdateExtension extends ImmutableObject implements CommandExt return urgent; } - public Remove getRemove() { - return remove; + public Optional getRemove() { + return Optional.ofNullable(remove); } - public Add getAdd() { - return add; + public Optional getAdd() { + return Optional.ofNullable(add); } - public Change getChange() { - return change; + public Optional getChange() { + return Optional.ofNullable(change); } @XmlTransient diff --git a/core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java index e60ef8da8..2799a0462 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java @@ -63,8 +63,6 @@ import google.registry.config.RegistryConfig; import google.registry.flows.EppException; import google.registry.flows.EppException.UnimplementedExtensionException; import google.registry.flows.EppRequestSource; -import google.registry.flows.FlowTestCase.CommitMode; -import google.registry.flows.FlowTestCase.UserPrivileges; import google.registry.flows.FlowUtils.NotLoggedInException; import google.registry.flows.ResourceFlowTestCase; import google.registry.flows.ResourceFlowUtils.AddExistingValueException; @@ -454,6 +452,7 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase + doSecDnsSuccessfulTest( + "domain_update_dsdata_add.xml", + ImmutableSet.of(SOME_DSDATA), + ImmutableSet.of(SOME_DSDATA), + ImmutableMap.of( + "KEY_TAG", + "1", + "ALG", + "2", + "DIGEST_TYPE", + "2", + "DIGEST", + "9F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08"), + false)); + assertAboutEppExceptions().that(thrown).marshalsToXml(); } @Test @@ -681,7 +685,7 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase + doSecDnsSuccessfulTest( + "domain_update_dsdata_add_rem_same.xml", + ImmutableSet.of( + SOME_DSDATA, + DomainDsData.create( + 12345, + 3, + 1, + base16().decode("A94A8FE5CCB19BA61C4C0873D391E987982FBBD3"))), + ImmutableSet.of( + SOME_DSDATA, + DomainDsData.create( + 12345, + 3, + 1, + base16().decode("A94A8FE5CCB19BA61C4C0873D391E987982FBBD3"))), + false)); + assertAboutEppExceptions().that(thrown).marshalsToXml(); } @Test - void testSuccess_secDnsRemoveAlreadyNotThere() throws Exception { - // Removing a dsData that isn't there is a no-op. - doSecDnsSuccessfulTest( - "domain_update_dsdata_rem.xml", - ImmutableSet.of(SOME_DSDATA), - ImmutableSet.of(SOME_DSDATA), - false); + void testFailure_secDnsRemoveAlreadyNotThere_throwsException() throws Exception { + EppException thrown = + assertThrows( + RemoveNonexistentValueException.class, + () -> + doSecDnsSuccessfulTest( + "domain_update_dsdata_rem.xml", + ImmutableSet.of(SOME_DSDATA), + ImmutableSet.of(SOME_DSDATA), + false)); + assertAboutEppExceptions().that(thrown).marshalsToXml(); } void doServerStatusBillingTest(String xmlFilename, boolean isBillable) throws Exception { @@ -856,14 +874,6 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase ns1.example.foo - diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_empty_registrant.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_empty_registrant.xml index 7ab6e5b4e..61713fb8d 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_update_empty_registrant.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_empty_registrant.xml @@ -17,7 +17,6 @@ ns1.example.foo sh8013 - diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_no_auth_change.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_no_auth_change.xml index 8d3d7a624..46a56f59d 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_update_no_auth_change.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_no_auth_change.xml @@ -15,7 +15,6 @@ ns1.example.foo - diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_no_cltrid.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_no_cltrid.xml index 721fcfe43..3c75d4013 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_update_no_cltrid.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_no_cltrid.xml @@ -15,7 +15,6 @@ ns1.example.foo - diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_prohibited_status.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_prohibited_status.xml index d8100d1ac..0ed42fda3 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_update_prohibited_status.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_prohibited_status.xml @@ -15,7 +15,6 @@ ns1.example.foo - diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_remove_client_update_prohibited.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_remove_client_update_prohibited.xml new file mode 100644 index 000000000..12a725b8e --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_remove_client_update_prohibited.xml @@ -0,0 +1,16 @@ + + + + + example.tld + + + + + + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_remove_contact.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_remove_contact.xml index 9b012cf22..9ee15eccf 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_update_remove_contact.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_remove_contact.xml @@ -15,7 +15,6 @@ ns1.example.foo - diff --git a/core/src/test/resources/google/registry/flows/domain_update_dsdata_rem.xml b/core/src/test/resources/google/registry/flows/domain_update_dsdata_rem.xml index 605e627eb..47a6c4c10 100644 --- a/core/src/test/resources/google/registry/flows/domain_update_dsdata_rem.xml +++ b/core/src/test/resources/google/registry/flows/domain_update_dsdata_rem.xml @@ -16,7 +16,7 @@ 12346 3 1 - 38EC35D5B3A34B44C39B + A94A8FE5CCB19BA61C4C0873D391E987982FBBD3