From d1ea3e3a68f1f9c1897e223cac1023dc22938e27 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Mon, 15 Aug 2016 12:14:40 -0700 Subject: [PATCH] Fix NPE bug in DomainCommand.cloneAndLinkReferences() See b/30806813 for more context. Copied from there: This appears to be happening if we get an EPP domain create command that is missing any contacts (but has a registrant; with no registrant we exercise a different codepath). In that case, JAXB leaves the contacts field on the Create null, and we try to pass it into Sets.union() as a result of Corey's refactoring in [] that changed contact loading to load the contacts and registrant all at once. The fix is just to apply nullToEmpty() first. Note that it's always an error to try to create a domain without any non-registrant contacts, but that's supposed to happen later on in BaseDomainCreateFlow.verifyCreateIsAllowed() via validateRequiredContactsPresent(), which will produce a nice error message. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=130309019 --- .../registry/model/domain/DomainCommand.java | 7 +- .../flows/domain/DomainCreateFlowTest.java | 8 ++ ...create_missing_non_registrant_contacts.xml | 20 ++++ .../model/domain/DomainCommandTest.java | 106 +++++++++++++++++- .../domain/testdata/domain_create_empty.xml | 14 +++ ...create_missing_non_registrant_contacts.xml | 15 +++ .../domain/testdata/domain_update_empty.xml | 11 ++ 7 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 javatests/google/registry/flows/domain/testdata/domain_create_missing_non_registrant_contacts.xml create mode 100644 javatests/google/registry/model/domain/testdata/domain_create_empty.xml create mode 100644 javatests/google/registry/model/domain/testdata/domain_create_missing_non_registrant_contacts.xml create mode 100644 javatests/google/registry/model/domain/testdata/domain_update_empty.xml diff --git a/java/google/registry/model/domain/DomainCommand.java b/java/google/registry/model/domain/DomainCommand.java index 1d8d04c37..780aa0fb7 100644 --- a/java/google/registry/model/domain/DomainCommand.java +++ b/java/google/registry/model/domain/DomainCommand.java @@ -22,7 +22,9 @@ import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.intersection; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.util.CollectionUtils.difference; +import static google.registry.util.CollectionUtils.forceEmptyToNull; 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; @@ -204,12 +206,12 @@ public class DomainCommand { registrantPlaceholder.contactId = clone.registrantContactId; registrantPlaceholder.type = DesignatedContact.Type.REGISTRANT; Set contacts = linkContacts( - union(clone.foreignKeyedDesignatedContacts, registrantPlaceholder), + union(nullToEmpty(clone.foreignKeyedDesignatedContacts), registrantPlaceholder), now); for (DesignatedContact contact : contacts) { if (DesignatedContact.Type.REGISTRANT.equals(contact.getType())) { clone.registrant = contact.getContactRef(); - clone.contacts = difference(contacts, contact); + clone.contacts = forceEmptyToNull(difference(contacts, contact)); break; } } @@ -511,6 +513,7 @@ public class DomainCommand { private final Class type; InvalidReferencesException(Class type, ImmutableSet foreignKeys) { + super(String.format("Invalid %s reference IDs: %s", type.getSimpleName(), foreignKeys)); this.type = checkNotNull(type); this.foreignKeys = foreignKeys; } diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index e10fef938..c5aa23f39 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -1025,6 +1025,14 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase + + + + example.tld + 2 + + ns1.example.net + ns2.example.net + + jd1234 + + 2fooBAR + + + + ABC-12345 + + diff --git a/javatests/google/registry/model/domain/DomainCommandTest.java b/javatests/google/registry/model/domain/DomainCommandTest.java index c3fcf3695..4a477f853 100644 --- a/javatests/google/registry/model/domain/DomainCommandTest.java +++ b/javatests/google/registry/model/domain/DomainCommandTest.java @@ -14,11 +14,42 @@ package google.registry.model.domain; +import static google.registry.testing.DatastoreHelper.persistActiveContact; +import static google.registry.testing.DatastoreHelper.persistActiveHost; +import static org.joda.time.DateTimeZone.UTC; + import google.registry.model.ResourceCommandTestCase; +import google.registry.model.eppinput.EppInput; +import google.registry.model.eppinput.EppInput.ResourceCommandWrapper; +import google.registry.model.eppinput.ResourceCommand; +import google.registry.model.ofy.Ofy; +import google.registry.testing.AppEngineRule; +import google.registry.testing.EppLoader; +import google.registry.testing.FakeClock; +import google.registry.testing.InjectRule; +import org.joda.time.DateTime; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; -/** Test xml roundtripping of commands. */ +/** Tests for DomainCommand. */ public class DomainCommandTest extends ResourceCommandTestCase { + + @Rule + public final AppEngineRule appEngine = AppEngineRule.builder() + .withDatastore() + .build(); + + @Rule + public InjectRule inject = new InjectRule(); + + private FakeClock clock = new FakeClock(DateTime.now(UTC)); + + @Before + public void beforeDomainCommandTest() throws Exception { + inject.setStaticField(Ofy.class, "clock", clock); + } + @Test public void testCreate() throws Exception { doXmlRoundtripTest("domain_create.xml"); @@ -64,6 +95,47 @@ public class DomainCommandTest extends ResourceCommandTestCase { doXmlRoundtripTest("domain_create_flags.xml"); } + @Test + public 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 + public 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 + public void testCreate_cloneAndLinkReferences() 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(clock.nowUtc()); + } + + @Test + public void testCreate_emptyCommand_cloneAndLinkReferences() throws Exception { + // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. + DomainCommand.Create create = + (DomainCommand.Create) loadEppResourceCommand("domain_create_empty.xml"); + create.cloneAndLinkReferences(clock.nowUtc()); + } + + @Test + public void testCreate_missingNonRegistrantContacts_cloneAndLinkReferences() throws Exception { + persistActiveContact("jd1234"); + // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. + DomainCommand.Create create = + (DomainCommand.Create) + loadEppResourceCommand("domain_create_missing_non_registrant_contacts.xml"); + create.cloneAndLinkReferences(clock.nowUtc()); + } + @Test public void testDelete() throws Exception { doXmlRoundtripTest("domain_delete.xml"); @@ -84,6 +156,31 @@ public class DomainCommandTest extends ResourceCommandTestCase { doXmlRoundtripTest("domain_update_flags.xml"); } + @Test + public void testUpdate_emptyCommand() throws Exception { + // This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine. + doXmlRoundtripTest("domain_update_empty.xml"); + } + + @Test + public void testUpdate_cloneAndLinkReferences() 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(clock.nowUtc()); + } + + @Test + public void testUpdate_emptyCommand_cloneAndLinkReferences() throws Exception { + // This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine. + DomainCommand.Update update = + (DomainCommand.Update) loadEppResourceCommand("domain_update_empty.xml"); + update.cloneAndLinkReferences(clock.nowUtc()); + } + @Test public void testInfo() throws Exception { doXmlRoundtripTest("domain_info.xml"); @@ -178,4 +275,11 @@ public class DomainCommandTest extends ResourceCommandTestCase { public void testRenew_fee() throws Exception { doXmlRoundtripTest("domain_renew_fee.xml"); } + + /** Loads the EppInput from the given filename and returns its ResourceCommand element. */ + private ResourceCommand loadEppResourceCommand(String filename) throws Exception { + EppInput eppInput = new EppLoader(this, filename).getEpp(); + return ((ResourceCommandWrapper) eppInput.getCommandWrapper().getCommand()) + .getResourceCommand(); + } } diff --git a/javatests/google/registry/model/domain/testdata/domain_create_empty.xml b/javatests/google/registry/model/domain/testdata/domain_create_empty.xml new file mode 100644 index 000000000..c5ad327cc --- /dev/null +++ b/javatests/google/registry/model/domain/testdata/domain_create_empty.xml @@ -0,0 +1,14 @@ + + + + + example.com + + 2fooBAR + + + + ABC-12345 + + diff --git a/javatests/google/registry/model/domain/testdata/domain_create_missing_non_registrant_contacts.xml b/javatests/google/registry/model/domain/testdata/domain_create_missing_non_registrant_contacts.xml new file mode 100644 index 000000000..cd2c344fc --- /dev/null +++ b/javatests/google/registry/model/domain/testdata/domain_create_missing_non_registrant_contacts.xml @@ -0,0 +1,15 @@ + + + + + example.com + jd1234 + + 2fooBAR + + + + ABC-12345 + + diff --git a/javatests/google/registry/model/domain/testdata/domain_update_empty.xml b/javatests/google/registry/model/domain/testdata/domain_update_empty.xml new file mode 100644 index 000000000..8e7ec1bfa --- /dev/null +++ b/javatests/google/registry/model/domain/testdata/domain_update_empty.xml @@ -0,0 +1,11 @@ + + + + + example.com + + + ABC-12345 + +