Forbid contacts earlier in the domain EPP parsing process (#2962)

This will make testing easier, as well as allow us to remove contact code from other parts of the codebase.
2026-07-06 00:04:50 +00:00 · 2026-02-19 16:33:29 -05:00
parent 6747cc894d
commit c7f2db177b
7 changed files with 119 additions and 88 deletions
@@ -1252,7 +1252,7 @@ public class DomainFlowUtils {
  }

  /** 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");
    }
@@ -16,20 +16,18 @@ package google.registry.model.domain;

 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 class DomainCommand {

    /** 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,10 +357,13 @@ public class DomainCommand {
      }

      /** 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 @@ public class DomainCommand {
    /** 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 @@ public class DomainCommand {
     * 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 @@ public class DomainCommand {
    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;
  }
@@ -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 @@ class DomainCommandTest extends ResourceCommandTestCase {

  @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 @@ class DomainCommandTest extends ResourceCommandTestCase {
  }

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