diff --git a/core/build.gradle b/core/build.gradle index 3799a7161..d5c92a7b3 100644 --- a/core/build.gradle +++ b/core/build.gradle @@ -134,9 +134,7 @@ dependencies { compile deps['com.google.apis:google-api-services-monitoring'] compile deps['com.google.apis:google-api-services-sheets'] maybeRuntime deps['com.google.apis:google-api-services-storage'] - // TODO(b/71631624): change appengine:appengine-api-1.0-sdk to - // testCompileOnly after BillingEmailUtilsTest.java is fixed. - compile deps['com.google.appengine:appengine-api-1.0-sdk'] + testCompileOnly deps['com.google.appengine:appengine-api-1.0-sdk'] maybeRuntime deps['com.google.appengine:appengine-api-labs'] maybeRuntime deps['com.google.appengine:appengine-api-stubs'] testCompile deps['com.google.appengine:appengine-api-stubs'] diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 56d416856..a53ee2e8d 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -16,6 +16,7 @@ package google.registry.model.registrar; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Strings.emptyToNull; import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.collect.ImmutableMap.toImmutableMap; @@ -84,6 +85,8 @@ import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import javax.annotation.Nullable; +import javax.mail.internet.AddressException; +import javax.mail.internet.InternetAddress; import org.joda.money.CurrencyUnit; import org.joda.time.DateTime; @@ -836,7 +839,7 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } public Builder setEmailAddress(String emailAddress) { - getInstance().emailAddress = emailAddress; + getInstance().emailAddress = checkValidEmail(emailAddress); return this; } @@ -861,7 +864,7 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } public Builder setIcannReferralEmail(String icannReferralEmail) { - getInstance().icannReferralEmail = icannReferralEmail; + getInstance().icannReferralEmail = checkValidEmail(icannReferralEmail); return this; } @@ -904,6 +907,19 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } } + /** Verifies that the email address in question is not null and has a valid format. */ + static String checkValidEmail(String email) { + checkNotNull(email, "Provided email was null"); + try { + InternetAddress internetAddress = new InternetAddress(email, true); + internetAddress.validate(); + } catch (AddressException e) { + throw new IllegalArgumentException( + String.format("Provided email %s is not a valid email address", email)); + } + return email; + } + /** Loads all registrar entities directly from Datastore. */ public static Iterable loadAll() { return ImmutableList.copyOf(ofy().load().type(Registrar.class).ancestor(getCrossTldKey())); diff --git a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java b/core/src/main/java/google/registry/model/registrar/RegistrarContact.java index 3faef7fbf..b7ff0d85f 100644 --- a/core/src/main/java/google/registry/model/registrar/RegistrarContact.java +++ b/core/src/main/java/google/registry/model/registrar/RegistrarContact.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Sets.difference; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.registrar.Registrar.checkValidEmail; import static google.registry.util.CollectionUtils.nullToEmptyImmutableSortedCopy; import static java.util.stream.Collectors.joining; @@ -296,7 +297,7 @@ public class RegistrarContact extends ImmutableObject implements Jsonifiable { @Override public RegistrarContact build() { checkNotNull(getInstance().parent, "Registrar parent cannot be null"); - checkNotNull(getInstance().emailAddress, "Email address cannot be null"); + checkValidEmail(getInstance().emailAddress); return cloneEmptyToNull(super.build()); } diff --git a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java index 21d4041f3..9fa471426 100644 --- a/core/src/test/java/google/registry/model/registrar/RegistrarTest.java +++ b/core/src/test/java/google/registry/model/registrar/RegistrarTest.java @@ -452,6 +452,42 @@ public class RegistrarTest extends EntityTestCase { () -> registrar.asBuilder().setAllowedTldsUncached(ImmutableSet.of("bad"))); } + @Test + public void testFailure_nullEmail() { + NullPointerException thrown = + assertThrows(NullPointerException.class, () -> registrar.asBuilder().setEmailAddress(null)); + assertThat(thrown).hasMessageThat().isEqualTo("Provided email was null"); + } + + @Test + public void testFailure_invalidEmail() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, () -> registrar.asBuilder().setEmailAddress("lolcat")); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Provided email lolcat is not a valid email address"); + } + + @Test + public void testFailure_nullIcannReferralEmail() { + NullPointerException thrown = + assertThrows( + NullPointerException.class, () -> registrar.asBuilder().setIcannReferralEmail(null)); + assertThat(thrown).hasMessageThat().isEqualTo("Provided email was null"); + } + + @Test + public void testFailure_invalidIcannEmail() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> registrar.asBuilder().setIcannReferralEmail("lolcat")); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Provided email lolcat is not a valid email address"); + } + @Test public void testSuccess_setAllowedTldsUncached_newTldNotInCache() { int origSingletonCacheRefreshSeconds = diff --git a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java index bcf1780e1..e048ebd04 100644 --- a/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java @@ -1771,4 +1771,28 @@ public class CreateRegistrarCommandTest extends CommandTestCase + runCommandForced( + "--name=blobio", + "--password=some_password", + "--registrar_type=REAL", + "--iana_id=8", + "--passcode=01234", + "--icann_referral_email=lolcat", + "--street=\"123 Fake St\"", + "--city Fakington", + "--state MA", + "--zip 00351", + "--cc US", + "clientz")); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Provided email lolcat is not a valid email address"); + } } diff --git a/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java b/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java index 9b007ef07..12b04b8ea 100644 --- a/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java +++ b/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java @@ -367,4 +367,26 @@ public class RegistrarContactCommandTest extends CommandTestCase + runCommandForced( + "--mode=CREATE", "--name=Jim Doe", "--email=lolcat", "NewRegistrar")); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Provided email lolcat is not a valid email address"); + } + + @Test + public void testCreate_failure_nullEmail() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> runCommandForced("--mode=CREATE", "--name=Jim Doe", "NewRegistrar")); + assertThat(thrown).hasMessageThat().isEqualTo("--email is required when --mode=CREATE"); + } } diff --git a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java index 6f458926a..aab7aa5c0 100644 --- a/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java @@ -815,6 +815,17 @@ public class UpdateRegistrarCommandTest extends CommandTestCase runCommand("--email=lolcat", "--force", "NewRegistrar")); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Provided email lolcat is not a valid email address"); + } + private void persistWhoisAbuseContact() { persistResource( AppEngineRule.makeRegistrarContact1() diff --git a/core/src/test/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java index 2a9d15a8d..826189df5 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java @@ -412,6 +412,27 @@ public final class ConsoleRegistrarCreatorActionTest { assertThat(registrar.getPhonePasscode()).isEqualTo("10203"); } + @Test + public void testPost_badEmailFails() { + action.clientId = Optional.of("myclientid"); + action.name = Optional.of("registrar name"); + action.billingAccount = Optional.of("USD=billing-account"); + action.ianaId = Optional.of(12321); + action.referralEmail = Optional.of("lolcat"); + action.driveId = Optional.of("drive-id"); + action.consoleUserEmail = Optional.of("myclientid@registry.example"); + + action.street1 = Optional.of("my street"); + action.city = Optional.of("my city"); + action.countryCode = Optional.of("CC"); + + action.method = Method.POST; + action.run(); + + assertThat(response.getPayload()) + .contains("Failed: Provided email lolcat is not a valid email address"); + } + @Test public void testPost_unauthorized() { action.registrarAccessor = diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index c6c71ecbf..ddeaf476e 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -151,24 +151,6 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException"); } - @Test - public void testUpdate_emptyJsonObject_emailFieldNotRequiredWhenEmpty() { - persistResource(loadRegistrar(CLIENT_ID).asBuilder().setEmailAddress(null).build()); - Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); - args.remove("emailAddress"); - - Map response = action.handleJsonRequest(ImmutableMap.of( - "op", "update", - "id", CLIENT_ID, - "args", args)); - - assertThat(response).containsExactly( - "status", "SUCCESS", - "message", "Saved TheRegistrar", - "results", ImmutableList.of(loadRegistrar(CLIENT_ID).toJsonMap())); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); - } - @Test public void testFailure_updateRegistrarInfo_notAuthorized() { setUserWithoutAccess();