Skip to content

Rework project users for sanity and consistency #7731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-adam wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Rework project users for sanity and consistency #7731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d398203
Rework project users for sanity and consistency
labkey-adam Jun 7, 2026
d60f224
Adjust mothership assigned-to: all users with update permission
labkey-adam Jun 7, 2026
f5581dd
Adjust issues assigned-to: determination
labkey-adam Jun 7, 2026
d9941c6
Migrate more methods
labkey-adam Jun 8, 2026
1f3f5ec
Adjust user-getUsers.api behavior. Eliminate final "project users" me…
labkey-adam Jun 8, 2026
accc85a
Check read permissions in current folder (not project). This is more …
labkey-adam Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/data/NestedRenderContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
121 changes: 0 additions & 121 deletions api/src/org/labkey/api/security/SecurityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand Down
20 changes: 9 additions & 11 deletions core/src/org/labkey/core/query/CoreQuerySchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
35 changes: 6 additions & 29 deletions core/src/org/labkey/core/query/UsersTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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.");
Expand Down Expand Up @@ -414,6 +409,7 @@ public boolean hasPermission(@NotNull UserPrincipal user, @NotNull Class<? exten
return super.hasPermission(user, perm);
}

// Used on by ShowUserHistoryAction
public static SimpleFilter authorizeAndGetProjectMemberFilter(@NotNull Container c, @NotNull User u, String userIdColumnName) throws UnauthorizedException
{
SimpleFilter filter = new SimpleFilter();
Expand All @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions core/src/org/labkey/core/security/SecurityApiActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Loading