From ed910455b09ee34bbc2aeed386c2d13c83d4ac31 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Mon, 25 Jun 2018 12:39:41 -0700 Subject: [PATCH] Add more absent clTrid unit tests In RFC 5730, clTrid is specified as optional. We ran into an error earlier this year in which a registrar was not passing a client transaction id and we didn't handle it correctly. So, this CL adds some tests of common EPP operations verify that they work correctly when the clTrid is not specified. This also slightly improves some flow logic to make it more obvious at first glance that clTrid is indeed optional. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=202000845 --- java/google/registry/flows/FlowModule.java | 3 +- .../flows/async/AsyncFlowEnqueuer.java | 5 +-- .../google/registry/model/eppcommon/Trid.java | 3 +- .../registry/model/eppinput/EppInput.java | 11 ++++-- .../google/registry/flows/FlowTestCase.java | 4 +- .../registry/flows/ResourceFlowTestCase.java | 5 +-- .../flows/domain/DomainCheckFlowTest.java | 10 +++++ .../flows/domain/DomainCreateFlowTest.java | 7 ++++ .../flows/domain/DomainInfoFlowTest.java | 6 +++ .../flows/domain/DomainUpdateFlowTest.java | 21 ++++++++--- .../testdata/domain_check_no_cltrid.xml | 11 ++++++ .../testdata/domain_create_no_cltrid.xml | 21 +++++++++++ .../domain_create_response_no_cltrid.xml | 18 +++++++++ .../domain/testdata/domain_info_no_cltrid.xml | 10 +++++ .../domain_info_response_no_cltrid.xml | 37 +++++++++++++++++++ .../testdata/domain_update_no_cltrid.xml | 31 ++++++++++++++++ .../generic_success_response_no_cltrid.xml | 10 +++++ .../flows/poll/PollRequestFlowTest.java | 36 ++++++++++++------ .../flows/poll/testdata/poll_no_cltrid.xml | 5 +++ ...oll_response_domain_transfer_no_cltrid.xml | 25 +++++++++++++ .../registry/model/eppinput/EppInputTest.java | 6 +-- 21 files changed, 252 insertions(+), 33 deletions(-) create mode 100644 javatests/google/registry/flows/domain/testdata/domain_check_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/domain_create_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/domain_create_response_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/domain_info_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/domain_info_response_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/domain_update_no_cltrid.xml create mode 100644 javatests/google/registry/flows/domain/testdata/generic_success_response_no_cltrid.xml create mode 100644 javatests/google/registry/flows/poll/testdata/poll_no_cltrid.xml create mode 100644 javatests/google/registry/flows/poll/testdata/poll_response_domain_transfer_no_cltrid.xml diff --git a/java/google/registry/flows/FlowModule.java b/java/google/registry/flows/FlowModule.java index ac16c18fe..057524e85 100644 --- a/java/google/registry/flows/FlowModule.java +++ b/java/google/registry/flows/FlowModule.java @@ -156,7 +156,8 @@ public class FlowModule { @FlowScope Trid provideTrid(EppInput eppInput, ServerTridProvider serverTridProvider) { return Trid.create( - eppInput.getCommandWrapper().getClTrid(), serverTridProvider.createServerTrid()); + eppInput.getCommandWrapper().getClTrid().orElse(null), + serverTridProvider.createServerTrid()); } @Provides diff --git a/java/google/registry/flows/async/AsyncFlowEnqueuer.java b/java/google/registry/flows/async/AsyncFlowEnqueuer.java index 93e9d1aae..c311d421b 100644 --- a/java/google/registry/flows/async/AsyncFlowEnqueuer.java +++ b/java/google/registry/flows/async/AsyncFlowEnqueuer.java @@ -124,9 +124,8 @@ public final class AsyncFlowEnqueuer { .param(PARAM_SERVER_TRANSACTION_ID, trid.getServerTransactionId()) .param(PARAM_IS_SUPERUSER, Boolean.toString(isSuperuser)) .param(PARAM_REQUESTED_TIME, now.toString()); - if (trid.getClientTransactionId().isPresent()) { - task.param(PARAM_CLIENT_TRANSACTION_ID, trid.getClientTransactionId().get()); - } + trid.getClientTransactionId() + .ifPresent(clTrid -> task.param(PARAM_CLIENT_TRANSACTION_ID, clTrid)); addTaskToQueueWithRetry(asyncDeletePullQueue, task); } diff --git a/java/google/registry/model/eppcommon/Trid.java b/java/google/registry/model/eppcommon/Trid.java index ba3a0a175..0dc6d17db 100644 --- a/java/google/registry/model/eppcommon/Trid.java +++ b/java/google/registry/model/eppcommon/Trid.java @@ -16,7 +16,6 @@ package google.registry.model.eppcommon; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; -import com.google.common.annotations.VisibleForTesting; import com.googlecode.objectify.annotation.Embed; import google.registry.model.ImmutableObject; import java.util.Optional; @@ -40,6 +39,7 @@ public class Trid extends ImmutableObject { /** The client transaction id, if provided by the client, otherwise null. */ @XmlElement(name = "clTRID", namespace = "urn:ietf:params:xml:ns:epp-1.0") + @Nullable String clientTransactionId; public String getServerTransactionId() { @@ -50,7 +50,6 @@ public class Trid extends ImmutableObject { return Optional.ofNullable(clientTransactionId); } - @VisibleForTesting public static Trid create(@Nullable String clientTransactionId, String serverTransactionId) { checkArgumentNotNull(serverTransactionId, "serverTransactionId cannot be null"); Trid instance = new Trid(); diff --git a/java/google/registry/model/eppinput/EppInput.java b/java/google/registry/model/eppinput/EppInput.java index bb9924fa7..5d127462d 100644 --- a/java/google/registry/model/eppinput/EppInput.java +++ b/java/google/registry/model/eppinput/EppInput.java @@ -366,10 +366,15 @@ public class EppInput extends ImmutableObject { @XmlElementWrapper List extension; - String clTRID; + @Nullable String clTRID; - public String getClTrid() { - return clTRID; + /** + * Returns the client transaction ID. + * + *

This is optional (i.e. it may not be specified) per RFC 5730. + */ + public Optional getClTrid() { + return Optional.ofNullable(clTRID); } public InnerCommand getCommand() { diff --git a/javatests/google/registry/flows/FlowTestCase.java b/javatests/google/registry/flows/FlowTestCase.java index 409af27f1..e1a5c0df6 100644 --- a/javatests/google/registry/flows/FlowTestCase.java +++ b/javatests/google/registry/flows/FlowTestCase.java @@ -63,6 +63,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.function.Function; +import javax.annotation.Nullable; import org.joda.time.DateTime; import org.junit.Before; import org.junit.Rule; @@ -141,8 +142,9 @@ public abstract class FlowTestCase extends ShardableTestCase { return TestDataHelper.loadFile(getClass(), filename, substitutions); } + @Nullable protected String getClientTrid() throws Exception { - return eppLoader.getEpp().getCommandWrapper().getClTrid(); + return eppLoader.getEpp().getCommandWrapper().getClTrid().orElse(null); } /** Gets the client ID that the flow will run as. */ diff --git a/javatests/google/registry/flows/ResourceFlowTestCase.java b/javatests/google/registry/flows/ResourceFlowTestCase.java index 488973f89..9a0f19478 100644 --- a/javatests/google/registry/flows/ResourceFlowTestCase.java +++ b/javatests/google/registry/flows/ResourceFlowTestCase.java @@ -170,9 +170,8 @@ public abstract class ResourceFlowTestCase expected.param("clientTransactionId", clTrid)); assertTasksEnqueued("async-delete-pull", expected); } diff --git a/javatests/google/registry/flows/domain/DomainCheckFlowTest.java b/javatests/google/registry/flows/domain/DomainCheckFlowTest.java index 5e3b00255..7bd06e453 100644 --- a/javatests/google/registry/flows/domain/DomainCheckFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCheckFlowTest.java @@ -114,6 +114,16 @@ public class DomainCheckFlowTest create(true, "example3.tld", null)); } + @Test + public void testSuccess_clTridNotSpecified() throws Exception { + setEppInput("domain_check_no_cltrid.xml"); + persistActiveDomain("example1.tld"); + doCheckTest( + create(false, "example1.tld", "In use"), + create(true, "example2.tld", null), + create(true, "example3.tld", null)); + } + @Test public void testSuccess_oneExists_allocationTokenIsInvalid() throws Exception { setEppInput("domain_check_allocationtoken.xml"); diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index 111c3b5aa..e438fe9a5 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -393,6 +393,13 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase + + + + example1.tld + example2.tld + example3.tld + + + + diff --git a/javatests/google/registry/flows/domain/testdata/domain_create_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/domain_create_no_cltrid.xml new file mode 100644 index 000000000..3c3e0516d --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/domain_create_no_cltrid.xml @@ -0,0 +1,21 @@ + + + + + example.tld + 2 + + ns1.example.net + ns2.example.net + + jd1234 + sh8013 + sh8013 + + 2fooBAR + + + + + diff --git a/javatests/google/registry/flows/domain/testdata/domain_create_response_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/domain_create_response_no_cltrid.xml new file mode 100644 index 000000000..2b6b3c65a --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/domain_create_response_no_cltrid.xml @@ -0,0 +1,18 @@ + + + + Command completed successfully + + + + example.tld + 1999-04-03T22:00:00.0Z + 2001-04-03T22:00:00.0Z + + + + server-trid + + + diff --git a/javatests/google/registry/flows/domain/testdata/domain_info_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/domain_info_no_cltrid.xml new file mode 100644 index 000000000..74ed06c2c --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/domain_info_no_cltrid.xml @@ -0,0 +1,10 @@ + + + + + example.tld + + + + diff --git a/javatests/google/registry/flows/domain/testdata/domain_info_response_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/domain_info_response_no_cltrid.xml new file mode 100644 index 000000000..79e4b4497 --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/domain_info_response_no_cltrid.xml @@ -0,0 +1,37 @@ + + + + Command completed successfully + + + + example.tld + %ROID% + + jd1234 + sh8013 + sh8013 + + ns1.example.tld + ns1.example.net + + ns1.example.tld + ns2.example.tld + NewRegistrar + TheRegistrar + 1999-04-03T22:00:00.0Z + NewRegistrar + 1999-12-03T09:00:00.0Z + 2005-04-03T22:00:00.0Z + 2000-04-08T09:00:00.0Z + + 2fooBAR + + + + + server-trid + + + diff --git a/javatests/google/registry/flows/domain/testdata/domain_update_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/domain_update_no_cltrid.xml new file mode 100644 index 000000000..f66cc8e5b --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/domain_update_no_cltrid.xml @@ -0,0 +1,31 @@ + + + + + example.tld + + + ns2.example.foo + + mak21 + Payment overdue. + + + + ns1.example.foo + + sh8013 + + + + sh8013 + + 2BARfoo + + + + + + diff --git a/javatests/google/registry/flows/domain/testdata/generic_success_response_no_cltrid.xml b/javatests/google/registry/flows/domain/testdata/generic_success_response_no_cltrid.xml new file mode 100644 index 000000000..32dd8b167 --- /dev/null +++ b/javatests/google/registry/flows/domain/testdata/generic_success_response_no_cltrid.xml @@ -0,0 +1,10 @@ + + + + Command completed successfully + + + server-trid + + + diff --git a/javatests/google/registry/flows/poll/PollRequestFlowTest.java b/javatests/google/registry/flows/poll/PollRequestFlowTest.java index 5e820ecae..bad4886ad 100644 --- a/javatests/google/registry/flows/poll/PollRequestFlowTest.java +++ b/javatests/google/registry/flows/poll/PollRequestFlowTest.java @@ -59,28 +59,42 @@ public class PollRequestFlowTest extends FlowTestCase { host = persistActiveHost("ns1.test.example"); } - @Test - public void testSuccess_domainTransferApproved() throws Exception { + private void persistPendingTransferPollMessage() { persistResource( new PollMessage.OneTime.Builder() .setClientId(getClientIdForFlow()) .setEventTime(clock.nowUtc().minusDays(1)) .setMsg("Transfer approved.") - .setResponseData(ImmutableList.of(new DomainTransferResponse.Builder() - .setFullyQualifiedDomainName("test.example") - .setTransferStatus(TransferStatus.SERVER_APPROVED) - .setGainingClientId(getClientIdForFlow()) - .setTransferRequestTime(clock.nowUtc().minusDays(5)) - .setLosingClientId("TheRegistrar") - .setPendingTransferExpirationTime(clock.nowUtc().minusDays(1)) - .setExtendedRegistrationExpirationTime(clock.nowUtc().plusYears(1)) - .build())) + .setResponseData( + ImmutableList.of( + new DomainTransferResponse.Builder() + .setFullyQualifiedDomainName("test.example") + .setTransferStatus(TransferStatus.SERVER_APPROVED) + .setGainingClientId(getClientIdForFlow()) + .setTransferRequestTime(clock.nowUtc().minusDays(5)) + .setLosingClientId("TheRegistrar") + .setPendingTransferExpirationTime(clock.nowUtc().minusDays(1)) + .setExtendedRegistrationExpirationTime(clock.nowUtc().plusYears(1)) + .build())) .setParent(createHistoryEntryForEppResource(domain)) .build()); + } + + @Test + public void testSuccess_domainTransferApproved() throws Exception { + persistPendingTransferPollMessage(); assertTransactionalFlow(false); runFlowAssertResponse(loadFile("poll_response_domain_transfer.xml")); } + @Test + public void testSuccess_clTridNotSpecified() throws Exception { + setEppInput("poll_no_cltrid.xml"); + persistPendingTransferPollMessage(); + assertTransactionalFlow(false); + runFlowAssertResponse(loadFile("poll_response_domain_transfer_no_cltrid.xml")); + } + @Test public void testSuccess_contactTransferPending() throws Exception { clock.setTo(DateTime.parse("2000-06-13T22:00:00.0Z")); diff --git a/javatests/google/registry/flows/poll/testdata/poll_no_cltrid.xml b/javatests/google/registry/flows/poll/testdata/poll_no_cltrid.xml new file mode 100644 index 000000000..d00ad8534 --- /dev/null +++ b/javatests/google/registry/flows/poll/testdata/poll_no_cltrid.xml @@ -0,0 +1,5 @@ + + + + + diff --git a/javatests/google/registry/flows/poll/testdata/poll_response_domain_transfer_no_cltrid.xml b/javatests/google/registry/flows/poll/testdata/poll_response_domain_transfer_no_cltrid.xml new file mode 100644 index 000000000..0525a5c8a --- /dev/null +++ b/javatests/google/registry/flows/poll/testdata/poll_response_domain_transfer_no_cltrid.xml @@ -0,0 +1,25 @@ + + + + Command completed successfully; ack to dequeue + + + 2011-01-01T01:01:01Z + Transfer approved. + + + + test.example + serverApproved + NewRegistrar + 2010-12-28T01:01:01Z + TheRegistrar + 2011-01-01T01:01:01Z + 2012-01-02T01:01:01Z + + + + server-trid + + + diff --git a/javatests/google/registry/model/eppinput/EppInputTest.java b/javatests/google/registry/model/eppinput/EppInputTest.java index f7bc46b3e..6a3084783 100644 --- a/javatests/google/registry/model/eppinput/EppInputTest.java +++ b/javatests/google/registry/model/eppinput/EppInputTest.java @@ -35,7 +35,7 @@ public class EppInputTest { public void testUnmarshalling_contactInfo() throws Exception { EppInput input = unmarshal(EppInput.class, loadBytes(ContactResourceTest.class, "contact_info.xml").read()); - assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345"); + assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345"); assertThat(input.getCommandType()).isEqualTo("info"); assertThat(input.getResourceType()).hasValue("contact"); assertThat(input.getSingleTargetId()).hasValue("sh8013"); @@ -46,7 +46,7 @@ public class EppInputTest { public void testUnmarshalling_domainCheck() throws Exception { EppInput input = unmarshal(EppInput.class, loadBytes(DomainResourceTest.class, "domain_check.xml").read()); - assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345"); + assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345"); assertThat(input.getCommandType()).isEqualTo("check"); assertThat(input.getResourceType()).hasValue("domain"); assertThat(input.getSingleTargetId()).isEmpty(); @@ -56,7 +56,7 @@ public class EppInputTest { @Test public void testUnmarshalling_login() throws Exception { EppInput input = unmarshal(EppInput.class, loadBytes(getClass(), "login_valid.xml").read()); - assertThat(input.getCommandWrapper().getClTrid()).isEqualTo("ABC-12345"); + assertThat(input.getCommandWrapper().getClTrid()).hasValue("ABC-12345"); assertThat(input.getCommandType()).isEqualTo("login"); assertThat(input.getResourceType()).isEmpty(); assertThat(input.getSingleTargetId()).isEmpty();