diff --git a/java/google/registry/config/RegistryConfig.java b/java/google/registry/config/RegistryConfig.java index 5dd547e26..b153eff2c 100644 --- a/java/google/registry/config/RegistryConfig.java +++ b/java/google/registry/config/RegistryConfig.java @@ -22,6 +22,7 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Ascii; import com.google.common.base.Splitter; +import com.google.common.base.Strings; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -424,8 +425,9 @@ public final class RegistryConfig { */ @Provides @Config("gSuiteSupportGroupEmailAddress") - public static String provideGSuiteSupportGroupEmailAddress(RegistryConfigSettings config) { - return config.gSuite.supportGroupEmailAddress; + public static Optional provideGSuiteSupportGroupEmailAddress( + RegistryConfigSettings config) { + return Optional.ofNullable(Strings.emptyToNull(config.gSuite.supportGroupEmailAddress)); } /** diff --git a/java/google/registry/config/files/nomulus-config-unittest.yaml b/java/google/registry/config/files/nomulus-config-unittest.yaml index a41fd9cf6..00501f26e 100644 --- a/java/google/registry/config/files/nomulus-config-unittest.yaml +++ b/java/google/registry/config/files/nomulus-config-unittest.yaml @@ -22,3 +22,8 @@ caching: staticPremiumListMaxCachedEntries: 50 eppResourceCachingEnabled: true eppResourceCachingSeconds: 0 + +# Remove the support G Suite group, because we don't want to try connecting to G Suite servers from +# tests +gSuite: + supportGroupEmailAddress: diff --git a/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java index b49b5f3d4..b04475a65 100644 --- a/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java +++ b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java @@ -29,6 +29,7 @@ import google.registry.config.RegistryConfig.Config; import google.registry.groups.GroupsConnection; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarContact; +import java.util.Optional; import javax.annotation.concurrent.Immutable; import javax.inject.Inject; @@ -43,8 +44,16 @@ import javax.inject.Inject; * *

An "admin" also has ADMIN role on ALL registrars. * - *

A user is an "admin" if they are a GAE-admin, or if their email is in the "Support" G-Suite + *

A user is an "admin" if they are a GAE-admin, or if their email is in the "Support" G Suite * group. + * + *

NOTE: to check whether the user is in the "Support" G Suite group, we need a connection to + * G Suite. This in turn requires we have valid JsonCredentials, which not all environments have set + * up. This connection will be created lazily (only if needed). + * + *

Specifically, we don't instantiate the connection if: (a) gSuiteSupportGroupEmailAddress isn't + * defined, or (b) the user is logged out, or (c) the user is a GAE-admin, or (d) bypassAdminCheck + * is true. */ @Immutable public class AuthenticatedRegistrarAccessor { @@ -69,40 +78,38 @@ public class AuthenticatedRegistrarAccessor { private final ImmutableSetMultimap roleMap; /** - * Overriding the injected {@link GroupsConnection} for tests. + * Bypass the "isAdmin" check making all users NOT admins. * - *

{@link GroupsConnection} needs the injected DelegatedCredential GoogleCredential. However, - * this can't be initialized in the test environment. + *

Currently our test server doesn't let you change the user after the test server was created. + * This means we'd need multiple test files to test the same actions as both a "regular" user and + * an admin. * - *

The test server used in the javatests/google/registry/webdriver/ tests will hang if we try - * to instantiate the {@link GroupsConnection}. So instead we inject a {@link Lazy} version and - * allow tests to override the injected instace with (presumabley) a mock insteance. + *

To overcome this - we add a flag that lets you dynamically choose whether a user is an admin + * or not by creating a fake "GAE-admin" user and then bypassing the admin check if they want to + * fake a "regular" user. + * + *

The reason we don't do it the other way around (have a flag that makes anyone an admin) is + * that such a flag would be a security risk, especially since VisibleForTesting is unenforced + * (and you could set it with reflection anyway). + * + *

Instead of having a test flag that elevates permissions (which has security concerns) we add + * this flag that reduces permissions. */ - @VisibleForTesting public static GroupsConnection overrideGroupsConnection = null; + @VisibleForTesting public static boolean bypassAdminCheck = false; @Inject public AuthenticatedRegistrarAccessor( AuthResult authResult, @Config("registryAdminClientId") String registryAdminClientId, - @Config("gSuiteSupportGroupEmailAddress") String gSuiteSupportGroupEmailAddress, - Lazy groupsConnection) { - this( - authResult, - registryAdminClientId, - gSuiteSupportGroupEmailAddress, - overrideGroupsConnection != null ? overrideGroupsConnection : groupsConnection.get()); - } - - @VisibleForTesting - AuthenticatedRegistrarAccessor( - AuthResult authResult, - String registryAdminClientId, - String gSuiteSupportGroupEmailAddress, - GroupsConnection groupsConnection) { + @Config("gSuiteSupportGroupEmailAddress") Optional gSuiteSupportGroupEmailAddress, + Lazy lazyGroupsConnection) { this( authResult.userIdForLogging(), createRoleMap( - authResult, registryAdminClientId, groupsConnection, gSuiteSupportGroupEmailAddress)); + authResult, + registryAdminClientId, + lazyGroupsConnection, + gSuiteSupportGroupEmailAddress)); logger.atInfo().log( "%s has the following roles: %s", authResult.userIdForLogging(), roleMap); @@ -235,17 +242,21 @@ public class AuthenticatedRegistrarAccessor { } private static boolean checkIsSupport( - GroupsConnection groupsConnection, String userEmail, String supportEmail) { - if (Strings.isNullOrEmpty(supportEmail)) { + Lazy lazyGroupsConnection, + String userEmail, + Optional gSuiteSupportGroupEmailAddress) { + if (!gSuiteSupportGroupEmailAddress.isPresent()) { return false; } try { - return groupsConnection.isMemberOfGroup(userEmail, supportEmail); + return lazyGroupsConnection + .get() + .isMemberOfGroup(userEmail, gSuiteSupportGroupEmailAddress.get()); } catch (RuntimeException e) { logger.atSevere().withCause(e).log( "Error checking whether email %s belongs to support group %s." + " Skipping support role check", - userEmail, supportEmail); + userEmail, gSuiteSupportGroupEmailAddress); return false; } } @@ -253,8 +264,8 @@ public class AuthenticatedRegistrarAccessor { private static ImmutableSetMultimap createRoleMap( AuthResult authResult, String registryAdminClientId, - GroupsConnection groupsConnection, - String gSuiteSupportGroupEmailAddress) { + Lazy lazyGroupsConnection, + Optional gSuiteSupportGroupEmailAddress) { if (!authResult.userAuthInfo().isPresent()) { return ImmutableSetMultimap.of(); @@ -263,9 +274,14 @@ public class AuthenticatedRegistrarAccessor { UserAuthInfo userAuthInfo = authResult.userAuthInfo().get(); User user = userAuthInfo.user(); - boolean isAdmin = userAuthInfo.isUserAdmin(); - boolean isSupport = - checkIsSupport(groupsConnection, user.getEmail(), gSuiteSupportGroupEmailAddress); + // both GAE project admin and members of the gSuiteSupportGroupEmailAddress are considered + // admins for the RegistrarConsole. + boolean isAdmin = + bypassAdminCheck + ? false + : userAuthInfo.isUserAdmin() + || checkIsSupport( + lazyGroupsConnection, user.getEmail(), gSuiteSupportGroupEmailAddress); ImmutableSetMultimap.Builder builder = new ImmutableSetMultimap.Builder<>(); @@ -278,9 +294,8 @@ public class AuthenticatedRegistrarAccessor { builder.put(registryAdminClientId, Role.OWNER); } - if (isAdmin || isSupport) { - // Admins and support have ADMIN access to all registrars, and OWNER access to all non-REAL - // registrars + if (isAdmin) { + // Admins have ADMIN access to all registrars, and OWNER access to all non-REAL registrars ofy() .load() .type(Registrar.class) diff --git a/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java b/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java index 4845b4f61..199614e01 100644 --- a/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java +++ b/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java @@ -23,9 +23,8 @@ import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.JUnitBackports.assertThrows; import static google.registry.testing.LogsSubject.assertAboutLogs; import static org.mockito.Matchers.any; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; import com.google.appengine.api.users.User; @@ -33,11 +32,14 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.flogger.LoggerConfig; import com.google.common.testing.NullPointerTester; import com.google.common.testing.TestLogHandler; +import dagger.Lazy; import google.registry.groups.GroupsConnection; import google.registry.model.registrar.Registrar; import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException; import google.registry.testing.AppEngineRule; import google.registry.testing.InjectRule; +import google.registry.testing.MockitoJUnitRule; +import java.util.Optional; import java.util.logging.Level; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -47,6 +49,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mock; /** Unit tests for {@link AuthenticatedRegistrarAccessor}. */ @RunWith(JUnit4.class) @@ -54,16 +57,19 @@ public class AuthenticatedRegistrarAccessorTest { @Rule public final AppEngineRule appEngine = AppEngineRule.builder().withDatastore().build(); @Rule public final InjectRule inject = new InjectRule(); + @Rule public final MockitoJUnitRule mocks = MockitoJUnitRule.create(); + + @Mock private HttpServletRequest req; + @Mock private HttpServletResponse rsp; + @Mock private GroupsConnection groupsConnection; + @Mock private Lazy lazyGroupsConnection; - private final HttpServletRequest req = mock(HttpServletRequest.class); - private final HttpServletResponse rsp = mock(HttpServletResponse.class); - private final GroupsConnection groupsConnection = mock(GroupsConnection.class); private final TestLogHandler testLogHandler = new TestLogHandler(); private static final AuthResult USER = createAuthResult(false); private static final AuthResult GAE_ADMIN = createAuthResult(true); private static final AuthResult NO_USER = AuthResult.create(AuthLevel.NONE); - private static final String SUPPORT_GROUP = "support@registry.example"; + private static final Optional SUPPORT_GROUP = Optional.of("support@registry.example"); /** Client ID of a REAL registrar with a RegistrarContact for USER and GAE_ADMIN. */ private static final String CLIENT_ID_WITH_CONTACT = "TheRegistrar"; /** Client ID of a REAL registrar without a RegistrarContact. */ @@ -95,6 +101,7 @@ public class AuthenticatedRegistrarAccessorTest { @Before public void before() { + when(lazyGroupsConnection.get()).thenReturn(groupsConnection); LoggerConfig.getConfig(AuthenticatedRegistrarAccessor.class).addHandler(testLogHandler); // persistResource(loadRegistrar(ADMIN_CLIENT_ID)); persistResource( @@ -124,10 +131,11 @@ public class AuthenticatedRegistrarAccessorTest { public void getAllClientIdWithAccess_user() { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER); + verify(lazyGroupsConnection).get(); } /** Logged out users don't have access to anything. */ @@ -135,9 +143,10 @@ public class AuthenticatedRegistrarAccessorTest { public void getAllClientIdWithAccess_loggedOutUser() { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - NO_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + NO_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty(); + verifyZeroInteractions(lazyGroupsConnection); } /** @@ -155,7 +164,7 @@ public class AuthenticatedRegistrarAccessorTest { public void getAllClientIdWithAccess_gaeAdmin() { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - GAE_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + GAE_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly( @@ -169,6 +178,7 @@ public class AuthenticatedRegistrarAccessorTest { ADMIN_CLIENT_ID, ADMIN, ADMIN_CLIENT_ID, OWNER); + verifyZeroInteractions(lazyGroupsConnection); } /** @@ -184,10 +194,10 @@ public class AuthenticatedRegistrarAccessorTest { */ @Test public void getAllClientIdWithAccess_userInSupportGroup() { - when(groupsConnection.isMemberOfGroup("user@gmail.com", SUPPORT_GROUP)).thenReturn(true); + when(groupsConnection.isMemberOfGroup("user@gmail.com", SUPPORT_GROUP.get())).thenReturn(true); AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly( @@ -201,18 +211,20 @@ public class AuthenticatedRegistrarAccessorTest { ADMIN_CLIENT_ID, ADMIN, ADMIN_CLIENT_ID, OWNER); + verify(lazyGroupsConnection).get(); } - /** Empty Support group email - skips check. */ + /** Empty Support group email - skips check and doesn't generate the lazy. */ @Test public void getAllClientIdWithAccess_emptySupportEmail_works() { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - USER, ADMIN_CLIENT_ID, "", groupsConnection); + USER, ADMIN_CLIENT_ID, Optional.empty(), lazyGroupsConnection); - verifyNoMoreInteractions(groupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER); + // Make sure we didn't instantiate the lazyGroupsConnection + verifyZeroInteractions(lazyGroupsConnection); } /** Support group check throws - continue anyway. */ @@ -221,11 +233,12 @@ public class AuthenticatedRegistrarAccessorTest { when(groupsConnection.isMemberOfGroup(any(), any())).thenThrow(new RuntimeException("blah")); AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); - verify(groupsConnection).isMemberOfGroup("user@gmail.com", SUPPORT_GROUP); + verify(groupsConnection).isMemberOfGroup("user@gmail.com", SUPPORT_GROUP.get()); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER); + verify(lazyGroupsConnection).get(); } /** Fail loading registrar if user doesn't have access to it. */ @@ -235,6 +248,7 @@ public class AuthenticatedRegistrarAccessorTest { REAL_CLIENT_ID_WITHOUT_CONTACT, USER, "user user@gmail.com doesn't have access to registrar NewRegistrar"); + verify(lazyGroupsConnection).get(); } /** Fail loading registrar if user doesn't have access to it, even if it's not REAL. */ @@ -244,6 +258,7 @@ public class AuthenticatedRegistrarAccessorTest { OTE_CLIENT_ID_WITHOUT_CONTACT, USER, "user user@gmail.com doesn't have access to registrar OteRegistrar"); + verify(lazyGroupsConnection).get(); } /** Fail loading registrar if there's no user associated with the request. */ @@ -253,6 +268,7 @@ public class AuthenticatedRegistrarAccessorTest { CLIENT_ID_WITH_CONTACT, NO_USER, " doesn't have access to registrar TheRegistrar"); + verifyZeroInteractions(lazyGroupsConnection); } /** Succeed loading registrar if user has access to it. */ @@ -262,6 +278,7 @@ public class AuthenticatedRegistrarAccessorTest { CLIENT_ID_WITH_CONTACT, USER, "user user@gmail.com has [OWNER] access to registrar TheRegistrar"); + verify(lazyGroupsConnection).get(); } /** Succeed loading registrar if admin with access. */ @@ -271,6 +288,7 @@ public class AuthenticatedRegistrarAccessorTest { CLIENT_ID_WITH_CONTACT, GAE_ADMIN, "admin admin@gmail.com has [OWNER, ADMIN] access to registrar TheRegistrar"); + verifyZeroInteractions(lazyGroupsConnection); } /** Succeed loading registrar for admin even if they aren't on the approved contacts list. */ @@ -280,6 +298,7 @@ public class AuthenticatedRegistrarAccessorTest { REAL_CLIENT_ID_WITHOUT_CONTACT, GAE_ADMIN, "admin admin@gmail.com has [ADMIN] access to registrar NewRegistrar."); + verifyZeroInteractions(lazyGroupsConnection); } /** Succeed loading non-REAL registrar for admin. */ @@ -289,6 +308,7 @@ public class AuthenticatedRegistrarAccessorTest { OTE_CLIENT_ID_WITHOUT_CONTACT, GAE_ADMIN, "admin admin@gmail.com has [OWNER, ADMIN] access to registrar OteRegistrar."); + verifyZeroInteractions(lazyGroupsConnection); } /** Fail loading registrar even if admin, if registrar doesn't exist. */ @@ -298,13 +318,14 @@ public class AuthenticatedRegistrarAccessorTest { "BadClientId", GAE_ADMIN, "admin admin@gmail.com doesn't have access to registrar BadClientId"); + verifyZeroInteractions(lazyGroupsConnection); } private void expectGetRegistrarSuccess(String clientId, AuthResult authResult, String message) throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); // make sure loading the registrar succeeds and returns a value assertThat(registrarAccessor.getRegistrar(clientId)).isNotNull(); @@ -315,7 +336,7 @@ public class AuthenticatedRegistrarAccessorTest { String clientId, AuthResult authResult, String message) { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( - authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, lazyGroupsConnection); // make sure getRegistrar fails RegistrarAccessDeniedException exception = diff --git a/javatests/google/registry/request/auth/BUILD b/javatests/google/registry/request/auth/BUILD index f048d9a2a..468154397 100644 --- a/javatests/google/registry/request/auth/BUILD +++ b/javatests/google/registry/request/auth/BUILD @@ -21,6 +21,7 @@ java_library( "@com_google_appengine_api_1_0_sdk", "@com_google_appengine_tools_appengine_gcs_client", "@com_google_appengine_tools_sdk", + "@com_google_dagger", "@com_google_flogger", "@com_google_flogger_system_backend", "@com_google_guava",