Don't destroy existing registry lock passwords in contacts (#317)

* Don't destroy existing registry lock passwords in contacts

The existing code assumes that the "contacts" segment of the form
contains an exact representation of the registrar contacts. This breaks
when we have a contact with an existing registry lock password because
we don't want to keep passing around that password in plain text (we
never store it in plain text)

This PR changes the code so that instead of assuming the contact is
provided in its entirety, we load the contact from storage first
(matching by email address) if it exists. We then set the required
fields from the JSON object, and set the password optionally if it was
provided.

Alternatives:
- Create a separate RegistrarContactPassword object with a
RegistrarContact parent. This increases complexity significantly since
we'd be adding a parent-child relationship and adding more objects to
Datastore during the transition to SQL. It also doesn't completely solve
the problem of "When should we set the password?" because the password
field still must be part of the same form.
- Rearrange the UI so that the password is set as part of a completely
separate form with a separate submit action. This would be possible but
is sub-optimal for two reasons. First, we are trying to not re-engineer
the web console as much as possible since we're likely starting it from
scratch before too long anyway. Second, we want the
lock-password-setting to be part of the standard contact modification
workflow.

* Responses to CR

* Actually we need to allow "removal" of fields

* Remove optional

* one-statement building the contacts

core/src/main/java/google/registry/ui/server/RegistrarFormFields.java

@@ -21,7 +21,9 @@ import static google.registry.util.DomainNameUtils.canonicalizeDomainName;
 
 import com.google.common.base.Ascii;
 import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.net.InternetDomainName;
 import com.google.re2j.Pattern;
 import google.registry.model.registrar.Registrar;
@@ -194,12 +196,10 @@ public final class RegistrarFormFields {
           FormField.named("visibleInDomainWhoisAsAbuse", Boolean.class).build();
 
   public static final FormField<String, String> CONTACT_PHONE_NUMBER_FIELD =
-      FormFields.PHONE_NUMBER.asBuilder()
+      FormFields.PHONE_NUMBER.asBuilder().build();
-          .build();
 
   public static final FormField<String, String> CONTACT_FAX_NUMBER_FIELD =
-      FormFields.PHONE_NUMBER.asBuilderNamed("faxNumber")
+      FormFields.PHONE_NUMBER.asBuilderNamed("faxNumber").build();
-          .build();
 
   public static final FormField<String, String> CONTACT_GAE_USER_ID_FIELD =
       FormFields.NAME.asBuilderNamed("gaeUserId").build();
@@ -217,11 +217,8 @@ public final class RegistrarFormFields {
           .asSet(Splitter.on(',').omitEmptyStrings().trimResults())
           .build();
 
-  public static final FormField<List<Map<String, ?>>, List<RegistrarContact.Builder>>
+  public static final FormField<List<Map<String, ?>>, List<Map<String, ?>>> CONTACTS_AS_MAPS =
-      CONTACTS_FIELD = FormField.mapNamed("contacts")
+      FormField.mapNamed("contacts").asList().build();
-          .transform(RegistrarContact.Builder.class, RegistrarFormFields::toRegistrarContactBuilder)
-          .asList()
-          .build();
 
   public static final FormField<List<String>, List<String>> I18N_STREET_FIELD =
       FormFields.XS_NORMALIZED_STRING.asBuilderNamed("street")
@@ -344,33 +341,60 @@ public final class RegistrarFormFields {
     }
   }
 
-  private static @Nullable RegistrarContact.Builder toRegistrarContactBuilder(
+  public static ImmutableList<RegistrarContact.Builder> getRegistrarContactBuilders(
-      @Nullable Map<String, ?> args) {
+      ImmutableSet<RegistrarContact> existingContacts, @Nullable Map<String, ?> args) {
     if (args == null) {
-      return null;
+      return ImmutableList.of();
     }
-    RegistrarContact.Builder builder = new RegistrarContact.Builder();
+    Optional<List<Map<String, ?>>> contactsAsMaps = CONTACTS_AS_MAPS.extractUntyped(args);
-    CONTACT_NAME_FIELD.extractUntyped(args).ifPresent(builder::setName);
+    if (!contactsAsMaps.isPresent()) {
-    CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).ifPresent(builder::setEmailAddress);
+      return ImmutableList.of();
-    CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD
+    }
-        .extractUntyped(args)
+    ImmutableList.Builder<RegistrarContact.Builder> result = new ImmutableList.Builder<>();
-        .ifPresent(builder::setVisibleInWhoisAsAdmin);
+    for (Map<String, ?> contactAsMap : contactsAsMaps.get()) {
-    CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD
+      String emailAddress =
-        .extractUntyped(args)
+          CONTACT_EMAIL_ADDRESS_FIELD
-        .ifPresent(builder::setVisibleInWhoisAsTech);
+              .extractUntyped(contactAsMap)
-    PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD
+              .orElseThrow(
-        .extractUntyped(args)
+                  () -> new IllegalArgumentException("Contacts from UI must have email addresses"));
-        .ifPresent(builder::setVisibleInDomainWhoisAsAbuse);
+      // Start with a new builder if the contact didn't previously exist
-    CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setPhoneNumber);
+      RegistrarContact.Builder contactBuilder =
-    CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).ifPresent(builder::setFaxNumber);
+          existingContacts.stream()
-    CONTACT_TYPES.extractUntyped(args).ifPresent(builder::setTypes);
+              .filter(rc -> rc.getEmailAddress().equals(emailAddress))
-    CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).ifPresent(builder::setGaeUserId);
+              .findFirst()
-    CONTACT_ALLOWED_TO_SET_REGISTRY_LOCK_PASSWORD
+              .map(RegistrarContact::asBuilder)
-        .extractUntyped(args)
+              .orElse(new RegistrarContact.Builder());
-        .ifPresent(builder::setAllowedToSetRegistryLockPassword);
+      applyRegistrarContactArgs(contactBuilder, contactAsMap);
+      result.add(contactBuilder);
+    }
+    return result.build();
+  }
+
+  private static void applyRegistrarContactArgs(
+      RegistrarContact.Builder builder, Map<String, ?> args) {
+    builder.setName(CONTACT_NAME_FIELD.extractUntyped(args).orElse(null));
+    builder.setEmailAddress(CONTACT_EMAIL_ADDRESS_FIELD.extractUntyped(args).orElse(null));
+    builder.setVisibleInWhoisAsAdmin(
+        CONTACT_VISIBLE_IN_WHOIS_AS_ADMIN_FIELD.extractUntyped(args).orElse(false));
+    builder.setVisibleInWhoisAsTech(
+        CONTACT_VISIBLE_IN_WHOIS_AS_TECH_FIELD.extractUntyped(args).orElse(false));
+    builder.setVisibleInDomainWhoisAsAbuse(
+        PHONE_AND_EMAIL_VISIBLE_IN_DOMAIN_WHOIS_AS_ABUSE_FIELD.extractUntyped(args).orElse(false));
+    builder.setPhoneNumber(CONTACT_PHONE_NUMBER_FIELD.extractUntyped(args).orElse(null));
+    builder.setFaxNumber(CONTACT_FAX_NUMBER_FIELD.extractUntyped(args).orElse(null));
+    builder.setTypes(CONTACT_TYPES.extractUntyped(args).orElse(ImmutableSet.of()));
+    builder.setGaeUserId(CONTACT_GAE_USER_ID_FIELD.extractUntyped(args).orElse(null));
+    builder.setAllowedToSetRegistryLockPassword(
+        CONTACT_ALLOWED_TO_SET_REGISTRY_LOCK_PASSWORD.extractUntyped(args).orElse(false));
+
+    // Registry lock password does not need to be set every time
     CONTACT_REGISTRY_LOCK_PASSWORD_FIELD
         .extractUntyped(args)
-        .ifPresent(builder::setRegistryLockPassword);
+        .ifPresent(
-    return builder;
+            password -> {
+              if (!Strings.isNullOrEmpty(password)) {
+                builder.setRegistryLockPassword(password);
+              }
+            });
   }
 }

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

@@ -60,7 +60,6 @@ import google.registry.util.CollectionUtils;
 import google.registry.util.DiffUtils;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -175,8 +174,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
   }
 
   private RegistrarResult update(final Map<String, ?> args, String clientId) {
-    return tm()
+    return tm().transact(
-        .transact(
             () -> {
               // We load the registrar here rather than outside of the transaction - to make
               // sure we have the latest version. This one is loaded inside the transaction, so it's
@@ -215,7 +213,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
               updatedRegistrar = checkAndUpdateAdminControlledFields(updatedRegistrar, args);
 
               // read the contacts from the request.
-              ImmutableSet<RegistrarContact> updatedContacts = readContacts(registrar, args);
+              ImmutableSet<RegistrarContact> updatedContacts =
+                  readContacts(registrar, contacts, args);
 
               // Save the updated contacts
               if (!updatedContacts.equals(contacts)) {
@@ -384,16 +383,10 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
 
   /** Reads the contacts from the supplied args. */
   public static ImmutableSet<RegistrarContact> readContacts(
-      Registrar registrar, Map<String, ?> args) {
+      Registrar registrar, ImmutableSet<RegistrarContact> existingContacts, Map<String, ?> args) {
-
+    return RegistrarFormFields.getRegistrarContactBuilders(existingContacts, args).stream()
-    ImmutableSet.Builder<RegistrarContact> contacts = new ImmutableSet.Builder<>();
+        .map(builder -> builder.setParent(registrar).build())
-    Optional<List<RegistrarContact.Builder>> builders =
+        .collect(toImmutableSet());
-        RegistrarFormFields.CONTACTS_FIELD.extractUntyped(args);
-    if (builders.isPresent()) {
-      builders.get().forEach(c -> contacts.add(c.setParent(registrar).build()));
-    }
-
-    return contacts.build();
   }
 
   /**
@@ -459,8 +452,17 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
                     () ->
                         new FormException(
                             "Not allowed to set registry lock password directly on new contact"));
-        if (!existingContact.isAllowedToSetRegistryLockPassword()) {
+        if (updatedContact.isRegistryLockAllowed()) {
-          throw new FormException("Registrar contact not allowed to set registry lock password");
+          // the password must have been set before or the user was allowed to set it now
+          if (!existingContact.isAllowedToSetRegistryLockPassword()
+              && !existingContact.isRegistryLockAllowed()) {
+            throw new FormException("Registrar contact not allowed to set registry lock password");
+          }
+        }
+        if (updatedContact.isAllowedToSetRegistryLockPassword()) {
+          if (!existingContact.isAllowedToSetRegistryLockPassword()) {
+            throw new FormException("Cannot set isAllowedToSetRegistryLockPassword through UI");
+          }
         }
       }
     }

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

@@ -188,6 +188,33 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
 
   @Test
   public void testSuccess_setRegistryLockPassword() {
+    addPasswordToTechContact();
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+  }
+
+  @Test
+  public void testSuccess_setRegistryLockPassword_notOverriddenLater() {
+    addPasswordToTechContact();
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+
+    techContact = Iterables.getOnlyElement(loadRegistrar(CLIENT_ID).getContactsOfType(Type.TECH));
+    Map<String, Object> techContactMap = techContact.toJsonMap();
+    techContactMap.put("name", "Some Other Name");
+    Map<String, Object> reqJson = loadRegistrar(CLIENT_ID).toJsonMap();
+    reqJson.put(
+        "contacts",
+        ImmutableList.of(AppEngineRule.makeRegistrarContact2().toJsonMap(), techContactMap));
+    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));
+    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
+  }
+
+  private void addPasswordToTechContact() {
     techContact =
         persistResource(techContact.asBuilder().setAllowedToSetRegistryLockPassword(true).build());
     Map<String, Object> contactMap = techContact.toJsonMap();
@@ -199,9 +226,6 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
     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));
-    assertThat(techContact.verifyRegistryLockPassword("hi")).isTrue();
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
@@ -275,7 +299,7 @@ public class ContactSettingsTest extends RegistrarSettingsActionTestCase {
             "results",
             ImmutableList.of(),
             "message",
-            "Registrar contact not allowed to set registry lock password");
+            "Cannot set isAllowedToSetRegistryLockPassword through UI");
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormException");
   }
 }