From e7c1c6c7831cce8e5f3705b9410df749f163145c Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 7 Apr 2026 07:43:03 -0700 Subject: [PATCH 1/4] Give inactive users no permissions --- .../labkey/api/security/SecurityManager.java | 69 ++++++++++++++----- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index e6ccfc41323..f074a2d052b 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -313,7 +313,7 @@ public static void addGroupListener(GroupListener listener) public static void addGroupListener(GroupListener listener, boolean meFirst) { if (meFirst) - _listeners.add(0, listener); + _listeners.addFirst(listener); else _listeners.add(listener); } @@ -2604,16 +2604,20 @@ public boolean isValid(String[] error) public static class RegistrationEmailTemplate extends SecurityEmailTemplate { protected static final String DEFAULT_SUBJECT = - "Welcome to the ^organizationName^ ^siteShortName^ Web Site new user registration"; + "Welcome to the ^organizationName^ ^siteShortName^ Web Site new user registration"; protected static final String DEFAULT_BODY = - "^optionalMessage^\n\n" + - "You now have an account on the ^organizationName^ ^siteShortName^ web site. We are sending " + - "you this message to verify your email address and to allow you to create a password that will provide secure " + - "access to your data on the web site. To complete the registration process, simply click the link below or " + - "copy it to your browser's address bar. You will then be asked to choose a password.\n\n" + - "^verificationURL^\n\n" + - "The ^siteShortName^ home page is ^homePageURL^. If you have any questions don't hesitate to " + - "contact the ^siteShortName^ team at ^systemEmail^."; + """ + ^optionalMessage^ + + You now have an account on the ^organizationName^ ^siteShortName^ web site. We are sending \ + you this message to verify your email address and to allow you to create a password that will provide secure \ + access to your data on the web site. To complete the registration process, simply click the link below or \ + copy it to your browser's address bar. You will then be asked to choose a password. + + ^verificationURL^ + + The ^siteShortName^ home page is ^homePageURL^. If you have any questions don't hesitate to \ + contact the ^siteShortName^ team at ^systemEmail^."""; @SuppressWarnings("UnusedDeclaration") // Constructor called via reflection public RegistrationEmailTemplate() @@ -2647,14 +2651,17 @@ public RegistrationAdminEmailTemplate() public static class PasswordResetEmailTemplate extends SecurityEmailTemplate { protected static final String DEFAULT_SUBJECT = - "Reset Password Notification from the ^siteShortName^ Web Site"; + "Reset Password Notification from the ^siteShortName^ Web Site"; protected static final String DEFAULT_BODY = - "We have reset your password on the ^organizationName^ ^siteShortName^ web site. " + - "To sign in to the system you will need " + - "to specify a new password. Click the link below or copy it to your browser's address bar. You will then be " + - "asked to enter a new password.\n\n" + - "^verificationURL^\n\n" + - "The ^siteShortName^ home page is ^homePageURL^."; + """ + We have reset your password on the ^organizationName^ ^siteShortName^ web site. \ + To sign in to the system you will need \ + to specify a new password. Click the link below or copy it to your browser's address bar. You will then be \ + asked to enter a new password. + + ^verificationURL^ + + The ^siteShortName^ home page is ^homePageURL^."""; public PasswordResetEmailTemplate() { @@ -3070,7 +3077,7 @@ private static boolean hasPermissions(@Nullable String logMsg, SecurableResource */ public static Set> getPermissions(SecurableResource resource, UserPrincipal principal, Set contextualRoles) { - if (null == resource || null == principal) + if (null == resource || null == principal || !principal.isActive()) return Set.of(); if (principal instanceof User user && resource.getResourceContainer().isForbiddenProject(user, contextualRoles)) @@ -3279,7 +3286,7 @@ public void testAddMemberToGroup() throws InvalidGroupMembershipException for(Object[] groupMemberResponse : groupMemberResponses) { addMemberToGroupVerifyResponse((Group) groupMemberResponse[0], - (UserPrincipal) groupMemberResponse[1], (String) groupMemberResponse[2]); + (UserPrincipal) groupMemberResponse[1], (String) groupMemberResponse[2]); } addMember(groupA, groupB); @@ -3334,6 +3341,30 @@ public void testCreateUser() throws Exception User user2 = AuthenticationManager.authenticate(ViewServlet.mockRequest("GET", new ActionURL(), null, null, null), rawEmail, password); assertNotNull("\"" + rawEmail + "\" failed to authenticate with password \"" + password + "\"; check labkey.log around timestamp " + DateUtil.formatDateTime(new Date(), "HH:mm:ss,SSS") + " for the reason", user2); assertEquals(user, user2); + + // Now test setting that user to inactive + Container testContainer = JunitUtil.getTestContainer(); + if (!testContainer.hasPermission(user, ReadPermission.class)) + { + addRoleAssignment(new MutableSecurityPolicy(testContainer), user, ReaderRole.class, TestContext.get().getUser()); + assertTrue(testContainer.hasPermission(user, ReadPermission.class)); + } + // Set the user to inactive + UserManager.setUserActive(TestContext.get().getUser(), user, false); + // Refresh the user from the cache + user = UserManager.getUser(user.getUserId()); + assertNotNull(user); + assertFalse(user.isActive()); + try + { + user2 = AuthenticationManager.authenticate(ViewServlet.mockRequest("GET", new ActionURL(), null, null, null), rawEmail, password); + fail("Expected authenticate() to throw for inactive user, but it returned " + user2); + } + catch (UnauthorizedException ue) + { + // Expected that inactive user can't authenticate + } + assertFalse(testContainer.hasPermission(user, ReadPermission.class)); } finally { From 06557fb91028b634659ecc00efa04ce46bfe8570 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 17 Apr 2026 10:34:03 -0700 Subject: [PATCH 2/4] Fix FileLinkMetricsMaintenanceTask user. Rename LimitedUserImpersonatingContext -> LimitedUserPermissionContext. --- api/src/org/labkey/api/security/LimitedUser.java | 8 ++++---- .../experiment/FileLinkMetricsMaintenanceTask.java | 14 +++----------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/security/LimitedUser.java b/api/src/org/labkey/api/security/LimitedUser.java index e771d259bf5..be66f9b64bf 100644 --- a/api/src/org/labkey/api/security/LimitedUser.java +++ b/api/src/org/labkey/api/security/LimitedUser.java @@ -52,17 +52,17 @@ public class LimitedUser extends ClonedUser { // Must be a named class to allow Jackson deserialization (e.g., Evaluation Content loads folder archives via the pipeline using AdminUser) - private static class LimitedUserImpersonatingContext extends NormalPermissionsContext + private static class LimitedUserPermissionContext extends NormalPermissionsContext { private final Set _roles; @SuppressWarnings("unused") // Needed for deserialization - private LimitedUserImpersonatingContext() + private LimitedUserPermissionContext() { _roles = null; } - private LimitedUserImpersonatingContext(Set roles) + private LimitedUserPermissionContext(Set roles) { _roles = roles; } @@ -83,7 +83,7 @@ public Stream getAssignedRoles(User user, SecurableResource resource) @SafeVarargs public LimitedUser(User user, Class... roleClasses) { - super(user, new LimitedUserImpersonatingContext(getRoles(roleClasses))); + super(user, new LimitedUserPermissionContext(getRoles(roleClasses))); } @JsonCreator diff --git a/experiment/src/org/labkey/experiment/FileLinkMetricsMaintenanceTask.java b/experiment/src/org/labkey/experiment/FileLinkMetricsMaintenanceTask.java index b63d47f42ff..2e17d004a12 100644 --- a/experiment/src/org/labkey/experiment/FileLinkMetricsMaintenanceTask.java +++ b/experiment/src/org/labkey/experiment/FileLinkMetricsMaintenanceTask.java @@ -2,21 +2,17 @@ import org.apache.logging.log4j.Logger; import org.labkey.api.data.ContainerManager; -import org.labkey.api.security.LimitedUser; -import org.labkey.api.security.PrincipalType; import org.labkey.api.security.User; -import org.labkey.api.security.roles.ProjectAdminRole; -import org.labkey.api.util.SystemMaintenance; +import org.labkey.api.util.SystemMaintenance.MaintenanceTask; import org.labkey.experiment.api.ExperimentServiceImpl; import java.util.Date; import java.util.HashMap; import java.util.Map; -public class FileLinkMetricsMaintenanceTask implements SystemMaintenance.MaintenanceTask +public class FileLinkMetricsMaintenanceTask implements MaintenanceTask { public static final String NAME = "FileLinkMetricsMaintenanceTask"; - public static final String STARTUP_SCOPE = "FileLinkMetrics"; @Override public String getDescription() @@ -32,10 +28,7 @@ public String getName() private User getTaskUser() { - User taskUser = new User("FileLinkMetricsMaintenanceUser", -1); - taskUser.setPrincipalType(PrincipalType.SERVICE); - taskUser.setDisplayName("FileLinkMetricsMaintenanceUser"); - return new LimitedUser(taskUser, ProjectAdminRole.class); + return User.getAdminServiceUser(); } @Override @@ -77,5 +70,4 @@ public void run(Logger log) log.error("Unable to run missing files check task. {}", e); } } - } From 0e2351326944899f80a9cd4241a62c1d5433c156 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 22 Apr 2026 18:12:36 -0700 Subject: [PATCH 3/4] Throw if "includeInactive" is specified with getUsersWithPermissions() --- api/src/org/labkey/api/security/SecurityManager.java | 5 ++++- .../impersonation/UserImpersonationContextFactory.java | 2 +- core/src/org/labkey/core/query/CoreQuerySchema.java | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index f074a2d052b..042e34801d4 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -2005,8 +2005,11 @@ public static Collection getFolderUserids(Container c) */ public static List getUsersWithPermissions(Container c, boolean includeInactive, Set> perms) { + if (includeInactive) + throw new IllegalArgumentException("includeInactive parameter is no longer supported since inactive users have no permissions"); + // No cache right now, but performance seems fine. After the user list and policy are cached, no other queries occur. - return UserManager.getUsers(includeInactive).stream() + return UserManager.getUsers(false).stream() .filter(user -> hasAllPermissions(null, c, user, perms, Set.of())) .toList(); } diff --git a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java index 686c0c74f1e..3fccb6bc0fe 100644 --- a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java @@ -163,7 +163,7 @@ public static Collection getValidImpersonationUsers(@Nullable Container pr else if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class)) { // 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))); + validUsers = new ArrayList<>(SecurityManager.getUsersWithPermissions(project, false, Set.of(ReadPermission.class))); } else { diff --git a/core/src/org/labkey/core/query/CoreQuerySchema.java b/core/src/org/labkey/core/query/CoreQuerySchema.java index bc8bdcc2f0b..5b8c059e26e 100644 --- a/core/src/org/labkey/core/query/CoreQuerySchema.java +++ b/core/src/org/labkey/core/query/CoreQuerySchema.java @@ -481,7 +481,7 @@ public TableInfo getUsers() { Set projectUserIds = new HashSet<>(SecurityManager.getFolderUserids(getContainer())); // Add app admins and site admins (they both have ApplicationAdminPermission) - SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), true, Set.of(ApplicationAdminPermission.class)).stream() + SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), false, Set.of(ApplicationAdminPermission.class)).stream() .map(User::getUserId) .forEach(projectUserIds::add); _projectUserIds = projectUserIds; From c3490a99f428227ad16eb5092cbfb7ace6b507b3 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 22 Apr 2026 18:18:52 -0700 Subject: [PATCH 4/4] Stop calling the variant that takes the flag --- .../org/labkey/api/security/SecurityManager.java | 14 ++++++-------- .../UserImpersonationContextFactory.java | 2 +- .../src/org/labkey/core/query/CoreQuerySchema.java | 2 +- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 042e34801d4..f47ae7cdfa6 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -2000,18 +2000,13 @@ public static Collection getFolderUserids(Container c) return userIds; } - /** - * @return an immutable list of Users who have been assigned all the requested permissions in the given container - */ + @Deprecated // Call the other variant public static List getUsersWithPermissions(Container c, boolean includeInactive, Set> perms) { if (includeInactive) throw new IllegalArgumentException("includeInactive parameter is no longer supported since inactive users have no permissions"); - // No cache right now, but performance seems fine. After the user list and policy are cached, no other queries occur. - return UserManager.getUsers(false).stream() - .filter(user -> hasAllPermissions(null, c, user, perms, Set.of())) - .toList(); + return getUsersWithPermissions(c, perms); } /** @@ -2019,7 +2014,10 @@ public static List getUsersWithPermissions(Container c, boolean includeIna */ public static List getUsersWithPermissions(Container c, Set> perms) { - return getUsersWithPermissions(c, false, perms); + // No cache right now, but performance seems fine. After the user list and policy are cached, no other queries occur. + return UserManager.getUsers(false).stream() + .filter(user -> hasAllPermissions(null, c, user, perms, Set.of())) + .toList(); } /** diff --git a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java index 3fccb6bc0fe..0f894a43a7b 100644 --- a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java @@ -163,7 +163,7 @@ public static Collection getValidImpersonationUsers(@Nullable Container pr else if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class)) { // 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, false, Set.of(ReadPermission.class))); + validUsers = new ArrayList<>(SecurityManager.getUsersWithPermissions(project, Set.of(ReadPermission.class))); } else { diff --git a/core/src/org/labkey/core/query/CoreQuerySchema.java b/core/src/org/labkey/core/query/CoreQuerySchema.java index 5b8c059e26e..72aa3d8043c 100644 --- a/core/src/org/labkey/core/query/CoreQuerySchema.java +++ b/core/src/org/labkey/core/query/CoreQuerySchema.java @@ -481,7 +481,7 @@ public TableInfo getUsers() { Set projectUserIds = new HashSet<>(SecurityManager.getFolderUserids(getContainer())); // Add app admins and site admins (they both have ApplicationAdminPermission) - SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), false, Set.of(ApplicationAdminPermission.class)).stream() + SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(ApplicationAdminPermission.class)).stream() .map(User::getUserId) .forEach(projectUserIds::add); _projectUserIds = projectUserIds;