diff --git a/gradle/core/build.gradle b/gradle/core/build.gradle index bdf2e17cd..9b36cf148 100644 --- a/gradle/core/build.gradle +++ b/gradle/core/build.gradle @@ -125,9 +125,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/gradle/core/gradle/dependency-locks/testCompileOnly.lockfile b/gradle/core/gradle/dependency-locks/testCompileOnly.lockfile index 656c5dbcc..b308d7773 100644 --- a/gradle/core/gradle/dependency-locks/testCompileOnly.lockfile +++ b/gradle/core/gradle/dependency-locks/testCompileOnly.lockfile @@ -1,3 +1,4 @@ # This is a Gradle generated file for dependency locking. # Manual edits can break the build and are not advised. # This file is expected to be part of source control. +com.google.appengine:appengine-api-1.0-sdk:1.9.48 diff --git a/java/google/registry/model/registrar/Registrar.java b/java/google/registry/model/registrar/Registrar.java index 019ccd549..cfa24ac60 100644 --- a/java/google/registry/model/registrar/Registrar.java +++ b/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; @@ -828,7 +831,7 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } public Builder setEmailAddress(String emailAddress) { - getInstance().emailAddress = emailAddress; + getInstance().emailAddress = checkValidEmail(emailAddress); return this; } @@ -848,7 +851,7 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } public Builder setIcannReferralEmail(String icannReferralEmail) { - getInstance().icannReferralEmail = icannReferralEmail; + getInstance().icannReferralEmail = checkValidEmail(icannReferralEmail); return this; } @@ -891,6 +894,20 @@ 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/java/google/registry/model/registrar/RegistrarContact.java b/java/google/registry/model/registrar/RegistrarContact.java index 3faef7fbf..b7ff0d85f 100644 --- a/java/google/registry/model/registrar/RegistrarContact.java +++ b/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/javatests/google/registry/model/registrar/RegistrarTest.java b/javatests/google/registry/model/registrar/RegistrarTest.java index 21d4041f3..9fa471426 100644 --- a/javatests/google/registry/model/registrar/RegistrarTest.java +++ b/javatests/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/javatests/google/registry/tools/CreateRegistrarCommandTest.java b/javatests/google/registry/tools/CreateRegistrarCommandTest.java index 1f5b3f9ba..e978bdfa1 100644 --- a/javatests/google/registry/tools/CreateRegistrarCommandTest.java +++ b/javatests/google/registry/tools/CreateRegistrarCommandTest.java @@ -1772,4 +1772,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/javatests/google/registry/tools/RegistrarContactCommandTest.java b/javatests/google/registry/tools/RegistrarContactCommandTest.java index 9b007ef07..12b04b8ea 100644 --- a/javatests/google/registry/tools/RegistrarContactCommandTest.java +++ b/javatests/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/javatests/google/registry/tools/UpdateRegistrarCommandTest.java b/javatests/google/registry/tools/UpdateRegistrarCommandTest.java index 6f458926a..aab7aa5c0 100644 --- a/javatests/google/registry/tools/UpdateRegistrarCommandTest.java +++ b/javatests/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/javatests/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java b/javatests/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java index a08278d23..1afd70a72 100644 --- a/javatests/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java +++ b/javatests/google/registry/ui/server/registrar/ConsoleRegistrarCreatorActionTest.java @@ -405,6 +405,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/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index cfbe6c122..97b9c0379 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -149,24 +149,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();