From 3c591cd9286837abc31e4eedb33cd2bb6b8f80ef Mon Sep 17 00:00:00 2001 From: Josh Eckels Date: Mon, 15 Jun 2026 09:50:01 -0700 Subject: [PATCH 01/13] Use centralized XML parser config - backport (#7751) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Rationale Backport changes already in develop to help configure XML parsers consistently #### Changes - Configure parsers for security #### Tasks 📍 - [x] Claude Code Review - ~Manual Testing~ - ~Test Automation~ --- api/src/org/labkey/api/util/XmlBeansUtil.java | 71 +++++++++++++++---- 1 file changed, 57 insertions(+), 14 deletions(-) diff --git a/api/src/org/labkey/api/util/XmlBeansUtil.java b/api/src/org/labkey/api/util/XmlBeansUtil.java index 6f0eafdd61e..397e2eb7d47 100644 --- a/api/src/org/labkey/api/util/XmlBeansUtil.java +++ b/api/src/org/labkey/api/util/XmlBeansUtil.java @@ -28,6 +28,7 @@ import org.labkey.api.settings.LookAndFeelProperties; import org.xml.sax.SAXException; +import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParserFactory; @@ -121,36 +122,78 @@ public static void addComment(XmlTokenSource doc, String comment) cursor.dispose(); } - /** XML parsing factories preconfigured to prevent XML external entity references (XXE) */ + /** + * XML parsing factories preconfigured to prevent XML external entity references (XXE). + * These are static and are unfortunately mutable. We could switch to a factory pattern to create + * freshly configured factories. + */ public static final SAXParserFactory SAX_PARSER_FACTORY; + public static final SAXParserFactory SAX_PARSER_FACTORY_ALLOWING_DOCTYPE; public static final XMLInputFactory XML_INPUT_FACTORY; public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY; + public static final DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE; static { + //noinspection XMLInputFactory XML_INPUT_FACTORY = XMLInputFactory.newInstance(); XML_INPUT_FACTORY.setProperty(XMLInputFactory.SUPPORT_DTD, false); XML_INPUT_FACTORY.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); - SAX_PARSER_FACTORY = SAXParserFactory.newInstance(); try { - SAX_PARSER_FACTORY.setNamespaceAware(true); - SAX_PARSER_FACTORY.setFeature("http://xml.org/sax/features/validation", false); - SAX_PARSER_FACTORY.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); - SAX_PARSER_FACTORY.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - - DOCUMENT_BUILDER_FACTORY = DocumentBuilderFactory.newInstance(); - DOCUMENT_BUILDER_FACTORY.setNamespaceAware(true); - DOCUMENT_BUILDER_FACTORY.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); - DOCUMENT_BUILDER_FACTORY.setFeature("http://xml.org/sax/features/external-general-entities", false); - DOCUMENT_BUILDER_FACTORY.setFeature("http://xml.org/sax/features/external-parameter-entities", false); - DOCUMENT_BUILDER_FACTORY.setXIncludeAware(false); - DOCUMENT_BUILDER_FACTORY.setExpandEntityReferences(false); + SAX_PARSER_FACTORY = saxParserFactory(false); + SAX_PARSER_FACTORY_ALLOWING_DOCTYPE = saxParserFactory(true); + + DOCUMENT_BUILDER_FACTORY = documentBuilderFactory(false); + // Use the ALLOWING_DOCTYPE variant when parsing XML that contains a declaration (e.g. NCBI's eSummary responses) + DOCUMENT_BUILDER_FACTORY_ALLOWING_DOCTYPE = documentBuilderFactory(true); } catch (ParserConfigurationException | SAXException e) { throw UnexpectedException.wrap(e); } } + + private static SAXParserFactory saxParserFactory(boolean allowDocType) throws SAXException, ParserConfigurationException + { + //noinspection XMLInputFactory + SAXParserFactory result = SAXParserFactory.newInstance(); + result.setNamespaceAware(true); + result.setFeature("http://xml.org/sax/features/validation", false); + result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + + // Disable features that could lead to XXE or other vulnerabilities + // Keep in sync with ModuleArchive.nameFromModuleXML() + if (!allowDocType) + { + result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } + result.setFeature("http://xml.org/sax/features/external-general-entities", false); + result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + return result; + } + + private static DocumentBuilderFactory documentBuilderFactory(boolean allowDocType) throws ParserConfigurationException + { + //noinspection XMLInputFactory + DocumentBuilderFactory result = DocumentBuilderFactory.newInstance(); + result.setNamespaceAware(true); + + // Disable features that could lead to XXE or other vulnerabilities. + // When allowDocType is true the DOCTYPE declaration is permitted. External entity + // resolution remains disabled, so XXE protection is still in effect. + if (!allowDocType) + { + result.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + } + result.setFeature("http://xml.org/sax/features/external-general-entities", false); + result.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + result.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + result.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + result.setXIncludeAware(false); + result.setExpandEntityReferences(false); + return result; + } } From b29668ab5a6d68572062ea8c9b265fceb5caab30 Mon Sep 17 00:00:00 2001 From: Karl Lum Date: Mon, 15 Jun 2026 11:08:31 -0700 Subject: [PATCH 02/13] Container scoping improvements for NAb assay (#7747) #### Rationale Small refactors to help introduce better container scoping checks for NAb actions. #### Related Pull Requests - https://github.com/LabKey/platform/pull/7747 - https://github.com/LabKey/commonAssays/pull/1022 - https://github.com/LabKey/testAutomation/pull/3042 --------- Co-authored-by: labkey-nicka Co-authored-by: cnathe --- .../org/labkey/api/exp/api/ExperimentService.java | 3 +++ api/src/org/labkey/api/view/ViewServlet.java | 6 ++++++ .../api/assay/nab/view/DilutionGraphAction.java | 3 ++- .../api/assay/nab/view/GraphSelectedAction.java | 9 +++++++++ .../api/assay/nab/view/MultiGraphAction.java | 12 ++++++++++-- .../experiment/api/ExperimentServiceImpl.java | 15 ++++++++++++++- 6 files changed, 44 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/exp/api/ExperimentService.java b/api/src/org/labkey/api/exp/api/ExperimentService.java index eb195043dee..b744dcbf3ca 100644 --- a/api/src/org/labkey/api/exp/api/ExperimentService.java +++ b/api/src/org/labkey/api/exp/api/ExperimentService.java @@ -163,6 +163,9 @@ enum DataTypeForExclusion @Nullable ExpRun getExpRun(int rowId); + @Nullable + ExpRun getExpRun(int rowId, @Nullable Container container); + List getExpRuns(Collection rowIds); @Nullable diff --git a/api/src/org/labkey/api/view/ViewServlet.java b/api/src/org/labkey/api/view/ViewServlet.java index 2f446760067..48273010c3b 100644 --- a/api/src/org/labkey/api/view/ViewServlet.java +++ b/api/src/org/labkey/api/view/ViewServlet.java @@ -506,6 +506,12 @@ public void setActionURL(ActionURL actionURL) return _actionURL.getParameterMap(); } + @Override + public String getParameter(@NotNull String name) + { + return _actionURL.getParameter(name); + } + @Override public @NotNull String @NotNull [] getParameterValues(@NotNull String name) { diff --git a/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java b/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java index 43787be934c..194313e095a 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/DilutionGraphAction.java @@ -44,7 +44,8 @@ public ModelAndView getView(GraphForm form, BindException errors) throws Excepti { if (form.getRowId() == -1) throw new NotFoundException("Run ID not specified."); - ExpRun run = ExperimentService.get().getExpRun(form.getRowId()); + // GitHub Issue #1892: Resolve the run scoped to the current container + ExpRun run = ExperimentService.get().getExpRun(form.getRowId(), getContainer()); if (run == null) throw new NotFoundException("Run " + form.getRowId() + " does not exist."); diff --git a/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java b/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java index 2db8d7c6c69..4c5e9a033e2 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/GraphSelectedAction.java @@ -71,6 +71,8 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio objectIds[idx++] = Integer.parseInt(objectIdString); } + // GitHub Issue #1892: (NAB-9) The object ids come straight from the request and getDilutionSummaries() resolves them to runs + verifyObjectIdsReadable(objectIds); Set cutoffSet = new HashSet<>(); DilutionAssayProvider provider = (DilutionAssayProvider) AssayService.get().getProvider(_protocol); Map summaries = provider.getDataHandler().getDilutionSummaries(getUser(), form.getFitTypeEnum(), objectIds); @@ -92,6 +94,13 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio return new VBox(new AssayHeaderView(_protocol, provider, false, true, null), multiGraphView); } + /** + * Verify that the current user may view each of the requested object ids before any run data is loaded. + */ + protected void verifyObjectIdsReadable(int[] ids) throws Exception + { + } + protected abstract GraphSelectedBean createSelectionBean(ViewContext context, ExpProtocol protocol, int[] cutoffs, int[] dataObjectIds, String caption, String title); diff --git a/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java b/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java index 09924804c8c..ef6bbb09afc 100644 --- a/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java +++ b/assay/api-src/org/labkey/api/assay/nab/view/MultiGraphAction.java @@ -16,13 +16,13 @@ package org.labkey.api.assay.nab.view; import org.labkey.api.action.SimpleViewAction; +import org.labkey.api.assay.AssayService; import org.labkey.api.assay.dilution.DilutionAssayProvider; import org.labkey.api.assay.dilution.DilutionAssayRun; import org.labkey.api.assay.dilution.DilutionSummary; import org.labkey.api.assay.nab.NabGraph; import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExperimentService; -import org.labkey.api.assay.AssayService; import org.labkey.api.view.NavTree; import org.springframework.validation.BindException; import org.springframework.web.servlet.ModelAndView; @@ -35,12 +35,13 @@ * User: klum * Date: 6/11/13 */ -public class MultiGraphAction extends SimpleViewAction +public abstract class MultiGraphAction extends SimpleViewAction { @Override public ModelAndView getView(FormType form, BindException errors) throws Exception { int[] ids = form.getId(); + verifyObjectIdsReadable(ids); ExpProtocol protocol = ExperimentService.get().getExpProtocol(form.getProtocolId()); DilutionAssayProvider provider = (DilutionAssayProvider)AssayService.get().getProvider(protocol); Map summaries = provider.getDataHandler().getDilutionSummaries(getUser(), form.getFitTypeEnum(), ids); @@ -63,6 +64,13 @@ public ModelAndView getView(FormType form, BindException errors) throws Exceptio return null; } + /** + * Verify that the current user may view each of the requested object ids before any run data is loaded. + */ + protected void verifyObjectIdsReadable(int[] ids) throws Exception + { + } + protected NabGraph.Config getGraphConfig(FormType form) { NabGraph.Config config = new NabGraph.Config(); diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index fe53e505d8f..a5ea0f69505 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -374,10 +374,23 @@ public void clearDataClassCache(@Nullable Container c) @Override public @Nullable ExpRunImpl getExpRun(int rowId) + { + return getExpRun(rowId, null); + } + + @Override + public @Nullable ExpRunImpl getExpRun(int rowId, @Nullable Container container) { SimpleFilter filter = new SimpleFilter(FieldKey.fromParts(ExpRunTable.Column.RowId.name()), rowId); ExperimentRun run = new TableSelector(getTinfoExperimentRun(), filter, null).getObject(ExperimentRun.class); - return run == null ? null : new ExpRunImpl(run); + if (run == null) + return null; + + // GitHub Issue #1892: if container provided, ensure the run belongs to the container + if (container != null && !run.getContainer().equals(container)) + return null; + + return new ExpRunImpl(run); } private List getExpRuns(SimpleFilter filter) From 85ecead18eacb954137b8d50361f7c9e950aec5e Mon Sep 17 00:00:00 2001 From: Cory Nathe Date: Tue, 16 Jun 2026 14:30:03 -0500 Subject: [PATCH 03/13] TreatmentManager container scoping (#7758) #### Rationale Update TreatmentManager getStudyProductsDoseAndRoute / getDoseAndRoute / deleteStudyProduct filters to scope by container --- .../studydesign/model/TreatmentManager.java | 141 +++++++++++++++++- 1 file changed, 138 insertions(+), 3 deletions(-) diff --git a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java index 8bfae0273ed..1c441aef779 100644 --- a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java +++ b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java @@ -54,6 +54,7 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.TestContext; +import org.labkey.api.view.NotFoundException; import java.math.BigDecimal; import java.util.ArrayList; @@ -318,7 +319,10 @@ public void deleteStudyProduct(Container container, User user, int rowId) deleteProductAntigens(container, user, rowId); // delete the associated doses and routes for this product - Table.delete(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), new SimpleFilter(FieldKey.fromParts("ProductId"), rowId)); + // GitHub Kanban #1929: scope to the container in addition to ProductId + SimpleFilter doseAndRouteFilter = SimpleFilter.createContainerFilter(container); + doseAndRouteFilter.addCondition(FieldKey.fromParts("ProductId"), rowId); + Table.delete(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRouteFilter); // delete the associated treatment study product mappings (provision table) SimpleFilter filter = SimpleFilter.createContainerFilter(container); @@ -363,19 +367,31 @@ public DoseAndRoute saveStudyProductDoseAndRoute(Container container, User user, if (doseAndRoute.isNew()) return Table.insert(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute); else + { + // GitHub Kanban #1929: verify the existing row is in this container before updating + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("RowId"), doseAndRoute.getRowId()); + if (!new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), filter, null).exists()) + throw new NotFoundException("No dose and route found for rowId: " + doseAndRoute.getRowId()); + return Table.update(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute, doseAndRoute.getRowId()); + } } public Collection getStudyProductsDoseAndRoute(Container container, User user, int productId) { - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("ProductId"), productId); + // GitHub Kanban #1929: scope to the container in addition to ProductId + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("ProductId"), productId); return new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), filter, null).getCollection(DoseAndRoute.class); } @Nullable public DoseAndRoute getDoseAndRoute(Container container, String dose, String route, int productId) { - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("ProductId"), productId); + // GitHub Kanban #1929: scope to the container in addition to ProductId + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("ProductId"), productId); if (dose != null) filter.addCondition(FieldKey.fromParts("Dose"), dose); else @@ -989,6 +1005,125 @@ private void tearDown() assertTrue(ContainerManager.delete(_junitStudy.getContainer(), _context.getUser())); } } + + // GitHub Kanban #1929: getStudyProductsDoseAndRoute / getDoseAndRoute / deleteStudyProduct container scoping checks + @Test + public void testDoseAndRouteContainerScoping() throws Exception + { + TestContext context = TestContext.get(); + User user = context.getUser(); + Container containerA = null; + Container containerB = null; + try + { + containerA = createStudyContainer(context, GUID.makeHash()); + containerB = createStudyContainer(context, GUID.makeHash()); + + int productIdA = insertStudyProduct(containerA, user, "Immunogen A", "Immunogen"); + int productIdB = insertStudyProduct(containerB, user, "Immunogen B", "Immunogen"); + + _manager.saveStudyProductDoseAndRoute(containerA, user, new DoseAndRoute("Dose A", "Route A", productIdA, containerA)); + _manager.saveStudyProductDoseAndRoute(containerB, user, new DoseAndRoute("Dose B", "Route B", productIdB, containerB)); + + // getStudyProductsDoseAndRoute: scoped by container, not ProductId alone + assertEquals("Dose/route should be returned within its own container", 1, + _manager.getStudyProductsDoseAndRoute(containerB, user, productIdB).size()); + assertEquals("Dose/route must not be returned from another container", 0, + _manager.getStudyProductsDoseAndRoute(containerA, user, productIdB).size()); + + // getDoseAndRoute: same scoping + assertNotNull("getDoseAndRoute should find the row within its own container", + _manager.getDoseAndRoute(containerB, "Dose B", "Route B", productIdB)); + assertNull("getDoseAndRoute must not find the row from another container", + _manager.getDoseAndRoute(containerA, "Dose B", "Route B", productIdB)); + + // deleteStudyProduct's dose/route delete is now container scoped; deleting the product in its own + // container removes its dose/route (the cross-container case is unreachable since ProductId is unique). + _manager.deleteStudyProduct(containerB, user, productIdB); + assertEquals("deleteStudyProduct should remove the dose/route within its own container", 0, + _manager.getStudyProductsDoseAndRoute(containerB, user, productIdB).size()); + } + finally + { + if (null != containerB) + ContainerManager.delete(containerB, user); + if (null != containerA) + ContainerManager.delete(containerA, user); + } + } + + // GitHub Kanban #1929: saveStudyProductDoseAndRoute rejects updating a dose/route row from another container + @Test + public void testCrossContainerDoseAndRouteUpdateDenied() + { + TestContext context = TestContext.get(); + User user = context.getUser(); + Container containerA = null; + Container containerB = null; + try + { + containerA = createStudyContainer(context, GUID.makeHash()); + containerB = createStudyContainer(context, GUID.makeHash()); + + int productIdA = insertStudyProduct(containerA, user, "Immunogen A", "Immunogen"); + int productIdB = insertStudyProduct(containerB, user, "Immunogen B", "Immunogen"); + + // a dose/route owned by container B + DoseAndRoute savedB = _manager.saveStudyProductDoseAndRoute(containerB, user, new DoseAndRoute("Dose B", "Route B", productIdB, containerB)); + int rowIdB = savedB.getRowId(); + + // an editor in container A submits an update carrying container B's RowId + DoseAndRoute foreign = new DoseAndRoute("Rejected", "Rejected", productIdA, containerA); + foreign.setRowId(rowIdB); + try + { + _manager.saveStudyProductDoseAndRoute(containerA, user, foreign); + fail("Expected NotFoundException updating a dose/route row from another container"); + } + catch (NotFoundException expected) + { + // expected + } + + // container B's row must be untouched (neither overwritten nor repointed into container A) + assertNotNull("Cross-container update must not modify container B's dose/route", + _manager.getDoseAndRoute(containerB, "Dose B", "Route B", productIdB)); + assertNull("Cross-container update must not have repointed the row into container A", + _manager.getDoseAndRoute(containerA, "Rejected", "Rejected", productIdA)); + + // positive control: updating the row from within its own container succeeds + DoseAndRoute updateB = new DoseAndRoute("Dose B2", "Route B2", productIdB, containerB); + updateB.setRowId(rowIdB); + _manager.saveStudyProductDoseAndRoute(containerB, user, updateB); + assertNotNull("In-container update should succeed", + _manager.getDoseAndRoute(containerB, "Dose B2", "Route B2", productIdB)); + } + finally + { + if (null != containerB) + ContainerManager.delete(containerB, user); + if (null != containerA) + ContainerManager.delete(containerA, user); + } + } + + private Container createStudyContainer(TestContext context, String name) + { + Container junit = JunitUtil.getTestContainer(); + Container c = ContainerManager.createContainer(junit, name, context.getUser()); + Set modules = new HashSet<>(c.getActiveModules()); + modules.add(ModuleLoader.getInstance().getModule("studydesign")); + c.setActiveModules(modules); + StudyService.get().createStudy(c, context.getUser(), "Junit Study " + name, TimepointType.VISIT, true); + return c; + } + + private int insertStudyProduct(Container c, User user, String label, String role) + { + UserSchema schema = QueryService.get().getUserSchema(user, c, StudyDesignQuerySchema.STUDY_SCHEMA_NAME); + TableInfo ti = ((FilteredTable) schema.getTable(StudyDesignQuerySchema.PRODUCT_TABLE_NAME)).getRealTable(); + return Table.insert(user, ti, new ProductImpl(c, label, role)).getRowId(); + } } @TestWhen(TestWhen.When.BVT) From 5815b3537a86b013ea9e19ae556a55ec6581845e Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 16 Jun 2026 14:15:46 -0700 Subject: [PATCH 04/13] Modernize specimen, report, and messages rendering (#7746) --- .../AnnouncementsController.java | 19 +- .../announcements/announcementThread.jsp | 3 +- .../model/AnnouncementFullModel.java | 23 ++ .../model/AnnouncementManager.java | 15 +- .../model/AnnouncementModel.java | 17 -- .../org/labkey/api/action/ApiJsonWriter.java | 4 +- .../labkey/api/action/ApiResponseWriter.java | 1 - .../api/defaults/DefaultValuesAction.java | 12 ++ .../api/defaults/SetDefaultValuesAction.java | 46 ---- .../api/qc/AbstractManageQCStatesAction.java | 1 - .../org/labkey/api/qc/DataStateManager.java | 7 +- .../AbstractContainerScopingTest.java | 3 +- core/src/org/labkey/core/CoreController.java | 201 ++++++++++++++---- core/src/org/labkey/core/CoreModule.java | 1 + .../org/labkey/core/webdav/DavController.java | 23 +- .../controllers/exp/ExperimentController.java | 15 +- .../filecontent/FileContentController.java | 51 +++++ .../mothership/MothershipController.java | 42 ++-- .../labkey/mothership/MothershipManager.java | 7 + .../mothership/query/MothershipSchema.java | 28 ++- .../specimen/actions/SpecimenController.java | 12 +- .../specimen/actions/SpecimenHeaderBean.java | 26 +-- .../specimen/view/manageRequirement.jsp | 2 +- .../labkey/specimen/view/specimenHeader.jsp | 62 +++--- study/src/org/labkey/study/StudyModule.java | 3 + .../reports/ReportsController.java | 147 +++++++++++-- .../labkey/study/dataset/DataStatesTest.java | 56 +++++ 27 files changed, 582 insertions(+), 245 deletions(-) create mode 100644 announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java create mode 100644 study/src/org/labkey/study/dataset/DataStatesTest.java diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index a52c4c4278e..d14ba2e4332 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -26,6 +26,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONObject; import org.labkey.announcements.model.AnnouncementDigestProvider; +import org.labkey.announcements.model.AnnouncementFullModel; import org.labkey.announcements.model.AnnouncementManager; import org.labkey.announcements.model.AnnouncementModel; import org.labkey.announcements.model.DailyDigestEmailPrefsSelector; @@ -107,11 +108,11 @@ import org.labkey.api.util.DateUtil; import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; +import org.labkey.api.util.OptionBuilder; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; -import org.labkey.api.util.URLHelper; -import org.labkey.api.util.OptionBuilder; import org.labkey.api.util.SelectBuilder; +import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.AjaxCompletion; import org.labkey.api.view.AlwaysAvailableWebPartFactory; @@ -137,7 +138,6 @@ import org.springframework.web.servlet.ModelAndView; import java.io.IOException; -import java.io.PrintWriter; import java.util.ArrayList; import java.util.Arrays; import java.util.Calendar; @@ -218,7 +218,6 @@ public static ActionURL getBeginURL(Container c) public static AnnouncementModel copyEditableProps(AnnouncementModel target, AnnouncementModel source, boolean isInsert) { - if (source.getApproved() != null) target.setApproved(source.getApproved()); if (source.getAssignedTo() != null) target.setAssignedTo(source.getAssignedTo()); if (source.getBody() != null) target.setBody(source.getBody()); if (source.getExpires() != null) target.setExpires(source.getExpires()); @@ -940,7 +939,7 @@ public BindException bindParameters(PropertyValues m) throws Exception public ModelAndView getInsertUpdateView(AnnouncementForm form, boolean reshow, BindException errors) { Permissions perm = getPermissions(); - AnnouncementModel parent = null; + AnnouncementFullModel parent = null; Container c = getContainer(); if (null != form.getParentId()) @@ -2308,7 +2307,7 @@ protected DataRegion getDataRegion(Permissions perm, Settings settings) public static class ThreadViewBean { - public AnnouncementModel announcementModel; + public AnnouncementFullModel announcementModel; public String message = ""; public Permissions perm = null; public boolean isResponse = false; @@ -2336,7 +2335,7 @@ public ThreadView(Container c, URLHelper currentURL, User user, String rowId, St init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false); } - public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm) + public ThreadView(Container c, ActionURL url, AnnouncementFullModel ann, Permissions perm) { this(); init(c, ann, url, perm, true, false); @@ -2345,11 +2344,11 @@ public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print) { this(); - AnnouncementModel ann = findThread(c, form.get("rowId"), form.get("entityId")); + AnnouncementFullModel ann = findThread(c, form.get("rowId"), form.get("entityId")); init(c, ann, url, perm, false, print); } - protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) + protected void init(Container c, AnnouncementFullModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) { if (null == c || !perm.allowRead(ann)) { @@ -2454,7 +2453,7 @@ public AnnouncementModel getAnnouncement() } - private static @Nullable AnnouncementModel findThread(Container c, String rowIdVal, String entityId) + private static @Nullable AnnouncementFullModel findThread(Container c, String rowIdVal, String entityId) { int rowId = 0; if (rowIdVal != null) diff --git a/announcements/src/org/labkey/announcements/announcementThread.jsp b/announcements/src/org/labkey/announcements/announcementThread.jsp index 78ddd926c94..de02d2b148f 100644 --- a/announcements/src/org/labkey/announcements/announcementThread.jsp +++ b/announcements/src/org/labkey/announcements/announcementThread.jsp @@ -20,6 +20,7 @@ <%@ page import="org.labkey.announcements.AnnouncementsController.RespondAction" %> <%@ page import="org.labkey.announcements.AnnouncementsController.ThreadView" %> <%@ page import="org.labkey.announcements.AnnouncementsController.ThreadViewBean" %> +<%@ page import="org.labkey.announcements.model.AnnouncementFullModel" %> <%@ page import="org.labkey.announcements.model.AnnouncementManager" %> <%@ page import="org.labkey.announcements.model.AnnouncementModel" %> <%@ page import="org.labkey.announcements.model.DiscussionServiceImpl" %> @@ -40,7 +41,7 @@ Container c = getContainer(); User user = getUser(); ThreadViewBean bean = me.getModelBean(); - AnnouncementModel announcementModel = bean.announcementModel; + AnnouncementFullModel announcementModel = bean.announcementModel; DiscussionService.Settings settings = bean.settings; if (null == announcementModel) diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java b/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java new file mode 100644 index 00000000000..fb6c5e07ccd --- /dev/null +++ b/announcements/src/org/labkey/announcements/model/AnnouncementFullModel.java @@ -0,0 +1,23 @@ +package org.labkey.announcements.model; + +import java.util.Date; + +public class AnnouncementFullModel extends AnnouncementModel +{ + private Date _approved = null; + + public Date getApproved() + { + return _approved; + } + + public void setApproved(Date approved) + { + _approved = approved; + } + + public boolean isSpam() + { + return AnnouncementManager.SPAM_MAGIC_DATE.equals(getApproved()); + } +} diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementManager.java b/announcements/src/org/labkey/announcements/model/AnnouncementManager.java index 7c50d985c2a..b1053019ba2 100644 --- a/announcements/src/org/labkey/announcements/model/AnnouncementManager.java +++ b/announcements/src/org/labkey/announcements/model/AnnouncementManager.java @@ -124,20 +124,20 @@ private AnnouncementManager() { } - private static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, @NotNull SimpleFilter filter) + private static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, @NotNull SimpleFilter filter) { if (c != null) filter.addCondition(FieldKey.fromParts("Container"), c); - return new TableSelector(_comm.getTableInfoAnnouncements(), filter, null).getObject(AnnouncementModel.class); + return new TableSelector(_comm.getTableInfoAnnouncements(), filter, null).getObject(AnnouncementFullModel.class); } - public static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, int rowId) + public static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, int rowId) { return getAnnouncement(c, new SimpleFilter(FieldKey.fromParts("RowId"), rowId)); } - public static @Nullable AnnouncementModel getAnnouncement(@Nullable Container c, String entityId) + public static @Nullable AnnouncementFullModel getAnnouncement(@Nullable Container c, String entityId) { try { @@ -531,7 +531,7 @@ private static AnnouncementModel validateModelWithSideEffects(AnnouncementModel } // Magic date value used to mark an announcement that a moderator has reviewed and marked as spam - private static final Date SPAM_MAGIC_DATE = new Date(0); + static final Date SPAM_MAGIC_DATE = new Date(0); // Standard filters for retrieving specific classes of messages (approved, spam, needs review) public static final SimpleFilter IS_APPROVED_FILTER = new SimpleFilter(FieldKey.fromParts("Approved"), AnnouncementManager.SPAM_MAGIC_DATE, CompareType.GT); @@ -543,11 +543,6 @@ public static void markAsSpam(Container c, AnnouncementModel ann) updateApproved(c, ann, SPAM_MAGIC_DATE); } - public static boolean isSpam(AnnouncementModel ann) - { - return SPAM_MAGIC_DATE.equals(ann.getApproved()); - } - // Execute direct SQL (not Table.update())... I don't think we want to change Modified or ModifiedBy. Could consider adding column for Moderator, though. // Returns true if an update was made, false if not (e.g., message was already reviewed). private static boolean updateApproved(Container c, AnnouncementModel ann, Date date) diff --git a/announcements/src/org/labkey/announcements/model/AnnouncementModel.java b/announcements/src/org/labkey/announcements/model/AnnouncementModel.java index 3a1e2ad5960..46bcc85abdb 100644 --- a/announcements/src/org/labkey/announcements/model/AnnouncementModel.java +++ b/announcements/src/org/labkey/announcements/model/AnnouncementModel.java @@ -81,7 +81,6 @@ public class AnnouncementModel extends Entity implements Serializable private Collection _responses = null; private Set _authors; - private Date _approved = null; /** * Standard constructor. @@ -429,21 +428,5 @@ public AttachmentParent getAttachmentParent() { return new AnnouncementAttachmentParent(this); } - - public Date getApproved() - { - return _approved; - } - - public void setApproved(Date approved) - { - _approved = approved; - } - - @JsonIgnore - public boolean isSpam() - { - return AnnouncementManager.isSpam(this); - } } diff --git a/api/src/org/labkey/api/action/ApiJsonWriter.java b/api/src/org/labkey/api/action/ApiJsonWriter.java index f98a4e23ade..6ce20dbc676 100644 --- a/api/src/org/labkey/api/action/ApiJsonWriter.java +++ b/api/src/org/labkey/api/action/ApiJsonWriter.java @@ -475,7 +475,9 @@ public void testExceptionNotCommitted() throws IOException var responseText = ((MockHttpServletResponse)writer.getResponse()).getContentAsString(); var json = new JSONObject(responseText); assertEquals("throwing up", json.getString("exception")); - assertTrue(json.has("stackTrace")); + assertFalse(json.getBoolean("success")); + assertEquals("java.lang.IllegalStateException", json.get("exceptionClass")); + assertFalse(json.has("stackTrace")); assertFalse(json.has("schemaName")); } diff --git a/api/src/org/labkey/api/action/ApiResponseWriter.java b/api/src/org/labkey/api/action/ApiResponseWriter.java index 3d202d3b298..f85716c02c2 100644 --- a/api/src/org/labkey/api/action/ApiResponseWriter.java +++ b/api/src/org/labkey/api/action/ApiResponseWriter.java @@ -446,7 +446,6 @@ public JSONObject toJSON(Throwable e) JSONObject json = new JSONObject(); json.put("exception", e.getMessage() != null ? e.getMessage() : e.getClass().getName()); json.put("exceptionClass", e.getClass().getName()); - json.put("stackTrace", e.getStackTrace()); return json; } diff --git a/api/src/org/labkey/api/defaults/DefaultValuesAction.java b/api/src/org/labkey/api/defaults/DefaultValuesAction.java index 0d1c34c01ad..46f13b18b85 100644 --- a/api/src/org/labkey/api/defaults/DefaultValuesAction.java +++ b/api/src/org/labkey/api/defaults/DefaultValuesAction.java @@ -18,8 +18,10 @@ import org.labkey.api.action.FormViewAction; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.PropertyService; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.UnauthorizedException; import org.springframework.validation.Errors; public abstract class DefaultValuesAction extends FormViewAction @@ -41,6 +43,16 @@ protected Domain getDomain(FormType domainIdForm) { throw new NotFoundException(); } + // Multi-container domains allow overriding default values outside the domain's container. The subclasses of + // this action ensure that the user has admin (or list/assay designer) permission in the current container + // (where writes will occur). We also want to ensure the user has read in the domain's container to prevent + // arbitrary reads of domain information. We're not checking that current container is in scope for this domain + // because there's no definitive way to do that. An admin creating default values outside a domain's scope is + // fairly harmless. + if (!domain.getContainer().hasPermission(getUser(), ReadPermission.class)) + { + throw new UnauthorizedException(); + } return domain; } diff --git a/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java b/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java index c6d6adbdc0c..79797906462 100644 --- a/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java +++ b/api/src/org/labkey/api/defaults/SetDefaultValuesAction.java @@ -180,52 +180,6 @@ public void render(RenderContext ctx, HtmlWriter out) return app; } ).appendTo(out); -// oldWriter.write(""); - - -// renderer.renderDetailsCaptionCell(ctx, out, "control-label lk-form-row-label"); -// -// if (isFile) -// TD().appendTo(out); // No input for file -// else -// renderer.renderInputCell(ctx, out); -// -// TD( -// (Renderable) ret -> { -// if (isFile) -// { -// out.write("Defaults cannot be set for file fields."); -// } -// else -// { -// DefaultValueType defaultType = ((DefaultableDisplayColumn) renderer).getDefaultValueType(); -// if (defaultType == null) -// defaultType = DefaultValueType.FIXED_EDITABLE; -// out.write(defaultType.getLabel()); -// out.write(PageFlowUtil.popupHelp(HtmlString.of(defaultType.getHelpText()), "Default Value Type: " + defaultType.getLabel())); -// } -// return ret; -// } -// ).appendTo(out); -// oldWriter.write(""); - -// oldWriter.write(""); -// } -// oldWriter.write("
"); -// if (isFile) -// { -// out.write("Defaults cannot be set for file fields."); -// } -// else -// { -// DefaultValueType defaultType = ((DefaultableDisplayColumn) renderer).getDefaultValueType(); -// if (defaultType == null) -// defaultType = DefaultValueType.FIXED_EDITABLE; -// out.write(defaultType.getLabel()); -// out.write(PageFlowUtil.popupHelp(HtmlString.of(defaultType.getHelpText()), "Default Value Type: " + defaultType.getLabel())); -// } -// oldWriter.write("
"); - ButtonBar bbar = getButtonBar(MODE_INSERT); bbar.setStyle(ButtonBar.Style.separateButtons); diff --git a/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java b/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java index 0731902e111..ca68b7d4158 100644 --- a/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java +++ b/api/src/org/labkey/api/qc/AbstractManageQCStatesAction.java @@ -192,5 +192,4 @@ protected HtmlString getQcStateHtml(Container container, DataStateHandler qcStat return qcStateHtml.getHtmlString(); } - } diff --git a/api/src/org/labkey/api/qc/DataStateManager.java b/api/src/org/labkey/api/qc/DataStateManager.java index 9de7b1ae387..d2cc4394cbe 100644 --- a/api/src/org/labkey/api/qc/DataStateManager.java +++ b/api/src/org/labkey/api/qc/DataStateManager.java @@ -15,6 +15,8 @@ */ package org.labkey.api.qc; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; @@ -25,6 +27,7 @@ import org.labkey.api.data.Table; import org.labkey.api.data.TableSelector; import org.labkey.api.security.User; +import org.labkey.api.util.logging.LogHelper; import java.util.ArrayList; import java.util.Collections; @@ -34,6 +37,7 @@ public class DataStateManager { + private static final Logger LOG = LogHelper.getLogger(DataStateManager.class, "Data state persistence issues"); private static final DataStateManager _instance = new DataStateManager(); private static final Map> _DataStateHandlers = new HashMap<>(); private static final Cache DATA_STATE_DB_CACHE = CacheManager.getBlockingCache(CacheManager.UNLIMITED, CacheManager.DAY, "Data states", @@ -124,7 +128,8 @@ public DataState insertState(User user, DataState state) public DataState updateState(User user, DataState state) { DATA_STATE_DB_CACHE.remove(state.getContainer()); - return Table.update(user, CoreSchema.getInstance().getTableInfoDataStates(), state, state.getRowId()); + SimpleFilter filter = SimpleFilter.createContainerFilter(state.getContainer()); + return Table.update(user, CoreSchema.getInstance().getTableInfoDataStates(), state, state.getRowId(), filter, Level.WARN); } public boolean deleteState(DataState state) diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java index bb5db4d34a6..fd0ea652544 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -15,6 +15,7 @@ */ package org.labkey.api.security.permissions; +import jakarta.servlet.http.HttpServletResponse; import org.junit.After; import org.junit.Assert; import org.labkey.api.data.Container; @@ -124,7 +125,7 @@ protected MockHttpServletResponse post(ActionURL url, User user) throws Exceptio } /** Assert that a dispatched response has the expected HTTP status code. */ - protected void assertStatus(int expected, MockHttpServletResponse response) + protected void assertStatus(int expected, HttpServletResponse response) { assertEquals("Unexpected HTTP status", expected, response.getStatus()); } diff --git a/core/src/org/labkey/core/CoreController.java b/core/src/org/labkey/core/CoreController.java index 9f70b100609..10d71253623 100644 --- a/core/src/org/labkey/core/CoreController.java +++ b/core/src/org/labkey/core/CoreController.java @@ -29,6 +29,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Assert; import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -107,10 +108,16 @@ import org.labkey.api.reports.LabKeyScriptEngineManager; import org.labkey.api.security.AdminConsoleAction; import org.labkey.api.security.IgnoresTermsOfUse; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.NewUserStatus; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; @@ -119,6 +126,9 @@ import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.ProjectAdminRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.services.ServiceRegistry; import org.labkey.api.settings.AdminConsole; @@ -135,6 +145,7 @@ import org.labkey.api.util.HtmlString; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.JsonUtil; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.MimeMap; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.PageFlowUtil.Content; @@ -156,6 +167,7 @@ import org.labkey.api.view.RedirectException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.template.ClientDependency; import org.labkey.api.view.template.WarningService; import org.labkey.api.view.template.Warnings; @@ -176,8 +188,10 @@ import org.labkey.core.workbook.MoveWorkbooksBean; import org.labkey.core.workbook.WorkbookFolderType; import org.labkey.folder.xml.FolderDocument; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.mvc.Controller; @@ -205,10 +219,6 @@ import static org.labkey.api.view.template.WarningService.SESSION_WARNINGS_BANNER_KEY; -/** - * User: jeckels - * Date: Jan 4, 2007 - */ public class CoreController extends SpringActionController { private static final Map _customStylesheetCache = new ConcurrentHashMap<>(); @@ -863,36 +873,11 @@ public static class MoveContainerAction extends MutatingApiAction children = ContainerManager.getAllChildren(target, getUser()); // assumes read permission if (children.contains(parent)) { - errors.reject(ERROR_MSG, "The container '" + parentIdentifier + "' is not a valid parent folder."); - return; + errors.reject(ERROR_MSG, "The container '" + json.get("parent") + "' is not a valid parent folder."); + } + } + + private @Nullable Container validateContainer(JSONObject json, String key, String description, Errors errors) + { + String identifier = json.optString(key, null); + + if (null == identifier) + { + errors.reject(ERROR_MSG, description + " container must be specified for move operation."); + return null; + } + + Path path = Path.parse(identifier); + Container c = ContainerManager.getForPath(path); + + if (null == c) + { + c = ContainerManager.getForId(identifier); + } + + // Treat non-existent and bad permissions equivalently to not leak any info + if (null == c || !c.hasPermission(getUser(), AdminPermission.class)) + { + throw new NotFoundException(description + " not found"); } + + return c; } @Override @@ -949,7 +956,7 @@ public ApiResponse execute(SimpleApiJsonForm form, BindException errors) throws // Prepare aliases JSONObject object = form.getJsonObject(); - Boolean addAlias = (Boolean) object.get("addAlias"); + boolean addAlias = object.optBoolean("addAlias"); // optional, false by default List aliasList = new ArrayList<>(ContainerManager.getAliasesForContainer(target)); aliasList.add(target.getPath()); @@ -2914,4 +2921,106 @@ public void setProvider(String provider) } + public static class MoveContainerTestCase extends Assert + { + private static final String FOLDER_NAME = "MoveContainerFolder"; + private static final String NEW_PARENT = "NewParent"; + private static final ValidEmail TEST_EMAIL; + + static + { + try + { + TEST_EMAIL = new ValidEmail("testMove@myDomain.com"); + } + catch (ValidEmail.InvalidEmailException e) + { + throw new RuntimeException(e); + } + } + + @Test + public void testMoveContainer() throws Exception + { + doCleanup(); + + User adminUser = TestContext.get().getUser(); + Container junit = JunitUtil.getTestContainer(); + Container folder = ContainerManager.createContainer(junit, FOLDER_NAME, adminUser); + Container newParent = ContainerManager.createContainer(junit, NEW_PARENT, adminUser); + + NewUserStatus newUserStatus = SecurityManager.addUser(TEST_EMAIL, null); + User nonAdminUser = newUserStatus.getUser(); + // Create and save a new, non-empty policy for the folder so it doesn't inherit permissions + MutableSecurityPolicy policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + + // Admin permissions nowhere... should fail + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in just the folder being moved and try again (should fail) + policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in the new parent as well and try again (should still fail) + MutableSecurityPolicy newParentPolicy = new MutableSecurityPolicy(newParent); + newParentPolicy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(newParentPolicy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_FORBIDDEN); + + // Give nonAdminUser admin permission in the folder's current parent and try again (should now succeed) + policy = new MutableSecurityPolicy(folder.getParent().getPolicy()); + policy.addRoleAssignment(nonAdminUser, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + moveFolder(nonAdminUser, folder, newParent, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + NEW_PARENT + "/" + FOLDER_NAME, folder.getPath()); + // Should be able to move it back + moveFolder(nonAdminUser, folder, junit, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + FOLDER_NAME, folder.getPath()); + + // Happy path -- admin user should be able to move folder to new parent and back + moveFolder(adminUser, folder, newParent, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + NEW_PARENT + "/" + FOLDER_NAME, folder.getPath()); + moveFolder(adminUser, folder, junit, HttpServletResponse.SC_OK); + folder = ContainerManager.getForId(folder.getId()); // Refresh its path + assertNotNull(folder); + assertEquals(junit.getPath() + "/" + FOLDER_NAME, folder.getPath()); + } + + private void moveFolder(User user, Container folder, Container newParent, int expectedResponseCode) throws Exception + { + JSONObject json = new JSONObject().put("container", folder.getId()).put("parent", newParent.getId()); + HttpServletRequest request = ViewServlet.mockRequest(RequestMethod.POST.name(), new ActionURL(MoveContainerAction.class, folder), user, Map.of("Content-Type", "application/json"), json.toString()); + MockHttpServletResponse response = ViewServlet.mockDispatch(request, null); + assertEquals("Unexpected response code", expectedResponseCode, response.getStatus()); + } + + private static void doCleanup() throws Exception + { + Container folder = ContainerManager.getForPath(JunitUtil.getTestContainer().getPath() + "/" + FOLDER_NAME); + if (folder != null) + { + ContainerManager.deleteAll(folder, TestContext.get().getUser()); + } + + Container newParent = ContainerManager.getForPath(JunitUtil.getTestContainer().getPath() + "/" + NEW_PARENT); + if (newParent != null) + { + ContainerManager.deleteAll(newParent, TestContext.get().getUser()); + } + + User u = UserManager.getUser(TEST_EMAIL); + if (u != null) + UserManager.deleteUser(u.getUserId()); + } + } } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index bd342ae5afa..3c862288961 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1398,6 +1398,7 @@ public Set getIntegrationTests() AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, + CoreController.MoveContainerTestCase.class, DataRegion.TestCase.class, DavController.TestCase.class, DavController.MoveActionContainerScopingTestCase.class, diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 4b5590749db..4a71e7b134e 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -3538,17 +3538,24 @@ WebdavStatus doMethod() throws DavException, IOException throw new DavException(WebdavStatus.SC_METHOD_NOT_ALLOWED); } + WebdavResource resource = resolvePath(); + if (null == resource) + { + throw new DavException(WebdavStatus.SC_NOT_FOUND); + } + + if (!resource.canWrite(getUser(), true)) + { + throw new DavException(WebdavStatus.SC_FORBIDDEN); + } + FileTime lastModified = getLastModified(getRequest().getInputStream()); if (lastModified != null) { try { - WebdavResource resource = resolvePath(); - if (resource != null) - { - resource.setLastModified(lastModified.toMillis()); - } + resource.setLastModified(lastModified.toMillis()); } catch (ConversionException ignored) {} } @@ -3558,12 +3565,6 @@ WebdavStatus doMethod() throws DavException, IOException assert track(writer); try { - WebdavResource resource = resolvePath(); - if (resource == null) - { - throw new DavException(WebdavStatus.SC_NOT_FOUND); - } - XMLResourceWriter resourceWriter = new XMLResourceWriter(writer); resourceWriter.beginResponse(getResponse()); diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index cc2ffd6445f..33bfd5a917d 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -2415,7 +2415,8 @@ public static class CheckDataFileAction extends MutatingApiAction public void validateForm(DataFileForm form, Errors errors) { _data = form.lookupData(); - if (_data == null) + // Not using ensureCorrectContainer() because we don't redirect API actions + if (_data == null || !getContainer().equals(_data.getContainer())) { errors.reject(ERROR_MSG, "No ExpData found for id: " + form.getRowId()); } @@ -8387,7 +8388,17 @@ public void testDataClassAttachmentContainerScoping() throws Exception .addParameter("lsid", lsid) .addParameter("name", attachmentName); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + + ActionURL checkDataFileUrl = new ActionURL(CheckDataFileAction.class, folderB) + .addParameter("rowId", data.getRowId()); + assertStatus(HttpServletResponse.SC_OK, post(checkDataFileUrl, admin)); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(checkDataFileUrl, readerA)); // No perms + checkDataFileUrl.setContainer(folderA); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(checkDataFileUrl, readerA)); // Has read in folder A, but not admin + resp = post(checkDataFileUrl, admin); // Wrong container. Not found. + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + JSONObject json = new JSONObject(resp.getContentAsString()); + assertEquals("No ExpData found for id: " + data.getRowId(), json.get("exception")); } } - } diff --git a/filecontent/src/org/labkey/filecontent/FileContentController.java b/filecontent/src/org/labkey/filecontent/FileContentController.java index 10e144ca207..3f01703bb95 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentController.java +++ b/filecontent/src/org/labkey/filecontent/FileContentController.java @@ -16,10 +16,13 @@ package org.labkey.filecontent; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiJsonWriter; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiResponseWriter; @@ -79,19 +82,27 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.query.ValidationError; import org.labkey.api.reader.Readers; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresLogin; import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.RequiresSiteAdmin; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.NewUserStatus; +import org.labkey.api.security.SecurityPolicyManager; import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.util.DOM; import org.labkey.api.util.FileUtil; import org.labkey.api.util.HtmlString; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.LinkBuilder; import org.labkey.api.util.MimeMap; import org.labkey.api.util.NetworkDrive; @@ -108,6 +119,7 @@ import org.labkey.api.view.Portal; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.api.view.ViewServlet; import org.labkey.api.view.WebPartView; import org.labkey.api.view.template.PageConfig; import org.labkey.api.webdav.WebdavResource; @@ -115,10 +127,13 @@ import org.labkey.api.writer.HtmlWriter; import org.labkey.filecontent.message.FileEmailConfig; import org.labkey.filecontent.message.ShortMessageDigest; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.validation.ObjectError; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.servlet.ModelAndView; +import org.springframework.web.servlet.mvc.Controller; import java.io.BufferedReader; import java.io.File; @@ -720,6 +735,9 @@ public Set> getChildren(NodeForm form, BindException errors) if (c == null) c = ContainerManager.getRoot(); + if (!c.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this summary."); + ActionURL browse = new ActionURL(BeginAction.class, c); Set> children = FileContentServiceImpl.getInstance().getNodes(form.isShowOverridesOnly(), browse.getEncodedLocalURIString(), c); @@ -756,6 +774,9 @@ protected Set> getChildren(NodeForm form, BindException erro if (c == null) c = ContainerManager.getRoot(); + if (!c.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this project summary."); + Set> children = new LinkedHashSet<>(); FileContentService svc = FileContentService.get(); @@ -1605,5 +1626,35 @@ public void testActionPermissions() new SendShortDigestAction() ); } + + @Test + public void testSummaryActions() throws Exception + { + Container folder = JunitUtil.getTestContainer(); + User adminUser = TestContext.get().getUser(); + + // Happy path -- admin should be able to invoke summary actions in root + testAction(FileContentSummaryAction.class, folder, adminUser, HttpServletResponse.SC_OK); + testAction(FileContentProjectSummaryAction.class, folder, adminUser, HttpServletResponse.SC_OK); + + NewUserStatus newUserStatus = SecurityManager.addUser(new ValidEmail("testSummaryActions@myDomain.com"), null); + User nonAdminUser = newUserStatus.getUser(); + MutableSecurityPolicy policy = new MutableSecurityPolicy(folder.getPolicy()); + policy.addRoleAssignment(nonAdminUser, ReaderRole.class); + SecurityPolicyManager.savePolicyForTests(policy, adminUser); + + // Non-admin user should be forbidden + testAction(FileContentSummaryAction.class, folder, nonAdminUser, HttpServletResponse.SC_FORBIDDEN); + testAction(FileContentProjectSummaryAction.class, folder, nonAdminUser, HttpServletResponse.SC_FORBIDDEN); + + UserManager.deleteUser(nonAdminUser.getUserId()); + } + + private void testAction(Class actionClass, Container folder, User user, int expectedResponseCode) throws Exception + { + HttpServletRequest request = ViewServlet.mockRequest(RequestMethod.POST.name(), new ActionURL(actionClass, folder), user, Map.of("Content-Type", "application/json"), null); + MockHttpServletResponse response = ViewServlet.mockDispatch(request, null); + assertEquals("Unexpected response code", expectedResponseCode, response.getStatus()); + } } } diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index 398dec07b60..d5b8579530c 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -363,12 +363,21 @@ public static class ShowServerSessionDetailAction extends SimpleViewAction { - public ServerInstallationForm(ServerInstallation installation) - { - this(); - setBean(installation); - } - public ServerInstallationForm() { super(ServerInstallation.class, MothershipManager.get().getTableInfoServerInstallation()); } } - public static class ServerSessionForm extends BeanViewForm + public static class ServerSessionForm { - public ServerSessionForm(ServerSession session) + private Integer _serverSessionId = null; + + public Integer getServerSessionId() { - this(); - setBean(session); + return _serverSessionId; } - public ServerSessionForm() + public void setServerSessionId(Integer serverSessionId) { - super(ServerSession.class, MothershipManager.get().getTableInfoServerSession()); + _serverSessionId = serverSessionId; } } diff --git a/mothership/src/org/labkey/mothership/MothershipManager.java b/mothership/src/org/labkey/mothership/MothershipManager.java index f5a9c22f104..26eede9da92 100644 --- a/mothership/src/org/labkey/mothership/MothershipManager.java +++ b/mothership/src/org/labkey/mothership/MothershipManager.java @@ -226,6 +226,13 @@ public ServerSession getServerSession(String serverSessionGUID, Container c) return new TableSelector(getTableInfoServerSession(), filter, null).getObject(ServerSession.class); } + public ServerSession getServerSession(int serverSessionId, Container c) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c); + filter.addCondition(FieldKey.fromString("ServerSessionId"), serverSessionId); + return new TableSelector(getTableInfoServerSession(), filter, null).getObject(ServerSession.class); + } + public ExceptionStackTrace getExceptionStackTrace(String stackTraceHash, String containerId) { SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Container"), containerId); diff --git a/mothership/src/org/labkey/mothership/query/MothershipSchema.java b/mothership/src/org/labkey/mothership/query/MothershipSchema.java index 39aead99dbf..1845a0e7731 100644 --- a/mothership/src/org/labkey/mothership/query/MothershipSchema.java +++ b/mothership/src/org/labkey/mothership/query/MothershipSchema.java @@ -27,6 +27,7 @@ import org.labkey.api.data.DataColumn; import org.labkey.api.data.ForeignKey; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.RenderContext; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.TableInfo; import org.labkey.api.data.dialect.SqlDialect; @@ -405,18 +406,25 @@ public FilteredTable createExceptionReportTable(ContainerFilte FilteredTable result = new FilteredTable<>(MothershipManager.get().getTableInfoExceptionReport(), this, cf); result.setDetailsURL(AbstractTableInfo.LINK_DISABLER); result.wrapAllColumns(true); + // Reports are submitted by anonymous users, so untrusted. Don't render these two URLs as links. result.getMutableColumnOrThrow("URL").setDisplayColumnFactory(colInfo -> - { - DataColumn result1 = new DataColumn(colInfo); - result1.setURLExpression(StringExpressionFactory.create("${URL}", false)); - return result1; - }); + new DataColumn(colInfo) + { + @Override + protected String renderURLorValueURL(RenderContext ctx) + { + return null; + } + }); result.getMutableColumnOrThrow("ReferrerURL").setDisplayColumnFactory(colInfo -> - { - DataColumn result12 = new DataColumn(colInfo); - result12.setURLExpression(StringExpressionFactory.create("${ReferrerURL}", false)); - return result12; - }); + new DataColumn(colInfo) + { + @Override + protected String renderURLorValueURL(RenderContext ctx) + { + return null; + } + }); // Container column is on another table so join to it to filter appropriately SQLFragment containerSQL = new SQLFragment("ExceptionStackTraceId IN (SELECT ExceptionStackTraceId FROM "); diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index f3d79169d7e..9d48a30e7c7 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -5546,6 +5546,8 @@ public void addNavTrail(NavTree root) } } + public record PtidVisit(String ptid, String visit){} + @RequiresPermission(ReadPermission.class) public class SelectedSpecimensAction extends QueryViewAction { @@ -5561,26 +5563,26 @@ public SelectedSpecimensAction() protected ModelAndView getHtmlView(SpecimenViewTypeForm form, BindException errors) throws Exception { Study study = getStudyRedirectIfNull(); - Set> ptidVisits = new HashSet<>(); + Set ptidVisits = new HashSet<>(); for (ParticipantDataset pd : getFilterPds()) { if (pd.getSequenceNum() == null) { - ptidVisits.add(new Pair<>(pd.getParticipantId(), null)); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), null)); } else if (study.getTimepointType() != TimepointType.VISIT && pd.getVisitDate() != null) { - ptidVisits.add(new Pair<>(pd.getParticipantId(), DateUtil.formatDate(pd.getContainer(), pd.getVisitDate()))); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), DateUtil.formatDate(pd.getContainer(), pd.getVisitDate()))); } else { Visit visit = pd.getSequenceNum() != null ? StudyInternalService.get().getVisitForSequence(study, pd.getSequenceNum()) : null; - ptidVisits.add(new Pair<>(pd.getParticipantId(), visit != null ? visit.getLabel() : "" + StudyInternalService.get().formatSequenceNum(pd.getSequenceNum()))); + ptidVisits.add(new PtidVisit(pd.getParticipantId(), visit != null ? visit.getLabel() : StudyInternalService.get().formatSequenceNum(pd.getSequenceNum()))); } } SpecimenQueryView view = createInitializedQueryView(form, errors, form.getExportType() != null, null); JspView header = new JspView<>("/org/labkey/specimen/view/specimenHeader.jsp", - new SpecimenHeaderBean(getViewContext(), view, ptidVisits)); + new SpecimenHeaderBean(getViewContext(), view, ptidVisits)); return new VBox(header, view); } diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java b/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java index fe32d12dc24..fd0cbd575df 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenHeaderBean.java @@ -5,13 +5,13 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.specimen.SpecimenQuerySchema; -import org.labkey.specimen.query.SpecimenQueryView; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; -import org.labkey.api.util.Pair; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.ViewContext; +import org.labkey.specimen.actions.SpecimenController.PtidVisit; +import org.labkey.specimen.query.SpecimenQueryView; import java.util.Collections; import java.util.Iterator; @@ -24,7 +24,7 @@ public final class SpecimenHeaderBean private final ActionURL _otherViewURL; private final ViewContext _viewContext; private final boolean _showingVials; - private final Set> _filteredPtidVisits; + private final Set _ptidVisits; private Integer _selectedRequest; @@ -33,7 +33,7 @@ public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view) this(context, view, Collections.emptySet()); } - public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set> filteredPtidVisits) throws RuntimeException + public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set ptidVisits) throws RuntimeException { Map params = context.getRequest().getParameterMap(); @@ -90,11 +90,11 @@ public SpecimenHeaderBean(ViewContext context, SpecimenQueryView view, Set> getFilteredPtidVisits() + public Set getPtidVisits() { - return _filteredPtidVisits; + return _ptidVisits; } public boolean isSingleVisitFilter() { - if (getFilteredPtidVisits().isEmpty()) + if (getPtidVisits().isEmpty()) return false; - Iterator> visitIt = getFilteredPtidVisits().iterator(); - String firstVisit = visitIt.next().getValue(); - while (visitIt.hasNext()) + Iterator ptidVisit = getPtidVisits().iterator(); + String firstVisit = ptidVisit.next().visit(); + while (ptidVisit.hasNext()) { - if (!Objects.equals(firstVisit, visitIt.next().getValue())) + if (!Objects.equals(firstVisit, ptidVisit.next().visit())) return false; } return true; diff --git a/specimen/src/org/labkey/specimen/view/manageRequirement.jsp b/specimen/src/org/labkey/specimen/view/manageRequirement.jsp index 68109262af7..6721bbc61de 100644 --- a/specimen/src/org/labkey/specimen/view/manageRequirement.jsp +++ b/specimen/src/org/labkey/specimen/view/manageRequirement.jsp @@ -84,7 +84,7 @@ Description - <%= unsafe(requirement.getDescription()) %> + <%= h(requirement.getDescription()) %> <% if (!bean.isRequestManager()) diff --git a/specimen/src/org/labkey/specimen/view/specimenHeader.jsp b/specimen/src/org/labkey/specimen/view/specimenHeader.jsp index b0599197eab..b1e2c7b5089 100644 --- a/specimen/src/org/labkey/specimen/view/specimenHeader.jsp +++ b/specimen/src/org/labkey/specimen/view/specimenHeader.jsp @@ -15,16 +15,18 @@ * limitations under the License. */ %> -<%@ page import="org.labkey.api.security.permissions.AdminPermission"%> -<%@ page import="org.labkey.api.study.StudyService"%> -<%@ page import="org.labkey.api.study.StudyUrls"%> -<%@ page import="org.labkey.api.util.Pair"%> +<%@ page import="org.apache.commons.lang3.StringUtils" %> +<%@ page import="org.labkey.api.security.permissions.AdminPermission" %> +<%@ page import="org.labkey.api.study.StudyService" %> +<%@ page import="org.labkey.api.study.StudyUrls" %> +<%@ page import="org.labkey.api.util.HtmlStringBuilder" %> <%@ page import="org.labkey.api.view.ActionURL" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.api.view.JspView" %> <%@ page import="org.labkey.api.view.template.ClientDependencies" %> <%@ page import="org.labkey.specimen.actions.ShowSearchAction" %> <%@ page import="org.labkey.specimen.actions.SpecimenController" %> +<%@ page import="org.labkey.specimen.actions.SpecimenController.PtidVisit" %> <%@ page import="org.labkey.specimen.actions.SpecimenController.SpecimensAction" %> <%@ page import="org.labkey.specimen.actions.SpecimenHeaderBean" %> <%@ page import="java.util.Iterator" %> @@ -74,47 +76,57 @@ <%=link("Search", ShowSearchAction.getShowSearchURL(getContainer(), bean.isShowingVials()))%>  <%=link("Reports", urlFor(SpecimenController.AutoReportListAction.class)) %> <% - if (!bean.getFilteredPtidVisits().isEmpty()) + if (!bean.getPtidVisits().isEmpty()) { // get the first visit label: - StringBuilder filterString = new StringBuilder(); - filterString.append("This view is displaying specimens only from "); - boolean usePlural = bean.getFilteredPtidVisits().size() != 1; + HtmlStringBuilder builder = HtmlStringBuilder.of() + .unsafeAppend("") + .append("This view is displaying specimens only from "); + boolean usePlural = bean.getPtidVisits().size() != 1; if (bean.isSingleVisitFilter()) { - filterString.append(h((usePlural?subjectNounPlural:subjectNounSingle).toLowerCase())).append(" "); - for (Iterator> it = bean.getFilteredPtidVisits().iterator(); it.hasNext();) + builder.append((usePlural ? subjectNounPlural : subjectNounSingle).toLowerCase()) + .append(" "); + for (Iterator it = bean.getPtidVisits().iterator(); it.hasNext();) { - String ptid = it.next().getKey(); - filterString.append(ptid); + String ptid = it.next().ptid(); + builder.append(ptid); if (it.hasNext()) - filterString.append(", "); + builder.append(", "); } - String visit = bean.getFilteredPtidVisits().iterator().next().getValue(); + String visit = bean.getPtidVisits().iterator().next().visit(); if (visit != null) - filterString.append(" at visit ").append(visit); - filterString.append(".
"); + builder.append(" at visit ").append(visit); + + builder.append(".") + .unsafeAppend("
"); } else { - filterString.append(" the following ").append(h(subjectNounSingle.toLowerCase())).append("/visit ").append(usePlural?"pairs":"pair").append(":
"); - for (Iterator> it = bean.getFilteredPtidVisits().iterator(); it.hasNext();) + builder.append(" the following ") + .append(subjectNounSingle.toLowerCase()) + .append("/visit ").append(usePlural ? "pairs" : "pair") + .append(":") + .unsafeAppend("
"); + for (Iterator it = bean.getPtidVisits().iterator(); it.hasNext();) { - Pair ptidVisit = it.next(); - filterString.append(ptidVisit.getKey()).append("/").append(ptidVisit.getValue()); + PtidVisit ptidVisit = it.next(); + builder.append(ptidVisit.ptid()) + .append("/") + .append(StringUtils.trimToEmpty(ptidVisit.visit())); if (it.hasNext()) - filterString.append(", "); + builder.append(", "); } - filterString.append("."); + builder.append("."); } - ActionURL noFitlerUrl = getViewContext().cloneActionURL().setAction(SpecimensAction.class); + ActionURL noFilterUrl = getViewContext().cloneActionURL().setAction(SpecimensAction.class); %>

- +
<%= unsafe(filterString.toString()) %>
<%=builder%>

-<%= link("Remove " + subjectNounSingle + "/Visit Filter", noFitlerUrl)%><% +<%= link("Remove " + subjectNounSingle + "/Visit Filter", noFilterUrl)%><% } %>
diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 60bb3b9d968..322655a8487 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -134,6 +134,7 @@ import org.labkey.study.controllers.publish.PublishController; import org.labkey.study.controllers.reports.ReportsController; import org.labkey.study.controllers.security.SecurityController; +import org.labkey.study.dataset.DataStatesTest; import org.labkey.study.dataset.DatasetAuditProvider; import org.labkey.study.dataset.DatasetNotificationInfoProvider; import org.labkey.study.dataset.DatasetSnapshotProvider; @@ -710,6 +711,7 @@ public Set getIntegrationTests() { return Set.of( DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, + DataStatesTest.class, ParticipantGroupManager.ParticipantGroupTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, @@ -737,6 +739,7 @@ public Set getUnitTests() DatasetDataWriter.TestCase.class, DefaultStudyDesignWriter.TestCase.class, ParticipantIdImportHelper.ParticipantIdTest.class, + ReportsController.TestCase.class, SequenceNumImportHelper.SequenceNumTest.class, StudyImpl.DateMathTestCase.class ); diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index e09c6265b59..f2b1b06dae4 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -22,6 +22,7 @@ import org.json.JSONArray; import org.json.JSONException; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.Action; import org.labkey.api.action.ActionType; import org.labkey.api.action.ApiResponse; @@ -49,9 +50,9 @@ import org.labkey.api.reports.report.view.ReportUtil; import org.labkey.api.reports.report.view.ScriptReportBean; import org.labkey.api.security.RequiresLogin; -import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; @@ -63,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.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.UniqueID; import org.labkey.api.view.ActionURL; @@ -533,7 +535,7 @@ public void addNavTrail(NavTree root) } } - @RequiresNoPermission + @RequiresPermission(ReadPermission.class) public class CreateCrosstabReportAction extends SimpleViewAction { @Override @@ -661,13 +663,22 @@ public int getReportId() return reportId; } + @SuppressWarnings("unused") public void setReportId(int reportId) { this.reportId = reportId; } - public void setReportView(String label){_reportView = label;} - public String getReportView(){return _reportView;} + public String getReportView() + { + return _reportView; + } + + @SuppressWarnings("unused") + public void setReportView(String label) + { + _reportView = label; + } } public static class SaveReportForm extends ViewForm @@ -765,13 +776,35 @@ public void setShowWithDataset(int showWithDataset) this.showWithDataset = showWithDataset; } - public void setRedirectToDataset(Integer dataset){redirectToDataset = dataset;} - public Integer getRedirectToDataset(){return redirectToDataset;} + public Integer getRedirectToDataset() + { + return redirectToDataset; + } + + public void setRedirectToDataset(Integer dataset) + { + redirectToDataset = dataset; + } + + public String getDescription() + { + return this.description; + } + + public void setDescription(String description) + { + this.description = description; + } + + public BindException getErrors() + { + return _errors; + } - public void setDescription(String description){this.description = description;} - public String getDescription(){return this.description;} - public void setErrors(BindException errors){_errors = errors;} - public BindException getErrors(){return _errors;} + public void setErrors(BindException errors) + { + _errors = errors; + } public String getDataRegionName() { @@ -829,19 +862,57 @@ public Report getReport(ContainerUser cu) return null; } - public void setShareReport(boolean shareReport){_shareReport = shareReport;} - public boolean getShareReport(){return _shareReport;} + public void setShareReport(boolean shareReport) + { + _shareReport = shareReport; + } + + public boolean getShareReport() + { + return _shareReport; + } + + public void setSchemaName(String schemaName) + { + _schemaName = schemaName; + } + + public String getSchemaName() + { + return _schemaName; + } + + public void setQueryName(String queryName) + { + _queryName = queryName; + } + + public String getQueryName() + { + return _queryName; + } + + public void setViewName(String viewName) + { + _viewName = viewName; + } + + public String getViewName() + { + return _viewName; + } - public void setSchemaName(String schemaName){_schemaName = schemaName;} - public String getSchemaName(){return _schemaName;} - public void setQueryName(String queryName){_queryName = queryName;} - public String getQueryName(){return _queryName;} - public void setViewName(String viewName){_viewName = viewName;} - public String getViewName(){return _viewName;} @Override - public void setDataRegionName(String dataRegionName){_dataRegionName = dataRegionName;} + public void setDataRegionName(String dataRegionName) + { + _dataRegionName = dataRegionName; + } + @Override - public String getDataRegionName(){return _dataRegionName;} + public String getDataRegionName() + { + return _dataRegionName; + } public String getRedirectUrl() { @@ -969,7 +1040,9 @@ private void _addNavTrail(NavTree root, String name) if (getContainer().hasPermission(getUser(), AdminPermission.class)) root.addChild("Manage Views", urlProvider(ReportUrls.class).urlManageViews(getContainer())); } - catch (Exception ignored) {} + catch (Exception ignored) + { + } root.addChild(name); } @@ -1217,4 +1290,36 @@ public void bindJson(JSONObject json) } } } + + public static class TestCase extends AbstractActionPermissionTest + { + @Override + @Test + public void testActionPermissions() + { + User user = TestContext.get().getUser(); + assertTrue(user.hasSiteAdminPermission()); + + ReportsController controller = new ReportsController(); + + // @RequiresPermission(ReadPermission.class) + assertForReadPermission(user, false, + controller.new BeginAction(), + new StreamFileAction(), + controller.new SaveReportAction(), + controller.new SaveReportViewAction(), + new ShowReportAction(), + new ParticipantCrosstabAction(), + controller.new CreateCrosstabReportAction(), + controller.new RunRReportAction(), + controller.new ParticipantReportAction(), + new SaveParticipantReportAction() + ); + + // @RequiresPermission(AdminPermission.class) + assertForAdminPermission(user, + controller.new CreateQueryReportAction() + ); + } + } } diff --git a/study/src/org/labkey/study/dataset/DataStatesTest.java b/study/src/org/labkey/study/dataset/DataStatesTest.java new file mode 100644 index 00000000000..7d231cf4bf0 --- /dev/null +++ b/study/src/org/labkey/study/dataset/DataStatesTest.java @@ -0,0 +1,56 @@ +package org.labkey.study.dataset; + +import jakarta.servlet.http.HttpServletResponse; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.qc.DataState; +import org.labkey.api.qc.DataStateManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.view.ActionURL; +import org.labkey.study.controllers.StudyController; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.util.List; + +public class DataStatesTest extends AbstractContainerScopingTest +{ + @Test + public void testManageDataStates() throws Exception + { + // Note: This test will log OptimisticConflictException warnings + // We're exercising DataStateManger via study ManageQCStatesAction, but we don't need to create actual studies + Container folderA = createContainer("Folder A"); + Container folderB = createContainer("Folder B"); + + // Create a basic data state in Folder A + DataState state1 = new DataState(); + state1.setContainer(folderA); + state1.setLabel("State 1"); + state1.setPublicData(true); + state1 = DataStateManager.getInstance().insertState(getAdmin(), state1); + List statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + assertTrue(statesInA.contains(state1)); + + // Attempt to update that data state from the wrong folder, Folder B. Admin should *not* be able to update it. + ActionURL url = new ActionURL(StudyController.ManageQCStatesAction.class, folderB) + .addParameter("ids", state1.getRowId()) + .addParameter("labels", "Here's my new label"); + HttpServletResponse response = post(url, getAdmin()); + List statesInB = DataStateManager.getInstance().getStates(folderB); + assertTrue(statesInB.isEmpty()); + statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + assertTrue(statesInA.contains(state1)); + assertStatus(MockHttpServletResponse.SC_INTERNAL_SERVER_ERROR, response); // Error response + + // Admin should be able to update the data state in Folder A + url.setContainer(folderA); + post(url, getAdmin()); + statesInA = DataStateManager.getInstance().getStates(folderA); + assertEquals(1, statesInA.size()); + DataState updatedState = statesInA.get(0); + assertEquals("Here's my new label", updatedState.getLabel()); + assertFalse(updatedState.isPublicData()); + } +} From 1d6b99a614f2ed55413f0e9f13b5fd188dd2a159 Mon Sep 17 00:00:00 2001 From: Susan Hert Date: Tue, 16 Jun 2026 15:30:29 -0700 Subject: [PATCH 05/13] Kanban Issue 1924: Additional container scoping updates (#7753) --- api/src/org/labkey/api/exp/OntologyManager.java | 5 +++-- .../publish/AbstractPublishConfirmAction.java | 5 +++++ assay/src/org/labkey/assay/AssayController.java | 8 ++++++++ core/src/org/labkey/core/CoreController.java | 8 ++++++++ core/src/org/labkey/core/user/UserController.java | 4 ++++ .../controllers/exp/ExperimentController.java | 14 +++++++++++++- .../controllers/property/PropertyController.java | 2 +- 7 files changed, 42 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/exp/OntologyManager.java b/api/src/org/labkey/api/exp/OntologyManager.java index c5fd1af0111..5268b45d518 100644 --- a/api/src/org/labkey/api/exp/OntologyManager.java +++ b/api/src/org/labkey/api/exp/OntologyManager.java @@ -2841,13 +2841,14 @@ public static Object getRemappedValueForLookup(User user, Container container, R return cache.remap(SchemaKey.fromParts(lookup.getSchemaKey()), lookup.getQueryName(), user, lkContainer, ContainerFilter.Type.CurrentPlusProjectAndShared, String.valueOf(value)); } - public static List findPropertyUsages(User user, List propertyIds, int maxUsageCount) + public static List findPropertyUsagesByIds(User user, Container container, List propertyIds, int maxUsageCount) { List ret = new ArrayList<>(propertyIds.size()); for (int propertyId : propertyIds) { var pd = getPropertyDescriptor(propertyId); - if (pd == null) + // Kanban #1924: Get property descriptors for the current container only + if (pd == null || !pd.getContainer().equals(container)) throw new IllegalArgumentException("property not found: " + propertyId); ret.add(findPropertyUsages(user, pd, maxUsageCount)); diff --git a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java index e004ad49333..aee4cb7f297 100644 --- a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java +++ b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java @@ -42,6 +42,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.RedirectException; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.VBox; import org.labkey.api.view.template.ClientDependency; import org.springframework.validation.BindException; @@ -107,6 +108,10 @@ public void validateCommand(FORM form, Errors errors) { errors.reject(SpringActionController.ERROR_MSG, "Could not find target study"); } + else if (!_targetStudy.hasPermission(getUser(), InsertPermission.class)) + { + throw new UnauthorizedException("You do not have permission to insert into the target study"); + } } if (_targetStudy != null) diff --git a/assay/src/org/labkey/assay/AssayController.java b/assay/src/org/labkey/assay/AssayController.java index 507630bae50..3a8233bfc34 100644 --- a/assay/src/org/labkey/assay/AssayController.java +++ b/assay/src/org/labkey/assay/AssayController.java @@ -1457,6 +1457,9 @@ public Object execute(Object form, BindException errors) throws Exception ExpRun expRun = ExperimentService.get().getExpRun(NumberUtils.toInt(run)); if (expRun != null) { + // Kanban #1924 assure permissions to the run's container, which might be different from the current container + if (!expRun.getContainer().hasPermission(getUser(), AssayReadPermission.class)) + throw new UnauthorizedException("User does not have " + AssayReadPermission.class.getSimpleName() + " for run " + run); response.put("success", true); DataState state = AssayQCService.getProvider().getQCState(expRun.getProtocol(), expRun.getRowId()); if (state != null) @@ -1762,6 +1765,11 @@ public Object execute(AssayOperationConfirmationForm form, BindException errors) ExperimentService service = ExperimentService.get(); ExpProtocol protocol = service.getExpProtocol(form.getProtocolId()); + if (protocol == null) + throw new NotFoundException("Protocol with id " + form.getProtocolId() + " not found."); + // Kanban #1924: Assure permission in the protocol's container, which may be different than the current container + if (!protocol.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("User does not have permission to read protocol " + protocol.getName()); AssayProvider provider = AssayService.get().getProvider(protocol); if (provider == null) throw new NotFoundException("No provider found for protocol " + form.getProtocolId()); diff --git a/core/src/org/labkey/core/CoreController.java b/core/src/org/labkey/core/CoreController.java index 10d71253623..139091a8e22 100644 --- a/core/src/org/labkey/core/CoreController.java +++ b/core/src/org/labkey/core/CoreController.java @@ -413,6 +413,9 @@ else if (form.getObjectURI() != null) if (!obj.getContainer().equals(getContainer())) { + // Kanban #1924: Assure permission in the object's container + if (!obj.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException(); ActionURL correctedURL = getViewContext().getActionURL().clone(); Container objectContainer = obj.getContainer(); if (objectContainer == null) @@ -1775,6 +1778,11 @@ public ApiResponse execute(ContainerInfoForm form, BindException errors) { // Provide information about container, specifically an array of child tab folders that were deleted Container container = form.getContainerPath() != null ? ContainerManager.getForPath(form.getContainerPath()) : getContainer(); + if (container == null) + throw new NotFoundException("No container found for path: " + form.getContainerPath()); + // Kanban #1924: Assure permission to the container + if (!container.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to view the container information."); JSONArray deletedFolders = new JSONArray(); for (FolderTab folderTab : container.getDeletedTabFolders(form.getNewFolderType())) { diff --git a/core/src/org/labkey/core/user/UserController.java b/core/src/org/labkey/core/user/UserController.java index 49d9ef0d694..00141a80732 100644 --- a/core/src/org/labkey/core/user/UserController.java +++ b/core/src/org/labkey/core/user/UserController.java @@ -2636,6 +2636,10 @@ protected Collection getProjectGroupUsers(GetUsersForm form, ApiSimpleResp if (null == group) throw new NotFoundException("Cannot find group with id " + groupId); + // Kanban #1924: Assure permission in the group's container + Container groupContainer = ContainerManager.getForId(group.getContainer()); + if (null != groupContainer && !groupContainer.hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to see information about the group '" + group.getName() + "'"); response.put("groupId", group.getUserId()); response.put("groupName", group.getName()); response.put("groupCaption", SecurityManager.getDisambiguatedGroupName(group)); diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 33bfd5a917d..3e12c530585 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -737,7 +737,8 @@ public ApiResponse execute(SimpleApiJsonForm form, BindException errors) throws JSONArray runIds = json.getJSONArray("runIds"); for (int i = 0; i < runIds.length(); i++) { - ExpRunImpl run = ExperimentServiceImpl.get().getExpRun(runIds.getInt(i)); + // Kanban #1924: Make sure the run belongs to the current container. + ExpRunImpl run = ExperimentServiceImpl.get().getExpRun(runIds.getInt(i), getContainer()); if (run != null) { runs.add(run); @@ -7962,7 +7963,13 @@ public Object execute(EntitySequenceForm form, BindException errors) throws Exce { ExpSampleType sampleType = SampleTypeService.get().getSampleType(form.getRowId()); if (sampleType != null) + { + // Kanban #1924: Assure permission in the sample type's container + if (!sampleType.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this sample type."); value = sampleType.getCurrentGenId(); + } + } else { @@ -7974,7 +7981,12 @@ else if (DataClassDomainKind.NAME.equalsIgnoreCase(form.getKindName())) { ExpDataClass dataClass = ExperimentService.get().getDataClass(form.getRowId()); if (dataClass != null) + { + // Kanban #1924: assure permission in the data class's container + if (!dataClass.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new UnauthorizedException("You do not have permission to read this data class."); value = dataClass.getCurrentGenId(); + } } ApiSimpleResponse resp = new ApiSimpleResponse(); diff --git a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java index c1ee2680a67..30975c0b247 100644 --- a/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java +++ b/experiment/src/org/labkey/experiment/controllers/property/PropertyController.java @@ -2067,7 +2067,7 @@ public Object execute(PropertyUsagesForm form, BindException errors) throws Exce List usages = null; if (form.getPropertyIds() != null) { - usages = OntologyManager.findPropertyUsages(getUser(), form.getPropertyIds(), form.maxUsageCount); + usages = OntologyManager.findPropertyUsagesByIds(getUser(), getContainer(), form.getPropertyIds(), form.maxUsageCount); } else if (form.getPropertyURIs() != null) { From b4d38c458f339d9f9500490019c7a10824c67ab0 Mon Sep 17 00:00:00 2001 From: Josh Eckels Date: Tue, 16 Jun 2026 16:44:55 -0700 Subject: [PATCH 06/13] Improve scoping checks for objects, round 2 (#7749) #### Rationale We can improve our parameter validation #### Changes - New test coverage - Assorted scoping checks --- .../announcements/AnnouncementModule.java | 3 +- .../labkey/announcements/ToursController.java | 41 ++ .../api/data/AbstractParticipantCategory.java | 5 +- .../org/labkey/api/data/TableViewForm.java | 67 +++- .../AbstractContainerScopingTest.java | 20 +- .../src/org/labkey/assay/AssayController.java | 170 ++++++++ assay/src/org/labkey/assay/AssayModule.java | 1 + .../org/labkey/assay/plate/PlateManager.java | 11 + core/src/org/labkey/core/CoreModule.java | 1 + .../labkey/core/admin/AdminController.java | 70 ++++ .../miniprofiler/MiniProfilerController.java | 10 +- .../controllers/exp/ExperimentController.java | 373 +++++++++++++++++- .../mothership/MothershipController.java | 53 ++- .../labkey/pipeline/PipelineController.java | 68 +++- .../org/labkey/pipeline/PipelineModule.java | 1 + .../pipeline/api/PipelineStatusManager.java | 5 +- .../query/TriggerConfigurationsTable.java | 5 +- .../pipeline/status/StatusController.java | 149 ++++++- query/src/org/labkey/query/QueryModule.java | 1 + .../query/controllers/OlapController.java | 67 ++++ .../actions/SpecimenApiController.java | 1 + .../specimen/actions/SpecimenController.java | 78 ++++ study/src/org/labkey/study/StudyModule.java | 6 +- .../study/controllers/StudyController.java | 55 ++- .../reports/ReportsController.java | 41 ++ .../study/model/ParticipantGroupManager.java | 64 ++- .../studydesign/StudyDesignController.java | 52 +++ .../labkey/studydesign/StudyDesignModule.java | 3 +- .../studydesign/model/TreatmentManager.java | 18 +- .../src/org/labkey/survey/SurveyManager.java | 31 +- wiki/src/org/labkey/wiki/WikiController.java | 73 +++- 31 files changed, 1441 insertions(+), 102 deletions(-) diff --git a/announcements/src/org/labkey/announcements/AnnouncementModule.java b/announcements/src/org/labkey/announcements/AnnouncementModule.java index 0d1e6466ad9..f4888b00295 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementModule.java +++ b/announcements/src/org/labkey/announcements/AnnouncementModule.java @@ -216,7 +216,8 @@ public void startBackgroundThreads() public Set getIntegrationTests() { return Set.of( - AnnouncementManager.TestCase.class + AnnouncementManager.TestCase.class, + ToursController.ContainerScopingTestCase.class ); } diff --git a/announcements/src/org/labkey/announcements/ToursController.java b/announcements/src/org/labkey/announcements/ToursController.java index 7fa4054f84c..49c6bd7bbb6 100644 --- a/announcements/src/org/labkey/announcements/ToursController.java +++ b/announcements/src/org/labkey/announcements/ToursController.java @@ -15,7 +15,9 @@ */ package org.labkey.announcements; +import jakarta.servlet.http.HttpServletResponse; import org.json.JSONObject; +import org.junit.Test; import org.labkey.announcements.model.TourManager; import org.labkey.announcements.model.TourModel; import org.labkey.announcements.query.AnnouncementSchema; @@ -30,10 +32,17 @@ import org.labkey.api.query.QueryView; import org.labkey.api.security.ActionNames; import org.labkey.api.security.RequiresPermission; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.UnauthorizedException; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -123,6 +132,12 @@ public static class SaveTourAction extends MutatingApiAction @Override public void validateForm(SimpleApiJsonForm form, Errors errors) { + // The "//will check below" gate on the annotation was never implemented: this action inserts/updates tour + // content (a folder-level configuration asset) but performed no insert/update/admin check, so a Read user + // could create or overwrite tours. Require folder admin to manage tours. + if (!getContainer().hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException("You do not have permission to modify tours in this folder."); + TourModel model; JSONObject json = form.getJsonObject(); int rowId = json.getInt("rowId"); @@ -203,4 +218,30 @@ public void setRowid(String rowid) _rowid = rowid; } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSaveTourRequiresAdmin() throws Exception + { + Container folder = createContainer("A"); + ActionURL url = new ActionURL(SaveTourAction.class, folder); + + // A Reader must not be able to create/modify tours + User reader = createUserInRole(folder, ReaderRole.class); + JSONObject body = new JSONObject().put("rowId", -1); + assertStatus(HttpServletResponse.SC_FORBIDDEN, postJson(url, reader, body)); + + // Positive control: a folder admin passes the permission gate and the tour is created (success, 200). + User folderAdmin = createUserInRole(folder, FolderAdminRole.class); + JSONObject adminBody = new JSONObject() + .put("rowId", -1) + .put("title", "scoping-test-tour") + .put("description", "d") + .put("mode", "0") + .put("tour", new JSONObject()); + MockHttpServletResponse resp = postJson(url, folderAdmin, adminBody); + assertStatus(HttpServletResponse.SC_OK, resp); + } + } } diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index 914e1a939ee..00ab7ad438b 100644 --- a/api/src/org/labkey/api/data/AbstractParticipantCategory.java +++ b/api/src/org/labkey/api/data/AbstractParticipantCategory.java @@ -229,7 +229,10 @@ public boolean canEdit(Container container, User user, List err else { User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; + boolean allowed = + container.hasPermission(user, SharedParticipantGroupPermission.class) || + container.hasPermission(user, AdminPermission.class) || + (owner != null && !owner.isGuest() && owner.equals(user)); if (!allowed) errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); diff --git a/api/src/org/labkey/api/data/TableViewForm.java b/api/src/org/labkey/api/data/TableViewForm.java index 7254abb1e69..6b4a2b62506 100644 --- a/api/src/org/labkey/api/data/TableViewForm.java +++ b/api/src/org/labkey/api/data/TableViewForm.java @@ -34,6 +34,7 @@ import org.labkey.api.action.NullSafeBindException; import org.labkey.api.action.SpringActionController; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.query.FieldKey; import org.labkey.api.query.SchemaKey; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; @@ -183,8 +184,21 @@ public void doUpdate() throws SQLException throw new UnauthorizedException(); } - if (null != _tinfo.getColumn("container")) + FieldKey containerFK = FieldKey.fromParts("Container"); + if (null != _tinfo.getColumn(containerFK)) + { + // The hasPermission() check above only proves the user can update the *current* container. The UPDATE below + // keys on the PK alone and stamps the row into the current container, so without this guard a user with + // update permission here could edit (and re-home) a row that actually lives in another container simply by + // POSTing its PK. Confirm the existing row is in this container; 404 otherwise. PkFilter validates and + // converts the PK as well, so a missing or malformed key likewise 404s here rather than later. + SimpleFilter filter = new PkFilter(_tinfo, getPkVals()); + filter.addCondition(containerFK, _c.getId()); + if (!new TableSelector(_tinfo, filter, null).exists()) + throw new NotFoundException("No matching row found in this folder"); + set("container", _c.getId()); + } Object[] pkVal = getPkVals(); Map newMap = Table.update(_user, _tinfo, getTypedValues(), pkVal); @@ -207,21 +221,50 @@ public void doDelete() throw new UnauthorizedException(); } - if (null != _selectedRows && _selectedRows.length > 0) - { - for (String selectedRow : _selectedRows) - Table.delete(_tinfo, selectedRow); - } - else + // Table.delete() keys on the PK alone. As with doUpdate(), the DeletePermission check only proves the user can + // delete in the *current* container, so for container-scoped tables we must confirm each target row lives here; + // otherwise a user could delete a row that belongs to another container by POSTing (or grid-selecting) its PK. + FieldKey containerFK = FieldKey.fromParts("Container"); + boolean scopeToContainer = null != _tinfo.getColumn(containerFK); + + try (DbScope.Transaction t = _tinfo.getSchema().getScope().ensureTransaction()) { - Object[] pkVal = getPkVals(); - if (null != pkVal && null != pkVal[0]) - Table.delete(_tinfo, pkVal); - else //Hmm, throw an exception here???? - _log.warn("Nothing to delete for table " + _tinfo.getName() + " on request " + _request.getRequestURI()); + if (null != _selectedRows && _selectedRows.length > 0) + { + for (String selectedRow : _selectedRows) + { + if (scopeToContainer) + deleteInContainer(selectedRow, containerFK); + else + Table.delete(_tinfo, selectedRow); + } + } + else + { + Object[] pkVal = getPkVals(); + if (null != pkVal && null != pkVal[0]) + { + if (scopeToContainer) + deleteInContainer(pkVal, containerFK); + else + Table.delete(_tinfo, pkVal); + } + else //Hmm, throw an exception here???? + _log.warn("Nothing to delete for table " + _tinfo.getName() + " on request " + _request.getRequestURI()); + } } } + // Deletes a single row only if it lives in the form's container, scoping the DELETE's WHERE clause to the PK and the + // container together. 404s on a miss (cross-container or already gone), mirroring doUpdate(). + private void deleteInContainer(Object pkVal, FieldKey containerFK) + { + SimpleFilter filter = new PkFilter(_tinfo, pkVal); + filter.addCondition(containerFK, _c.getId()); + if (Table.delete(_tinfo, filter) == 0) + throw new NotFoundException("No matching row found in this folder"); + } + /** * Pulls in the data from the current row of the database. */ diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java index fd0ea652544..37841fadeaa 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -16,6 +16,7 @@ package org.labkey.api.security.permissions; import jakarta.servlet.http.HttpServletResponse; +import org.json.JSONObject; import org.junit.After; import org.junit.Assert; import org.labkey.api.data.Container; @@ -57,6 +58,7 @@ public abstract class AbstractContainerScopingTest extends Assert { private static final Map FORM_HEADERS = Map.of("Content-Type", "application/x-www-form-urlencoded"); + private static final Map JSON_HEADERS = Map.of("Content-Type", "application/json"); private final List _containers = new ArrayList<>(); private final List _users = new ArrayList<>(); @@ -75,7 +77,17 @@ protected User getAdmin() protected Container createContainer(String name) { Container junit = JunitUtil.getTestContainer(); - Container c = ContainerManager.ensureContainer(junit.getParsedPath().append(getClass().getSimpleName() + "-" + name, true), getAdmin()); + // Use the fully-qualified class name, not getSimpleName(): the nested test class is named + // "ContainerScopingTestCase" in nearly every controller, so getSimpleName() would give them all the SAME + // /_junit child path and they would share fixtures (and collide on unique constraints across runs). Sanitize + // to a valid folder name, and force-delete any fixture an interrupted prior run left behind so each run starts + // from a clean container even when a previous @After could not complete. + String prefix = getClass().getName().replaceAll("[^A-Za-z0-9]", "_"); + var path = junit.getParsedPath().append(prefix + "-" + name, true); + Container existing = ContainerManager.getForPath(path); + if (existing != null) + ContainerManager.deleteAll(existing, getAdmin()); + Container c = ContainerManager.ensureContainer(path, getAdmin()); _containers.add(c); return c; } @@ -124,6 +136,12 @@ protected MockHttpServletResponse post(ActionURL url, User user) throws Exceptio return ViewServlet.POST(url, user, FORM_HEADERS, null); } + /** Dispatch a JSON POST to a {@code @Marshal(Jackson)} API action, with {@code body} supplied as the request body. */ + protected MockHttpServletResponse postJson(ActionURL url, User user, JSONObject body) throws Exception + { + return ViewServlet.POST(url, user, JSON_HEADERS, body.toString()); + } + /** Assert that a dispatched response has the expected HTTP status code. */ protected void assertStatus(int expected, HttpServletResponse response) { diff --git a/assay/src/org/labkey/assay/AssayController.java b/assay/src/org/labkey/assay/AssayController.java index 3a8233bfc34..6f3c5819eed 100644 --- a/assay/src/org/labkey/assay/AssayController.java +++ b/assay/src/org/labkey/assay/AssayController.java @@ -160,11 +160,31 @@ import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.mvc.Controller; +import org.springframework.mock.web.MockMultipartHttpServletRequest; +import jakarta.servlet.http.HttpSession; +import org.junit.After; +import org.junit.Test; +import org.labkey.api.assay.AssayDataCollector; +import org.labkey.api.assay.AssayRunCreator; +import org.labkey.api.assay.PipelineDataCollector; +import org.labkey.api.exp.api.ExpExperiment; +import org.labkey.api.gwt.client.assay.model.GWTProtocol; +import org.labkey.api.gwt.client.model.GWTDomain; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; +import org.labkey.api.view.ViewBackgroundInfo; +import org.labkey.api.view.ViewContext; import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -1419,6 +1439,24 @@ public ApiResponse execute(SetResultFlagForm form, BindException errors) if (!org.labkey.api.gwt.client.ui.PropertyType.expFlag.getURI().equals(flagCol.getConceptURI())) throw new NotFoundException(); + // Confirm every requested result row actually belongs to the request container by reading its run's container + Set requestedIds = new HashSet<>(form.getRowList()); + if (!requestedIds.isEmpty()) + { + SimpleFilter idFilter = new SimpleFilter(); + idFilter.addInClause(FieldKey.fromParts("RowId"), requestedIds); + Map[] rows = new TableSelector(assayResultTable, PageFlowUtil.set("RowId", "Folder"), idFilter, null).getMapArray(); + Set allowedIds = new HashSet<>(); + String currentContainerId = getContainer().getId(); + for (Map row : rows) + { + if (currentContainerId.equals(String.valueOf(row.get("Folder"))) && row.get("RowId") instanceof Number n) + allowedIds.add(n.intValue()); + } + if (!allowedIds.containsAll(requestedIds)) + throw new NotFoundException("One or more result rows were not found in this folder"); + } + DbScope scope = ti.getSchema().getScope(); int rowsAffected = 0 ; try (DbScope.Transaction transaction = scope.ensureTransaction()) @@ -1608,6 +1646,14 @@ public ApiSimpleResponse execute(UpdateQCStateForm form, BindException errors) t ApiSimpleResponse response = new ApiSimpleResponse(); if (form.getRuns() != null && _firstRun != null) { + for (int id : form.getRuns()) + { + // Support cross-container operations but confirm permission + ExpRun run = ExperimentService.get().getExpRun(id); + if (run == null || !run.getContainer().hasPermission(getUser(), QCAnalystPermission.class)) + throw new NotFoundException("Run " + id + " not found in this folder"); + } + DataState state = DataStateManager.getInstance().getStateForRowId(_firstRun.getProtocol().getContainer(), form.getState()); if (state != null) AssayQCService.getProvider().setQCStates(_firstRun.getProtocol(), getContainer(), getUser(), List.copyOf(form.getRuns()), state, form.getComment()); @@ -1965,4 +2011,128 @@ public Object execute(FilterCriteriaColumnsForm form, BindException errors) thro return AssayPlateMetadataService.get().previewFilterCriteriaColumns(protocol, form.getColumnNames()); } } + + /** + * Verifies that assay actions resolving result rows by global RowId reject rows that belong to a different + * container, even when the caller has the action's required permission in the request folder. The design is created + * in /Shared so its protocol resolves from any sibling folder (project + shared scope, per ProtocolIdForm), while + * the result rows live in a folder where the caller has no rights. + */ + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private ExpProtocol _sharedProtocol; // lives in /Shared; not auto-cleaned, so we delete it in @After + + @After + public void deleteSharedAssay() + { + if (_sharedProtocol != null) + { + try { _sharedProtocol.delete(getAdmin()); } catch (Exception ignored) {} + _sharedProtocol = null; + } + } + + // Build a General (GPAT) design in /Shared so its protocol resolves from any sibling folder, with a results + // "Flag" property (conceptURI expFlag) that SetResultFlagAction requires plus a plain int result field so a + // one-row file can be imported. + private Pair createSharedAssay() throws Exception + { + Container shared = ContainerManager.getSharedContainer(); + ViewContext ctx = new ViewContext(new ViewBackgroundInfo(shared, getAdmin(), null)); + AssayDomainServiceImpl svc = new AssayDomainServiceImpl(ctx); + GWTProtocol tmpl = svc.getAssayTemplate("General"); + tmpl.setName("scoping-assay-" + getClass().getSimpleName()); + tmpl.setEditableResults(true); + + for (GWTDomain d : tmpl.getDomains()) + { + if ("Batch Fields".equals(d.getName()) || "Run Fields".equals(d.getName())) + d.getFields(true).clear(); + else if ("Data Fields".equals(d.getName())) + { + d.getFields(true).clear(); + d.getFields(true).add(new GWTPropertyDescriptor("result", "int")); + GWTPropertyDescriptor flag = new GWTPropertyDescriptor("Flag", "string"); + // SetResultFlagAction requires the target column to carry the expFlag conceptURI + flag.setConceptURI(org.labkey.api.gwt.client.ui.PropertyType.expFlag.getURI()); + d.getFields(true).add(flag); + } + } + + GWTProtocol saved = svc.saveChanges(tmpl, true); + ExpProtocol protocol = ExperimentService.get().getExpProtocol(saved.getProtocolId()); + _sharedProtocol = protocol; + return Pair.of(AssayService.get().getProvider(protocol), protocol); + } + + // Import a one-row run into container c (as the site admin) and return its assay-result RowId. + private int importOneResult(Container c, AssayProvider provider, ExpProtocol protocol) throws Exception + { + var pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("Test requires a pipeline root for " + c.getPath(), pipeRoot); + File file = FileUtil.createTempFile(getClass().getSimpleName(), ".tsv", pipeRoot.getRootPath()); + Files.writeString(file.toPath(), "result\n100\n", StandardCharsets.UTF_8); + + HttpSession session = TestContext.get().getRequest().getSession(); + PipelineDataCollector.setFileCollection(session, c, protocol, List.of(Map.of(AssayDataCollector.PRIMARY_FILE, file))); + + MockMultipartHttpServletRequest req = new MockMultipartHttpServletRequest(); + req.setUserPrincipal(getAdmin()); + req.setSession(session); + AssayRunUploadForm uploadForm = new AssayRunUploadForm<>(); + uploadForm.setViewContext(new ViewContext(req, null, null)); + uploadForm.setContainer(c); + uploadForm.setUser(getAdmin()); + uploadForm.setRowId(protocol.getRowId()); + uploadForm.setName("scoping-run-" + c.getName()); + uploadForm.setDataCollectorName("Pipeline"); + + AssayRunCreator runCreator = provider.getRunCreator(); + Pair pair = runCreator.saveExperimentRun(uploadForm, null); + ExpRun run = pair.second; + + AssayProtocolSchema schema = provider.createProtocolSchema(getAdmin(), c, protocol, null); + TableInfo results = schema.getTable("Data"); + FieldKey runRowIdFk = new FieldKey(FieldKey.fromParts("Run"), "RowId"); + Map row = new TableSelector(results, Set.of("RowId"), new SimpleFilter(runRowIdFk, run.getRowId()), null).getMap(); + return (Integer) row.get("RowId"); + } + + @Test + public void testSetResultFlagRejectsForeignResultRow() throws Exception + { + Container shared = ContainerManager.getSharedContainer(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + Pair assay = createSharedAssay(); + AssayProvider provider = assay.first; + ExpProtocol protocol = assay.second; + + int resultRowId = importOneResult(folderB, provider, protocol); // result row lives in folder B + + // Caller can Update folder A and Read the shared design, but has NO rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + grantRole(editorA, shared, ReaderRole.class); + + // Flagging B's result row through folder A must 404 at the per-row Folder guard (not at getProtocol). + ActionURL foreignUrl = new ActionURL(SetResultFlagAction.class, folderA) + .addParameter("rowId", String.valueOf(protocol.getRowId())) + .addParameter("providerName", provider.getName()) + .addParameter("resultRowIds", String.valueOf(resultRowId)) + .addParameter("columnName", "Flag") + .addParameter("comment", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // Positive control: an Editor in B (with Read on the shared design) flags the same row through folder B → 200. + User editorB = createUserInRole(folderB, EditorRole.class); + grantRole(editorB, shared, ReaderRole.class); + ActionURL ownUrl = new ActionURL(SetResultFlagAction.class, folderB) + .addParameter("rowId", String.valueOf(protocol.getRowId())) + .addParameter("providerName", provider.getName()) + .addParameter("resultRowIds", String.valueOf(resultRowId)) + .addParameter("columnName", "Flag") + .addParameter("comment", "ok"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, editorB)); + } + } } diff --git a/assay/src/org/labkey/assay/AssayModule.java b/assay/src/org/labkey/assay/AssayModule.java index e03b6e1d23d..5978d7c0d5d 100644 --- a/assay/src/org/labkey/assay/AssayModule.java +++ b/assay/src/org/labkey/assay/AssayModule.java @@ -317,6 +317,7 @@ public Set getProvisionedSchemaNames() { return Set.of( ModuleAssayCache.TestCase.class, + AssayController.ContainerScopingTestCase.class, PlateManagerTest.class, PlateSchemaTest.class ); diff --git a/assay/src/org/labkey/assay/plate/PlateManager.java b/assay/src/org/labkey/assay/plate/PlateManager.java index c62458a3b65..d9c320787ed 100644 --- a/assay/src/org/labkey/assay/plate/PlateManager.java +++ b/assay/src/org/labkey/assay/plate/PlateManager.java @@ -3224,6 +3224,17 @@ public void markHits( } else { + // Verify the caller has UpdatePermission on the container of every hit row being removed + SimpleFilter hitFilter = new SimpleFilter(FieldKey.fromParts("ResultId"), rowIds, CompareType.IN); + hitFilter.addCondition(FieldKey.fromParts("ProtocolId"), protocol.getRowId()); + Set hitContainerIds = new HashSet<>(new TableSelector(hitTable, Collections.singleton("Container"), hitFilter, null).getArrayList(String.class)); + for (String hitContainerId : hitContainerIds) + { + Container hitContainer = ContainerManager.getForId(hitContainerId); + if (hitContainer == null || !hitContainer.hasPermission(user, UpdatePermission.class)) + throw new UnauthorizedException("Failed to unmark hits. You do not have permissions to update hits in this folder."); + } + deleteHits(protocolId, rowIds); } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 3c862288961..fb6dddf9a93 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1395,6 +1395,7 @@ public Set getIntegrationTests() AdminController.TestCase.class, AdminController.WorkbookDeleteTestCase.class, AdminController.ImportFolderSourceScopingTestCase.class, + AdminController.RevertFolderScopingTestCase.class, AllowListType.TestCase.class, AttachmentServiceImpl.TestCase.class, CoreController.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 8868dcbf947..69cc3884501 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -8003,6 +8003,8 @@ public boolean handlePost(SetFolderPermissionsForm form, BindException errors) { throw new NotFoundException("An unknown project was specified to copy permissions from: " + targetProject); } + if (!source.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException("You do not have permission to copy permissions from the specified project."); Map groupMap = GroupManager.copyGroupsToContainer(source, c, getUser()); //copy role assignments @@ -8425,6 +8427,9 @@ public ApiResponse execute(RevertFolderForm form, BindException errors) Container revertContainer = ContainerManager.getForPath(form.getContainerPath()); if (null != revertContainer) { + if (!revertContainer.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); + if (revertContainer.isContainerTab()) { FolderTab tab = revertContainer.getParent().getFolderType().findTab(revertContainer.getName()); @@ -10205,6 +10210,10 @@ public ApiResponse execute(DeletedFoldersForm form, BindException errors) if (isBlank(form.getContainerPath())) throw new NotFoundException(); Container container = ContainerManager.getForPath(form.getContainerPath()); + if (container == null) + throw new NotFoundException(); + if (!container.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException(); for (String tabName : form.getResurrectFolders()) { ContainerManager.clearContainerTabDeleted(container, tabName, form.getNewFolderType()); @@ -12321,4 +12330,65 @@ public void testImportFromTemplateRequiresSourceAdmin() throws Exception // Positive control performed in S3ImportTest.testS3Import(). Difficult to mock here due to pipeline job } } + + public static class RevertFolderScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testRevertFolderRequiresTargetAdmin() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + ActionURL foreignUrl = new ActionURL(RevertFolderAction.class, folderA) + .addParameter("containerPath", folderB.getPath()); + + // Cross-container attempt by a caller who is not an admin of the target -> 403, before any mutation. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a site admin (who IS an admin of the target) is allowed through even cross-container, + // proving the fix re-checks AdminPermission on the target rather than locking to the request container. + // folderB is a plain folder with no container tabs, so the action makes no change and returns success:false + // at status 200 -- the point is that the guard does not reject an authorized caller. + assertStatus(HttpServletResponse.SC_OK, post(foreignUrl, admin)); + } + + @Test + public void testClearDeletedTabFoldersRequiresTargetAdmin() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + ActionURL foreignUrl = new ActionURL(ClearDeletedTabFoldersAction.class, folderA) + .addParameter("containerPath", folderB.getPath()) + .addParameter("resurrectFolders", "anyTab"); + + // A folder admin in A only must not clear deleted-tab markers in folder B (resolved by containerPath) -> 403. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a site admin (admin of the target) is allowed through -> 200. + assertStatus(HttpServletResponse.SC_OK, post(foreignUrl, admin)); + } + + @Test + public void testSetFolderPermissionsCopyRequiresSourceAdmin() throws Exception + { + Container dest = createContainer("Dest"); + Container source = createContainer("Source"); + User destAdmin = createUserInRole(dest, FolderAdminRole.class); + + // Copying groups/role assignments from a project the caller does not administer must be rejected. The + // action's @RequiresPermission(AdminPermission.class) only proves admin on the destination container, so a + // dest-only admin supplying another project's id as targetProject must get 403, not a copy of its security + // configuration. (Positive control omitted: a successful copy needs real project group/policy fixtures.) + ActionURL url = new ActionURL(SetFolderPermissionsAction.class, dest) + .addParameter("permissionType", "CopyExistingProject") + .addParameter("targetProject", source.getId()); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, destAdmin)); + } + } } diff --git a/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java b/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java index 75eae655108..05fa70c112b 100644 --- a/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java +++ b/core/src/org/labkey/core/admin/miniprofiler/MiniProfilerController.java @@ -243,17 +243,17 @@ public Object execute(RequestForm form, BindException errors) throw new UnauthorizedException(); RequestInfo req = MemTracker.getInstance().getRequest(form.getId()); + if (req != null && !getUser().equals(req.getUser()) && !getUser().hasApplicationAdminPermission()) + { + throw new UnauthorizedException(); + } + MemTracker.get().setViewed(getUser(), form.getId()); // Reset the X-MiniProfiler-Ids header to only include remaining unviewed (without the id we are returning) LinkedHashSet ids = new LinkedHashSet<>(MemTracker.get().getUnviewed(getUser())); getViewContext().getResponse().setHeader("X-MiniProfiler-Ids", ids.toString()); - if (req != null && !getUser().equals(req.getUser()) && !getUser().hasApplicationAdminPermission()) - { - throw new UnauthorizedException(); - } - return req; } } diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 3e12c530585..766bdaf1477 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -150,6 +150,7 @@ import org.labkey.api.exp.xar.LsidUtils; import org.labkey.api.files.FileContentService; import org.labkey.api.gwt.client.AuditBehaviorType; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.inventory.InventoryService; import org.labkey.api.module.ModuleHtmlView; import org.labkey.api.module.ModuleLoader; @@ -201,7 +202,10 @@ import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; 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.ReaderRole; +import org.labkey.api.security.roles.Role; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.ConceptURIProperties; import org.labkey.api.sql.LabKeySql; @@ -3754,6 +3758,10 @@ protected void deleteObjects(DeleteForm form) { for (ExpProtocol protocol : getProtocolsForDeletion(form)) { + // Re-check here - cannot delete a run-less assay design owned by another container via a forged rowId. + if (!protocol.getContainer().hasPermission(getUser(), DesignAssayPermission.class)) + throw new UnauthorizedException("You do not have sufficient permissions to delete this assay design."); + protocol.delete(getUser(), form.getUserComment()); } } @@ -4715,6 +4723,12 @@ public void validateCommand(ExperimentForm target, Errors errors) @Override public boolean handlePost(ExperimentForm form, BindException errors) throws Exception { + // Confirm the run group actually lives in this container before updating, like the GET sibling ShowUpdateAction. + Experiment bean = form.getBean(); + ExpExperiment exp = bean == null ? null : ExperimentService.get().getExpExperiment(bean.getRowId()); + if (exp == null || !getContainer().equals(exp.getContainer())) + throw new NotFoundException("Run group not found in this folder"); + form.doUpdate(); form.refreshFromDb(); _exp = form.getBean(); @@ -5281,7 +5295,19 @@ public void validateCommand(ExperimentRunListForm target, Errors errors) @Override public boolean handlePost(ExperimentRunListForm form, BindException errors) { - addSelectedRunsToExperiment(form.lookupExperiment(), form.getDataRegionSelectionKey()); + ExpExperiment exp = form.lookupExperiment(); + if (exp == null || !exp.getContainer().hasPermission(getUser(), InsertPermission.class)) + throw new NotFoundException("Could not find run group with RowId " + form.getExpRowId()); + + List runs = new ArrayList<>(); + for (int runId : DataRegionSelection.getSelectedIntegers(getViewContext(), form.getDataRegionSelectionKey(), true)) + { + ExpRun run = ExperimentService.get().getExpRun(runId); + if (run == null || !run.getContainer().hasPermission(getUser(), InsertPermission.class)) + throw new NotFoundException("Could not find run with RowId " + runId); + runs.add(run); + } + exp.addRuns(getUser(), runs.toArray(new ExpRun[0])); return true; } @@ -6022,9 +6048,9 @@ else if (in.rowId > 0) errors.reject(ERROR_MSG, "Can't resolve sample '" + in.rowId + "'"); } - if (m == null) + if (m == null || !m.getContainer().hasPermission(getUser(), ReadPermission.class)) { - errors.reject(ERROR_MSG, "Material input lsid or rowId required"); + errors.reject(ERROR_MSG, "Material input couldn't be resolved"); continue; } @@ -6064,9 +6090,9 @@ else if (in.rowId > 0) errors.reject(ERROR_MSG, "Can't resolve data '" + in.rowId + "'"); } - if (d == null) + if (d == null || !d.getContainer().hasPermission(getUser(), ReadPermission.class)) { - errors.reject(ERROR_MSG, "Data input lsid or rowId required"); + errors.reject(ERROR_MSG, "Data input couldn't be resolved"); continue; } @@ -6089,9 +6115,8 @@ else if (in.rowId > 0) ExpSampleType outSampleType; if (form.targetSampleType != null) { - // TODO: check in scope and has permission outSampleType = SampleTypeService.get().getSampleType(form.targetSampleType.toString()); - if (outSampleType == null) + if (outSampleType == null || !outSampleType.getContainer().hasPermission(getUser(), ReadPermission.class)) errors.reject(ERROR_MSG, "Sample type not found: " + form.targetSampleType.toString()); } else @@ -6102,9 +6127,8 @@ else if (in.rowId > 0) ExpDataClass outDataClass; if (form.targetDataClass != null) { - // TODO: check in scope and has permission outDataClass = ExperimentServiceImpl.get().getDataClass(form.targetDataClass.toString()); - if (outDataClass == null) + if (outDataClass == null || !outDataClass.getContainer().hasPermission(getUser(), ReadPermission.class)) errors.reject(ERROR_MSG, "DataClass not found: " + form.targetDataClass.toString()); } else @@ -6585,10 +6609,11 @@ public boolean handlePost(MoveRunsForm form, BindException errors) for (Integer runId : runIds) { ExpRun run = ExperimentService.get().getExpRun(runId); - if (run != null) + if (run == null || !run.getContainer().equals(getContainer())) { - runs.add(run); + throw new NotFoundException("Could not find run with RowId " + runId + " in this folder"); } + runs.add(run); } ViewBackgroundInfo info = getViewBackgroundInfo(); @@ -8027,6 +8052,9 @@ public Object execute(EntitySequenceForm form, BindException errors) ExpSampleType sampleType = SampleTypeService.get().getSampleType(form.getRowId()); if (sampleType != null) { + if (!sampleType.getContainer().hasPermission(getUser(), DesignSampleTypePermission.class)) + throw new UnauthorizedException("Insufficient permissions."); + sampleType.ensureMinGenId(form.getNewValue()); domain = sampleType.getDomain(); } @@ -8412,5 +8440,328 @@ public void testDataClassAttachmentContainerScoping() throws Exception JSONObject json = new JSONObject(resp.getContentAsString()); assertEquals("No ExpData found for id: " + data.getRowId(), json.get("exception")); } + + @Test + public void testUpdateRunGroupContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run group (Experiment) that lives in folder B + ExpExperiment runGroup = ExperimentService.get().createExpExperiment(folderB, "scoping-test-run-group"); + runGroup.save(admin); + int rowId = runGroup.getRowId(); + + // A caller who can Update folder A (Editor) but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Updating B's run group through folder A must 404 rather than overwrite/re-home it. The action's + // @RequiresPermission(UpdatePermission.class) passes in folder A, so without the handlePost guard the + // unscoped doUpdate() would edit B's row and rewrite its container to A. + ActionURL foreignUrl = new ActionURL(UpdateAction.class, folderA) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Name", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + // A site admin, who CAN update folder B, still gets 404 through folder A (no cross-container write). + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, admin)); + + // The run group must be untouched in its own container + ExpExperiment after = ExperimentService.get().getExpExperiment(rowId); + assertNotNull("Run group must still exist", after); + assertEquals("Name must be unchanged after a cross-container update", "scoping-test-run-group", after.getName()); + assertEquals("Container must be unchanged after a cross-container update", folderB, after.getContainer()); + + // Positive control: updating through its own container succeeds (302 redirect to the success URL) and applies. + ActionURL ownUrl = new ActionURL(UpdateAction.class, folderB) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Name", "renamed"); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + assertEquals("Name should be updated by a same-container request", "renamed", + ExperimentService.get().getExpExperiment(rowId).getName()); + } + + @Test + public void testAddRunsToExperimentContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run group (Experiment) that lives in folder B + ExpExperiment runGroup = ExperimentService.get().createExpExperiment(folderB, "scoping-test-add-runs"); + runGroup.save(admin); + int expRowId = runGroup.getRowId(); + + // A caller who can Insert in folder A but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Adding runs to B's run group through folder A must 404: the run group is resolved by a global RowId and + // ExpExperimentImpl.addRuns does a raw INSERT with no authorization, so without the handlePost guard a + // forged expRowId would let a folder-A user mutate a foreign run group. + ActionURL foreignUrl = new ActionURL(AddRunsToExperimentAction.class, folderA) + .addParameter("expRowId", String.valueOf(expRowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // Positive control: addressing the run group through its own container passes the guard. No runs are + // selected, so the action makes no change and redirects to the group's details page (302). + ActionURL ownUrl = new ActionURL(AddRunsToExperimentAction.class, folderB) + .addParameter("expRowId", String.valueOf(expRowId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + } + + @Test + public void testMoveRunsContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run that lives in folder B + ExpRun run = createRun(folderB, "scoping-test-move-run"); + int runId = run.getRowId(); + + // A caller with Insert+Delete in folder A (Editor) but no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // MoveRuns is scoped to getContainer() as the source and only checks Insert on the target; runs are resolved + // from the client-supplied selection by global RowId. Moving B's run via folder A (as both source and + // target) must 404 because the run does not live in the source container the caller is operating in. + ActionURL foreignUrl = new ActionURL(MoveRunsAction.class, folderA) + .addParameter("targetContainerId", folderA.getId()) + .addParameter(DataRegion.SELECT_CHECKBOX_NAME, String.valueOf(runId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // The run must remain in folder B + ExpRun after = ExperimentService.get().getExpRun(runId); + assertNotNull("Run must still exist", after); + assertEquals("Run must not have been moved out of its container", folderB, after.getContainer()); + + // Positive control: a successful move queues a MoveRunsPipelineJob, which is exercised by existing run-move + // tests; this case verifies only that the cross-container request is rejected before any job is queued. + } + + @Test + public void testDeleteProtocolByRowIdsContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A run-less protocol (assay design) that lives in folder B. Run-less so deleteProtocolByRowIds skips its + // AdminPermission check, leaving the per-protocol DesignAssay guard in deleteObjects as the only gate. + ExpProtocol protocol = ExperimentService.get().createExpProtocol(folderB, ExpProtocol.ApplicationType.ExperimentRun, "scoping-test-protocol"); + protocol.save(getAdmin()); + int rowId = protocol.getRowId(); + + // A caller who can design assays in folder A only. DesignAssayPermission's role lives in the assay module, + // resolved by name here to avoid a compile-time dependency from the experiment module. + @SuppressWarnings("unchecked") + Class assayDesigner = (Class) Class.forName("org.labkey.assay.security.AssayDesignerRole"); + User designerA = createUserInRole(folderA, assayDesigner); + + // Force-deleting B's protocol through folder A must be rejected. The forceDelete POST path runs handlePost + // -> deleteObjects directly, bypassing the getView container check, so the deleteObjects guard is what stops + // it (403 for an authenticated caller lacking DesignAssay on the protocol's own container). + ActionURL foreignUrl = new ActionURL(DeleteProtocolByRowIdsAction.class, folderA) + .addParameter("forceDelete", "true") + .addParameter("singleObjectRowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, designerA)); + + // The protocol must still exist in folder B + assertNotNull("Protocol must not have been deleted cross-container", ExperimentService.get().getExpProtocol(rowId)); + + // Positive control: once the caller is granted design rights in folder B, the same forceDelete through + // folder B succeeds (302) and removes the protocol -- proving the guard rejects only the cross-container case. + grantRole(designerA, folderB, assayDesigner); + ActionURL ownUrl = new ActionURL(DeleteProtocolByRowIdsAction.class, folderB) + .addParameter("forceDelete", "true") + .addParameter("singleObjectRowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, designerA)); + assertNull("Protocol should be deleted by a same-container request", ExperimentService.get().getExpProtocol(rowId)); + } + + @Test + public void testSetEntitySequenceContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A sample type that lives in folder B + List props = List.of(new GWTPropertyDescriptor("name", "string")); + ExpSampleType sampleType = SampleTypeService.get().createSampleType(folderB, getAdmin(), "scoping-test-st", null, + props, Collections.emptyList(), -1, -1, -1, -1, null, null); + int rowId = sampleType.getRowId(); + + // A folder admin in A only (has DesignSampleType in A via FolderAdminRole, no rights in B) + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + // Advancing the genId of B's sample type through folder A must 403. The request-container DesignSampleType + // check passes in A, but ensureMinGenId operates on the type's OWN container, so the per-object check rejects it. + ActionURL foreignUrl = new ActionURL(SetEntitySequenceAction.class, folderA) + .addParameter("kindName", SampleTypeDomainKind.NAME) + .addParameter("seqType", NameGenerator.EntityCounter.genId.name()) + .addParameter("rowId", String.valueOf(rowId)) + .addParameter("newValue", "100"); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(foreignUrl, adminA)); + + // Positive control: a folder admin in B can advance the sequence through folder B (success, 200). + User adminB = createUserInRole(folderB, FolderAdminRole.class); + ActionURL ownUrl = new ActionURL(SetEntitySequenceAction.class, folderB) + .addParameter("kindName", SampleTypeDomainKind.NAME) + .addParameter("seqType", NameGenerator.EntityCounter.genId.name()) + .addParameter("rowId", String.valueOf(rowId)) + .addParameter("newValue", "100"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, adminB)); + } + + @Test + public void testDeriveActionMaterialContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // Editor in folder A only: holds the InsertPermission the action requires in A, but has no rights in folder B. + User editorA = createUserInRole(folderA, EditorRole.class); + + // A sample type with one sample in each folder. The editor can read its own folder A but not folder B. + ExpSampleType stA = createSampleType(folderA, "DeriveScopeStA"); + ExpSampleType stB = createSampleType(folderB, "DeriveScopeStB"); + ExpMaterial sampleA = createSample(folderA, stA, "srcA"); + ExpMaterial sampleB = createSample(folderB, stB, "srcB"); + + ActionURL url = new ActionURL(DeriveAction.class, folderA); + + // Negative (input): a material input resolved by global rowId that lives in folder B must not be usable as a + // derivation parent by a caller who cannot read B. Without the per-input Read check the foreign sample would + // be silently consumed (an IDOR). The output target is in folder A, so only the foreign input can be at fault. + JSONObject foreignInput = new JSONObject() + .put("materialInputs", new JSONArray().put(new JSONObject().put("rowId", sampleB.getRowId()))) + .put("targetSampleType", stA.getLSID()) + .put("materialOutputCount", 1); + MockHttpServletResponse resp = postJson(url, editorA, foreignInput); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a material-input scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Material input couldn't be resolved")); + + // Negative (target): deriving INTO a sample type the caller cannot read must be rejected as "not found", so + // the caller can't probe which foreign sample types exist by their LSID. + JSONObject foreignTarget = new JSONObject() + .put("targetSampleType", stB.getLSID()) + .put("materialOutputCount", 1); + resp = postJson(url, editorA, foreignTarget); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a target sample type scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Sample type not found")); + + // Positive control: the same request shape with the input and target both in folder A -- which the editor + // can read and insert into -- succeeds and actually derives a new sample, proving the checks reject only the + // cross-container case rather than every request. + JSONObject ok = new JSONObject() + .put("materialInputs", new JSONArray().put(new JSONObject().put("rowId", sampleA.getRowId()))) + .put("targetSampleType", stA.getLSID()) + .put("materialOutputs", new JSONArray().put(new JSONObject().put("values", new JSONObject().put("name", "derivedA")))); + resp = postJson(url, editorA, ok); + assertStatus(HttpServletResponse.SC_OK, resp); + assertTrue("Derivation should report success, was: " + resp.getContentAsString(), + new JSONObject(resp.getContentAsString()).getBoolean("success")); + assertNotNull("A new sample should have been derived in folder A", stA.getSample(folderA, "derivedA")); + } + + @Test + public void testDeriveActionDataContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // Editor in folder A only: holds the InsertPermission the action requires in A, but has no rights in folder B. + User editorA = createUserInRole(folderA, EditorRole.class); + + // A data class with one data object in each folder. The editor can read its own folder A but not folder B. + ExpDataClass dcA = createDataClass(folderA, "DeriveScopeDcA"); + ExpDataClass dcB = createDataClass(folderB, "DeriveScopeDcB"); + ExpData dataA = createData(folderA, dcA, "srcDataA"); + ExpData dataB = createData(folderB, dcB, "srcDataB"); + + ActionURL url = new ActionURL(DeriveAction.class, folderA); + + // Negative (input): a data input resolved by global rowId that lives in folder B must not be usable as a + // derivation parent by a caller who cannot read B. The Read check fires before the data-class membership + // check, so a foreign data object is rejected as unresolvable rather than silently consumed (an IDOR). + JSONObject foreignInput = new JSONObject() + .put("dataInputs", new JSONArray().put(new JSONObject().put("rowId", dataB.getRowId()))) + .put("targetDataClass", dcA.getLSID()) + .put("dataOutputCount", 1); + MockHttpServletResponse resp = postJson(url, editorA, foreignInput); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a data-input scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("Data input couldn't be resolved")); + + // Negative (target): deriving INTO a data class the caller cannot read must be rejected as "not found", so + // the caller can't probe which foreign data classes exist by their LSID. + JSONObject foreignTarget = new JSONObject() + .put("targetDataClass", dcB.getLSID()) + .put("dataOutputCount", 1); + resp = postJson(url, editorA, foreignTarget); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, resp); + assertTrue("Expected a target data class scope rejection, was: " + resp.getContentAsString(), + resp.getContentAsString().contains("DataClass not found")); + + // Positive control: the same request shape with the input and target both in folder A -- which the editor + // can read and insert into -- succeeds and actually derives a new data object, proving the checks reject + // only the cross-container case rather than every request. + JSONObject ok = new JSONObject() + .put("dataInputs", new JSONArray().put(new JSONObject().put("rowId", dataA.getRowId()))) + .put("targetDataClass", dcA.getLSID()) + .put("dataOutputs", new JSONArray().put(new JSONObject().put("values", new JSONObject().put("name", "derivedDataA")))); + resp = postJson(url, editorA, ok); + assertStatus(HttpServletResponse.SC_OK, resp); + assertTrue("Derivation should report success, was: " + resp.getContentAsString(), + new JSONObject(resp.getContentAsString()).getBoolean("success")); + assertNotNull("A new data object should have been derived in folder A", ExperimentService.get().getExpData(dcA, "derivedDataA")); + } + + // Create a sample type with a single string "name" property, mirroring the idiom in testSetEntitySequenceContainerScoping. + private ExpSampleType createSampleType(Container c, String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("name", "string")); + return SampleTypeService.get().createSampleType(c, getAdmin(), name, null, props, Collections.emptyList(), -1, -1, -1, -1, null, null); + } + + // Create a saved sample in the given sample type, mirroring the sample-creation idiom in LineageTest. + private ExpMaterial createSample(Container c, ExpSampleType st, String name) throws Exception + { + ExpMaterial m = ExperimentService.get().createExpMaterial(c, st.generateSampleLSID().setObjectId(name).toString(), name); + m.setCpasType(st.getLSID()); + m.save(getAdmin()); + return m; + } + + // Create a data class with a single string "name" property, mirroring the idiom in LineageTest. + private ExpDataClass createDataClass(Container c, String name) throws Exception + { + List props = List.of(new GWTPropertyDescriptor("name", "string")); + return ExperimentServiceImpl.get().createDataClass(c, getAdmin(), name, null, props, Collections.emptyList(), null, null); + } + + // Insert a single named row into the data class and return the resulting ExpData. + private ExpData createData(Container c, ExpDataClass dc, String name) throws Exception + { + UserSchema dataSchema = new ExpSchema(getAdmin(), c).getUserSchema(ExpSchema.NestedSchemas.data.name()); + BatchValidationException errors = new BatchValidationException(); + dataSchema.getTable(dc.getName()).getUpdateService() + .insertRows(getAdmin(), c, List.of(CaseInsensitiveHashMap.of("name", name)), errors, null, null); + if (errors.hasErrors()) + throw errors; + return ExperimentService.get().getExpData(dc, name); + } + + // Create a minimal saved experiment run in the given container, mirroring the run-creation idiom in LineageTest. + private ExpRun createRun(Container c, String name) throws Exception + { + ExpRun run = ExperimentService.get().createExperimentRun(c, name); + run.setFilePathRoot(PipelineService.get().findPipelineRoot(c).getRootPath()); + run.setProtocol(ExperimentService.get().ensureSampleDerivationProtocol(getAdmin())); + return ExperimentService.get().saveSimpleExperimentRun(run, Map.of(), Map.of(), Map.of(), Map.of(), Map.of(), + new ViewBackgroundInfo(c, getAdmin(), null), null, false); + } } } diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index d5b8579530c..3d4e59cbc23 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -575,22 +575,6 @@ public void validateCommand(ServerInstallationForm target, Errors errors) @Override public boolean handlePost(ServerInstallationForm form, BindException errors) throws Exception { - // Confirm the row belongs to the current container - Object pk = form.getPkVal(); - if (pk == null) - throw new NotFoundException("No server installation specified"); - int installationId; - try - { - installationId = Integer.parseInt(String.valueOf(pk)); - } - catch (NumberFormatException e) - { - throw new NotFoundException("Invalid server installation id: " + pk); - } - if (MothershipManager.get().getServerInstallation(installationId, getContainer()) == null) - throw new NotFoundException("Server installation not found in this folder"); - form.doUpdate(); return true; } @@ -2030,6 +2014,43 @@ public void testUpdateInstallationContainerScoping() throws Exception assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); assertEquals("Note should have been updated through the row's own container", "updated", MothershipManager.get().getServerInstallation(id, folderB).getNote()); } + + @Test + public void testUpdateStackTraceContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // An exception stack trace that lives in folder B (StackTraceHash is derived from the stack trace text) + ExceptionStackTrace st = new ExceptionStackTrace(); + st.setContainer(folderB.getId()); + st.setStackTrace("java.lang.NullPointerException\n\tat org.labkey.scoping.Test.run(Test.java:1)\n"); + st.setComments("original"); + st = Table.insert(admin, MothershipManager.get().getTableInfoExceptionStackTrace(), st); + int id = st.getExceptionStackTraceId(); + + // Updating it through folder A must 404 rather than overwrite/re-home it. doUpdate() keys Table.update on + // the id alone and rewrites the container, so without the handlePost guard a site admin (who CAN update + // folder B) would edit B's row through folder A and re-home it. + ActionURL url = new ActionURL(UpdateStackTraceAction.class, folderA) + .addParameter("ExceptionStackTraceId", String.valueOf(id)) + .addParameter("Comments", "hacked"); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, admin)); + + // The row in folder B must be untouched + ExceptionStackTrace reloaded = MothershipManager.get().getExceptionStackTrace(id, folderB); + assertNotNull("Stack trace should still exist in its own container", reloaded); + assertEquals("Comments must not have been overwritten", "original", reloaded.getComments()); + + // Positive control: updating through the row's own container (folder B) succeeds and persists the change, + // proving the guard rejects only the cross-container case, not every update. + ActionURL ownUrl = new ActionURL(UpdateStackTraceAction.class, folderB) + .addParameter("ExceptionStackTraceId", String.valueOf(id)) + .addParameter("Comments", "updated"); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, admin)); + assertEquals("Comments should have been updated through the row's own container", "updated", MothershipManager.get().getExceptionStackTrace(id, folderB).getComments()); + } } } diff --git a/pipeline/src/org/labkey/pipeline/PipelineController.java b/pipeline/src/org/labkey/pipeline/PipelineController.java index bb3f466f0c5..73269af8da1 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineController.java +++ b/pipeline/src/org/labkey/pipeline/PipelineController.java @@ -15,9 +15,11 @@ */ package org.labkey.pipeline; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.ApiJsonForm; @@ -46,6 +48,7 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.Table; import org.labkey.api.data.TableSelector; import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.files.FileContentService; @@ -64,6 +67,7 @@ import org.labkey.api.pipeline.PipelineStatusUrls; import org.labkey.api.pipeline.PipelineUrls; import org.labkey.api.pipeline.browse.PipelinePathForm; +import org.labkey.api.pipeline.file.FileAnalysisTaskPipeline; import org.labkey.api.pipeline.view.SetupForm; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryUrls; @@ -77,11 +81,13 @@ import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AbstractActionPermissionTest; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UserManagementPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.settings.AdminConsole; @@ -89,6 +95,7 @@ import org.labkey.api.trigger.TriggerConfiguration; import org.labkey.api.util.DateUtil; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlStringBuilder; import org.labkey.api.util.LinkBuilder; import org.labkey.api.util.NetworkDrive; @@ -117,6 +124,7 @@ import org.labkey.pipeline.api.PipelineStatusManager; import org.labkey.pipeline.status.StatusController; import org.springframework.beans.MutablePropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -1551,6 +1559,15 @@ public static class SavePipelineTriggerAction extends MutatingApiAction getIntegrationTests() PipelineServiceImpl.TestCase.class, StatusController.TestCase.class, StatusController.ContainerScopingTestCase.class, + PipelineController.ContainerScopingTestCase.class, ClusterStartup.TestCase.class ); } diff --git a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java index e0b34abe367..78dc459a7c0 100644 --- a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java +++ b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java @@ -690,10 +690,13 @@ public static void completeStatus(User user, Collection rowIds) if (!statusSet) { - // Fall back to updating the simple bean in the case where can can't deserialize the job itself + // Fall back to updating the simple bean in the case where we can't deserialize the job itself PipelineStatusFileImpl sf = PipelineStatusManager.getStatusFile(rowId); if (sf != null) { + Container c = sf.lookupContainer(); + if (c == null || !c.hasPermission(user, UpdatePermission.class)) + throw new UnauthorizedException(); LOG.info("Job " + sf.getFilePath() + " was marked as complete by " + user); sf.setStatus(PipelineJob.TaskStatus.complete.toString()); sf.setInfo(null); diff --git a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java index 009a03b5061..d5b33d4ac72 100644 --- a/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java +++ b/pipeline/src/org/labkey/pipeline/query/TriggerConfigurationsTable.java @@ -320,7 +320,10 @@ protected Map deleteRow(User user, Container container, Map deleteRow = super.deleteRow(user, container, oldRowMap); // call the stop() method for this config if it was successfully deleted diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 94965407a05..1e5050799a8 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -60,9 +60,11 @@ import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; import org.labkey.api.util.NetworkDrive; import org.labkey.api.util.PageFlowUtil; @@ -97,6 +99,7 @@ import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.util.Date; +import java.util.List; import java.util.Set; import java.util.TreeSet; @@ -397,8 +400,12 @@ public ModelAndView getView(RowIdForm form, BindException errors) if (!getContainer().equals(_statusFile.lookupContainer())) { + Container target = _statusFile.lookupContainer(); + // Only redirect if the user can read the job's container; otherwise don't reveal that it exists + if (target == null || !target.hasPermission(getUser(), ReadPermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); ActionURL url = getViewContext().cloneActionURL(); - url.setContainer(_statusFile.lookupContainer()); + url.setContainer(target); throw new RedirectException(url); } @@ -494,15 +501,20 @@ public ActionURL getRedirectURL(RowIdForm form) Container c = getContainerCheckAdmin(); PipelineStatusFile sf = getStatusFile(form.getRowId()); + if (sf == null) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + // Only navigate to the job's container if the user can read it + if (sfContainer == null || !sfContainer.hasPermission(getUser(), ReadPermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); if (sf.getDataUrl() != null) { throw new RedirectException(sf.getDataUrl()); } - return urlDetails(c, form.getRowId()); + return urlDetails(sfContainer, form.getRowId()); } } @@ -517,8 +529,12 @@ public ActionURL getRedirectURL(RowIdForm form) if (c == null || c.isRoot()) { PipelineStatusFileImpl sf = getStatusFile(form.getRowId()); - if (sf.getContainerId() != null) - c = ContainerManager.getForId(sf.getContainerId()); + if (sf == null) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + // Only navigate to the job's container if the user can read it; otherwise don't reveal it exists + if (sfContainer != null && sfContainer.hasPermission(getUser(), ReadPermission.class)) + c = sfContainer; } if (c != null) @@ -550,7 +566,8 @@ public void render(ShowFileForm form, BindException errors, PrintWriter out) thr String fileName; PipelineStatusFile sf = getStatusFile(form.getRowId()); - if (sf != null) + // Resolved by global rowId; only serve files for a job that belongs to the current container + if (sf != null && getContainer().equals(sf.lookupContainer())) { fileName = form.getFilename(); @@ -924,8 +941,12 @@ public boolean handlePost(RowIdForm form, BindException errors) for (Integer rowId : rowIds) { var sf = getStatusFile(rowId); + // Resolved by global rowId; reject a job that belongs to another container if (sf == null) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); + Container sfContainer = sf.lookupContainer(); + if (sfContainer == null || !sfContainer.hasPermission(getUser(), UpdatePermission.class)) + throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); if (firstDetailsURL == null) firstDetailsURL = urlDetails(sf); @@ -1094,13 +1115,7 @@ public void testStatusDetailsContainerScoping() throws Exception Container folderB = createContainer("B"); User readerA = createUserInRole(folderA, ReaderRole.class); - // A status file that lives in folder B. FilePath is a required column; point it at a non-existent log so - // StatusDetailsBean skips reading it (it only reads when the file exists) without affecting the scoping check. - PipelineStatusFileImpl sf = new PipelineStatusFileImpl(); - sf.beforeInsert(admin, folderB.getId()); - sf.setStatus(PipelineJob.TaskStatus.complete.toString()); - sf.setFilePath(FileUtil.appendName(FileUtil.getTempDirectory(), "pipeline-scoping-test-" + folderB.getRowId() + ".log").getAbsolutePath()); - sf = Table.insert(admin, PipelineSchema.getInstance().getTableInfoStatusFiles(), sf); + PipelineStatusFileImpl sf = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()); long rowId = sf.getRowId(); ActionURL foreignUrl = new ActionURL(StatusDetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); @@ -1117,5 +1132,115 @@ public void testStatusDetailsContainerScoping() throws Exception ActionURL ownUrl = new ActionURL(StatusDetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } + + @Test + public void testRetryStatusContainerScoping() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A job that lives in folder B + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.error.toString()).getRowId(); + + // A caller who can Update folder A (Editor) but has no rights in folder B + User editorA = createUserInRole(folderA, EditorRole.class); + + // Retrying B's job through folder A must 404 rather than re-queue a job in a container the caller can't touch. + ActionURL foreignUrl = new ActionURL(RetryStatusAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, editorA)); + + // The job must be untouched in its own container + assertEquals("Job status must be unchanged after a cross-container retry", + PipelineJob.TaskStatus.error.toString(), getStatusFile((int) rowId).getStatus()); + + // Positive control (a real successful retry needs a serialized job) is covered by existing pipeline retry tests. + } + + @Test + public void testShowDataContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + + // Addressing B's job through folder A must 404 rather than redirect to / expose it + ActionURL foreignUrl = new ActionURL(ShowDataAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + // A site admin who CAN read folder B gets a redirect + assertStatus(HttpServletResponse.SC_FOUND, get(foreignUrl, admin)); + + // Positive control: through its own container the action redirects (302) rather than 404 + ActionURL ownUrl = new ActionURL(ShowDataAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_FOUND, get(ownUrl, admin)); + } + + @Test + public void testCompleteStatusContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + // A job in folder B with no serialized job store, so completeStatus() takes its fallback (bean update) path + int rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.error.toString()).getRowId(); + + // A caller with no rights in folder B must not be able to mark B's job complete via the fallback path + try + { + completeStatus(readerA, List.of(rowId)); + fail("Expected UnauthorizedException marking a job complete in a container the caller can't update"); + } + catch (UnauthorizedException ignored) {} + assertEquals("Status must be unchanged after an unauthorized complete", + PipelineJob.TaskStatus.error.toString(), getStatusFile(rowId).getStatus()); + + // Positive control: a site admin (Update in B) can complete it through the same fallback path + completeStatus(admin, List.of(rowId)); + assertEquals("Admin should be able to mark the job complete", + PipelineJob.TaskStatus.complete.toString(), getStatusFile(rowId).getStatus()); + } + + @Test + public void testDetailsContainerScoping() throws Exception + { + // DetailsAction is the HTML job-details page (distinct from the StatusDetailsAction API). It resolves the + // job by global rowId; without the guard a caller could view another folder's job details through their own + // folder. The guard 404s when the caller cannot read the job's container, and otherwise redirects to it. + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + + ActionURL foreignUrl = new ActionURL(DetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); + // A caller who can read folder A but NOT folder B must get 404 -- the page must not reveal B's job exists. + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + // A site admin who CAN read folder B is redirected (302) to B rather than rendering B's job through A. + assertStatus(HttpServletResponse.SC_FOUND, get(foreignUrl, admin)); + + // Positive control: through its own container the details page renders (200), proving the guard rejects + // only the cross-container case rather than every request. + ActionURL ownUrl = new ActionURL(DetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + } + + // Insert a bare status file in the given container. FilePath is a required column; point it at a non-existent + // log so nothing tries to read it, and so getJobStore().getJob() returns null (exercising the bean-only paths). + private PipelineStatusFileImpl insertStatusFile(Container c, String status) + { + User admin = getAdmin(); + PipelineStatusFileImpl sf = new PipelineStatusFileImpl(); + sf.beforeInsert(admin, c.getId()); + sf.setStatus(status); + // uq_statusfiles_filepath is a global unique constraint, so the path must be unique per run -- a GUID keeps + // a leftover row from an interrupted prior run from colliding with this insert. + sf.setFilePath(FileUtil.appendName(FileUtil.getTempDirectory(), "pipeline-scoping-test-" + c.getRowId() + "-" + status + "-" + GUID.makeGUID() + ".log").getAbsolutePath()); + return Table.insert(admin, PipelineSchema.getInstance().getTableInfoStatusFiles(), sf); + } } } diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index ada49cf15ba..f0e028f724f 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -353,6 +353,7 @@ public Set getIntegrationTests() return Set.of( ModuleReportCache.TestCase.class, OlapController.TestCase.class, + OlapController.ContainerScopingTestCase.class, QueryController.TestCase.class, QueryController.SaveRowsTestCase.class, QueryServiceImpl.TestCase.class, diff --git a/query/src/org/labkey/query/controllers/OlapController.java b/query/src/org/labkey/query/controllers/OlapController.java index eac4835c06f..382c3557716 100644 --- a/query/src/org/labkey/query/controllers/OlapController.java +++ b/query/src/org/labkey/query/controllers/OlapController.java @@ -23,6 +23,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.junit.Test; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; @@ -56,8 +57,11 @@ import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.Table; import org.labkey.api.data.queryprofiler.QueryProfiler; import org.labkey.api.query.DefaultSchema; +import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.TableSelector; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryParseException; import org.labkey.api.query.QueryParseExceptionUnresolvedField; @@ -67,8 +71,10 @@ import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractActionPermissionTest; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.study.DataspaceContainerFilter; import org.labkey.api.util.Compress; import org.labkey.api.util.ConfigurationException; @@ -463,6 +469,21 @@ protected DataView createView(CustomOlapDescriptorForm form, Errors errors) return new UpdateView(form, (BindException)errors); } + @Override + public void validateCommand(CustomOlapDescriptorForm form, Errors errors) + { + // Confirm definition lives in this container before touching it + Object pk = form.getPkVal(); + if (pk == null) + throw new NotFoundException("Custom olap definition not found"); + SimpleFilter filter = SimpleFilter.createContainerFilter(getContainer()); + filter.addCondition(FieldKey.fromParts("RowId"), pk); + if (!new TableSelector(QueryManager.get().getTableInfoOlapDef(), filter, null).exists()) + throw new NotFoundException("Custom olap definition not found in this folder"); + + super.validateCommand(form, errors); + } + @Override protected boolean doAction(CustomOlapDescriptorForm form, Errors errors) throws SQLException { @@ -1595,4 +1616,50 @@ controller.new ListAppsAction() ); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testEditDefinitionContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A custom OLAP definition that lives in folder B. Its stored content need not be valid Rolap: the update + // action validates only the *submitted* form, and the container guard runs before that validation. + OlapDef def = new OlapDef(); + def.beforeInsert(admin, folderB.getId()); + def.setName("scoping-test-cube"); + def.setModule("query"); + def.setDefinition(""); + def = Table.insert(admin, QueryManager.get().getTableInfoOlapDef(), def); + int rowId = def.getRowId(); + + // A caller who is folder admin in A (so the @RequiresPermission(AdminPermission.class) check passes) but + // has no rights in folder B. + User adminA = createUserInRole(folderA, FolderAdminRole.class); + + // Editing B's definition through folder A must 404 rather than overwrite/re-home it. Without the + // validateCommand guard the unscoped doUpdate() would edit B's row and rewrite its container to A. + ActionURL foreignUrl = new ActionURL(EditDefinitionAction.class, folderA).addParameter("RowId", String.valueOf(rowId)); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, adminA)); + // A site admin, who CAN administer folder B, still gets 404 through folder A (no cross-container write). + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(foreignUrl, admin)); + + // The definition must be untouched in its own container. + OlapDef after = new TableSelector(QueryManager.get().getTableInfoOlapDef()).getObject(rowId, OlapDef.class); + assertNotNull("Definition must still exist", after); + assertEquals("Container must be unchanged after a cross-container edit", folderB.getId(), after.getContainerId()); + + // Positive control: a same-container request passes the container guard and proceeds to form validation. The + // submitted definition is intentionally invalid Rolap, so the action redisplays the form (200) instead of + // 404 -- proving the guard rejects only the cross-container case, not legitimate same-container edits. + ActionURL ownUrl = new ActionURL(EditDefinitionAction.class, folderB) + .addParameter("RowId", String.valueOf(rowId)) + .addParameter("Module", "query") + .addParameter("Definition", "not-valid-rolap-xml"); + assertStatus(HttpServletResponse.SC_OK, post(ownUrl, admin)); + } + } } diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java b/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java index e9a7dd6dd86..2621a19e267 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenApiController.java @@ -286,6 +286,7 @@ public class GetRequestAction extends ReadOnlyApiAction @Override public ApiResponse execute(RequestIdForm requestIdForm, BindException errors) { + // OK for anyone with read access to see any request in this container, even if they didn't create it SpecimenRequest request = getRequest(getUser(), getContainer(), requestIdForm.getRequestId(), false, false); final Map response = new HashMap<>(); response.put("request", request != null ? getRequestResponse(getViewContext(), request) : null); diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index 9d48a30e7c7..059d63a9fcf 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -63,6 +63,7 @@ import org.labkey.api.gwt.client.AuditBehaviorType; import org.labkey.api.module.FolderType; import org.labkey.api.module.Module; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.pipeline.PipeRoot; import org.labkey.api.pipeline.PipelineService; import org.labkey.api.pipeline.PipelineStatusUrls; @@ -87,6 +88,7 @@ import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.specimen.SpecimenQuerySchema; import org.labkey.api.specimen.SpecimenSchema; import org.labkey.api.specimen.Vial; @@ -151,8 +153,10 @@ import org.labkey.specimen.RequestedSpecimens; import org.labkey.specimen.SpecimenManager; import org.labkey.specimen.SpecimenRequestException; +import org.labkey.specimen.SpecimenModule; import org.labkey.specimen.SpecimenRequestManager; import org.labkey.specimen.SpecimenRequestStatus; +import org.labkey.specimen.security.roles.SpecimenRequesterRole; import org.labkey.specimen.importer.RequestabilityManager; import org.labkey.specimen.importer.SimpleSpecimenImporter; import org.labkey.specimen.model.ExtendedSpecimenRequestView; @@ -3801,6 +3805,8 @@ public boolean handlePost(final ManageRequestStatusForm form, BindException erro if (_specimenRequest == null) throw new NotFoundException(); + requiresEditRequestPermissions(_specimenRequest); + boolean statusChanged = form.getStatus() != _specimenRequest.getStatusId(); boolean detailsChanged = !nullSafeEqual(form.getRequestDescription(), _specimenRequest.getComments()); @@ -5849,5 +5855,77 @@ public void testDeleteRequirementActionRejectsForeignRequirement() throws Except post(ownUrl, getAdmin()); assertNull("Same-container delete should remove the requirement", provider.getRequirement(folderA, requirementId)); } + + @Test + public void testDeleteActorActionRejectsForeignActor() throws Exception + { + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + SpecimenRequestRequirementProvider provider = SpecimenRequestRequirementProvider.get(); + + // An actor that lives in folder A + SpecimenRequestActor actor = createActor(folderA, "Delete actor scoping actor"); + int actorId = actor.getRowId(); + + // Deleting through folder B, where the actor does not live, must be a graceful no-op rather than a 500: + // the fix makes getActor container-scoped, so handlePost sees null and skips actor.delete() instead of + // NPEing. The action still redirects (302) and the actor must survive in its own folder. + ActionURL foreignUrl = new ActionURL(DeleteActorAction.class, folderB) + .addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_FOUND, post(foreignUrl, getAdmin())); + assertNotNull("Cross-container delete must not remove the actor", provider.getActor(folderA, actorId)); + + // Positive control: deleting through the actor's own folder removes it, proving the guard rejects only the + // cross-container case rather than every delete + ActionURL ownUrl = new ActionURL(DeleteActorAction.class, folderA) + .addParameter("id", String.valueOf(actorId)); + assertStatus(HttpServletResponse.SC_FOUND, post(ownUrl, getAdmin())); + assertNull("Same-container delete should remove the actor", provider.getActor(folderA, actorId)); + } + + @Test + public void testManageRequestStatusRequiresEditPermission() throws Exception + { + // ManageRequestStatusAction is annotated @RequiresPermission(RequestSpecimensPermission.class), which only + // proves the caller may make requests. However, a requester shouldn't be able to change ANOTHER user's request + // + // SpecimenRequesterRole (RequestSpecimensPermission only, no ManageRequestsPermission) is applicable only + // in a study folder that has the Specimen module, so stand up a minimal study. + Container folder = createContainer("ManageStatus"); + Set modules = new HashSet<>(folder.getActiveModules()); + modules.add(ModuleLoader.getInstance().getModule("study")); + modules.add(ModuleLoader.getInstance().getModule(SpecimenModule.NAME)); + folder.setActiveModules(modules, getAdmin()); + StudyService.get().createStudy(folder, getAdmin(), "Specimen scoping study", TimepointType.VISIT, true); + + // A request OWNED BY THE ADMIN (not the attacker), in a freshly created status + SpecimenRequestStatus status = new SpecimenRequestStatus(); + status.setContainer(folder); + status.setLabel("Manage status scoping status"); + status.setSortOrder(1); // non-null and >= 0 so the bean isn't treated as a system status + status = Table.insert(getAdmin(), SpecimenSchema.get().getTableInfoSampleRequestStatus(), status); + + SpecimenRequest request = new SpecimenRequest(); + request.setContainer(folder); + request.setStatusId(status.getRowId()); + request = SpecimenRequestManager.get().createRequest(getAdmin(), request, false); + int requestId = request.getRowId(); + + // A plain requester: holds RequestSpecimensPermission (so the action's annotation passes) but is neither a + // coordinator nor the request's owner, so hasEditRequestPermissions() is false. + User requester = createUserInRole(folder, SpecimenRequesterRole.class); + + // Mutating another user's request must be rejected at the per-request guard (403) rather than applied. The + // request id and a no-op status keep the form valid, so without the fix the action proceeds and does NOT 403. + ActionURL url = new ActionURL(ManageRequestStatusAction.class, folder) + .addParameter("id", String.valueOf(requestId)) + .addParameter("status", String.valueOf(status.getRowId())); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, requester)); + + // Positive control: an admin holds ManageRequestsPermission, so the same request passes the guard rather + // than being blocked at 403 -- proving the guard rejects only the unprivileged requester, not every caller. + assertNotEquals("An admin must pass the edit-request guard, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, post(url, getAdmin()).getStatus()); + } } } diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 322655a8487..44a8ddb7a11 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -125,6 +125,7 @@ import org.labkey.study.audit.ParticipantGroupAuditProvider; import org.labkey.study.audit.StudyAuditProvider; import org.labkey.study.controllers.CohortController; +import org.labkey.study.controllers.CreateChildStudyAction; import org.labkey.study.controllers.DatasetController; import org.labkey.study.controllers.ParticipantGroupController; import org.labkey.study.controllers.SharedStudyController; @@ -713,6 +714,7 @@ public Set getIntegrationTests() DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, DataStatesTest.class, ParticipantGroupManager.ParticipantGroupTestCase.class, + ParticipantGroupManager.ContainerScopingTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, StudyManager.VisitCreationTestCase.class, @@ -720,7 +722,9 @@ public Set getIntegrationTests() VisitImpl.TestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, - org.labkey.study.controllers.CreateChildStudyAction.ContainerScopingTestCase.class); + CreateChildStudyAction.ContainerScopingTestCase.class, + StudyController.ContainerScopingTestCase.class, + ReportsController.ContainerScopingTestCase.class); } @Override diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 6ca457fdf43..fe3cc9c94ae 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -163,7 +163,9 @@ import org.labkey.api.security.RequiresNoPermission; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.SecurityManager; +import org.junit.Test; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.BrowserDeveloperPermission; import org.labkey.api.security.permissions.DeletePermission; @@ -1588,7 +1590,8 @@ private void deleteFromParticipantGroupMapTable(TableInfo ti, String participant { try { - SQLFragment sql = new SQLFragment("DELETE FROM study.participantgroupmap WHERE participantid = ?", participantId); + // Scope the raw DELETE to the request container (DeletePermission is only proven there) + SQLFragment sql = new SQLFragment("DELETE FROM study.participantgroupmap WHERE participantid = ? AND container = ?", participantId, getContainer().getId()); new SqlExecutor(ti.getSchema()).execute(sql); } catch (Exception e) @@ -7899,4 +7902,54 @@ public void addNavTrail(NavTree root) root.addChild(_study != null ? "Overview: " + _study.getLabel() : "No Study"); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testDeleteParticipantContainerScoping() throws Exception + { + // DeleteParticipantAction removes a participant's dataset rows AND its participant-group memberships. The + // group-map DELETE was keyed only by participantId, so deleting a participant through folder A also wiped + // that participant's group memberships in OTHER folders. The fix scopes the DELETE to the request container. + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + StudyService.get().createStudy(folderA, getAdmin(), "Study A", TimepointType.VISIT, true); + StudyService.get().createStudy(folderB, getAdmin(), "Study B", TimepointType.VISIT, true); + + // P1 must be a known participant in folder B before it can be added to a group there + insertParticipant(folderB, "P1"); + + // Put P1 into a participant group in folder B (creates a study.participantgroupmap row scoped to B) + ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); + cat.setContainer(folderB.getId()); + cat.setLabel("scoping-category"); + cat.setType("list"); + ParticipantGroupManager.getInstance().setParticipantCategory(folderB, getAdmin(), cat, new String[]{"P1"}, null, "scoping"); + assertEquals("Setup: P1 must be in a group in folder B", 1, groupMapCount(folderB, "P1")); + + // Delete P1 through folder A. Folder A has its own study, so the action runs; without the fix its group-map + // DELETE (keyed only by participantId) would also remove P1's membership in folder B. + ActionURL url = new ActionURL(DeleteParticipantAction.class, folderA).addParameter("participantId", "P1"); + post(url, getAdmin()); + + // P1's group membership in folder B must survive a delete issued through folder A. + assertEquals("P1's group membership in folder B must survive a cross-container participant delete", + 1, groupMapCount(folderB, "P1")); + } + + private void insertParticipant(Container c, String ptid) + { + Map row = new HashMap<>(); + row.put("Container", c.getId()); + row.put("ParticipantId", ptid); + Table.insert(getAdmin(), StudySchema.getInstance().getTableInfoParticipant(), row); + } + + private long groupMapCount(Container c, String ptid) + { + SimpleFilter f = new SimpleFilter(FieldKey.fromParts("ParticipantId"), ptid); + f.addCondition(FieldKey.fromParts("Container"), c.getId()); + return new TableSelector(StudySchema.getInstance().getTableInfoParticipantGroupMap(), f, null).getRowCount(); + } + } } diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index f2b1b06dae4..65f56b36497 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -43,6 +43,12 @@ import org.labkey.api.query.ValidationError; import org.labkey.api.reports.Report; import org.labkey.api.reports.ReportService; +import org.labkey.api.reports.report.QueryReport; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.writer.DefaultContainerUser; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.Test; import org.labkey.api.reports.report.ReportDescriptor; import org.labkey.api.reports.report.ReportIdentifier; import org.labkey.api.reports.report.ReportUrls; @@ -73,6 +79,7 @@ import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.VBox; import org.labkey.api.view.ViewContext; import org.labkey.api.view.ViewForm; @@ -317,6 +324,11 @@ public ModelAndView getView(ShowReportForm form, BindException errors) throws Ex throw new NotFoundException(message); } + // getReport(container, rowId) is container-scoped but does no owner/SecurityPolicy filtering. Enforce the + // per-report read check + if (!ReportManager.get().canReadReport(getUser(), getContainer(), report)) + throw new UnauthorizedException("You do not have permission to view this report."); + return report.renderReport(getViewContext()); } @@ -1291,6 +1303,35 @@ public void bindJson(JSONObject json) } } + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testShowReportRequiresReadPermission() throws Exception + { + // ShowReportAction resolves a report by global rowId with getReport(container, rowId) -- container-scoped, + // but with no owner/policy check. Without the fix a plain container Reader could render another user's + // PRIVATE report by guessing its rowId. The fix adds the per-report canReadReport() check. + Container folder = createContainer("A"); + + // A PRIVATE report: owned by the admin (descriptor owner set), so only the owner / a site admin may read it. + Report report = ReportService.get().createReportInstance(QueryReport.TYPE); + report.getDescriptor().setReportName("scoping-private-report"); + report.getDescriptor().setOwner(getAdmin().getUserId()); + int reportId = ReportService.get().saveReportEx(new DefaultContainerUser(folder, getAdmin()), "scoping-report-key", report).getRowId(); + + ActionURL url = new ActionURL(ShowReportAction.class, folder).addParameter("reportId", String.valueOf(reportId)); + + // A Reader who does not own the report must be rejected (403) rather than shown another user's private report. + User reader = createUserInRole(folder, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, get(url, reader)); + + // Positive control: the report's owner passes the per-report read check rather than being blocked at 403, + // proving the guard rejects only the unauthorized reader. + assertNotEquals("The report owner must pass the read check, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, get(url, getAdmin()).getStatus()); + } + } + public static class TestCase extends AbstractActionPermissionTest { @Override diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 945b07a8029..7a148b7210a 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -41,6 +41,8 @@ import org.labkey.api.query.ValidationError; import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.settings.ResourceURL; @@ -48,6 +50,7 @@ import org.labkey.api.study.ParticipantCategory; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; import org.labkey.api.study.model.ParticipantGroup; import org.labkey.api.study.permissions.SharedParticipantGroupPermission; import org.labkey.api.util.MemTracker; @@ -578,11 +581,10 @@ private ParticipantGroup _setParticipantGroup(Container c, User user, Participan if (cat == null) throw new ValidationException("The specified category was not found."); - if (cat.isShared()) - { - if (!c.hasPermission(user, SharedParticipantGroupPermission.class) && !c.hasPermission(user, AdminPermission.class)) - throw new ValidationException("You must be in the Editor role or an Admin to assign a group to a shared participant category"); - } + // canEdit enforces the SharedParticipantGroupPermission/Admin check for shared categories AND the + // owner check for private categories + if (!cat.canEdit(c, user)) + throw new ValidationException("You do not have permission to modify groups in this participant category"); } ParticipantGroup ret; @@ -1176,4 +1178,56 @@ public void test() p.getParticipantGroups(null, null, def); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSetParticipantGroupRequiresOwnership() throws Exception + { + // setParticipantGroup() saves a group keyed by a global rowId. For a PRIVATE category, canEdit() allows only + // the category's creator -- but the manager previously enforced only the shared-category case, so a Read + // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup + // on canEdit() for the private case too. + Container folder = createContainer("A"); + StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); + insertParticipant(folder, "P1"); + + ParticipantGroupManager mgr = ParticipantGroupManager.getInstance(); + + // A PRIVATE participant category + group owned by the admin (ownerId != OWNER_SHARED makes it private; the + // admin is its creator, so only the admin may edit it). + ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); + cat.setContainer(folder.getId()); + cat.setLabel("private-category"); + cat.setType("list"); + cat.setOwnerId(getAdmin().getUserId()); + cat = mgr.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); + ParticipantGroup group = mgr.getParticipantGroups(folder, getAdmin(), cat).get(0); + + // A different user with only Read access (not the owner, not an admin) + User attacker = createUserInRole(folder, ReaderRole.class); + + // Saving (overwriting) the admin's private group as the attacker must be rejected. + try + { + mgr.setParticipantGroup(folder, attacker, group); + fail("A non-owner must not be able to modify another user's private participant group"); + } + catch (ValidationException expected) + { + } + + // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the + // non-owner, not every caller. + mgr.setParticipantGroup(folder, getAdmin(), group); + } + + private void insertParticipant(Container c, String ptid) + { + Map row = new HashMap<>(); + row.put("Container", c.getId()); + row.put("ParticipantId", ptid); + Table.insert(getAdmin(), StudySchema.getInstance().getTableInfoParticipant(), row); + } + } } diff --git a/studydesign/src/org/labkey/studydesign/StudyDesignController.java b/studydesign/src/org/labkey/studydesign/StudyDesignController.java index 12b5224afff..18122fcbe5e 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignController.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignController.java @@ -17,6 +17,7 @@ import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.Nullable; +import org.junit.Test; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.ApiJsonForm; @@ -31,11 +32,13 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; +import org.labkey.api.data.TableSelector; import org.labkey.api.query.FieldKey; import org.labkey.api.query.ValidationException; import org.labkey.api.security.ActionNames; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.study.Cohort; @@ -51,6 +54,7 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; +import org.labkey.api.view.NotFoundException; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.studydesign.model.AssaySpecimenConfigImpl; @@ -946,4 +950,52 @@ private void updateAssayPlan(User user, Study study, String assayPlan) StudyService.get().updateAssayPlan(user, study, assayPlan); } } + + /** + * Verifies the cross-container guard on the DoseAndRoute save path reached by UpdateStudyProductsAction. That + * action's DoseAndRoute branch alone writes through the raw storage table keyed by a client-supplied RowId, so the + * guard lives in TreatmentManager.saveStudyProductDoseAndRoute and is exercised directly here (building a full study + * + product + JSON product payload to drive the action end-to-end would add a large fixture for the same assertion). + */ + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + @Test + public void testSaveDoseAndRouteContainerScoping() + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + + // A DoseAndRoute row that lives in folder B + DoseAndRoute existing = Table.insert(admin, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), + new DoseAndRoute("1 mg", "IV", 1, folderB)); + int rowId = existing.getRowId(); + + // Deny: an update keyed by B's RowId while operating in folder A must be rejected, not silently + // overwrite/re-home the foreign row. This is the path an Editor in folder A reaches via + // UpdateStudyProductsAction by submitting a products[].DoseAndRoute[].RowId owned by folder B. + DoseAndRoute attack = new DoseAndRoute("HACKED", "HACKED", 1, folderA); + attack.setRowId(rowId); + try + { + TreatmentManager.getInstance().saveStudyProductDoseAndRoute(folderA, admin, attack); + fail("Expected NotFoundException updating a DoseAndRoute owned by another container"); + } + catch (NotFoundException ignored) {} + + // The row in folder B must be untouched + DoseAndRoute reloaded = new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute()) + .getObject(rowId, DoseAndRoute.class); + assertNotNull("DoseAndRoute must still exist", reloaded); + assertEquals("Dose must be unchanged after a cross-container update", "1 mg", reloaded.getDose()); + + // Positive control: updating through the row's own container succeeds and persists the change + DoseAndRoute ok = new DoseAndRoute("2 mg", "IM", 1, folderB); + ok.setRowId(rowId); + TreatmentManager.getInstance().saveStudyProductDoseAndRoute(folderB, admin, ok); + DoseAndRoute afterOk = new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute()) + .getObject(rowId, DoseAndRoute.class); + assertEquals("Dose should be updated by a same-container request", "2 mg", afterOk.getDose()); + } + } } \ No newline at end of file diff --git a/studydesign/src/org/labkey/studydesign/StudyDesignModule.java b/studydesign/src/org/labkey/studydesign/StudyDesignModule.java index eff8232833b..1de6480178a 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignModule.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignModule.java @@ -77,7 +77,8 @@ public Set getIntegrationTests() { return Set.of( TreatmentManager.TreatmentDataTestCase.class, - TreatmentManager.AssayScheduleTestCase.class + TreatmentManager.AssayScheduleTestCase.class, + StudyDesignController.ContainerScopingTestCase.class ); } } \ No newline at end of file diff --git a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java index 1c441aef779..71ec931600b 100644 --- a/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java +++ b/studydesign/src/org/labkey/studydesign/model/TreatmentManager.java @@ -33,6 +33,7 @@ import org.labkey.api.module.ModuleLoader; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.FieldKey; +import org.labkey.api.view.NotFoundException; import org.labkey.api.query.FilteredTable; import org.labkey.api.query.QueryService; import org.labkey.api.query.QueryUpdateService; @@ -54,7 +55,6 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.TestContext; -import org.labkey.api.view.NotFoundException; import java.math.BigDecimal; import java.util.ArrayList; @@ -366,16 +366,14 @@ public DoseAndRoute saveStudyProductDoseAndRoute(Container container, User user, { if (doseAndRoute.isNew()) return Table.insert(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute); - else - { - // GitHub Kanban #1929: verify the existing row is in this container before updating - SimpleFilter filter = SimpleFilter.createContainerFilter(container); - filter.addCondition(FieldKey.fromParts("RowId"), doseAndRoute.getRowId()); - if (!new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), filter, null).exists()) - throw new NotFoundException("No dose and route found for rowId: " + doseAndRoute.getRowId()); - return Table.update(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute, doseAndRoute.getRowId()); - } + // GitHub Kanban #1929: verify the existing row is in this container before updating + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromParts("RowId"), doseAndRoute.getRowId()); + if (!new TableSelector(StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), filter, null).exists()) + throw new NotFoundException("No dose and route found for rowId: " + doseAndRoute.getRowId()); + + return Table.update(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute, doseAndRoute.getRowId()); } public Collection getStudyProductsDoseAndRoute(Container container, User user, int productId) diff --git a/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index 66be29ef8b0..67c9338f582 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -63,6 +63,8 @@ import org.labkey.api.resource.Resource; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; import org.labkey.api.survey.model.SurveyListener; @@ -266,13 +268,16 @@ public Survey saveSurvey(Container container, User user, Survey survey) } } + /** Checks that the user has read permission to the container that owns the design, but we accept cross-container references */ @Nullable public SurveyDesign getSurveyDesign(Container container, User user, int surveyId) { - // Scope by container SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId); - filter.addCondition(FieldKey.fromParts("Container"), container); - return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); + SurveyDesign result = new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); + if (result == null) + return null; + Container actualContainer = ContainerManager.getForId(result.getContainerId()); + return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : result; } /** @@ -808,7 +813,7 @@ public void setUp() } @Test - public void testSurveyDesignContainerScoping() + public void testSurveyDesignContainerScoping() throws Exception { SurveyManager sm = SurveyManager.get(); @@ -817,9 +822,21 @@ public void testSurveyDesignContainerScoping() design = sm.saveSurveyDesign(_projectA, _user, design); int designId = design.getRowId(); - // Same-container lookup succeeds; cross-container lookup must return null - assertNotNull("Design should be visible from its own container", sm.getSurveyDesign(_projectA, _user, designId)); - assertNull("Design must NOT be visible from another container", sm.getSurveyDesign(_projectB, _user, designId)); + // 1. Same container: a user with read access in the design's container sees it. + User readerA = createUserInRole(_projectA, ReaderRole.class); + assertNotNull("Design should be visible from its own container to a user with read access", + sm.getSurveyDesign(_projectA, readerA, designId)); + + // 2. Different container, caller can read the design's container: tolerated, design is returned. + User readerAB = createUserInRole(_projectA, ReaderRole.class); + grantRole(readerAB, _projectB, ReaderRole.class); + assertNotNull("Design should be visible from another container when the caller can read the design's container", + sm.getSurveyDesign(_projectB, readerAB, designId)); + + // 3. Different container, caller cannot read the design's container: must return null. + User readerB = createUserInRole(_projectB, ReaderRole.class); + assertNull("Design must NOT be visible to a caller without read access to the design's container", + sm.getSurveyDesign(_projectB, readerB, designId)); // A delete issued from the wrong container must not remove the design sm.deleteSurveyDesign(_projectB, _user, designId, true); diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index 0525ecec9d7..3fed40cf90f 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -62,7 +62,9 @@ import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.FolderAdminRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.AdminConsole; import org.labkey.api.settings.AppProps; import org.labkey.api.util.GUID; @@ -2502,6 +2504,10 @@ public ApiResponse execute(AttachFilesForm form, BindException errors) if (null == wiki) throw new IllegalArgumentException("Could not find the wiki with entity id '" + form.getEntityId() + "'!"); + // Mutating a page's attachments requires UpdatePermission on the page + if (!getPermissions().allowUpdate(wiki)) + throw new UnauthorizedException("You do not have permission to modify this wiki page's attachments."); + if (!(getViewContext().getRequest() instanceof MultipartHttpServletRequest)) throw new IllegalArgumentException("You must use the 'multipart/form-data' mimetype when posting to attachFiles.api"); @@ -2932,29 +2938,66 @@ private WikiManager getWikiManager() public static class CopyWikiContainerScopingTestCase extends AbstractContainerScopingTest { + private Container _source; + private Container _dest; + + @Before + public void createWikiToCopy() + { + // Two folders and a wiki page that lives in the source. Each test makes a limited user an admin on exactly + // one of the folders to exercise a different side of CopyWikiAction's cross-container authorization. + _source = createContainer("Source"); + _dest = createContainer("Dest"); + WikiManager.get().insertWiki(getAdmin(), _source, "page", "body", WikiRendererType.HTML, "Page"); + } + @Test public void testCopyWikiRequiresSourceAdmin() throws Exception { - Container dest = createContainer("Dest"); - Container source = createContainer("Source"); + // Caller administers the destination only + assertCrossContainerCopyRejected(_dest); + } - // A user who is a folder admin in the destination only (no rights in the source) - User destAdminOnly = createUserInRole(dest, FolderAdminRole.class); + @Test + public void testCopyWikiRequiresDestAdmin() throws Exception + { + // Caller administers the source only + assertCrossContainerCopyRejected(_source); + } - // A wiki page that lives in the source folder - WikiManager.get().insertWiki(getAdmin(), source, "secretPage", "secret body", WikiRendererType.HTML, "Secret Page"); + // Drives a source->dest copy posted through callerAdminContainer an admin on BOTH folders must be + // accepted (redirect) and actually copy the page + private void assertCrossContainerCopyRejected(Container callerAdminContainer) throws Exception + { + User limitedAdmin = createUserInRole(callerAdminContainer, FolderAdminRole.class); + ActionURL url = new ActionURL(CopyWikiAction.class, callerAdminContainer) + .addParameter("sourceContainer", _source.getPath()) + .addParameter("destContainer", _dest.getPath()); - ActionURL url = new ActionURL(CopyWikiAction.class, dest) - .addParameter("sourceContainer", source.getPath()) - .addParameter("destContainer", dest.getPath()); - assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, destAdminOnly)); - assertTrue("No pages should have been copied into the destination", WikiSelectManager.getPageNames(dest).isEmpty()); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(url, limitedAdmin)); + assertTrue("No pages should have been copied into the destination", WikiSelectManager.getPageNames(_dest).isEmpty()); - // Positive control: the same copy by a user who is admin on BOTH folders is accepted (redirect to the - // destination) and actually copies the page, proving the guard rejects only the cross-container case - // rather than every copy. assertStatus(HttpServletResponse.SC_FOUND, post(url, getAdmin())); - assertFalse("Admin copy from a readable source should have copied the wiki page", WikiSelectManager.getPageNames(dest).isEmpty()); + assertFalse("An admin copy across both folders should have copied the wiki page", WikiSelectManager.getPageNames(_dest).isEmpty()); + } + + @Test + public void testAttachFilesRequiresUpdate() throws Exception + { + Container folder = createContainer("AttachFolder"); + WikiManager.get().insertWiki(getAdmin(), folder, "attachPage", "body", WikiRendererType.HTML, "Attach Page"); + String entityId = WikiSelectManager.getWiki(folder, "attachPage").getEntityId(); + + ActionURL url = new ActionURL(AttachFilesAction.class, folder).addParameter("entityId", entityId); + + // A Reader must not be able to delete/replace the page's attachments + User reader = createUserInRole(folder, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, reader)); + + // Positive control: an Editor passes the UpdatePermission guard. + User editor = createUserInRole(folder, EditorRole.class); + assertNotEquals("An editor must pass the attachment UpdatePermission guard, not be blocked at 403", + HttpServletResponse.SC_FORBIDDEN, post(url, editor).getStatus()); } } } From 6ef777f2c3188a8ba1fd46224e3df6cfd2881394 Mon Sep 17 00:00:00 2001 From: Nick Kerr Date: Wed, 17 Jun 2026 12:39:40 -0700 Subject: [PATCH 07/13] Study container scope (#7762) --- .../api/data/AbstractParticipantCategory.java | 60 ++-- study/src/org/labkey/study/StudyModule.java | 2 +- .../study/controllers/DatasetController.java | 64 ++++ .../ParticipantGroupController.java | 90 +++--- .../study/controllers/StudyController.java | 14 +- .../study/model/ParticipantGroupManager.java | 290 ++++++++++++------ 6 files changed, 326 insertions(+), 194 deletions(-) diff --git a/api/src/org/labkey/api/data/AbstractParticipantCategory.java b/api/src/org/labkey/api/data/AbstractParticipantCategory.java index 00ab7ad438b..35e40383e13 100644 --- a/api/src/org/labkey/api/data/AbstractParticipantCategory.java +++ b/api/src/org/labkey/api/data/AbstractParticipantCategory.java @@ -15,6 +15,7 @@ */ package org.labkey.api.data; +import org.jetbrains.annotations.NotNull; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.query.SimpleValidationError; @@ -226,18 +227,11 @@ public boolean canEdit(Container container, User user, List err { if (isNew()) return true; - else - { - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = - container.hasPermission(user, SharedParticipantGroupPermission.class) || - container.hasPermission(user, AdminPermission.class) || - (owner != null && !owner.isGuest() && owner.equals(user)); - - if (!allowed) - errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); - } + + if (!isOwner(user)) + errors.add(new SimpleValidationError("You must be the owner to unshare this participant category")); } + return errors.isEmpty(); } @@ -257,44 +251,28 @@ public boolean canDelete(Container container, User user, List e { if (isNew()) return true; - else - { - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; - if (!allowed) - errors.add(new SimpleValidationError("You must be the owner to delete this participant category")); - } + if (!isOwner(user)) + errors.add(new SimpleValidationError("You must be the owner to delete this participant category")); } + return errors.isEmpty(); } - public boolean canRead(Container c, User user) + public boolean canRead(@NotNull User user) { - return canRead(c, user, new ArrayList<>()); + if (isShared() || isNew()) + return true; + + // Issue 16645: Do not show participant groups that may have been created by guests, which was possible + // before this bug was fixed. When admins can update and delete private groups, we can make + // guest-created groups visible again. + return isOwner(user); } - public boolean canRead(Container c, User user, List errors) + private boolean isOwner(@NotNull User user) { - if (!isShared()) - { - if (isNew()) - return true; - else - { - // issue 16645 : don't show participant groups that may have been created by guests, which was possible - // before this bug was fixed. When admins have the ability to update and delete private groups we can - // make guest created groups visible again. - User owner = UserManager.getUser(getCreatedBy()); - boolean allowed = (owner != null && !owner.isGuest()) ? owner.equals(user) : false; - - if (!allowed) - { - errors.add(new SimpleValidationError("You don't have permission to read this private participant category")); - return false; - } - } - } - return true; + User owner = UserManager.getUser(getCreatedBy()); + return owner != null && !owner.isGuest() && owner.equals(user); } } diff --git a/study/src/org/labkey/study/StudyModule.java b/study/src/org/labkey/study/StudyModule.java index 44a8ddb7a11..98f7579ea8a 100644 --- a/study/src/org/labkey/study/StudyModule.java +++ b/study/src/org/labkey/study/StudyModule.java @@ -713,13 +713,13 @@ public Set getIntegrationTests() return Set.of( DatasetDefinition.TestCleanupOrphanedDatasetDomains.class, DataStatesTest.class, - ParticipantGroupManager.ParticipantGroupTestCase.class, ParticipantGroupManager.ContainerScopingTestCase.class, StudyImpl.ProtocolDocumentTestCase.class, StudyManager.StudySnapshotTestCase.class, StudyManager.VisitCreationTestCase.class, StudyModule.TestCase.class, VisitImpl.TestCase.class, + DatasetController.DatasetAuditHistoryScopingTestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, CreateChildStudyAction.ContainerScopingTestCase.class, diff --git a/study/src/org/labkey/study/controllers/DatasetController.java b/study/src/org/labkey/study/controllers/DatasetController.java index 394cdf23fc8..bddda0a3331 100644 --- a/study/src/org/labkey/study/controllers/DatasetController.java +++ b/study/src/org/labkey/study/controllers/DatasetController.java @@ -17,8 +17,11 @@ package org.labkey.study.controllers; import org.apache.commons.lang3.StringUtils; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.SimpleViewAction; +import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.audit.view.AuditChangesView; @@ -26,11 +29,15 @@ import org.labkey.api.data.DataRegion; import org.labkey.api.data.DbScope; import org.labkey.api.security.RequiresPermission; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.study.EditDatasetRowForm; import org.labkey.api.study.InsertUpdateAction; import org.labkey.api.study.Study; +import org.labkey.api.study.TimepointType; +import org.labkey.api.util.GUID; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HtmlView; @@ -42,6 +49,7 @@ import org.labkey.study.model.DatasetDefinition; import org.labkey.study.model.StudyImpl; import org.labkey.study.model.StudyManager; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -118,6 +126,8 @@ public ModelAndView getView(DatasetAuditHistoryForm form, BindException errors) VBox view = new VBox(); + // getAuditEvent() resolves against the current container's audit schema (default ContainerFilter.Current + + // CanSeeAuditLog clause), so a foreign auditRowId resolves to null and cannot disclose a record in another folder. DatasetAuditProvider.DatasetAuditEvent event = AuditLogService.get().getAuditEvent(getUser(), DatasetAuditProvider.DATASET_AUDIT_EVENT, auditRowId); if (event != null) { @@ -283,4 +293,58 @@ public static class DatasetAuditHistoryForm public void setAuditRowId(int auditRowId) {this.auditRowId = auditRowId;} } + + public static class DatasetAuditHistoryScopingTestCase extends AbstractContainerScopingTest + { + private static final String FIELD_VALUE = GUID.makeGUID(); + private Container _requestContainer; + private long _foreignAuditRowId; + + @Before + public void setup() + { + _requestContainer = createContainer("Request"); + StudyImpl study = new StudyImpl(_requestContainer, "Request Study"); + study.setTimepointType(TimepointType.VISIT); + StudyManager.getInstance().createTestStudy(getAdmin(), study); + + _foreignAuditRowId = addDatasetAuditEvent(createContainer("Event")); + } + + @Test + public void doesNotDiscloseForeignFolderDatasetAuditEvent() throws Exception + { + // Even the site auditor, who may see audit logs everywhere, must not be served another folder's dataset + // audit record when requesting it through this folder's URL: the lookup is scoped to the request container. + String content = requestAuditHistory(_foreignAuditRowId, getAdmin()).getContentAsString(); + + assertFalse("A foreign auditRowId must not disclose dataset audit record in another folder", content.contains(FIELD_VALUE)); + assertTrue("Should fall through to the 'no additional details' view", content.contains("No additional details recorded")); + } + + @Test + public void disclosesOwnFolderDatasetAuditEvent() throws Exception + { + // Control: an event in the request folder must be shown, proving the negative case isn't passing simply + // because the view never renders record data. + long localRowId = addDatasetAuditEvent(_requestContainer); + String content = requestAuditHistory(localRowId, getAdmin()).getContentAsString(); + assertTrue("An audit event in the request folder must be shown", content.contains(FIELD_VALUE)); + } + + private long addDatasetAuditEvent(Container c) + { + DatasetAuditProvider.DatasetAuditEvent event = new DatasetAuditProvider.DatasetAuditEvent(c, "test dataset audit", 1); + event.setNewRecordMap(AbstractAuditTypeProvider.encodeForDataMap(Map.of("SecretField", FIELD_VALUE))); + event = AuditLogService.get().addEvent(getAdmin(), event); + return event.getRowId(); + } + + private MockHttpServletResponse requestAuditHistory(long auditRowId, User user) throws Exception + { + ActionURL url = new ActionURL(DatasetAuditHistoryAction.class, _requestContainer) + .addParameter("auditRowId", auditRowId); + return get(url, user); + } + } } diff --git a/study/src/org/labkey/study/controllers/ParticipantGroupController.java b/study/src/org/labkey/study/controllers/ParticipantGroupController.java index b37e76997f2..ed5f2a7c16d 100644 --- a/study/src/org/labkey/study/controllers/ParticipantGroupController.java +++ b/study/src/org/labkey/study/controllers/ParticipantGroupController.java @@ -160,26 +160,21 @@ public static class UpdateParticipantCategory extends MutatingApiAction categories; if (form.getCategoryType() != null && form.getCategoryType().equals("manual")) { @@ -274,11 +269,10 @@ public ApiResponse execute(GetParticipantCategoriesForm form, BindException erro } JSONArray defs = new JSONArray(); - for (ParticipantCategoryImpl pc : categories) - { defs.put(pc.toJSON()); - } + + ApiSimpleResponse resp = new ApiSimpleResponse(); resp.put("success", true); resp.put("categories", defs); @@ -295,7 +289,6 @@ public ApiResponse execute(Object form, BindException errors) Container c = getContainer(); User u = getUser(); JSONArray jsonGroups = new JSONArray(); - ApiSimpleResponse resp = new ApiSimpleResponse(); for (ParticipantCategoryImpl category : ParticipantGroupManager.getInstance().getParticipantCategories(c, u)) { @@ -303,11 +296,12 @@ public ApiResponse execute(Object form, BindException errors) { if (group.hasLiveFilter()) { - jsonGroups.put(group.toJSON(false /*includeParticipants*/)); + jsonGroups.put(group.toJSON(false)); } } } + ApiSimpleResponse resp = new ApiSimpleResponse(); resp.put("success", true); resp.put("participantGroups", jsonGroups); return resp; @@ -537,7 +531,7 @@ public ApiResponse execute(BrowseGroupsForm form, BindException errors) { GroupType groupType = GroupType.valueOf(type); Set selectedParticipants = new HashSet<>(); - switch(groupType) + switch (groupType) { case participantGroup: // the api will support either requesting a specific participant category/group or all of @@ -545,7 +539,8 @@ public ApiResponse execute(BrowseGroupsForm form, BindException errors) if (form.getCategoryId() != -1) { ParticipantCategoryImpl category = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), form.getCategoryId()); - addCategory(form, category, groups); + if (category != null) + addCategory(form, category, groups); } else if (form.getGroupId() != -1) { @@ -558,6 +553,7 @@ else if (form.getGroupId() != -1) // NOTE: This is intentional as 'personal' groups are not secured and can be shared. group = ParticipantGroupManager.getInstance().getParticipantGroupFromGroupRowId(getContainer(), getUser(), form.getGroupId()); } + if (group != null) { ParticipantCategoryImpl category = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), group.getCategoryId()); @@ -620,11 +616,8 @@ private void addCategory(BrowseGroupsForm form, ParticipantCategoryImpl category { if (form.isIncludePrivateGroups() || category.isShared()) { - Set selectedParticipants = new HashSet<>(); - for (ParticipantGroup group : category.getGroups()) { - selectedParticipants.addAll(group.getParticipantSet()); JSONGroup jsonGroup = new JSONGroup(group, category); if (form.includeParticipantIds()) jsonGroup.setParticipantIds(group.getParticipantSet()); @@ -1114,16 +1107,15 @@ public ApiResponse execute(ParticipantGroupSpecification form, BindException err } /** - * Code to check whether an implicitly created category needs to be deleted so we don't accumulate orpaned list type + * Code to check whether an implicitly created category needs to be deleted so we don't accumulate orphaned list type * categories. - * */ private void deleteImplicitCategory(Integer prevCategoryId, ParticipantCategoryImpl current) throws ValidationException { if (prevCategoryId != null) { ParticipantCategoryImpl oldCategory = ParticipantGroupManager.getInstance().getParticipantCategory(getContainer(), getUser(), prevCategoryId); - if (oldCategory.getType().equals("list") && !current.getType().equals("list")) + if (oldCategory != null && "list".equals(oldCategory.getType()) && !"list".equals(current.getType())) { ParticipantGroupManager.getInstance().deleteParticipantCategory(getContainer(), getUser(), oldCategory); } diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index fe3cc9c94ae..fa8b9d980f1 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -2953,12 +2953,16 @@ public static class DatasetItemDetailsAction extends SimpleViewAction("/org/labkey/study/view/sendParticipantGroup.jsp", form, errors); diff --git a/study/src/org/labkey/study/model/ParticipantGroupManager.java b/study/src/org/labkey/study/model/ParticipantGroupManager.java index 7a148b7210a..e62017e5bd6 100644 --- a/study/src/org/labkey/study/model/ParticipantGroupManager.java +++ b/study/src/org/labkey/study/model/ParticipantGroupManager.java @@ -16,7 +16,7 @@ package org.labkey.study.model; import org.jetbrains.annotations.Nullable; -import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import org.labkey.api.audit.AuditLogService; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -31,7 +31,6 @@ import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SqlExecutor; -import org.labkey.api.data.SqlSelector; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; @@ -42,9 +41,9 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractContainerScopingTest; -import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.settings.ResourceURL; import org.labkey.api.study.CohortFilter; import org.labkey.api.study.ParticipantCategory; @@ -78,17 +77,11 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; -/** - * User: klum - * Date: Jun 1, 2011 - * Time: 2:26:02 PM - */ public class ParticipantGroupManager { private static final ParticipantGroupManager _instance = new ParticipantGroupManager(); @@ -119,17 +112,13 @@ public static TableInfo getTableInfoParticipantCategory() return StudySchema.getInstance().getTableInfoParticipantCategory(); } - public ParticipantCategoryImpl getParticipantCategory(Container c, User user, String label) + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, User user, String label) { ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategoryForLabel(c, label); - if (category != null) + if (category != null && category.canRead(user)) return category; - ParticipantCategoryImpl def = new ParticipantCategoryImpl(); - def.setContainer(c.getId()); - def.setLabel(label); - - return def; + return null; } public boolean categoryExists(Container c, User user, String label, boolean shared) @@ -137,14 +126,14 @@ public boolean categoryExists(Container c, User user, String label, boolean shar assert label != null : "Label cannot be null"; SimpleFilter filter = SimpleFilter.createContainerFilter(c); - Set exsitingCategories = new HashSet<>(); + Set existingCategories = new HashSet<>(); filter.addCondition(FieldKey.fromString("OwnerId"), shared ? ParticipantCategory.OWNER_SHARED : user.getUserId()); TableSelector selector = new TableSelector(getTableInfoParticipantCategory(), Collections.singleton("Label"), filter, null); for (String name : selector.getArrayList(String.class)) - exsitingCategories.add(name.toLowerCase()); + existingCategories.add(name.toLowerCase()); - return exsitingCategories.contains(label.toLowerCase()); + return existingCategories.contains(label.toLowerCase()); } public List getParticipantCategoriesByType(final Container c, final User user, @Nullable String type) @@ -152,12 +141,11 @@ public List getParticipantCategoriesByType(final Contai if (type == null) return _getParticipantCategories(c, user); - List categories = new ArrayList<>(); Collection types = ParticipantGroupCache.getParticipantCategoryForType(c, type); - if (types != null) - categories.addAll(types); + if (types == null) + return Collections.emptyList(); - return categories; + return types.stream().filter(category -> category != null && category.canRead(user)).toList(); } public List getParticipantCategoriesByLabel(final Container c, final User user, @Nullable String label) @@ -165,12 +153,11 @@ public List getParticipantCategoriesByLabel(final Conta if (label == null) return _getParticipantCategories(c, user); - List categories = new ArrayList<>(); - ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategoryForLabel(c, label); + ParticipantCategoryImpl category = getParticipantCategory(c, user, label); if (category != null) - categories.add(category); + return List.of(category); - return categories; + return Collections.emptyList(); } public ActionButton createParticipantGroupButton(ViewContext context, String dataRegionName, CohortFilter cohortFilter, @@ -406,31 +393,26 @@ private String createNewParticipantGroupScript(ViewContext context, String dataR } /** - * Returns the list participant categories that the specified user is allowed to see + * Returns the list of participant categories that the specified user is allowed to see. * - * @param distinctCategories if true returns the unique (by label) set of categories. A private category will + * @param distinctCategories if true, returns the unique (by label) set of categories. A private category will * supersede a public category. */ public List getParticipantCategories(Container c, User user, boolean distinctCategories) { - if (distinctCategories) - { - Map categoryMap = new HashMap<>(); - for (ParticipantCategoryImpl category : _getParticipantCategories(c, user)) - { - if (categoryMap.containsKey(category.getLabel())) - { - if (!category.isShared()) - categoryMap.put(category.getLabel(), category); - } - else - categoryMap.put(category.getLabel(), category); - } - return new LinkedList<>(categoryMap.values()); + List categories = _getParticipantCategories(c, user); + + if (!distinctCategories) + return categories; + Map categoryMap = new HashMap<>(); + for (ParticipantCategoryImpl category : categories) + { + if (!categoryMap.containsKey(category.getLabel()) || !category.isShared()) + categoryMap.put(category.getLabel(), category); } - else - return _getParticipantCategories(c, user); + + return new ArrayList<>(categoryMap.values()); } public List getParticipantCategories(Container c, User user) @@ -440,14 +422,10 @@ public List getParticipantCategories(Container c, User private List _getParticipantCategories(Container c, User user) { - Collection categories = ParticipantGroupCache.getParticipantCategories(c); - List filtered = new ArrayList<>(); - - // TODO: Switch ParticipantCategoryImpl internals from arrays to lists... but not right now - categories.stream().filter(category -> category.canRead(c, user)).forEach(category -> { - filtered.add(category); - }); - return filtered; + return ParticipantGroupCache.getParticipantCategories(c) + .stream() + .filter(category -> category.canRead(user)) + .toList(); } @Deprecated // create participant categories and groups separately @@ -559,7 +537,7 @@ private ParticipantCategoryImpl _saveParticipantCategory(Container c, User user, } else { - ParticipantCategoryImpl prev = getParticipantCategory(c, user, def.getRowId()); + ParticipantCategoryImpl prev = getParticipantCategory(c, def.getRowId()); ret = Table.update(user, StudySchema.getInstance().getTableInfoParticipantCategory(), def, def.getRowId()); event = ParticipantGroupAuditProvider.EventFactory.categoryChange(c, user, prev, ret); } @@ -577,12 +555,9 @@ private ParticipantGroup _setParticipantGroup(Container c, User user, Participan if (verifyCategory) { ParticipantCategoryImpl cat = getParticipantCategory(c, user, group.getCategoryId()); - if (cat == null) throw new ValidationException("The specified category was not found."); - // canEdit enforces the SharedParticipantGroupPermission/Admin check for shared categories AND the - // owner check for private categories if (!cat.canEdit(c, user)) throw new ValidationException("You do not have permission to modify groups in this participant category"); } @@ -848,7 +823,6 @@ private void addGroupParticipants(Container c, User user, ParticipantGroup group } } - private void removeGroupParticipants(Container c, User user, ParticipantGroup group, String[] participantsToRemove) { // remove the mapping from group to participants @@ -861,7 +835,6 @@ private void removeGroupParticipants(Container c, User user, ParticipantGroup gr ParticipantGroupCache.uncache(c); } - private void deleteGroupParticipants(Container c, User user, ParticipantGroup group) { // remove the mapping from group to participants @@ -873,12 +846,20 @@ private void deleteGroupParticipants(Container c, User user, ParticipantGroup gr ParticipantGroupCache.uncache(c); } - @Nullable - public ParticipantCategoryImpl getParticipantCategory(Container c, User user, int rowId) + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, int rowId) { return ParticipantGroupCache.getParticipantCategory(c, rowId); } + public @Nullable ParticipantCategoryImpl getParticipantCategory(Container c, User user, int rowId) + { + ParticipantCategoryImpl category = getParticipantCategory(c, rowId); + if (category != null && category.canRead(user)) + return category; + + return null; + } + public ParticipantGroup getParticipantGroup(Container container, User user, int rowId) { return ParticipantGroupCache.getParticipantGroup(container, rowId); @@ -893,21 +874,11 @@ public ParticipantGroup getParticipantGroupFromGroupRowId(Container container, U return selector.getObject(ParticipantGroup.class); } - public List getAllGroupedParticipants(Container container) - { - SQLFragment sql = new SQLFragment("SELECT DISTINCT ParticipantId FROM "); - sql.append(getTableInfoParticipantGroupMap(), "GroupMap"); - sql.append(" WHERE Container = ? ORDER BY ParticipantId"); - sql.add(container); - - return new SqlSelector(StudySchema.getInstance().getSchema(), sql).getArrayList(String.class); - } - public List getParticipantGroups(final Container c, User user, ParticipantCategoryImpl def) { if (!def.isNew()) { - ParticipantCategoryImpl category = ParticipantGroupCache.getParticipantCategory(c, def.getRowId()); + ParticipantCategoryImpl category = getParticipantCategory(c, def.getRowId()); if (category != null) return Arrays.stream(category.getGroups()).toList(); } @@ -1028,8 +999,10 @@ public void deleteParticipantCategory(Container c, User user, ParticipantCategor public void deleteParticipantGroup(Container c, User user, ParticipantGroup group) throws ValidationException { ParticipantCategoryImpl cat = getParticipantCategory(c, user, group.getCategoryId()); - List errors = new ArrayList<>(); + if (cat == null) + return; + List errors = new ArrayList<>(); if (!cat.canDelete(c, user, errors)) throw new ValidationException(errors); @@ -1168,19 +1141,21 @@ public void clearCache(Container c) ParticipantGroupCache.uncache(c); } - public static class ParticipantGroupTestCase extends Assert + public static class ContainerScopingTestCase extends AbstractContainerScopingTest { - @Test - public void test() + private static final ParticipantGroupManager MANAGER = ParticipantGroupManager.getInstance(); + private Container _container; + private User _owner; + private User _otherUser; + + @Before + public void setupParticipantGroupFixtures() throws Exception { - ParticipantGroupManager p = new ParticipantGroupManager(); - ParticipantCategoryImpl def = new ParticipantCategoryImpl(); - p.getParticipantGroups(null, null, def); + _container = createContainer("study"); + _owner = createUserInRole(_container, ReaderRole.class); + _otherUser = createUserInRole(_container, ReaderRole.class); } - } - public static class ContainerScopingTestCase extends AbstractContainerScopingTest - { @Test public void testSetParticipantGroupRequiresOwnership() throws Exception { @@ -1188,38 +1163,157 @@ public void testSetParticipantGroupRequiresOwnership() throws Exception // the category's creator -- but the manager previously enforced only the shared-category case, so a Read // user could overwrite another user's private group via a guessable rowId. The fix gates _setParticipantGroup // on canEdit() for the private case too. - Container folder = createContainer("A"); - StudyService.get().createStudy(folder, getAdmin(), "Study", TimepointType.VISIT, true); - insertParticipant(folder, "P1"); - - ParticipantGroupManager mgr = ParticipantGroupManager.getInstance(); + StudyService.get().createStudy(_container, getAdmin(), "Study", TimepointType.VISIT, true); + insertParticipant(_container, "P1"); // A PRIVATE participant category + group owned by the admin (ownerId != OWNER_SHARED makes it private; the // admin is its creator, so only the admin may edit it). ParticipantCategoryImpl cat = new ParticipantCategoryImpl(); - cat.setContainer(folder.getId()); + cat.setContainer(_container.getId()); cat.setLabel("private-category"); - cat.setType("list"); + cat.setType(ParticipantCategory.Type.list.name()); cat.setOwnerId(getAdmin().getUserId()); - cat = mgr.setParticipantCategory(folder, getAdmin(), cat, new String[]{"P1"}, null, "private"); - ParticipantGroup group = mgr.getParticipantGroups(folder, getAdmin(), cat).get(0); + cat = MANAGER.setParticipantCategory(_container, getAdmin(), cat, new String[]{"P1"}, null, "private"); + ParticipantGroup group = MANAGER.getParticipantGroups(_container, getAdmin(), cat).get(0); - // A different user with only Read access (not the owner, not an admin) - User attacker = createUserInRole(folder, ReaderRole.class); - - // Saving (overwriting) the admin's private group as the attacker must be rejected. + // Saving (overwriting) the admin's private group as a non-owner must be rejected. try { - mgr.setParticipantGroup(folder, attacker, group); + // A different user with only Read access (not the owner, not an admin) + MANAGER.setParticipantGroup(_container, _otherUser, group); fail("A non-owner must not be able to modify another user's private participant group"); } - catch (ValidationException expected) + catch (ValidationException ignored) { } // Positive control: the owner (admin) can still save their own private group -- the guard rejects only the // non-owner, not every caller. - mgr.setParticipantGroup(folder, getAdmin(), group); + MANAGER.setParticipantGroup(_container, getAdmin(), group); + } + + /** The by-label getter must apply canRead(), so by-label does not disclose a private category to a non-owner. */ + @Test + public void byLabelGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-label"); + + ParticipantCategoryImpl asOwner = MANAGER.getParticipantCategory(_container, _owner, "Private-by-label"); + assertNotNull("Owner should resolve their own private category", asOwner); + assertEquals("Owner should resolve the real (persisted) category", owned.getRowId(), asOwner.getRowId()); + assertFalse("Resolved category should be private", asOwner.isShared()); + assertEquals(_owner.getUserId(), asOwner.getOwnerId()); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, "Private-by-label"); + assertNull("Another user must not resolve the owner's private category by label", asOther); + } + + /** A shared category is readable by any user, so the by-label getter must still return it for a non-owner. */ + @Test + public void byLabelGetterReturnsSharedCategoryForAnyUser() throws Exception + { + ParticipantCategoryImpl shared = createSharedCategory("Shared-by-label"); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, "Shared-by-label"); + assertNotNull("A shared category must be readable by any user", asOther); + assertEquals(shared.getRowId(), asOther.getRowId()); + assertTrue(asOther.isShared()); + } + + /** The by-rowId getter must apply canRead(), so listing a categoryId does not disclose a private category to a non-owner. */ + @Test + public void byRowIdGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-rowId"); + + ParticipantCategoryImpl asOwner = MANAGER.getParticipantCategory(_container, _owner, owned.getRowId()); + assertNotNull("Owner should resolve their own private category by rowId", asOwner); + assertEquals(owned.getRowId(), asOwner.getRowId()); + assertFalse("Resolved category should be private", asOwner.isShared()); + assertEquals(_owner.getUserId(), asOwner.getOwnerId()); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, owned.getRowId()); + assertNull("Another user must not resolve the owner's private category by rowId", asOther); + } + + /** A shared category is readable by any user, so the by-rowId getter must still return it for a non-owner. */ + @Test + public void byRowIdGetterReturnsSharedCategoryForAnyUser() throws Exception + { + ParticipantCategoryImpl shared = createSharedCategory("Shared-by-rowId"); + + ParticipantCategoryImpl asOther = MANAGER.getParticipantCategory(_container, _otherUser, shared.getRowId()); + assertNotNull("A shared category must be readable by any user", asOther); + assertEquals(shared.getRowId(), asOther.getRowId()); + assertTrue(asOther.isShared()); + } + + /** getParticipantCategoriesByLabel delegates to the by-label getter and must inherit the same canRead() check. */ + @Test + public void byLabelListGetterHidesAnotherUsersPrivateCategory() throws Exception + { + createPrivateCategory(_owner, "Private-by-label-list"); + + assertEquals("Owner should see their private category by label", + 1, MANAGER.getParticipantCategoriesByLabel(_container, _owner, "Private-by-label-list").size()); + assertTrue("Another user must not see the owner's private category by label", + MANAGER.getParticipantCategoriesByLabel(_container, _otherUser, "Private-by-label-list").isEmpty()); + } + + /** getParticipantCategoriesByType must also filter by canRead(), so a private category isn't leaked by type. */ + @Test + public void byTypeGetterHidesAnotherUsersPrivateCategory() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-by-type"); + String type = ParticipantCategory.Type.list.name(); + + assertTrue("Owner should see their private category among the typed categories", + containsRowId(MANAGER.getParticipantCategoriesByType(_container, _owner, type), owned.getRowId())); + assertFalse("Another user must not see the owner's private category among the typed categories", + containsRowId(MANAGER.getParticipantCategoriesByType(_container, _otherUser, type), owned.getRowId())); + } + + /** The collection getter must hide another user's private category while still surfacing shared categories. */ + @Test + public void collectionGetterHidesPrivateButReturnsShared() throws Exception + { + ParticipantCategoryImpl owned = createPrivateCategory(_owner, "Private-collection"); + ParticipantCategoryImpl shared = createSharedCategory("Shared-collection"); + + List asOwner = MANAGER.getParticipantCategories(_container, _owner); + assertTrue("Owner should see their own private category", containsRowId(asOwner, owned.getRowId())); + assertTrue("Owner should see the shared category", containsRowId(asOwner, shared.getRowId())); + + List asOther = MANAGER.getParticipantCategories(_container, _otherUser); + assertFalse("Another user must not see the owner's private category", containsRowId(asOther, owned.getRowId())); + assertTrue("Shared categories remain visible to everyone", containsRowId(asOther, shared.getRowId())); + } + + private static boolean containsRowId(Collection categories, int rowId) + { + return categories.stream().anyMatch(category -> category.getRowId() == rowId); + } + + /** Create a private (owner-scoped) participant category, persisted and owned by {@code owner}. */ + private ParticipantCategoryImpl createPrivateCategory(User owner, String label) throws ValidationException + { + ParticipantCategoryImpl def = new ParticipantCategoryImpl(); + def.setContainer(_container.getId()); + def.setLabel(label); + def.setType(ParticipantCategory.Type.list.name()); + def.setOwnerId(owner.getUserId()); + return MANAGER.setParticipantCategory(_container, owner, def); + } + + /** Create a shared participant category. Created as the site admin, who may create shared categories. */ + private ParticipantCategoryImpl createSharedCategory(String label) throws ValidationException + { + ParticipantCategoryImpl def = new ParticipantCategoryImpl(); + def.setContainer(_container.getId()); + def.setLabel(label); + def.setType(ParticipantCategory.Type.list.name()); + // OwnerId defaults to OWNER_SHARED + return MANAGER.setParticipantCategory(_container, getAdmin(), def); } private void insertParticipant(Container c, String ptid) From ce4b290dc8ae0d9e20785036b6f146f4db172443 Mon Sep 17 00:00:00 2001 From: Binal Patel Date: Wed, 17 Jun 2026 15:35:44 -0600 Subject: [PATCH 08/13] Validate list audit event matches URL-requested list, and unit test. (#7757) - Pass an explicit ContainerFilter.current(_list.getContainer(), user) to getAuditEvent - Add ListAuditProvider.auditEventMatchesList() - Add ListAuditProvider.TestCase --- list/src/org/labkey/list/ListModule.java | 3 +- .../list/controllers/ListController.java | 9 +- .../labkey/list/model/ListAuditProvider.java | 83 +++++++++++++++++++ 3 files changed, 93 insertions(+), 2 deletions(-) diff --git a/list/src/org/labkey/list/ListModule.java b/list/src/org/labkey/list/ListModule.java index 6ca75a7db52..b8566275e9d 100644 --- a/list/src/org/labkey/list/ListModule.java +++ b/list/src/org/labkey/list/ListModule.java @@ -221,7 +221,8 @@ public Set getUnitTests() { return Set.of( ListManager.TestCase.class, - ListWriter.TestCase.class + ListWriter.TestCase.class, + ListAuditProvider.TestCase.class ); } } diff --git a/list/src/org/labkey/list/controllers/ListController.java b/list/src/org/labkey/list/controllers/ListController.java index 1cd6be9104c..9ee7bb13778 100644 --- a/list/src/org/labkey/list/controllers/ListController.java +++ b/list/src/org/labkey/list/controllers/ListController.java @@ -845,7 +845,14 @@ public ModelAndView getView(Object o, BindException errors) String oldRecord = null; String newRecord = null; - ListAuditProvider.ListAuditEvent event = AuditLogService.get().getAuditEvent(getUser(), ListManager.LIST_AUDIT_EVENT, id); + ListAuditProvider.ListAuditEvent event = AuditLogService.get().getAuditEvent( + getUser(), ListManager.LIST_AUDIT_EVENT, id, ContainerFilter.current(_list.getContainer(), getUser())); + + // Tie the loaded event to the URL-requested listId — rowId is user-controlled (CWE-639). + if (!ListAuditProvider.auditEventMatchesList(event, listId, _list.getContainer())) + { + event = null; + } if (event != null) { diff --git a/list/src/org/labkey/list/model/ListAuditProvider.java b/list/src/org/labkey/list/model/ListAuditProvider.java index 87415abdf58..11fcd2f8b81 100644 --- a/list/src/org/labkey/list/model/ListAuditProvider.java +++ b/list/src/org/labkey/list/model/ListAuditProvider.java @@ -34,13 +34,19 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.UserSchema; import org.labkey.api.util.StringExpressionFactory; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import java.util.ArrayList; import java.util.Collections; +import java.util.Date; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; /** @@ -142,6 +148,22 @@ public Class getEventClass() return (Class)ListAuditEvent.class; } + /** + * Verifies that a loaded {@link ListAuditEvent} actually pertains to the requested list and container before its + * old/new record maps are surfaced to the caller. + *

+ * Called by {@code ListController.ListItemDetailsAction} to close CWE-639 (IDOR via user-controlled {@code rowId}): + * without this predicate, a user with audit-read access in container A could pass {@code listId=X&rowId=N-for-Y} + * and have List Y's audit payload render inside List X's details page. The container check is defense-in-depth + * in case the audit schema's default ContainerFilter is ever changed away from {@code Current}. + */ + public static boolean auditEventMatchesList(@Nullable ListAuditEvent event, int expectedListId, @NotNull Container expectedContainer) + { + return event != null + && event.getListId() == expectedListId + && Objects.equals(event.getContainer(), expectedContainer); + } + public static class ListAuditEvent extends DetailedAuditTypeEvent { private int _listId; @@ -214,6 +236,67 @@ public Map getAuditLogMessageElements() } } + public static class TestCase extends Assert + { + private static final String CONTAINER_A = "11111111-1111-1111-1111-111111111111"; + private static final String CONTAINER_B = "22222222-2222-2222-2222-222222222222"; + + @Test + public void matches_whenListIdAndContainerAgree() + { + Container c = testContainer(CONTAINER_A); + assertTrue(auditEventMatchesList(eventFor(42, c), 42, c)); + } + + @Test + public void rejects_nullEvent() + { + assertFalse(auditEventMatchesList(null, 42, testContainer(CONTAINER_A))); + } + + @Test + public void rejects_wrongListId() + { + // Event's listId is 99, but URL asked for list 42: the cross-list-in-same-container + // attack. Without this check, List 42's details page renders List 99's payload. + Container c = testContainer(CONTAINER_A); + assertFalse(auditEventMatchesList(eventFor(99, c), 42, c)); + } + + @Test + public void rejects_wrongContainer() + { + // Defense-in-depth: even if a future audit-schema CF change ever let an event from + // another container leak through getAuditEvent(), this check would still block it. + assertFalse(auditEventMatchesList( + eventFor(42, testContainer(CONTAINER_B)), 42, testContainer(CONTAINER_A))); + } + + @Test + public void rejects_nullEventContainer() + { + // Event present but its container is null (could happen if the audit event was + // hand-constructed or persisted without a container): must not match. + assertFalse(auditEventMatchesList(eventFor(42, null), 42, testContainer(CONTAINER_A))); + } + + private static Container testContainer(String guid) + { + // Container.equals compares the GUID id, so any non-null parent / name / rowId + // values are fine for this test; only the id field is read by the assertion. + return new Container(null, "junit-" + guid.substring(0, 8), guid, 0, 0, new Date(0), 0, false); + } + + private static ListAuditEvent eventFor(int listId, @Nullable Container container) + { + ListAuditEvent event = new ListAuditEvent(); + event.setListId(listId); + if (container != null) + event.setContainer(container); + return event; + } + } + public static class ListAuditDomainKind extends AbstractAuditDomainKind { public static final String NAME = "ListAuditDomain"; From 2dd91ea816e94eb3b52360a52a0ea7e50493feb0 Mon Sep 17 00:00:00 2001 From: Josh Eckels Date: Wed, 17 Jun 2026 16:47:44 -0700 Subject: [PATCH 09/13] Improve checks for file move and pipeline status API (#7768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Rationale Previous scoping changes weren't correct given some important use cases for compability. #### Changes - Don't consider an import of a file to prevent moving it via WebDAV - Allow users with read access to the parent container to access the pipeline job status API via any container #### Tasks 📍 - [x] Claude Code Review - ~Manual Testing~ - [x] Test Automation --- .../api/webdav/AbstractWebdavResource.java | 9 ++ .../labkey/api/webdav/FileSystemResource.java | 50 +++++-- .../org/labkey/api/webdav/WebdavResource.java | 8 + .../api/webdav/WebdavResourceReadOnly.java | 6 + .../org/labkey/core/webdav/DavController.java | 137 +++++++++++++++++- .../experiment/ScriptsResourceProvider.java | 6 + .../pipeline/PipelineWebdavProvider.java | 8 + .../pipeline/status/StatusController.java | 17 ++- 8 files changed, 215 insertions(+), 26 deletions(-) diff --git a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java index 9bb1ccafa7c..44c267477a0 100644 --- a/api/src/org/labkey/api/webdav/AbstractWebdavResource.java +++ b/api/src/org/labkey/api/webdav/AbstractWebdavResource.java @@ -404,6 +404,15 @@ public boolean canDelete(User user, boolean forDelete, /* OUT */ @Nullable List< return perms.contains(DeletePermission.class); } + @Override + public boolean canMove(User user) + { + // A MOVE removes the resource from its source location, so by default it requires the same rights + // as deleting it from there. Resource types where moving and deleting differ (see FileSystemResource) + // override this. + return canDelete(user, true, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/FileSystemResource.java b/api/src/org/labkey/api/webdav/FileSystemResource.java index 8a1b316a4f8..be7a88b24b8 100644 --- a/api/src/org/labkey/api/webdav/FileSystemResource.java +++ b/api/src/org/labkey/api/webdav/FileSystemResource.java @@ -507,27 +507,38 @@ public boolean canCreate(User user, boolean forCreate) } + // Shared access checks for canDelete() and canMove(): Delete permission plus a writable file on disk. + // Deliberately does NOT apply the "imported by an assay" restriction, which is specific to outright + // deletion - a file that has been imported may still be relocated within the file root. + private boolean hasDeletePermissionAndWritableFile(User user, boolean forDelete, @Nullable List message) + { + if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) + return false; + File f = getFile(); + if (null == f) + return false; + if (!f.canWrite()) + { + SecurityLogger.log("File.canWrite()==false",user,null,false); + if (forDelete) + { + if (null != message) + message.add("File is not writable on server"); + _log.warn(user.getEmail() + " attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: " + f.getPath()); + } + return false; + } + return true; + } + + @Override public boolean canDelete(User user, boolean forDelete, @Nullable List message) { try { - if (!super.canDelete(user, forDelete, message) || !hasFileSystem()) - return false; - File f = getFile(); - if (null == f) + if (!hasDeletePermissionAndWritableFile(user, forDelete, message)) return false; - if (!f.canWrite()) - { - SecurityLogger.log("File.canWrite()==false",user,null,false); - if (forDelete) - { - if (null != message) - message.add("File is not writable on server"); - _log.warn(user.getEmail() + " attempted to delete file that is not writable by LabKey Server. This may be a configuration problem. file: " + f.getPath()); - } - return false; - } // can't delete if already processed if (!getActions(user).isEmpty()) { @@ -544,6 +555,15 @@ public boolean canDelete(User user, boolean forDelete, @Nullable List me } + @Override + public boolean canMove(User user) + { + // A MOVE removes the file from its source location, so it requires Delete permission and a writable + // file. Unlike canDelete(), it does NOT require the file to be eligible for outright deletion: a file + // that has been imported by an assay may still be relocated within the file root. + return hasDeletePermissionAndWritableFile(user, false, null); + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/api/src/org/labkey/api/webdav/WebdavResource.java b/api/src/org/labkey/api/webdav/WebdavResource.java index 5b503d49f1f..db1592e4ccc 100644 --- a/api/src/org/labkey/api/webdav/WebdavResource.java +++ b/api/src/org/labkey/api/webdav/WebdavResource.java @@ -189,6 +189,14 @@ public interface WebdavResource extends Resource */ boolean canDelete(User user, boolean forDelete); boolean canDelete(User user, boolean forDelete, /* OUT */ List message); + /** + * A MOVE removes the resource from its source location, so it requires Delete permission there. + * Unlike {@link #canDelete}, it does not require the resource to be eligible for outright deletion + * (for example, a file that has been imported by an assay may still be relocated within the file root). + * @param user authenticated user + * @return true if the user has permission and server has capability + */ + boolean canMove(User user); /** * @param user authenticated user * @param forRename true if user wants to rename, false if checking capabilities (affects logging) diff --git a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java index e5cdd8c3def..fe32e3fc7ce 100644 --- a/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java +++ b/api/src/org/labkey/api/webdav/WebdavResourceReadOnly.java @@ -291,6 +291,12 @@ public boolean canDelete(User user, boolean forDelete, List message) return false; } + @Override + public boolean canMove(User user) + { + return false; + } + @Override public boolean canRename(User user, boolean forRename) { diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 4a71e7b134e..9af60ed35ff 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -50,12 +50,17 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.ConvertHelper; import org.labkey.api.data.Sort; +import org.labkey.api.exp.api.DataType; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExpProtocol; +import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.files.FileContentService; import org.labkey.api.miniprofiler.MiniProfiler; import org.labkey.api.miniprofiler.RequestInfo; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.pipeline.PipeRoot; +import org.labkey.api.pipeline.PipelineService; import org.labkey.api.premium.AntiVirusService; import org.labkey.api.premium.PremiumService; import org.labkey.api.query.ValidationException; @@ -3727,8 +3732,8 @@ WebdavStatus doMethod() throws DavException, IOException if (!src.canRead(getUser(), true)) return unauthorized(src); - // MOVE is effectively a delete operation from the source's perspective so confirm access - if (!src.canDelete(getUser(), true)) + // MOVE removes the resource from the source location, so confirm the caller could delete it there. + if (!src.canMove(getUser())) return unauthorized(src); if (exists && !dest.canWrite(getUser(),true) || !exists && !dest.canCreate(getUser(),true)) return unauthorized(dest); @@ -6727,11 +6732,91 @@ public void testMoveActionRequiresTargetCreate() throws Exception assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); } + // A.1: canDelete() and canMove() diverge ONLY for a file that backs experiment data (used by a run). + // Such a file may not be deleted outright, but it may still be relocated. This pins that divergence at + // the resource level, independent of the HTTP layer, as an admin who has every relevant permission. + @Test + public void testImportedFileCanMoveButNotDelete() throws Exception + { + File dir = ensureFilesDir(_folder); + File srcFile = writeFile(dir, "imported.txt"); + importFileIntoRun(_folder, srcFile); + + User admin = getAdmin(); + WebdavResource resource = WebdavService.get().lookup(filesPath(_folder).append("imported.txt")); + assertNotNull("Imported file should resolve through the resolver", resource); + assertTrue("Imported file should exist", resource.exists()); + assertFalse("Precondition: a file used by a run must report a non-empty action list", resource.getActions(admin).isEmpty()); + + List messages = new ArrayList<>(); + assertFalse("Admin must NOT be able to delete a file that has been imported by an assay", resource.canDelete(admin, true, messages)); + assertTrue("canDelete() should explain that the file was imported by an assay", messages.stream().anyMatch(m -> m.contains("imported by an assay"))); + assertTrue("Admin MUST still be able to move a file that has been imported by an assay", resource.canMove(admin)); + } + + // A.2: The same divergence end-to-end through WebdavServlet. A file that backs experiment data is refused + // by DELETE (SC_FORBIDDEN) but relocated by MOVE (SC_CREATED). This is the platform-level analog of the + // PanoramaPublicMoveSkyDocTest scenario and guards against a recurrence of that regression. + @Test + public void testMoveActionMovesImportedFileButDeleteForbidden() throws Exception + { + File dir = ensureFilesDir(_folder); + + // DELETE of a file imported by a run is forbidden, and the file survives. + File importedForDelete = writeFile(dir, "imported-delete.txt"); + importFileIntoRun(_folder, importedForDelete); + MockHttpServletResponse deleteResp = doDelete(_folder, filesPath(_folder).append("imported-delete.txt"), getAdmin()); + assertEquals("DELETE of a file imported by an assay must be forbidden", HttpServletResponse.SC_FORBIDDEN, deleteResp.getStatus()); + assertTrue("File must still exist after a forbidden delete", importedForDelete.exists()); + + // MOVE of an equally-imported file succeeds, relocating it within the file root. + File importedForMove = writeFile(dir, "imported-move.txt"); + importFileIntoRun(_folder, importedForMove); + Path src = filesPath(_folder).append("imported-move.txt"); + Path dest = filesPath(_folder).append("moved.txt"); + MockHttpServletResponse moveResp = doMove(_folder, src, dest, getAdmin()); + assertEquals("MOVE of a file imported by an assay must succeed", HttpServletResponse.SC_CREATED, moveResp.getStatus()); + assertFalse("Source file should no longer exist after a successful move", importedForMove.exists()); + assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); + } + + // Regression guard for the canMove()/canDelete() divergence introduced alongside the MOVE fix. A + // FileSystemResource subclass whose canDelete() forbids deletion outright (PipelineFolderResource returns + // false) must also forbid MOVE. Otherwise the new FileSystemResource.canMove() default - Delete permission + // plus a writable file - would let an admin relocate a node that was never deletable, because that default + // calls super.canDelete() and so bypasses the subclass override entirely. + @Test + public void testNonDeletableNodeIsNotMovable() throws Exception + { + // Configure an explicit pipeline root so the @pipeline webdav node resolves to a PipelineFolderResource. + File fileRoot = ensureFilesDir(_folder).getParentFile(); + File pipelineDir = new File(fileRoot, "pipelineOverrideRoot"); + if (!pipelineDir.exists()) + assertTrue("Test requires a writable pipeline root directory", pipelineDir.mkdirs()); + PipelineService.get().setPipelineRoot(getAdmin(), _folder, PipelineService.PRIMARY_ROOT, false, pipelineDir.toURI()); + + WebdavResource pipelineNode = WebdavService.get().lookup(pipelinePath(_folder)); + assertNotNull("Test requires the @pipeline webdav node to resolve to a PipelineFolderResource", pipelineNode); + assertNotNull("The pipeline node must be backed by a writable file root", pipelineNode.getFile()); + + User admin = getAdmin(); + // An admin has Delete permission and the pipeline root is writable, so the inherited + // FileSystemResource.canMove() would return true. The override must keep canMove() aligned with the + // categorical canDelete()==false, so the node remains immovable. + assertFalse("The pipeline node must never be deletable", pipelineNode.canDelete(admin, true, null)); + assertFalse("A node that is not deletable must also not be movable", pipelineNode.canMove(admin)); + } + private static Path filesPath(Container c) { return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.FILES_LINK); } + private static Path pipelinePath(Container c) + { + return WebdavService.getPath().append(c.getParsedPath()).append(FileContentService.PIPELINE_LINK); + } + private static File ensureFilesDir(Container c) { WebdavResource filesNode = WebdavService.get().lookup(filesPath(c)); @@ -6745,7 +6830,12 @@ private static File ensureFilesDir(Container c) private static File writeFile(File dir) throws IOException { - File f = FileUtil.appendName(dir, "secret.txt"); + return writeFile(dir, "secret.txt"); + } + + private static File writeFile(File dir, String name) throws IOException + { + File f = FileUtil.appendName(dir, name); try (FileOutputStream os = new FileOutputStream(f)) { os.write("secret".getBytes(StandardCharsets.UTF_8)); @@ -6753,6 +6843,47 @@ private static File writeFile(File dir) throws IOException return f; } + // Registers the file as experiment data used by a run, so the resolver's WebdavResource reports a + // non-empty action list - the same state that, in production, blocks deletion of an imported file + // while still permitting a move. + private void importFileIntoRun(Container c, File file) throws Exception + { + ExperimentService expSvc = ExperimentService.get(); + User admin = getAdmin(); + + ExpData data = expSvc.createData(c, new DataType("Data"), file.getName()); + data.setDataFileURI(file.toURI()); + data.save(admin); + + ExpRun run = expSvc.createExperimentRun(c, "import of " + file.getName()); + PipeRoot pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("Test requires a pipeline root for " + c.getName(), pipeRoot); + run.setFilePathRoot(pipeRoot.getRootPath()); + ExpProtocol protocol = expSvc.ensureSampleDerivationProtocol(admin); + run.setProtocol(protocol); + + ViewBackgroundInfo info = new ViewBackgroundInfo(c, admin, null); + // Wiring the data as a run input is enough for getRunsUsingDatas() to associate the run with the file. + expSvc.saveSimpleExperimentRun(run, Collections.emptyMap(), Map.of(data, "Data"), Collections.emptyMap(), + Collections.emptyMap(), Collections.emptyMap(), info, null, false); + } + + private static MockHttpServletResponse doDelete(Container sourceContainer, Path srcResource, User user) throws Exception + { + String srcWebdav = srcResource.toString(); + String servletPath = "/" + WebdavService.getServletPath(); + + HttpServletRequest base = ViewServlet.mockRequest("DELETE", new ActionURL(name, "delete", sourceContainer), user, Map.of(), null); + MockHttpServletRequest req = (MockHttpServletRequest) base; + req.setServletPath(servletPath); + req.setPathInfo(srcWebdav.substring(servletPath.length())); + req.setRequestURI(srcWebdav); + + MockHttpServletResponse resp = new MockHttpServletResponse(); + new WebdavServlet(false).service(req, resp); + return resp; + } + private static MockHttpServletResponse doMove(Container sourceContainer, Path srcResource, Path destResource, User user) throws Exception { String srcWebdav = srcResource.toString(); diff --git a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java index 0ec70c020ab..0cbc665d68e 100644 --- a/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java +++ b/experiment/src/org/labkey/experiment/ScriptsResourceProvider.java @@ -146,6 +146,12 @@ public boolean canList(User user, boolean forRead) return hasAccess(user); } + @Override + public boolean canMove(User user) + { + return hasAccess(user); + } + @Override public boolean shouldIndex() { diff --git a/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java b/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java index dcd298bc74a..dc256cb85e6 100644 --- a/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java +++ b/pipeline/src/org/labkey/pipeline/PipelineWebdavProvider.java @@ -106,6 +106,14 @@ public boolean canDelete(User user, boolean forDelete, List msg) return false; } + @Override + public boolean canMove(User user) + { + // The pipeline folder node is never deletable (see canDelete()), so it must not be movable either. + // FileSystemResource.canMove() relaxes only the assay-import restriction, not this categorical block. + return false; + } + @Override protected boolean hasAccess(User user) { diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 1e5050799a8..eacd8bf1fc8 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -484,7 +484,7 @@ public Object execute(StatusDetailsForm form, BindException errors) throws Excep Container c = getContainerCheckAdmin(); PipelineStatusFile psf = getStatusFile(form.getRowId()); - if (psf == null || !getContainer().equals(psf.lookupContainer())) + if (psf == null || psf.lookupContainer() == null || !psf.lookupContainer().hasPermission(getUser(), ReadPermission.class)) throw new NotFoundException("Could not find status file for rowId " + form.getRowId()); var status = StatusDetailsBean.create(c, psf, form.getOffset(), form.getCount()); @@ -1120,15 +1120,16 @@ public void testStatusDetailsContainerScoping() throws Exception ActionURL foreignUrl = new ActionURL(StatusDetailsAction.class, folderA).addParameter("rowId", String.valueOf(rowId)); - // The API is scoped to its own container: addressing B's job through folder A is 404, regardless of the - // caller's rights in B. This is the case that fails without the fix (the unscoped action would serve B's - // job through folder A). - // A caller who can read folder A but NOT folder B: + // This API authorizes against the job's OWN container (folder B), not the container in the URL, so it + // intentionally supports referencing a job from another container -- but only for a caller who can read the + // container the job actually lives in. + // A caller who can read folder A but NOT folder B must still get 404: the job must not be revealed to + // someone without rights to its container. This is the case that fails without the fix. assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); - // ...and a site admin, who CAN read folder B, still gets 404 through folder A (no cross-container redirect). - assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, admin)); + // A site admin, who CAN read folder B, is served B's job through folder A -- the supported cross-container reference. + assertStatus(HttpServletResponse.SC_OK, get(foreignUrl, admin)); - // Positive control: addressing the job through its own container still succeeds. + // Positive control: addressing the job through its own container also succeeds for a caller who can read it. ActionURL ownUrl = new ActionURL(StatusDetailsAction.class, folderB).addParameter("rowId", String.valueOf(rowId)); assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } From 5190cda530e926452baea152286e1c56571a64c4 Mon Sep 17 00:00:00 2001 From: Karl Lum Date: Wed, 17 Jun 2026 17:29:18 -0700 Subject: [PATCH 10/13] Container scoping for miscellaneous server objects (#7767) --- .../publish/AbstractPublishConfirmAction.java | 9 ++ .../filecontent/FileContentController.java | 10 +- .../org/labkey/issue/IssuesController.java | 95 +++++++++++++- issues/src/org/labkey/issue/IssuesModule.java | 3 +- .../specimen/actions/SpecimenController.java | 10 +- .../report/SpecimenVisitReportParameters.java | 47 ++++--- study/src/org/labkey/study/StudyModule.java | 2 + .../PublishConfirmContainerScopingTest.java | 117 ++++++++++++++++++ .../reports/ReportsController.java | 42 ++++++- 9 files changed, 305 insertions(+), 30 deletions(-) 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 aee4cb7f297..f4d214d5ae8 100644 --- a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java +++ b/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) 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; 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 ); } diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index 059d63a9fcf..1e917b547ef 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -1182,12 +1182,16 @@ public static class SpecimenEventsRedirectAction extends SimpleViewAction getIntegrationTests() DatasetController.DatasetAuditHistoryScopingTestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, + PublishConfirmContainerScopingTest.class, CreateChildStudyAction.ContainerScopingTestCase.class, StudyController.ContainerScopingTestCase.class, ReportsController.ContainerScopingTestCase.class); 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..5685ce2b157 --- /dev/null +++ b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java @@ -0,0 +1,117 @@ +/* + * 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.UnauthorizedException; +import org.labkey.api.view.ViewContext; +import org.springframework.validation.BindException; + +import java.util.List; + +public class PublishConfirmContainerScopingTest extends AbstractContainerScopingTest +{ + 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); + + assertFalse("Linking into a target study the caller cannot insert into must be rejected", + 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); + + 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}, returns true if validation succeeds, false otherwise. + */ + 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); + + 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()); + + 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 + { + 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); + } +} diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index 65f56b36497..3aeedeaba80 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -70,6 +70,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; @@ -290,8 +291,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); @@ -461,7 +468,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()) @@ -836,7 +844,7 @@ public static class SaveReportViewForm extends SaveReportForm private String _schemaName; private String _viewName; private String _dataRegionName; - private String _redirectUrl; + private ReturnURLString _redirectUrl; public SaveReportViewForm() { @@ -926,12 +934,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; } @@ -1362,5 +1370,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 d2667da8b1ee3e81c114e896e4d7c8cc47fe28bb Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Thu, 18 Jun 2026 05:56:42 -0600 Subject: [PATCH 11/13] Scope mothership updateSoftwareRelease to the caller's container (#7763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Rationale MothershipManager.updateSoftwareRelease performed a raw Table.update keyed only on the SoftwareRelease primary key, with no container filter, and reassigned the row's container to the caller's folder. Because the update form binds softwareReleaseId directly from the request and UpdateAction only checks UpdatePermission against the current folder, a user with update rights in one folder could edit — and re-home into their own folder — a SoftwareRelease owned by another folder. This scopes the update to the caller's container so only rows that already belong to that folder can be modified. #### Related Pull Requests - None #### Changes - Add a container-scoped MothershipManager.getSoftwareRelease(int, Container) lookup, mirroring the existing getExceptionStackTrace/getServerInstallation helpers. - Verify the target SoftwareRelease belongs to the caller's container in updateSoftwareRelease before updating; throw NotFoundException otherwise. - Fix an incidental NPE in BulkUpdateAction where updateExceptionStackTrace was invoked even when the container-scoped lookup returned null; the call now happens only inside the null check. --- .../labkey/mothership/MothershipController.java | 2 +- .../org/labkey/mothership/MothershipManager.java | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index 3d4e59cbc23..8b24d7a1c22 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -1599,8 +1599,8 @@ else if (!form.isIgnore()) { exceptionStackTrace.setBugNumber(-1); } + MothershipManager.get().updateExceptionStackTrace(exceptionStackTrace, getUser()); } - MothershipManager.get().updateExceptionStackTrace(exceptionStackTrace, getUser()); } catch (NumberFormatException e) { diff --git a/mothership/src/org/labkey/mothership/MothershipManager.java b/mothership/src/org/labkey/mothership/MothershipManager.java index 26eede9da92..1584030d4e2 100644 --- a/mothership/src/org/labkey/mothership/MothershipManager.java +++ b/mothership/src/org/labkey/mothership/MothershipManager.java @@ -42,6 +42,7 @@ import org.labkey.api.util.MothershipReport; import org.labkey.api.util.ReentrantLockWithName; import org.labkey.api.util.logging.LogHelper; +import org.labkey.api.view.NotFoundException; import java.io.IOException; import java.util.ArrayList; @@ -211,6 +212,13 @@ public SoftwareRelease ensureSoftwareRelease(Container container, String revisio } } + public SoftwareRelease getSoftwareRelease(int softwareReleaseId, Container container) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(container); + filter.addCondition(FieldKey.fromString("SoftwareReleaseId"), softwareReleaseId); + return new TableSelector(getTableInfoSoftwareRelease(), filter, null).getObject(SoftwareRelease.class); + } + public ServerInstallation getServerInstallation(@NotNull String serverGUID, @NotNull String serverHostName, @NotNull Container c) { SimpleFilter filter = SimpleFilter.createContainerFilter(c); @@ -604,6 +612,12 @@ public void setStatusCakeApiKey(String statusCakeApiKey) public void updateSoftwareRelease(Container container, User user, SoftwareRelease bean) { + // Verify the target row actually belongs to this container before updating. The raw Table.update below is + // keyed only on the primary key, so without this check a user with UpdatePermission in one folder could edit + // (and, via setContainer, re-home) a SoftwareRelease owned by another folder. + if (getSoftwareRelease(bean.getSoftwareReleaseId(), container) == null) + throw new NotFoundException("SoftwareRelease not found in this folder: " + bean.getSoftwareReleaseId()); + bean.setContainer(container.getId()); Table.update(user, getTableInfoSoftwareRelease(), bean, bean.getSoftwareReleaseId()); } From 9087a57a15228c086dd1736e7fd08a3862834b4e Mon Sep 17 00:00:00 2001 From: Josh Eckels Date: Thu, 18 Jun 2026 10:22:16 -0700 Subject: [PATCH 12/13] Improve comments for tests in DavController (#7769) #### Rationale The fix for some scoping problems got committed with some overly verbose comments. To be merged when the queue is under less pressure. #### Changes - Edit the comments to be more relevant going forward --- .../org/labkey/core/webdav/DavController.java | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 9af60ed35ff..cac04a360a6 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -6732,9 +6732,7 @@ public void testMoveActionRequiresTargetCreate() throws Exception assertTrue("Destination file should exist after a successful move", FileUtil.appendName(targetDir, "moved.txt").exists()); } - // A.1: canDelete() and canMove() diverge ONLY for a file that backs experiment data (used by a run). - // Such a file may not be deleted outright, but it may still be relocated. This pins that divergence at - // the resource level, independent of the HTTP layer, as an admin who has every relevant permission. + // A file that backs experiment data (used by a run) can be moved but not deleted. Test via Java API @Test public void testImportedFileCanMoveButNotDelete() throws Exception { @@ -6754,43 +6752,35 @@ public void testImportedFileCanMoveButNotDelete() throws Exception assertTrue("Admin MUST still be able to move a file that has been imported by an assay", resource.canMove(admin)); } - // A.2: The same divergence end-to-end through WebdavServlet. A file that backs experiment data is refused - // by DELETE (SC_FORBIDDEN) but relocated by MOVE (SC_CREATED). This is the platform-level analog of the - // PanoramaPublicMoveSkyDocTest scenario and guards against a recurrence of that regression. + // A file that backs experiment data (used by a run) can be moved but not deleted. Test via WebDAV HTTP request @Test public void testMoveActionMovesImportedFileButDeleteForbidden() throws Exception { File dir = ensureFilesDir(_folder); // DELETE of a file imported by a run is forbidden, and the file survives. - File importedForDelete = writeFile(dir, "imported-delete.txt"); - importFileIntoRun(_folder, importedForDelete); - MockHttpServletResponse deleteResp = doDelete(_folder, filesPath(_folder).append("imported-delete.txt"), getAdmin()); + File imported = writeFile(dir, "imported.txt"); + importFileIntoRun(_folder, imported); + MockHttpServletResponse deleteResp = doDelete(_folder, filesPath(_folder).append(imported.getName()), getAdmin()); assertEquals("DELETE of a file imported by an assay must be forbidden", HttpServletResponse.SC_FORBIDDEN, deleteResp.getStatus()); - assertTrue("File must still exist after a forbidden delete", importedForDelete.exists()); + assertTrue("File must still exist after a forbidden delete", imported.exists()); - // MOVE of an equally-imported file succeeds, relocating it within the file root. - File importedForMove = writeFile(dir, "imported-move.txt"); - importFileIntoRun(_folder, importedForMove); - Path src = filesPath(_folder).append("imported-move.txt"); + // MOVE of an imported file succeeds, relocating it within the file root. + Path src = filesPath(_folder).append(imported.getName()); Path dest = filesPath(_folder).append("moved.txt"); MockHttpServletResponse moveResp = doMove(_folder, src, dest, getAdmin()); assertEquals("MOVE of a file imported by an assay must succeed", HttpServletResponse.SC_CREATED, moveResp.getStatus()); - assertFalse("Source file should no longer exist after a successful move", importedForMove.exists()); - assertTrue("Destination file should exist after a successful move", new File(dir, "moved.txt").exists()); + assertFalse("Source file should no longer exist after a successful move", imported.exists()); + assertTrue("Destination file should exist after a successful move", FileUtil.appendName(dir, dest.getName()).exists()); } - // Regression guard for the canMove()/canDelete() divergence introduced alongside the MOVE fix. A - // FileSystemResource subclass whose canDelete() forbids deletion outright (PipelineFolderResource returns - // false) must also forbid MOVE. Otherwise the new FileSystemResource.canMove() default - Delete permission - // plus a writable file - would let an admin relocate a node that was never deletable, because that default - // calls super.canDelete() and so bypasses the subclass override entirely. + // Ensure special nodes like @pipeline can't be deleted or moved @Test public void testNonDeletableNodeIsNotMovable() throws Exception { // Configure an explicit pipeline root so the @pipeline webdav node resolves to a PipelineFolderResource. File fileRoot = ensureFilesDir(_folder).getParentFile(); - File pipelineDir = new File(fileRoot, "pipelineOverrideRoot"); + File pipelineDir = FileUtil.appendName(fileRoot, "pipelineOverrideRoot"); if (!pipelineDir.exists()) assertTrue("Test requires a writable pipeline root directory", pipelineDir.mkdirs()); PipelineService.get().setPipelineRoot(getAdmin(), _folder, PipelineService.PRIMARY_ROOT, false, pipelineDir.toURI()); @@ -6800,9 +6790,6 @@ public void testNonDeletableNodeIsNotMovable() throws Exception assertNotNull("The pipeline node must be backed by a writable file root", pipelineNode.getFile()); User admin = getAdmin(); - // An admin has Delete permission and the pipeline root is writable, so the inherited - // FileSystemResource.canMove() would return true. The override must keep canMove() aligned with the - // categorical canDelete()==false, so the node remains immovable. assertFalse("The pipeline node must never be deletable", pipelineNode.canDelete(admin, true, null)); assertFalse("A node that is not deletable must also not be movable", pipelineNode.canMove(admin)); } From 489b41f4427697a0cbff58031fb8b3200ab75931 Mon Sep 17 00:00:00 2001 From: Alan Vezina Date: Thu, 18 Jun 2026 17:07:30 -0500 Subject: [PATCH 13/13] Delete Assay DeleteAction (#7770) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Rationale The Assay DeleteAction is not used by our UI and is no longer needed. ## Related Pull Requests - ## Changes - Delete assay/actions/DeleteAction.java - Remove getDeleteDesignUrl from AssayUrls and AssayController ## Tasks 📍 - [x] Claude Code Review - [x] Manual Testing - [ ] ~Test Automation~ - [x] Verify Fix --- api/src/org/labkey/api/assay/AssayUrls.java | 2 - .../api/assay/actions/AssayHeaderView.java | 11 --- .../src/org/labkey/assay/AssayController.java | 10 --- .../labkey/assay/actions/DeleteAction.java | 74 ------------------- 4 files changed, 97 deletions(-) delete mode 100644 assay/src/org/labkey/assay/actions/DeleteAction.java diff --git a/api/src/org/labkey/api/assay/AssayUrls.java b/api/src/org/labkey/api/assay/AssayUrls.java index 805954917d0..ac91e567f8d 100644 --- a/api/src/org/labkey/api/assay/AssayUrls.java +++ b/api/src/org/labkey/api/assay/AssayUrls.java @@ -61,8 +61,6 @@ public interface AssayUrls extends UrlProvider ActionURL getChooseCopyDestinationURL(ExpProtocol protocol, Container container); - ActionURL getDeleteDesignURL(ExpProtocol protocol); - /** * Returns the URL for the assay import data wizard for an existing assay definition. * path and files may be null, in which case it is assumed that the POST will include data object RowIds diff --git a/api/src/org/labkey/api/assay/actions/AssayHeaderView.java b/api/src/org/labkey/api/assay/actions/AssayHeaderView.java index 1f2ffc8cfdc..8e2573c6e90 100644 --- a/api/src/org/labkey/api/assay/actions/AssayHeaderView.java +++ b/api/src/org/labkey/api/assay/actions/AssayHeaderView.java @@ -94,17 +94,6 @@ public List getLinks() return links; } - public static String getDeleteOnClick(ExpProtocol protocol, Container currentContainer) - { - ActionURL deleteURL = PageFlowUtil.urlProvider(AssayUrls.class).getDeleteDesignURL(protocol); - String extraWarning = ""; - if (!protocol.getContainer().equals(currentContainer)) - { - extraWarning = " It is defined in " + protocol.getContainer().getPath() + " and deleting it will remove it from all subfolders."; - } - return "if (window.confirm('Are you sure you want to delete this assay design and all of its runs?" + extraWarning + "')) { window.location = '" + deleteURL + "' }"; - } - public ExpProtocol getProtocol() { return _protocol; diff --git a/assay/src/org/labkey/assay/AssayController.java b/assay/src/org/labkey/assay/AssayController.java index 6f3c5819eed..b87693ba4ec 100644 --- a/assay/src/org/labkey/assay/AssayController.java +++ b/assay/src/org/labkey/assay/AssayController.java @@ -139,7 +139,6 @@ import org.labkey.assay.actions.AssayBatchDetailsAction; import org.labkey.assay.actions.AssayBatchesAction; import org.labkey.assay.actions.AssayResultsAction; -import org.labkey.assay.actions.DeleteAction; import org.labkey.assay.actions.DeleteProtocolAction; import org.labkey.assay.actions.GetAssayBatchAction; import org.labkey.assay.actions.GetAssayBatchesAction; @@ -174,7 +173,6 @@ import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.ReaderRole; -import org.labkey.api.util.JunitUtil; import org.labkey.api.util.TestContext; import org.labkey.api.view.ViewBackgroundInfo; import org.labkey.api.view.ViewContext; @@ -212,7 +210,6 @@ public class AssayController extends SpringActionController AssayResultsAction.class, AssayRunDetailsAction.class, AssayRunsAction.class, - DeleteAction.class, DeleteProtocolAction.class, DesignerAction.class, GetAssayBatchAction.class, @@ -444,7 +441,6 @@ private static Map serializeAssayLinks(ExpProtocol protocol, Ass links.put("batches", urlProvider.getAssayBatchesURL(c, protocol, null)); links.put("begin", urlProvider.getProtocolURL(c, protocol, AssayBeginAction.class)); links.put("designCopy", urlProvider.getDesignerURL(c, protocol, true, null)); - links.put("designDelete", urlProvider.getDeleteDesignURL(protocol)); links.put("designEdit", urlProvider.getDesignerURL(c, protocol, false, null)); links.put("import", provider.getImportURL(c, protocol)); links.put("results", urlProvider.getAssayResultsURL(c, protocol)); @@ -1175,12 +1171,6 @@ public ActionURL getChooseCopyDestinationURL(ExpProtocol protocol, Container con return getProtocolURL(container, protocol, ChooseCopyDestinationAction.class); } - @Override - public ActionURL getDeleteDesignURL(ExpProtocol protocol) - { - return getProtocolURL(protocol.getContainer(), protocol, DeleteAction.class); - } - @Override public ActionURL getImportURL(Container container, ExpProtocol protocol, String path, File[] files) { diff --git a/assay/src/org/labkey/assay/actions/DeleteAction.java b/assay/src/org/labkey/assay/actions/DeleteAction.java deleted file mode 100644 index adae1bb54cc..00000000000 --- a/assay/src/org/labkey/assay/actions/DeleteAction.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright (c) 2019 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.assay.actions; - -import org.labkey.api.assay.AssayUrls; -import org.labkey.api.assay.actions.BaseAssayAction; -import org.labkey.api.assay.actions.ProtocolIdForm; -import org.labkey.api.assay.security.DesignAssayPermission; -import org.labkey.api.exp.api.ExpProtocol; -import org.labkey.api.security.RequiresPermission; -import org.labkey.api.security.User; -import org.labkey.api.security.permissions.DeletePermission; -import org.labkey.api.security.permissions.ReadPermission; -import org.labkey.api.util.PageFlowUtil; -import org.labkey.api.view.RedirectException; -import org.labkey.api.view.UnauthorizedException; -import org.labkey.api.view.ViewContext; -import org.springframework.validation.BindException; -import org.springframework.web.servlet.ModelAndView; - -import java.util.Set; - -/** - * User: brittp -* Date: Jul 26, 2007 -* Time: 7:23:24 PM -*/ -@RequiresPermission(ReadPermission.class) //will check explicitly in code below -public class DeleteAction extends BaseAssayAction -{ - @Override - public ModelAndView getView(ProtocolIdForm protocolIdForm, BindException errors) - { - ExpProtocol protocol; - try - { - protocol = protocolIdForm.getProtocol(); - } - catch (ProtocolIdForm.ProviderNotFoundException e) - { - // We're OK attempting to delete even though the AssayProvider can't be found - protocol = e.getProtocol(); - } - - if(!allowDelete(protocol)) - throw new UnauthorizedException("You do not have sufficient permissions to delete this assay design."); - - protocol.delete(getUser()); - throw new RedirectException(PageFlowUtil.urlProvider(AssayUrls.class).getAssayListURL(getContainer())); - } - - private boolean allowDelete(ExpProtocol protocol) - { - ViewContext ctx = getViewContext(); - User user = ctx.getUser(); - - //user must have both design assay AND delete permission, as this will delete both the design and uploaded data - return protocol.getContainer().hasPermissions(user, Set.of(DesignAssayPermission.class, DeletePermission.class)); - } -}