Remove email-editing footgun (#503)

* Remove email-editing footgun

Email address is used as the primary key so we should be very careful
about changing it. This will have even more importance when this is the
location to which we will be sending registry lock confirmation emails.

Note: we allow addition or removal of contacts through the UI (and don't
want to disable that) and because all edits are performed by saving the
entire list of contacts, we can't explicitly prevent all possible edits
of email address in the backend. So this doesn't technically prevent
anything security-wise, but it makes it much more difficult to
accidentally edit an email when you shouldn't.

* Enforce non-deletion of registry-lock-enabled contacts

* Fix tests

* Specify contact

core/src/main/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java

@@ -391,7 +391,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
    * @throws FormException if the checks fail.
    */
   void checkContactRequirements(
-      Set<RegistrarContact> existingContacts, Set<RegistrarContact> updatedContacts) {
+      ImmutableSet<RegistrarContact> existingContacts,
+      ImmutableSet<RegistrarContact> updatedContacts) {
     // Check that no two contacts use the same email address.
     Set<String> emails = new HashSet<>();
     for (RegistrarContact contact : updatedContacts) {
@@ -435,7 +436,12 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
       throw new ContactRequirementException(
           "An abuse contact visible in domain WHOIS query must be designated");
     }
+    checkContactRegistryLockRequirements(existingContacts, updatedContacts);
+  }
 
+  private static void checkContactRegistryLockRequirements(
+      ImmutableSet<RegistrarContact> existingContacts,
+      ImmutableSet<RegistrarContact> updatedContacts) {
     // Any contact(s) with new passwords must be allowed to set them
     for (RegistrarContact updatedContact : updatedContacts) {
       if (updatedContact.isRegistryLockAllowed()
@@ -463,6 +469,31 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
         }
       }
     }
+
+    // Any previously-existing contacts with registry lock enabled cannot be deleted
+    existingContacts.stream()
+        .filter(RegistrarContact::isRegistryLockAllowed)
+        .forEach(
+            contact -> {
+              Optional<RegistrarContact> updatedContactOptional =
+                  updatedContacts.stream()
+                      .filter(
+                          updatedContact ->
+                              updatedContact.getEmailAddress().equals(contact.getEmailAddress()))
+                      .findFirst();
+              if (!updatedContactOptional.isPresent()) {
+                throw new FormException(
+                    String.format(
+                        "Cannot delete the contact %s that has registry lock enabled",
+                        contact.getEmailAddress()));
+              }
+              if (!updatedContactOptional.get().isRegistryLockAllowed()) {
+                throw new FormException(
+                    String.format(
+                        "Cannot remove the ability to use registry lock on the contact %s",
+                        contact.getEmailAddress()));
+              }
+            });
   }
 
   /**

core/src/main/resources/google/registry/ui/soy/registrar/ContactSettings.soy

@@ -145,6 +145,7 @@
         {param label: 'Email' /}
         {param namePrefix: $namePrefix /}
         {param name: 'emailAddress' /}
+        {param disabled: not $readonly and $item['emailAddress'] != null /}
       {/call}
       {call registry.soy.forms.inputFieldRow data="all"}
         {param label: 'Phone' /}

core/src/test/java/google/registry/ui/server/registrar/ContactSettingsTest.java

@@ -66,13 +66,12 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
 
   @Test
   public void testPost_updateContacts_success() throws Exception {
-    // Remove all the contacts but the first by updating with list of
+    // Remove all the contacts but one by updating with a list of just it
-    // just it.
     ImmutableMap<String, String> adminContact1 =
         ImmutableMap.of(
-            "name", "contact1",
+            "name", "Marla Singer",
-            "emailAddress", "contact1@email.com",
+            "emailAddress", "Marla.Singer@crr.com",
-            "phoneNumber", "+1.2125650001",
+            "phoneNumber", "+1.2128675309",
             // Have to keep ADMIN or else expect FormException for at-least-one.
             "types", "ADMIN,TECH");
 
@@ -83,15 +82,12 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", regMap));
     assertThat(response).containsEntry("status", "SUCCESS");
 
-    RegistrarContact newContact =
+    RegistrarContact foundContact =
-        new RegistrarContact.Builder()
+        Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContacts());
-            .setParent(registrar)
+    assertThat(foundContact.getName()).isEqualTo(adminContact1.get("name"));
-            .setName(adminContact1.get("name"))
+    assertThat(foundContact.getEmailAddress()).isEqualTo(adminContact1.get("emailAddress"));
-            .setEmailAddress(adminContact1.get("emailAddress"))
+    assertThat(foundContact.getPhoneNumber()).isEqualTo(adminContact1.get("phoneNumber"));
-            .setPhoneNumber(adminContact1.get("phoneNumber"))
+    assertThat(foundContact.getTypes()).containsExactly(Type.ADMIN, Type.TECH);
-            .setTypes(ImmutableList.of(Type.ADMIN, Type.TECH))
-            .build();
-    assertThat(loadRegistrar(CLIENT_ID).getContacts()).containsExactly(newContact);
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
     verifyNotificationEmailsSent();
   }
@@ -188,41 +184,65 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
 
   @Test
   public void testSuccess_setRegistryLockPassword() {
-    addPasswordToTechContact();
+    addPasswordToContactTwo();
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    String emailAddress = AppEngineRule.makeRegistrarContact2().getEmailAddress();
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+    RegistrarContact newContactWithPassword =
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();
     assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testSuccess_setRegistryLockPassword_notOverriddenLater() {
-    addPasswordToTechContact();
+    addPasswordToContactTwo();
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    String emailAddress = AppEngineRule.makeRegistrarContact2().getEmailAddress();
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+    RegistrarContact newContactWithPassword =
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();
 
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    Map<String, Object> newContactMap = newContactWithPassword.toJsonMap();
-    Map<String, Object> techContactMap = techContact.toJsonMap();
+    newContactMap.put("name", "Some Other Name");
-    techContactMap.put("name", "Some Other Name");
     Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
     reqJson.put(
         "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), techContactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            newContactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));
     Map<String, Object> response =
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
     assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
-    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    newContactWithPassword =
-    assertThat(techContact.verifyRegistryLockPassword("hellothere")).isTrue();
+        loadRegistrar(CLIENT_ID).getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals(emailAddress))
+            .findFirst()
+            .get();
+    assertThat(newContactWithPassword.verifyRegistryLockPassword("hellothere")).isTrue();
   }
 
-  private void addPasswordToTechContact() {
+  private void addPasswordToContactTwo() {
-    techContact =
+    RegistrarContact contact =
-        persistResource(techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build());
+        persistResource(
-    Map<String, Object> contactMap = techContact.toJsonMap();
+            AppEngineRule.makeRegistrarContact2()
+                .asBuilder()
+                .setRegistryLockEmailAddress("johndoe@theregistrar.com")
+                .setAllowedToSetRegistryLockPassword(true)
+                .build());
+    Map<String, Object> contactMap = contact.toJsonMap();
     contactMap.put("registryLockPassword", "hellothere");
     Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
     reqJson.put(
         "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), contactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            contactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));
     Map<String, Object> response =
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
     assertThat(response).containsAtLeastEntriesIn(ImmutableMap.of("status", "SUCCESS"));
@@ -260,13 +280,16 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
   public void testPost_failure_setRegistryLockPassword_notAllowed() {
     // "allowedToSetRegistryLockPassword" must be set through the back end first
     // before we can set a password through the UI
-    Map<String, Object> contactMap =
+    Map<String, Object> contactMap = AppEngineRule.makeRegistrarContact2().toJsonMap();
-        techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build().toJsonMap();
+    contactMap.put("allowedToSetRegistryLockPassword", true);
     contactMap.put("registryLockPassword", "hellothere");
     Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
     reqJson.put(
         "contacts",
-        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), contactMap));
+        ImmutableList.of(
+            AppEngineRule.makeRegistrarContact1().toJsonMap(),
+            contactMap,
+            AppEngineRule.makeRegistrarContact3().toJsonMap()));
 
     Map<String, Object> response =
         action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", reqJson));
@@ -304,6 +327,30 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
   }
 
+  @Test
+  public void testPost_failure_removingRegistryLockContact() {
+    ImmutableMap<String, String> contact =
+        ImmutableMap.of(
+            "name", "contact1",
+            "emailAddress", "contact1@email.com",
+            "phoneNumber", "+1.2125650001",
+            // Have to keep ADMIN or else expect FormException for at-least-one.
+            "types", "ADMIN,TECH");
+    Map<String, Object> regMap = loadRegistrar(CLIENT_ID).toJsonMap();
+    regMap.put("contacts", ImmutableList.of(contact));
+    Map<String, Object> response =
+        action.handleJsonRequest(ImmutableMap.of("op", "update", "id", CLIENT_ID, "args", regMap));
+    assertThat(response)
+        .containsExactly(
+            "status",
+            "ERROR",
+            "results",
+            ImmutableList.of(),
+            "message",
+            "Cannot delete the contact Marla.Singer@crr.com that has registry lock enabled");
+    assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
+  }
+
   @Test
   public void testPost_failure_setRegistryLock_passwordTooShort() {
     techContact =

core/src/test/java/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -14,6 +14,7 @@
 
 package google.registry.ui.server.registrar;
 
+import static com.google.appengine.repackaged.com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.monitoring.metrics.contrib.LongMetricSubject.assertThat;
 import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailAddress;
 import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDisplayName;
@@ -23,14 +24,12 @@ import static google.registry.security.JsonHttpTestUtils.createJsonPayload;
 import static google.registry.testing.DatastoreHelper.createTlds;
 import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;
-import static google.registry.testing.DatastoreHelper.persistResource;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.appengine.api.users.User;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.truth.Truth;
 import google.registry.model.ofy.Ofy;
@@ -95,21 +94,13 @@ public abstract class RegistrarSettingsActionTestCase {
     // Add a technical contact to the registrar (in addition to the default admin contact created by
     // AppEngineRule).
     techContact =
-        persistResource(
+        getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(RegistrarContact.Type.TECH));
-            new RegistrarContact.Builder()
-                .setParent(loadRegistrar(CLIENT_ID))
-                .setName("Jian-Yang")
-                .setEmailAddress("jyang@bachman.accelerator")
-                .setRegistryLockEmailAddress("jyang.registrylock@bachman.accelerator")
-                .setPhoneNumber("+1.1234567890")
-                .setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
-                .build());
 
     action.registrarAccessor = null;
     action.appEngineServiceUtils = appEngineServiceUtils;
     when(appEngineServiceUtils.getCurrentVersionHostname("backend")).thenReturn("backend.hostname");
-    action.jsonActionRunner = new JsonActionRunner(
+    action.jsonActionRunner =
-        ImmutableMap.of(), new JsonResponse(new ResponseImpl(rsp)));
+        new JsonActionRunner(ImmutableMap.of(), new JsonResponse(new ResponseImpl(rsp)));
     action.sendEmailUtils =
         new SendEmailUtils(
             getGSuiteOutgoingEmailAddress(),

core/src/test/resources/google/registry/ui/server/registrar/update_registrar.json

@@ -18,6 +18,17 @@
         "emailAddress": "etphonehome@example.com",
         "gaeUserId": null,
         "types": "ADMIN,BILLING,TECH,WHOIS"
+      },
+      {
+        "visibleInWhoisAsAdmin": false,
+        "faxNumber": null,
+        "phoneNumber": "+1.2128675309",
+        "name": "Marla Singer",
+        "visibleInWhoisAsTech": false,
+        "emailAddress": "Marla.Singer@crr.com",
+        "registryLockEmailAddress": "Marla.Singer.RegistryLock@crr.com",
+        "gaeUserId": "12345",
+        "types": "TECH"
       }
     ],
     "allowedTlds": [

core/src/test/resources/google/registry/ui/server/registrar/update_registrar_email.txt

@@ -13,8 +13,7 @@ contacts:
     ADDED:
         {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
     REMOVED:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, registryLockEmailAddress=Marla.Singer.RegistryLock@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=12345, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, registryLockEmailAddress=null, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, registryLockEmailAddress=null, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, registryLockEmailAddress=jyang.registrylock@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
     FINAL CONTENTS:
-        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, registryLockEmailAddress=null, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
+        {parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, registryLockEmailAddress=Marla.Singer.RegistryLock@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=12345, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}