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

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<Duration> relockDuration) {
     DateTime now = jpaTm().getTransactionTime();
-    DomainBase domainBase = getDomain(domainName, now);
+    DomainBase domainBase = getDomain(domainName, registrarId, now);
     Optional<RegistryLock> lockOptional =
         RegistryLockDao.getMostRecentVerifiedLockByRepoId(domainBase.getRepoId());
 
@@ -303,10 +307,19 @@ public final class DomainLockUtils {
         domainBase.getDomainName());
   }
 
-  private static DomainBase getDomain(String domainName, DateTime now) {
+  private DomainBase getDomain(String domainName, String registrarId, DateTime now) {
-    return loadByForeignKeyCached(DomainBase.class, domainName, now)
+    DomainBase domain =
-        .orElseThrow(
+        loadByForeignKeyCached(DomainBase.class, domainName, now)
-            () -> new IllegalArgumentException(String.format("Unknown domain %s", domainName)));
+            .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) {
+  private void applyLockStatuses(RegistryLock lock, DateTime lockTime) {
-    DomainBase domain = getDomain(lock.getDomainName(), 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) {
+  private void removeLockStatuses(RegistryLock lock, boolean isAdmin, DateTime unlockTime) {
-    DomainBase domain = getDomain(lock.getDomainName(), unlockTime);
+    DomainBase domain = getDomain(lock.getDomainName(), lock.getRegistrarId(), unlockTime);
     if (!isAdmin) {
       verifyDomainLocked(domain);
     }

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(
+      String registrarId = postInput.registrarId;
-          !Strings.isNullOrEmpty(postInput.clientId),
+      checkArgument(!Strings.isNullOrEmpty(registrarId), "Missing key for registrarId");
-          "Missing key for client: %s",
-          PARAM_CLIENT_ID);
       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;

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,

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));
 

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)));
 

core/src/test/java/google/registry/tools/LockDomainCommandTest.java

@@ -51,6 +51,7 @@ public class LockDomainCommandTest extends CommandTestCase<LockDomainCommand> {
     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<LockDomainCommand> {
   @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<LockDomainCommand> {
                 .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<LockDomainCommand> {
     }
     runCommandForced(
         ImmutableList.<String>builder()
-            .add("--client=NewRegistrar")
+            .add("--client=TheRegistrar")
             .addAll(domains.stream().map(DomainBase::getDomainName).collect(Collectors.toList()))
             .build());
     for (DomainBase domain : domains) {

core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java

@@ -54,6 +54,7 @@ public class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand
     command.domainLockUtils =
         new DomainLockUtils(
             new DeterministicStringGenerator(Alphabets.BASE_58),
+            "adminreg",
             AsyncTaskEnqueuerTest.createForTesting(
                 mock(AppEngineServiceUtils.class), fakeClock, Duration.ZERO));
   }
@@ -68,14 +69,14 @@ public class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand
 
   @Test
   public void testSuccess_unlocksDomain() throws Exception {
-    DomainBase domain = persistLockedDomain("example.tld", "NewRegistrar");
+    DomainBase domain = persistLockedDomain("example.tld", "TheRegistrar");
-    runCommandForced("--client=NewRegistrar", "example.tld");
+    runCommandForced("--client=TheRegistrar", "example.tld");
     assertThat(reloadResource(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES);
   }
 
   @Test
   public void testSuccess_partiallyUpdatesStatuses() throws Exception {
-    DomainBase domain = persistLockedDomain("example.tld", "NewRegistrar");
+    DomainBase domain = persistLockedDomain("example.tld", "TheRegistrar");
     domain =
         persistResource(
             domain
@@ -83,7 +84,7 @@ public class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand
                 .setStatusValues(
                     ImmutableSet.of(SERVER_DELETE_PROHIBITED, SERVER_UPDATE_PROHIBITED))
                 .build());
-    runCommandForced("--client=NewRegistrar", "example.tld");
+    runCommandForced("--client=TheRegistrar", "example.tld");
     assertThat(reloadResource(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES);
   }
 
@@ -94,11 +95,11 @@ public class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand
     List<DomainBase> 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.<String>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<UnlockDomainCommand
     IllegalArgumentException e =
         assertThrows(
             IllegalArgumentException.class,
-            () -> 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<UnlockDomainCommand
     IllegalArgumentException e =
         assertThrows(
             IllegalArgumentException.class,
-            () -> runCommandForced("--client=NewRegistrar", "dupe.tld", "dupe.tld"));
+            () -> runCommandForced("--client=TheRegistrar", "dupe.tld", "dupe.tld"));
     assertThat(e).hasMessageThat().isEqualTo("Duplicate domain arguments found: 'dupe.tld'");
   }
 }

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<String, ?> 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<String, ?> response = action.handleJsonRequest(ImmutableMap.of());
-    assertFailureWithMessage(response, "Missing key for client: clientId");
+    assertFailureWithMessage(response, "Missing key for registrarId");
   }
 
   @Test
-  public void testFailure_emptyClientId() {
+  public void testFailure_emptyRegistrarId() {
-    Map<String, ?> response = action.handleJsonRequest(ImmutableMap.of("clientId", ""));
+    Map<String, ?> response = action.handleJsonRequest(ImmutableMap.of("registrarId", ""));
-    assertFailureWithMessage(response, "Missing key for client: clientId");
+    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<String, ?> 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<String, ?> response = action.handleJsonRequest(lockRequest());
+    assertFailureWithMessage(
+        response, "Domain example.tld is not owned by registrar TheRegistrar");
   }
 
   @Test
   public void testFailure_noDomainName() {
     Map<String, ?> 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<String, ?> 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<String, ?> 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<String, ?> 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<String, ?> 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<String, ?> 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<String, Object> 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<String> 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(

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,