Container scoping for miscellaneous server objects

api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java

@@ -116,6 +116,9 @@ else if (!_targetStudy.hasPermission(getUser(), InsertPermission.class))
 
         if (_targetStudy != null)
         {
+            if (!_targetStudy.hasPermission(getUser(), InsertPermission.class))
+                errors.reject(SpringActionController.ERROR_MSG, "You do not have permission to link data to the study in " + _targetStudy.getPath() + ".");
+
             Study study = StudyService.get().getStudy(_targetStudy);
             if (study == null)
             {
@@ -336,6 +339,7 @@ private void attemptLinkage(FORM form, BindException errors,
         boolean missingStudy = false;
         boolean badVisitIds = false;
         boolean badDates = false;
+        boolean noPermissionStudy = false;
         int index = 0;
         for (int objectId : allObjects)
         {
@@ -380,6 +384,9 @@ private void attemptLinkage(FORM form, BindException errors,
             }
             else
             {
+                if (selected && !rowLevelTargetStudy.hasPermission(getUser(), InsertPermission.class))
+                    noPermissionStudy = true;
+
                 postedTargetStudies.put(objectId, rowLevelTargetStudy.getId());
 
                 if (StudyPublishService.get().getTimepointType(rowLevelTargetStudy) == TimepointType.VISIT)
@@ -442,6 +449,8 @@ private void attemptLinkage(FORM form, BindException errors,
 
         if (missingStudy)
             errors.reject(null, "You must specify a Target Study for all selected rows.");
+        if (noPermissionStudy)
+            errors.reject(null, "You do not have permission to link data to one or more of the selected target studies.");
         if (missingPtid)
             errors.reject(null, "You must specify a Participant ID for all selected rows.");
         if (missingVisitId)

filecontent/src/org/labkey/filecontent/FileContentController.java

@@ -945,7 +945,15 @@ public void validateForm(SimpleApiJsonForm form, Errors errors)
                     {
                         FileContentServiceImpl fileContentService = FileContentServiceImpl.getInstance();
                         WebdavResource resource = fileContentService.getResource(String.valueOf(fileProps.get("id")));
-                        if (resource != null && !resource.getActions(getUser()).isEmpty())
+
+                        // GitHub Issue 1243: check resource container
+                        if (resource == null || !getContainer().getEntityId().equals(resource.getContainerId()))
+                        {
+                            errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, "Invalid file", fileProps.get("id")));
+                            return;
+                        }
+
+                        if (!resource.getActions(getUser()).isEmpty())
                         {
                             errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, resource.getName(), "has been previously processed, properties cannot be edited"));
                             return;

issues/src/org/labkey/issue/IssuesController.java

@@ -28,6 +28,8 @@
 import org.jetbrains.annotations.Nullable;
 import org.json.JSONArray;
 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;
@@ -106,6 +108,7 @@
 import org.labkey.api.security.permissions.InsertPermission;
 import org.labkey.api.security.permissions.ReadPermission;
 import org.labkey.api.security.permissions.UpdatePermission;
+import org.labkey.api.security.roles.EditorRole;
 import org.labkey.api.security.roles.FolderAdminRole;
 import org.labkey.api.security.roles.OwnerRole;
 import org.labkey.api.security.roles.ReaderRole;
@@ -115,12 +118,12 @@
 import org.labkey.api.util.DOM;
 import org.labkey.api.util.GUID;
 import org.labkey.api.util.HtmlString;
+import org.labkey.api.util.InputBuilder;
 import org.labkey.api.util.JsonUtil;
 import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.Pair;
 import org.labkey.api.util.TestContext;
 import org.labkey.api.util.URLHelper;
-import org.labkey.api.util.InputBuilder;
 import org.labkey.api.view.ActionURL;
 import org.labkey.api.view.HtmlView;
 import org.labkey.api.view.HttpView;
@@ -151,6 +154,7 @@
 import org.labkey.issue.view.IssuesListView;
 import org.springframework.beans.MutablePropertyValues;
 import org.springframework.beans.PropertyValues;
+import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.util.LinkedCaseInsensitiveMap;
 import org.springframework.validation.BindException;
 import org.springframework.validation.Errors;
@@ -1731,6 +1735,13 @@ public URLHelper getSuccessURL(IssuesDomainKindProperties form)
         }
     }
 
+    private static List<Group> getSelectableAssignmentGroups(Container c, User user)
+    {
+        return SecurityManager.getGroups(c.getProject(), true).stream()
+                .filter(group -> !group.isGuests() && (!group.isUsers() || user.hasRootAdminPermission()))
+                .toList();
+    }
+
     @Marshal(Marshaller.Jackson)
     @RequiresPermission(AdminPermission.class)
     public static class GetProjectGroupsAction extends ReadOnlyApiAction<Object>
@@ -1740,7 +1751,7 @@ public Object execute(Object form, BindException errors)
         {
             List<UserGroupForm> groups = new ArrayList<>();
 
-            SecurityManager.getGroups(getContainer().getProject(), true).stream().filter(group -> !group.isGuests() && (!group.isUsers() || getUser().hasRootAdminPermission())).forEach(group -> {
+            getSelectableAssignmentGroups(getContainer(), getUser()).forEach(group -> {
                 String displayText = (group.isProjectGroup() ? "" : "Site: ") + group.getName();
 
                 UserGroupForm userGroups = new UserGroupForm();
@@ -1764,8 +1775,11 @@ public Object execute(UserGroupForm form, BindException errors)
 
             if (null != form.getGroupId())
             {
+                // ensure group is in scope for the container
                 Group group = SecurityManager.getGroup(form.getGroupId());
-                if (group != null)
+                boolean inScope = group != null && getSelectableAssignmentGroups(getContainer(), getUser())
+                        .stream().anyMatch(g -> g.getUserId() == group.getUserId());
+                if (inScope)
                 {
                     for (User user : SecurityManager.getAllGroupMembers(group, MemberType.ACTIVE_USERS, group.isUsers()))
                     {
@@ -2458,4 +2472,79 @@ private static void ensureIssuesEnabled(Container c)
             }
         }
     }
+
+    /**
+     * GitHub Issue #1243 regression test.
+     */
+    public static class GetUsersForGroupScopingTestCase extends AbstractContainerScopingTest
+    {
+        private static final String PROJECT_A = "IssuesGroupScopingA";
+        private static final String PROJECT_B = "IssuesGroupScopingB";
+
+        private Container _projectA;
+        private Group _groupA;
+        private Group _groupB;
+        private User _member;
+
+        @Before
+        public void setup() throws Exception
+        {
+            deleteProjects();
+            User admin = getAdmin();
+            _projectA = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_A, admin);
+            Container projectB = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_B, admin);
+
+            // Security groups must be associated with a project, so each group's container is its owning project.
+            _groupA = SecurityManager.createGroup(_projectA, "ScopingGroupA", admin);
+            _groupB = SecurityManager.createGroup(projectB, "ScopingGroupB", admin);
+
+            // A user who can be assigned issues in project A (UpdatePermission via Editor) and is a member of both groups.
+            _member = createUserInRole(_projectA, EditorRole.class);
+            SecurityManager.addMember(_groupA, _member);
+            SecurityManager.addMember(_groupB, _member);
+        }
+
+        @After
+        public void tearDown()
+        {
+            deleteProjects();
+        }
+
+        @Test
+        public void doesNotEnumerateAnotherProjectsGroupMembers() throws Exception
+        {
+            // group B is in a different project from the source request.
+            String content = requestUsersForGroup(_projectA, _groupB).getContentAsString();
+            assertFalse("A group id from another project must not enumerate that group's members through this folder",
+                    content.contains("\"userId\" : " + _member.getUserId()));
+        }
+
+        @Test
+        public void enumeratesOwnProjectGroupMembers() throws Exception
+        {
+            // Control: the request project's own group resolves in scope and returns its members.
+            String content = requestUsersForGroup(_projectA, _groupA).getContentAsString();
+            assertTrue("An in-project group id must enumerate its members",
+                    content.contains("\"userId\" : " + _member.getUserId()));
+        }
+
+        private MockHttpServletResponse requestUsersForGroup(Container project, Group group) throws Exception
+        {
+            ActionURL url = new ActionURL(GetUsersForGroupAction.class, project)
+                    .addParameter("groupId", group.getUserId());
+            return get(url, getAdmin());
+        }
+
+        private void deleteProjects()
+        {
+            User admin = getAdmin();
+            for (String name : new String[]{PROJECT_A, PROJECT_B})
+            {
+                Container c = ContainerManager.getForPath("/" + name);
+                if (c != null)
+                    ContainerManager.deleteAll(c, admin);
+            }
+            _projectA = null;
+        }
+    }
 }

issues/src/org/labkey/issue/IssuesModule.java

@@ -195,7 +195,8 @@ public Set<Class> getIntegrationTests()
     {
         return Set.of(
             org.labkey.issue.model.IssueManager.TestCase.class,
-            org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class
+            org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class,
+            org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class
         );
     }
 

specimen/src/org/labkey/specimen/actions/SpecimenController.java

@@ -1182,12 +1182,16 @@ public static class SpecimenEventsRedirectAction extends SimpleViewAction<Specim
         @Override
         public ModelAndView getView(SpecimenEventForm form, BindException errors)
         {
-            if (form.getId() != null && form.getTargetStudy() != null)
+            Container targetStudy = form.getTargetStudy();
+            // Note that the query schema used by getVial() below will not select the vial if the user doesn't have
+            // read permissions in the target study, so technically this permission check is redundant. However,
+            // getVial() without read permissions leads to an NPE, so checking here improves behavior.
+            if (form.getId() != null && targetStudy != null && targetStudy.hasPermission(getUser(), ReadPermission.class))
             {
-                Vial vial = SpecimenManager.get().getVial(form.getTargetStudy(), getUser(), form.getId());
+                Vial vial = SpecimenManager.get().getVial(targetStudy, getUser(), form.getId());
                 if (vial != null)
                 {
-                    ActionURL url = new ActionURL(SpecimenEventsAction.class, form.getTargetStudy()).addParameter("id", vial.getRowId());
+                    ActionURL url = new ActionURL(SpecimenEventsAction.class, targetStudy).addParameter("id", vial.getRowId());
                     throw new RedirectException(url);
                 }
             }

specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java

@@ -289,9 +289,13 @@ protected void addParticipantGroupFilter(SimpleFilter filter, int ptidListId)
         ParticipantGroup group = ParticipantGroupService.get().getParticipantGroup(getContainer(), getUser(), ptidListId);
         if (group != null)
         {
-            SQLFragment sql = new SQLFragment();
-            sql.append("(").append(StudyService.get().getSubjectColumnName(getContainer())).append(" IN (SELECT ");
-            sql.append("ParticipantId FROM ").append(SpecimenSchema.get().getTableInfoParticipantGroupMap()).append(" WHERE GroupId = ?))").add(ptidListId);
+            SQLFragment sql = new SQLFragment("(")
+                .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer()))
+                .append(" IN (SELECT ")
+                .append("ParticipantId FROM ")
+                .append(SpecimenSchema.get().getTableInfoParticipantGroupMap())
+                .append(" WHERE GroupId = ?))")
+                .add(ptidListId);
             filter.addWhereClause(sql);
         }
     }
@@ -303,11 +307,15 @@ protected void addCohortFilter(SimpleFilter filter, CohortFilter cohortFilter)
 
         if (cohortFilter == CohortService.get().getUnassignedCohortFilter())
         {
-            filter.addWhereClause("(" + StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" +
-                "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId IS NULL AND Container = ?)" +
-                " OR (" + StudyService.get().getSubjectColumnName(getContainer()) +
-                " NOT IN (SELECT ParticipantId FROM study.participant WHERE Container = ?)))",
-                    new Object[] { getContainer().getId(), getContainer().getId()});
+            SQLFragment whereClause = new SQLFragment("(")
+                .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer()))
+                .append(" IN\n" + "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId IS NULL AND Container = ?)" + " OR (")
+                .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer()))
+                .append(" NOT IN (SELECT ParticipantId FROM study.participant WHERE Container = ?)))")
+                .add(getContainer())
+                .add(getContainer());
+
+            filter.addWhereClause(whereClause);
         }
         else if (cohortFilter != null)
         {
@@ -317,18 +325,25 @@ else if (cohortFilter != null)
             switch (cohortFilter.getType())
             {
                 case DATA_COLLECTION:
-                    filter.addWhereClause("CollectionCohort = ? AND Container = ?",
-                            new Object[] { cohortId, getContainer().getId()} );
+                    filter.addWhereClause(new SQLFragment("CollectionCohort = ? AND Container = ?")
+                        .add(cohortId)
+                        .add(getContainer())
+                    );
                     break;
                 case PTID_CURRENT:
-                    filter.addWhereClause(StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" +
-                                    "(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId = ? AND Container = ?)",
-                            new Object[] { cohortId, getContainer().getId()});
+                    filter.addWhereClause(new SQLFragment()
+                        .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer()))
+                        .append(" IN\n(SELECT ParticipantId FROM study.participant WHERE CurrentCohortId = ? AND Container = ?)")
+                        .add(cohortId)
+                        .add(getContainer())
+                    );
                     break;
                 case PTID_INITIAL:
-                    filter.addWhereClause(StudyService.get().getSubjectColumnName(getContainer()) + " IN\n" +
-                                    "(SELECT ParticipantId FROM study.participant WHERE InitialCohortId = ? AND Container = ?)",
-                            new Object[] { cohortId, getContainer().getId()});
+                    filter.addWhereClause(new SQLFragment()
+                        .appendIdentifier(StudyService.get().getSubjectColumnName(getContainer()))
+                        .append(" IN\n(SELECT ParticipantId FROM study.participant WHERE InitialCohortId = ? AND Container = ?)")
+                        .add(cohortId).add(getContainer())
+                    );
                     break;
             }
         }

study/src/org/labkey/study/StudyModule.java

@@ -132,6 +132,7 @@
 import org.labkey.study.controllers.StudyController;
 import org.labkey.study.controllers.StudyDefinitionController;
 import org.labkey.study.controllers.StudyPropertiesController;
+import org.labkey.study.controllers.publish.PublishConfirmContainerScopingTest;
 import org.labkey.study.controllers.publish.PublishController;
 import org.labkey.study.controllers.reports.ReportsController;
 import org.labkey.study.controllers.security.SecurityController;
@@ -722,6 +723,7 @@ public Set<Class> getIntegrationTests()
                 DatasetController.DatasetAuditHistoryScopingTestCase.class,
                 DatasetUpdateService.TestCase.class,
                 DatasetLsidImportHelper.TestCase.class,
+                PublishConfirmContainerScopingTest.class,
                 CreateChildStudyAction.ContainerScopingTestCase.class,
                 StudyController.ContainerScopingTestCase.class,
                 ReportsController.ContainerScopingTestCase.class);