From 66c688ec3d1e640eea65181c8b6f7962be49b06b Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 22 Jun 2026 13:40:51 -0600 Subject: [PATCH 1/2] Scope billing run deletion filters to the current container (#1146) #### Rationale EHR_BillingManager.deleteBillingRuns() filtered the invoice, invoicedItems, and miscCharges tables by objectid/invoiceId alone, with no container clause. Since DeleteBillingPeriodAction only checks EHR_BillingAdminPermission in the current container, a billing admin in one container could delete invoices and invoiced items, or detach misc charges, in any other container by submitting foreign objectids. All filters are now container-scoped via SimpleFilter.createContainerFilter(). #### Related Pull Requests * None #### Changes * EHR_BillingManager: all delete/preview filters in deleteBillingRuns() are now container-scoped via a new createContainerScopedInFilter() helper. * EHR_BillingManager.TestCase: new integration test that seeds a complete billing run in each of two folders and verifies cross-container ids are ignored by both the testOnly preview and the actual delete, while same-container deletion still removes the run and detaches its misc charges. * EHR_BillingModule: registers the test via getIntegrationTests(). --- .../ehr_billing/EHR_BillingManager.java | 224 +++++++++++++++++- .../labkey/ehr_billing/EHR_BillingModule.java | 7 + 2 files changed, 226 insertions(+), 5 deletions(-) diff --git a/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java b/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java index 88bdeaab8..a3d03fde6 100644 --- a/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java +++ b/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java @@ -16,6 +16,10 @@ package org.labkey.ehr_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; @@ -36,14 +40,20 @@ import org.labkey.api.query.QueryUpdateServiceException; import org.labkey.api.security.User; import org.labkey.api.data.RuntimeSQLException; +import org.labkey.api.util.GUID; +import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.TestContext; import java.sql.SQLException; 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; public class EHR_BillingManager { @@ -68,11 +78,24 @@ public List deleteBillingRuns(User user, Container container, Collection TableInfo miscCharges = EHR_BillingSchema.getInstance().getSchema().getTable(EHR_BillingSchema.TABLE_MISC_CHARGES); //create filters - SimpleFilter objectIdFilter = new SimpleFilter(FieldKey.fromString("objectid"), pks, CompareType.IN); - SimpleFilter invoiceIdFilter = new SimpleFilter(FieldKey.fromString("invoiceId"), pks, CompareType.IN); - SimpleFilter invoiceRunIdFilter = new SimpleFilter(FieldKey.fromString("invoiceRunId"), pks, CompareType.IN); + SimpleFilter objectIdFilter = createContainerScopedInFilter(container, "objectid", pks); + Set invoiceRunIds = getInvoiceRunIds(invoiceRuns, objectIdFilter); + if (invoiceRunIds.isEmpty()) + { + List ret = new ArrayList<>(); + if (testOnly) + { + ret.add("0 records from invoiced items"); + ret.add("0 records from invoice"); + ret.add("0 invoice 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 invoiceIdFilter = createContainerScopedInFilter(container, "invoiceId", invoiceRunIds); + SimpleFilter invoiceRunIdFilter = createContainerScopedInFilter(container, "invoiceRunId", invoiceRunIds); + + SimpleFilter miscChargesFilter = createMiscChargesFilter(invoiceRunIds); //perform the work List ret = new ArrayList<>(); @@ -114,6 +137,33 @@ public List deleteBillingRuns(User user, Container container, Collection 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 any number of 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; + } + private void deleteInvoiceRuns(TableInfo tableInfo, Map[] rows, User user, Container container) throws QueryUpdateServiceException, BatchValidationException, InvalidKeyException { if(rows.length>0) @@ -141,4 +191,168 @@ public Container getBillingContainer(Container c) } -} \ No newline at end of file + public static class TestCase extends Assert + { + private static final String FOLDER_A = "EHRBillingDeleteTestA"; + private static final String FOLDER_B = "EHRBillingDeleteTestB"; + private static final String FOLDER_EHR = "EHRBillingDeleteTestEHR"; + private static final String FOLDER_SATELLITE = "EHRBillingDeleteTestSatellite"; + + private User _user; + private Container _containerA; + private Container _containerB; + private Container _containerEHR; + 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); + _containerEHR = createBillingFolder(junit, FOLDER_EHR); + _containerSatellite = createBillingFolder(junit, FOLDER_SATELLITE); + setEHRContainer(_containerA, _containerEHR); + + _runIdA = insertBillingRun(_containerA); + _runIdB = insertBillingRun(_containerB); + } + + @After + public void tearDown() + { + deleteTestFolders(); + } + + @Test + public void testDeleteBillingRunsIsContainerScoped() throws Exception + { + EHR_BillingManager manager = EHR_BillingManager.get(); + EHR_BillingSchema schema = EHR_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(schema.getTableInvoiceRuns(), _containerB)); + assertEquals("invoice row in container B should survive a delete issued from container A", 1, containerRowCount(schema.getInvoice(), _containerB)); + assertEquals("invoicedItems row in container B should survive a delete issued from container A", 1, containerRowCount(schema.getTableInvoiceItems(), _containerB)); + assertEquals("miscCharges row in container B should still reference its invoice", 1, miscChargesWithInvoiceCount(_containerB)); + + // WNPRC-style source data: billing artifacts live in the finance container, but miscCharges live in the EHR container + String runIdWithEHRMiscCharge = insertBillingRun(_containerA, _containerEHR); + List preview = manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithEHRMiscCharge), true); + assertTrue("Preview should count miscCharges rows in the EHR source container: " + preview, + preview.stream().anyMatch(summary -> summary.startsWith("1 invoice records from misc charges"))); + + manager.deleteBillingRuns(_user, _containerA, List.of(runIdWithEHRMiscCharge), false); + assertEquals("miscCharges row in the EHR source container should be detached from the deleted invoice", 0, miscChargesWithInvoiceCount(_containerEHR)); + assertEquals("miscCharges row in the EHR source container should not be deleted", 1, containerRowCount(schema.getMiscCharges(), _containerEHR)); + + // Satellite source data: miscCharges can live in a container that is neither the finance container nor + // the configured EHR study 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 invoice 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(schema.getMiscCharges(), _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(schema.getTableInvoiceRuns(), _containerA)); + assertEquals("invoice row in container A should be deleted", 0, containerRowCount(schema.getInvoice(), _containerA)); + assertEquals("invoicedItems row in container A should be deleted", 0, containerRowCount(schema.getTableInvoiceItems(), _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(schema.getMiscCharges(), _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(EHR_BillingModule.NAME)); + c.setActiveModules(active, _user); + return c; + } + + private String insertBillingRun(Container c) + { + return insertBillingRun(c, c); + } + + private String insertBillingRun(Container billingContainer, Container miscChargesContainer) + { + EHR_BillingSchema schema = EHR_BillingSchema.getInstance(); + String runId = GUID.makeGUID(); + String invoiceNumber = billingContainer.getName() + "-" + runId; + + Map run = new CaseInsensitiveHashMap<>(); + run.put("objectid", runId); + run.put("runDate", new Date()); + run.put("container", billingContainer.getId()); + Table.insert(_user, schema.getTableInvoiceRuns(), run); + + Map invoice = new CaseInsensitiveHashMap<>(); + invoice.put("invoiceNumber", invoiceNumber); + invoice.put("invoiceRunId", runId); + invoice.put("container", billingContainer.getId()); + Table.insert(_user, schema.getInvoice(), invoice); + + Map invoicedItem = new CaseInsensitiveHashMap<>(); + invoicedItem.put("objectId", GUID.makeGUID()); + invoicedItem.put("invoiceId", runId); + invoicedItem.put("invoiceNumber", invoiceNumber); + invoicedItem.put("container", billingContainer.getId()); + Table.insert(_user, schema.getTableInvoiceItems(), invoicedItem); + + Map miscCharge = new CaseInsensitiveHashMap<>(); + miscCharge.put("objectid", GUID.makeGUID()); + miscCharge.put("invoiceId", runId); + miscCharge.put("container", miscChargesContainer.getId()); + Table.insert(_user, schema.getMiscCharges(), miscCharge); + + return runId; + } + + private void setEHRContainer(Container source, Container ehrContainer) + { + Module ehr = ModuleLoader.getInstance().getModule("EHR"); + ModuleProperty mp = ehr.getModuleProperties().get("EHRStudyContainer"); + mp.saveValue(_user, source, ehrContainer.getPath()); + } + + 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(EHR_BillingSchema.getInstance().getMiscCharges(), filter, null).getRowCount(); + } + + private void deleteTestFolders() + { + Container junit = JunitUtil.getTestContainer(); + for (String name : List.of(FOLDER_A, FOLDER_B, FOLDER_EHR, FOLDER_SATELLITE)) + { + Container c = junit.getChild(name); + if (c != null) + ContainerManager.delete(c, _user); + } + } + } +} diff --git a/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java b/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java index 2599c5c76..f983d4546 100644 --- a/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java +++ b/ehr_billing/src/org/labkey/ehr_billing/EHR_BillingModule.java @@ -127,6 +127,13 @@ public Set getSchemaNames() return Collections.singleton(EHR_BillingSchema.NAME); } + @Override + @NotNull + public Set getIntegrationTests() + { + return Collections.singleton(EHR_BillingManager.TestCase.class); + } + @NotNull @Override public JSONObject getPageContextJson(ContainerUser ctx) From 34d2839ed62cbb3e858adbd4fbd4a6f5994a5345 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 22 Jun 2026 13:49:36 -0600 Subject: [PATCH 2/2] EHR security: demographics elevated-access docs + genetic-calc container authz (#1152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Rationale Two related EHR security items surfaced during a security review. (1) EHRDemographicsService serves animal records built under the elevated EHR service user (EHRService.getEHRUser), so callers receive aggregated demographic/derived fields regardless of their per-dataset / QCState row permissions — and many DemographicsProviders set _supportsQCState=false, applying no QCState filter at all. This is intended behavior (within an EHR study folder, Read access is the trust boundary for the demographics summary, which backs the shared EHR.DemographicsCache UIs), so it is now documented in code to prevent a future caller from re-exposing these records at a lower trust boundary. (2) SetGeneticCalculationTaskSettingsAction enforced AdminPermission only on the request container while resolving and acting on a caller-supplied containerPath for the server-global genetic-calculation schedule, so a user who administers the request container could point the schedule at a container they do not administer. That gap is closed and covered by an integration test. #### Related Pull Requests * None #### Changes * Document elevated-access-by-design on EHRDemographicsServiceImpl.getAnimals and EHRController.GetDemographicsAction, warning callers not to forward these records to a lower trust boundary without re-securing them against the consuming user. * Re-verify AdminPermission on the resolved target container in SetGeneticCalculationTaskSettingsAction and throw UnauthorizedException when the caller does not administer it. * Add EHRController.TestCase, an integration test verifying that a user who administers the request container but not the resolved target is rejected while a user who administers the target succeeds; registered in EHRModule.getIntegrationTests(). --- ehr/src/org/labkey/ehr/EHRController.java | 177 ++++++++++++++++++ ehr/src/org/labkey/ehr/EHRModule.java | 6 + .../EHRDemographicsServiceImpl.java | 11 +- 3 files changed, 193 insertions(+), 1 deletion(-) diff --git a/ehr/src/org/labkey/ehr/EHRController.java b/ehr/src/org/labkey/ehr/EHRController.java index 6ba08ce24..47a39095e 100644 --- a/ehr/src/org/labkey/ehr/EHRController.java +++ b/ehr/src/org/labkey/ehr/EHRController.java @@ -16,15 +16,21 @@ package org.labkey.ehr; +import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; import org.json.JSONArray; import org.json.JSONObject; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; import org.labkey.api.action.ConfirmAction; import org.labkey.api.action.MutatingApiAction; +import org.labkey.api.action.NullSafeBindException; import org.labkey.api.action.ReadOnlyApiAction; import org.labkey.api.action.SimpleErrorView; import org.labkey.api.action.SimpleViewAction; @@ -38,6 +44,9 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.DisplayColumn; import org.labkey.api.data.JsonWriter; +import org.labkey.api.data.NormalContainerType; +import org.labkey.api.data.PropertyManager; +import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; @@ -76,17 +85,27 @@ import org.labkey.api.reader.TabLoader; import org.labkey.api.resource.FileResource; import org.labkey.api.resource.Resource; +import org.labkey.api.security.MutableSecurityPolicy; import org.labkey.api.security.RequiresPermission; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityPolicyManager; +import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.settings.AppProps; import org.labkey.api.study.DatasetTable; import org.labkey.api.util.ExceptionUtil; +import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlStringBuilder; +import org.labkey.api.util.JunitUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; +import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; @@ -95,6 +114,7 @@ import org.labkey.api.view.NavTree; import org.labkey.api.view.Portal; import org.labkey.api.view.UnauthorizedException; +import org.labkey.api.view.ViewContext; import org.labkey.api.view.WebPartFactory; import org.labkey.api.view.WebPartView; import org.labkey.api.view.template.ClientDependency; @@ -589,6 +609,16 @@ public void setIds(String[] ids) } } + /** + * SECURITY — ELEVATED ACCESS BY DESIGN: + * Demographics are served from EHRDemographicsService, whose cache is intentionally built under the configured + * EHR service user (EHRService.getEHRUser(c)), NOT the requesting user. Callers therefore receive aggregated + * demographic/derived fields regardless of their per-dataset / per-QCState row permissions. This is the intended + * EHR design: Read access to an EHR study folder is the trust gate for the demographics summary, which backs the + * core data-entry and animal-history UIs via the shared EHR.DemographicsCache client. See + * {@link org.labkey.ehr.demographics.EHRDemographicsServiceImpl#getAnimals}. Do not expose the returned record map + * to a lower trust boundary without re-securing it. + */ @RequiresPermission(ReadPermission.class) public static class GetDemographicsAction extends ReadOnlyApiAction { @@ -640,6 +670,13 @@ public ApiResponse execute(ScheduleGeneticCalculationForm form, BindException er errors.reject(ERROR_MSG, "Unable to find container for path: " + form.getContainerPath()); return null; } + + // The @RequiresPermission check only covers the request container; re-verify admin on the + // resolved target so a folder admin can't redirect the (global) genetic-calc job at a + // container they don't administer. + if (!c.hasPermission(getUser(), AdminPermission.class)) + throw new UnauthorizedException("You do not have permission to configure genetic calculations for container: " + c.getPath()); + GeneticCalculationsJob.setProperties(form.isEnabled(), c, form.getHourOfDay(), form.getDayOfWeek(), form.isKinshipValidation(), form.isAllowImportDuringBusinessHours()); return new ApiSimpleResponse("success", true); @@ -2437,4 +2474,144 @@ public void setStartingId(Integer startingId) _startingId = startingId; } } + + /** + * Verifies the cross-container authorization of {@link SetGeneticCalculationTaskSettingsAction}. The + * {@code @RequiresPermission(AdminPermission.class)} annotation only guards the request container, but the action + * resolves and acts on a caller-supplied {@code containerPath}. This test confirms a user who administers the + * request container but not the resolved target container is rejected, while a user who administers the target + * succeeds. + */ + public static class TestCase extends Assert + { + private static final String REQUEST_ADMIN_EMAIL = "geneticcalc_request_admin@ehrcontroller.test"; + private static final String BOTH_ADMIN_EMAIL = "geneticcalc_both_admin@ehrcontroller.test"; + + private static Container _requestContainer; + private static Container _targetContainer; + private static User _requestAdmin; // folder admin on the request container only + private static User _bothAdmin; // folder admin on both the request and target containers + + @BeforeClass + public static void setup() throws Exception + { + cleanup(); + + User testUser = TestContext.get().getUser(); + Container junit = JunitUtil.getTestContainer(); + _requestContainer = ContainerManager.createContainer(junit, "EHRGeneticCalcTest-request-" + GUID.makeGUID(), null, null, NormalContainerType.NAME, testUser); + _targetContainer = ContainerManager.createContainer(junit, "EHRGeneticCalcTest-target-" + GUID.makeGUID(), null, null, NormalContainerType.NAME, testUser); + + _requestAdmin = SecurityManager.addUser(new ValidEmail(REQUEST_ADMIN_EMAIL), null).getUser(); + _bothAdmin = SecurityManager.addUser(new ValidEmail(BOTH_ADMIN_EMAIL), null).getUser(); + + MutableSecurityPolicy requestPolicy = new MutableSecurityPolicy(_requestContainer, _requestContainer.getPolicy()); + requestPolicy.addRoleAssignment(_requestAdmin, FolderAdminRole.class); + requestPolicy.addRoleAssignment(_bothAdmin, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(requestPolicy, testUser); + + MutableSecurityPolicy targetPolicy = new MutableSecurityPolicy(_targetContainer, _targetContainer.getPolicy()); + targetPolicy.addRoleAssignment(_bothAdmin, FolderAdminRole.class); + SecurityPolicyManager.savePolicyForTests(targetPolicy, testUser); + } + + @Test + public void testCannotConfigureForeignContainer() + { + // _requestAdmin administers the request container but NOT the target container, so pointing the + // (global) genetic-calc schedule at the target container must be rejected. + SetGeneticCalculationTaskSettingsAction action = new SetGeneticCalculationTaskSettingsAction(); + action.setViewContext(makeContext(_requestAdmin, _requestContainer)); + + ScheduleGeneticCalculationForm form = new ScheduleGeneticCalculationForm(); + form.setEnabled(false); + form.setContainerPath(_targetContainer.getPath()); + form.setHourOfDay(2); + + try + { + action.execute(form, new NullSafeBindException(form, "form")); + fail("Expected UnauthorizedException when targeting a container the user does not administer"); + } + catch (UnauthorizedException expected) + { + // expected + } + } + + @Test + public void testCanConfigureAdministeredContainer() + { + // _bothAdmin administers the target container, so configuring the schedule for it must succeed. Preserve + // and restore the server-global settings this action mutates so the test leaves no side effects. + Map original = new HashMap<>(PropertyManager.getProperties(GeneticCalculationsJob.GENETICCALCULATIONS_PROPERTY_DOMAIN)); + try + { + SetGeneticCalculationTaskSettingsAction action = new SetGeneticCalculationTaskSettingsAction(); + action.setViewContext(makeContext(_bothAdmin, _requestContainer)); + + ScheduleGeneticCalculationForm form = new ScheduleGeneticCalculationForm(); + form.setEnabled(false); // keep disabled so no Quartz job is actually scheduled + form.setContainerPath(_targetContainer.getPath()); + form.setHourOfDay(2); + + ApiResponse response = action.execute(form, new NullSafeBindException(form, "form")); + assertNotNull("Expected a response when configuring an administered container", response); + assertNotNull("Expected the target container to be persisted", GeneticCalculationsJob.getContainer()); + assertEquals("Schedule should target the requested container", _targetContainer.getId(), GeneticCalculationsJob.getContainer().getId()); + } + finally + { + WritablePropertyMap pm = PropertyManager.getWritableProperties(GeneticCalculationsJob.GENETICCALCULATIONS_PROPERTY_DOMAIN, true); + pm.clear(); + pm.putAll(original); + pm.save(); + // Resync the live scheduler to the restored settings. + GeneticCalculationsJob.unschedule(); + GeneticCalculationsJob.schedule(); + } + } + + private ViewContext makeContext(User u, Container c) + { + HttpServletRequest request = TestContext.get().getRequest(); + ViewContext context = new ViewContext(); + context.setContainer(c); + context.setUser(u); + context.setRequest(request); + return context; + } + + @AfterClass + public static void cleanup() + { + User testUser = TestContext.get().getUser(); + + if (_targetContainer != null) + { + ContainerManager.delete(_targetContainer, testUser); + _targetContainer = null; + } + if (_requestContainer != null) + { + ContainerManager.delete(_requestContainer, testUser); + _requestContainer = null; + } + + for (String email : new String[]{REQUEST_ADMIN_EMAIL, BOTH_ADMIN_EMAIL}) + { + try + { + User u = UserManager.getUser(new ValidEmail(email)); + if (u != null) + UserManager.deleteUser(u.getUserId()); + } + catch (ValidEmail.InvalidEmailException | SecurityManager.UserManagementException ignored) + { + } + } + _requestAdmin = null; + _bothAdmin = null; + } + } } diff --git a/ehr/src/org/labkey/ehr/EHRModule.java b/ehr/src/org/labkey/ehr/EHRModule.java index e37f59948..7046552a1 100644 --- a/ehr/src/org/labkey/ehr/EHRModule.java +++ b/ehr/src/org/labkey/ehr/EHRModule.java @@ -136,6 +136,12 @@ public String getName() return 26.001; } + @Override + public @NotNull Set getIntegrationTests() + { + return Set.of(EHRController.TestCase.class); + } + @Override protected void init() { diff --git a/ehr/src/org/labkey/ehr/demographics/EHRDemographicsServiceImpl.java b/ehr/src/org/labkey/ehr/demographics/EHRDemographicsServiceImpl.java index ba696f427..2de678e0b 100644 --- a/ehr/src/org/labkey/ehr/demographics/EHRDemographicsServiceImpl.java +++ b/ehr/src/org/labkey/ehr/demographics/EHRDemographicsServiceImpl.java @@ -118,7 +118,16 @@ public AnimalRecord getAnimal(Container c, String id) /** * Queries the cache for the animal record, creating if not found. - * Always returns a copy of the original + * Always returns a copy of the original. + * + *

SECURITY — ELEVATED ACCESS BY DESIGN: records are built and cached under the configured EHR service + * user ({@link EHRService#getEHRUser(Container)}), NOT the calling user, and are shared across all users in the + * container. The aggregated demographic/derived fields therefore reflect the EHR service user's access, bypassing + * the caller's per-dataset / per-QCState row permissions (note that many {@code DemographicsProvider}s set + * {@code _supportsQCState = false}, so no QCState filter is applied at all). This is intentional: within an EHR + * study folder, Read access is the trust gate for the demographics summary. Callers exposing these records to an + * end user must treat folder Read as the boundary and must NOT forward them to a lower trust boundary without + * re-securing the data against the consuming user. */ @Override public List getAnimals(Container c, Collection ids)