Allow admins read/write access to all registrar in web console

This CL removes the "READ vs UPDATE" feature completely. Now anyone with access
has full read+write access.

We still keep track of which role a user has (did they get access "explicitly"
because they are an "allowed access" contact? Or do they have access because
they are admins?) for the logs and UI, and also so we could in the (very near)
future have features only available to admins.

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

java/google/registry/rdap/RdapActionBase.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Charsets.UTF_8;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN;
 import static google.registry.model.ofy.ObjectifyService.ofy;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.DomainNameUtils.canonicalizeDomainName;
 import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
@@ -27,6 +26,7 @@ import static javax.servlet.http.HttpServletResponse.SC_OK;
 
 import com.google.api.client.json.jackson2.JacksonFactory;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.net.MediaType;
 import com.google.re2j.Pattern;
@@ -176,23 +176,6 @@ public abstract class RdapActionBase implements Runnable {
     }
   }
 
-  /**
-   * Returns a clientId the given user has console access on, or Optional.empty if there is none.
-   */
-  private Optional<String> getAuthorizedClientId() {
-    try {
-      String clientId = registrarAccessor.guessClientId();
-      // We load the Registrar to make sure the user has access to it. We don't actually need it,
-      // we're just checking if an exception is thrown.
-      registrarAccessor.getRegistrarForUserCached(clientId, READ);
-      return Optional.of(clientId);
-    } catch (Exception e) {
-      logger.atWarning().withCause(e).log(
-          "Couldn't find registrar for User %s.", authResult.userIdForLogging());
-      return Optional.empty();
-    }
-  }
-
   void setPayload(ImmutableMap<String, Object> rdapJson) {
     if (requestMethod == Action.Method.HEAD) {
       return;
@@ -217,11 +200,12 @@ public abstract class RdapActionBase implements Runnable {
     if (userAuthInfo.isUserAdmin()) {
       return RdapAuthorization.ADMINISTRATOR_AUTHORIZATION;
     }
-    Optional<String> clientId = getAuthorizedClientId();
+    ImmutableSet<String> clientIds = registrarAccessor.getAllClientIdWithRoles().keySet();
-    if (!clientId.isPresent()) {
+    if (clientIds.isEmpty()) {
+      logger.atWarning().log("Couldn't find registrar for User %s.", authResult.userIdForLogging());
       return RdapAuthorization.PUBLIC_AUTHORIZATION;
     }
-    return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientId.get());
+    return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientIds);
   }
 
   /** Returns the registrar on which results should be filtered, or absent(). */
@@ -251,7 +235,7 @@ public abstract class RdapActionBase implements Runnable {
     if (userAuthInfo.isUserAdmin()) {
       return true;
     }
-    if (!getAuthorizedClientId().isPresent()) {
+    if (registrarAccessor.getAllClientIdWithRoles().isEmpty()) {
       return false;
     }
     return true;

java/google/registry/rdap/RdapAuthorization.java

@@ -15,7 +15,7 @@
 package google.registry.rdap;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import google.registry.model.ImmutableObject;
 
 /** Authorization information for RDAP data access. */
@@ -32,13 +32,13 @@ public abstract class RdapAuthorization extends ImmutableObject {
   public abstract Role role();
 
   /** The registrar client IDs for which access is granted (used only if the role is REGISTRAR. */
-  public abstract ImmutableList<String> clientIds();
+  public abstract ImmutableSet<String> clientIds();
 
   static RdapAuthorization create(Role role, String clientId) {
-    return new AutoValue_RdapAuthorization(role, ImmutableList.of(clientId));
+    return new AutoValue_RdapAuthorization(role, ImmutableSet.of(clientId));
   }
 
-  static RdapAuthorization create(Role role, ImmutableList<String> clientIds) {
+  static RdapAuthorization create(Role role, ImmutableSet<String> clientIds) {
     return new AutoValue_RdapAuthorization(role, clientIds);
   }
 
@@ -54,9 +54,8 @@ public abstract class RdapAuthorization extends ImmutableObject {
   }
 
   public static final RdapAuthorization PUBLIC_AUTHORIZATION =
-      create(Role.PUBLIC, ImmutableList.of());
+      create(Role.PUBLIC, ImmutableSet.of());
 
   public static final RdapAuthorization ADMINISTRATOR_AUTHORIZATION =
-      create(Role.ADMINISTRATOR, ImmutableList.of());
+      create(Role.ADMINISTRATOR, ImmutableSet.of());
 }
-

java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java

@@ -18,6 +18,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy;
 
 import com.google.appengine.api.users.User;
 import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.flogger.FluentLogger;
 import google.registry.config.RegistryConfig.Config;
@@ -26,83 +27,53 @@ import google.registry.model.registrar.RegistrarContact;
 import google.registry.request.HttpException.ForbiddenException;
 import google.registry.request.auth.AuthResult;
 import google.registry.request.auth.UserAuthInfo;
-import java.util.Optional;
-import java.util.function.Function;
 import javax.annotation.concurrent.Immutable;
 import javax.inject.Inject;
 
 /**
  * Allows access only to {@link Registrar}s the current user has access to.
  *
- * <p>A user has read+write access to a Registrar if there exists a {@link RegistrarContact} with
+ * <p>A user has OWNER role on a Registrar if there exists a {@link RegistrarContact} with
  * that user's gaeId and the registrar as a parent.
  *
- * <p>An admin has in addition read+write access to {@link #registryAdminClientId}.
+ * <p>An admin has in addition OWNER role on {@link #registryAdminClientId}.
  *
- * <p>An admin also has read access to ALL registrars.
+ * <p>An admin also has ADMIN role on ALL registrars.
  */
 @Immutable
 public class AuthenticatedRegistrarAccessor {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
-  /** Type of access we're requesting. */
+  /** The role under which access is granted. */
-  public enum AccessType {
+  public enum Role {
-    READ,
+    OWNER,
-    UPDATE
+    ADMIN
   }
 
   AuthResult authResult;
   String registryAdminClientId;
 
   /**
-   * For any AccessType requested - all clientIDs this user is allowed that AccessType on.
+   * Gives all roles a user has for a given clientId.
    *
    * <p>The order is significant, with "more specific to this user" coming first.
    */
-  private final ImmutableSetMultimap<AccessType, String> accessMap;
+  private final ImmutableSetMultimap<String, Role> roleMap;
 
   @Inject
   public AuthenticatedRegistrarAccessor(
       AuthResult authResult, @Config("registryAdminClientId") String registryAdminClientId) {
     this.authResult = authResult;
     this.registryAdminClientId = registryAdminClientId;
-    this.accessMap = createAccessMap(authResult, registryAdminClientId);
+    this.roleMap = createRoleMap(authResult, registryAdminClientId);
 
     logger.atInfo().log(
-        "User %s has the following accesses: %s", authResult.userIdForLogging(), accessMap);
+        "%s has the following roles: %s", authResult.userIdForLogging(), roleMap);
   }
 
   /**
-   * Loads a Registrar IFF the user is authorized.
+   * A map that gives all roles a user has for a given clientId.
-   *
-   * <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to
-   * access the requested registrar.
-   *
-   * @param clientId ID of the registrar we request
-   * @param accessType what kind of access do we want for this registrar - just read it or write as
-   *     well? (different users might have different access levels)
-   */
-  public Registrar getRegistrar(String clientId, AccessType accessType) {
-    return getAndAuthorize(Registrar::loadByClientId, clientId, accessType);
-  }
-
-  /**
-   * Loads a Registrar from the cache IFF the user is authorized.
-   *
-   * <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to
-   * access the requested registrar.
-   *
-   * @param clientId ID of the registrar we request
-   * @param accessType what kind of access do we want for this registrar - just read it or write as
-   *     well? (different users might have different access levels)
-   */
-  public Registrar getRegistrarForUserCached(String clientId, AccessType accessType) {
-    return getAndAuthorize(Registrar::loadByClientIdCached, clientId, accessType);
-  }
-
-  /**
-   * For all {@link AccessType}s, Returns all ClientIds this user is allowed this access.
    *
    * <p>Throws a {@link ForbiddenException} if the user is not logged in.
    *
@@ -116,9 +87,8 @@ public class AuthenticatedRegistrarAccessor {
    * other clue as to the requested {@code clientId}. It is perfectly OK to get a {@code clientId}
    * from any other source, as long as the registrar is then loaded using {@link #getRegistrar}.
    */
-  public ImmutableSetMultimap<AccessType, String> getAllClientIdWithAccess() {
+  public ImmutableSetMultimap<String, Role> getAllClientIdWithRoles() {
-    verifyLoggedIn();
+    return roleMap;
-    return accessMap;
   }
 
   /**
@@ -138,32 +108,38 @@ public class AuthenticatedRegistrarAccessor {
    */
   public String guessClientId() {
     verifyLoggedIn();
-    return getAllClientIdWithAccess().values().stream()
+    return getAllClientIdWithRoles().keySet().stream()
         .findFirst()
         .orElseThrow(
             () ->
                 new ForbiddenException(
                     String.format(
-                        "User %s isn't associated with any registrar",
+                        "%s isn't associated with any registrar",
                         authResult.userIdForLogging())));
   }
 
-  private Registrar getAndAuthorize(
+  /**
-      Function<String, Optional<Registrar>> registrarLoader,
+   * Loads a Registrar IFF the user is authorized.
-      String clientId,
+   *
-      AccessType accessType) {
+   * <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to
+   * access the requested registrar.
+   *
+   * @param clientId ID of the registrar we request
+   */
+  public Registrar getRegistrar(String clientId) {
     verifyLoggedIn();
 
-    if (!accessMap.containsEntry(accessType, clientId)) {
+    ImmutableSet<Role> roles = getAllClientIdWithRoles().get(clientId);
+
+    if (roles.isEmpty()) {
       throw new ForbiddenException(
           String.format(
-              "User %s doesn't have %s access to registrar %s",
+              "%s doesn't have access to registrar %s",
-              authResult.userIdForLogging(), accessType, clientId));
+              authResult.userIdForLogging(), clientId));
     }
 
     Registrar registrar =
-        registrarLoader
+        Registrar.loadByClientId(clientId)
-            .apply(clientId)
             .orElseThrow(
                 () -> new ForbiddenException(String.format("Registrar %s not found", clientId)));
 
@@ -176,12 +152,11 @@ public class AuthenticatedRegistrarAccessor {
     }
 
     logger.atInfo().log(
-        "User %s has %s access to registrar %s.",
+        "%s has %s access to registrar %s.", authResult.userIdForLogging(), roles, clientId);
-        authResult.userIdForLogging(), accessType, clientId);
     return registrar;
   }
 
-  private static ImmutableSetMultimap<AccessType, String> createAccessMap(
+  private static ImmutableSetMultimap<String, Role> createRoleMap(
       AuthResult authResult, String registryAdminClientId) {
 
     if (!authResult.userAuthInfo().isPresent()) {
@@ -193,7 +168,7 @@ public class AuthenticatedRegistrarAccessor {
     boolean isAdmin = userAuthInfo.isUserAdmin();
     User user = userAuthInfo.user();
 
-    ImmutableSetMultimap.Builder<AccessType, String> builder = new ImmutableSetMultimap.Builder<>();
+    ImmutableSetMultimap.Builder<String, Role> builder = new ImmutableSetMultimap.Builder<>();
 
     ofy()
         .load()
@@ -202,25 +177,18 @@ public class AuthenticatedRegistrarAccessor {
         .forEach(
             contact ->
                 builder
-                    .put(AccessType.UPDATE, contact.getParent().getName())
+                    .put(contact.getParent().getName(), Role.OWNER));
-                    .put(AccessType.READ, contact.getParent().getName()));
     if (isAdmin && !Strings.isNullOrEmpty(registryAdminClientId)) {
-      logger.atInfo().log(
-          "Giving admin %s read+write access to admin registrar %s",
-          authResult.userIdForLogging(), registryAdminClientId);
       builder
-          .put(AccessType.UPDATE, registryAdminClientId)
+          .put(registryAdminClientId, Role.OWNER);
-          .put(AccessType.READ, registryAdminClientId);
     }
 
     if (isAdmin) {
-      // Admins have READ access to all registrars
+      // Admins have access to all registrars
-      logger.atInfo().log(
-          "Giving admin %s read-only access to all registrars", authResult.userIdForLogging());
       ofy()
           .load()
           .type(Registrar.class)
-          .forEach(registrar -> builder.put(AccessType.READ, registrar.getClientId()));
+          .forEach(registrar -> builder.put(registrar.getClientId(), Role.ADMIN));
     }
 
     return builder.build();

java/google/registry/ui/server/registrar/ConsoleUiAction.java

@@ -16,8 +16,8 @@ package google.registry.ui.server.registrar;
 
 import static com.google.common.net.HttpHeaders.LOCATION;
 import static com.google.common.net.HttpHeaders.X_FRAME_OPTIONS;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER;
 import static google.registry.ui.server.registrar.RegistrarConsoleModule.PARAM_CLIENT_ID;
 import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
 import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY;
@@ -27,6 +27,7 @@ import com.google.appengine.api.users.User;
 import com.google.appengine.api.users.UserService;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Supplier;
+import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Sets;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.io.Resources;
@@ -44,6 +45,7 @@ import google.registry.request.auth.Auth;
 import google.registry.request.auth.AuthResult;
 import google.registry.security.XsrfTokenManager;
 import google.registry.ui.server.SoyTemplateUtils;
+import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role;
 import google.registry.ui.soy.registrar.ConsoleSoyInfo;
 import java.util.Optional;
 import javax.inject.Inject;
@@ -136,12 +138,10 @@ public final class ConsoleUiAction implements Runnable {
       String clientId = paramClientId.orElse(registrarAccessor.guessClientId());
       data.put("clientId", clientId);
 
-      data.put("readWriteClientIds", registrarAccessor.getAllClientIdWithAccess().get(UPDATE));
+      ImmutableSetMultimap<Role, String> roleMap =
-      data.put(
+          registrarAccessor.getAllClientIdWithRoles().inverse();
-          "readOnlyClientIds",
+      data.put("ownerClientIds", roleMap.get(OWNER));
-          Sets.difference(
+      data.put("adminClientIds", Sets.difference(roleMap.get(ADMIN), roleMap.get(OWNER)));
-              registrarAccessor.getAllClientIdWithAccess().get(READ),
-              registrarAccessor.getAllClientIdWithAccess().get(UPDATE)));
 
       // We want to load the registrar even if we won't use it later (even if we remove the
       // requireFeeExtension) - to make sure the user indeed has access to the guessed registrar.
@@ -150,7 +150,7 @@ public final class ConsoleUiAction implements Runnable {
       // since we double check the access to the registrar on any read / update request. We have to
       // - since the access might get revoked between the initial page load and the request! (also
       // because the requests come from the browser, and can easily be faked)
-      Registrar registrar = registrarAccessor.getRegistrar(clientId, READ);
+      Registrar registrar = registrarAccessor.getRegistrar(clientId);
       data.put("requireFeeExtension", registrar.getPremiumPriceAckRequired());
     } catch (ForbiddenException e) {
       logger.atWarning().withCause(e).log(

java/google/registry/ui/server/registrar/RegistrarSettingsAction.java

@@ -20,8 +20,6 @@ import static google.registry.export.sheet.SyncRegistrarsSheetAction.enqueueRegi
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.security.JsonResponseHelper.Status.ERROR;
 import static google.registry.security.JsonResponseHelper.Status.SUCCESS;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE;
 
 import com.google.common.base.Ascii;
 import com.google.common.base.Strings;
@@ -138,7 +136,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
 
   Map<String, Object> read(String clientId) {
     return JsonResponseHelper.create(
-        SUCCESS, "Success", registrarAccessor.getRegistrar(clientId, READ).toJsonMap());
+        SUCCESS, "Success", registrarAccessor.getRegistrar(clientId).toJsonMap());
   }
 
   Map<String, Object> update(final Map<String, ?> args, String clientId) {
@@ -148,7 +146,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
               // We load the registrar here rather than outside of the transaction - to make
               // sure we have the latest version. This one is loaded inside the transaction, so it's
               // guaranteed to not change before we update it.
-              Registrar registrar = registrarAccessor.getRegistrar(clientId, UPDATE);
+              Registrar registrar = registrarAccessor.getRegistrar(clientId);
               // Verify that the registrar hasn't been changed.
               // To do that - we find the latest update time (or null if the registrar has been
               // deleted) and compare to the update time from the args. The update time in the args

java/google/registry/ui/soy/registrar/Console.soy

@@ -23,8 +23,8 @@
 {template .main}
   {@param xsrfToken: string} /** Security token. */
   {@param clientId: string} /** Registrar client identifier. */
-  {@param readWriteClientIds: list<string>}
+  {@param ownerClientIds: list<string>}
-  {@param readOnlyClientIds: list<string>}
+  {@param adminClientIds: list<string>}
   {@param username: string} /** Arbitrary username to display. */
   {@param logoutUrl: string} /** Generated URL for logging out of Google. */
   {@param productName: string} /** Name to display for this software product. */
@@ -78,8 +78,8 @@
 /** Sidebar nav. Ids on each elt for testing only. */
 {template .navbar_ visibility="private"}
   {@param clientId: string} /** Registrar client identifier. */
-  {@param readWriteClientIds: list<string>}
+  {@param ownerClientIds: list<string>}
-  {@param readOnlyClientIds: list<string>}
+  {@param adminClientIds: list<string>}
 
   <div id="reg-nav" class="{css('kd-content-sidebar')}">
     <form>
@@ -88,16 +88,16 @@
               class="{css('kd-button')} {css('kd-button-submit')}"
               onchange='this.form.submit()'>
         <option value="">[auto select]</option>
-        {if length($readWriteClientIds) > 0}
+        {if length($ownerClientIds) > 0}
-          <optgroup label="READ/WRITE">
+          <optgroup label="OWNER">
-          {for $id in $readWriteClientIds}
+          {for $id in $ownerClientIds}
             <option value="{$id}" {if $id == $clientId}selected{/if}>{$id}</option>
           {/for}
           </optgroup>
         {/if}
-        {if length($readOnlyClientIds) > 0}
+        {if length($adminClientIds) > 0}
-          <optgroup label="READ ONLY">
+          <optgroup label="ADMIN">
-          {for $id in $readOnlyClientIds}
+          {for $id in $adminClientIds}
             <option value="{$id}" {if $id == $clientId}selected{/if}>{$id}</option>
           {/for}
           </optgroup>

javatests/google/registry/rdap/RdapActionBaseTestCase.java

@@ -19,13 +19,14 @@ import static google.registry.rdap.RdapAuthorization.Role.PUBLIC;
 import static google.registry.rdap.RdapAuthorization.Role.REGISTRAR;
 import static google.registry.request.Action.Method.GET;
 import static google.registry.request.Action.Method.HEAD;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import com.google.appengine.api.users.User;
+import com.google.common.collect.ImmutableSetMultimap;
 import google.registry.model.ofy.Ofy;
 import google.registry.request.Action;
-import google.registry.request.HttpException.ForbiddenException;
 import google.registry.request.auth.AuthLevel;
 import google.registry.request.auth.AuthResult;
 import google.registry.request.auth.UserAuthInfo;
@@ -99,22 +100,27 @@ public class RdapActionBaseTestCase<A extends RdapActionBase> {
     action.requestMethod = Action.Method.GET;
     action.fullServletPath = "https://example.tld/rdap";
     action.rdapWhoisServer = null;
+    logout();
   }
 
   protected void login(String clientId) {
-    when(registrarAccessor.guessClientId()).thenReturn(clientId);
+    when(registrarAccessor.getAllClientIdWithRoles())
+        .thenReturn(ImmutableSetMultimap.of(clientId, OWNER));
     action.authResult = AUTH_RESULT;
     metricRole = REGISTRAR;
   }
 
   protected void logout() {
-    when(registrarAccessor.guessClientId()).thenThrow(new ForbiddenException("not logged in"));
+    when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of());
     action.authResult = AUTH_RESULT;
     metricRole = PUBLIC;
   }
 
   protected void loginAsAdmin() {
-    when(registrarAccessor.guessClientId()).thenReturn("irrelevant");
+    // when admin, we don't actually check what they have access to - so it doesn't matter what we
+    // return.
+    // null isn't actually a legal value, we just want to make sure it's never actually used.
+    when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(null);
     action.authResult = AUTH_RESULT_ADMIN;
     metricRole = ADMINISTRATOR;
   }

javatests/google/registry/ui/js/registrar/console_test_util.js

@@ -62,8 +62,8 @@ registry.registrar.ConsoleTestUtil.renderConsoleMain = function(
     logoutUrl: args.logoutUrl || 'https://logout.url.com',
     isAdmin: goog.isDefAndNotNull(args.isAdmin) ? args.isAdmin : true,
     clientId: args.clientId || 'ignore',
-    readWriteClientIds: args.readWriteClientIds || ['readWrite'],
+    ownerClientIds: args.ownerClientIds || ['owner'],
-    readOnlyClientIds: args.readOnlyClientIds || ['readOnly'],
+    adminClientIds: args.adminClientIds || ['admin'],
     logoFilename: args.logoFilename || 'logo.png',
     productName: args.productName || 'Nomulus',
     integrationEmail: args.integrationEmail || 'integration@example.com',

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

@@ -20,8 +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.AuthenticatedRegistrarAccessor.AccessType.READ;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER;
 import static org.mockito.Mockito.mock;
 
 import com.google.appengine.api.users.User;
@@ -34,7 +34,6 @@ 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.AuthenticatedRegistrarAccessor.AccessType;
 import java.util.logging.Level;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -99,11 +98,8 @@ public class AuthenticatedRegistrarAccessorTest {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(AUTHORIZED_USER, ADMIN_CLIENT_ID);
 
-    assertThat(registrarAccessor.getAllClientIdWithAccess())
+    assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(
+        .containsExactly(DEFAULT_CLIENT_ID, OWNER);
-            UPDATE, DEFAULT_CLIENT_ID,
-            READ, DEFAULT_CLIENT_ID)
-        .inOrder();
   }
 
   /** Logged out users don't have access to anything. */
@@ -112,9 +108,7 @@ public class AuthenticatedRegistrarAccessorTest {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(NO_USER, ADMIN_CLIENT_ID);
 
-    ForbiddenException exception =
+    assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty();
-        assertThrows(ForbiddenException.class, () -> registrarAccessor.getAllClientIdWithAccess());
-    assertThat(exception).hasMessageThat().contains("Not logged in");
   }
 
   /** Unauthorized users don't have access to anything. */
@@ -123,7 +117,7 @@ public class AuthenticatedRegistrarAccessorTest {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(UNAUTHORIZED_USER, ADMIN_CLIENT_ID);
 
-    assertThat(registrarAccessor.getAllClientIdWithAccess()).isEmpty();
+    assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty();
   }
 
   /** Admins have read/write access to the authorized registrars, AND the admin registrar. */
@@ -132,12 +126,12 @@ public class AuthenticatedRegistrarAccessorTest {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(AUTHORIZED_ADMIN, ADMIN_CLIENT_ID);
 
-    assertThat(registrarAccessor.getAllClientIdWithAccess())
+    assertThat(registrarAccessor.getAllClientIdWithRoles())
         .containsExactly(
-            UPDATE, DEFAULT_CLIENT_ID,
+            DEFAULT_CLIENT_ID, OWNER,
-            READ, DEFAULT_CLIENT_ID,
+            DEFAULT_CLIENT_ID, ADMIN,
-            UPDATE, ADMIN_CLIENT_ID,
+            ADMIN_CLIENT_ID, OWNER,
-            READ, ADMIN_CLIENT_ID)
+            ADMIN_CLIENT_ID, ADMIN)
         .inOrder();
   }
 
@@ -149,117 +143,65 @@ public class AuthenticatedRegistrarAccessorTest {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(UNAUTHORIZED_ADMIN, ADMIN_CLIENT_ID);
 
-    assertThat(registrarAccessor.getAllClientIdWithAccess())
+    assertThat(registrarAccessor.getAllClientIdWithRoles())
         .containsExactly(
-            UPDATE, ADMIN_CLIENT_ID,
+            ADMIN_CLIENT_ID, OWNER,
-            READ, ADMIN_CLIENT_ID,
+            ADMIN_CLIENT_ID, ADMIN,
-            READ, DEFAULT_CLIENT_ID)
+            DEFAULT_CLIENT_ID, ADMIN)
         .inOrder();
   }
 
   /** Fail loading registrar if user doesn't have access to it. */
   @Test
-  public void testGetRegistrarForUser_readOnly_noAccess_isNotAdmin() {
+  public void testGetRegistrarForUser_noAccess_isNotAdmin() {
     expectGetRegistrarFailure(
         DEFAULT_CLIENT_ID,
-        READ,
         UNAUTHORIZED_USER,
-        "User {user} doesn't have READ access to registrar {clientId}");
+        "{user} doesn't have 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,
-        UPDATE,
-        UNAUTHORIZED_USER,
-        "User {user} doesn't have UPDATE access to registrar {clientId}");
   }
 
   /** Fail loading registrar if there's no user associated with the request. */
   @Test
-  public void testGetRegistrarForUser_readOnly_noUser() {
+  public void testGetRegistrarForUser_noUser() {
-    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, READ, NO_USER, "Not logged in");
+    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, NO_USER, "Not logged in");
   }
 
-  /** Fail loading registrar if there's no user associated with the request. */
+  /** Succeed loading registrar if user has access to it. */
   @Test
-  public void testGetRegistrarForUser_readWrite_noUser() {
+  public void testGetRegistrarForUser_hasAccess_isNotAdmin() {
-    expectGetRegistrarFailure(DEFAULT_CLIENT_ID, UPDATE, NO_USER, "Not logged in");
-  }
-
-  /** Succeed loading registrar in read-only mode if user has access to it. */
-  @Test
-  public void testGetRegistrarForUser_readOnly_hasAccess_isNotAdmin() {
     expectGetRegistrarSuccess(
-        AUTHORIZED_USER, READ, "User {user} has READ access to registrar {clientId}");
+        AUTHORIZED_USER, "{user} has [OWNER] access to registrar {clientId}");
   }
 
-  /** Succeed loading registrar in read-write mode if user has access to it. */
+  /** Succeed loading registrar if admin with access. */
   @Test
-  public void testGetRegistrarForUser_readWrite_hasAccess_isNotAdmin() {
+  public void testGetRegistrarForUser_hasAccess_isAdmin() {
     expectGetRegistrarSuccess(
-        AUTHORIZED_USER, UPDATE, "User {user} has UPDATE access to registrar {clientId}");
+        AUTHORIZED_ADMIN, "{user} has [OWNER, ADMIN] access to registrar {clientId}");
   }
 
-  /** Succeed loading registrar in read-only mode if admin with access. */
+  /** Succeed loading registrar for admin even if they aren't on the approved contacts list. */
   @Test
-  public void testGetRegistrarForUser_readOnly_hasAccess_isAdmin() {
+  public void testGetRegistrarForUser_noAccess_isAdmin() {
     expectGetRegistrarSuccess(
-        AUTHORIZED_ADMIN, READ, "User {user} has READ access to registrar {clientId}");
+        UNAUTHORIZED_ADMIN, "{user} has [ADMIN] 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, UPDATE, "User {user} has UPDATE 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, "User {user} has READ 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,
-        UPDATE,
-        UNAUTHORIZED_ADMIN,
-        "User {user} doesn't have UPDATE access to registrar {clientId}");
   }
 
   /** Fail loading registrar even if admin, if registrar doesn't exist. */
   @Test
-  public void testGetRegistrarForUser_readOnly_doesntExist_isAdmin() {
+  public void testGetRegistrarForUser_doesntExist_isAdmin() {
     expectGetRegistrarFailure(
         "BadClientId",
-        READ,
         AUTHORIZED_ADMIN,
-        "User {user} doesn't have READ access to registrar {clientId}");
+        "{user} doesn't have access to registrar {clientId}");
-  }
-
-  /** Fail loading registrar even if admin, if registrar doesn't exist. */
-  @Test
-  public void testGetRegistrarForUser_readWrite_doesntExist_isAdmin() {
-    expectGetRegistrarFailure(
-        "BadClientId",
-        UPDATE,
-        AUTHORIZED_ADMIN,
-        "User {user} doesn't have UPDATE access to registrar {clientId}");
   }
 
   private void expectGetRegistrarSuccess(
-      AuthResult authResult, AccessType accessType, String message) {
+      AuthResult authResult, String message) {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(authResult, ADMIN_CLIENT_ID);
 
-    assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID, accessType)).isNotNull();
+    assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID)).isNotNull();
     assertAboutLogs()
         .that(testLogHandler)
         .hasLogAtLevelWithMessage(
@@ -267,13 +209,13 @@ public class AuthenticatedRegistrarAccessorTest {
   }
 
   private void expectGetRegistrarFailure(
-      String clientId, AccessType accessType, AuthResult authResult, String message) {
+      String clientId, AuthResult authResult, String message) {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(authResult, ADMIN_CLIENT_ID);
 
     ForbiddenException exception =
         assertThrows(
-            ForbiddenException.class, () -> registrarAccessor.getRegistrar(clientId, accessType));
+            ForbiddenException.class, () -> registrarAccessor.getRegistrar(clientId));
 
     assertThat(exception).hasMessageThat().contains(formatMessage(message, authResult, clientId));
   }
@@ -290,8 +232,7 @@ public class AuthenticatedRegistrarAccessorTest {
   /** If a user doesn't have access to any registrars, guess returns nothing. */
   @Test
   public void testGuessClientIdForUser_noAccess_isNotAdmin() {
-    expectGuessRegistrarFailure(
+    expectGuessRegistrarFailure(UNAUTHORIZED_USER, "{user} isn't associated with any registrar");
-        UNAUTHORIZED_USER, "User {user} isn't associated with any registrar");
   }
 
   /**

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

@@ -17,8 +17,8 @@ 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.AuthenticatedRegistrarAccessor.AccessType.READ;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE;
+import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER;
 import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -79,16 +79,15 @@ public class ConsoleUiActionTest {
     action.paramClientId = Optional.empty();
     AuthResult authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false));
     action.authResult = authResult;
-    when(registrarAccessor.getRegistrar("TheRegistrar", READ))
+    when(registrarAccessor.getRegistrar("TheRegistrar"))
         .thenReturn(loadRegistrar("TheRegistrar"));
-    when(registrarAccessor.getAllClientIdWithAccess())
+    when(registrarAccessor.getAllClientIdWithRoles())
         .thenReturn(
             ImmutableSetMultimap.of(
-                UPDATE, "TheRegistrar",
+                "TheRegistrar", OWNER,
-                READ, "TheRegistrar",
+                "OtherRegistrar", OWNER,
-                UPDATE, "ReadWriteRegistrar",
+                "OtherRegistrar", ADMIN,
-                READ, "ReadWriteRegistrar",
+                "AdminRegistrar", ADMIN));
-                READ, "ReadOnlyRegistrar"));
     when(registrarAccessor.guessClientId()).thenCallRealMethod();
     // Used for error message in guessClientId
     registrarAccessor.authResult = authResult;
@@ -128,7 +127,7 @@ public class ConsoleUiActionTest {
 
   @Test
   public void testUserDoesntHaveAccessToAnyRegistrar_showsWhoAreYouPage() {
-    when(registrarAccessor.getAllClientIdWithAccess()).thenReturn(ImmutableSetMultimap.of());
+    when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of());
     action.run();
     assertThat(response.getPayload()).contains("<h1>You need permission</h1>");
     assertThat(response.getPayload()).contains("not associated with Nomulus.");
@@ -155,7 +154,7 @@ public class ConsoleUiActionTest {
   @Test
   public void testSettingClientId_notAllowed_showsNeedPermissionPage() {
     action.paramClientId = Optional.of("OtherClientId");
-    when(registrarAccessor.getRegistrar("OtherClientId", READ))
+    when(registrarAccessor.getRegistrar("OtherClientId"))
         .thenThrow(new ForbiddenException("forbidden"));
     action.run();
     assertThat(response.getPayload()).contains("<h1>You need permission</h1>");
@@ -165,7 +164,7 @@ public class ConsoleUiActionTest {
   @Test
   public void testSettingClientId_allowed_showsRegistrarConsole() {
     action.paramClientId = Optional.of("OtherClientId");
-    when(registrarAccessor.getRegistrar("OtherClientId", READ))
+    when(registrarAccessor.getRegistrar("OtherClientId"))
         .thenReturn(loadRegistrar("TheRegistrar"));
     action.run();
     assertThat(response.getPayload()).contains("Registrar Console");
@@ -176,7 +175,7 @@ public class ConsoleUiActionTest {
   public void testUserHasAccessAsTheRegistrar_showsClientIdChooser() {
     action.run();
     assertThat(response.getPayload()).contains("<option value=\"TheRegistrar\" selected>");
-    assertThat(response.getPayload()).contains("<option value=\"ReadWriteRegistrar\">");
+    assertThat(response.getPayload()).contains("<option value=\"OtherRegistrar\">");
-    assertThat(response.getPayload()).contains("<option value=\"ReadOnlyRegistrar\">");
+    assertThat(response.getPayload()).contains("<option value=\"AdminRegistrar\">");
   }
 }

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

@@ -101,18 +101,6 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
             "results", asList(loadRegistrar(CLIENT_ID).toJsonMap()));
   }
 
-  /** This is the default read test for the registrar settings actions. */
-  @Test
-  public void testSuccess_readRegistrarInfo_authorizedReadOnly() {
-    setUserReadOnlyAccess();
-    Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID));
-    assertThat(response)
-        .containsExactly(
-            "status", "SUCCESS",
-            "message", "Success",
-            "results", asList(loadRegistrar(CLIENT_ID).toJsonMap()));
-  }
-
   @Test
   public void testUpdate_emptyJsonObject_errorLastUpdateTimeFieldRequired() {
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
@@ -169,20 +157,6 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
     assertNoTasksEnqueued("sheet");
   }
 
-  @Test
-  public void testFailure_updateRegistrarInfo_readOnlyAccess() {
-    setUserReadOnlyAccess();
-    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,8 +18,6 @@ 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.AuthenticatedRegistrarAccessor.AccessType.READ;
-import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -105,41 +103,24 @@ public class RegistrarSettingsActionTestCase {
     when(rsp.getWriter()).thenReturn(new PrintWriter(writer));
     when(req.getContentType()).thenReturn("application/json");
     when(req.getReader()).thenReturn(createJsonPayload(ImmutableMap.of("op", "read")));
-    // We set the default user to one with read-write access, as that's the most common test case.
+    // We set the default to a user with access, as that's the most common test case. When we want
-    // When we want to specifically check read-only or unauthorized, we can switch the user here.
+    // to specifically check a user without access, we can switch user for that specific test.
-    setUserReadWriteAccess();
+    setUserWithAccess();
   }
 
   /** Sets registrarAccessor.getRegistrar to succeed for all AccessTypes. */
-  protected void setUserReadWriteAccess() {
+  protected void setUserWithAccess() {
     action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class);
 
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, READ))
+    when(action.registrarAccessor.getRegistrar(CLIENT_ID))
         .thenAnswer(x -> loadRegistrar(CLIENT_ID));
-
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, UPDATE))
-        .thenAnswer(x -> loadRegistrar(CLIENT_ID));
-  }
-
-  /** Sets registrarAccessor.getRegistrar to only succeed for READ. */
-  protected void setUserReadOnlyAccess() {
-    action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class);
-
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, READ))
-        .thenAnswer(x -> loadRegistrar(CLIENT_ID));
-
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, UPDATE))
-        .thenThrow(new ForbiddenException("forbidden test error"));
   }
 
   /** Sets registrarAccessor.getRegistrar to always fail. */
   protected void setUserWithoutAccess() {
     action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class);
 
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, READ))
+    when(action.registrarAccessor.getRegistrar(CLIENT_ID))
-        .thenThrow(new ForbiddenException("forbidden test error"));
-
-    when(action.registrarAccessor.getRegistrar(CLIENT_ID, UPDATE))
         .thenThrow(new ForbiddenException("forbidden test error"));
   }
 }