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