Merge pull request #91 from google/gb/validateEmails

Validate provided email addresses when creating a registrar

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 deps['com.google.appengine:appengine-api-1.0-sdk']
-  // testCompileOnly after BillingEmailUtilsTest.java is fixed.
-  compile 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']

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<Registrar> loadAll() {
     return ImmutableList.copyOf(ofy().load().type(Registrar.class).ancestor(getCrossTldKey()));

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());
     }
 

core/src/test/java/google/registry/model/registrar/RegistrarTest.java

@@ -452,6 +452,62 @@ 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_emptyEmail() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class, () -> registrar.asBuilder().setEmailAddress(""));
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Provided email  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_invalidIcannReferralEmail() {
+    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 testFailure_emptyIcannReferralEmail() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class, () -> registrar.asBuilder().setEmailAddress(""));
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Provided email  is not a valid email address");
+  }
+
   @Test
   public void testSuccess_setAllowedTldsUncached_newTldNotInCache() {
     int origSingletonCacheRefreshSeconds =

core/src/test/java/google/registry/tools/CreateRegistrarCommandTest.java

@@ -1771,4 +1771,28 @@ public class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarC
                 "--cc US",
                 "clientz"));
   }
+
+  @Test
+  public void testFailure_badEmail() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                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");
+  }
 }

core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java

@@ -367,4 +367,26 @@ public class RegistrarContactCommandTest extends CommandTestCase<RegistrarContac
         "--mode=CREATE", "--name=Jim Doe", "--email=jim.doe@example.com", "NewRegistrar");
     assertThat(loadRegistrar("NewRegistrar").getContactsRequireSyncing()).isTrue();
   }
+
+  @Test
+  public void testCreate_failure_badEmail() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () ->
+                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");
+  }
 }

core/src/test/java/google/registry/tools/UpdateRegistrarCommandTest.java

@@ -815,6 +815,17 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
     assertThat(loadRegistrar("NewRegistrar").getPoNumber()).isEmpty();
   }
 
+  @Test
+  public void testFailure_badEmail() {
+    IllegalArgumentException thrown =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> runCommand("--email=lolcat", "--force", "NewRegistrar"));
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Provided email lolcat is not a valid email address");
+  }
+
   private void persistWhoisAbuseContact() {
     persistResource(
         AppEngineRule.makeRegistrarContact1()

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 =

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<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
-    args.remove("emailAddress");
-
-    Map<String, Object> 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();