diff --git a/api/src/org/labkey/api/reports/report/view/ReportUtil.java b/api/src/org/labkey/api/reports/report/view/ReportUtil.java index 2d7991ece6d..69dfb11332e 100644 --- a/api/src/org/labkey/api/reports/report/view/ReportUtil.java +++ b/api/src/org/labkey/api/reports/report/view/ReportUtil.java @@ -738,10 +738,10 @@ public static void updateReportSecurityPolicy(ViewContext context, @NotNull Repo { MutableSecurityPolicy policy = new MutableSecurityPolicy(report.getDescriptor(), SecurityPolicyManager.getPolicy(report.getDescriptor(), false)); - List principalAssignedRoles = policy.getAssignedRoles(principal); - if (toAdd && principalAssignedRoles.isEmpty()) + boolean hasNoAssignedRoles = policy.getAssignedRoles(principal).findAny().isEmpty(); + if (toAdd && hasNoAssignedRoles) policy.addRoleAssignment(principal, ReaderRole.class); - else if (!toAdd && !principalAssignedRoles.isEmpty()) + else if (!toAdd && !hasNoAssignedRoles) policy.addRoleAssignment(principal, NoPermissionsRole.class); SecurityPolicyManager.savePolicy(policy, context.getUser()); diff --git a/api/src/org/labkey/api/security/Group.java b/api/src/org/labkey/api/security/Group.java index 5f71ea6461c..bb491eb4caa 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.getRoles(getGroups()); } @Override diff --git a/api/src/org/labkey/api/security/LimitedUser.java b/api/src/org/labkey/api/security/LimitedUser.java index 207050584a2..d86a6d0b443 100644 --- a/api/src/org/labkey/api/security/LimitedUser.java +++ b/api/src/org/labkey/api/security/LimitedUser.java @@ -123,6 +123,7 @@ public void testElevatedUser() { User user = TestContext.get().getUser(); Container c = JunitUtil.getTestContainer(); + Container root = ContainerManager.getRoot(); testPermissions(ElevatedUser.getElevatedUser(new LimitedUser(user, SubmitterRole.class, null), ReaderRole.class, null), 2, true, true, false, false, false); testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, new LimitedUser(user)), 1, false, false, false, false, true); @@ -131,18 +132,18 @@ public void testElevatedUser() int groupCount = user.getGroups().size(); int roleCount = (int)user.getAssignedRoles(c).count(); - int siteRolesCount = (int)user.getSiteRoles().count(); + int siteRolesCount = (int)user.getSiteRoles(root).count(); User elevated = ElevatedUser.getElevatedUser(user); assertEquals(groupCount, elevated.getGroups().size()); assertEquals(roleCount, (int)elevated.getAssignedRoles(c).count()); - assertEquals(siteRolesCount, (int)elevated.getSiteRoles().count()); + assertEquals(siteRolesCount, (int)elevated.getSiteRoles(root).count()); } private void testPermissions(User user, int roleCount, boolean hasRead, boolean hasInsert, boolean hasUpdate, boolean hasAdmin, boolean hasCanSeeAuditLog) { Container c = JunitUtil.getTestContainer(); assertEquals(roleCount, (int)user.getAssignedRoles(c).count()); - assertTrue(user.getSiteRoles().findAny().isEmpty()); + assertTrue(user.getSiteRoles(ContainerManager.getRoot()).findAny().isEmpty()); assertFalse(user.hasSiteAdminPermission()); assertEquals(0, user.getGroups().stream().count()); assertFalse(user.hasPrivilegedRole()); diff --git a/api/src/org/labkey/api/security/RoleSet.java b/api/src/org/labkey/api/security/RoleSet.java index 10b1b03ed6c..85b666b4f2c 100644 --- a/api/src/org/labkey/api/security/RoleSet.java +++ b/api/src/org/labkey/api/security/RoleSet.java @@ -20,6 +20,7 @@ import org.junit.Assert; import org.junit.Test; import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; import org.labkey.api.pipeline.PipelineJob; import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; import org.labkey.api.security.roles.CanSeeAuditLogRole; @@ -153,7 +154,7 @@ private void testImpersonateRoles(User adminUser, @Nullable Container project, C impersonatingUser.setImpersonationContext(factory.getImpersonationContext()); if (null == project) - assertEquals(roles, impersonatingUser.getSiteRoles().collect(Collectors.toSet())); + assertEquals(roles, impersonatingUser.getSiteRoles(ContainerManager.getRoot()).collect(Collectors.toSet())); else assertEquals(roles, impersonatingUser.getAssignedRoles(project).collect(Collectors.toSet())); @@ -161,7 +162,7 @@ private void testImpersonateRoles(User adminUser, @Nullable Container project, C User reconstitutedUser = mapper.readValue(json, User.class); if (null == project) - assertEquals(roles, reconstitutedUser.getSiteRoles().collect(Collectors.toSet())); + assertEquals(roles, reconstitutedUser.getSiteRoles(ContainerManager.getRoot()).collect(Collectors.toSet())); else assertEquals(roles, reconstitutedUser.getAssignedRoles(project).collect(Collectors.toSet())); } diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 72406beaeaf..607ce340b85 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -1950,7 +1950,7 @@ public static Collection getFolderUserids(Container c) //don't filter if all site users is playing a role Group allSiteUsers = getGroup(Group.groupUsers); - if (!policy.getAssignedRoles(allSiteUsers).isEmpty()) + if (policy.getAssignedRoles(allSiteUsers).findAny().isPresent()) { // Just select all users SQLFragment sql = new SQLFragment("SELECT u.UserId FROM "); @@ -3122,6 +3122,8 @@ public static List getPermissionNames(SecurableResource resource, @NotNu * appropriate only for generating reports about role assignments for administrators. * Returns the roles the principal is playing in this securable resource, either due to direct assignment or due * to membership in a group that is assigned the role. + * Note: The returned stream may duplicate some roles; if a distinct stream of roles is required, callers should + * invoke {@code distinct()} or collect to a set. * @param principal The principal * @return The roles this principal is playing in the securable resource */ @@ -3376,7 +3378,7 @@ public PrincipalArray getGroups() public Stream getAssignedRoles(SecurableResource resource) { SecurityPolicy policy = SecurityPolicyManager.getPolicy(resource); - return policy.getRoles(getGroups()).stream(); + return policy.getRoles(getGroups()); } @Override @@ -3560,9 +3562,13 @@ public boolean performChecks() // check that the user has the expected role Container rootContainer = ContainerManager.getRoot(); User user = UserManager.getUser(userEmail); - Collection roles = rootContainer.getPolicy().getAssignedRoles(user); + assertNotNull(user); Role role = RoleManager.getRole(TEST_USER_1_ROLE_NAME); - assertTrue("The user defined in the startup properties: " + userEmail + " did not have the specified role: " + TEST_USER_1_ROLE_NAME, roles.contains(role)); + assertTrue( + "The user defined in the startup properties: " + userEmail + " did not have the specified role: " + TEST_USER_1_ROLE_NAME, + rootContainer.getPolicy().getAssignedRoles(user) + .anyMatch(r -> r.equals(role)) + ); // delete the test user that was added UserManager.deleteUser(user.getUserId()); @@ -3598,9 +3604,14 @@ public boolean performChecks() // check that the group has the expected role Group group = GroupManager.getGroup(rootContainer, TEST_GROUP_1_NAME, GroupEnumType.SITE); - Collection roles = rootContainer.getPolicy().getAssignedRoles(group); + assertNotNull(group); Role role = RoleManager.getRole(TEST_GROUP_1_ROLE_NAME); - assertTrue("The group defined in the startup properties: " + TEST_GROUP_1_NAME + " did not have the specified role: " + TEST_GROUP_1_ROLE_NAME, roles.contains(role)); + assertNotNull(role); + assertTrue( + "The group defined in the startup properties: " + TEST_GROUP_1_NAME + " did not have the specified role: " + TEST_GROUP_1_ROLE_NAME, + rootContainer.getPolicy().getAssignedRoles(group) + .anyMatch(r -> r.equals(role)) + ); // delete the test group that was added deleteGroup(group, TestContext.get().getUser()); diff --git a/api/src/org/labkey/api/security/SecurityPolicy.java b/api/src/org/labkey/api/security/SecurityPolicy.java index 0b98851a771..06097404581 100644 --- a/api/src/org/labkey/api/security/SecurityPolicy.java +++ b/api/src/org/labkey/api/security/SecurityPolicy.java @@ -15,7 +15,6 @@ */ package org.labkey.api.security; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -26,31 +25,33 @@ import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.util.logging.LogHelper; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.SortedSet; +import java.util.Spliterator; +import java.util.Spliterators; import java.util.TreeSet; -import java.util.function.Consumer; +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 { - private static final Logger LOG = LogManager.getLogger(SecurityPolicy.class); + private static final Logger LOG = LogHelper.getLogger(SecurityPolicy.class, "Unregistered permission warnings"); protected final SortedSet _assignments = new TreeSet<>(); protected final String _resourceId; @@ -149,43 +150,38 @@ public SortedSet getAssignments() } /** - * Returns only the roles directly assigned to this principal - * (not other roles the principal is playing due to group - * memberships). + * Returns only the roles directly assigned to this principal (not other roles the principal is playing due to group + * memberships). Since a single principal can't be directly assigned the same role twice, the returned roles should + * be distinct (no duplicates). * @param principal The principal * @return The roles this principal is directly assigned */ @NotNull - public List getAssignedRoles(@NotNull UserPrincipal principal) + public Stream getAssignedRoles(@NotNull UserPrincipal principal) { - List roles = new ArrayList<>(); - for (RoleAssignment assignment : _assignments) - { - if (assignment.getUserId() == principal.getUserId()) - roles.add(assignment.getRole()); - } - return roles; + return _assignments.stream() + .filter(assignment -> assignment.getUserId() == principal.getUserId()) + .map(RoleAssignment::getRole); } - /** - * Return set of permissions explicitly granted by this SecurityPolicy, will not inspect any - * contextual roles (does not call UserPrincipal.getContextualRoles()). E.g. this will not - * reflect any permission granted due to assignment of site-wide roles, and it will not reflect - * permission filtering by the impersonation context. + * Return stream of permissions explicitly granted by this SecurityPolicy, will not inspect any contextual roles + * (does not call UserPrincipal.getContextualRoles()). E.g. this will not reflect any permission granted due to + * assignment of site-wide roles, and it will not reflect permission filtering by the impersonation context. + * Note: The permissions returned are not distinct, i.e., they may be duplicated. This shouldn't matter for most + * cases (e.g., existence checks); if a distinct set of permissions is needed, invoke distinct() or collect to a set. */ @NotNull - public Set> getOwnPermissions(@NotNull UserPrincipal principal) + public Stream> getOwnPermissions(@NotNull UserPrincipal principal) { - return getOwnPermissions(principal.getGroups()); + return getRoles(principal.getGroups()) + .flatMap(role -> role.getPermissions().stream()); } - /** - * Returns true if this policy is empty (i.e., no role assignments). - * This method is useful for distinguishing between a policy that has - * been established for a SecurableResource and a cached "miss" - * (i.e., no explicit policy defined). + * Returns true if this policy is empty (i.e., no role assignments). This method is useful for distinguishing + * between a policy that has been established for a SecurableResource and a cached "miss" (i.e., no explicit + * policy defined). * @return True if this policy is empty */ public boolean isEmpty() @@ -205,57 +201,84 @@ static void testPermissionIsRegistered(Class permission) } /* Does not inspect any contextual roles, just the roles explicitly given by this SecurityPolicy */ - @NotNull - private Set> getOwnPermissions(PrincipalArray principalArray) + public boolean hasRole(UserPrincipal principal, Class roleClass) { - Set> permClasses = new HashSet<>(); - handleRoles(principalArray, role -> permClasses.addAll(role.getPermissions())); - - return permClasses; + Role targetRole = RoleManager.getRole(roleClass); + return getRoles(principal.getGroups()) + .anyMatch(r -> r.equals(targetRole)); } - /* Does not inspect any contextual roles, just the roles explicitly given by this SecurityPolicy */ + /** + * Does not return any contextual roles, just the roles explicitly granted by this SecurityPolicy. + * Note: The returned stream may duplicate some roles; if a distinct stream of roles is required, callers should + * invoke {@code distinct()} or collect to a set. + **/ @NotNull - public Set getRoles(PrincipalArray principalArray) + public Stream getRoles(PrincipalArray principalArray) { - Set roles = new HashSet<>(); - handleRoles(principalArray, roles::add); - - return roles; + SortedSet assignments = getAssignments(); + return StreamSupport.stream( + Spliterators.spliterator( + new RoleIterator(principalArray, assignments), + // Estimate: one role per assigment, so this is the max size + assignments.size(), + // Not guaranteed to be DISTINCT or SIZED (though we do provide an estimated size above). + // Likely that none of this is important, since we're not specifying parallel. + Spliterator.IMMUTABLE | Spliterator.NONNULL + ), + // Iterator-based Spliterators don't work well with parallel. Plus the iterator is computationally simple. + false + ); } - /* Does not inspect any contextual roles, just the roles explicitly given by this SecurityPolicy */ - public boolean hasRole(UserPrincipal principal, Class roleClass) + private static class RoleIterator implements Iterator { - return getRoles(principal.getGroups()).contains(RoleManager.getRole(roleClass)); - } + private final Iterator _assignmentIterator; + private final List _principals; - private void handleRoles(PrincipalArray principalArray, Consumer consumer) - { - List principals = principalArray.getList(); + private RoleAssignment _assignment; + private int _principalsIdx = 0; + private Role _nextRole = null; - //role assignments are sorted by user id, - //as are the principal ids, - //so iterate over both of them in one pass - Iterator assignmentIter = getAssignments().iterator(); - RoleAssignment assignment = assignmentIter.hasNext() ? assignmentIter.next() : null; - int principalsIdx = 0; + public RoleIterator(PrincipalArray principals, SortedSet roleAssignments) + { + _assignmentIterator = roleAssignments.iterator(); + _principals = principals.getList(); + _assignment = _assignmentIterator.hasNext() ? _assignmentIterator.next() : null; + } - while (null != assignment && principalsIdx < principals.size()) + private @Nullable Role getNextRole() { - int principalId = principals.get(principalsIdx); - if (assignment.getUserId() == principalId) + while (null != _assignment && _principalsIdx < _principals.size()) { - Role role = assignment.getRole(); - if (null != role) - consumer.accept(role); - - assignment = assignmentIter.hasNext() ? assignmentIter.next() : null; + 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; } - else if (assignment.getUserId() < principalId) - assignment = assignmentIter.hasNext() ? assignmentIter.next() : null; - else - ++principalsIdx; + + return null; + } + + @Override + public boolean hasNext() + { + _nextRole = getNextRole(); + return _nextRole != null; + } + + @Override + public Role next() + { + return _nextRole; } } @@ -345,12 +368,7 @@ public Map>> getAssignmentsAsMap( public boolean hasNonInheritedPermission(@NotNull UserPrincipal principal, Class perm) { - for (Role role : getRoles(new PrincipalArray(List.of(principal.getUserId())))) - { - if (role.getPermissions().contains(perm)) - return true; - } - - return false; + return getRoles(new PrincipalArray(List.of(principal.getUserId()))) + .anyMatch(role -> role.getPermissions().contains(perm)); } } diff --git a/api/src/org/labkey/api/security/TestSecurityPolicy.java b/api/src/org/labkey/api/security/TestSecurityPolicy.java index 7bafeaa1ac1..3f588c73fc9 100644 --- a/api/src/org/labkey/api/security/TestSecurityPolicy.java +++ b/api/src/org/labkey/api/security/TestSecurityPolicy.java @@ -14,6 +14,6 @@ public TestSecurityPolicy(@NotNull SecurableResource resource) public boolean hasPermission(@NotNull UserPrincipal principal, @NotNull Class permission) { - return getOwnPermissions(principal).contains(permission); + return getOwnPermissions(principal).anyMatch(permClass -> permClass == permission); } } diff --git a/api/src/org/labkey/api/security/User.java b/api/src/org/labkey/api/security/User.java index b842a0d5742..d4f255d3f64 100644 --- a/api/src/org/labkey/api/security/User.java +++ b/api/src/org/labkey/api/security/User.java @@ -357,9 +357,9 @@ public JSONObject getUserProps() return User.getUserProps(this); } - public final Stream getSiteRoles() + public final Stream getSiteRoles(SecurableResource resource) { - return _impersonationContext.getSiteRoles(this); + return _impersonationContext.getSiteRoles(this, resource); } @Override diff --git a/api/src/org/labkey/api/security/UserPrincipal.java b/api/src/org/labkey/api/security/UserPrincipal.java index 15473cad268..4d249a518e3 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().getRoles(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..15a0138f32c 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; @@ -32,7 +33,6 @@ import org.labkey.api.view.NavTree; import java.io.Serializable; -import java.util.Set; import java.util.stream.Stream; /** @@ -54,34 +54,39 @@ public interface ImpersonationContext extends Serializable /** * @return The roles assigned to this user in the provided resource's policy as well as the root. The roles may be - * modified and/or filtered by the impersonation context. + * modified and/or filtered by the impersonation context. Note: The returned stream may duplicate some roles; if a + * distinct stream of roles is required, callers should invoke {@code distinct()} or collect to a set. */ 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.getRoles(user.getGroups())); } /** - * @return The roles assigned to this user in the root. The roles may be modified and/or filtered by the - * impersonation context. + * @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) + default Stream getSiteRoles(User user, SecurableResource resource) { Container root = ContainerManager.getRoot(); SecurityPolicy policy = root.getPolicy(); - Set roles = policy.getRoles(getGroups(user)); - roles.remove(RoleManager.getRole(NoPermissionsRole.class)); - for (Role role : roles) - assert role.isApplicable(policy, root); + return policy.getRoles(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 roles.stream(); + 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/impersonation/RoleImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java index caa50dbad12..4f872623667 100644 --- a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java @@ -278,7 +278,7 @@ public PrincipalArray getGroups(User user) } @Override - public Stream getSiteRoles(User user) + public Stream getSiteRoles(User user, SecurableResource resource) { // Return the site roles that are being impersonated return _roles.stream().filter(role->role instanceof AbstractRootContainerRole); 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/api/src/org/labkey/api/util/SelectBuilder.java b/api/src/org/labkey/api/util/SelectBuilder.java index 33a4e1dd5e4..7145c4f0a51 100644 --- a/api/src/org/labkey/api/util/SelectBuilder.java +++ b/api/src/org/labkey/api/util/SelectBuilder.java @@ -59,7 +59,7 @@ public SelectBuilder addOption(String label, String value) } /** - * Adds multiple options to the <select> elements by supplying a collection of objects. See {@link #addOptions(Stream)} + * Adds multiple options to the <select> element by supplying a collection of objects. See {@link #addOptions(Stream)} * for more details. * * @param options A collection of Options, OptionBuilders, Strings, or Objects @@ -83,24 +83,24 @@ public SelectBuilder addOptions(@NotNull Collection options) public SelectBuilder addOptions(@NotNull Stream options) { options - .filter(Objects::nonNull) - .map(o -> { - if (o instanceof OptionBuilder.Option) - { - return (OptionBuilder.Option) o; - } - else if (o instanceof OptionBuilder) - { - return ((OptionBuilder) o).build(); - } - else - { - String value = o.toString(); - return new OptionBuilder(value, value).build(); - } - - }) - .forEach(_options::add); + .filter(Objects::nonNull) + .map(o -> { + if (o instanceof OptionBuilder.Option) + { + return (OptionBuilder.Option) o; + } + else if (o instanceof OptionBuilder) + { + return ((OptionBuilder) o).build(); + } + else + { + String value = o.toString(); + return new OptionBuilder(value, value).build(); + } + + }) + .forEach(_options::add); return this; } @@ -114,8 +114,8 @@ else if (o instanceof OptionBuilder) public SelectBuilder addOptions(Map options) { return addOptions( - options.entrySet().stream() - .map(e -> new OptionBuilder(e.getValue(), e.getKey()).build()) + options.entrySet().stream() + .map(e -> new OptionBuilder(e.getValue(), e.getKey()).build()) ); } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index f959bf5aa1e..0d34bf56ce1 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -644,7 +644,7 @@ private void sendSystemReadyEmail(List users) // For audit purposes, we use the first user as the originator of the message. // Would be better to have this be a site admin, but we aren't guaranteed to have such a user // for hosted sites. Another option is to use the guest user here, but that's strange. - svc.sendMessages(messages, users.get(0), ContainerManager.getRoot()); + svc.sendMessages(messages, users.getFirst(), ContainerManager.getRoot()); } private @Nullable String getValue(Map map, StashedStartupProperties prop) @@ -1594,15 +1594,17 @@ public void enumerateDocuments(SearchService.TaskIndexingQueue queue, Date since properties.put(SearchService.PROPERTY.categories.toString(), SearchService.navigationCategory.getName()); ActionURL startURL = PageFlowUtil.urlProvider(ProjectUrls.class).getStartURL(c); startURL.setExtraPath(c.getId()); - WebdavResource doc = new SimpleDocumentResource(c.getParsedPath(), - "link:" + c.getId(), - c.getEntityId(), - "text/plain", - body, - startURL, - UserManager.getUser(c.getCreatedBy()), c.getCreated(), - null, null, - properties); + WebdavResource doc = new SimpleDocumentResource( + c.getParsedPath(), + "link:" + c.getId(), + c.getEntityId(), + "text/plain", + body, + startURL, + UserManager.getUser(c.getCreatedBy()), c.getCreated(), + null, null, + properties + ); queue.addResource(doc); }; r.run(); @@ -1672,8 +1674,9 @@ private void populateSiteSettingsWithStartupProps() if (supportFolder != null) { MutableSecurityPolicy supportPolicy = new MutableSecurityPolicy(supportFolder.getPolicy()); - for (Role assignedRole : supportPolicy.getAssignedRoles(guests)) - supportPolicy.removeRoleAssignment(guests, assignedRole); + supportPolicy.getAssignedRoles(guests).forEach(assignedRole -> + supportPolicy.removeRoleAssignment(guests, assignedRole) + ); SecurityPolicyManager.savePolicy(supportPolicy, User.getAdminServiceUser()); } } 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..a6ee08fece4 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 { @@ -1217,19 +1208,19 @@ private void addAuditEvent(User user, String comment, Group group) private void addAuditEvent(Group group, SecurityPolicy newPolicy, SecurityPolicy oldPolicy, AuditChangeType changeType) { Role oldRole = RoleManager.getRole(NoPermissionsRole.class); - if(null != oldPolicy) + if (null != oldPolicy) { - List oldRoles = oldPolicy.getAssignedRoles(group); - if(!oldRoles.isEmpty()) - oldRole = oldRoles.get(0); + oldRole = oldPolicy.getAssignedRoles(group) + .findFirst() + .orElse(oldRole); } Role newRole = RoleManager.getRole(NoPermissionsRole.class); - if(null != newPolicy) + if (null != newPolicy) { - List newRoles = newPolicy.getAssignedRoles(group); - if(!newRoles.isEmpty()) - newRole = newRoles.get(0); + newRole = newPolicy.getAssignedRoles(group) + .findFirst() + .orElse(newRole); } switch (changeType) @@ -1619,7 +1610,7 @@ private void handleDirectRoleAssignments(User user, BiConsumer roles = policy.getAssignedRoles(user); + Collection roles = policy.getAssignedRoles(user).toList(); if (!roles.isEmpty()) { @@ -2244,18 +2235,12 @@ public void addNavTrail(NavTree root) public static void fillUserAccessGroups(UserPrincipal user, List groups, SecurityPolicy policy, Collection effectiveRoles, Map> userAccessGroups) { - for (Group group : groups) - { - if (user.isInGroup(group.getUserId())) - { - Collection groupRoles = policy.getAssignedRoles(group); - for (Role role : effectiveRoles) - { - if (groupRoles.contains(role)) - userAccessGroups.get(role.getName()).add(group); - } - } - } + groups.stream().filter(group -> user.isInGroup(group.getUserId())).forEach(group -> { + Collection groupRoles = policy.getAssignedRoles(group).collect(Collectors.toSet()); + effectiveRoles.stream() + .filter(groupRoles::contains) + .forEach(role -> userAccessGroups.get(role.getName()).add(group)); + }); } public static class FolderAccessForm diff --git a/pipeline/src/org/labkey/pipeline/permission.jsp b/pipeline/src/org/labkey/pipeline/permission.jsp index 103ae76bb8b..5562e819722 100644 --- a/pipeline/src/org/labkey/pipeline/permission.jsp +++ b/pipeline/src/org/labkey/pipeline/permission.jsp @@ -28,9 +28,9 @@ <%@ page import="org.labkey.api.security.roles.Role" %> <%@ page import="org.labkey.api.security.roles.RoleManager" %> <%@ page import="org.labkey.api.util.HtmlString" %> +<%@ page import="org.labkey.api.util.OptionBuilder" %> <%@ page import="org.labkey.api.util.Pair" %> <%@ page import="org.labkey.api.util.SafeToRender" %> -<%@ page import="org.labkey.api.util.OptionBuilder" %> <%@ page import="org.labkey.api.util.SelectBuilder" %> <%@ page import="org.labkey.api.view.ActionURL" %> <%@ page import="org.labkey.api.view.HttpView" %> @@ -75,13 +75,10 @@ These permissions control whether pipeline files can be downloaded and updated v { if (g.isProjectGroup()) continue; - List assignedRoles = policy.getAssignedRoles(g); - Role assignedRole = !assignedRoles.isEmpty() ? assignedRoles.get(0) : null; - final HtmlString name; - if (g.isUsers()) - name = HtmlString.of("All Users"); - else - name = h(g.getName()); + Role assignedRole = policy.getAssignedRoles(g) + .findFirst() + .orElse(null); + HtmlString name = h(g.isUsers() ? "All Users" : g.getName()); %> <%=name%> <%=getSelect(i, g.isGuests() ? optionsGuest : optionsFull, assignedRole, policy, pipeRoot)%> @@ -97,8 +94,9 @@ These permissions control whether pipeline files can be downloaded and updated v { if (!g.isProjectGroup()) continue; - List assignedRoles = policy.getAssignedRoles(g); - Role assignedRole = !assignedRoles.isEmpty() ? assignedRoles.get(0) : RoleManager.getRole(NoPermissionsRole.class); + Role assignedRole = policy.getAssignedRoles(g) + .findFirst() + .orElse(RoleManager.getRole(NoPermissionsRole.class)); %> <%=h(g.getName())%> <%=getSelect(i, g.isGuests() ? optionsGuest : optionsFull, assignedRole, policy, pipeRoot)%> diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index 6dc3e2bae93..13678c5f513 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -168,6 +168,7 @@ import java.util.TreeSet; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Collectors; import static org.labkey.api.query.QueryService.AuditAction.DELETE; import static org.labkey.api.query.QueryService.AuditAction.TRUNCATE; @@ -975,7 +976,7 @@ public Set> getPermissions(UserPrincipal user, @Null copyReadPerms(studyPermissions, result); if (studyPermissions.contains(ReadSomePermission.class)) { - Set> datasetPermissions = SecurityPolicyManager.getPolicy(this).getOwnPermissions(user); + Set> datasetPermissions = SecurityPolicyManager.getPolicy(this).getOwnPermissions(user).collect(Collectors.toSet()); copyReadPerms(datasetPermissions, result); } diff --git a/study/src/org/labkey/study/security/datasets.jsp b/study/src/org/labkey/study/security/datasets.jsp index d231bbd8649..607a74e6f8e 100644 --- a/study/src/org/labkey/study/security/datasets.jsp +++ b/study/src/org/labkey/study/security/datasets.jsp @@ -15,6 +15,7 @@ * limitations under the License. */ %> +<%@ page import="org.jetbrains.annotations.Nullable"%> <%@ page import="org.labkey.api.security.Group"%> <%@ page import="org.labkey.api.security.SecurityManager"%> <%@ page import="org.labkey.api.security.SecurityPolicy"%> @@ -22,14 +23,14 @@ <%@ page import="org.labkey.api.security.permissions.ReadPermission"%> <%@ page import="org.labkey.api.security.permissions.ReadSomePermission"%> <%@ page import="org.labkey.api.security.roles.EditorRole"%> -<%@ page import="org.labkey.api.security.roles.ReaderRole"%> +<%@ page import="org.labkey.api.security.roles.ReaderRole" %> <%@ page import="org.labkey.api.security.roles.Role" %> <%@ page import="org.labkey.api.security.roles.RoleManager" %> <%@ page import="org.labkey.api.study.Dataset" %> <%@ page import="org.labkey.api.util.HtmlString" %> <%@ page import="org.labkey.api.util.HtmlStringBuilder" %> -<%@ page import="org.labkey.api.util.Pair" %> <%@ page import="org.labkey.api.util.OptionBuilder" %> +<%@ page import="org.labkey.api.util.Pair" %> <%@ page import="org.labkey.api.view.ActionURL" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.study.controllers.StudyController.ManageStudyAction" %> @@ -42,7 +43,6 @@ <%@ page import="java.util.LinkedList" %> <%@ page import="java.util.List" %> <%@ page import="java.util.Objects" %> -<%@ page import="org.jetbrains.annotations.Nullable" %> <%@ page extends="org.labkey.study.view.BaseStudyPage" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <%! @@ -270,8 +270,9 @@ set on the alternate ID dataset will affect who can edit other datasets. Hover o for (Group g : restrictedGroups) { - List roles = dsPolicy.getAssignedRoles(g); - Role assignedRole = roles.isEmpty() ? null : roles.get(0); + Role assignedRole = dsPolicy.getAssignedRoles(g) + .findFirst() + .orElse(null); boolean writePerm = assignedRole != null && assignedRole.getClass() == EditorRole.class; boolean readPerm = !writePerm && dsPolicy.hasNonInheritedPermission(g, ReadPermission.class); diff --git a/study/src/org/labkey/study/security/permissions/StudyPermissionExporter.java b/study/src/org/labkey/study/security/permissions/StudyPermissionExporter.java index ead5b808b10..3530b76b742 100644 --- a/study/src/org/labkey/study/security/permissions/StudyPermissionExporter.java +++ b/study/src/org/labkey/study/security/permissions/StudyPermissionExporter.java @@ -116,8 +116,9 @@ public StudySecurityPolicyDocument getStudySecurityPolicyDocument(StudyImpl stud for (Group g : restrictedGroups) { - java.util.List roles = dsPolicy.getAssignedRoles(g); - Role assignedRole = roles.isEmpty() ? null : roles.get(0); + Role assignedRole = dsPolicy.getAssignedRoles(g) + .findFirst() + .orElse(null); boolean writePerm = assignedRole != null && assignedRole.getClass() == EditorRole.class; boolean readPerm = !writePerm && dsPolicy.hasNonInheritedPermission(g, ReadPermission.class);