Improve the uniform_rapid_suspension command (#739)

- Reuse DS record format processing from the create/update domain commands
  (BIND format, commonly used in URS requests)
- Remove the CLIENT_HOLD status from domains that have it (this blocks us from
  serving the new nameservers and DS record)

core/src/main/java/google/registry/tools/CreateOrUpdateDomainCommand.java

@@ -15,20 +15,11 @@
 package google.registry.tools;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static google.registry.util.CollectionUtils.findDuplicates;
 
-import com.beust.jcommander.IStringConverter;
 import com.beust.jcommander.Parameter;
-import com.google.auto.value.AutoValue;
-import com.google.common.base.Ascii;
-import com.google.common.base.CharMatcher;
 import com.google.common.base.Joiner;
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.io.BaseEncoding;
-import com.google.template.soy.data.SoyListData;
-import com.google.template.soy.data.SoyMapData;
 import google.registry.tools.params.NameserversParameter;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -80,77 +71,15 @@ abstract class CreateOrUpdateDomainCommand extends MutatingEppToolCommand {
   String password;
 
   @Parameter(
-    names = "--ds_records",
+      names = "--ds_records",
-    description =
+      description =
-        "Comma-separated list of DS records. Each DS record is given as "
+          "Comma-separated list of DS records. Each DS record is given as "
-        + "<keyTag> <alg> <digestType> <digest>, in order, as it appears in the Zonefile.",
+              + "<keyTag> <alg> <digestType> <digest>, in order, as it appears in the Zonefile.",
-    converter = DsRecordConverter.class
+      converter = DsRecord.Converter.class)
-  )
   List<DsRecord> dsRecords = new ArrayList<>();
 
   Set<String> domains;
 
-  @AutoValue
-  abstract static class DsRecord {
-    private static final Splitter SPLITTER =
-        Splitter.on(CharMatcher.whitespace()).omitEmptyStrings();
-
-    public abstract int keyTag();
-    public abstract int alg();
-    public abstract int digestType();
-    public abstract String digest();
-
-    private static DsRecord create(int keyTag, int alg, int digestType, String digest) {
-      digest = Ascii.toUpperCase(digest);
-      checkArgument(
-          BaseEncoding.base16().canDecode(digest),
-          "digest should be even-lengthed hex, but is %s (length %s)",
-          digest,
-          digest.length());
-      return new AutoValue_CreateOrUpdateDomainCommand_DsRecord(keyTag, alg, digestType, digest);
-    }
-
-    /**
-     * Parses a string representation of the DS record.
-     *
-     * <p>The string format accepted is "[keyTag] [alg] [digestType] [digest]" (i.e., the 4
-     * arguments separated by any number of spaces, as it appears in the Zone file)
-     */
-    public static DsRecord parse(String dsRecord) {
-      List<String> elements = SPLITTER.splitToList(dsRecord);
-      checkArgument(
-          elements.size() == 4,
-          "dsRecord %s should have 4 parts, but has %s",
-          dsRecord,
-          elements.size());
-      return DsRecord.create(
-          Integer.parseUnsignedInt(elements.get(0)),
-          Integer.parseUnsignedInt(elements.get(1)),
-          Integer.parseUnsignedInt(elements.get(2)),
-          elements.get(3));
-    }
-
-    public SoyMapData toSoyData() {
-      return new SoyMapData(
-          "keyTag", keyTag(),
-          "alg", alg(),
-          "digestType", digestType(),
-          "digest", digest());
-    }
-
-    public static SoyListData convertToSoy(List<DsRecord> dsRecords) {
-      return new SoyListData(
-          dsRecords.stream().map(DsRecord::toSoyData).collect(toImmutableList()));
-    }
-  }
-
-  public static class DsRecordConverter implements IStringConverter<DsRecord> {
-    @Override
-    public DsRecord convert(String dsRecord) {
-      return DsRecord.parse(dsRecord);
-    }
-  }
-
   @Override
   protected void initEppToolCommand() throws Exception {
     checkArgument(nameservers.size() <= 13, "There can be at most 13 nameservers.");

core/src/main/java/google/registry/tools/DsRecord.java

@@ -0,0 +1,90 @@
+// Copyright 2017 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.tools;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.beust.jcommander.IStringConverter;
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Ascii;
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Splitter;
+import com.google.common.io.BaseEncoding;
+import com.google.template.soy.data.SoyListData;
+import com.google.template.soy.data.SoyMapData;
+import java.util.List;
+
+@AutoValue
+abstract class DsRecord {
+  private static final Splitter SPLITTER = Splitter.on(CharMatcher.whitespace()).omitEmptyStrings();
+
+  public abstract int keyTag();
+
+  public abstract int alg();
+
+  public abstract int digestType();
+
+  public abstract String digest();
+
+  private static DsRecord create(int keyTag, int alg, int digestType, String digest) {
+    digest = Ascii.toUpperCase(digest);
+    checkArgument(
+        BaseEncoding.base16().canDecode(digest),
+        "digest should be even-lengthed hex, but is %s (length %s)",
+        digest,
+        digest.length());
+    return new AutoValue_DsRecord(keyTag, alg, digestType, digest);
+  }
+
+  /**
+   * Parses a string representation of the DS record.
+   *
+   * <p>The string format accepted is "[keyTag] [alg] [digestType] [digest]" (i.e., the 4 arguments
+   * separated by any number of spaces, as it appears in the Zone file)
+   */
+  public static DsRecord parse(String dsRecord) {
+    List<String> elements = SPLITTER.splitToList(dsRecord);
+    checkArgument(
+        elements.size() == 4,
+        "dsRecord %s should have 4 parts, but has %s",
+        dsRecord,
+        elements.size());
+    return DsRecord.create(
+        Integer.parseUnsignedInt(elements.get(0)),
+        Integer.parseUnsignedInt(elements.get(1)),
+        Integer.parseUnsignedInt(elements.get(2)),
+        elements.get(3));
+  }
+
+  public SoyMapData toSoyData() {
+    return new SoyMapData(
+        "keyTag", keyTag(),
+        "alg", alg(),
+        "digestType", digestType(),
+        "digest", digest());
+  }
+
+  public static SoyListData convertToSoy(List<DsRecord> dsRecords) {
+    return new SoyListData(dsRecords.stream().map(DsRecord::toSoyData).collect(toImmutableList()));
+  }
+
+  public static class Converter implements IStringConverter<DsRecord> {
+    @Override
+    public DsRecord convert(String dsRecord) {
+      return DsRecord.parse(dsRecord);
+    }
+  }
+}

core/src/main/java/google/registry/tools/UniformRapidSuspensionCommand.java

@@ -15,7 +15,7 @@
 package google.registry.tools;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Strings.nullToEmpty;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.Sets.difference;
 import static google.registry.model.EppResourceUtils.checkResourcesExist;
 import static google.registry.model.EppResourceUtils.loadByForeignKey;
@@ -26,9 +26,11 @@ import static org.joda.time.DateTimeZone.UTC;
 import com.beust.jcommander.Parameter;
 import com.beust.jcommander.Parameters;
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.template.soy.data.SoyListData;
 import com.google.template.soy.data.SoyMapData;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.domain.secdns.DelegationSignerData;
@@ -37,14 +39,10 @@ import google.registry.model.host.HostResource;
 import google.registry.tools.soy.UniformRapidSuspensionSoyInfo;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import javax.xml.bind.annotation.adapters.HexBinaryAdapter;
 import org.joda.time.DateTime;
-import org.json.simple.JSONArray;
-import org.json.simple.JSONValue;
-import org.json.simple.parser.ParseException;
 
 /** A command to suspend a domain for the Uniform Rapid Suspension process. */
 @Parameters(separators = " =",
@@ -59,9 +57,6 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
   /** Client id that made this change. Only recorded in the history entry. **/
   private static final String CLIENT_ID = "CharlestonRoad";
 
-  private static final ImmutableSet<String> DSDATA_FIELDS =
-      ImmutableSet.of("keyTag", "alg", "digestType", "digest");
-
   @Parameter(
       names = {"-n", "--domain_name"},
       description = "Domain to suspend.",
@@ -76,10 +71,12 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
 
   @Parameter(
       names = {"-s", "--dsdata"},
-      description = "Comma-delimited set of dsdata to replace the current dsdata on the domain, "
+      description =
-          + "where each dsdata is represented as a JSON object with fields 'keyTag', 'alg', "
+          "Comma-delimited set of dsdata to replace the current dsdata on the domain, "
-          + "'digestType' and 'digest'.")
+              + "Each DS record is given as <keyTag> <alg> <digestType> <digest>, in order, as it "
-  private String newDsData;
+              + "appears in the Zonefile.",
+      converter = DsRecord.Converter.class)
+  private List<DsRecord> newDsData;
 
   @Parameter(
       names = {"-p", "--locks_to_preserve"},
@@ -88,6 +85,13 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
               + "locks: serverDeleteProhibited, serverTransferProhibited, serverUpdateProhibited")
   private List<String> locksToPreserve = new ArrayList<>();
 
+  @Parameter(
+      names = {"--restore_client_hold"},
+      description =
+          "Restores a CLIENT_HOLD status that was previously removed for a URS suspension (only "
+              + "valid with --undo).")
+  private boolean restoreClientHold;
+
   @Parameter(
       names = {"--undo"},
       description = "Flag indicating that is is an undo command, which removes locks.")
@@ -100,28 +104,16 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
   ImmutableSortedSet<String> existingNameservers;
 
   /** Set of existing dsdata jsons that need to be restored during undo, sorted for nicer output. */
-  ImmutableSortedSet<String> existingDsData;
+  ImmutableList<ImmutableMap<String, Object>> existingDsData;
+
+  /** Set of status values to remove. */
+  ImmutableSet<String> removeStatuses;
 
   @Override
   protected void initMutatingEppToolCommand() {
     superuser = true;
     DateTime now = DateTime.now(UTC);
     ImmutableSet<String> newHostsSet = ImmutableSet.copyOf(newHosts);
-    ImmutableSet.Builder<Map<String, Object>> newDsDataBuilder = new ImmutableSet.Builder<>();
-    try {
-      // Add brackets around newDsData to convert it to a parsable JSON array.
-      String jsonArrayString = String.format("[%s]", nullToEmpty(newDsData));
-      for (Object dsData : (JSONArray) JSONValue.parseWithException(jsonArrayString)) {
-        @SuppressWarnings("unchecked")
-        Map<String, Object> dsDataJson = (Map<String, Object>) dsData;
-        checkArgument(
-            dsDataJson.keySet().equals(DSDATA_FIELDS),
-            "Incorrect fields on --dsdata JSON: " + JSONValue.toJSONString(dsDataJson));
-        newDsDataBuilder.add(dsDataJson);
-      }
-    } catch (ClassCastException | ParseException e) {
-      throw new IllegalArgumentException("Invalid --dsdata JSON", e);
-    }
     Optional<DomainBase> domain = loadByForeignKey(DomainBase.class, domainName, now);
     checkArgumentPresent(domain, "Domain '%s' does not exist or is deleted", domainName);
     Set<String> missingHosts =
@@ -133,18 +125,39 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     existingNameservers = getExistingNameservers(domain.get());
     existingLocks = getExistingLocks(domain.get());
     existingDsData = getExistingDsData(domain.get());
+    removeStatuses =
+        (hasClientHold(domain.get()) && !undo)
+            ? ImmutableSet.of(StatusValue.CLIENT_HOLD.getXmlName())
+            : ImmutableSet.of();
+    ImmutableSet<String> statusesToApply;
+    if (undo) {
+      statusesToApply =
+          restoreClientHold
+              ? ImmutableSet.of(StatusValue.CLIENT_HOLD.getXmlName())
+              : ImmutableSet.of();
+    } else {
+      statusesToApply = URS_LOCKS;
+    }
     setSoyTemplate(
         UniformRapidSuspensionSoyInfo.getInstance(),
         UniformRapidSuspensionSoyInfo.UNIFORMRAPIDSUSPENSION);
-    addSoyRecord(CLIENT_ID, new SoyMapData(
+    addSoyRecord(
-        "domainName", domainName,
+        CLIENT_ID,
-        "hostsToAdd", difference(newHostsSet, existingNameservers),
+        new SoyMapData(
-        "hostsToRemove", difference(existingNameservers, newHostsSet),
+            "domainName",
-        "locksToApply", undo ? ImmutableSet.of() : URS_LOCKS,
+            domainName,
-        "locksToRemove",
+            "hostsToAdd",
-            undo ? difference(URS_LOCKS, ImmutableSet.copyOf(locksToPreserve)) : ImmutableSet.of(),
+            difference(newHostsSet, existingNameservers),
-        "newDsData", newDsDataBuilder.build(),
+            "hostsToRemove",
-        "reason", (undo ? "Undo " : "") + "Uniform Rapid Suspension"));
+            difference(existingNameservers, newHostsSet),
+            "statusesToApply",
+            statusesToApply,
+            "statusesToRemove",
+            undo ? difference(URS_LOCKS, ImmutableSet.copyOf(locksToPreserve)) : removeStatuses,
+            "newDsData",
+            newDsData != null ? DsRecord.convertToSoy(newDsData) : new SoyListData(),
+            "reason",
+            (undo ? "Undo " : "") + "Uniform Rapid Suspension"));
   }
 
   private ImmutableSortedSet<String> getExistingNameservers(DomainBase domain) {
@@ -165,15 +178,25 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     return locks.build();
   }
 
-  private ImmutableSortedSet<String> getExistingDsData(DomainBase domain) {
+  private boolean hasClientHold(DomainBase domain) {
-    ImmutableSortedSet.Builder<String> dsDataJsons = ImmutableSortedSet.naturalOrder();
+    for (StatusValue status : domain.getStatusValues()) {
+      if (status == StatusValue.CLIENT_HOLD) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private ImmutableList<ImmutableMap<String, Object>> getExistingDsData(DomainBase domain) {
+    ImmutableList.Builder<ImmutableMap<String, Object>> dsDataJsons = new ImmutableList.Builder();
     HexBinaryAdapter hexBinaryAdapter = new HexBinaryAdapter();
     for (DelegationSignerData dsData : domain.getDsData()) {
-      dsDataJsons.add(JSONValue.toJSONString(ImmutableMap.of(
+      dsDataJsons.add(
-          "keyTag", dsData.getKeyTag(),
+          ImmutableMap.of(
-          "algorithm", dsData.getAlgorithm(),
+              "keyTag", dsData.getKeyTag(),
-          "digestType", dsData.getDigestType(),
+              "algorithm", dsData.getAlgorithm(),
-          "digest", hexBinaryAdapter.marshal(dsData.getDigest()))));
+              "digestType", dsData.getDigestType(),
+              "digest", hexBinaryAdapter.marshal(dsData.getDigest())));
     }
     return dsDataJsons.build();
   }
@@ -194,8 +217,23 @@ final class UniformRapidSuspensionCommand extends MutatingEppToolCommand {
     if (!existingLocks.isEmpty()) {
       undoBuilder.append(" --locks_to_preserve ").append(Joiner.on(',').join(existingLocks));
     }
+    if (removeStatuses.contains(StatusValue.CLIENT_HOLD.getXmlName())) {
+      undoBuilder.append(" --restore_client_hold");
+    }
     if (!existingDsData.isEmpty()) {
-      undoBuilder.append(" --dsdata ").append(Joiner.on(',').join(existingDsData));
+      ImmutableList<String> formattedDsRecords =
+          existingDsData.stream()
+              .map(
+                  rec ->
+                      String.format(
+                          "%s %s %s %s",
+                          rec.get("keyTag"),
+                          rec.get("algorithm"),
+                          rec.get("digestType"),
+                          rec.get("digest")))
+              .sorted()
+              .collect(toImmutableList());
+      undoBuilder.append(" --dsdata ").append(Joiner.on(',').join(formattedDsRecords));
     }
     return undoBuilder.toString();
   }

core/src/main/java/google/registry/tools/UpdateDomainCommand.java

@@ -76,10 +76,10 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   private List<String> addStatuses = new ArrayList<>();
 
   @Parameter(
-    names = "--add_ds_records",
+      names = "--add_ds_records",
-    description = "DS records to add. Cannot be set if --ds_records or --clear_ds_records is set.",
+      description =
-    converter = DsRecordConverter.class
+          "DS records to add. Cannot be set if --ds_records or --clear_ds_records is set.",
-  )
+      converter = DsRecord.Converter.class)
   private List<DsRecord> addDsRecords = new ArrayList<>();
 
   @Parameter(
@@ -110,11 +110,10 @@ final class UpdateDomainCommand extends CreateOrUpdateDomainCommand {
   private List<String> removeStatuses = new ArrayList<>();
 
   @Parameter(
-    names = "--remove_ds_records",
+      names = "--remove_ds_records",
-    description =
+      description =
-        "DS records to remove. Cannot be set if --ds_records or --clear_ds_records is set.",
+          "DS records to remove. Cannot be set if --ds_records or --clear_ds_records is set.",
-    converter = DsRecordConverter.class
+      converter = DsRecord.Converter.class)
-  )
   private List<DsRecord> removeDsRecords = new ArrayList<>();
 
   @Parameter(

core/src/main/resources/google/registry/tools/soy/UniformRapidSuspension.soy

@@ -21,8 +21,8 @@
 {@param domainName: string}
 {@param hostsToAdd: list<string>}
 {@param hostsToRemove: list<string>}
-{@param locksToApply: list<string>}
+{@param statusesToApply: list<string>}
-{@param locksToRemove: list<string>}
+{@param statusesToRemove: list<string>}
 {@param newDsData: list<[keyTag:int, alg:int, digestType:int, digest:string]>}
 {@param reason: string}
 <?xml version="1.0" encoding="UTF-8" standalone="yes"?>
@@ -39,7 +39,7 @@
           {/for}
           </domain:ns>
         {/if}
-        {for $la in $locksToApply}
+        {for $la in $statusesToApply}
           <domain:status s="{$la}" />
         {/for}
         </domain:add>
@@ -51,7 +51,7 @@
           {/for}
           </domain:ns>
         {/if}
-        {for $lr in $locksToRemove}
+        {for $lr in $statusesToRemove}
           <domain:status s="{$lr}" />
         {/for}
         </domain:rem>

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

@@ -71,7 +71,7 @@ class UniformRapidSuspensionCommandTest
     runCommandForced(
         "--domain_name=evil.tld",
         "--hosts=urs1.example.com,urs2.example.com",
-        "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1,\"digest\":\"abc\"}");
+        "--dsdata=1 1 1 abcd");
     eppVerifier
         .expectClientId("CharlestonRoad")
         .expectSuperuser()
@@ -79,10 +79,9 @@ class UniformRapidSuspensionCommandTest
     assertInStdout("uniform_rapid_suspension --undo");
     assertInStdout("--domain_name evil.tld");
     assertInStdout("--hosts ns1.example.com,ns2.example.com");
-    assertInStdout("--dsdata "
+    assertInStdout("--dsdata 1 2 3 DEAD,4 5 6 BEEF");
-        + "{\"keyTag\":1,\"algorithm\":2,\"digestType\":3,\"digest\":\"DEAD\"},"
-        + "{\"keyTag\":4,\"algorithm\":5,\"digestType\":6,\"digest\":\"BEEF\"}");
     assertNotInStdout("--locks_to_preserve");
+    assertNotInStdout("--restore_client_hold");
   }
 
   @Test
@@ -122,6 +121,29 @@ class UniformRapidSuspensionCommandTest
     assertInStdout("--locks_to_preserve serverDeleteProhibited");
   }
 
+  @Test
+  void testCommand_removeClientHold() throws Exception {
+    persistResource(
+        newDomainBase("evil.tld")
+            .asBuilder()
+            .addStatusValue(StatusValue.CLIENT_HOLD)
+            .addNameserver(ns1.createVKey())
+            .addNameserver(ns2.createVKey())
+            .build());
+    runCommandForced(
+        "--domain_name=evil.tld",
+        "--hosts=urs1.example.com,urs2.example.com",
+        "--dsdata=1 1 1 abcd");
+    eppVerifier
+        .expectClientId("CharlestonRoad")
+        .expectSuperuser()
+        .verifySent("uniform_rapid_suspension_with_client_hold.xml");
+    assertInStdout("uniform_rapid_suspension --undo");
+    assertInStdout("--domain_name evil.tld");
+    assertInStdout("--hosts ns1.example.com,ns2.example.com");
+    assertInStdout("--restore_client_hold");
+  }
+
   @Test
   void testUndo_removesLocksReplacesHostsAndDsData() throws Exception {
     persistDomainWithHosts(urs1, urs2);
@@ -149,6 +171,21 @@ class UniformRapidSuspensionCommandTest
     assertNotInStdout("--undo");  // Undo shouldn't print a new undo command.
   }
 
+  @Test
+  void testUndo_restoresClientHolds() throws Exception {
+    persistDomainWithHosts(urs1, urs2);
+    runCommandForced(
+        "--domain_name=evil.tld",
+        "--undo",
+        "--hosts=ns1.example.com,ns2.example.com",
+        "--restore_client_hold");
+    eppVerifier
+        .expectClientId("CharlestonRoad")
+        .expectSuperuser()
+        .verifySent("uniform_rapid_suspension_undo_client_hold.xml");
+    assertNotInStdout("--undo"); // Undo shouldn't print a new undo command.
+  }
+
   @Test
   void testFailure_locksToPreserveWithoutUndo() {
     persistActiveDomain("evil.tld");
@@ -177,11 +214,10 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () ->
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1 1 1 abc 1"));
-                runCommandForced(
+    assertThat(thrown)
-                    "--domain_name=evil.tld",
+        .hasMessageThat()
-                    "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1,\"digest\":\"abc\",\"foo\":1}"));
+        .contains("dsRecord 1 1 1 abc 1 should have 4 parts, but has 5");
-    assertThat(thrown).hasMessageThat().contains("Incorrect fields on --dsdata JSON");
   }
 
   @Test
@@ -190,11 +226,8 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () ->
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1 1 1"));
-                runCommandForced(
+    assertThat(thrown).hasMessageThat().contains("dsRecord 1 1 1 should have 4 parts, but has 3");
-                    "--domain_name=evil.tld",
-                    "--dsdata={\"keyTag\":1,\"alg\":1,\"digestType\":1}"));
-    assertThat(thrown).hasMessageThat().contains("Incorrect fields on --dsdata JSON");
   }
 
   @Test
@@ -203,7 +236,7 @@ class UniformRapidSuspensionCommandTest
     IllegalArgumentException thrown =
         assertThrows(
             IllegalArgumentException.class,
-            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=[1,2,3]"));
+            () -> runCommandForced("--domain_name=evil.tld", "--dsdata=1,2,3"));
-    assertThat(thrown).hasMessageThat().contains("Invalid --dsdata JSON");
+    assertThat(thrown).hasMessageThat().contains("dsRecord 1 should have 4 parts, but has 1");
   }
 }

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension.xml

@@ -31,7 +31,7 @@
             <secDNS:keyTag>1</secDNS:keyTag>
             <secDNS:alg>1</secDNS:alg>
             <secDNS:digestType>1</secDNS:digestType>
-            <secDNS:digest>abc</secDNS:digest>
+            <secDNS:digest>ABCD</secDNS:digest>
           </secDNS:dsData>
         </secDNS:add>
       </secDNS:update>

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension_undo_client_hold.xml

@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>evil.tld</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>ns1.example.com</domain:hostObj>
+            <domain:hostObj>ns2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="clientHold" />
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>urs1.example.com</domain:hostObj>
+            <domain:hostObj>urs2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="serverDeleteProhibited" />
+          <domain:status s="serverTransferProhibited" />
+          <domain:status s="serverUpdateProhibited" />
+        </domain:rem>
+      </domain:update>
+    </update>
+    <extension>
+      <secDNS:update xmlns:secDNS="urn:ietf:params:xml:ns:secDNS-1.1">
+        <secDNS:rem>
+          <secDNS:all>true</secDNS:all>
+        </secDNS:rem>
+      </secDNS:update>
+      <metadata:metadata xmlns:metadata="urn:google:params:xml:ns:metadata-1.0">
+        <metadata:reason>Undo Uniform Rapid Suspension</metadata:reason>
+        <metadata:requestedByRegistrar>false</metadata:requestedByRegistrar>
+      </metadata:metadata>
+    </extension>
+    <clTRID>RegistryTool</clTRID>
+  </command>
+</epp>

core/src/test/resources/google/registry/tools/server/uniform_rapid_suspension_with_client_hold.xml

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
+  <command>
+    <update>
+      <domain:update xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
+        <domain:name>evil.tld</domain:name>
+        <domain:add>
+          <domain:ns>
+            <domain:hostObj>urs1.example.com</domain:hostObj>
+            <domain:hostObj>urs2.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="serverDeleteProhibited"/>
+          <domain:status s="serverTransferProhibited"/>
+          <domain:status s="serverUpdateProhibited"/>
+        </domain:add>
+        <domain:rem>
+          <domain:ns>
+            <domain:hostObj>ns2.example.com</domain:hostObj>
+            <domain:hostObj>ns1.example.com</domain:hostObj>
+          </domain:ns>
+          <domain:status s="clientHold"/>
+        </domain:rem>
+      </domain:update>
+    </update>
+    <extension>
+      <secDNS:update xmlns:secDNS="urn:ietf:params:xml:ns:secDNS-1.1">
+        <secDNS:rem>
+          <secDNS:all>true</secDNS:all>
+        </secDNS:rem>
+        <secDNS:add>
+          <secDNS:dsData>
+            <secDNS:keyTag>1</secDNS:keyTag>
+            <secDNS:alg>1</secDNS:alg>
+            <secDNS:digestType>1</secDNS:digestType>
+            <secDNS:digest>ABCD</secDNS:digest>
+          </secDNS:dsData>
+        </secDNS:add>
+      </secDNS:update>
+      <metadata:metadata xmlns:metadata="urn:google:params:xml:ns:metadata-1.0">
+        <metadata:reason>Uniform Rapid Suspension</metadata:reason>
+        <metadata:requestedByRegistrar>false</metadata:requestedByRegistrar>
+      </metadata:metadata>
+    </extension>
+    <clTRID>RegistryTool</clTRID>
+  </command>
+</epp>