Give inactive users no permissions

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<Role> _roles;
 
         @SuppressWarnings("unused") // Needed for deserialization
-        private LimitedUserImpersonatingContext()
+        private LimitedUserPermissionContext()
         {
             _roles = null;
         }
 
-        private LimitedUserImpersonatingContext(Set<Role> roles)
+        private LimitedUserPermissionContext(Set<Role> roles)
         {
             _roles = roles;
         }
@@ -83,7 +83,7 @@ public Stream<Role> getAssignedRoles(User user, SecurableResource resource)
     @SafeVarargs
     public LimitedUser(User user, Class<? extends Role>... roleClasses)
     {
-        super(user, new LimitedUserImpersonatingContext(getRoles(roleClasses)));
+        super(user, new LimitedUserPermissionContext(getRoles(roleClasses)));
     }
 
     @JsonCreator

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);
     }
@@ -2000,23 +2000,24 @@ public static Collection<Integer> 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<User> getUsersWithPermissions(Container c, boolean includeInactive, Set<Class<? extends Permission>> perms)
     {
-        // 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()
-            .filter(user -> hasAllPermissions(null, c, user, perms, Set.of()))
-            .toList();
+        if (includeInactive)
+            throw new IllegalArgumentException("includeInactive parameter is no longer supported since inactive users have no permissions");
+
+        return getUsersWithPermissions(c, perms);
     }
 
     /**
      * @return an immutable list of active Users who have been assigned all the requested permissions in the given container
      */
     public static List<User> getUsersWithPermissions(Container c, Set<Class<? extends Permission>> 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();
     }
 
     /**
@@ -2604,16 +2605,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 +2652,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 +3078,7 @@ private static boolean hasPermissions(@Nullable String logMsg, SecurableResource
      */
     public static Set<Class<? extends Permission>> getPermissions(SecurableResource resource, UserPrincipal principal, Set<Role> 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 +3287,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 +3342,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
             {

api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java

@@ -163,7 +163,7 @@ public static Collection<User> 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, Set.of(ReadPermission.class)));
         }
         else
         {

core/src/org/labkey/core/query/CoreQuerySchema.java

@@ -481,7 +481,7 @@ public TableInfo getUsers()
             {
                 Set<Integer> 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(), Set.of(ApplicationAdminPermission.class)).stream()
                     .map(User::getUserId)
                     .forEach(projectUserIds::add);
                 _projectUserIds = projectUserIds;

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);
         }
     }
-
 }