From 806f3b2456ce0f09feab847d257452b3c8dc46c6 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Sun, 5 Jul 2020 22:31:14 -0400 Subject: [PATCH] Verify that the RegistryLock input has the correct registrar ID (#661) * Verify that the RegistryLock input has the correct registrar ID We already verify (correctly) that the user has access to the registrar they specify, but nowhere did we verify that the registrar ID they used is actually the current sponsor ID for the domain in question. This is an oversight caused by the fact that our testing framework only uses admin accounts, which by the nature of things have access to all registrars and domains. In addition, rename "clientId" to "registrarId" in the RLPA object * Change the wording on the incorrect-registrar message --- .../registry/tools/DomainLockUtils.java | 33 +++++++--- .../registrar/RegistryLockPostAction.java | 17 +++--- .../registry/ui/js/registrar/registry_lock.js | 2 +- .../batch/RelockDomainActionTest.java | 1 + .../registry/tools/DomainLockUtilsTest.java | 1 + .../registry/tools/LockDomainCommandTest.java | 7 ++- .../tools/UnlockDomainCommandTest.java | 21 ++++--- .../registrar/RegistryLockPostActionTest.java | 61 ++++++++++++++----- .../RegistryLockVerifyActionTest.java | 1 + 9 files changed, 95 insertions(+), 49 deletions(-) diff --git a/core/src/main/java/google/registry/tools/DomainLockUtils.java b/core/src/main/java/google/registry/tools/DomainLockUtils.java index ab5f8054f..39facc25c 100644 --- a/core/src/main/java/google/registry/tools/DomainLockUtils.java +++ b/core/src/main/java/google/registry/tools/DomainLockUtils.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.googlecode.objectify.Key; import google.registry.batch.AsyncTaskEnqueuer; +import google.registry.config.RegistryConfig.Config; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Reason; import google.registry.model.domain.DomainBase; @@ -52,13 +53,16 @@ public final class DomainLockUtils { private static final int VERIFICATION_CODE_LENGTH = 32; private final StringGenerator stringGenerator; + private final String registryAdminRegistrarId; private final AsyncTaskEnqueuer asyncTaskEnqueuer; @Inject public DomainLockUtils( @Named("base58StringGenerator") StringGenerator stringGenerator, + @Config("registryAdminClientId") String registryAdminRegistrarId, AsyncTaskEnqueuer asyncTaskEnqueuer) { this.stringGenerator = stringGenerator; + this.registryAdminRegistrarId = registryAdminRegistrarId; this.asyncTaskEnqueuer = asyncTaskEnqueuer; } @@ -217,7 +221,7 @@ public final class DomainLockUtils { private RegistryLock.Builder createLockBuilder( String domainName, String registrarId, @Nullable String registrarPocId, boolean isAdmin) { DateTime now = jpaTm().getTransactionTime(); - DomainBase domainBase = getDomain(domainName, now); + DomainBase domainBase = getDomain(domainName, registrarId, now); verifyDomainNotLocked(domainBase); // Multiple pending actions are not allowed @@ -242,7 +246,7 @@ public final class DomainLockUtils { private RegistryLock.Builder createUnlockBuilder( String domainName, String registrarId, boolean isAdmin, Optional relockDuration) { DateTime now = jpaTm().getTransactionTime(); - DomainBase domainBase = getDomain(domainName, now); + DomainBase domainBase = getDomain(domainName, registrarId, now); Optional lockOptional = RegistryLockDao.getMostRecentVerifiedLockByRepoId(domainBase.getRepoId()); @@ -303,10 +307,19 @@ public final class DomainLockUtils { domainBase.getDomainName()); } - private static DomainBase getDomain(String domainName, DateTime now) { - return loadByForeignKeyCached(DomainBase.class, domainName, now) - .orElseThrow( - () -> new IllegalArgumentException(String.format("Unknown domain %s", domainName))); + private DomainBase getDomain(String domainName, String registrarId, DateTime now) { + DomainBase domain = + loadByForeignKeyCached(DomainBase.class, domainName, now) + .orElseThrow( + () -> new IllegalArgumentException(String.format("Unknown domain %s", domainName))); + // The user must have specified either the correct registrar ID or the admin registrar ID + checkArgument( + registryAdminRegistrarId.equals(registrarId) + || domain.getCurrentSponsorClientId().equals(registrarId), + "Domain %s is not owned by registrar %s", + domainName, + registrarId); + return domain; } private static RegistryLock getByVerificationCode(String verificationCode) { @@ -317,8 +330,8 @@ public final class DomainLockUtils { String.format("Invalid verification code %s", verificationCode))); } - private static void applyLockStatuses(RegistryLock lock, DateTime lockTime) { - DomainBase domain = getDomain(lock.getDomainName(), lockTime); + private void applyLockStatuses(RegistryLock lock, DateTime lockTime) { + DomainBase domain = getDomain(lock.getDomainName(), lock.getRegistrarId(), lockTime); verifyDomainNotLocked(domain); DomainBase newDomain = @@ -330,8 +343,8 @@ public final class DomainLockUtils { saveEntities(newDomain, lock, lockTime, true); } - private static void removeLockStatuses(RegistryLock lock, boolean isAdmin, DateTime unlockTime) { - DomainBase domain = getDomain(lock.getDomainName(), unlockTime); + private void removeLockStatuses(RegistryLock lock, boolean isAdmin, DateTime unlockTime) { + DomainBase domain = getDomain(lock.getDomainName(), lock.getRegistrarId(), unlockTime); if (!isAdmin) { verifyDomainLocked(domain); } diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java index b3f59964a..a21e0e5d8 100644 --- a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java +++ b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java @@ -19,7 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.security.JsonResponseHelper.Status.ERROR; import static google.registry.security.JsonResponseHelper.Status.SUCCESS; -import static google.registry.ui.server.registrar.RegistrarConsoleModule.PARAM_CLIENT_ID; import static google.registry.ui.server.registrar.RegistryLockGetAction.getContactMatchingLogin; import static google.registry.ui.server.registrar.RegistryLockGetAction.getRegistrarAndVerifyLockAccess; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; @@ -116,10 +115,8 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc checkArgumentNotNull(input, "Null JSON"); RegistryLockPostInput postInput = GSON.fromJson(GSON.toJsonTree(input), RegistryLockPostInput.class); - checkArgument( - !Strings.isNullOrEmpty(postInput.clientId), - "Missing key for client: %s", - PARAM_CLIENT_ID); + String registrarId = postInput.registrarId; + checkArgument(!Strings.isNullOrEmpty(registrarId), "Missing key for registrarId"); checkArgument(!Strings.isNullOrEmpty(postInput.domainName), "Missing key for domainName"); checkNotNull(postInput.isLock, "Missing key for isLock"); UserAuthInfo userAuthInfo = @@ -135,12 +132,12 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc postInput.isLock ? domainLockUtils.saveNewRegistryLockRequest( postInput.domainName, - postInput.clientId, + registrarId, userEmail, registrarAccessor.isAdmin()) : domainLockUtils.saveNewRegistryUnlockRequest( postInput.domainName, - postInput.clientId, + registrarId, registrarAccessor.isAdmin(), Optional.ofNullable(postInput.relockDurationMillis).map(Duration::new)); sendVerificationEmail(registryLock, userEmail, postInput.isLock); @@ -190,9 +187,9 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc return user.getEmail(); } // Verify that the user can access the registrar, that the user has - // registry lock enabled, and that the user providjed a correct password + // registry lock enabled, and that the user provided a correct password Registrar registrar = - getRegistrarAndVerifyLockAccess(registrarAccessor, postInput.clientId, false); + getRegistrarAndVerifyLockAccess(registrarAccessor, postInput.registrarId, false); RegistrarContact registrarContact = getContactMatchingLogin(user, registrar) .orElseThrow( @@ -215,7 +212,7 @@ public class RegistryLockPostAction implements Runnable, JsonActionRunner.JsonAc /** Value class that represents the expected input body from the UI request. */ private static class RegistryLockPostInput { - private String clientId; + private String registrarId; private String domainName; private Boolean isLock; private String password; diff --git a/core/src/main/javascript/google/registry/ui/js/registrar/registry_lock.js b/core/src/main/javascript/google/registry/ui/js/registrar/registry_lock.js index 779bef2d4..5c1304685 100644 --- a/core/src/main/javascript/google/registry/ui/js/registrar/registry_lock.js +++ b/core/src/main/javascript/google/registry/ui/js/registrar/registry_lock.js @@ -172,7 +172,7 @@ registry.registrar.RegistryLock.prototype.lockOrUnlockDomain_ = function(isLock, e => this.fillLocksPage_(e), 'POST', goog.json.serialize({ - 'clientId': this.clientId, + 'registrarId': this.clientId, 'domainName': domain, 'isLock': isLock, 'password': password, diff --git a/core/src/test/java/google/registry/batch/RelockDomainActionTest.java b/core/src/test/java/google/registry/batch/RelockDomainActionTest.java index 1e7feea04..18a88891b 100644 --- a/core/src/test/java/google/registry/batch/RelockDomainActionTest.java +++ b/core/src/test/java/google/registry/batch/RelockDomainActionTest.java @@ -63,6 +63,7 @@ public class RelockDomainActionTest { private final DomainLockUtils domainLockUtils = new DomainLockUtils( new DeterministicStringGenerator(Alphabets.BASE_58), + "adminreg", AsyncTaskEnqueuerTest.createForTesting( mock(AppEngineServiceUtils.class), clock, Duration.ZERO)); diff --git a/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java b/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java index 3ee5706ae..aa0f10ac6 100644 --- a/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java +++ b/core/src/test/java/google/registry/tools/DomainLockUtilsTest.java @@ -74,6 +74,7 @@ public final class DomainLockUtilsTest { private final DomainLockUtils domainLockUtils = new DomainLockUtils( new DeterministicStringGenerator(Alphabets.BASE_58), + "adminreg", AsyncTaskEnqueuerTest.createForTesting( mock(AppEngineServiceUtils.class), clock, standardSeconds(90))); diff --git a/core/src/test/java/google/registry/tools/LockDomainCommandTest.java b/core/src/test/java/google/registry/tools/LockDomainCommandTest.java index 853c02c6c..d9987ad8d 100644 --- a/core/src/test/java/google/registry/tools/LockDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/LockDomainCommandTest.java @@ -51,6 +51,7 @@ public class LockDomainCommandTest extends CommandTestCase { command.domainLockUtils = new DomainLockUtils( new DeterministicStringGenerator(Alphabets.BASE_58), + "adminreg", AsyncTaskEnqueuerTest.createForTesting( mock(AppEngineServiceUtils.class), fakeClock, Duration.ZERO)); } @@ -58,7 +59,7 @@ public class LockDomainCommandTest extends CommandTestCase { @Test public void testSuccess_locksDomain() throws Exception { DomainBase domain = persistActiveDomain("example.tld"); - runCommandForced("--client=NewRegistrar", "example.tld"); + runCommandForced("--client=TheRegistrar", "example.tld"); assertThat(reloadResource(domain).getStatusValues()) .containsAtLeastElementsIn(REGISTRY_LOCK_STATUSES); } @@ -71,7 +72,7 @@ public class LockDomainCommandTest extends CommandTestCase { .asBuilder() .addStatusValue(SERVER_TRANSFER_PROHIBITED) .build()); - runCommandForced("--client=NewRegistrar", "example.tld"); + runCommandForced("--client=TheRegistrar", "example.tld"); assertThat(reloadResource(domain).getStatusValues()) .containsAtLeastElementsIn(REGISTRY_LOCK_STATUSES); } @@ -87,7 +88,7 @@ public class LockDomainCommandTest extends CommandTestCase { } runCommandForced( ImmutableList.builder() - .add("--client=NewRegistrar") + .add("--client=TheRegistrar") .addAll(domains.stream().map(DomainBase::getDomainName).collect(Collectors.toList())) .build()); for (DomainBase domain : domains) { diff --git a/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java b/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java index 9ce5aad4a..cfd8a43ad 100644 --- a/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java +++ b/core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java @@ -54,6 +54,7 @@ public class UnlockDomainCommandTest extends CommandTestCase domains = new ArrayList<>(); for (int n = 0; n < 26; n++) { String domain = String.format("domain%d.tld", n); - domains.add(persistLockedDomain(domain, "NewRegistrar")); + domains.add(persistLockedDomain(domain, "TheRegistrar")); } runCommandForced( ImmutableList.builder() - .add("--client=NewRegistrar") + .add("--client=TheRegistrar") .addAll(domains.stream().map(DomainBase::getDomainName).collect(Collectors.toList())) .build()); for (DomainBase domain : domains) { @@ -111,20 +112,20 @@ public class UnlockDomainCommandTest extends CommandTestCase runCommandForced("--client=NewRegistrar", "missing.tld")); + () -> runCommandForced("--client=TheRegistrar", "missing.tld")); assertThat(e).hasMessageThat().isEqualTo("Domain 'missing.tld' does not exist or is deleted"); } @Test public void testSuccess_alreadyUnlockedDomain_performsNoAction() throws Exception { DomainBase domain = persistActiveDomain("example.tld"); - runCommandForced("--client=NewRegistrar", "example.tld"); + runCommandForced("--client=TheRegistrar", "example.tld"); assertThat(reloadResource(domain)).isEqualTo(domain); } @Test public void testSuccess_defaultsToAdminRegistrar_ifUnspecified() throws Exception { - DomainBase domain = persistLockedDomain("example.tld", "NewRegistrar"); + DomainBase domain = persistLockedDomain("example.tld", "TheRegistrar"); runCommandForced("example.tld"); assertThat(getMostRecentRegistryLockByRepoId(domain.getRepoId()).get().getRegistrarId()) .isEqualTo("adminreg"); @@ -135,7 +136,7 @@ public class UnlockDomainCommandTest extends CommandTestCase runCommandForced("--client=NewRegistrar", "dupe.tld", "dupe.tld")); + () -> runCommandForced("--client=TheRegistrar", "dupe.tld", "dupe.tld")); assertThat(e).hasMessageThat().isEqualTo("Duplicate domain arguments found: 'dupe.tld'"); } } diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java index ce1cecbdb..7851d23f4 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockPostActionTest.java @@ -14,6 +14,7 @@ package google.registry.ui.server.registrar; +import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.testing.DatastoreHelper.createTld; @@ -33,7 +34,7 @@ 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 com.google.common.collect.ImmutableSet; import google.registry.batch.AsyncTaskEnqueuerTest; import google.registry.model.domain.DomainBase; import google.registry.request.JsonActionRunner; @@ -104,6 +105,7 @@ public final class RegistryLockPostActionTest { userWithoutPermission = userFromRegistrarContact(AppEngineRule.makeRegistrarContact2()); createTld("tld"); domain = persistResource(newDomainBase("example.tld")); + outgoingAddress = new InternetAddress("domain-registry@example.com"); when(mockRequest.getServerName()).thenReturn("registrarconsole.tld"); @@ -224,7 +226,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "isLock", true)); assertSuccess(response, "lock", "johndoe@theregistrar.com"); @@ -237,22 +239,45 @@ public final class RegistryLockPostActionTest { } @Test - public void testFailure_noClientId() { + public void testFailure_noRegistrarId() { Map response = action.handleJsonRequest(ImmutableMap.of()); - assertFailureWithMessage(response, "Missing key for client: clientId"); + assertFailureWithMessage(response, "Missing key for registrarId"); } @Test - public void testFailure_emptyClientId() { - Map response = action.handleJsonRequest(ImmutableMap.of("clientId", "")); - assertFailureWithMessage(response, "Missing key for client: clientId"); + public void testFailure_emptyRegistrarId() { + Map response = action.handleJsonRequest(ImmutableMap.of("registrarId", "")); + assertFailureWithMessage(response, "Missing key for registrarId"); + } + + @Test + public void testFailure_unauthorizedRegistrarId() { + AuthResult authResult = + AuthResult.create(AuthLevel.USER, UserAuthInfo.create(userWithLockPermission, false)); + action = createAction(authResult, ImmutableSet.of("TheRegistrar")); + Map response = + action.handleJsonRequest( + ImmutableMap.of( + "isLock", true, + "registrarId", "NewRegistrar", + "domainName", "example.tld", + "password", "hi")); + assertFailureWithMessage(response, "TestUserId doesn't have access to registrar NewRegistrar"); + } + + @Test + public void testFailure_incorrectRegistrarIdForDomain() { + persistResource(domain.asBuilder().setPersistedCurrentSponsorClientId("NewRegistrar").build()); + Map response = action.handleJsonRequest(lockRequest()); + assertFailureWithMessage( + response, "Domain example.tld is not owned by registrar TheRegistrar"); } @Test public void testFailure_noDomainName() { Map response = action.handleJsonRequest( - ImmutableMap.of("clientId", "TheRegistrar", "password", "hi", "isLock", true)); + ImmutableMap.of("registrarId", "TheRegistrar", "password", "hi", "isLock", true)); assertFailureWithMessage(response, "Missing key for domainName"); } @@ -261,7 +286,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "password", "hi")); assertFailureWithMessage(response, "Missing key for isLock"); @@ -280,7 +305,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "isLock", true)); assertFailureWithMessage(response, "Incorrect registry lock password for contact"); @@ -294,7 +319,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "isLock", true, "password", "hi")); @@ -306,7 +331,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "isLock", true, "password", "badPassword")); @@ -318,7 +343,7 @@ public final class RegistryLockPostActionTest { Map response = action.handleJsonRequest( ImmutableMap.of( - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "bad.tld", "isLock", true, "password", "hi")); @@ -381,7 +406,7 @@ public final class RegistryLockPostActionTest { private ImmutableMap fullRequest(boolean lock) { return ImmutableMap.of( "isLock", lock, - "clientId", "TheRegistrar", + "registrarId", "TheRegistrar", "domainName", "example.tld", "password", "hi"); } @@ -425,15 +450,21 @@ public final class RegistryLockPostActionTest { } private RegistryLockPostAction createAction(AuthResult authResult) { + return createAction(authResult, ImmutableSet.of("TheRegistrar", "NewRegistrar")); + } + + private RegistryLockPostAction createAction( + AuthResult authResult, ImmutableSet accessibleRegistrars) { Role role = authResult.userAuthInfo().get().isUserAdmin() ? Role.ADMIN : Role.OWNER; AuthenticatedRegistrarAccessor registrarAccessor = AuthenticatedRegistrarAccessor.createForTesting( - ImmutableSetMultimap.of("TheRegistrar", role, "NewRegistrar", role)); + accessibleRegistrars.stream().collect(toImmutableSetMultimap(r -> r, r -> role))); JsonActionRunner jsonActionRunner = new JsonActionRunner(ImmutableMap.of(), new JsonResponse(new ResponseImpl(mockResponse))); DomainLockUtils domainLockUtils = new DomainLockUtils( new DeterministicStringGenerator(Alphabets.BASE_58), + "adminreg", AsyncTaskEnqueuerTest.createForTesting( mock(AppEngineServiceUtils.class), clock, Duration.ZERO)); return new RegistryLockPostAction( diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockVerifyActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockVerifyActionTest.java index b300d96fa..aab6a320b 100644 --- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockVerifyActionTest.java +++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockVerifyActionTest.java @@ -332,6 +332,7 @@ public final class RegistryLockVerifyActionTest { new RegistryLockVerifyAction( new DomainLockUtils( stringGenerator, + "adminreg", AsyncTaskEnqueuerTest.createForTesting( mock(AppEngineServiceUtils.class), fakeClock, Duration.ZERO)), lockVerificationCode,