Allow admins read-only access to all registrars

We want to be able to view / test / debug how the registrar console looks for our clients.

However, we don't want to accidentally change the data for registrars, especially in a "non-accountable" way (where we later don't know who did that change)

So we do 2 things here:

- Add a "mode" (read-only and read-write) to the getRegistrarForUser function. We set it according to what we want to do with the registrar. Currently, read-write is only requested for the "update" RegistrarSetting action. Admins will have read-only access to all registrars, but read-write access only to the "admin registrar" (or whatever registrar they are contacts for).

- Support an undocumented "clientId=XXX" query param that replaces the "guessClientIdForUser" function in the original page load. We can then set it when we want to view a different account.

We also change the navigation links on the HTML page to preserve the query.

-------------------------

This might be used also for a better user experience for our clients, especially those with multiple "clientId"s (some registrar entities have multiple "registrar" objects)

Currently, they have to have a separate user for each clientId, and only have one user allowed which has both read and write permissions.

Using this change, we can give them the possibility to add users on their own, some with read-only access (to view billing information without being able to change anything), and use a single user for all their clientIds.

-------------------------

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

javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java

@@ -17,6 +17,7 @@ package google.registry.ui.server.registrar;
 import static com.google.common.net.HttpHeaders.LOCATION;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;
+import static google.registry.ui.server.registrar.SessionUtils.AccessType.READ_ONLY;
 import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
@@ -34,6 +35,7 @@ import google.registry.testing.AppEngineRule;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeResponse;
 import google.registry.testing.UserInfo;
+import java.util.Optional;
 import javax.servlet.http.HttpServletRequest;
 import org.junit.Before;
 import org.junit.Rule;
@@ -72,10 +74,11 @@ public class ConsoleUiActionTest {
     action.sessionUtils = sessionUtils;
     action.userService = UserServiceFactory.getUserService();
     action.xsrfTokenManager = new XsrfTokenManager(new FakeClock(), action.userService);
+    action.paramClientId = Optional.empty();
     AuthResult authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false));
     action.authResult = authResult;
     when(sessionUtils.guessClientIdForUser(authResult)).thenReturn("TheRegistrar");
-    when(sessionUtils.getRegistrarForUser("TheRegistrar", authResult))
+    when(sessionUtils.getRegistrarForUser("TheRegistrar", READ_ONLY, authResult))
         .thenReturn(loadRegistrar("TheRegistrar"));
   }
 
@@ -117,6 +120,7 @@ public class ConsoleUiActionTest {
         .thenThrow(new ForbiddenException("forbidden"));
     action.run();
     assertThat(response.getPayload()).contains("<h1>You need permission</h1>");
+    assertThat(response.getPayload()).contains("not associated with Nomulus.");
   }
 
   @Test
@@ -136,4 +140,24 @@ public class ConsoleUiActionTest {
     assertThat(response.getStatus()).isEqualTo(SC_MOVED_TEMPORARILY);
     assertThat(response.getHeaders().get(LOCATION)).isEqualTo("/");
   }
+
+  @Test
+  public void testSettingClientId_notAllowed_showsNeedPermissionPage() {
+    action.paramClientId = Optional.of("OtherClientId");
+    when(sessionUtils.getRegistrarForUser("OtherClientId", READ_ONLY, action.authResult))
+        .thenThrow(new ForbiddenException("forbidden"));
+    action.run();
+    assertThat(response.getPayload()).contains("<h1>You need permission</h1>");
+    assertThat(response.getPayload()).contains("not associated with the registrar OtherClientId.");
+  }
+
+  @Test
+  public void testSettingClientId_allowed_showsRegistrarConsole() {
+    action.paramClientId = Optional.of("OtherClientId");
+    when(sessionUtils.getRegistrarForUser("OtherClientId", READ_ONLY, action.authResult))
+        .thenReturn(loadRegistrar("TheRegistrar"));
+    action.run();
+    assertThat(response.getPayload()).contains("Registrar Console");
+    assertThat(response.getPayload()).contains("reg-content-and-footer");
+  }
 }

javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java

@@ -92,7 +92,19 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   /** This is the default read test for the registrar settings actions. */
   @Test
-  public void testSuccess_readRegistrarInfo_authorized() {
+  public void testSuccess_readRegistrarInfo_authorizedReadWrite() {
+    Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID));
+    assertThat(response)
+        .containsExactly(
+            "status", "SUCCESS",
+            "message", "Success",
+            "results", asList(loadRegistrar(CLIENT_ID).toJsonMap()));
+  }
+
+  /** This is the default read test for the registrar settings actions. */
+  @Test
+  public void testSuccess_readRegistrarInfo_authorizedReadOnly() {
+    action.authResult = USER_READ_ONLY;
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID));
     assertThat(response)
         .containsExactly(
@@ -144,7 +156,7 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   }
 
   @Test
-  public void testFailute_updateRegistrarInfo_notAuthorized() {
+  public void testFailure_updateRegistrarInfo_notAuthorized() {
     action.authResult = USER_UNAUTHORIZED;
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
@@ -157,6 +169,20 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
     assertNoTasksEnqueued("sheet");
   }
 
+  @Test
+  public void testFailure_updateRegistrarInfo_readOnlyAccess() {
+    action.authResult = USER_READ_ONLY;
+    Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
+        "op", "update",
+        "id", CLIENT_ID,
+        "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime())));
+    assertThat(response).containsExactly(
+        "status", "ERROR",
+        "results", ImmutableList.of(),
+        "message", "forbidden test error");
+    assertNoTasksEnqueued("sheet");
+  }
+
   @Test
   public void testUpdate_badEmail_errorEmailField() {
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(

javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -18,6 +18,8 @@ import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailAddres
 import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDisplayName;
 import static google.registry.security.JsonHttpTestUtils.createJsonPayload;
 import static google.registry.testing.DatastoreHelper.loadRegistrar;
+import static google.registry.ui.server.registrar.SessionUtils.AccessType.READ_ONLY;
+import static google.registry.ui.server.registrar.SessionUtils.AccessType.READ_WRITE;
 import static org.mockito.Mockito.when;
 
 import com.google.appengine.api.users.User;
@@ -58,12 +60,14 @@ public class RegistrarSettingsActionTestCase {
 
   static final String CLIENT_ID = "TheRegistrar";
 
-  static final AuthResult USER_AUTHORIZED =
-      AuthResult.create(AuthLevel.USER, UserAuthInfo.create(new User("user", "gmail.com"), false));
+  static final AuthResult USER_READ_WRITE =
+      AuthResult.create(AuthLevel.USER, UserAuthInfo.create(new User("rw", "gmail.com"), false));
+
+  static final AuthResult USER_READ_ONLY =
+      AuthResult.create(AuthLevel.USER, UserAuthInfo.create(new User("ro", "gmail.com"), false));
 
   static final AuthResult USER_UNAUTHORIZED =
-      AuthResult.create(
-          AuthLevel.USER, UserAuthInfo.create(new User("unauthorized", "gmail.com"), false));
+      AuthResult.create(AuthLevel.USER, UserAuthInfo.create(new User("evil", "gmail.com"), false));
 
   @Rule
   public final AppEngineRule appEngine =
@@ -90,7 +94,9 @@ public class RegistrarSettingsActionTestCase {
     action.sessionUtils = sessionUtils;
     action.appEngineServiceUtils = appEngineServiceUtils;
     when(appEngineServiceUtils.getCurrentVersionHostname("backend")).thenReturn("backend.hostname");
-    action.authResult = USER_AUTHORIZED;
+    // We set the default user to one with read-write access, as that's the most common test case.
+    // When we want to specifically check read-only or unauthorized, we can switch the user here.
+    action.authResult = USER_READ_WRITE;
     action.jsonActionRunner = new JsonActionRunner(
         ImmutableMap.of(), new JsonResponse(new ResponseImpl(rsp)));
     action.registrarChangesNotificationEmailAddresses = ImmutableList.of(
@@ -109,9 +115,19 @@ public class RegistrarSettingsActionTestCase {
     // the result is out of date after mutations.
     // (for example, if someone wants to change the registrar to prepare for a test, the function
     // would still return the old value)
-    when(sessionUtils.getRegistrarForUser(CLIENT_ID, USER_AUTHORIZED))
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_ONLY, USER_READ_WRITE))
         .thenAnswer(x -> loadRegistrar(CLIENT_ID));
-    when(sessionUtils.getRegistrarForUser(CLIENT_ID, USER_UNAUTHORIZED))
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_WRITE, USER_READ_WRITE))
+        .thenAnswer(x -> loadRegistrar(CLIENT_ID));
+
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_ONLY, USER_READ_ONLY))
+        .thenAnswer(x -> loadRegistrar(CLIENT_ID));
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_WRITE, USER_READ_ONLY))
+        .thenThrow(new ForbiddenException("forbidden test error"));
+
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_ONLY, USER_UNAUTHORIZED))
+        .thenThrow(new ForbiddenException("forbidden test error"));
+    when(sessionUtils.getRegistrarForUser(CLIENT_ID, READ_WRITE, USER_UNAUTHORIZED))
         .thenThrow(new ForbiddenException("forbidden test error"));
   }
 }

javatests/google/registry/ui/server/registrar/SessionUtilsTest.java

@@ -20,6 +20,8 @@ import static google.registry.testing.DatastoreHelper.loadRegistrar;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.JUnitBackports.assertThrows;
 import static google.registry.testing.LogsSubject.assertAboutLogs;
+import static google.registry.ui.server.registrar.SessionUtils.AccessType.READ_ONLY;
+import static google.registry.ui.server.registrar.SessionUtils.AccessType.READ_WRITE;
 import static org.mockito.Mockito.mock;
 
 import com.google.appengine.api.users.User;
@@ -32,6 +34,7 @@ import google.registry.request.auth.AuthResult;
 import google.registry.request.auth.UserAuthInfo;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.InjectRule;
+import google.registry.ui.server.registrar.SessionUtils.AccessType;
 import java.util.logging.Level;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -96,66 +99,121 @@ public class SessionUtilsTest {
 
   /** Fail loading registrar if user doesn't have access to it. */
   @Test
-  public void testGetRegistrarForUser_noAccess_isNotAdmin() {
+  public void testGetRegistrarForUser_readOnly_noAccess_isNotAdmin() {
     expectGetRegistrarFailure(
         DEFAULT_CLIENT_ID,
+        READ_ONLY,
         UNAUTHORIZED_USER,
-        "User {user} doesn't have access to registrar {clientId}");
+        "User {user} doesn't have READ_ONLY access to registrar {clientId}");
+  }
+
+  /** Fail loading registrar if user doesn't have access to it. */
+  @Test
+  public void testGetRegistrarForUser_readWrite_noAccess_isNotAdmin() {
+    expectGetRegistrarFailure(
+        DEFAULT_CLIENT_ID,
+        READ_WRITE,
+        UNAUTHORIZED_USER,
+        "User {user} doesn't have READ_WRITE access to registrar {clientId}");
   }
 
   /** Fail loading registrar if there's no user associated with the request. */
   @Test
-  public void testGetRegistrarForUser_noUser() {
-    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, NO_USER, "Not logged in");
+  public void testGetRegistrarForUser_readOnly_noUser() {
+    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, READ_ONLY, NO_USER, "Not logged in");
   }
 
-  /** Succeed loading registrar if user has access to it. */
+  /** Fail loading registrar if there's no user associated with the request. */
   @Test
-  public void testGetRegistrarForUser_hasAccess_isNotAdmin() {
-    expectGetRegistrarSuccess(AUTHORIZED_USER, "User {user} has access to registrar {clientId}");
+  public void testGetRegistrarForUser_readWrite_noUser() {
+    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, READ_WRITE, NO_USER, "Not logged in");
+
+    assertAboutLogs().that(testLogHandler).hasNoLogsAtLevel(Level.INFO);
   }
 
-  /** Succeed loading registrar if admin. */
+  /** Succeed loading registrar in read-only mode if user has access to it. */
   @Test
-  public void testGetRegistrarForUser_hasAccess_isAdmin() {
-    expectGetRegistrarSuccess(AUTHORIZED_ADMIN, "User {user} has access to registrar {clientId}");
+  public void testGetRegistrarForUser_readOnly_hasAccess_isNotAdmin() {
+    expectGetRegistrarSuccess(
+        AUTHORIZED_USER, READ_ONLY, "User {user} has access to registrar {clientId}");
   }
 
-  /** Fail loading registrar if admin isn't on the approved contacts list. */
+  /** Succeed loading registrar in read-write mode if user has access to it. */
   @Test
-  public void testGetRegistrarForUser_noAccess_isAdmin() {
+  public void testGetRegistrarForUser_readWrite_hasAccess_isNotAdmin() {
+    expectGetRegistrarSuccess(
+        AUTHORIZED_USER, READ_WRITE, "User {user} has access to registrar {clientId}");
+  }
+
+  /** Succeed loading registrar in read-only mode if admin with access. */
+  @Test
+  public void testGetRegistrarForUser_readOnly_hasAccess_isAdmin() {
+    expectGetRegistrarSuccess(
+        AUTHORIZED_ADMIN, READ_ONLY, "User {user} has access to registrar {clientId}");
+  }
+
+  /** Succeed loading registrar in read-write mode if admin with access. */
+  @Test
+  public void testGetRegistrarForUser_readWrite_hasAccess_isAdmin() {
+    expectGetRegistrarSuccess(
+        AUTHORIZED_ADMIN, READ_WRITE, "User {user} has access to registrar {clientId}");
+  }
+
+  /** Succeed loading registrar for read-only when admin isn't on the approved contacts list. */
+  @Test
+  public void testGetRegistrarForUser_readOnly_noAccess_isAdmin() {
+    expectGetRegistrarSuccess(
+        UNAUTHORIZED_ADMIN,
+        READ_ONLY,
+        "Allowing admin {user} read-only access to registrar {clientId}.");
+  }
+
+  /** Fail loading registrar for read-write when admin isn't on the approved contacts list. */
+  @Test
+  public void testGetRegistrarForUser_readWrite_noAccess_isAdmin() {
     expectGetRegistrarFailure(
         DEFAULT_CLIENT_ID,
+        READ_WRITE,
         UNAUTHORIZED_ADMIN,
-        "User {user} doesn't have access to registrar {clientId}");
-  }
-
-  /** Succeed loading registrarAdmin even if unauthorized admin. */
-  @Test
-  public void testGetRegistrarForUser_registrarAdminClientId() {
-    sessionUtils.registryAdminClientId = DEFAULT_CLIENT_ID;
-    expectGetRegistrarSuccess(
-        UNAUTHORIZED_ADMIN, "Allowing admin {user} access to registrar {clientId}.");
+        "User {user} doesn't have READ_WRITE access to registrar {clientId}");
   }
 
   /** Fail loading registrar even if admin, if registrar doesn't exist. */
   @Test
-  public void testGetRegistrarForUser_doesntExist_isAdmin() {
-    expectGetRegistrarFailure("BadClientId", UNAUTHORIZED_ADMIN, "Registrar {clientId} not found");
+  public void testGetRegistrarForUser_readOnly_doesntExist_isAdmin() {
+    expectGetRegistrarFailure(
+        "BadClientId",
+        READ_ONLY,
+        AUTHORIZED_ADMIN,
+        "Registrar {clientId} not found");
   }
 
-  private void expectGetRegistrarSuccess(AuthResult authResult, String message) {
-    assertThat(sessionUtils.getRegistrarForUser(DEFAULT_CLIENT_ID, authResult)).isNotNull();
+  /** Fail loading registrar even if admin, if registrar doesn't exist. */
+  @Test
+  public void testGetRegistrarForUser_readWrite_doesntExist_isAdmin() {
+    expectGetRegistrarFailure(
+        "BadClientId",
+        READ_WRITE,
+        AUTHORIZED_ADMIN,
+        "Registrar {clientId} not found");
+  }
+
+  private void expectGetRegistrarSuccess(
+      AuthResult authResult, AccessType accessType, String message) {
+    assertThat(sessionUtils.getRegistrarForUser(DEFAULT_CLIENT_ID, accessType, authResult))
+        .isNotNull();
     assertAboutLogs()
         .that(testLogHandler)
         .hasLogAtLevelWithMessage(
             Level.INFO, formatMessage(message, authResult, DEFAULT_CLIENT_ID));
   }
 
-  private void expectGetRegistrarFailure(String clientId, AuthResult authResult, String message) {
+  private void expectGetRegistrarFailure(
+      String clientId, AccessType accessType, AuthResult authResult, String message) {
     ForbiddenException exception =
         assertThrows(
-            ForbiddenException.class, () -> sessionUtils.getRegistrarForUser(clientId, authResult));
+            ForbiddenException.class,
+            () -> sessionUtils.getRegistrarForUser(clientId, accessType, authResult));
 
     assertThat(exception).hasMessageThat().contains(formatMessage(message, authResult, clientId));
     assertAboutLogs().that(testLogHandler).hasNoLogsAtLevel(Level.INFO);
@@ -221,7 +279,7 @@ public class SessionUtilsTest {
 
   /**
    * If an admin is not associated with a registrar and the configured adminClientId points to a
-   * non-existent registrar, we still guess it (we will later failing loading the registrar).
+   * non-existent registrar, we still guess it (we will later fail loading the registrar).
    */
   @Test
   public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdInvalid() {