From 268d1d530de6995e51649e9e631cfa01a2793de7 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Sat, 17 Jan 2026 06:57:32 -0800 Subject: [PATCH] Give Troubleshooters read permission in the root --- api/src/org/labkey/api/security/Group.java | 2 +- .../labkey/api/security/SecurityManager.java | 2 +- .../labkey/api/security/SecurityPolicy.java | 95 ++++++++++++++++++- api/src/org/labkey/api/security/User.java | 5 + .../labkey/api/security/UserPrincipal.java | 3 +- .../impersonation/ImpersonationContext.java | 28 +++++- .../roles/AbstractRootContainerRole.java | 5 + .../security/roles/TroubleshooterRole.java | 10 +- .../labkey/core/admin/AdminController.java | 22 ++--- .../labkey/core/query/CoreQuerySchema.java | 4 +- .../core/security/SecurityController.java | 9 -- 11 files changed, 146 insertions(+), 39 deletions(-) diff --git a/api/src/org/labkey/api/security/Group.java b/api/src/org/labkey/api/security/Group.java index 5f71ea6461c..20e726ec0a8 100644 --- a/api/src/org/labkey/api/security/Group.java +++ b/api/src/org/labkey/api/security/Group.java @@ -111,7 +111,7 @@ public boolean isInGroup(int group) public Stream getAssignedRoles(SecurableResource resource) { SecurityPolicy policy = SecurityPolicyManager.getPolicy(resource); - return policy.getRoles(getGroups()).stream(); + return policy.streamRoles(getGroups()); } @Override diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 72406beaeaf..b226c893f32 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -3376,7 +3376,7 @@ public PrincipalArray getGroups() public Stream getAssignedRoles(SecurableResource resource) { SecurityPolicy policy = SecurityPolicyManager.getPolicy(resource); - return policy.getRoles(getGroups()).stream(); + return policy.streamRoles(getGroups()); } @Override diff --git a/api/src/org/labkey/api/security/SecurityPolicy.java b/api/src/org/labkey/api/security/SecurityPolicy.java index 0b98851a771..b30bddd67b4 100644 --- a/api/src/org/labkey/api/security/SecurityPolicy.java +++ b/api/src/org/labkey/api/security/SecurityPolicy.java @@ -38,15 +38,19 @@ import java.util.Map; import java.util.Set; import java.util.SortedSet; +import java.util.Spliterators; import java.util.TreeSet; import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; /** - * Represents a security policy for a {@link org.labkey.api.security.SecurableResource}. You can get a security policy for a resource - * using SecurityManager.getPolicy(). Note that this class is immutable once constructed, so it may - * be used by multiple threads at the same time. To make changes to an existing policy, construct a new + * Represents a security policy for a {@link org.labkey.api.security.SecurableResource}. You can get a security policy + * for a resource using SecurityManager.getPolicy(). Note that this class is immutable once constructed, so it may be + * used by multiple threads at the same time. To make changes to an existing policy, construct a new * {@link MutableSecurityPolicy} passing the existing SecurityPolicy instance in the constructor. - * Note: intentionally does not implement HasPermission, use that interface for things that have a SecurityPolicy + * Note: intentionally does not implement HasPermission; use that interface for things that have a SecurityPolicy. */ public class SecurityPolicy { @@ -211,6 +215,12 @@ private Set> getOwnPermissions(PrincipalArray princi Set> permClasses = new HashSet<>(); handleRoles(principalArray, role -> permClasses.addAll(role.getPermissions())); + Set> permClasses2 = streamRoles(principalArray) + .flatMap(role -> role.getPermissions().stream()) + .collect(Collectors.toSet()); + + assert permClasses2.equals(permClasses); + return permClasses; } @@ -221,13 +231,18 @@ public Set getRoles(PrincipalArray principalArray) Set roles = new HashSet<>(); handleRoles(principalArray, roles::add); + Set roles2 = streamRoles(principalArray).collect(Collectors.toSet()); + + assert roles.equals(roles2); + return roles; } /* Does not inspect any contextual roles, just the roles explicitly given by this SecurityPolicy */ public boolean hasRole(UserPrincipal principal, Class roleClass) { - return getRoles(principal.getGroups()).contains(RoleManager.getRole(roleClass)); + return streamRoles(principal.getGroups()) + .anyMatch(r -> r.equals(RoleManager.getRole(roleClass))); } private void handleRoles(PrincipalArray principalArray, Consumer consumer) @@ -259,6 +274,70 @@ else if (assignment.getUserId() < principalId) } } + /* Does not return any contextual roles, just the roles explicitly granted by this SecurityPolicy */ + @NotNull + public Stream streamRoles(PrincipalArray principalArray) + { + return StreamSupport.stream( + Spliterators.spliteratorUnknownSize( + new RoleIterator(principalArray, getAssignments()), + 0 + ), + false + ); + } + + private static class RoleIterator implements Iterator + { + private final Iterator _assignmentIterator; + private final List _principals; + + private RoleAssignment _assignment; + private int _principalsIdx = 0; + private Role _nextRole = null; + + public RoleIterator(PrincipalArray principals, SortedSet roleAssignments) + { + _assignmentIterator = roleAssignments.iterator(); + _principals = principals.getList(); + _assignment = _assignmentIterator.hasNext() ? _assignmentIterator.next() : null; + } + + private @Nullable Role getNextRole() + { + while (null != _assignment && _principalsIdx < _principals.size()) + { + int principalId = _principals.get(_principalsIdx); + if (_assignment.getUserId() == principalId) + { + Role role = _assignment.getRole(); + _assignment = _assignmentIterator.hasNext() ? _assignmentIterator.next() : null; + if (null != role) + return role; + } + else if (_assignment.getUserId() < principalId) + _assignment = _assignmentIterator.hasNext() ? _assignmentIterator.next() : null; + else + ++_principalsIdx; + } + + return null; + } + + @Override + public boolean hasNext() + { + _nextRole = getNextRole(); + return _nextRole != null; + } + + @Override + public Role next() + { + return _nextRole; + } + } + @Nullable public Date getModified() { @@ -345,12 +424,18 @@ public Map>> getAssignmentsAsMap( public boolean hasNonInheritedPermission(@NotNull UserPrincipal principal, Class perm) { + boolean ret = streamRoles(new PrincipalArray(List.of(principal.getUserId()))).anyMatch(role -> role.getPermissions().contains(perm)); + for (Role role : getRoles(new PrincipalArray(List.of(principal.getUserId())))) { if (role.getPermissions().contains(perm)) + { + assert ret; return true; + } } + assert !ret; return false; } } diff --git a/api/src/org/labkey/api/security/User.java b/api/src/org/labkey/api/security/User.java index b842a0d5742..e83baffeac9 100644 --- a/api/src/org/labkey/api/security/User.java +++ b/api/src/org/labkey/api/security/User.java @@ -362,6 +362,11 @@ public final Stream getSiteRoles() return _impersonationContext.getSiteRoles(this); } + public final Stream getSiteRoles(SecurableResource resource) + { + return _impersonationContext.getSiteRoles(this, resource); + } + @Override public Object clone() throws CloneNotSupportedException { diff --git a/api/src/org/labkey/api/security/UserPrincipal.java b/api/src/org/labkey/api/security/UserPrincipal.java index 15473cad268..1f33d25e60e 100644 --- a/api/src/org/labkey/api/security/UserPrincipal.java +++ b/api/src/org/labkey/api/security/UserPrincipal.java @@ -146,7 +146,8 @@ public JdbcType getJdbcParameterType() public boolean hasPrivilegedRole() { // Check for any privileged role assigned to this principal at the root - return ContainerManager.getRoot().getPolicy().getRoles(getGroups()).stream().anyMatch(Role::isPrivileged); + return ContainerManager.getRoot().getPolicy().streamRoles(getGroups()) + .anyMatch(Role::isPrivileged); } @Override diff --git a/api/src/org/labkey/api/security/impersonation/ImpersonationContext.java b/api/src/org/labkey/api/security/impersonation/ImpersonationContext.java index 5b74e1e53c2..a0150fc7cdb 100644 --- a/api/src/org/labkey/api/security/impersonation/ImpersonationContext.java +++ b/api/src/org/labkey/api/security/impersonation/ImpersonationContext.java @@ -25,6 +25,7 @@ import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; import org.labkey.api.security.permissions.Permission; +import org.labkey.api.security.roles.AbstractRootContainerRole; import org.labkey.api.security.roles.NoPermissionsRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; @@ -58,15 +59,16 @@ public interface ImpersonationContext extends Serializable */ default Stream getAssignedRoles(User user, SecurableResource resource) { - Stream roles = getSiteRoles(user); + Stream roles = getSiteRoles(user, resource); SecurityPolicy policy = SecurityPolicyManager.getPolicy(resource); - return Streams.concat(roles, policy.getRoles(user.getGroups()).stream()).distinct(); + return Streams.concat(roles, policy.streamRoles(user.getGroups())).distinct(); } /** * @return The roles assigned to this user in the root. The roles may be modified and/or filtered by the * impersonation context. */ + @Deprecated default Stream getSiteRoles(User user) { Container root = ContainerManager.getRoot(); @@ -79,9 +81,29 @@ default Stream getSiteRoles(User user) return roles.stream(); } + /** + * @return The roles assigned to this user in the root that are applicable to the passed in resource. The roles may + * be modified and/or filtered by the impersonation context. + */ + default Stream getSiteRoles(User user, SecurableResource resource) + { + Container root = ContainerManager.getRoot(); + SecurityPolicy policy = root.getPolicy(); + return policy.streamRoles(getGroups(user)) + .filter(role -> !role.equals(RoleManager.getRole(NoPermissionsRole.class))) + .filter(role -> { + if (!role.isApplicable(policy, root)) + throw new IllegalStateException("Root role " + role.getName() + " is not applicable"); + if (!(role instanceof AbstractRootContainerRole siteRole)) + throw new IllegalStateException("Root roles should all be AbstractRootContainerRole"); + + return siteRole.isAvailableEverywhere() || resource.equals(ContainerManager.getRoot()); + }); + } + ImpersonationContextFactory getFactory(); - /** Responsible for adding menu items to allow the user to initiate or stop impersonating, based on the current state */ + /** Responsible for adding menu items to allow the user to initiate, adjust, or stop impersonating, based on the current state */ void addMenu(NavTree menu, Container c, User user, ActionURL currentURL); // restrict the permissions this user is allowed diff --git a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java index 774365591e0..4ccb841b53a 100644 --- a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java +++ b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java @@ -60,4 +60,9 @@ public boolean isApplicable(SecurityPolicy policy, SecurableResource resource) { return resource instanceof Container && ((Container)resource).isRoot(); } + + public boolean isAvailableEverywhere() + { + return true; + } } diff --git a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java index ddd6a1af79b..03c4da04936 100644 --- a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java +++ b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java @@ -17,6 +17,7 @@ import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.security.permissions.Permission; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.SeeUserDetailsPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; @@ -28,11 +29,18 @@ public class TroubleshooterRole extends AbstractRootContainerRole static Collection> PERMISSIONS = Set.of( TroubleshooterPermission.class, SeeUserDetailsPermission.class, - CanSeeAuditLogPermission.class + CanSeeAuditLogPermission.class, + ReadPermission.class ); public TroubleshooterRole() { super("Troubleshooter", "Troubleshooters may view administration settings but may not change them.", PERMISSIONS); } + + @Override + public boolean isAvailableEverywhere() + { + return false; // This ensures troubleshooters get these permissions (esp. ReadPermission) only in the root + } } \ No newline at end of file diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 19266ce4987..ab6f39e61fd 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -2640,7 +2640,8 @@ public void addNavTrail(NavTree root) } } - @AdminConsoleAction + // This allows Troubleshooters to GET and POST to the action, supporting export to Excel and script, e.g. + @RequiresPermission(TroubleshooterPermission.class) public class PostgresStatActivityAction extends AbstractPostgresAction { public PostgresStatActivityAction() @@ -2649,7 +2650,7 @@ public PostgresStatActivityAction() } } - @AdminConsoleAction + @RequiresPermission(TroubleshooterPermission.class) public class PostgresLocksAction extends AbstractPostgresAction { public PostgresLocksAction() @@ -2658,7 +2659,7 @@ public PostgresLocksAction() } } - @AdminConsoleAction + @RequiresPermission(TroubleshooterPermission.class) public class PostgresTableSizesAction extends AbstractPostgresAction { public PostgresTableSizesAction() @@ -3579,16 +3580,6 @@ protected AbstractAdminQueryAction(String schemaName, String queryName) _queryName = queryName; } - @Override - public void setViewContext(ViewContext context) - { - // Troubleshooters don't have read permissions but DataRegion requires it. I don't love poking an elevated - // user into the ViewContext, but this is the only way I could get DataRegion to see read permission on - // tables that are wrapped by a query (e.g., core.Documents used by DocumentsGroupedByParentType.sql). - context.setUser(ElevatedUser.getElevatedUser(context.getUser(), ReaderRole.class)); - super.setViewContext(context); - } - @Override protected QueryView createQueryView(QueryExportForm form, BindException errors, boolean forExport, @Nullable String dataRegion) throws Exception { @@ -3609,7 +3600,8 @@ protected String getQueryName() abstract protected UserSchema getUserSchema(); } - @AdminConsoleAction + // This allows Troubleshooters to GET and POST to the action, supporting export to Excel and script, e.g. + @RequiresPermission(TroubleshooterPermission.class) public class AttachmentsAction extends AbstractAdminQueryAction { @SuppressWarnings("unused") // Invoked via reflection @@ -3632,7 +3624,7 @@ public void addNavTrail(NavTree root) } @SuppressWarnings("unused") // Linked from core.DocumentsGroupedByParentTypeAdmin - @AdminConsoleAction + @RequiresPermission(TroubleshooterPermission.class) public class AttachmentsForTypeAction extends AbstractAdminQueryAction { @SuppressWarnings("unused") // Invoked via reflection diff --git a/core/src/org/labkey/core/query/CoreQuerySchema.java b/core/src/org/labkey/core/query/CoreQuerySchema.java index 1606e415333..bc8bdcc2f0b 100644 --- a/core/src/org/labkey/core/query/CoreQuerySchema.java +++ b/core/src/org/labkey/core/query/CoreQuerySchema.java @@ -142,8 +142,7 @@ public Set getTableNames() CONTAINERS_TABLE_NAME, WORKBOOKS_TABLE_NAME, QCSTATE_TABLE_NAME, DATA_STATES_TABLE_NAME, VIEW_CATEGORY_TABLE_NAME, MISSING_VALUE_INDICATOR_TABLE_NAME); - // Don't show troubleshooters the query in the schema browser since query-execute.view requires read permissions - if (getUser().hasRootPermission(ApplicationAdminPermission.class)) + if (getUser().hasRootPermission(TroubleshooterPermission.class)) names.add(DOCUMENTS_TABLE_NAME); if (getUser().hasRootPermission(UserManagementPermission.class)) @@ -204,7 +203,6 @@ public TableInfo createTable(String name, ContainerFilter cf) return getMVIndicatorTable(cf); if (SHORT_URL_TABLE_NAME.equalsIgnoreCase(name) && ShortUrlTableInfo.canDisplayTable(getUser(), getContainer())) return new ShortUrlTableInfo(this); - // Allow troubleshooters to view this query from the admin console if (DOCUMENTS_TABLE_NAME.equalsIgnoreCase(name) && getUser().hasRootPermission(TroubleshooterPermission.class)) return new DocumentsTable(this, cf); diff --git a/core/src/org/labkey/core/security/SecurityController.java b/core/src/org/labkey/core/security/SecurityController.java index 5e4dec6e87f..136f6cf612c 100644 --- a/core/src/org/labkey/core/security/SecurityController.java +++ b/core/src/org/labkey/core/security/SecurityController.java @@ -77,7 +77,6 @@ import org.labkey.api.security.LoginManager; import org.labkey.api.security.MemberType; import org.labkey.api.security.MutableSecurityPolicy; -import org.labkey.api.security.PrincipalArray; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; @@ -721,14 +720,6 @@ private ModelAndView renderGroup(Group group, BindException errors, List roles = ContainerManager.getRoot().getPolicy().getRoles(new PrincipalArray(List.of(group.getUserId()))); - if (!roles.contains(role)) - errors.reject(ERROR_MSG, "Warning: This group is not assigned its standard role, " - + role.getDisplayName() + "! Consider assigning it on the Site Permissions page."); - } - @RequiresPermission(AdminPermission.class) public class GroupAction extends FormViewAction {