From 8ac8ecf8f6ac86a967c7f34db7d62c8be7312da1 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Mon, 14 Jan 2019 08:27:52 -0800 Subject: [PATCH] Rationalize OT&E client ID validation rules This makes the validation rules much simpler, thus placing less cognitive load on the users of the console and nomulus tool. The changes are: 1. Don't allow hyphens. No real registrars use hyphens in their client IDs, and it's better to reserve these solely as the delimiter between the base client ID and the number representing the environment. 2. Allow the first character to be a number. This has affected multiple real registrars, causing their OT&E and production client IDs to be different. There's no reason for this restriction, as the only reason motivating it was that there are no TLDs that start with a number. However, the OT&E TLDs are created only in sandbox and never have DNS syncing enabled, so this restriction is purposeless. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=229187198 --- .../registry/model/OteAccountBuilder.java | 14 ++++++- .../registry/tools/SetupOteCommand.java | 42 +++++++------------ .../registry/ui/soy/otesetup/Console.soy | 10 +---- .../registry/model/OteAccountBuilderTest.java | 9 +--- .../registry/tools/SetupOteCommandTest.java | 4 +- .../server/registrar/OteStatusActionTest.java | 8 ++-- 6 files changed, 37 insertions(+), 50 deletions(-) diff --git a/java/google/registry/model/OteAccountBuilder.java b/java/google/registry/model/OteAccountBuilder.java index 8ce511a87..5c10742df 100644 --- a/java/google/registry/model/OteAccountBuilder.java +++ b/java/google/registry/model/OteAccountBuilder.java @@ -72,8 +72,18 @@ import org.joda.time.Duration; */ public final class OteAccountBuilder { - // Regex: 3-14 lower-case alphanumeric characters or hyphens, the first of which must be a letter. - private static final Pattern REGISTRAR_PATTERN = Pattern.compile("^[a-z][-a-z0-9]{2,13}$"); + /** + * Validation regex for registrar base client IDs (3-14 lowercase alphanumeric characters). + * + *

The base client ID is appended with numbers to create four different test registrar accounts + * (e.g. reg-1, reg-3, reg-4, reg-5). Registrar client IDs are of type clIDType in eppcom.xsd + * which is limited to 16 characters, hence the limit of 14 here to account for the dash and + * numbers. + * + *

The base client ID is also used to generate the OT&E TLDs, hence the restriction to + * lowercase alphanumeric characters. + */ + private static final Pattern REGISTRAR_PATTERN = Pattern.compile("^[a-z0-9]{3,14}$"); // Durations are short so that registrars can test with quick transfer (etc.) turnaround. private static final Duration SHORT_ADD_GRACE_PERIOD = Duration.standardMinutes(60); diff --git a/java/google/registry/tools/SetupOteCommand.java b/java/google/registry/tools/SetupOteCommand.java index b53bfcc7c..31197cd4f 100644 --- a/java/google/registry/tools/SetupOteCommand.java +++ b/java/google/registry/tools/SetupOteCommand.java @@ -40,51 +40,41 @@ final class SetupOteCommand extends ConfirmingCommand implements CommandWithRemo private static final int PASSWORD_LENGTH = 16; @Parameter( - names = {"-r", "--registrar"}, - description = - "must 1) consist of only lowercase letters, numbers, or hyphens, " - + "2) start with a letter, and 3) be between 3 and 14 characters (inclusive). " - + "We require 1 and 2 since the registrar name will be used to create TLDs," - + "and we require 3 since we append \"-[1234]\" to the name to create client" - + "IDs which are required by the EPP XML schema to be between 3-16 chars.", - required = true - ) + names = {"-r", "--registrar"}, + description = "The registrar client ID, consisting of 3-14 lowercase letters and numbers.", + required = true) private String registrar; @Parameter( - names = {"-w", "--ip_whitelist"}, - description = "comma separated list of IP addreses or CIDR ranges", - required = true - ) + names = {"-w", "--ip_whitelist"}, + description = "Comma-separated list of IP addreses or CIDR ranges.", + required = true) private List ipWhitelist = new ArrayList<>(); @Parameter( names = {"--email"}, description = - "the registrar's account to use for console access. " + "The registrar's account to use for console access. " + "Must be on the registry's G Suite domain.", required = true) private String email; @Parameter( - names = {"-c", "--certfile"}, - description = "full path to cert file in PEM format (best if on local storage)", - validateWith = PathParameter.InputFile.class - ) + names = {"-c", "--certfile"}, + description = "Full path to cert file in PEM format (best if on local storage).", + validateWith = PathParameter.InputFile.class) private Path certFile; @Parameter( - names = {"-h", "--certhash"}, - description = - "Hash of client certificate (SHA256 base64 no padding). Do not use this unless " - + "you want to store ONLY the hash and not the full certificate" - ) + names = {"-h", "--certhash"}, + description = + "Hash of client certificate (SHA256 base64 no padding). Do not use this unless " + + "you want to store ONLY the hash and not the full certificate.") private String certHash; @Parameter( - names = {"--overwrite"}, - description = "whether to replace existing entities if we encounter any, instead of failing" - ) + names = {"--overwrite"}, + description = "Whether to replace existing entities if we encounter any, instead of failing.") private boolean overwrite = false; @Inject diff --git a/java/google/registry/ui/soy/otesetup/Console.soy b/java/google/registry/ui/soy/otesetup/Console.soy index 3a1f41789..81e306aa3 100644 --- a/java/google/registry/ui/soy/otesetup/Console.soy +++ b/java/google/registry/ui/soy/otesetup/Console.soy @@ -116,15 +116,7 @@ {param required: true /} {/call} - Must: -

    -
  1. consist of only lowercase letters, numbers, or hyphens,
    2. -
  2. start with a letter, and
    3. -
  3. be between 3 and 14 characters (inclusive).
    4. -
- We require 1 and 2 since the registrar name will be used to create TLDs, and we - require 3 since we append "-[12345]" to the name to create client IDs which are - required by the EPP XML schema to be between 3-16 chars. + Must consist of 3-14 lower-case letters and numbers. diff --git a/javatests/google/registry/model/OteAccountBuilderTest.java b/javatests/google/registry/model/OteAccountBuilderTest.java index f022df3cd..94347a62d 100644 --- a/javatests/google/registry/model/OteAccountBuilderTest.java +++ b/javatests/google/registry/model/OteAccountBuilderTest.java @@ -192,9 +192,9 @@ public final class OteAccountBuilderTest { public void testCreateOteEntities_invalidClientId_fails() { assertThat( assertThrows( - IllegalArgumentException.class, () -> OteAccountBuilder.forClientId("3blobio"))) + IllegalArgumentException.class, () -> OteAccountBuilder.forClientId("3blo-bio"))) .hasMessageThat() - .isEqualTo("Invalid registrar name: 3blobio"); + .isEqualTo("Invalid registrar name: 3blo-bio"); } @Test @@ -319,11 +319,6 @@ public final class OteAccountBuilderTest { assertThat(OteAccountBuilder.getBaseClientId("myclientid-4")).isEqualTo("myclientid"); } - @Test - public void testGetBaseClientId_multipleDashesValid() { - assertThat(OteAccountBuilder.getBaseClientId("two-dashes-3")).isEqualTo("two-dashes"); - } - @Test public void testGetBaseClientId_invalidInput_malformed() { assertThat( diff --git a/javatests/google/registry/tools/SetupOteCommandTest.java b/javatests/google/registry/tools/SetupOteCommandTest.java index ff4a3f2dc..f544930ef 100644 --- a/javatests/google/registry/tools/SetupOteCommandTest.java +++ b/javatests/google/registry/tools/SetupOteCommandTest.java @@ -324,10 +324,10 @@ public class SetupOteCommandTest extends CommandTestCase { () -> runCommandForced( "--ip_whitelist=1.1.1.1", - "--registrar=3blobio", + "--registrar=3blo-bio", "--email=contact@email.com", "--certfile=" + getCertFilename())); - assertThat(thrown).hasMessageThat().contains("Invalid registrar name: 3blobio"); + assertThat(thrown).hasMessageThat().contains("Invalid registrar name: 3blo-bio"); } @Test diff --git a/javatests/google/registry/ui/server/registrar/OteStatusActionTest.java b/javatests/google/registry/ui/server/registrar/OteStatusActionTest.java index 1e876bf82..29c6ce6fb 100644 --- a/javatests/google/registry/ui/server/registrar/OteStatusActionTest.java +++ b/javatests/google/registry/ui/server/registrar/OteStatusActionTest.java @@ -105,9 +105,9 @@ public final class OteStatusActionTest { @Test public void testFailure_noRegistrar() { - assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "nonexistent-id-3"))) + assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "nonexistent-3"))) .containsExactlyEntriesIn( - errorResultWithMessage("TestUserId doesn't have access to registrar nonexistent-id-3")); + errorResultWithMessage("TestUserId doesn't have access to registrar nonexistent-3")); } @Test @@ -121,10 +121,10 @@ public final class OteStatusActionTest { @Test public void testFailure_malformedRegistrarName() { - assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "bad-client-id"))) + assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "badclient-id"))) .containsExactlyEntriesIn( errorResultWithMessage( - "ID bad-client-id is not one of the OT&E client IDs for base bad-client")); + "ID badclient-id is not one of the OT&E client IDs for base badclient")); } @Test