From fdfcd8d73e665fd0d4382993fb88a826308ecf9e Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 30 Oct 2026 22:03:55 -0700 Subject: [PATCH 1/2] GH Issue 1695: Require group-owning container for all group membership changes --- .../labkey/api/security/SecurityManager.java | 3 + core/src/org/labkey/core/CoreModule.java | 1 + .../SecurityGroupImporterFactory.java | 9 +- .../core/security/SecurityApiActions.java | 6 +- .../core/security/SecurityController.java | 137 ++++++++++++++++-- 5 files changed, 143 insertions(+), 13 deletions(-) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 203ddbd6eb7..0ca00ba3dd6 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -1443,6 +1443,9 @@ public static void deleteMembers(Group group, Collection 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); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 27c44ea8b2d..0060d529c4a 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -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, diff --git a/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java b/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java index dff59447465..1f5a528903f 100644 --- a/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java +++ b/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java @@ -22,9 +22,12 @@ 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.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; @@ -81,7 +84,11 @@ 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) + SecurityController.verifyUserCanModifyGroup(group, ctx.getContainer(), ctx.getUser()); + 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 diff --git a/core/src/org/labkey/core/security/SecurityApiActions.java b/core/src/org/labkey/core/security/SecurityApiActions.java index ff4ca871fe8..65f4e01c3e8 100644 --- a/core/src/org/labkey/core/security/SecurityApiActions.java +++ b/core/src/org/labkey/core/security/SecurityApiActions.java @@ -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 memberErrors = new HashMap<>(); List newUsers = new ArrayList<>(); @@ -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()) { @@ -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()) { diff --git a/core/src/org/labkey/core/security/SecurityController.java b/core/src/org/labkey/core/security/SecurityController.java index 7477aa6bd50..6ae916610c1 100644 --- a/core/src/org/labkey/core/security/SecurityController.java +++ b/core/src/org/labkey/core/security/SecurityController.java @@ -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; @@ -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; @@ -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; @@ -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; @@ -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; @@ -775,7 +785,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<>(); @@ -918,18 +928,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 @@ -2536,4 +2546,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); + } + } } From 9968b2503f20469c02b430e1bcbb65543c4af5bf Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 30 Oct 2026 22:37:53 -0700 Subject: [PATCH 2/2] Claude feedback --- .../importer/SecurityGroupImporterFactory.java | 13 ++++++++++++- .../labkey/core/security/SecurityController.java | 1 - 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java b/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java index 1f5a528903f..0690c5d8130 100644 --- a/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java +++ b/core/src/org/labkey/core/admin/importer/SecurityGroupImporterFactory.java @@ -25,6 +25,7 @@ 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; @@ -87,7 +88,17 @@ public void process(@Nullable PipelineJob job, FolderImportContext ctx, VirtualF // 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) - SecurityController.verifyUserCanModifyGroup(group, ctx.getContainer(), ctx.getUser()); + { + 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()); } diff --git a/core/src/org/labkey/core/security/SecurityController.java b/core/src/org/labkey/core/security/SecurityController.java index 6ae916610c1..bb190f51bc4 100644 --- a/core/src/org/labkey/core/security/SecurityController.java +++ b/core/src/org/labkey/core/security/SecurityController.java @@ -771,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.