diff --git a/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java index bd3c7df1f..b49b5f3d4 100644 --- a/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java +++ b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java @@ -38,9 +38,13 @@ import javax.inject.Inject; *

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. * - *

An admin has in addition OWNER role on {@code #registryAdminClientId}. + *

An "admin" has in addition OWNER role on {@code #registryAdminClientId} and to all non-{@code + * REAL} registrars (see {@link Registrar#getType}). * - *

An admin also has ADMIN role on ALL registrars. + *

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

A user is an "admin" if they are a GAE-admin, or if their email is in the "Support" G-Suite + * 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. + * + *

This is syntactic sugar for {@code getAllClientIdWithRoles().get(clientId)}. + */ + public ImmutableSet getRolesForRegistrar(String clientId) { + return getAllClientIdWithRoles().get(clientId); + } + + /** + * Checks if we have a given role for a given registrar. + * + *

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(); diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index b49c676ec..7da38302b 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/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 contacts = registrar.getContacts(); - // Update the registrar from the request. - Registrar.Builder builder = registrar.asBuilder(); - Set roles = registrarAccessor.getAllClientIdWithRoles().get(clientId); - changeRegistrarFields(registrar, roles, builder, args); + Registrar updatedRegistrar = registrar; + // Do OWNER only updates to the registrar from the request. + updatedRegistrar = checkAndUpdateOwnerControlledFields(updatedRegistrar, args); + // Do ADMIN only updates to the registrar from the request. + updatedRegistrar = checkAndUpdateAdminControlledFields(updatedRegistrar, args); // read the contacts from the request. ImmutableSet 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( - Registrar existingRegistrarObj, - Set roles, - Registrar.Builder builder, - Map args) { + /** + * Updates registrar with the OWNER-controlled args from the http request. + * + *

If any changes were made and the user isn't an OWNER - throws a {@link ForbiddenException}. + */ + private Registrar checkAndUpdateOwnerControlledFields( + Registrar initialRegistrar, Map 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. + * + *

If any changes were made and the user isn't an ADMIN - throws a {@link ForbiddenException}. + */ + private Registrar checkAndUpdateAdminControlledFields( + Registrar initialRegistrar, Map args) { + Registrar.Builder builder = initialRegistrar.asBuilder(); + Set 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 removedTlds = - Sets.difference(existingRegistrarObj.getAllowedTlds(), updatedAllowedTlds); - if (!removedTlds.isEmpty()) { - throw new ForbiddenException("Can't remove allowed TLDs using the console."); - } - builder.setAllowedTlds(updatedAllowedTlds); + // 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. + if (!Sets.difference(initialRegistrar.getAllowedTlds(), updatedAllowedTlds).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. + * + *

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. */ diff --git a/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java b/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java index 115aee1d8..4845b4f61 100644 --- a/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java +++ b/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 UNAUTHORIZED_USER = createAuthResult(false, false); - private static final AuthResult AUTHORIZED_ADMIN = createAuthResult(true, true); - private static final AuthResult UNAUTHORIZED_ADMIN = createAuthResult(false, true); + private static final AuthResult USER = createAuthResult(false); + private static final AuthResult GAE_ADMIN = createAuthResult(true); private static final AuthResult NO_USER = AuthResult.create(AuthLevel.NONE); private static final String SUPPORT_GROUP = "support@registry.example"; - private static final String DEFAULT_CLIENT_ID = "TheRegistrar"; - private static final String ADMIN_CLIENT_ID = "NewRegistrar"; + /** Client ID of a REAL registrar with a RegistrarContact for USER and GAE_ADMIN. */ + private static final String CLIENT_ID_WITH_CONTACT = "TheRegistrar"; + /** Client ID of a REAL registrar without a RegistrarContact. */ + 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", - isAuthorized ? "auth" : "unauth", isAdmin ? "admin" : "user"), + "%s@gmail.com", + 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); - } - - /** 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); + .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER); } /** 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. + * + *

They also have OWNER access if they are in the RegistrarContacts. + * + *

They also have OWNER access to the Admin Registrar. + * + *

They also have OWNER access to non-REAL Registrars. + * + *

(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); - - 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); + GAE_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly( - DEFAULT_CLIENT_ID, ADMIN, - ADMIN_CLIENT_ID, ADMIN); + 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); + } + + /** + * Users in support group have admin access to everything. + * + *

They also have OWNER access if they are in the RegistrarContacts. + * + *

They also have OWNER access to the Admin Registrar. + * + *

They also have OWNER access to non-REAL Registrars. + * + *

(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); - } - - /** 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(); + .containsExactly(CLIENT_ID_WITH_CONTACT, OWNER); } /** Fail loading registrar if user doesn't have access to it. */ @Test public void testGetRegistrarForUser_noAccess_isNotAdmin() { expectGetRegistrarFailure( - DEFAULT_CLIENT_ID, - UNAUTHORIZED_USER, - "user unauth_user@gmail.com doesn't have access to registrar TheRegistrar"); + REAL_CLIENT_ID_WITHOUT_CONTACT, + USER, + "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, " 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, - "admin auth_admin@gmail.com has [OWNER, ADMIN] access to registrar TheRegistrar"); + CLIENT_ID_WITH_CONTACT, + 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, - "admin unauth_admin@gmail.com has [ADMIN] access to registrar TheRegistrar."); + REAL_CLIENT_ID_WITHOUT_CONTACT, + 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, - "admin auth_admin@gmail.com doesn't have access to registrar BadClientId"); + GAE_ADMIN, + "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( - AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + AuthenticatedRegistrarAccessor.createForTesting( + 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() { - 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 { + public void testGuessClientIdForUser_noAccess_fails() { AuthenticatedRegistrarAccessor registrarAccessor = - new AuthenticatedRegistrarAccessor( - AUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of()); - assertThat(registrarAccessor.guessClientId()).isEqualTo(DEFAULT_CLIENT_ID); - } - - /** 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); + assertThat(assertThrows(RegistrarAccessDeniedException.class, registrarAccessor::guessClientId)) + .hasMessageThat() + .isEqualTo("TestUserId isn't associated with any registrar"); } @Test diff --git a/javatests/google/registry/request/auth/BUILD b/javatests/google/registry/request/auth/BUILD index cda5fe006..f048d9a2a 100644 --- a/javatests/google/registry/request/auth/BUILD +++ b/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", diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 8a9aca361..b7c23d4dd 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.remove("lastUpdateTime"); + Map 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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.remove("emailAddress"); + Map 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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.remove("emailAddress"); Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", ImmutableMap.of( - "allowedTlds", ImmutableList.of("currenttld"), - "lastUpdateTime", getLastUpdateTime()))); + "args", args)); + 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 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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("emailAddress", "lolcat"); + Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", ImmutableMap.of( - "lastUpdateTime", getLastUpdateTime(), - "emailAddress", "lolcat"))); + "args", args)); + assertThat(response).containsExactly( "status", "ERROR", "field", "emailAddress", @@ -189,10 +207,14 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testPost_nonParsableTime_getsAngry() { + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("lastUpdateTime", "cookies"); + Map 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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("emailAddress", "ヘ(◕。◕ヘ)@example.com"); + Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", ImmutableMap.of( - "lastUpdateTime", getLastUpdateTime(), - "emailAddress", "ヘ(◕。◕ヘ)@example.com"))); + "args", args)); + 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 void doTestUpdate( + Role correctRole, Function getter, T newValue, BiFunction setter) { + doTestUpdateWithCorrectRole_succeeds(correctRole, getter, newValue, setter); + doTestUpdateWithoutCorrectRole_failes(correctRole, getter, newValue, setter); + } + + private void doTestUpdateWithCorrectRole_succeeds( + Role role, + Function getter, + T newValue, + BiFunction 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 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(getter.apply(registrar)).isEqualTo(newValue); + assertThat(response).containsEntry("results", asList(updatedRegistrar.toJsonMap())); + // 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 void doTestUpdateWithoutCorrectRole_failes( + Role correctRole, + Function getter, + T newValue, + BiFunction setter) { + // Set the user to only have the current role for this registrar + ImmutableSet allExceptCorrectRoles = + Sets.difference(ImmutableSet.copyOf(Role.values()), ImmutableSet.of(correctRole)) + .immutableCopy(); + ImmutableSetMultimap accessMap = + new ImmutableSetMultimap.Builder() + .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 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); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + Role.OWNER, + Registrar::getPremiumPriceAckRequired, + true, + Registrar.Builder::setPremiumPriceAckRequired); } @Test public void testUpdate_whoisServer() { - doTestUpdate(Registrar::getWhoisServer, "new-whois.example", Registrar.Builder::setWhoisServer); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + doTestUpdate( + Role.OWNER, + Registrar::getWhoisServer, + "new-whois.example", + Registrar.Builder::setWhoisServer); } @Test public void testUpdate_phoneNumber() { - doTestUpdate(Registrar::getPhoneNumber, "+1.2345678900", Registrar.Builder::setPhoneNumber); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + doTestUpdate( + Role.OWNER, Registrar::getPhoneNumber, "+1.2345678900", Registrar.Builder::setPhoneNumber); } @Test public void testUpdate_faxNumber() { - doTestUpdate(Registrar::getFaxNumber, "+1.2345678900", Registrar.Builder::setFaxNumber); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + doTestUpdate( + Role.OWNER, Registrar::getFaxNumber, "+1.2345678900", Registrar.Builder::setFaxNumber); } @Test public void testUpdate_url() { - doTestUpdate(Registrar::getUrl, "new-url.example", Registrar.Builder::setUrl); - assertMetric(CLIENT_ID, "update", "[OWNER]", "SUCCESS"); + doTestUpdate(Role.OWNER, Registrar::getUrl, "new-url.example", Registrar.Builder::setUrl); } @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() { - setUserAdmin(); + public void testUpdate_allowedTlds() { 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 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 args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("allowedTlds", ImmutableList.of("invalidtld", "currenttld")); + Map response = action.handleJsonRequest( ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", - ImmutableMap.of( - "lastUpdateTime", getLastUpdateTime(), - "emailAddress", "abc@def.com", - "allowedTlds", ImmutableList.of("invalidtld", "currenttld")))); + "args", args)); + assertThat(response) .containsExactly( "status", "ERROR", @@ -353,16 +441,16 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testUpdate_allowedTlds_failedWhenRemovingTld() { setUserAdmin(); + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("allowedTlds", ImmutableList.of("newtld")); + Map response = action.handleJsonRequest( ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", - ImmutableMap.of( - "lastUpdateTime", getLastUpdateTime(), - "emailAddress", "abc@def.com", - "allowedTlds", ImmutableList.of("newTld")))); + "args", args)); + assertThat(response) .containsExactly( "status", "ERROR", @@ -374,16 +462,16 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testUpdate_allowedTlds_noChange_successWhenUserIsNotAdmin() { + Map args = Maps.newHashMap(loadRegistrar(CLIENT_ID).toJsonMap()); + args.put("allowedTlds", ImmutableList.of("currenttld")); + Map response = action.handleJsonRequest( ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", - ImmutableMap.of( - "lastUpdateTime", getLastUpdateTime(), - "emailAddress", "abc@def.com", - "allowedTlds", ImmutableList.of("currenttld")))); + "args", args)); + 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() { diff --git a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java b/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java index eef3bbfb9..dd9ddd691 100644 --- a/javatests/google/registry/ui/server/registrar/SecuritySettingsTest.java +++ b/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);