diff --git a/core/src/main/java/google/registry/tools/DomainLockUtils.java b/core/src/main/java/google/registry/tools/DomainLockUtils.java index 39facc25c..30af71e72 100644 --- a/core/src/main/java/google/registry/tools/DomainLockUtils.java +++ b/core/src/main/java/google/registry/tools/DomainLockUtils.java @@ -117,7 +117,7 @@ public final class DomainLockUtils { RegistryLock newLock = RegistryLockDao.save(lock.asBuilder().setLockCompletionTimestamp(now).build()); setAsRelock(newLock); - tm().transact(() -> applyLockStatuses(newLock, now)); + tm().transact(() -> applyLockStatuses(newLock, now, isAdmin)); return newLock; }); } @@ -171,7 +171,7 @@ public final class DomainLockUtils { createLockBuilder(domainName, registrarId, registrarPocId, isAdmin) .setLockCompletionTimestamp(now) .build()); - tm().transact(() -> applyLockStatuses(newLock, now)); + tm().transact(() -> applyLockStatuses(newLock, now, isAdmin)); setAsRelock(newLock); return newLock; }); @@ -222,18 +222,18 @@ public final class DomainLockUtils { String domainName, String registrarId, @Nullable String registrarPocId, boolean isAdmin) { DateTime now = jpaTm().getTransactionTime(); DomainBase domainBase = getDomain(domainName, registrarId, now); - verifyDomainNotLocked(domainBase); + verifyDomainNotLocked(domainBase, isAdmin); - // Multiple pending actions are not allowed + // Multiple pending actions are not allowed for non-admins RegistryLockDao.getMostRecentByRepoId(domainBase.getRepoId()) .ifPresent( previousLock -> checkArgument( previousLock.isLockRequestExpired(now) - || previousLock.getUnlockCompletionTimestamp().isPresent(), + || previousLock.getUnlockCompletionTimestamp().isPresent() + || isAdmin, "A pending or completed lock action already exists for %s", previousLock.getDomainName())); - return new RegistryLock.Builder() .setVerificationCode(stringGenerator.createString(VERIFICATION_CODE_LENGTH)) .setDomainName(domainName) @@ -250,6 +250,8 @@ public final class DomainLockUtils { Optional lockOptional = RegistryLockDao.getMostRecentVerifiedLockByRepoId(domainBase.getRepoId()); + verifyDomainLocked(domainBase, isAdmin); + RegistryLock.Builder newLockBuilder; if (isAdmin) { // Admins should always be able to unlock domains in case we get in a bad state @@ -265,7 +267,6 @@ public final class DomainLockUtils { .setLockCompletionTimestamp(now) .setRegistrarId(registrarId)); } else { - verifyDomainLocked(domainBase); RegistryLock lock = lockOptional.orElseThrow( () -> @@ -293,16 +294,17 @@ public final class DomainLockUtils { .setRegistrarId(registrarId); } - private static void verifyDomainNotLocked(DomainBase domainBase) { + private static void verifyDomainNotLocked(DomainBase domainBase, boolean isAdmin) { checkArgument( - !domainBase.getStatusValues().containsAll(REGISTRY_LOCK_STATUSES), + isAdmin || !domainBase.getStatusValues().containsAll(REGISTRY_LOCK_STATUSES), "Domain %s is already locked", domainBase.getDomainName()); } - private static void verifyDomainLocked(DomainBase domainBase) { + private static void verifyDomainLocked(DomainBase domainBase, boolean isAdmin) { checkArgument( - !Sets.intersection(domainBase.getStatusValues(), REGISTRY_LOCK_STATUSES).isEmpty(), + isAdmin || !Sets.intersection(domainBase.getStatusValues(), REGISTRY_LOCK_STATUSES) + .isEmpty(), "Domain %s is already unlocked", domainBase.getDomainName()); } @@ -311,7 +313,7 @@ public final class DomainLockUtils { DomainBase domain = loadByForeignKeyCached(DomainBase.class, domainName, now) .orElseThrow( - () -> new IllegalArgumentException(String.format("Unknown domain %s", domainName))); + () -> new IllegalArgumentException("Domain doesn't exist")); // The user must have specified either the correct registrar ID or the admin registrar ID checkArgument( registryAdminRegistrarId.equals(registrarId) @@ -330,9 +332,9 @@ public final class DomainLockUtils { String.format("Invalid verification code %s", verificationCode))); } - private void applyLockStatuses(RegistryLock lock, DateTime lockTime) { + private void applyLockStatuses(RegistryLock lock, DateTime lockTime, boolean isAdmin) { DomainBase domain = getDomain(lock.getDomainName(), lock.getRegistrarId(), lockTime); - verifyDomainNotLocked(domain); + verifyDomainNotLocked(domain, isAdmin); DomainBase newDomain = domain @@ -345,9 +347,7 @@ public final class DomainLockUtils { private void removeLockStatuses(RegistryLock lock, boolean isAdmin, DateTime unlockTime) { DomainBase domain = getDomain(lock.getDomainName(), lock.getRegistrarId(), unlockTime); - if (!isAdmin) { - verifyDomainLocked(domain); - } + verifyDomainLocked(domain, isAdmin); DomainBase newDomain = domain diff --git a/core/src/main/java/google/registry/tools/LockDomainCommand.java b/core/src/main/java/google/registry/tools/LockDomainCommand.java index 0e94cbd3e..4382cce63 100644 --- a/core/src/main/java/google/registry/tools/LockDomainCommand.java +++ b/core/src/main/java/google/registry/tools/LockDomainCommand.java @@ -14,15 +14,7 @@ package google.registry.tools; -import static google.registry.model.EppResourceUtils.loadByForeignKey; - import com.beust.jcommander.Parameters; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; -import com.google.common.flogger.FluentLogger; -import google.registry.model.domain.DomainBase; -import google.registry.model.eppcommon.StatusValue; -import org.joda.time.DateTime; /** * A command to registry lock domain names. @@ -32,25 +24,6 @@ import org.joda.time.DateTime; @Parameters(separators = " =", commandDescription = "Registry lock a domain via EPP.") public class LockDomainCommand extends LockOrUnlockDomainCommand { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - - @Override - protected boolean shouldApplyToDomain(String domain, DateTime now) { - DomainBase domainBase = - loadByForeignKey(DomainBase.class, domain, now) - .orElseThrow( - () -> - new IllegalArgumentException( - String.format("Domain '%s' does not exist or is deleted", domain))); - ImmutableSet statusesToAdd = - Sets.difference(REGISTRY_LOCK_STATUSES, domainBase.getStatusValues()).immutableCopy(); - if (statusesToAdd.isEmpty()) { - logger.atInfo().log("Domain '%s' is already locked and needs no updates.", domain); - return false; - } - return true; - } - @Override protected void createAndApplyRequest(String domain) { domainLockUtils.administrativelyApplyLock(domain, clientId, null, true); diff --git a/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java b/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java index 67d7c0382..9b68621a0 100644 --- a/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java +++ b/core/src/main/java/google/registry/tools/LockOrUnlockDomainCommand.java @@ -15,24 +15,28 @@ package google.registry.tools; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.partition; import static google.registry.model.eppcommon.StatusValue.SERVER_DELETE_PROHIBITED; import static google.registry.model.eppcommon.StatusValue.SERVER_TRANSFER_PROHIBITED; import static google.registry.model.eppcommon.StatusValue.SERVER_UPDATE_PROHIBITED; +import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.util.CollectionUtils.findDuplicates; import com.beust.jcommander.Parameter; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; import google.registry.config.RegistryConfig.Config; import google.registry.model.eppcommon.StatusValue; 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 { @@ -57,7 +61,8 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand @Config("registryAdminClientId") String registryAdminClientId; - @Inject DomainLockUtils domainLockUtils; + @Inject + DomainLockUtils domainLockUtils; protected ImmutableSet getDomains() { return ImmutableSet.copyOf(mainParameters); @@ -78,37 +83,34 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand @Override protected String execute() { ImmutableSet.Builder successfulDomainsBuilder = new ImmutableSet.Builder<>(); - ImmutableSet.Builder skippedDomainsBuilder = new ImmutableSet.Builder<>(); - ImmutableSet.Builder failedDomainsBuilder = new ImmutableSet.Builder<>(); + ImmutableMap.Builder failedDomainsToReasons = new ImmutableMap.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); - } - } - })); + // we require that the jpaTm is the outer transaction in DomainLockUtils + jpaTm().transact(() -> tm().transact( + () -> { + for (String domain : batch) { + try { + createAndApplyRequest(domain); + } catch (Throwable t) { + logger.atSevere().withCause(t).log( + "Error when (un)locking domain %s.", domain); + failedDomainsToReasons.put(domain, t.getMessage()); + continue; + } + successfulDomainsBuilder.add(domain); + } + }))); ImmutableSet successfulDomains = successfulDomainsBuilder.build(); - ImmutableSet skippedDomains = skippedDomainsBuilder.build(); - ImmutableSet failedDomains = failedDomainsBuilder.build(); + ImmutableSet failedDomains = failedDomainsToReasons.build().entrySet() + .stream() + .map(entry -> String.format("%s (%s)", entry.getKey(), entry.getValue())) + .collect(toImmutableSet()); return String.format( - "Successfully locked/unlocked domains:\n%s\nSkipped domains:\n%s\nFailed domains:\n%s", - successfulDomains, skippedDomains, failedDomains); + "Successfully locked/unlocked domains:\n%s\nFailed domains:\n%s", + successfulDomains, failedDomains); } - protected abstract boolean shouldApplyToDomain(String domain, DateTime now); - protected abstract void createAndApplyRequest(String domain); } diff --git a/core/src/main/java/google/registry/tools/UnlockDomainCommand.java b/core/src/main/java/google/registry/tools/UnlockDomainCommand.java index 49a1ac508..be9b89704 100644 --- a/core/src/main/java/google/registry/tools/UnlockDomainCommand.java +++ b/core/src/main/java/google/registry/tools/UnlockDomainCommand.java @@ -14,16 +14,8 @@ package google.registry.tools; -import static google.registry.model.EppResourceUtils.loadByForeignKey; - import com.beust.jcommander.Parameters; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; -import com.google.common.flogger.FluentLogger; -import google.registry.model.domain.DomainBase; -import google.registry.model.eppcommon.StatusValue; import java.util.Optional; -import org.joda.time.DateTime; /** * A command to registry unlock domain names. @@ -33,25 +25,6 @@ import org.joda.time.DateTime; @Parameters(separators = " =", commandDescription = "Registry unlock a domain via EPP.") public class UnlockDomainCommand extends LockOrUnlockDomainCommand { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - - @Override - protected boolean shouldApplyToDomain(String domain, DateTime now) { - DomainBase domainBase = - loadByForeignKey(DomainBase.class, domain, now) - .orElseThrow( - () -> - new IllegalArgumentException( - String.format("Domain '%s' does not exist or is deleted", domain))); - ImmutableSet statusesToRemove = - Sets.intersection(domainBase.getStatusValues(), REGISTRY_LOCK_STATUSES).immutableCopy(); - if (statusesToRemove.isEmpty()) { - logger.atInfo().log("Domain '%s' is already unlocked and needs no updates.", domain); - return false; - } - return true; - } - @Override protected void createAndApplyRequest(String domain) { domainLockUtils.administrativelyApplyUnlock(domain, clientId, true, Optional.empty()); diff --git a/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java b/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java index 1e286480f..043fe96fa 100644 --- a/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java +++ b/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java @@ -49,6 +49,7 @@ import google.registry.testing.AppEngineRule; import google.registry.testing.DatastoreHelper; import google.registry.testing.DeterministicStringGenerator; import google.registry.testing.FakeClock; +import google.registry.testing.SqlHelper; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.testing.UserInfo; import google.registry.util.AppEngineServiceUtils; @@ -275,6 +276,37 @@ public final class DomainLockUtilsTest { standardDays(6).plus(standardSeconds(30)))); } + @Test + void testSuccess_adminCanLockLockedDomain_withNoSavedLock() { + // in the case of inconsistencies / errors, admins should have the ability to override + // whatever statuses exist on the domain + persistResource(domain.asBuilder().setStatusValues(REGISTRY_LOCK_STATUSES).build()); + RegistryLock resultLock = domainLockUtils + .administrativelyApplyLock(DOMAIN_NAME, "TheRegistrar", POC_ID, true); + verifyProperlyLockedDomain(true); + assertThat(resultLock.getLockCompletionTimestamp()).isEqualTo(Optional.of(clock.nowUtc())); + } + + @Test + void testSuccess_adminCanLockUnlockedDomain_withSavedLock() { + // in the case of inconsistencies / errors, admins should have the ability to override + // what the RegistryLock table says + SqlHelper.saveRegistryLock(new RegistryLock.Builder() + .setLockCompletionTimestamp(clock.nowUtc()) + .setDomainName(DOMAIN_NAME) + .setVerificationCode("hi") + .setRegistrarId("TheRegistrar") + .setRepoId(domain.getRepoId()) + .isSuperuser(false) + .setRegistrarPocId(POC_ID) + .build()); + clock.advanceOneMilli(); + RegistryLock resultLock = domainLockUtils + .administrativelyApplyLock(DOMAIN_NAME, "TheRegistrar", POC_ID, true); + verifyProperlyLockedDomain(true); + assertThat(resultLock.getLockCompletionTimestamp()).isEqualTo(Optional.of(clock.nowUtc())); + } + @Test void testFailure_createUnlock_alreadyPendingUnlock() { RegistryLock lock = @@ -317,7 +349,7 @@ public final class DomainLockUtilsTest { domainLockUtils.saveNewRegistryLockRequest( "asdf.tld", "TheRegistrar", POC_ID, false))) .hasMessageThat() - .isEqualTo("Unknown domain asdf.tld"); + .isEqualTo("Domain doesn't exist"); } @Test diff --git a/core/src/test/java/google/registry/tools/LockDomainCommandTest.java b/core/src/test/java/google/registry/tools/LockDomainCommandTest.java index a8742990c..7b10251fe 100644 --- a/core/src/test/java/google/registry/tools/LockDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/LockDomainCommandTest.java @@ -98,12 +98,9 @@ class LockDomainCommandTest extends CommandTestCase { } @Test - void testFailure_domainDoesntExist() { - IllegalArgumentException e = - assertThrows( - IllegalArgumentException.class, - () -> runCommandForced("--client=NewRegistrar", "missing.tld")); - assertThat(e).hasMessageThat().isEqualTo("Domain 'missing.tld' does not exist or is deleted"); + void testFailure_domainDoesntExist() throws Exception { + runCommandForced("--client=NewRegistrar", "missing.tld"); + assertInStdout("Failed domains:\n[missing.tld (Domain doesn't exist)]"); } @Test diff --git a/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java b/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java index e5885d851..2ab272fbe 100644 --- a/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java @@ -108,19 +108,16 @@ class UnlockDomainCommandTest extends CommandTestCase { } @Test - void testFailure_domainDoesntExist() { - IllegalArgumentException e = - assertThrows( - IllegalArgumentException.class, - () -> runCommandForced("--client=TheRegistrar", "missing.tld")); - assertThat(e).hasMessageThat().isEqualTo("Domain 'missing.tld' does not exist or is deleted"); + void testFailure_domainDoesntExist() throws Exception { + runCommandForced("--client=NewRegistrar", "missing.tld"); + assertInStdout("Failed domains:\n[missing.tld (Domain doesn't exist)]"); } @Test - void testSuccess_alreadyUnlockedDomain_performsNoAction() throws Exception { + void testSuccess_alreadyUnlockedDomain_staysUnlocked() throws Exception { DomainBase domain = persistActiveDomain("example.tld"); runCommandForced("--client=TheRegistrar", "example.tld"); - assertThat(reloadResource(domain)).isEqualTo(domain); + assertThat(reloadResource(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES); } @Test diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java index 233a8add2..2eed8d69c 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java @@ -358,7 +358,7 @@ public final class RegistryLockPostActionTest { "domainName", "bad.tld", "isLock", true, "password", "hi")); - assertFailureWithMessage(response, "Unknown domain bad.tld"); + assertFailureWithMessage(response, "Domain doesn't exist"); } @Test