zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-05-17 17:59:41 +02:00
Code Issues Projects Releases Packages Wiki Activity

Fix error reply from RegistrarSettingsAction

Browse source 
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
This commit is contained in:
guyben 2018-10-01 07:57:29 -07:00 • committed by jianglai
parent 2bf06eac77
commit 70273fa791
6 changed files with 45 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

2
java/google/registry/request/auth/AuthResult.java
View file

@ -38,7 +38,7 @@ public abstract class AuthResult {
public String userIdForLogging() {
return userAuthInfo()
.map(userAuthInfo -> userAuthInfo.user().getUserId())
.map(userAuthInfo -> userAuthInfo.user().getEmail())
.orElse("<logged-out user>");
}

2
java/google/registry/ui/server/registrar/RegistrarSettingsAction.java
View file

@ -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());

32
java/google/registry/ui/server/registrar/SessionUtils.java
View file

@ -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.
*
* <p>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()

22
javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTest.java
View file

@ -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<String, Object> 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(
Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
"op", "update",
"id", CLIENT_ID,
"args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime()))));
"args", ImmutableMap.of("lastUpdateTime", getLastUpdateTime())));
assertThat(response).containsExactly(
"status", "ERROR",
"results", ImmutableList.of(),
"message", "forbidden test error");
assertNoTasksEnqueued("sheet");
}
@Test

2
javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java
View file

@ -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"));
}
}

14
javatests/google/registry/ui/server/registrar/SessionUtilsTest.java
View file

@ -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