From ceb5c2117e7309c1153d2d7fd6268b4e19cad4ff Mon Sep 17 00:00:00 2001 From: nickfelt Date: Fri, 9 Sep 2016 11:26:47 -0700 Subject: [PATCH] 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 --- .../registry/flows/EppConsoleAction.java | 6 +- .../registry/flows/GaeUserCredentials.java | 42 ++++++- .../registry/flows/EppConsoleActionTest.java | 5 +- .../registry/flows/EppLoginAdminUserTest.java | 2 +- .../registry/flows/EppLoginUserTest.java | 2 +- .../google/registry/flows/FlowRunnerTest.java | 5 +- .../session/LoginFlowViaConsoleTest.java | 110 +++--------------- 7 files changed, 61 insertions(+), 111 deletions(-) diff --git a/java/google/registry/flows/EppConsoleAction.java b/java/google/registry/flows/EppConsoleAction.java index 6e0061d31..8bffa4be9 100644 --- a/java/google/registry/flows/EppConsoleAction.java +++ b/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. diff --git a/java/google/registry/flows/GaeUserCredentials.java b/java/google/registry/flows/GaeUserCredentials.java index 2d4d91345..62908d59e 100644 --- a/java/google/registry/flows/GaeUserCredentials.java +++ b/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}. + * + *

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) { - this.gaeUser = gaeUser; + User getUser() { + 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(); } diff --git a/javatests/google/registry/flows/EppConsoleActionTest.java b/javatests/google/registry/flows/EppConsoleActionTest.java index d03892689..daf937433 100644 --- a/javatests/google/registry/flows/EppConsoleActionTest.java +++ b/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 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"); } diff --git a/javatests/google/registry/flows/EppLoginAdminUserTest.java b/javatests/google/registry/flows/EppLoginAdminUserTest.java index b7bc34a41..9ca703028 100644 --- a/javatests/google/registry/flows/EppLoginAdminUserTest.java +++ b/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 diff --git a/javatests/google/registry/flows/EppLoginUserTest.java b/javatests/google/registry/flows/EppLoginUserTest.java index c10a0043b..affa90d16 100644 --- a/javatests/google/registry/flows/EppLoginUserTest.java +++ b/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 diff --git a/javatests/google/registry/flows/FlowRunnerTest.java b/javatests/google/registry/flows/FlowRunnerTest.java index 7141ea34b..0caf5a94e 100644 --- a/javatests/google/registry/flows/FlowRunnerTest.java +++ b/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 diff --git a/javatests/google/registry/flows/session/LoginFlowViaConsoleTest.java b/javatests/google/registry/flows/session/LoginFlowViaConsoleTest.java index 8db4726d3..b3c97a4a1 100644 --- a/javatests/google/registry/flows/session/LoginFlowViaConsoleTest.java +++ b/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.apphosting.api.ApiProxy.Environment; +import com.google.appengine.api.users.User; 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); - try { - doSuccessfulTest("login_valid.xml"); - } finally { - ApiProxy.setEnvironmentForCurrentThread(oldEnv); - } + credentials = + GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false); + doSuccessfulTest("login_valid.xml"); } @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); - try { - doFailingTest("login_valid.xml", BadGaeUserIdException.class); - } finally { - ApiProxy.setEnvironmentForCurrentThread(oldEnv); - } + credentials = + GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false); + doFailingTest("login_valid.xml", BadGaeUserIdException.class); } @Test public void testFailure_withLoginAndNoMatchingLinkedAccount() throws Exception { persistLinkedAccount("joe@example.com", GAE_USER_ID2); - login("person", "example.com", GAE_USER_ID1); - try { - doFailingTest("login_valid.xml", BadGaeUserIdException.class); - } finally { - ApiProxy.setEnvironmentForCurrentThread(oldEnv); - } + credentials = + GaeUserCredentials.forTestingUser(new User("person", "example.com", GAE_USER_ID1), false); + doFailingTest("login_valid.xml", BadGaeUserIdException.class); } - Environment login(final String name, final String authDomain, final 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 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 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) { + private void persistLinkedAccount(String email, String gaeUserId) { Registrar registrar = Registrar.loadByClientId("NewRegistrar"); RegistrarContact c = new RegistrarContact.Builder() .setParent(registrar)