Allow unsetting of the support email group, disabling "support users"

In addition to just making good sense to not have support group for some
environments (local? unittest? crash?) - connecting with G Suit requires
additional permissions that are harder to find.

Specifically, it requires the Json Credentials that just aren't set in the
Dummy Keyring used on some environments.

So we make sure to not even *try* to create the credentials if the support
email isn't set

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

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) {
+    public static Optional<String> provideGSuiteSupportGroupEmailAddress(
-      return config.gSuite.supportGroupEmailAddress;
+        RegistryConfigSettings config) {
+      return Optional.ofNullable(Strings.emptyToNull(config.gSuite.supportGroupEmailAddress));
     }
 
     /**

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:

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;
  *
  * <p>An "admin" also has ADMIN role on ALL registrars.
  *
- * <p>A user is an "admin" if they are a GAE-admin, or if their email is in the "Support" G-Suite
+ * <p>A user is an "admin" if they are a GAE-admin, or if their email is in the "Support" G Suite
  * group.
+ *
+ * <p>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).
+ *
+ * <p>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<String, Role> roleMap;
 
   /**
-   * Overriding the injected {@link GroupsConnection} for tests.
+   * Bypass the "isAdmin" check making all users NOT admins.
    *
-   * <p>{@link GroupsConnection} needs the injected DelegatedCredential GoogleCredential. However,
+   * <p>Currently our test server doesn't let you change the user after the test server was created.
-   * this can't be initialized in the test environment.
+   * This means we'd need multiple test files to test the same actions as both a "regular" user and
+   * an admin.
    *
-   * <p>The test server used in the javatests/google/registry/webdriver/ tests will hang if we try
+   * <p>To overcome this - we add a flag that lets you dynamically choose whether a user is an admin
-   * to instantiate the {@link GroupsConnection}. So instead we inject a {@link Lazy} version and
+   * or not by creating a fake "GAE-admin" user and then bypassing the admin check if they want to
-   * allow tests to override the injected instace with (presumabley) a mock insteance.
+   * fake a "regular" user.
+   *
+   * <p>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).
+   *
+   * <p>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,
+      @Config("gSuiteSupportGroupEmailAddress") Optional<String> gSuiteSupportGroupEmailAddress,
-      Lazy<GroupsConnection> groupsConnection) {
+      Lazy<GroupsConnection> lazyGroupsConnection) {
-    this(
-        authResult,
-        registryAdminClientId,
-        gSuiteSupportGroupEmailAddress,
-        overrideGroupsConnection != null ? overrideGroupsConnection : groupsConnection.get());
-  }
-
-  @VisibleForTesting
-  AuthenticatedRegistrarAccessor(
-      AuthResult authResult,
-      String registryAdminClientId,
-      String gSuiteSupportGroupEmailAddress,
-      GroupsConnection groupsConnection) {
     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) {
+      Lazy<GroupsConnection> lazyGroupsConnection,
-    if (Strings.isNullOrEmpty(supportEmail)) {
+      String userEmail,
+      Optional<String> 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<String, Role> createRoleMap(
       AuthResult authResult,
       String registryAdminClientId,
-      GroupsConnection groupsConnection,
+      Lazy<GroupsConnection> lazyGroupsConnection,
-      String gSuiteSupportGroupEmailAddress) {
+      Optional<String> 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();
+    // both GAE project admin and members of the gSuiteSupportGroupEmailAddress are considered
-    boolean isSupport =
+    // admins for the RegistrarConsole.
-        checkIsSupport(groupsConnection, user.getEmail(), gSuiteSupportGroupEmailAddress);
+    boolean isAdmin =
+        bypassAdminCheck
+            ? false
+            : userAuthInfo.isUserAdmin()
+                || checkIsSupport(
+                    lazyGroupsConnection, user.getEmail(), gSuiteSupportGroupEmailAddress);
 
     ImmutableSetMultimap.Builder<String, Role> builder = new ImmutableSetMultimap.Builder<>();
 
@@ -278,9 +294,8 @@ public class AuthenticatedRegistrarAccessor {
       builder.put(registryAdminClientId, Role.OWNER);
     }
 
-    if (isAdmin || isSupport) {
+    if (isAdmin) {
-      // Admins and support have ADMIN access to all registrars, and OWNER access to all non-REAL
+      // Admins have ADMIN access to all registrars, and OWNER access to all non-REAL registrars
-      // registrars
       ofy()
           .load()
           .type(Registrar.class)

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<GroupsConnection> 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<String> 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,
         "<logged-out 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 =

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",