Don't allow a list of the empty string in List<String> fields (#2011)

If the user does, e.g. `--allowed_nameservers=` (or contact ids) that
shouldn't mean a list consisting solely of the empty string.

Using this parameter / converter allows us to ensure that lists of
strings look reasonable.

core/src/main/java/google/registry/tools/CreateOrUpdateDomainCommand.java

@@ -21,6 +21,7 @@ import com.beust.jcommander.Parameter;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableSet;
 import google.registry.tools.params.NameserversParameter;
+import google.registry.tools.params.StringListParameter;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -53,15 +54,15 @@ abstract class CreateOrUpdateDomainCommand extends MutatingEppToolCommand {
   String registrant;
 
   @Parameter(
-    names = {"-a", "--admins"},
-    description = "Comma-separated list of admin contacts."
-  )
+      names = {"-a", "--admins"},
+      description = "Comma-separated list of admin contacts.",
+      listConverter = StringListParameter.class)
   List<String> admins = new ArrayList<>();
 
   @Parameter(
-    names = {"-t", "--techs"},
-    description = "Comma-separated list of technical contacts."
-  )
+      names = {"-t", "--techs"},
+      description = "Comma-separated list of technical contacts.",
+      listConverter = StringListParameter.class)
   List<String> techs = new ArrayList<>();
 
   @Parameter(

core/src/main/java/google/registry/tools/CreateOrUpdateRegistrarCommand.java

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Predicates.isNull;
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.common.base.Verify.verify;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.util.DomainNameUtils.canonicalizeHostname;
 import static google.registry.util.RegistrarUtils.normalizeRegistrarName;
@@ -35,6 +36,7 @@ import google.registry.tools.params.OptionalLongParameter;
 import google.registry.tools.params.OptionalPhoneNumberParameter;
 import google.registry.tools.params.OptionalStringParameter;
 import google.registry.tools.params.PathParameter;
+import google.registry.tools.params.StringListParameter;
 import google.registry.util.CidrAddressBlock;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -70,12 +72,14 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
 
   @Parameter(
       names = "--allowed_tlds",
-      description = "Comma-delimited list of TLDs which the registrar is allowed to use")
+      description = "Comma-delimited list of TLDs which the registrar is allowed to use",
+      listConverter = StringListParameter.class)
   List<String> allowedTlds = new ArrayList<>();
 
   @Parameter(
       names = "--add_allowed_tlds",
-      description = "Comma-delimited list of TLDs to add to TLDs a registrar is allowed to use")
+      description = "Comma-delimited list of TLDs to add to TLDs a registrar is allowed to use",
+      listConverter = StringListParameter.class)
   List<String> addAllowedTlds = new ArrayList<>();
 
   @Nullable
@@ -147,8 +151,9 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
 
   @Parameter(
       names = "--ip_allow_list",
-      description = "Comma-delimited list of IP ranges. An empty string clears the allow list.")
-  List<String> ipAllowList = new ArrayList<>();
+      description = "Comma-delimited list of IP ranges. An empty string clears the allow list.",
+      listConverter = StringListParameter.class)
+  List<String> ipAllowList;
 
   @Nullable
   @Parameter(
@@ -256,7 +261,8 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
       description =
           "Comma-delimited list of RDAP servers. An empty argument clears the list."
               + " Note that for real registrars this could get overridden periodically by"
-              + " ICANN-registered values.")
+              + " ICANN-registered values.",
+      listConverter = StringListParameter.class)
   List<String> rdapServers = new ArrayList<>();
 
   /** Returns the existing registrar (for update) or null (for creates). */
@@ -330,16 +336,9 @@ abstract class CreateOrUpdateRegistrarCommand extends MutatingCommand {
         }
         builder.setAllowedTlds(allowedTldsBuilder.build());
       }
-      if (!ipAllowList.isEmpty()) {
-        ImmutableList.Builder<CidrAddressBlock> ipAllowListBuilder = new ImmutableList.Builder<>();
-        if (!(ipAllowList.size() == 1 && ipAllowList.get(0).contains("null"))) {
-          for (String ipRange : ipAllowList) {
-            if (!ipRange.isEmpty()) {
-              ipAllowListBuilder.add(CidrAddressBlock.create(ipRange));
-            }
-          }
-        }
-        builder.setIpAddressAllowList(ipAllowListBuilder.build());
+      if (ipAllowList != null) {
+        builder.setIpAddressAllowList(
+            ipAllowList.stream().map(CidrAddressBlock::create).collect(toImmutableList()));
       }
       if (clientCertificateFilename != null) {
         String asciiCert = new String(Files.readAllBytes(clientCertificateFilename), US_ASCII);

core/src/main/java/google/registry/tools/CreateOrUpdateTldCommand.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.tools.UpdateOrDeleteAllocationTokensCommand.getTokenKeys;
 import static google.registry.util.CollectionUtils.findDuplicates;
-import static google.registry.util.CollectionUtils.isNullOrEmpty;
 import static google.registry.util.DomainNameUtils.canonicalizeHostname;
 
 import com.beust.jcommander.Parameter;
@@ -37,6 +36,7 @@ import google.registry.model.tld.label.PremiumList;
 import google.registry.model.tld.label.PremiumListDao;
 import google.registry.tldconfig.idn.IdnTableEnum;
 import google.registry.tools.params.OptionalStringParameter;
+import google.registry.tools.params.StringListParameter;
 import google.registry.tools.params.TransitionListParameter.BillingCostTransitions;
 import google.registry.tools.params.TransitionListParameter.TldStateTransitions;
 import java.util.Arrays;
@@ -195,19 +195,22 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
   @Nullable
   @Parameter(
       names = "--reserved_lists",
-      description = "A comma-separated list of reserved list names to be applied to the TLD")
+      description = "A comma-separated list of reserved list names to be applied to the TLD",
+      listConverter = StringListParameter.class)
   List<String> reservedListNames;
 
   @Nullable
   @Parameter(
       names = "--allowed_registrants",
-      description = "A comma-separated list of allowed registrants for the TLD")
+      description = "A comma-separated list of allowed registrants for the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedRegistrants;
 
   @Nullable
   @Parameter(
       names = "--allowed_nameservers",
-      description = "A comma-separated list of allowed nameservers for the TLD")
+      description = "A comma-separated list of allowed nameservers for the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedNameservers;
 
   @Parameter(
@@ -223,8 +226,9 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
 
   @Nullable
   @Parameter(
-    names = "--dns_writers",
-    description = "A comma-separated list of DnsWriter implementations to use")
+      names = "--dns_writers",
+      description = "A comma-separated list of DnsWriter implementations to use",
+      listConverter = StringListParameter.class)
   List<String> dnsWriters;
 
   @Nullable
@@ -262,7 +266,8 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
           "A comma-separated list of default allocation tokens to be applied to the TLD. The"
               + " ordering of this list will determine which token is used in the case where"
               + " multiple tokens are valid for a registration. Use an empty string to clear all"
-              + " present default tokens.")
+              + " present default tokens.",
+      listConverter = StringListParameter.class)
   List<String> defaultTokens;
 
   @Nullable
@@ -271,7 +276,8 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
       description =
           "A comma-separated list of the IDN tables to use for this TLD. Specify an empty list to"
               + " remove any previously-set tables and to use the default. All elements must be"
-              + " IdnTableEnum values")
+              + " IdnTableEnum values",
+      listConverter = StringListParameter.class)
   List<String> idnTables;
 
   /** Returns the existing tld (for update) or null (for creates). */
@@ -420,12 +426,8 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand {
 
       builder.setAllowedFullyQualifiedHostNames(getAllowedNameservers(oldTld));
 
-      if (!isNullOrEmpty(defaultTokens)) {
-        if (defaultTokens.equals(ImmutableList.of(""))) {
-          builder.setDefaultPromoTokens(ImmutableList.of());
-        } else {
-          builder.setDefaultPromoTokens(getTokenKeys(defaultTokens, null));
-        }
+      if (defaultTokens != null) {
+        builder.setDefaultPromoTokens(getTokenKeys(defaultTokens, null));
       }
       if (idnTables != null) {
         if (idnTables.equals(ImmutableList.of(""))) {

core/src/main/java/google/registry/tools/RegistrarPocCommand.java

@@ -32,6 +32,7 @@ import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.RegistrarPoc;
 import google.registry.tools.params.OptionalPhoneNumberParameter;
 import google.registry.tools.params.PathParameter;
+import google.registry.tools.params.StringListParameter;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -72,8 +73,10 @@ final class RegistrarPocCommand extends MutatingCommand {
   @Nullable
   @Parameter(
       names = "--contact_type",
-      description = "Type of communications for this contact; separate multiple with commas."
-          + " Allowed values are ABUSE, ADMIN, BILLING, LEGAL, MARKETING, TECH, WHOIS.")
+      description =
+          "Type of communications for this contact; separate multiple with commas."
+              + " Allowed values are ABUSE, ADMIN, BILLING, LEGAL, MARKETING, TECH, WHOIS.",
+      listConverter = StringListParameter.class)
   private List<String> contactTypeNames;
 
   @Nullable
@@ -177,15 +180,6 @@ final class RegistrarPocCommand extends MutatingCommand {
     // If the contact_type parameter is not specified, we should not make any changes.
     if (contactTypeNames == null) {
       contactTypes = null;
-      // It appears that when the user specifies "--contact_type=" with no types following,
-      // JCommander sets contactTypeNames to a one-element list containing the empty string. This is
-      // strange, but we need to handle this by setting the contact types to the empty set. Also do
-      // this if contactTypeNames is empty, which is what I would hope JCommander would return in
-      // some future, better world.
-    } else //noinspection UnnecessaryParentheses
-    if (contactTypeNames.isEmpty()
-        || (contactTypeNames.size() == 1 && contactTypeNames.get(0).isEmpty())) {
-      contactTypes = ImmutableSet.of();
     } else {
       contactTypes =
           contactTypeNames.stream()

core/src/main/java/google/registry/tools/UpdateAllocationTokensCommand.java

@@ -34,6 +34,7 @@ import google.registry.model.domain.token.AllocationToken;
 import google.registry.model.domain.token.AllocationToken.RegistrationBehavior;
 import google.registry.model.domain.token.AllocationToken.TokenStatus;
 import google.registry.model.domain.token.AllocationToken.TokenType;
+import google.registry.tools.params.StringListParameter;
 import google.registry.tools.params.TransitionListParameter.TokenStatusTransitions;
 import java.util.List;
 import java.util.Map;
@@ -54,21 +55,24 @@ final class UpdateAllocationTokensCommand extends UpdateOrDeleteAllocationTokens
       names = {"--allowed_client_ids"},
       description =
           "Comma-separated list of allowed client IDs. Use the empty string to clear the "
-              + "existing list.")
+              + "existing list.",
+      listConverter = StringListParameter.class)
   private List<String> allowedClientIds;
 
   @Parameter(
       names = {"--allowed_tlds"},
       description =
           "Comma-separated list of allowed TLDs. Use the empty string to clear the "
-              + "existing list.")
+              + "existing list.",
+      listConverter = StringListParameter.class)
   private List<String> allowedTlds;
 
   @Parameter(
       names = {"--allowed_epp_actions"},
       description =
           "Comma-separated list of allowed EPP actions. Use an empty string to clear the existing"
-              + " list.")
+              + " list.",
+      listConverter = StringListParameter.class)
   private List<String> allowedEppActions;
 
   @Parameter(
@@ -128,18 +132,6 @@ final class UpdateAllocationTokensCommand extends UpdateOrDeleteAllocationTokens
 
   @Override
   public void init() {
-    // A single entry with the empty string means that the user passed an empty argument to the
-    // lists, so we should clear them
-    if (ImmutableList.of("").equals(allowedClientIds)) {
-      allowedClientIds = ImmutableList.of();
-    }
-    if (ImmutableList.of("").equals(allowedTlds)) {
-      allowedTlds = ImmutableList.of();
-    }
-    if (ImmutableList.of("").equals(allowedEppActions)) {
-      allowedEppActions = ImmutableList.of();
-    }
-
     if (tokenStatusTransitions != null
         && (tokenStatusTransitions.containsValue(TokenStatus.ENDED)
             || tokenStatusTransitions.containsValue(TokenStatus.CANCELLED))) {

core/src/main/java/google/registry/tools/UpdateTldCommand.java

@@ -28,6 +28,7 @@ import com.google.common.collect.Maps;
 import google.registry.config.RegistryEnvironment;
 import google.registry.model.tld.Tld;
 import google.registry.model.tld.Tld.TldState;
+import google.registry.tools.params.StringListParameter;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -39,40 +40,47 @@ import org.joda.time.DateTimeZone;
 /** Command to update a TLD. */
 @Parameters(separators = " =", commandDescription = "Update existing TLD(s)")
 public class UpdateTldCommand extends CreateOrUpdateTldCommand {
+
   @Nullable
   @Parameter(
       names = "--add_reserved_lists",
-      description = "A comma-separated list of reserved list names to be added to the TLD")
+      description = "A comma-separated list of reserved list names to be added to the TLD",
+      listConverter = StringListParameter.class)
   List<String> reservedListsAdd;
 
   @Nullable
   @Parameter(
       names = "--remove_reserved_lists",
-      description = "A comma-separated list of reserved list names to be removed from the TLD")
+      description = "A comma-separated list of reserved list names to be removed from the TLD",
+      listConverter = StringListParameter.class)
   List<String> reservedListsRemove;
 
   @Nullable
   @Parameter(
       names = "--add_allowed_registrants",
-      description = "A comma-separated list of allowed registrants to be added to the TLD")
+      description = "A comma-separated list of allowed registrants to be added to the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedRegistrantsAdd;
 
   @Nullable
   @Parameter(
       names = "--remove_allowed_registrants",
-      description = "A comma-separated list of allowed registrants to be removed from the TLD")
+      description = "A comma-separated list of allowed registrants to be removed from the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedRegistrantsRemove;
 
   @Nullable
   @Parameter(
       names = "--add_allowed_nameservers",
-      description = "A comma-separated list of allowed nameservers to be added to the TLD")
+      description = "A comma-separated list of allowed nameservers to be added to the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedNameserversAdd;
 
   @Nullable
   @Parameter(
       names = "--remove_allowed_nameservers",
-      description = "A comma-separated list of allowed nameservers to be removed from the TLD")
+      description = "A comma-separated list of allowed nameservers to be removed from the TLD",
+      listConverter = StringListParameter.class)
   List<String> allowedNameserversRemove;
 
   @Nullable

core/src/main/java/google/registry/tools/params/StringListParameter.java

@@ -0,0 +1,37 @@
+// Copyright 2023 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools.params;
+
+import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
+import java.util.List;
+
+/**
+ * Converter for lists of String params that omits any empty strings.
+ *
+ * <p>JCommander automatically parses a comma-separated list well enough, but it parses the empty
+ * list, e.g. "--foo=" as a list consisting solely of the empty string, which is not what we want.
+ */
+public class StringListParameter extends ParameterConverterValidator<List<String>> {
+
+  @Override
+  public List<String> convert(String value) {
+    if (Strings.isNullOrEmpty(value)) {
+      return ImmutableList.of();
+    }
+    return Splitter.on(',').trimResults().omitEmptyStrings().splitToList(value);
+  }
+}

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

@@ -321,28 +321,6 @@ class CreateRegistrarCommandTest extends CommandTestCase<CreateRegistrarCommand>
         .inOrder();
   }
 
-  @Test
-  void testSuccess_ipAllowListFlagNull() throws Exception {
-    runCommandForced(
-        "--name=blobio",
-        "--password=some_password",
-        "--registrar_type=REAL",
-        "--iana_id=8",
-        "--ip_allow_list=null",
-        "--passcode=01234",
-        "--icann_referral_email=foo@bar.test",
-        "--street=\"123 Fake St\"",
-        "--city Fakington",
-        "--state MA",
-        "--zip 00351",
-        "--cc US",
-        "clientz");
-
-    Optional<Registrar> registrar = Registrar.loadByRegistrarId("clientz");
-    assertThat(registrar).isPresent();
-    assertThat(registrar.get().getIpAddressAllowList()).isEmpty();
-  }
-
   @Test
   void testSuccess_clientCertFileFlag() throws Exception {
     fakeClock.setTo(DateTime.parse("2020-11-01T00:00:00Z"));

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

@@ -451,6 +451,16 @@ class CreateTldCommandTest extends CommandTestCase<CreateTldCommand> {
         .containsExactly("alice", "bob");
   }
 
+  @Test
+  void testSuccess_emptyAllowedRegistrants() throws Exception {
+    runCommandForced(
+        "--allowed_registrants=",
+        "--roid_suffix=Q9JYB4C",
+        "--dns_writers=VoidDnsWriter",
+        "xn--q9jyb4c");
+    assertThat(Tld.get("xn--q9jyb4c").getAllowedRegistrantContactIds()).isEmpty();
+  }
+
   @Test
   void testSuccess_setAllowedNameservers() throws Exception {
     runCommandForced(
@@ -462,6 +472,16 @@ class CreateTldCommandTest extends CommandTestCase<CreateTldCommand> {
         .containsExactly("ns1.example.com", "ns2.example.com");
   }
 
+  @Test
+  void testSuccess_emptyAllowedNameservers() throws Exception {
+    runCommandForced(
+        "--allowed_nameservers=",
+        "--roid_suffix=Q9JYB4C",
+        "--dns_writers=FooDnsWriter",
+        "xn--q9jyb4c");
+    assertThat(Tld.get("xn--q9jyb4c").getAllowedFullyQualifiedHostNames()).isEmpty();
+  }
+
   @Test
   void testSuccess_setCommonReservedListOnTld() throws Exception {
     runSuccessfulReservedListsTest("common_abuse");

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

@@ -207,21 +207,6 @@ class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarCommand>
         .inOrder();
   }
 
-  @Test
-  void testSuccess_clearIpAllowList_useNull() throws Exception {
-    persistResource(
-        loadRegistrar("NewRegistrar")
-            .asBuilder()
-            .setIpAddressAllowList(
-                ImmutableList.of(
-                    CidrAddressBlock.create("192.168.1.1"),
-                    CidrAddressBlock.create("192.168.0.2/16")))
-            .build());
-    assertThat(loadRegistrar("NewRegistrar").getIpAddressAllowList()).isNotEmpty();
-    runCommand("--ip_allow_list=null", "--force", "NewRegistrar");
-    assertThat(loadRegistrar("NewRegistrar").getIpAddressAllowList()).isEmpty();
-  }
-
   @Test
   void testSuccess_clearIpAllowList_useEmpty() throws Exception {
     persistResource(

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

@@ -429,6 +429,17 @@ class UpdateTldCommandTest extends CommandTestCase<UpdateTldCommand> {
         .containsExactly("alice", "bob");
   }
 
+  @Test
+  void testSuccess_emptyAllowedRegistrants() throws Exception {
+    persistResource(
+        Tld.get("xn--q9jyb4c")
+            .asBuilder()
+            .setAllowedRegistrantContactIds(ImmutableSet.of("jane", "john"))
+            .build());
+    runCommandForced("--allowed_registrants=", "xn--q9jyb4c");
+    assertThat(Tld.get("xn--q9jyb4c").getAllowedRegistrantContactIds()).isEmpty();
+  }
+
   @Test
   void testSuccess_addAllowedRegistrants() throws Exception {
     persistResource(
@@ -483,6 +494,17 @@ class UpdateTldCommandTest extends CommandTestCase<UpdateTldCommand> {
         .containsExactly("ns1.example.com", "ns2.example.com");
   }
 
+  @Test
+  void testSuccess_emptyAllowedNameservers() throws Exception {
+    persistResource(
+        Tld.get("xn--q9jyb4c")
+            .asBuilder()
+            .setAllowedFullyQualifiedHostNames(ImmutableSet.of("ns1.example.com"))
+            .build());
+    runCommandForced("--allowed_nameservers=", "xn--q9jyb4c");
+    assertThat(Tld.get("xn--q9jyb4c").getAllowedFullyQualifiedHostNames()).isEmpty();
+  }
+
   @Test
   void testSuccess_addAllowedNameservers() throws Exception {
     persistResource(

core/src/test/java/google/registry/tools/params/StringListParameterTest.java

@@ -0,0 +1,45 @@
+// Copyright 2023 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools.params;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.jupiter.api.Test;
+
+/** Tests for {@link StringListParameter}. */
+public class StringListParameterTest {
+
+  private final StringListParameter instance = new StringListParameter();
+
+  @Test
+  void testSingleItem() {
+    assertThat(instance.convert("foo")).containsExactly("foo");
+  }
+
+  @Test
+  void testMultipleItems() {
+    assertThat(instance.convert("foo,bar")).containsExactly("foo", "bar");
+  }
+
+  @Test
+  void testOmitsEmpty() {
+    assertThat(instance.convert("foo,,bar")).containsExactly("foo", "bar");
+  }
+
+  @Test
+  void testEntirelyEmpty() {
+    assertThat(instance.convert("")).isEmpty();
+  }
+}