From 4209ee79c953d3095e3ad52de5aefa994bec4a6c Mon Sep 17 00:00:00 2001 From: Josh Eckels Date: Mon, 15 Jun 2026 09:50:13 -0700 Subject: [PATCH 1/4] Use centralized XML parser config (#1024) #### Rationale We want consistent XML parser config #### Related Pull Requests * https://github.com/LabKey/platform/pull/7751 #### Changes * Use central XML parser config * Clarify permissions on protein naming --- .../org/systemsbiology/jrap/MSXMLParser.java | 61 ++----------------- .../protein/annotation/XMLProteinHandler.java | 21 ++++--- .../org/labkey/protein/ProteinController.java | 10 +-- 3 files changed, 22 insertions(+), 70 deletions(-) diff --git a/ms2/src/org/systemsbiology/jrap/MSXMLParser.java b/ms2/src/org/systemsbiology/jrap/MSXMLParser.java index afdbcd57be..a4a83ecfcc 100644 --- a/ms2/src/org/systemsbiology/jrap/MSXMLParser.java +++ b/ms2/src/org/systemsbiology/jrap/MSXMLParser.java @@ -40,8 +40,8 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.labkey.api.util.XmlBeansUtil; import org.xml.sax.*; -import org.xml.sax.helpers.XMLReaderFactory; import java.io.FileInputStream; import java.io.File; @@ -136,35 +136,22 @@ public MSXMLParser(File file) { this.file = file; // The defaults for the parser - boolean namespaces = DEFAULT_NAMESPACES; boolean namespacePrefixes = DEFAULT_NAMESPACE_PREFIXES; - boolean validation = DEFAULT_VALIDATION; - boolean schemaValidation = DEFAULT_SCHEMA_VALIDATION; - boolean schemaFullChecking = DEFAULT_SCHEMA_FULL_CHECKING; - boolean dynamicValidation = DEFAULT_DYNAMIC_VALIDATION; // Create a new index handler indexHandler = new SAX2IndexHandler(); - // Create parser + // Create parser using the centrally-configured, XXE-safe factory try { - parser = XMLReaderFactory.createXMLReader(DEFAULT_PARSER_NAME); + parser = XmlBeansUtil.SAX_PARSER_FACTORY_ALLOWING_DOCTYPE.newSAXParser().getXMLReader(); } catch (Exception e) { - System.err.println("error: Unable to instantiate parser (" - + DEFAULT_PARSER_NAME + ")"); + System.err.println("error: Unable to instantiate parser"); } - // Set parser features - try - { - parser.setFeature(NAMESPACES_FEATURE_ID, namespaces); - } catch (SAXException e) - { - System.err.println("warning: Parser does not support feature (" - + NAMESPACES_FEATURE_ID + ")"); - } + // Set parser features. Namespaces and validation are configured by the factory; only the + // remaining, non-security-relevant features need to be set here. try { parser.setFeature(NAMESPACE_PREFIXES_FEATURE_ID, namespacePrefixes); @@ -174,42 +161,6 @@ public MSXMLParser(File file) { + NAMESPACE_PREFIXES_FEATURE_ID + ")"); } try - { - parser.setFeature(VALIDATION_FEATURE_ID, validation); - } catch (SAXException e) - { - System.err.println("warning: Parser does not support feature (" - + VALIDATION_FEATURE_ID + ")"); - } - try - { - parser.setFeature(SCHEMA_VALIDATION_FEATURE_ID, schemaValidation); - } catch (SAXNotRecognizedException | SAXNotSupportedException e) - { - System.err.println("warning: Parser does not support feature (" - + SCHEMA_VALIDATION_FEATURE_ID + ")"); - - } - try - { - parser.setFeature(SCHEMA_FULL_CHECKING_FEATURE_ID, - schemaFullChecking); - } catch (SAXNotRecognizedException | SAXNotSupportedException e) - { - System.err.println("warning: Parser does not support feature (" - + SCHEMA_FULL_CHECKING_FEATURE_ID + ")"); - - } - try - { - parser.setFeature(DYNAMIC_VALIDATION_FEATURE_ID, dynamicValidation); - } catch (SAXNotRecognizedException | SAXNotSupportedException e) - { - System.err.println("warning: Parser does not support feature (" - + DYNAMIC_VALIDATION_FEATURE_ID + ")"); - - } - try { parser.setFeature(CONT_AFTER_FATAL_ERROR_ID, false); } catch (SAXNotRecognizedException | SAXNotSupportedException e) diff --git a/protein/api-src/org/labkey/api/protein/annotation/XMLProteinHandler.java b/protein/api-src/org/labkey/api/protein/annotation/XMLProteinHandler.java index 83b8898547..9fd53b4881 100644 --- a/protein/api-src/org/labkey/api/protein/annotation/XMLProteinHandler.java +++ b/protein/api-src/org/labkey/api/protein/annotation/XMLProteinHandler.java @@ -19,12 +19,13 @@ import org.labkey.api.protein.uniprot.ParseActions; import org.labkey.api.protein.uniprot.ParseContext; import org.labkey.api.protein.uniprot.ParserTree; +import org.labkey.api.util.XmlBeansUtil; import org.xml.sax.Attributes; import org.xml.sax.SAXException; import org.xml.sax.XMLReader; import org.xml.sax.helpers.DefaultHandler; -import org.xml.sax.helpers.XMLReaderFactory; +import javax.xml.parsers.ParserConfigurationException; import java.io.File; import java.io.IOException; import java.sql.Connection; @@ -111,17 +112,21 @@ public XMLProteinHandler(Connection conn, XMLProteinLoader loader) throws SAXExc ) ); - // create parser - setParser(XMLReaderFactory.createXMLReader(DEFAULT_PARSER_NAME)); + // create parser using the centrally-configured, XXE-safe factory + try + { + setParser(XmlBeansUtil.SAX_PARSER_FACTORY.newSAXParser().getXMLReader()); + } + catch (ParserConfigurationException e) + { + throw new SAXException(e); + } try { - getParser().setFeature(NAMESPACES_FEATURE_ID, DEFAULT_NAMESPACES); + // Namespaces and validation are configured by the factory; only the remaining, + // non-security-relevant features need to be set here. getParser().setFeature(NAMESPACE_PREFIXES_FEATURE_ID, DEFAULT_NAMESPACE_PREFIXES); - getParser().setFeature(VALIDATION_FEATURE_ID, DEFAULT_VALIDATION); - getParser().setFeature(SCHEMA_VALIDATION_FEATURE_ID, DEFAULT_SCHEMA_VALIDATION); - getParser().setFeature(SCHEMA_FULL_CHECKING_FEATURE_ID, DEFAULT_SCHEMA_FULL_CHECKING); - getParser().setFeature(DYNAMIC_VALIDATION_FEATURE_ID, DEFAULT_DYNAMIC_VALIDATION); } catch (Exception e) { diff --git a/protein/src/org/labkey/protein/ProteinController.java b/protein/src/org/labkey/protein/ProteinController.java index 323d00f720..c1a7c9af65 100644 --- a/protein/src/org/labkey/protein/ProteinController.java +++ b/protein/src/org/labkey/protein/ProteinController.java @@ -1101,7 +1101,7 @@ public void setNameType(String nameType) } } - @RequiresPermission(AdminPermission.class) + @RequiresPermission(AdminOperationsPermission.class) public static class SetBestNameAction extends FormHandlerAction { @Override @@ -1424,11 +1424,6 @@ public void testActionPermissions() ProteinController controller = new ProteinController(); - // @RequiresPermission(AdminPermission.class) - assertForAdminPermission(user, - new SetBestNameAction() - ); - // @RequiresSiteAdmin assertForRequiresSiteAdmin(user, controller.new LoadGoAction(), @@ -1445,7 +1440,8 @@ controller.new ShowAnnotInsertDetailsAction() // @AdminConsoleAction // @RequiresPermission(AdminOperationsPermission.class) assertForAdminOperationsPermission(ContainerManager.getRoot(), user, - controller.new ShowProteinAdminAction() + controller.new ShowProteinAdminAction(), + new SetBestNameAction() ); } } From c52147eecbc446aaf79c759616f989d1c2500342 Mon Sep 17 00:00:00 2001 From: Nick Kerr Date: Mon, 15 Jun 2026 10:52:48 -0700 Subject: [PATCH 2/4] Container scope (#1023) --- .../labkey/microarray/MicroarrayManager.java | 121 +++++++++------ .../labkey/microarray/MicroarrayModule.java | 13 +- .../FeatureAnnotationSetController.java | 143 +++++++++++++++++- 3 files changed, 223 insertions(+), 54 deletions(-) diff --git a/microarray/src/org/labkey/microarray/MicroarrayManager.java b/microarray/src/org/labkey/microarray/MicroarrayManager.java index 5f5531089f..cd84bf16ec 100644 --- a/microarray/src/org/labkey/microarray/MicroarrayManager.java +++ b/microarray/src/org/labkey/microarray/MicroarrayManager.java @@ -21,11 +21,15 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; +import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.JdbcType; +import org.labkey.api.data.Results; +import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SqlExecutor; @@ -50,20 +54,25 @@ import org.labkey.api.reader.DataLoader; import org.labkey.api.reader.TabLoader; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.util.ContainerUtil; import org.labkey.api.util.FileUtil; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.view.UnauthorizedException; import org.labkey.microarray.controllers.FeatureAnnotationSetController; import org.labkey.microarray.matrix.ExpressionMatrixProtocolSchema; import org.labkey.microarray.query.MicroarrayUserSchema; import java.io.File; import java.sql.SQLException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; public class MicroarrayManager { @@ -87,7 +96,7 @@ private static TableInfo getAnnotationQueryTableInfo(User user, Container contai return schema.getTable(MicroarrayUserSchema.TABLE_FEATURE_ANNOTATION, false); } - private static TableInfo getAnnotationSetSchemaTableInfo() + public static TableInfo getAnnotationSetSchemaTableInfo() { DbSchema schema = MicroarrayUserSchema.getSchema(); return schema.getTable(MicroarrayUserSchema.TABLE_FEATURE_ANNOTATION_SET); @@ -105,32 +114,22 @@ public long featureAnnotationSetCount(Container c) return selector.getRowCount(); } - public int deleteFeatureAnnotationSet(int... rowId) + public int deleteFeatureAnnotationSet(Collection ids) { - DbScope scope = MicroarrayUserSchema.getSchema().getScope(); - - Integer[] ids = ArrayUtils.toObject(rowId); - - try (DbScope.Transaction tx = scope.ensureTransaction()) + try (DbScope.Transaction tx = MicroarrayUserSchema.getSchema().getScope().ensureTransaction()) { // Delete all annotations first. - TableInfo annotationSchemaTableInfo = getAnnotationSchemaTableInfo(); - SimpleFilter filter = new SimpleFilter(); - filter.addInClause(FieldKey.fromParts("FeatureAnnotationSetId"), Arrays.asList(ids)); - int rowsDeleted = Table.delete(annotationSchemaTableInfo, filter); + int rowsDeleted = Table.delete(getAnnotationSchemaTableInfo(), new SimpleFilter(FieldKey.fromParts("FeatureAnnotationSetId"), ids, CompareType.IN)); // Then delete annotation set. - TableInfo annotationSetSchemaTableInfo = getAnnotationSetSchemaTableInfo(); - filter = new SimpleFilter(); - filter.addInClause(FieldKey.fromParts("RowId"), Arrays.asList(ids)); - Table.delete(annotationSetSchemaTableInfo, filter); + Table.delete(getAnnotationSetSchemaTableInfo(), new SimpleFilter(FieldKey.fromParts("RowId"), ids, CompareType.IN)); tx.commit(); return rowsDeleted; } } - private Integer insertFeatureAnnotationSet(User user, Container container, String name, String vendor, String description, String comment, BatchValidationException errors) + public Integer insertFeatureAnnotationSet(User user, Container container, String name, String vendor, String description, String comment, BatchValidationException errors) throws SQLException, BatchValidationException, QueryUpdateServiceException, DuplicateKeyException { MicroarrayUserSchema schema = new MicroarrayUserSchema(user, container); @@ -175,7 +174,7 @@ private Integer insertFeatureAnnotations(User user, Container container, Integer return -1; } - /** Creates feature annotation set AND inserts all feature annotations from TSV */ + /** Creates a feature annotation set AND inserts all feature annotations from TSV */ public Integer createFeatureAnnotationSet(User user, Container c, FeatureAnnotationSetController.FeatureAnnotationSetForm form, DataLoader loader, BatchValidationException errors) throws SQLException, BatchValidationException, QueryUpdateServiceException, DuplicateKeyException { @@ -187,48 +186,82 @@ public Integer createFeatureAnnotationSet(User user, Container c, FeatureAnnotat return -1; } - /** - * Get feature annotation set by name if it is in scope (current, project, and shared container). - */ - @Nullable - public Integer getFeatureAnnotationSet(Container c, User user, String featureSetName) + private void applyContainerFilter(Container c, User user, SimpleFilter filter) { - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("Name"), featureSetName); - // The container filter matches the assay's featureSet run property lookup ContainerFilter cf = new ContainerFilter.CurrentPlusProjectAndShared(c, user); filter.addClause(cf.createFilterClause(MicroarrayUserSchema.getSchema(), FieldKey.fromParts("container"))); + } + + /** + * Get a feature annotation set by name if it is in scope (current, project, and shared container). + */ + public @Nullable Integer getFeatureAnnotationSet(Container c, User user, String featureSetName) + { + return getFeatureAnnotationSet(c, user, new SimpleFilter(FieldKey.fromParts("Name"), featureSetName)); + } + + /** + * Get a feature annotation set by id if it is in scope (current, project, and shared container). + */ + public @Nullable Integer getFeatureAnnotationSet(Container c, User user, int id) + { + return getFeatureAnnotationSet(c, user, new SimpleFilter(FieldKey.fromParts("RowId"), id)); + } + + private @Nullable Integer getFeatureAnnotationSet(Container c, User user, SimpleFilter filter) + { + applyContainerFilter(c, user, filter); + List rowIds = new TableSelector(getAnnotationSetSchemaTableInfo(), PageFlowUtil.set("RowId"), filter, null).getArrayList(Integer.class); - TableSelector featureAnnotationSelector = new TableSelector(getAnnotationSetSchemaTableInfo(), PageFlowUtil.set("RowId"), filter, null); - List rowIds = featureAnnotationSelector.getArrayList(Integer.class); // TODO: Order results by container depth - if (!rowIds.isEmpty()) - return rowIds.get(0); + if (rowIds.isEmpty()) + return null; - return null; + return rowIds.get(0); } /** - * Get feature annotation set by id if it is in scope (current, project, and shared container). + * Gets feature annotation sets by id that are in folder scope (current, project, and shared container). The user + * must have the provided permissions in all containers that the feature annotation sets are declared in. */ - @Nullable - public Integer getFeatureAnnotationSet(Container c, User user, int id) + public @NotNull List getFeatureAnnotationSets(Container c, User user, Collection rowIds, Class permission) { - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("RowId"), id); + if (rowIds == null || rowIds.isEmpty()) + return Collections.emptyList(); - // The container filter matches the assay's featureSet run property lookup - ContainerFilter cf = new ContainerFilter.CurrentPlusProjectAndShared(c, user); - filter.addClause(cf.createFilterClause(MicroarrayUserSchema.getSchema(), FieldKey.fromParts("container"))); + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("RowId"), rowIds, CompareType.IN); + applyContainerFilter(c, user, filter); - TableSelector featureAnnotationSelector = new TableSelector(getAnnotationSetSchemaTableInfo(), PageFlowUtil.set("RowId"), filter, null); - List rowIds = featureAnnotationSelector.getArrayList(Integer.class); - // TODO: Order results by container depth - if (!rowIds.isEmpty()) - return rowIds.get(0); + List setRowIds = new ArrayList<>(); + Set processed = new HashSet<>(); + try (Results results = new TableSelector(getAnnotationSetSchemaTableInfo(), PageFlowUtil.set("RowId", "Container"), filter, null).getResults()) + { + while (results.next()) + { + Integer rowId = results.getInt(FieldKey.fromParts("RowId")); + String containerId = results.getString(FieldKey.fromParts("Container")); - return null; + if (!processed.contains(containerId)) + { + Container container = ContainerManager.getForId(containerId); + if (container == null) + throw new UnauthorizedException("Unable to determine container for feature annotation set (" + rowId + ")"); + if (!container.hasPermission(user, permission)) + throw new UnauthorizedException("You do not have sufficient permission in " + container.getPath() + " for feature annotation set (" + rowId + ")"); + + processed.add(containerId); + } + + setRowIds.add(rowId); + } + } + catch (SQLException e) + { + throw new RuntimeSQLException(e); + } + + return Collections.unmodifiableList(setRowIds); } /** diff --git a/microarray/src/org/labkey/microarray/MicroarrayModule.java b/microarray/src/org/labkey/microarray/MicroarrayModule.java index d8f02790f9..f78168331a 100644 --- a/microarray/src/org/labkey/microarray/MicroarrayModule.java +++ b/microarray/src/org/labkey/microarray/MicroarrayModule.java @@ -44,15 +44,14 @@ public class MicroarrayModule extends SpringModule { private static final String WEBPART_FEATURE_ANNOTATION_SET = "Feature Annotation Sets"; - private static final String FEATURE_ANNOTATION_SET_CONTROLLER_NAME = "feature-annotationset"; - public static final String DB_SCHEMA_NAME = "microarray"; + public static final String NAME = "Microarray"; @Override public String getName() { - return "Microarray"; + return NAME; } @Override @@ -121,4 +120,12 @@ public Set getSchemaNames() { return Collections.singleton(DB_SCHEMA_NAME); } + + @Override + public @NotNull Set getIntegrationTests() + { + return Set.of( + FeatureAnnotationSetController.ContainerScopingTestCase.class + ); + } } diff --git a/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java b/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java index 8aa00bc4f7..812f05ed45 100644 --- a/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java +++ b/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java @@ -15,7 +15,11 @@ */ package org.labkey.microarray.controllers; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.lang3.StringUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.SimpleRedirectAction; import org.labkey.api.action.SimpleViewAction; @@ -27,10 +31,10 @@ import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DataRegion; import org.labkey.api.data.DataRegionSelection; -import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TableSelector; import org.labkey.api.query.BatchValidationException; import org.labkey.api.query.DuplicateKeyException; import org.labkey.api.query.FieldKey; @@ -40,12 +44,19 @@ import org.labkey.api.query.QueryUpdateServiceException; import org.labkey.api.query.QueryView; import org.labkey.api.query.ValidationException; +import org.labkey.api.module.Module; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.reader.DataLoader; 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.DeletePermission; 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.ReaderRole; +import org.labkey.api.test.TestWhen; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; @@ -58,6 +69,7 @@ import org.labkey.api.view.ViewForm; import org.labkey.api.view.WebPartView; import org.labkey.microarray.MicroarrayManager; +import org.labkey.microarray.MicroarrayModule; import org.labkey.microarray.query.MicroarrayUserSchema; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -67,7 +79,10 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Set; /** * User: kevink @@ -118,9 +133,9 @@ public static class DeleteAction extends FormViewAction deletableRowIds = MicroarrayManager.get().getFeatureAnnotationSets(getContainer(), getUser(), form.getIds(false), DeletePermission.class); + if (!deletableRowIds.isEmpty()) + MicroarrayManager.get().deleteFeatureAnnotationSet(deletableRowIds); // TODO catch somewhere on attempting to delete one that is in use, prompt to cascade the delete // Similarly, deleting a referenced sample type currently throws an FK exception. again, deal with it @@ -355,13 +370,14 @@ public void setRowId(Integer rowId) private String _dataRegionSelectionKey; private Integer _singleObjectRowId; - public int[] getIds(boolean clear) + public Set getIds(boolean clear) { if (_singleObjectRowId != null) { - return new int[] {_singleObjectRowId}; + return Set.of(_singleObjectRowId); } - return PageFlowUtil.toInts(DataRegionSelection.getSelected(getViewContext(), clear)); + + return DataRegionSelection.getSelectedIntegers(getViewContext(), clear); } public Integer getSingleObjectRowId() @@ -467,4 +483,117 @@ public void setTargetContainer(int container) } } + @TestWhen(TestWhen.When.BVT) + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + // Executed in its own project due to folder scoping rules for microarray + private static final String PROJECT_NAME = "FeatureAnnotationContainerScopingTestCase Project"; + + private Container project; + private Container folderA; + private int projectSetRowId; + + @Before + public void setUp() throws Exception + { + deleteProject(); + project = activateModules(ContainerManager.ensureContainer(PROJECT_NAME, getAdmin())); + folderA = activateModules(ContainerManager.ensureContainer(project, "Folder A", getAdmin())); + projectSetRowId = insertFeatureAnnotationSet(project, "Project Set"); + } + + @After + public void tearDown() + { + deleteProject(); + } + + private void deleteProject() + { + Container c = ContainerManager.getForPath(PROJECT_NAME); + if (c != null) + ContainerManager.deleteAll(c, getAdmin()); + } + + private Container activateModules(Container c) + { + Set modules = new HashSet<>(c.getActiveModules(getAdmin())); + modules.add(ModuleLoader.getInstance().getModule(MicroarrayModule.NAME)); + c.setActiveModules(modules, getAdmin()); + return c; + } + + @Test + public void testDeleteActionCannotDeleteForeignSet() throws Exception + { + // The caller is an Editor (has DeletePermission) in folderA only, with no access to the project. + User editor = createUserInRole(folderA, EditorRole.class); + + // Negative control: the caller can't read the project, so the project set is out of scope and survives. + post(deleteUrl(folderA, projectSetRowId), editor); + assertTrue("A feature annotation set the caller cannot see must not be deleted", + featureAnnotationSetExists(projectSetRowId)); + + // Positive control: deleting a set that actually lives in the caller's own folder succeeds. + int folderASetRowId = insertFeatureAnnotationSet(folderA, "Folder A Set"); + post(deleteUrl(folderA, folderASetRowId), editor); + assertFalse("A feature annotation set in the caller's own folder should be deleted", + featureAnnotationSetExists(folderASetRowId)); + } + + @Test + public void testDeleteActionRejectsInScopeSetWithoutDeletePermission() throws Exception + { + // Editor in folderA (passes the action's DeletePermission check) plus Reader in the project. + User editor = createUserInRole(folderA, EditorRole.class); + grantRole(editor, project, ReaderRole.class); + + // The project set is now in scope, but the caller can't delete it there, so the action rejects the request. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(deleteUrl(folderA, projectSetRowId), editor)); + assertTrue("A set the caller may read but not delete must not be deleted", + featureAnnotationSetExists(projectSetRowId)); + } + + @Test + public void testDeleteActionRejectsBatchContainingUndeletableSet() throws Exception + { + User editor = createUserInRole(folderA, EditorRole.class); + grantRole(editor, project, ReaderRole.class); + int folderASetRowId = insertFeatureAnnotationSet(folderA, "Folder A Set"); + + // A multi-select delete mixing a deletable set with one in a Reader-only folder must be rejected wholesale. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(deleteUrl(folderA, folderASetRowId, projectSetRowId), editor)); + + // Fail-closed: neither set is deleted + assertTrue("Deletable set in a rejected batch must be preserved", featureAnnotationSetExists(folderASetRowId)); + assertTrue("Undeletable set in another container must be preserved", featureAnnotationSetExists(projectSetRowId)); + } + + private static ActionURL deleteUrl(Container c, int rowId) + { + return new ActionURL(DeleteAction.class, c).addParameter("singleObjectRowId", rowId); + } + + private static ActionURL deleteUrl(Container c, int... rowIds) + { + ActionURL url = new ActionURL(DeleteAction.class, c); + for (int rowId : rowIds) + url.addParameter(DataRegion.SELECT_CHECKBOX_NAME, rowId); + return url; + } + + private int insertFeatureAnnotationSet(Container c, String name) throws Exception + { + BatchValidationException errors = new BatchValidationException(); + Integer rowId = MicroarrayManager.get().insertFeatureAnnotationSet(getAdmin(), c, name, "Test", null, null, errors); + if (errors.hasErrors()) + throw errors; + return rowId; + } + + private boolean featureAnnotationSetExists(int rowId) + { + return new TableSelector(MicroarrayManager.getAnnotationSetSchemaTableInfo(), new SimpleFilter(FieldKey.fromParts("RowId"), rowId), null).exists(); + } + } } From 3537cba2f38562f83d3880db17edbc574b2ed143 Mon Sep 17 00:00:00 2001 From: Karl Lum Date: Mon, 15 Jun 2026 11:09:00 -0700 Subject: [PATCH 3/4] 25.7 fb container scoping (#1022) #### Rationale Add permission checks for objects referenced by rowID. - Flow module updates with FlowManager integration tests. - NAb module updates. - Elispot module updates - Luminex module updates #### 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/elispot/ElispotController.java | 10 ++ flow/src/org/labkey/flow/FlowModule.java | 5 +- .../editscript/EditScriptForm.java | 4 +- .../AnalysisScriptController.java | 79 +++++++++- .../controllers/protocol/ProtocolForm.java | 2 +- .../flow/controllers/run/RunController.java | 8 + .../flow/controllers/well/WellController.java | 137 ++++++++++++++-- .../org/labkey/flow/data/FlowProtocol.java | 17 +- .../org/labkey/flow/persist/FlowManager.java | 148 ++++++++++++++++++ .../labkey/luminex/query/GuideSetTable.java | 14 +- .../tests/luminex/LuminexGuideSetTest.java | 72 +++++++++ .../org/labkey/nab/NabAssayController.java | 60 ++++++- 12 files changed, 528 insertions(+), 28 deletions(-) diff --git a/elispotassay/src/org/labkey/elispot/ElispotController.java b/elispotassay/src/org/labkey/elispot/ElispotController.java index 3c357b789e..888a37b528 100644 --- a/elispotassay/src/org/labkey/elispot/ElispotController.java +++ b/elispotassay/src/org/labkey/elispot/ElispotController.java @@ -16,6 +16,7 @@ package org.labkey.elispot; +import org.apache.commons.lang3.math.NumberUtils; import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; @@ -543,6 +544,15 @@ public boolean handlePost(Object o, BindException errors) throws Exception Set selections = DataRegionSelection.getSelected(getViewContext(), true); if (!selections.isEmpty()) { + // GitHub Kanban #1892: Verify each selected run belongs to the current container before queuing + for (String selection : selections) + { + int rowId = NumberUtils.toInt(selection, -1); + ExpRun run = rowId != -1 ? ExperimentService.get().getExpRun(rowId, getContainer()) : null; + if (run == null) + throw new NotFoundException("Run " + selection + " does not exist."); + } + ViewBackgroundInfo info = new ViewBackgroundInfo(getContainer(), getUser(), getViewContext().getActionURL()); BackgroundSubtractionJob job = new BackgroundSubtractionJob(ElispotPipelineProvider.NAME, info, PipelineService.get().findPipelineRoot(getContainer()), selections); diff --git a/flow/src/org/labkey/flow/FlowModule.java b/flow/src/org/labkey/flow/FlowModule.java index cbe2d66b21..211007e1f1 100644 --- a/flow/src/org/labkey/flow/FlowModule.java +++ b/flow/src/org/labkey/flow/FlowModule.java @@ -342,7 +342,10 @@ public Set getIntegrationTests() FlowController.TestCase.class, FlowProtocol.TestCase.class, PersistTests.class, - FlowManager.TestCase.class + FlowManager.TestCase.class, + FlowManager.ContainerScopingTestCase.class, + WellController.WellContainerScopingTestCase.class, + AnalysisScriptController.AnalysisScriptContainerScopingTestCase.class ); } diff --git a/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java b/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java index b8632153ff..357673f0d3 100644 --- a/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java +++ b/flow/src/org/labkey/flow/controllers/editscript/EditScriptForm.java @@ -16,8 +16,8 @@ package org.labkey.flow.controllers.editscript; -import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.fhcrc.cpas.flow.script.xml.ScriptDocument; import org.jetbrains.annotations.NotNull; import org.labkey.api.security.permissions.UpdatePermission; @@ -92,6 +92,8 @@ public void reset() { throw new NotFoundException("scriptId not found: " + scriptIdStr); } + // GitHub Issue #1892: validate container + flowObject.checkContainer(getContainer(), getUser(), getViewContext().getActionURL()); _runCount = flowObject.getRunCount(); step = FlowProtocolStep.fromRequest(getRequest()); _run = FlowRun.fromURL(getViewContext().getActionURL(), getRequest(), getViewContext().getContainer(), getUser()); diff --git a/flow/src/org/labkey/flow/controllers/executescript/AnalysisScriptController.java b/flow/src/org/labkey/flow/controllers/executescript/AnalysisScriptController.java index 8089be37c2..8e901c4657 100644 --- a/flow/src/org/labkey/flow/controllers/executescript/AnalysisScriptController.java +++ b/flow/src/org/labkey/flow/controllers/executescript/AnalysisScriptController.java @@ -20,8 +20,11 @@ import org.apache.commons.io.filefilter.IOFileFilter; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; +import org.fhcrc.cpas.flow.script.xml.ScriptDocument; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.SimpleRedirectAction; import org.labkey.api.action.SimpleViewAction; @@ -36,11 +39,16 @@ import org.labkey.api.pipeline.PipelineUrls; import org.labkey.api.pipeline.browse.PipelinePathForm; 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.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.ReaderRole; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; +import org.labkey.api.test.TestWhen; import org.labkey.api.usageMetrics.SimpleMetricsService; import org.labkey.api.util.FileUtil; import org.labkey.api.util.PageFlowUtil; @@ -54,6 +62,7 @@ import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.NotFoundException; import org.labkey.api.view.RedirectException; import org.labkey.api.view.ViewBackgroundInfo; import org.labkey.api.view.template.PageConfig; @@ -86,11 +95,14 @@ import org.labkey.flow.util.SampleUtil; import org.labkey.vfs.FileLike; import org.labkey.vfs.FileSystemLike; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.servlet.ModelAndView; +import jakarta.servlet.http.HttpServletResponse; + import java.io.File; import java.io.IOException; import java.net.URI; @@ -185,16 +197,20 @@ protected ModelAndView analyzeRuns(ChooseRunsToAnalyzeForm form, int[] runIds, S DataRegionSelection.clearAll(getViewContext()); FlowExperiment experiment = FlowExperiment.fromLSID(experimentLSID); + checkContainer(experiment); String experimentName = form.ff_analysisName; if (experiment != null) { experimentName = experiment.getName(); } FlowScript analysis = form.getProtocol(); + checkContainer(analysis); AnalyzeJob job = new AnalyzeJob(getViewBackgroundInfo(), experimentName, experimentLSID, FlowProtocol.ensureForContainer(getUser(), getContainer()), analysis, form.getProtocolStep(), runIds, PipelineService.get().findPipelineRoot(getContainer())); if (form.getCompensationMatrixId() != 0) { - job.setCompensationMatrix(FlowCompensationMatrix.fromCompId(form.getCompensationMatrixId())); + FlowCompensationMatrix comp = FlowCompensationMatrix.fromCompId(form.getCompensationMatrixId()); + checkContainer(comp); + job.setCompensationMatrix(comp); } job.setCompensationExperimentLSID(form.getCompensationExperimentLSID()); return HttpView.redirect(executeScript(job)); @@ -216,6 +232,7 @@ public class ChooseRunsToAnalyzeAction extends BaseAnalyzeRunsAction public ModelAndView getView(ChooseRunsToAnalyzeForm form, BindException errors) { script = form.getProtocol(); + checkContainer(script); return chooseRunsToAnalyze(form, errors); } } @@ -227,12 +244,19 @@ public class AnalyzeSelectedRunsAction extends BaseAnalyzeRunsAction public ModelAndView getView(ChooseRunsToAnalyzeForm form, BindException errors) throws Exception { script = form.getProtocol(); + checkContainer(script); int[] runIds = form.getSelectedRunIds(); if (runIds.length == 0) { errors.reject(ERROR_MSG, "Please select at least one run to analyze."); return chooseRunsToAnalyze(form, errors); } + for (int runId : runIds) + { + FlowRun run = FlowRun.fromRunId(runId); + if (run == null || !run.getContainer().hasPermission(getUser(), ReadPermission.class)) + throw new NotFoundException("Run not found: " + runId); + } String experimentLSID = form.getAnalysisLSID(); if (experimentLSID == null) { @@ -843,6 +867,12 @@ private SampleIdMap getSelectedFCSFiles(ImportAnalysisForm form, Er return null; } + if (!file.getContainer().equals(getContainer())) + { + errors.reject(ERROR_MSG, "Resolved FCS file '" + file.getName() + "' is not in this folder."); + return null; + } + if (!file.isOriginalFCSFile()) { errors.reject(ERROR_MSG, "Resolved FCS file '" + file.getName() + "' is a FCS files created from importing an external analysis."); @@ -1495,4 +1525,51 @@ public void addNavTrail(NavTree root) root.addChild(display); } } + + @TestWhen(TestWhen.When.BVT) + public static class AnalysisScriptContainerScopingTestCase extends AbstractContainerScopingTest + { + private User _admin; + private Container _folderA; + private Container _folderB; + private int _scriptIdB; + + @Before + public void setUp() throws Exception + { + _admin = getAdmin(); + _folderA = createContainer("A"); + _folderB = createContainer("B"); + + // Pre-create the flow protocol/steps in B so the valid-case ChooseRunsToAnalyzeForm.populate() finds them. + FlowProtocol.ensureForContainer(_admin, _folderB); + + // Create a flow analysis script with a single analysis step so populate() has a usable protocol step. + ScriptDocument doc = ScriptDocument.Factory.newInstance(); + doc.addNewScript().addNewAnalysis(); + _scriptIdB = FlowScript.create(_admin, _folderB, getClass().getSimpleName() + "-" + "scriptB", doc.toString()).getScriptId(); + } + + // ChooseRunsToAnalyzeForm resolves the analysis script by global rowId; a foreign script must be scoped. + @Test + public void testAnalysisScriptContainerScoping() throws Exception + { + ActionURL foreignUrl = new ActionURL(ChooseRunsToAnalyzeAction.class, _folderA).addParameter("scriptId", _scriptIdB); + + User folderAEditor = createUserInRole(_folderA, EditorRole.class); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, folderAEditor)); + + User folderBEditor = createUserInRole(_folderA, EditorRole.class); + grantRole(folderBEditor, _folderB, ReaderRole.class); + MockHttpServletResponse resp = get(foreignUrl, folderBEditor); + assertStatus(HttpServletResponse.SC_FOUND, resp); + String location = resp.getRedirectedUrl(); + assertNotNull("Redirect must have a Location", location); + assertTrue("Redirect should target the script's own container, was: " + location, location.contains(_folderB.getPath())); + + // valid case: the script in its own container is accepted (renders the choose-runs wizard). + assertStatus(HttpServletResponse.SC_OK, + get(new ActionURL(ChooseRunsToAnalyzeAction.class, _folderB).addParameter("scriptId", _scriptIdB), _admin)); + } + } } diff --git a/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java b/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java index b1b7d75dc0..311d04c42c 100644 --- a/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java +++ b/flow/src/org/labkey/flow/controllers/protocol/ProtocolForm.java @@ -35,7 +35,7 @@ public FlowProtocol getProtocol() throws UnauthorizedException { if (_protocol != null) return _protocol; - _protocol = FlowProtocol.fromURLRedirectIfNull(getUser(), getViewContext().getActionURL(), getRequest()); + _protocol = FlowProtocol.fromURLRedirectIfNull(getUser(), getViewContext(), getRequest()); return _protocol; } diff --git a/flow/src/org/labkey/flow/controllers/run/RunController.java b/flow/src/org/labkey/flow/controllers/run/RunController.java index 01dbc2a010..7a90149d16 100644 --- a/flow/src/org/labkey/flow/controllers/run/RunController.java +++ b/flow/src/org/labkey/flow/controllers/run/RunController.java @@ -232,6 +232,8 @@ public void validate(DownloadRunForm form, BindException errors) errors.reject(ERROR_MSG, "run not found"); return; } + // GitHub Issue #1892: validate container + _run.checkContainer(getContainer(), getUser(), getActionURL()); FlowWell[] wells = _run.getWells(true); if (wells.length == 0) @@ -464,6 +466,8 @@ else if (form.getSendTo() == ExportAnalysisForm.SendTo.Script) if (run == null) throw new NotFoundException("Flow run not found"); + // GitHub Issue #1892: validate container + run.checkContainer(getContainer(), getUser(), getActionURL()); runs.add(run); } _runs = runs; @@ -477,6 +481,8 @@ else if (wellId != null && wellId.length > 0) if (well == null) throw new NotFoundException("Flow well not found"); + // GitHub Issue #1892: validate container + well.checkContainer(getContainer(), getUser(), getActionURL()); wells.add(well); } _wells = wells; @@ -941,6 +947,8 @@ public static class DownloadAttachmentAction extends BaseDownloadAction(new ExpRunAttachmentParent(run.getExperimentRun()), form.getName()); } diff --git a/flow/src/org/labkey/flow/controllers/well/WellController.java b/flow/src/org/labkey/flow/controllers/well/WellController.java index a76cb82093..366365f472 100644 --- a/flow/src/org/labkey/flow/controllers/well/WellController.java +++ b/flow/src/org/labkey/flow/controllers/well/WellController.java @@ -20,16 +20,22 @@ import org.apache.commons.lang3.time.DateUtils; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.action.FormViewAction; import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.ReturnUrlForm; import org.labkey.api.action.SimpleErrorView; import org.labkey.api.action.SimpleViewAction; +import org.labkey.api.data.Container; import org.labkey.api.data.DataRegionSelection; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.SqlSelector; +import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.jsp.FormPage; import org.labkey.api.jsp.JspBase; import org.labkey.api.pipeline.PipeRoot; @@ -38,11 +44,16 @@ import org.labkey.api.security.ContextualRoles; import org.labkey.api.security.RequiresNoPermission; 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.permissions.UpdatePermission; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.test.TestWhen; import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.HeartBeat; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.URIUtil; @@ -52,6 +63,7 @@ import org.labkey.api.view.JspView; import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; +import org.labkey.api.view.ViewBackgroundInfo; import org.labkey.api.view.ViewContext; import org.labkey.flow.analysis.model.FCSHeader; import org.labkey.flow.analysis.web.FCSAnalyzer; @@ -63,16 +75,23 @@ import org.labkey.flow.controllers.FlowParam; import org.labkey.flow.data.FlowCompensationMatrix; import org.labkey.flow.data.FlowDataObject; +import org.labkey.flow.data.FlowDataType; +import org.labkey.flow.data.FlowFCSFile; +import org.labkey.flow.data.FlowProtocol; import org.labkey.flow.data.FlowProtocolStep; import org.labkey.flow.data.FlowRun; import org.labkey.flow.data.FlowWell; import org.labkey.flow.persist.AttributeCache; +import org.labkey.flow.persist.AttributeSet; +import org.labkey.flow.persist.AttributeSetHelper; import org.labkey.flow.persist.FlowManager; import org.labkey.flow.persist.ObjectType; import org.labkey.flow.query.FlowTableType; import org.labkey.flow.script.FlowAnalyzer; +import org.labkey.flow.script.KeywordsJob; import org.labkey.flow.util.KeywordUtil; import org.labkey.flow.view.GraphColumn; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; import org.springframework.validation.Errors; import org.springframework.web.servlet.ModelAndView; @@ -286,7 +305,9 @@ public ModelAndView getView(EditWellForm form, boolean reshow, BindException err for (String wellId : selected) { - _wells.add(FlowWell.fromWellId(Integer.parseInt(wellId))); + FlowWell well = FlowWell.fromWellId(Integer.parseInt(wellId)); + well.checkContainer(getContainer(), getUser(), getActionURL()); + _wells.add(well); } DataRegionSelection.clearAll(form.getViewContext()); } @@ -372,6 +393,19 @@ public void addNavTrail(NavTree root) } } + private @NotNull FlowWell checkContainer(ChooseGraphForm form) + { + FlowWell well = form.getWell(); + if (well == null) + throw new NotFoundException("Well not found"); + + checkContainer(well); + checkContainer(form.getScript()); + checkContainer(form.getCompensationMatrix()); + + return well; + } + @RequiresPermission(ReadPermission.class) public class ChooseGraphAction extends SimpleViewAction { @@ -380,11 +414,7 @@ public class ChooseGraphAction extends SimpleViewAction @Override public ModelAndView getView(ChooseGraphForm form, BindException errors) { - _well = form.getWell(); - if (null == _well) - { - throw new NotFoundException(); - } + _well = checkContainer(form); URI fileURI = _well.getFCSURI(); if (fileURI == null) @@ -497,10 +527,7 @@ public class GenerateGraphAction extends SimpleViewAction @Override public ModelAndView getView(ChooseGraphForm form, BindException errors) throws IOException { - FlowWell well = form.getWell(); - if (well == null) - throw new NotFoundException("Well not found"); - + checkContainer(form); String graph = getParam(FlowParam.graph); if (graph == null) throw new NotFoundException("Graph spec required"); @@ -886,4 +913,94 @@ public Object execute(Object o, BindException errors) return null; } } + + @TestWhen(TestWhen.When.BVT) + public static class WellContainerScopingTestCase extends AbstractContainerScopingTest + { + private User _admin; + private Container _folderA; + private Container _folderB; + private int _wellIdA; + private int _scriptIdB; + private int _compIdB; + + @Before + public void setUp() throws Exception + { + _admin = getAdmin(); + _folderA = createContainer("A"); + _folderB = createContainer("B"); + + _wellIdA = createFlowDataId(_folderA, FlowDataType.FCSFile, "wellA"); + _scriptIdB = createFlowDataId(_folderB, FlowDataType.Script, "scriptB"); + _compIdB = createFlowDataId(_folderB, FlowDataType.CompensationMatrix, "compB"); + } + + private int createFlowDataId(Container c, FlowDataType type, String name) throws Exception + { + URI uri = new URI("file:///" + getClass().getSimpleName() + "-" + name + ".flowdata.xml"); + ExpData data = ExperimentService.get().createData(c, type, getClass().getSimpleName() + "-" + name); + data.setDataFileURI(uri); + data.save(_admin); + // fromWellId/fromCompId resolve via FlowDataObject.fromData(), which returns null unless the data has an + // AttrObject (these FlowDataTypes set requireAttrObject). Attach a minimal one so the object resolves. + AttributeSetHelper.save(new AttributeSet(type.getObjectType(), uri), _admin, data); + return data.getRowId(); + } + + // A foreign object must be 404 for a caller with no rights to its container, and a 302 redirect to that + // container for a caller who can read it. + private void assertForeignRejected(ActionURL foreignUrl) throws Exception + { + User readerAonly = createUserInRole(_folderA, ReaderRole.class); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerAonly)); + + User readerAreaderB = createUserInRole(_folderA, ReaderRole.class); + grantRole(readerAreaderB, _folderB, ReaderRole.class); + MockHttpServletResponse resp = get(foreignUrl, readerAreaderB); + assertStatus(HttpServletResponse.SC_FOUND, resp); + String location = resp.getRedirectedUrl(); + assertNotNull("Redirect must have a Location", location); + assertTrue("Redirect should target the object's own container, was: " + location, location.contains(_folderB.getPath())); + } + + // ChooseGraphForm.getWell() resolves the well by global rowId. The valid case imports a legitimate run, so the + // well is backed by a readable FCS file, and the graph actually renders. + @Test + public void testWellContainerScoping() throws Exception + { + int foreignWellId = createFlowDataId(_folderB, FlowDataType.FCSFile, "wellB"); + assertForeignRejected(new ActionURL(GenerateGraphAction.class, _folderA).addParameter("wellId", foreignWellId)); + + // valid case: import a real run into B, then render a graph from its well in its own container + FlowProtocol protocol = FlowProtocol.ensureForContainer(_admin, _folderB); + PipeRoot root = PipelineService.get().findPipelineRoot(_folderB); + ViewBackgroundInfo info = new ViewBackgroundInfo(_folderB, _admin, null); + File dir = JunitUtil.getSampleData(null, "flow/flowjoquery/microFCS"); + FlowFCSFile realWell = new KeywordsJob(info, protocol, List.of(dir), null, root).go().get(0).getFCSFiles()[0]; + + // Build a graph spec from two of the well's real FCS parameter names. + List params = new ArrayList<>(FlowAnalyzer.getParameters(realWell, null).keySet()); + String graph = "(" + params.get(0) + ":" + params.get(1) + ")"; + ActionURL ownUrl = new ActionURL(GenerateGraphAction.class, _folderB) + .addParameter("wellId", realWell.getWellId()).addParameter("graph", graph); + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, _admin)); + } + + // ChooseGraphForm.getScript() resolves an analysis script by global rowId; a foreign script must be scoped. + @Test + public void testScriptContainerScoping() throws Exception + { + assertForeignRejected(new ActionURL(GenerateGraphAction.class, _folderA) + .addParameter("wellId", _wellIdA).addParameter("scriptId", _scriptIdB)); + } + + // ChooseGraphForm.getCompensationMatrix() resolves a comp matrix by global rowId; a foreign one must be scoped. + @Test + public void testCompensationMatrixContainerScoping() throws Exception + { + assertForeignRejected(new ActionURL(GenerateGraphAction.class, _folderA) + .addParameter("wellId", _wellIdA).addParameter("compId", _compIdB)); + } + } } diff --git a/flow/src/org/labkey/flow/data/FlowProtocol.java b/flow/src/org/labkey/flow/data/FlowProtocol.java index c58bf74a9e..c34c860c97 100644 --- a/flow/src/org/labkey/flow/data/FlowProtocol.java +++ b/flow/src/org/labkey/flow/data/FlowProtocol.java @@ -16,6 +16,7 @@ package org.labkey.flow.data; +import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -70,6 +71,7 @@ import org.labkey.api.view.RedirectException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewBackgroundInfo; +import org.labkey.api.view.ViewContext; import org.labkey.flow.controllers.FlowController; import org.labkey.flow.controllers.FlowParam; import org.labkey.flow.controllers.protocol.ProtocolController; @@ -79,7 +81,6 @@ import org.labkey.flow.query.FlowTableType; import org.labkey.flow.script.KeywordsJob; -import jakarta.servlet.http.HttpServletRequest; import java.io.File; import java.sql.ResultSet; import java.sql.SQLException; @@ -149,15 +150,19 @@ static public boolean isDefaultProtocol(ExpProtocol protocol) DEFAULT_PROTOCOL_NAME.equals(protocol.getName()); } - static public FlowProtocol fromURL(User user, ActionURL url, HttpServletRequest request) throws UnauthorizedException + static public FlowProtocol fromURL(User user, ViewContext context, HttpServletRequest request) throws UnauthorizedException { + ActionURL url = context.getActionURL(); FlowProtocol ret = fromProtocolId(getIntParam(url, request, FlowParam.experimentId)); if (ret == null) { - ret = FlowProtocol.getForContainer(ContainerManager.getForPath(url.getExtraPath())); + ret = FlowProtocol.getForContainer(context.getContainer()); } if (ret == null) return null; + + // GitHub Issue #1892: validate container + ret.checkContainer(context.getContainer(), user, url); if (!ret.getContainer().hasPermission(user, ReadPermission.class)) { throw new UnauthorizedException(); @@ -165,11 +170,11 @@ static public FlowProtocol fromURL(User user, ActionURL url, HttpServletRequest return ret; } - public static FlowProtocol fromURLRedirectIfNull(User user, ActionURL url, HttpServletRequest request) + public static FlowProtocol fromURLRedirectIfNull(User user, ViewContext context, HttpServletRequest request) { - FlowProtocol protocol = fromURL(user, url, request); + FlowProtocol protocol = fromURL(user, context, request); if (protocol == null) - throw new RedirectException(url.clone().setAction(FlowController.BeginAction.class)); + throw new RedirectException(context.getActionURL().clone().setAction(FlowController.BeginAction.class)); return protocol; } diff --git a/flow/src/org/labkey/flow/persist/FlowManager.java b/flow/src/org/labkey/flow/persist/FlowManager.java index 6e43a77375..d8451d818f 100644 --- a/flow/src/org/labkey/flow/persist/FlowManager.java +++ b/flow/src/org/labkey/flow/persist/FlowManager.java @@ -16,12 +16,14 @@ package org.labkey.flow.persist; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.iterators.ArrayIterator; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Before; import org.junit.Test; import org.labkey.api.audit.AuditLogService; import org.labkey.api.data.Aggregate; @@ -46,6 +48,7 @@ import org.labkey.api.exp.OntologyManager; import org.labkey.api.exp.PropertyDescriptor; import org.labkey.api.exp.api.ExpData; +import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExpSampleType; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.query.SamplesSchema; @@ -55,20 +58,31 @@ import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; +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.DateUtil; import org.labkey.api.util.FileUtil; import org.labkey.api.util.UnexpectedException; +import org.labkey.api.view.ActionURL; import org.labkey.flow.FlowModule; import org.labkey.flow.analysis.web.GraphSpec; import org.labkey.flow.analysis.web.StatisticSpec; +import org.labkey.flow.controllers.FlowParam; +import org.labkey.flow.controllers.editscript.ScriptController; import org.labkey.flow.controllers.executescript.AnalysisEngine; +import org.labkey.flow.controllers.protocol.ProtocolController; +import org.labkey.flow.controllers.run.RunController; import org.labkey.flow.data.AttributeType; import org.labkey.flow.data.FlowDataObject; +import org.labkey.flow.data.FlowDataType; import org.labkey.flow.data.FlowProperty; import org.labkey.flow.data.FlowProtocol; +import org.labkey.flow.data.FlowScript; import org.labkey.flow.data.ICSMetadata; import org.labkey.flow.query.FlowSchema; import org.labkey.flow.query.FlowTableType; +import org.springframework.mock.web.MockHttpServletResponse; import java.net.URI; import java.sql.ResultSet; @@ -1802,4 +1816,138 @@ public void metrics() assertNotNull(metrics.get("flowTempTableCount")); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folderA; + private Container _folderB; + private User _admin; + + @Before + public void setup() + { + // Regression test for FLOW-1. A FlowScript is addressed by a global scriptId. + _admin = getAdmin(); + _folderA = createContainer("A"); + _folderB = createContainer("B"); + } + + @Test + public void testScriptContainerScoping() throws Exception + { + // GitHub Issue #1892: Regression test for FLOW-1. + FlowScript script = FlowScript.create(_admin, _folderB, "scope-test", + "