Rework project users for sanity and consistency (#7731)

Author: labkey-adam

api/src/org/labkey/api/data/NestedRenderContext.java

@@ -131,7 +131,7 @@ public SimpleFilter buildFilter(TableInfo tinfo, List<ColumnInfo> displayColumns
             fullSQL.append(" ) Limited )");
 
             // Apply a filter that restricts the group ids to the right "page" of data
-            result.addClause(new SimpleFilter.SQLClause(fullSQL.getSQL(), fullSQL.getParamsArray()));
+            result.addClause(new SimpleFilter.SQLClause(fullSQL));
         }
         return result;
     }

api/src/org/labkey/api/security/SecurityManager.java

@@ -55,7 +55,6 @@
 import org.labkey.api.data.PropertyManager.WritablePropertyMap;
 import org.labkey.api.data.RuntimeSQLException;
 import org.labkey.api.data.SQLFragment;
-import org.labkey.api.data.Selector;
 import org.labkey.api.data.SimpleFilter;
 import org.labkey.api.data.SqlExecutor;
 import org.labkey.api.data.SqlSelector;
@@ -1883,126 +1882,6 @@ public static String getMembershipPathwayHTMLDisplay(Set<List<UserPrincipal>> pa
             sb.append("<BR/>");
         }
         return sb.toString();
-    }    
-
-    // TODO: Redundant with getProjectUsers() -- this approach should be more efficient for simple cases
-    // TODO: Also redundant with getFolderUserids()
-    // TODO: Cache this set
-    public static Set<Integer> getProjectUsersIds(Container c)
-    {
-        SQLFragment sql = getProjectUsersSQL(c.getProject());
-        sql.insert(0, "SELECT DISTINCT members.UserId ");
-
-        Selector selector = new SqlSelector(core.getSchema(), sql);
-        return new HashSet<>(selector.getCollection(Integer.class));
-    }
-
-    // True fragment -- need to prepend SELECT DISTINCT() or IN () for this to be valid SQL
-    public static SQLFragment getProjectUsersSQL(Container c)
-    {
-        return new SQLFragment("FROM " + core.getTableInfoMembers() + " members INNER JOIN " + core.getTableInfoUsers() + " users ON members.UserId = users.UserId\n" +
-                                    "INNER JOIN " + core.getTableInfoPrincipals() + " groups ON members.GroupId = groups.UserId\n" +
-                                    "WHERE (groups.Container = ?)", c);
-    }
-
-    public static @NotNull List<User> getProjectUsers(Container c)
-    {
-        return getProjectUsers(c, false, true);
-    }
-
-    public static @NotNull List<User> getProjectUsers(Container c, boolean includeGlobal, boolean includeInactive)
-    {
-        if (c != null && !c.isProject())
-            c = c.getProject();
-
-        List<Group> groups = getGroups(c, includeGlobal);
-        Set<String> emails = new HashSet<>();
-
-        //get members for each group
-        ArrayList<User> projectUsers = new ArrayList<>();
-        Set<User> members;
-
-        for (Group g : groups)
-        {
-            if (g.isGuests() || g.isUsers())
-                continue;
-
-            // TODO: currently only getting members that are users (no groups). should this be changed to get users of member groups?
-            members = getGroupMembers(g, includeInactive ? MemberType.ACTIVE_AND_INACTIVE_USERS : MemberType.ACTIVE_USERS);
-
-            //add this group's members to hashset
-            if (!members.isEmpty())
-            {
-                //get list of users from email
-                for (UserPrincipal member : members)
-                {
-                    User user = UserManager.getUser(member.getUserId());
-                    if (null != user && emails.add(user.getEmail()))
-                        projectUsers.add(user);
-                }
-            }
-        }
-
-        return projectUsers;
-    }
-
-    public static Collection<Integer> getFolderUserids(Container c)
-    {
-        Container project = (c.isProject() || c.isRoot()) ? c : c.getProject();
-        SecurityPolicy policy = c.getPolicy();
-
-        //don't filter if all site users is playing a role
-        Group allSiteUsers = getGroup(Group.groupUsers);
-        if (policy.getAssignedRoles(allSiteUsers).findAny().isPresent())
-        {
-            // Just select all users
-            SQLFragment sql = new SQLFragment("SELECT u.UserId FROM ");
-            sql.append(core.getTableInfoPrincipals(), "u");
-            sql.append(" WHERE u.type='u'");
-
-            return new SqlSelector(core.getSchema(), sql).getCollection(Integer.class);
-        }
-
-        //users "in the project" consists of:
-        // - users who are members of a project group
-        // - users who belong to a site group that has a role assignment in the policy for the specified folder
-        // - users who have a direct role assignment in the policy for the specified folder
-
-        Set<Integer> userIds = new HashSet<>();
-
-        // Add all project groups
-        Set<Group> groupsToExpand = new HashSet<>(getGroups(project, false));
-
-        // Look for users and site groups that have direct assignment to the container
-        for (RoleAssignment roleAssignment : c.getPolicy().getAssignments())
-        {
-            User user = UserManager.getUser(roleAssignment.getUserId());
-            if (user != null)
-            {
-                userIds.add(user.getUserId());
-            }
-            else
-            {
-                Group assignedGroup = getGroup(roleAssignment.getUserId());
-                if (assignedGroup != null && !assignedGroup.isProjectGroup())
-                {
-                    // Add all site groups
-                    groupsToExpand.add(assignedGroup);
-                }
-            }
-        }
-
-        // Find the users who are members of all the relevant site groups
-        for (Group group : groupsToExpand)
-        {
-            Set<User> groupMembers = getAllGroupMembers(group, MemberType.ACTIVE_AND_INACTIVE_USERS);
-            for (User groupMember : groupMembers)
-            {
-                userIds.add(groupMember.getUserId());
-            }
-        }
-
-        return userIds;
     }
 
     /**

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

@@ -62,6 +62,7 @@
 import org.labkey.api.security.UserPrincipal;
 import org.labkey.api.security.permissions.AdminPermission;
 import org.labkey.api.security.permissions.ApplicationAdminPermission;
+import org.labkey.api.security.permissions.ReadPermission;
 import org.labkey.api.security.permissions.SeeGroupDetailsPermission;
 import org.labkey.api.security.permissions.SeeUserDetailsPermission;
 import org.labkey.api.security.permissions.TroubleshooterPermission;
@@ -76,14 +77,13 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 public class CoreQuerySchema extends UserSchema
 {
-    private Set<Integer> _projectUserIds;
+    private Set<Integer> _folderUserIds;
     private final boolean _mustCheckPermissions;
 
     public static final String NAME = "core";
@@ -483,17 +483,15 @@ public TableInfo getUsers()
         }
         else
         {
-            if (_projectUserIds == null)
+            // All users with read permissions in this folder
+            if (_folderUserIds == null)
             {
-                Set<Integer> projectUserIds = new HashSet<>(SecurityManager.getFolderUserids(getContainer()));
-                // Add app admins and site admins (they both have ApplicationAdminPermission)
-                SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(ApplicationAdminPermission.class)).stream()
-                    .map(User::getUserId)
-                    .forEach(projectUserIds::add);
-                _projectUserIds = projectUserIds;
+                _folderUserIds = SecurityManager.getUsersWithPermissions(getContainer(), Set.of(ReadPermission.class)).stream()
+                    .map(UserPrincipal::getUserId)
+                    .collect(Collectors.toSet());
             }
             ColumnInfo userid = users.getRealTable().getColumn("userid");
-            users.addInClause(userid, _projectUserIds);
+            users.addInClause(userid, _folderUserIds);
 
             addGroupsColumn(users);
             addAvatarColumn(users);

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

@@ -28,10 +28,9 @@
 import org.labkey.api.data.Container;
 import org.labkey.api.data.ContainerManager;
 import org.labkey.api.data.NullColumnInfo;
-import org.labkey.api.data.SQLFragment;
 import org.labkey.api.data.SimpleFilter;
+import org.labkey.api.data.SimpleFilter.InClause;
 import org.labkey.api.data.TableInfo;
-import org.labkey.api.data.dialect.SqlDialect;
 import org.labkey.api.exp.PropertyColumn;
 import org.labkey.api.exp.PropertyDescriptor;
 import org.labkey.api.exp.property.Domain;
@@ -92,10 +91,6 @@
 
 import static org.labkey.api.util.IntegerUtils.asInteger;
 
-/**
-* User: klum
-* Date: 9/19/12
-*/
 public class UsersTable extends SimpleUserSchema.SimpleTable<UserSchema>
 {
     private Set<String> _illegalColumns;
@@ -116,7 +111,7 @@ public UsersTable(UserSchema schema, TableInfo table)
     {
         super(schema, table, null);
 
-        setDescription("Contains all users who are members of the current project." +
+        setDescription("Contains all users with read permissions in the current project." +
             " The data in this table are available only to users who are signed-in (not guests). Guests see no rows." +
             " Signed-in users see the columns UserId, EntityId, and DisplayName." +
             " Users granted the '" + SeeUserAndGroupDetailsRole.NAME + "' role see all standard and custom columns.");
@@ -414,6 +409,7 @@ public boolean hasPermission(@NotNull UserPrincipal user, @NotNull Class<? exten
             return super.hasPermission(user, perm);
     }
 
+    // Used by ShowUserHistoryAction only (is not applied to UsersTable)
     public static SimpleFilter authorizeAndGetProjectMemberFilter(@NotNull Container c, @NotNull User u, String userIdColumnName) throws UnauthorizedException
     {
         SimpleFilter filter = new SimpleFilter();
@@ -425,29 +421,10 @@ public static SimpleFilter authorizeAndGetProjectMemberFilter(@NotNull Container
         }
         else
         {
-            SQLFragment sql = SecurityManager.getProjectUsersSQL(c.getProject());
-
             final FieldKey userIdColumnFieldKey = new FieldKey(null, userIdColumnName);
-            filter.addClause(new SimpleFilter.SQLClause(sql.getSQL(), sql.getParamsArray(), userIdColumnFieldKey)
-            {
-                @Override
-                public SQLFragment toSQLFragment(Map<FieldKey, ? extends ColumnInfo> columnMap, SqlDialect dialect)
-                {
-                    ColumnInfo col = columnMap.get(userIdColumnFieldKey);
-
-                    // NOTE: Ideally we would use col.getValueSql() here instead
-                    SQLFragment sql = new SQLFragment();
-
-                    if (col != null)
-                        sql.appendIdentifier(col.getAlias());
-                    else
-                        sql.append(userIdColumnFieldKey);
-                    sql.append(" IN (SELECT members.UserId ");
-                    sql.append(super.toSQLFragment(columnMap, dialect));
-                    sql.append(")");
-                    return sql;
-                }
-            });
+            // Consider: could short-circuit optimize if guests or all site users have read permissions
+            InClause clause = new InClause(userIdColumnFieldKey, SecurityManager.getUsersWithPermissions(c, Set.of(ReadPermission.class)));
+            filter.addClause(clause);
         }
         return filter;
     }

core/src/org/labkey/core/security/SecurityApiActions.java

@@ -1578,8 +1578,7 @@ public ApiResponse execute(IdForm form, BindException errors) throws Exception
             Container c = getContainer();
             if (!c.isRoot())
             {
-                List<User> projectUsers = SecurityManager.getProjectUsers(c);
-                if (!projectUsers.contains(user))
+                if (!c.hasPermission(user, ReadPermission.class))
                     throw new IllegalArgumentException("User id " + form.getId() + " does not exist in the folder: " + c.getPath());
             }