Allow EPP password to be set during login flow (#2080)

This is part of the spec in RFC 5730 that we hadn't implemented until now. Note
that this requires changing LoginFlow to be transactional, but I don't think
that should cause any issues.

core/src/main/java/google/registry/batch/AsyncTaskEnqueuer.java

@@ -50,12 +50,6 @@ public final class AsyncTaskEnqueuer {
     this.cloudTasksUtils = cloudTasksUtils;
   }
 
-  /** Enqueues a task to asynchronously re-save an entity at some point in the future. */
-  public void enqueueAsyncResave(
-      VKey<? extends EppResource> entityToResave, DateTime now, DateTime whenToResave) {
-    enqueueAsyncResave(entityToResave, now, ImmutableSortedSet.of(whenToResave));
-  }
-
   /**
    * Enqueues a task to asynchronously re-save an entity at some point(s) in the future.
    *

core/src/main/java/google/registry/flows/FlowRunner.java

@@ -72,22 +72,21 @@ public class FlowRunner {
     }
     eppMetricBuilder.setCommandNameFromFlow(flowClass.getSimpleName());
     if (!isTransactional) {
-      EppOutput eppOutput = EppOutput.create(flowProvider.get().run());
-      if (flowClass.equals(LoginFlow.class)) {
-        // In LoginFlow, registrarId isn't known until after the flow executes, so save it then.
-        eppMetricBuilder.setRegistrarId(sessionMetadata.getRegistrarId());
-      }
-      return eppOutput;
+      return EppOutput.create(flowProvider.get().run());
     }
     try {
-      return tm()
-          .transact(
+      return tm().transact(
               () -> {
                 try {
                   EppOutput output = EppOutput.create(flowProvider.get().run());
                   if (isDryRun) {
                     throw new DryRunException(output);
                   }
+                  if (flowClass.equals(LoginFlow.class)) {
+                    // In LoginFlow, registrarId isn't known until after the flow executes, so save
+                    // it then.
+                    eppMetricBuilder.setRegistrarId(sessionMetadata.getRegistrarId());
+                  }
                   return output;
                 } catch (EppException e) {
                   throw new EppRuntimeException(e);

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

@@ -38,6 +38,7 @@ import static google.registry.persistence.transaction.TransactionManagerFactory.
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
 import google.registry.batch.AsyncTaskEnqueuer;
 import google.registry.flows.EppException;
 import google.registry.flows.ExtensionManager;
@@ -286,7 +287,8 @@ public final class DomainTransferRequestFlow implements TransactionalFlow {
             .build();
     DomainHistory domainHistory = buildDomainHistory(newDomain, tld, now, period);
 
-    asyncTaskEnqueuer.enqueueAsyncResave(newDomain.createVKey(), now, automaticTransferTime);
+    asyncTaskEnqueuer.enqueueAsyncResave(
+        newDomain.createVKey(), now, ImmutableSortedSet.of(automaticTransferTime));
     tm().putAll(
             new ImmutableSet.Builder<>()
                 .add(newDomain, domainHistory, requestPollMessage)

core/src/main/java/google/registry/flows/session/LoginFlow.java

@@ -15,6 +15,7 @@
 package google.registry.flows.session;
 
 import static com.google.common.collect.Sets.difference;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.CollectionUtils.nullToEmpty;
 
 import com.google.common.collect.ImmutableSet;
@@ -27,11 +28,10 @@ import google.registry.flows.EppException.CommandUseErrorException;
 import google.registry.flows.EppException.ParameterValuePolicyErrorException;
 import google.registry.flows.EppException.UnimplementedExtensionException;
 import google.registry.flows.EppException.UnimplementedObjectServiceException;
-import google.registry.flows.EppException.UnimplementedOptionException;
 import google.registry.flows.ExtensionManager;
-import google.registry.flows.Flow;
 import google.registry.flows.FlowModule.RegistrarId;
 import google.registry.flows.SessionMetadata;
+import google.registry.flows.TransactionalFlow;
 import google.registry.flows.TransportCredentials;
 import google.registry.model.eppcommon.ProtocolDefinition;
 import google.registry.model.eppcommon.ProtocolDefinition.ServiceExtension;
@@ -51,6 +51,7 @@ import javax.inject.Inject;
  * @error {@link google.registry.flows.EppException.UnimplementedExtensionException}
  * @error {@link google.registry.flows.EppException.UnimplementedObjectServiceException}
  * @error {@link google.registry.flows.EppException.UnimplementedProtocolVersionException}
+ * @error {@link google.registry.flows.FlowUtils.GenericXmlSyntaxErrorException}
  * @error {@link google.registry.flows.TlsCredentials.BadRegistrarCertificateException}
  * @error {@link google.registry.flows.TlsCredentials.BadRegistrarIpAddressException}
  * @error {@link google.registry.flows.TlsCredentials.MissingRegistrarCertificateException}
@@ -58,11 +59,10 @@ import javax.inject.Inject;
  * @error {@link LoginFlow.AlreadyLoggedInException}
  * @error {@link BadRegistrarIdException}
  * @error {@link LoginFlow.TooManyFailedLoginsException}
- * @error {@link LoginFlow.PasswordChangesNotSupportedException}
  * @error {@link LoginFlow.RegistrarAccountNotActiveException}
  * @error {@link LoginFlow.UnsupportedLanguageException}
  */
-public class LoginFlow implements Flow {
+public class LoginFlow implements TransactionalFlow {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
@@ -134,8 +134,13 @@ public class LoginFlow implements Flow {
     if (!registrar.get().isLive()) {
       throw new RegistrarAccountNotActiveException();
     }
-    if (login.getNewPassword() != null) {  // We don't support in-band password changes.
-      throw new PasswordChangesNotSupportedException();
+    if (login.getNewPassword().isPresent()) {
+      // Load fresh from database (bypassing the cache) to ensure we don't save stale data.
+      Optional<Registrar> freshRegistrar = Registrar.loadByRegistrarId(login.getClientId());
+      if (!freshRegistrar.isPresent()) {
+        throw new BadRegistrarIdException(login.getClientId());
+      }
+      tm().put(freshRegistrar.get().asBuilder().setPassword(login.getNewPassword().get()).build());
     }
 
     // We are in!
@@ -179,11 +184,4 @@ public class LoginFlow implements Flow {
       super("Specified language is not supported");
     }
   }
-
-  /** In-band password changes are not supported. */
-  static class PasswordChangesNotSupportedException extends UnimplementedOptionException {
-    public PasswordChangesNotSupportedException() {
-      super("In-band password changes are not supported");
-    }
-  }
 }

core/src/main/java/google/registry/model/eppinput/EppInput.java

@@ -291,8 +291,8 @@ public class EppInput extends ImmutableObject {
       return password;
     }
 
-    public String getNewPassword() {
-      return newPassword;
+    public Optional<String> getNewPassword() {
+      return Optional.ofNullable(newPassword);
     }
 
     public Options getOptions() {

core/src/test/java/google/registry/batch/AsyncTaskEnqueuerTest.java

@@ -69,7 +69,7 @@ public class AsyncTaskEnqueuerTest {
   void test_enqueueAsyncResave_success() {
     Contact contact = persistActiveContact("jd23456");
     asyncTaskEnqueuer.enqueueAsyncResave(
-        contact.createVKey(), clock.nowUtc(), clock.nowUtc().plusDays(5));
+        contact.createVKey(), clock.nowUtc(), ImmutableSortedSet.of(clock.nowUtc().plusDays(5)));
     cloudTasksHelper.assertTasksEnqueued(
         QUEUE_ASYNC_ACTIONS,
         new CloudTasksHelper.TaskMatcher()
@@ -108,7 +108,7 @@ public class AsyncTaskEnqueuerTest {
   void test_enqueueAsyncResave_ignoresTasksTooFarIntoFuture() {
     Contact contact = persistActiveContact("jd23456");
     asyncTaskEnqueuer.enqueueAsyncResave(
-        contact.createVKey(), clock.nowUtc(), clock.nowUtc().plusDays(31));
+        contact.createVKey(), clock.nowUtc(), ImmutableSortedSet.of(clock.nowUtc().plusDays(31)));
     cloudTasksHelper.assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
     assertLogMessage(logHandler, Level.INFO, "Ignoring async re-save");
   }

core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java

@@ -21,15 +21,16 @@ import static google.registry.testing.DatabaseHelper.persistResource;
 import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.google.common.collect.ImmutableMap;
 import google.registry.flows.EppException;
 import google.registry.flows.EppException.UnimplementedExtensionException;
 import google.registry.flows.EppException.UnimplementedObjectServiceException;
 import google.registry.flows.EppException.UnimplementedProtocolVersionException;
 import google.registry.flows.FlowTestCase;
+import google.registry.flows.FlowUtils.GenericXmlSyntaxErrorException;
 import google.registry.flows.TransportCredentials.BadRegistrarPasswordException;
 import google.registry.flows.session.LoginFlow.AlreadyLoggedInException;
 import google.registry.flows.session.LoginFlow.BadRegistrarIdException;
-import google.registry.flows.session.LoginFlow.PasswordChangesNotSupportedException;
 import google.registry.flows.session.LoginFlow.RegistrarAccountNotActiveException;
 import google.registry.flows.session.LoginFlow.TooManyFailedLoginsException;
 import google.registry.flows.session.LoginFlow.UnsupportedLanguageException;
@@ -61,7 +62,7 @@ public abstract class LoginFlowTestCase extends FlowTestCase<LoginFlow> {
   // Also called in subclasses.
   void doSuccessfulTest(String xmlFilename) throws Exception {
     setEppInput(xmlFilename);
-    assertTransactionalFlow(false);
+    assertTransactionalFlow(true);
     runFlowAssertResponse(loadFile("generic_success_response.xml"));
   }
 
@@ -80,7 +81,7 @@ public abstract class LoginFlowTestCase extends FlowTestCase<LoginFlow> {
   @Test
   void testSuccess_setsIsLoginResponse() throws Exception {
     setEppInput("login_valid.xml");
-    assertTransactionalFlow(false);
+    assertTransactionalFlow(true);
     EppOutput output = runFlow();
     assertThat(output.getResponse().isLoginResponse()).isTrue();
   }
@@ -118,8 +119,52 @@ public abstract class LoginFlowTestCase extends FlowTestCase<LoginFlow> {
   }
 
   @Test
-  void testFailure_newPassword() {
-    doFailingTest("login_invalid_newpw.xml", PasswordChangesNotSupportedException.class);
+  void testSetNewPassword() throws Exception {
+    assertThat(registrar.verifyPassword("foo-BAR2")).isTrue();
+    assertThat(registrar.verifyPassword("ANewPassword")).isFalse();
+    assertThat(registrar.verifyPassword("randomstring")).isFalse();
+
+    setEppInput("login_set_new_password.xml", ImmutableMap.of("NEWPW", "ANewPassword"));
+    assertTransactionalFlow(true);
+    runFlowAssertResponse(loadFile("generic_success_response.xml"));
+
+    Registrar newRegistrar = loadRegistrar("NewRegistrar");
+    assertThat(newRegistrar.verifyPassword("foo-BAR2")).isFalse();
+    assertThat(newRegistrar.verifyPassword("ANewPassword")).isTrue();
+    assertThat(registrar.verifyPassword("randomstring")).isFalse();
+  }
+
+  @Test
+  void testFailure_invalidNewPassword_tooShort() throws Exception {
+    setEppInput("login_set_new_password.xml", ImmutableMap.of("NEWPW", "5Char"));
+    EppException thrown = assertThrows(GenericXmlSyntaxErrorException.class, this::runFlow);
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "length = '5' is not facet-valid with respect to minLength '6' for type 'pwType'");
+  }
+
+  @Test
+  void testFailure_invalidNewPassword_tooLong() throws Exception {
+    setEppInput(
+        "login_set_new_password.xml", ImmutableMap.of("NEWPW", "ThisIsMoreThan16Characters"));
+    EppException thrown = assertThrows(GenericXmlSyntaxErrorException.class, this::runFlow);
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+    assertThat(thrown)
+        .hasMessageThat()
+        .contains(
+            "length = '26' is not facet-valid with respect to maxLength '16' for type 'pwType'");
+  }
+
+  @Test
+  void testFailure_invalidNewPassword_containsInvalidCharacter() throws Exception {
+    setEppInput("login_set_new_password.xml", ImmutableMap.of("NEWPW", "TheChar&IsNotValid"));
+    EppException thrown = assertThrows(GenericXmlSyntaxErrorException.class, this::runFlow);
+    assertAboutEppExceptions().that(thrown).marshalsToXml();
+    // Just generically assert on this error message because it's a pretty broad error owing to the
+    // overall XML simply not parsing correctly.
+    assertThat(thrown).hasMessageThat().contains("Syntax error");
   }
 
   @Test

core/src/test/resources/google/registry/flows/session/login_set_new_password.xml

@@ -3,7 +3,7 @@
     <login>
       <clID>NewRegistrar</clID>
       <pw>foo-BAR2</pw>
-      <newPW>ANewPassword</newPW>
+      <newPW>%NEWPW%</newPW>
       <options>
         <version>1.0</version>
         <lang>en</lang>

docs/flows.md

@@ -992,12 +992,12 @@ An EPP flow for login.
 
 ### Errors
 
+*   2001
+    *   Generic XML syntax error that can be thrown by any flow.
 *   2002
     *   Registrar is already logged in.
 *   2100
     *   Specified protocol version is not implemented.
-*   2102
-    *   In-band password changes are not supported.
 *   2103
     *   Specified extension is not implemented.
 *   2200

java_common.gradle

@@ -70,6 +70,27 @@ configurations {
   }
 }
 
+// Custom resolution strategy to get around tricky dependency issues.
+configurations.all {
+  // jackson-core is a transitive dependency that we cannot explicity specify
+  // versions for, without triggering linter alerts. Even though the lockfiles
+  // lock it to a lower version, Gradle will first try to resolve the
+  // dependency tree based on what is available in the Maven repo before
+  // applying any dependency locks. The newer version (v2.15) of jackson-core
+  // results in a build failure for pre-7.6 Gradle versions during that stage.
+  // This custom resolution strategy will modify the dependency tree as it was
+  // being built and prevent Gradle from even trying v2.15.
+  // See: https://github.com/FasterXML/jackson-core/issues/955
+  // TODO: Remove the custom stragegy after we upgrade to Gradle 7.6+.
+  resolutionStrategy.eachDependency { DependencyResolveDetails details ->
+    if (details.requested.group == 'com.fasterxml.jackson.core'
+            && details.requested.name == 'jackson-core'
+            && details.requested.version.startsWith('2.15')) {
+      details.useVersion '2.14.2'
+    }
+  }
+}
+
 dependencies {
     // compatibility with Java 8
     errorprone("com.google.errorprone:error_prone_core:2.3.4")