diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index cd6765adee2..d613909b2b6 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.ImpersonationTestCase; 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, + ImpersonationTestCase.class, InlineInClauseGenerator.TestCase.class, JSONDataLoader.HeaderMatchTest.class, JSONDataLoader.MetadataTest.class, diff --git a/api/src/org/labkey/api/data/Container.java b/api/src/org/labkey/api/data/Container.java index 0d5186156a7..0a397c0340d 100644 --- a/api/src/org/labkey/api/data/Container.java +++ b/api/src/org/labkey/api/data/Container.java @@ -20,6 +20,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -827,7 +828,7 @@ public static boolean isLegalName(String name, boolean isProject, StringBuilder return false; } - if (StringUtils.endsWithIgnoreCase(name, ".view") || StringUtils.endsWithIgnoreCase(name, ".api") || StringUtils.endsWithIgnoreCase(name, ".post")) + if (Strings.CI.endsWith(name, ".view") || Strings.CI.endsWith(name, ".api") || Strings.CI.endsWith(name, ".post")) { error.append("Folder name should not end with '.view', '.api', or '.post'."); return false; @@ -1249,7 +1250,7 @@ public boolean getAsBoolean() } // always put the required modules in the set - // note that this will pickup the modules from the folder type's getActiveModules() + // note that this will pick up the modules from the folder type's getActiveModules() Set modules = new HashSet<>(getRequiredModules()); // add all modules found in user preferences: diff --git a/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java b/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java index 811d3375cf0..971d12f6c2c 100644 --- a/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java +++ b/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java @@ -7,7 +7,7 @@ /** * Designates actions that should not enforce forbidden project checking, which is used to support impersonation * container restrictions and project locking. Use with caution. Allows project admins to perform actions outside - * of the project they administer and non-admins the ability to invoke actions in locked projects. + * the project they administer and non-admins the ability to invoke actions in locked projects. * * @see org.labkey.api.action.PermissionCheckableAction#checkPermissions() * @see org.labkey.api.data.Container#isForbiddenProject(org.labkey.api.security.User) 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..f3663d2163f 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; @@ -29,6 +29,104 @@ import java.util.stream.Stream; +/// +/// The ability to impersonate users, groups, and roles is a powerful and useful security feature of LabKey. +/// Administrators use impersonation to review and troubleshoot their permissions configurations. Developers, testers, +/// and automated tests use impersonation widely to develop and validate LabKey features. This comment documents the +/// rules we've developed over the years to ensure impersonation is useful and secure. +/// +/// ### Guiding Principles +/// Before delving into details, here are some guiding principles we've tried to follow while designing the +/// impersonation features: +/// - Those who can impersonate are already trusted, high-level administrators with wide-ranging permissions. +/// - We strive for flexibility in allowing impersonations. Any restrictions should be there for a good reason. +/// - Starting and stopping impersonation is recorded in the audit log. All key actions taken while impersonating are +/// recorded in the audit log with a clear indication of who was impersonating at the time. +/// - Escalation of permissions is prevented in the vast majority of most cases. We intentionally allow limited +/// exceptions to this and accept the audit log record of impersonators' actions as mitigation for this. +/// +/// ### Three Impersonation Options +/// - Impersonating a user effectively turns the admin into that user. However, the impersonating admin is listed in +/// all audit log entries associated with their actions while impersonating. Admins can't impersonate inactive users +/// or themselves. +/// - Impersonating a group means the admin is still themselves (they'll see their own profile, updates will be tagged +/// with their user ID, etc.), but they are stripped of all role assignments except for the roles granted to three +/// groups: Guests, All Site Users, and the impersonated group. This provides the ability to test the effects of +/// a single group's role assignments in isolation. Admins can't impersonate the Guests group; they can just log out +/// to see the site from a guest's perspective. +/// - Impersonating roles means the admin is still themselves, but they are stripped of all role assignments except +/// the role(s) selected during impersonation. Unlike user and group impersonation (which allow a single principal +/// to be impersonated at a time), admins can impersonate multiple roles. They can also impersonate roles and +/// subsequently adjust that impersonation, adding and removing roles from the impersonation session. Note that many +/// roles don't include read permission; the impersonate roles dialog will encourage (though not require) the +/// addition of a read-granting role if the current selection lacks read permission. +/// +/// ### Three Root Roles Can Impersonate +/// - Three roles that are assigned at the root that can impersonate: Site Administrator, Impersonating Troubleshooter, +/// and Application Admin. +/// - Site Administrators and Impersonating Troubleshooters are all-powerful and already have (or can instantly give +/// themselves) any permission in the system. Other than the exceptions mentioned above, they can impersonate any +/// user, group, or role in the system without restriction. +/// - Application Admins can impersonate the same users and groups as the other two root roles. They can impersonate +/// any role except for three especially powerful roles that are designated as "privileged": Site Administrator, +/// Platform Developer, and Impersonating Troubleshooter. Application Admins can't impersonate privileged roles +/// directly, and when they impersonate a user or group, any privileged roles granted to that principal are filtered +/// out during permission checks. As such, an Application Admin can't take on permissions that are exclusive to a +/// Site Administrator or a Platform Developer through impersonation. +/// - These three root roles can impersonate from the root, in which case they have the option to impersonate any +/// user, any site group, or any site role. They can also impersonate from a project or folder, in which case they +/// can impersonate any user, any site group, any project group from that project, or any role that's applicable to +/// the current folder. When they impersonate, they are free to navigate to any container allowed by their new +/// impersonated permissions. +/// - Troubleshooters and other root roles can't impersonate. +/// +/// ### Project Administrators Can Also Impersonate +/// - Project Administrators can impersonate, but with less freedom than the root roles. Project Administrators are +/// granted most permissions in the system, but only in the context of their project, so their impersonation +/// power is limited accordingly. +/// - Unlike the other three impersonating roles, Project Administrators are completely restricted to the project +/// where impersonation starts. While impersonating, they can't navigate to any other project or the root, no +/// matter what their new impersonation permissions grant. +/// - Project Administrators can impersonate any user with read permissions in the project. This isn't a security +/// restriction, since Project Administrators can grant any user read permissions in their project and impersonate +/// them. This limit is a convenience to focus on just the users with access to that project. Some deployments are +/// partitioned with disjoint sets of users in each project; showing all users would be confusing and inconvenient. +/// Also, it's not very useful to impersonate a user who lacks read permissions. +/// - Project Administrators can impersonate any project group in that project or any site group where they are +/// already a member (though see the first "Considerations" point below). +/// - Project Administrators can impersonate any role that's applicable to the current folder. That means they can't +/// impersonate site roles because they can't impersonate from the root. +/// - Like Application Admins, Project Administrators can't impersonate privileged roles; these roles are filtered +/// out when impersonating a user or group that has them. +/// - Folder Administrators have wide-ranging permissions in their folder, but they can't impersonate. +/// +/// ### Miscellaneous Details +/// - A handful of operations are prohibited while impersonating. These include creating API keys, applying an +/// electronic signature, and impersonating (no chaining of impersonations). +/// - The four admins who can impersonate have automatically been granted the vast majority of permissions in the +/// system, but there are some permissions that must be granted explicitly, even to high-level admins. Areas where +/// admins don't automatically receive permissions include PHI reading and adjudication. For these types of +/// permissions, an admin could escalate their permissions while impersonating, receiving permissions they lack +/// normally (although they could easily grant these permissions to themselves). Also, Project Administrators can +/// impersonate an Application Admin and gain at least one new permission within the project (update user profiles); +/// project sandboxing means they can't take any action as an Application Admin in the root or other projects. In +/// both escalation cases, the audit log tags all actions they take with the impersonating admin's user id. +/// - The UI and the API disallow impersonating site roles and project/folder roles at the same time (though see the +/// second "Considerations" point below). +/// +/// ### Considerations +/// - Eliminate the requirement that Project Administrators must be a member of a site group in order to impersonate +/// it? Project Administrators can already impersonate any user (who could be a member of any group) and we already +/// filter out the privileged roles, so this restriction is hard to justify. +/// - Allow impersonation of site roles from the project and eliminate the prohibition on impersonating site roles +/// and project/folder roles at the same time? Admins can already impersonate users and groups that are granted a +/// mix of roles, and we already filter out the privileged roles, so this restriction seems arbitrary and hard to +/// justify. We would eliminate the isApplicable() check on roles, which would also make it easier to impersonate +/// roles with special requirements, such as EHR- or study-related roles. If we took this step, we could likely +/// combine the permissions-checking methods in RoleImpersonationContextFactory (getValidImpersonationRoles() and +/// verifyPermissions()). +/// + public abstract class AbstractImpersonationContext implements PermissionsContext { private final User _adminUser; @@ -88,7 +186,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..ab3c708a955 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,33 +126,6 @@ public User getAdminUser() return UserManager.getUser(_adminUserId); } - private static boolean canImpersonateGroup(@Nullable Container project, User user, 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()) - return true; - - // Project admin... - if (null != project && project.hasPermission(user, AdminPermission.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 false; - } - public static void addMenu(NavTree menu) { NavTree groupMenu = new NavTree("Group"); @@ -160,11 +133,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) @@ -174,6 +148,31 @@ public static Collection getValidImpersonationGroups(Container c, User us return validGroups; } + 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; + + // 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(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 adminUser.isInGroup(group.getUserId()); + } + + return false; + } + private static class GroupImpersonationContext extends AbstractImpersonationContext { private final Group _group; @@ -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/ImpersonationTestCase.java b/api/src/org/labkey/api/security/impersonation/ImpersonationTestCase.java new file mode 100644 index 00000000000..6c6f49ea1a3 --- /dev/null +++ b/api/src/org/labkey/api/security/impersonation/ImpersonationTestCase.java @@ -0,0 +1,157 @@ +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.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 many of them. + */ +public class ImpersonationTestCase extends Assert +{ + private static final Logger LOG = LogHelper.getLogger(ImpersonationTestCase.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 privilegedGroup = 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 + privilegedGroup = SecurityManager.createGroup(root, "Test_Privileged", null); + MutableSecurityPolicy policy = new MutableSecurityPolicy(root.getPolicy()); + Role privilegedRole = RoleManager.getRole(PlatformDeveloperRole.class); + policy.addRoleAssignment(privilegedGroup, 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 (privilegedGroup != null) + SecurityManager.deleteGroup(privilegedGroup, 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 + 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.getValidImpersonationRoles(ContainerManager.getRoot(), user).toList(); + 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.getValidImpersonationRoles(project, user).toList(); + 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..6d8072cb784 100644 --- a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import jakarta.servlet.http.HttpServletRequest; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.audit.AuditLogService; import org.labkey.api.data.Container; @@ -31,9 +32,8 @@ 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; @@ -183,16 +183,72 @@ private static void addMenu(NavTree menu, String text) menu.addChild(newRoleMenu); } - public static Stream getValidImpersonationRoles(Container c, User user) + // Can this user impersonate in this container? + private static boolean canImpersonate(@Nullable Container c, User user) { - SecurityPolicy policy = SecurityPolicyManager.getPolicy(c); - boolean canImpersonatePrivilegedRoles = user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class); - - // Stream the valid roles - return RoleManager.getAllRoles().stream() - .filter(Role::isAssignable) - .filter(role -> role.isApplicable(policy, c)) - .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); + return user.hasRootPermission(ImpersonatePermission.class) || (c != null && !c.isRoot() && c.hasPermission(user, ImpersonatePermission.class)); + } + + public static Stream getValidImpersonationRoles(@NotNull Container c, User user) + { + if (canImpersonate(c, user)) + { + SecurityPolicy policy = SecurityPolicyManager.getPolicy(c); + boolean canImpersonatePrivilegedRoles = user.hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class); + + // Stream the valid roles + return RoleManager.getAllRoles().stream() + .filter(Role::isAssignable) + .filter(role -> role.isApplicable(policy, c)) + .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); + } + else + { + return Stream.empty(); + } + } + + // Throws if user is not authorized to impersonate all roles + private static void verifyPermissions(@Nullable Container project, User adminUser, Set roles, ImpersonationContextFactory factory) + { + if (canImpersonate(project, adminUser)) + { + // null project means a root admin is impersonating. Impersonation could have started in the root, project, or folder... we don't know. + if (null == project) + { + // Ensure we have either site roles or project roles, not both. UI prevents this, but crafty admin could + // attempt it by crafting a post with specific class names + if (roles.stream() + .collect(Collectors.groupingBy(role -> role instanceof AbstractRootContainerRole)) + .size() > 1) + { + throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles and project roles at the same time", factory); + } + + if (!adminUser.hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class)) + { + // Application Administrator is not allowed to impersonate privileged roles + 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"), factory); + } + } + else + { + // 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", factory); + } + } + else + { + // Admin's permissions must have been revoked since impersonation began + throw new UnauthorizedImpersonationException("You are not allowed to impersonate here", factory); + } } public static class RoleImpersonationContext extends AbstractImpersonationContext @@ -202,68 +258,29 @@ 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); } private RoleImpersonationContext( - @Nullable Container project, - User adminUser, - RoleSet roles, - ActionURL returnUrl, - ImpersonationContextFactory factory, - String cacheKey) + @Nullable Container project, + User adminUser, + RoleSet roles, + ActionURL returnUrl, + ImpersonationContextFactory factory, + String cacheKey + ) { super(adminUser, project, returnUrl, factory); _roles = roles; _cacheKey = cacheKey; - verifyPermissions(project, adminUser, _roles.getRoles()); - } - - // Throws if user is not authorized to impersonate all roles - private void verifyPermissions(@Nullable Container project, User user, Set roles) - { - if (null == project) - { - // Ensure we have either site roles or project roles, not both - var map = roles.stream() - .collect(Collectors.groupingBy(role -> role instanceof AbstractRootContainerRole)); - - // UI prevents this, but crafty admin could attempt it by crafting a post with specific class names - 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()); - } - 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()); - } + verifyPermissions(project, adminUser, _roles.getRoles(), factory); } @Override @@ -285,7 +302,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/AdminOperationsPermission.java b/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java index c5ca174f05a..f7bb5f57421 100644 --- a/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java +++ b/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to manage operational site administration settings, such as arbitrary paths on the server's + * Provides the ability to manage operational site administration settings, such as arbitrary paths on the server's * underlying file system. Use {@see org.labkey.api.security.permissions.ApplicationAdminPermission} for gating * access to server-wide settings that don't have the potential to access these underlying server resources. */ diff --git a/api/src/org/labkey/api/security/permissions/AdminPermission.java b/api/src/org/labkey/api/security/permissions/AdminPermission.java index eec7016aa03..e19c2ad60c8 100644 --- a/api/src/org/labkey/api/security/permissions/AdminPermission.java +++ b/api/src/org/labkey/api/security/permissions/AdminPermission.java @@ -25,11 +25,8 @@ import java.util.Set; /** - * Describes the ability to perform administration. Note that this is distinct from {@link org.labkey.api.security.roles.SiteAdminRole}, + * Provides the ability to perform administration. Note that this is distinct from {@link org.labkey.api.security.roles.SiteAdminRole}, * which is far more all-encompassing. - * - * User: Dave - * Date: Apr 28, 2009 */ public class AdminPermission extends AbstractPermission { 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/DeletePermission.java b/api/src/org/labkey/api/security/permissions/DeletePermission.java index d36a7ff08e3..855338d1a06 100644 --- a/api/src/org/labkey/api/security/permissions/DeletePermission.java +++ b/api/src/org/labkey/api/security/permissions/DeletePermission.java @@ -16,9 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to delete some sort of object. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to delete objects */ public class DeletePermission extends AbstractPermission { 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/permissions/InsertPermission.java b/api/src/org/labkey/api/security/permissions/InsertPermission.java index 74bed03132a..93e05a548b7 100644 --- a/api/src/org/labkey/api/security/permissions/InsertPermission.java +++ b/api/src/org/labkey/api/security/permissions/InsertPermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to add new objects to the system. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to add new objects to the system */ public class InsertPermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java b/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java index 3e36d9e7122..6571df30090 100644 --- a/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java +++ b/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to move entities between containers within the system. + * Provides the ability to move entities between containers within the system */ public class MoveEntitiesPermission extends UpdatePermission { diff --git a/api/src/org/labkey/api/security/permissions/ReadPermission.java b/api/src/org/labkey/api/security/permissions/ReadPermission.java index 4a9efc8e3cf..4664db9e7c8 100644 --- a/api/src/org/labkey/api/security/permissions/ReadPermission.java +++ b/api/src/org/labkey/api/security/permissions/ReadPermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to view information within the system. - * User: Dave - * Date: Apr 23, 2009 + * Provides the ability to view information within the system */ @AllowedForReadOnlyUser public class ReadPermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/ReadSomePermission.java b/api/src/org/labkey/api/security/permissions/ReadSomePermission.java index 686ea9b95e9..ba9cab4db27 100644 --- a/api/src/org/labkey/api/security/permissions/ReadSomePermission.java +++ b/api/src/org/labkey/api/security/permissions/ReadSomePermission.java @@ -16,9 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to read a limited subset of information within a specific context. - * User: Dave - * Date: Apr 30, 2009 + * Provides the ability to read a limited subset of information within a specific context */ @AllowedForReadOnlyUser public class ReadSomePermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java b/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java index b1d4be92641..4c54506e959 100644 --- a/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java +++ b/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java @@ -17,7 +17,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to view administration and configuration, but not change it. + * Provides the ability to view administration and configuration, but not change it */ @AllowedForReadOnlyUser public class TroubleshooterPermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/UpdatePermission.java b/api/src/org/labkey/api/security/permissions/UpdatePermission.java index 034d0a65a10..2179b29faaa 100644 --- a/api/src/org/labkey/api/security/permissions/UpdatePermission.java +++ b/api/src/org/labkey/api/security/permissions/UpdatePermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to change existing objects within the system. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to change existing objects within the system */ public class UpdatePermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/UserManagementPermission.java b/api/src/org/labkey/api/security/permissions/UserManagementPermission.java index c5d74da6442..47bce2b2db4 100644 --- a/api/src/org/labkey/api/security/permissions/UserManagementPermission.java +++ b/api/src/org/labkey/api/security/permissions/UserManagementPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to manage users (create, delete, deactivate) for the server. + * Provides the ability to manage users (create, delete, deactivate) */ public class UserManagementPermission extends AdminPermission { 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..6923b44f1d7 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,10 +37,10 @@ public class ApplicationAdminRole extends AbstractRootContainerRole implements A static Collection> PERMISSIONS = Arrays.asList( AddUserPermission.class, ApplicationAdminPermission.class, - CanImpersonateSiteRolesPermission.class, DeleteUserPermission.class, EnableRestrictedModules.class, ExemptFromAccountDisablingPermission.class, + ImpersonatePermission.class, TroubleshooterPermission.class, UpdateUserPermission.class, UserManagementPermission.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/Role.java b/api/src/org/labkey/api/security/roles/Role.java index 64e56dbc2e9..c5a1666c77c 100644 --- a/api/src/org/labkey/api/security/roles/Role.java +++ b/api/src/org/labkey/api/security/roles/Role.java @@ -121,8 +121,8 @@ default boolean isExcludedPrincipal(@NotNull UserPrincipal principal) } /** - * @return Whether this role is applicable to the policy. For example, some roles might only make sense in the context of a - * certain type of resource, such as a folder (or particular type of folder) or dataset + * @return Whether this role is applicable in this resource. For example, some roles might only make sense in the + * context of a dataset or a certain folder type. TODO: Eliminate policy parameter; no implementation uses it. */ boolean isApplicable(SecurityPolicy policy, SecurableResource resource); diff --git a/api/src/org/labkey/api/security/roles/SiteAdminRole.java b/api/src/org/labkey/api/security/roles/SiteAdminRole.java index df248dd1bec..063452eb844 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, CanUseSendMessageApiPermission.class, EmailNonUsersPermission.class, - ExemptFromAccountDisablingPermission.class, + ImpersonatePrivilegedSiteRolesPermission.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..79c3355f700 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,15 +2996,16 @@ 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(); @@ -3009,7 +3015,7 @@ public ApiResponse execute(Object object, BindException errors) 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) diff --git a/core/webapp/Impersonate.js b/core/webapp/Impersonate.js index 653b4accf6d..1bd300291b8 100644 --- a/core/webapp/Impersonate.js +++ b/core/webapp/Impersonate.js @@ -35,7 +35,7 @@ Ext4.define('LABKEY.Security.ImpersonateUser', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.isRootAdmin ? - "As a site or application administrator, you can impersonate any user on the site." + + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate any user on the site." + (!LABKEY.Security.currentUser.isSystemAdmin ? " While impersonating you will not inherit the user's " + "site-level roles (e.g., Site Administrator, Developer)." : "") : @@ -191,7 +191,7 @@ Ext4.define('LABKEY.Security.ImpersonateGroup', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.isRootAdmin ? - "As a site or application administrator, you can impersonate any site or project group." : + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate any site or project group." : "As a project administrator, you can impersonate any project group in this project or any site group in which you're member. While impersonating you will be restricted to this project."; var divContainer = Ext4.create('Ext.container.Container', { @@ -315,7 +315,7 @@ Ext4.define('LABKEY.Security.ImpersonateRoles', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.canImpersonateSiteRoles ? - "As a site administrator, application administrator, or impersonating troubleshooter you can impersonate one or more security roles. While impersonating you will have access to " + + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate one or more security roles. While impersonating you will have access to " + "the entire site, limited to the permissions provided by the selected roles(s)." : "As a project administrator, you can impersonate one or more security roles. While impersonating you will be restricted to this project.";