From 38bf86c0fdba402c9ccd27bd14e6892b0228f7d5 Mon Sep 17 00:00:00 2001 From: guyben Date: Mon, 9 Apr 2018 11:08:48 -0700 Subject: [PATCH] Incorporate some of the fixes done in RegistrarPremiumPriceAckAction This is in preparation for merging and then removing RegistrarPremiumPriceAckAction. This includes: test that the data the UI sent isn't stale --------------------------------------------- Our system is "read, modify, write". However, if between the "read" and the "write" someone else changed the registry, my write will undo their change even if I didn't touch any of their fields. To solve that - we use the "lastUpdateTime" timestamp of the registrar. the UI reads it with the rest of the data, and sends it back on "write". We will now make sure the registrar currently in datastore has the same timestamp. support premium-price-ack flag --------------------------------- Add support for reading and writing this flag. We still won't be using it - that's in a followup CL, but we support it. support changing the URL ------------------------ Add changing the URL in the UI, under the "whois" section Will replace the Ack endpoint with this (and remove that endpoint) in a followup CL ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=192154078 --- java/google/registry/ui/externs/json.js | 2 + .../ui/server/RegistrarFormFields.java | 165 ++++++++++-------- .../google/registry/ui/server/registrar/BUILD | 1 + .../registrar/RegistrarSettingsAction.java | 133 +++++++++----- .../server/registrar/ContactSettingsTest.java | 9 +- .../RegistrarSettingsActionTest.java | 56 +++++- .../RegistrarSettingsActionTestCase.java | 14 -- .../registrar/testdata/update_registrar.json | 2 +- .../update_registrar_duplicate_contacts.json | 2 +- 9 files changed, 237 insertions(+), 147 deletions(-) diff --git a/java/google/registry/ui/externs/json.js b/java/google/registry/ui/externs/json.js index 13d72b53f..b5624949a 100644 --- a/java/google/registry/ui/externs/json.js +++ b/java/google/registry/ui/externs/json.js @@ -78,6 +78,8 @@ registry.json.Response.prototype.results; * icannReferralEmail: string, * ipAddressWhitelist: !Array, * emailAddress: (string?|undefined), + * lastUpdateTime: string, + * url: (string?|undefined), * phonePasscode: (string?|undefined), * phoneNumber: (string?|undefined), * faxNumber: (string?|undefined), diff --git a/java/google/registry/ui/server/RegistrarFormFields.java b/java/google/registry/ui/server/RegistrarFormFields.java index 3d288c426..61fc412ad 100644 --- a/java/google/registry/ui/server/RegistrarFormFields.java +++ b/java/google/registry/ui/server/RegistrarFormFields.java @@ -37,7 +37,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Function; +import javax.annotation.Nullable; +import org.joda.time.DateTime; /** Form fields for validating input for the {@code Registrar} class. */ public final class RegistrarFormFields { @@ -46,31 +47,18 @@ public final class RegistrarFormFields { public static final Pattern ASCII_PATTERN = Pattern.compile("[[:ascii:]]*"); public static final String ASCII_ERROR = "Please only use ASCII-US characters."; - private static final Function CIDR_TRANSFORM = - input -> { - try { - return input != null ? CidrAddressBlock.create(input) : null; - } catch (IllegalArgumentException e) { - throw new FormFieldException("Not a valid CIDR notation IP-address block.", e); - } - }; - - private static final Function HOSTNAME_TRANSFORM = - input -> { - if (input == null) { - return null; - } - if (!InternetDomainName.isValid(input)) { - throw new FormFieldException("Not a valid hostname."); - } - return canonicalizeDomainName(input); - }; - public static final FormField NAME_FIELD = FormFields.NAME.asBuilderNamed("registrarName") .required() .build(); + public static final FormField LAST_UPDATE_TIME = + FormFields.LABEL + .asBuilderNamed("lastUpdateTime") + .transform(DateTime.class, RegistrarFormFields::parseDateTime) + .required() + .build(); + public static final FormField EMAIL_ADDRESS_FIELD_REQUIRED = FormFields.EMAIL.asBuilderNamed("emailAddress") .matches(ASCII_PATTERN, ASCII_ERROR) @@ -111,7 +99,7 @@ public final class RegistrarFormFields { public static final FormField WHOIS_SERVER_FIELD = FormFields.LABEL.asBuilderNamed("whoisServer") - .transform(HOSTNAME_TRANSFORM) + .transform(RegistrarFormFields::parseHostname) .build(); public static final FormField BLOCK_PREMIUM_NAMES_FIELD = @@ -173,7 +161,7 @@ public final class RegistrarFormFields { public static final FormField, List> IP_ADDRESS_WHITELIST_FIELD = FormField.named("ipAddressWhitelist") .emptyToNull() - .transform(CidrAddressBlock.class, CIDR_TRANSFORM) + .transform(CidrAddressBlock.class, RegistrarFormFields::parseCidr) .asList() .build(); @@ -228,34 +216,9 @@ public final class RegistrarFormFields { .asSet(Splitter.on(',').omitEmptyStrings().trimResults()) .build(); - public static final Function, RegistrarContact.Builder> - REGISTRAR_CONTACT_TRANSFORM = - args -> { - if (args == null) { - return null; - } - RegistrarContact.Builder builder = new RegistrarContact.Builder(); - CONTACT_NAME_FIELD.extractUntyped(args).ifPresent(builder::setName); - CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).ifPresent(builder::setEmailAddress); - CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD - .extractUntyped(args) - .ifPresent(builder::setVisibleInWhoisAsAdmin); - CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD - .extractUntyped(args) - .ifPresent(builder::setVisibleInWhoisAsTech); - PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD - .extractUntyped(args) - .ifPresent(builder::setVisibleInDomainWhoisAsAbuse); - CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setPhoneNumber); - CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setFaxNumber); - CONTACT_TYPES.extractUntyped(args).ifPresent(builder::setTypes); - CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).ifPresent(builder::setGaeUserId); - return builder; - }; - public static final FormField>, List> CONTACTS_FIELD = FormField.mapNamed("contacts") - .transform(RegistrarContact.Builder.class, REGISTRAR_CONTACT_TRANSFORM) + .transform(RegistrarContact.Builder.class, RegistrarFormFields::toRegistrarContactBuilder) .asList() .build(); @@ -316,42 +279,94 @@ public final class RegistrarFormFields { public static final FormField, RegistrarAddress> L10N_ADDRESS_FIELD = FormField.mapNamed("localizedAddress") - .transform(RegistrarAddress.class, newAddressTransform( - L10N_STREET_FIELD, L10N_CITY_FIELD, L10N_STATE_FIELD, L10N_ZIP_FIELD)) + .transform(RegistrarAddress.class, (args) -> toNewAddress( + args, L10N_STREET_FIELD, L10N_CITY_FIELD, L10N_STATE_FIELD, L10N_ZIP_FIELD)) .build(); public static final FormField PREMIUM_PRICE_ACK_REQUIRED = FormField.named("premiumPriceAckRequired", Boolean.class).build(); - private static Function, RegistrarAddress> newAddressTransform( + private static @Nullable RegistrarAddress toNewAddress( + @Nullable Map args, final FormField, List> streetField, final FormField cityField, final FormField stateField, final FormField zipField) { - return args -> { - if (args == null) { - return null; - } - RegistrarAddress.Builder builder = new RegistrarAddress.Builder(); - String countryCode = COUNTRY_CODE_FIELD.extractUntyped(args).get(); - builder.setCountryCode(countryCode); - streetField - .extractUntyped(args) - .ifPresent(streets -> builder.setStreet(ImmutableList.copyOf(streets))); - cityField.extractUntyped(args).ifPresent(builder::setCity); - Optional stateFieldValue = stateField.extractUntyped(args); - if (stateFieldValue.isPresent()) { - String state = stateFieldValue.get(); - if ("US".equals(countryCode)) { - state = Ascii.toUpperCase(state); - if (!StateCode.US_MAP.containsKey(state)) { - throw new FormFieldException(stateField, "Unknown US state code."); - } + if (args == null) { + return null; + } + RegistrarAddress.Builder builder = new RegistrarAddress.Builder(); + String countryCode = COUNTRY_CODE_FIELD.extractUntyped(args).get(); + builder.setCountryCode(countryCode); + streetField + .extractUntyped(args) + .ifPresent(streets -> builder.setStreet(ImmutableList.copyOf(streets))); + cityField.extractUntyped(args).ifPresent(builder::setCity); + Optional stateFieldValue = stateField.extractUntyped(args); + if (stateFieldValue.isPresent()) { + String state = stateFieldValue.get(); + if ("US".equals(countryCode)) { + state = Ascii.toUpperCase(state); + if (!StateCode.US_MAP.containsKey(state)) { + throw new FormFieldException(stateField, "Unknown US state code."); } - builder.setState(state); } - zipField.extractUntyped(args).ifPresent(builder::setZip); - return builder.build(); - }; + builder.setState(state); + } + zipField.extractUntyped(args).ifPresent(builder::setZip); + return builder.build(); + } + + private static CidrAddressBlock parseCidr(String input) { + try { + return input != null ? CidrAddressBlock.create(input) : null; + } catch (IllegalArgumentException e) { + throw new FormFieldException("Not a valid CIDR notation IP-address block.", e); + } + } + + private static @Nullable String parseHostname(@Nullable String input) { + if (input == null) { + return null; + } + if (!InternetDomainName.isValid(input)) { + throw new FormFieldException("Not a valid hostname."); + } + return canonicalizeDomainName(input); + } + + public static @Nullable DateTime parseDateTime(@Nullable String input) { + if (input == null) { + return null; + } + try { + return DateTime.parse(input); + } catch (IllegalArgumentException e) { + throw new FormFieldException("Not a valid ISO date-time string.", e); + } + } + + private static @Nullable RegistrarContact.Builder toRegistrarContactBuilder( + @Nullable Map args) { + if (args == null) { + return null; + } + RegistrarContact.Builder builder = new RegistrarContact.Builder(); + CONTACT_NAME_FIELD.extractUntyped(args).ifPresent(builder::setName); + CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).ifPresent(builder::setEmailAddress); + CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD + .extractUntyped(args) + .ifPresent(builder::setVisibleInWhoisAsAdmin); + CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD + .extractUntyped(args) + .ifPresent(builder::setVisibleInWhoisAsTech); + PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD + .extractUntyped(args) + .ifPresent(builder::setVisibleInDomainWhoisAsAbuse); + CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setPhoneNumber); + CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setFaxNumber); + CONTACT_TYPES.extractUntyped(args).ifPresent(builder::setTypes); + CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).ifPresent(builder::setGaeUserId); + return builder; } } diff --git a/java/google/registry/ui/server/registrar/BUILD b/java/google/registry/ui/server/registrar/BUILD index 897085ea8..3384d5598 100644 --- a/java/google/registry/ui/server/registrar/BUILD +++ b/java/google/registry/ui/server/registrar/BUILD @@ -34,6 +34,7 @@ java_library( "@com_google_re2j", "@io_bazel_rules_closure//closure/templates", "@javax_servlet_api", + "@joda_time", "@org_joda_money", ], ) diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index f85c94853..89687d337 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -27,7 +27,6 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Multimap; import com.google.common.collect.Streams; -import com.googlecode.objectify.Work; import google.registry.config.RegistryConfig.Config; import google.registry.export.sheet.SyncRegistrarsSheetAction; import google.registry.model.registrar.Registrar; @@ -55,6 +54,7 @@ import java.util.Set; import java.util.function.Predicate; import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; +import org.joda.time.DateTime; /** * Admin servlet that allows creating or updating a registrar. Deletes are not allowed so as to @@ -112,7 +112,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA try { switch (op) { case "update": - return update(args, initialRegistrar); + return update(args, initialRegistrar.getClientId()); case "read": return JsonResponseHelper.create(SUCCESS, "Success", initialRegistrar.toJsonMap()); default: @@ -135,41 +135,66 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA args); return JsonResponseHelper.create(ERROR, e.getMessage()); } - } - Map update(final Map args, final Registrar registrar) { - final String clientId = sessionUtils.getRegistrarClientId(request); + Map update(final Map args, String clientId) { return ofy() .transact( - (Work>) - () -> { - ImmutableSet oldContacts = registrar.getContacts(); - Map existingRegistrarMap = - expandRegistrarWithContacts(oldContacts, registrar); - Registrar.Builder builder = registrar.asBuilder(); - ImmutableSet updatedContacts = - changeRegistrarFields(registrar, builder, args); - if (!updatedContacts.isEmpty()) { - builder.setContactsRequireSyncing(true); - } - Registrar updatedRegistrar = builder.build(); - ofy().save().entity(updatedRegistrar); - if (!updatedContacts.isEmpty()) { - checkContactRequirements(oldContacts, updatedContacts); - RegistrarContact.updateContacts(updatedRegistrar, updatedContacts); - } - // Update the registrar map with updated contacts to bypass Objectify caching - // issues that come into play with calling getContacts(). - Map updatedRegistrarMap = - expandRegistrarWithContacts(updatedContacts, updatedRegistrar); - sendExternalUpdatesIfNecessary( - updatedRegistrar.getRegistrarName(), - existingRegistrarMap, - updatedRegistrarMap); - return JsonResponseHelper.create( - SUCCESS, "Saved " + clientId, updatedRegistrar.toJsonMap()); - }); + () -> { + // We load the registrar here rather than use the initialRegistrar above - to make + // sure we have the latest version. This one is loaded inside the transaction, so it's + // guaranteed to not change before we update it. + Registrar registrar = Registrar.loadByClientId(clientId).get(); + // Verify that the registrar hasn't been changed. + // To do that - we find the latest update time (or null if the registrar has been + // deleted) and compare to the update time from the args. The update time in the args + // comes from the read that gave the UI the data - if it's out of date, then the UI + // had out of date data. + DateTime latest = registrar.getLastUpdateTime(); + DateTime latestFromArgs = + RegistrarFormFields.LAST_UPDATE_TIME.extractUntyped(args).get(); + if (!latestFromArgs.equals(latest)) { + logger.warningfmt( + "registrar changed since reading the data! " + + " Last updated at %s, but args data last updated at %s", + latest, latestFromArgs); + return JsonResponseHelper.create( + ERROR, "registrar has been changed by someone else. Please reload and retry."); + } + + // Keep the current contacts so we can later check that no required contact was + // removed, email the changes to the contacts + ImmutableSet contacts = registrar.getContacts(); + + // Update the registrar from the request. + Registrar.Builder builder = registrar.asBuilder(); + changeRegistrarFields(registrar, builder, args); + + // read the contacts from the request. + ImmutableSet updatedContacts = + readContacts(registrar, args); + if (!updatedContacts.isEmpty()) { + builder.setContactsRequireSyncing(true); + } + + // Save the updated registrar + Registrar updatedRegistrar = builder.build(); + if (!updatedRegistrar.equals(registrar)) { + ofy().save().entity(updatedRegistrar); + } + + // Save the updated contacts + if (!updatedContacts.isEmpty()) { + checkContactRequirements(contacts, updatedContacts); + RegistrarContact.updateContacts(updatedRegistrar, updatedContacts); + } + + // Email and return update. + sendExternalUpdatesIfNecessary( + registrar, contacts, updatedRegistrar, updatedContacts); + return JsonResponseHelper.create( + SUCCESS, "Saved " + clientId, updatedRegistrar.toJsonMap()); + }); } private Map expandRegistrarWithContacts(Iterable contacts, @@ -186,12 +211,16 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA } /** - * Updates a registrar builder with the supplied args from the http request, and returns a list of - * the new registrar contacts. + * Updates a registrar builder with the supplied args from the http request; */ - public static ImmutableSet changeRegistrarFields( + public static void changeRegistrarFields( Registrar existingRegistrarObj, Registrar.Builder builder, Map args) { + // BILLING + RegistrarFormFields.PREMIUM_PRICE_ACK_REQUIRED + .extractUntyped(args) + .ifPresent(builder::setPremiumPriceAckRequired); + // WHOIS builder.setWhoisServer( RegistrarFormFields.WHOIS_SERVER_FIELD.extractUntyped(args).orElse(null)); @@ -212,6 +241,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA builder.setLocalizedAddress( RegistrarFormFields.L10N_ADDRESS_FIELD.extractUntyped(args).orElse(null)); + builder.setUrl(RegistrarFormFields.URL_FIELD.extractUntyped(args).orElse(null)); + // Security builder.setIpAddressWhitelist( RegistrarFormFields.IP_ADDRESS_WHITELIST_FIELD @@ -226,17 +257,16 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA .ifPresent( certificate -> builder.setFailoverClientCertificate(certificate, ofy().getTransactionTime())); + } - builder.setUrl( - RegistrarFormFields.URL_FIELD.extractUntyped(args).orElse(null)); - builder.setReferralUrl( - RegistrarFormFields.REFERRAL_URL_FIELD.extractUntyped(args).orElse(null)); + /** Reads the contacts from the supplied args. */ + public static ImmutableSet readContacts( + Registrar registrar, Map args) { - // Contact ImmutableSet.Builder contacts = new ImmutableSet.Builder<>(); Optional> builders = RegistrarFormFields.CONTACTS_FIELD.extractUntyped(args); if (builders.isPresent()) { - builders.get().forEach(c -> contacts.add(c.setParent(existingRegistrarObj).build())); + builders.get().forEach(c -> contacts.add(c.setParent(registrar).build())); } return contacts.build(); @@ -332,10 +362,19 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA * enqueues a task to re-sync the registrar sheet. */ private void sendExternalUpdatesIfNecessary( - String registrarName, - Map existingRegistrar, - Map updatedRegistrar) { - Map diffs = DiffUtils.deepDiff(existingRegistrar, updatedRegistrar, true); + Registrar existingRegistrar, + ImmutableSet existingContacts, + Registrar updatedRegistrar, + ImmutableSet updatedContacts) { + if (registrarChangesNotificationEmailAddresses.isEmpty()) { + return; + } + + Map diffs = + DiffUtils.deepDiff( + expandRegistrarWithContacts(existingContacts, existingRegistrar), + expandRegistrarWithContacts(updatedContacts, updatedRegistrar), + true); @SuppressWarnings("unchecked") Set changedKeys = (Set) diffs.keySet(); if (CollectionUtils.difference(changedKeys, "lastUpdateTime").isEmpty()) { @@ -345,7 +384,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA if (!registrarChangesNotificationEmailAddresses.isEmpty()) { sendEmailUtils.sendEmail( registrarChangesNotificationEmailAddresses, - String.format("Registrar %s updated", registrarName), + String.format("Registrar %s updated", existingRegistrar.getRegistrarName()), "The following changes were made to the registrar:\n" + DiffUtils.prettyPrintDiffedMap(diffs, null)); } diff --git a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java b/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java index 0eaf8c40a..12c965279 100644 --- a/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java +++ b/javatests/google/registry/ui/server/registrar/ContactSettingsTest.java @@ -113,7 +113,8 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase { .setTypes(ImmutableSet.of(RegistrarContact.Type.ADMIN, RegistrarContact.Type.TECH)) .build(); // Lest we anger the timestamp inversion bug. - persistResource(registrar); + // (we also update the registrar so we get the timestamp right) + registrar = persistResource(registrar); persistSimpleResource(rc); // Now try to remove the phone number. @@ -138,7 +139,8 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase { .setVisibleInDomainWhoisAsAbuse(true) .build(); // Lest we anger the timestamp inversion bug. - persistResource(registrar); + // (we also update the registrar so we get the timestamp right) + registrar = persistResource(registrar); persistSimpleResource(rc); // Now try to remove the contact. @@ -164,7 +166,8 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase { .setVisibleInDomainWhoisAsAbuse(true) .build(); // Lest we anger the timestamp inversion bug. - persistResource(registrar); + // (we also update the registrar so we get the timestamp right) + registrar = persistResource(registrar); persistSimpleResource(rc); // Now try to set the phone number to null. diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 48f453804..4fce2b8b6 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -36,6 +36,8 @@ import google.registry.testing.TaskQueueHelper.TaskMatcher; import java.util.Map; import javax.mail.internet.InternetAddress; import javax.servlet.http.HttpServletRequest; +import org.json.simple.JSONValue; +import org.json.simple.parser.ParseException; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -47,7 +49,7 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testSuccess_updateRegistrarInfo_andSendsNotificationEmail() throws Exception { String expectedEmailBody = loadFile(getClass(), "update_registrar_email.txt"); - action.handleJsonRequest(readJsonFromFile("update_registrar.json")); + action.handleJsonRequest(readJsonFromFile("update_registrar.json", getLastUpdateTime())); verify(rsp, never()).setStatus(anyInt()); verify(emailService).createMessage(); verify(emailService).sendMessage(message); @@ -64,7 +66,8 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testFailure_updateRegistrarInfo_duplicateContacts() throws Exception { Map response = - action.handleJsonRequest(readJsonFromFile("update_registrar_duplicate_contacts.json")); + action.handleJsonRequest( + readJsonFromFile("update_registrar_duplicate_contacts.json", getLastUpdateTime())); assertThat(response).containsEntry("status", "ERROR"); assertThat((String) response.get("message")).startsWith("One email address"); } @@ -89,11 +92,22 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase } @Test - public void testUpdate_emptyJsonObject_errorEmailFieldRequired() throws Exception { + public void testUpdate_emptyJsonObject_errorLastUpdateTimeFieldRequired() throws Exception { Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "args", ImmutableMap.of())); assertThat(response).containsEntry("status", "ERROR"); + assertThat(response).containsEntry("field", "lastUpdateTime"); + assertThat(response).containsEntry("message", "This field is required."); + assertNoTasksEnqueued("sheet"); + } + + @Test + public void testUpdate_noEmail_errorEmailFieldRequired() throws Exception { + Map response = action.handleJsonRequest(ImmutableMap.of( + "op", "update", + "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime()))); + assertThat(response).containsEntry("status", "ERROR"); assertThat(response).containsEntry("field", "emailAddress"); assertThat(response).containsEntry("message", "This field is required."); assertNoTasksEnqueued("sheet"); @@ -105,7 +119,7 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", - "args", ImmutableMap.of())); + "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime()))); assertThat(response).containsEntry("status", "SUCCESS"); } @@ -113,23 +127,53 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase public void testUpdate_badEmail_errorEmailField() throws Exception { Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", - "args", ImmutableMap.of( - "emailAddress", "lolcat"))); + "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime(), "emailAddress", "lolcat"))); assertThat(response).containsEntry("status", "ERROR"); assertThat(response).containsEntry("field", "emailAddress"); assertThat(response).containsEntry("message", "Please enter a valid email address."); assertNoTasksEnqueued("sheet"); } + @Test + public void testPost_nonParsableTime_getsAngry() throws Exception { + Map response = action.handleJsonRequest(ImmutableMap.of( + "op", "update", + "args", ImmutableMap.of("lastUpdateTime", "cookies"))); + assertThat(response).containsEntry("status", "ERROR"); + assertThat(response).containsEntry("field", "lastUpdateTime"); + assertThat(response).containsEntry("message", "Not a valid ISO date-time string."); + assertNoTasksEnqueued("sheet"); + } + @Test public void testPost_nonAsciiCharacters_getsAngry() throws Exception { Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "args", ImmutableMap.of( + "lastUpdateTime", getLastUpdateTime(), "emailAddress", "ヘ(◕。◕ヘ)@example.com"))); assertThat(response).containsEntry("status", "ERROR"); assertThat(response).containsEntry("field", "emailAddress"); assertThat(response).containsEntry("message", "Please only use ASCII-US characters."); assertNoTasksEnqueued("sheet"); } + + private static String getLastUpdateTime() { + return loadRegistrar(CLIENT_ID).getLastUpdateTime().toString(); + } + + static Map readJsonFromFile(String filename, String lastUpdateTime) { + String contents = + loadFile( + RegistrarSettingsActionTestCase.class, + filename, + ImmutableMap.of("LAST_UPDATE_TIME", lastUpdateTime)); + try { + @SuppressWarnings("unchecked") + Map json = (Map) JSONValue.parseWithException(contents); + return json; + } catch (ParseException ex) { + throw new RuntimeException(ex); + } + } } diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 9fb60d424..f0e2ed244 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -19,7 +19,6 @@ import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDispla import static google.registry.security.JsonHttpTestUtils.createJsonPayload; import static google.registry.security.JsonHttpTestUtils.createJsonResponseSupplier; import static google.registry.testing.DatastoreHelper.loadRegistrar; -import static google.registry.testing.TestDataHelper.loadFile; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -50,8 +49,6 @@ import javax.mail.internet.MimeMessage; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.joda.time.DateTime; -import org.json.simple.JSONValue; -import org.json.simple.parser.ParseException; import org.junit.Before; import org.junit.Rule; import org.junit.runner.RunWith; @@ -114,15 +111,4 @@ public class RegistrarSettingsActionTestCase { .thenAnswer(x -> loadRegistrar(CLIENT_ID)); when(modulesService.getVersionHostname("backend", null)).thenReturn("backend.hostname"); } - - static Map readJsonFromFile(String filename) { - String contents = loadFile(RegistrarSettingsActionTestCase.class, filename); - try { - @SuppressWarnings("unchecked") - Map json = (Map) JSONValue.parseWithException(contents); - return json; - } catch (ParseException ex) { - throw new RuntimeException(ex); - } - } } diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json b/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json index 9dc90420a..b15ad1d2a 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json +++ b/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json @@ -4,7 +4,7 @@ "clientIdentifier": "theregistrar", "driveFolderId": null, "registrarName": "The Registrar", - "lastUpdateTime": "2015-01-22T17:27:36.999Z", + "lastUpdateTime": "%LAST_UPDATE_TIME%", "state": "ACTIVE", "type": "REAL", "contacts": [ diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json b/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json index 12e13f466..228eaf39a 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json +++ b/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json @@ -4,7 +4,7 @@ "clientIdentifier": "theregistrar", "driveFolderId": null, "registrarName": "The Registrar", - "lastUpdateTime": "2015-01-22T17:27:36.999Z", + "lastUpdateTime": "%LAST_UPDATE_TIME%", "state": "ACTIVE", "type": "REAL", "contacts": [