Enforce nonnegative costs for TLDs

core/src/main/java/google/registry/model/tld/Tld.java

@@ -814,6 +814,55 @@ public Builder asBuilder() {
     return new Builder(clone(this));
   }
 
+  /** Checks the validity of the TLD object, for use during building or deserializing. */
+  public void validateState() {
+    checkArgument(tldStr != null, "No registry TLD specified");
+    // Check for canonical form by converting to an InternetDomainName and then back.
+    checkArgument(
+        InternetDomainName.isValid(tldStr)
+            && tldStr.equals(InternetDomainName.from(tldStr).toString()),
+        "Cannot create registry for TLD that is not a valid, canonical domain name");
+    // Check the validity of all TimedTransitionProperties to ensure that they have values for
+    // START_INSTANT.  The setters above have already checked this for new values, but also check
+    // here to catch cases where we loaded an invalid TimedTransitionProperty from the database
+    // and cloned it into a new builder, to block re-building a Tld in an invalid state.
+    tldStateTransitions.checkValidity();
+    createBillingCostTransitions.checkValidity();
+    renewBillingCostTransitions.checkValidity();
+    eapFeeSchedule.checkValidity();
+    // All costs must be in the expected currency.
+    checkArgumentNotNull(getCurrency(), "Currency must be set");
+    Predicate<Money> currencyCheck =
+        (Money money) -> money.getCurrencyUnit().equals(currency) && money.isPositiveOrZero();
+    checkArgument(
+        currencyCheck.test(getRestoreBillingCost()),
+        "Restore cost is negative or in the wrong currency");
+    checkArgument(
+        currencyCheck.test(getServerStatusChangeBillingCost()),
+        "Server status change cost is negative or in the wrong currency");
+    checkArgument(
+        currencyCheck.test(getRegistryLockOrUnlockBillingCost()),
+        "Registry lock/unlock cost is negative or in the wrong currency");
+    checkArgument(
+        getRenewBillingCostTransitions().values().stream().allMatch(currencyCheck),
+        "Some renew cost(s) are negative or in the wrong currency");
+    checkArgument(
+        getCreateBillingCostTransitions().values().stream().allMatch(currencyCheck),
+        "Some create cost(s) are negative or in the wrong currency");
+    checkArgument(
+        eapFeeSchedule.toValueMap().values().stream().allMatch(currencyCheck),
+        "Some EAP fee cost(s) are negative or in the wrong currency'");
+    checkArgumentNotNull(
+        pricingEngineClassName, "All registries must have a configured pricing engine");
+    checkArgument(
+        dnsWriters != null && !dnsWriters.isEmpty(),
+        "At least one DNS writer must be specified."
+            + " VoidDnsWriter can be used if DNS writing isn't desired");
+    checkArgument(
+        numDnsPublishLocks > 0,
+        "Number of DNS publish locks must be positive. Use 1 for TLD-wide locks.");
+  }
+
   /** A builder for constructing {@link Tld} objects, since they are immutable. */
   public static class Builder extends Buildable.Builder<Tld> {
     public Builder() {}
@@ -966,10 +1015,6 @@ public Builder setCurrency(CurrencyUnit currency) {
 
     public Builder setCreateBillingCostTransitions(
         ImmutableSortedMap<Instant, Money> createCostsMap) {
-      checkArgumentNotNull(createCostsMap, "Create billing costs map cannot be null");
-      checkArgument(
-          createCostsMap.values().stream().allMatch(Money::isPositiveOrZero),
-          "Create billing cost cannot be negative");
       getInstance().createBillingCostTransitions =
           TimedTransitionProperty.fromValueMap(createCostsMap);
       return this;
@@ -1012,28 +1057,19 @@ public Builder setPremiumList(@Nullable PremiumList premiumList) {
     }
 
     public Builder setRestoreBillingCost(Money amount) {
-      checkArgument(amount.isPositiveOrZero(), "restoreBillingCost cannot be negative");
       getInstance().restoreBillingCost = amount;
       return this;
     }
 
     public Builder setRenewBillingCostTransitions(
         ImmutableSortedMap<Instant, Money> renewCostsMap) {
-      checkArgumentNotNull(renewCostsMap, "Renew billing costs map cannot be null");
-      checkArgument(
-          renewCostsMap.values().stream().allMatch(Money::isPositiveOrZero),
-          "Renew billing cost cannot be negative");
       getInstance().renewBillingCostTransitions =
           TimedTransitionProperty.fromValueMap(renewCostsMap);
       return this;
     }
 
     /** Sets the EAP fee schedule for the TLD. */
     public Builder setEapFeeSchedule(ImmutableSortedMap<Instant, Money> eapFeeSchedule) {
-      checkArgumentNotNull(eapFeeSchedule, "EAP fee schedule cannot be null");
-      checkArgument(
-          eapFeeSchedule.values().stream().allMatch(Money::isPositiveOrZero),
-          "EAP fee cannot be negative");
       getInstance().eapFeeSchedule = TimedTransitionProperty.fromValueMap(eapFeeSchedule);
       return this;
     }
@@ -1051,14 +1087,11 @@ public Builder setRoidSuffix(String roidSuffix) {
     }
 
     public Builder setServerStatusChangeBillingCost(Money amount) {
-      checkArgument(
-          amount.isPositiveOrZero(), "Server status change billing cost cannot be negative");
       getInstance().serverStatusChangeBillingCost = amount;
       return this;
     }
 
     public Builder setRegistryLockOrUnlockBillingCost(Money amount) {
-      checkArgument(amount.isPositiveOrZero(), "Registry lock/unlock cost cannot be negative");
       getInstance().registryLockOrUnlockBillingCost = amount;
       return this;
     }
@@ -1120,58 +1153,10 @@ public Builder setBsaEnrollStartTime(Optional<Instant> enrollTime) {
 
     @Override
     public Tld build() {
-      final Tld instance = getInstance();
-      // Pick up the name of the associated TLD from the instance object.
-      String tldName = instance.tldStr;
-      checkArgument(tldName != null, "No registry TLD specified");
-      // Check for canonical form by converting to an InternetDomainName and then back.
-      checkArgument(
-          InternetDomainName.isValid(tldName)
-              && tldName.equals(InternetDomainName.from(tldName).toString()),
-          "Cannot create registry for TLD that is not a valid, canonical domain name");
-      // Check the validity of all TimedTransitionProperties to ensure that they have values for
-      // START_INSTANT.  The setters above have already checked this for new values, but also check
-      // here to catch cases where we loaded an invalid TimedTransitionProperty from the database
-      // and cloned it into a new builder, to block re-building a Tld in an invalid state.
-      instance.tldStateTransitions.checkValidity();
-      instance.createBillingCostTransitions.checkValidity();
-      instance.renewBillingCostTransitions.checkValidity();
-      instance.eapFeeSchedule.checkValidity();
-      // All costs must be in the expected currency.
-      checkArgumentNotNull(instance.getCurrency(), "Currency must be set");
-      checkArgument(
-          instance.getRestoreBillingCost().getCurrencyUnit().equals(instance.currency),
-          "Restore cost must be in the TLD's currency");
-      checkArgument(
-          instance.getServerStatusChangeBillingCost().getCurrencyUnit().equals(instance.currency),
-          "Server status change cost must be in the TLD's currency");
-      checkArgument(
-          instance.getRegistryLockOrUnlockBillingCost().getCurrencyUnit().equals(instance.currency),
-          "Registry lock/unlock cost must be in the TLD's currency");
-      Predicate<Money> currencyCheck =
-          (Money money) -> money.getCurrencyUnit().equals(instance.currency);
-      checkArgument(
-          instance.getRenewBillingCostTransitions().values().stream().allMatch(currencyCheck),
-          "Renew cost must be in the TLD's currency");
-      checkArgument(
-          instance.getCreateBillingCostTransitions().values().stream().allMatch(currencyCheck),
-          "Create cost must be in the TLD's currency");
-      checkArgument(
-          instance.eapFeeSchedule.toValueMap().values().stream().allMatch(currencyCheck),
-          "All EAP fees must be in the TLD's currency");
-      checkArgumentNotNull(
-          instance.pricingEngineClassName, "All registries must have a configured pricing engine");
-      checkArgument(
-          instance.dnsWriters != null && !instance.dnsWriters.isEmpty(),
-          "At least one DNS writer must be specified."
-              + " VoidDnsWriter can be used if DNS writing isn't desired");
       // If not set explicitly, numDnsPublishLocks defaults to 1.
-      instance.setDefaultNumDnsPublishLocks();
-      checkArgument(
-          instance.numDnsPublishLocks > 0,
-          "Number of DNS publish locks must be positive. Use 1 for TLD-wide locks.");
-      instance.tldStr = tldName;
-      instance.tldUnicode = Idn.toUnicode(tldName);
+      getInstance().setDefaultNumDnsPublishLocks();
+      getInstance().validateState();
+      getInstance().tldUnicode = Idn.toUnicode(getInstance().tldStr);
       return super.build();
     }
   }

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

@@ -164,6 +164,8 @@ protected void init() throws Exception {
     if (Boolean.TRUE.equals(breakGlass)) {
       newTld = newTld.asBuilder().setBreakglassMode(true).build();
     }
+    // Enforce any restrictions, e.g. "no negative fees"
+    newTld.validateState();
     stageEntityChange(oldTld, newTld);
   }
 

core/src/test/java/google/registry/model/tld/TldTest.java

@@ -257,7 +257,8 @@ void testSetCreateBillingCostTransitionsNegativeCost() throws Exception {
                     .asBuilder()
                     .setCreateBillingCostTransitions(createCostTransitions)
                     .build());
-    assertThat(thrown.getMessage()).isEqualTo("Create billing cost cannot be negative");
+    assertThat(thrown.getMessage())
+        .isEqualTo("Some create cost(s) are negative or in the wrong currency");
   }
 
   @Test
@@ -614,17 +615,22 @@ void testFailure_negativeRenewBillingCostTransitionValue() {
                 Tld.get("tld")
                     .asBuilder()
                     .setRenewBillingCostTransitions(
-                        ImmutableSortedMap.of(START_INSTANT, Money.of(USD, -42))));
-    assertThat(thrown).hasMessageThat().contains("billing cost cannot be negative");
+                        ImmutableSortedMap.of(START_INSTANT, Money.of(USD, -42)))
+                    .build());
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Some renew cost(s) are negative or in the wrong currency");
   }
 
   @Test
   void testFailure_negativeRestoreBillingCost() {
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(USD, -42)));
-    assertThat(thrown).hasMessageThat().contains("restoreBillingCost cannot be negative");
+            () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(USD, -42)).build());
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Restore cost is negative or in the wrong currency");
   }
 
   @Test
@@ -652,8 +658,14 @@ void testFailure_negativeServerStatusChangeBillingCost() {
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () -> Tld.get("tld").asBuilder().setServerStatusChangeBillingCost(Money.of(USD, -42)));
-    assertThat(thrown).hasMessageThat().contains("billing cost cannot be negative");
+            () ->
+                Tld.get("tld")
+                    .asBuilder()
+                    .setServerStatusChangeBillingCost(Money.of(USD, -42))
+                    .build());
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Server status change cost is negative or in the wrong currency");
   }
 
   @Test
@@ -667,7 +679,9 @@ void testFailure_renewBillingCostTransitionValue_wrongCurrency() {
                     .setRenewBillingCostTransitions(
                         ImmutableSortedMap.of(START_INSTANT, Money.of(EUR, 42)))
                     .build());
-    assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency");
+    assertThat(thrown)
+        .hasMessageThat()
+        .isEqualTo("Some renew cost(s) are negative or in the wrong currency");
   }
 
   @Test
@@ -681,7 +695,7 @@ void testFailure_createBillingCost_wrongCurrency() {
                     .setCreateBillingCostTransitions(
                         ImmutableSortedMap.of(START_INSTANT, Money.of(EUR, 42)))
                     .build());
-    assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency");
+    assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency");
   }
 
   @Test
@@ -690,7 +704,7 @@ void testFailure_restoreBillingCost_wrongCurrency() {
         assertThrows(
             IllegalArgumentException.class,
             () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(EUR, 42)).build());
-    assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency");
+    assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency");
   }
 
   @Test
@@ -703,7 +717,7 @@ void testFailure_serverStatusChangeBillingCost_wrongCurrency() {
                     .asBuilder()
                     .setServerStatusChangeBillingCost(Money.of(EUR, 42))
                     .build());
-    assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency");
+    assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency");
   }
 
   @Test
@@ -742,7 +756,7 @@ void testFailure_eapFee_wrongCurrency() {
                     .asBuilder()
                     .setEapFeeSchedule(ImmutableSortedMap.of(START_INSTANT, Money.zero(EUR)))
                     .build());
-    assertThat(thrown).hasMessageThat().contains("All EAP fees must be in the TLD's currency");
+    assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency");
   }
 
   @Test

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

@@ -454,6 +454,16 @@ void testFailure_mismatchedCurrencyUnitsOnUpdate() throws Exception {
                 + " unit USD. Found [EUR] currency unit(s) in the renewBillingCostTransitionsMap");
   }
 
+  @Test
+  void testFailure_negativeCreateCost() throws Exception {
+    File tldFile = tmpDir.resolve("negativecost.yaml").toFile();
+    Files.asCharSink(tldFile, UTF_8).write(loadFile(getClass(), "negativecost.yaml"));
+    IllegalArgumentException thrown =
+        assertThrows(IllegalArgumentException.class, () -> runCommandForced("--input=" + tldFile));
+    assertThat(thrown.getMessage())
+        .isEqualTo("Some create cost(s) are negative or in the wrong currency");
+  }
+
   @Test
   void testSuccess_emptyStringClearsDefaultPromoTokens() throws Exception {
     Tld tld = createTld("tld");

core/src/test/resources/google/registry/tools/negativecost.yaml

@@ -0,0 +1,58 @@
+addGracePeriodLength: "PT120H"
+allowedFullyQualifiedHostNames: []
+anchorTenantAddGracePeriodLength: "PT720H"
+autoRenewGracePeriodLength: "PT1080H"
+automaticTransferLength: "PT120H"
+claimsPeriodEnd: "294247-01-10T04:00:54.775Z"
+createBillingCostTransitions:
+  "1970-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: 8.00
+  "2020-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: -25.00
+creationTime: "2022-09-01T00:00:00.000Z"
+currency: "USD"
+defaultPromoTokens: []
+dnsAPlusAaaaTtl: "PT15M"
+dnsDsTtl: null
+dnsNsTtl: null
+dnsPaused: false
+dnsWriters:
+- "VoidDnsWriter"
+driveFolderId: "driveFolder"
+eapFeeSchedule:
+  "1970-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: 0.00
+escrowEnabled: false
+idnTables: []
+invoicingEnabled: false
+lordnUsername: null
+numDnsPublishLocks: 1
+pendingDeleteLength: "PT120H"
+premiumListName: "test"
+pricingEngineClassName: "google.registry.model.pricing.StaticPremiumListPricingEngine"
+redemptionGracePeriodLength: "PT720H"
+registryLockOrUnlockBillingCost:
+  currency: "USD"
+  amount: 0.00
+renewBillingCostTransitions:
+  "1970-01-01T00:00:00.000Z":
+    currency: "USD"
+    amount: 11.00
+renewGracePeriodLength: "PT120H"
+reservedListNames: []
+restoreBillingCost:
+  currency: "USD"
+  amount: 17.00
+roidSuffix: "TLD"
+serverStatusChangeBillingCost:
+  currency: "USD"
+  amount: 19.00
+tldStateTransitions:
+  "1970-01-01T00:00:00.000Z": "GENERAL_AVAILABILITY"
+tldStr: "negativecost"
+tldType: "REAL"
+tldUnicode: "negativecost"
+transferGracePeriodLength: "PT120H"

core/src/test/resources/google/registry/tools/nullablefieldsallnull.yaml

@@ -16,6 +16,7 @@ tldStateTransitions:
 creationTime: "2022-09-01T00:00:00.000Z"
 reservedListNames: null
 premiumListName: null
+pricingEngineClassName: "google.registry.model.pricing.StaticPremiumListPricingEngine"
 escrowEnabled: false
 dnsPaused: false
 addGracePeriodLength: "PT720H"