Forbid contacts earlier in the domain EPP parsing process

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");
     }

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<T extends CreateOrUpdate<T>> 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 <a href="http://tools.ietf.org/html/rfc5731">RFC5731</a>. */
@@ -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<DesignatedContact> 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,27 +357,25 @@ public ImmutableSet<DesignatedContact> 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;
       }
     }
 
     /** The inner change type on a domain update command. */
     @XmlType(propOrder = {"registrantContactId", "authInfo"})
     public static class Change extends DomainCreateOrChange<Domain.Builder> {
-      /** 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<VKey<Host>> linkHosts(Set<String> 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<DesignatedContact> linkContacts(
-      Set<ForeignKeyedDesignatedContact> contacts, DateTime now) throws InvalidReferencesException {
-    if (contacts == null) {
-      return null;
-    }
-    ImmutableSet.Builder<String> foreignKeys = new ImmutableSet.Builder<>();
-    for (ForeignKeyedDesignatedContact contact : contacts) {
-      foreignKeys.add(contact.contactId);
-    }
-    ImmutableMap<String, VKey<Contact>> loadedContacts =
-        loadByForeignKeysCached(foreignKeys.build(), Contact.class, now);
-    ImmutableSet.Builder<DesignatedContact> 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 <T extends EppResource> ImmutableMap<String, VKey<T>> loadByForeignKeysCached(
-      final Set<String> foreignKeys, final Class<T> clazz, final DateTime now)
-      throws InvalidReferencesException {
-    ImmutableMap<String, VKey<T>> fks =
-        ForeignKeyUtils.loadKeysByCacheIfEnabled(clazz, foreignKeys, now);
+  /** Loads host keys to cached EPP resources by their foreign keys. */
+  private static ImmutableMap<String, VKey<Host>> loadByForeignKeysCached(
+      final Set<String> foreignKeys, final DateTime now) throws InvalidReferencesException {
+    ImmutableMap<String, VKey<Host>> 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;
   }

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.

core/src/test/resources/google/registry/model/domain/domain_create.xml

@@ -9,9 +9,6 @@
           <domain:hostObj>ns1.example.net</domain:hostObj>
           <domain:hostObj>ns2.example.net</domain:hostObj>
         </domain:ns>
-        <domain:registrant>jd1234</domain:registrant>
-        <domain:contact type="admin">sh8013</domain:contact>
-        <domain:contact type="tech">sh8013</domain:contact>
         <domain:authInfo>
           <domain:pw>2fooBAR</domain:pw>
         </domain:authInfo>

core/src/test/resources/google/registry/model/domain/domain_create_with_contacts.xml

@@ -0,0 +1,22 @@
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <create>
+      <domain:create
+       xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>example.com</domain:name>
+        <domain:period unit="y">2</domain:period>
+        <domain:ns>
+          <domain:hostObj>ns1.example.net</domain:hostObj>
+          <domain:hostObj>ns2.example.net</domain:hostObj>
+        </domain:ns>
+        <domain:registrant>jd1234</domain:registrant>
+        <domain:contact type="admin">sh8013</domain:contact>
+        <domain:contact type="tech">sh8013</domain:contact>
+        <domain:authInfo>
+          <domain:pw>2fooBAR</domain:pw>
+        </domain:authInfo>
+      </domain:create>
+    </create>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>

core/src/test/resources/google/registry/model/domain/domain_update.xml

@@ -8,18 +8,15 @@
           <domain:ns>
             <domain:hostObj>ns2.example.com</domain:hostObj>
           </domain:ns>
-          <domain:contact type="tech">mak21</domain:contact>
           <domain:status s="clientHold"/>
         </domain:add>
         <domain:rem>
           <domain:ns>
             <domain:hostObj>ns1.example.com</domain:hostObj>
           </domain:ns>
-          <domain:contact type="tech">sh8013</domain:contact>
           <domain:status s="clientUpdateProhibited"/>
         </domain:rem>
         <domain:chg>
-          <domain:registrant>sh8013</domain:registrant>
           <domain:authInfo>
             <domain:pw>2BARfoo</domain:pw>
           </domain:authInfo>

core/src/test/resources/google/registry/model/domain/domain_update_with_contacts.xml

@@ -0,0 +1,31 @@
+<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.com</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>ns2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:contact type="tech">mak21</domain:contact>
+          <domain:status s="clientHold"/>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>ns1.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:contact type="tech">sh8013</domain:contact>
+          <domain:status s="clientUpdateProhibited"/>
+        </domain:rem>
+        <domain:chg>
+          <domain:registrant>sh8013</domain:registrant>
+          <domain:authInfo>
+            <domain:pw>2BARfoo</domain:pw>
+          </domain:authInfo>
+        </domain:chg>
+      </domain:update>
+    </update>
+    <clTRID>ABC-12345</clTRID>
+  </command>
+</epp>