From 70273fa791bcc2543c5115a950427b064d171c26 Mon Sep 17 00:00:00 2001 From: guyben Date: Mon, 1 Oct 2018 07:57:29 -0700 Subject: [PATCH] Fix error reply from RegistrarSettingsAction RegistrarSettingsAction is a JSON in / JSON out endpoint, meaning the reply is consumed as JSON. The current state is that if an error occurs, there are two possible replies: - a JSON error reply is sent out, or - a 402 HTML reply is sent out with the exception.getMessage() The difference is only - do we actively catch the exception to translate it to JSON or not. This fix catches ALL exceptions and translates them to JSON format. Note that there's no security change by giving the getMessage in the JSON reply since we were returning that anyway (in the HTML). In addition - changed the "gaeUserId" to "user.getEmail" as the identifier, since it's clearer to the users who see that error - and I do want to transition to a more "email identifier" way of checking access (since that's what users put in the registrar contact info) This too isn't leaking new information because - the initial HTML page load already gives the user's email, and - the logs already log the user's email for every request ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=215213807 --- .../registry/request/auth/AuthResult.java | 2 +- .../registrar/RegistrarSettingsAction.java | 2 +- .../ui/server/registrar/SessionUtils.java | 32 +++++++++++-------- .../RegistrarSettingsActionTest.java | 26 ++++++++------- .../RegistrarSettingsActionTestCase.java | 2 +- .../ui/server/registrar/SessionUtilsTest.java | 14 +++++--- 6 files changed, 45 insertions(+), 33 deletions(-) diff --git a/java/google/registry/request/auth/AuthResult.java b/java/google/registry/request/auth/AuthResult.java index 0903f26a0..24291ee74 100644 --- a/java/google/registry/request/auth/AuthResult.java +++ b/java/google/registry/request/auth/AuthResult.java @@ -38,7 +38,7 @@ public abstract class AuthResult { public String userIdForLogging() { return userAuthInfo() - .map(userAuthInfo -> userAuthInfo.user().getUserId()) + .map(userAuthInfo -> userAuthInfo.user().getEmail()) .orElse(""); } diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index 74f890d4a..39c90aa31 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -123,7 +123,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA logger.atWarning().withCause(e).log( "Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args); return JsonResponseHelper.createFormFieldError(e.getMessage(), e.getFieldName()); - } catch (FormException e) { + } catch (Throwable e) { logger.atWarning().withCause(e).log( "Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args); return JsonResponseHelper.create(ERROR, e.getMessage()); diff --git a/java/google/registry/ui/server/registrar/SessionUtils.java b/java/google/registry/ui/server/registrar/SessionUtils.java index 91a375e03..4b8c13907 100644 --- a/java/google/registry/ui/server/registrar/SessionUtils.java +++ b/java/google/registry/ui/server/registrar/SessionUtils.java @@ -77,7 +77,7 @@ public class SessionUtils { authResult.userAuthInfo().orElseThrow(() -> new ForbiddenException("Not logged in")); boolean isAdmin = userAuthInfo.isUserAdmin(); User user = userAuthInfo.user(); - String gaeUserId = user.getUserId(); + String userIdForLogging = authResult.userIdForLogging(); Registrar registrar = registrarLoader @@ -85,20 +85,20 @@ public class SessionUtils { .orElseThrow( () -> new ForbiddenException(String.format("Registrar %s not found", clientId))); - if (isInAllowedContacts(registrar, gaeUserId)) { - logger.atInfo().log("User %s has access to registrar %s.", gaeUserId, clientId); + if (isInAllowedContacts(registrar, user)) { + logger.atInfo().log("User %s has access to registrar %s.", userIdForLogging, clientId); return registrar; } if (isAdmin && clientId.equals(registryAdminClientId)) { // Admins have access to the registryAdminClientId even if they aren't explicitly in the // allowed contacts - logger.atInfo().log("Allowing admin %s access to registrar %s.", gaeUserId, clientId); + logger.atInfo().log("Allowing admin %s access to registrar %s.", userIdForLogging, clientId); return registrar; } throw new ForbiddenException( - String.format("User %s doesn't have access to registrar %s", gaeUserId, clientId)); + String.format("User %s doesn't have access to registrar %s", userIdForLogging, clientId)); } /** @@ -126,14 +126,19 @@ public class SessionUtils { authResult.userAuthInfo().orElseThrow(() -> new ForbiddenException("No logged in")); boolean isAdmin = userAuthInfo.isUserAdmin(); User user = userAuthInfo.user(); - String gaeUserId = user.getUserId(); + String userIdForLogging = authResult.userIdForLogging(); RegistrarContact contact = - ofy().load().type(RegistrarContact.class).filter("gaeUserId", gaeUserId).first().now(); + ofy() + .load() + .type(RegistrarContact.class) + .filter("gaeUserId", user.getUserId()) + .first() + .now(); if (contact != null) { String registrarClientId = contact.getParent().getName(); logger.atInfo().log( - "Associating user %s with found registrar %s.", gaeUserId, registrarClientId); + "Associating user %s with found registrar %s.", userIdForLogging, registrarClientId); return registrarClientId; } @@ -144,28 +149,29 @@ public class SessionUtils { logger.atInfo().log( "User %s is an admin with no associated registrar." + " Automatically associating the user with configured client Id %s.", - gaeUserId, registryAdminClientId); + userIdForLogging, registryAdminClientId); return registryAdminClientId; } logger.atInfo().log( "Cannot associate admin user %s with configured client Id." + " ClientId is null or empty.", - gaeUserId); + userIdForLogging); } // We couldn't find any relevant clientId throw new ForbiddenException( - String.format("User %s isn't associated with any registrar", gaeUserId)); + String.format("User %s isn't associated with any registrar", userIdForLogging)); } /** - * Returns {@code true} if {@code gaeUserId} is listed in contacts with access to the registrar. + * Returns {@code true} if {@code user} is listed in contacts with access to the registrar. * *

Each registrar contact can either have getGaeUserId equals null or the user's gaeUserId. * Null means the contact doesn't have access to the registrar console. None-null means the * contact has access. */ - private static boolean isInAllowedContacts(Registrar registrar, final String gaeUserId) { + private static boolean isInAllowedContacts(Registrar registrar, User user) { + String gaeUserId = user.getUserId(); return registrar .getContacts() .stream() diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java index 68efd8c58..4ffd12324 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java @@ -17,7 +17,6 @@ package google.registry.ui.server.registrar; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.DatastoreHelper.persistResource; -import static google.registry.testing.JUnitBackports.assertThrows; import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued; import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued; import static google.registry.testing.TestDataHelper.loadFile; @@ -30,7 +29,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import google.registry.export.sheet.SyncRegistrarsSheetAction; import google.registry.model.registrar.Registrar; -import google.registry.request.HttpException.ForbiddenException; import google.registry.testing.CertificateSamples; import google.registry.testing.TaskQueueHelper.TaskMatcher; import google.registry.util.CidrAddressBlock; @@ -84,8 +82,11 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testFailure_readRegistrarInfo_notAuthorized() { action.authResult = USER_UNAUTHORIZED; - assertThrows( - ForbiddenException.class, () -> action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID))); + Map response = action.handleJsonRequest(ImmutableMap.of("id", CLIENT_ID)); + assertThat(response).containsExactly( + "status", "ERROR", + "results", ImmutableList.of(), + "message", "forbidden test error"); assertNoTasksEnqueued("sheet"); } @@ -145,14 +146,15 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase @Test public void testFailute_updateRegistrarInfo_notAuthorized() { action.authResult = USER_UNAUTHORIZED; - assertThrows( - ForbiddenException.class, - () -> - action.handleJsonRequest( - ImmutableMap.of( - "op", "update", - "id", CLIENT_ID, - "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime())))); + Map response = action.handleJsonRequest(ImmutableMap.of( + "op", "update", + "id", CLIENT_ID, + "args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime()))); + assertThat(response).containsExactly( + "status", "ERROR", + "results", ImmutableList.of(), + "message", "forbidden test error"); + assertNoTasksEnqueued("sheet"); } @Test diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 7c4e4c8ca..627c884e8 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -112,6 +112,6 @@ public class RegistrarSettingsActionTestCase { when(sessionUtils.getRegistrarForUser(CLIENT_ID, USER_AUTHORIZED)) .thenAnswer(x -> loadRegistrar(CLIENT_ID)); when(sessionUtils.getRegistrarForUser(CLIENT_ID, USER_UNAUTHORIZED)) - .thenThrow(new ForbiddenException("forbidden")); + .thenThrow(new ForbiddenException("forbidden test error")); } } diff --git a/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java b/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java index 3a5137b1b..22af3a5eb 100644 --- a/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java +++ b/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java @@ -68,8 +68,9 @@ public class SessionUtilsTest { AuthLevel.USER, UserAuthInfo.create( new User( - "user1@google.com", - "google.com", + String.format( + "%s_%s@gmail.com", isAuthorized ? "good" : "evil", isAdmin ? "admin" : "user"), + "gmail.com", isAuthorized ? THE_REGISTRAR_GAE_USER_ID : "badGaeUserId"), isAdmin)); } @@ -211,8 +212,11 @@ public class SessionUtilsTest { .that(testLogHandler) .hasLogAtLevelWithMessage( Level.INFO, - "Cannot associate admin user badGaeUserId with configured client Id." - + " ClientId is null or empty."); + formatMessage( + "Cannot associate admin user {user} with configured client Id." + + " ClientId is null or empty.", + UNAUTHORIZED_ADMIN, + null)); } /** @@ -241,7 +245,7 @@ public class SessionUtilsTest { assertThrows(ForbiddenException.class, () -> sessionUtils.guessClientIdForUser(authResult)); assertThat(exception) .hasMessageThat() - .contains(formatMessage(message, UNAUTHORIZED_USER, null)); + .contains(formatMessage(message, authResult, null)); } @Test