mirrors/nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus synced 2026-04-23 01:30:51 +00:00
Code Issues Releases Wiki Activity

Change domain update commands to be varipotent by status (#2930)

Browse Source 
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
This commit is contained in:
Ben McIlwain
2026-01-12 17:12:08 -05:00
committed by GitHub
parent 5725eb95e0
commit d6e0a7b979
15 changed files with 144 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
core/src/main/java/google/registry/flows/ResourceFlowUtils.java
View File

@@ -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 <T> void checkSameValuesNotAddedAndRemoved(
ImmutableSet<T> fieldsToAdd, ImmutableSet<T> fieldsToRemove)
throws AddRemoveSameValueException {
/**
* Verifies the adds and removes on a resource.
*
* <p>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 <T> void verifyAddsAndRemoves(
ImmutableSet<T> existingFields, ImmutableSet<T> fieldsToAdd, ImmutableSet<T> 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 <T> void checkExistingValueNotAdded(
ImmutableSet<T> fieldsToAdd, ImmutableSet<T> existingFields)
throws AddExistingValueException {
if (!intersection(fieldsToAdd, existingFields).isEmpty()) {
throw new AddExistingValueException();
}
}
public static <T> void checkNonexistentValueNotRemoved(
ImmutableSet<T> fieldsToRemove, ImmutableSet<T> existingFields)
throws RemoveNonexistentValueException {
if (intersection(fieldsToRemove, existingFields).size() != fieldsToRemove.size()) {
throw new RemoveNonexistentValueException();
}

16
core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java
View File

@@ -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> add = secDnsUpdate.getAdd();
Optional<Remove> 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<DomainDsData> toAdd = (add == null) ? ImmutableSet.of() : add.getDsData();
Set<DomainDsData> toAdd = add.map(Add::getDsData).orElse(ImmutableSet.of());
Set<DomainDsData> 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));
}

23
core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java
View File

@@ -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<SecDnsUpdateExtension> 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.

12
core/src/main/java/google/registry/flows/host/HostUpdateFlow.java
View File

@@ -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<Domain> newSuperordinateDomainKey =
newSuperordinateDomain.map(Domain::createVKey).orElse(null);

2
core/src/main/java/google/registry/model/domain/secdns/DomainDsDataBase.java
View File

@@ -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;

13
core/src/main/java/google/registry/model/domain/secdns/SecDnsUpdateExtension.java
View File

@@ -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<Remove> getRemove() {
return Optional.ofNullable(remove);
}
public Add getAdd() {
return add;
public Optional<Add> getAdd() {
return Optional.ofNullable(add);
}
public Change getChange() {
return change;
public Optional<Change> getChange() {
return Optional.ofNullable(change);
}
@XmlTransient

130
core/src/test/java/google/registry/flows/domain/DomainUpdateFlowTest.java
View File

@@ -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<DomainUpdateFlow, Domain
@Test
void testSuccess_removeClientUpdateProhibited() throws Exception {
setEppInput("domain_update_remove_client_update_prohibited.xml");
persistReferencedEntities();
persistResource(
persistDomain()
@@ -549,21 +548,26 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
}
@Test
void testSuccess_secDnsAddSameDoesNothing() throws Exception {
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);
void testFailure_secDnsAddSame_throwsException() throws Exception {
EppException thrown =
assertThrows(
AddExistingValueException.class,
() ->
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<DomainUpdateFlow, Domain
2,
2,
base16()
.decode("9F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08"))),
.decode("0F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08"))),
ImmutableMap.of(
"KEY_TAG",
"1",
@@ -690,8 +694,8 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
"DIGEST_TYPE",
"2",
"DIGEST",
"9F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08"),
false);
"0F86D081884C7D659A2FEAA0C55AD015A3BF4F1B2B0B822CD15D6C15B0F00A08"),
true);
}
@Test
@@ -799,29 +803,43 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
}
@Test
void testSuccess_secDnsAddRemoveSame() throws Exception {
// Adding and removing the same dsData is a no-op because removes are processed first.
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);
void testFailure_secDnsAddRemoveSame_throwsException() throws Exception {
EppException thrown =
assertThrows(
AddRemoveSameValueException.class,
() ->
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<DomainUpdateFlow, Domain
doServerStatusBillingTest("domain_update_add_server_status.xml", true);
}
@Test
void testSuccess_noBillingOnPreExistingServerStatus() throws Exception {
eppRequestSource = EppRequestSource.TOOL;
Domain addStatusDomain = persistActiveDomain(getUniqueIdFromCommand());
persistResource(addStatusDomain.asBuilder().addStatusValue(SERVER_RENEW_PROHIBITED).build());
doServerStatusBillingTest("domain_update_add_server_status.xml", false);
}
@Test
void testSuccess_removeServerStatusBillingEvent() throws Exception {
eppRequestSource = EppRequestSource.TOOL;
@@ -1390,7 +1400,20 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
}
@Test
void testFailure_removeNonexistentValue() throws Exception {
void testFailure_addExistingStatusValue() throws Exception {
persistResource(
DatabaseHelper.newDomain(getUniqueIdFromCommand())
.asBuilder()
.setStatusValues(ImmutableSet.of(CLIENT_RENEW_PROHIBITED))
.build());
setEppInput("domain_update_add_non_server_status.xml");
assertAboutEppExceptions()
.that(assertThrows(AddExistingValueException.class, this::runFlow))
.marshalsToXml();
}
@Test
void testFailure_removeNonexistentNameserver() throws Exception {
persistActiveDomain(getUniqueIdFromCommand());
persistActiveHost("ns1.example.foo");
setEppInput("domain_update_remove_nameserver.xml");
@@ -1399,6 +1422,15 @@ class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow, Domain
.marshalsToXml();
}
@Test
void testFailure_removeNonexistentStatusValue() throws Exception {
persistActiveDomain(getUniqueIdFromCommand());
setEppInput("domain_update_remove_client_update_prohibited.xml");
assertAboutEppExceptions()
.that(assertThrows(RemoveNonexistentValueException.class, this::runFlow))
.marshalsToXml();
}
@Test
void testFailure_minimumDataset_addingNewRegistrantFails() throws Exception {
persistReferencedEntities();

1
core/src/test/resources/google/registry/flows/domain/domain_update.xml
View File

@@ -15,7 +15,6 @@
<domain:ns>
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg>
<domain:authInfo>

1
core/src/test/resources/google/registry/flows/domain/domain_update_empty_registrant.xml
View File

@@ -17,7 +17,6 @@
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:contact type="tech">sh8013</domain:contact>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg>
<domain:registrant/>

1
core/src/test/resources/google/registry/flows/domain/domain_update_no_auth_change.xml
View File

@@ -15,7 +15,6 @@
<domain:ns>
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
</domain:update>
</update>

1
core/src/test/resources/google/registry/flows/domain/domain_update_no_cltrid.xml
View File

@@ -15,7 +15,6 @@
<domain:ns>
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg>
<domain:authInfo>

1
core/src/test/resources/google/registry/flows/domain/domain_update_prohibited_status.xml
View File

@@ -15,7 +15,6 @@
<domain:ns>
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg>
<domain:authInfo>

16
core/src/test/resources/google/registry/flows/domain/domain_update_remove_client_update_prohibited.xml Normal file
View File

@@ -0,0 +1,16 @@
<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
<command>
<update>
<domain:update
xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
<domain:name>example.tld</domain:name>
<domain:add />
<domain:rem>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg />
</domain:update>
</update>
<clTRID>ABC-12345</clTRID>
</command>
</epp>

1
core/src/test/resources/google/registry/flows/domain/domain_update_remove_contact.xml
View File

@@ -15,7 +15,6 @@
<domain:ns>
<domain:hostObj>ns1.example.foo</domain:hostObj>
</domain:ns>
<domain:status s="clientUpdateProhibited"/>
</domain:rem>
<domain:chg>
<domain:authInfo>

2
core/src/test/resources/google/registry/flows/domain_update_dsdata_rem.xml
View File

@@ -16,7 +16,7 @@
<secDNS:keyTag>12346</secDNS:keyTag>
<secDNS:alg>3</secDNS:alg>
<secDNS:digestType>1</secDNS:digestType>
<secDNS:digest>38EC35D5B3A34B44C39B</secDNS:digest>
<secDNS:digest>A94A8FE5CCB19BA61C4C0873D391E987982FBBD3</secDNS:digest>
</secDNS:dsData>
</secDNS:rem>
</secDNS:update>