diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index cd6765adee2..1338c0ae877 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -135,6 +135,7 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.impersonation.ImpersonationTest; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.AppPropsTestCase; import org.labkey.api.settings.BaseServerProperties; @@ -398,6 +399,7 @@ public void registerServlets(ServletContext servletCtx) FileUtil.TestCase.class, GenerateUniqueDataIterator.TestCase.class, HelpTopic.TestCase.class, + ImpersonationTest.class, InlineInClauseGenerator.TestCase.class, JSONDataLoader.HeaderMatchTest.class, JSONDataLoader.MetadataTest.class, diff --git a/api/src/org/labkey/api/security/NormalPermissionsContext.java b/api/src/org/labkey/api/security/NormalPermissionsContext.java index c5e7dc59115..7b987fca457 100644 --- a/api/src/org/labkey/api/security/NormalPermissionsContext.java +++ b/api/src/org/labkey/api/security/NormalPermissionsContext.java @@ -22,8 +22,7 @@ import org.labkey.api.security.impersonation.ImpersonationContextFactory; import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; import org.labkey.api.security.impersonation.UserImpersonationContextFactory; -import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NavTree; @@ -93,8 +92,9 @@ public void addMenu(NavTree menu, Container c, User user, ActionURL currentURL) { @Nullable Container project = c.getProject(); - // Must be site or project admin (folder admins can't impersonate) - if (user.hasRootAdminPermission() || (null != project && project.hasPermission(user, AdminPermission.class))) + // Site admin, app admin, and impersonating troubleshooter can impersonate anywhere; project admin can + // impersonate in that project. Folder admins can't impersonate. + if (user.hasRootPermission(ImpersonatePermission.class) || (project != null && project.hasPermission(user, ImpersonatePermission.class))) { NavTree impersonateMenu = new NavTree("Impersonate"); UserImpersonationContextFactory.addMenu(impersonateMenu); @@ -102,13 +102,6 @@ public void addMenu(NavTree menu, Container c, User user, ActionURL currentURL) RoleImpersonationContextFactory.addMenu(impersonateMenu); menu.addChild(impersonateMenu); } - // Or Impersonating Troubleshooter to impersonate site roles only - else if (null == project && user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) - { - NavTree impersonateMenu = new NavTree("Impersonate"); - RoleImpersonationContextFactory.addMenu(impersonateMenu); - menu.addChild(impersonateMenu); - } } NavTree signOut = new NavTree("Sign Out", PageFlowUtil.urlProvider(LoginUrls.class).getLogoutURL(c)); diff --git a/api/src/org/labkey/api/security/PermissionsContext.java b/api/src/org/labkey/api/security/PermissionsContext.java index 16672fd7081..bfd321c41ac 100644 --- a/api/src/org/labkey/api/security/PermissionsContext.java +++ b/api/src/org/labkey/api/security/PermissionsContext.java @@ -64,7 +64,7 @@ default Stream getAssignedRoles(User user, SecurableResource resource) if (!(role instanceof AbstractRootContainerRole siteRole)) throw new IllegalStateException("Root roles should all be AbstractRootContainerRole"); - return siteRole.isAvailableEverywhere() || resource.equals(root); + return siteRole.isApplicableOutsideRoot() || resource.equals(root); }); if (!resource.equals(root)) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 9eb00cbfa75..e6ccfc41323 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -74,7 +74,7 @@ import org.labkey.api.security.permissions.AbstractPermission; import org.labkey.api.security.permissions.AddUserPermission; import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -822,7 +822,7 @@ public static void impersonateUser(ViewContext viewContext, User impersonatedUse @Nullable Container project = viewContext.getContainer().getProject(); User user = viewContext.getUser(); - if (user.hasRootAdminPermission()) + if (user.hasRootPermission(ImpersonatePermission.class)) project = null; impersonate(viewContext, new UserImpersonationContextFactory(project, user, impersonatedUser, returnUrl)); @@ -839,7 +839,7 @@ public static void impersonateRoles(ViewContext viewContext, Collection ne @Nullable Container project = viewContext.getContainer().getProject(); User user = viewContext.getUser(); - if (user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) + if (user.hasRootPermission(ImpersonatePermission.class)) project = null; impersonate(viewContext, new RoleImpersonationContextFactory(project, user, newImpersonationRoles, currentImpersonationRoles, returnUrl)); @@ -1412,7 +1412,7 @@ public static void ensureAtLeastOneRootAdminExists() } // A permission class that uniquely identifies the root admins, of which we insist there must be at least one - public static final Class ROOT_ADMIN_PERMISSION = CanImpersonateSiteRolesPermission.class; + public static final Class ROOT_ADMIN_PERMISSION = ImpersonatePermission.class; public static boolean isRootAdmin(User user) { diff --git a/api/src/org/labkey/api/security/User.java b/api/src/org/labkey/api/security/User.java index 2d28b8263d2..70849d2320f 100644 --- a/api/src/org/labkey/api/security/User.java +++ b/api/src/org/labkey/api/security/User.java @@ -33,7 +33,7 @@ import org.labkey.api.security.permissions.AnalystPermission; import org.labkey.api.security.permissions.ApplicationAdminPermission; import org.labkey.api.security.permissions.BrowserDeveloperPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -594,7 +594,7 @@ public static JSONObject getUserProps(User user, User currentUser, @Nullable Con props.put("isAdmin", nonNullContainer && container.hasPermission(user, AdminPermission.class)); props.put("isRootAdmin", user.hasRootAdminPermission()); props.put("isSystemAdmin", user.hasSiteAdminPermission()); - props.put("canImpersonateSiteRoles", user.hasRootPermission(CanImpersonateSiteRolesPermission.class)); + props.put("canImpersonateSiteRoles", user.hasRootPermission(ImpersonatePermission.class)); props.put("isGuest", user.isGuest()); props.put("isDeveloper", user.isBrowserDev()); props.put("isAnalyst", user.hasRootPermission(AnalystPermission.class)); diff --git a/api/src/org/labkey/api/security/UserCache.java b/api/src/org/labkey/api/security/UserCache.java index f6dbd6bb6f7..72ff900595d 100644 --- a/api/src/org/labkey/api/security/UserCache.java +++ b/api/src/org/labkey/api/security/UserCache.java @@ -83,8 +83,8 @@ private UserCache() return null != user ? user.cloneUser() : null; } - // Returns a deep copy of the active users list, allowing callers to interrogate user permissions without affecting - // cached users. Collection is ordered by email... maybe it should be ordered by display name? + // Returns a mutable deep copy of the active users list, allowing callers to interrogate user permissions without + // affecting cached users. Collection is ordered by email... maybe it should be ordered by display name? static @NotNull Collection getActiveUsers() { List activeUsers = getUserCollections().getActiveUsers(); @@ -95,7 +95,7 @@ private UserCache() .collect(Collectors.toCollection(LinkedList::new)); } - // Returns a deep copy of the users list including deactivated accounts, allowing callers to interrogate user permissions + // Returns a mutable deep copy of the users list including deactivated accounts, allowing callers to interrogate user permissions // without affecting cached users. Collection is ordered randomly... maybe it should be ordered by display name? static @NotNull Collection getActiveAndInactiveUsers() { @@ -104,7 +104,7 @@ private UserCache() return users .stream() .map(User::cloneUser) - .collect(Collectors.toList()); + .collect(Collectors.toCollection(LinkedList::new)); } static @NotNull List getUserIds() @@ -114,7 +114,7 @@ private UserCache() static @NotNull Map getUserEmailMap() { - return Collections.unmodifiableMap(getUserCollections().getEmailMap()); + return getUserCollections().getEmailMap(); } static int getActiveUserCount() diff --git a/api/src/org/labkey/api/security/UserManager.java b/api/src/org/labkey/api/security/UserManager.java index b3bb5c15105..2d3aa600e01 100644 --- a/api/src/org/labkey/api/security/UserManager.java +++ b/api/src/org/labkey/api/security/UserManager.java @@ -59,7 +59,7 @@ import org.labkey.api.security.SecurityManager.UserManagementException; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.ApplicationAdminPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.roles.ApplicationAdminRole; import org.labkey.api.security.roles.SiteAdminRole; @@ -162,7 +162,7 @@ public static void addUserListener(UserListener listener) public static void addUserListener(UserListener listener, boolean meFirst) { if (meFirst) - _listeners.add(0, listener); + _listeners.addFirst(listener); else _listeners.add(listener); } @@ -670,12 +670,14 @@ public static String getEmailForId(Integer userId) } @NotNull + // Returns a mutable collection public static Collection getActiveUsers() { return getUsers(false); } @NotNull + // Returns a mutable collection public static Collection getUsers(boolean includeInactive) { return includeInactive ? UserCache.getActiveAndInactiveUsers() : UserCache.getActiveUsers() ; @@ -1305,7 +1307,7 @@ public void testPermissionsRetrieval() assertTrue("Expected all SiteAdmins to be in the AppAdmins list", appAdmins.containsAll(siteAdmins)); - List privilegedUsers = SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(CanImpersonatePrivilegedSiteRolesPermission.class)); + List privilegedUsers = SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(ImpersonatePrivilegedSiteRolesPermission.class)); assertTrue("Expected all users with privileged role impersonation permissions to have a privileged role", privilegedUsers.stream().allMatch(User::hasPrivilegedRole)); diff --git a/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java b/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java index e3ea1abbf15..55687db5d38 100644 --- a/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java +++ b/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java @@ -21,7 +21,7 @@ import org.labkey.api.security.LoginUrls; import org.labkey.api.security.PermissionsContext; import org.labkey.api.security.User; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; @@ -88,7 +88,7 @@ public ImpersonationContextFactory getFactory() */ protected Stream getFilteredRoles(Stream roles) { - if (getAdminUser() != null && !getAdminUser().hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) + if (getAdminUser() != null && !getAdminUser().hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class)) return roles.filter(role -> !role.isPrivileged()); return roles; diff --git a/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java index 8580ae943dd..66e50c7240a 100644 --- a/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java @@ -31,7 +31,7 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; -import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.GUID; import org.labkey.api.view.ActionURL; @@ -126,28 +126,26 @@ public User getAdminUser() return UserManager.getUser(_adminUserId); } - private static boolean canImpersonateGroup(@Nullable Container project, User user, Group group) + private static boolean canImpersonateGroup(@Nullable Container project, User adminUser, Group group) { // Impersonating the "Site: Guests" group leads to confusion and is not useful. Better to just log out. See #20140. if (group.isGuests()) return false; - // Impersonating "Site: Administrators" or any other group assigned a privileged role by a non-site admin is confusing as well. - if (group.hasPrivilegedRole() && !user.hasSiteAdminPermission()) - return false; - - // Site/app admin can impersonate any other group - if (user.hasRootAdminPermission()) + // Site admin, app admin, and impersonating troubleshooter can impersonate any group, even those assigned + // privileged roles. However, in the case where an app admin impersonates such a group, the privileged roles + // are filtered out (see GroupImpersonationContext.getAssignedRoles() below). + if (adminUser.hasRootPermission(ImpersonatePermission.class)) return true; // Project admin... - if (null != project && project.hasPermission(user, AdminPermission.class)) + if (null != project && project.hasPermission(adminUser, ImpersonatePermission.class)) { // ...can impersonate any project group but must be a member of a site group to impersonate it if (group.isProjectGroup()) return group.getContainer().equals(project.getId()); else - return user.isInGroup(group.getUserId()); + return adminUser.isInGroup(group.getUserId()); } return false; @@ -160,11 +158,12 @@ public static void addMenu(NavTree menu) menu.addChild(groupMenu); } + // Returns the groups this user is allowed to impersonate. Empty for users who aren't allowed to impersonate. public static Collection getValidImpersonationGroups(Container c, User user) { LinkedList validGroups = new LinkedList<>(); - List groups = SecurityManager.getGroups(c.getProject(), true); Container project = c.getProject(); + List groups = SecurityManager.getGroups(project, true); // Site groups are always first, followed by project groups for (Group group : groups) @@ -181,13 +180,13 @@ private static class GroupImpersonationContext extends AbstractImpersonationCont @JsonCreator protected GroupImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_group") Group group, - @JsonProperty("_groups") PrincipalArray groups, - @JsonProperty("_returnUrl") ActionURL returnUrl, - @JsonProperty("_factory") ImpersonationContextFactory factory) - + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_group") Group group, + @JsonProperty("_groups") PrincipalArray groups, + @JsonProperty("_returnUrl") ActionURL returnUrl, + @JsonProperty("_factory") ImpersonationContextFactory factory + ) { super(adminUser, project, returnUrl, factory); _group = group; diff --git a/api/src/org/labkey/api/security/impersonation/ImpersonationTest.java b/api/src/org/labkey/api/security/impersonation/ImpersonationTest.java new file mode 100644 index 00000000000..1865e40b293 --- /dev/null +++ b/api/src/org/labkey/api/security/impersonation/ImpersonationTest.java @@ -0,0 +1,159 @@ +package org.labkey.api.security.impersonation; + +import org.apache.logging.log4j.Logger; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.security.Group; +import org.labkey.api.security.MutableSecurityPolicy; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.UserManagementException; +import org.labkey.api.security.SecurityPolicyManager; +import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.UserPrincipal; +import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.ValidEmail.InvalidEmailException; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.PlatformDeveloperRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.util.TestContext; +import org.labkey.api.util.logging.LogHelper; + +import java.util.Collection; +import java.util.List; +import java.util.Set; + +/** + * We have many impersonation permissions rules. This attempts to test them all. + */ +public class ImpersonationTest extends Assert +{ + private static final Logger LOG = LogHelper.getLogger(ImpersonationTest.class, "Progress of ImpersonationTest"); + + @Test + public void testPermissions() throws Exception + { + Container root = ContainerManager.getRoot(); + User testUser = TestContext.get().getUser(); + + Container projectToDelete = null; + User noPermUser = null; + User readPermUser = null; + Group readPermGroup = null; + Group priviledgedGroup = null; + try + { + // Need to test in a new project since all users get read permission in /Shared, which we don't want + Container project = projectToDelete = ContainerManager.createContainer(root, "_testImpersonationPermissions", testUser); + + // Ensure there's at least one user without permissions in the project + noPermUser = SecurityManager.addUser(new ValidEmail("test_no_permissions@test.com"), null).getUser(); + + // Ensure there's a user assigned read permissions in the project + readPermUser = SecurityManager.addUser(new ValidEmail("test_read_permissions@test.com"), null).getUser(); + MutableSecurityPolicy projectPolicy = new MutableSecurityPolicy(project.getPolicy()); + projectPolicy.addRoleAssignment(readPermUser, ReaderRole.class); + SecurityPolicyManager.savePolicy(projectPolicy, testUser); + + // Ensure there's another project group + readPermGroup = SecurityManager.createGroup(root, "Test_Read_Perm", null); + + // Ensure there's at least one site group assigned to a privileged role + priviledgedGroup = SecurityManager.createGroup(root, "Test_Privileged", null); + MutableSecurityPolicy policy = new MutableSecurityPolicy(root.getPolicy()); + Role privilegedRole = RoleManager.getRole(PlatformDeveloperRole.class); + policy.addRoleAssignment(priviledgedGroup, privilegedRole); + SecurityPolicyManager.savePolicy(policy, testUser); + + // Site-related counts + int siteUserCount = UserManager.getUserIds().size(); // Includes inactive + List siteGroups = SecurityManager.getGroups(null, false); + int siteGroupCount = siteGroups.size() - 1; // Can't impersonate the guests group + int siteRoleCount = RoleManager.getSiteRoles().size(); + + // Project-related counts + int projectUserCount = SecurityManager.getUsersWithPermissions(project, Set.of(ReadPermission.class)).size(); + assertTrue(projectUserCount < siteUserCount); // Should be at least one less because test_no_permissions@test.com doesn't have read + int projectGroupCount = SecurityManager.getGroups(project, false).size(); + int allGroupCount = siteGroupCount + projectGroupCount; + List projectRoles = RoleManager.getAllRoles().stream().filter(role -> role.isAssignable() && role.isApplicable(projectPolicy, project)).toList(); + int projectRoleCount = projectRoles.size(); + + testImpersonator("SiteAdminRole", true, project, siteUserCount, siteGroupCount, siteRoleCount, siteUserCount, allGroupCount, projectRoleCount); + testImpersonator("ImpersonatingTroubleshooterRole", true, project, siteUserCount, siteGroupCount, siteRoleCount, siteUserCount, allGroupCount, projectRoleCount); + // Can't impersonate privileged roles, so SiteAdmin, ImpersonatingTroubleshooter, and PlatformDeveloper should not appear as valid site roles (siteRoleCount - 3) + // Can impersonate all users and groups (same counts as above), but privileged roles will get filtered out by the impersonation contexts + testImpersonator("ApplicationAdminRole", true, project, siteUserCount, siteGroupCount, siteRoleCount - 3, siteUserCount, allGroupCount, projectRoleCount); + + // Can impersonate any project group plus any site group where project admin is a member. As a brand-new + // user, they are a member in Site:Users only and we created one project group, so they can impersonate two + // groups at the project level. + testImpersonator("ProjectAdminRole", false, project, 0, 0, 0, projectUserCount, 2, projectRoleCount); + + // Can't impersonate anything anywhere + testImpersonator("FolderAdminRole", false, project, 0, 0, 0, 0, 0, 0); + testImpersonator("EditorRole", false, project, 0, 0, 0, 0, 0, 0); + testImpersonator("ReaderRole", false, project, 0, 0, 0, 0, 0, 0); + } + finally + { + if (priviledgedGroup != null) + SecurityManager.deleteGroup(priviledgedGroup, testUser); + if (readPermGroup != null) + SecurityManager.deleteGroup(readPermGroup, testUser); + if (readPermUser != null) + UserManager.deleteUser(readPermUser.getUserId()); + if (noPermUser != null) + UserManager.deleteUser(noPermUser.getUserId()); + if (projectToDelete != null) + ContainerManager.delete(projectToDelete, testUser); + } + } + + private void testImpersonator(String roleName, boolean siteRole, Container project, int expectedSiteUsers, int expectedSiteGroups, int expectedSiteRoles, int expectedProjectUsers, int expectedProjectGroups, int expectedProjectRoles) throws InvalidEmailException, UserManagementException + { + LOG.info("Testing {}", roleName); + Container root = ContainerManager.getRoot(); + User userToDelete = null; + + try + { + // Create user and assign role + String email = roleName + "@test.com"; + User user = userToDelete = SecurityManager.addUser(new ValidEmail(email), null).getUser(); + Role role = RoleManager.getRole("org.labkey.api.security.roles." + roleName); + assertNotNull("Could not resolve " + roleName, role); + Container roleContainer = siteRole ? root : project; + MutableSecurityPolicy rootPolicy = new MutableSecurityPolicy(roleContainer.getPolicy()); + rootPolicy.addRoleAssignment(user, role); + SecurityPolicyManager.savePolicyForTests(rootPolicy, TestContext.get().getUser()); + + // Test impersonating at the site level + // TODO: Active vs. all? + Collection validUsers = UserImpersonationContextFactory.getValidImpersonationUsers(null, user); + assertEquals(expectedSiteUsers, validUsers.size()); + assertTrue(user + " is able to impersonate themselves!", validUsers.stream().noneMatch(u -> u.equals(user))); + Collection validSiteGroups = GroupImpersonationContextFactory.getValidImpersonationGroups(root, user); + assertEquals(expectedSiteGroups, validSiteGroups.size()); + Collection validSiteRoles = RoleImpersonationContextFactory.filterImpersonationRoles(root, user, RoleManager.getAllRoles()); + assertEquals(expectedSiteRoles, validSiteRoles.size()); + + // Test impersonating at the project level + Collection projectUsers = UserImpersonationContextFactory.getValidImpersonationUsers(project, user); + assertEquals(expectedProjectUsers, projectUsers.size()); + Collection validProjectGroups = GroupImpersonationContextFactory.getValidImpersonationGroups(project, user); + assertEquals(expectedProjectGroups, validProjectGroups.size()); + Collection validProjectRoles = RoleImpersonationContextFactory.filterImpersonationRoles(project, user, RoleManager.getAllRoles()); + assertEquals(expectedProjectRoles, validProjectRoles.size()); + } + finally + { + if (userToDelete != null) + UserManager.deleteUser(userToDelete.getUserId()); + } + } +} diff --git a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java index a273596b4d4..2bebac04d2b 100644 --- a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java @@ -31,20 +31,17 @@ import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; -import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.roles.AbstractRootContainerRole; import org.labkey.api.security.roles.Role; -import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.GUID; -import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NavTree; import org.labkey.api.view.ViewContext; import java.util.Collection; -import java.util.List; +import java.util.Collections; import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -183,16 +180,30 @@ private static void addMenu(NavTree menu, String text) menu.addChild(newRoleMenu); } - public static Stream getValidImpersonationRoles(Container c, User user) + // Returns a collection of roles that this user is allowed to impersonate. Empty if user can't impersonate. + public static Collection filterImpersonationRoles(Container c, User adminUser, Collection candidates) { + boolean canImpersonate = adminUser.hasRootPermission(ImpersonatePermission.class); + + if (!canImpersonate && !c.isRoot()) + { + canImpersonate = c.hasPermission(adminUser, ImpersonatePermission.class); + } + + if (!canImpersonate) + { + return Collections.emptyList(); + } + + boolean canImpersonatePrivilegedRoles = adminUser.hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class); SecurityPolicy policy = SecurityPolicyManager.getPolicy(c); - boolean canImpersonatePrivilegedRoles = user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class); // Stream the valid roles - return RoleManager.getAllRoles().stream() + return candidates.stream() .filter(Role::isAssignable) .filter(role -> role.isApplicable(policy, c)) - .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); + .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles) + .toList(); } public static class RoleImpersonationContext extends AbstractImpersonationContext @@ -202,11 +213,12 @@ public static class RoleImpersonationContext extends AbstractImpersonationContex @JsonCreator private RoleImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_roles") RoleSet roles, - @JsonProperty("_factory") ImpersonationContextFactory factory, - @JsonProperty("_cacheKey") String cacheKey) + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_roles") RoleSet roles, + @JsonProperty("_factory") ImpersonationContextFactory factory, + @JsonProperty("_cacheKey") String cacheKey + ) { this(project, adminUser, roles, null, factory, cacheKey); } @@ -226,8 +238,10 @@ private RoleImpersonationContext( } // Throws if user is not authorized to impersonate all roles - private void verifyPermissions(@Nullable Container project, User user, Set roles) + private void verifyPermissions(@Nullable Container project, User adminUser, Set roles) { + final Collection filteredRoles; + if (null == project) { // Ensure we have either site roles or project roles, not both @@ -238,32 +252,15 @@ private void verifyPermissions(@Nullable Container project, User user, Set if (map.size() > 1) throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles and project roles at the same time", getFactory()); - // Site Administrator and Impersonating Troubleshooter can impersonate any site role - if (user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) - return; - - if (!user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles", getFactory()); - - // Application Administrator can impersonate all site roles except the privileged ones - List privileged = roles.stream() - .filter(Role::isPrivileged) - .map(Role::getDisplayName) - .toList(); - - if (!privileged.isEmpty()) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate " + StringUtilsLabKey.joinWithConjunction(privileged, "or"), getFactory()); + filteredRoles = filterImpersonationRoles(ContainerManager.getRoot(), adminUser, roles); } else { - // Must have admin permissions in this project - if (!project.hasPermission(user, AdminPermission.class)) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate a role in this project", getFactory()); - - // Must not be impersonating any site roles - if (roles.stream().anyMatch(role -> (role instanceof AbstractRootContainerRole))) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles", getFactory()); + filteredRoles = filterImpersonationRoles(project, adminUser, roles); } + + if (filteredRoles.size() != roles.size()) + throw new UnauthorizedImpersonationException("One or more impersonation roles are not authorized", getFactory()); } @Override @@ -285,7 +282,7 @@ public Stream getAssignedRoles(User user, SecurableResource resource) // impersonate the specified roles. See Issue #50248 to understand the "instanceof Container" check. return resource instanceof Container c ? _roles.stream() - .filter(role -> !(role instanceof AbstractRootContainerRole siteRole) || siteRole.isAvailableEverywhere() || c.isRoot()) : + .filter(role -> !(role instanceof AbstractRootContainerRole siteRole) || siteRole.isApplicableOutsideRoot() || c.isRoot()) : Stream.empty(); } diff --git a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java index 8976ed4d42c..686c0c74f1e 100644 --- a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java @@ -30,7 +30,8 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; -import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.GUID; import org.labkey.api.util.SessionHelper; @@ -40,6 +41,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Set; import java.util.stream.Stream; /** @@ -143,24 +145,29 @@ public static void addMenu(NavTree menu) menu.addChild(userMenu); } - // Somewhat redundant with verifyPermissions() below, but much more expensive in the single-user case. Keep these two methods in sync. - // project == null means return all site users (if authorized) + // Redundant with verifyPermissions() below, but much more expensive in the single-user case. Keep these two methods + // in sync. project == null means return all site users (if authorized). Includes inactive users so we can show them + // grayed out in the UI, but they can't be impersonated. Note that we allow app admins and project admins to + // impersonate site admins and other users with privileged roles, but those roles are filtered out by + // UserImpersonationContext. Project admins are also restricted to their project. public static Collection getValidImpersonationUsers(@Nullable Container project, User adminUser) { + // Must assign a mutable list in all scenarios to allow subsequent remove() call Collection validUsers; - // Site admin can impersonate any active user... - if (null == project) + // Site admin, app admin, and impersonating troubleshooter can impersonate any user, regardless of user's permissions and where impersonation is initiated + if (adminUser.hasRootPermission(ImpersonatePermission.class)) { - validUsers = adminUser.hasRootAdminPermission() ? UserManager.getUsers(true) : new ArrayList<>(0); // Mutable list to allow subsequent remove() call + validUsers = UserManager.getUsers(true); } - else if (!project.hasPermission(adminUser, AdminPermission.class)) + else if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class)) { - validUsers = new ArrayList<>(0); // Mutable list to allow subsequent remove() call + // The read permission check is not for security; it just seems useless to offer impersonation on a user who lacks read. + validUsers = new ArrayList<>(SecurityManager.getUsersWithPermissions(project, true, Set.of(ReadPermission.class))); } else { - validUsers = SecurityManager.getProjectUsers(project); + validUsers = new ArrayList<>(0); } validUsers.remove(adminUser); @@ -168,13 +175,34 @@ else if (!project.hasPermission(adminUser, AdminPermission.class)) return validUsers; } + // Keep in sync with getValidImpersonationUsers() above + private static void verifyPermissions(@Nullable Container project, User impersonatedUser, User adminUser, ImpersonationContextFactory factory) + { + if (impersonatedUser.equals(adminUser)) + throw new UnauthorizedImpersonationException("Can't impersonate yourself", factory); + + if (!impersonatedUser.isActive()) + throw new UnauthorizedImpersonationException("Can't impersonate an inactive user", factory); + + // Site admin, app admin, and impersonating troubleshooter can impersonate anywhere, regardless of user's permissions + if (adminUser.hasRootPermission(ImpersonatePermission.class)) + return; + + // Project admin... + if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class) && project.hasPermission(impersonatedUser, ReadPermission.class)) + return; + + throw new UnauthorizedImpersonationException("Can't impersonate this user", factory); + } + private static class UserImpersonationContext extends AbstractImpersonationContext { @JsonCreator protected UserImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_factory") ImpersonationContextFactory factory) + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_factory") ImpersonationContextFactory factory + ) { super(adminUser, project, null, factory); } @@ -182,24 +210,7 @@ protected UserImpersonationContext( private UserImpersonationContext(@Nullable Container project, User adminUser, User impersonatedUser, ActionURL returnUrl, ImpersonationContextFactory factory) { super(adminUser, project, returnUrl, factory); - verifyPermissions(project, impersonatedUser, adminUser); - } - - // Keep in sync with getValidImpersonationUsers() above - private void verifyPermissions(@Nullable Container project, User impersonatedUser, User adminUser) - { - if (impersonatedUser.equals(adminUser)) - throw new UnauthorizedImpersonationException("Can't impersonate yourself", getFactory()); - - // Site/app admin can impersonate anywhere - if (adminUser.hasRootAdminPermission()) - return; - - // Project admin... - if (null != project && project.hasPermission(adminUser, AdminPermission.class) && SecurityManager.getProjectUsersIds(project).contains(impersonatedUser.getUserId())) - return; - - throw new UnauthorizedImpersonationException("Can't impersonate this user", getFactory()); + verifyPermissions(project, impersonatedUser, adminUser, factory); } @Override diff --git a/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java b/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java index 40efb642e24..42d1441e186 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java @@ -67,6 +67,7 @@ public abstract class AbstractActionPermissionTest extends Assert private static final String AUTHOR_EMAIL = "author@actionpermission.test"; private static final String READER_EMAIL = "reader@actionpermission.test"; private static final String SUBMITTER_EMAIL = "submitter@actionpermission.test"; + private static final String NO_PERMISSION_EMAIL = "nopermission@actionpermission.test"; private static final String TRUSTED_EDITOR_EMAIL = "trustededitor@actionpermission.test"; private static final String TRUSTED_AUTHOR_EMAIL = "trustedauthor@actionpermission.test"; private static final String TROUBLESHOOTER_EMAIL = "troubleshooter@actionpermission.test"; @@ -74,7 +75,7 @@ public abstract class AbstractActionPermissionTest extends Assert protected static final String[] LKS_ROLE_EMAILS = { SITE_ADMIN_EMAIL, APPLICATION_ADMIN_EMAIL, PROJECT_ADMIN_EMAIL, FOLDER_ADMIN_EMAIL, EDITOR_EMAIL, AUTHOR_EMAIL, READER_EMAIL, SUBMITTER_EMAIL, TRUSTED_EDITOR_EMAIL, TRUSTED_AUTHOR_EMAIL, TROUBLESHOOTER_EMAIL, - IMPERSONATING_TROUBLESHOOTER_EMAIL + IMPERSONATING_TROUBLESHOOTER_EMAIL, NO_PERMISSION_EMAIL }; protected static Container _c; @@ -183,6 +184,21 @@ public static Map createUsers(String[] userEmails) return users; } + public void assertForNoPermission(User user, PermissionCheckableAction... actions) + { + for (PermissionCheckableAction action : actions) + { + assertPermission(_c, action, user); + + assertPermission(_c, action, + _users.get(READER_EMAIL), _users.get(AUTHOR_EMAIL), _users.get(EDITOR_EMAIL), + _users.get(FOLDER_ADMIN_EMAIL), _users.get(PROJECT_ADMIN_EMAIL), + _users.get(APPLICATION_ADMIN_EMAIL), _users.get(SITE_ADMIN_EMAIL), + _users.get(SUBMITTER_EMAIL) + ); + } + } + public void assertForReadPermission(User user, PermissionCheckableAction... actions) { assertForReadPermission(user, false, actions); diff --git a/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java b/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java deleted file mode 100644 index 47bbf29dca7..00000000000 --- a/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.labkey.api.security.permissions; - -public class CanImpersonateSiteRolesPermission extends AbstractPermission -{ - public CanImpersonateSiteRolesPermission() - { - super("Can Impersonate Site Roles", "Allows users to impersonate site roles except for privileged roles"); - } -} diff --git a/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java b/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java new file mode 100644 index 00000000000..26ee98a3351 --- /dev/null +++ b/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java @@ -0,0 +1,9 @@ +package org.labkey.api.security.permissions; + +public class ImpersonatePermission extends AbstractPermission +{ + public ImpersonatePermission() + { + super("Can Impersonate", "Allows users to impersonate users, groups, and roles, except for privileged roles"); + } +} diff --git a/api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java b/api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java similarity index 57% rename from api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java rename to api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java index 4c4454a3256..61e31d0fe35 100644 --- a/api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java +++ b/api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java @@ -1,8 +1,8 @@ package org.labkey.api.security.permissions; -public class CanImpersonatePrivilegedSiteRolesPermission extends AbstractPermission +public class ImpersonatePrivilegedSiteRolesPermission extends AbstractPermission { - public CanImpersonatePrivilegedSiteRolesPermission() + public ImpersonatePrivilegedSiteRolesPermission() { super("Can Impersonate Privileged Site Roles", "Allows users to impersonate privileged site roles including Site Admin"); } diff --git a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java index 4ccb841b53a..b5816bd6a7b 100644 --- a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java +++ b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java @@ -61,7 +61,9 @@ public boolean isApplicable(SecurityPolicy policy, SecurableResource resource) return resource instanceof Container && ((Container)resource).isRoot(); } - public boolean isAvailableEverywhere() + // Most site roles are applicable outside the root (i.e., every container). Troubleshooters are an exception + // because we want them to have read in the root but not elsewhere. + public boolean isApplicableOutsideRoot() { return true; } diff --git a/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java b/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java index 8c15081827b..d3caa0e45d1 100644 --- a/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java +++ b/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java @@ -17,10 +17,10 @@ import org.labkey.api.security.permissions.AddUserPermission; import org.labkey.api.security.permissions.ApplicationAdminPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; import org.labkey.api.security.permissions.DeleteUserPermission; import org.labkey.api.security.permissions.EnableRestrictedModules; import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.security.permissions.UpdateUserPermission; @@ -37,7 +37,7 @@ public class ApplicationAdminRole extends AbstractRootContainerRole implements A static Collection> PERMISSIONS = Arrays.asList( AddUserPermission.class, ApplicationAdminPermission.class, - CanImpersonateSiteRolesPermission.class, + ImpersonatePermission.class, DeleteUserPermission.class, EnableRestrictedModules.class, ExemptFromAccountDisablingPermission.class, diff --git a/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java b/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java index f6bdfcad095..fc04c2d0c8b 100644 --- a/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java +++ b/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java @@ -1,7 +1,7 @@ package org.labkey.api.security.roles; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; import java.util.Set; @@ -10,12 +10,12 @@ public class ImpersonatingTroubleshooterRole extends AbstractRootContainerRole { protected ImpersonatingTroubleshooterRole() { - super("Impersonating Troubleshooter", "Can impersonate site roles, including Site Administrator, in addition to having other standard Troubleshooter abilities.", + super("Impersonating Troubleshooter", "Can impersonate users, groups, and roles, including Site Administrator, in addition to having other standard Troubleshooter abilities.", TroubleshooterRole.PERMISSIONS, Set.of( - CanImpersonatePrivilegedSiteRolesPermission.class, - CanImpersonateSiteRolesPermission.class, - ExemptFromAccountDisablingPermission.class + ExemptFromAccountDisablingPermission.class, + ImpersonatePrivilegedSiteRolesPermission.class, + ImpersonatePermission.class ) ); excludeUsers(); @@ -28,7 +28,7 @@ public boolean isPrivileged() } @Override - public boolean isAvailableEverywhere() + public boolean isApplicableOutsideRoot() { return false; } diff --git a/api/src/org/labkey/api/security/roles/ProjectAdminRole.java b/api/src/org/labkey/api/security/roles/ProjectAdminRole.java index b225d5683fb..2ee1d953a4f 100644 --- a/api/src/org/labkey/api/security/roles/ProjectAdminRole.java +++ b/api/src/org/labkey/api/security/roles/ProjectAdminRole.java @@ -19,9 +19,10 @@ import org.labkey.api.security.SecurableResource; import org.labkey.api.security.SecurityPolicy; import org.labkey.api.security.permissions.AddUserPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; -import java.util.Collections; +import java.util.List; public class ProjectAdminRole extends AbstractRole implements AdminRoleListener { @@ -30,7 +31,10 @@ public ProjectAdminRole() super("Project Administrator", "Project Administrators have full control over the project, but not the entire system.", FolderAdminRole.PERMISSIONS, - Collections.singletonList(AddUserPermission.class) + List.of( + AddUserPermission.class, + ImpersonatePermission.class + ) ); excludeGuests(); diff --git a/api/src/org/labkey/api/security/roles/SiteAdminRole.java b/api/src/org/labkey/api/security/roles/SiteAdminRole.java index df248dd1bec..1541992fa1f 100644 --- a/api/src/org/labkey/api/security/roles/SiteAdminRole.java +++ b/api/src/org/labkey/api/security/roles/SiteAdminRole.java @@ -16,11 +16,9 @@ package org.labkey.api.security.roles; import org.labkey.api.security.permissions.AdminOperationsPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.permissions.CanUseSendMessageApiPermission; import org.labkey.api.security.permissions.EditModuleResourcesPermission; -import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.permissions.UploadFileBasedModulePermission; @@ -37,11 +35,9 @@ public class SiteAdminRole extends AbstractRootContainerRole implements AdminRol { private static final Collection> PERMISSIONS = Arrays.asList( AdminOperationsPermission.class, - CanImpersonatePrivilegedSiteRolesPermission.class, - CanImpersonateSiteRolesPermission.class, + ImpersonatePrivilegedSiteRolesPermission.class, CanUseSendMessageApiPermission.class, EmailNonUsersPermission.class, - ExemptFromAccountDisablingPermission.class, SiteAdminPermission.class, UploadFileBasedModulePermission.class ); diff --git a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java index 03c4da04936..b59d58867c9 100644 --- a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java +++ b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java @@ -39,7 +39,7 @@ public TroubleshooterRole() } @Override - public boolean isAvailableEverywhere() + public boolean isApplicableOutsideRoot() { return false; // This ensures troubleshooters get these permissions (esp. ReadPermission) only in the root } diff --git a/api/src/org/labkey/api/util/URLHelper.java b/api/src/org/labkey/api/util/URLHelper.java index 2eeadcfb3b5..65a18fc3d42 100644 --- a/api/src/org/labkey/api/util/URLHelper.java +++ b/api/src/org/labkey/api/util/URLHelper.java @@ -915,7 +915,7 @@ private static boolean isAllowedExternalHost(@NotNull String host, boolean devMo if (devMode && "localhost".equalsIgnoreCase(host)) return true; - // Count the dots in case there's a wild card + // Count the dots in case there's a wildcard int dotCount = StringUtils.countMatches(host, '.'); return allowedHostsSupplier.get().stream() @@ -928,14 +928,14 @@ private static boolean isMatch(@NotNull String host, int dotCount, @NotNull Stri if (allowedHost.startsWith("*.")) { - // This wild-card pattern matches the host if 1) they have the same number of dots and 2) the host ends with - // the portion of the pattern after the wild card. + // This wildcard pattern matches the host if 1) they have the same number of dots and 2) the host ends with + // the portion of the pattern after the wildcard and dot. int expectedDotCount = StringUtils.countMatches(allowedHost, '.'); ret = (dotCount == expectedDotCount && Strings.CI.endsWith(host, allowedHost.substring(2))); } else { - // Non-wild-card pattern must match the entire host + // Non-wildcard pattern must match the entire host ret = Strings.CI.equals(host, allowedHost); } @@ -1120,8 +1120,8 @@ public void testAllowedHosts() allowedHosts2.add("www.google.com"); assertTrue(isAllowedExternalHost("www.google.com", true, ()->allowedHosts2)); - // test wild cards - List wildCardHosts = List.of( + // test wildcards + List wildcardHosts = List.of( "*.labkey.com", "labkey.com", "*.lkpoc.labkey.com", @@ -1130,21 +1130,21 @@ public void testAllowedHosts() ); // good - assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.trial.labkey.host", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.trial.labkey.host", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.trial.labkey.host", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.trial.labkey.host", true, ()->wildcardHosts)); // bad - assertFalse(isAllowedExternalHost("trial.labkey.host", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("sub1.sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("google.com", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("sub1.sub2.labkey.com", true, ()->wildCardHosts)); + assertFalse(isAllowedExternalHost("trial.labkey.host", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("google.com", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.labkey.com", true, ()->wildcardHosts)); } } } diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index f2ebc545c3a..dc27efb07ea 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -41,7 +41,7 @@ public HtmlString getDescription()

Add allowed hosts based on the server name or IP address, as they will be referenced in parameters - such as returnUrl. An asterisk (*) follow by a dot acts as a wild card that matches any leading + such as returnUrl. An asterisk (*) follow by a dot acts as a wildcard that matches any leading subdomain for that host. Examples: www.myexternalhost.com, *.myexternalhost.com, or 1.2.3.4

@@ -75,21 +75,21 @@ public void validateValueFormat(String host, BindException errors) { if (AUTHORITY_VALIDATOR.isValidAuthority(host)) { - // Validate wild card patterns + // Validate wildcard patterns int starCount = StringUtils.countMatches(host, '*'); if (starCount > 0) { if (starCount > 1) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s has multiple wild card characters", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has multiple wildcard characters", host))); } else if (!host.startsWith("*.")) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s has an invalid wild card. The pattern must start with \"*.\".", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has an invalid wildcard. The pattern must start with \"*.\".", host))); } else if (StringUtils.countMatches(host, '.') < 2) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s with wild card is too short. The pattern must include at least two dots.", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s with wildcard is too short. The pattern must include at least two dots.", host))); } } } diff --git a/core/src/org/labkey/core/user/UserController.java b/core/src/org/labkey/core/user/UserController.java index fdf3d452105..5259b64f004 100644 --- a/core/src/org/labkey/core/user/UserController.java +++ b/core/src/org/labkey/core/user/UserController.java @@ -98,6 +98,7 @@ import org.labkey.api.security.ValidEmail.InvalidEmailException; import org.labkey.api.security.impersonation.GroupImpersonationContextFactory; import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; +import org.labkey.api.security.impersonation.RoleImpersonationContextFactory.RoleImpersonationContext; import org.labkey.api.security.impersonation.UnauthorizedImpersonationException; import org.labkey.api.security.impersonation.UserImpersonationContextFactory; import org.labkey.api.security.permissions.AbstractActionPermissionTest; @@ -105,8 +106,8 @@ import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.CanAccessLockedProjectsPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; import org.labkey.api.security.permissions.DeleteUserPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdateUserPermission; @@ -896,7 +897,7 @@ private boolean isProjectAdmin(User user) @RequiresPermission(AdminPermission.class) - public class ShowUserHistoryAction extends SimpleViewAction + public class ShowUserHistoryAction extends SimpleViewAction { @Override public void checkPermissions() throws UnauthorizedException @@ -2786,7 +2787,7 @@ public ApiResponse execute(GetUsersForm form, BindException errors) } - @RequiresPermission(AdminPermission.class) + @RequiresNoPermission // getValidImpersonationUsers() does all permission checking public static class GetImpersonationUsersAction extends MutatingApiAction { @Override @@ -2795,9 +2796,7 @@ public ApiResponse execute(Object object, BindException errors) ApiSimpleResponse response = new ApiSimpleResponse(); User currentUser = getUser(); - Container project = currentUser.hasRootAdminPermission() ? null : getContainer().getProject(); - Collection users = UserImpersonationContextFactory.getValidImpersonationUsers(project, getUser()); - + Collection users = UserImpersonationContextFactory.getValidImpersonationUsers(getContainer().getProject(), currentUser); Collection> responseUsers = new LinkedList<>(); for (User user : users) @@ -2848,6 +2847,15 @@ public void setEmail(ValidEmail email) // All three impersonate API actions have the same form private abstract static class ImpersonateApiAction
extends MutatingApiAction { + @Override + public void checkPermissions() throws UnauthorizedException + { + // Impersonating troubleshooter role is restricted to the root, but, as a convenience, we let them + // impersonate from any container. + if (!getUser().hasRootPermission(ImpersonatePermission.class)) + super.checkPermissions(); + } + @Override protected String getCommandClassMethodName() { @@ -2872,9 +2880,8 @@ public ApiResponse execute(FORM form, BindException errors) public abstract @Nullable String impersonate(FORM form); } - - @RequiresPermission(AdminPermission.class) - public class ImpersonateUserAction extends ImpersonateApiAction + @RequiresNoPermission + public static class ImpersonateUserAction extends ImpersonateApiAction { @Override public @Nullable String impersonate(ImpersonateUserForm form) @@ -2912,8 +2919,8 @@ else if (form.getEmail() != null) } } - - @RequiresPermission(AdminPermission.class) + // getValidImpersonationGroups() checks permissions and returns empty for user who can't impersonate + @RequiresNoPermission public static class GetImpersonationGroupsAction extends MutatingApiAction { @Override @@ -2937,7 +2944,6 @@ public ApiResponse execute(Object object, BindException errors) } } - public static class ImpersonateGroupForm extends ReturnUrlForm { private Integer _groupId = null; @@ -2954,11 +2960,10 @@ public void setGroupId(Integer groupId) } } - // TODO: Better instructions // TODO: Messages for no groups, no users - @RequiresPermission(AdminPermission.class) - public class ImpersonateGroupAction extends ImpersonateApiAction + @RequiresNoPermission + public static class ImpersonateGroupAction extends ImpersonateApiAction { @Override public @Nullable String impersonate(ImpersonateGroupForm form) @@ -2991,25 +2996,26 @@ public class ImpersonateGroupAction extends ImpersonateApiAction + public static class GetImpersonationRolesAction extends MutatingApiAction { @Override public ApiResponse execute(Object object, BindException errors) { - PermissionsContext context = authorizeImpersonateRoles(); - Set impersonationRoles = context.isImpersonating() ? context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : Collections.emptySet(); + PermissionsContext context = getUser().getPermissionsContext(); + Set currentRoles = context.isImpersonating() && context instanceof RoleImpersonationContext ? + context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : + Collections.emptySet(); User user = context.isImpersonating() ? context.getAdminUser() : getUser(); ApiSimpleResponse response = new ApiSimpleResponse(); - Collection> responseRoles = RoleImpersonationContextFactory.getValidImpersonationRoles(getContainer(), user) + Collection> responseRoles = RoleImpersonationContextFactory.filterImpersonationRoles(getContainer(), user, RoleManager.getAllRoles()).stream() .map(role -> { Map map = new HashMap<>(); map.put("displayName", role.getDisplayName()); map.put("roleName", role.getUniqueName()); map.put("hasRead", role.getPermissions().contains(ReadPermission.class)); - map.put("selected", impersonationRoles.contains(role)); + map.put("selected", currentRoles.contains(role)); return map; }) .toList(); @@ -3020,25 +3026,6 @@ public ApiResponse execute(Object object, BindException errors) } } - - private PermissionsContext authorizeImpersonateRoles() - { - User user = getUser(); - PermissionsContext context = user.getPermissionsContext(); - - if (context.isImpersonating()) - user = context.getAdminUser(); - - if (getContainer().isRoot() && user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) - return context; - - if (!getContainer().hasPermission(user, AdminPermission.class)) - throw new UnauthorizedException(); - - return context; - } - - public static class ImpersonateRolesForm extends ReturnUrlForm { private String[] _roleNames; @@ -3048,23 +3035,26 @@ public String[] getRoleNames() return _roleNames; } + @SuppressWarnings("unused") public void setRoleNames(String[] roleNames) { _roleNames = roleNames; } } - - // Permissions are checked in impersonate() to let an admin adjust an existing impersonation + // Permissions are checked by the RoleImpersonationFactory. This lets impersonators adjust an existing impersonation + // and impersonating troubleshooters impersonate in projects and folders. @RequiresNoPermission - public class ImpersonateRolesAction extends ImpersonateApiAction + public static class ImpersonateRolesAction extends ImpersonateApiAction { @Nullable @Override public String impersonate(ImpersonateRolesForm form) { - PermissionsContext context = authorizeImpersonateRoles(); - Set currentImpersonationRoles = context.isImpersonating() ? context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : Collections.emptySet(); + PermissionsContext context = getUser().getPermissionsContext(); + Set currentImpersonationRoles = context.isImpersonating() && context instanceof RoleImpersonationContext ? + context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : + Collections.emptySet(); String[] roleNames = form.getRoleNames(); @@ -3232,6 +3222,15 @@ public void testActionPermissions() UserController controller = new UserController(); + assertForNoPermission(user, + new GetImpersonationUsersAction(), + new ImpersonateUserAction(), + new GetImpersonationGroupsAction(), + new ImpersonateGroupAction(), + new GetImpersonationRolesAction(), + new ImpersonateRolesAction() + ); + // @RequiresPermission(ReadPermission.class) assertForReadPermission(user, false, new BeginAction(), @@ -3241,15 +3240,9 @@ public void testActionPermissions() // @RequiresPermission(AdminPermission.class) assertForAdminPermission(user, - controller.new ShowUsersAction(), + controller.new ShowUsersAction() //TODO controller.new ShowUserHistoryAction(), //TODO controller.new UserAccessAction(), - new GetImpersonationUsersAction(), - controller.new ImpersonateUserAction(), - new GetImpersonationGroupsAction(), - controller.new ImpersonateGroupAction() -// controller.new GetImpersonationRolesAction() Annotated as "no permission", to allow impersonation adjustments -// controller.new ImpersonateRolesAction() Annotated as "no permission", to allow impersonation adjustments ); // @RequiresPermission(UserManagementPermission.class)