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("