diff --git a/java/google/registry/flows/poll/PollAckFlow.java b/java/google/registry/flows/poll/PollAckFlow.java index 991d1990f..008629bd8 100644 --- a/java/google/registry/flows/poll/PollAckFlow.java +++ b/java/google/registry/flows/poll/PollAckFlow.java @@ -19,6 +19,7 @@ import static google.registry.flows.FlowUtils.validateClientIsLoggedIn; import static google.registry.flows.poll.PollFlowUtils.getPollMessagesQuery; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_NO_MESSAGES; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.poll.PollMessageExternalKeyConverter.makePollMessageExternalId; import static google.registry.model.poll.PollMessageExternalKeyConverter.parsePollMessageExternalId; import static google.registry.util.DateTimeUtils.isBeforeOrAt; @@ -80,14 +81,14 @@ public class PollAckFlow implements TransactionalFlow { final DateTime now = ofy().getTransactionTime(); // Load the message to be acked. If a message is queued to be delivered in the future, we treat - // it as if it doesn't exist yet. + // it as if it doesn't exist yet. Same for if the message ID year isn't the same as the actual + // poll message's event time (that means they're passing in an old already-acked ID). PollMessage pollMessage = ofy().load().key(pollMessageKey).now(); - if (pollMessage == null || !isBeforeOrAt(pollMessage.getEventTime(), now)) { + if (pollMessage == null + || !isBeforeOrAt(pollMessage.getEventTime(), now) + || !makePollMessageExternalId(pollMessage).equals(messageId)) { throw new MessageDoesNotExistException(messageId); } - // TODO(b/68953444): Once the year field on the external poll message ID becomes mandatory, add - // a check that the value of the year field is correct, by checking that - // makePollMessageExternalId(pollMessage) equals messageId. // Make sure this client is authorized to ack this message. It could be that the message is // supposed to go to a different registrar. diff --git a/java/google/registry/model/poll/PollMessageExternalKeyConverter.java b/java/google/registry/model/poll/PollMessageExternalKeyConverter.java index b2b4ec3bd..ecba67bf1 100644 --- a/java/google/registry/model/poll/PollMessageExternalKeyConverter.java +++ b/java/google/registry/model/poll/PollMessageExternalKeyConverter.java @@ -85,11 +85,9 @@ public class PollMessageExternalKeyConverter { * * @throws PollMessageExternalKeyParseException if the external key has an invalid format. */ - // TODO(b/68953444): Make the year field mandatory once sufficient time has elapsed and backwards - // compatibility is no longer necessary. public static Key parsePollMessageExternalId(String externalKey) { List idComponents = Splitter.on('-').splitToList(externalKey); - if (idComponents.size() != 5 && idComponents.size() != 6) { + if (idComponents.size() != 6) { throw new PollMessageExternalKeyParseException(); } try { diff --git a/javatests/google/registry/flows/poll/PollAckFlowTest.java b/javatests/google/registry/flows/poll/PollAckFlowTest.java index ab30213ea..bb697e769 100644 --- a/javatests/google/registry/flows/poll/PollAckFlowTest.java +++ b/javatests/google/registry/flows/poll/PollAckFlowTest.java @@ -104,24 +104,8 @@ public class PollAckFlowTest extends FlowTestCase { runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); } - // TODO(b/68953444): Remove test when missing year field backwards compatibility no longer needed. @Test - public void testSuccess_contactPollMessage_withMissingYearField() throws Exception { - setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3")); - persistResource( - new PollMessage.OneTime.Builder() - .setId(MESSAGE_ID) - .setClientId(getClientIdForFlow()) - .setEventTime(clock.nowUtc().minusDays(1)) - .setMsg("Some poll message.") - .setParent(createHistoryEntryForEppResource(contact)) - .build()); - assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); - } - - @Test - public void testSuccess_contactPollMessage_withIncorrectYearField() throws Exception { + public void testFailure_contactPollMessage_withIncorrectYearField() throws Exception { setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3-1999")); persistResource( new PollMessage.OneTime.Builder() @@ -132,7 +116,7 @@ public class PollAckFlowTest extends FlowTestCase { .setParent(createHistoryEntryForEppResource(contact)) .build()); assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); + assertThrows(MessageDoesNotExistException.class, this::runFlow); } @Test @@ -144,6 +128,7 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_recentActiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2010")); persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), END_OF_TIME); assertTransactionalFlow(true); runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); @@ -151,6 +136,7 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_oldActiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2009")); persistAutorenewPollMessage(clock.nowUtc().minusYears(2), END_OF_TIME); // Create three other messages to be queued for retrieval to get our count right, since the poll // ack response wants there to be 4 messages in the queue when the ack comes back. @@ -158,11 +144,15 @@ public class PollAckFlowTest extends FlowTestCase { persistOneTimePollMessage(MESSAGE_ID + i); } assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response.xml")); + runFlowAssertResponse( + loadFile( + "poll_ack_response.xml", + ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2009", "COUNT", "4"))); } @Test public void testSuccess_oldInactiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2010")); persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), clock.nowUtc()); assertTransactionalFlow(true); runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); @@ -175,7 +165,10 @@ public class PollAckFlowTest extends FlowTestCase { persistOneTimePollMessage(MESSAGE_ID + i); } assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response.xml")); + runFlowAssertResponse( + loadFile( + "poll_ack_response.xml", + ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2011", "COUNT", "4"))); } @Test @@ -201,6 +194,21 @@ public class PollAckFlowTest extends FlowTestCase { assertThrows(InvalidMessageIdException.class, this::runFlow); } + @Test + public void testFailure_contactPollMessage_withMissingYearField() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3")); + persistResource( + new PollMessage.OneTime.Builder() + .setId(MESSAGE_ID) + .setClientId(getClientIdForFlow()) + .setEventTime(clock.nowUtc().minusDays(1)) + .setMsg("Some poll message.") + .setParent(createHistoryEntryForEppResource(contact)) + .build()); + assertTransactionalFlow(true); + assertThrows(InvalidMessageIdException.class, this::runFlow); + } + @Test public void testFailure_invalidId_stringInsteadOfNumeric() throws Exception { setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "ABC-12345")); diff --git a/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml b/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml index 69183a40a..9aa2a562f 100644 --- a/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml +++ b/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml @@ -3,7 +3,7 @@ Command completed successfully - + ABC-12346 server-trid diff --git a/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java b/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java index 2c07c755f..6d254ad2d 100644 --- a/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java +++ b/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java @@ -122,19 +122,10 @@ public class PollMessageExternalKeyConverterTest { } @Test - // TODO(b/68953444): Remove leniency when backwards compatibility with missing field is no longer - // required. - public void testSuccess_stillParsesWithMissingYearField() { - PollMessage.OneTime pollMessage = - persistResource( - new PollMessage.OneTime.Builder() - .setClientId("TheRegistrar") - .setEventTime(clock.nowUtc()) - .setMsg("Test poll message") - .setParent(historyEntry) - .build()); - assertThat(makePollMessageExternalId(pollMessage)).isEqualTo("1-2-FOOBAR-4-5-2007"); - assertThat(parsePollMessageExternalId("1-2-FOOBAR-4-5")).isEqualTo(Key.create(pollMessage)); + public void testFailure_missingYearField() { + assertThrows( + PollMessageExternalKeyParseException.class, + () -> parsePollMessageExternalId("1-2-FOOBAR-4-5")); } @Test