From 629fdd41c42e89bf8c8b7ea8da0be79a3c6e8bb6 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Sat, 13 Jun 2026 06:23:36 -0600 Subject: [PATCH 1/2] Scope ONPRC billing run deletion to the current container deleteBillingRuns built SimpleFilters over the raw onprc_billing tables (invoicedItems, invoiceRuns, miscCharges) from request-supplied primary keys with no container predicate, so a billing admin in one folder could delete or detach billing rows belonging to any other folder by POSTing arbitrary object IDs. Thread the caller's Container into the method and add a container filter to every read, delete, and update so the operation is confined to the current folder. Add an integration test (ONPRC_BillingManager.TestCase) asserting that a delete issued from one container cannot affect another, and register it in ONPRC_BillingModule.getIntegrationTests(). Also drop two unused row-count locals from the Table.delete calls. --- .../ONPRC_BillingController.java | 6 +- .../onprc_billing/ONPRC_BillingManager.java | 153 +++++++++++++++++- .../onprc_billing/ONPRC_BillingModule.java | 7 + 3 files changed, 155 insertions(+), 11 deletions(-) diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java index bee72c3f8..4c00cda8c 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java @@ -162,7 +162,7 @@ public ModelAndView getConfirmView(QueryForm form, BindException errors) throws Set ids = DataRegionSelection.getSelected(form.getViewContext(), true); StringBuilder msg = new StringBuilder("You have selected " + ids.size() + " billing runs to delete. This will also delete:

"); - for (String m : ONPRC_BillingManager.get().deleteBillingRuns(getUser(), ids, true)) + for (String m : ONPRC_BillingManager.get().deleteBillingRuns(getUser(), getContainer(), ids, true)) { msg.append(m).append("
"); } @@ -176,7 +176,7 @@ public ModelAndView getConfirmView(QueryForm form, BindException errors) throws public boolean handlePost(QueryForm form, BindException errors) throws Exception { Set ids = DataRegionSelection.getSelected(form.getViewContext(), true); - ONPRC_BillingManager.get().deleteBillingRuns(getUser(), ids, false); + ONPRC_BillingManager.get().deleteBillingRuns(getUser(), getContainer(), ids, false); return true; } @@ -266,4 +266,4 @@ public void setEnd(Date end) _end = end; } } -} \ No newline at end of file +} diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java index 733ca3faa..5b7bf61f3 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java @@ -15,6 +15,11 @@ */ package org.labkey.onprc_billing; +import org.apache.logging.log4j.Level; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; @@ -31,12 +36,18 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.Queryable; import org.labkey.api.security.User; +import org.labkey.api.util.GUID; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Date; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * User: bimber @@ -71,16 +82,16 @@ public static ONPRC_BillingManager get() return _instance; } - public List deleteBillingRuns(User user, Collection pks, boolean testOnly) + public List deleteBillingRuns(User user, Container container, Collection pks, boolean testOnly) { TableInfo invoiceRuns = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_INVOICE_RUNS); TableInfo invoicedItems = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_INVOICED_ITEMS); TableInfo miscCharges = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_MISC_CHARGES); //create filters - SimpleFilter invoiceRunFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); - - SimpleFilter miscChargesFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); + SimpleFilter invoiceRunFilter = createContainerScopedInFilter(container, "invoiceId", pks); + SimpleFilter miscChargesFilter = createContainerScopedInFilter(container, "invoiceId", pks); + SimpleFilter invoiceRunsFilter = createContainerScopedInFilter(container, "objectid", pks); //perform the work List ret = new ArrayList<>(); @@ -96,7 +107,7 @@ public List deleteBillingRuns(User user, Collection pks, boolean { try (DbScope.Transaction transaction = ExperimentService.get().ensureTransaction()) { - long deleted1 = Table.delete(invoicedItems, invoiceRunFilter); + Table.delete(invoicedItems, invoiceRunFilter); TableSelector tsMiscCharges2 = new TableSelector(miscCharges, Collections.singleton("objectid"), miscChargesFilter, null); String[] miscChargesIds = tsMiscCharges2.getArray(String.class); @@ -104,10 +115,10 @@ public List deleteBillingRuns(User user, Collection pks, boolean { Map map = new CaseInsensitiveHashMap<>(); map.put("invoiceId", null); - Table.update(user, miscCharges, map, objectid); + Table.update(user, miscCharges, map, objectid, SimpleFilter.createContainerFilter(container), Level.WARN); } - long deleted3 = Table.delete(invoiceRuns, new SimpleFilter(FieldKey.fromString("objectid"), pks, CompareType.IN)); + Table.delete(invoiceRuns, invoiceRunsFilter); transaction.commit(); } @@ -116,6 +127,11 @@ public List deleteBillingRuns(User user, Collection pks, boolean return ret; } + private SimpleFilter createContainerScopedInFilter(Container container, String columnName, Collection values) + { + return SimpleFilter.createContainerFilter(container).addInClause(FieldKey.fromString(columnName), values); + } + public Container getBillingContainer(Container c) { Module billing = ModuleLoader.getInstance().getModule(ONPRC_BillingModule.NAME); @@ -139,4 +155,125 @@ public Container getSLADataFolder(Container c) return ContainerManager.getForPath(path); } -} \ No newline at end of file + + public static class TestCase extends Assert + { + private static final String FOLDER_A = "ONPRCBillingDeleteTestA"; + private static final String FOLDER_B = "ONPRCBillingDeleteTestB"; + + private User _user; + private Container _containerA; + private Container _containerB; + private String _runIdA; + private String _runIdB; + + @Before + public void setUp() + { + _user = TestContext.get().getUser(); + deleteTestFolders(); + + Container junit = JunitUtil.getTestContainer(); + _containerA = createBillingFolder(junit, FOLDER_A); + _containerB = createBillingFolder(junit, FOLDER_B); + + _runIdA = insertBillingRun(_containerA); + _runIdB = insertBillingRun(_containerB); + } + + @After + public void tearDown() + { + deleteTestFolders(); + } + + @Test + public void testDeleteBillingRunsIsContainerScoped() + { + ONPRC_BillingManager manager = ONPRC_BillingManager.get(); + ONPRC_BillingSchema schema = ONPRC_BillingSchema.getInstance(); + + // A testOnly preview issued from container A targeting container B's run must not see container B's rows. + for (String summary : manager.deleteBillingRuns(_user, _containerA, List.of(_runIdB), true)) + assertTrue("Preview from another container should count 0 rows, but got: " + summary, summary.startsWith("0 ")); + + // An actual delete issued from container A targeting container B's run must leave container B untouched. + manager.deleteBillingRuns(_user, _containerA, List.of(_runIdB), false); + assertEquals("invoiceRuns row in container B should survive a delete issued from container A", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), _containerB)); + assertEquals("invoicedItems row in container B should survive a delete issued from container A", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), _containerB)); + assertEquals("miscCharges row in container B should still reference its invoice", 1, miscChargesWithInvoiceCount(_containerB)); + + // Positive control: deleting a run from its own container removes its rows. + manager.deleteBillingRuns(_user, _containerA, List.of(_runIdA), false); + assertEquals("invoiceRuns row in container A should be deleted", 0, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), _containerA)); + assertEquals("invoicedItems row in container A should be deleted", 0, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), _containerA)); + assertEquals("miscCharges row in container A should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerA)); + assertEquals("miscCharges row in container A should not be deleted", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), _containerA)); + } + + private Container createBillingFolder(Container parent, String name) + { + Container c = ContainerManager.createContainer(parent, name, _user); + Set active = new HashSet<>(c.getActiveModules()); + active.add(ModuleLoader.getInstance().getModule(ONPRC_BillingModule.NAME)); + c.setActiveModules(active, _user); + return c; + } + + private String insertBillingRun(Container c) + { + ONPRC_BillingSchema schema = ONPRC_BillingSchema.getInstance(); + String runId = GUID.makeGUID(); + + Map run = new CaseInsensitiveHashMap<>(); + run.put("objectid", runId); + run.put("runDate", new Date()); + run.put("billingPeriodStart", new Date()); + run.put("billingPeriodEnd", new Date()); + run.put("container", c.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), run); + + Map invoicedItem = new CaseInsensitiveHashMap<>(); + invoicedItem.put("objectid", GUID.makeGUID()); + invoicedItem.put("invoiceId", runId); + invoicedItem.put("container", c.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), invoicedItem); + + Map miscCharge = new CaseInsensitiveHashMap<>(); + miscCharge.put("objectid", GUID.makeGUID()); + miscCharge.put("invoiceId", runId); + miscCharge.put("container", c.getId()); + Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), miscCharge); + + return runId; + } + + private TableInfo getTable(ONPRC_BillingSchema schema, String tableName) + { + return schema.getSchema().getTable(tableName); + } + + private long containerRowCount(TableInfo table, Container c) + { + return new TableSelector(table, SimpleFilter.createContainerFilter(c), null).getRowCount(); + } + + private long miscChargesWithInvoiceCount(Container c) + { + SimpleFilter filter = SimpleFilter.createContainerFilter(c); + filter.addCondition(FieldKey.fromParts("invoiceId"), null, CompareType.NONBLANK); + return new TableSelector(getTable(ONPRC_BillingSchema.getInstance(), ONPRC_BillingSchema.TABLE_MISC_CHARGES), filter, null).getRowCount(); + } + + private void deleteTestFolders() + { + Container junit = JunitUtil.getTestContainer(); + for (String name : List.of(FOLDER_A, FOLDER_B)) + { + Container c = junit.getChild(name); + if (c != null) + ContainerManager.delete(c, _user); + } + } + } +} diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java index d46499053..6cb63a80b 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java @@ -172,6 +172,13 @@ public Set getSchemaNames() return Collections.singleton(ONPRC_BillingSchema.NAME); } + @Override + @NotNull + public Set getIntegrationTests() + { + return Collections.singleton(ONPRC_BillingManager.TestCase.class); + } + @Override protected void registerSchemas() { From 4ffa316b2117b501b2769280aafdd18b53d05d07 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Tue, 16 Jun 2026 06:35:10 -0600 Subject: [PATCH 2/2] Scope misc charges delete by authorized run id only Mirror the ehr_billing fix (ehrModules#1146): narrow the requested run ids to those that actually exist in the requesting container, then match misc charges by run id alone rather than container. Source miscCharges records can live in containers other than the billing/finance container (the EHR study container or satellite source containers), so a container-scoped filter silently failed to detach them. The run ids are already authorized against the requesting container, so matching by invoiceId only is safe and complete. Adds a satellite-source-container case to the delete test. --- .../onprc_billing/ONPRC_BillingManager.java | 70 ++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java index 5b7bf61f3..d2e0c97cd 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingManager.java @@ -15,7 +15,6 @@ */ package org.labkey.onprc_billing; -import org.apache.logging.log4j.Level; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -41,6 +40,7 @@ import org.labkey.api.util.TestContext; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Date; @@ -89,9 +89,21 @@ public List deleteBillingRuns(User user, Container container, Collection TableInfo miscCharges = ONPRC_BillingSchema.getInstance().getSchema().getTable(ONPRC_BillingSchema.TABLE_MISC_CHARGES); //create filters - SimpleFilter invoiceRunFilter = createContainerScopedInFilter(container, "invoiceId", pks); - SimpleFilter miscChargesFilter = createContainerScopedInFilter(container, "invoiceId", pks); SimpleFilter invoiceRunsFilter = createContainerScopedInFilter(container, "objectid", pks); + Set invoiceRunIds = getInvoiceRunIds(invoiceRuns, invoiceRunsFilter); + if (invoiceRunIds.isEmpty()) + { + List ret = new ArrayList<>(); + if (testOnly) + { + ret.add("0 records from invoiced items"); + ret.add("0 records from misc charges will be removed from the deleted invoice, which means they will be picked up by the next billing period. They are not deleted."); + } + return ret; + } + + SimpleFilter invoiceRunFilter = createContainerScopedInFilter(container, "invoiceId", invoiceRunIds); + SimpleFilter miscChargesFilter = createMiscChargesFilter(invoiceRunIds); //perform the work List ret = new ArrayList<>(); @@ -115,7 +127,7 @@ public List deleteBillingRuns(User user, Container container, Collection { Map map = new CaseInsensitiveHashMap<>(); map.put("invoiceId", null); - Table.update(user, miscCharges, map, objectid, SimpleFilter.createContainerFilter(container), Level.WARN); + Table.update(user, miscCharges, map, objectid); } Table.delete(invoiceRuns, invoiceRunsFilter); @@ -132,6 +144,28 @@ private SimpleFilter createContainerScopedInFilter(Container container, String c return SimpleFilter.createContainerFilter(container).addInClause(FieldKey.fromString(columnName), values); } + private Set getInvoiceRunIds(TableInfo invoiceRuns, SimpleFilter objectIdFilter) + { + TableSelector tsInvoiceRuns = new TableSelector(invoiceRuns, Collections.singleton("objectid"), objectIdFilter, null); + String[] invoiceRunIds = tsInvoiceRuns.getArray(String.class); + return new HashSet<>(Arrays.asList(invoiceRunIds)); + } + + private SimpleFilter createMiscChargesFilter(Collection invoiceRunIds) + { + // Intentionally NOT container-scoped. Source miscCharges records can live in different containers + // (the billing/finance container, the configured EHR study container, or other satellite containers that + // feed charges into a billing run), so there is no reliable, complete set of "source" containers to filter on. + // + // This is not a cross-container security issue: invoiceRunIds has already been narrowed by getInvoiceRunIds() + // to the run ids that actually exist in the requesting container (see the container-scoped objectIdFilter). + // A forged or out-of-container run id never reaches this filter, so matching miscCharges solely by + // invoiceId only ever touches charges belonging to runs the caller is already authorized to delete. + SimpleFilter filter = new SimpleFilter(); + filter.addInClause(FieldKey.fromString("invoiceId"), invoiceRunIds); + return filter; + } + public Container getBillingContainer(Container c) { Module billing = ModuleLoader.getInstance().getModule(ONPRC_BillingModule.NAME); @@ -160,10 +194,12 @@ public static class TestCase extends Assert { private static final String FOLDER_A = "ONPRCBillingDeleteTestA"; private static final String FOLDER_B = "ONPRCBillingDeleteTestB"; + private static final String FOLDER_SATELLITE = "ONPRCBillingDeleteTestSatellite"; private User _user; private Container _containerA; private Container _containerB; + private Container _containerSatellite; private String _runIdA; private String _runIdB; @@ -176,6 +212,7 @@ public void setUp() Container junit = JunitUtil.getTestContainer(); _containerA = createBillingFolder(junit, FOLDER_A); _containerB = createBillingFolder(junit, FOLDER_B); + _containerSatellite = createBillingFolder(junit, FOLDER_SATELLITE); _runIdA = insertBillingRun(_containerA); _runIdB = insertBillingRun(_containerB); @@ -203,6 +240,18 @@ public void testDeleteBillingRunsIsContainerScoped() assertEquals("invoicedItems row in container B should survive a delete issued from container A", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), _containerB)); assertEquals("miscCharges row in container B should still reference its invoice", 1, miscChargesWithInvoiceCount(_containerB)); + // Satellite source data: miscCharges can live in a container that is neither the requesting/finance + // container nor the run's own container. The delete is keyed off the authorized run id, so these rows are + // still previewed and detached. + String runIdWithSatelliteMiscCharge = insertBillingRun(_containerA, _containerSatellite); + List satellitePreview = manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithSatelliteMiscCharge), true); + assertTrue("Preview should count miscCharges rows in an unrelated source container: " + satellitePreview, + satellitePreview.stream().anyMatch(summary -> summary.startsWith("1 records from misc charges"))); + + manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithSatelliteMiscCharge), false); + assertEquals("miscCharges row in the satellite source container should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerSatellite)); + assertEquals("miscCharges row in the satellite source container should not be deleted", 1, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), _containerSatellite)); + // Positive control: deleting a run from its own container removes its rows. manager.deleteBillingRuns(_user, _containerA, List.of(_runIdA), false); assertEquals("invoiceRuns row in container A should be deleted", 0, containerRowCount(getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), _containerA)); @@ -221,6 +270,11 @@ private Container createBillingFolder(Container parent, String name) } private String insertBillingRun(Container c) + { + return insertBillingRun(c, c); + } + + private String insertBillingRun(Container billingContainer, Container miscChargesContainer) { ONPRC_BillingSchema schema = ONPRC_BillingSchema.getInstance(); String runId = GUID.makeGUID(); @@ -230,19 +284,19 @@ private String insertBillingRun(Container c) run.put("runDate", new Date()); run.put("billingPeriodStart", new Date()); run.put("billingPeriodEnd", new Date()); - run.put("container", c.getId()); + run.put("container", billingContainer.getId()); Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICE_RUNS), run); Map invoicedItem = new CaseInsensitiveHashMap<>(); invoicedItem.put("objectid", GUID.makeGUID()); invoicedItem.put("invoiceId", runId); - invoicedItem.put("container", c.getId()); + invoicedItem.put("container", billingContainer.getId()); Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_INVOICED_ITEMS), invoicedItem); Map miscCharge = new CaseInsensitiveHashMap<>(); miscCharge.put("objectid", GUID.makeGUID()); miscCharge.put("invoiceId", runId); - miscCharge.put("container", c.getId()); + miscCharge.put("container", miscChargesContainer.getId()); Table.insert(_user, getTable(schema, ONPRC_BillingSchema.TABLE_MISC_CHARGES), miscCharge); return runId; @@ -268,7 +322,7 @@ private long miscChargesWithInvoiceCount(Container c) private void deleteTestFolders() { Container junit = JunitUtil.getTestContainer(); - for (String name : List.of(FOLDER_A, FOLDER_B)) + for (String name : List.of(FOLDER_A, FOLDER_B, FOLDER_SATELLITE)) { Container c = junit.getChild(name); if (c != null)