From 43000a5f80b3850d4da7b06d664d611399f0865e Mon Sep 17 00:00:00 2001 From: gbrodman Date: Thu, 16 May 2024 14:29:14 -0400 Subject: [PATCH] Refactor common exception handling in ConsoleApiAction (#2443) There are a bunch of cases where we want common exception handling and it's annoying to have to deal with the common "set failed response and make sure to return" a bunch of times. This allows us to break up request methods more easily, since we can now often throw exceptions that will break all the way back up to ConsoleApiAction. Previously, any error handling had to exist in the primary handler method so it could return. --- .../ui/server/console/ConsoleApiAction.java | 44 ++++++- .../console/ConsoleDomainListAction.java | 22 +--- .../console/ConsoleDumDownloadAction.java | 8 +- .../console/ConsoleEppPasswordAction.java | 57 +++----- .../console/ConsoleRegistryLockAction.java | 97 +++++--------- .../ui/server/console/RegistrarsAction.java | 122 ++++++++---------- .../console/settings/ContactAction.java | 24 +--- .../console/settings/SecurityAction.java | 13 +- .../settings/WhoisRegistrarFieldsAction.java | 24 +--- 9 files changed, 164 insertions(+), 247 deletions(-) diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleApiAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleApiAction.java index 46f81b09f..ea2f12d62 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleApiAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleApiAction.java @@ -17,8 +17,12 @@ package google.registry.ui.server.console; import static google.registry.request.Action.Method.GET; import com.google.api.client.http.HttpStatusCodes; +import com.google.common.base.Throwables; +import com.google.common.flogger.FluentLogger; +import google.registry.model.console.ConsolePermission; import google.registry.model.console.GlobalRole; import google.registry.model.console.User; +import google.registry.request.HttpException; import google.registry.request.auth.AuthResult; import google.registry.security.XsrfTokenManager; import google.registry.ui.server.registrar.ConsoleApiParams; @@ -31,6 +35,9 @@ import java.util.Optional; /** Base class for handling Console API requests */ public abstract class ConsoleApiAction implements Runnable { + + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + protected ConsoleApiParams consoleApiParams; public ConsoleApiAction(ConsoleApiParams consoleApiParams) { @@ -60,15 +67,36 @@ public abstract class ConsoleApiAction implements Runnable { } } - if (consoleApiParams.request().getMethod().equals(GET.toString())) { - getHandler(user); - } else { - if (verifyXSRF()) { - postHandler(user); + try { + if (consoleApiParams.request().getMethod().equals(GET.toString())) { + getHandler(user); + } else { + if (verifyXSRF()) { + postHandler(user); + } } + } catch (ConsolePermissionForbiddenException e) { + logger.atWarning().withCause(e).log("Forbidden"); + setFailedResponse("", HttpStatusCodes.STATUS_CODE_FORBIDDEN); + } catch (HttpException.BadRequestException | IllegalArgumentException e) { + logger.atWarning().withCause(e).log("Error in request"); + setFailedResponse( + Throwables.getRootCause(e).getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST); + } catch (Throwable t) { + logger.atWarning().withCause(t).log("Internal server error"); + setFailedResponse( + Throwables.getRootCause(t).getMessage(), HttpStatusCodes.STATUS_CODE_SERVER_ERROR); } } + protected void checkPermission(User user, String registrarId, ConsolePermission permission) { + if (!user.getUserRoles().hasPermission(registrarId, permission)) { + throw new ConsolePermissionForbiddenException( + String.format( + "User %s does not have permission %s on registrar %s", + user.getEmailAddress(), permission, registrarId)); + } + } protected void postHandler(User user) { throw new UnsupportedOperationException("Console API POST handler not implemented"); @@ -96,4 +124,10 @@ public abstract class ConsoleApiAction implements Runnable { return true; } + /** Specialized exception class used for failure when a user doesn't have the right permission. */ + private static class ConsolePermissionForbiddenException extends RuntimeException { + private ConsolePermissionForbiddenException(String message) { + super(message); + } + } } diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleDomainListAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleDomainListAction.java index 632ed4910..b8f2b0e9e 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleDomainListAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleDomainListAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console; +import static com.google.common.base.Preconditions.checkArgument; import static google.registry.model.console.ConsolePermission.DOWNLOAD_DOMAINS; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -82,22 +83,11 @@ public class ConsoleDomainListAction extends ConsoleApiAction { @Override protected void getHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, DOWNLOAD_DOMAINS)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } - if (resultsPerPage < 1 || resultsPerPage > 500) { - setFailedResponse( - "Results per page must be between 1 and 500 inclusive", - HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } - if (pageNumber < 0) { - setFailedResponse( - "Page number must be non-negative", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } - + checkPermission(user, registrarId, DOWNLOAD_DOMAINS); + checkArgument( + resultsPerPage > 0 && resultsPerPage <= 500, + "Results per page must be between 1 and 500 inclusive"); + checkArgument(pageNumber >= 0, "Page number must be non-negative"); tm().transact(this::runInTransaction); } diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleDumDownloadAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleDumDownloadAction.java index 23ed07f14..d621c197b 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleDumDownloadAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleDumDownloadAction.java @@ -58,7 +58,7 @@ public class ConsoleDumDownloadAction extends ConsoleApiAction { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); public static final String PATH = "/console-api/dum-download"; - private Clock clock; + private final Clock clock; private final String registrarId; private final String dumFileName; @@ -76,11 +76,7 @@ public class ConsoleDumDownloadAction extends ConsoleApiAction { @Override protected void getHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.DOWNLOAD_DOMAINS)) { - consoleApiParams.response().setStatus(HttpServletResponse.SC_FORBIDDEN); - return; - } - + checkPermission(user, registrarId, ConsolePermission.DOWNLOAD_DOMAINS); consoleApiParams.response().setContentType(MediaType.CSV_UTF_8); consoleApiParams .response() 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 d69d6144e..47da31168 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 @@ -14,26 +14,23 @@ package google.registry.ui.server.console; +import static com.google.common.base.Preconditions.checkArgument; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.POST; import static google.registry.request.RequestParameters.extractRequiredParameter; import com.google.api.client.http.HttpStatusCodes; -import com.google.common.base.Throwables; -import com.google.common.flogger.FluentLogger; import google.registry.flows.EppException.AuthenticationErrorException; import google.registry.flows.PasswordOnlyTransportCredentials; import google.registry.groups.GmailClient; import google.registry.model.console.User; import google.registry.model.registrar.Registrar; import google.registry.request.Action; -import google.registry.request.HttpException.BadRequestException; import google.registry.request.auth.Auth; import google.registry.request.auth.AuthenticatedRegistrarAccessor; import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException; import google.registry.ui.server.registrar.ConsoleApiParams; import google.registry.util.EmailMessage; -import java.util.Optional; import javax.inject.Inject; import javax.mail.internet.InternetAddress; @@ -44,8 +41,6 @@ import javax.mail.internet.InternetAddress; auth = Auth.AUTH_PUBLIC_LOGGED_IN) public class ConsoleEppPasswordAction extends ConsoleApiAction { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - protected static final String EMAIL_SUBJ = "EPP password update confirmation"; protected static final String EMAIL_BODY = "Dear %s,\n" + "This is to confirm that your account password has been changed."; @@ -69,25 +64,12 @@ public class ConsoleEppPasswordAction extends ConsoleApiAction { @Override protected void postHandler(User user) { - String registrarId; - String oldPassword; - String newPassword; - String newPasswordRepeat; - - try { - registrarId = extractRequiredParameter(consoleApiParams.request(), "registrarId"); - oldPassword = extractRequiredParameter(consoleApiParams.request(), "oldPassword"); - newPassword = extractRequiredParameter(consoleApiParams.request(), "newPassword"); - newPasswordRepeat = extractRequiredParameter(consoleApiParams.request(), "newPasswordRepeat"); - } catch (BadRequestException e) { - setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } - - if (!newPassword.equals(newPasswordRepeat)) { - setFailedResponse("New password fields don't match", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } + String registrarId = extractRequiredParameter(consoleApiParams.request(), "registrarId"); + String oldPassword = extractRequiredParameter(consoleApiParams.request(), "oldPassword"); + String newPassword = extractRequiredParameter(consoleApiParams.request(), "newPassword"); + String newPasswordRepeat = + extractRequiredParameter(consoleApiParams.request(), "newPasswordRepeat"); + checkArgument(newPassword.equals(newPasswordRepeat), "New password fields don't match"); Registrar registrar; try { @@ -104,22 +86,15 @@ public class ConsoleEppPasswordAction extends ConsoleApiAction { return; } - try { - tm().transact( - () -> { - tm().put(registrar.asBuilder().setPassword(newPassword).build()); - this.gmailClient.sendEmail( - EmailMessage.create( - EMAIL_SUBJ, - String.format(EMAIL_BODY, registrar.getRegistrarName()), - new InternetAddress(registrar.getEmailAddress(), true))); - }); - } catch (Throwable e) { - logger.atWarning().withCause(e).log("Failed to update password."); - String message = - Optional.ofNullable(Throwables.getRootCause(e).getMessage()).orElse("Unspecified error"); - setFailedResponse(message, HttpStatusCodes.STATUS_CODE_SERVER_ERROR); - } + tm().transact( + () -> { + tm().put(registrar.asBuilder().setPassword(newPassword).build()); + this.gmailClient.sendEmail( + EmailMessage.create( + EMAIL_SUBJ, + String.format(EMAIL_BODY, registrar.getRegistrarName()), + new InternetAddress(registrar.getEmailAddress(), true))); + }); consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK); } diff --git a/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java b/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java index ef8de8090..c10e8c2a8 100644 --- a/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java +++ b/core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; @@ -26,7 +27,6 @@ import static google.registry.ui.server.registrar.RegistryLockPostAction.VERIFIC import com.google.api.client.http.HttpStatusCodes; import com.google.common.collect.ImmutableList; -import com.google.common.flogger.FluentLogger; import com.google.gson.Gson; import google.registry.flows.EppException; import google.registry.flows.domain.DomainFlowUtils; @@ -37,7 +37,6 @@ import google.registry.model.domain.RegistryLock; import google.registry.model.registrar.Registrar; import google.registry.model.tld.RegistryLockDao; import google.registry.request.Action; -import google.registry.request.HttpException; import google.registry.request.Parameter; import google.registry.request.Response; import google.registry.request.auth.Auth; @@ -66,8 +65,6 @@ import org.joda.time.Duration; auth = Auth.AUTH_PUBLIC_LOGGED_IN) public class ConsoleRegistryLockAction extends ConsoleApiAction { - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - static final String PATH = "/console-api/registry-lock"; private final DomainLockUtils domainLockUtils; @@ -91,10 +88,7 @@ public class ConsoleRegistryLockAction extends ConsoleApiAction { @Override protected void getHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.REGISTRY_LOCK)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } + checkPermission(user, registrarId, ConsolePermission.REGISTRY_LOCK); consoleApiParams.response().setPayload(gson.toJson(getLockedDomains())); consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_OK); } @@ -104,45 +98,31 @@ public class ConsoleRegistryLockAction extends ConsoleApiAction { HttpServletRequest req = consoleApiParams.request(); Response response = consoleApiParams.response(); // User must have the proper permission on the registrar - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.REGISTRY_LOCK)) { - setFailedResponse("", HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } + checkPermission(user, registrarId, ConsolePermission.REGISTRY_LOCK); // Shouldn't happen, but double-check the registrar has registry lock enabled Registrar registrar = Registrar.loadByRegistrarIdCached(registrarId).get(); - if (!registrar.isRegistryLockAllowed()) { - setFailedResponse( - String.format("Registry lock not allowed for registrar %s", registrarId), - HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } + checkArgument( + registrar.isRegistryLockAllowed(), + "Registry lock not allowed for registrar %s", + registrarId); // Retrieve and validate the necessary params - String domainName; - boolean isLock; - Optional maybePassword; - Optional relockDurationMillis; + String domainName = extractRequiredParameter(req, "domainName"); + boolean isLock = extractBooleanParameter(req, "isLock"); + Optional maybePassword = extractOptionalParameter(req, "password"); + Optional relockDurationMillis = extractOptionalLongParameter(req, "relockDurationMillis"); try { - domainName = extractRequiredParameter(req, "domainName"); - isLock = extractBooleanParameter(req, "isLock"); - maybePassword = extractOptionalParameter(req, "password"); - relockDurationMillis = extractOptionalLongParameter(req, "relockDurationMillis"); DomainFlowUtils.validateDomainName(domainName); - } catch (HttpException.BadRequestException | EppException e) { - logger.atWarning().withCause(e).log("Bad request when attempting registry lock/unlock"); - setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; + } catch (EppException e) { + throw new IllegalArgumentException(e); } // Passwords aren't required for admin users, otherwise we need to validate it boolean isAdmin = user.getUserRoles().isAdmin(); if (!isAdmin) { - if (maybePassword.isEmpty()) { - setFailedResponse("No password provided", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } + checkArgument(maybePassword.isPresent(), "No password provided"); if (!user.verifyRegistryLockPassword(maybePassword.get())) { setFailedResponse( "Incorrect registry lock password", HttpStatusCodes.STATUS_CODE_UNAUTHORIZED); @@ -150,38 +130,23 @@ public class ConsoleRegistryLockAction extends ConsoleApiAction { } } - Optional maybeRegistryLockEmail = user.getRegistryLockEmailAddress(); - if (maybeRegistryLockEmail.isEmpty()) { - setFailedResponse( - "User has no registry lock email address", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } - String registryLockEmail = maybeRegistryLockEmail.get(); - - try { - tm().transact( - () -> { - RegistryLock registryLock = - isLock - ? domainLockUtils.saveNewRegistryLockRequest( - domainName, registrarId, registryLockEmail, isAdmin) - : domainLockUtils.saveNewRegistryUnlockRequest( - domainName, - registrarId, - isAdmin, - relockDurationMillis.map(Duration::new)); - sendVerificationEmail(registryLock, registryLockEmail, isLock); - }); - } catch (IllegalArgumentException e) { - // Catch IllegalArgumentExceptions separately to give a nicer error message and code - logger.atWarning().withCause(e).log("Failed to lock/unlock domain"); - setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } catch (Throwable t) { - logger.atWarning().withCause(t).log("Failed to lock/unlock domain"); - setFailedResponse("Internal server error", HttpStatusCodes.STATUS_CODE_SERVER_ERROR); - return; - } + String registryLockEmail = + user.getRegistryLockEmailAddress() + .orElseThrow( + () -> new IllegalArgumentException("User has no registry lock email address")); + tm().transact( + () -> { + RegistryLock registryLock = + isLock + ? domainLockUtils.saveNewRegistryLockRequest( + domainName, registrarId, registryLockEmail, isAdmin) + : domainLockUtils.saveNewRegistryUnlockRequest( + domainName, + registrarId, + isAdmin, + relockDurationMillis.map(Duration::new)); + sendVerificationEmail(registryLock, registryLockEmail, isLock); + }); response.setStatus(HttpStatusCodes.STATUS_CODE_OK); } diff --git a/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java b/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java index 9a7ca124b..12deffee5 100644 --- a/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java +++ b/core/src/main/java/google/registry/ui/server/console/RegistrarsAction.java @@ -51,10 +51,9 @@ public class RegistrarsAction extends ConsoleApiAction { ImmutableList.of(Registrar.Type.REAL, RegistrarBase.Type.OTE); static final String PATH = "/console-api/registrars"; private final Gson gson; - private Optional registrar; - private StringGenerator passwordGenerator; - private StringGenerator passcodeGenerator; - + private final Optional registrar; + private final StringGenerator passwordGenerator; + private final StringGenerator passcodeGenerator; @Inject public RegistrarsAction( @@ -93,73 +92,62 @@ public class RegistrarsAction extends ConsoleApiAction { return; } - if (registrar.isEmpty()) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - consoleApiParams.response().setPayload(gson.toJson("'registrar' parameter is not present")); - return; - } - - Registrar registrarParam = registrar.get(); + Registrar registrarParam = + registrar.orElseThrow( + () -> new IllegalArgumentException("'registrar' parameter is not present")); String errorMsg = "Missing value for %s"; - try { - checkArgument(!isNullOrEmpty(registrarParam.getRegistrarId()), errorMsg, "registrarId"); - checkArgument(!isNullOrEmpty(registrarParam.getRegistrarName()), errorMsg, "name"); - checkArgument(!registrarParam.getBillingAccountMap().isEmpty(), errorMsg, "billingAccount"); - checkArgument(registrarParam.getIanaIdentifier() != null, String.format(errorMsg, "ianaId")); - checkArgument( - !isNullOrEmpty(registrarParam.getIcannReferralEmail()), errorMsg, "referralEmail"); - checkArgument(!isNullOrEmpty(registrarParam.getDriveFolderId()), errorMsg, "driveId"); - checkArgument(!isNullOrEmpty(registrarParam.getEmailAddress()), errorMsg, "consoleUserEmail"); - checkArgument( - registrarParam.getLocalizedAddress() != null - && !isNullOrEmpty(registrarParam.getLocalizedAddress().getState()) - && !isNullOrEmpty(registrarParam.getLocalizedAddress().getCity()) - && !isNullOrEmpty(registrarParam.getLocalizedAddress().getZip()) - && !isNullOrEmpty(registrarParam.getLocalizedAddress().getCountryCode()) - && !registrarParam.getLocalizedAddress().getStreet().isEmpty(), - errorMsg, - "address"); + checkArgument(!isNullOrEmpty(registrarParam.getRegistrarId()), errorMsg, "registrarId"); + checkArgument(!isNullOrEmpty(registrarParam.getRegistrarName()), errorMsg, "name"); + checkArgument(!registrarParam.getBillingAccountMap().isEmpty(), errorMsg, "billingAccount"); + checkArgument(registrarParam.getIanaIdentifier() != null, String.format(errorMsg, "ianaId")); + checkArgument( + !isNullOrEmpty(registrarParam.getIcannReferralEmail()), errorMsg, "referralEmail"); + checkArgument(!isNullOrEmpty(registrarParam.getDriveFolderId()), errorMsg, "driveId"); + checkArgument(!isNullOrEmpty(registrarParam.getEmailAddress()), errorMsg, "consoleUserEmail"); + checkArgument( + registrarParam.getLocalizedAddress() != null + && !isNullOrEmpty(registrarParam.getLocalizedAddress().getState()) + && !isNullOrEmpty(registrarParam.getLocalizedAddress().getCity()) + && !isNullOrEmpty(registrarParam.getLocalizedAddress().getZip()) + && !isNullOrEmpty(registrarParam.getLocalizedAddress().getCountryCode()) + && !registrarParam.getLocalizedAddress().getStreet().isEmpty(), + errorMsg, + "address"); - String password = passwordGenerator.createString(PASSWORD_LENGTH); - String phonePasscode = passcodeGenerator.createString(PASSCODE_LENGTH); + String password = passwordGenerator.createString(PASSWORD_LENGTH); + String phonePasscode = passcodeGenerator.createString(PASSCODE_LENGTH); - Registrar registrar = - new Registrar.Builder() - .setRegistrarId(registrarParam.getRegistrarId()) - .setRegistrarName(registrarParam.getRegistrarName()) - .setBillingAccountMap(registrarParam.getBillingAccountMap()) - .setIanaIdentifier(Long.valueOf(registrarParam.getIanaIdentifier())) - .setIcannReferralEmail(registrarParam.getIcannReferralEmail()) - .setEmailAddress(registrarParam.getIcannReferralEmail()) - .setDriveFolderId(registrarParam.getDriveFolderId()) - .setType(Registrar.Type.REAL) - .setPassword(password) - .setPhonePasscode(phonePasscode) - .setState(State.PENDING) - .setLocalizedAddress(registrarParam.getLocalizedAddress()) - .build(); + Registrar registrar = + new Registrar.Builder() + .setRegistrarId(registrarParam.getRegistrarId()) + .setRegistrarName(registrarParam.getRegistrarName()) + .setBillingAccountMap(registrarParam.getBillingAccountMap()) + .setIanaIdentifier(registrarParam.getIanaIdentifier()) + .setIcannReferralEmail(registrarParam.getIcannReferralEmail()) + .setEmailAddress(registrarParam.getIcannReferralEmail()) + .setDriveFolderId(registrarParam.getDriveFolderId()) + .setType(Registrar.Type.REAL) + .setPassword(password) + .setPhonePasscode(phonePasscode) + .setState(State.PENDING) + .setLocalizedAddress(registrarParam.getLocalizedAddress()) + .build(); - RegistrarPoc contact = - new RegistrarPoc.Builder() - .setRegistrar(registrar) - .setName(registrarParam.getEmailAddress()) - .setEmailAddress(registrarParam.getEmailAddress()) - .setLoginEmailAddress(registrarParam.getEmailAddress()) - .build(); + RegistrarPoc contact = + new RegistrarPoc.Builder() + .setRegistrar(registrar) + .setName(registrarParam.getEmailAddress()) + .setEmailAddress(registrarParam.getEmailAddress()) + .setLoginEmailAddress(registrarParam.getEmailAddress()) + .build(); - tm().transact( - () -> { - checkArgument( - Registrar.loadByRegistrarId(registrar.getRegistrarId()).isEmpty(), - "Registrar with registrarId %s already exists", - registrar.getRegistrarId()); - tm().putAll(registrar, contact); - }); - - } catch (IllegalArgumentException e) { - setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - } catch (Throwable e) { - setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_SERVER_ERROR); - } + tm().transact( + () -> { + checkArgument( + Registrar.loadByRegistrarId(registrar.getRegistrarId()).isEmpty(), + "Registrar with registrarId %s already exists", + registrar.getRegistrarId()); + tm().putAll(registrar, contact); + }); } } diff --git a/core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java b/core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java index 5a411e456..ef84ff1ac 100644 --- a/core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java +++ b/core/src/main/java/google/registry/ui/server/console/settings/ContactAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console.settings; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableList.toImmutableList; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.GET; @@ -66,11 +67,7 @@ public class ContactAction extends ConsoleApiAction { @Override protected void getHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.VIEW_REGISTRAR_DETAILS)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } - + checkPermission(user, registrarId, ConsolePermission.VIEW_REGISTRAR_DETAILS); ImmutableList am = tm().transact( () -> @@ -87,17 +84,8 @@ public class ContactAction extends ConsoleApiAction { @Override protected void postHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.EDIT_REGISTRAR_DETAILS)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } - - if (contacts.isEmpty()) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - consoleApiParams.response().setPayload(gson.toJson("Contacts parameter is not present")); - return; - } - + checkPermission(user, registrarId, ConsolePermission.EDIT_REGISTRAR_DETAILS); + checkArgument(contacts.isPresent(), "Contacts parameter is not present"); Registrar registrar = Registrar.loadByRegistrarId(registrarId) .orElseThrow( @@ -120,9 +108,7 @@ public class ContactAction extends ConsoleApiAction { } catch (FormException e) { logger.atWarning().withCause(e).log( "Error processing contacts post request for registrar: %s", registrarId); - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - consoleApiParams.response().setPayload(e.getMessage()); - return; + throw new IllegalArgumentException(e); } RegistrarPoc.updateContacts(registrar, updatedContacts); diff --git a/core/src/main/java/google/registry/ui/server/console/settings/SecurityAction.java b/core/src/main/java/google/registry/ui/server/console/settings/SecurityAction.java index 398d5b182..48fb79bdb 100644 --- a/core/src/main/java/google/registry/ui/server/console/settings/SecurityAction.java +++ b/core/src/main/java/google/registry/ui/server/console/settings/SecurityAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console.settings; +import static com.google.common.base.Preconditions.checkArgument; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.POST; @@ -62,16 +63,8 @@ public class SecurityAction extends ConsoleApiAction { @Override protected void postHandler(User user) { - if (!user.getUserRoles().hasPermission(registrarId, ConsolePermission.EDIT_REGISTRAR_DETAILS)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } - - if (registrar.isEmpty()) { - setFailedResponse( - "'registrar' parameter is not present", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } + checkPermission(user, registrarId, ConsolePermission.EDIT_REGISTRAR_DETAILS); + checkArgument(registrar.isPresent(), "'registrar' parameter is not present"); Registrar savedRegistrar; try { diff --git a/core/src/main/java/google/registry/ui/server/console/settings/WhoisRegistrarFieldsAction.java b/core/src/main/java/google/registry/ui/server/console/settings/WhoisRegistrarFieldsAction.java index 8a2eaf95f..cbba5c70f 100644 --- a/core/src/main/java/google/registry/ui/server/console/settings/WhoisRegistrarFieldsAction.java +++ b/core/src/main/java/google/registry/ui/server/console/settings/WhoisRegistrarFieldsAction.java @@ -14,6 +14,7 @@ package google.registry.ui.server.console.settings; +import static com.google.common.base.Preconditions.checkArgument; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.POST; @@ -45,8 +46,8 @@ import javax.inject.Inject; public class WhoisRegistrarFieldsAction extends ConsoleApiAction { static final String PATH = "/console-api/settings/whois-fields"; - private AuthenticatedRegistrarAccessor registrarAccessor; - private Optional registrar; + private final AuthenticatedRegistrarAccessor registrarAccessor; + private final Optional registrar; @Inject public WhoisRegistrarFieldsAction( @@ -60,19 +61,9 @@ public class WhoisRegistrarFieldsAction extends ConsoleApiAction { @Override protected void postHandler(User user) { - if (registrar.isEmpty()) { - setFailedResponse( - "'registrar' parameter is not present", HttpStatusCodes.STATUS_CODE_BAD_REQUEST); - return; - } - - if (!user.getUserRoles() - .hasPermission( - registrar.get().getRegistrarId(), ConsolePermission.EDIT_REGISTRAR_DETAILS)) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - return; - } - + checkArgument(registrar.isPresent(), "'registrar' parameter is not present"); + checkPermission( + user, registrar.get().getRegistrarId(), ConsolePermission.EDIT_REGISTRAR_DETAILS); tm().transact(() -> loadAndModifyRegistrar(registrar.get())); } @@ -82,8 +73,7 @@ public class WhoisRegistrarFieldsAction extends ConsoleApiAction { // reload to make sure the object has all the correct fields savedRegistrar = registrarAccessor.getRegistrar(providedRegistrar.getRegistrarId()); } catch (RegistrarAccessDeniedException e) { - consoleApiParams.response().setStatus(HttpStatusCodes.STATUS_CODE_FORBIDDEN); - consoleApiParams.response().setPayload(e.getMessage()); + setFailedResponse(e.getMessage(), HttpStatusCodes.STATUS_CODE_FORBIDDEN); return; }