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

@@ -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<EppOutput>() {

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

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

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");
+  }
+}

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();
+  }
 }

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

javatests/google/registry/flows/FlowTestCase.java

@@ -288,11 +288,14 @@ public abstract class FlowTestCase<F extends Flow> extends ShardableTestCase {
         .isEqualTo(new TypeInstantiator<F>(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<F extends Flow> 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. */