From a7118dfbba186f993c38beeaf8ec22ea6e4607a4 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Wed, 8 Jul 2026 13:35:32 -0400 Subject: [PATCH] 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 --- .../registry/model/registrar/Registrar.java | 5 +- .../tools/server/GenerateZoneFilesAction.java | 15 ++- .../ui/server/console/ConsoleUsersAction.java | 22 ++-- .../domains/ConsoleDomainActionType.java | 2 +- .../server/GenerateZoneFilesActionTest.java | 30 ++++++ .../console/ConsoleUsersActionTest.java | 101 ++++++++++++++++++ .../console/settings/SecurityActionTest.java | 25 +++++ 7 files changed, 186 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index 85fc9a83a..14418a748 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/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 ipAddressAllowList) { - getInstance().ipAddressAllowList = ImmutableList.copyOf(ipAddressAllowList); + getInstance().ipAddressAllowList = + ipAddressAllowList == null + ? ImmutableList.of() + : ImmutableList.copyOf(ipAddressAllowList); return this; } diff --git a/core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java b/core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java index 147488361..be4a65459 100644 --- a/core/src/main/java/google/registry/tools/server/GenerateZoneFilesAction.java +++ b/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( diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java index 3a27e9f4a..4f188ad16 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleUsersAction.java +++ b/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 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); diff --git a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleDomainActionType.java b/core/src/main/java/google/registry/ui/server/console/domains/ConsoleDomainActionType.java index cbf528b31..f063bc315 100644 --- a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleDomainActionType.java +++ b/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)); } } diff --git a/core/src/test/java/google/registry/tools/server/GenerateZoneFilesActionTest.java b/core/src/test/java/google/registry/tools/server/GenerateZoneFilesActionTest.java index 047c35d04..06ede439a 100644 --- a/core/src/test/java/google/registry/tools/server/GenerateZoneFilesActionTest.java +++ b/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 response = + action.handleJsonRequest( + ImmutableMap.of("tlds", ImmutableList.of("tld"), "exportTime", now)); + assertThat(response) + .containsEntry("filenames", ImmutableList.of("gs://zonefiles-bucket/tld-" + now + ".zone")); + } } diff --git a/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java b/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java index bd874cb6e..43941f84f 100644 --- a/core/src/test/java/google/registry/ui/server/console/ConsoleUsersActionTest.java +++ b/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 maybeConsoleApiParams, Optional method, diff --git a/core/src/test/java/google/registry/ui/server/console/settings/SecurityActionTest.java b/core/src/test/java/google/registry/ui/server/console/settings/SecurityActionTest.java index ad1f61a62..ccf52b079 100644 --- a/core/src/test/java/google/registry/ui/server/console/settings/SecurityActionTest.java +++ b/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 =