diff --git a/java/google/registry/rdap/RdapActionBase.java b/java/google/registry/rdap/RdapActionBase.java index d1f448559..270b797d3 100644 --- a/java/google/registry/rdap/RdapActionBase.java +++ b/java/google/registry/rdap/RdapActionBase.java @@ -47,8 +47,8 @@ import google.registry.request.RequestMethod; import google.registry.request.RequestPath; import google.registry.request.Response; import google.registry.request.auth.AuthResult; +import google.registry.request.auth.AuthenticatedRegistrarAccessor; import google.registry.request.auth.UserAuthInfo; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor; import google.registry.util.Clock; import java.io.IOException; import java.net.URI; diff --git a/java/google/registry/request/HttpException.java b/java/google/registry/request/HttpException.java index 8f39fb403..be84d6ec7 100644 --- a/java/google/registry/request/HttpException.java +++ b/java/google/registry/request/HttpException.java @@ -115,6 +115,10 @@ public abstract class HttpException extends RuntimeException { super(HttpServletResponse.SC_FORBIDDEN, message, null); } + public ForbiddenException(String message, Exception cause) { + super(HttpServletResponse.SC_FORBIDDEN, message, cause); + } + @Override public String getResponseCodeString() { return "Forbidden"; diff --git a/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java similarity index 72% rename from java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java rename to java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java index c2d4bd77c..7c62d03c0 100644 --- a/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java +++ b/java/google/registry/request/auth/AuthenticatedRegistrarAccessor.java @@ -12,8 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -package google.registry.ui.server.registrar; +package google.registry.request.auth; +import static com.google.common.base.MoreObjects.toStringHelper; +import static com.google.common.base.Preconditions.checkNotNull; import static google.registry.model.ofy.ObjectifyService.ofy; import com.google.appengine.api.users.User; @@ -27,19 +29,16 @@ import google.registry.config.RegistryConfig.Config; import google.registry.groups.GroupsConnection; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarContact; -import google.registry.request.HttpException.ForbiddenException; -import google.registry.request.auth.AuthResult; -import google.registry.request.auth.UserAuthInfo; import javax.annotation.concurrent.Immutable; import javax.inject.Inject; /** * Allows access only to {@link Registrar}s the current user has access to. * - *

A user has OWNER role on a Registrar if there exists a {@link RegistrarContact} with - * that user's gaeId and the registrar as a parent. + *

A user has OWNER role on a Registrar if there exists a {@link RegistrarContact} with that + * user's gaeId and the registrar as a parent. * - *

An admin has in addition OWNER role on {@link #registryAdminClientId}. + *

An admin has in addition OWNER role on {@code #registryAdminClientId}. * *

An admin also has ADMIN role on ALL registrars. */ @@ -54,13 +53,14 @@ public class AuthenticatedRegistrarAccessor { ADMIN } - AuthResult authResult; - String registryAdminClientId; + private final String userIdForLogging; /** * Gives all roles a user has for a given clientId. * *

The order is significant, with "more specific to this user" coming first. + * + *

Logged out users have an empty roleMap. */ private final ImmutableSetMultimap roleMap; @@ -89,28 +89,42 @@ public class AuthenticatedRegistrarAccessor { overrideGroupsConnection != null ? overrideGroupsConnection : groupsConnection.get()); } + @VisibleForTesting AuthenticatedRegistrarAccessor( AuthResult authResult, - @Config("registryAdminClientId") String registryAdminClientId, - @Config("gSuiteSupportGroupEmailAddress") String gSuiteSupportGroupEmailAddress, + String registryAdminClientId, + String gSuiteSupportGroupEmailAddress, GroupsConnection groupsConnection) { - this.authResult = authResult; - this.registryAdminClientId = registryAdminClientId; - this.roleMap = + this( + authResult.userIdForLogging(), createRoleMap( - authResult, - registryAdminClientId, - groupsConnection, - gSuiteSupportGroupEmailAddress); + authResult, registryAdminClientId, groupsConnection, gSuiteSupportGroupEmailAddress)); logger.atInfo().log( "%s has the following roles: %s", authResult.userIdForLogging(), roleMap); } + private AuthenticatedRegistrarAccessor( + String userIdForLogging, ImmutableSetMultimap roleMap) { + this.userIdForLogging = checkNotNull(userIdForLogging); + this.roleMap = checkNotNull(roleMap); + } + + /** + * Creates a "logged-in user" accessor with a given role map, used for tests. + * + *

The user's "name" in logs and exception messages is "TestUserId". + */ + @VisibleForTesting + public static AuthenticatedRegistrarAccessor createForTesting( + ImmutableSetMultimap roleMap) { + return new AuthenticatedRegistrarAccessor("TestUserId", roleMap); + } + /** * A map that gives all roles a user has for a given clientId. * - *

Throws a {@link ForbiddenException} if the user is not logged in. + *

Throws a {@link RegistrarAccessDeniedException} if the user is not logged in. * *

The result is ordered starting from "most specific to this user". * @@ -129,7 +143,7 @@ public class AuthenticatedRegistrarAccessor { /** * "Guesses" which client ID the user wants from all those they have access to. * - *

If no such ClientIds exist, throws a ForbiddenException. + *

If no such ClientIds exist, throws a RegistrarAccessDeniedException. * *

This should be the ClientId "most likely wanted by the user". * @@ -141,56 +155,59 @@ public class AuthenticatedRegistrarAccessor { * 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 #getRegistrar}. */ - public String guessClientId() { - verifyLoggedIn(); + public String guessClientId() throws RegistrarAccessDeniedException { return getAllClientIdWithRoles().keySet().stream() .findFirst() .orElseThrow( () -> - new ForbiddenException( - String.format( - "%s isn't associated with any registrar", - authResult.userIdForLogging()))); + new RegistrarAccessDeniedException( + String.format("%s isn't associated with any registrar", userIdForLogging))); } /** * Loads a Registrar IFF the user is authorized. * - *

Throws a {@link ForbiddenException} if the user is not logged in, or not authorized to - * access the requested registrar. + *

Throws a {@link RegistrarAccessDeniedException} if the user is not logged in, or not + * authorized to access the requested registrar. * * @param clientId ID of the registrar we request */ - public Registrar getRegistrar(String clientId) { - verifyLoggedIn(); - - ImmutableSet roles = getAllClientIdWithRoles().get(clientId); - - if (roles.isEmpty()) { - throw new ForbiddenException( - String.format( - "%s doesn't have access to registrar %s", - authResult.userIdForLogging(), clientId)); - } + public Registrar getRegistrar(String clientId) throws RegistrarAccessDeniedException { + verifyAccess(clientId); Registrar registrar = Registrar.loadByClientId(clientId) .orElseThrow( - () -> new ForbiddenException(String.format("Registrar %s not found", clientId))); + () -> + new RegistrarAccessDeniedException( + String.format("Registrar %s not found", clientId))); if (!clientId.equals(registrar.getClientId())) { logger.atSevere().log( "registrarLoader.apply(clientId) returned a Registrar with a different clientId. " + "Requested: %s, returned: %s.", clientId, registrar.getClientId()); - throw new ForbiddenException("Internal error - please check logs"); + throw new RegistrarAccessDeniedException("Internal error - please check logs"); } - logger.atInfo().log( - "%s has %s access to registrar %s.", authResult.userIdForLogging(), roles, clientId); return registrar; } + public void verifyAccess(String clientId) throws RegistrarAccessDeniedException { + ImmutableSet roles = getAllClientIdWithRoles().get(clientId); + + if (roles.isEmpty()) { + throw new RegistrarAccessDeniedException( + String.format("%s doesn't have access to registrar %s", userIdForLogging, clientId)); + } + logger.atInfo().log("%s has %s access to registrar %s.", userIdForLogging, roles, clientId); + } + + @Override + public String toString() { + return toStringHelper(getClass()).add("authResult", userIdForLogging).toString(); + } + private static boolean checkIsSupport( GroupsConnection groupsConnection, String userEmail, String supportEmail) { if (Strings.isNullOrEmpty(supportEmail)) { @@ -246,9 +263,10 @@ public class AuthenticatedRegistrarAccessor { return builder.build(); } - private void verifyLoggedIn() { - if (!authResult.userAuthInfo().isPresent()) { - throw new ForbiddenException("Not logged in"); + /** Exception thrown when the current user doesn't have access to the requested Registrar. */ + public static class RegistrarAccessDeniedException extends Exception { + RegistrarAccessDeniedException(String message) { + super(message); } } } diff --git a/java/google/registry/request/auth/BUILD b/java/google/registry/request/auth/BUILD index 445110eb5..ac1ff20de 100644 --- a/java/google/registry/request/auth/BUILD +++ b/java/google/registry/request/auth/BUILD @@ -9,6 +9,8 @@ java_library( srcs = glob(["*.java"]), deps = [ "//java/google/registry/config", + "//java/google/registry/groups", + "//java/google/registry/model", "//java/google/registry/security", "@com_google_appengine_api_1_0_sdk", "@com_google_auto_value", diff --git a/java/google/registry/ui/server/registrar/BUILD b/java/google/registry/ui/server/registrar/BUILD index b9666094c..e815fa505 100644 --- a/java/google/registry/ui/server/registrar/BUILD +++ b/java/google/registry/ui/server/registrar/BUILD @@ -15,7 +15,6 @@ java_library( "//java/google/registry/config", "//java/google/registry/export/sheet", "//java/google/registry/flows", - "//java/google/registry/groups", "//java/google/registry/model", "//java/google/registry/request", "//java/google/registry/request/auth", diff --git a/java/google/registry/ui/server/registrar/ConsoleUiAction.java b/java/google/registry/ui/server/registrar/ConsoleUiAction.java index 34add2820..39668b9f0 100644 --- a/java/google/registry/ui/server/registrar/ConsoleUiAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleUiAction.java @@ -16,7 +16,7 @@ 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.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.ADMIN; import static google.registry.ui.server.registrar.RegistrarConsoleModule.PARAM_CLIENT_ID; import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY; @@ -36,14 +36,15 @@ 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.Parameter; import google.registry.request.Response; import google.registry.request.auth.Auth; import google.registry.request.auth.AuthResult; +import google.registry.request.auth.AuthenticatedRegistrarAccessor; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.Role; import google.registry.security.XsrfTokenManager; import google.registry.ui.server.SoyTemplateUtils; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role; import google.registry.ui.soy.registrar.ConsoleSoyInfo; import java.util.Optional; import javax.inject.Inject; @@ -151,7 +152,7 @@ public final class ConsoleUiAction implements Runnable { // because the requests come from the browser, and can easily be faked) Registrar registrar = registrarAccessor.getRegistrar(clientId); data.put("requireFeeExtension", registrar.getPremiumPriceAckRequired()); - } catch (ForbiddenException e) { + } catch (RegistrarAccessDeniedException e) { logger.atWarning().withCause(e).log( "User %s doesn't have access to registrar console.", authResult.userIdForLogging()); response.setStatus(SC_FORBIDDEN); diff --git a/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java b/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java index 338a595e4..e1993bb1c 100644 --- a/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java +++ b/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java @@ -18,7 +18,7 @@ import com.google.common.collect.ImmutableSet; import com.google.monitoring.metrics.IncrementableMetric; import com.google.monitoring.metrics.LabelDescriptor; import com.google.monitoring.metrics.MetricRegistryImpl; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.Role; import javax.inject.Inject; final class RegistrarConsoleMetrics { diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index 412d03fa1..90d2dbe47 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -42,11 +42,13 @@ import google.registry.request.HttpException.ForbiddenException; import google.registry.request.JsonActionRunner; import google.registry.request.auth.Auth; import google.registry.request.auth.AuthResult; +import google.registry.request.auth.AuthenticatedRegistrarAccessor; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.Role; import google.registry.security.JsonResponseHelper; import google.registry.ui.forms.FormException; import google.registry.ui.forms.FormFieldException; import google.registry.ui.server.RegistrarFormFields; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role; import google.registry.util.AppEngineServiceUtils; import google.registry.util.CollectionUtils; import google.registry.util.DiffUtils; @@ -161,7 +163,11 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA } private RegistrarResult read(String clientId) { - return RegistrarResult.create("Success", registrarAccessor.getRegistrar(clientId)); + try { + return RegistrarResult.create("Success", registrarAccessor.getRegistrar(clientId)); + } catch (RegistrarAccessDeniedException e) { + throw new ForbiddenException(e.getMessage(), e); + } } private RegistrarResult update(final Map args, String clientId) { @@ -171,7 +177,12 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA // 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 = registrarAccessor.getRegistrar(clientId); + Registrar registrar; + try { + registrar = registrarAccessor.getRegistrar(clientId); + } catch (RegistrarAccessDeniedException e) { + throw new ForbiddenException(e.getMessage(), e); + } // 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 diff --git a/javatests/google/registry/rdap/RdapActionBaseTestCase.java b/javatests/google/registry/rdap/RdapActionBaseTestCase.java index 9ddb7f35f..e3578462c 100644 --- a/javatests/google/registry/rdap/RdapActionBaseTestCase.java +++ b/javatests/google/registry/rdap/RdapActionBaseTestCase.java @@ -19,7 +19,7 @@ import static google.registry.rdap.RdapAuthorization.Role.PUBLIC; import static google.registry.rdap.RdapAuthorization.Role.REGISTRAR; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.OWNER; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -29,12 +29,12 @@ import google.registry.model.ofy.Ofy; import google.registry.request.Action; import google.registry.request.auth.AuthLevel; import google.registry.request.auth.AuthResult; +import google.registry.request.auth.AuthenticatedRegistrarAccessor; import google.registry.request.auth.UserAuthInfo; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; import google.registry.testing.FakeResponse; import google.registry.testing.InjectRule; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor; import google.registry.util.TypeUtils; import java.util.Optional; import org.joda.time.DateTime; diff --git a/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java b/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java similarity index 82% rename from javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java rename to javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java index 8c6341d04..115aee1d8 100644 --- a/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java +++ b/javatests/google/registry/request/auth/AuthenticatedRegistrarAccessorTest.java @@ -12,16 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -package google.registry.ui.server.registrar; +package google.registry.request.auth; import static com.google.common.truth.Truth.assertThat; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.OWNER; import static google.registry.testing.AppEngineRule.THE_REGISTRAR_GAE_USER_ID; 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.LogsSubject.assertAboutLogs; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -33,10 +33,7 @@ import com.google.common.flogger.LoggerConfig; import com.google.common.testing.NullPointerTester; import com.google.common.testing.TestLogHandler; import google.registry.groups.GroupsConnection; -import google.registry.request.HttpException.ForbiddenException; -import google.registry.request.auth.AuthLevel; -import google.registry.request.auth.AuthResult; -import google.registry.request.auth.UserAuthInfo; +import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException; import google.registry.testing.AppEngineRule; import google.registry.testing.InjectRule; import java.util.logging.Level; @@ -76,7 +73,8 @@ public class AuthenticatedRegistrarAccessorTest { UserAuthInfo.create( new User( String.format( - "%s_%s@gmail.com", isAuthorized ? "good" : "evil", isAdmin ? "admin" : "user"), + "%s_%s@gmail.com", + isAuthorized ? "auth" : "unauth", isAdmin ? "admin" : "user"), "gmail.com", isAuthorized ? THE_REGISTRAR_GAE_USER_ID : "badGaeUserId"), isAdmin)); @@ -94,12 +92,6 @@ public class AuthenticatedRegistrarAccessorTest { LoggerConfig.getConfig(AuthenticatedRegistrarAccessor.class).removeHandler(testLogHandler); } - private String formatMessage(String message, AuthResult authResult, String clientId) { - return message - .replace("{user}", authResult.userIdForLogging()) - .replace("{clientId}", String.valueOf(clientId)); - } - /** Users only have access to the registrars they are a contact for. */ @Test public void getAllClientIdWithAccess_authorizedUser() { @@ -114,7 +106,7 @@ public class AuthenticatedRegistrarAccessorTest { /** Users in support group have admin access to everything. */ @Test public void getAllClientIdWithAccess_authorizedUser_isSupportGroup() { - when(groupsConnection.isMemberOfGroup("good_user@gmail.com", SUPPORT_GROUP)).thenReturn(true); + when(groupsConnection.isMemberOfGroup("auth_user@gmail.com", SUPPORT_GROUP)).thenReturn(true); AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); @@ -149,7 +141,7 @@ public class AuthenticatedRegistrarAccessorTest { /** Unauthorized users who are in support group have admin access. */ @Test public void getAllClientIdWithAccess_unauthorizedUser_inSupportGroup() { - when(groupsConnection.isMemberOfGroup("evil_user@gmail.com", SUPPORT_GROUP)).thenReturn(true); + when(groupsConnection.isMemberOfGroup("unauth_user@gmail.com", SUPPORT_GROUP)).thenReturn(true); AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( UNAUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); @@ -179,7 +171,7 @@ public class AuthenticatedRegistrarAccessorTest { new AuthenticatedRegistrarAccessor( AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); - verify(groupsConnection).isMemberOfGroup("good_user@gmail.com", SUPPORT_GROUP); + verify(groupsConnection).isMemberOfGroup("auth_user@gmail.com", SUPPORT_GROUP); assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly(DEFAULT_CLIENT_ID, OWNER); } @@ -223,34 +215,39 @@ public class AuthenticatedRegistrarAccessorTest { expectGetRegistrarFailure( DEFAULT_CLIENT_ID, UNAUTHORIZED_USER, - "{user} doesn't have access to registrar {clientId}"); + "user unauth_user@gmail.com doesn't have access to registrar TheRegistrar"); } /** Fail loading registrar if there's no user associated with the request. */ @Test public void testGetRegistrarForUser_noUser() { - expectGetRegistrarFailure(DEFAULT_CLIENT_ID, NO_USER, "Not logged in"); + expectGetRegistrarFailure( + DEFAULT_CLIENT_ID, + NO_USER, + " doesn't have access to registrar TheRegistrar"); } /** Succeed loading registrar if user has access to it. */ @Test - public void testGetRegistrarForUser_hasAccess_isNotAdmin() { + public void testGetRegistrarForUser_hasAccess_isNotAdmin() throws Exception { expectGetRegistrarSuccess( - AUTHORIZED_USER, "{user} has [OWNER] access to registrar {clientId}"); + AUTHORIZED_USER, "user auth_user@gmail.com has [OWNER] access to registrar TheRegistrar"); } /** Succeed loading registrar if admin with access. */ @Test - public void testGetRegistrarForUser_hasAccess_isAdmin() { + public void testGetRegistrarForUser_hasAccess_isAdmin() throws Exception { expectGetRegistrarSuccess( - AUTHORIZED_ADMIN, "{user} has [OWNER, ADMIN] access to registrar {clientId}"); + AUTHORIZED_ADMIN, + "admin auth_admin@gmail.com has [OWNER, ADMIN] access to registrar TheRegistrar"); } /** Succeed loading registrar for admin even if they aren't on the approved contacts list. */ @Test - public void testGetRegistrarForUser_noAccess_isAdmin() { + public void testGetRegistrarForUser_noAccess_isAdmin() throws Exception { expectGetRegistrarSuccess( - UNAUTHORIZED_ADMIN, "{user} has [ADMIN] access to registrar {clientId}."); + UNAUTHORIZED_ADMIN, + "admin unauth_admin@gmail.com has [ADMIN] access to registrar TheRegistrar."); } /** Fail loading registrar even if admin, if registrar doesn't exist. */ @@ -259,20 +256,17 @@ public class AuthenticatedRegistrarAccessorTest { expectGetRegistrarFailure( "BadClientId", AUTHORIZED_ADMIN, - "{user} doesn't have access to registrar {clientId}"); + "admin auth_admin@gmail.com doesn't have access to registrar BadClientId"); } - private void expectGetRegistrarSuccess( - AuthResult authResult, String message) { + private void expectGetRegistrarSuccess(AuthResult authResult, String message) throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); + // make sure loading the registrar succeeds and returns a value assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID)).isNotNull(); - assertAboutLogs() - .that(testLogHandler) - .hasLogAtLevelWithMessage( - Level.INFO, formatMessage(message, authResult, DEFAULT_CLIENT_ID)); + assertAboutLogs().that(testLogHandler).hasLogAtLevelWithMessage(Level.INFO, message); } private void expectGetRegistrarFailure( @@ -281,16 +275,17 @@ public class AuthenticatedRegistrarAccessorTest { new AuthenticatedRegistrarAccessor( authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); - ForbiddenException exception = + // make sure getRegistrar fails + RegistrarAccessDeniedException exception = assertThrows( - ForbiddenException.class, () -> registrarAccessor.getRegistrar(clientId)); + RegistrarAccessDeniedException.class, () -> registrarAccessor.getRegistrar(clientId)); - assertThat(exception).hasMessageThat().contains(formatMessage(message, authResult, clientId)); + assertThat(exception).hasMessageThat().contains(message); } /** If a user has access to a registrar, we should guess that registrar. */ @Test - public void testGuessClientIdForUser_hasAccess_isNotAdmin() { + public void testGuessClientIdForUser_hasAccess_isNotAdmin() throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( AUTHORIZED_USER, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); @@ -301,7 +296,8 @@ public class AuthenticatedRegistrarAccessorTest { /** If a user doesn't have access to any registrars, guess returns nothing. */ @Test public void testGuessClientIdForUser_noAccess_isNotAdmin() { - expectGuessRegistrarFailure(UNAUTHORIZED_USER, "{user} isn't associated with any registrar"); + expectGuessRegistrarFailure( + UNAUTHORIZED_USER, "user unauth_user@gmail.com isn't associated with any registrar"); } /** @@ -309,7 +305,7 @@ public class AuthenticatedRegistrarAccessorTest { * ADMIN_CLIENT_ID). */ @Test - public void testGuessClientIdForUser_hasAccess_isAdmin() { + public void testGuessClientIdForUser_hasAccess_isAdmin() throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( AUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); @@ -319,7 +315,7 @@ public class AuthenticatedRegistrarAccessorTest { /** If an admin doesn't have access to a registrar, we should guess the ADMIN_CLIENT_ID. */ @Test - public void testGuessClientIdForUser_noAccess_isAdmin() { + public void testGuessClientIdForUser_noAccess_isAdmin() throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( UNAUTHORIZED_ADMIN, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); @@ -333,7 +329,7 @@ public class AuthenticatedRegistrarAccessorTest { * registrars. */ @Test - public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdEmpty() { + public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdEmpty() throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(UNAUTHORIZED_ADMIN, "", SUPPORT_GROUP, groupsConnection); @@ -345,7 +341,7 @@ public class AuthenticatedRegistrarAccessorTest { * non-existent registrar, we still guess it (we will later fail loading the registrar). */ @Test - public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdInvalid() { + public void testGuessClientIdForUser_noAccess_isAdmin_adminClientIdInvalid() throws Exception { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor( UNAUTHORIZED_ADMIN, "NonexistentRegistrar", SUPPORT_GROUP, groupsConnection); @@ -358,11 +354,9 @@ public class AuthenticatedRegistrarAccessorTest { new AuthenticatedRegistrarAccessor( authResult, ADMIN_CLIENT_ID, SUPPORT_GROUP, groupsConnection); - ForbiddenException exception = - assertThrows(ForbiddenException.class, () -> registrarAccessor.guessClientId()); - assertThat(exception) - .hasMessageThat() - .contains(formatMessage(message, authResult, null)); + RegistrarAccessDeniedException exception = + assertThrows(RegistrarAccessDeniedException.class, () -> registrarAccessor.guessClientId()); + assertThat(exception).hasMessageThat().contains(message); } @Test diff --git a/javatests/google/registry/request/auth/BUILD b/javatests/google/registry/request/auth/BUILD index 7f3fe6645..cda5fe006 100644 --- a/javatests/google/registry/request/auth/BUILD +++ b/javatests/google/registry/request/auth/BUILD @@ -12,6 +12,7 @@ java_library( srcs = glob(["*.java"]), resources = glob(["testdata/*"]), deps = [ + "//java/google/registry/groups", "//java/google/registry/request/auth", "//java/google/registry/security", "//javatests/google/registry/testing", @@ -19,7 +20,10 @@ java_library( "@com_google_appengine_api_1_0_sdk", "@com_google_appengine_tools_appengine_gcs_client", "@com_google_appengine_tools_sdk", + "@com_google_flogger", + "@com_google_flogger_system_backend", "@com_google_guava", + "@com_google_guava_testlib", "@com_google_truth", "@com_google_truth_extensions_truth_java8_extension", "@javax_servlet_api", diff --git a/javatests/google/registry/ui/server/BUILD b/javatests/google/registry/ui/server/BUILD index b8b408db4..a9779cfb7 100644 --- a/javatests/google/registry/ui/server/BUILD +++ b/javatests/google/registry/ui/server/BUILD @@ -14,6 +14,7 @@ java_library( "//java/google/registry/ui/forms", "//java/google/registry/ui/server", "//javatests/google/registry/testing", + "@com_google_guava", "@com_google_truth", "@com_google_truth_extensions_truth_java8_extension", "@junit", diff --git a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java index 5cfb05ac9..e4147462a 100644 --- a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java +++ b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java @@ -17,9 +17,8 @@ package google.registry.ui.server.registrar; import static com.google.common.net.HttpHeaders.LOCATION; import static com.google.common.truth.Truth.assertThat; import static com.google.monitoring.metrics.contrib.LongMetricSubject.assertThat; -import static google.registry.testing.DatastoreHelper.loadRegistrar; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.OWNER; import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -28,9 +27,9 @@ import com.google.appengine.api.users.User; import com.google.appengine.api.users.UserServiceFactory; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.net.MediaType; -import google.registry.request.HttpException.ForbiddenException; import google.registry.request.auth.AuthLevel; import google.registry.request.auth.AuthResult; +import google.registry.request.auth.AuthenticatedRegistrarAccessor; import google.registry.request.auth.UserAuthInfo; import google.registry.security.XsrfTokenManager; import google.registry.testing.AppEngineRule; @@ -56,8 +55,6 @@ public class ConsoleUiActionTest { .withUserService(UserInfo.create("marla.singer@example.com", "12345")) .build(); - private final AuthenticatedRegistrarAccessor registrarAccessor = - mock(AuthenticatedRegistrarAccessor.class); private final HttpServletRequest request = mock(HttpServletRequest.class); private final FakeResponse response = new FakeResponse(); private final ConsoleUiAction action = new ConsoleUiAction(); @@ -76,24 +73,19 @@ public class ConsoleUiActionTest { action.req = request; action.response = response; action.registrarConsoleMetrics = new RegistrarConsoleMetrics(); - action.registrarAccessor = registrarAccessor; action.userService = UserServiceFactory.getUserService(); action.xsrfTokenManager = new XsrfTokenManager(new FakeClock(), action.userService); action.paramClientId = Optional.empty(); AuthResult authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false)); action.authResult = authResult; - when(registrarAccessor.getRegistrar("TheRegistrar")) - .thenReturn(loadRegistrar("TheRegistrar")); - when(registrarAccessor.getAllClientIdWithRoles()) - .thenReturn( + + action.registrarAccessor = + AuthenticatedRegistrarAccessor.createForTesting( ImmutableSetMultimap.of( "TheRegistrar", OWNER, - "OtherRegistrar", OWNER, - "OtherRegistrar", ADMIN, + "NewRegistrar", OWNER, + "NewRegistrar", ADMIN, "AdminRegistrar", ADMIN)); - when(registrarAccessor.guessClientId()).thenCallRealMethod(); - // Used for error message in guessClientId - registrarAccessor.authResult = authResult; RegistrarConsoleMetrics.consoleRequestMetric.reset(); } @@ -146,7 +138,8 @@ public class ConsoleUiActionTest { @Test public void testUserDoesntHaveAccessToAnyRegistrar_showsWhoAreYouPage() { - when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of()); + action.registrarAccessor = + AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of()); action.run(); assertThat(response.getPayload()).contains("

You need permission

"); assertThat(response.getPayload()).contains("not associated with Nomulus."); @@ -175,8 +168,6 @@ public class ConsoleUiActionTest { public void testSettingClientId_notAllowed_showsNeedPermissionPage() { // Behaves the same way if fakeRegistrar exists, but we don't have access to it action.paramClientId = Optional.of("fakeRegistrar"); - when(registrarAccessor.getRegistrar("fakeRegistrar")) - .thenThrow(new ForbiddenException("forbidden")); action.run(); assertThat(response.getPayload()).contains("

You need permission

"); assertThat(response.getPayload()).contains("not associated with the registrar fakeRegistrar."); @@ -185,20 +176,18 @@ public class ConsoleUiActionTest { @Test public void testSettingClientId_allowed_showsRegistrarConsole() { - action.paramClientId = Optional.of("OtherRegistrar"); - when(registrarAccessor.getRegistrar("OtherRegistrar")) - .thenReturn(loadRegistrar("TheRegistrar")); + action.paramClientId = Optional.of("NewRegistrar"); action.run(); assertThat(response.getPayload()).contains("Registrar Console"); assertThat(response.getPayload()).contains("reg-content-and-footer"); - assertMetric("OtherRegistrar", "true", "[OWNER, ADMIN]", "SUCCESS"); + assertMetric("NewRegistrar", "true", "[OWNER, ADMIN]", "SUCCESS"); } @Test public void testUserHasAccessAsTheRegistrar_showsClientIdChooser() { action.run(); assertThat(response.getPayload()).contains("