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 1db88bfbe13..90fa4c2e595 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -1252,7 +1252,7 @@ public InvalidIdnDomainLabelException() { } /** Having a registrant is prohibited by registry policy. */ - static class RegistrantProhibitedException extends ParameterValuePolicyErrorException { + public static class RegistrantProhibitedException extends ParameterValuePolicyErrorException { public RegistrantProhibitedException() { super("Having a registrant is prohibited by registry policy"); } diff --git a/core/src/main/java/google/registry/model/domain/DomainCommand.java b/core/src/main/java/google/registry/model/domain/DomainCommand.java index 4d23db7f77c..2900060c37a 100644 --- a/core/src/main/java/google/registry/model/domain/DomainCommand.java +++ b/core/src/main/java/google/registry/model/domain/DomainCommand.java @@ -16,20 +16,18 @@ import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.collect.Sets.difference; import static google.registry.util.CollectionUtils.difference; -import static google.registry.util.CollectionUtils.forceEmptyToNull; +import static google.registry.util.CollectionUtils.isNullOrEmpty; import static google.registry.util.CollectionUtils.nullSafeImmutableCopy; -import static google.registry.util.CollectionUtils.nullToEmpty; import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy; -import static google.registry.util.CollectionUtils.union; import com.google.common.base.MoreObjects; -import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import google.registry.model.EppResource; +import google.registry.flows.EppException.ParameterValuePolicyErrorException; +import google.registry.flows.domain.DomainFlowUtils.RegistrantProhibitedException; +import google.registry.flows.exceptions.ContactsProhibitedException; import google.registry.model.ForeignKeyUtils; import google.registry.model.ImmutableObject; import google.registry.model.contact.Contact; @@ -67,7 +65,8 @@ public class DomainCommand { */ public interface CreateOrUpdate> extends SingleResourceCommand { /** Creates a copy of this command with hard links to hosts and contacts. */ - T cloneAndLinkReferences(DateTime now) throws InvalidReferencesException; + T cloneAndLinkReferences(DateTime now) + throws InvalidReferencesException, ParameterValuePolicyErrorException; } /** The fields on "chgType" from RFC5731. */ @@ -171,26 +170,15 @@ public DomainAuthInfo getAuthInfo() { /** Creates a copy of this {@link Create} with hard links to hosts and contacts. */ @Override - public Create cloneAndLinkReferences(DateTime now) throws InvalidReferencesException { + public Create cloneAndLinkReferences(DateTime now) + throws InvalidReferencesException, ParameterValuePolicyErrorException { Create clone = clone(this); clone.nameservers = linkHosts(clone.nameserverHostNames, now); - if (registrantContactId == null) { - clone.contacts = linkContacts(clone.foreignKeyedDesignatedContacts, now); - } else { - // Load the registrant and contacts in one shot. - ForeignKeyedDesignatedContact registrantPlaceholder = new ForeignKeyedDesignatedContact(); - registrantPlaceholder.contactId = clone.registrantContactId; - registrantPlaceholder.type = DesignatedContact.Type.REGISTRANT; - Set contacts = linkContacts( - union(nullToEmpty(clone.foreignKeyedDesignatedContacts), registrantPlaceholder), - now); - for (DesignatedContact contact : contacts) { - if (DesignatedContact.Type.REGISTRANT.equals(contact.getType())) { - clone.registrant = contact.getContactKey(); - clone.contacts = forceEmptyToNull(difference(contacts, contact)); - break; - } - } + if (registrantContactId != null) { + throw new RegistrantProhibitedException(); + } + if (!isNullOrEmpty(foreignKeyedDesignatedContacts)) { + throw new ContactsProhibitedException(); } return clone; } @@ -369,10 +357,13 @@ public ImmutableSet getContacts() { } /** Creates a copy of this {@link AddRemove} with hard links to hosts and contacts. */ - private AddRemove cloneAndLinkReferences(DateTime now) throws InvalidReferencesException { + private AddRemove cloneAndLinkReferences(DateTime now) + throws InvalidReferencesException, ContactsProhibitedException { AddRemove clone = clone(this); clone.nameservers = linkHosts(clone.nameserverHostNames, now); - clone.contacts = linkContacts(clone.foreignKeyedDesignatedContacts, now); + if (!isNullOrEmpty(foreignKeyedDesignatedContacts)) { + throw new ContactsProhibitedException(); + } return clone; } } @@ -380,16 +371,11 @@ private AddRemove cloneAndLinkReferences(DateTime now) throws InvalidReferencesE /** The inner change type on a domain update command. */ @XmlType(propOrder = {"registrantContactId", "authInfo"}) public static class Change extends DomainCreateOrChange { - /** Creates a copy of this {@link Change} with hard links to hosts and contacts. */ - Change cloneAndLinkReferences(DateTime now) throws InvalidReferencesException { + Change cloneAndLinkReferences() throws RegistrantProhibitedException { Change clone = clone(this); - clone.registrant = - Strings.isNullOrEmpty(clone.registrantContactId) - ? null - : getOnlyElement( - loadByForeignKeysCached( - ImmutableSet.of(clone.registrantContactId), Contact.class, now) - .values()); + if (clone.registrantContactId != null) { + throw new RegistrantProhibitedException(); + } return clone; } } @@ -401,11 +387,12 @@ Change cloneAndLinkReferences(DateTime now) throws InvalidReferencesException { * of those classes, which is harmless because the getters do that anyways. */ @Override - public Update cloneAndLinkReferences(DateTime now) throws InvalidReferencesException { + public Update cloneAndLinkReferences(DateTime now) + throws InvalidReferencesException, ParameterValuePolicyErrorException { Update clone = clone(this); clone.innerAdd = clone.getInnerAdd().cloneAndLinkReferences(now); clone.innerRemove = clone.getInnerRemove().cloneAndLinkReferences(now); - clone.innerChange = clone.getInnerChange().cloneAndLinkReferences(now); + clone.innerChange = clone.getInnerChange().cloneAndLinkReferences(); return clone; } } @@ -415,37 +402,17 @@ private static Set> linkHosts(Set hostNames, DateTime now) if (hostNames == null) { return null; } - return ImmutableSet.copyOf(loadByForeignKeysCached(hostNames, Host.class, now).values()); + return ImmutableSet.copyOf(loadByForeignKeysCached(hostNames, now).values()); } - private static Set linkContacts( - Set contacts, DateTime now) throws InvalidReferencesException { - if (contacts == null) { - return null; - } - ImmutableSet.Builder foreignKeys = new ImmutableSet.Builder<>(); - for (ForeignKeyedDesignatedContact contact : contacts) { - foreignKeys.add(contact.contactId); - } - ImmutableMap> loadedContacts = - loadByForeignKeysCached(foreignKeys.build(), Contact.class, now); - ImmutableSet.Builder linkedContacts = new ImmutableSet.Builder<>(); - for (ForeignKeyedDesignatedContact contact : contacts) { - linkedContacts.add( - DesignatedContact.create(contact.type, loadedContacts.get(contact.contactId))); - } - return linkedContacts.build(); - } - - /** Loads keys to cached EPP resources by their foreign keys. */ - private static ImmutableMap> loadByForeignKeysCached( - final Set foreignKeys, final Class clazz, final DateTime now) - throws InvalidReferencesException { - ImmutableMap> fks = - ForeignKeyUtils.loadKeysByCacheIfEnabled(clazz, foreignKeys, now); + /** Loads host keys to cached EPP resources by their foreign keys. */ + private static ImmutableMap> loadByForeignKeysCached( + final Set foreignKeys, final DateTime now) throws InvalidReferencesException { + ImmutableMap> fks = + ForeignKeyUtils.loadKeysByCacheIfEnabled(Host.class, foreignKeys, now); if (!fks.keySet().equals(foreignKeys)) { throw new InvalidReferencesException( - clazz, ImmutableSet.copyOf(difference(foreignKeys, fks.keySet()))); + Host.class, ImmutableSet.copyOf(difference(foreignKeys, fks.keySet()))); } return fks; } diff --git a/core/src/test/java/google/registry/model/domain/DomainCommandTest.java b/core/src/test/java/google/registry/model/domain/DomainCommandTest.java index 17f0fb2d0a9..78ac8c306ed 100644 --- a/core/src/test/java/google/registry/model/domain/DomainCommandTest.java +++ b/core/src/test/java/google/registry/model/domain/DomainCommandTest.java @@ -14,9 +14,11 @@ package google.registry.model.domain; -import static google.registry.testing.DatabaseHelper.persistActiveContact; import static google.registry.testing.DatabaseHelper.persistActiveHost; +import static org.junit.Assert.assertThrows; +import google.registry.flows.domain.DomainFlowUtils.RegistrantProhibitedException; +import google.registry.flows.exceptions.ContactsProhibitedException; import google.registry.model.ResourceCommandTestCase; import google.registry.model.eppinput.EppInput; import google.registry.model.eppinput.EppInput.ResourceCommandWrapper; @@ -29,6 +31,7 @@ class DomainCommandTest extends ResourceCommandTestCase { @Test void testCreate() throws Exception { + // This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine. doXmlRoundtripTest("domain_create.xml"); } @@ -64,42 +67,48 @@ void testCreate_fee() throws Exception { @Test void testCreate_emptyCommand() throws Exception { - // This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine. doXmlRoundtripTest("domain_create_empty.xml"); } @Test void testCreate_missingNonRegistrantContacts() throws Exception { - // This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine. doXmlRoundtripTest("domain_create_missing_non_registrant_contacts.xml"); } @Test - void testCreate_cloneAndLinkReferences() throws Exception { + void testCreate_cloneAndLinkReferences_withHosts() throws Exception { persistActiveHost("ns1.example.net"); persistActiveHost("ns2.example.net"); - persistActiveContact("sh8013"); - persistActiveContact("jd1234"); DomainCommand.Create create = (DomainCommand.Create) loadEppResourceCommand("domain_create.xml"); create.cloneAndLinkReferences(fakeClock.nowUtc()); } @Test - void testCreate_emptyCommand_cloneAndLinkReferences() throws Exception { - // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. + void testCreate_cloneAndLinkReferences_failsWithContacts() throws Exception { + persistActiveHost("ns1.example.net"); + persistActiveHost("ns2.example.net"); DomainCommand.Create create = - (DomainCommand.Create) loadEppResourceCommand("domain_create_empty.xml"); - create.cloneAndLinkReferences(fakeClock.nowUtc()); + (DomainCommand.Create) loadEppResourceCommand("domain_create_with_contacts.xml"); + assertThrows( + RegistrantProhibitedException.class, + () -> create.cloneAndLinkReferences(fakeClock.nowUtc())); } @Test - void testCreate_missingNonRegistrantContacts_cloneAndLinkReferences() throws Exception { - persistActiveContact("jd1234"); - // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. + void testCreate_cloneAndLinkReferences_failsWithRegistrant() throws Exception { DomainCommand.Create create = (DomainCommand.Create) loadEppResourceCommand("domain_create_missing_non_registrant_contacts.xml"); + assertThrows( + RegistrantProhibitedException.class, + () -> create.cloneAndLinkReferences(fakeClock.nowUtc())); + } + + @Test + void testCreate_emptyCommand_cloneAndLinkReferences() throws Exception { + DomainCommand.Create create = + (DomainCommand.Create) loadEppResourceCommand("domain_create_empty.xml"); create.cloneAndLinkReferences(fakeClock.nowUtc()); } @@ -120,16 +129,24 @@ void testUpdate_emptyCommand() throws Exception { } @Test - void testUpdate_cloneAndLinkReferences() throws Exception { + void testUpdate_cloneAndLinkReferences_hosts() throws Exception { persistActiveHost("ns1.example.com"); persistActiveHost("ns2.example.com"); - persistActiveContact("mak21"); - persistActiveContact("sh8013"); DomainCommand.Update update = (DomainCommand.Update) loadEppResourceCommand("domain_update.xml"); update.cloneAndLinkReferences(fakeClock.nowUtc()); } + @Test + void testUpdate_cloneAndLinkReferences_failsWithContacts() throws Exception { + persistActiveHost("ns1.example.com"); + persistActiveHost("ns2.example.com"); + DomainCommand.Update update = + (DomainCommand.Update) loadEppResourceCommand("domain_update_with_contacts.xml"); + assertThrows( + ContactsProhibitedException.class, () -> update.cloneAndLinkReferences(fakeClock.nowUtc())); + } + @Test void testUpdate_emptyCommand_cloneAndLinkReferences() throws Exception { // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. diff --git a/core/src/test/resources/google/registry/model/domain/domain_create.xml b/core/src/test/resources/google/registry/model/domain/domain_create.xml index c02f220d4fb..dbcbcee1f9c 100644 --- a/core/src/test/resources/google/registry/model/domain/domain_create.xml +++ b/core/src/test/resources/google/registry/model/domain/domain_create.xml @@ -9,9 +9,6 @@ ns1.example.net ns2.example.net - jd1234 - sh8013 - sh8013 2fooBAR diff --git a/core/src/test/resources/google/registry/model/domain/domain_create_with_contacts.xml b/core/src/test/resources/google/registry/model/domain/domain_create_with_contacts.xml new file mode 100644 index 00000000000..c02f220d4fb --- /dev/null +++ b/core/src/test/resources/google/registry/model/domain/domain_create_with_contacts.xml @@ -0,0 +1,22 @@ + + + + + example.com + 2 + + ns1.example.net + ns2.example.net + + jd1234 + sh8013 + sh8013 + + 2fooBAR + + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/model/domain/domain_update.xml b/core/src/test/resources/google/registry/model/domain/domain_update.xml index b51735bae68..34c5b0dac41 100644 --- a/core/src/test/resources/google/registry/model/domain/domain_update.xml +++ b/core/src/test/resources/google/registry/model/domain/domain_update.xml @@ -8,18 +8,15 @@ ns2.example.com - mak21 ns1.example.com - sh8013 - sh8013 2BARfoo diff --git a/core/src/test/resources/google/registry/model/domain/domain_update_with_contacts.xml b/core/src/test/resources/google/registry/model/domain/domain_update_with_contacts.xml new file mode 100644 index 00000000000..b51735bae68 --- /dev/null +++ b/core/src/test/resources/google/registry/model/domain/domain_update_with_contacts.xml @@ -0,0 +1,31 @@ + + + + + example.com + + + ns2.example.com + + mak21 + + + + + ns1.example.com + + sh8013 + + + + sh8013 + + 2BARfoo + + + + + ABC-12345 + +