Skip to content

GH Issue 1695: Require group-owning container for all group membershi… #8264

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

Closed
labkey-adam wants to merge 2 commits into from
Closed

GH Issue 1695: Require group-owning container for all group membershi… #8264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdfcd8d
GH Issue 1695: Require group-owning container for all group membershi…
labkey-adam Oct 31, 2026
9968b25
Claude feedback
labkey-adam Oct 31, 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
3 changes: 3 additions & 0 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 @@ -1443,6 +1443,9 @@ public static void deleteMembers(Group group, Collection<UserPrincipal> membersT

if (membersToDelete != null && !membersToDelete.isEmpty())
{
if (group.isSystemGroup())
throw new IllegalArgumentException("Can't remove a member from the " + group.getName() + " group");

SQLFragment sql = new SQLFragment(
"DELETE FROM " + core.getTableInfoMembers() + "\nWHERE GroupId = ? AND UserId ");
sql.add(groupId);
Expand Down
1 change: 1 addition & 0 deletions core/src/org/labkey/core/CoreModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1531,6 +1531,7 @@ public TabDisplayMode getTabDisplayMode()
RhinoService.TestCase.class,
SchemaXMLTestCase.class,
SecurityApiActions.TestCase.class,
SecurityController.GroupMembershipScopingTestCase.class,
SecurityController.TestCase.class,
SiteSettingsUpdater.TestCase.class,
SqlDialect.DialectTestCase.class,
Expand Down
20 changes: 19 additions & 1 deletion core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,13 @@
import org.labkey.api.admin.FolderImporter;
import org.labkey.api.admin.ImportException;
import org.labkey.api.pipeline.PipelineJob;
import org.labkey.api.security.Group;
import org.labkey.api.security.GroupManager;
import org.labkey.api.security.SecurityManager;
import org.labkey.api.view.UnauthorizedException;
import org.labkey.api.writer.VirtualFile;
import org.labkey.core.security.SecurityController;
import org.labkey.security.xml.GroupEnumType;
import org.labkey.security.xml.GroupType;
import org.labkey.security.xml.GroupsType;

Expand Down Expand Up @@ -81,7 +85,21 @@ public void process(@Nullable PipelineJob job, FolderImportContext ctx, VirtualF
// now populate the groups with their members
for (GroupType xmlGroupType : groups.getGroupArray())
{
GroupManager.importGroupMembers(GroupManager.getGroup(ctx.getContainer(), xmlGroupType.getName(), xmlGroupType.getType()), xmlGroupType, ctx.getLogger(), ctx.getContainer());
// Ignore the archive's group type; it would otherwise let the archive target a site group
Group group = GroupManager.getGroup(ctx.getContainer(), xmlGroupType.getName(), GroupEnumType.PROJECT);
if (group != null)
{
try
{
SecurityController.verifyUserCanModifyGroup(group, ctx.getContainer(), ctx.getUser());
}
catch (UnauthorizedException e)
{
ctx.getLogger().warn("Skipping members of group \"{}\": {}", group.getName(), e.getMessage());
continue;
}
}
GroupManager.importGroupMembers(group, xmlGroupType, ctx.getLogger(), ctx.getContainer());
}

ctx.getUser().refreshGroups(); // The import user's own groups may have changed as the result of this import, Issue #45273
Expand Down
6 changes: 3 additions & 3 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 @@ -1111,7 +1111,7 @@ public ApiResponse execute(GroupForm form, BindException errors)
throw new UnauthorizedException("The specified group does not belong to this project.");
}

SecurityController.verifyUserCanModifyGroup(_group, getUser());
SecurityController.verifyUserCanModifyGroup(_group, container, getUser());

Map<String, String> memberErrors = new HashMap<>();
List<User> newUsers = new ArrayList<>();
Expand Down Expand Up @@ -1763,7 +1763,7 @@ public ApiResponse execute(GroupMemberForm form, BindException errors)
Group group = getGroup(form);

if (group != null)
SecurityController.verifyUserCanModifyGroup(group, getUser());
SecurityController.verifyUserCanModifyGroup(group, getContainer(), getUser());

for (int id : form.getPrincipalIds())
{
Expand Down Expand Up @@ -1791,7 +1791,7 @@ public ApiResponse execute(GroupMemberForm form, BindException errors)
Group group = getGroup(form);

if (group != null)
SecurityController.verifyUserCanModifyGroup(group, getUser());
SecurityController.verifyUserCanModifyGroup(group, getContainer(), getUser());

for (int id : form.getPrincipalIds())
{
Expand Down
138 changes: 128 additions & 10 deletions core/src/org/labkey/core/security/SecurityController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.json.JSONObject;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.labkey.api.action.ApiResponse;
import org.labkey.api.action.ApiSimpleResponse;
Expand All @@ -43,6 +45,8 @@
import org.labkey.api.action.SimpleViewAction;
import org.labkey.api.action.SpringActionController;
import org.labkey.api.admin.FolderExportPermission;
import org.labkey.api.admin.FolderImportContext;
import org.labkey.api.admin.StaticLoggerGetter;
import org.labkey.api.audit.AuditLogService;
import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
import org.labkey.api.audit.provider.GroupAuditProvider;
Expand Down Expand Up @@ -98,6 +102,7 @@
import org.labkey.api.security.ValidEmail;
import org.labkey.api.security.ValidEmail.InvalidEmailException;
import org.labkey.api.security.permissions.AbstractActionPermissionTest;
import org.labkey.api.security.permissions.AbstractContainerScopingTest;
import org.labkey.api.security.permissions.AddUserPermission;
import org.labkey.api.security.permissions.AdminOperationsPermission;
import org.labkey.api.security.permissions.AdminPermission;
Expand Down Expand Up @@ -135,6 +140,7 @@
import org.labkey.api.util.Pair;
import org.labkey.api.util.TestContext;
import org.labkey.api.util.URLHelper;
import org.labkey.api.util.logging.LogHelper;
import org.labkey.api.view.ActionURL;
import org.labkey.api.view.AjaxCompletion;
import org.labkey.api.view.HtmlView;
Expand All @@ -147,10 +153,14 @@
import org.labkey.api.view.ViewContext;
import org.labkey.api.view.WebPartView;
import org.labkey.api.view.template.PageConfig;
import org.labkey.core.admin.importer.SecurityGroupImporterFactory;
import org.labkey.core.query.CoreQuerySchema;
import org.labkey.core.user.SecurityAccessView;
import org.labkey.core.user.UserController;
import org.labkey.core.user.UserController.AccessDetailRow;
import org.labkey.folder.xml.FolderDocument;
import org.labkey.security.xml.GroupEnumType;
import org.labkey.security.xml.GroupType;
import org.springframework.validation.BindException;
import org.springframework.validation.Errors;
import org.springframework.validation.ObjectError;
Expand Down Expand Up @@ -761,7 +771,6 @@ public void validateCommand(UpdateMembersForm target, Errors errors)
@Override
public boolean handlePost(UpdateMembersForm form, BindException errors) throws Exception
{
// 0 - only site admins can modify members of system groups
// 1 - only site admins can modify groups that are assigned a privileged role
// 2 - warn if you are deleting yourself from global or project admins
// 3 - if user confirms delete, post to action again, with list of users to delete and confirmation flag.
Expand All @@ -775,7 +784,7 @@ public boolean handlePost(UpdateMembersForm form, BindException errors) throws E
if (null == _group)
throw new RedirectException(new ActionURL(PermissionsAction.class, container));

verifyUserCanModifyGroup(_group, getUser());
verifyUserCanModifyGroup(_group, getContainer(), getUser());

_messages = new ArrayList<>();

Expand Down Expand Up @@ -918,18 +927,18 @@ public URLHelper getSuccessURL(UpdateMembersForm updateMembersForm)
/**
* Throws UnauthorizedException if user is not allowed to modify this group
* @param group the Group being modified
* @param c the current container, which must own the group
* @param user the current admin user
*/
public static void verifyUserCanModifyGroup(Group group, User user)
public static void verifyUserCanModifyGroup(Group group, Container c, User user)
{
if (!user.hasSiteAdminPermission())
{
if (group.isSystemGroup())
throw new UnauthorizedException("Can not update members of system group: " + group.getName());
ensureGroupInContainer(group, c);

if (group.hasPrivilegedRole())
throw new UnauthorizedException("Can not update members of a group assigned a privileged role: " + group.getName());
}
if (group.isSystemGroup())
throw new UnauthorizedException("Can not update members of system group: " + group.getName());

if (!user.hasSiteAdminPermission() && group.hasPrivilegedRole())
throw new UnauthorizedException("Can not update members of a group assigned a privileged role: " + group.getName());
}

public static class CompleteMemberForm
Expand Down Expand Up @@ -2536,4 +2545,113 @@ public void validateAdministratorRolePermissionAssignment()
);
}
}

/**
* GH Issue 1695 regression tests.
*/
public static class GroupMembershipScopingTestCase extends AbstractContainerScopingTest
{
// ProjectAdminRole only applies to projects, so these fixtures are projects rather than /_junit children
private static final String PROJECT_NAME = "GroupMembershipScopingProject";
private static final String SITE_GROUP_NAME = "GroupMembershipScopingSiteGroup";

private Container _project;
private Group _siteGroup;
private User _siteGroupMember;
private User _projectAdmin;

@Before
public void setup() throws Exception
{
cleanup();
_project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, getAdmin());
_siteGroup = SecurityManager.createGroup(ContainerManager.getRoot(), SITE_GROUP_NAME, getAdmin());
_siteGroupMember = createUserInRole(_project, ReaderRole.class);
SecurityManager.addMember(_siteGroup, _siteGroupMember);
_projectAdmin = createUserInRole(_project, ProjectAdminRole.class);
}

@After
public void cleanup()
{
Container project = ContainerManager.getForPath("/" + PROJECT_NAME);
if (project != null)
ContainerManager.deleteAll(project, getAdmin());

Integer siteGroupId = SecurityManager.getGroupId(null, SITE_GROUP_NAME, false);
if (siteGroupId != null)
SecurityManager.deleteGroup(SecurityManager.getGroup(siteGroupId), getAdmin());
}

@Test
public void projectAdminCannotJoinSiteGroup() throws Exception
{
assertStatus(HttpServletResponse.SC_FORBIDDEN, post(addMemberUrl(_project, _siteGroup, _projectAdmin), _projectAdmin));
assertFalse(isMember(_siteGroup, _projectAdmin));

// Positive control: a site admin can change the site group's membership from the root
assertStatus(HttpServletResponse.SC_FOUND, post(addMemberUrl(ContainerManager.getRoot(), _siteGroup, _projectAdmin), getAdmin()));
assertTrue(isMember(_siteGroup, _projectAdmin));
}

@Test
public void subfolderAdminCannotModifyProjectGroup() throws Exception
{
Container subfolder = ContainerManager.createContainer(_project, "sub", getAdmin());
User subfolderAdmin = createUserInRole(subfolder, FolderAdminRole.class);
Group projectGroup = SecurityManager.createGroup(_project, "ProjectGroup", getAdmin());

assertStatus(HttpServletResponse.SC_FORBIDDEN, post(addMemberUrl(subfolder, projectGroup, subfolderAdmin), subfolderAdmin));
assertFalse(isMember(projectGroup, subfolderAdmin));

// Positive control: the project admin can change the project group's membership from the project
assertStatus(HttpServletResponse.SC_FOUND, post(addMemberUrl(_project, projectGroup, subfolderAdmin), _projectAdmin));
assertTrue(isMember(projectGroup, subfolderAdmin));
}

@Test
public void siteAdminCannotModifySystemGroups() throws Exception
{
Group siteUsers = SecurityManager.getGroup(Group.groupUsers);
ActionURL url = new ActionURL(GroupAction.class, ContainerManager.getRoot())
.addParameter("id", siteUsers.getUserId())
.addParameter("delete", _siteGroupMember.getEmail());
assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, getAdmin()));

assertThrows(IllegalArgumentException.class, () -> SecurityManager.deleteMember(siteUsers, _siteGroupMember));
}

@Test
public void folderImportCannotTargetSiteGroup() throws Exception
{
FolderDocument doc = FolderDocument.Factory.newInstance();
GroupType xmlGroup = doc.addNewFolder().addNewGroups().addNewGroup();
xmlGroup.setName(SITE_GROUP_NAME);
xmlGroup.setType(GroupEnumType.SITE);
xmlGroup.addNewUsers().addNewUser().setName(_projectAdmin.getEmail());

FolderImportContext ctx = new FolderImportContext(_projectAdmin, _project, doc, null,
new StaticLoggerGetter(LogHelper.getLogger(GroupMembershipScopingTestCase.class, "Folder import test")), null);
new SecurityGroupImporterFactory.SecurityGroupImporter().process(null, ctx, null);

assertTrue(isMember(_siteGroup, _siteGroupMember));
assertFalse(isMember(_siteGroup, _projectAdmin));

// The archive's group lands as a project group of the same name instead
Group projectGroup = SecurityManager.getGroup(SecurityManager.getGroupId(_project, SITE_GROUP_NAME));
assertTrue(isMember(projectGroup, _projectAdmin));
}

private ActionURL addMemberUrl(Container c, Group group, User newMember)
{
return new ActionURL(GroupAction.class, c)
.addParameter("id", group.getUserId())
.addParameter("names", newMember.getEmail());
}

private boolean isMember(Group group, User user)
{
return SecurityManager.getGroupMembers(group, MemberType.ALL_GROUPS_AND_USERS).contains(user);
}
}
}
Loading