Decouple GaeUserCredentials from UserService and simplify tests

This disentangles GaeUserCredentials and UserService, which lets us remove a
bunch of hacky and brittle code from LoginFlowViaConsoleTest.

Previously, GaeUserCredentials was constructed for a user, but then was still
directly calling UserService to check if the user was an admin.  UserService
can be adjusted in tests (via AppEngineRule / LocalServiceTestHelper) but it's
a pain, especially to do dynamically within a single test file.  The hacky
code in LoginFlowViaConsoleTest was working around that restriction.

With this CL, you can pass into GaeUserCredentials whether the user is an
admin or not (for testing) or construct one directly from a UserService object
(for production, and for convenience in tests using an AppEngineRule user).

Note that I also changed EppConsoleAction to @Inject UserService.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=132696391

java/google/registry/flows/EppConsoleAction.java

@@ -14,8 +14,7 @@
 
 package google.registry.flows;
 
-import static com.google.appengine.api.users.UserServiceFactory.getUserService;
+import com.google.appengine.api.users.UserService;
-
 import google.registry.request.Action;
 import google.registry.request.Action.Method;
 import google.registry.request.Payload;
@@ -35,13 +34,14 @@ public class EppConsoleAction implements Runnable {
   @Inject @Payload byte[] inputXmlBytes;
   @Inject HttpSession session;
   @Inject EppRequestHandler eppRequestHandler;
+  @Inject UserService userService;
   @Inject EppConsoleAction() {}
 
   @Override
   public void run() {
     eppRequestHandler.executeEpp(
         new HttpSessionMetadata(session),
-        new GaeUserCredentials(getUserService().getCurrentUser()),
+        GaeUserCredentials.forCurrentUser(userService),
         EppRequestSource.CONSOLE,
         false,  // This endpoint is never a dry run.
         false,  // This endpoint is never a superuser.

java/google/registry/flows/GaeUserCredentials.java

@@ -14,11 +14,12 @@
 
 package google.registry.flows;
 
-import static com.google.appengine.api.users.UserServiceFactory.getUserService;
 import static com.google.common.base.MoreObjects.toStringHelper;
 import static com.google.common.base.Strings.nullToEmpty;
+import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
 
 import com.google.appengine.api.users.User;
+import com.google.appengine.api.users.UserService;
 import com.google.common.annotations.VisibleForTesting;
 import google.registry.flows.EppException.AuthenticationErrorException;
 import google.registry.model.registrar.Registrar;
@@ -28,11 +29,41 @@ import javax.annotation.Nullable;
 /** Credentials provided by {@link com.google.appengine.api.users.UserService}. */
 public class GaeUserCredentials implements TransportCredentials {
 
-  final User gaeUser;
+  private final User gaeUser;
+  private final Boolean isAdmin;
+
+  /**
+   * Create an instance for the current user, as determined by {@code UserService}.
+   *
+   * <p>Note that the current user may be null (i.e. there is no logged in user).
+   */
+  public static GaeUserCredentials forCurrentUser(UserService userService) {
+    User user = userService.getCurrentUser();
+    return new GaeUserCredentials(user, user != null ? userService.isUserAdmin() : null);
+  }
+
+  /** Create an instance that represents an explicit user (for testing purposes). */
+  @VisibleForTesting
+  public static GaeUserCredentials forTestingUser(User gaeUser, Boolean isAdmin) {
+    checkArgumentNotNull(gaeUser);
+    checkArgumentNotNull(isAdmin);
+    return new GaeUserCredentials(gaeUser, isAdmin);
+  }
+
+  /** Create an instance that represents a non-logged in user (for testing purposes). */
+  @VisibleForTesting
+  public static GaeUserCredentials forLoggedOutUser() {
+    return new GaeUserCredentials(null, null);
+  }
+
+  private GaeUserCredentials(@Nullable User gaeUser, @Nullable Boolean isAdmin) {
+    this.gaeUser = gaeUser;
+    this.isAdmin = isAdmin;
+  }
 
   @VisibleForTesting
-  public GaeUserCredentials(@Nullable User gaeUser) {
+  User getUser() {
-    this.gaeUser = gaeUser;
+    return gaeUser;
   }
 
   @Override
@@ -42,7 +73,7 @@ public class GaeUserCredentials implements TransportCredentials {
       throw new UserNotLoggedInException();
     }
     // Allow admins to act as any registrar.
-    if (getUserService().isUserAdmin()) {
+    if (Boolean.TRUE.equals(isAdmin)) {
       return;
     }
     // Check Registrar's contacts to see if any are associated with this gaeUserId.
@@ -59,6 +90,7 @@ public class GaeUserCredentials implements TransportCredentials {
   public String toString() {
     return toStringHelper(getClass())
         .add("gaeUser", gaeUser)
+        .add("isAdmin", isAdmin)
         .toString();
   }
 

javatests/google/registry/flows/EppConsoleActionTest.java

@@ -14,7 +14,7 @@
 
 
 package google.registry.flows;
-
+import static com.google.appengine.api.users.UserServiceFactory.getUserService;
 import static com.google.common.truth.Truth.assertThat;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.mockito.Matchers.eq;
@@ -49,6 +49,7 @@ public class EppConsoleActionTest extends ShardableTestCase {
     action.session = new FakeHttpSession();
     action.session.setAttribute("CLIENT_ID", "ClientIdentifier");
     action.eppRequestHandler = mock(EppRequestHandler.class);
+    action.userService = getUserService();
     action.run();
     ArgumentCaptor<TransportCredentials> credentialsCaptor =
         ArgumentCaptor.forClass(TransportCredentials.class);
@@ -60,7 +61,7 @@ public class EppConsoleActionTest extends ShardableTestCase {
         eq(false),
         eq(false),
         eq(INPUT_XML_BYTES));
-    assertThat(((GaeUserCredentials) credentialsCaptor.getValue()).gaeUser.getEmail())
+    assertThat(((GaeUserCredentials) credentialsCaptor.getValue()).getUser().getEmail())
         .isEqualTo("person@example.com");
     assertThat(metadataCaptor.getValue().getClientId()).isEqualTo("ClientIdentifier");
   }

javatests/google/registry/flows/EppLoginAdminUserTest.java

@@ -36,7 +36,7 @@ public class EppLoginAdminUserTest extends EppTestCase {
 
   @Before
   public void initTransportCredentials() {
-    setTransportCredentials(new GaeUserCredentials(getUserService().getCurrentUser()));
+    setTransportCredentials(GaeUserCredentials.forCurrentUser(getUserService()));
   }
 
   @Test

javatests/google/registry/flows/EppLoginUserTest.java

@@ -48,7 +48,7 @@ public class EppLoginUserTest extends EppTestCase {
         .setGaeUserId(user.getUserId())
         .setTypes(ImmutableSet.of(RegistrarContact.Type.ADMIN))
         .build());
-    setTransportCredentials(new GaeUserCredentials(user));
+    setTransportCredentials(GaeUserCredentials.forCurrentUser(getUserService()));
   }
 
   @Test

javatests/google/registry/flows/FlowRunnerTest.java

@@ -178,10 +178,11 @@ public class FlowRunnerTest extends ShardableTestCase {
 
   @Test
   public void testRun_legacyLoggingStatement_gaeUserCredentials() throws Exception {
-    flowRunner.credentials = new GaeUserCredentials(new User("user@example.com", "authDomain"));
+    flowRunner.credentials =
+        GaeUserCredentials.forTestingUser(new User("user@example.com", "authDomain"), false);
     flowRunner.run();
     assertThat(Splitter.on("\n\t").split(findLogMessageByPrefix(handler, "EPP Command\n\t")))
-        .contains("GaeUserCredentials{gaeUser=user@example.com}");
+        .contains("GaeUserCredentials{gaeUser=user@example.com, isAdmin=false}");
   }
 
   @Test

javatests/google/registry/flows/session/LoginFlowViaConsoleTest.java

@@ -15,19 +15,15 @@
 package google.registry.flows.session;
 
 
-import static com.google.appengine.api.users.UserServiceFactory.getUserService;
 import static google.registry.testing.DatastoreHelper.persistResource;
 
-import com.google.apphosting.api.ApiProxy;
+import com.google.appengine.api.users.User;
-import com.google.apphosting.api.ApiProxy.Environment;
 import com.google.common.collect.ImmutableSet;
 import google.registry.flows.GaeUserCredentials;
 import google.registry.flows.GaeUserCredentials.BadGaeUserIdException;
 import google.registry.flows.GaeUserCredentials.UserNotLoggedInException;
 import google.registry.model.registrar.Registrar;
 import google.registry.model.registrar.RegistrarContact;
-import java.util.HashMap;
-import java.util.Map;
 import org.junit.Test;
 
 /**
@@ -39,123 +35,43 @@ public class LoginFlowViaConsoleTest extends LoginFlowTestCase {
   private static final String GAE_USER_ID1 = "12345";
   private static final String GAE_USER_ID2 = "54321";
 
-  Environment oldEnv = null;
-
   @Test
   public void testSuccess_withLoginAndLinkedAccount() throws Exception {
     persistLinkedAccount("person@example.com", GAE_USER_ID1);
-    login("person", "example.com", GAE_USER_ID1);
+    credentials =
-    try {
+        GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false);
     doSuccessfulTest("login_valid.xml");
-    } finally {
-      ApiProxy.setEnvironmentForCurrentThread(oldEnv);
-    }
   }
 
   @Test
   public void testFailure_withoutLoginAndLinkedAccount() throws Exception {
     persistLinkedAccount("person@example.com", GAE_USER_ID1);
-    noLogin();
+    credentials = GaeUserCredentials.forLoggedOutUser();
     doFailingTest("login_valid.xml", UserNotLoggedInException.class);
   }
 
   @Test
   public void testFailure_withoutLoginAndWithoutLinkedAccount() throws Exception {
-    noLogin();
+    credentials = GaeUserCredentials.forLoggedOutUser();
     doFailingTest("login_valid.xml", UserNotLoggedInException.class);
   }
 
   @Test
   public void testFailure_withLoginAndWithoutLinkedAccount() throws Exception {
-    login("person", "example.com", GAE_USER_ID1);
+    credentials =
-    try {
+        GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false);
     doFailingTest("login_valid.xml", BadGaeUserIdException.class);
-    } finally {
-      ApiProxy.setEnvironmentForCurrentThread(oldEnv);
-    }
   }
 
   @Test
   public void testFailure_withLoginAndNoMatchingLinkedAccount() throws Exception {
     persistLinkedAccount("joe@example.com", GAE_USER_ID2);
-    login("person", "example.com", GAE_USER_ID1);
+    credentials =
-    try {
+        GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false);
     doFailingTest("login_valid.xml", BadGaeUserIdException.class);
-    } finally {
-      ApiProxy.setEnvironmentForCurrentThread(oldEnv);
-    }
   }
 
-  Environment login(final String name, final String authDomain, final String gaeUserId) {
+  private void persistLinkedAccount(String email, String gaeUserId) {
-    oldEnv = ApiProxy.getCurrentEnvironment();
-    // This envAttr thing is the only way to set userId.
-    // see https://code.google.com/p/googleappengine/issues/detail?id=3579
-    final HashMap<String, Object> envAttr = new HashMap<>(oldEnv.getAttributes());
-    envAttr.put("com.google.appengine.api.users.UserService.user_id_key", gaeUserId);
-
-    final Environment e = oldEnv;
-    ApiProxy.setEnvironmentForCurrentThread(new Environment() {
-      @Override
-      public String getAppId() {
-        return e.getAppId();
-      }
-
-      @Override
-      public String getModuleId() {
-        return e.getModuleId();
-      }
-
-      @Override
-      public String getVersionId() {
-        return e.getVersionId();
-      }
-
-      @Override
-      public String getEmail() {
-        return name + "@" + authDomain;
-      }
-
-      @Override
-      public boolean isLoggedIn() {
-        return true;
-      }
-
-      @Override
-      public boolean isAdmin() {
-        return e.isAdmin();
-      }
-
-      @Override
-      public String getAuthDomain() {
-        return authDomain;
-      }
-
-      @Override
-      @SuppressWarnings("deprecation")
-      public String getRequestNamespace() {
-        return e.getRequestNamespace();
-      }
-
-      @Override
-      public long getRemainingMillis() {
-        return e.getRemainingMillis();
-      }
-
-      @Override
-      public Map<String, Object> getAttributes() {
-        return envAttr;
-      }
-    });
-    credentials = new GaeUserCredentials(getUserService().getCurrentUser());
-    return oldEnv;
-  }
-
-  void noLogin() {
-    oldEnv = ApiProxy.getCurrentEnvironment();
-    credentials = new GaeUserCredentials(getUserService().getCurrentUser());
-  }
-
-  void persistLinkedAccount(String email, String gaeUserId) {
     Registrar registrar = Registrar.loadByClientId("NewRegistrar");
     RegistrarContact c = new RegistrarContact.Builder()
         .setParent(registrar)