Run the (Un)lockDomainCommand in an outer JPA txn (#688)

* Run the (Un)lockDomainCommand in an outer JPA txn

There are a couple things going on here in this commit.

First, we add an external JPA transaction in the
LockOrUnlockDomainCommand class. This doesn't appear to do much, but it
avoids a situation similar to deadlock if an error occurs in Datastore
when saving the domain object. Specifically, DomainLockUtils relies on
the fact that any error in Datastore will be re-thrown in the JPA
transaction, meaning that any Datastore error will back out of the SQL
transaction as well. However, this is no longer true if we are already
in a Datastore transaction when calling DomainLockUtils (unless, again,
we are also in a JPA transaction). Basically, we require that the outer
transaction is the JPA one.

Secondly, this just allows for more breakglass operations in the lock or
unlock domain commands -- in a situation where things possibly go
haywire, we should allow admins to make sure with certainty that a
domain is locked or unlocked.

* Add more robustness and tests for admins locking locked domains

* Fix expected exception message in tests

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<RegistryLock> 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, isAdmin);
-      verifyDomainLocked(domain);
-    }
 
     DomainBase newDomain =
         domain

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<StatusValue> 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);

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<String> getDomains() {
     return ImmutableSet.copyOf(mainParameters);
@@ -78,37 +83,34 @@ public abstract class LockOrUnlockDomainCommand extends ConfirmingCommand
   @Override
   protected String execute() {
     ImmutableSet.Builder<String> successfulDomainsBuilder = new ImmutableSet.Builder<>();
-    ImmutableSet.Builder<String> skippedDomainsBuilder = new ImmutableSet.Builder<>();
+    ImmutableMap.Builder<String, String> failedDomainsToReasons = new ImmutableMap.Builder<>();
-    ImmutableSet.Builder<String> failedDomainsBuilder = new ImmutableSet.Builder<>();
     partition(getDomains(), BATCH_SIZE)
         .forEach(
             batch ->
-                tm().transact(
+                // we require that the jpaTm is the outer transaction in DomainLockUtils
+                jpaTm().transact(() -> 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);
+                          failedDomainsToReasons.put(domain, t.getMessage());
+                          continue;
                         }
                         successfulDomainsBuilder.add(domain);
-                            } else {
-                              skippedDomainsBuilder.add(domain);
                       }
-                          }
+                    })));
-                        }));
     ImmutableSet<String> successfulDomains = successfulDomainsBuilder.build();
-    ImmutableSet<String> skippedDomains = skippedDomainsBuilder.build();
+    ImmutableSet<String> failedDomains = failedDomainsToReasons.build().entrySet()
-    ImmutableSet<String> failedDomains = failedDomainsBuilder.build();
+        .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",
+        "Successfully locked/unlocked domains:\n%s\nFailed domains:\n%s",
-        successfulDomains, skippedDomains, failedDomains);
+        successfulDomains, failedDomains);
   }
 
-  protected abstract boolean shouldApplyToDomain(String domain, DateTime now);
-
   protected abstract void createAndApplyRequest(String domain);
 }

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<StatusValue> 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());

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

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

@@ -98,12 +98,9 @@ class LockDomainCommandTest extends CommandTestCase<LockDomainCommand> {
   }
 
   @Test
-  void testFailure_domainDoesntExist() {
+  void testFailure_domainDoesntExist() throws Exception {
-    IllegalArgumentException e =
+    runCommandForced("--client=NewRegistrar", "missing.tld");
-        assertThrows(
+    assertInStdout("Failed domains:\n[missing.tld (Domain doesn't exist)]");
-            IllegalArgumentException.class,
-            () -> runCommandForced("--client=NewRegistrar", "missing.tld"));
-    assertThat(e).hasMessageThat().isEqualTo("Domain 'missing.tld' does not exist or is deleted");
   }
 
   @Test

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

@@ -108,19 +108,16 @@ class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand> {
   }
 
   @Test
-  void testFailure_domainDoesntExist() {
+  void testFailure_domainDoesntExist() throws Exception {
-    IllegalArgumentException e =
+    runCommandForced("--client=NewRegistrar", "missing.tld");
-        assertThrows(
+    assertInStdout("Failed domains:\n[missing.tld (Domain doesn't exist)]");
-            IllegalArgumentException.class,
-            () -> runCommandForced("--client=TheRegistrar", "missing.tld"));
-    assertThat(e).hasMessageThat().isEqualTo("Domain 'missing.tld' does not exist or is deleted");
   }
 
   @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

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