Add small perms (etc) fixes to actions (#3138)

- nicer error in Registrar builder instead of an NPE
- nicer error if hosts were deleted in GenerateZoneFilesAction
- extra permissions checks in ConsoleUsersAction
- using "replace" (text) instead of "replaceAll" (regex) in
  BulkDomainAction

core/src/main/java/google/registry/model/registrar/Registrar.java

@@ -871,7 +871,10 @@ public class Registrar extends UpdateAutoTimestampEntity implements Buildable, J
     }
 
     public Builder setIpAddressAllowList(Iterable<CidrAddressBlock> ipAddressAllowList) {
-      getInstance().ipAddressAllowList = ImmutableList.copyOf(ipAddressAllowList);
+      getInstance().ipAddressAllowList =
+          ipAddressAllowList == null
+              ? ImmutableList.of()
+              : ImmutableList.copyOf(ipAddressAllowList);
       return this;
     }
 

core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java

@@ -235,15 +235,20 @@ public class GenerateZoneFilesAction implements Runnable, JsonActionRunner.JsonA
     String domainLabel = stripTld(domain.getDomainName(), domain.getTld());
     Tld tld = Tld.get(domain.getTld());
     for (Host nameserver : tm().loadByKeys(domain.getNameservers()).values()) {
+      // Load the nameservers at the export time in case they've been renamed or deleted
+      Host host = loadAtPointInTime(nameserver, exportTime);
+      if (host == null) {
+        log.atSevere().log(
+            "Domain %s contained nameserver %s that didn't exist at time %s",
+            domain.getRepoId(), nameserver.getRepoId(), exportTime);
+        continue;
+      }
       result.append(
           String.format(
               NS_FORMAT,
               domainLabel,
-              tld.getDnsNsTtl()
-                  .orElse(dnsDefaultNsTtl)
-                  .toSeconds(),
-              // Load the nameservers at the export time in case they've been renamed or deleted.
-              loadAtPointInTime(nameserver, exportTime).getHostName()));
+              tld.getDnsNsTtl().orElse(dnsDefaultNsTtl).toSeconds(),
+              host.getHostName()));
     }
     for (DomainDsData dsData : domain.getDsData()) {
       result.append(

core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java

@@ -102,7 +102,7 @@ public class ConsoleUsersAction extends ConsoleApiAction {
   @Override
   protected void postHandler(User user) {
     checkPermission(user, registrarId, ConsolePermission.MANAGE_USERS);
-    tm().transact(this::runPostInTransaction);
+    tm().transact(() -> runPostInTransaction(user));
   }
 
   @Override
@@ -135,16 +135,16 @@ public class ConsoleUsersAction extends ConsoleApiAction {
     tm().transact(this::runDeleteInTransaction);
   }
 
-  private void runPostInTransaction() throws IOException {
+  private void runPostInTransaction(User callingUser) throws IOException {
     validateRequestParams();
     if (!tm().exists(VKey.create(User.class, this.userData.get().emailAddress))) {
       this.runCreate();
     } else {
-      this.runAppendUserToExistingRegistrar();
+      this.runAppendUserToExistingRegistrar(callingUser);
     }
   }
 
-  private void runAppendUserToExistingRegistrar() {
+  private void runAppendUserToExistingRegistrar(User callingUser) {
     ImmutableList<User> allRegistrarUsers = getAllRegistrarUsers(registrarId);
     if (allRegistrarUsers.size() >= 4) {
       throw new BadRequestException("Total users amount per registrar is limited to 4");
@@ -156,6 +156,9 @@ public class ConsoleUsersAction extends ConsoleApiAction {
       throw new BadRequestException(
           "Cannot append a global administrator or user with a global role to a registrar");
     }
+    for (String existingRegistrarId : userToAppend.getUserRoles().getRegistrarRoles().keySet()) {
+      checkPermission(callingUser, existingRegistrarId, ConsolePermission.MANAGE_USERS);
+    }
 
     updateUserRegistrarRoles(
         userToAppend, registrarId, requestRoleToAllowedRoles(this.userData.get().role));
@@ -263,10 +266,15 @@ public class ConsoleUsersAction extends ConsoleApiAction {
       return;
     }
 
+    User userToUpdate = verifyUserExists(this.userData.get().emailAddress);
+    if (userToUpdate.getUserRoles().isAdmin()
+        || !userToUpdate.getUserRoles().getGlobalRole().equals(GlobalRole.NONE)) {
+      throw new BadRequestException(
+          "Cannot update a global administrator or user with a global role");
+    }
+
     updateUserRegistrarRoles(
-        verifyUserExists(this.userData.get().emailAddress),
-        registrarId,
-        requestRoleToAllowedRoles(this.userData.get().role));
+        userToUpdate, registrarId, requestRoleToAllowedRoles(this.userData.get().role));
 
     sendConfirmationEmail(registrarId, this.userData.get().emailAddress, "Updated user");
     consoleApiParams.response().setStatus(SC_OK);

core/src/main/java/google/registry/ui/server/console/domains/ConsoleDomainActionType.java

@@ -87,6 +87,6 @@ public abstract class ConsoleDomainActionType {
   }
 
   private String replaceValue(String xml, String key, String value) {
-    return xml.replaceAll("%" + key + "%", XML_ESCAPER.escape(value));
+    return xml.replace("%" + key + "%", XML_ESCAPER.escape(value));
   }
 }

core/src/test/java/google/registry/tools/server/GenerateZoneFilesActionTest.java

@@ -182,4 +182,34 @@ class GenerateZoneFilesActionTest {
     // The remaining lines can be in any order.
     assertThat(generatedFileLines).containsExactlyElementsIn(goldenFileLines);
   }
+
+  @Test
+  void testGenerate_deletedNameserver_ignoresAndLogsSevere() throws Exception {
+    createTlds("tld");
+    Instant now = Instant.parse("2024-03-27T00:00:00Z");
+    Host deletedHost = persistResource(newHost("ns.deleted.tld"));
+    // Delete the host before exportTime
+    persistResource(deletedHost.asBuilder().setDeletionTime(now.minusSeconds(3600)).build());
+
+    persistResource(
+        DatabaseHelper.newDomain("example.tld")
+            .asBuilder()
+            .addNameservers(ImmutableSet.of(deletedHost.createVKey()))
+            .build());
+
+    GenerateZoneFilesAction action = new GenerateZoneFilesAction();
+    action.bucket = "zonefiles-bucket";
+    action.gcsUtils = gcsUtils;
+    action.databaseRetention = Duration.ofDays(29);
+    action.dnsDefaultATtl = Duration.ofSeconds(11);
+    action.dnsDefaultNsTtl = Duration.ofSeconds(222);
+    action.dnsDefaultDsTtl = Duration.ofSeconds(3333);
+    action.clock = new FakeClock(plusMinutes(now, 2));
+
+    Map<String, Object> response =
+        action.handleJsonRequest(
+            ImmutableMap.<String, Object>of("tlds", ImmutableList.of("tld"), "exportTime", now));
+    assertThat(response)
+        .containsEntry("filenames", ImmutableList.of("gs://zonefiles-bucket/tld-" + now + ".zone"));
+  }
 }

core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java

@@ -424,6 +424,51 @@ class ConsoleUsersActionTest extends ConsoleActionBaseTestCase {
         .isEqualTo(RegistrarRole.TECH_CONTACT);
   }
 
+  @Test
+  void testFailure_appendUser_crossTenantNoPermission() throws IOException {
+    User callingUser = DatabaseHelper.loadByKey(VKey.create(User.class, "test1@test.com"));
+    AuthResult authResult = AuthResult.createUser(callingUser);
+    ConsoleUsersAction action =
+        createAction(
+            Optional.of(ConsoleApiParamsUtils.createFake(authResult)),
+            Optional.of("POST"),
+            Optional.of(
+                new UserData("test3@test.com", null, RegistrarRole.TECH_CONTACT.name(), null)));
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_FORBIDDEN);
+  }
+
+  @Test
+  void testSuccess_appendUser_crossTenantWithPermission() throws IOException {
+    User callingUser =
+        DatabaseHelper.persistResource(
+            new User.Builder()
+                .setEmailAddress("multitenant@test.com")
+                .setUserRoles(
+                    new UserRoles()
+                        .asBuilder()
+                        .setRegistrarRoles(
+                            ImmutableMap.of(
+                                "TheRegistrar",
+                                RegistrarRole.PRIMARY_CONTACT,
+                                "NewRegistrar",
+                                RegistrarRole.PRIMARY_CONTACT))
+                        .build())
+                .build());
+    AuthResult authResult = AuthResult.createUser(callingUser);
+    ConsoleUsersAction action =
+        createAction(
+            Optional.of(ConsoleApiParamsUtils.createFake(authResult)),
+            Optional.of("POST"),
+            Optional.of(
+                new UserData("test3@test.com", null, RegistrarRole.TECH_CONTACT.name(), null)));
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_OK);
+    User appendedUser = DatabaseHelper.loadByKey(VKey.create(User.class, "test3@test.com"));
+    assertThat(appendedUser.getUserRoles().getRegistrarRoles().get("TheRegistrar"))
+        .isEqualTo(RegistrarRole.TECH_CONTACT);
+  }
+
   @Test
   void testFailure_appendUser_globalAdmin() throws IOException {
     User user = DatabaseHelper.createAdminUser("email@email.com");
@@ -504,6 +549,62 @@ class ConsoleUsersActionTest extends ConsoleActionBaseTestCase {
         .contains("Cannot delete a global administrator or user with a global role");
   }
 
+  @Test
+  void testFailure_updateUser_globalAdmin() throws IOException {
+    User user = DatabaseHelper.createAdminUser("email@email.com");
+    AuthResult authResult = AuthResult.createUser(user);
+    // Historically associated global admin
+    DatabaseHelper.persistResource(
+        new User.Builder()
+            .setEmailAddress("globaladmin@test.com")
+            .setUserRoles(
+                new UserRoles.Builder()
+                    .setIsAdmin(true)
+                    .setGlobalRole(GlobalRole.NONE)
+                    .setRegistrarRoles(
+                        ImmutableMap.of("TheRegistrar", RegistrarRole.PRIMARY_CONTACT))
+                    .build())
+            .build());
+
+    ConsoleUsersAction action =
+        createAction(
+            Optional.of(ConsoleApiParamsUtils.createFake(authResult)),
+            Optional.of("PUT"),
+            Optional.of(
+                new UserData(
+                    "globaladmin@test.com", null, RegistrarRole.ACCOUNT_MANAGER.toString(), null)));
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST);
+    assertThat(response.getPayload())
+        .contains("Cannot update a global administrator or user with a global role");
+  }
+
+  @Test
+  void testFailure_updateUser_globalRole() throws IOException {
+    User user = DatabaseHelper.createAdminUser("email@email.com");
+    AuthResult authResult = AuthResult.createUser(user);
+    // Historically associated user with global role
+    DatabaseHelper.persistResource(
+        new User.Builder()
+            .setEmailAddress("support@test.com")
+            .setUserRoles(
+                new UserRoles.Builder()
+                    .setIsAdmin(false)
+                    .setGlobalRole(GlobalRole.SUPPORT_AGENT)
+                    .build())
+            .build());
+
+    ConsoleUsersAction action =
+        createAction(
+            Optional.of(ConsoleApiParamsUtils.createFake(authResult)),
+            Optional.of("PUT"),
+            Optional.of(
+                new UserData(
+                    "support@test.com", null, RegistrarRole.ACCOUNT_MANAGER.toString(), null)));
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_FORBIDDEN);
+  }
+
   private ConsoleUsersAction createAction(
       Optional<ConsoleApiParams> maybeConsoleApiParams,
       Optional<String> method,

core/src/test/java/google/registry/ui/server/console/settings/SecurityActionTest.java

@@ -113,6 +113,31 @@ class SecurityActionTest extends ConsoleActionBaseTestCase {
     assertThat(history.getDescription()).hasValue("registrarId|IP_CHANGE,PRIMARY_SSL_CERT_CHANGE");
   }
 
+  @Test
+  void testSuccess_postRegistrarInfo_nullIpAllowList() throws IOException {
+    CertificateChecker lenientChecker =
+        new CertificateChecker(
+            ImmutableSortedMap.of(START_INSTANT, 20825, Instant.parse("2020-09-01T00:00:00Z"), 398),
+            30,
+            15,
+            2048,
+            ImmutableSet.of("secp256r1", "secp384r1"),
+            clock);
+
+    clock.setTo(Instant.parse("2020-11-01T00:00:00Z"));
+    String jsonWithNullIp =
+        String.format(
+            "{\"registrarId\": \"registrarId\", \"clientCertificate\": \"%s\","
+                + " \"ipAddressAllowList\": null}",
+            SAMPLE_CERT2);
+    SecurityAction action =
+        createAction(testRegistrar.getRegistrarId(), jsonWithNullIp, lenientChecker);
+    action.run();
+    assertThat(((FakeResponse) consoleApiParams.response()).getStatus()).isEqualTo(SC_OK);
+    Registrar r = loadRegistrar(testRegistrar.getRegistrarId());
+    assertThat(r.getIpAddressAllowList()).isEmpty();
+  }
+
   @Test
   void testFailure_validityPeriodTooLong_returnsSpecificError() throws IOException {
     CertificateChecker strictChecker =