diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index d4802ffc6..a4b09ec5b 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -37,6 +37,7 @@ import google.registry.model.registrar.RegistrarContact; import google.registry.model.registrar.RegistrarContact.Type; import google.registry.request.Action; import google.registry.request.HttpException.BadRequestException; +import google.registry.request.HttpException.ForbiddenException; import google.registry.request.JsonActionRunner; import google.registry.request.auth.Auth; import google.registry.request.auth.AuthResult; @@ -44,6 +45,7 @@ import google.registry.security.JsonResponseHelper; import google.registry.ui.forms.FormException; import google.registry.ui.forms.FormFieldException; import google.registry.ui.server.RegistrarFormFields; +import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role; import google.registry.util.AppEngineServiceUtils; import google.registry.util.CollectionUtils; import google.registry.util.DiffUtils; @@ -160,8 +162,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA "registrar changed since reading the data! " + " Last updated at %s, but args data last updated at %s", latest, latestFromArgs); - return JsonResponseHelper.create( - ERROR, "registrar has been changed by someone else. Please reload and retry."); + throw new IllegalStateException( + "registrar has been changed by someone else. Please reload and retry."); } // Keep the current contacts so we can later check that no required contact was @@ -170,7 +172,8 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA // Update the registrar from the request. Registrar.Builder builder = registrar.asBuilder(); - changeRegistrarFields(registrar, builder, args); + Set roles = registrarAccessor.getAllClientIdWithRoles().get(clientId); + changeRegistrarFields(registrar, roles, builder, args); // read the contacts from the request. ImmutableSet updatedContacts = readContacts(registrar, args); @@ -211,11 +214,12 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA return result; } - /** - * Updates a registrar builder with the supplied args from the http request; - */ + /** Updates a registrar builder with the supplied args from the http request; */ public static void changeRegistrarFields( - Registrar existingRegistrarObj, Registrar.Builder builder, Map args) { + Registrar existingRegistrarObj, + Set roles, + Registrar.Builder builder, + Map args) { // BILLING RegistrarFormFields.PREMIUM_PRICE_ACK_REQUIRED @@ -255,6 +259,18 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA .ifPresent( certificate -> builder.setFailoverClientCertificate(certificate, ofy().getTransactionTime())); + + // Update allowed TLDs only when it is modified + 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)) { + builder.setAllowedTlds(updatedAllowedTlds); + } else { + throw new ForbiddenException("Only admin can update allowed TLDs."); + } + } } /** Reads the contacts from the supplied args. */ diff --git a/javatests/google/registry/testing/DatastoreHelper.java b/javatests/google/registry/testing/DatastoreHelper.java index 897d0431c..56b3b52fc 100644 --- a/javatests/google/registry/testing/DatastoreHelper.java +++ b/javatests/google/registry/testing/DatastoreHelper.java @@ -454,7 +454,7 @@ public class DatastoreHelper { registrar.asBuilder().setAllowedTlds(union(registrar.getAllowedTlds(), tld)).build()); } - private static void disallowRegistrarAccess(String clientId, String tld) { + public static void disallowRegistrarAccess(String clientId, String tld) { Registrar registrar = loadRegistrar(clientId); persistResource( registrar.asBuilder().setAllowedTlds(difference(registrar.getAllowedTlds(), tld)).build()); diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index e7a72c927..585ffcccc 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -27,6 +27,7 @@ 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 google.registry.export.sheet.SyncRegistrarsSheetAction; import google.registry.model.registrar.Registrar; import google.registry.testing.CertificateSamples; @@ -136,7 +137,9 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase Map response = action.handleJsonRequest(ImmutableMap.of( "op", "update", "id", CLIENT_ID, - "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime()))); + "args", ImmutableMap.of( + "allowedTlds", ImmutableList.of("currenttld"), + "lastUpdateTime", getLastUpdateTime()))); assertThat(response).containsExactly( "status", "SUCCESS", "message", "Saved TheRegistrar", @@ -273,6 +276,75 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase (builder, s) -> builder.setFailoverClientCertificate(s, clock.nowUtc())); } + @Test + public void testUpdate_allowedTlds_succeedWhenUserIsAdmin() { + setUserAdmin(); + doTestUpdate( + Registrar::getAllowedTlds, + ImmutableSet.of("newtld"), + (builder, s) -> builder.setAllowedTlds(s)); + } + + @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."); + assertNoTasksEnqueued("sheet"); + } + + @Test + public void testUpdate_allowedTlds_failedWhenTldNotExist() { + setUserAdmin(); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", + ImmutableMap.of( + "lastUpdateTime", getLastUpdateTime(), + "emailAddress", "abc@def.com", + "allowedTlds", ImmutableList.of("invalidtld")))); + assertThat(response) + .containsExactly( + "status", "ERROR", + "results", ImmutableList.of(), + "message", "TLDs do not exist: invalidtld"); + assertNoTasksEnqueued("sheet"); + } + + @Test + public void testUpdate_allowedTlds_noChange_successWhenUserIsNotAdmin() { + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", + ImmutableMap.of( + "lastUpdateTime", getLastUpdateTime(), + "emailAddress", "abc@def.com", + "allowedTlds", ImmutableList.of("currenttld")))); + assertThat(response) + .containsExactly( + "status", "SUCCESS", + "message", "Saved TheRegistrar", + "results", asList(loadRegistrar(CLIENT_ID).toJsonMap())); + } + @Test public void testUpdate_localizedAddress_city() { doTestUpdate( diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 95497579b..47dc1ce6d 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -17,13 +17,18 @@ package google.registry.ui.server.registrar; import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailAddress; import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDisplayName; import static google.registry.security.JsonHttpTestUtils.createJsonPayload; +import static google.registry.testing.DatastoreHelper.createTlds; +import static google.registry.testing.DatastoreHelper.disallowRegistrarAccess; import static google.registry.testing.DatastoreHelper.loadRegistrar; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.google.appengine.api.users.User; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSetMultimap; import google.registry.config.RegistryEnvironment; import google.registry.model.ofy.Ofy; import google.registry.request.HttpException.ForbiddenException; @@ -81,7 +86,13 @@ public class RegistrarSettingsActionTestCase { @Before public void setUp() throws Exception { - action.registrarAccessor = null; + // Create a tld and give access to registrar "TheRegistrar" for most common test case. + createTlds("currenttld"); + // Create another tld but remove access for registrar "TheRegistrar". This is for the test case + // of updating allowed tld for registrar + createTlds("newtld"); + disallowRegistrarAccess(CLIENT_ID, "newtld"); + action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class); action.appEngineServiceUtils = appEngineServiceUtils; when(appEngineServiceUtils.getCurrentVersionHostname("backend")).thenReturn("backend.hostname"); action.jsonActionRunner = new JsonActionRunner( @@ -110,17 +121,24 @@ public class RegistrarSettingsActionTestCase { /** Sets registrarAccessor.getRegistrar to succeed for all AccessTypes. */ protected void setUserWithAccess() { - action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class); - + when(action.registrarAccessor.getAllClientIdWithRoles()) + .thenReturn(ImmutableSetMultimap.of(CLIENT_ID, OWNER)); when(action.registrarAccessor.getRegistrar(CLIENT_ID)) .thenAnswer(x -> loadRegistrar(CLIENT_ID)); } /** Sets registrarAccessor.getRegistrar to always fail. */ protected void setUserWithoutAccess() { - action.registrarAccessor = mock(AuthenticatedRegistrarAccessor.class); - + when(action.registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of()); when(action.registrarAccessor.getRegistrar(CLIENT_ID)) .thenThrow(new ForbiddenException("forbidden test error")); } + + /** + * Sets registrarAccessor.getAllClientIdWithRoles to return a map with admin role for CLIENT_ID + */ + protected void setUserAdmin() { + when(action.registrarAccessor.getAllClientIdWithRoles()) + .thenReturn(ImmutableSetMultimap.of(CLIENT_ID, ADMIN)); + } } diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json b/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json index 672e1bd33..a4508e3c4 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json +++ b/javatests/google/registry/ui/server/registrar/testdata/update_registrar.json @@ -21,7 +21,7 @@ } ], "allowedTlds": [ - "ga" + "currenttld" ], "clientCertificateHash": null, "faxNumber": "", diff --git a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json b/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json index b5b979c1f..9a66daf69 100644 --- a/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json +++ b/javatests/google/registry/ui/server/registrar/testdata/update_registrar_duplicate_contacts.json @@ -31,7 +31,7 @@ } ], "allowedTlds": [ - "ga" + "currenttld" ], "clientCertificateHash": null, "faxNumber": "",