From 32c75cf6e222de8edf9f1407ed13b5ae6063c440 Mon Sep 17 00:00:00 2001 From: Lum Date: Tue, 16 Jun 2026 13:44:32 -0700 Subject: [PATCH 1/7] Link to study container scoping and test --- .../publish/AbstractPublishConfirmAction.java | 9 ++ study/src/org/labkey/study/StudyModule.java | 3 +- .../PublishConfirmContainerScopingTest.java | 123 ++++++++++++++++++ 3 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java diff --git a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java index e004ad49333..ae4980316f0 100644 --- a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java +++ b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java @@ -111,6 +111,9 @@ public void validateCommand(FORM form, Errors errors) 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) { @@ -331,6 +334,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) { @@ -375,6 +379,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) @@ -437,6 +444,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) diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 60bb3b9d968..981634a4403 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -718,7 +718,8 @@ public Set getIntegrationTests() VisitImpl.TestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, - org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class); + org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class, + org.labkey.study.controllers.publish.PublishConfirmContainerScopingTest.class); } @Override diff --git a/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java new file mode 100644 index 00000000000..9bced0bcdfd --- /dev/null +++ b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java @@ -0,0 +1,123 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.study.controllers.publish; + +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.exp.PropertyType; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.ViewContext; +import org.springframework.validation.BindException; +import org.springframework.validation.ObjectError; + +import java.util.List; +import java.util.Objects; + +public class PublishConfirmContainerScopingTest extends AbstractContainerScopingTest +{ + private static final String PERMISSION_ERROR = "You do not have permission to link data to the study"; + + private Container _source; + private Container _target; + private ExpSampleType _sampleType; + + @Before + public void setup() throws Exception + { + _source = createContainer("Source"); + _target = createContainer("Target"); + + StudyService.get().createStudy(_target, getAdmin(), "STUDY-4 scoping target", TimepointType.DATE, false); + _sampleType = createSampleType(_source); + + } + + @Test + public void testConfirmRejectsTargetStudyWithoutInsertPermission() throws Exception + { + // Editor in the source folder only: holds InsertPermission where the action runs, but none in the target study + User editor = createUserInRole(_source, EditorRole.class); + + assertTrue("Linking into a target study the caller cannot insert into must be rejected", + rejectedForPermission(validate(_source, _sampleType, _target, editor))); + } + + @Test + public void testConfirmAllowsTargetStudyWithInsertPermission() throws Exception + { + // Positive control: the same caller is also granted insert (Editor) in the target study, so the guard must not + // fire — proving it rejects only the cross-container case rather than every link + User editor = createUserInRole(_source, EditorRole.class); + grantRole(editor, _target, EditorRole.class); + + assertFalse("Linking into a target study the caller can insert into must be allowed", + rejectedForPermission(validate(_source, _sampleType, _target, editor))); + } + + /** + * Run the publish-confirm form's validation as {@code user}, linking {@code sampleType} (in {@code source}) to the + * study in {@code target}, and return the resulting errors. The STUDY-4 guard lives in the shared + * {@code AbstractPublishConfirmAction.validateCommand()}, reached here through the production + * {@link SampleTypePublishConfirmAction} so its {@code @RequiresPermission} wiring is exercised too. + */ + private BindException validate(Container source, ExpSampleType sampleType, Container target, User user) + { + ActionURL url = new ActionURL("study", "sampleTypePublishConfirm", source); + ViewContext context = ViewContext.getMockViewContext(user, source, url, false); + + SampleTypePublishConfirmAction action = new SampleTypePublishConfirmAction(); + action.setViewContext(context); + + SampleTypePublishConfirmAction.SampleTypePublishConfirmForm form = new SampleTypePublishConfirmAction.SampleTypePublishConfirmForm(); + form.setViewContext(context); + form.setRowId(sampleType.getRowId()); + form.setTargetStudy(new String[]{ target.getId() }); + form.setReturnUrl(url.toString()); + + BindException errors = new BindException(form, "form"); + action.validateCommand(form, errors); + return errors; + } + + private boolean rejectedForPermission(BindException errors) + { + return errors.getAllErrors().stream() + .map(ObjectError::getDefaultMessage) + .filter(Objects::nonNull) + .anyMatch(message -> message.contains(PERMISSION_ERROR)); + } + + private ExpSampleType createSampleType(Container c) throws Exception + { + List props = List.of( + new GWTPropertyDescriptor("Name", "string"), + new GWTPropertyDescriptor("Date", PropertyType.DATE.getTypeUri()), + new GWTPropertyDescriptor("PTID", "string") + ); + + return SampleTypeService.get().createSampleType(c, getAdmin(), "STUDY4ScopingSamples", null, + props, List.of(), -1,-1,-1,-1,null); + } +} From ac9f083fd7d5fd5aa1c366acc15b6fd6e93c5fbb Mon Sep 17 00:00:00 2001 From: Lum Date: Tue, 16 Jun 2026 15:52:26 -0700 Subject: [PATCH 2/7] Sanitize form furnished redirectUrl to validate --- .../reports/ReportsController.java | 42 ++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index f2b1b06dae4..3266ae27362 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -64,6 +64,7 @@ import org.labkey.api.study.reports.CrosstabReportDescriptor; import org.labkey.api.util.ImageUtil; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.ReturnURLString; import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.UniqueID; @@ -283,8 +284,14 @@ public boolean handlePost(SaveReportViewForm form, BindException errors) @Override public ActionURL getSuccessURL(SaveReportViewForm form) { - if (!StringUtils.isBlank(form.getRedirectUrl())) - return new ActionURL(form.getRedirectUrl()); + // Issue: redirectUrl is client-controlled. ReturnURLString validates it at bind time. + ReturnURLString redirectUrl = form.getRedirectUrl(); + if (redirectUrl != null && !redirectUrl.isEmpty()) + { + ActionURL url = redirectUrl.getActionURL(); + if (url != null) + return url; + } // after the save just redirect to the newly created view, ask the report for its run URL Report r = ReportService.get().getReport(getContainer(), _savedReportId); @@ -449,7 +456,8 @@ public ModelAndView getView(CrosstabDesignBean form, boolean reshow, BindExcepti bean.setQueryName(form.getQueryName()); bean.setDataRegionName(form.getDataRegionName()); bean.setViewName(form.getViewName()); - bean.setRedirectUrl(form.getRedirectUrl()); + if (form.getRedirectUrl() != null) + bean.setRedirectUrl(new ReturnURLString(form.getRedirectUrl())); bean.setErrors(errors); if (!getUser().isGuest()) @@ -824,7 +832,7 @@ public static class SaveReportViewForm extends SaveReportForm private String _schemaName; private String _viewName; private String _dataRegionName; - private String _redirectUrl; + private ReturnURLString _redirectUrl; public SaveReportViewForm() { @@ -914,12 +922,12 @@ public String getDataRegionName() return _dataRegionName; } - public String getRedirectUrl() + public ReturnURLString getRedirectUrl() { return _redirectUrl; } - public void setRedirectUrl(String redirectUrl) + public void setRedirectUrl(ReturnURLString redirectUrl) { _redirectUrl = redirectUrl; } @@ -1321,5 +1329,27 @@ controller.new ParticipantReportAction(), controller.new CreateQueryReportAction() ); } + + // GitHub Issue #1243 regression test. + @Test + public void redirectUrlConstrainedToAllowableHost() + { + SaveReportViewForm offHost = new SaveReportViewForm(); + offHost.setRedirectUrl(new ReturnURLString("https://evil.example.com/phish")); + assertNull("Off-host redirectUrl must not resolve to an off-site ActionURL", + offHost.getRedirectUrl().getActionURL()); + + SaveReportViewForm local = new SaveReportViewForm(); + local.setRedirectUrl(new ReturnURLString("/study-reports/begin.view?x=1")); + ActionURL localUrl = local.getRedirectUrl().getActionURL(); + assertNotNull("A local redirectUrl should resolve to an ActionURL", localUrl); + assertTrue("A local redirectUrl should be preserved same-origin", + localUrl.getLocalURIString().contains("begin.view")); + + SaveReportViewForm blank = new SaveReportViewForm(); + blank.setRedirectUrl(new ReturnURLString("")); + assertTrue("A blank redirectUrl should be empty so the action falls through to the run URL", + blank.getRedirectUrl().isEmpty()); + } } } From d882547a42ee5cdda2d047d91a7516a6a90d4ebc Mon Sep 17 00:00:00 2001 From: XingY Date: Tue, 16 Jun 2026 16:02:20 -0700 Subject: [PATCH 3/7] GitHub Issue 1243: FILE-3: check resource container --- .../org/labkey/filecontent/FileContentController.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/filecontent/src/org/labkey/filecontent/FileContentController.java b/filecontent/src/org/labkey/filecontent/FileContentController.java index 3f01703bb95..a6fe85c88c6 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentController.java +++ b/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; From f403074c3febc121c40c23c80bc8aad734d5049f Mon Sep 17 00:00:00 2001 From: Lum Date: Wed, 17 Jun 2026 09:50:27 -0700 Subject: [PATCH 4/7] Ensure group scoping matches what is allowed by the server. --- .../org/labkey/issue/IssuesController.java | 95 ++++++++++++++++++- issues/src/org/labkey/issue/IssuesModule.java | 3 +- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index ee2eb357a1a..8a90727cca3 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/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 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 @@ -1740,7 +1751,7 @@ public Object execute(Object form, BindException errors) { List 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; + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 06630d6dbad..dc9aad60385 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -195,7 +195,8 @@ public Set 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 ); } From 2d953c0a00cb1fbabb00c78a5e1029413d3cb3d5 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 17 Jun 2026 11:40:46 -0700 Subject: [PATCH 5/7] Use SQLFragment and appendIdentifier() --- .../report/SpecimenVisitReportParameters.java | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java b/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java index 882a5e078b7..7106faff06b 100644 --- a/specimen/src/org/labkey/specimen/report/SpecimenVisitReportParameters.java +++ b/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; } } From e8844b9054e5760759fc7a2af7f17d83e2503847 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 17 Jun 2026 12:38:50 -0700 Subject: [PATCH 6/7] Add redundant permission check and test --- .../labkey/specimen/actions/SpecimenController.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index 9d48a30e7c7..8b408d66585 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -1178,12 +1178,16 @@ public static class SpecimenEventsRedirectAction extends SimpleViewAction Date: Wed, 17 Jun 2026 15:54:19 -0700 Subject: [PATCH 7/7] Adjust existing test for different server validation failure --- .../PublishConfirmContainerScopingTest.java | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java index 9bced0bcdfd..5685ce2b157 100644 --- a/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java +++ b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java @@ -28,17 +28,14 @@ import org.labkey.api.study.StudyService; import org.labkey.api.study.TimepointType; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; import org.springframework.validation.BindException; -import org.springframework.validation.ObjectError; import java.util.List; -import java.util.Objects; public class PublishConfirmContainerScopingTest extends AbstractContainerScopingTest { - private static final String PERMISSION_ERROR = "You do not have permission to link data to the study"; - private Container _source; private Container _target; private ExpSampleType _sampleType; @@ -60,8 +57,8 @@ public void testConfirmRejectsTargetStudyWithoutInsertPermission() throws Except // Editor in the source folder only: holds InsertPermission where the action runs, but none in the target study User editor = createUserInRole(_source, EditorRole.class); - assertTrue("Linking into a target study the caller cannot insert into must be rejected", - rejectedForPermission(validate(_source, _sampleType, _target, editor))); + assertFalse("Linking into a target study the caller cannot insert into must be rejected", + validate(_source, _sampleType, _target, editor)); } @Test @@ -72,17 +69,15 @@ public void testConfirmAllowsTargetStudyWithInsertPermission() throws Exception User editor = createUserInRole(_source, EditorRole.class); grantRole(editor, _target, EditorRole.class); - assertFalse("Linking into a target study the caller can insert into must be allowed", - rejectedForPermission(validate(_source, _sampleType, _target, editor))); + assertTrue("Linking into a target study the caller can insert into must be allowed", + validate(_source, _sampleType, _target, editor)); } /** * Run the publish-confirm form's validation as {@code user}, linking {@code sampleType} (in {@code source}) to the - * study in {@code target}, and return the resulting errors. The STUDY-4 guard lives in the shared - * {@code AbstractPublishConfirmAction.validateCommand()}, reached here through the production - * {@link SampleTypePublishConfirmAction} so its {@code @RequiresPermission} wiring is exercised too. + * study in {@code target}, returns true if validation succeeds, false otherwise. */ - private BindException validate(Container source, ExpSampleType sampleType, Container target, User user) + private boolean validate(Container source, ExpSampleType sampleType, Container target, User user) { ActionURL url = new ActionURL("study", "sampleTypePublishConfirm", source); ViewContext context = ViewContext.getMockViewContext(user, source, url, false); @@ -93,20 +88,19 @@ private BindException validate(Container source, ExpSampleType sampleType, Conta SampleTypePublishConfirmAction.SampleTypePublishConfirmForm form = new SampleTypePublishConfirmAction.SampleTypePublishConfirmForm(); form.setViewContext(context); form.setRowId(sampleType.getRowId()); - form.setTargetStudy(new String[]{ target.getId() }); + form.setTargetStudy(new String[]{target.getId()}); form.setReturnUrl(url.toString()); - BindException errors = new BindException(form, "form"); - action.validateCommand(form, errors); - return errors; - } - - private boolean rejectedForPermission(BindException errors) - { - return errors.getAllErrors().stream() - .map(ObjectError::getDefaultMessage) - .filter(Objects::nonNull) - .anyMatch(message -> message.contains(PERMISSION_ERROR)); + try + { + BindException errors = new BindException(form, "form"); + action.validateCommand(form, errors); + } + catch (UnauthorizedException e) + { + return false; + } + return true; } private ExpSampleType createSampleType(Container c) throws Exception