Always validate domain name on allocation token (#498)

* Always validate domain name on allocation token

This is in response to a client-reported error, where they accidentally sent the
wrong domain name on a domain create that included an allocation token. What
should have happened (and that now happens as of this commit) is an error being
thrown that the allocation token does not match the domain name being created.
What happened instead was that, since the incorrectly submitted domain name was
not reserved, the create succeeded (as it would for all creates of unreserved
domains in GA) and the allocation token was redeemed, which is not what you'd
expect.

* Fix tests to reflect changed check behavior

core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java

@@ -120,6 +120,8 @@ import org.joda.time.Duration;
  * @error {@link
  *     google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotInPromotionException}
  * @error {@link
+ *     google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForDomainException}
+ * @error {@link
  *     google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForRegistrarException}
  * @error {@link
  *     google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForTldException}

core/src/main/java/google/registry/flows/domain/token/AllocationTokenFlowUtils.java

@@ -41,7 +41,7 @@ import org.joda.time.DateTime;
 /** Utility functions for dealing with {@link AllocationToken}s in domain flows. */
 public class AllocationTokenFlowUtils {
 
-  final AllocationTokenCustomLogic tokenCustomLogic;
+  private final AllocationTokenCustomLogic tokenCustomLogic;
 
   @Inject
   AllocationTokenFlowUtils(AllocationTokenCustomLogic tokenCustomLogic) {
@@ -60,7 +60,8 @@ public class AllocationTokenFlowUtils {
       DomainCommand.Create command, String token, Registry registry, String clientId, DateTime now)
       throws EppException {
     AllocationToken tokenEntity = loadToken(token);
-    validateToken(tokenEntity, clientId, registry.getTldStr(), now);
+    validateToken(
+        InternetDomainName.from(command.getFullyQualifiedDomainName()), tokenEntity, clientId, now);
     return tokenCustomLogic.validateToken(command, tokenEntity, registry, clientId, now);
   }
 
@@ -89,7 +90,7 @@ public class AllocationTokenFlowUtils {
     ImmutableMap.Builder<InternetDomainName, String> resultsBuilder = new ImmutableMap.Builder<>();
     for (InternetDomainName domainName : domainNames) {
       try {
-        validateToken(tokenEntity, clientId, domainName.parent().toString(), now);
+        validateToken(domainName, tokenEntity, clientId, now);
         validDomainNames.add(domainName);
       } catch (EppException e) {
         resultsBuilder.put(domainName, e.getMessage());
@@ -120,14 +121,20 @@ public class AllocationTokenFlowUtils {
    *
    * @throws EppException if the token is invalid in any way
    */
-  private void validateToken(AllocationToken token, String clientId, String tld, DateTime now)
+  private void validateToken(
+      InternetDomainName domainName, AllocationToken token, String clientId, DateTime now)
       throws EppException {
     if (!token.getAllowedClientIds().isEmpty() && !token.getAllowedClientIds().contains(clientId)) {
       throw new AllocationTokenNotValidForRegistrarException();
     }
-    if (!token.getAllowedTlds().isEmpty() && !token.getAllowedTlds().contains(tld)) {
+    if (!token.getAllowedTlds().isEmpty()
+        && !token.getAllowedTlds().contains(domainName.parent().toString())) {
       throw new AllocationTokenNotValidForTldException();
     }
+    if (token.getDomainName().isPresent()
+        && !token.getDomainName().get().equals(domainName.toString())) {
+      throw new AllocationTokenNotValidForDomainException();
+    }
     // Tokens without status transitions will just have a single-entry NOT_STARTED map, so only
     // check the status transitions map if it's non-trivial.
     if (token.getTokenStatusTransitions().size() > 1
@@ -159,22 +166,30 @@ public class AllocationTokenFlowUtils {
   /** The allocation token is not currently valid. */
   public static class AllocationTokenNotInPromotionException
       extends StatusProhibitsOperationException {
-    public AllocationTokenNotInPromotionException() {
+    AllocationTokenNotInPromotionException() {
       super("Alloc token not in promo period");
     }
   }
   /** The allocation token is not valid for this TLD. */
   public static class AllocationTokenNotValidForTldException
       extends AssociationProhibitsOperationException {
-    public AllocationTokenNotValidForTldException() {
+    AllocationTokenNotValidForTldException() {
       super("Alloc token invalid for TLD");
     }
   }
 
+  /** The allocation token is not valid for this domain. */
+  public static class AllocationTokenNotValidForDomainException
+      extends AssociationProhibitsOperationException {
+    AllocationTokenNotValidForDomainException() {
+      super("Alloc token invalid for domain");
+    }
+  }
+
   /** The allocation token is not valid for this registrar. */
   public static class AllocationTokenNotValidForRegistrarException
       extends AssociationProhibitsOperationException {
-    public AllocationTokenNotValidForRegistrarException() {
+    AllocationTokenNotValidForRegistrarException() {
       super("Alloc token invalid for client");
     }
   }
@@ -182,14 +197,14 @@ public class AllocationTokenFlowUtils {
   /** The allocation token was already redeemed. */
   public static class AlreadyRedeemedAllocationTokenException
       extends AssociationProhibitsOperationException {
-    public AlreadyRedeemedAllocationTokenException() {
+    AlreadyRedeemedAllocationTokenException() {
       super("Alloc token was already redeemed");
     }
   }
 
   /** The allocation token is invalid. */
   public static class InvalidAllocationTokenException extends AuthorizationErrorException {
-    public InvalidAllocationTokenException() {
+    InvalidAllocationTokenException() {
       super("The allocation token is invalid");
     }
   }

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

@@ -32,9 +32,7 @@ import java.util.List;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
 
-/**
+/** Shared base class for commands to registry lock or unlock a domain via EPP. */
- * Shared base class for commands to registry lock or unlock a domain via EPP.
- */
 public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand
     implements CommandWithRemoteApi {
 
@@ -59,8 +57,7 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand
   @Config("registryAdminClientId")
   String registryAdminClientId;
 
-  @Inject
+  @Inject DomainLockUtils domainLockUtils;
-  DomainLockUtils domainLockUtils;
 
   protected ImmutableSet<String> getDomains() {
     return ImmutableSet.copyOf(mainParameters);
@@ -83,21 +80,26 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand
     ImmutableSet.Builder<String> successfulDomainsBuilder = new ImmutableSet.Builder<>();
     ImmutableSet.Builder<String> skippedDomainsBuilder = new ImmutableSet.Builder<>();
     ImmutableSet.Builder<String> failedDomainsBuilder = new ImmutableSet.Builder<>();
-    partition(getDomains(), BATCH_SIZE).forEach(batch -> tm().transact(() -> {
+    partition(getDomains(), BATCH_SIZE)
-      for (String domain : batch) {
+        .forEach(
-        if (shouldApplyToDomain(domain, tm().getTransactionTime())) {
+            batch ->
-          try {
+                tm().transact(
-            createAndApplyRequest(domain);
+                        () -> {
-          } catch (Throwable t) {
+                          for (String domain : batch) {
-            logger.atSevere().withCause(t).log("Error when (un)locking domain %s.", domain);
+                            if (shouldApplyToDomain(domain, tm().getTransactionTime())) {
-            failedDomainsBuilder.add(domain);
+                              try {
-          }
+                                createAndApplyRequest(domain);
-          successfulDomainsBuilder.add(domain);
+                              } catch (Throwable t) {
-        } else {
+                                logger.atSevere().withCause(t).log(
-          skippedDomainsBuilder.add(domain);
+                                    "Error when (un)locking domain %s.", domain);
-        }
+                                failedDomainsBuilder.add(domain);
-      }
+                              }
-    }));
+                              successfulDomainsBuilder.add(domain);
+                            } else {
+                              skippedDomainsBuilder.add(domain);
+                            }
+                          }
+                        }));
     ImmutableSet<String> successfulDomains = successfulDomainsBuilder.build();
     ImmutableSet<String> skippedDomains = skippedDomainsBuilder.build();
     ImmutableSet<String> failedDomains = failedDomainsBuilder.build();

core/src/main/java/google/registry/tools/javascrap/BackfillRegistryLocksCommand.java

@@ -144,12 +144,14 @@ public class BackfillRegistryLocksCommand extends ConfirmingCommand
 
   private ImmutableList<DomainBase> getLockedDomainsWithoutLocks(DateTime now) {
     return ImmutableList.copyOf(
-        ofy().load()
+        ofy()
+            .load()
             .keys(
                 roids.stream()
                     .map(roid -> Key.create(DomainBase.class, roid))
                     .collect(toImmutableList()))
-            .values().stream()
+            .values()
+            .stream()
             .filter(d -> d.getDeletionTime().isAfter(now))
             .filter(d -> d.getStatusValues().containsAll(REGISTRY_LOCK_STATUSES))
             .filter(d -> !RegistryLockDao.getMostRecentByRepoId(d.getRepoId()).isPresent())

core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java

@@ -186,7 +186,7 @@ public class DomainCheckFlowTest extends ResourceCheckFlowTestCase<DomainCheckFl
             .build());
     doCheckTest(
         create(false, "example1.tld", "In use"),
-        create(true, "example2.tld", null),
+        create(false, "example2.tld", "Alloc token invalid for domain"),
         create(false, "reserved.tld", "Reserved"),
         create(true, "specificuse.tld", null));
   }
@@ -203,7 +203,7 @@ public class DomainCheckFlowTest extends ResourceCheckFlowTestCase<DomainCheckFl
             .build());
     doCheckTest(
         create(false, "example1.tld", "In use"),
-        create(true, "example2.tld", null),
+        create(false, "example2.tld", "Alloc token invalid for domain"),
         create(false, "reserved.tld", "Reserved"),
         create(false, "specificuse.tld", "Allocation token required"));
   }
@@ -224,8 +224,8 @@ public class DomainCheckFlowTest extends ResourceCheckFlowTestCase<DomainCheckFl
                     .build())
             .build());
     doCheckTest(
-        create(true, "example1.tld", null),
+        create(false, "example1.tld", "Alloc token invalid for domain"),
-        create(true, "example2.tld", null),
+        create(false, "example2.tld", "Alloc token invalid for domain"),
         create(false, "reserved.tld", "Reserved"),
         create(true, "specificuse.tld", null));
   }
@@ -246,8 +246,8 @@ public class DomainCheckFlowTest extends ResourceCheckFlowTestCase<DomainCheckFl
                     .build())
             .build());
     doCheckTest(
-        create(false, "example1.tld", "Alloc token not in promo period"),
+        create(false, "example1.tld", "Alloc token invalid for domain"),
-        create(false, "example2.tld", "Alloc token not in promo period"),
+        create(false, "example2.tld", "Alloc token invalid for domain"),
         create(false, "reserved.tld", "Reserved"),
         create(false, "specificuse.tld", "Alloc token not in promo period"));
   }

core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java

@@ -131,6 +131,7 @@ import google.registry.flows.domain.DomainFlowUtils.UnsupportedFeeAttributeExcep
 import google.registry.flows.domain.DomainFlowUtils.UnsupportedMarkTypeException;
 import google.registry.flows.domain.DomainPricingLogic.AllocationTokenInvalidForPremiumNameException;
 import google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotInPromotionException;
+import google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForDomainException;
 import google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForRegistrarException;
 import google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForTldException;
 import google.registry.flows.domain.token.AllocationTokenFlowUtils.AlreadyRedeemedAllocationTokenException;
@@ -440,6 +441,44 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow,
     assertAboutEppExceptions().that(thrown).marshalsToXml();
   }
 
+  @Test
+  public void testFailure_reservedDomainCreate_allocationTokenIsForADifferentDomain() {
+    // Try to register a reserved domain name with an allocation token valid for a different domain
+    // name.
+    setEppInput("domain_create_allocationtoken.xml", ImmutableMap.of("DOMAIN", "resdom.tld"));
+    persistContactsAndHosts();
+    persistResource(
+        new AllocationToken.Builder()
+            .setToken("abc123")
+            .setTokenType(SINGLE_USE)
+            .setDomainName("otherdomain.tld")
+            .build());
+    clock.advanceOneMilli();
+    EppException thrown =
+        assertThrows(AllocationTokenNotValidForDomainException.class, this::runFlow);
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+    assertAllocationTokenWasNotRedeemed("abc123");
+  }
+
+  @Test
+  public void testFailure_nonreservedDomainCreate_allocationTokenIsForADifferentDomain() {
+    // Try to register a non-reserved domain name with an allocation token valid for a different
+    // domain name.
+    setEppInput("domain_create_allocationtoken.xml", ImmutableMap.of("DOMAIN", "example.tld"));
+    persistContactsAndHosts();
+    persistResource(
+        new AllocationToken.Builder()
+            .setToken("abc123")
+            .setTokenType(SINGLE_USE)
+            .setDomainName("otherdomain.tld")
+            .build());
+    clock.advanceOneMilli();
+    EppException thrown =
+        assertThrows(AllocationTokenNotValidForDomainException.class, this::runFlow);
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+    assertAllocationTokenWasNotRedeemed("abc123");
+  }
+
   @Test
   public void testFailure_alreadyRedemeedAllocationToken() {
     setEppInput("domain_create_allocationtoken.xml", ImmutableMap.of("DOMAIN", "example.tld"));
@@ -1212,6 +1251,12 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase<DomainCreateFlow,
         .isEqualTo(Key.create(getHistoryEntries(reloadResourceByForeignKey()).get(0)));
   }
 
+  private void assertAllocationTokenWasNotRedeemed(String token) {
+    AllocationToken reloadedToken =
+        ofy().load().key(Key.create(AllocationToken.class, token)).now();
+    assertThat(reloadedToken.isRedeemed()).isFalse();
+  }
+
   @Test
   public void testSuccess_allocationTokenPromotion() throws Exception {
     // A discount of 0.5 means that the first-year cost (13) is cut in half, so a discount of 6.5

docs/flows.md

@@ -357,6 +357,7 @@ An EPP flow that creates a new domain resource.
     *   Registrant is not whitelisted for this TLD.
     *   Requested domain does not require a claims notice.
 *   2305
+    *   The allocation token is not valid for this domain.
     *   The allocation token is not valid for this registrar.
     *   The allocation token is not valid for this TLD.
     *   The allocation token was already redeemed.