From 8616ebfc239cf6ddf39b838ee4dd0aef4bdd108d Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 12 Jun 2026 16:10:04 -0700 Subject: [PATCH 1/2] Microarray container scope --- .../labkey/microarray/MicroarrayManager.java | 121 +++++++++++------- .../labkey/microarray/MicroarrayModule.java | 13 +- .../FeatureAnnotationSetController.java | 120 ++++++++++++++++- 3 files changed, 200 insertions(+), 54 deletions(-) diff --git a/microarray/src/org/labkey/microarray/MicroarrayManager.java b/microarray/src/org/labkey/microarray/MicroarrayManager.java index 5f5531089f..15ab3c0234 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,27 @@ 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; + +import static org.labkey.api.data.DataRegion.MessagePart.filter; public class MicroarrayManager { @@ -87,7 +98,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 +116,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 +176,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 +188,80 @@ 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.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..168d3aa61e 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,94 @@ 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)); + } + + private static ActionURL deleteUrl(Container c, int rowId) + { + return new ActionURL(DeleteAction.class, c).addParameter("singleObjectRowId", rowId); + } + + 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 9a8b204a42db89758c818e6c5b141c1baa90d506 Mon Sep 17 00:00:00 2001 From: labkey-nicka Date: Fri, 12 Jun 2026 16:43:01 -0700 Subject: [PATCH 2/2] Review feedback --- .../labkey/microarray/MicroarrayManager.java | 6 ++--- .../FeatureAnnotationSetController.java | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/microarray/src/org/labkey/microarray/MicroarrayManager.java b/microarray/src/org/labkey/microarray/MicroarrayManager.java index 15ab3c0234..cd84bf16ec 100644 --- a/microarray/src/org/labkey/microarray/MicroarrayManager.java +++ b/microarray/src/org/labkey/microarray/MicroarrayManager.java @@ -74,8 +74,6 @@ import java.util.Map; import java.util.Set; -import static org.labkey.api.data.DataRegion.MessagePart.filter; - public class MicroarrayManager { private static final MicroarrayManager _instance = new MicroarrayManager(); @@ -247,8 +245,10 @@ private void applyContainerFilter(Container c, User user, SimpleFilter filter) 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 + ")."); + throw new UnauthorizedException("You do not have sufficient permission in " + container.getPath() + " for feature annotation set (" + rowId + ")"); processed.add(containerId); } diff --git a/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java b/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java index 168d3aa61e..812f05ed45 100644 --- a/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java +++ b/microarray/src/org/labkey/microarray/controllers/FeatureAnnotationSetController.java @@ -554,11 +554,34 @@ public void testDeleteActionRejectsInScopeSetWithoutDeletePermission() throws Ex 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();