diff --git a/GEMINI.md b/GEMINI.md index b393c07733a..ab00dcdb1a2 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -94,7 +94,7 @@ Do not wait for the user to tell you to improve the skills; it is your responsib # Gemini Engineering Guide: Nomulus Codebase -This document captures high-level architectural patterns, lessons learned from large-scale refactorings (like the Joda-Time to `java.time` migration), and specific instructions to avoid common pitfalls in this environment. +This document captures high-level architectural patterns, lessons learned from large-scale refactorings, and specific instructions to avoid common pitfalls in this environment. ## 🏛 Architecture Overview diff --git a/console-webapp/src/app/domains/domainList.component.ts b/console-webapp/src/app/domains/domainList.component.ts index 0cc3e8ccf96..21327f1dff2 100644 --- a/console-webapp/src/app/domains/domainList.component.ts +++ b/console-webapp/src/app/domains/domainList.component.ts @@ -48,7 +48,11 @@ interface DomainData { selector: 'app-response-dialog', template: `

{{ data.title }}

- + + @for (line of data.content; track line) { +
{{ line }}
+ } + @@ -59,7 +63,7 @@ export class ResponseDialogComponent { constructor( public dialogRef: MatDialogRef, @Inject(MAT_DIALOG_DATA) - public data: { title: string; content: string } + public data: { title: string; content: string[] } ) {} onClose(): void { @@ -312,11 +316,13 @@ export class DomainListComponent { this.dialog.open(ResponseDialogComponent, { data: { title: 'Domain Deletion Results', - content: `Successfully deleted - ${successCount} domain(s)
Failed to delete - ${failureCount} domain(s)
${ + content: [ + `Successfully deleted - ${successCount} domain(s)`, + `Failed to delete - ${failureCount} domain(s)`, failureCount - ? 'Some domains could not be deleted due to ongoing processes or server errors. ' - : '' - }Please check the table for more information.`, + ? 'Some domains could not be deleted due to ongoing processes or server errors. Please check the table for more information.' + : 'Please check the table for more information.', + ], }, }); this.selection.clear(); diff --git a/console-webapp/src/app/registrar/registrarDetails.component.html b/console-webapp/src/app/registrar/registrarDetails.component.html index 1eda62f338f..38937c15e0c 100644 --- a/console-webapp/src/app/registrar/registrarDetails.component.html +++ b/console-webapp/src/app/registrar/registrarDetails.component.html @@ -97,10 +97,9 @@

Registrar details

@for (column of columns; track column.columnDef) { {{ column.header }} - + {{ + column.cell(registrarInEdit) + }} } diff --git a/console-webapp/src/app/registrar/registrarsTable.component.html b/console-webapp/src/app/registrar/registrarsTable.component.html index d2878b7f283..5ec5f42da0e 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.html +++ b/console-webapp/src/app/registrar/registrarsTable.component.html @@ -49,10 +49,9 @@

Registrars

{{ column.header }} - + {{ + column.cell(row) + }} } diff --git a/console-webapp/src/app/registrar/registrarsTable.component.ts b/console-webapp/src/app/registrar/registrarsTable.component.ts index 702a61bcd2d..69f892a3a58 100644 --- a/console-webapp/src/app/registrar/registrarsTable.component.ts +++ b/console-webapp/src/app/registrar/registrarsTable.component.ts @@ -56,7 +56,7 @@ export const columns = [ cell: (record: Registrar) => `${Object.entries(record.billingAccountMap || {}).reduce( (acc, [key, val]) => { - return `${acc}${key}=${val}
`; + return `${acc}${key}=${val}\n`; }, '' )}`, diff --git a/console-webapp/src/app/settings/contact/contact.component.html b/console-webapp/src/app/settings/contact/contact.component.html index 362c8396c6b..4b3528ab608 100644 --- a/console-webapp/src/app/settings/contact/contact.component.html +++ b/console-webapp/src/app/settings/contact/contact.component.html @@ -25,7 +25,18 @@

No contacts found

@for (column of columns; track column) { {{ column.header }} - + + @if (column.columnDef === 'name') { +
+
{{ row.name }}
+
+ {{ row.userFriendlyTypes.join(" • ") }} +
+
+ } @else { + {{ column.cell(row) }} + } + } diff --git a/console-webapp/src/app/settings/contact/contact.component.ts b/console-webapp/src/app/settings/contact/contact.component.ts index cc23a7c3b89..c9dd0c2f3d9 100644 --- a/console-webapp/src/app/settings/contact/contact.component.ts +++ b/console-webapp/src/app/settings/contact/contact.component.ts @@ -34,14 +34,7 @@ export default class ContactComponent { { columnDef: 'name', header: 'Name', - cell: (contact: ViewReadyContact) => ` -
-
${contact.name}
-
${contact.userFriendlyTypes.join( - ' • ' - )}
-
- `, + cell: (contact: ViewReadyContact) => `${contact.name}`, }, { columnDef: 'emailAddress', diff --git a/console-webapp/src/app/users/usersList.component.html b/console-webapp/src/app/users/usersList.component.html index eb88a02b3b7..8cc7b2a0c61 100644 --- a/console-webapp/src/app/users/usersList.component.html +++ b/console-webapp/src/app/users/usersList.component.html @@ -10,7 +10,7 @@ {{ column.header }} - + {{ column.cell(row) }} } diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index 7c7b9448d77..cee88303343 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -173,6 +173,18 @@ public static String provideSupportEmail(RegistryConfigSettings config) { return config.registrarConsole.supportEmailAddress; } + @Provides + @Config("consoleHistoryQueryLimit") + public static int provideConsoleHistoryQueryLimit(RegistryConfigSettings config) { + return config.registrarConsole.historyQueryLimit; + } + + @Provides + @Config("consoleBulkDomainActionLimit") + public static int provideConsoleBulkDomainActionLimit(RegistryConfigSettings config) { + return config.registrarConsole.bulkDomainActionLimit; + } + /** * The DUM file name, used as a file name base for DUM csv file * diff --git a/core/src/main/java/google/registry/config/RegistryConfigSettings.java b/core/src/main/java/google/registry/config/RegistryConfigSettings.java index 1f2331b3769..5b8ddedadcd 100644 --- a/core/src/main/java/google/registry/config/RegistryConfigSettings.java +++ b/core/src/main/java/google/registry/config/RegistryConfigSettings.java @@ -181,6 +181,8 @@ public static class RegistrarConsole { public String supportPhoneNumber; public String supportEmailAddress; public String technicalDocsUrl; + public int historyQueryLimit; + public int bulkDomainActionLimit; } /** Configuration for monitoring. */ diff --git a/core/src/main/java/google/registry/config/files/default-config.yaml b/core/src/main/java/google/registry/config/files/default-config.yaml index 22f751de10c..9b8719d7511 100644 --- a/core/src/main/java/google/registry/config/files/default-config.yaml +++ b/core/src/main/java/google/registry/config/files/default-config.yaml @@ -380,6 +380,12 @@ registrarConsole: # URL linking to directory of technical support docs on the registry. technicalDocsUrl: http://example.com/your_support_docs/ + # Maximum number of history records returned in a single query. + historyQueryLimit: 500 + + # Maximum number of domains allowed in a single bulk action. + bulkDomainActionLimit: 500 + monitoring: # Max queries per second for the Google Cloud Monitoring V3 (aka Stackdriver) # API. The limit can be adjusted by contacting Cloud Support. diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java index a9c8943315f..a7384e361c7 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleEppPasswordAction.java @@ -27,6 +27,7 @@ import com.google.gson.annotations.Expose; import google.registry.flows.EppException.AuthenticationErrorException; import google.registry.flows.PasswordOnlyTransportCredentials; +import google.registry.model.console.ConsolePermission; import google.registry.model.console.ConsoleUpdateHistory; import google.registry.model.console.User; import google.registry.model.registrar.Registrar; @@ -84,6 +85,8 @@ protected void postHandler(User user) { eppRequestBody.newPassword().equals(eppRequestBody.newPasswordRepeat()), "New password fields don't match"); + checkPermission(user, eppRequestBody.registrarId(), ConsolePermission.CONFIGURE_EPP_CONNECTION); + Registrar registrar; try { registrar = registrarAccessor.getRegistrar(eppRequestBody.registrarId()); diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java index d967e1e3e02..138e1a1f4b8 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleHistoryDataAction.java @@ -62,17 +62,20 @@ public class ConsoleHistoryDataAction extends ConsoleApiAction { private final Optional consoleUserEmail; private final String supportEmail; + private final int historyQueryLimit; @Inject public ConsoleHistoryDataAction( ConsoleApiParams consoleApiParams, @Config("supportEmail") String supportEmail, + @Config("consoleHistoryQueryLimit") int historyQueryLimit, @Parameter("registrarId") String registrarId, @Parameter("consoleUserEmail") Optional consoleUserEmail) { super(consoleApiParams); this.registrarId = registrarId; this.consoleUserEmail = consoleUserEmail; this.supportEmail = supportEmail; + this.historyQueryLimit = historyQueryLimit; } @Override @@ -117,6 +120,7 @@ private void historyByRegistrarId(User user, String registrarId) { .createNativeQuery(SQL_REGISTRAR_HISTORY, ConsoleUpdateHistory.class) .setParameter("registrarId", registrarId) .setHint("org.hibernate.fetchSize", 1000) + .setMaxResults(historyQueryLimit) .getResultList()); List formattedHistoryList = diff --git a/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java b/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java index 732e97f5a20..899b2aa69f8 100644 --- a/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java +++ b/core/src/main/java/google/registry/ui/server/console/PasswordResetRequestAction.java @@ -86,7 +86,7 @@ private void performRequest(User user) { "Must provide registry lock email to reset"); requiredPermission = ConsolePermission.MANAGE_USERS; destinationEmail = passwordResetRequestData.registryLockEmail; - checkUserExistsWithRegistryLockEmail(destinationEmail); + checkUserExistsWithRegistryLockEmail(destinationEmail, registrarId); emailSubject = "Registry lock password reset request"; } default -> throw new IllegalArgumentException("Unknown type " + type); @@ -121,12 +121,24 @@ private void performRequest(User user) { .sendEmail(EmailMessage.create(emailSubject, body, destinationAddress)); } - static User checkUserExistsWithRegistryLockEmail(String destinationEmail) { - return tm().createQueryComposer(User.class) - .where("registryLockEmailAddress", QueryComposer.Comparator.EQ, destinationEmail) - .first() - .orElseThrow( - () -> new IllegalArgumentException("Unknown user with lock email " + destinationEmail)); + static User checkUserExistsWithRegistryLockEmail(String destinationEmail, String registrarId) { + User targetUser = + tm().createQueryComposer(User.class) + .where("registryLockEmailAddress", QueryComposer.Comparator.EQ, destinationEmail) + .first() + .orElseThrow( + () -> + new IllegalArgumentException( + "Unknown user with lock email " + destinationEmail)); + + // Prevent IDOR: Ensure the resolved user actually belongs to the registrar the requester + // has permissions for, or is a global admin. + if (!targetUser.getUserRoles().isAdmin() + && !targetUser.getUserRoles().getRegistrarRoles().containsKey(registrarId)) { + throw new IllegalArgumentException( + "User with lock email " + destinationEmail + " is not associated with " + registrarId); + } + return targetUser; } private String getAdminPocEmail(String registrarId) { diff --git a/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java b/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java index 796c4512fad..52c2cc889a2 100644 --- a/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java +++ b/core/src/main/java/google/registry/ui/server/console/PasswordResetVerifyAction.java @@ -98,7 +98,9 @@ private void handleEppPasswordReset(PasswordResetRequest request) { } private void handleRegistryLockPasswordReset(PasswordResetRequest request) { - User affectedUser = checkUserExistsWithRegistryLockEmail(request.getDestinationEmail()); + User affectedUser = + checkUserExistsWithRegistryLockEmail( + request.getDestinationEmail(), request.getRegistrarId()); tm().put( affectedUser .asBuilder() diff --git a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java b/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java index b4ed47db12e..7de63e1999c 100644 --- a/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java +++ b/core/src/main/java/google/registry/ui/server/console/domains/ConsoleBulkDomainAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console.domains; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static jakarta.servlet.http.HttpServletResponse.SC_OK; @@ -22,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.gson.JsonElement; import com.google.gson.annotations.Expose; +import google.registry.config.RegistryConfig.Config; import google.registry.flows.EppController; import google.registry.flows.EppRequestSource; import google.registry.flows.PasswordOnlyTransportCredentials; @@ -64,11 +66,13 @@ public record BulkDomainList(@Expose List domainList) {} private final String registrarId; private final String bulkDomainAction; private final Optional optionalJsonPayload; + private final int bulkDomainActionLimit; @Inject public ConsoleBulkDomainAction( ConsoleApiParams consoleApiParams, EppController eppController, + @Config("consoleBulkDomainActionLimit") int bulkDomainActionLimit, @Parameter("registrarId") String registrarId, @Parameter("bulkDomainAction") String bulkDomainAction, @OptionalJsonPayload Optional optionalJsonPayload) { @@ -77,6 +81,7 @@ public ConsoleBulkDomainAction( this.registrarId = registrarId; this.bulkDomainAction = bulkDomainAction; this.optionalJsonPayload = optionalJsonPayload; + this.bulkDomainActionLimit = bulkDomainActionLimit; } @Override @@ -85,6 +90,13 @@ protected void postHandler(User user) { optionalJsonPayload.orElseThrow( () -> new IllegalArgumentException("Bulk action payload must be present")); BulkDomainList domainList = consoleApiParams.gson().fromJson(jsonPayload, BulkDomainList.class); + checkArgument( + domainList.domainList != null && !domainList.domainList.isEmpty(), + "Domain list cannot be empty"); + checkArgument( + domainList.domainList.size() <= bulkDomainActionLimit, + "Cannot process more than %s domains in a single bulk action", + bulkDomainActionLimit); ConsoleDomainActionType actionType = ConsoleDomainActionType.parseActionType(bulkDomainAction, jsonPayload); diff --git a/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java b/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java index d7fee4abce5..137d315724d 100644 --- a/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/ConsoleHistoryDataActionTest.java @@ -23,6 +23,8 @@ import com.google.common.collect.ImmutableMap; import com.google.gson.Gson; +import com.google.gson.JsonArray; +import com.google.gson.JsonParser; import google.registry.model.console.ConsoleUpdateHistory; import google.registry.model.console.RegistrarRole; import google.registry.model.console.User; @@ -162,6 +164,27 @@ void testSuccess_noResults() { assertThat(response.getPayload()).isEqualTo("[]"); } + @Test + void testSuccess_limitsResults() { + for (int i = 0; i < 10; i++) { + DatabaseHelper.persistResource( + new ConsoleUpdateHistory.Builder() + .setType(ConsoleUpdateHistory.Type.REGISTRAR_UPDATE) + .setDescription("TheRegistrar|some detail " + i) + .setActingUser(fteUser) + .setUrl("https://test.com") + .setMethod("GET") + .setModificationTime(clock.now()) + .build()); + } + ConsoleHistoryDataAction action = + createAction(AuthResult.createUser(fteUser), "TheRegistrar", Optional.empty(), 5); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_OK); + JsonArray payload = JsonParser.parseString(response.getPayload()).getAsJsonArray(); + assertThat(payload.size()).isEqualTo(5); + } + @Test void testFailure_getByRegistrar_noPermission() { ConsoleHistoryDataAction action = @@ -192,10 +215,15 @@ void testFailure_emptyRegistrarId() { private ConsoleHistoryDataAction createAction( AuthResult authResult, String registrarId, Optional consoleUserEmail) { + return createAction(authResult, registrarId, consoleUserEmail, 500); + } + + private ConsoleHistoryDataAction createAction( + AuthResult authResult, String registrarId, Optional consoleUserEmail, int limit) { consoleApiParams = ConsoleApiParamsUtils.createFake(authResult); when(consoleApiParams.request().getMethod()).thenReturn("GET"); response = (FakeResponse) consoleApiParams.response(); return new ConsoleHistoryDataAction( - consoleApiParams, SUPPORT_EMAIL, registrarId, consoleUserEmail); + consoleApiParams, SUPPORT_EMAIL, limit, registrarId, consoleUserEmail); } } diff --git a/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java b/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java index 84afa9956da..ed83fd74178 100644 --- a/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java +++ b/core/src/test/java/google/registry/ui/server/console/domains/ConsoleBulkDomainActionTest.java @@ -45,6 +45,7 @@ import google.registry.testing.FakeResponse; import google.registry.ui.server.console.ConsoleActionBaseTestCase; import google.registry.ui.server.console.ConsoleApiParams; +import java.util.Collections; import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -173,7 +174,9 @@ void testHalfSuccess_halfNonexistent() throws Exception { @Test void testFailure_badActionString() { - ConsoleBulkDomainAction action = createAction("bad", GSON.toJsonTree(ImmutableMap.of())); + ConsoleBulkDomainAction action = + createAction( + "bad", GSON.toJsonTree(ImmutableMap.of("domainList", ImmutableList.of("domain.tld")))); action.run(); assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); assertThat(response.getPayload()) @@ -190,6 +193,29 @@ void testFailure_emptyBody() { assertThat(response.getPayload()).isEqualTo("Bulk action payload must be present"); } + @Test + void testFailure_listTooLarge() { + JsonElement payload = + GSON.toJsonTree( + ImmutableMap.of( + "domainList", Collections.nCopies(6, "domain.tld"), "reason", "reason")); + ConsoleBulkDomainAction action = createAction("DELETE", payload, fteUser, 5); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()) + .isEqualTo("Cannot process more than 5 domains in a single bulk action"); + } + + @Test + void testFailure_emptyList() { + JsonElement payload = + GSON.toJsonTree(ImmutableMap.of("domainList", ImmutableList.of(), "reason", "reason")); + ConsoleBulkDomainAction action = createAction("DELETE", payload); + action.run(); + assertThat(response.getStatus()).isEqualTo(SC_BAD_REQUEST); + assertThat(response.getPayload()).isEqualTo("Domain list cannot be empty"); + } + @Test void testFailure_noPermission() { JsonElement payload = @@ -232,15 +258,20 @@ void testFailure_noPermission() { // } private ConsoleBulkDomainAction createAction(String action, JsonElement payload) { - return createAction(action, payload, fteUser); + return createAction(action, payload, fteUser, 500); } private ConsoleBulkDomainAction createAction(String action, JsonElement payload, User user) { + return createAction(action, payload, user, 500); + } + + private ConsoleBulkDomainAction createAction( + String action, JsonElement payload, User user, int limit) { AuthResult authResult = AuthResult.createUser(user); ConsoleApiParams params = ConsoleApiParamsUtils.createFake(authResult); when(params.request().getMethod()).thenReturn("POST"); response = (FakeResponse) params.response(); return new ConsoleBulkDomainAction( - params, eppController, "TheRegistrar", action, Optional.ofNullable(payload)); + params, eppController, limit, "TheRegistrar", action, Optional.ofNullable(payload)); } }