From c1643fa3cd4ba41219bf6e2f063cb0f7a809553e Mon Sep 17 00:00:00 2001 From: mcilwain Date: Mon, 24 Apr 2017 07:44:31 -0700 Subject: [PATCH] Correctly set clientId on EPP metrics in LoginFlow This wasn't being recorded correctly because the clientId is only set in LoginFlow after the flow succeeds, whereas we were previously logging the clientId before executing the flow. This adds special handling for LoginFlow. Note that we only set the metric label to the clientId for valid registrar logins, to ensure that metric cardinality doesn't grow unbounded (as it might if we used every arbitrary string passed in as an attempted login). This also refactors creation and handling of FakesAndMocksModule so as to be able to make test assertions about EPP metrics from integration flow tests. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=154048280 --- java/google/registry/flows/FlowRunner.java | 8 +++- .../registry/flows/CheckApiActionTest.java | 2 +- .../registry/flows/EppCommitLogsTest.java | 23 ++++++----- .../registry/flows/EppLifecycleLoginTest.java | 40 +++++++++++++++++++ .../google/registry/flows/EppTestCase.java | 9 ++++- .../registry/flows/EppTestComponent.java | 38 ++++++++++-------- .../google/registry/flows/FlowTestCase.java | 13 +++--- 7 files changed, 98 insertions(+), 35 deletions(-) create mode 100644 javatests/google/registry/flows/EppLifecycleLoginTest.java diff --git a/java/google/registry/flows/FlowRunner.java b/java/google/registry/flows/FlowRunner.java index f7f0b9d71..e56d829d9 100644 --- a/java/google/registry/flows/FlowRunner.java +++ b/java/google/registry/flows/FlowRunner.java @@ -25,6 +25,7 @@ import google.registry.flows.FlowModule.DryRun; import google.registry.flows.FlowModule.InputXml; import google.registry.flows.FlowModule.Superuser; import google.registry.flows.FlowModule.Transactional; +import google.registry.flows.session.LoginFlow; import google.registry.model.eppcommon.Trid; import google.registry.model.eppoutput.EppOutput; import google.registry.monitoring.whitebox.EppMetric; @@ -80,7 +81,12 @@ public class FlowRunner { eppMetricBuilder.setCommandNameFromFlow(flowClass.getSimpleName()); if (!isTransactional) { eppMetricBuilder.incrementAttempts(); - return EppOutput.create(flowProvider.get().run()); + EppOutput eppOutput = EppOutput.create(flowProvider.get().run()); + if (flowClass.equals(LoginFlow.class)) { + // In LoginFlow, clientId isn't known until after the flow executes, so save it then. + eppMetricBuilder.setClientId(sessionMetadata.getClientId()); + } + return eppOutput; } try { return ofy().transact(new Work() { diff --git a/javatests/google/registry/flows/CheckApiActionTest.java b/javatests/google/registry/flows/CheckApiActionTest.java index 6a24bf358..b15801662 100644 --- a/javatests/google/registry/flows/CheckApiActionTest.java +++ b/javatests/google/registry/flows/CheckApiActionTest.java @@ -61,7 +61,7 @@ public class CheckApiActionTest { action.response = new FakeResponse(); action.checkApiServletRegistrarClientId = "TheRegistrar"; action.eppController = DaggerEppTestComponent.builder() - .fakesAndMocksModule(new FakesAndMocksModule()) + .fakesAndMocksModule(FakesAndMocksModule.create()) .build() .startRequest() .eppController(); diff --git a/javatests/google/registry/flows/EppCommitLogsTest.java b/javatests/google/registry/flows/EppCommitLogsTest.java index d98974088..6685167f4 100644 --- a/javatests/google/registry/flows/EppCommitLogsTest.java +++ b/javatests/google/registry/flows/EppCommitLogsTest.java @@ -73,19 +73,22 @@ public class EppCommitLogsTest extends ShardableTestCase { SessionMetadata sessionMetadata = new HttpSessionMetadata(new FakeHttpSession()); sessionMetadata.setClientId("TheRegistrar"); DaggerEppTestComponent.builder() - .fakesAndMocksModule(new FakesAndMocksModule(clock, TmchCaMode.PILOT)) + .fakesAndMocksModule( + FakesAndMocksModule.create( + clock, TmchCaMode.PILOT, EppMetric.builderForRequest("request-id-1", clock))) .build() .startRequest() .flowComponentBuilder() - .flowModule(new FlowModule.Builder() - .setSessionMetadata(sessionMetadata) - .setCredentials(new PasswordOnlyTransportCredentials()) - .setEppRequestSource(EppRequestSource.UNIT_TEST) - .setIsDryRun(false) - .setIsSuperuser(false) - .setInputXmlBytes(eppLoader.getEppXml().getBytes(UTF_8)) - .setEppInput(eppLoader.getEpp()) - .build()) + .flowModule( + new FlowModule.Builder() + .setSessionMetadata(sessionMetadata) + .setCredentials(new PasswordOnlyTransportCredentials()) + .setEppRequestSource(EppRequestSource.UNIT_TEST) + .setIsDryRun(false) + .setIsSuperuser(false) + .setInputXmlBytes(eppLoader.getEppXml().getBytes(UTF_8)) + .setEppInput(eppLoader.getEpp()) + .build()) .build() .flowRunner() .run(EppMetric.builder()); diff --git a/javatests/google/registry/flows/EppLifecycleLoginTest.java b/javatests/google/registry/flows/EppLifecycleLoginTest.java new file mode 100644 index 000000000..7a8bf4a15 --- /dev/null +++ b/javatests/google/registry/flows/EppLifecycleLoginTest.java @@ -0,0 +1,40 @@ +// Copyright 2017 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package google.registry.flows; + +import static com.google.common.truth.Truth.assertThat; + +import google.registry.testing.AppEngineRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for login lifecycle. */ +@RunWith(JUnit4.class) +public class EppLifecycleLoginTest extends EppTestCase { + + @Rule + public final AppEngineRule appEngine = + AppEngineRule.builder().withDatastore().withTaskQueue().build(); + + @Test + public void testLoginAndLogout_recordClientIdInEppMetric() throws Exception { + assertCommandAndResponse("login_valid.xml", "login_response.xml"); + assertThat(getRecordedEppMetric().getClientId()).hasValue("NewRegistrar"); + assertCommandAndResponse("logout.xml", "logout_response.xml"); + assertThat(getRecordedEppMetric().getClientId()).hasValue("NewRegistrar"); + } +} diff --git a/javatests/google/registry/flows/EppTestCase.java b/javatests/google/registry/flows/EppTestCase.java index 7294e1198..327dd4832 100644 --- a/javatests/google/registry/flows/EppTestCase.java +++ b/javatests/google/registry/flows/EppTestCase.java @@ -26,6 +26,7 @@ import com.google.common.net.MediaType; import google.registry.config.RegistryConfig.ConfigModule.TmchCaMode; import google.registry.flows.EppTestComponent.FakesAndMocksModule; import google.registry.model.ofy.Ofy; +import google.registry.monitoring.whitebox.EppMetric; import google.registry.testing.FakeClock; import google.registry.testing.FakeHttpSession; import google.registry.testing.FakeResponse; @@ -48,6 +49,7 @@ public class EppTestCase extends ShardableTestCase { private SessionMetadata sessionMetadata; private TransportCredentials credentials = new PasswordOnlyTransportCredentials(); + private EppMetric.Builder eppMetricBuilder; private boolean isSuperuser; @Before @@ -114,8 +116,9 @@ public class EppTestCase extends ShardableTestCase { EppRequestHandler handler = new EppRequestHandler(); FakeResponse response = new FakeResponse(); handler.response = response; + eppMetricBuilder = EppMetric.builderForRequest("request-id-1", clock); handler.eppController = DaggerEppTestComponent.builder() - .fakesAndMocksModule(new FakesAndMocksModule(clock, TmchCaMode.PILOT)) + .fakesAndMocksModule(FakesAndMocksModule.create(clock, TmchCaMode.PILOT, eppMetricBuilder)) .build() .startRequest() .eppController(); @@ -133,4 +136,8 @@ public class EppTestCase extends ShardableTestCase { EppXmlTransformer.validateOutput(result); return result; } + + protected EppMetric getRecordedEppMetric() { + return eppMetricBuilder.build(); + } } diff --git a/javatests/google/registry/flows/EppTestComponent.java b/javatests/google/registry/flows/EppTestComponent.java index 5fbad9b57..cad904756 100644 --- a/javatests/google/registry/flows/EppTestComponent.java +++ b/javatests/google/registry/flows/EppTestComponent.java @@ -53,27 +53,31 @@ interface EppTestComponent { @Module static class FakesAndMocksModule { - final BigQueryMetricsEnqueuer metricsEnqueuer; - final DnsQueue dnsQueue; - final DomainFlowTmchUtils domainFlowTmchUtils; - final EppMetric.Builder metricBuilder; - final FakeClock clock; - final ModulesService modulesService; - final Sleeper sleeper; + private BigQueryMetricsEnqueuer metricsEnqueuer; + private DnsQueue dnsQueue; + private DomainFlowTmchUtils domainFlowTmchUtils; + private EppMetric.Builder metricBuilder; + private FakeClock clock; + private ModulesService modulesService; + private Sleeper sleeper; - FakesAndMocksModule() { - this(new FakeClock(), TmchCaMode.PILOT); + public static FakesAndMocksModule create() { + FakeClock clock = new FakeClock(); + return create(clock, TmchCaMode.PILOT, EppMetric.builderForRequest("request-id-1", clock)); } - FakesAndMocksModule(FakeClock clock, TmchCaMode tmchCaMode) { - this.clock = clock; - this.domainFlowTmchUtils = + public static FakesAndMocksModule create( + FakeClock clock, TmchCaMode tmchCaMode, EppMetric.Builder eppMetricBuilder) { + FakesAndMocksModule instance = new FakesAndMocksModule(); + instance.clock = clock; + instance.domainFlowTmchUtils = new DomainFlowTmchUtils(new TmchXmlSignature(new TmchCertificateAuthority(tmchCaMode))); - this.sleeper = new FakeSleeper(clock); - this.dnsQueue = DnsQueue.create(); - this.metricBuilder = EppMetric.builderForRequest("request-id-1", clock); - this.modulesService = mock(ModulesService.class); - this.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class); + instance.sleeper = new FakeSleeper(clock); + instance.dnsQueue = DnsQueue.create(); + instance.metricBuilder = eppMetricBuilder; + instance.modulesService = mock(ModulesService.class); + instance.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class); + return instance; } @Provides diff --git a/javatests/google/registry/flows/FlowTestCase.java b/javatests/google/registry/flows/FlowTestCase.java index 99ac8012b..12e0858f3 100644 --- a/javatests/google/registry/flows/FlowTestCase.java +++ b/javatests/google/registry/flows/FlowTestCase.java @@ -288,11 +288,14 @@ public abstract class FlowTestCase extends ShardableTestCase { .isEqualTo(new TypeInstantiator(getClass()){}.getExactType()); // Run the flow. return DaggerEppTestComponent.builder() - .fakesAndMocksModule(new FakesAndMocksModule(clock, tmchCaMode)) + .fakesAndMocksModule( + FakesAndMocksModule.create( + clock, tmchCaMode, EppMetric.builderForRequest("request-id-1", clock))) .build() .startRequest() .flowComponentBuilder() - .flowModule(new FlowModule.Builder() + .flowModule( + new FlowModule.Builder() .setSessionMetadata(sessionMetadata) .setCredentials(credentials) .setEppRequestSource(eppRequestSource) @@ -301,9 +304,9 @@ public abstract class FlowTestCase extends ShardableTestCase { .setInputXmlBytes(eppLoader.getEppXml().getBytes(UTF_8)) .setEppInput(eppLoader.getEpp()) .build()) - .build() - .flowRunner() - .run(eppMetricBuilder); + .build() + .flowRunner() + .run(eppMetricBuilder); } /** Run a flow and marshal the result to EPP, or throw if it doesn't validate. */