Clean up registrar console login flow

Replaced the plethora of inter winding access functions and inputs in SessionUtils with just 2 functions, that both accept the same type for the user (AuthResult):

guessRegistrarForUser: given an AuthResult, finds a registrar that they have access to. If none is found - a ForbiddenException is thrown.

getRegistrarForUser[Cached]: (maybe should be called getRegistrarOnBehalfOfUser?) given an AuthResult and a clientId, loads and returns the registrar ONLY IF the user has access to it. Otherwise throws a ForbiddenException.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=214630657
This commit is contained in:
guyben 2018-09-26 10:53:14 -07:00 committed by jianglai
parent 6bddd5a8cb
commit 84a0ace2ea
16 changed files with 431 additions and 523 deletions

View file

@ -16,7 +16,6 @@ package google.registry.rdap;
import static com.google.common.base.Charsets.UTF_8;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN;
import static google.registry.model.ofy.ObjectifyService.ofy;
import static google.registry.util.DateTimeUtils.END_OF_TIME;
@ -57,7 +56,6 @@ import java.util.List;
import java.util.Optional;
import javax.annotation.Nullable;
import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.DateTime;
import org.json.simple.JSONValue;
@ -87,7 +85,6 @@ public abstract class RdapActionBase implements Runnable {
INCLUDE
}
@Inject HttpServletRequest request;
@Inject Response response;
@Inject @RequestMethod Action.Method requestMethod;
@Inject @RequestPath String requestPath;
@ -176,6 +173,23 @@ public abstract class RdapActionBase implements Runnable {
}
}
/**
* Returns a clientId the given user has console access on, or Optional.empty if there is none.
*/
private Optional<String> getAuthorizedClientId(AuthResult authResult) {
try {
String clientId = sessionUtils.guessClientIdForUser(authResult);
// We load the Registrar to make sure the user has access to it. We don't actually need it,
// we're just checking if an exception is thrown.
sessionUtils.getRegistrarForUserCached(clientId, authResult);
return Optional.of(clientId);
} catch (Exception e) {
logger.atWarning().withCause(e).log(
"Couldn't find registrar for User %s.", authResult.userIdForLogging());
return Optional.empty();
}
}
void setPayload(ImmutableMap<String, Object> rdapJson) {
if (requestMethod == Action.Method.HEAD) {
return;
@ -200,15 +214,11 @@ public abstract class RdapActionBase implements Runnable {
if (userAuthInfo.isUserAdmin()) {
return RdapAuthorization.ADMINISTRATOR_AUTHORIZATION;
}
if (!sessionUtils.checkRegistrarConsoleLogin(request, userAuthInfo)) {
Optional<String> clientId = getAuthorizedClientId(authResult);
if (!clientId.isPresent()) {
return RdapAuthorization.PUBLIC_AUTHORIZATION;
}
String clientId = sessionUtils.getRegistrarClientId(request);
Optional<Registrar> registrar = Registrar.loadByClientIdCached(clientId);
if (!registrar.isPresent()) {
return RdapAuthorization.PUBLIC_AUTHORIZATION;
}
return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientId);
return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientId.get());
}
/** Returns the registrar on which results should be filtered, or absent(). */
@ -238,14 +248,9 @@ public abstract class RdapActionBase implements Runnable {
if (userAuthInfo.isUserAdmin()) {
return true;
}
if (!sessionUtils.checkRegistrarConsoleLogin(request, userAuthInfo)) {
if (!getAuthorizedClientId(authResult).isPresent()) {
return false;
}
String clientId = sessionUtils.getRegistrarClientId(request);
checkState(
Registrar.loadByClientIdCached(clientId).isPresent(),
"Registrar with clientId %s doesn't exist",
clientId);
return true;
}

View file

@ -36,6 +36,12 @@ public abstract class AuthResult {
return authLevel() != AuthLevel.NONE;
}
public String userIdForLogging() {
return userAuthInfo()
.map(userAuthInfo -> userAuthInfo.user().getUserId())
.orElse("<logged-out user>");
}
public static AuthResult create(AuthLevel authLevel) {
return new AutoValue_AuthResult(authLevel, Optional.empty());
}

View file

@ -16,14 +16,15 @@ package google.registry.ui.server.registrar;
import static com.google.common.net.HttpHeaders.LOCATION;
import static com.google.common.net.HttpHeaders.X_FRAME_OPTIONS;
import static google.registry.util.PreconditionsUtils.checkArgumentPresent;
import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY;
import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE;
import com.google.appengine.api.users.User;
import com.google.appengine.api.users.UserService;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Supplier;
import com.google.common.flogger.FluentLogger;
import com.google.common.io.Resources;
import com.google.common.net.MediaType;
import com.google.template.soy.data.SoyMapData;
@ -32,10 +33,10 @@ import com.google.template.soy.tofu.SoyTofu;
import google.registry.config.RegistryConfig.Config;
import google.registry.model.registrar.Registrar;
import google.registry.request.Action;
import google.registry.request.HttpException.ForbiddenException;
import google.registry.request.Response;
import google.registry.request.auth.Auth;
import google.registry.request.auth.AuthResult;
import google.registry.request.auth.UserAuthInfo;
import google.registry.security.XsrfTokenManager;
import google.registry.ui.server.SoyTemplateUtils;
import google.registry.ui.soy.registrar.ConsoleSoyInfo;
@ -49,6 +50,8 @@ import javax.servlet.http.HttpServletRequest;
)
public final class ConsoleUiAction implements Runnable {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public static final String PATH = "/registrar";
private static final Supplier<SoyTofu> TOFU_SUPPLIER =
@ -97,7 +100,7 @@ public final class ConsoleUiAction implements Runnable {
response.setHeader(LOCATION, location);
return;
}
UserAuthInfo userAuthInfo = authResult.userAuthInfo().get();
User user = authResult.userAuthInfo().get().user();
response.setContentType(MediaType.HTML_UTF_8);
response.setHeader(X_FRAME_OPTIONS, "SAMEORIGIN"); // Disallow iframing.
response.setHeader("X-Ui-Compatible", "IE=edge"); // Ask IE not to be silly.
@ -119,9 +122,24 @@ public final class ConsoleUiAction implements Runnable {
.render());
return;
}
data.put("username", userAuthInfo.user().getNickname());
data.put("username", user.getNickname());
data.put("logoutUrl", userService.createLogoutURL(PATH));
if (!sessionUtils.checkRegistrarConsoleLogin(req, userAuthInfo)) {
data.put("xsrfToken", xsrfTokenManager.generateToken(user.getEmail()));
try {
String clientId = sessionUtils.guessClientIdForUser(authResult);
data.put("clientId", clientId);
// We want to load the registrar even if we won't use it later (even if we remove the
// requireFeeExtension) - to make sure the user indeed has access to the guessed registrar.
//
// Note that not doing so (and just passing the "clientId" as given) isn't a security issue
// since we double check the access to the registrar on any read / update request. We have to
// - since the access might get revoked between the initial page load and the request! (also
// because the requests come from the browser, and can easily be faked)
Registrar registrar = sessionUtils.getRegistrarForUser(clientId, authResult);
data.put("requireFeeExtension", registrar.getPremiumPriceAckRequired());
} catch (ForbiddenException e) {
logger.atWarning().withCause(e).log(
"User %s doesn't have access to registrar console.", authResult.userIdForLogging());
response.setStatus(SC_FORBIDDEN);
response.setPayload(
TOFU_SUPPLIER.get()
@ -131,13 +149,6 @@ public final class ConsoleUiAction implements Runnable {
.render());
return;
}
String clientId = sessionUtils.getRegistrarClientId(req);
Registrar registrar =
checkArgumentPresent(
Registrar.loadByClientIdCached(clientId), "Registrar %s does not exist", clientId);
data.put("xsrfToken", xsrfTokenManager.generateToken(userAuthInfo.user().getEmail()));
data.put("clientId", clientId);
data.put("requireFeeExtension", registrar.getPremiumPriceAckRequired());
String payload = TOFU_SUPPLIER.get()
.newRenderer(ConsoleSoyInfo.MAIN)

View file

@ -54,7 +54,6 @@ import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;
import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
import org.joda.time.DateTime;
/**
@ -76,7 +75,6 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
static final String ARGS_PARAM = "args";
static final String ID_PARAM = "id";
@Inject HttpServletRequest request;
@Inject JsonActionRunner jsonActionRunner;
@Inject AppEngineServiceUtils appEngineServiceUtils;
@Inject AuthResult authResult;
@ -100,60 +98,51 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
throw new BadRequestException("Malformed JSON");
}
Registrar initialRegistrar = sessionUtils.getRegistrarForAuthResult(request, authResult);
// Check that the clientId requested is the same as the one we get in the
// getRegistrarForAuthResult.
// TODO(b/113925293): remove this check, and instead use the requested clientId to select the
// registrar (in a secure way making sure authResult has access to that registrar!)
String clientId = (String) input.get(ID_PARAM);
if (Strings.isNullOrEmpty(clientId)) {
throw new BadRequestException(String.format("Missing key for resource ID: %s", ID_PARAM));
}
if (!clientId.equals(initialRegistrar.getClientId())) {
throw new BadRequestException(
String.format(
"User's clientId changed from %s to %s. Please reload page",
clientId, initialRegistrar.getClientId()));
}
// Process the operation. Though originally derived from a CRUD
// handler, registrar-settings really only supports read and update.
String op = Optional.ofNullable((String) input.get(OP_PARAM)).orElse("read");
@SuppressWarnings("unchecked")
Map<String, ?> args = (Map<String, Object>)
Optional.<Object>ofNullable(input.get(ARGS_PARAM)).orElse(ImmutableMap.of());
logger.atInfo().log(
"Received request '%s' on registrar '%s' with args %s",
op, initialRegistrar.getClientId(), args);
logger.atInfo().log("Received request '%s' on registrar '%s' with args %s", op, clientId, args);
try {
switch (op) {
case "update":
return update(args, initialRegistrar.getClientId());
return update(args, clientId);
case "read":
return JsonResponseHelper.create(SUCCESS, "Success", initialRegistrar.toJsonMap());
return read(clientId);
default:
return JsonResponseHelper.create(ERROR, "Unknown or unsupported operation: " + op);
}
} catch (FormFieldException e) {
logger.atWarning().withCause(e).log(
"Failed to perform operation '%s' on registrar '%s' for args %s",
op, initialRegistrar.getClientId(), args);
"Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args);
return JsonResponseHelper.createFormFieldError(e.getMessage(), e.getFieldName());
} catch (FormException e) {
logger.atWarning().withCause(e).log(
"Failed to perform operation '%s' on registrar '%s' for args %s",
op, initialRegistrar.getClientId(), args);
"Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args);
return JsonResponseHelper.create(ERROR, e.getMessage());
}
}
Map<String, Object> read(String clientId) {
return JsonResponseHelper.create(
SUCCESS, "Success", sessionUtils.getRegistrarForUser(clientId, authResult).toJsonMap());
}
Map<String, Object> update(final Map<String, ?> args, String clientId) {
return ofy()
.transact(
() -> {
// We load the registrar here rather than use the initialRegistrar above - to make
// We load the registrar here rather than outside of the transaction - to make
// sure we have the latest version. This one is loaded inside the transaction, so it's
// guaranteed to not change before we update it.
Registrar registrar = Registrar.loadByClientId(clientId).get();
Registrar registrar = sessionUtils.getRegistrarForUser(clientId, authResult);
// Verify that the registrar hasn't been changed.
// To do that - we find the latest update time (or null if the registrar has been
// deleted) and compare to the update time from the args. The update time in the args

View file

@ -14,15 +14,11 @@
package google.registry.ui.server.registrar;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Verify.verify;
import static google.registry.model.ofy.ObjectifyService.ofy;
import static google.registry.util.PreconditionsUtils.checkArgumentPresent;
import com.google.appengine.api.users.User;
import com.google.common.base.Strings;
import com.google.common.flogger.FluentLogger;
import com.googlecode.objectify.Key;
import google.registry.config.RegistryConfig.Config;
import google.registry.model.registrar.Registrar;
import google.registry.model.registrar.RegistrarContact;
@ -30,20 +26,16 @@ import google.registry.request.HttpException.ForbiddenException;
import google.registry.request.auth.AuthResult;
import google.registry.request.auth.UserAuthInfo;
import java.util.Optional;
import javax.annotation.CheckReturnValue;
import java.util.function.Function;
import javax.annotation.concurrent.Immutable;
import javax.inject.Inject;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;
/** HTTP session management helper class. */
/** Authenticated Registrar access helper class. */
@Immutable
public class SessionUtils {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private static final String CLIENT_ID_ATTRIBUTE = "clientId";
@Inject
@Config("registryAdminClientId")
String registryAdminClientId;
@ -52,145 +44,118 @@ public class SessionUtils {
public SessionUtils() {}
/**
* Checks that the authentication result indicates a user that has access to the registrar
* console, then gets the associated registrar.
* Loads Registrar on behalf of an authorised user.
*
* <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to use
* the registrar console.
* <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to
* access the requested registrar.
*
* @param clientId ID of the registrar we request
* @param authResult AuthResult of the user on behalf of which we want to access the data
*/
@CheckReturnValue
Registrar getRegistrarForAuthResult(HttpServletRequest request, AuthResult authResult) {
if (!authResult.userAuthInfo().isPresent()) {
throw new ForbiddenException("Not logged in");
}
if (!checkRegistrarConsoleLogin(request, authResult.userAuthInfo().get())) {
throw new ForbiddenException("Not authorized to access Registrar Console");
}
String clientId = getRegistrarClientId(request);
return checkArgumentPresent(
Registrar.loadByClientId(clientId),
"Registrar %s not found",
clientId);
public Registrar getRegistrarForUser(String clientId, AuthResult authResult) {
return getAndAuthorize(Registrar::loadByClientId, clientId, authResult);
}
/**
* Checks that the specified user has access to the Registrar Console.
* Loads a Registrar from the cache on behalf of an authorised user.
*
* <p>This routine will first check the HTTP session (creating one if it doesn't exist) for the
* {@code clientId} attribute:
* <p>Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to
* access the requested registrar.
*
* <ul>
* <li>If it does not exist, then we will attempt to guess the {@link Registrar} with which the
* user is associated. The {@code clientId} of the first matching {@code Registrar} will
* then be stored to the HTTP session.
* <li>If it does exist, then we'll fetch the Registrar from Datastore to make sure access
* wasn't revoked.
* </ul>
*
* <p><b>Note:</b> You must ensure the user has logged in before calling this method.
*
* @return {@code false} if user does not have access, in which case the caller should write an
* error response and abort the request.
* @param clientId ID of the registrar we request
* @param authResult AuthResult of the user on behalf of which we want to access the data
*/
@CheckReturnValue
public boolean checkRegistrarConsoleLogin(HttpServletRequest req, UserAuthInfo userAuthInfo) {
checkState(userAuthInfo != null, "No logged in user found");
public Registrar getRegistrarForUserCached(String clientId, AuthResult authResult) {
return getAndAuthorize(Registrar::loadByClientIdCached, clientId, authResult);
}
Registrar getAndAuthorize(
Function<String, Optional<Registrar>> registrarLoader,
String clientId,
AuthResult authResult) {
UserAuthInfo userAuthInfo =
authResult.userAuthInfo().orElseThrow(() -> new ForbiddenException("Not logged in"));
boolean isAdmin = userAuthInfo.isUserAdmin();
User user = userAuthInfo.user();
HttpSession session = req.getSession();
String clientId = (String) session.getAttribute(CLIENT_ID_ATTRIBUTE);
String gaeUserId = user.getUserId();
// Use the clientId if it exists
if (clientId != null) {
if (!hasAccessToRegistrar(clientId, user.getUserId(), userAuthInfo.isUserAdmin())) {
logger.atInfo().log("Registrar Console access revoked: %s", clientId);
session.invalidate();
return false;
}
logger.atInfo().log(
"Associating user %s with given registrar %s.", user.getUserId(), clientId);
return true;
Registrar registrar =
registrarLoader
.apply(clientId)
.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);
return registrar;
}
// The clientId was null, so let's try and find a registrar this user is associated with
Optional<Registrar> registrar = findRegistrarForUser(user.getUserId());
if (registrar.isPresent()) {
verify(isInAllowedContacts(registrar.get(), user.getUserId()));
logger.atInfo().log(
"Associating user %s with found registrar %s.",
user.getUserId(), registrar.get().getClientId());
session.setAttribute(CLIENT_ID_ATTRIBUTE, registrar.get().getClientId());
return true;
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);
return registrar;
}
// We couldn't guess the registrar, but maybe the user is an admin and we can use the
throw new ForbiddenException(
String.format("User %s doesn't have access to registrar %s", gaeUserId, clientId));
}
/**
* Tries to guess the {@link Registrar} with which the user is associated.
*
* <p>Returns the {@code clientId} of a {@link Registrar} the user has access to (is on the
* contact list). If the user has access to multiple {@link Registrar}s, an arbitrary one is
* selected. If the user is an admin without access to any {@link Registrar}s, {@link
* #registryAdminClientId} is returned if it is defined.
*
* <p>If no {@code clientId} is found, throws a {@link ForbiddenException}.
*
* <p>If you want to load the {@link Registrar} object from this (or any other) {@code clientId},
* in order to perform actions on behalf of a user, you must use {@link #getRegistrarForUser}
* which makes sure the user has permissions.
*
* <p>Note that this is an OPTIONAL step in the authentication - only used if we don't have any
* other clue as to the requested {@code clientId}. It is perfectly OK to get a {@code clientId}
* from any other source, as long as the registrar is then loaded using {@link
* #getRegistrarForUser}.
*/
public String guessClientIdForUser(AuthResult authResult) {
UserAuthInfo userAuthInfo =
authResult.userAuthInfo().orElseThrow(() -> new ForbiddenException("No logged in"));
boolean isAdmin = userAuthInfo.isUserAdmin();
User user = userAuthInfo.user();
String gaeUserId = user.getUserId();
RegistrarContact contact =
ofy().load().type(RegistrarContact.class).filter("gaeUserId", gaeUserId).first().now();
if (contact != null) {
String registrarClientId = contact.getParent().getName();
logger.atInfo().log(
"Associating user %s with found registrar %s.", gaeUserId, registrarClientId);
return registrarClientId;
}
// We couldn't find the registrar, but maybe the user is an admin and we can use the
// registryAdminClientId
if (userAuthInfo.isUserAdmin()) {
if (Strings.isNullOrEmpty(registryAdminClientId)) {
if (isAdmin) {
if (!Strings.isNullOrEmpty(registryAdminClientId)) {
logger.atInfo().log(
"Cannot associate admin user %s with configured client Id."
+ " ClientId is null or empty.",
user.getUserId());
return false;
}
if (!Registrar.loadByClientIdCached(registryAdminClientId).isPresent()) {
logger.atInfo().log(
"Cannot associate admin user %s with configured client Id %s."
+ " Registrar does not exist.",
user.getUserId(), registryAdminClientId);
return false;
"User %s is an admin with no associated registrar."
+ " Automatically associating the user with configured client Id %s.",
gaeUserId, registryAdminClientId);
return registryAdminClientId;
}
logger.atInfo().log(
"User %s is an admin with no associated registrar."
+ " Automatically associating the user with configured client Id %s.",
user.getUserId(), registryAdminClientId);
session.setAttribute(CLIENT_ID_ATTRIBUTE, registryAdminClientId);
return true;
"Cannot associate admin user %s with configured client Id."
+ " ClientId is null or empty.",
gaeUserId);
}
// We couldn't find any relevant clientId
logger.atInfo().log("User not associated with any Registrar: %s", user.getUserId());
return false;
}
/**
* Returns {@link Registrar} clientId associated with HTTP session.
*
* @throws IllegalStateException if you forgot to call {@link #checkRegistrarConsoleLogin}.
*/
@CheckReturnValue
public String getRegistrarClientId(HttpServletRequest req) {
String clientId = (String) req.getSession().getAttribute(CLIENT_ID_ATTRIBUTE);
checkState(clientId != null, "You forgot to call checkRegistrarConsoleLogin()");
return clientId;
}
/** Returns first {@link Registrar} that {@code gaeUserId} is authorized to administer. */
private static Optional<Registrar> findRegistrarForUser(String gaeUserId) {
RegistrarContact contact =
ofy().load().type(RegistrarContact.class).filter("gaeUserId", gaeUserId).first().now();
if (contact == null) {
return Optional.empty();
}
String registrarClientId = contact.getParent().getName();
Optional<Registrar> result = Registrar.loadByClientIdCached(registrarClientId);
if (!result.isPresent()) {
logger.atSevere().log(
"A contact record exists for non-existent registrar: %s.", Key.create(contact));
}
return result;
}
/** @see #isInAllowedContacts(Registrar, String) */
boolean hasAccessToRegistrar(String clientId, String gaeUserId, boolean isAdmin) {
Optional<Registrar> registrar = Registrar.loadByClientIdCached(clientId);
if (!registrar.isPresent()) {
logger.atWarning().log("Registrar '%s' disappeared from Datastore!", clientId);
return false;
}
if (isAdmin && clientId.equals(registryAdminClientId)) {
return true;
}
return isInAllowedContacts(registrar.get(), gaeUserId);
throw new ForbiddenException(
String.format("User %s isn't associated with any registrar", gaeUserId));
}
/**