diff --git a/pepdb/src/org/scharp/atlas/pepdb/PepDBController.java b/pepdb/src/org/scharp/atlas/pepdb/PepDBController.java index ace2b6a4..aca6a883 100644 --- a/pepdb/src/org/scharp/atlas/pepdb/PepDBController.java +++ b/pepdb/src/org/scharp/atlas/pepdb/PepDBController.java @@ -427,7 +427,7 @@ public ModelAndView getView(PeptideAndGroupForm form, BindException errors) thro pg = PepDBManager.getPeptideGroupByID(peptideGroupId); } catch (NumberFormatException ignored) {} - if (pg == null || !getContainer().getId().equalsIgnoreCase(pg.getContainerId())) + if (pg == null) { throw new NotFoundException(); } diff --git a/pepdb/src/org/scharp/atlas/pepdb/PepDBManager.java b/pepdb/src/org/scharp/atlas/pepdb/PepDBManager.java index 09d144b6..758c94ca 100644 --- a/pepdb/src/org/scharp/atlas/pepdb/PepDBManager.java +++ b/pepdb/src/org/scharp/atlas/pepdb/PepDBManager.java @@ -295,10 +295,12 @@ public static Peptides[] getParentPeptides(Peptides p) throws SQLException public static Peptides[] getHyphanatedParents(Peptides p) throws SQLException { - String sql = "select * from "+schema.getTableInfoPeptides()+" where peptides.protein_cat_id = ? " + - " and peptides.peptide_sequence LIKE '%"+p.getPeptide_sequence()+"%' " + - " and child = false"; - Peptides[] peptides = new SqlSelector(schema.getSchema(), sql, new Object[]{p.getProtein_cat_id()}).getArray(Peptides.class); + // GitHub Kanban #1929: parameterize the LIKE clause for the stored peptide_sequence + SQLFragment sql = new SQLFragment("select * from "+schema.getTableInfoPeptides()+" where peptides.protein_cat_id = ? ", p.getProtein_cat_id()); + sql.append(" and peptides.peptide_sequence LIKE ? "); + sql.add("%" + schema.getSchema().getSqlDialect().encodeLikeOpSearchString(p.getPeptide_sequence()) + "%"); + sql.append(" and child = false"); + Peptides[] peptides = new SqlSelector(schema.getSchema(), sql).getArray(Peptides.class); return peptides; } diff --git a/pepdb/test/src/org/labkey/test/tests/pepdb/PepDBModuleTest.java b/pepdb/test/src/org/labkey/test/tests/pepdb/PepDBModuleTest.java index dfe49252..6b613a0e 100644 --- a/pepdb/test/src/org/labkey/test/tests/pepdb/PepDBModuleTest.java +++ b/pepdb/test/src/org/labkey/test/tests/pepdb/PepDBModuleTest.java @@ -31,6 +31,7 @@ import org.labkey.test.TestTimeoutException; import org.labkey.test.WebTestHelper; import org.labkey.test.categories.CustomModules; +import org.labkey.test.util.DataRegionTable; import org.labkey.test.util.LogMethod; import org.labkey.test.util.PostgresOnlyTest; @@ -180,6 +181,15 @@ public void testSteps() throws Exception clickAndWait(Locator.css("a.labkey-button > span")); + // Verify peptide group detail page + clickAndWait(Locator.linkWithText("List Peptide Groups")); + clickAndWait(Locator.linkWithText("gagptegprac")); + assertTextPresentInThisOrder( + "Group Information from peptide_group table for group : gagptegprac", + "There are (16) peptides in the 'gagptegprac' peptide group." + ); + DataRegionTable pepTable = new DataRegionTable("group_peptides", getDriver()); + assertEquals("Peptide table for group does not have expected number of rows", 16, pepTable.getDataRowCount()); /* * diff --git a/studydesign/src/org/labkey/studydesign/StudyDesignController.java b/studydesign/src/org/labkey/studydesign/StudyDesignController.java index 12b5224a..1ea77cc2 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignController.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignController.java @@ -19,6 +19,7 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Test; import org.labkey.api.action.ApiJsonForm; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -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; @@ -53,6 +56,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.studydesign.model.AssaySpecimenConfigImpl; import org.labkey.studydesign.model.AssaySpecimenVisitImpl; import org.labkey.studydesign.model.DoseAndRoute; @@ -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 44aa8c8b..b2196c4d 100644 --- a/studydesign/src/org/labkey/studydesign/StudyDesignModule.java +++ b/studydesign/src/org/labkey/studydesign/StudyDesignModule.java @@ -75,8 +75,9 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext) public @NotNull Set> getIntegrationTests() { return Set.of( - TreatmentManager.TreatmentDataTestCase.class, - TreatmentManager.AssayScheduleTestCase.class + TreatmentManager.TreatmentDataTestCase.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 34637a8c..ca886690 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; @@ -320,7 +321,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); @@ -364,20 +368,30 @@ public DoseAndRoute saveStudyProductDoseAndRoute(Container container, User user, { if (doseAndRoute.isNew()) return Table.insert(user, StudyDesignSchema.getInstance().getTableInfoDoseAndRoute(), doseAndRoute); - else - 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) { - 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 @@ -991,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)