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

@@ -75,8 +75,8 @@
 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.ImpersonatePermission;
 import org.labkey.api.security.permissions.DeletePermission;
+import org.labkey.api.security.permissions.ImpersonatePermission;
 import org.labkey.api.security.permissions.InsertPermission;
 import org.labkey.api.security.permissions.Permission;
 import org.labkey.api.security.permissions.ReadPermission;
@@ -314,7 +314,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);
     }
@@ -2009,24 +2009,16 @@ public static Collection<Integer> getFolderUserids(Container c)
     }
 
     /**
-     * @return an immutable list of Users who have been assigned all the requested permissions in the given container
+     * @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, boolean includeInactive, Set<Class<? extends Permission>> perms)
+    public static List<User> getUsersWithPermissions(Container c, 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()
+        return UserManager.getUsers(false).stream()
             .filter(user -> hasAllPermissions(null, c, user, perms, Set.of()))
             .toList();
     }
 
-    /**
-     * @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);
-    }
-
     /**
      * @return an immutable list of Users who have been assigned any of the requested permissions in the given container
      */
@@ -2612,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()
@@ -2655,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()
         {
@@ -3078,7 +3077,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))
@@ -3287,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);
@@ -3342,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
             {

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;

core/src/org/labkey/core/user/UserController.java

@@ -26,7 +26,6 @@
 import org.json.JSONObject;
 import org.labkey.api.action.ApiResponse;
 import org.labkey.api.action.ApiSimpleResponse;
-import org.labkey.api.action.ApiVersion;
 import org.labkey.api.action.FormViewAction;
 import org.labkey.api.action.MutatingApiAction;
 import org.labkey.api.action.QueryViewAction;
@@ -107,7 +106,6 @@
 import org.labkey.api.security.permissions.AdminPermission;
 import org.labkey.api.security.permissions.CanAccessLockedProjectsPermission;
 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;
@@ -117,7 +115,6 @@
 import org.labkey.api.security.roles.RoleManager;
 import org.labkey.api.settings.AdminConsole;
 import org.labkey.api.settings.AppProps;
-import org.labkey.api.usageMetrics.SimpleMetricsService;
 import org.labkey.api.util.HtmlStringBuilder;
 import org.labkey.api.util.MailHelper;
 import org.labkey.api.util.Pair;
@@ -142,7 +139,6 @@
 import org.labkey.api.view.ViewContext;
 import org.labkey.api.view.WebPartView;
 import org.labkey.api.view.template.PageConfig;
-import org.labkey.core.CoreModule;
 import org.labkey.core.login.DbLoginConfiguration;
 import org.labkey.core.login.DbLoginManager;
 import org.labkey.core.login.LoginController;
@@ -2693,14 +2689,13 @@ protected void setUsersList(GetUsersForm form, Collection<User> users, ApiSimple
     }
 
     /**
-     * Retrieves the set of users that have all of a specified set of permissions. A group
-     * may be provided and only users within that group will be returned. A name (prefix) may be
-     * provided and only users whose email or display name starts with the prefix will be returned.
-     * This will not return any deactivated users (since they do not have permissions of any sort).
+     * Retrieves the set of users that have all of a specified set of permissions. A group may be provided and only
+     * users within that group will be returned. A name (prefix) may be provided and only users whose email or display
+     * name starts with the prefix will be returned. This will not return any inactive users (since they do not have
+     * permissions of any sort).
      */
     @RequiresLogin
     @RequiresPermission(ReadPermission.class)
-    @ApiVersion(23.10)
     public static class GetUsersWithPermissionsAction extends GetUsersAction
     {
         @Override
@@ -2714,56 +2709,16 @@ else if (form.getPermissionClasses().isEmpty())
             {
                 errors.reject(ERROR_GENERIC, "No valid permission classes provided.");
             }
-        }
-
-        /**
-         * The older 23.10 response format does not honor the newer includeInactive flag. It only honors the active
-         * flag when requesting users of a group, and ignores the flag (only ever returning active users) when not
-         * requesting a group.
-         */
-        private ApiResponse response2310(ApiSimpleResponse response, GetUsersForm form)
-        {
-            SimpleMetricsService.get().increment(CoreModule.CORE_MODULE_NAME, "GetUsersWithPermissionsAction", "OldVersionCalls");
-            Collection<User> users;
-
-            //if requesting users in a specific group...
-            if (null != StringUtils.trimToNull(form.getGroup()) || null != form.getGroupId())
-            {
-                users = filterForPermissions(form, getProjectGroupUsers(form, response, !form.getActive()));
-            }
-            else
-            {
-                users = SecurityManager.getUsersWithPermissions(getContainer(), form.getPermissionClasses());
-            }
-
-            this.setUsersList(form, users, response);
-
-            return response;
-        }
-
-        /**
-         * The 23.11 response format does not honor the active flag, instead it honors the includeInactive flag, and
-         * it honors it when requesting users or groups. The flag defaults to false, so by default only active users
-         * will be returned.
-         */
-        private ApiResponse response2311(ApiSimpleResponse response, GetUsersForm form)
-        {
-            boolean includeInactive = form.getIncludeInactive();
-            Collection<User> users;
-
-            //if requesting users in a specific group...
-            if (null != StringUtils.trimToNull(form.getGroup()) || null != form.getGroupId())
+            // 26.5 changes: stop supporting "active" and "includeInactive" parameters. Return errors for now. We could
+            // remove these checks in 26.11+.
+            else if (form.getActive())
             {
-                users = filterForPermissions(form, getProjectGroupUsers(form, response, includeInactive));
+                errors.reject(ERROR_GENERIC, "The active parameter is no longer supported. Inactive users have no permissions.");
             }
-            else
+            else if (form.getIncludeInactive())
             {
-                users = SecurityManager.getUsersWithPermissions(getContainer(), includeInactive, form.getPermissionClasses());
+                errors.reject(ERROR_GENERIC, "The includeInactive parameter is no longer supported. Inactive users have no permissions.");
             }
-
-            this.setUsersList(form, users, response);
-
-            return response;
         }
 
         @Override
@@ -2778,10 +2733,21 @@ public ApiResponse execute(GetUsersForm form, BindException errors)
             ApiSimpleResponse response = new ApiSimpleResponse();
             response.put("container", container.getPath());
 
-            if (getRequestedApiVersion() <= 23.10)
-                return response2310(response, form);
+            Collection<User> users;
 
-            return response2311(response, form);
+            if (null != StringUtils.trimToNull(form.getGroup()) || null != form.getGroupId())
+            {
+                users = filterForPermissions(form, getProjectGroupUsers(form, response, false));
+            }
+            else
+            {
+                // Active users only
+                users = SecurityManager.getUsersWithPermissions(getContainer(), form.getPermissionClasses());
+            }
+
+            setUsersList(form, users, response);
+
+            return response;
         }
     }
 

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