From ca791519aa8e0d9a2197b1499784a146b5ec5435 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Wed, 17 Jun 2026 09:51:09 -0600 Subject: [PATCH 1/2] Document EHR demographics service elevated-access-by-design EHRDemographicsService builds and caches AnimalRecords under the configured EHR service user (EHRService.getEHRUser), shared across all users in a container, so the aggregated demographic/derived fields reflect the service user's access and bypass the caller's per-dataset and per-QCState row permissions; several DemographicsProviders set _supportsQCState=false so no QCState filter is applied at all. Add Javadoc on EHRDemographicsServiceImpl.getAnimals and on the EHRController.GetDemographicsAction endpoint marking this elevated access as by design, with folder Read as the intended trust boundary for the demographics summary, and warning callers not to forward these records to a lower trust boundary without re-securing. Documentation only; no functional change. --- ehr/src/org/labkey/ehr/EHRController.java | 10 ++++++++++ .../ehr/demographics/EHRDemographicsServiceImpl.java | 11 ++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/ehr/src/org/labkey/ehr/EHRController.java b/ehr/src/org/labkey/ehr/EHRController.java index 6ba08ce24..4714d82c4 100644 --- a/ehr/src/org/labkey/ehr/EHRController.java +++ b/ehr/src/org/labkey/ehr/EHRController.java @@ -589,6 +589,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 { 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) From c0d1d71ddb95d49662f845adb0e9b07353486647 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Wed, 17 Jun 2026 10:04:17 -0600 Subject: [PATCH 2/2] Re-check admin on resolved container for genetic-calc scheduling SetGeneticCalculationTaskSettingsAction is annotated @RequiresPermission(AdminPermission.class), but that check only covers the request container while the action resolves and acts on a caller-supplied containerPath to configure the server-global genetic calculation schedule. A user who administers the request container could therefore point the schedule at a container they do not administer. Re-verify AdminPermission on the resolved target container and throw UnauthorizedException when the caller lacks it. Add EHRController.TestCase, an integration test confirming that a user who administers the request container but not the resolved target is rejected while a user who administers the target succeeds, and register it in EHRModule.getIntegrationTests(). --- ehr/src/org/labkey/ehr/EHRController.java | 167 ++++++++++++++++++++++ ehr/src/org/labkey/ehr/EHRModule.java | 6 + 2 files changed, 173 insertions(+) diff --git a/ehr/src/org/labkey/ehr/EHRController.java b/ehr/src/org/labkey/ehr/EHRController.java index 4714d82c4..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; @@ -650,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); @@ -2447,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() {