From 9283cd263fe2faf8d7dc25ac5bf50f7f135ec579 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 27 Feb 2020 16:48:37 -0500 Subject: [PATCH] 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 --- .../flows/domain/DomainCreateFlow.java | 2 + .../token/AllocationTokenFlowUtils.java | 35 ++++++++++----- .../tools/LockOrUnlockDomainCommand.java | 42 ++++++++--------- .../BackfillRegistryLocksCommand.java | 6 ++- .../flows/domain/DomainCheckFlowTest.java | 12 ++--- .../flows/domain/DomainCreateFlowTest.java | 45 +++++++++++++++++++ docs/flows.md | 1 + 7 files changed, 105 insertions(+), 38 deletions(-) diff --git a/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java b/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java index 87eb664cf..053584093 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java +++ b/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} diff --git a/core/src/main/java/google/registry/flows/domain/token/AllocationTokenFlowUtils.java b/core/src/main/java/google/registry/flows/domain/token/AllocationTokenFlowUtils.java index 2b687e3a0..848396a43 100644 --- a/core/src/main/java/google/registry/flows/domain/token/AllocationTokenFlowUtils.java +++ b/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 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"); } } diff --git a/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java b/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java index 80bb1af7e..67d7c0382 100644 --- a/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java +++ b/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 - DomainLockUtils domainLockUtils; + @Inject DomainLockUtils domainLockUtils; protected ImmutableSet getDomains() { return ImmutableSet.copyOf(mainParameters); @@ -83,21 +80,26 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand ImmutableSet.Builder successfulDomainsBuilder = new ImmutableSet.Builder<>(); ImmutableSet.Builder skippedDomainsBuilder = new ImmutableSet.Builder<>(); ImmutableSet.Builder failedDomainsBuilder = new ImmutableSet.Builder<>(); - partition(getDomains(), BATCH_SIZE).forEach(batch -> tm().transact(() -> { - for (String domain : batch) { - if (shouldApplyToDomain(domain, tm().getTransactionTime())) { - try { - createAndApplyRequest(domain); - } catch (Throwable t) { - logger.atSevere().withCause(t).log("Error when (un)locking domain %s.", domain); - failedDomainsBuilder.add(domain); - } - successfulDomainsBuilder.add(domain); - } else { - skippedDomainsBuilder.add(domain); - } - } - })); + partition(getDomains(), BATCH_SIZE) + .forEach( + batch -> + tm().transact( + () -> { + for (String domain : batch) { + if (shouldApplyToDomain(domain, tm().getTransactionTime())) { + try { + createAndApplyRequest(domain); + } catch (Throwable t) { + logger.atSevere().withCause(t).log( + "Error when (un)locking domain %s.", domain); + failedDomainsBuilder.add(domain); + } + successfulDomainsBuilder.add(domain); + } else { + skippedDomainsBuilder.add(domain); + } + } + })); ImmutableSet successfulDomains = successfulDomainsBuilder.build(); ImmutableSet skippedDomains = skippedDomainsBuilder.build(); ImmutableSet failedDomains = failedDomainsBuilder.build(); diff --git a/core/src/main/java/google/registry/tools/javascrap/BackfillRegistryLocksCommand.java b/core/src/main/java/google/registry/tools/javascrap/BackfillRegistryLocksCommand.java index df826a059..2a56a9de6 100644 --- a/core/src/main/java/google/registry/tools/javascrap/BackfillRegistryLocksCommand.java +++ b/core/src/main/java/google/registry/tools/javascrap/BackfillRegistryLocksCommand.java @@ -144,12 +144,14 @@ public class BackfillRegistryLocksCommand extends ConfirmingCommand private ImmutableList 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()) diff --git a/core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java index adf857ec1..f07ddf205 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java @@ -186,7 +186,7 @@ public class DomainCheckFlowTest extends ResourceCheckFlowTestCase