From 30a00f256fc655b3e0d09010ef49a00e1399b47c Mon Sep 17 00:00:00 2001 From: Lakshmi Kolli <69940873+kollil@users.noreply.github.com> Date: Tue, 7 Jul 2026 10:48:41 -0700 Subject: [PATCH 1/2] ParentageDamMismatch query update (#1790) Added more filters: 1. Remove cases in which the ID has a surrogate dam entered in prime. (There are several but 41043 is one example) 2. Remove cases in which the observed dam is "unknown" 3. Remove cases in which the observed dam ID is not an ONPRC. E.g. there are several NEPRC IDs of the format ###-####. --- .../queries/study/ParentageDamMismatch.sql | 85 +++++++++++-------- 1 file changed, 50 insertions(+), 35 deletions(-) diff --git a/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql b/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql index f9e07f467..b43f54b41 100644 --- a/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql +++ b/onprc_ehr/resources/queries/study/ParentageDamMismatch.sql @@ -13,49 +13,64 @@ * 6. Excludes old parentage entries, enddate IS BLANK */ +/* + Modifications by Kolli, June 2026 + Refer tkt# 14891 for details + + * 1. Remove cases in which the ID has a surrogate dam entered in prime. (There are several but 41043 is one example) + * 2. Remove cases in which the observed dam is "unknown" + * 3. Remove cases in which the observed dam ID is not an ONPRC. E.g. there are several NEPRC IDs of the format ###-####. + */ + SELECT d.Id, d.Id.curLocation.area AS Area, - coalesce(p2.parent, '') as geneticDam, - coalesce(b.dam, '') as observedDam + gd.geneticDam, + b.dam AS observedDam + FROM study.demographics d - LEFT JOIN ( - SELECT - p2.Id, - MAX(p2.parent) AS parent - FROM study.parentage p2 - WHERE (p2.method = 'Genetic' OR p2.method = 'Provisional Genetic') - AND p2.relationship = 'Dam' - AND p2.enddate IS NULL - GROUP BY p2.Id -) p2 ON d.Id = p2.Id - - LEFT JOIN ( +LEFT JOIN ( SELECT - p3.Id, - MAX(p3.parent) AS parent - FROM study.parentage p3 - WHERE p3.relationship = 'Foster Dam' - AND p3.enddate IS NULL - GROUP BY p3.Id -) p3 ON d.Id = p3.Id - - LEFT JOIN study.birth b - ON b.Id = d.Id - -WHERE d.calculated_status.code IN ('Alive', 'Dead') AND d.qcstate = 18 - /* exclude foster-dam cases (NULL or blank only) */ - AND COALESCE(RTRIM(LTRIM(CAST(p3.parent AS VARCHAR(50)))), '') = '' - - /* exclude blank observed dam */ + p.Id, + MAX(p.parent) AS geneticDam + FROM study.parentage p + WHERE p.relationship = 'Dam' + AND p.method IN ('Genetic', 'Provisional Genetic') + AND p.enddate IS NULL + GROUP BY p.Id +) gd + ON gd.Id = d.Id + +LEFT JOIN study.birth b + ON b.Id = d.Id + +WHERE d.calculated_status.code IN ('Alive', 'Dead') + AND d.qcstate = 18 + + /* exclude animals with active Foster Dam */ + /* And, exclude animals with active Surrogate Dam */ + AND NOT EXISTS ( + SELECT 1 + FROM study.parentage fd + WHERE fd.Id = d.Id + AND fd.relationship IN ('Foster Dam', 'Surrogate Dam') + AND fd.enddate IS NULL + AND COALESCE(RTRIM(LTRIM(CAST(fd.parent AS VARCHAR(50)))), '') <> '' + ) + + /* observed dam is not blank */ AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> '' - /* exclude blank genetic dam */ - AND COALESCE(RTRIM(LTRIM(CAST(p2.parent AS VARCHAR(50)))), '') <> '' + /* observed dam is not unknown */ + AND LOWER(COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '')) <> 'unknown' - /* mismatch observed vs genetic */ - AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> - COALESCE(RTRIM(LTRIM(CAST(p2.parent AS VARCHAR(50)))), '') + /* observed dam is an ONPRC ID, not external format like ###-#### */ + AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') NOT LIKE '%-%' + /* genetic dam is not blank */ + AND COALESCE(RTRIM(LTRIM(CAST(gd.geneticDam AS VARCHAR(50)))), '') <> '' + /* observed dam does not match genetic dam */ + AND COALESCE(RTRIM(LTRIM(CAST(b.dam AS VARCHAR(50)))), '') <> + COALESCE(RTRIM(LTRIM(CAST(gd.geneticDam AS VARCHAR(50)))), ''); From 210bd3d0c7fff9384ded3cfb29915122fd52b3b5 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Tue, 7 Jul 2026 11:27:34 -0700 Subject: [PATCH 2/2] Scope ONPRC billing run deletion to the current container (#1767) ## Rationale ONPRC_BillingManager.deleteBillingRuns built SimpleFilters over the raw onprc_billing tables (invoicedItems, invoiceRuns, miscCharges) from request-supplied primary keys with no container predicate, and the method received no Container. Since DeleteBillingPeriodAction's permission check is evaluated only against the current folder, a billing admin in one folder could delete or detach billing rows belonging to any other folder by POSTing arbitrary object IDs - a cross-container horizontal privilege-escalation bug. This change confines every operation to the caller's folder. This is the 26.3 port of LabKey/onprcEHRModules#1759 (originally against release25.7-SNAPSHOT). ## Related Pull Requests - LabKey/onprcEHRModules#1759 (original, against release25.7-SNAPSHOT) ## Changes - Add a Container parameter to deleteBillingRuns and pass getContainer() from both DeleteBillingPeriodAction call sites. - Add integration test ONPRC_BillingManager.TestCase asserting a delete from one container cannot affect another, registered in ONPRC_BillingModule.getIntegrationTests(). - Drop two unused row-count locals from the Table.delete calls. ## Container filters added on All tables are in the onprc_billing schema: - invoiceRuns - container filter on the PK->run-id lookup and on the final delete. - invoicedItems - the preview read and the delete both carry a direct container filter (SimpleFilter.createContainerFilter) plus an invoiceId restriction to the container-scoped run ids. - miscCharges - the read and the detaching Table.update are keyed off container-authorized run ids only; intentionally NOT container-scoped (source charges may live in satellite containers) but can only match runs the caller already owns. --- .../ONPRC_BillingController.java | 4 +- .../onprc_billing/ONPRC_BillingManager.java | 201 +++++++++++++++++- .../onprc_billing/ONPRC_BillingModule.java | 7 + 3 files changed, 205 insertions(+), 7 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 fd7529311..8f8be3daa 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingController.java @@ -164,7 +164,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("
"); } @@ -178,7 +178,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; } 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..b2a016d4b 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,10 @@ */ package org.labkey.onprc_billing; +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 +35,19 @@ 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.Arrays; 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,28 @@ 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 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 miscChargesFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); + SimpleFilter invoiceRunFilter = createContainerScopedInFilter(container, "invoiceId", invoiceRunIds); + SimpleFilter miscChargesFilter = createMiscChargesFilter(invoiceRunIds); //perform the work List ret = new ArrayList<>(); @@ -96,7 +119,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); @@ -107,7 +130,7 @@ public List deleteBillingRuns(User user, Collection pks, boolean Table.update(user, miscCharges, map, objectid); } - long deleted3 = Table.delete(invoiceRuns, new SimpleFilter(FieldKey.fromString("objectid"), pks, CompareType.IN)); + Table.delete(invoiceRuns, invoiceRunsFilter); transaction.commit(); } @@ -116,6 +139,33 @@ 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); + } + + 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); @@ -139,4 +189,145 @@ public Container getSLADataFolder(Container c) return ContainerManager.getForPath(path); } + + 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; + + @Before + public void setUp() + { + _user = TestContext.get().getUser(); + deleteTestFolders(); + + 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); + } + + @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)); + + // 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)); + 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) + { + return insertBillingRun(c, c); + } + + private String insertBillingRun(Container billingContainer, Container miscChargesContainer) + { + 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", 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", 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", miscChargesContainer.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, FOLDER_SATELLITE)) + { + Container c = junit.getChild(name); + if (c != null) + ContainerManager.delete(c, _user); + } + } + } } \ No newline at end of file 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..af2f65ae5 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() {