From 98737721500b41d6083c2c7dd446b95111e155ed Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Tue, 25 Jul 2023 18:15:45 -0400 Subject: [PATCH] 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. --- .../google/registry/flows/FlowRunner.java | 15 +++-- .../registry/flows/session/LoginFlow.java | 24 ++++---- .../registry/model/eppinput/EppInput.java | 4 +- .../flows/session/LoginFlowTestCase.java | 55 +++++++++++++++++-- ...d_newpw.xml => login_set_new_password.xml} | 2 +- docs/flows.md | 4 +- 6 files changed, 73 insertions(+), 31 deletions(-) rename core/src/test/resources/google/registry/flows/session/{login_invalid_newpw.xml => login_set_new_password.xml} (93%) diff --git a/core/src/main/java/google/registry/flows/FlowRunner.java b/core/src/main/java/google/registry/flows/FlowRunner.java index e1859ba7c..64b5d038b 100644 --- a/core/src/main/java/google/registry/flows/FlowRunner.java +++ b/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); diff --git a/core/src/main/java/google/registry/flows/session/LoginFlow.java b/core/src/main/java/google/registry/flows/session/LoginFlow.java index 0879ee6b0..3f5216af5 100644 --- a/core/src/main/java/google/registry/flows/session/LoginFlow.java +++ b/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 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"); - } - } } diff --git a/core/src/main/java/google/registry/model/eppinput/EppInput.java b/core/src/main/java/google/registry/model/eppinput/EppInput.java index ca6071d0e..524c61db5 100644 --- a/core/src/main/java/google/registry/model/eppinput/EppInput.java +++ b/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 getNewPassword() { + return Optional.ofNullable(newPassword); } public Options getOptions() { diff --git a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java b/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java index 2abaf9db0..2d787eb34 100644 --- a/core/src/test/java/google/registry/flows/session/LoginFlowTestCase.java +++ b/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 { // 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 { @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 { } @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 diff --git a/core/src/test/resources/google/registry/flows/session/login_invalid_newpw.xml b/core/src/test/resources/google/registry/flows/session/login_set_new_password.xml similarity index 93% rename from core/src/test/resources/google/registry/flows/session/login_invalid_newpw.xml rename to core/src/test/resources/google/registry/flows/session/login_set_new_password.xml index 5acb13c65..2fd6ba91c 100644 --- a/core/src/test/resources/google/registry/flows/session/login_invalid_newpw.xml +++ b/core/src/test/resources/google/registry/flows/session/login_set_new_password.xml @@ -3,7 +3,7 @@ NewRegistrar foo-BAR2 - ANewPassword + %NEWPW% 1.0 en diff --git a/docs/flows.md b/docs/flows.md index 50d149f94..c51a49ca1 100644 --- a/docs/flows.md +++ b/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