From d2ca67460ce305f61ba667e18b44b7ed9b7c94ae Mon Sep 17 00:00:00 2001 From: guyben Date: Mon, 22 Oct 2018 08:11:10 -0700 Subject: [PATCH] Allow admins read/write access to all registrar in web console This CL removes the "READ vs UPDATE" feature completely. Now anyone with access has full read+write access. We still keep track of which role a user has (did they get access "explicitly" because they are an "allowed access" contact? Or do they have access because they are admins?) for the logs and UI, and also so we could in the (very near) future have features only available to admins. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=218169608 --- java/google/registry/rdap/RdapActionBase.java | 28 +--- .../registry/rdap/RdapAuthorization.java | 13 +- .../AuthenticatedRegistrarAccessor.java | 110 ++++++--------- .../ui/server/registrar/ConsoleUiAction.java | 18 +-- .../registrar/RegistrarSettingsAction.java | 6 +- .../registry/ui/soy/registrar/Console.soy | 20 +-- .../registry/rdap/RdapActionBaseTestCase.java | 14 +- .../ui/js/registrar/console_test_util.js | 4 +- .../AuthenticatedRegistrarAccessorTest.java | 129 +++++------------- .../server/registrar/ConsoleUiActionTest.java | 27 ++-- .../RegistrarSettingsActionTest.java | 26 ---- .../RegistrarSettingsActionTestCase.java | 31 +---- 12 files changed, 138 insertions(+), 288 deletions(-) diff --git a/java/google/registry/rdap/RdapActionBase.java b/java/google/registry/rdap/RdapActionBase.java index b3eb48001..d1f448559 100644 --- a/java/google/registry/rdap/RdapActionBase.java +++ b/java/google/registry/rdap/RdapActionBase.java @@ -18,7 +18,6 @@ import static com.google.common.base.Charsets.UTF_8; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DomainNameUtils.canonicalizeDomainName; import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; @@ -27,6 +26,7 @@ import static javax.servlet.http.HttpServletResponse.SC_OK; import com.google.api.client.json.jackson2.JacksonFactory; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; import com.google.common.net.MediaType; import com.google.re2j.Pattern; @@ -176,23 +176,6 @@ 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 getAuthorizedClientId() { - try { - String clientId = registrarAccessor.guessClientId(); - // 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. - registrarAccessor.getRegistrarForUserCached(clientId, READ); - 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 rdapJson) { if (requestMethod == Action.Method.HEAD) { return; @@ -217,11 +200,12 @@ public abstract class RdapActionBase implements Runnable { if (userAuthInfo.isUserAdmin()) { return RdapAuthorization.ADMINISTRATOR_AUTHORIZATION; } - Optional clientId = getAuthorizedClientId(); - if (!clientId.isPresent()) { + ImmutableSet clientIds = registrarAccessor.getAllClientIdWithRoles().keySet(); + if (clientIds.isEmpty()) { + logger.atWarning().log("Couldn't find registrar for User %s.", authResult.userIdForLogging()); return RdapAuthorization.PUBLIC_AUTHORIZATION; } - return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientId.get()); + return RdapAuthorization.create(RdapAuthorization.Role.REGISTRAR, clientIds); } /** Returns the registrar on which results should be filtered, or absent(). */ @@ -251,7 +235,7 @@ public abstract class RdapActionBase implements Runnable { if (userAuthInfo.isUserAdmin()) { return true; } - if (!getAuthorizedClientId().isPresent()) { + if (registrarAccessor.getAllClientIdWithRoles().isEmpty()) { return false; } return true; diff --git a/java/google/registry/rdap/RdapAuthorization.java b/java/google/registry/rdap/RdapAuthorization.java index b21c11e6b..991cf12a6 100644 --- a/java/google/registry/rdap/RdapAuthorization.java +++ b/java/google/registry/rdap/RdapAuthorization.java @@ -15,7 +15,7 @@ package google.registry.rdap; import com.google.auto.value.AutoValue; -import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import google.registry.model.ImmutableObject; /** Authorization information for RDAP data access. */ @@ -32,13 +32,13 @@ public abstract class RdapAuthorization extends ImmutableObject { public abstract Role role(); /** The registrar client IDs for which access is granted (used only if the role is REGISTRAR. */ - public abstract ImmutableList clientIds(); + public abstract ImmutableSet clientIds(); static RdapAuthorization create(Role role, String clientId) { - return new AutoValue_RdapAuthorization(role, ImmutableList.of(clientId)); + return new AutoValue_RdapAuthorization(role, ImmutableSet.of(clientId)); } - static RdapAuthorization create(Role role, ImmutableList clientIds) { + static RdapAuthorization create(Role role, ImmutableSet clientIds) { return new AutoValue_RdapAuthorization(role, clientIds); } @@ -54,9 +54,8 @@ public abstract class RdapAuthorization extends ImmutableObject { } public static final RdapAuthorization PUBLIC_AUTHORIZATION = - create(Role.PUBLIC, ImmutableList.of()); + create(Role.PUBLIC, ImmutableSet.of()); public static final RdapAuthorization ADMINISTRATOR_AUTHORIZATION = - create(Role.ADMINISTRATOR, ImmutableList.of()); + create(Role.ADMINISTRATOR, ImmutableSet.of()); } - diff --git a/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java b/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java index 44334676d..f1a531d93 100644 --- a/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java +++ b/java/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessor.java @@ -18,6 +18,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy; import com.google.appengine.api.users.User; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.flogger.FluentLogger; import google.registry.config.RegistryConfig.Config; @@ -26,83 +27,53 @@ 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 java.util.Optional; -import java.util.function.Function; 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 read+write access to a Registrar if there exists a {@link RegistrarContact} with + *

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 read+write access to {@link #registryAdminClientId}. + *

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

An admin also has read access to ALL registrars. + *

An admin also has ADMIN role on ALL registrars. */ @Immutable public class AuthenticatedRegistrarAccessor { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - /** Type of access we're requesting. */ - public enum AccessType { - READ, - UPDATE + /** The role under which access is granted. */ + public enum Role { + OWNER, + ADMIN } AuthResult authResult; String registryAdminClientId; /** - * For any AccessType requested - all clientIDs this user is allowed that AccessType on. + * Gives all roles a user has for a given clientId. * *

The order is significant, with "more specific to this user" coming first. */ - private final ImmutableSetMultimap accessMap; + private final ImmutableSetMultimap roleMap; @Inject public AuthenticatedRegistrarAccessor( AuthResult authResult, @Config("registryAdminClientId") String registryAdminClientId) { this.authResult = authResult; this.registryAdminClientId = registryAdminClientId; - this.accessMap = createAccessMap(authResult, registryAdminClientId); + this.roleMap = createRoleMap(authResult, registryAdminClientId); logger.atInfo().log( - "User %s has the following accesses: %s", authResult.userIdForLogging(), accessMap); + "%s has the following roles: %s", authResult.userIdForLogging(), roleMap); } /** - * 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. - * - * @param clientId ID of the registrar we request - * @param accessType what kind of access do we want for this registrar - just read it or write as - * well? (different users might have different access levels) - */ - public Registrar getRegistrar(String clientId, AccessType accessType) { - return getAndAuthorize(Registrar::loadByClientId, clientId, accessType); - } - - /** - * Loads a Registrar from the cache IFF the user is authorized. - * - *

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 accessType what kind of access do we want for this registrar - just read it or write as - * well? (different users might have different access levels) - */ - public Registrar getRegistrarForUserCached(String clientId, AccessType accessType) { - return getAndAuthorize(Registrar::loadByClientIdCached, clientId, accessType); - } - - /** - * For all {@link AccessType}s, Returns all ClientIds this user is allowed this access. + * A map that gives all roles a user has for a given clientId. * *

Throws a {@link ForbiddenException} if the user is not logged in. * @@ -116,9 +87,8 @@ 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 ImmutableSetMultimap getAllClientIdWithAccess() { - verifyLoggedIn(); - return accessMap; + public ImmutableSetMultimap getAllClientIdWithRoles() { + return roleMap; } /** @@ -138,32 +108,38 @@ public class AuthenticatedRegistrarAccessor { */ public String guessClientId() { verifyLoggedIn(); - return getAllClientIdWithAccess().values().stream() + return getAllClientIdWithRoles().keySet().stream() .findFirst() .orElseThrow( () -> new ForbiddenException( String.format( - "User %s isn't associated with any registrar", + "%s isn't associated with any registrar", authResult.userIdForLogging()))); } - private Registrar getAndAuthorize( - Function> registrarLoader, - String clientId, - AccessType accessType) { + /** + * 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. + * + * @param clientId ID of the registrar we request + */ + public Registrar getRegistrar(String clientId) { verifyLoggedIn(); - if (!accessMap.containsEntry(accessType, clientId)) { + ImmutableSet roles = getAllClientIdWithRoles().get(clientId); + + if (roles.isEmpty()) { throw new ForbiddenException( String.format( - "User %s doesn't have %s access to registrar %s", - authResult.userIdForLogging(), accessType, clientId)); + "%s doesn't have access to registrar %s", + authResult.userIdForLogging(), clientId)); } Registrar registrar = - registrarLoader - .apply(clientId) + Registrar.loadByClientId(clientId) .orElseThrow( () -> new ForbiddenException(String.format("Registrar %s not found", clientId))); @@ -176,12 +152,11 @@ public class AuthenticatedRegistrarAccessor { } logger.atInfo().log( - "User %s has %s access to registrar %s.", - authResult.userIdForLogging(), accessType, clientId); + "%s has %s access to registrar %s.", authResult.userIdForLogging(), roles, clientId); return registrar; } - private static ImmutableSetMultimap createAccessMap( + private static ImmutableSetMultimap createRoleMap( AuthResult authResult, String registryAdminClientId) { if (!authResult.userAuthInfo().isPresent()) { @@ -193,7 +168,7 @@ public class AuthenticatedRegistrarAccessor { boolean isAdmin = userAuthInfo.isUserAdmin(); User user = userAuthInfo.user(); - ImmutableSetMultimap.Builder builder = new ImmutableSetMultimap.Builder<>(); + ImmutableSetMultimap.Builder builder = new ImmutableSetMultimap.Builder<>(); ofy() .load() @@ -202,25 +177,18 @@ public class AuthenticatedRegistrarAccessor { .forEach( contact -> builder - .put(AccessType.UPDATE, contact.getParent().getName()) - .put(AccessType.READ, contact.getParent().getName())); + .put(contact.getParent().getName(), Role.OWNER)); if (isAdmin && !Strings.isNullOrEmpty(registryAdminClientId)) { - logger.atInfo().log( - "Giving admin %s read+write access to admin registrar %s", - authResult.userIdForLogging(), registryAdminClientId); builder - .put(AccessType.UPDATE, registryAdminClientId) - .put(AccessType.READ, registryAdminClientId); + .put(registryAdminClientId, Role.OWNER); } if (isAdmin) { - // Admins have READ access to all registrars - logger.atInfo().log( - "Giving admin %s read-only access to all registrars", authResult.userIdForLogging()); + // Admins have access to all registrars ofy() .load() .type(Registrar.class) - .forEach(registrar -> builder.put(AccessType.READ, registrar.getClientId())); + .forEach(registrar -> builder.put(registrar.getClientId(), Role.ADMIN)); } return builder.build(); diff --git a/java/google/registry/ui/server/registrar/ConsoleUiAction.java b/java/google/registry/ui/server/registrar/ConsoleUiAction.java index 6e5d9ff55..38e5b9c76 100644 --- a/java/google/registry/ui/server/registrar/ConsoleUiAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleUiAction.java @@ -16,8 +16,8 @@ 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.AccessType.READ; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; 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; @@ -27,6 +27,7 @@ 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.collect.ImmutableSetMultimap; import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.common.io.Resources; @@ -44,6 +45,7 @@ import google.registry.request.auth.Auth; import google.registry.request.auth.AuthResult; 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; @@ -136,12 +138,10 @@ public final class ConsoleUiAction implements Runnable { String clientId = paramClientId.orElse(registrarAccessor.guessClientId()); data.put("clientId", clientId); - data.put("readWriteClientIds", registrarAccessor.getAllClientIdWithAccess().get(UPDATE)); - data.put( - "readOnlyClientIds", - Sets.difference( - registrarAccessor.getAllClientIdWithAccess().get(READ), - registrarAccessor.getAllClientIdWithAccess().get(UPDATE))); + ImmutableSetMultimap roleMap = + registrarAccessor.getAllClientIdWithRoles().inverse(); + data.put("ownerClientIds", roleMap.get(OWNER)); + data.put("adminClientIds", Sets.difference(roleMap.get(ADMIN), roleMap.get(OWNER))); // 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. @@ -150,7 +150,7 @@ public final class ConsoleUiAction implements Runnable { // 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 = registrarAccessor.getRegistrar(clientId, READ); + Registrar registrar = registrarAccessor.getRegistrar(clientId); data.put("requireFeeExtension", registrar.getPremiumPriceAckRequired()); } catch (ForbiddenException e) { logger.atWarning().withCause(e).log( diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index f7cc41d61..d4802ffc6 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -20,8 +20,6 @@ import static google.registry.export.sheet.SyncRegistrarsSheetAction.enqueueRegi import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.security.JsonResponseHelper.Status.ERROR; import static google.registry.security.JsonResponseHelper.Status.SUCCESS; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE; import com.google.common.base.Ascii; import com.google.common.base.Strings; @@ -138,7 +136,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA Map read(String clientId) { return JsonResponseHelper.create( - SUCCESS, "Success", registrarAccessor.getRegistrar(clientId, READ).toJsonMap()); + SUCCESS, "Success", registrarAccessor.getRegistrar(clientId).toJsonMap()); } Map update(final Map args, String clientId) { @@ -148,7 +146,7 @@ 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, UPDATE); + Registrar registrar = registrarAccessor.getRegistrar(clientId); // 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/java/google/registry/ui/soy/registrar/Console.soy b/java/google/registry/ui/soy/registrar/Console.soy index 516a0212a..0c5445327 100644 --- a/java/google/registry/ui/soy/registrar/Console.soy +++ b/java/google/registry/ui/soy/registrar/Console.soy @@ -23,8 +23,8 @@ {template .main} {@param xsrfToken: string} /** Security token. */ {@param clientId: string} /** Registrar client identifier. */ - {@param readWriteClientIds: list} - {@param readOnlyClientIds: list} + {@param ownerClientIds: list} + {@param adminClientIds: list} {@param username: string} /** Arbitrary username to display. */ {@param logoutUrl: string} /** Generated URL for logging out of Google. */ {@param productName: string} /** Name to display for this software product. */ @@ -78,8 +78,8 @@ /** Sidebar nav. Ids on each elt for testing only. */ {template .navbar_ visibility="private"} {@param clientId: string} /** Registrar client identifier. */ - {@param readWriteClientIds: list} - {@param readOnlyClientIds: list} + {@param ownerClientIds: list} + {@param adminClientIds: list}

@@ -88,16 +88,16 @@ class="{css('kd-button')} {css('kd-button-submit')}" onchange='this.form.submit()'> - {if length($readWriteClientIds) > 0} - - {for $id in $readWriteClientIds} + {if length($ownerClientIds) > 0} + + {for $id in $ownerClientIds} {/for} {/if} - {if length($readOnlyClientIds) > 0} - - {for $id in $readOnlyClientIds} + {if length($adminClientIds) > 0} + + {for $id in $adminClientIds} {/for} diff --git a/javatests/google/registry/rdap/RdapActionBaseTestCase.java b/javatests/google/registry/rdap/RdapActionBaseTestCase.java index b7f7b1351..9ddb7f35f 100644 --- a/javatests/google/registry/rdap/RdapActionBaseTestCase.java +++ b/javatests/google/registry/rdap/RdapActionBaseTestCase.java @@ -19,13 +19,14 @@ 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 org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import com.google.appengine.api.users.User; +import com.google.common.collect.ImmutableSetMultimap; import google.registry.model.ofy.Ofy; import google.registry.request.Action; -import google.registry.request.HttpException.ForbiddenException; import google.registry.request.auth.AuthLevel; import google.registry.request.auth.AuthResult; import google.registry.request.auth.UserAuthInfo; @@ -99,22 +100,27 @@ public class RdapActionBaseTestCase { action.requestMethod = Action.Method.GET; action.fullServletPath = "https://example.tld/rdap"; action.rdapWhoisServer = null; + logout(); } protected void login(String clientId) { - when(registrarAccessor.guessClientId()).thenReturn(clientId); + when(registrarAccessor.getAllClientIdWithRoles()) + .thenReturn(ImmutableSetMultimap.of(clientId, OWNER)); action.authResult = AUTH_RESULT; metricRole = REGISTRAR; } protected void logout() { - when(registrarAccessor.guessClientId()).thenThrow(new ForbiddenException("not logged in")); + when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of()); action.authResult = AUTH_RESULT; metricRole = PUBLIC; } protected void loginAsAdmin() { - when(registrarAccessor.guessClientId()).thenReturn("irrelevant"); + // when admin, we don't actually check what they have access to - so it doesn't matter what we + // return. + // null isn't actually a legal value, we just want to make sure it's never actually used. + when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(null); action.authResult = AUTH_RESULT_ADMIN; metricRole = ADMINISTRATOR; } diff --git a/javatests/google/registry/ui/js/registrar/console_test_util.js b/javatests/google/registry/ui/js/registrar/console_test_util.js index 158f95277..073bdc64c 100644 --- a/javatests/google/registry/ui/js/registrar/console_test_util.js +++ b/javatests/google/registry/ui/js/registrar/console_test_util.js @@ -62,8 +62,8 @@ registry.registrar.ConsoleTestUtil.renderConsoleMain = function( logoutUrl: args.logoutUrl || 'https://logout.url.com', isAdmin: goog.isDefAndNotNull(args.isAdmin) ? args.isAdmin : true, clientId: args.clientId || 'ignore', - readWriteClientIds: args.readWriteClientIds || ['readWrite'], - readOnlyClientIds: args.readOnlyClientIds || ['readOnly'], + ownerClientIds: args.ownerClientIds || ['owner'], + adminClientIds: args.adminClientIds || ['admin'], logoFilename: args.logoFilename || 'logo.png', productName: args.productName || 'Nomulus', integrationEmail: args.integrationEmail || 'integration@example.com', diff --git a/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java b/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java index a97b6523c..dda2fde6c 100644 --- a/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java +++ b/javatests/google/registry/ui/server/registrar/AuthenticatedRegistrarAccessorTest.java @@ -20,8 +20,8 @@ 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.AccessType.READ; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; import static org.mockito.Mockito.mock; import com.google.appengine.api.users.User; @@ -34,7 +34,6 @@ import google.registry.request.auth.AuthResult; import google.registry.request.auth.UserAuthInfo; import google.registry.testing.AppEngineRule; import google.registry.testing.InjectRule; -import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType; import java.util.logging.Level; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -99,11 +98,8 @@ public class AuthenticatedRegistrarAccessorTest { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(AUTHORIZED_USER, ADMIN_CLIENT_ID); - assertThat(registrarAccessor.getAllClientIdWithAccess()) - .containsExactly( - UPDATE, DEFAULT_CLIENT_ID, - READ, DEFAULT_CLIENT_ID) - .inOrder(); + assertThat(registrarAccessor.getAllClientIdWithRoles()) + .containsExactly(DEFAULT_CLIENT_ID, OWNER); } /** Logged out users don't have access to anything. */ @@ -112,9 +108,7 @@ public class AuthenticatedRegistrarAccessorTest { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(NO_USER, ADMIN_CLIENT_ID); - ForbiddenException exception = - assertThrows(ForbiddenException.class, () -> registrarAccessor.getAllClientIdWithAccess()); - assertThat(exception).hasMessageThat().contains("Not logged in"); + assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty(); } /** Unauthorized users don't have access to anything. */ @@ -123,7 +117,7 @@ public class AuthenticatedRegistrarAccessorTest { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(UNAUTHORIZED_USER, ADMIN_CLIENT_ID); - assertThat(registrarAccessor.getAllClientIdWithAccess()).isEmpty(); + assertThat(registrarAccessor.getAllClientIdWithRoles()).isEmpty(); } /** Admins have read/write access to the authorized registrars, AND the admin registrar. */ @@ -132,12 +126,12 @@ public class AuthenticatedRegistrarAccessorTest { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(AUTHORIZED_ADMIN, ADMIN_CLIENT_ID); - assertThat(registrarAccessor.getAllClientIdWithAccess()) + assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly( - UPDATE, DEFAULT_CLIENT_ID, - READ, DEFAULT_CLIENT_ID, - UPDATE, ADMIN_CLIENT_ID, - READ, ADMIN_CLIENT_ID) + DEFAULT_CLIENT_ID, OWNER, + DEFAULT_CLIENT_ID, ADMIN, + ADMIN_CLIENT_ID, OWNER, + ADMIN_CLIENT_ID, ADMIN) .inOrder(); } @@ -149,117 +143,65 @@ public class AuthenticatedRegistrarAccessorTest { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(UNAUTHORIZED_ADMIN, ADMIN_CLIENT_ID); - assertThat(registrarAccessor.getAllClientIdWithAccess()) + assertThat(registrarAccessor.getAllClientIdWithRoles()) .containsExactly( - UPDATE, ADMIN_CLIENT_ID, - READ, ADMIN_CLIENT_ID, - READ, DEFAULT_CLIENT_ID) + ADMIN_CLIENT_ID, OWNER, + ADMIN_CLIENT_ID, ADMIN, + DEFAULT_CLIENT_ID, ADMIN) .inOrder(); } /** Fail loading registrar if user doesn't have access to it. */ @Test - public void testGetRegistrarForUser_readOnly_noAccess_isNotAdmin() { + public void testGetRegistrarForUser_noAccess_isNotAdmin() { expectGetRegistrarFailure( DEFAULT_CLIENT_ID, - READ, UNAUTHORIZED_USER, - "User {user} doesn't have READ access to registrar {clientId}"); - } - - /** Fail loading registrar if user doesn't have access to it. */ - @Test - public void testGetRegistrarForUser_readWrite_noAccess_isNotAdmin() { - expectGetRegistrarFailure( - DEFAULT_CLIENT_ID, - UPDATE, - UNAUTHORIZED_USER, - "User {user} doesn't have UPDATE access to registrar {clientId}"); + "{user} doesn't have access to registrar {clientId}"); } /** Fail loading registrar if there's no user associated with the request. */ @Test - public void testGetRegistrarForUser_readOnly_noUser() { - expectGetRegistrarFailure(DEFAULT_CLIENT_ID, READ, NO_USER, "Not logged in"); + public void testGetRegistrarForUser_noUser() { + expectGetRegistrarFailure(DEFAULT_CLIENT_ID, NO_USER, "Not logged in"); } - /** Fail loading registrar if there's no user associated with the request. */ + /** Succeed loading registrar if user has access to it. */ @Test - public void testGetRegistrarForUser_readWrite_noUser() { - expectGetRegistrarFailure(DEFAULT_CLIENT_ID, UPDATE, NO_USER, "Not logged in"); - } - - /** Succeed loading registrar in read-only mode if user has access to it. */ - @Test - public void testGetRegistrarForUser_readOnly_hasAccess_isNotAdmin() { + public void testGetRegistrarForUser_hasAccess_isNotAdmin() { expectGetRegistrarSuccess( - AUTHORIZED_USER, READ, "User {user} has READ access to registrar {clientId}"); + AUTHORIZED_USER, "{user} has [OWNER] access to registrar {clientId}"); } - /** Succeed loading registrar in read-write mode if user has access to it. */ + /** Succeed loading registrar if admin with access. */ @Test - public void testGetRegistrarForUser_readWrite_hasAccess_isNotAdmin() { + public void testGetRegistrarForUser_hasAccess_isAdmin() { expectGetRegistrarSuccess( - AUTHORIZED_USER, UPDATE, "User {user} has UPDATE access to registrar {clientId}"); + AUTHORIZED_ADMIN, "{user} has [OWNER, ADMIN] access to registrar {clientId}"); } - /** Succeed loading registrar in read-only mode if admin with access. */ + /** Succeed loading registrar for admin even if they aren't on the approved contacts list. */ @Test - public void testGetRegistrarForUser_readOnly_hasAccess_isAdmin() { + public void testGetRegistrarForUser_noAccess_isAdmin() { expectGetRegistrarSuccess( - AUTHORIZED_ADMIN, READ, "User {user} has READ access to registrar {clientId}"); - } - - /** Succeed loading registrar in read-write mode if admin with access. */ - @Test - public void testGetRegistrarForUser_readWrite_hasAccess_isAdmin() { - expectGetRegistrarSuccess( - AUTHORIZED_ADMIN, UPDATE, "User {user} has UPDATE access to registrar {clientId}"); - } - - /** Succeed loading registrar for read-only when admin isn't on the approved contacts list. */ - @Test - public void testGetRegistrarForUser_readOnly_noAccess_isAdmin() { - expectGetRegistrarSuccess( - UNAUTHORIZED_ADMIN, READ, "User {user} has READ access to registrar {clientId}."); - } - - /** Fail loading registrar for read-write when admin isn't on the approved contacts list. */ - @Test - public void testGetRegistrarForUser_readWrite_noAccess_isAdmin() { - expectGetRegistrarFailure( - DEFAULT_CLIENT_ID, - UPDATE, - UNAUTHORIZED_ADMIN, - "User {user} doesn't have UPDATE access to registrar {clientId}"); + UNAUTHORIZED_ADMIN, "{user} has [ADMIN] access to registrar {clientId}."); } /** Fail loading registrar even if admin, if registrar doesn't exist. */ @Test - public void testGetRegistrarForUser_readOnly_doesntExist_isAdmin() { + public void testGetRegistrarForUser_doesntExist_isAdmin() { expectGetRegistrarFailure( "BadClientId", - READ, AUTHORIZED_ADMIN, - "User {user} doesn't have READ access to registrar {clientId}"); - } - - /** Fail loading registrar even if admin, if registrar doesn't exist. */ - @Test - public void testGetRegistrarForUser_readWrite_doesntExist_isAdmin() { - expectGetRegistrarFailure( - "BadClientId", - UPDATE, - AUTHORIZED_ADMIN, - "User {user} doesn't have UPDATE access to registrar {clientId}"); + "{user} doesn't have access to registrar {clientId}"); } private void expectGetRegistrarSuccess( - AuthResult authResult, AccessType accessType, String message) { + AuthResult authResult, String message) { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(authResult, ADMIN_CLIENT_ID); - assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID, accessType)).isNotNull(); + assertThat(registrarAccessor.getRegistrar(DEFAULT_CLIENT_ID)).isNotNull(); assertAboutLogs() .that(testLogHandler) .hasLogAtLevelWithMessage( @@ -267,13 +209,13 @@ public class AuthenticatedRegistrarAccessorTest { } private void expectGetRegistrarFailure( - String clientId, AccessType accessType, AuthResult authResult, String message) { + String clientId, AuthResult authResult, String message) { AuthenticatedRegistrarAccessor registrarAccessor = new AuthenticatedRegistrarAccessor(authResult, ADMIN_CLIENT_ID); ForbiddenException exception = assertThrows( - ForbiddenException.class, () -> registrarAccessor.getRegistrar(clientId, accessType)); + ForbiddenException.class, () -> registrarAccessor.getRegistrar(clientId)); assertThat(exception).hasMessageThat().contains(formatMessage(message, authResult, clientId)); } @@ -290,8 +232,7 @@ 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 {user} isn't associated with any registrar"); + expectGuessRegistrarFailure(UNAUTHORIZED_USER, "{user} isn't associated with any registrar"); } /** diff --git a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java index 3e5f80112..705655ed4 100644 --- a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java +++ b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java @@ -17,8 +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 google.registry.testing.DatastoreHelper.loadRegistrar; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.READ; -import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.AccessType.UPDATE; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; +import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; import static javax.servlet.http.HttpServletResponse.SC_MOVED_TEMPORARILY; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -79,16 +79,15 @@ public class ConsoleUiActionTest { action.paramClientId = Optional.empty(); AuthResult authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false)); action.authResult = authResult; - when(registrarAccessor.getRegistrar("TheRegistrar", READ)) + when(registrarAccessor.getRegistrar("TheRegistrar")) .thenReturn(loadRegistrar("TheRegistrar")); - when(registrarAccessor.getAllClientIdWithAccess()) + when(registrarAccessor.getAllClientIdWithRoles()) .thenReturn( ImmutableSetMultimap.of( - UPDATE, "TheRegistrar", - READ, "TheRegistrar", - UPDATE, "ReadWriteRegistrar", - READ, "ReadWriteRegistrar", - READ, "ReadOnlyRegistrar")); + "TheRegistrar", OWNER, + "OtherRegistrar", OWNER, + "OtherRegistrar", ADMIN, + "AdminRegistrar", ADMIN)); when(registrarAccessor.guessClientId()).thenCallRealMethod(); // Used for error message in guessClientId registrarAccessor.authResult = authResult; @@ -128,7 +127,7 @@ public class ConsoleUiActionTest { @Test public void testUserDoesntHaveAccessToAnyRegistrar_showsWhoAreYouPage() { - when(registrarAccessor.getAllClientIdWithAccess()).thenReturn(ImmutableSetMultimap.of()); + when(registrarAccessor.getAllClientIdWithRoles()).thenReturn(ImmutableSetMultimap.of()); action.run(); assertThat(response.getPayload()).contains("

You need permission

"); assertThat(response.getPayload()).contains("not associated with Nomulus."); @@ -155,7 +154,7 @@ public class ConsoleUiActionTest { @Test public void testSettingClientId_notAllowed_showsNeedPermissionPage() { action.paramClientId = Optional.of("OtherClientId"); - when(registrarAccessor.getRegistrar("OtherClientId", READ)) + when(registrarAccessor.getRegistrar("OtherClientId")) .thenThrow(new ForbiddenException("forbidden")); action.run(); assertThat(response.getPayload()).contains("

You need permission

"); @@ -165,7 +164,7 @@ public class ConsoleUiActionTest { @Test public void testSettingClientId_allowed_showsRegistrarConsole() { action.paramClientId = Optional.of("OtherClientId"); - when(registrarAccessor.getRegistrar("OtherClientId", READ)) + when(registrarAccessor.getRegistrar("OtherClientId")) .thenReturn(loadRegistrar("TheRegistrar")); action.run(); assertThat(response.getPayload()).contains("Registrar Console"); @@ -176,7 +175,7 @@ public class ConsoleUiActionTest { public void testUserHasAccessAsTheRegistrar_showsClientIdChooser() { action.run(); assertThat(response.getPayload()).contains("