Allow only OWNERs to change owner-related data on registrar console

The console will have 2 different "updatable things":
- only ADMINs (GAE-admins and users in the support G-Suite group) can change the things in the "admin settings" tab (currently just the allowed TLDs)
- only OWNERs can change things from the other tabs: WHOIS info, certificates, whitelisted IPs, contacts etc.

Also, all ADMINs are now OWNERS of "non-REAL" registrars. Meaning - we're only
preventing ADMINs from editing "REAL" registrars (usually in production).

Specifically, OTE registrars on sandbox are NOT "REAL", meaning ADMINS will
still be able to update them.

This only changes the backend (registrar-settings endpoint). As-is, the console
website will still make ADMINs *think* they can change everything, but if they
try - they will get an error.

Changing the frontend will happen in the next CL - because I want to get this
out this release cycle and getting JS reviewed takes a long time :(

TESTED=deployed to alpha, and saw I can't update fields even as admin on REAL
registrars, but could change it on non-REAL registrars. Also checked that I can
update the allowed TLDs on REAL registrars

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

java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java

@@ -38,9 +38,13 @@ import javax.inject.Inject;
  * <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 OWNER role on {@code #registryAdminClientId}.
+ * <p>An "admin" has in addition OWNER role on {@code #registryAdminClientId} and to all non-{@code
+ * REAL} registrars (see {@link Registrar#getType}).
  *
- * <p>An admin also has ADMIN role on ALL registrars.
+ * <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
+ * group.
  */
 @Immutable
 public class AuthenticatedRegistrarAccessor {
@@ -140,6 +144,24 @@ public class AuthenticatedRegistrarAccessor {
     return roleMap;
   }
 
+  /**
+   * Returns all the roles the current user has on the given registrar.
+   *
+   * <p>This is syntactic sugar for {@code getAllClientIdWithRoles().get(clientId)}.
+   */
+  public ImmutableSet<Role> getRolesForRegistrar(String clientId) {
+    return getAllClientIdWithRoles().get(clientId);
+  }
+
+  /**
+   * Checks if we have a given role for a given registrar.
+   *
+   * <p>This is syntactic sugar for {@code getAllClientIdWithRoles().containsEntry(clientId, role)}.
+   */
+  public boolean hasRoleOnRegistrar(Role role, String clientId) {
+    return getAllClientIdWithRoles().containsEntry(clientId, role);
+  }
+
   /**
    * "Guesses" which client ID the user wants from all those they have access to.
    *
@@ -257,11 +279,17 @@ public class AuthenticatedRegistrarAccessor {
     }
 
     if (isAdmin || isSupport) {
-      // Admins and support have access to all registrars
+      // Admins and support have ADMIN access to all registrars, and OWNER access to all non-REAL
+      // registrars
       ofy()
           .load()
           .type(Registrar.class)
-          .forEach(registrar -> builder.put(registrar.getClientId(), Role.ADMIN));
+          .forEach(registrar -> {
+            if (!Registrar.Type.REAL.equals(registrar.getType())) {
+              builder.put(registrar.getClientId(), Role.OWNER);
+            }
+            builder.put(registrar.getClientId(), Role.ADMIN);
+          });
     }
 
     return builder.build();

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

@@ -14,6 +14,7 @@
 
 package google.registry.ui.server.registrar;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Sets.difference;
 import static google.registry.export.sheet.SyncRegistrarsSheetAction.enqueueRegistrarSheetSync;
@@ -57,6 +58,7 @@ import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Predicate;
@@ -144,7 +146,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
           ERROR, Optional.ofNullable(e.getMessage()).orElse("Unspecified error"));
     } finally {
       registrarConsoleMetrics.registerSettingsRequest(
-          clientId, op, registrarAccessor.getAllClientIdWithRoles().get(clientId), status);
+          clientId, op, registrarAccessor.getRolesForRegistrar(clientId), status);
     }
   }
 
@@ -205,29 +207,31 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
               // removed, email the changes to the contacts
               ImmutableSet<RegistrarContact> contacts = registrar.getContacts();
 
-              // Update the registrar from the request.
+              Registrar updatedRegistrar = registrar;
-              Registrar.Builder builder = registrar.asBuilder();
+              // Do OWNER only updates to the registrar from the request.
-              Set<Role> roles = registrarAccessor.getAllClientIdWithRoles().get(clientId);
+              updatedRegistrar = checkAndUpdateOwnerControlledFields(updatedRegistrar, args);
-              changeRegistrarFields(registrar, roles, builder, args);
+              // Do ADMIN only updates to the registrar from the request.
+              updatedRegistrar = checkAndUpdateAdminControlledFields(updatedRegistrar, args);
 
               // read the contacts from the request.
               ImmutableSet<RegistrarContact> updatedContacts = readContacts(registrar, args);
-              if (!updatedContacts.isEmpty()) {
+
-                builder.setContactsRequireSyncing(true);
+              // Save the updated contacts
+              if (!updatedContacts.equals(contacts)) {
+                if (!registrarAccessor.hasRoleOnRegistrar(Role.OWNER, registrar.getClientId())) {
+                  throw new ForbiddenException("Only OWNERs can update the contacts");
+                }
+                checkContactRequirements(contacts, updatedContacts);
+                RegistrarContact.updateContacts(updatedRegistrar, updatedContacts);
+                updatedRegistrar =
+                    updatedRegistrar.asBuilder().setContactsRequireSyncing(true).build();
               }
 
               // Save the updated registrar
-              Registrar updatedRegistrar = builder.build();
               if (!updatedRegistrar.equals(registrar)) {
                 ofy().save().entity(updatedRegistrar);
               }
 
-              // Save the updated contacts
-              if (!updatedContacts.isEmpty()) {
-                checkContactRequirements(contacts, updatedContacts);
-                RegistrarContact.updateContacts(updatedRegistrar, updatedContacts);
-              }
-
               // Email and return update.
               sendExternalUpdatesIfNecessary(
                   registrar, contacts, updatedRegistrar, updatedContacts);
@@ -248,12 +252,15 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
     return result;
   }
 
-  /** Updates a registrar builder with the supplied args from the http request; */
+  /**
-  public static void changeRegistrarFields(
+   * Updates registrar with the OWNER-controlled args from the http request.
-      Registrar existingRegistrarObj,
+   *
-      Set<Role> roles,
+   * <p>If any changes were made and the user isn't an OWNER - throws a {@link ForbiddenException}.
-      Registrar.Builder builder,
+   */
-      Map<String, ?> args) {
+  private Registrar checkAndUpdateOwnerControlledFields(
+      Registrar initialRegistrar, Map<String, ?> args) {
+
+    Registrar.Builder builder = initialRegistrar.asBuilder();
 
     // BILLING
     RegistrarFormFields.PREMIUM_PRICE_ACK_REQUIRED
@@ -261,13 +268,27 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
         .ifPresent(builder::setPremiumPriceAckRequired);
 
     // WHOIS
-    builder.setWhoisServer(
+    //
-        RegistrarFormFields.WHOIS_SERVER_FIELD.extractUntyped(args).orElse(null));
+    // Because of how whoisServer handles "default value", it's possible that setting the existing
+    // value will still change the Registrar. So we first check whether the value has changed.
+    //
+    // The problem is - if the Registrar has a "null" whoisServer value, the console gets the
+    // "default value" instead of the actual (null) value.
+    // This was done so we display the "default" value, but it also means that it always looks like
+    // the user updated the whoisServer value from "null" to the default value.
+    //
+    // TODO(b/119913848):once a null whoisServer value is sent to the console as "null", there's no
+    // need to check for equality before setting the value in the builder.
+    String updatedWhoisServer =
+        RegistrarFormFields.WHOIS_SERVER_FIELD.extractUntyped(args).orElse(null);
+    if (!Objects.equals(initialRegistrar.getWhoisServer(), updatedWhoisServer)) {
+      builder.setWhoisServer(updatedWhoisServer);
+    }
     builder.setUrl(RegistrarFormFields.URL_FIELD.extractUntyped(args).orElse(null));
 
     // If the email is already null / empty - we can keep it so. But if it's set - it's required to
     // remain set.
-    (Strings.isNullOrEmpty(existingRegistrarObj.getEmailAddress())
+    (Strings.isNullOrEmpty(initialRegistrar.getEmailAddress())
             ? RegistrarFormFields.EMAIL_ADDRESS_FIELD_OPTIONAL
             : RegistrarFormFields.EMAIL_ADDRESS_FIELD_REQUIRED)
         .extractUntyped(args)
@@ -294,25 +315,59 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
             certificate ->
                 builder.setFailoverClientCertificate(certificate, ofy().getTransactionTime()));
 
-    // Update allowed TLDs only when it is modified
+    return checkNotChangedUnlessAllowed(builder, initialRegistrar, Role.OWNER);
+  }
+
+  /**
+   * Updates a registrar with the ADMIN-controlled args from the http request.
+   *
+   * <p>If any changes were made and the user isn't an ADMIN - throws a {@link ForbiddenException}.
+   */
+  private Registrar checkAndUpdateAdminControlledFields(
+      Registrar initialRegistrar, Map<String, ?> args) {
+    Registrar.Builder builder = initialRegistrar.asBuilder();
+
     Set<String> updatedAllowedTlds =
         RegistrarFormFields.ALLOWED_TLDS_FIELD.extractUntyped(args).orElse(ImmutableSet.of());
-    if (!updatedAllowedTlds.equals(existingRegistrarObj.getAllowedTlds())) {
-      // Only admin is allowed to update allowed TLDs
-      if (!roles.contains(Role.ADMIN)) {
-        throw new ForbiddenException("Only admin can update allowed TLDs.");
-      }
     // Temporarily block anyone from removing an allowed TLD.
     // This is so we can start having Support users use the console in production before we finish
     // implementing configurable access control.
     // TODO(b/119549884): remove this code once configurable access control is implemented.
-      Set<String> removedTlds =
+    if (!Sets.difference(initialRegistrar.getAllowedTlds(), updatedAllowedTlds).isEmpty()) {
-          Sets.difference(existingRegistrarObj.getAllowedTlds(), updatedAllowedTlds);
-      if (!removedTlds.isEmpty()) {
       throw new ForbiddenException("Can't remove allowed TLDs using the console.");
     }
     builder.setAllowedTlds(updatedAllowedTlds);
+    return checkNotChangedUnlessAllowed(builder, initialRegistrar, Role.ADMIN);
   }
+
+  /**
+   * Makes sure builder.build is different than originalRegistrar only if we have the correct role.
+   *
+   * <p>On success, returns {@code builder.build()}.
+   */
+  private Registrar checkNotChangedUnlessAllowed(
+      Registrar.Builder builder,
+      Registrar originalRegistrar,
+      Role allowedRole) {
+    Registrar updatedRegistrar =  builder.build();
+    if (updatedRegistrar.equals(originalRegistrar)) {
+      return updatedRegistrar;
+    }
+    checkArgument(
+        updatedRegistrar.getClientId().equals(originalRegistrar.getClientId()),
+        "Can't change clientId (%s -> %s)",
+        originalRegistrar.getClientId(),
+        updatedRegistrar.getClientId());
+    if (registrarAccessor.hasRoleOnRegistrar(allowedRole, originalRegistrar.getClientId())) {
+      return updatedRegistrar;
+    }
+    Map<?, ?> diffs =
+        DiffUtils.deepDiff(
+            originalRegistrar.toDiffableFieldMap(),
+            updatedRegistrar.toDiffableFieldMap(),
+            true);
+    throw new ForbiddenException(
+        String.format("Unauthorized: only %s can change fields %s", allowedRole, diffs.keySet()));
   }
 
   /** Reads the contacts from the supplied args. */

javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java

@@ -29,10 +29,12 @@ import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 import com.google.appengine.api.users.User;
+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 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;
@@ -58,32 +60,57 @@ public class AuthenticatedRegistrarAccessorTest {
   private final GroupsConnection groupsConnection = mock(GroupsConnection.class);
   private final TestLogHandler testLogHandler = new TestLogHandler();
 
-  private static final AuthResult AUTHORIZED_USER = createAuthResult(true, false);
+  private static final AuthResult USER = createAuthResult(false);
-  private static final AuthResult UNAUTHORIZED_USER = createAuthResult(false, false);
+  private static final AuthResult GAE_ADMIN = createAuthResult(true);
-  private static final AuthResult AUTHORIZED_ADMIN = createAuthResult(true, true);
-  private static final AuthResult UNAUTHORIZED_ADMIN = createAuthResult(false, true);
   private static final AuthResult NO_USER = AuthResult.create(AuthLevel.NONE);
   private static final String SUPPORT_GROUP = "support@registry.example";
-  private static final String DEFAULT_CLIENT_ID = "TheRegistrar";
+  /** Client ID of a REAL registrar with a RegistrarContact for USER and GAE_ADMIN. */
-  private static final String ADMIN_CLIENT_ID = "NewRegistrar";
+  private static final String CLIENT_ID_WITH_CONTACT = "TheRegistrar";
+  /** Client ID of a REAL registrar without a RegistrarContact. */
+  private static final String REAL_CLIENT_ID_WITHOUT_CONTACT = "NewRegistrar";
+  /** Client ID of an OTE registrar without a RegistrarContact. */
+  private static final String OTE_CLIENT_ID_WITHOUT_CONTACT = "OteRegistrar";
+  /** Client ID of the Admin registrar without a RegistrarContact. */
+  private static final String ADMIN_CLIENT_ID = "AdminRegistrar";
 
-  private static AuthResult createAuthResult(boolean isAuthorized, boolean isAdmin) {
+  /**
+   * Creates an AuthResult for a fake user.
+   *
+   * The user will be a RegistrarContact for "TheRegistrar", but not for "NewRegistrar".
+   *
+   * @param isAdmin if true, the user is an administrator for the app-engine project.
+   */
+  private static AuthResult createAuthResult(boolean isAdmin) {
     return AuthResult.create(
         AuthLevel.USER,
         UserAuthInfo.create(
             new User(
                 String.format(
-                    "%s_%s@gmail.com",
+                    "%s@gmail.com",
-                    isAuthorized ? "auth" : "unauth", isAdmin ? "admin" : "user"),
+                    isAdmin ? "admin" : "user"),
                 "gmail.com",
-                isAuthorized ? THE_REGISTRAR_GAE_USER_ID : "badGaeUserId"),
+                THE_REGISTRAR_GAE_USER_ID),
             isAdmin));
   }
 
   @Before
   public void before() {
     LoggerConfig.getConfig(AuthenticatedRegistrarAccessor.class).addHandler(testLogHandler);
-    persistResource(loadRegistrar(ADMIN_CLIENT_ID));
+    // persistResource(loadRegistrar(ADMIN_CLIENT_ID));
+    persistResource(
+        loadRegistrar(REAL_CLIENT_ID_WITHOUT_CONTACT)
+            .asBuilder()
+            .setClientId(OTE_CLIENT_ID_WITHOUT_CONTACT)
+            .setType(Registrar.Type.OTE)
+            .setIanaIdentifier(null)
+            .build());
+    persistResource(
+        loadRegistrar(REAL_CLIENT_ID_WITHOUT_CONTACT)
+            .asBuilder()
+            .setClientId(ADMIN_CLIENT_ID)
+            .setType(Registrar.Type.OTE)
+            .setIanaIdentifier(null)
+            .build());
     when(groupsConnection.isMemberOfGroup(any(), any())).thenReturn(false);
   }
 
@@ -92,30 +119,15 @@ public class AuthenticatedRegistrarAccessorTest {
     LoggerConfig.getConfig(AuthenticatedRegistrarAccessor.class).removeHandler(testLogHandler);
   }
 
-  /** Users only have access to the registrars they are a contact for. */
+  /** Users are owners for registrars if and only if they are in the contacts for that registrar. */
   @Test
-  public void getAllClientIdWithAccess_authorizedUser() {
+  public void getAllClientIdWithAccess_user() {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(
-            AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
+            USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
 
     assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(DEFAULT_CLIENT_ID, OWNER);
+        .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER);
-  }
-
-  /** Users in support group have admin access to everything. */
-  @Test
-  public void getAllClientIdWithAccess_authorizedUser_isSupportGroup() {
-    when(groupsConnection.isMemberOfGroup("auth_user@gmail.com", SUPPORT_GROUP)).thenReturn(true);
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(
-            DEFAULT_CLIENT_ID, OWNER,
-            DEFAULT_CLIENT_ID, ADMIN,
-            ADMIN_CLIENT_ID, ADMIN);
   }
 
   /** Logged out users don't have access to anything. */
@@ -128,39 +140,79 @@ public class AuthenticatedRegistrarAccessorTest {
     assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty();
   }
 
-  /** Unauthorized users don't have access to anything. */
+  /**
+   * GAE admins have admin access to everything.
+   *
+   * <p>They also have OWNER access if they are in the RegistrarContacts.
+   *
+   * <p>They also have OWNER access to the Admin Registrar.
+   *
+   * <p>They also have OWNER access to non-REAL Registrars.
+   *
+   * <p>(in other words - they don't have OWNER access only to REAL registrars owned by others)
+   */
   @Test
-  public void getAllClientIdWithAccess_unauthorizedUser() {
+  public void getAllClientIdWithAccess_gaeAdmin() {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(
-            UNAUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
+            GAE_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty();
-  }
-
-  /** Unauthorized users who are in support group have admin access. */
-  @Test
-  public void getAllClientIdWithAccess_unauthorizedUser_inSupportGroup() {
-    when(groupsConnection.isMemberOfGroup("unauth_user@gmail.com", SUPPORT_GROUP)).thenReturn(true);
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            UNAUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
 
     assertThat(registrarAccessor.getAllClientIdWithRoles())
         .containsExactly(
-            DEFAULT_CLIENT_ID, ADMIN,
+            CLIENT_ID_WITH_CONTACT, ADMIN,
-            ADMIN_CLIENT_ID, ADMIN);
+            CLIENT_ID_WITH_CONTACT, OWNER,
+
+            REAL_CLIENT_ID_WITHOUT_CONTACT, ADMIN,
+
+            OTE_CLIENT_ID_WITHOUT_CONTACT, ADMIN,
+            OTE_CLIENT_ID_WITHOUT_CONTACT, OWNER,
+
+            ADMIN_CLIENT_ID, ADMIN,
+            ADMIN_CLIENT_ID, OWNER);
+  }
+
+  /**
+   * Users in support group have admin access to everything.
+   *
+   * <p>They also have OWNER access if they are in the RegistrarContacts.
+   *
+   * <p>They also have OWNER access to the Admin Registrar.
+   *
+   * <p>They also have OWNER access to non-REAL Registrars.
+   *
+   * <p>(in other words - they don't have OWNER access only to REAL registrars owned by others)
+   */
+  @Test
+  public void getAllClientIdWithAccess_userInSupportGroup() {
+    when(groupsConnection.isMemberOfGroup("user@gmail.com", SUPPORT_GROUP)).thenReturn(true);
+    AuthenticatedRegistrarAccessor registrarAccessor =
+        new AuthenticatedRegistrarAccessor(
+            USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
+
+    assertThat(registrarAccessor.getAllClientIdWithRoles())
+        .containsExactly(
+            CLIENT_ID_WITH_CONTACT, ADMIN,
+            CLIENT_ID_WITH_CONTACT, OWNER,
+
+            REAL_CLIENT_ID_WITHOUT_CONTACT, ADMIN,
+
+            OTE_CLIENT_ID_WITHOUT_CONTACT, ADMIN,
+            OTE_CLIENT_ID_WITHOUT_CONTACT, OWNER,
+
+            ADMIN_CLIENT_ID, ADMIN,
+            ADMIN_CLIENT_ID, OWNER);
   }
 
   /** Empty Support group email - skips check. */
   @Test
   public void getAllClientIdWithAccess_emptySupportEmail_works() {
     AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(AUTHORIZED_USER, ADMIN_CLIENT_ID, "", groupsConnection);
+        new AuthenticatedRegistrarAccessor(
+            USER, ADMIN_CLIENT_ID, "", groupsConnection);
 
     verifyNoMoreInteractions(groupsConnection);
     assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(DEFAULT_CLIENT_ID, OWNER);
+        .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER);
   }
 
   /** Support group check throws - continue anyway. */
@@ -169,85 +221,74 @@ public class AuthenticatedRegistrarAccessorTest {
     when(groupsConnection.isMemberOfGroup(any(), any())).thenThrow(new RuntimeException("blah"));
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(
-            AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
+            USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
 
-    verify(groupsConnection).isMemberOfGroup("auth_user@gmail.com", SUPPORT_GROUP);
+    verify(groupsConnection).isMemberOfGroup("user@gmail.com", SUPPORT_GROUP);
     assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(DEFAULT_CLIENT_ID, OWNER);
+        .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER);
-  }
-
-  /** Admins have read/write access to the authorized registrars, AND the admin registrar. */
-  @Test
-  public void getAllClientIdWithAccess_authorizedAdmin() {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            AUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(
-            DEFAULT_CLIENT_ID, OWNER,
-            DEFAULT_CLIENT_ID, ADMIN,
-            ADMIN_CLIENT_ID, OWNER,
-            ADMIN_CLIENT_ID, ADMIN)
-        .inOrder();
-  }
-
-  /**
-   * Unauthorized admins only have full access to the admin registrar, and read-only to the rest.
-   */
-  @Test
-  public void getAllClientIdWithAccess_unauthorizedAdmin() {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            UNAUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.getAllClientIdWithRoles())
-        .containsExactly(
-            ADMIN_CLIENT_ID, OWNER,
-            ADMIN_CLIENT_ID, ADMIN,
-            DEFAULT_CLIENT_ID, ADMIN)
-        .inOrder();
   }
 
   /** Fail loading registrar if user doesn't have access to it. */
   @Test
   public void testGetRegistrarForUser_noAccess_isNotAdmin() {
     expectGetRegistrarFailure(
-        DEFAULT_CLIENT_ID,
+        REAL_CLIENT_ID_WITHOUT_CONTACT,
-        UNAUTHORIZED_USER,
+        USER,
-        "user unauth_user@gmail.com doesn't have access to registrar TheRegistrar");
+        "user user@gmail.com doesn't have access to registrar NewRegistrar");
+  }
+
+  /** Fail loading registrar if user doesn't have access to it, even if it's not REAL. */
+  @Test
+  public void testGetRegistrarForUser_noAccess_isNotAdmin_notReal() {
+    expectGetRegistrarFailure(
+        OTE_CLIENT_ID_WITHOUT_CONTACT,
+        USER,
+        "user user@gmail.com doesn't have access to registrar OteRegistrar");
   }
 
   /** Fail loading registrar if there's no user associated with the request. */
   @Test
   public void testGetRegistrarForUser_noUser() {
     expectGetRegistrarFailure(
-        DEFAULT_CLIENT_ID,
+        CLIENT_ID_WITH_CONTACT,
         NO_USER,
         "<logged-out user> doesn't have access to registrar TheRegistrar");
   }
 
   /** Succeed loading registrar if user has access to it. */
   @Test
-  public void testGetRegistrarForUser_hasAccess_isNotAdmin() throws Exception {
+  public void testGetRegistrarForUser_inContacts_isNotAdmin() throws Exception {
     expectGetRegistrarSuccess(
-        AUTHORIZED_USER, "user auth_user@gmail.com has [OWNER] access to registrar TheRegistrar");
+        CLIENT_ID_WITH_CONTACT,
+        USER,
+        "user user@gmail.com has [OWNER] access to registrar TheRegistrar");
   }
 
   /** Succeed loading registrar if admin with access. */
   @Test
-  public void testGetRegistrarForUser_hasAccess_isAdmin() throws Exception {
+  public void testGetRegistrarForUser_inContacts_isAdmin() throws Exception {
     expectGetRegistrarSuccess(
-        AUTHORIZED_ADMIN,
+        CLIENT_ID_WITH_CONTACT,
-        "admin auth_admin@gmail.com has [OWNER, ADMIN] access to registrar TheRegistrar");
+        GAE_ADMIN,
+        "admin admin@gmail.com has [OWNER, ADMIN] access to registrar TheRegistrar");
   }
 
   /** Succeed loading registrar for admin even if they aren't on the approved contacts list. */
   @Test
-  public void testGetRegistrarForUser_noAccess_isAdmin() throws Exception {
+  public void testGetRegistrarForUser_notInContacts_isAdmin() throws Exception {
     expectGetRegistrarSuccess(
-        UNAUTHORIZED_ADMIN,
+        REAL_CLIENT_ID_WITHOUT_CONTACT,
-        "admin unauth_admin@gmail.com has [ADMIN] access to registrar TheRegistrar.");
+        GAE_ADMIN,
+        "admin admin@gmail.com has [ADMIN] access to registrar NewRegistrar.");
+  }
+
+  /** Succeed loading non-REAL registrar for admin. */
+  @Test
+  public void testGetRegistrarForUser_notInContacts_isAdmin_notReal() throws Exception {
+    expectGetRegistrarSuccess(
+        OTE_CLIENT_ID_WITHOUT_CONTACT,
+        GAE_ADMIN,
+        "admin admin@gmail.com has [OWNER, ADMIN] access to registrar OteRegistrar.");
   }
 
   /** Fail loading registrar even if admin, if registrar doesn't exist. */
@@ -255,17 +296,18 @@ public class AuthenticatedRegistrarAccessorTest {
   public void testGetRegistrarForUser_doesntExist_isAdmin() {
     expectGetRegistrarFailure(
         "BadClientId",
-        AUTHORIZED_ADMIN,
+        GAE_ADMIN,
-        "admin auth_admin@gmail.com doesn't have access to registrar BadClientId");
+        "admin admin@gmail.com doesn't have access to registrar BadClientId");
   }
 
-  private void expectGetRegistrarSuccess(AuthResult authResult, String message) throws Exception {
+  private void expectGetRegistrarSuccess(String clientId, AuthResult authResult, String message)
+      throws Exception {
     AuthenticatedRegistrarAccessor registrarAccessor =
         new AuthenticatedRegistrarAccessor(
             authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
 
     // make sure loading the registrar succeeds and returns a value
-    assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID)).isNotNull();
+    assertThat(registrarAccessor.getRegistrar(clientId)).isNotNull();
     assertAboutLogs().that(testLogHandler).hasLogAtLevelWithMessage(Level.INFO, message);
   }
 
@@ -283,80 +325,28 @@ public class AuthenticatedRegistrarAccessorTest {
     assertThat(exception).hasMessageThat().contains(message);
   }
 
-  /** If a user has access to a registrar, we should guess that registrar. */
+  /** guessClientIdForUser returns the first clientId in getAllClientIdWithRoles. */
   @Test
-  public void testGuessClientIdForUser_hasAccess_isNotAdmin() throws Exception {
+  public void testGuessClientIdForUser_hasAccess_returnsFirst() throws Exception {
     AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
+        AuthenticatedRegistrarAccessor.createForTesting(
-            AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
+            ImmutableSetMultimap.of(
+                "clientId-1", OWNER,
+                "clientId-2", OWNER,
+                "clientId-2", ADMIN));
 
-    assertThat(registrarAccessor.guessClientId()).isEqualTo(DEFAULT_CLIENT_ID);
+    assertThat(registrarAccessor.guessClientId()).isEqualTo("clientId-1");
   }
 
-  /** If a user doesn't have access to any registrars, guess returns nothing. */
+  /** If a user doesn't have access to any registrars, guess fails. */
   @Test
-  public void testGuessClientIdForUser_noAccess_isNotAdmin() {
+  public void testGuessClientIdForUser_noAccess_fails() {
-    expectGuessRegistrarFailure(
-        UNAUTHORIZED_USER, "user unauth_user@gmail.com isn't associated with any registrar");
-  }
-
-  /**
-   * If an admin has access to a registrar, we should guess that registrar (rather than the
-   * ADMIN_CLIENT_ID).
-   */
-  @Test
-  public void testGuessClientIdForUser_hasAccess_isAdmin() throws Exception {
     AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
+        AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of());
-            AUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
 
-    assertThat(registrarAccessor.guessClientId()).isEqualTo(DEFAULT_CLIENT_ID);
+    assertThat(assertThrows(RegistrarAccessDeniedException.class, registrarAccessor::guessClientId))
-  }
+        .hasMessageThat()
-
+        .isEqualTo("TestUserId isn't associated with any registrar");
-  /** If an admin doesn't have access to a registrar, we should guess the ADMIN_CLIENT_ID. */
-  @Test
-  public void testGuessClientIdForUser_noAccess_isAdmin() throws Exception {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            UNAUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.guessClientId()).isEqualTo(ADMIN_CLIENT_ID);
-  }
-
-  /**
-   * If an admin is not associated with a registrar and there is no configured adminClientId, but
-   * since it's an admin - we have read-only access to everything - return one of the existing
-   * registrars.
-   */
-  @Test
-  public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdEmpty() throws Exception {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(UNAUTHORIZED_ADMIN, "", SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.guessClientId()).isAnyOf(ADMIN_CLIENT_ID, DEFAULT_CLIENT_ID);
-  }
-
-  /**
-   * 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 fail loading the registrar).
-   */
-  @Test
-  public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdInvalid() throws Exception {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            UNAUTHORIZED_ADMIN, "NonexistentRegistrar", SUPPORT_GROUP, groupsConnection);
-
-    assertThat(registrarAccessor.guessClientId()).isEqualTo("NonexistentRegistrar");
-  }
-
-  private void expectGuessRegistrarFailure(AuthResult authResult, String message) {
-    AuthenticatedRegistrarAccessor registrarAccessor =
-        new AuthenticatedRegistrarAccessor(
-            authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection);
-
-    RegistrarAccessDeniedException exception =
-        assertThrows(RegistrarAccessDeniedException.class, () -> registrarAccessor.guessClientId());
-    assertThat(exception).hasMessageThat().contains(message);
   }
 
   @Test

javatests/google/registry/request/auth/BUILD

@@ -13,6 +13,7 @@ java_library(
     resources = glob(["testdata/*"]),
     deps = [
         "//java/google/registry/groups",
+        "//java/google/registry/model",
         "//java/google/registry/request/auth",
         "//java/google/registry/security",
         "//javatests/google/registry/testing",

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

@@ -28,8 +28,13 @@ import static org.mockito.Mockito.verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import google.registry.export.sheet.SyncRegistrarsSheetAction;
 import google.registry.model.registrar.Registrar;
+import google.registry.request.auth.AuthenticatedRegistrarAccessor;
+import google.registry.request.auth.AuthenticatedRegistrarAccessor.Role;
 import google.registry.testing.CertificateSamples;
 import google.registry.testing.TaskQueueHelper.TaskMatcher;
 import google.registry.util.CidrAddressBlock;
@@ -109,10 +114,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testUpdate_emptyJsonObject_errorLastUpdateTimeFieldRequired() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.remove("lastUpdateTime");
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of()));
+        "args", args));
+
     assertThat(response).containsExactly(
         "status", "ERROR",
         "field", "lastUpdateTime",
@@ -124,10 +133,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testUpdate_noEmail_errorEmailFieldRequired() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.remove("emailAddress");
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime())));
+        "args", args));
+
     assertThat(response).containsExactly(
         "status", "ERROR",
         "field", "emailAddress",
@@ -140,13 +153,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   @Test
   public void testUpdate_emptyJsonObject_emailFieldNotRequiredWhenEmpty() {
     persistResource(loadRegistrar(CLIENT_ID).asBuilder().setEmailAddress(null).build());
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.remove("emailAddress");
 
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of(
+        "args", args));
-            "allowedTlds", ImmutableList.of("currenttld"),
+
-            "lastUpdateTime", getLastUpdateTime())));
     assertThat(response).containsExactly(
         "status", "SUCCESS",
         "message", "Saved TheRegistrar",
@@ -157,10 +171,12 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   @Test
   public void testFailure_updateRegistrarInfo_notAuthorized() {
     setUserWithoutAccess();
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
         "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime())));
+
     assertThat(response)
         .containsExactly(
             "status", "ERROR",
@@ -172,12 +188,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testUpdate_badEmail_errorEmailField() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("emailAddress", "lolcat");
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of(
+        "args", args));
-            "lastUpdateTime", getLastUpdateTime(),
+
-            "emailAddress", "lolcat")));
     assertThat(response).containsExactly(
         "status", "ERROR",
         "field", "emailAddress",
@@ -189,10 +207,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testPost_nonParsableTime_getsAngry() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("lastUpdateTime", "cookies");
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of("lastUpdateTime", "cookies")));
+        "args", args));
+
     assertThat(response).containsExactly(
         "status", "ERROR",
         "field", "lastUpdateTime",
@@ -204,12 +226,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testPost_nonAsciiCharacters_getsAngry() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("emailAddress", "ヘ(◕。◕ヘ)@example.com");
+
     Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
         "op", "update",
         "id", CLIENT_ID,
-        "args", ImmutableMap.of(
+        "args", args));
-            "lastUpdateTime", getLastUpdateTime(),
+
-            "emailAddress", "ヘ(◕。◕ヘ)@example.com")));
     assertThat(response).containsExactly(
         "status", "ERROR",
         "field", "emailAddress",
@@ -219,128 +243,192 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
     assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: FormFieldException");
   }
 
+  /**
+   * Makes sure a field update succeeds IF AND ONLY IF we have the "correct" role.
+   *
+   * Each of the Registrar fields can be changed only by a single {@link Role}. We make sure that
+   * trying to update the field works if the user has the "correct" role, but failes if it doesn't.
+   */
   private <T> void doTestUpdate(
+      Role correctRole,
       Function<Registrar, T> getter,
       T newValue,
       BiFunction<Registrar.Builder, T, Registrar.Builder> setter) {
+    doTestUpdateWithCorrectRole_succeeds(correctRole, getter, newValue, setter);
+    doTestUpdateWithoutCorrectRole_failes(correctRole, getter, newValue, setter);
+  }
+
+  private <T> void doTestUpdateWithCorrectRole_succeeds(
+      Role role,
+      Function<Registrar, T> getter,
+      T newValue,
+      BiFunction<Registrar.Builder, T, Registrar.Builder> setter) {
+    // Set the user to only have the current role for this registrar
+    action.registrarAccessor =
+        AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of(CLIENT_ID, role));
+    // Load the registrar as it is currently in datastore, and make sure the requested update will
+    // actually change it
     Registrar registrar = loadRegistrar(CLIENT_ID);
     assertThat(getter.apply(registrar)).isNotEqualTo(newValue);
 
+    // Call the action to perform the requested update, then load the "updated" registrar and
+    // return the "datastore" registrar to its original state (for the next iteration)
     Map<String, Object> response =
         action.handleJsonRequest(
             ImmutableMap.of(
                 "op", "update",
                 "id", CLIENT_ID,
                 "args", setter.apply(registrar.asBuilder(), newValue).build().toJsonMap()));
+    Registrar updatedRegistrar = loadRegistrar(CLIENT_ID);
+    persistResource(registrar);
 
-    registrar = loadRegistrar(CLIENT_ID);
+    // This role is authorized to perform this change, make sure the change succeeded
+    // We got a success result:
     assertThat(response).containsEntry("status", "SUCCESS");
-    assertThat(response).containsEntry("results", asList(registrar.toJsonMap()));
+    assertThat(response).containsEntry("results", asList(updatedRegistrar.toJsonMap()));
-    assertThat(getter.apply(registrar)).isEqualTo(newValue);
+    // The updatedRegistrar had its value changed:
+    // (We check it separately from the next assert to get better error message on failure)
+    assertThat(getter.apply(updatedRegistrar)).isEqualTo(newValue);
+    // ONLY that value changed:
+    assertThat(updatedRegistrar).isEqualTo(setter.apply(registrar.asBuilder(), newValue).build());
+    // We increased the correct metric
+    assertMetric(CLIENT_ID, "update", String.format("[%s]", role), "SUCCESS");
+  }
+
+  private <T> void doTestUpdateWithoutCorrectRole_failes(
+      Role correctRole,
+      Function<Registrar, T> getter,
+      T newValue,
+      BiFunction<Registrar.Builder, T, Registrar.Builder> setter) {
+    // Set the user to only have the current role for this registrar
+    ImmutableSet<Role> allExceptCorrectRoles =
+        Sets.difference(ImmutableSet.copyOf(Role.values()), ImmutableSet.of(correctRole))
+            .immutableCopy();
+    ImmutableSetMultimap<String, Role> accessMap =
+        new ImmutableSetMultimap.Builder<String, Role>()
+            .putAll(CLIENT_ID, allExceptCorrectRoles)
+            .build();
+    action.registrarAccessor =
+        AuthenticatedRegistrarAccessor.createForTesting(accessMap);
+    // Load the registrar as it is currently in datastore, and make sure the requested update will
+    // actually change it
+    Registrar registrar = loadRegistrar(CLIENT_ID);
+    assertThat(getter.apply(registrar)).isNotEqualTo(newValue);
+
+    // Call the action to perform the requested update, then load the "updated" registrar and
+    // returned the "datastore" registrar to its original state (for the next iteration)
+    Map<String, Object> response =
+        action.handleJsonRequest(
+            ImmutableMap.of(
+                "op",
+                "update",
+                "id",
+                CLIENT_ID,
+                "args",
+                setter.apply(registrar.asBuilder(), newValue).build().toJsonMap()));
+    Registrar updatedRegistrar = loadRegistrar(CLIENT_ID);
+    persistResource(registrar);
+
+    // This role is NOT authorized to perform this change, make sure the change failed
+    // We got an error response with the correct message
+    assertThat(response).containsEntry("status", "ERROR");
+    assertThat(response).containsEntry("results", ImmutableList.of());
+    assertThat(response.get("message").toString())
+        .containsMatch("Unauthorized: only .* can change fields .*");
+    // Make sure the value hasn't changed
+    // (We check it separately from the next assert to get better error message on failure)
+    assertThat(getter.apply(updatedRegistrar)).isEqualTo(getter.apply(registrar));
+    // Make sure no other values have changed either
+    assertThat(updatedRegistrar).isEqualTo(registrar);
+    // We increased the correct metric
+    assertMetric(
+        CLIENT_ID, "update", allExceptCorrectRoles.toString(), "ERROR: ForbiddenException");
   }
 
   @Test
   public void testUpdate_premiumPriceAck() {
     doTestUpdate(
-        Registrar::getPremiumPriceAckRequired, true, Registrar.Builder::setPremiumPriceAckRequired);
+        Role.OWNER,
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+        Registrar::getPremiumPriceAckRequired,
+        true,
+        Registrar.Builder::setPremiumPriceAckRequired);
   }
 
   @Test
   public void testUpdate_whoisServer() {
-    doTestUpdate(Registrar::getWhoisServer, "new-whois.example", Registrar.Builder::setWhoisServer);
+    doTestUpdate(
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+        Role.OWNER,
+        Registrar::getWhoisServer,
+        "new-whois.example",
+        Registrar.Builder::setWhoisServer);
   }
 
   @Test
   public void testUpdate_phoneNumber() {
-    doTestUpdate(Registrar::getPhoneNumber, "+1.2345678900", Registrar.Builder::setPhoneNumber);
+    doTestUpdate(
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+        Role.OWNER, Registrar::getPhoneNumber, "+1.2345678900", Registrar.Builder::setPhoneNumber);
   }
 
   @Test
   public void testUpdate_faxNumber() {
-    doTestUpdate(Registrar::getFaxNumber, "+1.2345678900", Registrar.Builder::setFaxNumber);
+    doTestUpdate(
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
+        Role.OWNER, Registrar::getFaxNumber, "+1.2345678900", Registrar.Builder::setFaxNumber);
   }
 
   @Test
   public void testUpdate_url() {
-    doTestUpdate(Registrar::getUrl, "new-url.example", Registrar.Builder::setUrl);
+    doTestUpdate(Role.OWNER, Registrar::getUrl, "new-url.example", Registrar.Builder::setUrl);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_ipAddressWhitelist() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getIpAddressWhitelist,
         ImmutableList.of(CidrAddressBlock.create("1.1.1.0/24")),
         Registrar.Builder::setIpAddressWhitelist);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_clientCertificate() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getClientCertificate,
         CertificateSamples.SAMPLE_CERT,
         (builder, s) -> builder.setClientCertificate(s, clock.nowUtc()));
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_failoverClientCertificate() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getFailoverClientCertificate,
         CertificateSamples.SAMPLE_CERT,
         (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc()));
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
-  public void testUpdate_allowedTlds_succeedWhenUserIsAdmin() {
+  public void testUpdate_allowedTlds() {
-    setUserAdmin();
     doTestUpdate(
+        Role.ADMIN,
         Registrar::getAllowedTlds,
         ImmutableSet.of("newtld", "currenttld"),
         (builder, s) -> builder.setAllowedTlds(s));
-    assertMetric(CLIENT_ID, "update", "[ADMIN]", "SUCCESS");
-  }
-
-  @Test
-  public void testUpdate_allowedTlds_failedWhenUserIsNotAdmin() {
-    Map<String, Object> response =
-        action.handleJsonRequest(
-            ImmutableMap.of(
-                "op", "update",
-                "id", CLIENT_ID,
-                "args",
-                ImmutableMap.of(
-                    "lastUpdateTime", getLastUpdateTime(),
-                    "emailAddress", "abc@def.com",
-                    "allowedTlds", ImmutableList.of("newtld"))));
-    assertThat(response)
-        .containsExactly(
-            "status", "ERROR",
-            "results", ImmutableList.of(),
-            "message", "Only admin can update allowed TLDs.");
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "ERROR: ForbiddenException");
-    assertNoTasksEnqueued("sheet");
   }
 
   @Test
   public void testUpdate_allowedTlds_failedWhenTldNotExist() {
     setUserAdmin();
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("allowedTlds", ImmutableList.of("invalidtld", "currenttld"));
+
     Map<String, Object> response =
         action.handleJsonRequest(
             ImmutableMap.of(
                 "op", "update",
                 "id", CLIENT_ID,
-                "args",
+                "args", args));
-                ImmutableMap.of(
+
-                    "lastUpdateTime", getLastUpdateTime(),
-                    "emailAddress", "abc@def.com",
-                    "allowedTlds", ImmutableList.of("invalidtld", "currenttld"))));
     assertThat(response)
         .containsExactly(
             "status", "ERROR",
@@ -353,16 +441,16 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   @Test
   public void testUpdate_allowedTlds_failedWhenRemovingTld() {
     setUserAdmin();
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("allowedTlds", ImmutableList.of("newtld"));
+
     Map<String, Object> response =
         action.handleJsonRequest(
             ImmutableMap.of(
                 "op", "update",
                 "id", CLIENT_ID,
-                "args",
+                "args", args));
-                ImmutableMap.of(
+
-                    "lastUpdateTime", getLastUpdateTime(),
-                    "emailAddress", "abc@def.com",
-                    "allowedTlds", ImmutableList.of("newTld"))));
     assertThat(response)
         .containsExactly(
             "status", "ERROR",
@@ -374,16 +462,16 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
 
   @Test
   public void testUpdate_allowedTlds_noChange_successWhenUserIsNotAdmin() {
+    Map<String, Object> args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap());
+    args.put("allowedTlds", ImmutableList.of("currenttld"));
+
     Map<String, Object> response =
         action.handleJsonRequest(
             ImmutableMap.of(
                 "op", "update",
                 "id", CLIENT_ID,
-                "args",
+                "args", args));
-                ImmutableMap.of(
+
-                    "lastUpdateTime", getLastUpdateTime(),
-                    "emailAddress", "abc@def.com",
-                    "allowedTlds", ImmutableList.of("currenttld"))));
     assertThat(response)
         .containsExactly(
             "status", "SUCCESS",
@@ -395,33 +483,34 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
   @Test
   public void testUpdate_localizedAddress_city() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getLocalizedAddress,
         loadRegistrar(CLIENT_ID).getLocalizedAddress().asBuilder().setCity("newCity").build(),
         Registrar.Builder::setLocalizedAddress);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_localizedAddress_countryCode() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getLocalizedAddress,
         loadRegistrar(CLIENT_ID).getLocalizedAddress().asBuilder().setCountryCode("GB").build(),
         Registrar.Builder::setLocalizedAddress);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_localizedAddress_state() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getLocalizedAddress,
         loadRegistrar(CLIENT_ID).getLocalizedAddress().asBuilder().setState("NJ").build(),
         Registrar.Builder::setLocalizedAddress);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_localizedAddress_street() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getLocalizedAddress,
         loadRegistrar(CLIENT_ID)
             .getLocalizedAddress()
@@ -429,16 +518,15 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
             .setStreet(ImmutableList.of("new street"))
             .build(),
         Registrar.Builder::setLocalizedAddress);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   @Test
   public void testUpdate_localizedAddress_zip() {
     doTestUpdate(
+        Role.OWNER,
         Registrar::getLocalizedAddress,
         loadRegistrar(CLIENT_ID).getLocalizedAddress().asBuilder().setZip("new zip").build(),
         Registrar.Builder::setLocalizedAddress);
-    assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS");
   }
 
   private static String getLastUpdateTime() {

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

@@ -15,7 +15,6 @@
 package google.registry.ui.server.registrar;
 
 import static com.google.common.truth.Truth.assertThat;
-import static google.registry.config.RegistryConfig.getDefaultRegistrarWhoisServer;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT2;
 import static google.registry.testing.CertificateSamples.SAMPLE_CERT2_HASH;
@@ -52,12 +51,6 @@ public class SecuritySettingsTest extends RegistrarSettingsActionTestCase {
         "op", "update",
         "id", CLIENT_ID,
         "args", modified.toJsonMap()));
-    // Empty whoisServer field should be set to default by server.
-    modified =
-        modified
-            .asBuilder()
-            .setWhoisServer(getDefaultRegistrarWhoisServer())
-            .build();
     assertThat(response).containsEntry("status", "SUCCESS");
     assertThat(response).containsEntry("results", asList(modified.toJsonMap()));
     assertThat(loadRegistrar(CLIENT_ID)).isEqualTo(modified);