From 2d4eeae26c16ea32f8a1c448871d5164e8361cc5 Mon Sep 17 00:00:00 2001 From: guyben Date: Wed, 13 Jun 2018 09:26:23 -0700 Subject: [PATCH] Fix admin access to registryAdminClientId when they aren't associated with it An admin that isn't associated with any clientId, will default to the registryAdminClientId. However, if we set the registryAdminClientId as the session's CLIENT_ID_ATTRIBUTE, the next time we access the server we have a client-id attribute we aren't associated with - which returns a "403 Registrar Console access revoked" error (the assumption is - we were associated with it before but aren't anymore) To fix this - we just add all admins as "hasAccessTo" registryAdminClientId, even if it's not in the contacts. This will let admins stay on the admin registrar, without affecting where they log-in initially if they are also contacts in different registrars. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=200402856 --- .../ui/server/registrar/SessionUtils.java | 25 ++++++---- .../ui/server/registrar/SessionUtilsTest.java | 46 ++++++++++++++++++- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/java/google/registry/ui/server/registrar/SessionUtils.java b/java/google/registry/ui/server/registrar/SessionUtils.java index eb36bc6eb..38eeecc5d 100644 --- a/java/google/registry/ui/server/registrar/SessionUtils.java +++ b/java/google/registry/ui/server/registrar/SessionUtils.java @@ -42,7 +42,7 @@ public class SessionUtils { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - public static final String CLIENT_ID_ATTRIBUTE = "clientId"; + private static final String CLIENT_ID_ATTRIBUTE = "clientId"; @Inject @Config("registryAdminClientId") @@ -101,7 +101,7 @@ public class SessionUtils { // Use the clientId if it exists if (clientId != null) { - if (!hasAccessToRegistrar(clientId, user.getUserId())) { + if (!hasAccessToRegistrar(clientId, user.getUserId(), userAuthInfo.isUserAdmin())) { logger.atInfo().log("Registrar Console access revoked: %s", clientId); session.invalidate(); return false; @@ -114,7 +114,7 @@ public class SessionUtils { // The clientId was null, so let's try and find a registrar this user is associated with Optional registrar = findRegistrarForUser(user.getUserId()); if (registrar.isPresent()) { - verify(hasAccessToRegistrar(registrar.get(), user.getUserId())); + verify(isInAllowedContacts(registrar.get(), user.getUserId())); logger.atInfo().log( "Associating user %s with found registrar %s.", user.getUserId(), registrar.get().getClientId()); @@ -180,18 +180,27 @@ public class SessionUtils { return result; } - /** @see #hasAccessToRegistrar(Registrar, String) */ - protected static boolean hasAccessToRegistrar(String clientId, final String gaeUserId) { + /** @see #isInAllowedContacts(Registrar, String) */ + boolean hasAccessToRegistrar(String clientId, String gaeUserId, boolean isAdmin) { Optional registrar = Registrar.loadByClientIdCached(clientId); if (!registrar.isPresent()) { logger.atWarning().log("Registrar '%s' disappeared from Datastore!", clientId); return false; } - return hasAccessToRegistrar(registrar.get(), gaeUserId); + if (isAdmin && clientId.equals(registryAdminClientId)) { + return true; + } + return isInAllowedContacts(registrar.get(), gaeUserId); } - /** Returns {@code true} if {@code gaeUserId} is listed in contacts. */ - private static boolean hasAccessToRegistrar(Registrar registrar, final String gaeUserId) { + /** + * Returns {@code true} if {@code gaeUserId} is listed in contacts with access to the registrar. + * + *

Each registrar contact can either have getGaeUserId equals null or the user's gaeUserId. + * Null means the contact doesn't have access to the registrar console. None-null means the + * contact has access. + */ + private static boolean isInAllowedContacts(Registrar registrar, final String gaeUserId) { return registrar .getContacts() .stream() diff --git a/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java b/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java index 51c309ac1..a31e60e21 100644 --- a/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java +++ b/javatests/google/registry/ui/server/registrar/SessionUtilsTest.java @@ -260,6 +260,28 @@ public class SessionUtilsTest { UNAUTHORIZED_ADMIN.user().getUserId(), ADMIN_CLIENT_ID)); } + /** + * If session clientId points to the adminClientId, and the user is an admin that doesn't have + * access to this registrar - it means this is the second (or later) visit of this admin and they + * were granted access to the default registrar because they aren't associated with any other + * registrar. + * + * We continue to grant the admin access. + */ + @Test + public void testCheckRegistrarConsoleLogin_noSession_noAccess_isAdmin_secondRequest() + throws Exception { + when(session.getAttribute("clientId")).thenReturn(ADMIN_CLIENT_ID); + assertThat(sessionUtils.checkRegistrarConsoleLogin(req, UNAUTHORIZED_ADMIN)).isTrue(); + assertAboutLogs() + .that(testLogHandler) + .hasLogAtLevelWithMessage( + Level.INFO, + String.format( + "Associating user %s with given registrar %s.", + UNAUTHORIZED_ADMIN.user().getUserId(), ADMIN_CLIENT_ID)); + } + /** * If clientId does not exist in the session and the user is not associated with a registrar, then * access should not be granted. @@ -279,17 +301,37 @@ public class SessionUtilsTest { @Test public void testHasAccessToRegistrar_orphanedContact_returnsFalse() throws Exception { deleteResource(loadRegistrar(DEFAULT_CLIENT_ID)); - assertThat(SessionUtils.hasAccessToRegistrar(DEFAULT_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID)) + assertThat( + sessionUtils.hasAccessToRegistrar(DEFAULT_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID, false)) .isFalse(); } @Test public void testHasAccessToRegistrar_accessRevoked_returnsFalse() throws Exception { RegistrarContact.updateContacts(loadRegistrar(DEFAULT_CLIENT_ID), new java.util.HashSet<>()); - assertThat(SessionUtils.hasAccessToRegistrar(DEFAULT_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID)) + assertThat( + sessionUtils.hasAccessToRegistrar(DEFAULT_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID, false)) .isFalse(); } + @Test + public void testHasAccessToRegistrar_orphanedAdmin_notAdminRegistrar_returnsFalse() + throws Exception { + RegistrarContact.updateContacts(loadRegistrar(DEFAULT_CLIENT_ID), new java.util.HashSet<>()); + assertThat( + sessionUtils.hasAccessToRegistrar(DEFAULT_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID, true)) + .isFalse(); + } + + @Test + public void testHasAccessToRegistrar_orphanedAdmin_onAdminRegistrar_returnsTrue() + throws Exception { + RegistrarContact.updateContacts(loadRegistrar(ADMIN_CLIENT_ID), new java.util.HashSet<>()); + assertThat( + sessionUtils.hasAccessToRegistrar(ADMIN_CLIENT_ID, THE_REGISTRAR_GAE_USER_ID, true)) + .isTrue(); + } + @Test public void testNullness() throws Exception { new NullPointerTester()