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